Bug 1385421 - HTTP_X_FORWARDED_FOR incorrect in V3
Summary: HTTP_X_FORWARDED_FOR incorrect in V3
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: OpenShift Online
Classification: Red Hat
Component: Routing
Version: 3.x
Hardware: Unspecified
OS: Unspecified
high
high
Target Milestone: ---
: ---
Assignee: Miciah Dashiel Butler Masters
QA Contact: zhaozhanqi
URL:
Whiteboard:
Depends On:
Blocks: 1410156 1410157
TreeView+ depends on / blocked
 
Reported: 2016-10-17 01:29 UTC by Drew Anderson
Modified: 2021-09-12 05:19 UTC (History)
15 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
: 1410156 1410157 (view as bug list)
Environment:
Last Closed: 2017-05-01 20:39:19 UTC
Target Upstream Version:
Embargoed:


Attachments (Terms of Use)


Links
System ID Private Priority Status Summary Last Updated
Origin (Github) 12271 0 None None None 2017-01-13 12:03:11 UTC

Description Drew Anderson 2016-10-17 01:29:04 UTC
Description of problem:
The HTTP_X_FORWARDED_FOR HTTP Request Header does not contain the end client IP address.

Version-Release number of selected component (if applicable):
OpenShift 3.x

How reproducible:
100%

Steps to Reproduce:
1. Deploy a web application which prints the HTTP_X_FORWARDED_FOR HTTP header to screen 
2. Visit deployed application
3. Observe HTTP_X_FORWARDED_FOR contains an IP which is not the end client IP.

Actual results:
'HTTP_X_FORWARDED_FOR' => string '172.31.51.173' (length=13) // AWS ELB IP

Expected results:
'HTTP_X_FORWARDED_FOR' => {your client ip}

Additional info:
Customer has raised this in SNOW as a regression from OpenShift V2: INC0456848

Comment 1 Ben Bennett 2016-10-17 12:51:53 UTC
The issue is that we have:
  option forwardfor
