Skip to content

Commit e635e6d

Browse files
smolasmola
committed
Fix peer IP tags in Jetty/Dropwizard and Play 2.3/2.4/2.5
Jetty's ForwardedRequestCustomizer (enabled by default in Dropwizard) and Play's RequestHeader#remoteAddress() both return the IP resolved from x-forwarded-for / similar proxy headers. That value is correct for http.client_ip but must not be reported as the actual socket peer (peer.ip, peer.port, network.client.ip). Jetty / Dropwizard (APPSEC-62560): - Read the unwrapped socket address directly from the Jetty channel / endpoint in every JettyDecorator (jetty 7.0, 7.6, 9, 10, 11, 12). - Add jetty-server-9.0 as a testImplementation of dropwizard-0.8 so the test classpath matches production (Jetty owns the server span; Servlet3 skips when it sees the existing context). Update DropwizardTest's expectedIntegrationName to jetty-server. Play 2.3 / 2.4 / 2.5 (APPSEC-62562): - In PlayHttpServerDecorator, wrap the Request handed to super.onRequest with a play.api.mvc.WrappedRequest whose remoteAddress() returns the peer IP already captured on the span by the upstream akka / netty instrumentation. This prevents super.onRequest -> peerHostIP from overwriting the real socket peer with the forwarded value, and keeps IG callbacks consistent. Mirrors the Play 2.6+ behavior already provided by RemoteConnectionWithRawAddress. HttpServerTest: - Tighten PEER_HOST_IPV4 / PEER_HOST_IPV6 assertions on the FORWARDED endpoint to strictly expect the loopback address.
1 parent 840f009 commit e635e6d

13 files changed

Lines changed: 246 additions & 6 deletions

