From 583d43f2136191b47c0c0ab167b2fdbaa4b0ffeb Mon Sep 17 00:00:00 2001 From: Renzo F <45401804+rennf93@users.noreply.github.com> Date: Wed, 22 Jul 2026 20:29:54 +0200 Subject: [PATCH] fix(guard): resolve the tailnet client behind host-proxy hops for the WAF (#646) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit fastapi-guard peels a fixed trusted_proxy_depth=1 from X-Forwarded-For (the rightmost entry, which nginx itself recorded). That is correct for every chain except host-proxied tailnet traffic (Tailscale Serve → nginx), which arrives as [tailnet-client, loopback-or-bridge-gateway] — depth-1 resolves it to a whitelisted hop IP, leaving WAF/ban/rate-limit inert for the whole /tg surface (the documented ceiling). ClientIpResolutionMiddleware (pure ASGI, wraps SecurityMiddleware so it runs first) stamps guard_core's request.state.client_ip cache — its supported pre-resolution seam — for EXACTLY that shape: peel known local hops (loopback + docker bridge pool) from the right, stamp only when at least one hop was peeled AND the candidate is in the tailnet CGNAT range (100.64.0.0/10). Every other shape abstains, so direct LAN clients, agent containers relaying through nginx (even with forged public-IP prefixes), and all-hops operator traffic resolve byte-for-byte as before. Documented residual: a same-bridge container forging a CGNAT prefix only DE-privileges itself (loses its whitelist exemption). XFF is read first-occurrence to match Starlette's own header semantics, and a wiring test pins the middleware mount ORDER, not just presence. Co-authored-by: Renn F --- roboco/security.py | 123 +++++++++++++-- tests/unit/test_security_client_ip.py | 213 ++++++++++++++++++++++++++ 2 files changed, 327 insertions(+), 9 deletions(-) create mode 100644 tests/unit/test_security_client_ip.py diff --git a/roboco/security.py b/roboco/security.py index 8317ad77..b0dcd004 100644 --- a/roboco/security.py +++ b/roboco/security.py @@ -16,6 +16,7 @@ Cloud-host-ready but env-driven: ``enforce_https`` follows from __future__ import annotations import re +from ipaddress import ip_address, ip_network from typing import TYPE_CHECKING, Any from guard import SecurityConfig, SecurityDecorator, SecurityMiddleware @@ -337,21 +338,122 @@ def _redis_url() -> str: # `subnet:` for roboco_default/roboco_data, so this has to cover whatever # docker allocates them. # -# Known ceiling: this can't tell a real docker-bridge peer apart from -# host-loopback/NAT'd traffic landing on the same address family. A request -# proxied through the host (e.g. Tailscale Serve terminating on -# 127.0.0.1:3000) still resolves, after nginx's one XFF hop, to loopback or -# the bridge gateway IP — both inside this range — so it still rides the -# exemption. A second XFF hop ahead of nginx (Tailscale Serve prepends the -# tailnet peer's real IP before nginx appends its own) is silently lost: -# trusted_proxy_depth=1 always peels the RIGHTMOST XFF entry, which is the -# hop nginx itself recorded, not the original tailnet client. +# The variable-depth proxy chain (guard sees a fixed depth) is handled by +# ClientIpResolutionMiddleware below: it recursively skips known local hops +# in X-Forwarded-For and stamps the guard's request.state.client_ip cache, +# so Tailscale-Serve/host-proxied traffic resolves to the real tailnet/LAN +# client instead of loopback and no longer rides this exemption. _INTERNAL_NETWORKS = [ "127.0.0.1", "::1", "172.16.0.0/12", ] +# XFF entries that can legitimately be a HOP nginx recorded in front of the +# real client: loopback (a host-terminated proxy like Tailscale Serve) and +# the docker bridge pool (docker DNAT presents host-originated connections +# as the bridge gateway). Deliberately NOT the LAN/tailnet ranges — a +# 192.168.x / 100.64.x XFF entry IS the client, never a hop. +_TRUSTED_HOP_NETWORKS = ("127.0.0.1/32", "::1/128", "172.16.0.0/12") + +# Tailscale assigns every tailnet node an address in the CGNAT range. The +# resolver stamps ONLY a candidate in this range: it makes the fix exactly as +# wide as the broken case (host-proxied tailnet traffic resolving to a +# whitelisted hop IP) and no wider — for every other chain shape the stamp +# abstains and the guard's own depth-1 logic decides, so a same-bridge +# container relaying a forged public-IP prefix through nginx still resolves +# to its real bridge IP exactly as before this fix. Residual (accepted): such +# a container can forge a 100.64/10 prefix — that only DE-privileges it +# (loses its whitelist exemption; the fake tailnet IP eats the WAF/bans). +_TAILNET_NETWORK = "100.64.0.0/10" + +# The fixable shape needs at least [client, hop] — one real entry behind one +# recorded proxy hop. +_MIN_CHAIN_ENTRIES = 2 + + +def _in_networks(ip: str, networks: tuple[str, ...]) -> bool: + try: + addr = ip_address(ip.strip()) + return any(addr in ip_network(net) for net in networks) + except (ValueError, TypeError): + return False + + +def _is_trusted_hop(ip: str) -> bool: + return _in_networks(ip, _TRUSTED_HOP_NETWORKS) + + +def resolve_forwarded_client_ip(forwarded_for: str) -> str | None: + """Resolve the tailnet client behind host-proxy hops; None = abstain. + + fastapi-guard peels a FIXED number of XFF hops (trusted_proxy_depth=1: + the rightmost entry, which nginx itself recorded). That is correct for + every chain except one: host-proxied tailnet traffic (Tailscale Serve → + nginx) arrives as ``[tailnet-client, ]``, so + depth-1 resolves it to a whitelisted hop IP and WAF/ban/rate-limit go + inert for the whole /tg surface (the documented ceiling). + + This resolver fixes exactly that shape and nothing else: peel trusted + hops from the right; the remaining candidate is returned ONLY if at + least one hop was peeled and the candidate is in the tailnet CGNAT + range. Every other shape — direct LAN client, agent container via nginx + (even with a forged public-IP prefix), all-hops operator traffic, + malformed entries — returns None, leaving the guard's own depth-1 + resolution in charge, byte-for-byte identical to before this fix. + """ + entries = [e.strip() for e in forwarded_for.split(",") if e.strip()] + if len(entries) < _MIN_CHAIN_ENTRIES: + return None + idx = len(entries) - 1 + while idx >= 0 and _is_trusted_hop(entries[idx]): + idx -= 1 + if idx == len(entries) - 1 or idx < 0: + return None # no hop peeled, or all hops: baseline handles both + candidate = entries[idx] + if not _in_networks(candidate, (_TAILNET_NETWORK,)): + return None + return candidate + + +class ClientIpResolutionMiddleware: + """Stamp the guard's ``request.state.client_ip`` cache with the real + client resolved across the variable-depth local proxy chain. + + Pure ASGI, mounted OUTSIDE SecurityMiddleware (added after it, so it runs + first): guard_core's ``extract_client_ip`` returns a pre-cached + ``state.client_ip`` verbatim, which is the supported seam for custom + resolution. Only honors XFF when the CONNECTING peer is itself a known + local hop (nginx's bridge IP / loopback) — a directly-connected client's + forged XFF is never consulted here (the guard's own depth-1 logic keeps + handling that class unchanged). + """ + + def __init__(self, app: Any) -> None: + self.app = app + + async def __call__(self, scope: Any, receive: Any, send: Any) -> None: + if scope["type"] == "http": + client = scope.get("client") + connecting_ip = client[0] if client else None + if connecting_ip and _is_trusted_hop(connecting_ip): + # First occurrence on a repeated header, matching Starlette's + # Headers.get — so this layer and the guard's own fallback + # read the SAME header value. + forwarded_for = next( + ( + v.decode("latin-1") + for k, v in scope.get("headers", []) + if k.decode("latin-1").lower() == "x-forwarded-for" + ), + None, + ) + if forwarded_for: + resolved = resolve_forwarded_client_ip(forwarded_for) + if resolved: + scope.setdefault("state", {})["client_ip"] = resolved + await self.app(scope, receive, send) + def _guard_whitelist() -> list[str]: extra = [ @@ -449,6 +551,9 @@ def apply_guard(app: FastAPI) -> None: return try: app.add_middleware(SecurityMiddleware, config=security_config) + # Added AFTER SecurityMiddleware so it wraps it (runs first) and can + # stamp state.client_ip before the guard's extraction reads it. + app.add_middleware(ClientIpResolutionMiddleware) app.state.guard_decorator = guard_deco logger.info( "fastapi-guard armed", diff --git a/tests/unit/test_security_client_ip.py b/tests/unit/test_security_client_ip.py new file mode 100644 index 00000000..079d0791 --- /dev/null +++ b/tests/unit/test_security_client_ip.py @@ -0,0 +1,213 @@ +"""Host-proxied tailnet client-IP resolution for the guard. + +fastapi-guard peels a fixed trusted_proxy_depth=1 from X-Forwarded-For (the +rightmost entry, which nginx itself recorded). That is correct for every +chain except host-proxied tailnet traffic (Tailscale Serve → nginx), which +arrives as ``[tailnet-client, ]`` — depth-1 +resolves it to a whitelisted hop IP and the WAF goes inert for /tg. +``ClientIpResolutionMiddleware`` stamps guard_core's ``state.client_ip`` +cache (the supported pre-resolution seam) for EXACTLY that shape and +abstains on every other, so no path resolves differently from the depth-1 +baseline unless the candidate is a tailnet CGNAT address behind real hops. +""" + +from __future__ import annotations + +from typing import Any, ClassVar + +import pytest +from fastapi import FastAPI +from guard.adapters import StarletteGuardRequest +from guard_core.utils import extract_client_ip +from roboco import security +from roboco.config import settings +from roboco.security import ( + ClientIpResolutionMiddleware, + resolve_forwarded_client_ip, +) +from starlette.requests import Request + +# --------------------------------------------------------------------------- +# resolve_forwarded_client_ip — stamps ONLY the tailnet-behind-hops shape +# --------------------------------------------------------------------------- + + +def test_tailscale_serve_behind_loopback_resolves_tailnet_peer() -> None: + assert ( + resolve_forwarded_client_ip("100.101.102.103, 127.0.0.1") == "100.101.102.103" + ) + + +def test_tailscale_serve_behind_bridge_gateway_resolves_tailnet_peer() -> None: + # Docker DNAT presents host-originated connections as the bridge gateway, + # so nginx may record 172.x instead of loopback for Tailscale Serve. + assert ( + resolve_forwarded_client_ip("100.101.102.103, 172.18.0.1") == "100.101.102.103" + ) + + +def test_forged_prefix_behind_tailscale_chain_ignored() -> None: + assert ( + resolve_forwarded_client_ip("6.6.6.6, 100.101.102.103, 127.0.0.1") + == "100.101.102.103" + ) + + +def test_lan_client_single_entry_abstains() -> None: + # Depth-1 already resolves this correctly; the resolver must not engage. + assert resolve_forwarded_client_ip("192.168.1.50") is None + + +def test_bridge_peer_with_forged_public_prefix_abstains() -> None: + # THE regression case: a same-bridge container relays through nginx with + # a forged public-IP prefix; nginx appends the container's real 172.x. + # Content-based recursion would hand the attacker "9.9.9.9" — the + # resolver must abstain so the guard's depth-1 keeps the real bridge IP. + assert resolve_forwarded_client_ip("9.9.9.9, 172.20.0.7") is None + + +def test_bridge_peer_forging_tailnet_prefix_only_deprivileges() -> None: + # Documented residual: forging a CGNAT prefix IS stamped — the forger + # loses its whitelist exemption (fake tailnet IPs eat the WAF); it can + # never gain privilege this way. + assert resolve_forwarded_client_ip("100.99.1.1, 172.20.0.7") == "100.99.1.1" + + +def test_non_tailnet_client_behind_hop_abstains() -> None: + # A public/LAN client behind a genuine hop is left to the baseline. + assert resolve_forwarded_client_ip("203.0.113.9, 127.0.0.1") is None + assert resolve_forwarded_client_ip("192.168.1.50, 127.0.0.1") is None + + +def test_all_hops_chain_abstains() -> None: + # Operator curl on the host: baseline resolves to a whitelisted hop + # already; nothing to fix, so abstain. + assert resolve_forwarded_client_ip("127.0.0.1") is None + assert resolve_forwarded_client_ip("172.18.0.1, 127.0.0.1") is None + + +def test_malformed_entries_abstain() -> None: + assert resolve_forwarded_client_ip("not-an-ip, 127.0.0.1") is None + assert resolve_forwarded_client_ip("") is None + assert resolve_forwarded_client_ip(" , ") is None + assert resolve_forwarded_client_ip("100.99.1.1:443, 127.0.0.1") is None + + +# --------------------------------------------------------------------------- +# ClientIpResolutionMiddleware stamping +# --------------------------------------------------------------------------- + + +async def _run_middleware(scope: dict[str, Any]) -> dict[str, Any]: + captured: dict[str, Any] = {} + + async def app(inner_scope: Any, _receive: Any, _send: Any) -> None: + captured.update(inner_scope) + + async def receive() -> dict[str, Any]: # pragma: no cover - never called + return {} + + async def send(_message: Any) -> None: # pragma: no cover - never called + return None + + await ClientIpResolutionMiddleware(app)(scope, receive, send) + return captured + + +@pytest.mark.asyncio +async def test_stamps_state_for_trusted_connecting_hop() -> None: + scope = { + "type": "http", + "client": ("172.18.0.5", 1234), # nginx on the docker bridge + "headers": [(b"x-forwarded-for", b"100.101.102.103, 127.0.0.1")], + } + seen = await _run_middleware(scope) + assert seen["state"]["client_ip"] == "100.101.102.103" + + +@pytest.mark.asyncio +async def test_untrusted_connecting_peer_is_not_consulted() -> None: + # A directly-connected client's forged XFF must not be honored here. + scope = { + "type": "http", + "client": ("192.168.1.9", 1234), + "headers": [(b"x-forwarded-for", b"100.99.1.1, 127.0.0.1")], + } + seen = await _run_middleware(scope) + assert "state" not in seen or "client_ip" not in seen.get("state", {}) + + +@pytest.mark.asyncio +async def test_duplicate_forwarded_headers_use_first_occurrence() -> None: + # Starlette's Headers.get returns the FIRST occurrence — this layer must + # read the same value the guard's own fallback would. + scope = { + "type": "http", + "client": ("172.18.0.5", 1234), + "headers": [ + (b"x-forwarded-for", b"100.101.102.103, 127.0.0.1"), + (b"x-forwarded-for", b"100.66.6.6, 127.0.0.1"), + ], + } + seen = await _run_middleware(scope) + assert seen["state"]["client_ip"] == "100.101.102.103" + + +@pytest.mark.asyncio +async def test_abstain_stamps_nothing() -> None: + scope = { + "type": "http", + "client": ("172.18.0.5", 1234), + "headers": [(b"x-forwarded-for", b"9.9.9.9, 172.20.0.7")], + } + seen = await _run_middleware(scope) + assert "state" not in seen or "client_ip" not in seen.get("state", {}) + + +@pytest.mark.asyncio +async def test_non_http_scope_passthrough() -> None: + scope = {"type": "websocket", "client": ("172.18.0.5", 1234)} + seen = await _run_middleware(scope) + assert "state" not in seen + + +# --------------------------------------------------------------------------- +# Wiring: mount presence AND order (resolver must run before the guard) +# --------------------------------------------------------------------------- + + +def test_apply_guard_mounts_resolver_outermost( + monkeypatch: pytest.MonkeyPatch, +) -> None: + monkeypatch.setattr(settings, "guard_enabled", True) + app = FastAPI() + security.apply_guard(app) + names = [getattr(m.cls, "__name__", str(m.cls)) for m in app.user_middleware] + assert "ClientIpResolutionMiddleware" in names + assert "SecurityMiddleware" in names + # Starlette runs user_middleware in list order (index 0 = outermost), so + # the resolver must sit BEFORE the guard or the stamp arrives too late. + assert names.index("ClientIpResolutionMiddleware") < names.index( + "SecurityMiddleware" + ) + + +@pytest.mark.asyncio +async def test_guard_extract_honors_stamped_state() -> None: + """End-to-end seam check: guard_core's extractor returns the stamp.""" + scope = { + "type": "http", + "method": "GET", + "path": "/tg", + "query_string": b"", + "headers": [(b"x-forwarded-for", b"100.101.102.103, 127.0.0.1")], + "client": ("172.18.0.5", 1234), + "state": {"client_ip": "100.101.102.103"}, + } + request = StarletteGuardRequest(Request(scope)) + + class _Cfg: + trusted_proxies: ClassVar[list[str]] = ["127.0.0.1", "172.16.0.0/12"] + trusted_proxy_depth = 1 + + assert await extract_client_ip(request, _Cfg()) == "100.101.102.103"