Feature #7586
closedConsider disabling use of Client-IP header, at least by default
0%
Description
As of github wt 4.3.0-28-ge779486d, Wt will consider both "X-Forwarded-For" and "Client-IP" headers when determining the clientAddress of an http session when the "behind-reverse-proxy" option is enabled. Since the X-Forwarded-For header seems to have become the de facto standard, it may make it easier to configure reverse proxies if "X-Forwarded-For" is the only recognized header --- at least by default.
Attached, for your review, is a patch that adds support for a "reverse-proxy-client-ip" option in wt_config.xml. The option defaults to false, disabling recognition of "Client-IP" headers. However, the current behavior can be restored by setting the option to true.
The patch is mostly boilerplate and comes with some test code. NOTE: The code path for dedicated sessions is somewhat different and needs to be tested separately.
Alternatively, the issue could be addressed through additional documentation on behind-reverse-proxy or by removing "Client-IP" header support entirely, if it is no longer required.
Files
Updated by Roel Standaert about 4 years ago
- File 0001-Issue-7586-configurable-trusted-proxies-and-forwarde.patch 0001-Issue-7586-configurable-trusted-proxies-and-forwarde.patch added
- Status changed from New to Feedback
I decided to instead take inspiration from Apache's mod_remoteip, because I always felt that they got it right. Our main problem I think is that we would just trust everything when behind-reverse-proxy was enabled. I attached the patch so you can let me know what you think.
Updated by Bruce Toll about 4 years ago
- File 0002-Configurable-trustedProxies-minor-fixes.patch 0002-Configurable-trustedProxies-minor-fixes.patch added
Hi Roel,
I think this is a great enhancement! It provides a cleaner, safer, more general approach to reverse proxy support and, as usual, I learned some new techniques from your code.
The new tests did not initially pass on my system. I'm attaching a trivial patch that seems to fix them here, although I'm not sure it is correct.
The patch file also includes an additional test case which I believe should be valid --- and a small fix that gets it to pass.
NOTE: My testing has been limited to the test.http application. I plan to do some more testing with nginx as a reverse proxy, but I doubt that it will uncover any issues as our configuration is very basic.
Regards,
---Bruce
Updated by Roel Standaert about 4 years ago
- Status changed from Feedback to InProgress
- Assignee set to Roel Standaert
Oops, I shouldn't put up a patch without rerunning the tests first :-). That behindReverseProxy() check was a last minute tweak.
Updated by Roel Standaert about 4 years ago
- Status changed from InProgress to Resolved
I pushed a final version of this. We iterated on it a bit more. behind-reverse-proxy is now deprecated. It works as before if set to true, but otherwise, we added a <trusted-proxy-config> section:
<trusted-proxy-config>
<original-ip-header>X-Forwarded-For</original-ip-header>
<trusted-proxies>
<proxy>127.0.0.1</proxy>
<proxy>::1</proxy>
</trusted-proxies>
</trusted-proxy-config>
Having no trusted proxies (which is the default) is now the same as the old setting <behind-reverse-proxy> to false.
Updated by Bruce Toll about 4 years ago
Thanks for the update. Nice improvements!
Updated by Roel Standaert almost 4 years ago
- Description updated (diff)
- Status changed from Resolved to Closed