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

Improve ProxyHeadersMiddleware #2231

Closed
wants to merge 30 commits into from
Closed

Conversation

nhairs
Copy link
Contributor

@nhairs nhairs commented Jan 25, 2024

Foreword

This PR is a continuation of #1611 building upon the existing work of @pypae

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 Networks as trusted hosts. (Mentioned by @jalaziz) This greatly simplifies deployments on docker swarm / kubernetes, where the reverse proxy might have a dynamic IP.
    • This includes support for IPv6 Address / Networks

Checklist

  • I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
  • I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
    • I believe that the _TrustedHost test cases are adequate for testing that IPv6 is supported without also passing values through the ProxyHeaderMiddleware test cases as well.
  • I've updated the documentation accordingly.

Test plan

Advanced testing using the following NGINX config which can be tweaked to test different combinations of proxies.

server {
    server_name proxy1;

    listen 127.0.0.1:80;
    listen [::1]:80;
    listen unix:/tmp/proxy1.sock;

    location / {
        proxy_set_header Host $http_host;
        proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
        proxy_set_header X-Forwarded-Proto $scheme;
        proxy_set_header Upgrade $http_upgrade;
        proxy_set_header Connection $connection_upgrade;
        proxy_redirect off;
        proxy_buffering off;
        proxy_pass http://uvicorn;
    }
}


server {
    server_name proxy2;

    listen 127.0.100.1:80;
    listen [::1]:8080;

    # https://blog.davidsierra.dev/posts/connect-nginxs-through-sockets/
    listen unix:/tmp/proxy2.sock;

    location / {
        proxy_set_header Host $http_host;
        proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
        proxy_set_header X-Forwarded-Proto $scheme;
        proxy_set_header Upgrade $http_upgrade;
        proxy_set_header Connection $connection_upgrade;
        proxy_redirect off;
        proxy_buffering off;
        proxy_pass http://unix:/tmp/proxy1.sock;
    }
}

map $http_upgrade $connection_upgrade {
    default upgrade;
    '' close;
}

upstream uvicorn {
    server 127.0.0.1:9000;
    server unix:/tmp/uvicorn.sock;
}

@nhairs nhairs changed the title Proxy headers Improve ProxyHeadersMiddleware Jan 25, 2024
@nhairs
Copy link
Contributor Author

nhairs commented Jan 25, 2024

@Kludex - this is mostly done

A few questions though:

q1: I've noticed that in the README we state the project is 3.8+ though the pyproject.yaml is for 3.7+

Should we do anything about this?

q2: I'm getting a bunch of lint errors from elsewhere in the package, should I fix these up in the same PR?

Turns out I should have run ./scripts/install first 🙃

image

q3: I don't know enough about Uvicorn to understand what I should be doing here for this test case (in such a way that it fixes the lint errors)

Nevermind fixed it myself.

image

docs/deployment.md Outdated Show resolved Hide resolved
@nhairs nhairs marked this pull request as ready for review January 28, 2024 10:07
@nhairs
Copy link
Contributor Author

nhairs commented Jan 28, 2024

I still need to add some more test cases

I believe this is ready for review @Kludex

@Kludex
Copy link
Member

Kludex commented Feb 10, 2024

q1: I've noticed that in the README we state the project is 3.8+ though the pyproject.yaml is for 3.7+

I don't see that... 🤔

@Kludex
Copy link
Member

Kludex commented Feb 10, 2024

@nhairs You are adding much more stuff on top of the previous PR. Can we do this step by step?

The way I propose:

  1. Apply the comments from the previous PR (even your comments if you think they make sense), and open a PR with only that.
  2. Create new PRs with the other changes from this PR.

@nhairs
Copy link
Contributor Author

nhairs commented Feb 10, 2024

q1: I've noticed that in the README we state the project is 3.8+ though the pyproject.yaml is for 3.7+

I don't see that... 🤔

Ah never-mind, I suspect it's because of the original PR being so old and referencing the older code against the current website 🤦.

@nhairs
Copy link
Contributor Author

nhairs commented Feb 10, 2024

@nhairs You are adding much more stuff on top of the previous PR. Can we do this step by step?

...

Are you able to be more specific about what parts you're concerned about?

If we exclude update related tests / docs, I think we are left with the following:

A: supporting both IP version 4 and 6 (which is in the comments I left on the original PR).

I don't think it makes sense to separate generic support versus v4 only support.

The direct support of IP addresses instead of just networks is simply an optimisation / guard rail for users - strictly speaking you could remove the specialised handling code and the user experience would be the same. I chose to keep it because "justifying the PR" could be complicated when I could just keep it apart of the large changes occurring.

B: Better Unix socket handling.

Whilst I could pull this into a separate PR, I'd prefer to fix this at the same time. The current recommended solution of trusting everything could very easily introduce vulnerabilities to user's applications. I'd rather not tell users to aim a gun at their foot.

