Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add support for Forwarded / X-Forwarded-* headers #249

Closed
wants to merge 1 commit into from
Closed

Add support for Forwarded / X-Forwarded-* headers #249

wants to merge 1 commit into from

Conversation

bclozel
Copy link
Member

@bclozel bclozel commented Dec 21, 2017

This is a proposal for gh-220.

This implementation supports both the standard header "Forward", but also non-standard but quite common ones, such as "X-Forwarded-For", "X-Forwarded-Host", "X-Forwarded-Port" and "X-Forwarded-Proto".

I've made available that information directly in the HttpServerRequest. Of course this feature is opt-in and disabled by default.

Possible improvements:

  • support trusted proxies; the current implementation considers only the first information provided in the list of values. We could configure trusted proxies, walk back the listed values and stop at the first unknown proxy
  • refine the configuration; it's currently an on/off thing, but we could only enable support for the rfc or the non-standard X-Forwarded-*

simonbasle
simonbasle reviewed Dec 21, 2017
View reviewed changes
src/main/java/reactor/ipc/netty/http/server/ConnectionInfo.java
InetSocketAddress remoteAddress = channel.remoteAddress();
String scheme = channel.pipeline().get(SslHandler.class) != null ? "https" : "http";

String forwarded = request.requestHeaders().get(FORWARDED_HEADER).split(",")[0];
Copy link
Member

@simonbasle simonbasle Dec 21, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

is splitting on the comma , character ok? quoted strings in header values are allowed to contain commas BUT in this particular case:

  • proto= definition is same as URI scheme (no commas allowed 👍 )
  • for= definition is that of a Node identifier (no commas allowed 👍 )
  • host= definition is that of the Host header field / HTTP URI. This one is a bit more complex, but I don't think unencoded commas can appear (🤔)

So this should be ok, but might want to dig into the host one.

Copy link
Member Author

@bclozel bclozel Dec 21, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

See rfc7239 section 5.3 and rfc7230 section 3.2.6.

, is part of the US-ASCII delimiters, so not allowed there AFAIU.

Copy link
Member

@simonbasle simonbasle Dec 21, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

that's for the token part (here host), but the value can be a quoted string, which may contain characters in the %x23-5B range (including the comma %x2C), in addition to %x21 (!) but not %x22 (double quotes)

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't get it - isn't that encoded still at that point?
I just tried sending a Forward: for="exa%x2Cmple.com" header and serverRequest.remoteAddress().getHostString() does return "exa%x2Cmple.com". Do you have a test case in mind that reproduces the issue?

Copy link
Member

@simonbasle simonbasle Dec 21, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

