From 6b6ff29b9a6291dcf899d211b539c36dd0a81e82 Mon Sep 17 00:00:00 2001 From: Johan Lundberg Date: Sun, 5 Jul 2026 00:00:58 +0200 Subject: [PATCH] Harden forward-auth redirect and header handling Tighten forward-auth return URL validation to reject unsafe host syntax, including backslash-based browser/parser mismatches, and normalize scheme/port matching so bare allowlist entries are HTTPS default-port only. Support explicit scheme-qualified targets, including IPv6 host:port entries. Always emit deterministic identity headers with empty values when attributes are absent, zero unsafe header values, and clear stale forward-auth return targets when OIDC or registration flows take precedence. Add regression coverage for wildcard redirect bypasses, scheme/port handling, IPv6 allowlist entries, empty identity headers, unsafe header values, and stale return-target cleanup. Update docs with proxy header-stripping guidance and the new allowlist semantics. --- README.md | 13 +- src/porchlight/authn/routes.py | 5 +- src/porchlight/config.py | 5 +- src/porchlight/forward_auth.py | 231 +++++++++++++++++++++++++-------- tests/test_forward_auth.py | 131 ++++++++++++++++++- 5 files changed, 325 insertions(+), 60 deletions(-) diff --git a/README.md b/README.md index 9e50d22..d5cbe44 100644 --- a/README.md +++ b/README.md @@ -140,9 +140,16 @@ forward_auth_allowed_redirect_hosts = ["app.example.com", "*.apps.example.com"] The endpoint uses `X-Forwarded-Proto`, `X-Forwarded-Host`, and `X-Forwarded-Uri` (or `X-Original-Url`) to reconstruct the original request URL. -Only exact hosts, `host:port` entries, or explicit `*.example.com` wildcard -suffixes listed in `forward_auth_allowed_redirect_hosts` are accepted as return -targets. +Bare host patterns are HTTPS-only and match the default HTTPS port; `host:port` +patterns are also HTTPS-only unless you include an explicit scheme such as +`http://localhost:9000` for local cleartext deployments. Exact hosts, +`host:port` entries, scheme-qualified hosts, and explicit `*.example.com` +wildcard suffixes listed in `forward_auth_allowed_redirect_hosts` are accepted +as return targets. + +Configure the reverse proxy to strip any inbound `Remote-*` and `X-Forwarded-*` +identity headers from client requests before applying the headers returned by +`/forward-auth`. To use a config file at a different path: diff --git a/src/porchlight/authn/routes.py b/src/porchlight/authn/routes.py index 22840d2..c01fb13 100644 --- a/src/porchlight/authn/routes.py +++ b/src/porchlight/authn/routes.py @@ -9,6 +9,7 @@ from porchlight.authn.acr import ACR_PASSWORD, ACR_WEBAUTHN, SESSION_ACR_KEY from porchlight.forward_auth import ( FORWARD_AUTH_RETURN_TO_SESSION_KEY, capture_forward_auth_return_to, + clear_forward_auth_return_to, pop_forward_auth_return_to, ) from porchlight.models import User @@ -25,6 +26,7 @@ def _login_redirect_target(request: Request) -> str: Otherwise, redirect to credential management. """ if "oidc_auth_request" in request.session: + clear_forward_auth_return_to(request) return "/authorization/complete" forward_auth_return_to = pop_forward_auth_return_to(request) if forward_auth_return_to is not None: @@ -60,7 +62,7 @@ def _establish_authenticated_session(request: Request, user: User, acr: str) -> request.session.clear() if pending_oidc is not None: request.session["oidc_auth_request"] = pending_oidc - if pending_forward_auth is not None: + elif pending_forward_auth is not None: request.session[FORWARD_AUTH_RETURN_TO_SESSION_KEY] = pending_forward_auth request.session["userid"] = user.userid request.session["username"] = user.username @@ -172,6 +174,7 @@ async def register_magic_link(request: Request, token: str) -> Response: # Magic-link registration is single-factor (email possession); mark it as # such. It normally redirects to credential setup rather than completing an # OIDC flow, but the session acr governs any later authorization too. + clear_forward_auth_return_to(request) _establish_authenticated_session(request, user, ACR_PASSWORD) return RedirectResponse("/manage/credentials?setup=1", status_code=303) diff --git a/src/porchlight/config.py b/src/porchlight/config.py index 3036f58..2ac30ac 100644 --- a/src/porchlight/config.py +++ b/src/porchlight/config.py @@ -71,8 +71,9 @@ class Settings(BaseSettings): # Reverse-proxy forward-auth. Hosts listed here are eligible post-login # redirect targets when /forward-auth receives X-Forwarded-* request - # metadata. Supports exact hosts, host:port, and explicit "*.example.com" - # wildcard suffixes. + # metadata. Bare hosts and host:port entries are HTTPS-only; include an + # explicit scheme such as "http://localhost:9000" for cleartext targets. + # Supports exact hosts and explicit "*.example.com" wildcard suffixes. forward_auth_allowed_redirect_hosts: list[str] = [] # Signing keys diff --git a/src/porchlight/forward_auth.py b/src/porchlight/forward_auth.py index 9872797..07a9574 100644 --- a/src/porchlight/forward_auth.py +++ b/src/porchlight/forward_auth.py @@ -1,5 +1,7 @@ +import ipaddress +import re from dataclasses import dataclass -from urllib.parse import quote, urlsplit, urlunsplit +from urllib.parse import SplitResult, quote, urlsplit, urlunsplit from fastapi import APIRouter, Request from fastapi.responses import RedirectResponse @@ -16,13 +18,28 @@ router = APIRouter(tags=["forward-auth"]) _FORWARD_AUTH_METHODS = ("GET", "HEAD", "POST", "PUT", "PATCH", "DELETE", "OPTIONS") _ORIGINAL_URL_HEADERS = ("x-original-url", "x-forwarded-url") _ORIGINAL_URI_HEADERS = ("x-forwarded-uri", "x-original-uri") +_DNS_LABEL_RE = re.compile(r"^[a-z0-9](?:[a-z0-9-]{0,61}[a-z0-9])?$") +_DEFAULT_PORTS = {"http": 80, "https": 443} +_HEADER_MIN_VISIBLE = 0x20 +_HOST_MIN_VISIBLE = 0x21 +_ASCII_DELETE = 0x7F +_C1_CONTROL_MAX = 0x9F +_MAX_DNS_HOSTNAME_LENGTH = 253 @dataclass(frozen=True) class _HostPattern: host: str - port: int | None = None - scheme: str | None = None + port: int + scheme: str + wildcard: bool = False + + +@dataclass(frozen=True) +class _ReturnTarget: + host: str + port: int + scheme: str @router.api_route("/forward-auth", methods=list(_FORWARD_AUTH_METHODS)) @@ -61,6 +78,11 @@ def capture_forward_auth_return_to(request: Request) -> None: request.session.pop(FORWARD_AUTH_RETURN_TO_SESSION_KEY, None) +def clear_forward_auth_return_to(request: Request) -> None: + """Remove any stored forward-auth return URL from the current session.""" + request.session.pop(FORWARD_AUTH_RETURN_TO_SESSION_KEY, None) + + def pop_forward_auth_return_to(request: Request) -> str | None: """Consume the stored forward-auth return URL if it is still allowed.""" return_to = request.session.pop(FORWARD_AUTH_RETURN_TO_SESSION_KEY, None) @@ -75,25 +97,45 @@ def pop_forward_auth_return_to(request: Request) -> str | None: def is_forward_auth_return_to_allowed(settings: Settings, return_to: str) -> bool: """Return true when ``return_to`` is an HTTP(S) URL on an allowed host.""" + target = _parse_return_target(return_to) + if target is None: + return False + + return any( + _host_pattern_matches(pattern, target.host, target.port, target.scheme) + for pattern in settings.forward_auth_allowed_redirect_hosts + ) + + +def _parse_return_target(return_to: str) -> _ReturnTarget | None: try: parsed = urlsplit(return_to) except ValueError: - return False + return None - if parsed.scheme not in ("http", "https") or parsed.hostname is None: - return False - if parsed.username is not None or parsed.password is not None: - return False + return_port = _return_target_port(parsed) + if return_port is None or parsed.hostname is None: + return None + hostname = _normalize_url_hostname(parsed.hostname) + if hostname is None: + return None + return _ReturnTarget(host=hostname, port=return_port, scheme=parsed.scheme) + + +def _return_target_port(parsed: SplitResult) -> int | None: + if ( + parsed.scheme not in ("http", "https") + or parsed.hostname is None + or parsed.username is not None + or parsed.password is not None + or _has_unsafe_url_host_chars(parsed.netloc) + ): + return None try: - return_port = parsed.port + return _effective_port(parsed.scheme, parsed.port) except ValueError: - return False - - for pattern in settings.forward_auth_allowed_redirect_hosts: - if _host_pattern_matches(pattern, parsed.hostname, return_port, parsed.scheme): - return True - return False + return None def _login_redirect_response(request: Request) -> RedirectResponse: @@ -146,47 +188,45 @@ def _first_header_value(request: Request, names: tuple[str, ...]) -> str | None: def _identity_headers(user: User) -> dict[str, str]: display_name = user.preferred_username or user.username + groups = ",".join(sorted(user.groups)) headers = { "Remote-User": user.username, + "Remote-Email": user.email or "", + "Remote-Groups": groups, "X-Forwarded-User": user.username, "X-Forwarded-Userid": user.userid, "X-Forwarded-Preferred-Username": display_name, + "X-Forwarded-Email": user.email or "", + "X-Forwarded-Groups": groups, } - if user.email: - headers["Remote-Email"] = user.email - headers["X-Forwarded-Email"] = user.email - if user.groups: - groups = ",".join(sorted(user.groups)) - headers["Remote-Groups"] = groups - headers["X-Forwarded-Groups"] = groups - - return {name: value for name, value in headers.items() if _is_safe_header_value(value)} + return {name: _safe_header_value(value) for name, value in headers.items()} -def _is_safe_header_value(value: str) -> bool: - if "\r" in value or "\n" in value: - return False +def _safe_header_value(value: str) -> str: try: value.encode("latin-1") except UnicodeEncodeError: - return False - return True + return "" + if any(_is_http_header_control(ch) for ch in value): + return "" + return value -def _host_pattern_matches(pattern: str, hostname: str, port: int | None, scheme: str) -> bool: +def _is_http_header_control(ch: str) -> bool: + codepoint = ord(ch) + return (codepoint < _HEADER_MIN_VISIBLE and ch != "\t") or _ASCII_DELETE <= codepoint <= _C1_CONTROL_MAX + + +def _host_pattern_matches(pattern: str, hostname: str, port: int, scheme: str) -> bool: parsed = _parse_host_pattern(pattern) if parsed is None: return False - if parsed.scheme is not None and parsed.scheme != scheme: - return False - if parsed.port is not None and parsed.port != port: + if parsed.scheme != scheme or parsed.port != port: return False - hostname = hostname.lower().rstrip(".") - if parsed.host.startswith("*."): - suffix = parsed.host[1:] - return hostname.endswith(suffix) and hostname != suffix.removeprefix(".") + if parsed.wildcard: + return hostname.endswith(f".{parsed.host}") return hostname == parsed.host @@ -196,20 +236,105 @@ def _parse_host_pattern(pattern: str) -> _HostPattern | None: return None if "://" in pattern: - try: - parsed_pattern = urlsplit(pattern) - pattern_host = parsed_pattern.hostname - pattern_port = parsed_pattern.port - except ValueError: - return None - if parsed_pattern.scheme not in ("http", "https") or pattern_host is None: - return None - return _HostPattern(host=pattern_host.rstrip("."), port=pattern_port, scheme=parsed_pattern.scheme) + return _parse_scheme_qualified_host_pattern(pattern) + return _parse_host_port_pattern(pattern, default_scheme="https") - pattern_port = None - if pattern.count(":") == 1: - possible_host, possible_port = pattern.rsplit(":", 1) - if possible_port.isdigit(): - pattern = possible_host.rstrip(".") - pattern_port = int(possible_port) - return _HostPattern(host=pattern, port=pattern_port) + +def _parse_scheme_qualified_host_pattern(pattern: str) -> _HostPattern | None: + try: + parsed = urlsplit(pattern) + if parsed.scheme not in ("http", "https"): + return None + port = _effective_port(parsed.scheme, parsed.port) + except ValueError: + return None + + if ( + parsed.hostname is None + or parsed.username is not None + or parsed.password is not None + or parsed.path not in ("", "/") + or parsed.query + or parsed.fragment + or _has_unsafe_url_host_chars(parsed.netloc) + ): + return None + return _build_host_pattern(parsed.hostname, port=port, scheme=parsed.scheme) + + +def _parse_host_port_pattern(pattern: str, default_scheme: str) -> _HostPattern | None: + try: + parsed = urlsplit(f"//{pattern}") + port = _effective_port(default_scheme, parsed.port) + except ValueError: + return None + + if ( + parsed.hostname is None + or parsed.username is not None + or parsed.password is not None + or parsed.path + or parsed.query + or parsed.fragment + or _has_unsafe_url_host_chars(parsed.netloc) + ): + return None + return _build_host_pattern(parsed.hostname, port=port, scheme=default_scheme) + + +def _build_host_pattern(hostname: str, *, port: int, scheme: str) -> _HostPattern | None: + raw_hostname = hostname.lower().rstrip(".") + wildcard = raw_hostname.startswith("*.") + if wildcard: + normalized = _normalize_dns_hostname(raw_hostname.removeprefix("*.")) + if normalized is None: + return None + else: + normalized = _normalize_url_hostname(raw_hostname) + if normalized is None: + return None + return _HostPattern(host=normalized, port=port, scheme=scheme, wildcard=wildcard) + + +def _effective_port(scheme: str, port: int | None) -> int: + if port is not None: + return port + return _DEFAULT_PORTS[scheme] + + +def _normalize_url_hostname(hostname: str) -> str | None: + hostname = hostname.lower().rstrip(".") + if _has_unsafe_hostname_chars(hostname): + return None + + try: + ipaddress.ip_address(hostname) + except ValueError: + return _normalize_dns_hostname(hostname) + return hostname + + +def _normalize_dns_hostname(hostname: str) -> str | None: + if _has_unsafe_hostname_chars(hostname): + return None + try: + hostname.encode("ascii") + except UnicodeEncodeError: + return None + + labels = hostname.split(".") + if not labels or any(not _DNS_LABEL_RE.fullmatch(label) for label in labels): + return None + if len(hostname) > _MAX_DNS_HOSTNAME_LENGTH: + return None + return hostname + + +def _has_unsafe_url_host_chars(value: str) -> bool: + return "\\" in value or any(ord(ch) < _HOST_MIN_VISIBLE or ord(ch) == _ASCII_DELETE for ch in value) + + +def _has_unsafe_hostname_chars(value: str) -> bool: + if "\\" in value or "%" in value or "*" in value: + return True + return any(ord(ch) < _HOST_MIN_VISIBLE or ord(ch) == _ASCII_DELETE for ch in value) diff --git a/tests/test_forward_auth.py b/tests/test_forward_auth.py index ccf4c80..f085e24 100644 --- a/tests/test_forward_auth.py +++ b/tests/test_forward_auth.py @@ -1,11 +1,14 @@ import re from datetime import UTC, datetime +from unittest.mock import MagicMock from urllib.parse import parse_qs, urlsplit from argon2 import PasswordHasher from httpx import AsyncClient from porchlight.authn.password import PasswordService +from porchlight.authn.routes import _login_redirect_target +from porchlight.forward_auth import FORWARD_AUTH_RETURN_TO_SESSION_KEY from porchlight.models import PasswordCredential, User @@ -23,7 +26,7 @@ async def _create_user( username=username, preferred_username="Alice", email=email, - groups=groups or ["users"], + groups=groups if groups is not None else ["users"], created_at=datetime.now(UTC), updated_at=datetime.now(UTC), ) @@ -108,6 +111,81 @@ async def test_forward_auth_ignores_disallowed_return_host(client: AsyncClient) assert res.headers["location"] == "http://localhost:8000/login" +async def test_forward_auth_rejects_backslash_wildcard_return_host(client: AsyncClient) -> None: + _allow_forward_auth_hosts(client, ["*.apps.example.com"]) + + res = await client.get( + "/forward-auth", + headers={ + "X-Forwarded-Proto": "https", + "X-Forwarded-Host": r"evil.com\x.apps.example.com", + "X-Forwarded-Uri": "/private", + }, + follow_redirects=False, + ) + + assert res.status_code == 303 + assert res.headers["location"] == "http://localhost:8000/login" + + +async def test_forward_auth_bare_hosts_require_https_default_port(client: AsyncClient) -> None: + _allow_forward_auth_hosts(client, ["app.example.com"]) + + http_res = await client.get( + "/forward-auth", + headers={ + "X-Forwarded-Proto": "http", + "X-Forwarded-Host": "app.example.com", + "X-Forwarded-Uri": "/private", + }, + follow_redirects=False, + ) + custom_port_res = await client.get( + "/forward-auth", + headers={ + "X-Forwarded-Proto": "https", + "X-Forwarded-Host": "app.example.com:1337", + "X-Forwarded-Uri": "/private", + }, + follow_redirects=False, + ) + + assert http_res.headers["location"] == "http://localhost:8000/login" + assert custom_port_res.headers["location"] == "http://localhost:8000/login" + + +async def test_forward_auth_normalizes_default_port_patterns(client: AsyncClient) -> None: + _allow_forward_auth_hosts(client, ["app.example.com:443"]) + + res = await client.get( + "/forward-auth", + headers={ + "X-Forwarded-Proto": "https", + "X-Forwarded-Host": "app.example.com", + "X-Forwarded-Uri": "/private", + }, + follow_redirects=False, + ) + + assert parse_qs(urlsplit(res.headers["location"]).query) == {"return_to": ["https://app.example.com/private"]} + + +async def test_forward_auth_supports_scheme_qualified_ipv6_patterns(client: AsyncClient) -> None: + _allow_forward_auth_hosts(client, ["http://[::1]:8080"]) + + res = await client.get( + "/forward-auth", + headers={ + "X-Forwarded-Proto": "http", + "X-Forwarded-Host": "[::1]:8080", + "X-Forwarded-Uri": "/private", + }, + follow_redirects=False, + ) + + assert parse_qs(urlsplit(res.headers["location"]).query) == {"return_to": ["http://[::1]:8080/private"]} + + async def test_forward_auth_allows_non_safe_proxy_subrequests_without_csrf(client: AsyncClient) -> None: res = await client.post("/forward-auth", follow_redirects=False) @@ -153,6 +231,21 @@ async def test_login_ignores_disallowed_forward_auth_return_to(client: AsyncClie assert res.headers["HX-Redirect"] == "/manage/credentials" +async def test_login_rejects_backslash_wildcard_return_to(client: AsyncClient) -> None: + _allow_forward_auth_hosts(client, ["*.apps.example.com"]) + await _create_user(client) + + token = await _login_page_csrf(client, "https://evil.com%5Cx.apps.example.com/") + res = await client.post( + "/login/password", + data={"username": "alice", "password": "password123!Secure"}, + headers={"HX-Request": "true", "X-CSRF-Token": token}, + ) + + assert res.status_code == 200 + assert res.headers["HX-Redirect"] == "/manage/credentials" + + async def test_forward_auth_authenticated_user_returns_identity_headers(client: AsyncClient) -> None: await _create_user(client, groups=["users", "admin"], email="alice@example.com") await _login(client) @@ -171,6 +264,31 @@ async def test_forward_auth_authenticated_user_returns_identity_headers(client: assert res.headers["cache-control"] == "no-store" +async def test_forward_auth_authenticated_user_returns_empty_identity_headers(client: AsyncClient) -> None: + await _create_user(client, groups=[], email=None) + await _login(client) + + res = await client.get("/forward-auth", follow_redirects=False) + + assert res.status_code == 204 + assert res.headers["remote-email"] == "" + assert res.headers["remote-groups"] == "" + assert res.headers["x-forwarded-email"] == "" + assert res.headers["x-forwarded-groups"] == "" + + +async def test_forward_auth_unsafe_header_values_are_zeroed(client: AsyncClient) -> None: + user = await _create_user(client) + app = client._transport.app # type: ignore[union-attr] + await app.state.user_repo.update(user.model_copy(update={"preferred_username": "bad\x0bname"})) + await _login(client) + + res = await client.get("/forward-auth", follow_redirects=False) + + assert res.status_code == 204 + assert res.headers["x-forwarded-preferred-username"] == "" + + async def test_forward_auth_rejects_inactive_session_user(client: AsyncClient) -> None: user = await _create_user(client) await _login(client) @@ -182,3 +300,14 @@ async def test_forward_auth_rejects_inactive_session_user(client: AsyncClient) - assert res.status_code == 303 assert "remote-user" not in res.headers + + +def test_login_redirect_target_clears_forward_auth_when_oidc_wins() -> None: + request = MagicMock() + request.session = { + "oidc_auth_request": {"client_id": "test-rp"}, + FORWARD_AUTH_RETURN_TO_SESSION_KEY: "https://app.example.com/private", + } + + assert _login_redirect_target(request) == "/authorization/complete" + assert FORWARD_AUTH_RETURN_TO_SESSION_KEY not in request.session