Add support for 'X-Forwarded-For' header #87
Conversation
There was a problem hiding this comment.
Thanks for your contribution!
And apologies for taking almost 1 year to get back to you...
Overall this looks like a clean patch and a good addition. But I am wondering why we would enable this new header by default and giving an option to disable it, instead of leaving the default behaviour as is and giving an option to enable it. Could you explain?
Sorry, something went wrong.
i suppose you meant disable here ? |
|
I followed the path of 'Via' header, that serves similar task. It is controlled by 'disable' switch I see two ways: rename both options, or set a 'disable' default value. Remember, that changing a variable name for the option will call for a change of a field name in configuration file, and that requires updating of all configurations. So right now I'd suggest the latter way. |
|
Also I just realized that I forgot to add a proper line to |
|
@rofl0r wrote:
No. The current patch enables the new header by default and offers a disable option. I was suggesting to do the reverse. |
|
@vbelov42 wrote:
Tinyproxy currently has no capability of adding the XFF header. So the default behaviour is not to set it. The reasonable way to introduce the new feature (according to me, quite generally) should be to not change the current behaviour but leave it as is and maybe change the default in a later release if desired. I.e. you would have to actively do a configuration change in order to trigger the new behaviour (setting the XFF header). What adds to this, imho, is that the XFF header may not be universally desired, since it exposes the client IP to the server on the other side of the proxy. So if we take the behaviour proposed in the current patch, a user would suddenly get "unstealth-ed" by an upgrade. The fact that the via header is enabled by default is not relevant: The via header is an existing capability. This here is about introducing a new capability. It is more about expected behaviour than about similarity of code. |
|
Just for the case, I did what you requested. It's in 5df3615. |
|
Can this be merged? (Aside: The via header is a requirement as per the RFC, which is why it needs to be enabled by default) |
|
"the RFC" ? please be more specific. |
|
RFC 7230 marks it as a |
|
both the RFC and the cloudflare blog talk about the |
|
Yeah, mentioned that in my comment. Was just giving reason why the Via header is default On. See above for some discussion around it. |
|
oh, you're right. 👍 |
|
Is this PR still under consideration? |
|
TinyProxy is a great project - but it would be even better with XFF. Especially when working with backend services running in k8s. It is perfectly fine to disable XFF by default Therefore, please provide a status on this pull request |
that sounds like an ingress problem then. maybe you should take it there.
it would need some work for infrastructure changes that happened related to conf parsing. as the internet gets more and more hostile against proxy users (thanks cloudflare) i personally am not eager to add yet another option that removes privacy. |
Usually people care about privacy and anonymity on the Internet, so proxy software is frequently used to filter out or mange several headers, to hide client's identity. But sometimes situation is completely different and one needs to tell the server about client's real address. Here comes a 'X-Forwarded-For' header that was introduced by the Squid caching proxy server's developers.
'X-Tinyproxy' field serves the same target, but is much less recognized.
In a nutshell, I just mutated
write_via_header()function intowrite_xff_header(). And placed a call for it afterwrite_via_header()in processing. I'm not 100% positive that it's correct forprocess_server_headers()case.Also I added an option to the configuration file to control this. It has some description and default value that disables this feature.