File tree

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2573,14 +2573,14 @@ abstract class HttpServerTest<SERVER> extends WithHttpServer<SERVER> {
25732573
}
25742574
if (span.getTag(Tags.PEER_HOST_IPV6) != null) {
25752575
"$Tags.PEER_HOST_IPV6" {
2576-
it == "0:0:0:0:0:0:0:1" || (endpoint == FORWARDED && it == endpoint.body)
2576+
it == "0:0:0:0:0:0:0:1"
25772577
}
25782578
"$Tags.HTTP_CLIENT_IP" {
25792579
it == "0:0:0:0:0:0:0:1" || (endpoint == FORWARDED && it == endpoint.body)
25802580
}
25812581
} else {
25822582
"$Tags.PEER_HOST_IPV4" {
2583-
it == "127.0.0.1" || (endpoint == FORWARDED && it == endpoint.body)
2583+
it == "127.0.0.1"
25842584
}
25852585
"$Tags.HTTP_CLIENT_IP" {
25862586
it == "127.0.0.1" || (endpoint == FORWARDED && it == endpoint.body)

‎dd-java-agent/instrumentation/jetty/jetty-server/jetty-server-10.0/src/main/java11/datadog/trace/instrumentation/jetty10/JettyDecorator.java‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
import datadog.trace.bootstrap.instrumentation.decorator.HttpServerDecorator;
1111
import datadog.trace.instrumentation.jetty.JettyBlockResponseFunction;
1212
import datadog.trace.instrumentation.jetty9.RequestURIDataAdapter;
13+
import java.net.InetSocketAddress;
1314
import javax.servlet.ServletException;
1415
import org.eclipse.jetty.server.HttpChannel;
1516
import org.eclipse.jetty.server.Request;
@@ -61,11 +62,28 @@ protected URIDataAdapter url(final Request request) {
6162

6263
@Override
6364
protected String peerHostIP(final Request request) {
65+
// Avoid Request.getRemoteAddr() since ForwardedRequestCustomizer overrides it with
66+
// the value resolved from x-forwarded-for and similar proxy headers. Peer information
67+
// must be the actual socket peer.
68+
HttpChannel channel = request.getHttpChannel();
69+
if (channel != null) {
70+
InetSocketAddress remote = channel.getRemoteAddress();
71+
if (remote != null && remote.getAddress() != null) {
72+
return remote.getAddress().getHostAddress();
73+
}
74+
}
6475
return request.getRemoteAddr();
6576
}
6677

6778
@Override
6879
protected int peerPort(final Request request) {
80+
HttpChannel channel = request.getHttpChannel();
81+
if (channel != null) {
82+
InetSocketAddress remote = channel.getRemoteAddress();
83+
if (remote != null) {
84+
return remote.getPort();
85+
}
86+
}
6987
return request.getRemotePort();
7088
}
7189

‎dd-java-agent/instrumentation/jetty/jetty-server/jetty-server-11.0/src/main/java11/datadog/trace/instrumentation/jetty11/JettyDecorator.java‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
import datadog.trace.bootstrap.instrumentation.decorator.HttpServerDecorator;
1111
import datadog.trace.instrumentation.jetty.JettyBlockResponseFunction;
1212
import jakarta.servlet.ServletException;
13+
import java.net.InetSocketAddress;
1314
import org.eclipse.jetty.server.HttpChannel;
1415
import org.eclipse.jetty.server.Request;
1516
import org.eclipse.jetty.server.Response;
@@ -61,11 +62,28 @@ protected URIDataAdapter url(final Request request) {
6162

6263
@Override
6364
protected String peerHostIP(final Request request) {
65+
// Avoid Request.getRemoteAddr() since ForwardedRequestCustomizer overrides it with
66+
// the value resolved from x-forwarded-for and similar proxy headers. Peer information
67+
// must be the actual socket peer.
68+
HttpChannel channel = request.getHttpChannel();
69+
if (channel != null) {
70+
InetSocketAddress remote = channel.getRemoteAddress();
71+
if (remote != null && remote.getAddress() != null) {
72+
return remote.getAddress().getHostAddress();
73+
}
74+
}
6475
return request.getRemoteAddr();
6576
}
6677

6778
@Override
6879
protected int peerPort(final Request request) {
80+
HttpChannel channel = request.getHttpChannel();
81+
if (channel != null) {
82+
InetSocketAddress remote = channel.getRemoteAddress();
83+
if (remote != null) {
84+
return remote.getPort();
85+
}
86+
}
6987
return request.getRemotePort();
7088
}
7189

‎dd-java-agent/instrumentation/jetty/jetty-server/jetty-server-12.0/src/main/java17/datadog/trace/instrumentation/jetty12/JettyDecorator.java‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,12 @@
77
import datadog.trace.bootstrap.instrumentation.api.URIDataAdapter;
88
import datadog.trace.bootstrap.instrumentation.api.UTF8BytesString;
99
import datadog.trace.bootstrap.instrumentation.decorator.HttpServerDecorator;
10+
import java.net.InetAddress;
11+
import java.net.InetSocketAddress;
12+
import java.net.SocketAddress;
13+
import org.eclipse.jetty.io.Connection;
14+
import org.eclipse.jetty.io.EndPoint;
15+
import org.eclipse.jetty.server.ConnectionMetaData;
1016
import org.eclipse.jetty.server.Request;
1117
import org.eclipse.jetty.server.Response;
1218
import org.eclipse.jetty.server.internal.HttpChannelState;
@@ -71,14 +77,50 @@ protected URIDataAdapter url(final Request request) {
7177

7278
@Override
7379
protected String peerHostIP(final Request request) {
80+
// Avoid Request.getRemoteAddr() since ForwardedRequestCustomizer wraps
81+
// ConnectionMetaData.getRemoteSocketAddress() with the value resolved from
82+
// x-forwarded-for and similar proxy headers. Peer information must be the actual
83+
// socket peer.
84+
final InetSocketAddress remote = remoteSocketAddress(request);
85+
if (remote != null) {
86+
final InetAddress address = remote.getAddress();
87+
if (address != null) {
88+
return address.getHostAddress();
89+
}
90+
return Request.getRemoteAddr(request);
91+
}
7492
return Request.getRemoteAddr(request);
7593
}
7694

7795
@Override
7896
protected int peerPort(final Request request) {
97+
final InetSocketAddress remote = remoteSocketAddress(request);
98+
if (remote != null) {
99+
return remote.getPort();
100+
}
79101
return Request.getRemotePort(request);
80102
}
81103

104+
private static InetSocketAddress remoteSocketAddress(final Request request) {
105+
final ConnectionMetaData metaData = request.getConnectionMetaData();
106+
if (metaData == null) {
107+
return null;
108+
}
109+
final Connection connection = metaData.getConnection();
110+
if (connection == null) {
111+
return null;
112+
}
113+
final EndPoint endPoint = connection.getEndPoint();
114+
if (endPoint == null) {
115+
return null;
116+
}
117+
final SocketAddress remote = endPoint.getRemoteSocketAddress();
118+
if (remote instanceof InetSocketAddress) {
119+
return (InetSocketAddress) remote;
120+
}
121+
return null;
122+
}
123+
82124
@Override
83125
protected int status(final Response response) {
84126
return response.getStatus();

‎dd-java-agent/instrumentation/jetty/jetty-server/jetty-server-7.0/src/main/java/datadog/trace/instrumentation/jetty70/JettyDecorator.java‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
import datadog.trace.bootstrap.instrumentation.decorator.HttpServerDecorator;
1111
import datadog.trace.instrumentation.jetty.JettyBlockResponseFunction;
1212
import javax.servlet.ServletException;
13+
import org.eclipse.jetty.io.EndPoint;
1314
import org.eclipse.jetty.server.HttpConnection;
1415
import org.eclipse.jetty.server.Request;
1516
import org.eclipse.jetty.server.Response;
@@ -60,11 +61,31 @@ protected URIDataAdapter url(final Request request) {
6061

6162
@Override
6263
protected String peerHostIP(final Request request) {
64+
// Avoid Request.getRemoteAddr() since ForwardedRequestCustomizer overrides it with
65+
// the value resolved from x-forwarded-for and similar proxy headers. Peer information
66+
// must be the actual socket peer.
67+
final HttpConnection connection = request.getConnection();
68+
if (connection != null) {
69+
final EndPoint endPoint = connection.getEndPoint();
70+
if (endPoint != null) {
71+
final String remoteAddr = endPoint.getRemoteAddr();
72+
if (remoteAddr != null) {
73+
return remoteAddr;
74+
}
75+
}
76+
}
6377
return request.getRemoteAddr();
6478
}
6579

6680
@Override
6781
protected int peerPort(final Request request) {
82+
final HttpConnection connection = request.getConnection();
83+
if (connection != null) {
84+
final EndPoint endPoint = connection.getEndPoint();
85+
if (endPoint != null) {
86+
return endPoint.getRemotePort();
87+
}
88+
}
6889
return request.getRemotePort();
6990
}
7091

‎dd-java-agent/instrumentation/jetty/jetty-server/jetty-server-7.6/src/main/java/datadog/trace/instrumentation/jetty76/JettyDecorator.java‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
import datadog.trace.bootstrap.instrumentation.decorator.HttpServerDecorator;
1111
import datadog.trace.instrumentation.jetty.JettyBlockResponseFunction;
1212
import javax.servlet.ServletException;
13+
import org.eclipse.jetty.io.EndPoint;
1314
import org.eclipse.jetty.server.AbstractHttpConnection;
1415
import org.eclipse.jetty.server.Request;
1516
import org.eclipse.jetty.server.Response;
@@ -60,11 +61,31 @@ protected URIDataAdapter url(final Request request) {
6061

6162
@Override
6263
protected String peerHostIP(final Request request) {
64+
// Avoid Request.getRemoteAddr() since ForwardedRequestCustomizer overrides it with
65+
// the value resolved from x-forwarded-for and similar proxy headers. Peer information
66+
// must be the actual socket peer.
67+
final AbstractHttpConnection connection = request.getConnection();
68+
if (connection != null) {
69+
final EndPoint endPoint = connection.getEndPoint();
70+
if (endPoint != null) {
71+
final String remoteAddr = endPoint.getRemoteAddr();
72+
if (remoteAddr != null) {
73+
return remoteAddr;
74+
}
75+
}
76+
}
6377
return request.getRemoteAddr();
6478
}
6579

6680
@Override
6781
protected int peerPort(final Request request) {
82+
final AbstractHttpConnection connection = request.getConnection();
83+
if (connection != null) {
84+
final EndPoint endPoint = connection.getEndPoint();
85+
if (endPoint != null) {
86+
return endPoint.getRemotePort();
87+
}
88+
}
6889
return request.getRemotePort();
6990
}
7091

‎dd-java-agent/instrumentation/jetty/jetty-server/jetty-server-9.0/src/main/java/datadog/trace/instrumentation/jetty9/JettyDecorator.java‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
import datadog.trace.bootstrap.instrumentation.api.UTF8BytesString;
1010
import datadog.trace.bootstrap.instrumentation.decorator.HttpServerDecorator;
1111
import datadog.trace.instrumentation.jetty.JettyBlockResponseFunction;
12+
import java.net.InetSocketAddress;
1213
import javax.servlet.ServletException;
1314
import org.eclipse.jetty.server.HttpChannel;
1415
import org.eclipse.jetty.server.Request;
@@ -60,11 +61,28 @@ protected URIDataAdapter url(final Request request) {
6061

6162
@Override
6263
protected String peerHostIP(final Request request) {
64+
// Avoid Request.getRemoteAddr() since ForwardedRequestCustomizer (used by default in
65+
// Dropwizard) overrides it with the value resolved from x-forwarded-for and similar
66+
// proxy headers. Peer information must be the actual socket peer.
67+
HttpChannel<?> channel = request.getHttpChannel();
68+
if (channel != null) {
69+
InetSocketAddress remote = channel.getRemoteAddress();
70+
if (remote != null && remote.getAddress() != null) {
71+
return remote.getAddress().getHostAddress();
72+
}
73+
}
6374
return request.getRemoteAddr();
6475
}
6576

6677
@Override
6778
protected int peerPort(final Request request) {
79+
HttpChannel<?> channel = request.getHttpChannel();
80+
if (channel != null) {
81+
InetSocketAddress remote = channel.getRemoteAddress();
82+
if (remote != null) {
83+
return remote.getPort();
84+
}
85+
}
6886
return request.getRemotePort();
6987
}
7088

‎dd-java-agent/instrumentation/play/play-2.3/src/main/java/datadog/trace/instrumentation/play23/PlayHttpServerDecorator.java‎

Lines changed: 26 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
import datadog.trace.api.gateway.RequestContextSlot;
1111
import datadog.trace.bootstrap.instrumentation.api.AgentPropagation;
1212
import datadog.trace.bootstrap.instrumentation.api.AgentSpan;
13+
import datadog.trace.bootstrap.instrumentation.api.Tags;
1314
import datadog.trace.bootstrap.instrumentation.api.URIDataAdapter;
1415
import datadog.trace.bootstrap.instrumentation.api.UTF8BytesString;
1516
import datadog.trace.bootstrap.instrumentation.decorator.HttpServerDecorator;
@@ -88,7 +89,14 @@ public AgentSpan onRequest(
8889
final Request<?> connection,
8990
final Request<?> request,
9091
final Context parentContext) {
91-
super.onRequest(span, connection, request, parentContext);
92+
// Play's Request#remoteAddress() returns the address resolved from X-Forwarded-For (and
93+
// similar) proxy headers, not the actual TCP socket peer. If the upstream framework
94+
// (akka/netty) already populated peer information on the span from the real socket,
95+
// wrap the connection so super.onRequest sees the captured value instead of Play's
96+
// forwarded one. This way the IG callbacks and tags receive the correct peer.
97+
// (APPSEC-62562)
98+
final Request<?> connectionForSuper = withCapturedPeer(span, connection);
99+
super.onRequest(span, connectionForSuper, request, parentContext);
92100
if (request != null) {
93101
// more about routes here:
94102
// https://github.com/playframework/playframework/blob/master/documentation/manual/releases/release26/migration26/Migration26.md#router-tags-are-now-attributes
@@ -133,6 +141,23 @@ private void dispatchRoute(final AgentSpan span, final String route) {
133141
}
134142
}
135143

144+
/**
145+
* If the span already carries a peer IP (set by the upstream akka/netty instrumentation from the
146+
* real TCP socket peer), wrap the request so {@code remoteAddress()} returns that value instead
147+
* of Play's X-Forwarded-For-resolved one.
148+
*/
149+
private static Request<?> withCapturedPeer(final AgentSpan span, final Request<?> connection) {
150+
if (connection == null) {
151+
return null;
152+
}
153+
final Object capturedIp = span.getTag(Tags.PEER_HOST_IPV4);
154+
final Object peerIp = capturedIp != null ? capturedIp : span.getTag(Tags.PEER_HOST_IPV6);
155+
if (peerIp != null) {
156+
return new RequestWithCapturedPeer(connection, peerIp.toString());
157+
}
158+
return connection;
159+
}
160+
136161
@Override
137162
public AgentSpan onError(final AgentSpan span, Throwable throwable) {
138163
if (REPORT_HTTP_STATUS) {

‎dd-java-agent/instrumentation/play/play-2.3/src/main/java/datadog/trace/instrumentation/play23/PlayInstrumentation.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,8 @@ public String[] helperClassNames() {
3737
packageName + ".PlayHeaders$Result",
3838
packageName + ".PlayHttpServerDecorator",
3939
packageName + ".RequestCompleteCallback",
40-
packageName + ".RequestURIDataAdapter"
40+
packageName + ".RequestURIDataAdapter",
41+
packageName + ".RequestWithCapturedPeer"
4142
};
4243
}
4344

‎dd-java-agent/instrumentation/play/play-2.3/src/main/java/datadog/trace/instrumentation/play23/RequestWithCapturedPeer.java‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
package datadog.trace.instrumentation.play23;
2+
3+
import play.api.mvc.Request;
4+
import play.api.mvc.WrappedRequest;
5+
6+
/**
7+
* Wraps a {@link Request} so that {@link #remoteAddress()} returns a previously captured peer IP
8+
* (typically read from the upstream akka/netty span, which obtained it from the actual TCP socket
9+
* peer). This bypasses Play's X-Forwarded-For-resolved {@code remoteAddress} when tagging peer
10+
* information. See APPSEC-62562.
11+
*/
12+
public class RequestWithCapturedPeer extends WrappedRequest<Object> {
13+
private final String capturedPeerAddress;
14+
15+
@SuppressWarnings({"unchecked", "rawtypes"})
16+
public RequestWithCapturedPeer(final Request<?> request, final String capturedPeerAddress) {
17+
super((Request) request);
18+
this.capturedPeerAddress = capturedPeerAddress;
19+
}
20+
21+
@Override
22+
public String remoteAddress() {
23+
return capturedPeerAddress;
24+
}
25+
}

0 commit comments

Comments
 (0)