C: There's some other miscellaneous fixes referenced in the original PR that could be fixed at the same time.

Rather than introducing bugs that we'll either immediately replace with the refactor this PR does or immediately submit a PR for doesn't make sense to me.

If you have a different view of "what the chunks are" I'm happy to discuss 🙂


Related to your question in #2237 perhaps we are looking some of this the wrong way - specifically trying to auto-magically support all proxy-header use cases from the command line. Perhaps it would be better to deprecate the command-line support (that is probably best described as a stop-gap measure) and instead provide a more fleshed out set of proxy middle-ware that user's can configure themselves before wrapping their application in it.

If this were the case it's arguable that such "middleware" might be better abstracted into it's own package so that any ASGI compatible project can re-use the logic without needing to implement it itself (if there's one thing I've learnt looking into these headers its that properly handling them is non-trivial). Or even generic enough that it can provide ASGI, WSGI, or raw handlers.

@nhairs
Copy link
Contributor Author

nhairs commented Feb 10, 2024

Please remove the new introduced cli args and params to the middleware from this PR.

This is done

Copy link
Member

@Kludex Kludex left a comment

Choose a reason for hiding this comment

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

@nhairs Would you mind rebasing it, and checking if the proxy_headers.py file is fully covered by tests? I'll continue my review as soon as that is done.

I want to clean up the PRs we have here.

uvicorn/config.py Outdated Show resolved Hide resolved
uvicorn/middleware/proxy_headers.py Outdated Show resolved Hide resolved
docs/deployment.md Show resolved Hide resolved
uvicorn/middleware/proxy_headers.py Outdated Show resolved Hide resolved
uvicorn/middleware/proxy_headers.py Outdated Show resolved Hide resolved
@Kludex Kludex added the waiting author Waiting for author's reply label Mar 2, 2024
@nhairs
Copy link
Contributor Author

nhairs commented Mar 5, 2024

@Kludex - master has been merged in

Copy link
Member

@Kludex Kludex left a comment

Choose a reason for hiding this comment

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

I'll finish the review in a few days. I'm a bit AFK those days.

Checklist for myself

  • Check if tests makes sense, and think about the make_* utility functions (are they noisy or good?)

else:
scope["scheme"] = x_forwarded_proto

if x_forwarded_proto in {"http", "https", "ws", "wss"}:
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this conditional?

Copy link
Contributor Author

@nhairs nhairs Mar 5, 2024

Choose a reason for hiding this comment

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

To prevent garbage being passed through.

E.g. (from the tests) if you pass through X-forwarded-proto: asdf it will be ignored.

Copy link
Member

Choose a reason for hiding this comment

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

I mean, we know that the x-forwarded-proto will assume those values anyway. Is this check saving us from something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is nothing to guarantee that it will not receive garbage.

tests/middleware/test_proxy_headers.py Outdated Show resolved Hide resolved
nhairs and others added 3 commits March 5, 2024 23:37
Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>
tests/middleware/test_proxy_headers.py Outdated Show resolved Hide resolved
@Kludex Kludex removed the waiting author Waiting for author's reply label Apr 14, 2024
@nhairs
Copy link
Contributor Author

nhairs commented Apr 28, 2024

@Kludex - just making sure you got a notification that I updated this for requested changes.

@Kludex Kludex self-requested a review June 14, 2024 07:13
@easybe
Copy link

easybe commented Aug 13, 2024

What is the status here? I am interested in this (or more precisely #2237) and am willing to help if needed.

@nhairs
Copy link
Contributor Author

nhairs commented Aug 24, 2024

What is the status here? I am interested in this (or more precisely #2237) and am willing to help if needed.

AFAIAA I'm waiting on @Kludex to finish his review.

Comment on lines +40 to +42
# Note: because we always convert invalid IP types to literals it
# is not possible for the user to know they provided a malformed IP
# type - this may lead to unexpected / difficult to debug behaviour.
Copy link
Member

Choose a reason for hiding this comment

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

Right. Any suggestions?

@Kludex
Copy link
Member

Kludex commented Sep 27, 2024

I'm not able to push to this branch, I'm not sure why.

I have mainly nitpicks that I'd like to push.

@Kludex
Copy link
Member

Kludex commented Sep 27, 2024

@nhairs Beautiful work.

I think this is the best PR I've ever seen:

  • really descriptive description;
  • clean implementation;
  • meaningful in-code comments;
  • update docs;
  • and great test coverage.

I'm sorry for the delay on the review here.

I hope the delay on the review doesn't demotivate you to open more PRs here. 🙏


I have some docs fixes, typing improvements, and nitpicks that I'll address in #2468, since I can't push changes to this branch.

@Kludex Kludex closed this in #2468 Sep 27, 2024
@b4stien b4stien mentioned this pull request Oct 9, 2024
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants