Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,12 @@
import datadog.trace.api.aiguard.AIGuard.ToolCall.Function;
import datadog.trace.api.aiguard.Evaluator;
import datadog.trace.api.aiguard.noop.NoOpEvaluator;
import datadog.trace.api.gateway.RequestContext;
import datadog.trace.api.telemetry.WafMetricCollector;
import datadog.trace.bootstrap.instrumentation.api.AgentScope;
import datadog.trace.bootstrap.instrumentation.api.AgentSpan;
import datadog.trace.bootstrap.instrumentation.api.AgentTracer;
import datadog.trace.bootstrap.instrumentation.api.ClientIpAddressData;
import datadog.trace.bootstrap.instrumentation.api.Tags;
import java.io.IOException;
import java.lang.annotation.Annotation;
Expand Down Expand Up @@ -213,6 +215,32 @@ private boolean isBlockingEnabled(final Options options, final Object isBlocking
return options.block() && "true".equalsIgnoreCase(isBlockingEnabled.toString());
}

/**
* Applies the {@link ClientIpAddressData} captured during HTTP server request decoration to the
* local root span. This is the lazy half of AI Guard client IP collection: {@code
* HttpServerDecorator} resolves the IP eagerly and stashes it on the {@link RequestContext}; we
* consume it here once an {@code ai_guard} span is created, so IP tags are not added to spans of
* non-AI requests in services that have AI Guard enabled.
*/
private static void applyClientIpTags(final AgentSpan localRootSpan) {
final RequestContext requestContext = localRootSpan.getRequestContext();
if (requestContext == null) {
return;
}
final ClientIpAddressData clientIpAddressData = requestContext.getAndResetClientIpAddressData();
if (clientIpAddressData == null) {
return;
}
final String peerIp = clientIpAddressData.getPeerIp();
if (peerIp != null && localRootSpan.getTag(Tags.NETWORK_CLIENT_IP) == null) {
localRootSpan.setTag(Tags.NETWORK_CLIENT_IP, peerIp);
}
final String inferredClientIp = clientIpAddressData.getInferredClientIp();
if (inferredClientIp != null && localRootSpan.getTag(Tags.HTTP_CLIENT_IP) == null) {
localRootSpan.setTag(Tags.HTTP_CLIENT_IP, inferredClientIp);
}
Comment on lines +235 to +241
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if it's frequent enough that both tags are already set on the root span, it could make sense to check that before unfolding the request context ? maybe ?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the tags would generally not be present in the root span. In the normal path, if the http decorator is tagging these for all requests, then the IPs would not be stashed for later, and this function would see requestContext.getAndResetClientIpAddressData() == null).

}

