diff --git a/.env.example b/.env.example index d3574d1..490640e 100644 --- a/.env.example +++ b/.env.example @@ -32,6 +32,10 @@ JWT_ISSUER=fastapi-production-api CORS_ORIGINS=http://localhost:3000 +# Trusted reverse-proxy socket peers. Leave empty when connecting directly. +FORWARDED_ALLOW_IPS= + + # Redis and request rate limiting. Memory mode preserves single-process setup. REDIS_URL=redis://localhost:6379/0 REDIS_CONNECT_TIMEOUT_SECONDS=1 diff --git a/.env.production.example b/.env.production.example index 96c999a..0aad308 100644 --- a/.env.production.example +++ b/.env.production.example @@ -39,6 +39,11 @@ JWT_ISSUER=fastapi-production-api CORS_ORIGINS=https://your-domain.com +# Exact reverse-proxy peers/CIDRs; never use `*`. + +FORWARDED_ALLOW_IPS=127.0.0.1 + + # Redis and distributed request rate limiting REDIS_URL=rediss://app:password@redis.internal.example:6379/0 diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 0b2afca..c63c573 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -22,10 +22,13 @@ flowchart LR Metrics --> App ``` -The reverse proxy terminates TLS and controls trusted forwarding headers. The -application owns validation, authorization, business transactions, and -telemetry. PostgreSQL is required; Redis becomes a required readiness dependency -when distributed rate limiting is enabled. +The reverse proxy terminates TLS and replaces untrusted forwarding headers. +Uvicorn accepts those headers only from the explicit `FORWARDED_ALLOW_IPS` +address/CIDR allowlist, then exposes one canonical ASGI client address to both +request logging and rate limiting. The application owns validation, +authorization, business transactions, and telemetry. PostgreSQL is required; +Redis becomes a required readiness dependency when distributed rate limiting is +enabled. ## Source layout and responsibilities @@ -214,9 +217,10 @@ disabled delivery does not create unreachable tokens. MFA and OIDC transaction data use dedicated encryption keys rather than the JWT signing secret. Redis quota keys contain only versioned HMAC identifiers and bounded fixed-window counters. OIDC cache keys contain a fixed issuer digest, document kind, and -bounded TTL; values contain only public discovery/JWKS JSON. CORS origins, proxy -trust, metrics exposure, database permissions, and secret storage remain -deployment responsibilities. +bounded TTL; values contain only public discovery/JWKS JSON. Proxy trust is +fail-closed by default but its exact CIDRs, along with CORS origins, metrics +exposure, database permissions, and secret storage, remain deployment +responsibilities. ## Safe extension points diff --git a/CHANGELOG.md b/CHANGELOG.md index 9cc22c1..05a062c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,8 @@ project follows [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ### Added +- Explicit trusted-proxy IP/CIDR allowlisting for Uvicorn client-address + resolution, with canonical client IPs shared by rate limiting and request logs - Administrative account disable/re-enable lifecycle with immediate access rejection and atomic refresh-session revocation - Optional Redis-backed fixed-window rate limiting shared across API processes diff --git a/DEPLOYMENT.md b/DEPLOYMENT.md index 9605555..0a05494 100644 --- a/DEPLOYMENT.md +++ b/DEPLOYMENT.md @@ -267,8 +267,7 @@ server { proxy_pass http://127.0.0.1:8000; proxy_http_version 1.1; proxy_set_header Host $host; - proxy_set_header X-Real-IP $remote_addr; - proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; + proxy_set_header X-Forwarded-For $remote_addr; proxy_set_header X-Forwarded-Proto $scheme; } @@ -287,10 +286,21 @@ sudo nginx -t sudo systemctl reload nginx ``` -Only trust forwarded headers from proxies you control. Configure Uvicorn's -forwarded allowlist (for example `FORWARDED_ALLOW_IPS=127.0.0.1` for same-host -Nginx) and firewall the Gunicorn port from untrusted clients. The application -uses the ASGI scope client address and never parses forwarding headers itself. +`FORWARDED_ALLOW_IPS` is the trust boundary for both client IP and forwarded +scheme resolution. Set it to the socket peers that connect directly to +Gunicorn, for example `127.0.0.1` for same-host Nginx or a comma-separated list +of canonical proxy CIDRs. The default is empty, and the application rejects +wildcard, malformed, and non-canonical entries. Never list client networks. + +The single-proxy example overwrites `X-Forwarded-For` with `$remote_addr`, so a +client-supplied value cannot enter the trusted chain. For a controlled +multi-proxy topology, each trusted proxy may append its verified peer address; +list every direct intermediary CIDR and test the chain before deployment. + +Uvicorn validates the socket peer before updating the ASGI client address. Rate +limiting and request logs consume only that canonical ASGI address and do not +parse forwarding headers again. Keep the Gunicorn port firewalled from +untrusted clients even when the allowlist is configured. ## 8. Enable HTTPS @@ -386,6 +396,7 @@ trace-context columns; tracing metadata is not a correctness dependency. - [ ] Run test, lint, and dependency-audit jobs successfully - [ ] Restrict database and service-account permissions - [ ] Restrict the application port to the trusted proxy +- [ ] Set `FORWARDED_ALLOW_IPS` to direct proxy peers only and test spoofed headers - [ ] Enable HTTPS and renewal monitoring - [ ] Configure logs, metrics, alerts, and retention - [ ] Configure database backups and test restoration diff --git a/MONITORING.md b/MONITORING.md index f1087ee..898a779 100644 --- a/MONITORING.md +++ b/MONITORING.md @@ -89,7 +89,9 @@ configuration marks worker gauge files as dead when workers exit. Every application log is a single JSON object containing `timestamp`, `level`, `logger`, `message`, and `request_id`. HTTP completion records also contain -`method`, `path`, `route`, `status_code`, and `duration_ms`. +`method`, canonical `client_ip`, `path`, `route`, `status_code`, and +`duration_ms`. Treat client addresses as personal or security-sensitive data: +restrict log access and choose a retention period appropriate to local policy. When a valid OpenTelemetry span is active, the same JSON record also contains hexadecimal `trace_id` and `span_id` values. This allows operators to correlate diff --git a/gunicorn.conf.py b/gunicorn.conf.py index c03262c..c8d44e1 100644 --- a/gunicorn.conf.py +++ b/gunicorn.conf.py @@ -2,12 +2,18 @@ from prometheus_client import multiprocess +from app.core.config import settings + bind = "0.0.0.0:8000" workers = multiprocessing.cpu_count() * 2 + 1 worker_class = "uvicorn_worker.UvicornWorker" +# Fail closed: forwarded headers are ignored unless deployment explicitly lists +# the socket peers (reverse proxies) that are allowed to supply them. +forwarded_allow_ips = settings.FORWARDED_ALLOW_IPS + timeout = 120 accesslog = "-" diff --git a/src/app/core/client_ip.py b/src/app/core/client_ip.py new file mode 100644 index 0000000..a44312d --- /dev/null +++ b/src/app/core/client_ip.py @@ -0,0 +1,22 @@ +import ipaddress + +from fastapi import Request + + +def resolve_client_ip(request: Request) -> str: + """Return the canonical IP address already resolved by the ASGI server.""" + if request.client is None: + return "unknown" + + if len(request.client.host) > 45: + return "unknown" + + try: + address = ipaddress.ip_address(request.client.host) + except ValueError: + return "unknown" + + if isinstance(address, ipaddress.IPv6Address) and address.ipv4_mapped: + return address.ipv4_mapped.compressed + + return address.compressed diff --git a/src/app/core/config.py b/src/app/core/config.py index 3c9bd41..d3e790f 100644 --- a/src/app/core/config.py +++ b/src/app/core/config.py @@ -1,3 +1,4 @@ +import ipaddress from typing import Literal, Self from urllib.parse import urlsplit @@ -30,6 +31,8 @@ class Settings(BaseSettings): CORS_ORIGINS: str = "" + FORWARDED_ALLOW_IPS: str = "" + REDIS_URL: SecretStr = SecretStr("") REDIS_CONNECT_TIMEOUT_SECONDS: float = Field(default=1.0, gt=0, le=30) @@ -156,6 +159,31 @@ class Settings(BaseSettings): @model_validator(mode="after") def validate_production_settings(self) -> Self: + trusted_proxies = self.FORWARDED_ALLOW_IPS.split(",") + if self.FORWARDED_ALLOW_IPS and any( + not value.strip() for value in trusted_proxies + ): + raise ValueError("FORWARDED_ALLOW_IPS must not contain empty entries") + + for trusted_proxy in trusted_proxies: + trusted_proxy = trusted_proxy.strip() + if not trusted_proxy: + continue + + if trusted_proxy == "*": + raise ValueError("FORWARDED_ALLOW_IPS must not trust every client") + + try: + if "/" in trusted_proxy: + ipaddress.ip_network(trusted_proxy, strict=True) + else: + ipaddress.ip_address(trusted_proxy) + except ValueError as exc: + raise ValueError( + "FORWARDED_ALLOW_IPS must contain only valid IP addresses " + "or canonical CIDR networks" + ) from exc + if self.RATE_LIMIT_BACKEND == "redis" or self.OIDC_CACHE_BACKEND == "redis": redis_url_value = self.REDIS_URL.get_secret_value() redis_url = urlsplit(redis_url_value) diff --git a/src/app/core/logging.py b/src/app/core/logging.py index 0f3c9af..c778d99 100644 --- a/src/app/core/logging.py +++ b/src/app/core/logging.py @@ -49,6 +49,7 @@ def format(self, record: logging.LogRecord) -> str: for field in ( "method", + "client_ip", "path", "route", "status_code", diff --git a/src/app/middlewares/rate_limit.py b/src/app/middlewares/rate_limit.py index 8b12ce3..698d2b6 100644 --- a/src/app/middlewares/rate_limit.py +++ b/src/app/middlewares/rate_limit.py @@ -10,6 +10,7 @@ from fastapi.responses import JSONResponse from redis.exceptions import RedisError +from app.core.client_ip import resolve_client_ip from app.core.config import settings from app.core.metrics import ( RATE_LIMIT_BACKEND_ERRORS_TOTAL, @@ -129,7 +130,7 @@ def _redis_rate_limiter() -> RedisRateLimiter: def _client_address(request: Request) -> str: - return request.client.host if request.client else "unknown" + return resolve_client_ip(request) def setup_rate_limit(app: FastAPI) -> None: diff --git a/src/app/middlewares/request_logging.py b/src/app/middlewares/request_logging.py index 0788b99..023aa5e 100644 --- a/src/app/middlewares/request_logging.py +++ b/src/app/middlewares/request_logging.py @@ -3,6 +3,7 @@ from fastapi import FastAPI, Request +from app.core.client_ip import resolve_client_ip from app.core.metrics import ( HTTP_REQUEST_DURATION_SECONDS, HTTP_REQUESTS_IN_PROGRESS, @@ -70,6 +71,7 @@ async def request_logging_middleware( "http_request", extra={ "method": method, + "client_ip": resolve_client_ip(request), "path": request.url.path, "route": route, "status_code": status_code, diff --git a/tests/test_client_ip.py b/tests/test_client_ip.py new file mode 100644 index 0000000..4c0b8ca --- /dev/null +++ b/tests/test_client_ip.py @@ -0,0 +1,79 @@ +from fastapi import FastAPI, Request +from fastapi.testclient import TestClient +from uvicorn.middleware.proxy_headers import ProxyHeadersMiddleware + +from app.core.client_ip import resolve_client_ip + + +def _request(client_host: str | None) -> Request: + client = (client_host, 12345) if client_host is not None else None + return Request( + { + "type": "http", + "method": "GET", + "path": "/", + "headers": [], + "client": client, + "server": ("testserver", 80), + "scheme": "http", + "query_string": b"", + } + ) + + +def test_client_ip_is_canonicalized(): + assert resolve_client_ip(_request("2001:0db8:0:0:0:0:0:1")) == "2001:db8::1" + assert resolve_client_ip(_request("::ffff:192.0.2.10")) == "192.0.2.10" + + +def test_missing_or_non_ip_client_fails_to_one_bounded_identity(): + assert resolve_client_ip(_request(None)) == "unknown" + assert resolve_client_ip(_request("attacker-controlled-value")) == "unknown" + assert resolve_client_ip(_request("1" * 1024)) == "unknown" + + +def _proxy_test_client(*, socket_peer: str, trusted_hosts: str) -> TestClient: + inner_app = FastAPI() + + @inner_app.get("/") + def client_address(request: Request): + return {"client_ip": resolve_client_ip(request)} + + app = ProxyHeadersMiddleware(inner_app, trusted_hosts=trusted_hosts) + return TestClient(app, client=(socket_peer, 12345)) + + +def test_untrusted_socket_peer_cannot_spoof_forwarded_client_ip(): + client = _proxy_test_client( + socket_peer="192.0.2.10", + trusted_hosts="10.0.0.0/8", + ) + + response = client.get("/", headers={"X-Forwarded-For": "203.0.113.99"}) + + assert response.json() == {"client_ip": "192.0.2.10"} + + +def test_trusted_proxy_chain_uses_nearest_untrusted_address(): + client = _proxy_test_client( + socket_peer="10.0.0.3", + trusted_hosts="10.0.0.0/8", + ) + + response = client.get( + "/", + headers={"X-Forwarded-For": "203.0.113.99, 198.51.100.20, 10.0.0.2"}, + ) + + assert response.json() == {"client_ip": "198.51.100.20"} + + +def test_malformed_forwarded_address_fails_to_unknown_identity(): + client = _proxy_test_client( + socket_peer="10.0.0.3", + trusted_hosts="10.0.0.0/8", + ) + + response = client.get("/", headers={"X-Forwarded-For": "not-an-ip"}) + + assert response.json() == {"client_ip": "unknown"} diff --git a/tests/test_config.py b/tests/test_config.py index a9bff62..8d14ef6 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -1,3 +1,6 @@ +import runpy +from pathlib import Path + import pytest from cryptography.fernet import Fernet @@ -213,6 +216,39 @@ def test_rate_limit_numeric_settings_are_bounded(): ) +def test_trusted_proxy_allowlist_accepts_addresses_and_canonical_networks(): + configured = Settings( + DATABASE_URL="sqlite:///test.db", + SECRET_KEY="local-test-secret", + FORWARDED_ALLOW_IPS="127.0.0.1,10.0.0.0/8,2001:db8::/32", + _env_file=None, + ) + + assert configured.FORWARDED_ALLOW_IPS == "127.0.0.1,10.0.0.0/8,2001:db8::/32" + + +@pytest.mark.parametrize( + "allowlist", + ("*", "not-an-address", "10.1.2.3/8", "127.0.0.1,"), +) +def test_trusted_proxy_allowlist_rejects_unsafe_values(allowlist): + with pytest.raises(ValueError, match="FORWARDED_ALLOW_IPS"): + Settings( + DATABASE_URL="sqlite:///test.db", + SECRET_KEY="local-test-secret", + FORWARDED_ALLOW_IPS=allowlist, + _env_file=None, + ) + + +def test_gunicorn_uses_the_validated_trusted_proxy_allowlist(monkeypatch): + monkeypatch.setattr(settings, "FORWARDED_ALLOW_IPS", "10.0.0.0/8") + + config = runpy.run_path(str(Path(__file__).parents[1] / "gunicorn.conf.py")) + + assert config["forwarded_allow_ips"] == "10.0.0.0/8" + + def test_outbox_mode_requires_complete_secure_configuration(): with pytest.raises(ValueError, match="OUTBOX_ENCRYPTION_KEY"): Settings( diff --git a/tests/test_observability.py b/tests/test_observability.py index d348672..2f3e96a 100644 --- a/tests/test_observability.py +++ b/tests/test_observability.py @@ -74,6 +74,7 @@ def test_json_formatter_emits_correlation_fields(): ) record.request_id = "request-123" record.method = "GET" + record.client_ip = "192.0.2.10" record.route = "/health/live" record.status_code = 200 record.duration_ms = 1.25 @@ -86,6 +87,7 @@ def test_json_formatter_emits_correlation_fields(): assert payload["message"] == "http_request" assert payload["request_id"] == "request-123" assert payload["method"] == "GET" + assert payload["client_ip"] == "192.0.2.10" assert payload["route"] == "/health/live" assert payload["status_code"] == 200 assert payload["duration_ms"] == 1.25 diff --git a/tests/test_rate_limit.py b/tests/test_rate_limit.py index d016a6d..fa6de74 100644 --- a/tests/test_rate_limit.py +++ b/tests/test_rate_limit.py @@ -47,7 +47,7 @@ def test_redis_key_is_versioned_bounded_and_privacy_preserving(): assert RedisRateLimiter.normalize_client("2001:0db8::1") == "2001:db8::1" -def test_forwarded_headers_do_not_change_the_asgi_client_address(): +def test_forwarded_headers_are_not_parsed_by_the_rate_limiter(): request = Request( { "type": "http", diff --git a/tests/test_rate_limit_middleware.py b/tests/test_rate_limit_middleware.py index c2a01f1..c14ab28 100644 --- a/tests/test_rate_limit_middleware.py +++ b/tests/test_rate_limit_middleware.py @@ -2,6 +2,7 @@ from fastapi import FastAPI from fastapi.testclient import TestClient from redis.exceptions import ConnectionError, ResponseError, TimeoutError +from uvicorn.middleware.proxy_headers import ProxyHeadersMiddleware from app.middlewares import rate_limit from app.middlewares.rate_limit import RateLimitDecision, setup_rate_limit @@ -104,3 +105,27 @@ def test_exempt_path_never_calls_backend(monkeypatch): response = TestClient(create_test_app()).get("/health/live") assert response.status_code == 200 + + +def test_rate_limiter_uses_client_resolved_by_trusted_proxy(monkeypatch): + clients = [] + + class RecordingLimiter: + def check(self, client): + clients.append(client) + return RateLimitDecision(allowed=True, retry_after=1) + + monkeypatch.setattr(rate_limit.settings, "RATE_LIMIT_BACKEND", "memory") + monkeypatch.setattr(rate_limit, "rate_limiter", RecordingLimiter()) + app = ProxyHeadersMiddleware( + create_test_app(), + trusted_hosts="10.0.0.0/8", + ) + + response = TestClient(app, client=("10.0.0.3", 12345)).get( + "/protected", + headers={"X-Forwarded-For": "203.0.113.99, 198.51.100.20, 10.0.0.2"}, + ) + + assert response.status_code == 200 + assert clients == ["198.51.100.20"]