Reverted logic for HTTP_X_FORWARDED_HOST and HTTP_X_FORWARDED_FOR

[melo]

The code was using the last item on those headers but the correct one to
use is the first.

For HTTP_X_FORWARDED_FOR, this is reasonable documented:

• https://en.wikipedia.org/wiki/X-Forwarded-For#Format;
• https://metacpan.org/pod/Plack::Middleware::XForwardedFor also uses the
  fist element (ignoring trust for now, which ReverseProxy also ignores).

For HTTP_X_FORWARDED_HOST, I could not get a resonable specification, the best I found was a
test case on the Spring Framework:

https://jira.spring.io/browse/SPR-11140

I do belive that getting the first element is the correct one though, for
consistency with HTTP_X_FORWARDED_FOR.

Another point in favor of using the first element is that both Apache and
nginx add the current host if they find an x-* header in the requests.

Apache handling of X-Forwarded-* starts at
http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/proxy_util.c?view=markup#l3599
The key line is http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/proxy_util.c?view=markup#l3626
where the current IP is appended to the current value (spec for apr_table_mergen
is at http://apr.apache.org/docs/apr/1.5/group__apr__tables.html#ga1d50805448114c476cfcd00d5ee3e3a8 ).

nginx logic is at https://github.com/nginx/nginx/blob/master/src/http/modules/ngx_http_proxy_module.c#L2389
and it also concats the IP.

So on both servers, the first element is the client element.

Obligatory StackOverflow post: http://stackoverflow.com/questions/13111080/what-is-a-full-specification-of-x-forwarded-proto-http-header

Signed-off-by: Pedro Melo melo@simplicidade.org

[miyagawa]

I believe that this logic has been inherited from Catalyst and then HTTP::Engine, but the intent is that only the last bit can be trusted, assuming you have the reverse proxy server (nginx, Apache etc.) that adds the client IP directly close to your network.

You're right that in the multi-proxy network, the leftmost is the source client IP. But that may or may not be what you want depending on the configuration, and unless you configure the whole stack, including CDN and reverse proxy, in a way you override the XFF headers, picking the first one is vulnerable since it can be spoofed.

Having said that, this middleware has a lot of assumption like this, but also has a very generic name that could confuse users thinking this would do the right thing in all situations.

I think the options for this problem would be one or a combination of:

• Accept this patch, which will potentially make existing application vulnerable
• Add some options to denote that all XFF header values can be trusted
• Do not merge but add a documentation about the reverse proxy settings

I am reluctant to changing the default at this point, and adding an option would only complicate this middleware which can just be written as an inline sub. Putting a doc with a suggestion to use https://metacpan.org/pod/Plack::Middleware::XForwardedFor might be good.

[melo]

In my case, I had a single-level proxy, inside a trusted private network, and the IP I was expecting was the client address, and this worked fine.

Recently we added a CDN in front, and updated our nginx proxy to trust the x-forwarded-for from the CDN using ACL's, but with the current logic I stopped receiving the client address.

What I got was the CDN IP address.

I understand your reasons for not changing the default at this point, but it all depends on what expectations the users of this module have. IMVHO, If most of them use a single proxy setup, then they are getting the real client IP, and expect that to be the correct value on all situations.

In summary, and using as reference app <-- proxy1 <--- proxy2 <--- proxyN <--- client setup:

• with a single proxy, the module sets REMOTE_ADDR to the original client IP address;
• with double proxy layer, the module sets REMOTE_ADDR to the second proxy, proxy2;
• with more than 2 proxy layers, you still get REMOTE_ADDR of the second proxy, proxy2.

I don't see the value of this result, but I understand your reluctance to change this at this time. I understand that if users don't have the care needed to validare X-Forwarded-IP paths, they can get a spoofed IP, and the current logic gives the REMOTE_ADDR least likely or most difficult to spoof. That's the only redeeming quality I can find :).

If you agree, I'll close the PR and send another one with just the documentation update you suggest on your third bullet above, to at least warn users of the issues with multiple proxy layers.