From e3e1694dfc4acbbe386b1d949cd1a52adc35c838 Mon Sep 17 00:00:00 2001 From: holden093 Date: Sat, 18 Jul 2026 22:49:30 +0200 Subject: [PATCH] =?UTF-8?q?fix(auth):=20proactive=20hardening=20=E2=80=94?= =?UTF-8?q?=20cross-worker=20revocation,=20file=20perms,=20cookie=20policy?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up hardening beyond the explicit review findings: - Propagate session revocation across uvicorn workers: token validation now syncs issuance AND revocation from sessions.json (mtime-gated), _save_sessions merges on-disk state under an inter-process flock so concurrent workers can't lose each other's sessions, and revocation tombstones prevent a just-revoked token from being re-merged. - Restrict sessions.json and auth.json to 0600 (bearer tokens and password hashes; same policy as data/app.db, #4420), applied atomically at write time and retroactively at load. - Password-login session cookie: SECURE_COOKIES=false can no longer downgrade the cookie when the request arrived over HTTPS (spoofable X-Forwarded-Proto still requires TRUST_PROXY_HEADERS opt-in). - Document why OIDC state tokens are deliberately not single-use and which mechanisms bound the replay window. - Warn once per process (not twice per login) when OIDC_ALLOW_INSECURE_COOKIES is enabled; pass the variable through the Compose files so the documented dev override actually reaches containers. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01GRiLb12nnLnBnYsg14oSWd --- core/atomic_io.py | 14 +- core/auth.py | 155 ++++++++++++++++----- core/oidc.py | 10 ++ docker-compose.gpu-amd.yml | 2 + docker-compose.gpu-nvidia.yml | 2 + docker-compose.yml | 2 + routes/auth_routes.py | 22 ++- routes/oidc_routes.py | 18 ++- tests/test_password_login_cookie_secure.py | 47 +++++++ tests/test_session_cross_worker.py | 67 +++++++++ 10 files changed, 297 insertions(+), 42 deletions(-) create mode 100644 tests/test_password_login_cookie_secure.py diff --git a/core/atomic_io.py b/core/atomic_io.py index 81c640d8a..420c7a751 100644 --- a/core/atomic_io.py +++ b/core/atomic_io.py @@ -18,15 +18,27 @@ import os from typing import Any, Optional -def atomic_write_json(path: str, data: Any, *, indent: Optional[int] = None) -> None: +def atomic_write_json( + path: str, data: Any, *, indent: Optional[int] = None, mode: Optional[int] = None +) -> None: """Atomically persist `data` as JSON at `path`. The temp file uses the live PID as a suffix so two processes saving the same file (e.g. unit tests) don't collide on the rename target. + + When *mode* is given (e.g. ``0o600`` for files holding secrets), the + temp file is chmod'ed before the rename so the restricted permissions + are in place atomically with the content — there is no window where + the target exists with default-umask permissions. """ os.makedirs(os.path.dirname(path) or ".", exist_ok=True) tmp = f"{path}.tmp.{os.getpid()}" with open(tmp, "w", encoding="utf-8") as f: + if mode is not None: + try: + os.fchmod(f.fileno(), mode) + except AttributeError: # Windows has no fchmod + os.chmod(tmp, mode) json.dump(data, f, indent=indent) f.flush() os.fsync(f.fileno()) diff --git a/core/auth.py b/core/auth.py index aca22d304..4ce4ac095 100644 --- a/core/auth.py +++ b/core/auth.py @@ -140,6 +140,15 @@ class AuthManager: # detect sessions written by other uvicorn workers (see # _reload_sessions_if_changed). self._sessions_mtime_ns = -1 + # Tokens present in sessions.json at the last disk sync. Used to + # distinguish "revoked by another worker" (was on disk, now gone — + # drop it) from "issued locally moments ago, racing its own save" + # (never seen on disk — keep it). + self._disk_tokens: set = set() + # Tokens this worker revoked whose removal may not yet be visible + # on disk. A disk sync must never re-add these; pruned once the + # on-disk file no longer contains them. + self._revoked_tokens: set = set() self._load() self._load_sessions() self._migrate_single_user() @@ -149,6 +158,12 @@ class AuthManager: def _load(self): try: if os.path.exists(self.auth_path): + # Contains password hashes — restrict pre-existing files + # written before the 0600 policy. + try: + os.chmod(self.auth_path, 0o600) + except OSError: + pass with open(self.auth_path, "r", encoding="utf-8") as f: self._config = json.load(f) # Normalize all stored usernames to lowercase so they match @@ -172,11 +187,19 @@ class AuthManager: """Load persisted session tokens from disk, pruning expired ones.""" try: if os.path.exists(self._sessions_path): + # Session tokens are bearer credentials — never leave the + # file readable by other local users (same policy as + # data/app.db, #4420). + try: + os.chmod(self._sessions_path, 0o600) + except OSError: + pass self._sessions_mtime_ns = os.stat(self._sessions_path).st_mtime_ns with open(self._sessions_path, "r", encoding="utf-8") as f: data = json.load(f) now = time.time() self._sessions = {k: v for k, v in data.items() if v.get("expiry", 0) > now} + self._disk_tokens = set(data) pruned = len(data) - len(self._sessions) if pruned > 0: self._save_sessions() @@ -186,19 +209,23 @@ class AuthManager: self._sessions = {} def _reload_sessions_if_changed(self): - """Merge sessions written by other uvicorn workers. + """Sync session state written by other uvicorn workers. - The OIDC callback (or a password login) may run on one worker while - the browser's next request lands on another; each worker loads - sessions.json only at startup, so the new token would be rejected. - Called on a token miss: when the file's mtime has changed since the - last load, re-read it and add unknown unexpired tokens to the - in-memory map. The mtime gate keeps unknown-token spam at one - os.stat per request, not a JSON parse. + The OIDC callback (or a password login/logout) may run on one + worker while the browser's next request lands on another; each + worker loads sessions.json only at startup, so cross-worker + issuance and revocation would otherwise be invisible. Called on + every token validation: when the file's mtime has changed since + the last sync, re-read it and - Additive only — tokens missing from disk are NOT dropped from - memory, so a token issued moments ago on this worker can't be lost - to a reload racing its own _save_sessions. + - add unknown unexpired tokens (issued by another worker), and + - drop in-memory tokens that were on disk at the last sync but + are gone now (revoked by another worker). + + A token never yet seen on disk is kept — it was issued locally + moments ago and may be racing its own _save_sessions. The mtime + gate keeps the steady-state cost at one os.stat per validation, + not a JSON parse. """ try: stat = os.stat(self._sessions_path) @@ -216,21 +243,76 @@ class AuthManager: self._sessions_mtime_ns = stat.st_mtime_ns if not isinstance(data, dict): return - now = time.time() - for tok, sess in data.items(): - if ( - tok not in self._sessions - and isinstance(sess, dict) - and sess.get("expiry", 0) > now - ): - self._sessions[tok] = sess + self._apply_disk_sessions(data) + + def _apply_disk_sessions(self, data: Dict[str, Any]) -> None: + """Merge parsed sessions.json content into memory. + + Caller must hold ``_sessions_lock``. Adds unknown unexpired + tokens (unless this worker revoked them and the removal hasn't + reached disk yet), drops tokens revoked by other workers, and + refreshes the disk-snapshot bookkeeping. + """ + now = time.time() + for tok, sess in data.items(): + if ( + tok not in self._sessions + and tok not in self._revoked_tokens + and isinstance(sess, dict) + and sess.get("expiry", 0) > now + ): + self._sessions[tok] = sess + revoked_elsewhere = [ + tok for tok in self._sessions + if tok not in data and tok in self._disk_tokens + ] + for tok in revoked_elsewhere: + self._sessions.pop(tok, None) + self._disk_tokens = set(data) + # A tombstone is only needed while the token is still on disk. + self._revoked_tokens &= self._disk_tokens + + @contextmanager + def _interprocess_sessions_lock(self): + """Serialise sessions.json read-merge-write cycles across uvicorn + workers. Separate lock file from the auth.json IPC lock so a + session save can never deadlock a caller already holding the auth + lock (flock is not re-entrant across file descriptors).""" + if not HAS_FCNTL: + yield + return + fd = os.open(self._sessions_path + ".lock", os.O_CREAT | os.O_RDWR, 0o600) + try: + fcntl.flock(fd, fcntl.LOCK_EX) + yield + finally: + fcntl.flock(fd, fcntl.LOCK_UN) + os.close(fd) def _save_sessions(self): - """Persist session tokens to disk (atomic, lock-guarded).""" + """Persist session tokens to disk (atomic, merge-on-write). + + Merges the current on-disk state before writing, under an + inter-process flock — a plain overwrite would clobber sessions + issued by other workers since this worker's last sync (lost + update). Tombstones in ``_revoked_tokens`` keep just-revoked + tokens from being re-merged and resurrected. + """ try: - with self._sessions_lock: + with self._interprocess_sessions_lock(), self._sessions_lock: + try: + with open(self._sessions_path, "r", encoding="utf-8") as f: + data = json.load(f) + if isinstance(data, dict): + self._apply_disk_sessions(data) + except OSError: + pass # first save — no file yet + except Exception as e: + logger.error(f"Failed to merge sessions before save: {e}") snapshot = dict(self._sessions) - _atomic_write_json(self._sessions_path, snapshot) + _atomic_write_json(self._sessions_path, snapshot, mode=0o600) + self._disk_tokens = set(snapshot) + self._revoked_tokens &= self._disk_tokens except Exception as e: logger.error(f"Failed to save sessions: {e}") @@ -295,7 +377,8 @@ class AuthManager: self._save() def _save(self): - _atomic_write_json(self.auth_path, self._config, indent=2) + # Password hashes — owner-only, same policy as sessions.json. + _atomic_write_json(self.auth_path, self._config, indent=2, mode=0o600) @property def users(self) -> Dict[str, Any]: @@ -622,6 +705,7 @@ class AuthManager: if (sess or {}).get("username") == username] for tok in to_drop: self._sessions.pop(tok, None) + self._revoked_tokens.add(tok) revoked += 1 if revoked: self._save_sessions() @@ -923,11 +1007,8 @@ class AuthManager: def validate_token(self, token: Optional[str]) -> bool: if not token: return False - with self._sessions_lock: - known = token in self._sessions - if not known: - # May have been issued by another worker — read through to disk. - self._reload_sessions_if_changed() + # Sync issuance/revocation from other workers (mtime-gated). + self._reload_sessions_if_changed() expired = False deleted_user = False with self._sessions_lock: @@ -944,6 +1025,7 @@ class AuthManager: # silently authenticating against a non-existent account. if session.get("username") not in self.users: self._sessions.pop(token, None) + self._revoked_tokens.add(token) deleted_user = True if expired or deleted_user: self._save_sessions() @@ -954,11 +1036,8 @@ class AuthManager: """Return the username associated with a valid token.""" if not token: return None - with self._sessions_lock: - known = token in self._sessions - if not known: - # May have been issued by another worker — read through to disk. - self._reload_sessions_if_changed() + # Sync issuance/revocation from other workers (mtime-gated). + self._reload_sessions_if_changed() expired = False deleted_user = False with self._sessions_lock: @@ -973,6 +1052,7 @@ class AuthManager: # SECURITY: orphan check — same rationale as validate_token. if _u not in self.users: self._sessions.pop(token, None) + self._revoked_tokens.add(token) deleted_user = True else: return _u @@ -983,6 +1063,7 @@ class AuthManager: def revoke_token(self, token: str): with self._sessions_lock: self._sessions.pop(token, None) + self._revoked_tokens.add(token) self._save_sessions() def revoke_user_sessions(self, username: str, except_token: Optional[str] = None) -> int: @@ -996,9 +1077,13 @@ class AuthManager: ] for token in to_drop: self._sessions.pop(token, None) + self._revoked_tokens.add(token) revoked += 1 - if revoked: - self._save_sessions() + # Save outside _sessions_lock: _save_sessions acquires the + # inter-process flock before _sessions_lock, and taking them in + # the opposite order here could deadlock two threads. + if revoked: + self._save_sessions() return revoked def status(self, token: Optional[str]) -> Dict[str, Any]: diff --git a/core/oidc.py b/core/oidc.py index d009b5f99..00874b1db 100644 --- a/core/oidc.py +++ b/core/oidc.py @@ -51,6 +51,16 @@ logger = logging.getLogger(__name__) _STATE_TTL = 600 # 10 minutes +# DESIGN NOTE — state tokens are deliberately NOT single-use. Enforcing +# one-time consumption would require shared server-side storage, which +# this stateless design intentionally avoids (multi-worker support with +# no session store). Replay of a state within its TTL is mitigated by: +# - the authorization code being single-use at the IdP (a replayed +# callback fails the token exchange), +# - the nonce being bound into the signed id_token and verified, +# - the PKCE verifier being bound to the same encrypted state, and +# - the CSRF cookie requiring the completing browser to hold the state. + _state_fernet_lock = threading.Lock() _state_fernet = None diff --git a/docker-compose.gpu-amd.yml b/docker-compose.gpu-amd.yml index 1b4bce5a6..a461609f1 100644 --- a/docker-compose.gpu-amd.yml +++ b/docker-compose.gpu-amd.yml @@ -58,6 +58,8 @@ services: - OIDC_ADMIN_GROUPS=${OIDC_ADMIN_GROUPS:-} - OIDC_REDIRECT_URI=${OIDC_REDIRECT_URI:-} - OIDC_FIRST_USER_IS_ADMIN=${OIDC_FIRST_USER_IS_ADMIN:-true} + # Dev-only: allow OIDC cookies without the Secure flag (plain-HTTP testing). + - OIDC_ALLOW_INSECURE_COOKIES=${OIDC_ALLOW_INSECURE_COOKIES:-false} - EMBEDDING_URL=${EMBEDDING_URL:-} - EMBEDDING_MODEL=${EMBEDDING_MODEL:-} - EMBEDDING_API_KEY=${EMBEDDING_API_KEY:-} diff --git a/docker-compose.gpu-nvidia.yml b/docker-compose.gpu-nvidia.yml index 6267efe8a..e76bfccd0 100644 --- a/docker-compose.gpu-nvidia.yml +++ b/docker-compose.gpu-nvidia.yml @@ -57,6 +57,8 @@ services: - OIDC_ADMIN_GROUPS=${OIDC_ADMIN_GROUPS:-} - OIDC_REDIRECT_URI=${OIDC_REDIRECT_URI:-} - OIDC_FIRST_USER_IS_ADMIN=${OIDC_FIRST_USER_IS_ADMIN:-true} + # Dev-only: allow OIDC cookies without the Secure flag (plain-HTTP testing). + - OIDC_ALLOW_INSECURE_COOKIES=${OIDC_ALLOW_INSECURE_COOKIES:-false} - EMBEDDING_URL=${EMBEDDING_URL:-} - EMBEDDING_MODEL=${EMBEDDING_MODEL:-} - EMBEDDING_API_KEY=${EMBEDDING_API_KEY:-} diff --git a/docker-compose.yml b/docker-compose.yml index a6cf8d1f3..732ac80b1 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -46,6 +46,8 @@ services: - OIDC_ADMIN_GROUPS=${OIDC_ADMIN_GROUPS:-} - OIDC_REDIRECT_URI=${OIDC_REDIRECT_URI:-} - OIDC_FIRST_USER_IS_ADMIN=${OIDC_FIRST_USER_IS_ADMIN:-true} + # Dev-only: allow OIDC cookies without the Secure flag (plain-HTTP testing). + - OIDC_ALLOW_INSECURE_COOKIES=${OIDC_ALLOW_INSECURE_COOKIES:-false} - EMBEDDING_URL=${EMBEDDING_URL:-} - EMBEDDING_MODEL=${EMBEDDING_MODEL:-} - EMBEDDING_API_KEY=${EMBEDDING_API_KEY:-} diff --git a/routes/auth_routes.py b/routes/auth_routes.py index 97521f2e2..e591694a0 100644 --- a/routes/auth_routes.py +++ b/routes/auth_routes.py @@ -84,6 +84,26 @@ class SetOpenRegistrationRequest(BaseModel): SESSION_COOKIE = "odysseus_session" +def _session_cookie_secure(request: Request) -> bool: + """Secure flag for the password-login session cookie. + + SECURE_COOKIES=true always wins. Unlike the historical behaviour, + SECURE_COOKIES=false (the bundled Compose default) can no longer + downgrade the cookie when the request itself arrived over HTTPS — + a stock TLS deployment must not issue a bearer cookie eligible for + cleartext transmission. X-Forwarded-Proto is honoured only when the + deployment explicitly opts in via TRUST_PROXY_HEADERS, so a client + cannot influence cookie policy with a spoofed header. + """ + if os.getenv("SECURE_COOKIES", "").strip().lower() in ("true", "1", "yes"): + return True + forwarded = "" + if os.getenv("TRUST_PROXY_HEADERS", "").strip().lower() in ("true", "1", "yes"): + forwarded = getattr(request, "headers", {}).get("x-forwarded-proto", "") + scheme = forwarded or getattr(getattr(request, "url", None), "scheme", "") or "http" + return scheme == "https" + + def setup_auth_routes(auth_manager: AuthManager) -> APIRouter: router = APIRouter(prefix="/api/auth", tags=["auth"]) @@ -157,7 +177,7 @@ def setup_auth_routes(auth_manager: AuthManager) -> APIRouter: value=token, httponly=True, samesite="lax", - secure=os.getenv("SECURE_COOKIES", "false").lower() == "true", + secure=_session_cookie_secure(request), path="/", ) if body.remember: diff --git a/routes/oidc_routes.py b/routes/oidc_routes.py index f524dd218..c21e4c9a3 100644 --- a/routes/oidc_routes.py +++ b/routes/oidc_routes.py @@ -1,6 +1,7 @@ """OpenID Connect authentication routes — login, callback, config.""" import asyncio +import functools import logging import os import secrets @@ -319,10 +320,17 @@ def _oidc_cookie_secure() -> bool: downgrade OIDC cookies. """ if os.getenv("OIDC_ALLOW_INSECURE_COOKIES", "").strip().lower() in ("true", "1", "yes"): - logger.warning( - "OIDC_ALLOW_INSECURE_COOKIES=true — OIDC session and CSRF " - "cookies are issued without the Secure flag. Never use this " - "outside plain-HTTP local development." - ) + _warn_insecure_cookies_once() return False return True + + +@functools.lru_cache(maxsize=1) +def _warn_insecure_cookies_once() -> None: + # Once per process, not once per login — the flag doesn't change at + # runtime and repeating the warning twice per flow is pure log spam. + logger.warning( + "OIDC_ALLOW_INSECURE_COOKIES=true — OIDC session and CSRF " + "cookies are issued without the Secure flag. Never use this " + "outside plain-HTTP local development." + ) diff --git a/tests/test_password_login_cookie_secure.py b/tests/test_password_login_cookie_secure.py new file mode 100644 index 000000000..fa23b4637 --- /dev/null +++ b/tests/test_password_login_cookie_secure.py @@ -0,0 +1,47 @@ +"""Regression: the password-login session cookie must be Secure whenever +the request arrived over HTTPS, even with SECURE_COOKIES=false (the +bundled Compose default) — a stock TLS deployment must not issue a bearer +cookie eligible for cleartext transmission.""" + +from types import SimpleNamespace + + +def _fake_request(scheme="https", headers=None): + req = SimpleNamespace() + req.url = SimpleNamespace() + req.url.scheme = scheme + req.headers = headers or {} + return req + + +class TestSessionCookieSecure: + def _secure(self, request): + from routes.auth_routes import _session_cookie_secure + return _session_cookie_secure(request) + + def test_https_request_secure_despite_secure_cookies_false(self, monkeypatch): + monkeypatch.setenv("SECURE_COOKIES", "false") + monkeypatch.delenv("TRUST_PROXY_HEADERS", raising=False) + assert self._secure(_fake_request("https")) is True + + def test_explicit_true_always_secure(self, monkeypatch): + monkeypatch.setenv("SECURE_COOKIES", "true") + assert self._secure(_fake_request("http")) is True + + def test_plain_http_not_secure(self, monkeypatch): + monkeypatch.setenv("SECURE_COOKIES", "false") + monkeypatch.delenv("TRUST_PROXY_HEADERS", raising=False) + assert self._secure(_fake_request("http")) is False + + def test_forwarded_proto_ignored_without_trust_optin(self, monkeypatch): + """A client-spoofed X-Forwarded-Proto must not influence policy.""" + monkeypatch.setenv("SECURE_COOKIES", "false") + monkeypatch.delenv("TRUST_PROXY_HEADERS", raising=False) + req = _fake_request("http", {"x-forwarded-proto": "https"}) + assert self._secure(req) is False + + def test_forwarded_proto_honoured_with_trust_optin(self, monkeypatch): + monkeypatch.setenv("SECURE_COOKIES", "false") + monkeypatch.setenv("TRUST_PROXY_HEADERS", "true") + req = _fake_request("http", {"x-forwarded-proto": "https"}) + assert self._secure(req) is True diff --git a/tests/test_session_cross_worker.py b/tests/test_session_cross_worker.py index 48ffb8d58..65f9b7274 100644 --- a/tests/test_session_cross_worker.py +++ b/tests/test_session_cross_worker.py @@ -82,3 +82,70 @@ class TestCrossWorkerSessions: # B validating A's token triggers a reload; B's own token survives. assert worker_b.validate_token(token_a) is True assert worker_b.validate_token(token_b) is True + + +class TestCrossWorkerRevocation: + def test_revocation_propagates_to_other_worker(self, tmp_path): + """Logout on worker A must invalidate the token on worker B even + though B holds it in its in-memory map.""" + worker_a, worker_b = _two_workers(tmp_path) + token = worker_a.create_session_trusted("alice") + assert worker_b.validate_token(token) is True # B now caches it + worker_a.revoke_token(token) + assert worker_b.validate_token(token) is False + assert worker_b.get_username_for_token(token) is None + + def test_revoke_user_sessions_propagates(self, tmp_path): + """Admin-driven revocation (password change, user deletion) on one + worker must take effect on the others.""" + worker_a, worker_b = _two_workers(tmp_path) + token = worker_a.create_session_trusted("alice") + assert worker_b.validate_token(token) is True + assert worker_a.revoke_user_sessions("alice") == 1 + assert worker_b.validate_token(token) is False + + def test_never_persisted_token_survives_reload(self, tmp_path): + """A token in memory that was never written to disk (racing its own + save) must not be dropped when a reload observes another worker's + write that lacks it.""" + worker_a, worker_b = _two_workers(tmp_path) + import time as _time + phantom = "e" * 64 + with worker_b._sessions_lock: + worker_b._sessions[phantom] = { + "username": "alice", "expiry": _time.time() + 3600, + } + token_a = worker_a.create_session_trusted("alice") # bumps mtime + assert worker_b.validate_token(token_a) is True # triggers reload + assert worker_b.validate_token(phantom) is True # survived + + +class TestSecretFilePermissions: + def test_sessions_file_owner_only(self, tmp_path): + import stat + worker_a, _ = _two_workers(tmp_path) + worker_a.create_session_trusted("alice") + mode = stat.S_IMODE((tmp_path / "sessions.json").stat().st_mode) + assert mode == 0o600 + + def test_auth_file_owner_only(self, tmp_path): + import stat + _two_workers(tmp_path) + mode = stat.S_IMODE((tmp_path / "auth.json").stat().st_mode) + assert mode == 0o600 + + def test_preexisting_world_readable_files_restricted_on_load(self, tmp_path): + """Files written before the 0600 policy get restricted at startup.""" + import stat + auth_mod = _auth_module() + auth_mod._hash_password = lambda password: f"hash:{password}" + auth_mod._verify_password = lambda password, hashed: hashed == f"hash:{password}" + auth_path = str(tmp_path / "auth.json") + mgr = auth_mod.AuthManager(auth_path) + assert mgr.create_user("alice", "password-1", is_admin=False) + mgr.create_session_trusted("alice") + (tmp_path / "auth.json").chmod(0o644) + (tmp_path / "sessions.json").chmod(0o644) + auth_mod.AuthManager(auth_path) # fresh load restricts both + assert stat.S_IMODE((tmp_path / "auth.json").stat().st_mode) == 0o600 + assert stat.S_IMODE((tmp_path / "sessions.json").stat().st_mode) == 0o600