From daa067ad070a2c385d921490368886770d6dcca6 Mon Sep 17 00:00:00 2001 From: Don Beckham Date: Sun, 26 Jul 2026 14:25:48 -0500 Subject: [PATCH] Believe forwarded client addresses only from trusted proxies MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The anonymous rate limit and the upload audit trail are both keyed on the client address, so an address the client can choose is an address that resets the budget and writes fiction into the audit trail. Two layers were each trusting the header on their own: - app/fingerprint.py read X-Forwarded-For whenever TRUST_FORWARDED_FOR was set, and took split(",")[0] — the client-controlled end of the chain, since proxies append. - the container ran uvicorn with --proxy-headers --forwarded-allow-ips "*", which rewrote the peer address from the same header for any caller, before the application saw it. TRUST_FORWARDED_FOR=false did not prevent this; the documented off switch was not an off switch. The app is not published to a host port, so this is not reachable from the internet — the front replaces the header on the way through. It is reachable from the four other containers that share the idp_proxy network: a POST straight to beckham-share-app:8000 with a forged X-Forwarded-For was recorded verbatim in upload_events. Now uvicorn reports the socket peer and the application makes the decision in one place. Forwarded headers are honoured only when the request arrived from a peer in the new TRUSTED_PROXIES setting (addresses, CIDR ranges, or hostnames resolved at runtime, since Docker assigns container addresses), and the chain is read right to left so hops appended by the client are discarded. TRUST_FORWARDED_FOR is replaced by TRUSTED_PROXIES; nothing set it. The browser fingerprint is deliberately left as it is. It is client-supplied by nature, and it is OR-ed with the address rather than substituted for it, so rotating it cannot buy a fresh budget while the address key still matches. Its job is to catch one device rotating addresses. Documented in docs/SECURITY.md so the distinction is not mistaken for an oversight. Verified on a staging container: an untrusted neighbour's forged header is now ignored and the request is attributed to the address it came from; a trusted peer's header is still honoured; a chain of "forged, real" resolves to the real hop. Eight unit tests cover the same cases. --- .env.example | 5 ++ CHANGELOG.md | 11 +++++ Dockerfile | 8 +++- app/config.py | 10 +++- app/fingerprint.py | 76 +++++++++++++++++++++++++---- docs/CONFIGURATION.md | 2 +- docs/SECURITY.md | 23 +++++++-- tests/backend/test_client_ip.py | 84 +++++++++++++++++++++++++++++++++ 8 files changed, 201 insertions(+), 18 deletions(-) create mode 100644 tests/backend/test_client_ip.py diff --git a/.env.example b/.env.example index 83e4b94..4a0a07b 100644 --- a/.env.example +++ b/.env.example @@ -13,6 +13,11 @@ SOURCE_URL=https://github.com/dabeckham/beckham-share SECRET_KEY=CHANGE_ME DB_PASSWORD=CHANGE_ME +# Peers allowed to name the client via X-Forwarded-For / X-Real-IP: addresses, +# CIDR ranges, or hostnames. Anything arriving from elsewhere is attributed to +# the address it came from. Leave empty if the app is exposed directly. +TRUSTED_PROXIES=idp-caddy + # ── OIDC (Authentik) — filled in after scripts/setup-authentik.py runs ────── OIDC_DISCOVERY_URL=https://auth.beckham.ai/application/o/beckham-share/.well-known/openid-configuration OIDC_CLIENT_ID= diff --git a/CHANGELOG.md b/CHANGELOG.md index 90ecdb0..f8d8f9a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,17 @@ pre-1.0 scheme; dates are when the change reached `main`. ## Unreleased +### Security +- The client address behind the anonymous rate limit and the upload audit trail + is no longer take-your-word-for-it. Forwarded headers are honoured only from + peers listed in the new `TRUSTED_PROXIES` setting, and `X-Forwarded-For` is + read right to left so hops appended by the client are discarded. Previously + any caller that could open a socket to the app could name itself, which reset + the rate-limit budget and wrote a false address into the audit trail; + `TRUST_FORWARDED_FOR=false` did not prevent it, because the server was + rewriting the peer address before the application saw it. That setting is + replaced by `TRUSTED_PROXIES`. + ### Added - Full reference documentation under `docs/`: architecture, configuration, operations runbook, security model, and API reference, with a docs index and diff --git a/Dockerfile b/Dockerfile index 4429c00..2438296 100644 --- a/Dockerfile +++ b/Dockerfile @@ -23,5 +23,9 @@ EXPOSE 8000 HEALTHCHECK --interval=30s --timeout=5s --start-period=10s \ CMD curl -fsS http://localhost:8000/healthz || exit 1 -CMD ["uvicorn", "app.main:app", "--host", "0.0.0.0", "--port", "8000", \ - "--proxy-headers", "--forwarded-allow-ips", "*"] +# No --proxy-headers: it would rewrite the peer address from X-Forwarded-For +# before the app sees it, and with --forwarded-allow-ips "*" it did so for any +# caller. Deciding whose forwarded headers to believe belongs in one place — +# see TRUSTED_PROXIES and app/fingerprint.py — so uvicorn reports the socket +# peer and the app does the rest. +CMD ["uvicorn", "app.main:app", "--host", "0.0.0.0", "--port", "8000"] diff --git a/app/config.py b/app/config.py index 838b4dd..e4f3230 100644 --- a/app/config.py +++ b/app/config.py @@ -63,8 +63,14 @@ class Settings(BaseSettings): smtp_from: str = "Beckham Share " smtp_use_tls: bool = True - # Whether to trust X-Forwarded-For (true when behind the Caddy/HAProxy front). - trust_forwarded_for: bool = True + # ── Reverse proxy ──────────────────────────────────────────────────── + # Which peers are allowed to tell us who the client is. Comma-separated + # addresses, CIDR ranges, or resolvable hostnames; hostnames are looked up + # at runtime because container addresses are assigned by Docker. A request + # arriving from anywhere else is attributed to the address it actually came + # from, whatever X-Forwarded-For / X-Real-IP it carries. Leave empty to + # trust nothing. `idp-caddy` is the shared front this app sits behind. + trusted_proxies: str = "idp-caddy" @property def email_enabled(self) -> bool: diff --git a/app/fingerprint.py b/app/fingerprint.py index 67d2a96..0b6ae2a 100644 --- a/app/fingerprint.py +++ b/app/fingerprint.py @@ -8,23 +8,83 @@ from __future__ import annotations import hashlib +import ipaddress import json +import logging +import socket +import time from fastapi import Request from ua_parser import user_agent_parser from .config import settings +log = logging.getLogger("beckham_share.fingerprint") + +# Hostnames in TRUSTED_PROXIES resolve to container addresses that Docker can +# reassign, so the parsed list is rebuilt periodically. It is also rebuilt +# whenever the setting itself changes, which keeps it honest under test. +_TRUST_TTL_SECONDS = 60.0 +_trust_cache: tuple[str, float, list] = ("", -_TRUST_TTL_SECONDS, []) + + +def _trusted_networks() -> list: + global _trust_cache + configured = settings.trusted_proxies + key, stamp, nets = _trust_cache + if key == configured and time.monotonic() - stamp < _TRUST_TTL_SECONDS: + return nets + + nets = [] + for entry in (e.strip() for e in configured.split(",")): + if not entry: + continue + try: + nets.append(ipaddress.ip_network(entry, strict=False)) + continue + except ValueError: + pass # not an address or range — try resolving it as a hostname + try: + for info in socket.getaddrinfo(entry, None): + nets.append(ipaddress.ip_network(info[4][0])) + except OSError: + log.warning("TRUSTED_PROXIES: cannot resolve %r; ignoring it", entry) + _trust_cache = (configured, time.monotonic(), nets) + return nets + + +def _is_trusted(address: str | None) -> bool: + if not address: + return False + try: + parsed = ipaddress.ip_address(address) + except ValueError: + return False + return any(parsed in net for net in _trusted_networks()) + def client_ip(request: Request) -> str | None: - if settings.trust_forwarded_for: - xff = request.headers.get("x-forwarded-for") - if xff: - return xff.split(",")[0].strip() - xri = request.headers.get("x-real-ip") - if xri: - return xri.strip() - return request.client.host if request.client else None + """The client's address, believed only as far as the infrastructure vouches. + + Proxy headers are honoured only when the request actually arrived from a + trusted proxy — otherwise anyone who can open a socket to the app could + claim any address, which would reset the anonymous rate-limit budget and + poison the audit trail. ``X-Forwarded-For`` is read right to left: trusted + hops are skipped and the first untrusted address is the closest one that a + trusted proxy actually observed. Anything further left was appended by the + client and means nothing. + """ + peer = request.client.host if request.client else None + if not _is_trusted(peer): + return peer + for hop in reversed(request.headers.get("x-forwarded-for", "").split(",")): + hop = hop.strip() + if hop and not _is_trusted(hop): + return hop + real_ip = request.headers.get("x-real-ip") + if real_ip: + return real_ip.strip() + return peer def parse_ua(user_agent: str | None) -> dict[str, str | None]: diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index b34c943..054bfd6 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -24,7 +24,7 @@ ignored. Booleans accept `true`/`false`/`1`/`0`. | Variable | Default | Purpose | |---|---|---| | `SECRET_KEY` | `dev-insecure-change-me` | Signs the session cookie. **Must** be a long random value in production — rotating it logs everyone out. | -| `TRUST_FORWARDED_FOR` | `true` | Trust `X-Forwarded-For` / `X-Real-IP` for the client IP. Correct when behind the Caddy/HAProxy front; set `false` only if the app is exposed directly. | +| `TRUSTED_PROXIES` | `idp-caddy` | Peers allowed to name the client through `X-Forwarded-For` / `X-Real-IP`. Comma-separated addresses, CIDR ranges, or hostnames (resolved at runtime, since container addresses are assigned by Docker). Requests from anywhere else are attributed to the address they arrived from, whatever headers they carry. Leave empty to trust nothing — correct when the app is exposed directly. | ## Storage & database diff --git a/docs/SECURITY.md b/docs/SECURITY.md index 75e5940..09f37b1 100644 --- a/docs/SECURITY.md +++ b/docs/SECURITY.md @@ -41,7 +41,12 @@ fully buffered. Members have a separate, larger cap (`MAX_UPLOAD_BYTES`). - `ANON_UPLOADS_PER_DAY` (default 20) in the last 24 hours. The count is taken from the `upload_events` audit rows, keyed by **IP _or_ -fingerprint** (`OR`), so rotating just one signal does not reset the budget. +fingerprint** (`OR`), so rotating just one signal does not reset the budget. The +two keys carry different weight on purpose: the address is established by the +infrastructure (see `TRUSTED_PROXIES` below), while the browser fingerprint is +supplied by the client and can be changed at will. The fingerprint's job is to +catch one device rotating addresses; it is an additional key, never a +substitute for the address. Because it counts rows that already exist in the database, the limit holds across worker processes and restarts with no separate store. Over-budget requests get `429` with a `Retry-After` header. @@ -57,9 +62,17 @@ Every upload writes an `upload_events` row for review — see below. `app/fingerprint.py` records, per upload: -- **Client IP** — from `X-Forwarded-For` / `X-Real-IP` when `TRUST_FORWARDED_FOR` - is set (the real client IP is preserved through HAProxy's PROXY protocol), - otherwise the socket peer. +- **Client IP** — the address the request arrived from, unless it arrived from a + peer listed in `TRUSTED_PROXIES`, in which case that peer's + `X-Forwarded-For` / `X-Real-IP` is believed instead. The real client IP is + preserved to the front through HAProxy's PROXY protocol. + + `X-Forwarded-For` is read **right to left**: trusted hops are skipped and the + first untrusted address is the closest one a trusted proxy actually observed. + Reading left to right would return whatever the client itself put in the + header, because proxies append. This matters because the anonymous rate limit + is keyed on the address — an address the client can choose is an address that + resets the budget. - **User agent** — raw, plus a parsed `browser` / `os` / `device` breakdown for human-readable review. - **Accept-Language**. @@ -106,7 +119,7 @@ Every upload writes an `upload_events` row for review — see below. | Public share page abused as a spam relay | Server-side email is members-only; anonymous uses `mailto:`. | | Oversized-upload resource exhaustion | Streaming write with an early abort + partial-file cleanup. | | Session forgery | Signed session cookie (`SECRET_KEY`); `Secure` + `SameSite=Lax`. | -| Spoofed client IP | Real IP preserved via PROXY protocol; `X-Forwarded-For` trusted only behind the front. | +| Spoofed client IP (to reset the rate limit or poison the audit trail) | Forwarded headers are honoured only from peers in `TRUSTED_PROXIES`, and the chain is read right to left so client-appended hops are discarded. Everything else is attributed to the address it arrived from. | ## 7. Operational guidance diff --git a/tests/backend/test_client_ip.py b/tests/backend/test_client_ip.py new file mode 100644 index 0000000..41ac565 --- /dev/null +++ b/tests/backend/test_client_ip.py @@ -0,0 +1,84 @@ +"""Whose word we take for the client's address. + +The anonymous rate limit and the upload audit trail are both keyed on the +client IP, so an address the client gets to choose is an address that resets +the budget. These cover which peers may speak for a client, and how a +forwarded chain is read once one of them does. +""" +import pytest +from starlette.requests import Request + +from app import fingerprint +from app.config import settings + +TRUSTED = "10.9.0.1" + + +def _request(peer, **headers) -> Request: + raw = [(k.replace("_", "-").encode(), v.encode()) for k, v in headers.items()] + return Request({"type": "http", "headers": raw, "client": (peer, 51234) if peer else None}) + + +@pytest.fixture +def behind_proxy(monkeypatch): + """Trust one address, the way the deployment trusts the shared front.""" + monkeypatch.setattr(settings, "trusted_proxies", TRUSTED) + + +def test_forwarded_header_from_an_untrusted_peer_is_ignored(behind_proxy): + # A neighbour on the container network can reach the app directly. It does + # not get to name the client. + req = _request("172.23.0.9", x_forwarded_for="203.0.113.5", x_real_ip="203.0.113.5") + assert fingerprint.client_ip(req) == "172.23.0.9" + + +def test_forwarded_header_from_the_trusted_proxy_is_believed(behind_proxy): + req = _request(TRUSTED, x_forwarded_for="203.0.113.5") + assert fingerprint.client_ip(req) == "203.0.113.5" + + +def test_client_supplied_hops_are_discarded(behind_proxy): + # The client sent its own X-Forwarded-For and the proxy appended the address + # it saw. Reading left to right would return the client's invention. + req = _request(TRUSTED, x_forwarded_for="198.51.100.1, 203.0.113.5") + assert fingerprint.client_ip(req) == "203.0.113.5" + + +def test_trusted_hops_are_skipped(monkeypatch): + monkeypatch.setattr(settings, "trusted_proxies", "10.9.0.0/24") + req = _request("10.9.0.1", x_forwarded_for="203.0.113.5, 10.9.0.7") + assert fingerprint.client_ip(req) == "203.0.113.5" + + +def test_real_ip_is_used_when_there_is_no_forwarded_chain(behind_proxy): + req = _request(TRUSTED, x_real_ip="203.0.113.5") + assert fingerprint.client_ip(req) == "203.0.113.5" + + +def test_trusting_nothing_falls_back_to_the_socket_peer(monkeypatch): + monkeypatch.setattr(settings, "trusted_proxies", "") + req = _request(TRUSTED, x_forwarded_for="203.0.113.5") + assert fingerprint.client_ip(req) == TRUSTED + + +def test_unresolvable_entries_do_not_grant_trust(monkeypatch): + monkeypatch.setattr(settings, "trusted_proxies", "no-such-host.invalid") + req = _request("172.23.0.9", x_forwarded_for="203.0.113.5") + assert fingerprint.client_ip(req) == "172.23.0.9" + + +def test_rate_limit_survives_forwarded_header_rotation(client): + # conftest sets ANON_UPLOADS_PER_HOUR=3. The test client is not a trusted + # proxy, so rotating the header (and the browser fingerprint with it) must + # not buy a fresh budget. + for i in range(3): + r = client.post("/api/anon-upload", + files={"file": ("a.txt", b"hi", "text/plain")}, + data={"fp": f"fp-{i}"}, + headers={"x-forwarded-for": f"203.0.113.{i}"}) + assert r.status_code == 200 + blocked = client.post("/api/anon-upload", + files={"file": ("a.txt", b"hi", "text/plain")}, + data={"fp": "fp-fresh"}, + headers={"x-forwarded-for": "203.0.113.200"}) + assert blocked.status_code == 429