Improve ProxyHeadersMiddleware

[nhairs]

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]

@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 🙃

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.

[nhairs]

I still need to add some more test cases

I believe this is ready for review @Kludex

[Kludex]

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]

@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]

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]

@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]

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

This is done

[Kludex]

@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.

[nhairs]

@Kludex - master has been merged in

[Kludex]

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?)

[Kludex]

Why do we need this conditional?

[nhairs]

To prevent garbage being passed through.

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

[Kludex]

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

[nhairs]

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

[nhairs]

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

[easybe]

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

[nhairs]

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.

[Kludex]

Right. Any suggestions?

[Kludex]

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]

@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.