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

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

Open
sathieu opened this issue Sep 19, 2024 · 6 comments
Open

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

sathieu opened this issue Sep 19, 2024 · 6 comments
Labels
kind/bug Categorizes issue or PR as related to a bug. needs-priority needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one.

Comments

@sathieu
Copy link
Contributor

sathieu commented Sep 19, 2024

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

@sathieu sathieu added the kind/bug Categorizes issue or PR as related to a bug. label Sep 19, 2024
@k8s-ci-robot k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Sep 19, 2024
@k8s-ci-robot
Copy link
Contributor

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. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@longwuyuan
Copy link
Contributor

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
Copy link
Contributor Author

sathieu commented Sep 20, 2024

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
Copy link
Contributor

longwuyuan commented Sep 20, 2024

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
Copy link
Member

Gacko commented Sep 22, 2024

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
Copy link
Member

Gacko commented Sep 22, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. needs-priority needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one.
Projects
Development

No branches or pull requests

5 participants
@sathieu @Gacko @longwuyuan @k8s-ci-robot and others