Supporting the Forwarded header (RFC 7239)

[Maxim Dounin]

Hello!

On Sun, Aug 06, 2017 at 09:11:03PM +0300, Vasiliy Faronov wrote:

> Hi nginx developers,
> 
> As you know, RFC 7239 defines a Forwarded header to replace the zoo of
> X-Forwarded-* with a single extensible syntax.
> 
> There seems to be growing interest in deploying Forwarded. For
> example, aiohttp, a popular Python library, recently started reading
> Forwarded by default.
> 
> I'd like to see a $proxy_add_forwarded variable in nginx -- similar in
> essence to $proxy_add_x_forwarded_for -- and I'd like to try and
> contribute a patch.
> 
> Can you tell me if this sounds like a good idea to you, and if yes,
> which of the following approaches you prefer? (I'm sorry for the wall
> of text)

We've previously considered adding Forwarded support, though 
postponded it as it seems to be somewhat different from 
X-Forwarded-For / X-Real-IP we do support, and we haven't seen any 
practial implementations.

> 1. $proxy_add_forwarded blindly appends a "for=..." after a comma,
> just like $proxy_add_x_forwarded_for does. (This cannot be emulated in
> config because of ticket #1316; also, nginx's $remote_addr is not
> quite in the format required by RFC 7239.)
> 
> The problem here: suppose an external user sends:
> 
>     Forwarded: for=injected;by="
> 
> If you just append to this, you get:
> 
>     Forwarded: for=injected;by=", for=real
> 
> This puts the burden on the receiving application to parse this
> robustly and recover "for=real". They can't just split on comma
> (because comma can occur inside a valid quoted-string), they need more
> logic. It's doable, they just have to be aware of the problem and
> actively mitigate it.

Certainly blindly adding a value without checking the header 
syntax looks like a bad idea.  It opens an obvious vulnerability 
as RFC-complain parsing by the upstream server will produce an 
incorrect "for=injected" result.

It is also not clear how to deal with such syntactically incorrect 
headers: removing the previous headers will obviously result in an 
information loss, while "fixing" them also looks wrong.

>From this point of view, X-Forwarded-For is much better, as it 
does not require to parse anything.

> 2. Parse any incoming Forwarded headers into an internal
> representation, then re-serialize it with an added element.
> 
> This is obviously more expensive. But, if you think that supporting
> Forwarded is a good idea, then eventually you want to support it in
> ngx_http_realip_module and wherever else nginx looks at X-Forwarded-*.
> Then you need to parse anyway.

This looks like an overkill to me, especially considering that 
Forwarded can have arbitrary extensions.  

> 3. Validate the syntax of any incoming Forwarded headers with a regex.
> If they are valid, append to them. If they are invalid, replace them
> with a single "for=unknown" (explicitly allowed by RFC 7239), and
> append to that.

I don't see how this is explicitly allowed by RFC 7239.
And anyway this is an information loss, see above.

> 4. Do any of the above, but in a third-party module, where one could
> experiment more freely.
> 
> I think requiring a third-party module to support Forwarded will just
> lead to everybody sticking with X-Forwarded-*, or else trying to
> emulate it in config with poor results. It's just not enough of a
> feature on its own.

It should be more or less trivial to implement config-based 
emulation of $proxy_add_forwarded, using map{} and appropriate 
regular expression checks.  The main problem here is ticket #1316, 
yet it probably should be addressed separately.

On the other hand, such approach also faces the problem on 
what to do with syntactically invalid Forwarded headers, and I 
don't think I know a solution.

(Also, the interesting part is probably using Forwarded in the 
realip module, and this is certainly can't be done as a 3rd party 
module without re-implementing the whole module.)

> Then again, maybe RFC 7239 was a bad idea and everybody *should* just
> stick to X-Forwarded-*.

I think _eventually_ there should be some standard on this, and 
RFC 7239 looks like a step in the right direction.  It seems to 
have various problems though, and it may be a good idea to 
postpone the implementation till it is clear how to address these 
problems and/or an updated RFC to address them is available.

-- 
Maxim Dounin
http://nginx.org/