@Override
public Evaluation evaluate(final List<Message> messages, final Options options) {
if (messages == null || messages.isEmpty()) {
Expand All @@ -229,6 +257,7 @@ public Evaluation evaluate(final List<Message> messages, final Options options)
if (localRootSpan != null) {
localRootSpan.setTag(Tags.AI_GUARD_KEEP, true);
localRootSpan.setTag(EVENT_TAG, true);
applyClientIpTags(localRootSpan);
}
try (final AgentScope scope = tracer.activateSpan(span)) {
final Message last = messages.get(messages.size() - 1);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,11 @@ import com.squareup.moshi.Moshi
import datadog.common.version.VersionInfo
import datadog.trace.api.Config
import datadog.trace.api.aiguard.AIGuard
import datadog.trace.api.gateway.RequestContext
import datadog.trace.api.telemetry.WafMetricCollector
import datadog.trace.bootstrap.instrumentation.api.AgentSpan
import datadog.trace.bootstrap.instrumentation.api.AgentTracer
import datadog.trace.bootstrap.instrumentation.api.ClientIpAddressData
import datadog.trace.bootstrap.instrumentation.api.Tags
import datadog.trace.test.util.DDSpecification
import okhttp3.Call
Expand Down Expand Up @@ -272,6 +274,68 @@ class AIGuardInternalTests extends DDSpecification {
new AIGuard.Options().block(false) | true | false
}

void 'test evaluate applies captured client ip tags to local root span'() {
given:
final requestContext = Mock(RequestContext)
localRootSpan.getRequestContext() >> requestContext
requestContext.getAndResetClientIpAddressData() >> new ClientIpAddressData('4.4.4.4', '2.3.4.5')
final aiguard = mockClient(200, [data: [attributes: [action: 'ALLOW', reason: 'It is fine']]])

when:
aiguard.evaluate(TOOL_CALL, AIGuard.Options.DEFAULT)

then:
1 * localRootSpan.getTag(Tags.NETWORK_CLIENT_IP) >> null
1 * localRootSpan.setTag(Tags.NETWORK_CLIENT_IP, '4.4.4.4')
1 * localRootSpan.getTag(Tags.HTTP_CLIENT_IP) >> null
1 * localRootSpan.setTag(Tags.HTTP_CLIENT_IP, '2.3.4.5')
}

void 'test evaluate does not overwrite existing client ip tags'() {
given:
final requestContext = Mock(RequestContext)
localRootSpan.getRequestContext() >> requestContext
requestContext.getAndResetClientIpAddressData() >> new ClientIpAddressData('4.4.4.4', '2.3.4.5')
final aiguard = mockClient(200, [data: [attributes: [action: 'ALLOW', reason: 'It is fine']]])

when:
aiguard.evaluate(TOOL_CALL, AIGuard.Options.DEFAULT)

then:
1 * localRootSpan.getTag(Tags.NETWORK_CLIENT_IP) >> '9.9.9.9'
0 * localRootSpan.setTag(Tags.NETWORK_CLIENT_IP, _)
1 * localRootSpan.getTag(Tags.HTTP_CLIENT_IP) >> '8.8.8.8'
0 * localRootSpan.setTag(Tags.HTTP_CLIENT_IP, _)
}

void 'test evaluate is a noop for client ip tags when no data captured'() {
given:
final requestContext = Mock(RequestContext)
localRootSpan.getRequestContext() >> requestContext
requestContext.getAndResetClientIpAddressData() >> null
final aiguard = mockClient(200, [data: [attributes: [action: 'ALLOW', reason: 'It is fine']]])

when:
aiguard.evaluate(TOOL_CALL, AIGuard.Options.DEFAULT)

then:
0 * localRootSpan.setTag(Tags.NETWORK_CLIENT_IP, _)
0 * localRootSpan.setTag(Tags.HTTP_CLIENT_IP, _)
}

void 'test evaluate is a noop for client ip tags when no request context'() {
given:
localRootSpan.getRequestContext() >> null
final aiguard = mockClient(200, [data: [attributes: [action: 'ALLOW', reason: 'It is fine']]])

when:
aiguard.evaluate(TOOL_CALL, AIGuard.Options.DEFAULT)

then:
0 * localRootSpan.setTag(Tags.NETWORK_CLIENT_IP, _)
0 * localRootSpan.setTag(Tags.HTTP_CLIENT_IP, _)
}

void 'test evaluate with API errors'() {
given:
final errors = [[status: 400, title: 'Bad request']]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import datadog.trace.bootstrap.instrumentation.api.AgentSpan;
import datadog.trace.bootstrap.instrumentation.api.AgentSpanContext;
import datadog.trace.bootstrap.instrumentation.api.AgentTracer;
import datadog.trace.bootstrap.instrumentation.api.ClientIpAddressData;
import datadog.trace.bootstrap.instrumentation.api.ErrorPriorities;
import datadog.trace.bootstrap.instrumentation.api.InternalSpanTypes;
import datadog.trace.bootstrap.instrumentation.api.ResourceNamePriorities;
Expand Down Expand Up @@ -248,10 +249,31 @@ public AgentSpan onRequest(
}

AgentSpanContext.Extracted extracted = getExtractedSpanContext(parentContext);
boolean clientIpResolverEnabled =
// Whether to attach IP tags to all requests or not.
// This should be enabled if:
// - DD_TRACE_CLIENT_IP_ENABLED=true, or
// - DD_APPSEC_ENABLED=true (or AppSec enabled at runtime)
Comment on lines +253 to +255
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is kinda repeating the logic that's written below, not sure it's very useful + it risks being outdated if the condition changes

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic below is scattered across ~3 separate code blocks intermingled with other stuff. At least for me it was quite hard to follow every subtle detail about what gets collected when, and why. The whole logic for IP tagging should probably be splitted to a separate function so that it's grouped and fits once screen (material for another PR I guess). But until then, I would rather keep some consolidated clarification about what's going on.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, makes sense, I saw it as mostly rewording the bool computation below, but it's indeed carrying more information.

// This applies to tags:
// - http.client_ip (IP resolved from proxy tags)
// - network.client.ip (peer IP)
// - tags with proxy header values
// For backwards compatibility, it does not apply to:
// - peer.ipv4
// - peer.ipv6
final boolean shouldTagIps =
config.isClientIpEnabled() || traceClientIpResolverEnabled && APPSEC_ACTIVE;
// Whether to stash IP data for later tagging or not.
// AI Guard requires client IP tags on the local root span when an ai_guard span is created.
// Resolve the IPs eagerly but do not tag the span yet; stash them on the request context so
// AIGuardInternal can apply them lazily, only on requests that actually create an ai_guard
// span.
final boolean shouldStashIps =
!shouldTagIps && traceClientIpResolverEnabled && config.isAiGuardEnabled();
Comment on lines +270 to +271
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Enable IP header extraction for AI Guard-only mode

In the AI Guard-only configuration (DD_AI_GUARD_ENABLED=true with AppSec and dd.trace.client-ip.enabled off), this branch asks the resolver to stash IPs, but the extracted context never contains the forwarding headers: ContextInterpreter.reset still sets collectIpHeaders only for clientIpWithoutAppSec || clientIpResolutionEnabled && APPSEC_ACTIVE. As a result ClientIpAddressResolver.resolve(extracted, span) sees null X-Forwarded-For/Forwarded headers and falls back to the socket peer, so http.client_ip on the ai_guard trace is wrong in the mode this change adds.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed this is a bug. I'll work on a fix.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed this was a bug, subtly hidden in the smoke test. Fixed the smoke test, and fixed this in ContextInterpreter.

// Whether to resolve client IP based on proxy headers or no.
final boolean shouldResolveIp = shouldTagIps || shouldStashIps;

if (extracted != null) {
if (clientIpResolverEnabled) {
if (shouldTagIps) {
String forwarded = extracted.getForwarded();
if (forwarded != null) {
span.setTag(Tags.HTTP_FORWARDED, forwarded);
Expand Down Expand Up @@ -332,7 +354,7 @@ public AgentSpan onRequest(
}

String inferredAddressStr = null;
if (clientIpResolverEnabled && extracted != null) {
if (shouldResolveIp && extracted != null) {
InetAddress inferredAddress = ClientIpAddressResolver.resolve(extracted, span);
// the peer address should be used if:
// 1. the headers yield nothing, regardless of whether it is public or not
Expand All @@ -349,9 +371,11 @@ public AgentSpan onRequest(
}
if (inferredAddress != null) {
inferredAddressStr = inferredAddress.getHostAddress();
span.setTag(Tags.HTTP_CLIENT_IP, inferredAddressStr);
if (shouldTagIps) {
span.setTag(Tags.HTTP_CLIENT_IP, inferredAddressStr);
}
}
} else if (clientIpResolverEnabled && span.getLocalRootSpan() != span) {
} else if (shouldTagIps && span.getLocalRootSpan() != span) {
// in this case extracted == null
// If there is no extracted we can't do anything but use the peer addr.
// Additionally, extracted == null arises on subspans for which the resolution
Expand All @@ -370,10 +394,16 @@ public AgentSpan onRequest(
} else {
span.setTag(Tags.PEER_HOST_IPV4, peerIp);
}
if (clientIpResolverEnabled) {
if (shouldTagIps) {
span.setTag(Tags.NETWORK_CLIENT_IP, peerIp);
}
}
if (shouldStashIps && (peerIp != null || inferredAddressStr != null)) {
RequestContext requestContext = span.getRequestContext();
if (requestContext != null) {
requestContext.setClientIpAddressData(new ClientIpAddressData(peerIp, inferredAddressStr));
}
}
setPeerPort(span, peerPort);
Flow<Void> flow = callIGCallbackAddressAndPort(span, peerIp, peerPort, inferredAddressStr);
if (flow.getAction() instanceof RequestBlockingAction) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import datadog.trace.bootstrap.instrumentation.api.AgentSpan
import datadog.trace.bootstrap.instrumentation.api.AgentSpanContext
import datadog.trace.bootstrap.instrumentation.api.AgentTracer
import datadog.trace.bootstrap.instrumentation.api.AgentTracer.TracerAPI
import datadog.trace.bootstrap.instrumentation.api.ClientIpAddressData
import datadog.trace.bootstrap.instrumentation.api.ContextVisitors
import datadog.trace.bootstrap.instrumentation.api.ErrorPriorities
import datadog.trace.bootstrap.instrumentation.api.ResourceNamePriorities
Expand Down Expand Up @@ -300,6 +301,61 @@ class HttpServerDecoratorTest extends ServerDecoratorTest {
0 * this.span.setTag(Tags.NETWORK_CLIENT_IP, _)
}

void 'enabling ai guard captures client ip data without tagging the span during request decoration'() {
setup:
injectSysConfig('dd.ai_guard.enabled', 'true')
ActiveSubsystems.APPSEC_ACTIVE = false

def extracted = Mock(AgentSpanContext.Extracted)
def context = AgentSpan.fromSpanContext(extracted)
def requestContext = Mock(RequestContext)
def decorator = newDecorator()

when:
decorator.onRequest(this.span, [peerIp: '4.4.4.4'], null, context)

then:
_ * extracted.getXForwardedFor() >> '2.3.4.5'
_ * extracted.getXClusterClientIp() >> null
_ * extracted.getXRealIp() >> null
_ * extracted.getXClientIp() >> null
_ * extracted.getCustomIpHeader() >> null
_ * extracted.getTrueClientIp() >> null
_ * extracted.getFastlyClientIp() >> null
_ * extracted.getCfConnectingIp() >> null
_ * extracted.getCfConnectingIpv6() >> null
_ * this.span.getRequestContext() >> requestContext
1 * requestContext.setClientIpAddressData({ ClientIpAddressData data ->
data.peerIp == '4.4.4.4' && data.inferredClientIp == '2.3.4.5'
})
0 * this.span.setTag(Tags.HTTP_CLIENT_IP, _)
0 * this.span.setTag(Tags.NETWORK_CLIENT_IP, _)
0 * this.span.setTag(Tags.HTTP_FORWARDED_IP, _)
}

void 'enabling ai guard does not override client_ip_without_appsec tagging behavior'() {
setup:
injectSysConfig('dd.ai_guard.enabled', 'true')
injectSysConfig('dd.trace.client-ip.enabled', 'true')
ActiveSubsystems.APPSEC_ACTIVE = false

def extracted = Mock(AgentSpanContext.Extracted)
def context = AgentSpan.fromSpanContext(extracted)
def requestContext = Mock(RequestContext)
def decorator = newDecorator()

when:
decorator.onRequest(this.span, [peerIp: '4.4.4.4'], null, context)

then:
_ * this.span.getRequestContext() >> requestContext
2 * extracted.getXForwardedFor() >> '2.3.4.5'
1 * this.span.setTag(Tags.HTTP_CLIENT_IP, '2.3.4.5')
1 * this.span.setTag(Tags.NETWORK_CLIENT_IP, '4.4.4.4')
// ai guard capture is skipped because tags were already set
0 * requestContext.setClientIpAddressData(_)
}

void 'disabling appsec but enabling client_ip_without_appsec enables header collection and ip address resolution'() {
setup:
injectSysConfig('dd.trace.client-ip.enabled', 'true')
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import datadog.trace.api.gateway.RequestContextSlot;
import datadog.trace.api.gateway.SubscriptionService;
import datadog.trace.api.internal.TraceSegment;
import datadog.trace.bootstrap.instrumentation.api.ClientIpAddressData;
import datadog.trace.bootstrap.instrumentation.api.URIDataAdapter;
import datadog.trace.bootstrap.instrumentation.api.URIDefaultDataAdapter;
import java.io.IOException;
Expand Down Expand Up @@ -254,6 +255,14 @@ public <T> T getOrCreateMetaStructTop(String key, Function<String, T> defaultVal
return null;
}

@Override
public void setClientIpAddressData(ClientIpAddressData clientIpAddressData) {}

@Override
public ClientIpAddressData getAndResetClientIpAddressData() {
return null;
}

@Override
public void close() throws IOException {}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import datadog.trace.api.gateway.RequestContextSlot
import datadog.trace.api.gateway.SubscriptionService
import datadog.trace.api.http.StoredBodySupplier
import datadog.trace.api.internal.TraceSegment
import datadog.trace.bootstrap.instrumentation.api.ClientIpAddressData
import datadog.trace.api.telemetry.LoginEvent
import datadog.trace.api.telemetry.RuleType
import datadog.trace.api.telemetry.WafMetricCollector
Expand Down Expand Up @@ -79,6 +80,14 @@ class GatewayBridgeSpecification extends DDSpecification {
return null
}

@Override
void setClientIpAddressData(ClientIpAddressData clientIpAddressData) {}

@Override
ClientIpAddressData getAndResetClientIpAddressData() {
return null
}

@Override
void close() throws IOException {}
}
Expand Down Expand Up @@ -1253,6 +1262,14 @@ class GatewayBridgeSpecification extends DDSpecification {
return null
}

@Override
void setClientIpAddressData(ClientIpAddressData clientIpAddressData) {}

@Override
ClientIpAddressData getAndResetClientIpAddressData() {
return null
}

@Override
void close() throws IOException {}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import datadog.trace.api.gateway.BlockResponseFunction
import datadog.trace.api.gateway.RequestContext
import datadog.trace.api.gateway.RequestContextSlot
import datadog.trace.api.internal.TraceSegment
import datadog.trace.bootstrap.instrumentation.api.ClientIpAddressData

import java.util.function.Function

Expand Down Expand Up @@ -56,6 +57,16 @@ class ValidatingRequestContextDecorator implements RequestContext {
return delegate.getOrCreateMetaStructTop(key, defaultValue)
}

@Override
void setClientIpAddressData(ClientIpAddressData clientIpAddressData) {
delegate.setClientIpAddressData(clientIpAddressData)
}

@Override
ClientIpAddressData getAndResetClientIpAddressData() {
return delegate.getAndResetClientIpAddressData()
}

@Override
void close() throws IOException {
delegate.close()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,14 @@
public class SpringbootApplication {

public static void main(final String[] args) {
try {
activateAppSec();
} catch (Exception e) {
System.out.println("Could not activate appSec: " + e.getMessage());
if (!Boolean.getBoolean("smoketest.skipAppSecActivation")) {
try {
activateAppSec();
} catch (Exception e) {
System.out.println("Could not activate appSec: " + e.getMessage());
}
} else {
System.out.println("AppSec activation skipped");
}

SpringApplication.run(SpringbootApplication.class, args);
Expand Down
Loading