Using ProxyHeadersMiddleware with client on trusted proxy results in invalid client information

[jchristgit]

Checklist

• The bug is reproducible against the latest release and/or master.
• There are no similar issues or pull requests to fix it yet.

Describe the bug

Using ProxyHeadersMiddleware results in the client being [None, 0] when the original client runs on a trusted proxy.

In our setup, we have a client service which runs on one of our proxy servers. The proxy servers run on HAProxy with option forwardfor to add the X-Forwarded-For header. With today's upgrade to uvicorn 0.14.0, the client fields are no longer present for requests coming from the mentioned server.

To reproduce

This addition to the tests should illustrate the problem:

diff --git a/tests/middleware/test_proxy_headers.py b/tests/middleware/test_proxy_headers.py
index aacf789..b453892 100644
--- a/tests/middleware/test_proxy_headers.py
+++ b/tests/middleware/test_proxy_headers.py
@@ -27,6 +27,8 @@ async def app(scope, receive, send):
         ("127.0.0.1, 10.0.0.1", "Remote: https://1.2.3.4:0"),
         # request from untrusted proxy
         ("192.168.0.1", "Remote: http://127.0.0.1:123"),
+        # request from client running on proxy server
+        (["127.0.0.1", "1.2.3.4"], "Remote: https://1.2.3.4:0"),
     ],
 )
 async def test_proxy_headers_trusted_hosts(trusted_hosts, response_text):

which results in:

>       assert response.text == response_text
E       AssertionError: assert 'Remote: https://None:0' == 'Remote: https://1.2.3.4:0'
E         - Remote: https://1.2.3.4:0
E         ?                 ^^^^^^^
E         + Remote: https://None:0
E         ?                 ^^^^

Expected behavior

The proper client address should be forwarded as before.

Actual behavior

The client is set to [None, 0].

Debugging material

I assume this is a regression of #591, more specifically, see the fallthrough discussion on https://github.com/encode/uvicorn/pull/591/files#r438008416. That said, I am not entirely sure what the best way to fix this would be - based on the discussion, the original code seems to be security-related. Would returning the last entry in x_forwarded_for be sufficient?

Environment

Running uvicorn 0.14.0 with CPython 3.9.1 on Linux

I can provide proxy & detailed uvicorn configuration if needed, but I will need to sanitise it, that said, I am not sure if it's needed for this issue.

Important

• We're using Polar.sh so you can upvote and help fund this issue.
• We receive the funding once the issue is completed & confirmed by you.
• Thank you in advance for helping prioritize & fund our backlog.

[euri10]

Hi @jchristgit , yeah send us a repro, in that case the trusted host param you are passing and the headers received should be good I think, the None is weird.

[jchristgit]

Hello @euri10, thank you for the quick response.

I'm no longer at work, but I've set up a small reproducer which essentially has server, proxy, and client on the same host (my development machine), which results in the same effect:

# app.py
from tests.response import Response
from uvicorn.middleware.proxy_headers import ProxyHeadersMiddleware


async def inner(scope, receive, send):
    scheme = scope["scheme"]
    host, port = scope["client"]
    addr = "%s://%s:%d" % (scheme, host, port)
    response = Response("Remote: " + addr, media_type="text/plain")
    await response(scope, receive, send)


app = ProxyHeadersMiddleware(inner, trusted_hosts=['127.0.0.1'])

(This is the same app as used in the unittest).

Started with uvicorn app:app.

HAProxy configuration:

listen uvicorn
    bind 127.0.0.1:5000
    mode http
    option forwardfor
    server uvicorn 127.0.0.1:8000

As a client I just used curl, which hits HAProxy:

$ curl localhost:5000
Remote: http://None:0

The uvicorn log also displays the invalid host & port:

$ uvicorn app:app
INFO:     Started server process [3334]
INFO:     Waiting for application startup.
INFO:     ASGI 'lifespan' protocol appears unsupported.
INFO:     Application startup complete.
INFO:     Uvicorn running on http://127.0.0.1:8000 (Press CTRL+C to quit)
INFO:     None:0 - "GET / HTTP/1.1" 200 OK

The same issue appears when substituting 127.0.0.1 with my local address.

