From 1b3eb6f0a6a7069c0b3f43cd809172fceaf48878 Mon Sep 17 00:00:00 2001 From: Nicholas Hairs Date: Sat, 10 Feb 2024 22:16:17 +1100 Subject: [PATCH] Remove "better UDS handling" --- docs/deployment.md | 11 ----- docs/index.md | 4 -- docs/settings.md | 3 +- tests/middleware/test_proxy_headers.py | 65 +------------------------- uvicorn/config.py | 5 -- uvicorn/main.py | 11 ----- uvicorn/middleware/proxy_headers.py | 40 ++-------------- 7 files changed, 7 insertions(+), 132 deletions(-) diff --git a/docs/deployment.md b/docs/deployment.md index 419c2375f..2d3f576b0 100644 --- a/docs/deployment.md +++ b/docs/deployment.md @@ -99,10 +99,6 @@ Options: to the $FORWARDED_ALLOW_IPS environment variable if available, or '127.0.0.1'. The literal '*' means trust everything. - --forwarded-trust-literals Trust all literals in proxy headers. - Defaults to False. Useful when using - multiple proxies with UNIX Domain Sockets - for transport. --root-path TEXT Set the ASGI 'root_path' for applications submounted below a given URL path. --limit-concurrency INTEGER Maximum number of concurrent connections or @@ -370,13 +366,6 @@ For example: - when NGINX itself is running behind a UDS it will add the literal `unix:` as the client in the `X-Forwarded-For` header. - When Uvicorn is running behind a UDS the initial client will be `None`. -In the case of Uvicorn, if it run using `--uds`, then it will automatically trust `None` as the initial client. - -If you are running Uvicorn behind multiple layers of proxies which use UDS, or you are not able to know the path of the sockets at initialisation, you can use the `--forwarded-trust-literals` setting to trust all literals without specifying them. You can still set `--forwarded-trust-ips` to trust other addresses and networks. - -!!! Warning: Malformed `X-Forwarded-For` values - `X-Forwarded-For` values that cannot be converted to an IP Address will be treated as literals when processing the header. This means that if a proxy adds a malformed value it will be treated as a literal, in combination with `--forwarded-trust-literals` this may result in unexpected values being trusted. - ### Trust Everything diff --git a/docs/index.md b/docs/index.md index ef54e3d62..38a6892a5 100644 --- a/docs/index.md +++ b/docs/index.md @@ -169,10 +169,6 @@ Options: to the $FORWARDED_ALLOW_IPS environment variable if available, or '127.0.0.1'. The literal '*' means trust everything. - --forwarded-trust-literals Trust all literals in proxy headers. - Defaults to False. Useful when using - multiple proxies with UNIX Domain Sockets - for transport. --root-path TEXT Set the ASGI 'root_path' for applications submounted below a given URL path. --limit-concurrency INTEGER Maximum number of concurrent connections or diff --git a/docs/settings.md b/docs/settings.md index 5c2bb10a1..a4439c3d0 100644 --- a/docs/settings.md +++ b/docs/settings.md @@ -88,10 +88,9 @@ Note that WSGI mode always disables WebSocket support, as it is not supported by ## HTTP * `--root-path ` - Set the ASGI `root_path` for applications submounted below a given URL path. -* `--proxy-headers` / `--no-proxy-headers` - Enable/Disable X-Forwarded-Proto, X-Forwarded-For, X-Forwarded-Port to populate remote address info. Defaults to enabled, but is restricted to only trusting +* `--proxy-headers` / `--no-proxy-headers` - Enable/Disable X-Forwarded-Proto, X-Forwarded-For to populate remote address info. Defaults to enabled, but is restricted to only trusting connecting IPs in the `forwarded-allow-ips` configuration. * `--forwarded-allow-ips` Comma separated list of IP Addresses, IP Networks, or literals (e.g. UNIX Socket path) to trust with proxy headers. Defaults to the `$FORWARDED_ALLOW_IPS` environment variable if available, or '127.0.0.1'. The literal `'*'` means trust everything. -* `--forwarded-trust-literals` - Trust all literals in proxy headers. Defaults to False. Useful when using multiple proxies with UNIX Domain Sockets for transport. * `--server-header` / `--no-server-header` - Enable/Disable default `Server` header. * `--date-header` / `--no-date-header` - Enable/Disable default `Date` header. diff --git a/tests/middleware/test_proxy_headers.py b/tests/middleware/test_proxy_headers.py index c8b3b167a..26eb09081 100644 --- a/tests/middleware/test_proxy_headers.py +++ b/tests/middleware/test_proxy_headers.py @@ -36,50 +36,20 @@ async def default_app( await response(scope, receive, send) -class UdsMiddleware: - """Middleware that mimics ProxyHeadersMiddleware but first sets client to `None` - - Manually acts as if Uvicorn is running behind a UNIX Domain Socket - """ - - def __init__(self, *args, **kwargs) -> None: - self.app = ProxyHeadersMiddleware(*args, **kwargs) - return - - async def __call__( - self, - scope: Scope, - receive: ASGIReceiveCallable, - send: ASGISendCallable, - ) -> None: - if scope["type"] in ("http", "websocket"): - scope["client"] = None # type: ignore - return await self.app(scope, receive, send) - - def make_httpx_client( trusted_hosts: str | list[str], client: tuple[str, int] = ("127.0.0.1", 123), - trust_none_client: bool = False, - trust_all_literal_clients: bool = False, - app_class: type = ProxyHeadersMiddleware, ) -> httpx.AsyncClient: """Create async client for use in test cases Args: trusted_hosts: trusted_hosts for proxy middleware client: transport client to use - trust_none_client: trust_none_client for proxy middleware - trust_all_literal_clients: trust_all_literal_clients for proxy middleware - app_class: ASGI app to use with transport. - This is so that can use `UdsMiddleware`. """ app = cast( httpx._transports.asgi._ASGIApp, - app_class( - default_app, trusted_hosts, trust_none_client, trust_all_literal_clients - ), + ProxyHeadersMiddleware(default_app, trusted_hosts), ) transport = httpx.ASGITransport(app=app, client=client) return httpx.AsyncClient(transport=transport, base_url="http://testserver") @@ -391,25 +361,6 @@ def test_forwarded_hosts( assert (test_host in trusted_hosts) is expected -@pytest.mark.parametrize( - ("test_host", "expected"), - [ - ("127.0.0.1", True), - ("192.168.0.1", False), - ("::1", True), - ("ff::1", False), - ("some-literal", True), - ("1.2.34.5.6.7", True), # invalid ipv4 - (":::1", True), # invalip ipv6 - ("unix:", True), - ("", False), # empty string is not literal - ], -) -def test_forwarded_hosts_trust_literals(test_host: str, expected: bool) -> None: - trusted_hosts = _TrustedHosts("127.0.0.1, ::1, some-literal", True) - assert (test_host in trusted_hosts) is expected - - @pytest.mark.anyio @pytest.mark.parametrize( ("trusted_hosts", "expected"), @@ -517,20 +468,6 @@ async def websocket_app(scope, receive, send): assert data == "wss://1.2.3.4:0" -@pytest.mark.anyio -@pytest.mark.parametrize( - ("trust_none_client", "expected"), - [(False, "http://NONE"), (True, "https://1.2.3.4:0")], -) -async def test_proxy_headers_uds(trust_none_client: bool, expected: str) -> None: - async with make_httpx_client( - "10.0.0.0/8", trust_none_client=trust_none_client, app_class=UdsMiddleware - ) as client: - response = await client.get("/", headers=make_x_headers("1.2.3.4, 10.1.1.1")) - assert response.status_code == 200 - assert response.text == expected - - @pytest.mark.anyio async def test_proxy_headers_empty_x_forwarded_for() -> None: # fallback to the default behavior if x-forwarded-for is an empty list diff --git a/uvicorn/config.py b/uvicorn/config.py index 1ba52429e..8100933f9 100644 --- a/uvicorn/config.py +++ b/uvicorn/config.py @@ -208,7 +208,6 @@ def __init__( server_header: bool = True, date_header: bool = True, forwarded_allow_ips: list[str] | str | None = None, - forwarded_trust_literals: bool = False, root_path: str = "", limit_concurrency: int | None = None, limit_max_requests: int | None = None, @@ -350,8 +349,6 @@ def __init__( else: self.forwarded_allow_ips = forwarded_allow_ips - self.forwarded_trust_literals = forwarded_trust_literals - if self.reload and self.workers > 1: logger.warning('"workers" flag is ignored when reloading is enabled.') @@ -498,8 +495,6 @@ def load(self) -> None: self.loaded_app = ProxyHeadersMiddleware( self.loaded_app, trusted_hosts=self.forwarded_allow_ips, - trust_none_client=bool(self.uds), - trust_all_literal_clients=self.forwarded_trust_literals, ) self.loaded = True diff --git a/uvicorn/main.py b/uvicorn/main.py index 2eb688b56..82a7cc43b 100644 --- a/uvicorn/main.py +++ b/uvicorn/main.py @@ -250,13 +250,6 @@ def print_version(ctx: click.Context, param: click.Parameter, value: bool) -> No "$FORWARDED_ALLOW_IPS environment variable if available, or '127.0.0.1'. " "The literal '*' means trust everything.", ) -@click.option( - "--forwarded-trust-literals", - is_flag=True, - default=False, - help="Trust all literals in proxy headers. Defaults to False. Useful when using " - "multiple proxies with UNIX Domain Sockets for transport.", -) @click.option( "--root-path", type=str, @@ -405,7 +398,6 @@ def main( server_header: bool, date_header: bool, forwarded_allow_ips: str, - forwarded_trust_literals: bool, root_path: str, limit_concurrency: int, backlog: int, @@ -455,7 +447,6 @@ def main( server_header=server_header, date_header=date_header, forwarded_allow_ips=forwarded_allow_ips, - forwarded_trust_literals=forwarded_trust_literals, root_path=root_path, limit_concurrency=limit_concurrency, backlog=backlog, @@ -508,7 +499,6 @@ def run( server_header: bool = True, date_header: bool = True, forwarded_allow_ips: list[str] | str | None = None, - forwarded_trust_literals: bool = False, root_path: str = "", limit_concurrency: int | None = None, backlog: int = 2048, @@ -561,7 +551,6 @@ def run( server_header=server_header, date_header=date_header, forwarded_allow_ips=forwarded_allow_ips, - forwarded_trust_literals=forwarded_trust_literals, root_path=root_path, limit_concurrency=limit_concurrency, backlog=backlog, diff --git a/uvicorn/middleware/proxy_headers.py b/uvicorn/middleware/proxy_headers.py index 7a2e0e2e8..64b0abc5e 100644 --- a/uvicorn/middleware/proxy_headers.py +++ b/uvicorn/middleware/proxy_headers.py @@ -21,10 +21,8 @@ class _TrustedHosts: def __init__( self, trusted_hosts: Union[List[str], str], - trust_all_literal_clients: bool = False, ) -> None: self.always_trust: bool = trusted_hosts == "*" - self.trust_all_literal_clients = trust_all_literal_clients self.trusted_literals: Set[str] = set() self.trusted_hosts: Set[ipaddress._BaseAddress] = set() @@ -77,8 +75,6 @@ def __contains__(self, item: Optional[str]) -> bool: return any(ip in net for net in self.trusted_networks) except ValueError: - if self.trust_all_literal_clients: - return True return item in self.trusted_literals def get_trusted_client_host(self, x_forwarded_for: str) -> Optional[str]: @@ -109,8 +105,8 @@ def __str__(self) -> str: return ( f"{self.__class__.__name__}({self.trusted_hosts=}, " - f"{self.trusted_networks=}, {self.trusted_literals=}, " - f"{self.trust_all_literal_clients=}" + f"{self.trusted_networks=}, {self.trusted_literals=}" + ")" ) @@ -124,16 +120,6 @@ class ProxyHeadersMiddleware: Modifies the `client` and `scheme` information so that they reference the connecting client, rather that the connecting proxy. - Use `trust_none_client = True` to continue with proxy header handling when - the initial client is `None`. This does not affect the handling of client - adddresses in the proxy headers. If you are listening on a UNIX Domain Sockets - you will need to set this to `True` to enable proxy handling. - - Use `trust_all_literal_clients = True` to trust all non-IP clients in the header. - This is useful if you are using UNIX Domain Sockets to communicate between your - proxies and do not wish to list each literal or if you do not know the value of - each literal and startup. - References: - @@ -144,13 +130,9 @@ def __init__( self, app: "ASGI3Application", trusted_hosts: Union[List[str], str] = "127.0.0.1", - trust_none_client: bool = False, - trust_all_literal_clients: bool = False, ) -> None: self.app = app - self.trusted_hosts = _TrustedHosts(trusted_hosts, trust_all_literal_clients) - self.trust_none_client = trust_none_client - print(self.trusted_hosts) + self.trusted_hosts = _TrustedHosts(trusted_hosts) return async def __call__( @@ -159,20 +141,11 @@ async def __call__( if scope["type"] in ("http", "websocket"): scope = cast(Union[HTTPScope, WebSocketScope], scope) client_addr: Optional[Tuple[str, int]] = scope.get("client") + client_host = client_addr[0] if client_addr else None - if (client_addr is None and self.trust_none_client) or ( - client_addr is not None and client_addr[0] in self.trusted_hosts - ): + if client_host in self.trusted_hosts: headers = dict(scope["headers"]) - # if b"forwarded" in headers: - # ... - # https://github.com/encode/uvicorn/discussions/2236 - # TODO: We should probably also support the Forwarded header: - # https://datatracker.ietf.org/doc/html/rfc7239 - # https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Forwarded - # If we do it should take precedence over the x-forwarded-* headers. - if b"x-forwarded-proto" in headers: # Determine if the incoming request was http or https based on # the X-Forwarded-Proto header. @@ -204,8 +177,5 @@ async def __call__( # if b"x-forwarded-port" in headers: # ... # https://github.com/encode/uvicorn/issues/1974 - # TODO: Are we able to reliabily extract x-forwarded-port? - # https://docs.aws.amazon.com/elasticloadbalancing/latest/classic/x-forwarded-headers.html#x-forwarded-port - # If yes we should update the NGINX in docs/deployment.md return await self.app(scope, receive, send)