fix(guard): resolve the tailnet client behind host-proxy hops for the WAF (#646)

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 <rennf93@users.noreply.github.com>
2026-08-03 07:23:24 +02:00 · 2026-07-22 20:29:54 +02:00
parent 98a96bcd21
commit 583d43f213
2 changed files with 327 additions and 9 deletions
@@ -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, <loopback-or-bridge-gateway>]``, 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",
@@ -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, <loopback-or-bridge-gateway>]`` — 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"