Support for X-Forwarded-For breaks existing behavior

[vrozov]

After upgrading to the latest build, existing installation does not work and connections are rejected with "Presto is configured to REJECT this header: X-Forwarded-For". It is not clear why the default is REJECT and not IGNORE, so it preserves the functionality. If REJECT is more suitable, why not to log a warning and proceed. Additionally, dispatcher.forwarded-header should not be case sensitive and should equally accept ignore or accept.

[findepi]

@vrozov
thank you for your feedback. Please accept apologies that I broke your installation.

First, the simple thing:

Additionally, dispatcher.forwarded-header should not be case sensitive and should equally accept ignore or accept.

This is standard behavior for config values that are backed by a Java enum, so quite a few other Presto configuration properties.
IIRC, this is handled here: https://github.com/airlift/airlift/blob/2a88cf63ee2c45d1b3b2a063397637f54ce407b3/configuration/src/main/java/io/airlift/configuration/ConfigurationFactory.java#L598-L606

We make all case-insensitive, or none. Let's keep this discussion aside though.

It is not clear why the default is REJECT and not IGNORE, so it preserves the functionality.

I see where you're coming from and I partially agree. Let me copy my explanation from #1033 (comment)

[...]
If this is disabled by default, admins configuring proxy in front of Presto may wonder why this doesn't work out of the box. They would need to search docs to know what to enable. So "off" is not a perfect default either.

Therefore I propose to have a feature toggle like support-x-forwarder-for with 3 possible values: REJECT, IGNORE, RESPECT. And REJECT would be the default.
This way, default behavior would be secure. Yet, admins configuring proxy would not wonder why this doesn't work by default.

I see two options going forward:

1. accept implemented behavior and it drawbacks
   • pro: If you put a reverse proxy in front of Presto, then you quite certainly want to enable this behavior (switch to ACCEPT)
   • cons: breaks some existing environments (requires one additional config property)
2. change REJECT to something like DISREGARD or IGNORE_WITH_WARNING
   • pro: backwards compatible
   • cons: awareness of the feature will be low, people will not realize they should turn it on when running with a proxy. This also has a security impact if you want to use query history for audit purposes.

@electrum @kokosing what do you think?

[findepi]

(In case we choose to go with option 2, I already made a PR with the change. I picked "WARN" as the name for the default behavior.)

[kokosing]

I prefer option 1, we need to make sure that this is properly described in release notes.

[martint]

@findepi, can you elaborate on what are the security implications you mention? I’ve never run into an HTTP server implementation that rejects a request when that header is provided.

Btw here’s the “standardization” of that header: https://tools.ietf.org/html/rfc7239

[findepi]

"remote user address" gives information about a user. You may rely on this information. Yet, if you use proxy in front of Presto, you don't have the information.

[electrum]

Unfortunately, I missed the original issue. But after reading the explanation, and the proposed options, I believe the current implementation is the right way. This is one of those situations where both options are a security hole, depending on the environment:

• ACCEPT allows spoofing if Presto is not behind a proxy.
• IGNORE logs the wrong IP address when behind a proxy.

Using REJECT as the default allows us to be secure by default in the common case of no proxy. The downside is that it breaks existing installations that use a proxy. Fortunately, it breaks immediately, with a clear error message and is easy to resolve.

On the other hand, respecting the header is a new feature, so WARN might be a reasonable default, at least during a transition period.

[findepi]

We have discuss this at length and decided to change the default behavior to WARN and drop REJECT behavior for now. We still may revisit this decision later, because there still are valid reasons not to ignore the header.

Thanks @vrozov for your report!