Skip to content

Commit

Permalink
Add support for --forwarded-allow-ips
Browse files Browse the repository at this point in the history
  • Loading branch information
@tomchristie
tomchristie committed Oct 29, 2019
1 parent dc63f8b commit 723d3dc
Show file tree
Hide file tree
Showing 5 changed files with 61 additions and 32 deletions.
2 changes: 1 addition & 1 deletion tests/middleware/test_proxy_headers.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ async def app(scope, receive, send):
await response(scope, receive, send)


app = ProxyHeadersMiddleware(app)
app = ProxyHeadersMiddleware(app, trusted_hosts="*")


def test_proxy_headers():
Expand Down
8 changes: 5 additions & 3 deletions tests/test_config.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
from uvicorn import protocols
from uvicorn.config import Config
from uvicorn.middleware.debug import DebugMiddleware
from uvicorn.middleware.proxy_headers import ProxyHeadersMiddleware
from uvicorn.middleware.wsgi import WSGIMiddleware


Expand All @@ -17,26 +18,27 @@ def wsgi_app():


def test_debug_app():
config = Config(app=asgi_app, debug=True)
config = Config(app=asgi_app, debug=True, proxy_headers=False)
config.load()

assert config.debug is True
assert isinstance(config.loaded_app, DebugMiddleware)


def test_wsgi_app():
config = Config(app=wsgi_app, interface="wsgi")
config = Config(app=wsgi_app, interface="wsgi", proxy_headers=False)
config.load()

assert isinstance(config.loaded_app, WSGIMiddleware)
assert config.interface == "wsgi"


def test_proxy_headers():
config = Config(app=asgi_app, proxy_headers=True)
config = Config(app=asgi_app)
config.load()

assert config.proxy_headers is True
assert isinstance(config.loaded_app, ProxyHeadersMiddleware)


def test_app_unimportable():
Expand Down
11 changes: 9 additions & 2 deletions uvicorn/config.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,9 @@
except ValueError:
DEFAULT_WORKERS = 1

DEFAULT_FORWARDED_IPS = os.environ.get("FORWARDED_ALLOW_IPS", "127.0.0.1")


# Fallback to 'ssl.PROTOCOL_SSLv23' in order to support Python < 3.5.3.
SSL_PROTOCOL_VERSION = getattr(ssl, "PROTOCOL_TLS", ssl.PROTOCOL_SSLv23)

