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/
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel