diff --git a/deploy/nginx.conf.template b/deploy/nginx.conf.template index a56bc33..0611a76 100644 --- a/deploy/nginx.conf.template +++ b/deploy/nginx.conf.template @@ -27,7 +27,9 @@ server { proxy_set_header Host $host; proxy_set_header X-Forwarded-Host $host; proxy_set_header X-Forwarded-Proto https; - proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; + # Overwrite, never append: $proxy_add_x_forwarded_for keeps any header the + # client sent, and the leftmost value would then be attacker-controlled. + proxy_set_header X-Forwarded-For $remote_addr; proxy_connect_timeout 5s; proxy_read_timeout 30s; client_max_body_size 2m; diff --git a/local/app.py b/local/app.py index 7476c57..e09f6fd 100644 --- a/local/app.py +++ b/local/app.py @@ -17,7 +17,7 @@ from . import db from .adapters import FakeFreight, FakePayment, LocalS3Storage, require_runtime from .models import Freight, Move, Pay, QuoteRequest, Review, UploadStart, OperatorLogin from .pricing import price -from .auth import COOKIE_SECURE, owner, session_row, new_session, operator, throttle, audit, rate_limit +from .auth import COOKIE_SECURE, client_ip, owner, session_row, new_session, operator, throttle, audit, rate_limit from .scanning import require_clean require_runtime() @@ -28,6 +28,10 @@ ENVIRONMENT = os.environ.get('APP_ENV', 'local') PUBLIC_ORIGIN = os.environ.get('PUBLIC_ORIGIN', 'http://localhost') ALLOWED_HOSTS = [host for host in os.environ.get('ALLOWED_HOSTS', 'localhost,127.0.0.1').split(',') if host] ALLOWED_ORIGINS = [origin for origin in os.environ.get('ALLOWED_ORIGINS', PUBLIC_ORIGIN).split(',') if origin] +# Per source and generous: a browser needs one session and keeps the cookie, but +# offices and mobile carriers put many real customers behind one address, so a +# tight per-IP ceiling would lock out the same people the old global one did. +GUEST_SESSION_LIMIT = int(os.environ.get('GUEST_SESSION_LIMIT', '240')) PART_BYTES = int(os.environ.get('UPLOAD_PART_BYTES', '8388608')) if not 5242880 <= PART_BYTES <= 67108864: raise RuntimeError('UPLOAD_PART_BYTES must be between 5 and 64 MiB') @@ -56,7 +60,7 @@ def operator_login(body: OperatorLogin, request: Request, response: Response): valid_user = secrets.compare_digest(email.encode(), configured_email.encode()) valid_password = secrets.compare_digest(body.password.encode(), os.environ['OPERATOR_PASSWORD'].encode()) if not (valid_user and valid_password): - audit('operator_login_failed') + audit('operator_login_failed', ip=client_ip(request)) raise HTTPException(401, 'Invalid operator login') token = secrets.token_urlsafe(32) with db.connect() as c: @@ -84,11 +88,11 @@ async def safe_headers(request, call_next): if request.method not in ('GET','HEAD','OPTIONS'): origin = request.headers.get('origin') if request.headers.get('sec-fetch-site') == 'cross-site' or (origin and origin not in ALLOWED_ORIGINS): - audit('cross_origin_rejected') + audit('cross_origin_rejected', ip=client_ip(request)) return JSONResponse({'detail':'Cross-origin request rejected'}, status_code=403) response = await call_next(request) if response.status_code in (401,403,429) or response.status_code>=500: - audit('http_security_event', method=request.method, status=response.status_code) + audit('http_security_event', method=request.method, status=response.status_code, ip=client_ip(request)) response.headers['Cache-Control'] = 'no-store' response.headers['X-Content-Type-Options'] = 'nosniff' response.headers['Referrer-Policy'] = 'no-referrer' @@ -111,7 +115,9 @@ def session(request: Request, response: Response): try: session_id = owner(request) except HTTPException: - rate_limit('guest-sessions', ENVIRONMENT, 120, 900) + # Per source, not per deployment: keyed on the environment name this was a + # single global bucket, so ~8 new visitors a minute exhausted it site-wide. + rate_limit('guest-sessions', client_ip(request), GUEST_SESSION_LIMIT, 900) with db.connect() as c: session_id = new_session(c, response) return {'environment': ENVIRONMENT, 'cart_scope': str(session_id), 'part_bytes': PART_BYTES, diff --git a/local/auth.py b/local/auth.py index cd4bae7..68e8209 100644 --- a/local/auth.py +++ b/local/auth.py @@ -10,6 +10,20 @@ from .db import connect COOKIE_SECURE = os.environ.get('COOKIE_SECURE', 'false').lower() == 'true' +def client_ip(request: Request): + """The requester's address, for rate-limit buckets and security events. + + The API is only reachable through the web gateway, which replaces + X-Forwarded-For with the peer address it observed, so the header carries + exactly one value the client could not choose. Without this every request + looks like the gateway, which collapses per-source limits into one global + bucket and strips the address from the audit trail. + """ + forwarded = request.headers.get('x-forwarded-for', '').split(',')[0].strip() + if forwarded: + return forwarded[:64] + return request.client.host if request.client else 'unknown' + def password_hash(password, salt=None): salt = salt or secrets.token_hex(16) digest = hashlib.scrypt(password.encode(), salt=bytes.fromhex(salt), n=16384, r=8, p=5).hex() @@ -82,7 +96,7 @@ def rate_limit(scope, identity, limit, seconds=900): def throttle(email, request): # Independent account and source buckets prevent bypass by rotating emails. - rate_limit('auth-source', request.client.host if request.client else 'local', 60) + rate_limit('auth-source', client_ip(request), 60) rate_limit('auth-account', email, 10) def operator(request: Request): diff --git a/local/customer.py b/local/customer.py index 28de74d..3ace336 100644 --- a/local/customer.py +++ b/local/customer.py @@ -5,7 +5,7 @@ from fastapi import Depends, HTTPException, Request, Response from psycopg.errors import UniqueViolation from psycopg.types.json import Jsonb from . import db -from .auth import owner, session_row, new_session, password_hash, password_matches, transfer_guest, throttle, DUMMY_PASSWORD_HASH, audit +from .auth import owner, session_row, new_session, password_hash, password_matches, transfer_guest, throttle, DUMMY_PASSWORD_HASH, audit, client_ip from .models import Register, Login, UploadStart, ArtworkSubmission from .scanning import require_clean @@ -45,7 +45,7 @@ def install_routes(app, operator, storage, begin, status, part, complete, upload stored = account['password_hash'] if account else DUMMY_PASSWORD_HASH matches = password_matches(body.password, stored) if not account or not matches: - audit('customer_login_failed') + audit('customer_login_failed', ip=client_ip(request)) raise HTTPException(401, 'Invalid email or password') previous = current(request) with db.connect() as c: diff --git a/local/nginx.conf.template b/local/nginx.conf.template index d97d99c..5497c1a 100644 --- a/local/nginx.conf.template +++ b/local/nginx.conf.template @@ -16,6 +16,7 @@ server { limit_req_status 429; proxy_pass http://api:8000; proxy_set_header Host $http_host; + proxy_set_header X-Forwarded-For $remote_addr; client_max_body_size 2m; } location / { try_files $uri $uri/ =404; } @@ -30,5 +31,6 @@ server { limit_req_status 429; proxy_pass http://api:8000; proxy_set_header Host $http_host; + proxy_set_header X-Forwarded-For $remote_addr; } } diff --git a/local/runtime_security_test.py b/local/runtime_security_test.py index 38dc44d..0f7142c 100644 --- a/local/runtime_security_test.py +++ b/local/runtime_security_test.py @@ -4,11 +4,30 @@ from unittest.mock import patch from botocore.exceptions import ClientError from .db import connect from .adapters import LocalS3Storage -from .auth import password_hash, password_matches +from .auth import client_ip, password_hash, password_matches from .scanning import ClamAV, require_clean from fastapi import HTTPException +class FakeRequest: + def __init__(self, headers=None, peer='10.0.0.2'): + self.headers=headers or {} + self.client=type('C',(),{'host':peer})() if peer else None + +def check_client_ip(): + """The gateway hands one address; a direct peer falls back to its own.""" + # Gateway-set header wins over the connection peer, which is the gateway. + assert client_ip(FakeRequest({'x-forwarded-for':'198.51.100.9'}))=='198.51.100.9' + # Only the first entry is used, and it is length-capped. + assert client_ip(FakeRequest({'x-forwarded-for':'198.51.100.9, 10.0.0.2'}))=='198.51.100.9' + assert len(client_ip(FakeRequest({'x-forwarded-for':'a'*500})))<=64 + # No header: the peer address, never a constant shared by every request. + assert client_ip(FakeRequest(peer='192.0.2.5'))=='192.0.2.5' + assert client_ip(FakeRequest({'x-forwarded-for':' '},peer='192.0.2.5'))=='192.0.2.5' + assert client_ip(FakeRequest(peer=None))=='unknown' + print('PASS: client address resolution for rate-limit buckets and audit events') + def run(): + check_client_ip() with connect() as c: role=c.execute('SELECT rolsuper,rolcreatedb,rolcreaterole,rolbypassrls FROM pg_roles WHERE rolname=current_user').fetchone() assert not any(role.values()),role diff --git a/local/security_test.py b/local/security_test.py index cec1920..0ffa0ab 100644 --- a/local/security_test.py +++ b/local/security_test.py @@ -63,4 +63,15 @@ def run(): attacker.call('/operator/login',{'email':email,'password':'invalid'},expected=429) print('PASS: operator login throttling (only synthetic account bucket exhausted)') + # Guest sessions are limited per source, not once for the whole deployment. + # Keyed on the environment name this was a single global bucket of 120 per + # 15 minutes, which the suites above would already have eaten into. + for _ in range(25): + Client().call('/session') + # A forged forwarded address must not let a client pick another bucket: the + # gateway overwrites the header, so these count against the real source too. + for _ in range(5): + raw('/api/session',200,{'X-Forwarded-For':'203.0.113.7'}) + print('PASS: guest sessions limited per source, forwarded address not client-controlled') + if __name__=='__main__':run()