dashboard-auth: trust X-Forwarded-For only from a loopback peer, and only its last hop

hermes_cli/dashboard_auth/base.py

@@ -1,11 +1,33 @@
 """Abstract base + dataclasses + exceptions for dashboard auth providers."""
 from __future__ import annotations
 
+import ipaddress
 from abc import ABC, abstractmethod
 from dataclasses import dataclass
 from typing import Optional
 
 
+def is_loopback_peer(peer: str) -> bool:
+    """True when ``peer`` is this machine talking to itself.
+
+    Gates whether X-Forwarded-For may be believed at all: only a request that
+    genuinely arrived through the local reverse proxy has a trustworthy
+    appended hop.  Lives here so the three copies of ``_client_ip``
+    (``routes``, ``token_auth``, ``middleware``) cannot drift apart — they are
+    three implementations of one security decision.
+
+    Must not be a literal ``("127.0.0.1", "::1")`` membership test: under a
+    dual-stack bind (``--host ::``) an IPv4 loopback proxy peer arrives as the
+    IPv4-mapped ``::ffff:127.0.0.1``, which such a test rejects — discarding
+    XFF and collapsing every client into a single throttle bucket.
+    """
+    try:
+        return ipaddress.ip_address(peer).is_loopback
+    except ValueError:
+        # Not an IP literal (unix socket, empty, hostname) — never trusted.
+        return False
+
+
 @dataclass(frozen=True)
 class Session:
     """A verified identity. Returned by ``complete_login`` and ``verify_session``.

hermes_cli/dashboard_auth/middleware.py

@@ -28,6 +28,7 @@
     DashboardAuthProvider,
     ProviderError,
     RefreshExpiredError,
+    is_loopback_peer,
 )
 from hermes_cli.dashboard_auth.cookies import (
     clear_sso_attempt_cookie,
@@ -87,10 +88,23 @@ def _path_is_public(path: str) -> bool:
 
 
 def _client_ip(request: Request) -> str:
+    """Best-effort client IP for the ``ip=`` field on gate audit events.
+
+    Mirrors ``routes._client_ip`` / ``token_auth._client_ip``: X-Forwarded-For
+    is only consulted when the connection peer is loopback (the request came
+    through the local reverse proxy), and then the LAST hop is taken — the
+    element that proxy appended.  Trusting the client-supplied first element
+    lets any caller forge the source IP on every SESSION_VERIFY_FAILURE,
+    REFRESH_FAILURE and LOGIN_START record this module writes.
+    """
+    peer = request.client.host if request.client else ""
+    if not is_loopback_peer(peer):
+        return peer
     fwd = request.headers.get("x-forwarded-for", "")
-    if fwd:
-        return fwd.split(",")[0].strip()
-    return request.client.host if request.client else ""
+    # Drop empty hops so a trailing separator can't bucket every caller
+    # under one shared key.
+    hops = [h.strip() for h in fwd.split(",") if h.strip()]
+    return hops[-1] if hops else peer
 
 
 def _ordered_session_providers(

hermes_cli/dashboard_auth/routes.py

@@ -35,6 +35,7 @@
     InvalidCodeError,
     InvalidCredentialsError,
     ProviderError,
+    is_loopback_peer,
 )
 from hermes_cli.dashboard_auth.cookies import (
     clear_pkce_cookie,
@@ -106,10 +107,28 @@ def _redirect_uri(request: Request) -> str:
 
 
 def _client_ip(request: Request) -> str:
+    """Best-effort client IP: audit ``ip=`` field and login-throttle bucket key.
+
+    Only honours X-Forwarded-For when the connection peer is loopback, i.e.
+    the request actually arrived through the local reverse proxy, and then
+    takes the LAST hop — the one that proxy appended, and the only element
+    it cannot be made to lie about.
+    Trusting the FIRST element instead lets any client mint an arbitrary
+    value: rotating the header hands out a fresh ``_password_rate_limited``
+    bucket per request (defeating the only brute-force control in front of
+    the scrypt verify) and forges the ip on every auth audit event.
+    uvicorn's ``proxy_headers`` (web_server.py, in ``uvicorn.Config``) does
+    not cover this — it rewrites ``request.client``, while this reads the raw
+    header.
+    """
+    peer = request.client.host if request.client else ""
+    if not is_loopback_peer(peer):
+        return peer
     fwd = request.headers.get("x-forwarded-for", "")
-    if fwd:
-        return fwd.split(",")[0].strip()
-    return request.client.host if request.client else ""
+    # A proxy may emit a trailing separator; an empty last element would
+    # otherwise collapse every caller into one shared throttle bucket.
+    hops = [h.strip() for h in fwd.split(",") if h.strip()]
+    return hops[-1] if hops else peer
 
 
 def _prefix(request: Request) -> str:
@@ -601,10 +620,11 @@ def _validate_post_login_target(raw: str) -> str:
 # password we verify locally, so it's a credential-stuffing target. A
 # simple in-process sliding-window limiter per client IP raises the cost
 # of online guessing without any external dependency. It is intentionally
-# best-effort: process-local (resets on restart), and behind a trusting
-# proxy the IP is the proxy's unless X-Forwarded-For is set — which is why
-# this is defence-in-depth on top of the provider's own constant-time
-# verify, not the only line of defence.
+# best-effort: process-local (resets on restart), and only as granular as
+# ``_client_ip`` can safely make it — behind a proxy that appends no
+# X-Forwarded-For every remote client shares the proxy's single bucket —
+# which is why this is defence-in-depth on top of the provider's own
+# constant-time verify, not the only line of defence.
 
 _PW_RATE_MAX_ATTEMPTS = 10
 _PW_RATE_WINDOW_SEC = 60.0

hermes_cli/dashboard_auth/token_auth.py

@@ -47,7 +47,11 @@
 
 from hermes_cli.dashboard_auth import list_token_providers
 from hermes_cli.dashboard_auth.audit import AuditEvent, audit_log
-from hermes_cli.dashboard_auth.base import ProviderError, TokenPrincipal
+from hermes_cli.dashboard_auth.base import (
+    ProviderError,
+    TokenPrincipal,
+    is_loopback_peer,
+)
 
 _log = logging.getLogger(__name__)
 
@@ -81,10 +85,22 @@ def clear_token_routes() -> None:
 
 
 def _client_ip(request: Request) -> str:
+    """Best-effort client IP for the ``ip=`` field on TOKEN_AUTH_FAILURE events.
+
+    Mirrors ``routes._client_ip``: X-Forwarded-For is only consulted when the
+    connection peer is loopback (the request came through the local reverse
+    proxy), and then the LAST hop is taken — the element that proxy appended.
+    A client-supplied first element is attacker-controlled, so trusting it
+    lets a caller forge the source IP on every failed-token audit record.
+    """
+    peer = request.client.host if request.client else ""
+    if not is_loopback_peer(peer):
+        return peer
     fwd = request.headers.get("x-forwarded-for", "")
-    if fwd:
-        return fwd.split(",")[0].strip()
-    return request.client.host if request.client else ""
+    # Drop empty hops so a trailing separator can't bucket every caller
+    # under one shared key.
+    hops = [h.strip() for h in fwd.split(",") if h.strip()]
+    return hops[-1] if hops else peer
 
 
 def extract_bearer_token(request: Request) -> str:

tests/hermes_cli/test_dashboard_auth_xff_trusted_peer.py

@@ -0,0 +1,198 @@
+"""X-Forwarded-For is only trusted from a loopback peer, and only its LAST hop.
+
+``_client_ip`` is the sole bucket key for the ``/auth/password-login``
+brute-force throttle (``routes._password_rate_limited``) and the ``ip=`` field
+on every auth audit event. It previously took ``fwd.split(",")[0]`` — the
+first element, which is entirely client-supplied — so rotating the header
+handed out a fresh 10-per-60s bucket per request and forged the audit trail.
+
+Both copies of the helper (``routes`` and ``token_auth``) must:
+  * ignore X-Forwarded-For entirely when the connection peer is NOT loopback;
+  * take the LAST hop when the peer IS loopback (the single local reverse
+    proxy appends it, so it is the one element a client cannot forge).
+"""
+from __future__ import annotations
+
+import pytest
+
+from fastapi.testclient import TestClient
+from starlette.requests import Request
+
+from hermes_cli import web_server
+from hermes_cli.dashboard_auth import clear_providers
+from hermes_cli.dashboard_auth import middleware, token_auth
+from hermes_cli.dashboard_auth.routes import (
+    _PW_RATE_MAX_ATTEMPTS,
+    _client_ip,
+    _reset_password_rate_limit,
+)
+
+
+# THREE call sites carry an identical helper; every case below must hold for
+# each of them, so they are parametrized rather than duplicated.
+#
+# middleware._client_ip was missing from this list while this PR patched it,
+# so the one copy that feeds the audit log's ip= field on every authenticated
+# request was changed with nothing exercising it. A helper tuple that silently
+# lags the call sites is worse than no parametrization, because the coverage it
+# implies is the reason nobody looks again.
+_HELPERS = (_client_ip, token_auth._client_ip, middleware._client_ip)
+
+
+def _req(peer, xff=None) -> Request:
+    """A real Starlette Request with the given connection peer and XFF."""
+    headers = []
+    if xff is not None:
+        headers.append((b"x-forwarded-for", xff.encode()))
+    return Request(
+        {
+            "type": "http",
+            "http_version": "1.1",
+            "method": "POST",
+            "path": "/auth/password-login",
+            "raw_path": b"/auth/password-login",
+            "query_string": b"",
+            "root_path": "",
+            "scheme": "https",
+            "headers": headers,
+            "client": peer,
+            "server": ("testserver", 443),
+        }
+    )
+
+
+# ---------------------------------------------------------------------------
+# Helper contract
+# ---------------------------------------------------------------------------
+
+
+@pytest.mark.parametrize("helper", _HELPERS)
+class TestClientIpTrustedPeer:
+    def test_loopback_peer_takes_last_hop_of_spoofed_chain(self, helper):
+        # The client sent two forged hops; the proxy appended the real one.
+        # Only the last element may be believed.
+        req = _req(("127.0.0.1", 51234), "192.0.2.1, 192.0.2.9, 198.51.100.7")
+        assert helper(req) == "198.51.100.7"
+
+    def test_loopback_peer_single_hop(self, helper):
+        req = _req(("127.0.0.1", 51234), "198.51.100.7")
+        assert helper(req) == "198.51.100.7"
+
+    def test_loopback_peer_strips_whitespace(self, helper):
+        req = _req(("127.0.0.1", 51234), "192.0.2.1,   198.51.100.7   ")
+        assert helper(req) == "198.51.100.7"
+
+    def test_loopback_peer_without_xff_falls_back_to_peer(self, helper):
+        assert helper(_req(("127.0.0.1", 51234))) == "127.0.0.1"
+
+    def test_empty_xff_from_loopback_falls_back_to_peer(self, helper):
+        assert helper(_req(("127.0.0.1", 51234), "")) == "127.0.0.1"
+
+    def test_trailing_separator_does_not_yield_empty_bucket_key(self, helper):
+        # A proxy emitting "10.0.0.7," must not resolve to "" — that lands
+        # every such caller in the throttle's shared "_unknown_" bucket.
+        req = _req(("127.0.0.1", 51234), "10.0.0.7,")
+        assert helper(req) == "10.0.0.7"
+
+    def test_ipv6_loopback_is_trusted(self, helper):
+        req = _req(("::1", 51234), "192.0.2.1, 198.51.100.7")
+        assert helper(req) == "198.51.100.7"
+
+    def test_ipv4_mapped_loopback_is_trusted(self, helper):
+        # Under a dual-stack bind (--host ::) the IPv4 loopback proxy peer
+        # arrives IPv4-mapped. A literal ("127.0.0.1", "::1") test rejects
+        # it, discarding XFF and collapsing every client into one bucket.
+        req = _req(("::ffff:127.0.0.1", 51234), "192.0.2.1, 198.51.100.7")
+        assert helper(req) == "198.51.100.7"
+
+    def test_non_loopback_peer_ignores_xff_entirely(self, helper):
+        # Direct (non-proxied) connection: the header is pure client input.
+        req = _req(("203.0.113.9", 44321), "192.0.2.1, 198.51.100.7")
+        assert helper(req) == "203.0.113.9"
+
+    def test_private_lan_peer_is_not_loopback(self, helper):
+        # Only the local proxy is trusted — not "anything that looks internal".
+        req = _req(("192.168.1.50", 44321), "192.0.2.1")
+        assert helper(req) == "192.168.1.50"
+
+    def test_no_client_returns_empty(self, helper):
+        assert helper(_req(None, "192.0.2.1")) == ""
+
+    def test_no_client_ignores_xff(self, helper):
+        # No discernible peer → the throttle's shared "_unknown_" bucket,
+        # never an attacker-chosen one.
+        assert helper(_req(None, "192.0.2.1, 198.51.100.7")) == ""
+
+
+# ---------------------------------------------------------------------------
+# End-to-end: the login throttle can no longer be reset by header rotation
+# ---------------------------------------------------------------------------
+
+
+@pytest.fixture
+def login_client():
+    clear_providers()
+    _reset_password_rate_limit()
+    prev_host = getattr(web_server.app.state, "bound_host", None)
+    prev_port = getattr(web_server.app.state, "bound_port", None)
+    prev_required = getattr(web_server.app.state, "auth_required", None)
+    web_server.app.state.bound_host = "fly-app.fly.dev"
+    web_server.app.state.bound_port = 443
+    web_server.app.state.auth_required = True
+    yield
+    clear_providers()
+    _reset_password_rate_limit()
+    web_server.app.state.bound_host = prev_host
+    web_server.app.state.bound_port = prev_port
+    web_server.app.state.auth_required = prev_required
+
+
+def _client(peer):
+    return TestClient(
+        web_server.app, base_url="https://fly-app.fly.dev", client=peer
+    )
+
+
+def _attempt(client, xff):
+    # No provider is registered, so every allowed attempt 404s; the throttle
+    # is evaluated before provider lookup, so 429 still marks bucket
+    # exhaustion. Keeps the test about bucketing, not about credentials.
+    return client.post(
+        "/auth/password-login",
+        json={"provider": "nope", "username": "admin", "password": "x"},
+        headers={"X-Forwarded-For": xff},
+    )
+
+
+class TestThrottleBucketing:
+    def test_rotating_spoofed_first_hop_shares_one_bucket(self, login_client):
+        # Proxied request: attacker varies the forgeable prefix on every
+        # attempt but the proxy-appended last hop is constant, so all
+        # attempts must land in ONE bucket and exhaust it.
+        client = _client(("127.0.0.1", 51234))
+        codes = [
+            _attempt(client, f"10.0.0.{i}, 198.51.100.5").status_code
+            for i in range(_PW_RATE_MAX_ATTEMPTS + 2)
+        ]
+        assert codes[:_PW_RATE_MAX_ATTEMPTS] == [404] * _PW_RATE_MAX_ATTEMPTS
+        assert codes[_PW_RATE_MAX_ATTEMPTS:] == [429, 429]
+
+    def test_distinct_last_hops_get_distinct_buckets(self, login_client):
+        # The flip side: two genuinely different clients behind the proxy
+        # must not share a budget.
+        client = _client(("127.0.0.1", 51234))
+        for _ in range(_PW_RATE_MAX_ATTEMPTS):
+            _attempt(client, "198.51.100.5")
+        assert _attempt(client, "198.51.100.5").status_code == 429
+        assert _attempt(client, "198.51.100.6").status_code == 404
+
+    def test_non_loopback_peer_rotation_does_not_reset_bucket(self, login_client):
+        # Unproxied request: XFF is ignored outright, so a fully rotated
+        # header still buckets on the real connection peer.
+        client = _client(("203.0.113.9", 44321))
+        codes = [
+            _attempt(client, f"10.0.0.{i}, 172.16.0.{i}").status_code
+            for i in range(_PW_RATE_MAX_ATTEMPTS + 2)
+        ]
+        assert codes[:_PW_RATE_MAX_ATTEMPTS] == [404] * _PW_RATE_MAX_ATTEMPTS
+        assert codes[_PW_RATE_MAX_ATTEMPTS:] == [429, 429]