Compute a real X-Forwarded-For header

[maxlaverse]

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-reviewable]

This change is 

[coveralls]

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

[aledbf]

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

[aledbf]

Please use the image in #1487

[maxlaverse]

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]

@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]

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]

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

[aledbf]

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

[maxlaverse]

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.

[maxlaverse]

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]

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

[abh]

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]

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]

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]

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]

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 ?

[komapa]

too soon

[coveralls]

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

[maxlaverse]

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

[maxlaverse]

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

[aledbf]

/lgtm

[abh]

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

[maxlaverse]

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]

I think when proxy protocol is used it should use the X-Forwarded-For that’s in the request plus the IP from the proxy protocol. A variation would be adding the IP from the proxy protocol and the IP from the tcp connection (in that order), but I think using the proxy protocol implies that you trust the proxy and don’t care about its IP.

[maxlaverse]

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