Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Compute a real X-Forwarded-For header #1489

Merged
merged 2 commits into from
Oct 26, 2017

Conversation

maxlaverse
Copy link
Contributor

@maxlaverse maxlaverse commented Oct 6, 2017

Hi !
It's great that the real-ip module has been built in the Ingress. It simplifies the work of a lot of applications by extracting the real client ip automatically for them, avoid the hassle of implementing this is a lot of different languages.

However, the X-Forwarded-For header is supposed to contain all the L7 LB or Reverse-proxies that have been crossed, from the client to the application. Of course this information could still be computed by appending remote_addr to the X-Original-Forwarded-For header on the server side, but it sounds a bit weird.

I would expect the Ingress to behave as any classic reverse-proxy installation, but maybe my "classic" picture is wrong.

On the other hand, if you are only interested in having the real client IP and you extract it from the x-forwarded-for header, since you already trust the Ingress it should not be an issue to loose the addresses of other L7 reverse-proxy that have been traversed.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 6, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Oct 6, 2017
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 1b5af450fe3be08a1d5e3e2d6f1113f3b4536259 on maxlaverse:fix_x_forwarded_for into ** on kubernetes:master**.

@aledbf
Copy link
Member

aledbf commented Oct 6, 2017

@maxlaverse the current code returns the correct IP address when the ingress controller is running behind a L7 load balancer.

@aledbf
Copy link
Member

aledbf commented Oct 6, 2017

Please use the image in #1487

@maxlaverse
Copy link
Contributor Author

maxlaverse commented Oct 6, 2017

Hi @aledbf ! I've not used this image but I built one based on your commit 48a58bb, which was the most recent commit a few hours ago.

The PR is not about the content of X-Real-IP which I know has been fixed with one of your recent commit. This is only about the content of the X-Forwarded-For header.

In its current form, the Ingress currently erases that header and puts the computed real-ip in it. Even if it should work with most of the implementation that parse this header to get the true client ip out of it, it erases some information. You expect from a L7 load-balancer to append the tcp client IP to the x-forwarded-for header, when forwarding the request to an upstream. Not to erase it.

With the current implementation of the Ingress, I can't know anymore through which load-balancers a requests went.

But I as I said, in most cases you don't care actually, through which L7 component your request went and the current behavior is just fine. This is just a proposition PR to make the Ingress behave as any other L7 load-balancer/reverse-proxy.

@aledbf
Copy link
Member

aledbf commented Oct 6, 2017

@maxlaverse the issue is that you can fake the header. That is the reason why I added a copy of the original header. I'm not sure I can provide a one size fits all for this. Maybe we can add a new configuration in the configmap?

@maxlaverse
Copy link
Contributor Author

The last commit makes this behavior configurable.
The header can be faked yes, but that's why we have an equivalent of the proxy-real-ip-cidr in the implementations that extract the client IP server-side (instead of proxy-side).

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 33.505% when pulling 65ba0c1 on maxlaverse:fix_x_forwarded_for into 1f269d4 on kubernetes:master.

# replaces the remote_addr to soon
map $http_x_forwarded_for $full_x_forwarded_for {
default "$http_x_forwarded_for, $realip_remote_addr";
'' "$realip_remote_addr";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

realip_remote_addr does not contains the client IP address. Please use $the_real_ip

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's on purpose. Here I want to append the ip of the actual tcp client connected to the Ingress.

The goal of this PR is to have the X-Forwarded-For header arriving on upstreams has if it had been through any classic L7 load-balancer, and the last part of it should be the last proxy the request went through.

Maybe my explanations are not clear enough, let me add an example.

Client (1.1.1.1) => L7 Load-balancer A (2.2.2.2) => L7 Ingress B (3.3.3.3) => Pods

The Load-balancer A appends its IP to the X-Forwarded-For header of the request, or create the header if it doesn't exist (which should be the case).

If 3.3.3.3 was another L7 load-balancer/reverse proxy, it would also append 2.2.2.2 to the value of X-Forwarded-For and the client would receive X-Forwarded-For: 1.1.1.1, 2.2.2.2.

But with the current Ingress, the Pod receives the request with the header X-Forwarded-For: 1.1.1.1 if proxy-real-ip-cidr: 2.2.2.2. We loose the information that the request has been through 2.2.2.2, expect if we look in X-Original-X-Forwarded-For but this is not standard.

With the PR, no matter the value of proxy-real-ip-cidr, the Pod would receive the request with the header X-Forwarded-For: 1.1.1.1, 2.2.2.2. The implementation of the pod could check the header, compare 2.2.2.2 to a list of trusted proxy and because it matches, set the real-ip to 1.1.1.1, or rely of X-Real-IP directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you know, I would vote for having this the default behavior and X-Original-Forwarded-For removed :)
But I understand it could break some older Ingress Nginx setup :/

