Project

General

Profile

Actions

Feature #7586

closed

Consider disabling use of Client-IP header, at least by default

Added by Bruce Toll almost 4 years ago. Updated over 3 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Roel Standaert
Target version:
Start date:
05/28/2020
Due date:
% Done:

0%

Estimated time:

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

Actions #1

Updated by Roel Standaert over 3 years ago

  • Target version set to 4.5.0
Actions #2

Updated by Roel Standaert over 3 years ago

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.

Actions #3

Updated by Bruce Toll over 3 years ago

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

Actions #4

Updated by Roel Standaert over 3 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.

Actions #5

Updated by Roel Standaert over 3 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.

Actions #6

Updated by Bruce Toll over 3 years ago

Thanks for the update. Nice improvements!

Actions #7

Updated by Roel Standaert over 3 years ago

  • Description updated (diff)
  • Status changed from Resolved to Closed
Actions

Also available in: Atom PDF