Reject X-Forwarded-* headers by default

[findepi]

Today we ignore X-Forwarded-* headers by default. This leads to a lot of confusion for people setting up Trino behind a load balancer. They have hard time learning about http-server.process-forwarded.

We cannot accept the headers out of the box, as it would create security vulnerability. The X-Forwarded-For value is logged by the system and so can be used to mask request origin.

In order to improve out of the box experience, let's reject requests with X-Forwarded-* headers by default.

This follows up to #1159, #1193, #1189, #1033 (comment)

[findepi]

cc @dain @electrum @kokosing @vrozov

[martint]

as it would create security vulnerability

It would allow spoofing, but not necessarily a security vulnerability, since there are no security decisions that depend on this information.

An alternative would be to make both the forwarded address and the remote address available to the parts that need that info within Presto. Then, we could turn this on unconditionally and let those parts make decisions about what information they need or can use.

[losipiuk]

In order to improve out of the box experience, let's reject requests with X-Forwarded-* headers by default.

@findepi It feels to me that rejecting requests with those headers, if not explicitly allowed makes sense. To allow an easy workaround for (probably rare ) situations when someone actually needs to send those headers to Presto and does not want them interpreted, we can modify the http-server.process-forwarded config property to allow explicit ignoring of headers from the request. So possible config values for http-server.process-forwarded could be:

• REJECT - throw if X-Forwarded-.. header is present in request (default)
• PROCESS - enable processing of headers
• IGNORE - ignore the headers

For backward compatibility, we can still allow setting http-server.process-forwarded to false which would map to REJECT and true which would map to PROCESS.

An alternative would be to make both the forwarded address and the remote address available to the parts that need that info within Presto. Then, we could turn this on unconditionally and let those parts make decisions about what information they need or can use.

@martint Would we have more information in the decision point then, than we have right now at the point when we decide if we should process the headers or not? Or we are just moving problem to another layer and we still need similar config property as we have now, just named differently, due to different context.

[electrum]

This isn't just about addresses. It's also the host header and protocol. We moved it into Jetty specifically to handle it uniformly in a single place. There's no way for anything to "make decisions" about using the information since that is deployment specific, which is what the existing config provides.

There are two scenarios in which we receive a legitimate request with a forwarded header when http-server.process-forwarded=false:

1. The server is behind a load balancer and the admin didn't know to enable forwarding.
2. The server is not behind a load balancer, but something else is adding that header, such as a corporate proxy.

The first scenario occurs often and it causes confusion and support requests. There's no way to tell how often the second scenario occurs, but it seems far less likely. This new config would favor the first scenario, by making it an explicit error, while still allowing the second scenario by setting a config.

[findepi]

This isn't just about addresses. It's also the host header and protocol.

Correct, the http-server.process-forwarded impacts handling of a bunch of related headers.
However, most of them we can safely process by default with no apparent drawbacks, except for the X-Forwarded-For.
The others are taken into account for rendering the response to the client, so cannot be used maliciously.

The server is not behind a load balancer, but something else is adding that header, such as a corporate proxy.

I am not sure what kind of setups you have in mind here and also I am definitely no expert on corporate proxies either, so this may be a dumb question....
If a such a proxy adds X-Forwarded- headers and we ignore them (which we do today by default):

• if we ignore X-Forwarded-For we record proxy address instead of original user, which is not the intention
• if we ignore others X-Forwarded- headers, the next request may bypass the proxy, which was not supposed to happen (from proxy perspective)

seems like both options as wrong, wherein rejecting by default would be helpful for a corporate proxy case too. Am i missing something?

[dain]

I think we should go ahead with this.

[electrum]

It would allow spoofing, but not necessarily a security vulnerability

We make remoteClientAddress available in the event listener, which can be used for auditing purposes. It could be argued that allowing the user to spoof the IP address is a vulnerability. I imagine it could fail a compliance policy that requires logging of IP addresses.

The others are taken into account for rendering the response to the client

The forwarded protocol is used to determine if the request is "secure" for the purposes of accepting authentication. Spoofing the header would allow users to send authentication over plain HTTP. At minimum, it is a violation of policy, and I can see it being abused, as people sometimes ask for such a feature. There's a reason we don't have it, and allowing any client to do it seems like a bad idea.

for a corporate proxy case

I was imagining a branch office that accesses the internet and/or the corporate intranet via a caching proxy like Squid (maybe because they have a slow or limited connection). For whatever reason, the proxy adds the X-Forwarded-For header, maybe intentionally, or perhaps it is misconfigured. But from the standpoint of the Trino cluster, that proxy is NOT trusted, since it's run by some random IT staff and not part of the data center operations. So the requests to Trino will have this header, legitimately, not due to spoofing, although perhaps undesirable and yet unavoidable.

I don't know the likelihood of this scenario. This was just me brainstorming about a remotely feasible scenario in which a Trino cluster would legitimately receive such headers but that the administrator would want to ignore them. It seems unlikely, but since we have no visibility and this would be a breaking change, it seems best to pay the small price to allow for it via config.

[electrum]

I agree we should do this. @losipiuk's suggestion of modifying the Airlift code seems good. We can have our own version of ForwardedRequestCustomer that does the rejection. I'm not sure if the response status code should be 400 or 403.

There's also the question of if we should log the failure, and if so, exactly what to log and at what level. We generally try to avoid default logging such that an unauthenticated client could flood the logs, especially in Airlift code. The requests will be visible in the HTTP request log, but not the reason for the rejection.

[findepi]

cc @wendigo

[wendigo]

Here you go @findepi: airlift/airlift#1183

[wendigo]

I've implemented this in Airlift. Update pending