Skip to content

Commit c716c82

Browse files
smolasmola
committed
Move network.client.ip out of AppSec into HttpServerDecorator
network.client.ip now shares the same activation logic as http.client_ip (DD_APPSEC_ENABLED or DD_TRACE_CLIENT_IP_ENABLED) and is no longer exclusive to AppSec events. Move it from GatewayBridge (where it was set only when security events fired) into HttpServerDecorator alongside http.client_ip, using the raw peer/socket IP. actor.ip remains in AppSec as a deprecated backward-compatibility tag.
1 parent 1b63a9d commit c716c82

30 files changed

Lines changed: 131 additions & 27 deletions

File tree

‎dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/instrumentation/decorator/HttpServerDecorator.java‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -370,6 +370,9 @@ public AgentSpan onRequest(
370370
} else {
371371
span.setTag(Tags.PEER_HOST_IPV4, peerIp);
372372
}
373+
if (clientIpResolverEnabled) {
374+
span.setTag(Tags.NETWORK_CLIENT_IP, peerIp);
375+
}
373376
}
374377
setPeerPort(span, peerPort);
375378
Flow<Void> flow = callIGCallbackAddressAndPort(span, peerIp, peerPort, inferredAddressStr);

‎dd-java-agent/agent-bootstrap/src/test/groovy/datadog/trace/bootstrap/instrumentation/decorator/HttpServerDecoratorTest.groovy‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -218,6 +218,7 @@ class HttpServerDecoratorTest extends ServerDecoratorTest {
218218
1 * this.span.setTag(Tags.HTTP_FORWARDED_HOST, "somehost")
219219
if (conn?.peerIp) {
220220
1 * this.span.setTag(ipv4 ? Tags.PEER_HOST_IPV4 : Tags.PEER_HOST_IPV6, conn.peerIp)
221+
1 * this.span.setTag(Tags.NETWORK_CLIENT_IP, conn.peerIp)
221222
}
222223
if (conn?.ip) {
223224
1 * this.span.setTag(Tags.HTTP_CLIENT_IP, conn.ip)
@@ -253,6 +254,7 @@ class HttpServerDecoratorTest extends ServerDecoratorTest {
253254

254255
then:
255256
1 * this.span.setTag(Tags.HTTP_CLIENT_IP, result)
257+
1 * this.span.setTag(Tags.NETWORK_CLIENT_IP, peerIpAddr)
256258

257259
where:
258260
headerIpAddr | peerIpAddr | result
@@ -277,6 +279,7 @@ class HttpServerDecoratorTest extends ServerDecoratorTest {
277279
0 * extracted.getForwarded()
278280
0 * span.setTag(Tags.HTTP_FORWARDED, _)
279281
0 * this.span.setTag(Tags.HTTP_CLIENT_IP, _)
282+
0 * this.span.setTag(Tags.NETWORK_CLIENT_IP, _)
280283
}
281284

282285
void 'disabling appsec disables header collection and ip address resolution'() {
@@ -294,6 +297,7 @@ class HttpServerDecoratorTest extends ServerDecoratorTest {
294297
0 * extracted.getForwarded()
295298
0 * span.setTag(Tags.HTTP_FORWARDED, _)
296299
0 * this.span.setTag(Tags.HTTP_CLIENT_IP, _)
300+
0 * this.span.setTag(Tags.NETWORK_CLIENT_IP, _)
297301
}
298302

299303
void 'disabling appsec but enabling client_ip_without_appsec enables header collection and ip address resolution'() {
@@ -313,6 +317,7 @@ class HttpServerDecoratorTest extends ServerDecoratorTest {
313317
2 * extracted.getXForwardedFor() >> '2.3.4.5'
314318
1 * this.span.setTag(Tags.HTTP_CLIENT_IP, '2.3.4.5')
315319
1 * this.span.setTag(Tags.HTTP_FORWARDED_IP, '2.3.4.5')
320+
1 * this.span.setTag(Tags.NETWORK_CLIENT_IP, '4.4.4.4')
316321

317322
// Forwarded doesn't participate in client ip resolution anymore
318323
1 * extracted.getForwarded() >> 'for=9.9.9.9'
@@ -333,6 +338,7 @@ class HttpServerDecoratorTest extends ServerDecoratorTest {
333338
then:
334339
1 * extracted.getCustomIpHeader() >> '5.5.5.5'
335340
1 * this.span.setTag(Tags.HTTP_CLIENT_IP, '5.5.5.5')
341+
1 * this.span.setTag(Tags.NETWORK_CLIENT_IP, '4.4.4.4')
336342
}
337343

338344
def "test onResponse"() {

‎dd-java-agent/appsec/src/main/java/com/datadog/appsec/gateway/GatewayBridge.java‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -986,9 +986,6 @@ private NoopFlow onRequestEnded(RequestContext ctx_, IGSpanInfo spanInfo) {
986986

987987
span.setTag("appsec.event", true);
988988

989-
String peerAddress = ctx.getPeerAddress();
990-
span.setTag("network.client.ip", peerAddress);
991-
992989
// Reflect client_ip as actor.ip for backward compatibility
993990
Object clientIp = tags.get(Tags.HTTP_CLIENT_IP);
994991
if (clientIp != null) {

‎dd-java-agent/appsec/src/test/groovy/com/datadog/appsec/gateway/GatewayBridgeSpecification.groovy‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -189,12 +189,10 @@ class GatewayBridgeSpecification extends DDSpecification {
189189
then:
190190
1 * spanInfo.getTags() >> TagMap.fromMap(['http.client_ip': '1.1.1.1'])
191191
1 * mockAppSecCtx.transferCollectedEvents() >> [event]
192-
1 * mockAppSecCtx.peerAddress >> '2001::1'
193192
1 * mockAppSecCtx.close()
194193
1 * spanInfo.setMetric("_dd.appsec.enabled", 1)
195194
1 * spanInfo.setTag("_dd.runtime_family", "jvm")
196195
1 * spanInfo.setTag('appsec.event', true)
197-
1 * spanInfo.setTag('network.client.ip', '2001::1')
198196
1 * spanInfo.setTag('actor.ip', '1.1.1.1')
199197
1 * traceSegment.setDataTop('appsec', new AppSecEventWrapper([event]))
200198
1 * mockAppSecCtx.isWafBlocked()

‎dd-java-agent/instrumentation-testing/src/main/groovy/datadog/trace/agent/test/base/HttpServerTest.groovy‎

Lines changed: 33 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -296,6 +296,11 @@ abstract class HttpServerTest<SERVER> extends WithHttpServer<SERVER> {
296296
true
297297
}
298298

299+
/** Some frameworks (incorrectly) expose the forwarded header as the request peer address. */
300+
boolean forwardedIpAsPeerInformation() {
301+
false
302+
}
303+
299304
boolean hasExtraErrorInformation() {
300305
false
301306
}
@@ -2466,6 +2471,7 @@ abstract class HttpServerTest<SERVER> extends WithHttpServer<SERVER> {
24662471
boolean hasPeerInformation = hasPeerInformation()
24672472
boolean hasPeerPort = hasPeerPort()
24682473
boolean hasForwardedIP = hasForwardedIP()
2474+
boolean useForwardedIpAsPeerInformation = forwardedIpAsPeerInformation()
24692475
def expectedExtraServerTags = expectedExtraServerTags(endpoint)
24702476
def expectedStatus = expectedStatus(endpoint)
24712477
def expectedQueryTag = expectedQueryTag(endpoint)
@@ -2490,29 +2496,43 @@ abstract class HttpServerTest<SERVER> extends WithHttpServer<SERVER> {
24902496
if (hasPeerPort) {
24912497
"$Tags.PEER_PORT" Integer
24922498
}
2499+
def peerHostTag = Tags.PEER_HOST_IPV4
2500+
def expectedPeerIp = "127.0.0.1"
24932501
if (span.getTag(Tags.PEER_HOST_IPV6) != null) {
2494-
"$Tags.PEER_HOST_IPV6" {
2495-
it == "0:0:0:0:0:0:0:1" || (endpoint == FORWARDED && it == endpoint.body)
2496-
}
2497-
"$Tags.HTTP_CLIENT_IP" {
2498-
it == "0:0:0:0:0:0:0:1" || (endpoint == FORWARDED && it == endpoint.body)
2499-
}
2500-
} else {
2501-
"$Tags.PEER_HOST_IPV4" {
2502-
it == "127.0.0.1" || (endpoint == FORWARDED && it == endpoint.body)
2503-
}
2504-
"$Tags.HTTP_CLIENT_IP" {
2505-
it == "127.0.0.1" || (endpoint == FORWARDED && it == endpoint.body)
2506-
}
2502+
peerHostTag = Tags.PEER_HOST_IPV6
2503+
expectedPeerIp = "0:0:0:0:0:0:0:1"
2504+
}
2505+
// Special-case for the FORWARDED endpoint, which is exercised by the
2506+
// "test forwarded request" feature in this class. That test sends a
2507+
// GET /forwarded request with an `X-Forwarded-For: 1.2.3.4` header.
2508+
// Some frameworks (e.g. Dropwizard, Play 2.3/2.5 via
2509+
// `request.remoteAddress()`) substitute the forwarded IP into their
2510+
// remote-address API, so the peer/network IP tags become 1.2.3.4
2511+
// instead of the actual TCP peer (loopback). Frameworks with that
2512+
// behaviour opt in via `forwardedIpAsPeerInformation()`.
2513+
// FIXME(santiago.mola): This is actually a bug, and ideally instrumentation
2514+
// for Dropwizard or Play should be fixed to retrieve the correct peer IP
2515+
// for network.client.ip tags.
2516+
if (endpoint == FORWARDED && useForwardedIpAsPeerInformation) {
2517+
expectedPeerIp = endpoint.body
25072518
}
2519+
tag(peerHostTag, expectedPeerIp)
2520+
"$Tags.HTTP_CLIENT_IP" clientIp ?: expectedPeerIp
2521+
"$Tags.NETWORK_CLIENT_IP" expectedPeerIp
25082522
} else {
2523+
// http.client_ip is inferred from forwarded headers; network.client.ip requires peerIp.
2524+
"$Tags.NETWORK_CLIENT_IP" null
25092525
"$Tags.HTTP_CLIENT_IP" clientIp
25102526
}
25112527
"$Tags.HTTP_HOSTNAME" address.host
25122528
"$Tags.HTTP_URL" "$expectedUrl"
25132529
"$Tags.HTTP_METHOD" method
25142530
"$Tags.HTTP_STATUS" expectedStatus
25152531
"$Tags.HTTP_USER_AGENT" String
2532+
// The FORWARDED endpoint is hit by the "test forwarded request" feature
2533+
// with an `X-Forwarded-For: 1.2.3.4` header; FORWARDED.body is that
2534+
// same "1.2.3.4" string, which is what we expect to surface as the
2535+
// forwarded-IP tag when the framework extracts it.
25162536
if (endpoint == FORWARDED && hasForwardedIP) {
25172537
"$Tags.HTTP_FORWARDED_IP" endpoint.body
25182538
}

‎dd-java-agent/instrumentation/akka/akka-http/akka-http-10.0/src/lagomTest/groovy/LagomTest.groovy‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,7 @@ class LagomTest extends InstrumentationSpecification {
7474
"$Tags.PEER_HOST_IPV4" '127.0.0.1'
7575
"$Tags.PEER_PORT" Integer
7676
"$Tags.HTTP_CLIENT_IP" '127.0.0.1'
77+
"$Tags.NETWORK_CLIENT_IP" '127.0.0.1'
7778
defaultTags()
7879
}
7980
}
@@ -121,6 +122,7 @@ class LagomTest extends InstrumentationSpecification {
121122
"$Tags.PEER_HOST_IPV4" '127.0.0.1'
122123
"$Tags.PEER_PORT" Integer
123124
"$Tags.HTTP_CLIENT_IP" '127.0.0.1'
125+
"$Tags.NETWORK_CLIENT_IP" '127.0.0.1'
124126
defaultTags()
125127
}
126128
}

‎dd-java-agent/instrumentation/cxf-2.1/src/latestDepTest/groovy/CxfContextPropagationTest.groovy‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,7 @@ class CxfContextPropagationTest extends InstrumentationSpecification {
7575
"$InstrumentationTags.SERVLET_PATH" "/test"
7676
"$Tags.HTTP_USER_AGENT" String
7777
"$Tags.HTTP_CLIENT_IP" "127.0.0.1"
78+
"$Tags.NETWORK_CLIENT_IP" "127.0.0.1"
7879
withCustomIntegrationName("jetty-server")
7980
defaultTags()
8081
}

‎dd-java-agent/instrumentation/cxf-2.1/src/test/groovy/CxfContextPropagationTest.groovy‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,7 @@ class CxfContextPropagationTest extends InstrumentationSpecification {
7373
"servlet.path" { it == null || it == "/test" }
7474
"$Tags.HTTP_USER_AGENT" String
7575
"$Tags.HTTP_CLIENT_IP" "127.0.0.1"
76+
"$Tags.NETWORK_CLIENT_IP" "127.0.0.1"
7677
withCustomIntegrationName("jetty-server")
7778
defaultTags()
7879
}

‎dd-java-agent/instrumentation/dropwizard/dropwizard-0.8/src/test/groovy/DropwizardTest.groovy‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -122,6 +122,11 @@ class DropwizardTest extends HttpServerTest<DropwizardTestSupport> {
122122
return true
123123
}
124124

125+
@Override
126+
boolean forwardedIpAsPeerInformation() {
127+
true
128+
}
129+
125130
@Override
126131
boolean changesAll404s() {
127132
true

‎dd-java-agent/instrumentation/jsp-2.3/src/test/groovy/JSPInstrumentationBasicTests.groovy‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@ class JSPInstrumentationBasicTests extends JSPTestBase {
3737
"$Tags.HTTP_STATUS" 200
3838
"$Tags.HTTP_USER_AGENT" String
3939
"$Tags.HTTP_CLIENT_IP" "127.0.0.1"
40+
"$Tags.NETWORK_CLIENT_IP" "127.0.0.1"
4041
"servlet.context" "/$jspWebappContext"
4142
"servlet.path" "/$jspFileName"
4243
defaultTags()
@@ -114,6 +115,7 @@ class JSPInstrumentationBasicTests extends JSPTestBase {
114115
"$Tags.HTTP_STATUS" 200
115116
"$Tags.HTTP_USER_AGENT" String
116117
"$Tags.HTTP_CLIENT_IP" "127.0.0.1"
118+
"$Tags.NETWORK_CLIENT_IP" "127.0.0.1"
117119
"servlet.context" "/$jspWebappContext"
118120
"servlet.path" "/getQuery.jsp"
119121
defaultTags()
@@ -187,6 +189,7 @@ class JSPInstrumentationBasicTests extends JSPTestBase {
187189
"$Tags.HTTP_STATUS" 200
188190
"$Tags.HTTP_USER_AGENT" String
189191
"$Tags.HTTP_CLIENT_IP" "127.0.0.1"
192+
"$Tags.NETWORK_CLIENT_IP" "127.0.0.1"
190193
"servlet.context" "/$jspWebappContext"
191194
"servlet.path" "/post.jsp"
192195
defaultTags()
@@ -256,6 +259,7 @@ class JSPInstrumentationBasicTests extends JSPTestBase {
256259
"$Tags.HTTP_STATUS" 500
257260
"$Tags.HTTP_USER_AGENT" String
258261
"$Tags.HTTP_CLIENT_IP" "127.0.0.1"
262+
"$Tags.NETWORK_CLIENT_IP" "127.0.0.1"
259263
"servlet.context" "/$jspWebappContext"
260264
"servlet.path" "/$jspFileName"
261265
"error.type" { String tagExceptionType ->
@@ -345,6 +349,7 @@ class JSPInstrumentationBasicTests extends JSPTestBase {
345349
"$Tags.HTTP_STATUS" 200
346350
"$Tags.HTTP_USER_AGENT" String
347351
"$Tags.HTTP_CLIENT_IP" "127.0.0.1"
352+
"$Tags.NETWORK_CLIENT_IP" "127.0.0.1"
348353
"servlet.context" "/$jspWebappContext"
349354
"servlet.path" "/includes/includeHtml.jsp"
350355
defaultTags()
@@ -414,6 +419,7 @@ class JSPInstrumentationBasicTests extends JSPTestBase {
414419
"$Tags.HTTP_STATUS" 200
415420
"$Tags.HTTP_USER_AGENT" String
416421
"$Tags.HTTP_CLIENT_IP" "127.0.0.1"
422+
"$Tags.NETWORK_CLIENT_IP" "127.0.0.1"
417423
"servlet.context" "/$jspWebappContext"
418424
"servlet.path" "/includes/includeMulti.jsp"
419425
defaultTags()
@@ -537,6 +543,7 @@ class JSPInstrumentationBasicTests extends JSPTestBase {
537543
"$Tags.HTTP_STATUS" 500
538544
"$Tags.HTTP_USER_AGENT" String
539545
"$Tags.HTTP_CLIENT_IP" "127.0.0.1"
546+
"$Tags.NETWORK_CLIENT_IP" "127.0.0.1"
540547
"servlet.context" "/$jspWebappContext"
541548
"servlet.path" "/$jspFileName"
542549
errorTags(JasperException, String)
@@ -602,6 +609,7 @@ class JSPInstrumentationBasicTests extends JSPTestBase {
602609
"$Tags.HTTP_STATUS" 200
603610
"$Tags.HTTP_USER_AGENT" String
604611
"$Tags.HTTP_CLIENT_IP" "127.0.0.1"
612+
"$Tags.NETWORK_CLIENT_IP" "127.0.0.1"
605613
"servlet.context" "/$jspWebappContext"
606614
"servlet.path" "/$staticFile"
607615
defaultTags()

0 commit comments

Comments
 (0)