Expand Down Expand Up @@ -124,7 +127,8 @@ def __init__(
reload=False,
reload_dirs=None,
workers=DEFAULT_WORKERS,
proxy_headers=False,
proxy_headers=True,
forwarded_allow_ips=DEFAULT_FORWARDED_IPS,
root_path="",
limit_concurrency=None,
limit_max_requests=None,
Expand Down Expand Up @@ -156,6 +160,7 @@ def __init__(
self.reload = reload
self.workers = workers
self.proxy_headers = proxy_headers
self.forwarded_allow_ips = forwarded_allow_ips
self.root_path = root_path
self.limit_concurrency = limit_concurrency
self.limit_max_requests = limit_max_requests
Expand Down Expand Up @@ -279,7 +284,9 @@ def load(self):
if logger.level <= logging.DEBUG:
self.loaded_app = MessageLoggerMiddleware(self.loaded_app)
if self.proxy_headers:
self.loaded_app = ProxyHeadersMiddleware(self.loaded_app)
self.loaded_app = ProxyHeadersMiddleware(
self.loaded_app, trusted_hosts=self.forwarded_allow_ips
)

self.loaded = True

Expand Down
24 changes: 18 additions & 6 deletions uvicorn/main.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
import click

from uvicorn.config import (
DEFAULT_FORWARDED_IPS,
DEFAULT_WORKERS,
HTTP_PROTOCOLS,
INTERFACES,
Expand Down Expand Up @@ -127,13 +128,22 @@
show_default=True,
)
@click.option(
"--no-access-log", is_flag=True, default=False, help="Disable access log."
"--access-log/--no-access-log",
is_flag=True,
default=True,
help="Enable/Disable access log.",
)
@click.option(
"--proxy-headers",
"--proxy-headers/--no-proxy-headers",
is_flag=True,
default=False,
help="Use X-Forwarded-Proto, X-Forwarded-For, X-Forwarded-Port to populate remote address info.",
default=None,
help="Enable/Disable X-Forwarded-Proto, X-Forwarded-For, X-Forwarded-Port to populate remote address info.",
)
@click.option(
"--forwarded-allow-ips",
type=str,
default=DEFAULT_FORWARDED_IPS,
help="Comma seperated list of IPs to trust with proxy headers. Defaults to the $FORWARDED_ALLOW_IPS environment variable if available, or '127.0.0.1'.",
)
@click.option(
"--root-path",
Expand Down Expand Up @@ -221,8 +231,9 @@ def main(
workers: int,
log_config: str,
log_level: str,
no_access_log: bool,
access_log: bool,
proxy_headers: bool,
forwarded_allow_ips: str,
root_path: str,
limit_concurrency: int,
limit_max_requests: int,
Expand All @@ -249,13 +260,14 @@ def main(
"lifespan": lifespan,
"log_config": LOGGING_CONFIG if log_config is None else log_config,
"log_level": log_level,
"access_log": not no_access_log,
"access_log": access_log,
"interface": interface,
"debug": debug,
"reload": reload,
"reload_dirs": reload_dirs if reload_dirs else None,
"workers": workers,
"proxy_headers": proxy_headers,
"forwarded_allow_ips": forwarded_allow_ips,
"root_path": root_path,
"limit_concurrency": limit_concurrency,
"limit_max_requests": limit_max_requests,
Expand Down
48 changes: 28 additions & 20 deletions uvicorn/middleware/proxy_headers.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,27 +11,35 @@


class ProxyHeadersMiddleware:
def __init__(self, app, num_proxies=1):
def __init__(self, app, trusted_hosts="127.0.0.1"):
self.app = app
self.num_proxies = num_proxies
if isinstance(trusted_hosts, str):
self.trusted_hosts = [item.strip() for item in trusted_hosts.split(",")]
else:
self.trusted_hosts = trusted_hosts
self.always_trust = "*" in self.trusted_hosts

async def __call__(self, scope, receive, send):
if scope["type"] in ("http", "websocket"):
headers = dict(scope["headers"])

if b"x-forwarded-proto" in headers:
# Determine if the incoming request was http or https based on
# the X-Forwarded-Proto header.
x_forwarded_proto = headers[b"x-forwarded-proto"].decode("ascii")
scope["scheme"] = x_forwarded_proto.strip()

if b"x-forwarded-for" in headers:
# Determine the client address from the last trusted IP in the
# X-Forwarded-For header. We've lost the connecting client's port
# information by now, so only include the host.
x_forwarded_for = headers[b"x-forwarded-for"].decode("ascii")
host = x_forwarded_for.split(",")[-self.num_proxies].strip()
port = 0
scope["client"] = (host, port)

await self.app(scope, receive, send)
client_addr = scope.get("client")
client_host = client_addr[0] if client_addr else None

if self.always_trust or client_host in self.trusted_hosts:
headers = dict(scope["headers"])

if b"x-forwarded-proto" in headers:
# Determine if the incoming request was http or https based on
# the X-Forwarded-Proto header.
x_forwarded_proto = headers[b"x-forwarded-proto"].decode("ascii")
scope["scheme"] = x_forwarded_proto.strip()

if b"x-forwarded-for" in headers:
# Determine the client address from the last trusted IP in the
# X-Forwarded-For header. We've lost the connecting client's port
# information by now, so only include the host.
x_forwarded_for = headers[b"x-forwarded-for"].decode("ascii")
host = x_forwarded_for.split(",")[-1].strip()

This comment has been minimized.

Sign in to view

Copy link
@glexey

glexey Nov 21, 2019

Previous code was working for arbitrary number of proxies in the chain, but new code only designed to work for a single proxy?
[-1] will extract the IP of the host connecting to the last proxy. Which in case of a single proxy will be client; in case of multiple proxies it will be a proxy before the last one. https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-Forwarded-For

This comment has been minimized.

Sign in to view

Copy link
@tomchristie

tomchristie Nov 22, 2019

Author Member

Previously num_proxies was always being set to 1, so there's no change here.

(You can't typically trust anything other than the proxy that you're directly behind, because clients can perfectly well include a mock X-Forwarded-For header.)

This comment has been minimized.

Sign in to view

Copy link
@glexey

glexey Nov 30, 2019 •
edited
Loading

Previously num_proxies was always being set to 1

The default setting was 1, the user instantiating the middleware could set it to anything.

You can't typically trust anything other than the proxy that you're directly behind

That depends on the network topology. If your server can only be accessed by going through 1 trusted proxy, you can trust last record in x-forwarded-for. If all requests always go through 2 trusted proxies, you can trust 2 last records, and so on.

This comment has been minimized.

Sign in to view

Copy link
@tomchristie

tomchristie Nov 30, 2019

Author Member

The default setting was 1, the user instantiating the middleware could set it to anything.

True - my point was that we weren't actually exposing that anywhere in uvicorn's command line settings or API.

That depends on the network topology. If your server can only be accessed by going through 1 trusted proxy, you can trust last record in x-forwarded-for. If all requests always go through 2 trusted proxies, you can trust 2 last records, and so on.

It's not clear if we should actually bother exposing this a public API or not. A really good guideline onto that would be "does gunicorn support it".

We might want to instead guide users into using a custom middleware class to deal with more complex setups.

Is this being requested as a hypotetical, or an actual case of "this describes our network"?

This comment has been minimized.

Sign in to view

Copy link
@glexey

glexey Dec 2, 2019

It is an actual network topology case (2 proxies). This is not as much a request, as a clarification on the intent of the code. It looked like a mistake to me, given the comment for the old code was unchanged (and is now confusing). If that's a conscious decision in favor of complexity reduction it's fine, as you said users can always use the custom middleware. I just found it convenient that werkzeug (https://werkzeug.palletsprojects.com/en/0.15.x/middleware/proxy_fix/) and uvicorn were supporting this proxy header "fix" out of the box.

This comment has been minimized.

Sign in to view

Copy link
@Yabk

Yabk Dec 23, 2019 •
edited
Loading

Can we iterate through X-Forwarded-For backwards and take the first address that isn't in the forwarded_allow_ips?

This comment has been minimized.

Sign in to view

Copy link
@b0g3r

b0g3r Mar 22, 2020

Contributor

@Yabk, thank you for the idea! I implemented this in #591

uvicorn/uvicorn/middleware/proxy_headers.py

Lines 22 to 28 in f418bfe

def get_trusted_client_host(self, x_forwarded_for_hosts): # type: (List[str]) -> str
if self.always_trust:
return x_forwarded_for_hosts[0]
for host in reversed(x_forwarded_for_hosts):
if host not in self.trusted_hosts:
return host

port = 0
scope["client"] = (host, port)

return await self.app(scope, receive, send)

0 comments on commit 723d3dc

Please sign in to comment.