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.

Sign up for GitHub

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

Jump to bottom

Improve ProxyHeadersMiddleware #1611

Closed
wants to merge 8 commits into from
Closed

Improve ProxyHeadersMiddleware #1611

wants to merge 8 commits into from

Conversation

pypae
Copy link
Contributor

@pypae pypae commented Aug 22, 2022 •
edited
Loading

Summary
This PR addresses multiple issues mentioned in #1068 to improve the ProxyHeadersMiddleware.

  • 🐛 Fix the host for requests from clients running on the proxy server itself. (The main issue)
  • 🐛 Fallback to host that was already set for empty x-forwarded-for headers. (Mentioned by @b0g3r)
  • ✨ Also allow to specify IP ranges in CIDR notation as trusted hosts. (Mentioned by @jalaziz) This greatly simplifies deployments on docker swarm / kubernetes, where the reverse proxy might have a dynamic IP.

@pypae
Copy link
Contributor Author

pypae commented Aug 22, 2022

@b0g3r I don't fully grasp your third issue:

  • It doesn't check request_addr like it is one of the proxies. It means that hacker who has direct network access to the server can impersonate IPs (example)

Isn't this covered by the following line?

uvicorn/uvicorn/middleware/proxy_headers.py

Line 81 in 6947d19

if client_host in self.trusted_hosts:

@pypae
Copy link
Contributor Author

pypae commented Sep 8, 2022

Is this a feature that you would like to see merged? Can we do anything on our side?

For the time being, we're using this middleware outside of uvicorn.
For anyone coming across the same issue: Just copy the code, manually add it as a middleware and disable proxy headers in uvicorn. (For example by setting the environment variable UVICORN_PROXY_HEADERS=False)

@purplesyringa
Copy link

purplesyringa commented Oct 25, 2022 •
edited
Loading

As a side note -- would it be reasonable to add support for X-Forwarded-Host and X-Forwarded-Port, like Werkzeug does? (There's also X-Forwarded-Prefix, but I'm not familiar enough with Quart to know if that would be the right place to handle it)

UPD: Actually, that would be #965. Also, docs mention X-Forwarded-Port but it doesn't seem to be handled anywhere in the code, as far as I can see.

@Kludex Kludex added this to the Version 0.20.0 milestone Oct 30, 2022
@Kludex Kludex mentioned this pull request Oct 30, 2022
Closed
@Kludex Kludex modified the milestones: Version 0.20.0, Version 0.21.0 Nov 10, 2022
@Kludex Kludex mentioned this pull request Dec 16, 2022
Closed
16 tasks
Kludex
Kludex reviewed Mar 10, 2023
View reviewed changes
uvicorn/middleware/proxy_headers.py
Comment on lines +44 to +45
except ValueError:
self.trusted_literals.add(host)
Copy link
Member

Choose a reason for hiding this comment

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

Can we have a test case for this?

uvicorn/middleware/proxy_headers.py
# See https://github.com/encode/uvicorn/issues/1068#issuecomment-855371576
if host in self:
return x_forwarded_for_hosts[0]
return host
Copy link
Member

Choose a reason for hiding this comment

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

Can this be reached? What's the scenario?

@Kludex
Copy link
Member

Kludex commented Mar 10, 2023 •
edited
Loading

I think we may need to add more strategic comments on this PR, since it's not trivial to follow. 😅

Ref.: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-Forwarded-For

@Kludex Kludex modified the milestones: Version 0.21.0, Version 0.22.0 Mar 10, 2023
@Kludex Kludex added enhancement waiting author Waiting for author's reply labels Mar 10, 2023
@Kludex
Copy link
Member

Kludex commented Mar 20, 2023

@pypae Are you still interested in this PR?

@pypae
Copy link
Contributor Author

pypae commented Mar 20, 2023

Are you still interested in this PR?

Yes, I still think this feature could be of use for many users. I'll have another look at it later this week and add some strategic comments.

@Kludex
Copy link
Member

Kludex commented Apr 15, 2023

👋 🏃 💨

@Kludex
Copy link
Member

Kludex commented Apr 22, 2023

@pypae Are you still interested in this PR?

@amadeuszhercog-ocado
Copy link

Hello @Kludex @pypae I hope you're doing well. I'm also interested in this functionality. Currently in our code we have to do this manually, and it would be very nice to have the out-of-the-box mechanism in uvicorn.

@pypae are you planning to further work on this functionality?

If not, are you @Kludex planning to take over and finish this pull request? Or are you going to close it without merging?

@Kludex
Copy link
Member

Kludex commented May 17, 2023

I'm waiting for the author here. If someone wants to take over, feel free to do it. I'm not closing this PR anyway.

I need my review comments to be replied.

@gonchik
Copy link

gonchik commented Dec 14, 2023

It will be nice to have it. :)

nhairs
nhairs reviewed Jan 24, 2024
View reviewed changes
Copy link
Contributor

@nhairs nhairs left a comment

Choose a reason for hiding this comment

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

Since it's been 10 months since the last activity from @pypae I'd be willing to pick this up and try finish it off.

Apart from the outstanding comments in the code is there anything I should be aware of @Kludex? (This would also be my first time contributing to uvicorn if you have info on that).

uvicorn/middleware/proxy_headers.py
# Try parsing the trusted host as an IPv4Network
# to allow checking a whole range.
# https://github.com/encode/uvicorn/issues/1068
self.trusted_networks.add(ipaddress.IPv4Network(host))
Copy link
Contributor

@nhairs nhairs Jan 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Is there a reason for using IPv4Network instead of ipaddress.ip_network which is version agnostic?

uvicorn/middleware/proxy_headers.py
return True

try:
ip = ipaddress.IPv4Address(item)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason we're using IPv4Address instead of ipaddress.ip_address which is version agnostic?

@Kludex
Copy link
Member

Kludex commented Jan 24, 2024

Nothing to add. Go ahead. 🙏

@nhairs nhairs mentioned this pull request Jan 25, 2024
Closed
3 tasks
@nhairs
Copy link
Contributor

nhairs commented Jan 25, 2024

For those subscribed - I've started a new PR #2231

@Kludex Kludex removed this from the Version 0.22.0 milestone Feb 10, 2024
@Kludex
Copy link
Member

Kludex commented Feb 29, 2024 •
edited
Loading

@Kludex Kludex closed this Feb 29, 2024
@gonchik
Copy link

gonchik commented Mar 1, 2024

Thanks @Kludex

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nhairs nhairs nhairs left review comments

@Kludex Kludex Kludex left review comments

Assignees
No one assigned
Labels
enhancement waiting author Waiting for author's reply
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@pypae @purplesyringa @Kludex @amadeuszhercog-ocado @gonchik @nhairs