[euri10]

ok I can reproduce, the comment here might be of interest in the PR:

https://github.com/encode/uvicorn/pull/591/files#r402461347

so we're in the case where trusted_host = A and X-Forwarded-For: A if I get that correctly

This could translate in adding this in our test suite this last set of params (last test fails as expected)

@pytest.mark.asyncio
@pytest.mark.parametrize(
    ("trusted_hosts", "x_forwarded_for", "response_text"),
    [
        # # always trust
        # ("*", "1.2.3.4", "Remote: https://1.2.3.4:0"),
        # # trusted proxy
        # ("127.0.0.1", "1.2.3.4", "Remote: https://1.2.3.4:0"),
        # (["127.0.0.1"], "1.2.3.4", "Remote: https://1.2.3.4:0"),
        # # trusted proxy list
        # (["127.0.0.1", "10.0.0.1"], "1.2.3.4", "Remote: https://1.2.3.4:0"),
        # ("127.0.0.1, 10.0.0.1", "1.2.3.4", "Remote: https://1.2.3.4:0"),
        # # request from untrusted proxy
        # ("192.168.0.1", "1.2.3.4", "Remote: http://127.0.0.1:123"),
        # https://github.com/encode/uvicorn/issues/1068
        ("127.0.0.1", "127.0.0.1", "Remote: https://127.0.0.1:0"),
    ],
)
async def test_proxy_headers_trusted_hosts(
    trusted_hosts, x_forwarded_for, response_text
):
    app_with_middleware = ProxyHeadersMiddleware(app, trusted_hosts=trusted_hosts)
    async with httpx.AsyncClient(
        app=app_with_middleware, base_url="http://testserver"
    ) as client:
        headers = {"X-Forwarded-Proto": "https", "X-Forwarded-For": x_forwarded_for}
        response = await client.get("/", headers=headers)

    assert response.status_code == 200
    assert response.text == response_text

so the None possibility was discussed , but I'm now not sure how to handle the situation, any idea @b0g3r since you seemed quite aware of the logic here ?

[euri10]

could do that, this works but not certain of the edge case or implications

        if self.always_trust or (self.trusted_hosts == set(x_forwarded_for_hosts)):
            return x_forwarded_for_hosts[0]

[b0g3r]

I think the current implementation has multiple errors in it:

• It returns the next IP in the list after the last trusted, but it also should return the last trusted in case if it was the last host in the x-forwarded-for list (request came from proxy itself)
• It hasn't fallback to the default behavior if x-forwarded-for is an empty list for some reason
• 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)

I will add a few tests and will try to fix implementation, but the Iack of ready implementations on Github confuses me. Maybe someone sees any other examples of parsing x-forwarded-for in this way?

[euri10]

I will add a few tests and will try to fix implementation, but the Iack of ready implementations on Github confuses me. Maybe someone sees any other examples of parsing x-forwarded-for in this way?

that would be awesome !

[AYMENJD]

In my case i use cloudflare.com and it proxy the requests by its Global CDNs when i get a request on uvicorn it shows to me the cloudflare cdn ip address not the main client ip address but still i can get the client ip by the request header x-forwarded-for and cf-connecting-ip both include client ip

[euri10]

In my case i use cloudflare.com and it proxy the requests by its Global CDNs when i get a request on uvicorn it shows to me the cloudflare cdn ip address not the main client ip address but still i can get the client ip by the request header x-forwarded-for and cf-connecting-ip both include client ip

if you dont mind could you please open a separate issue filling the required information

[jalaziz]

I think the current implementation has multiple errors in it:

• It returns the next IP in the list after the last trusted, but it also should return the last trusted in case if it was the last host in the x-forwarded-for list (request came from proxy itself)
• It hasn't fallback to the default behavior if x-forwarded-for is an empty list for some reason
• 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)

I will add a few tests and will try to fix implementation, but the Iack of ready implementations on Github confuses me. Maybe someone sees any other examples of parsing x-forwarded-for in this way?

Tacking onto this thread, but happy to open a new issue. Another issue I've come across is the inability to use CIDRs for the allowed IPs. In a k8s environment, the proxy could be a different pod(s) which forces you to trust everything which is a bit extreme.