Set in haproxy (https://cbonte.github.io/haproxy-dconv/configuration-1.5.html#4-option%20forwardfor)

We may need to allow that to be under the control of the admin for cases like ELB (e.g. we may want to have the option if-none allowed to be set).

We also should look at this because the reencrypt backend seems not to set it.

However, this may be something that should be addressed in the Online router config for now.  Miciah, can you weigh in please?

Comment 2 Miciah Dashiel Butler Masters 2016-10-17 14:55:38 UTC
bbennett, are you saying ELB sets the correct X-Forwarded-For header, and we overwrite it? If so, I agree with what you are saying.  For completeness's sake,

• For openshift_default, we specify `option forwardfor`—seems pointless since this backend serves static content, right?

• For be_http_* and be_edge_http_*, we specify `option forwardfor` but should specify `option forwardfor if-none` in Online so as to leave the X-Forwarded-For header set by ELB.

• For be_tcp_*, we do not specify `option forwardfor` because the option is only applicable to HTTP.

• For be_secure_*, we do not specify `option forwardfor` at all but should specify `option forwardfor if-none`, same as for be_http_* and be_edge_http_*.

Does the foregoing all make sense?

Assuming you do not already have a name picked out for an option to toggle the `if-none` flag in the upstream template, I'll hardcode it in the Online template for now.

Looks like we are using this Bugzilla report to track the upstream issue, as opposed to Online, so I will refrain from hijacking it and shall instead clone for Online as appropriate.

Comment 3 Drew Anderson 2016-10-17 21:39:06 UTC
It is my understanding that specifically with AWS ELB, HTTPS terminates at the OpenShift Router and passes through the ELB untouched. 

Due to certificates being managed within the OpenShift Router, the HTTPS (TLS) connection cannot be understood and re-written by the ELB. It therefore must pass through untouched.

This means that the OpenShift Router considers the ELB to be the client for all HTTPS connections, setting the client IP to the ELB IP.

Comment 4 Ben Bennett 2016-10-20 15:48:00 UTC
Drew is spot on if this is doing TLS termination at the router... can you get me the route definition?  You can perhaps turn on proxy protocol support on ELB, but I have never tried it, and don't know if our haproxy version can support it:
  https://aws.amazon.com/blogs/aws/elastic-load-balancing-adds-support-for-proxy-protocol/

Otherwise:

> • For openshift_default, we specify `option forwardfor`—seems pointless since this backend serves static content, right?

Yes, it's pointless.

> • For be_http_* and be_edge_http_*, we specify `option forwardfor` but should specify `option forwardfor if-none` in Online so as to leave the X-Forwarded-For header set by ELB.

Correct.  Add if-none.

> • For be_tcp_*, we do not specify `option forwardfor` because the option is only applicable to HTTP.

Correct.  We can't add a header.

> • For be_secure_*, we do not specify `option forwardfor` at all but should specify `option forwardfor if-none`, same as for be_http_* and be_edge_http_*.

Correct, but with the caveat that Drew raised.



> Does the foregoing all make sense?

Yep!

> Assuming you do not already have a name picked out for an option to toggle the `if-none` flag in the upstream template, I'll hardcode it in the Online template for now.

I don't yet... but hard-coding it is perfectly reasonable for now.  The only reason we would not enable it all the time is that people can generate the header and fake a source IP if there is no upstream load-balancer setting it.


> Looks like we are using this Bugzilla report to track the upstream issue, as opposed to Online, so I will refrain from hijacking it and shall instead clone for Online as appropriate.

Thanks!

Comment 5 Miciah Dashiel Butler Masters 2016-10-21 15:42:23 UTC
Drew, if this is a regression from v2, we must be talking only about non-TLS routes, right? We have a similar configuration in v2, with ELB in front of Apache reverse proxies that terminate TLS.

Comment 7 Drew Anderson 2016-10-26 06:01:23 UTC
@Ben, to test this I simply created a new application and visited it. 

The code for which can be found on the test-branch here: https://github.com/drewandersonnz/cakephp-ex/tree/test-branch .. The code changes from the base example was to ensure it output the http headers to screen. 

There was no additional or special configuration to achieve this test.

Comment 8 Drew Anderson 2016-10-26 06:02:05 UTC
@Miciah, Kenjiro (knakayam) may have more information on the regression from V2, I am new to the team and don't know V2 very well.

Comment 12 Ben Bennett 2016-10-31 15:18:00 UTC
Re-reading the haproxy manual, the way that forwardfor works is that it appends another header.  So when the pod gets the traffic it will see two X-Forwarded-For headers (or more) and it can choose which to use.  Based on that, I think the router is behaving properly.  It may be a difference in how we have ELB set up.  Deferring to the Online team.

Comment 23 Miciah Dashiel Butler Masters 2016-12-09 19:27:32 UTC
QE, the ded-int-aws cluster has been configured to use the proxy-protocol so that encrypted connections will have the X-Forwarded-For header set correctly (i.e., it will show the address of the client connecting to ELB, rather than the address of the ELB itself).  Could you please verify the solution on the ded-int-aws cluster, and also check for regressions related to the router?

In case it helps, I used this test application to print the X-Forwarded-For header: oc new-app 'centos/ruby-22-centos7~https://github.com/Miciah/ruby-ex.git#print-X-Forwarded-For'

Comment 36 openshift-github-bot 2016-12-21 16:10:55 UTC
Commit pushed to master at https://github.com/openshift/origin

https://github.com/openshift/origin/commit/9db4fe74a509815df859fd58d9bae8609c3890c4
HAProxy Router: Add option to use PROXY protocol

Configure HAProxy to expect incoming connections to use the PROXY protocol
if the ROUTER_USE_PROXY_PROTOCOL environment variable is set to "true" or
"TRUE".

This commit fixes bug 1385421.

https://bugzilla.redhat.com/show_bug.cgi?id=1385421

Comment 38 Kenjiro Nakayama 2017-01-14 11:22:28 UTC
I think that this feature should be tested on GCP as well, since Red Hat's Dedicated support it. QE/QA will not be conducted on it? (although my customer is using AWS and we would like to rush for the release on AWS.)

Comment 39 Aleksandar Kostadinov 2017-01-31 16:18:09 UTC
I've tested this in 3.4.1.2 stg and I still see:
X_FORWARDED_FOR: 172.31.7.225

What I did is
1. create ruby-ex app with a modification to allow viewing headers [1]
2. expose service

Is patch applied to stg?

[1] https://github.com/openshift/ruby-ex/pull/8

Comment 40 Miciah Dashiel Butler Masters 2017-01-31 17:03:58 UTC
This Bugzilla report concerns Dedicated clusters.  PROXY protocol has not been enabled in Developer Preview.  I'll start the discussion with ops and PM about the latter.

Comment 41 Aleksandar Kostadinov 2017-01-31 17:06:45 UTC
If worth anything, my opinion is that we need this enabled in online so users can have proper logging and possibly security restrictions in their apps. Current behavior does not appear useful to me.


Note You need to log in before you can comment on or make changes to this bug.