@aledbf
Copy link
Member

aledbf commented Oct 10, 2017

@abh ping. This is the expected behavior you mentioned with X-Forwarded-For?

@abh
Copy link

abh commented Oct 10, 2017

Yeah, it looks right.

nginx should just be adding the "upstream" IP address to the list of X-Forwarded-For IPs. If using the proxy protocol (and proxy-real-ip-cidr matches?), add the IP from that rather than the "tcp IP".

Downstream clients have to implement their own whitelist and decide how many "hops" in the list to trust. That's only possible if it can trust each hop to add to the list as described above.

For the X-Real-IP header, maybe that'd be the right place to just put the last upstream IP (or proxy IP).

@maxlaverse
Copy link
Contributor Author

maxlaverse commented Oct 10, 2017

nginx should just be adding the "upstream" IP address to the list of X-Forwarded-For IPs.

Agreed

If using the proxy protocol (and proxy-real-ip-cidr matches?), add the IP from that rather than the "tcp IP".

I never used the proxy protocol but I thought even with proxy protocol activated, you would still want the tcp ip in x-forwarded-for. The only difference for me with the proxy protocol it that the client IP is immediately available to Nginx without having to compute it from the X-Forwarded-For header using proxy-real-ip-cidr, because the original client IP is sent at the beginning of the proxy request. Did I get that wrong ?

For the X-Real-IP header, maybe that'd be the right place to just put the last upstream IP (or proxy IP).

X-Real-IP is the client IP extracted from the X-Forwarded-For using the proxy-real-ip-cidr.
I think it's okay to have the real client ip in X-Real-IP, as it is what you expect from the http_realip module.

@abh
Copy link

abh commented Oct 10, 2017

The thing with the proxy protocol is that it still might not be the real client IP, it could just be "one more proxy hop". The proxy that's using PROXY protocol likely is doing so because it's not able to add HTTP headers, for example if it's a TCP proxy passing through TLS.

client (1.1.1.1) -> proxy1 (2.2.2.2, adds 1.1.1.1 to XFF) -> tcp proxy2 (10.0.0.10) --[proxy protocol says 2.2.2.2]--> kube ingress (10.2.0.1) --> pod

The pod gets a connection from 10.2.0.1 (which it trusts because it's internal in kube or maybe even it's looked up that this is indeed an IP from the nginx ingress). In X-Forwarded-For you want "1.1.1.1, 2.2.2.2".

The pod can now decide if it just wants to use 2.2.2.2 as the client IP, or if it trusts 2.2.2.2 and then uses 1.1.1.1.

Alternatively (as you maybe suggested) the last hop could add both the IP from PROXY and the actual ip making it "1.1.1.1, 2.2.2.2, 10.0.0.10".

@maxlaverse
Copy link
Contributor Author

maxlaverse commented Oct 10, 2017

Sorry if I repeat what you already said, the proxy protocol is new to me.
So a proxy server forwarding http requests using the proxy protocol acts as a tcp proxy. It doesn't modify the http request and since the remote addr is sent at the beginning of the forwarded request, it's completely transparent to the server it forwards the request to.

