Open jchristgit opened 3 years ago
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.
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.
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 ?
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]
I think the current implementation has multiple errors in it:
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?
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 !
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
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
andcf-connecting-ip
both include client ip
if you dont mind could you please open a separate issue filling the required information
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.
Checklist
master
.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 theX-Forwarded-For
header. With today's upgrade to uvicorn0.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:
which results in:
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 https://github.com/encode/uvicorn/pull/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
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.