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

Only trust direct proxy in the trusted downstream #2579

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Only trust direct proxy in the trusted downstream #2579

wants to merge 1 commit into from

Conversation

lisongmin
Copy link

Only trust direct proxy in the trusted downstream to avoid get xheader info from an untrusted proxy.

User must add direct proxy ip address to trusted downstream.

user1 ----------> real-proxy ----------> server
untrusted-proxy --x--^                     ^
                `-------------x------------|

This may be a break change, but more security.

@lisongmin
Copy link
Author

It seams unittest failure is not relative to this MR.

@ploxiln
Copy link
Contributor

ploxiln commented Feb 1, 2019

looks correct to me
(extra blank lines may not match the surrounding code style though)
(only test failure was pypy timeout, may pass if you amend and force-push)

get xheader info from an untrusted proxy.

User must add direct proxy ip address to trusted downstream.

```
user1 ----------> real-proxy ----------> server
untrusted-proxy --x--^                     ^
                `-------------x------------|
```
@lisongmin
Copy link
Author

code style is fixed now.

@bdarnell
Copy link
Member

bdarnell commented Feb 2, 2019

This breaks backwards compatibility in a couple of ways.

First, it's valid to specify xheaders=True without specifying any trusted downstream addresses. This is partially for historical reasons and partially because if you're using a proxy you're often running behind a firewall where no untrusted connections are possible.

Second, I'm not sure it's compatible with the existing uses of trusted_downstream. This option is used when there are multiple proxies in the chain:

user  ->  proxy 1         -> proxy 2       -> server
          adds user ip       adds proxy1      sees proxy2 as remote_ip
          to XFF             to XFF           XFF contains proxy1 and user ip

In this case trusted_downstream would contain proxy 1 but not necessarily proxy 2.

What we really need is a more sophisticated way of configuring the xheaders system: which remote_ips can send xheaders, which xheaders are in use (the current behavior of trying both X-Real-IP and X-Forwarded-For is probably exploitable in some environments), which addresses should be filtered out of XFF, etc. (Maybe this would also be the way of configuring the haproxy protocol for #2492). The trusted_downstream option was a step in this direction but it was incompletely thought-out.

@ploxiln
Copy link
Contributor

ploxiln commented Feb 2, 2019

Ah, right, users who set xheaders but not trusted_downstream expect it to use the last/closest address from the headers.

(And looks like I forgot to have my nginx configs also remove X-Real-Ip headers ...)

@lisongmin
Copy link
Author

Yes, I agree with you. this patch attempt resolve ** which remote ips can send xheader** with a non proper way. Feel free to close this MR.

By the way, are there any plan to design a new xheader system?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants