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