Misleading port number defaults when parsing 'Forwarded' header

[jespersm]

The root cause

ForwardedHeaderUtils.parseStandardHeader uses surprising defaults for port numbers for ForwardedInfo.forAddress in the cases where the reverse proxy didn't supply it, and the ForwardedHeaderFilter deepens this issue, perhaps for historical reasons.

Background

Forwarded: for=xxx is useful when logging the original IP and port number of clients, for when the HTTP connection is terminated by a (reverse) proxy, such as a CDN or load-balancer. With Carrier Grade NAT, the port number is as important as the IP address, but is not set by all proxies.

For extracting the info from the original client an application (and Spring Web itself) uses ForwardedHeaderUtils, and methods such as parseStandardHeader. When using those, a "fallback" InetSocketAddress can be passed, in remoteAddress, but only the port number part is ever used.

Problem 1:

Take the case where a web app receives a header Forwarded: for=1.2.3.4 (i.e. with no remote port). With the defaults in ForwardedHeaderFilter, this will end up resolving the forAddress with a the IP address sent by the proxy, but the port number from the socket, i.e. the client port of the proxy servers outgoing interface. This is wildly confusing IMHO, and as an application, it's impossible to know in general whether or not the port number is right. Using a remote port = 0 (which can never be a real client port) would be more correct.

In other words, when using the ForwardedHeaderFilter (e.g. by autoconfig with server.forward-headers-strategy=framework) this code will mix info from the real client and the proxy:

    @RequestMapping(value = "/whoami", produces = "text/plain")
    public ResponseEntity<String> index(HttpServletRequest request) {
        var response = String.format("You are '%s' at port '%s'\n", request.getRemoteAddr(), request.getRemotePort());
        return ResponseEntity.ok(response);
    }

If we don't have the remote port, I'd prefer returning something to indicate that.

However, since this is also how Tomcat does it, I realize that there may be some friction to ever changing it.

Problem 2:

Due to the limitation in problem 1, one might not use the filter, since it will remove the original Forwarded header(s), and instead only rely on ForwardedHeaderUtils for parsing and logging. An example (in test form):

		MockHttpServletRequest request = new MockHttpServletRequest();
		request.addHeader("Forwarded", "for=192.0.2.0");
		request.setScheme("http");
		request.setServerName("proxied.com");
		request.setRequestURI("/");

		HttpRequest httpRequest = new ServletServerHttpRequest(request);
		ForwardedHeaderUtils.ForwardedInfo info =
				ForwardedHeaderUtils.parseStandardHeader(httpRequest.getURI(), httpRequest.getHeaders(), null, null);

		// The values we want to log:
		assertThat(info.forAddress().getHostString()).isEqualTo("192.0.2.0");
		assertThat(info.forAddress().getPort()).isEqualTo(80); // I doubt many client would come from port 80 !

Filling the remote (= client) port number with an inferred value, relevant to the server side only, is very misleading.

Workarounds

Problem 1 has no workaround, short of A) either dumping the filter, or B) checking if your proxy gives you port numbers, and if it doesn't, then never use the value from getRemotePort().

For problem 2, there is a workaround, which is to put a sane default into a dummy remoteAddress:

[...]
		ForwardedHeaderUtils.ForwardedInfo info =
				ForwardedHeaderUtils.parseStandardHeader(httpRequest.getURI(), httpRequest.getHeaders(), new InetSocketAddress(0), null);

		// Now, we know what we get
		assertThat(info.forAddress().getHostString()).isEqualTo("192.0.2.0");
		assertThat(info.forAddress().getPort()).isEqualTo(0); // Ah, not in the header

PR is coming for problem 2.
If you want a patch for problem 1, I'd happily provide it!

[rstoyanchev]

In Spring's URI parsing the port is the actual port or -1. There is no precedent for setting it to 0. That would be a bigger (and very unlikely) change than just forwarded header parsing. It would also show through HttpServletRequest#getRemotePort, and I'm not sure that's expected to ever return 0. Every use of the port would now have to have a check for port 0.

The fallback logic to fill in a port value is the most we can do if the port is not specified in "Forwarded: for=...". It's all we have, and using 0 is just pushing the problem downstream causing every use of the port to check for a value of 0.

Do you have more details on cases where proxies don't set the port or why? That seems like the root cause.

[jespersm]

The URI parsing shouldn't affect the remote port, i.e. the client port of the proxy.

Why would anyone need to check for '0'? I can't think of other uses of the result from getRemotePort than logging or forwarding. And my point is that if you log or forward the value, it should be correct, which it isn't. And I missing something there?

I totally agree that proxies in front should forward the port number, but many don't (or they don't do by default). AWS load balancer, for example.

[jespersm]

Apologies if my language is unclear: I'm not suggesting changing the URI parsing, just that we don't use the server port as a default for the client's port.