Reactor Netty does not support X-Forwarded-* request headers

[mbhave]

No description provided.

[bclozel]

As discussed with @mbhave @rstoyanchev @wilkinsona @rwinch

This probably happens when a proxy terminates the SSL connection and uses X-Forward-* request headers to convey information to the application.
Spring WebFlux provides a ForwardedHeaderFilter for those cases, but its MVC counterpart is not being auto-configured in Spring Boot currently (see #5677).

This issue should be about how we should deal with those cases in Spring Boot WebFlux applications (with all supported servers).

[wilkinsona]

If we're going down the route of using our own filter, we should consider #5677 again as there'd certainly be some benefit in a consistent approach across the WebFlux and Servlet worlds. I know @rwinch would still like to see us do that. On the other hand the existing support probably works with WebFlux on top of Tomcat, Undertow, and Jetty so something that's Reactor Netty-specific would be another option.

[bclozel]

I've asked for an enhancement on the Reactor Netty project as this feature probably belongs in the server codebase itself anyway. See reactor/reactor-netty#220.

In the meantime, this can be partially solved for other servers by implementing it in DefaultReactiveWebServerCustomizer (and mirroring what we're doing with Servlet-based servers).

[snicoll]

@bclozel same thing, this issue is blocked by a reactor-netty issue that isn't scheduled for 0.8.0 at the moment.

[bclozel]

@snicoll I've provided a PR for that, scheduled for 0.8.

[rwinch]

this issue is blocked by a reactor-netty issue that isn't scheduled for 0.8.0 at the moment.

Have we come to a conclusion about using ForwardedHeaderFilter? It seems that this would be an ideal and consistent way of supporting X-Forward-* headers

[rstoyanchev]

There is one extra concern to consider. While server.use-forward-headers is off by default, which is a safe default, forwarded headers can still be consumed when a UriComponentsBuilder is created with the fromHttpRequest factory method -- e.g. when injecting a UriComponentsBuilder into a controller method, or using MvcUriComponentsBuilder to create links, or making CORS checks. This is documented but not hard to miss.

This is one reason why the ForwardedHeaderFilter provides an option to remove (discard) forwarded headers when not expected, or when not behind a trusted proxy, ensuring the choice that such headers will not be used.

One, it would be useful to have some sort of server.remove-forward-headers flag that would be mutually exclusive with server.use-forward-headers. Two, setting that flag on by default would be consistent with the default setting of server.use-forward-headers, enforcing it more strongly.

Something to consider in this discussion.

[bclozel]

Thanks @rstoyanchev , I've created #11525 to track that enhancement.

We probably won't be using the ForwardedHeaderFilter across the board, for several reasons:

• we currently support that with the native server support. Each server handles that its own way, supporting some/all non-standard X-Forwarded or standard Forwarded headers. Supporting that consistently would be better, but we'd break existing applications
• using that filter means we're creating two request/response wrapper objects for each exchange; that will cause some performance degradation
• some containers (like undertow) might not work well if we change the request/response information in a filter (see UNDERTOW-1197)
• this filter does not support additional features, like the list of trusted proxies. Applications might be relying on that.

We've decided to use reactor-netty's support as soon as it's merged by the team.

[rwinch]

I hope the boot team will reconsider. Inconsistency with how X-Forwarded is supported is one of the largest issues I come across in support of Boot + Security. It has been the source of plenty of CVEs and switching to ForwardedHeaderFilter would help alleviate all of these concerns.

Supporting that consistently would be better, but we'd break existing applications

Boot has made quite a few breaking changes in 2.0, so I don't think this provides a good reason. Especially since users can easily revert to the previous behavior using EmbeddedServletContainerCustomizer.

using that filter means we're creating two request/response wrapper objects for each exchange; that will cause some performance degradation

The X-Forwarded headers will be removed as soon as one of the wrappers is done. This means that additional wrapping will not be done. Additionally, users should be able to easily disable ForwardedHeaderFilter if they provide the native support.

some containers (like undertow) might not work well if we change the request/response information in a filter (see UNDERTOW-1197)

We have not had any issues with this. However, it should be easily resolved by adjusting the dispatch types properly.

this filter does not support additional features, like the list of trusted proxies. Applications might be relying on that

True. However, containers do not support all the features of the ForwardedHeaderFilter either. Again, users can easily revert to using EmbeddedServletContainerCustomizer.

I'm guessing by using a consistent mechanism Boot would be able to delete quite a bit of app server specific code. Users would get a more consistent experience. In many cases they would get more features (if there are features missing that are important, please log an issue). As @rstoyanchev pointed out they would be more secure as well.

[rstoyanchev]

#11525 answers my concern.

As for the overall question of using the filter or not, I do agree and see the point that Boot is in a position to handle forwarded headers more efficiently at the server level. It's arguably justified even if it means a bit of extra code. However that should not come at the expense of any potential for CVEs.

Inconsistency with how X-Forwarded is supported is one of the largest issues I come across in support of Boot + Security. It has been the source of plenty of CVEs and switching to ForwardedHeaderFilter would help alleviate all of these concerns.

@rwinch can you provide a little more detail? This is a crucial point IMO and needs more elaboration.

[rwinch]

I do agree and see the point that Boot is in a position to handle forwarded headers more efficiently at the server level

Can you elaborate on how this is more efficient?

can you provide a little more detail?

There are inconsistencies in the fact that Tomcat does not support X-Forwarded-Host but other application servers do. I have seen this where an application was verified to be secure on Tomcat and then it was later ran on Jetty where the X-Forwarded-Host header was used to produce malicious links (think any link with a token on it that should be restricted to the actual host SSO tokens, password resets, etc).

Another issue is Tomcat is the only container that provides validation of proxies. This validation provides little value since under most circumstances if you can forge an X-Forwarded-Proto header you can also forge a X-Forwarded-For header. This causes users difficulty when trying to setup X-Forarded- support on Tomcat, but works without the need to configure trusted proxies on other containers (i.e. Jetty). This issue is one of the most frequent issues I support in regards to single sign on when using TLS termination.

[wilkinsona]

We have not had any issues with this. However, it should be easily resolved by adjusting the dispatch types properly.

Unfortunately, it's more than that. A key difference is that, when using the native option, Undertow configures an internal object (its exchange type, IIRC) based on the headers. The filter loses that functionality. I haven't reviewed Undertow's code to see precisely how the configuration on the exchange is used, but losing the functionality makes me nervous as it must be there for a reason. I wouldn't want someone to be lulled into a false sense of security as they assumed Undertow was using the native header functionality and would be behaving as Undertow can reasonably be expected to behave in such a situation.

Users would get a more consistent experience

I think this argument cuts both ways as there are two types of consistency: consistency across containers and consistency with how users expect a particular container to behave. Which you consider to be more important is subjective, but we're choosing between consistency with two different things rather than consistency vs inconsistency.

For other functionality where it can be done natively vs in a filter (response compression, for example), we've opted to configure it at the container level. That has served us well thus far as users have welcomed things working exactly as they'd expect for a particular container.