X-Forwarded-For wrong with enable-real-ip

[sathieu]

What happened:

When setting :

proxy-real-ip-cidr: 10.20.30.40
enable-real-ip: true

and testing from 10.20.30.40 with:

curl -kv https://hello-world.example.org--header "X-Forwarded-For: 192.168.1.1"

The following variables will be defined:

remote_addr: 192.168.1.1
# X-Forwarded-For header sent upstream
proxy_add_x_forwarded_for: 192.168.1.1 192.168.1.1

What you expected to happen:

The following variables to be defined:

remote_addr: 192.168.1.1
proxy_add_x_forwarded_for: 192.168.1.1 10.20.30.40

Other tests

We tried the following:

proxy-real-ip-cidr: 10.20.30.40	enable-real-ip: true	use-forwarded-headers: true	compute-full-forwarded-for: true	$remote_addr	$proxy_add_x_forwarded_for
✅				❌ 10.20.30.40	✅192.168.1.1, 10.20.30.40
✅	✅			❌ 10.20.30.40	❌192.168.1.1, 192.168.1.1
✅		✅		✅ 192.168.1.1	❌192.168.1.1, 192.168.1.1
✅	✅	✅		✅ 192.168.1.1	❌192.168.1.1, 192.168.1.1
✅		✅	✅	✅ 192.168.1.1	❌192.168.1.1, 192.168.1.1
✅	✅	✅	✅	✅ 192.168.1.1	❌192.168.1.1, 192.168.1.1
✅			✅	❌ 10.20.30.40	✅192.168.1.1, 10.20.30.40

NGINX Ingress controller version (exec into the pod and run nginx-ingress-controller --version.): v1.11.2

[k8s-ci-robot]

This issue is currently awaiting triage.

If Ingress contributors determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.

[longwuyuan]

The description in docs says this

If use-forwarded-headers or use-proxy-protocol is enabled, proxy-real-ip-cidr defines the default IP/network address of your external load balancer. Can be a comma-separated list of CIDR blocks. default: "0.0.0.0/0"

so I had assumed until now that its just the CIDR of the external-LB. And the reason for configuring this was to trust only and only that specific external-LB, for sending the valid X-Forwarded-* info to the controller and the backend .

[sathieu]

I am ok to trust only LB set in proxy-real-ip-cidr, the problem is that this changes the $proxy_add_x_forwarded_for to a strange value (with duplicated ip).

I don't see any use for this.

The resulting X-Forwarded-For header should be <client-ip> <lb-ip> instead.

[longwuyuan]

Thanks for comments.

Your comments suggest a authoritative info needs to become available here as a comment. There are not many resources available here on github. There are at least some more than here on the Kubernetes slack.

I myself am seeing my limited knowledge on this. Because when you say

and testing from 10.20.30.40 with:

I assume you had a shell on the host, whose default route or own ipaddress was 10.20.30.40 . Or I could be confused. I usually put metallb in a Kind cluster to simulate a LB with external-IP but now I am not even sure if I can use that for test here because metallb is not L7 LB.

[Gacko]

Just a dumb question, not sure if it makes a difference, but have you tried actually setting a CIDR and not just an IP address? So 10.20.30.40/32 instead of 10.20.30.40?

[Gacko]

Apart from that I remember that proxy_add_x_forwarded_for simply doesn't work the way one would it expect to work in combination with the Real IP module. I even had these issues with just a plain NGINX a while ago. So you might wanna test this with a plain NGINX and see it's nothing in the Ingress NGINX project.

[sathieu]

I've done tests with plain nginx.

:one: First,$proxy_add_x_forwarded_for is not the sent X-Forwarded-For header, but :

$proxy_add_x_forwarded_for

the “X-Forwarded-For” client request header field with the $remote_addr variable appended to it, separated by a comma. If the “X-Forwarded-For” field is not present in the client request header, the $proxy_add_x_forwarded_for variable is equal to the $remote_addr variable.

ref: https://nginx.org/en/docs/http/ngx_http_proxy_module.html#variables

So this has no use with realip module.

:two: Second: use-forwarded-headers: true and use-forwarded-headers: true are not safe to use, because they are not linked to set_real_ip_from. i.e. anybody can forge the X-Forwarded-For header set (or at least forge its prefix).

:three: Third. In the short term we'll use:

enable-real-ip: true
proxy-real-ip-cidr: 10.20.30.40

and add $realip_remote_addr in the logs (while keeping $remote_addr)

This is suboptimal, because the LB IP is not sent to backend.

:four: Fourth. It would be great to send a safe X-Forwarded-For header to backend including the LB IP.

Here is how I've done it with plain nginx:

server {
    set_real_ip_from 10.20.30.40;
    real_ip_header X-Forwarded-For;
    real_ip_recursive on;

    if ($remote_addr = $realip_remote_addr) {
        set $safe_x_forwarded_for "$remote_addr";
    }
    if ($remote_addr != $realip_remote_addr) {
        set $safe_x_forwarded_for "$http_x_forwarded_for, $realip_remote_addr";
    }
    proxy_set_header X-Real-IP $remote_addr;
    proxy_set_header X-Forwarded-For $safe_x_forwarded_for;

   # ...
  location / {
    proxy_pass https://10.11.12.13:8080
  }
  # ...
}

and adding $safe_x_forwarded_for to the logs.

NB: I've tested with httpbin image to the path /headers?show_env=true.

[github-actions[bot]]

This is stale, but we won't close it automatically, just bare in mind the maintainers may be busy with other tasks and will reach your issue ASAP. If you have any question or request to prioritize this, please reach #ingress-nginx-dev on Kubernetes Slack.

[sathieu]

This is not stale.

I can move this forward and add a new parameter, but this will be harder to understand how all parameters go together. How to move forward?