nope, just had a gut feeling splitting on , might be "too simple". I have thus briefly looked at the RFC, which I have a limited understanding of (since I don't deal with as often as you guys ;) )

from what I understand of the ABNF notation used in the RFC, %x2C means "a literal comma", not "a comma encoded as %x2C" [1] (so that might be were I'm interpreting things wrong).

rfc7230 section 3.2.6 defines the quoted string qdtext with such a range: %x23-5B (which I assume would mean "the literal characters between # an [, including A-Z and the comma.

if that's working well enough for SPR, then that's probably good 👍 just challenging the assumptions a bit.

[1] see ABNF rfc section 3.4

Copy link
Member Author

Choose a reason for hiding this comment

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

You may be right, actually.
At the same time it seems commas aren't allowed in hostnames (in other RFCs) and it looks like Tomcat is doing the same: https://github.com/apache/tomcat/blob/trunk/java/org/apache/catalina/valves/RemoteIpValve.java#L354

@violetagg violetagg added this to the 0.8.0.RELEASE milestone Dec 21, 2017
@bclozel bclozel mentioned this pull request Jan 4, 2018
Closed
violetagg
violetagg reviewed Jan 9, 2018
View reviewed changes
src/main/java/reactor/ipc/netty/http/server/HttpServerOperations.java Outdated
@@ -107,9 +112,13 @@ static HttpServerOperations bindHttp(Channel channel,
this.nettyResponse = new DefaultHttpResponse(HttpVersion.HTTP_1_1, HttpResponseStatus.OK);
this.responseHeaders = nettyResponse.headers();
this.cookieHolder = Cookies.newServerRequestHolder(requestHeaders());
if (ch.attr(USE_FORWARDED).get()) {
Copy link
Member

Choose a reason for hiding this comment

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

Check for null
Something like

		Attribute<Boolean> useForwarded = ch.attr(USE_FORWARDED);
		if (useForwarded != null && useForwarded.get() != null && useForwarded.get()) {

violetagg
violetagg reviewed Jan 9, 2018
View reviewed changes
src/test/java/reactor/ipc/netty/http/server/ConnectionInfoTests.java

@Test
public void forwardedForIpV6() {
testClientRequest(
Copy link
Member

Choose a reason for hiding this comment

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

this is failing for me

    org.junit.ComparisonFailure: expected:<"[OK]"> but was:<"[expected:<"[[2001:db8:cafe::17]]"> but was:<"[2001:db8:cafe:0:0:0:0:17]">]">
        at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
        at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
        at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
        at reactor.ipc.netty.http.server.ConnectionInfoTests.testClientRequest(ConnectionInfoTests.java:206)
        at reactor.ipc.netty.http.server.ConnectionInfoTests.forwardedForIpV6(ConnectionInfoTests.java:176)

violetagg
violetagg reviewed Jan 9, 2018
View reviewed changes
src/test/java/reactor/ipc/netty/http/server/ConnectionInfoTests.java

@Test
public void forwardedHostIpV6() {
testClientRequest(
Copy link
Member

Choose a reason for hiding this comment

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

this is failing for me

    org.junit.ComparisonFailure: expected:<"[OK]"> but was:<"[expected:<"[[1abc:2abc:3abc::5ABC:6abc]]"> but was:<"[1abc:2abc:3abc:0:0:0:5abc:6abc]">]">
        at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
        at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
        at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
        at reactor.ipc.netty.http.server.ConnectionInfoTests.testClientRequest(ConnectionInfoTests.java:206)
        at reactor.ipc.netty.http.server.ConnectionInfoTests.forwardedHostIpV6(ConnectionInfoTests.java:50)

violetagg
violetagg reviewed Jan 9, 2018
View reviewed changes
src/test/java/reactor/ipc/netty/http/server/ConnectionInfoTests.java

@Test
public void xForwardedFor() {
testClientRequest(
Copy link
Member

Choose a reason for hiding this comment

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

this is failing for me

    org.junit.ComparisonFailure: expected:<"[OK]"> but was:<"[expected:<"[[1abc:2abc:3abc::5ABC:6abc]]"> but was:<"[1abc:2abc:3abc:0:0:0:5abc:6abc]">]">
        at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
        at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
        at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
        at reactor.ipc.netty.http.server.ConnectionInfoTests.testClientRequest(ConnectionInfoTests.java:206)
        at reactor.ipc.netty.http.server.ConnectionInfoTests.xForwardedFor(ConnectionInfoTests.java:62)

violetagg
violetagg reviewed Jan 9, 2018
View reviewed changes
src/test/java/reactor/ipc/netty/http/server/ConnectionInfoTests.java

@Test
public void xForwardedHost() {
testClientRequest(
Copy link
Member

Choose a reason for hiding this comment

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

this is failing for me

    org.junit.ComparisonFailure: expected:<"[OK]"> but was:<"[expected:<"[[1abc:2abc:3abc::5ABC:6abc]]"> but was:<"[1abc:2abc:3abc:0:0:0:5abc:6abc]">]">
        at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
        at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
        at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
        at reactor.ipc.netty.http.server.ConnectionInfoTests.testClientRequest(ConnectionInfoTests.java:206)
        at reactor.ipc.netty.http.server.ConnectionInfoTests.xForwardedHost(ConnectionInfoTests.java:74)

violetagg
violetagg reviewed Jan 9, 2018
View reviewed changes
src/test/java/reactor/ipc/netty/http/server/ConnectionInfoTests.java
@@ -0,0 +1,214 @@
package reactor.ipc.netty.http.server;
Copy link
Member

Choose a reason for hiding this comment

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

add copyright header

violetagg
violetagg reviewed Jan 9, 2018
View reviewed changes
src/main/java/reactor/ipc/netty/http/server/ConnectionInfo.java
@@ -0,0 +1,146 @@
package reactor.ipc.netty.http.server;
Copy link
Member

Choose a reason for hiding this comment

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

add copyright header

@bclozel
Copy link
Member Author

bclozel commented Jan 9, 2018

Thanks for the review @violetagg !
I've updated my PR. The failing tests were about the change with InetSocketAddressUtil - this utility class is turning the IPv6 addresses into their byte representation.
I've also used the Channel.hasAttr(AttributeKey) method to test for the forwarded configuration attribute.

@bclozel
Copy link
Member Author

bclozel commented Jan 9, 2018

@violetagg Note that the CI build is still failing because I think that the license automatic check is checking for Copyright (c) 2011-2017 Pivotal Software Inc, All Rights Reserved. and I'm using 2018 in the files I've modified.

Happy new year 🎉, I guess!

@violetagg
Copy link
Member

violetagg commented Jan 9, 2018 •
edited
Loading

@violetagg Note that the CI build is still failing because I think that the license automatic check is checking for Copyright (c) 2011-2017 Pivotal Software Inc, All Rights Reserved. and I'm using 2018 in the files I've modified.

yes we need to rebase from master and then it will be ok

@smaldini
Copy link
Contributor

smaldini commented Jan 14, 2018 •
edited by simonbasle
Loading

Mightt tbe good on 0.7 branch as well to discuss this week. edit: as of 0.8.0.M1 this hasn't been backported in 0.7.

@bclozel
Add support for Forwarded / X-Forwarded-* headers
e5a729c 
This commit adds new information to the `HttpServerRequest`:
* the host (server) address
* the remote (client) address
* the scheme used by the current request

This information can be either derived from the current channel, or
extracted from the incoming HTTP request headers using "Forwarded"
or "X-Forwarded-*".

This feature is opt-in, and must be configured during server setup:

  HttpServer.create().forwarded().port(8080);

Closes reactorgh-220
@violetagg
Copy link
Member

Closed by a8c9b24

@violetagg violetagg closed this Jan 15, 2018
@violetagg violetagg mentioned this pull request Jan 15, 2018
Closed
@kakawait kakawait mentioned this pull request May 9, 2018
Closed
@violetagg violetagg mentioned this pull request May 14, 2018
Closed
24 tasks
@violetagg violetagg modified the milestones: 0.8.0.RELEASE, 0.8.0.M1 Jun 8, 2018
simonbasle pushed a commit that referenced this pull request Jun 13, 2018
@bclozel @simonbasle
see #249 Fix X-Forwarded-For to use remote host information
a9fba51 
The "X-Forwarded-For" request header is about the remote host
information, not the server host information.

Follow-up to #249, reviewed in #373
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simonbasle simonbasle simonbasle left review comments

@smaldini smaldini smaldini approved these changes

@violetagg violetagg violetagg approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.8.0.M1
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@bclozel @violetagg @smaldini @simonbasle