diff --git a/hermes_cli/dashboard_auth/base.py b/hermes_cli/dashboard_auth/base.py index e8b8a7730b1d..ff3094f7ad86 100644 --- a/hermes_cli/dashboard_auth/base.py +++ b/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``. diff --git a/hermes_cli/dashboard_auth/middleware.py b/hermes_cli/dashboard_auth/middleware.py index 5b11e98cf2b1..38136073aad4 100644 --- a/hermes_cli/dashboard_auth/middleware.py +++ b/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( diff --git a/hermes_cli/dashboard_auth/routes.py b/hermes_cli/dashboard_auth/routes.py index 0c142963bcc8..f46837316af7 100644 --- a/hermes_cli/dashboard_auth/routes.py +++ b/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 diff --git a/hermes_cli/dashboard_auth/token_auth.py b/hermes_cli/dashboard_auth/token_auth.py index 320b4cdb52b9..18d6fb325562 100644 --- a/hermes_cli/dashboard_auth/token_auth.py +++ b/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: diff --git a/tests/hermes_cli/test_dashboard_auth_xff_trusted_peer.py b/tests/hermes_cli/test_dashboard_auth_xff_trusted_peer.py new file mode 100644 index 000000000000..c98db0929e99 --- /dev/null +++ b/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]