Closes #23260

Comment From: pivotal-issuemaster

@molassar Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

Comment From: pivotal-issuemaster

@molassar Thank you for signing the Contributor License Agreement!

Comment From: rstoyanchev

@molassar, currently ForwardedHeaderFilter (Servlet API) and ForwardedHeaderTransformer (WebFlux) share forwarded headers parsing via UriComponentsBuilder#fromHttpRequest which in turn delegates to adaptFromForwardedHeaders. I think we'd want some similar arrangement. Perhaps an additional adaptFromForwardedForHeaders on UriComponentsBuilder that sets the host and port to the ones from the "Forwarded for" or "X-Forwarded-For" header (i.e. the remote ones). We even have logic already for parsing the server host and port that might be possible to re-use for parsing the remote host and port.

Comment From: molassar

@molassar, currently ForwardedHeaderFilter (Servlet API) and ForwardedHeaderTransformer (WebFlux) share forwarded headers parsing via UriComponentsBuilder#fromHttpRequest which in turn delegates to adaptFromForwardedHeaders. I think we'd want some similar arrangement. Perhaps an additional adaptFromForwardedForHeaders on UriComponentsBuilder that sets the host and port to the ones from the "Forwarded for" or "X-Forwarded-For" header (i.e. the remote ones). We even have logic already for parsing the server host and port that might be possible to re-use for parsing the remote host and port.

As far as I have understood, UriComponents is just more powerful alternative of URI. Will it be semantically correct to put remote host and port in it? Since, remote host has nothing to do with uri, it seems that UriComponents is not a proper place for it.

Comment From: rstoyanchev

Indeed it wouldn't make sense to add anything to do with "remote" host or port. UriComponentsBuilder is just another way of building URIs as you said, and in this case we would be building a URI that represents the remote address. Just like we currently create ServerHttpRequest from the URL and then call adaptFromForwardedHeaders, I was thinking we could also separately create a ServerHttpRequest from the remote address URL and then call adaptFromForwardedForHeaders to update it. That way the logic for extracting such header values is encapsulated which also provide opportunity for re-use in the implementation.

Comment From: molassar

Indeed it wouldn't make sense to add anything to do with "remote" host or port. UriComponentsBuilder is just another way of building URIs as you said, and in this case we would be building a URI that represents the remote address. Just like we currently create ServerHttpRequest from the URL and then call adaptFromForwardedHeaders, I was thinking we could also separately create a ServerHttpRequest from the remote address URL and then call adaptFromForwardedForHeaders to update it.

Ok, now I see your point

Comment From: rstoyanchev

@molassar I've merged this and made some further updates. Notably I removed the logic related to obfuscated host. My reading of the spec is that this is something proxies might do but we only read the first remote address which should be the client address.

Comment From: T3rm1

Would have been nice to add a warning that having this enabled makes you vulnerable to IP spoofing (client can just send X-Forwarded-For header himself). Tomcat has a nice implementation with it's RemoteIpFilter where you can define which IPs you trust and which does not use the leftmost value (if untrusted).

Comment From: bclozel

The "Forwarded" section in the reference documentation mentions:

There are security considerations for forwarded headers since an application cannot know if the headers were added by a proxy, as intended, or by a malicious client. This is why a proxy at the boundary of trust should be configured to remove untrusted Forwarded headers that come from the outside. You can also configure the ForwardedHeaderFilter with removeOnly=true, in which case it removes but does not use the headers.

Is there anything missing here?

Also, the trustedProxies option for the RemoteIpFilter doesn't seem to be about ignoring request headers depending on the IPs listed, but rather about setting the proxiesHeader value or not. An IP from the Forwarded header is used as the remote IP no matter what (see otherwise, the ip/host is declared to be the remote ip and looping is stopped.).

As you've said already, this header can be easily manipulated if not properly cleared by the proxy, so I don't think the RemoteIpFilter is more secure in that regard.

Comment From: T3rm1

Ah, I didn't know there is documentation for the Filter on the web. I was referring to the Javadoc of the class. That's where I always look first and I didn't expect that there is additional documentation on the web. I would still suggest to have a warning in the class as this topic is not so easy to understand at the first glance.

I think you missunderstood how the RemoteIpFilter works. The key difference is that values are processed right to left unlike using always the leftmost value. Here is an example:

Say we have a client (1.1.1.1) that manipulated his request and sets X-Forwarded-For: 2.2.2.2.

There are now two cases:

  1. The request reaches backend directly. The Spring filter would set 2.2.2.2 as the remote address. The RemoteIpFilter would not, because 2.2.2.2 is neither trusted nor internal. The remote address would stay the same -> 1.1.1.1.

  2. We have an intermediate proxy/loadbalancer/cdn (3.3.3.3) that forwards the requests. It adds the client ip to the right of X-Forwarded-For. Now we have X-Forwarded-For: 2.2.2.2, 1.1.1.1 Again, the Spring filter sets 2.2.2.2 as the remote address. The RemoteIpFilter however (given the proxy is trusted) detects that 1.1.1.1 is the first untrusted ip and thus uses this as the new remote address.

So no matter what the client sends, it won't be picked up by the RemoteIpFilter if it is not trusted/internal. (Edit: Not 100% true for the first case. See example from @bclozel below.

Comment From: bclozel

@T3rm1 Interesting, what happens if the client sends a request directly to the backend (so not going through any proxy) with the following header: X-Forwarded-For: 2.2.2.2, 3.3.3.3?

Comment From: rstoyanchev

Looking at RemoteIpFilter it updates the forwarded header so that only trusted proxies remain. This is the sort of cleanup we expect to occur before the request gets to the ForwardedHeaderFilter and is best done as early as possible (as Brian alluded to with his most recent comment). Point taken though that we can improve the javadoc on ForwardedHeaderFilter. I would have expected this to also be mentioned there.

Comment From: T3rm1

@T3rm1 Interesting, what happens if the client sends a request directly to the backend (so not going through any proxy) with the following header: X-Forwarded-For: 2.2.2.2, 3.3.3.3?

Interesting case, although unlikely because the client would need to know that 3.3.3.3 is configured as trusted. In this case the remote address would be 2.2.2.2 (as 3.3.3.3 is trusted and 2.2.2.2 is not). Same result for Spring filter.