If proxy2 talks to the nginx ingress using the proxy protocol, I would expect "1.1.1.1, 2.2.2.2" in the xff header arriving on the pod. "1.1.1.1" because it's the value of the xff header nginx gets. 2.2.2.2 because it's the remote_addr according to the proxy protocol as received by nginx (and not the "tcp ip")

I wouldn't add 10.0.0.10 to the xff in that case. Expecting "1.1.1.1, 2.2.2.2" sounds right. As you said, it's to the pod to decide if it trusts 2.2.2.2.

If using the proxy protocol (and proxy-real-ip-cidr matches?), add the IP from that rather than the "tcp addr".

Now I get that. If we receive a request on the Nginx with the proxy protocol, we should append the client address as sent in the proxy request (if we trust the proxy), not the tcp addr.
I believe this address is in the "$proxy_protocol_addr" variable.
Edit: In that case, xff should be set to $proxy_add_x_forwarded_for and not $full_x_forwarded_for. Do you agree ?

@maxlaverse
Copy link
Contributor Author

maxlaverse commented Oct 10, 2017

On another topic, does the X-Real-IP has correct values with the setup you mentioned in your example ? (mixed http proxies and proxy proxies)

If you activate the proxy_protocol on the Ingress, if will add the directive real_ip_header proxy_protocol; which means that internally the remote_addr of the request received by Nginx will be replaced with the tcp addr of the client that connected to the proxy in front on the ingress.

client (1.1.1.1) -> proxy1 (2.2.2.2, adds 1.1.1.1 to XFF) -> tcp proxy2 (10.0.0.10) --[proxy protocol says 2.2.2.2]--> kube ingress (10.2.0.1) --> pod

Wouldn't the remote_addr on Nginx as modified by the real_ip module become 2.2.2.2 instead of 1.1.1.1, if 2.2.2.2 is a trusted proxy ?
With such a setup, the real ip module should look at the proxy_protocol addr first, check if it's a trusted proxy and then look at the XFF header to get the client IP and again, check the proxy ip are trusted. But it doesn't, right ?

@@ -195,6 +195,15 @@ http {
'' $host;
}

{{ if $cfg.ComputeFullForwardedFor }}
# We can't use $proxy_add_x_forwarded_for because the realip module
# replaces the remote_addr to soon
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

too soon

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 33.505% when pulling 55c2097a9492a2ac22f363f34ccd34502685573e on maxlaverse:fix_x_forwarded_for into 1f269d4 on kubernetes:master.

@maxlaverse
Copy link
Contributor Author

maxlaverse commented Oct 16, 2017

Would you have time to look at my last comments this week @abh ? (or anyone else)

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 26, 2017
@maxlaverse
Copy link
Contributor Author

To backup this proposition: https://tools.ietf.org/html/rfc7239, section 5.2

@aledbf
Copy link
Member

aledbf commented Oct 26, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 26, 2017
@aledbf aledbf merged commit 52ee9d3 into kubernetes:master Oct 26, 2017
@abh
Copy link

abh commented Oct 27, 2017

@maxlaverse: nice work, looks good to me. Thank you.

@maxlaverse
Copy link
Contributor Author

maxlaverse commented Oct 27, 2017

Thanks for merging.

@abh @aledbf, I still have this open question when compute-full-forwarded-for and the proxy protocol are activated. In that case, should the X-Forwarded-For bet set to $proxy_add_x_forwarded_for instead of $full_x_forwarded_for ? Or is it alright as it is ?

Don't hesitate to ping me if this needs to be changed. I would push a new PR.

@abh
Copy link

abh commented Oct 27, 2017 via email

@maxlaverse
Copy link
Contributor Author

Thanks for your time @abh and @aledbf
I opened a follow up PR to fix the header value with the PROXY protocol: #1618

@maxlaverse maxlaverse deleted the fix_x_forwarded_for branch October 28, 2017 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants