IPv6 address needs normalization (without brackets) in ForwardedRequestCustomizer

[gueuselambix]

According to https://tools.ietf.org/html/rfc7239#section-6.1 the IPv6 address should be enclosed by square brackets.

The ForwardedRequestCustomizer now extracts the header without normalization and sets in on the request (request.setRemoteAddr(InetSocketAddress)), see [1]
Code that depends on the remote address will fail because it assumes the content represents a valid IP address.

Is it possible to normalize the value to a valid IPv4 or IPv6 address (if the header contains the format "IPv6address" or "IPv4address" as specified in https://tools.ietf.org/html/rfc3986#appendix-A).

[1] https://github.com/eclipse/jetty.project/blob/5fefd5d8bd68bb87a2bad16138dfc0ee121eddcf/jetty-server/src/main/java/org/eclipse/jetty/server/ForwardedRequestCustomizer.java#L388

[joakime]

What's a good example of this non-normalized header causing problems in ForwardRequestCustomizer?

[gueuselambix]

First example:
If you host Jetty directly on an IPv6 address and you use an IPv6 client, the request log says:
0:0:0:0:0:0:0:1 - - [25/Apr/2017:08:27:20 +0000] "GET / HTTP/1.1" 200 2802

If you however enable the http-forwarded module and send a request using curl like:
curl -H 'Forwarded: for="[2001:db8:cafe::17]", for=192.0.2.43' -g "http://[::1]:8080/"

[2001:db8:cafe::17] - - [25/Apr/2017:14:16:50 +0000] "GET / HTTP/1.1" 200 2802

So the behaviour is different. With IPv4 it's the same.

Second example:
Our application uses the IPRangePredicate [1] from OpenSAML.
They pull the IP address from the request and check via com.google.common.net.InetAddresses to see if its a valid address, which it isn't because of the brackets.

final String address = httpRequest != null ? httpRequest.getRemoteAddr() : null;
if (address == null || !InetAddresses.isInetAddress(address)) {
return false;
}

I was wondering where it should be normalized...
But since rfc7239 states that it MUST be enclosed by brackets in the Forwarded-header, I think it's up to the consumer (jetty in this case) of this header to normalize this into an parsable IP as if Jetty was approached directly and not via a proxy.

[1] https://git.shibboleth.net/view/?p=java-opensaml.git;a=blob;f=opensaml-profile-api/src/main/java/org/opensaml/profile/logic/IPRangePredicate.java;h=0161ce469a7d8531ca5f96c909cc14f79fde705a;hb=HEAD#l100

[gregw]

Hmmm looks like we may be returning the [ipv6] form from getServerName for a normal host header. So we need to be consistent with that... but we may be wrong... researching...

[gregw]

Ah I thought this was familiar - I have raised this issue with the servlet spec as the javadoc for getServerName is ambiguous: jakartaee/servlet#97 But it didn't get resolved in the current servlet 4.0 discussions

[gueuselambix]

I don't see where you use getServerName() for the ForwardedRequestCustomizer?
The host part is extracted using basic substring operation: https://github.com/eclipse/jetty.project/blob/5fefd5d8bd68bb87a2bad16138dfc0ee121eddcf/jetty-http/src/main/java/org/eclipse/jetty/http/HostPortHttpField.java#L52

If you want to represent a hostname, the brackets are good. If you want it to represent an IPv6 address (request.setRemoteAddr()) , the brackets should be stripped off.

[gregw]

So i have found something definitely wrong in our IPv6 handling. getRemoteAddr() must return an IP address, but we return the bracketed value. So I'm thinking we should change everywhere to strip the brackets... but that's a big change to make in dot release....

[gregw]

Ah no - discussion with @sbordet it is clear now that InetAddress accepts addresses in the [] so there is no need to strip for standard java APIs.

So I think perhaps com.google.common.net.InetAddresses needs to be updated to handle [] ?

[gregw]

Closing for now unless we get guidance from servlet spec EG

[gregw]

@gueuselambix sorry missed your comment. I mention the other APIs because we want to be consistent with them. Either we always strip [] or never strip.

InetAddress accepts the [] form as in:

com.google.common.net.InetAddresses

So in java at least the [] form is treated like an IP address.

[gueuselambix]

I believe you are missing the point I am trying to make.

If I use IPv6 directly to a Jetty server, com.google.common.net.InetAddresses.isInetAddress(request.getRemoteAddr()) evaluates to true for an IPv6 address.

If I proxy the Jetty server and configure it with the ForwardedRequestCustomizer as documented at [1]
com.google.common.net.InetAddresses.isInetAddress(request.getRemoteAddr()) evaluates to false for an IPv6 address.
The proxy server is configured to set the "Forwarded"-header, according to the RFC https://tools.ietf.org/html/rfc7239#section-5.2 . The RFC states the IPv6 address is always enclosed by brackets:

Also, note that an IPv6 address is always enclosed in square brackets.

e.g. 'Forwarded: for="[2001:db8:cafe::17]", for=192.0.2.43'

So the behaviour of Jetty server and a proxied Jetty server is different, from my point of view that is a bug in the ForwardedRequestCustomizer which sets the RemoteAddr without normalizing.

[1] http://www.eclipse.org/jetty/documentation/current/configuring-connectors.html#_proxy_load_balancer_connection_configuration

[gregw]

@gueuselambix I understand completely the point you are trying to make, and I have a lot of sympathy with it.

However, the issue remains that the specification of IPv6 handling is ambiguous in the spec and I've not yet received the clarification from the EG needed to make such a big change to the server.

More over the InetAddress class does accept the [] form as an address, so it would be handy if com.google.common.net.InetAddresses implemented the same flexibility.

However, it is a fine point you make about the differences in the Addr returned when obtained directly from a connector or from a forwarded header. This could perhaps help convince me to switch to all non [] form (I already have the patch for this implemented and stashed).

But I need to consult more widely before making such a change. For now I'll reopen and seek other input.

[joakime]

This is not unique to ForwardRequestCustomizer btw.

If you request (without ForwardRequestCustomizer) a url like https://[2001:db8:85a3:8d3:1319:8a2e:370:7348]:443/ then the getRemoteAddr() also has the brackets.

[gregw]

@sbordet what do you think of the issue of the difference between a directly obtained address (from the connection) vs a header set address?

Note also that the result of InetAddress.getByName("[::1]") is just "::1"

[gregw]

@joakime All the handling of this is via the common HostPort class, so if we change the behaviour there to strip the [] it will change for all usages, which at least makes us consistent.

Although there could be an argument to leave the [] in the Name methods but strip them from the Addr methods..... hmmm

[joakime]

@gregw The https://tools.ietf.org/html/rfc3986#section-3.2.2 has some other things to think about with HostPort (things like [v07::1], a versioned IPvFuture should trigger an unsupported error?)

[joakime]

Alternative approach: if you are using Jetty exclusively, you can skip the entire InetSocketAddress -> String -> String -> String conversion you are doing in the hopes of not triggering a DNS lookup in the JVM (which has likely already occurred if needed by the request) and access the valid InetSocketAddress that Jetty already has in its request.

InetSocketAddress remote = ((org.eclipse.jetty.server.Request) httpServletRequest).getRemoteInetSocketAddress();

[sbordet]

Request.getRemoteAddr() is implemented in terms of InetSocketAddress.getHostString() or InetAddress.getHostAddress().

See below for JDK API behavior.

If there is no Forwarded header, we use the above JDK methods and therefore always return the unbracketed address.

If there is a Forwarded header, we use InetSocketAddress.createUnresolved() that behaves differently from the JDK methods above in that if a bracketed address is passed to it, it returns it bracketed (it actually returns the string as-is).

IMHO we should be bracketed in all cases, but I can live with unbracketed in all cases.

Perhaps simplest solution would be to unbracket the unresolved address in ForwardedRequestCustomizer, or find a way to obtain a resolved InetSocketAddress.

jshell> new InetSocketAddress("::1", 80).getHostString()
$1 ==> "0:0:0:0:0:0:0:1"

jshell> new InetSocketAddress("[::1]", 80).getHostString()
$2 ==> "0:0:0:0:0:0:0:1"

jshell> InetAddress.getByName("::1").getHostAddress();
$3 ==> "0:0:0:0:0:0:0:1"

jshell> InetAddress.getByName("[::1]").getHostAddress();
$4 ==> "0:0:0:0:0:0:0:1"

jshell> InetSocketAddress.createUnresolved("::1", 80).getHostString()
$5 ==> "::1"

jshell> InetSocketAddress.createUnresolved("[::1]", 80).getHostString()
$6 ==> "[::1]"

jshell> InetSocketAddress.createUnresolved("?", 80).getHostString()
$7 ==> "?"

[gregw]

@sbordet The simplest solution is to change HostPort and it will change the behaviour everywhere. I've been testing this in 9.3, but I'm thinking we should only make such a change in 9.4 onwards.

Let me prepare a PR against 9.4 for consideration. I'm pushing the EG for comment also.

[gregw]

I've created the PR. As much as I loath system properties for configuration, I have added one that allows the original behaviour to be restored. This makes me happier to do this in the middle of the 9.y.x series rather than just in 10.0

thoughts?

[gregw]

I'm wondering if we should merge this, but in 9.4 we keep the current behaviour by default, but in 10.0 we switch the default to the new behaviour?

thoughts?

[gregw]

It is not optional behaviour with STRIP_IPV6 false in 9.4 and true in 10.0

[gueuselambix]

I have noticed that this only works when the IPv6 address is enclosed by brackets [].
However if the IPv6 address is not enclosed with brackets, the code adds the brackets, despite of the STRIP_IPV6 setting.

I tested this with v9.4.28.v20200408:

curl -H 'X-Forwarded-For: 2001:db8:cafe::17' -g "http://[::1]:8080/" -s
tail -n1 /opt/jetty-base/logs/2020_05_19.request.log
[2001:db8:cafe::17] - - [19/May/2020:19:54:03 +0000] "GET / HTTP/1.1" 200 2722

curl -H 'X-Forwarded-For: [2001:db8:cafe::17]' -g "http://[::1]:8080/" -s
tail -n1 /opt/jetty-base/logs/2020_05_19.request.log
2001:db8:cafe::17 - - [19/May/2020:19:54:18 +0000] "GET / HTTP/1.1" 200 2722

Since X-Forwarded-For is not a real standard (as opposed to rfc7239) the precence of these brackets are arguable... However in most cases the IPv6 address is represented without brackets in the X-Forwarded-For header. This is also mentioned in RFC7239:

https://tools.ietf.org/html/rfc7239#section-7.4

7.4. Transition
Note that IPv6 addresses may not be quoted in X-Forwarded-For and may not be enclosed by square brackets, but they are quoted and enclosed in square brackets in "Forwarded".

   X-Forwarded-For: 192.0.2.43, 2001:db8:cafe::17

becomes:

   Forwarded: for=192.0.2.43, for="[2001:db8:cafe::17]"

https://github.com/eclipse/jetty.project/blob/jetty-9.4.28.v20200408/jetty-util/src/main/java/org/eclipse/jetty/util/HostPort.java#L77
causes to add the brackets in case the X-Forwarded-For header does not contain brackets.

I think it would be better to add those brackets only when STRIP_IPV6=false. In other words if it is true you would not expect IPv6 addresses with brackets.

[joakime]

I'll reopen this one for you.

[sbordet]

This has been fixed with the work for #5079.

[gueuselambix]

This issue has not been fixed. Brackets are still added to the IPv6 address. The remote address is not a hostname, so brackets should not be added.

curl -H 'X-Forwarded-For: 2001:db8:cafe::17' -g "http://[::1]:8080/" -s
tail -n1 /opt/jetty-base/logs/*.request.log
[2001:db8:cafe::17] - - [19/May/2020:19:54:03 +0000] "GET / HTTP/1.1" 200 2722

You marked this as fixed, but is isn't.
With the removal of the STRIP_IPV6 property it's even worse now.

[joakime]

@gueuselambix your issue has nothing to do with ForwardedRequestCustomizer.

The reason you are seeing brackets is because HttpServletRequest.getRemoteAddr() is returning the brackets on IPv6 addresses because of changes from issue #5079.
An IPv6 address with brackets is not a hostname, it's an IP-Literal per spec.

Please open a new issue if you feel this is inappropriate.

[sbordet]

@gueuselambix can you please detail what the problem actually is?

It's just a logging issue on the request log?
Is it just a formatting issue?

Using the Java APIs, there is no difference if an IPv6 address is bracketed or not.

Our point is that the latest specification for Forwarded treats IPv6 addresses always as bracketed, so while X-Forwarded-* may not bracket them, they should be.

Since bracketing seems the way to go, I don't see a case where un-bracketing would be necessary.
If you have one, by all means please report it (in a new issue) and we'll discuss it.

[joakime]

If you use X-Forwarded-For with a IPv6 address that also contains a port, then you must use the IP-Literal form with brackets as well.

[gueuselambix]

After processing of the X-Forwarded-For, ServletRequest.getRemoteAddr() returns the bracketed address. But I would expect the unbracketed IP address according to
https://www.javadoc.io/doc/javax.servlet/javax.servlet-api/latest/javax/servlet/ServletRequest.html#getRemoteAddr--

The brackets should IMHO only be added in a context where it is not clear to distinguish it from the port.

I am using a software which uses https://guava.dev/releases/19.0/api/docs/com/google/common/net/InetAddresses.html#forString(java.lang.String) which does not expect brackets.
I know they could use forUriString(java.lang.String) method, but IMHO it should be normalized by the servlet container.

[sbordet]

JDK's InetAddress is able to handle both un-bracketed and bracketed, so I'd suggest that Guava should do the same (maybe file an issue there?).

@gueuselambix getRemoteAddr() is unfortunately loosely defined when it mentions proxies or CGI. When the Forwarded header brackets the IP it's not clear that we should unbracket it.

@gregw we go back to "what's the difference between getRemoteHost() and getRemoteAddr()" which certainly is reverse DNS lookup, but should also be bracketing? Thoughts?

[gueuselambix]

@sbordet "For HTTP servlets, same as the value of the CGI variable REMOTE_ADDR." If you look at the spec of REMOTE_ADDR in the CGI spec https://tools.ietf.org/html/rfc3875#section-4.1.8, it revers to "Internet Protocol Version 6 (IPv6) Addressing Architecture" https://tools.ietf.org/html/rfc3513#section-2.2
That latter spec does not mention anything about adding brackets to the string repepresentation of an IPv6 address.

PHP suffered from a similar issue, which they fixed: https://bugs.php.net/bug.php?id=77722

[sbordet]

@gueuselambix by the same rfc3875, REMOTE_HOST also would be unbracketed.

No matter what, the moment we decide on one behavior, we break someone.

I think in the long run it would be better to not care at all about bracketing, like JDK's InetAddress.
IMHO Guava should be more lenient about this, since it seems to replicate a JDK class.

@gregw maybe you can raise it at the Servlet Specification?
See also jakartaee/servlet#368

[joakime]

RFC3513 is obsolete, replaced by RFC4291 in 2006.
RFC4291 is now also obsolete, replaced by RFC5952, RFC6052, RFC7136, RFC7346, RFC7371, and RFC8064 as far back as 2010. (and still being updated!)

Guava has a long history of issues around IPv6 support.
It doesn't support the same level of IPv6 that the JVM itself supports.

Guava doesn't support ...

• IPv6 with scope (supported by JVM, and is what is returned from JVM when iterating local network interfaces)
• IPv6 with IPv4 notation (RFC6052, supported by JVM, and is what is returned by the JVM when IPv4 is disabled by system property, but you need to talk to an IPv4 destination)
• IPv6 with cidr (supported by JVM, which is returned within some SSLEngine implementations)
• IPv6 with subnet mask (supported by JVM, which is returned within some Security providers)

They have no plans to support these (per their own responses in their issue trackers).

The fact that the Servlet spec HttpServletResponse.getRemoteAddr() returns a String and not a InetAddress is one of the root causes here.
It returns a String, which in the world of IPv6 is ambiguous.
The spec leads tried to address "how to represent IPv6 as text" in https://tools.ietf.org/html/rfc5952, but they even messed that up, there's enough errata on that spec that a new spec is being worked on, and those that implemented against the spec now have wildly differing opinions on how IPv6 as text should be represented (note, some of the errata points out that recommendations they have in the spec are actually forbidden per other specs related to IPv6!)
Because of this mess, we opted to support what the JVM supports for InetAddress, and since it supports the IP-Literal spec format we leaned that way, as it's the most stable and most reliable format for IPv6 as Text (and doesn't break users that take the string and tack on a port, which a SURPRISINGLY large number of projects do)

[gregw]

I'm very much against changing what we return by default.... again. We have been pushed back and forth by this as the world appears split between those that expect []; those that don't; and the growing group of those that can go either way.

Possibnly, we could possible add a compliance mode that stripped them from getRemoteAddr.