diff --git a/.env.example b/.env.example index 36cba2f47..8466be67b 100644 --- a/.env.example +++ b/.env.example @@ -130,6 +130,11 @@ SEARXNG_INSTANCE=http://localhost:8080 # TRUST_PROXY_HEADERS=true only when the app is behind a trusted proxy that # strips/replaces inbound X-Forwarded-Proto; otherwise client-supplied # forwarded headers are ignored for cookie security decisions. +# +# OIDC session and CSRF cookies always carry the Secure flag, regardless +# of SECURE_COOKIES — SSO implies a TLS deployment. The only opt-out is +# the development-only override below; never enable it in production. +# OIDC_ALLOW_INSECURE_COOKIES=false # ============================================================ # ChromaDB (vector store) diff --git a/core/auth.py b/core/auth.py index f3700693a..aca22d304 100644 --- a/core/auth.py +++ b/core/auth.py @@ -136,6 +136,10 @@ class AuthManager: # all uvicorn workers so first-admin bootstrap and auth.json mutations # are serialised across processes, not just threads within one worker. self._ipc_lock_path = auth_path + ".lock" + # mtime of sessions.json at last load — lets validate_token cheaply + # detect sessions written by other uvicorn workers (see + # _reload_sessions_if_changed). + self._sessions_mtime_ns = -1 self._load() self._load_sessions() self._migrate_single_user() @@ -168,6 +172,7 @@ class AuthManager: """Load persisted session tokens from disk, pruning expired ones.""" try: if os.path.exists(self._sessions_path): + 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() @@ -180,6 +185,46 @@ class AuthManager: logger.error(f"Failed to load sessions: {e}") self._sessions = {} + def _reload_sessions_if_changed(self): + """Merge sessions 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. + + 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. + """ + try: + stat = os.stat(self._sessions_path) + except OSError: + return + with self._sessions_lock: + if stat.st_mtime_ns == self._sessions_mtime_ns: + return + try: + with open(self._sessions_path, "r", encoding="utf-8") as f: + data = json.load(f) + except Exception as e: + logger.error(f"Failed to reload sessions: {e}") + return + 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 + def _save_sessions(self): """Persist session tokens to disk (atomic, lock-guarded).""" try: @@ -878,6 +923,11 @@ 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() expired = False deleted_user = False with self._sessions_lock: @@ -904,6 +954,11 @@ 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() expired = False deleted_user = False with self._sessions_lock: diff --git a/core/oidc.py b/core/oidc.py index c6c34ef2a..d009b5f99 100644 --- a/core/oidc.py +++ b/core/oidc.py @@ -26,6 +26,8 @@ JWKS keys are cached after first fetch and refreshed only when an unknown 60-second cooldown throttles both successful and failed refreshes. """ +import base64 +import hashlib import json import logging import math @@ -74,12 +76,13 @@ def _get_state_fernet(): return _state_fernet -def _encode_state(nonce: str, redirect_uri: str) -> str: +def _encode_state(nonce: str, redirect_uri: str, code_verifier: str) -> str: """Return a Fernet-encrypted state token containing nonce + metadata.""" fernet = _get_state_fernet() payload = json.dumps({ "nonce": nonce, "redirect_uri": redirect_uri, + "code_verifier": code_verifier, "created": time.time(), }) return fernet.encrypt(payload.encode()).decode() @@ -97,11 +100,14 @@ def _decode_state(state: str) -> Optional[Dict[str, Any]]: return None nonce = data.get("nonce") redirect_uri = data.get("redirect_uri") + code_verifier = data.get("code_verifier") created = data.get("created") if not isinstance(nonce, str) or not nonce: return None if not isinstance(redirect_uri, str): return None + if not isinstance(code_verifier, str) or not code_verifier: + return None if not _is_numericdate(created): return None now = time.time() @@ -164,8 +170,19 @@ class OidcManager: self._jwks_cache: Dict[str, Dict[str, Any]] = {} self._jwks_cache_lock = threading.Lock() self._allowed_algs: Optional[List[str]] = None + self._token_auth_methods: List[str] = ["client_secret_basic"] self._discover() + def _use_basic_auth(self) -> bool: + """True when the token endpoint should use client_secret_basic.""" + if "client_secret_basic" in self._token_auth_methods: + return True + if "client_secret_post" in self._token_auth_methods: + return False + # Provider advertises neither shared-secret method — use the OIDC + # default rather than silently leaking the secret in the body. + return True + # -- discovery ----------------------------------------------------------- def _discover(self) -> None: @@ -176,6 +193,15 @@ class OidcManager: well_known_url = self.issuer + "/.well-known/openid-configuration" if not well_known_url.startswith(("http://", "https://")): well_known_url = f"https://{well_known_url}" + # The issuer (and therefore the discovery document) must be HTTPS: + # the authorization redirect carries state/nonce, and an http:// + # issuer lets an active network attacker substitute authorization + # codes or rewrite the discovery document entirely. + if not well_known_url.startswith("https://"): + raise OidcError( + f"OIDC issuer must use HTTPS, got {self.issuer!r}. " + "Configure the IdP with TLS or set OIDC_ENABLED=false." + ) try: resp = httpx.get(well_known_url, timeout=15.0) @@ -204,10 +230,12 @@ class OidcManager: f"discovery doc returned {doc_issuer!r}" ) - # Client credentials and bearer tokens must never be sent over - # cleartext transport. Authorization is browser-facing and is not - # included because it does not carry those secrets. - for name in ("token_endpoint", "jwks_uri", "userinfo_endpoint"): + # No OIDC endpoint may use cleartext transport. The back-channel + # endpoints carry client credentials and bearer tokens; the browser- + # facing authorization endpoint carries state/nonce and returns the + # authorization code, so an http:// endpoint enables code + # substitution by an active network observer. + for name in ("authorization_endpoint", "token_endpoint", "jwks_uri", "userinfo_endpoint"): url = self._config.get(name) if url and not isinstance(url, str): raise OidcError(f"OIDC {name} must be a URL string") @@ -224,6 +252,13 @@ class OidcManager: safe = [a for a in supported if a in ("RS256", "RS384", "RS512", "ES256", "ES384", "ES512", "PS256", "PS384", "PS512")] self._allowed_algs = safe or ["RS256"] + # Token-endpoint auth methods. Per OIDC Discovery §3, an omitted + # token_endpoint_auth_methods_supported means client_secret_basic. + methods = self._config.get("token_endpoint_auth_methods_supported") + if not isinstance(methods, list) or not methods: + methods = ["client_secret_basic"] + self._token_auth_methods = methods + logger.info( "OIDC provider discovered: issuer=%r auth=%r token=%r algs=%s", self.issuer, @@ -264,9 +299,19 @@ class OidcManager: """ nonce = secrets.token_hex(32) + # PKCE (RFC 7636, S256). The verifier travels inside the encrypted + # state token, so the callback can recover it without server-side + # storage — same carrier as the nonce. + code_verifier = secrets.token_urlsafe(64) + code_challenge = ( + base64.urlsafe_b64encode(hashlib.sha256(code_verifier.encode("ascii")).digest()) + .rstrip(b"=") + .decode("ascii") + ) + # Encode the nonce + metadata into the state parameter (Fernet- # encrypted, stateless — works across multiple workers/processes). - state = _encode_state(nonce, redirect_uri) + state = _encode_state(nonce, redirect_uri, code_verifier) from urllib.parse import urlencode params = { @@ -276,6 +321,8 @@ class OidcManager: "scope": self.scopes, "state": state, "nonce": nonce, + "code_challenge": code_challenge, + "code_challenge_method": "S256", } # Request forced re-authentication when OIDC_MAX_AGE is configured. # The claim is later verified in _verify_id_token against auth_time. @@ -300,6 +347,7 @@ class OidcManager: raise OidcError("OIDC state not found — may be expired, reused, or from a different worker") nonce = stored.get("nonce", "") stored_redirect_uri = stored.get("redirect_uri", "") + code_verifier = stored.get("code_verifier", "") # Bind the token exchange to the redirect_uri that was used in the # authorization request (carried in the signed state token). Reject @@ -312,7 +360,9 @@ class OidcManager: ) # 2. Exchange code for tokens (using the stored redirect_uri) - token_data = self._token_request(code, stored_redirect_uri or redirect_uri) + token_data = self._token_request( + code, stored_redirect_uri or redirect_uri, code_verifier + ) # 3. Verify id_token id_token = token_data.get("id_token") @@ -349,9 +399,13 @@ class OidcManager: ) userinfo = {} else: - # Require a non-empty sub that matches the verified - # id_token subject before trusting any UserInfo claims. - ui_sub = (userinfo.get("sub") or "").strip() + # Require a non-empty sub that exactly matches the + # verified id_token subject before trusting any UserInfo + # claims. No normalization: subs are opaque identifiers + # and trimming could equate two distinct subjects. + ui_sub = userinfo.get("sub") + if not isinstance(ui_sub, str): + ui_sub = "" if not ui_sub: logger.warning( "UserInfo response missing sub claim — " @@ -388,18 +442,34 @@ class OidcManager: claims["_userinfo_available"] = userinfo_available return claims - def _token_request(self, code: str, redirect_uri: str) -> Dict[str, Any]: + def _token_request( + self, code: str, redirect_uri: str, code_verifier: str + ) -> Dict[str, Any]: """POST the token endpoint to exchange code for tokens.""" token_endpoint = self._config["token_endpoint"] payload = { "grant_type": "authorization_code", "code": code, "redirect_uri": redirect_uri, - "client_id": self.client_id, - "client_secret": self.client_secret, + "code_verifier": code_verifier, } + # client_secret_basic is the OIDC default and the method the + # conformance suite expects; use it whenever the provider supports + # it (or doesn't advertise methods at all, which per Discovery §3 + # means client_secret_basic). Fall back to client_secret_post only + # when the provider explicitly excludes basic. + auth = None + if self._use_basic_auth(): + from urllib.parse import quote + auth = ( + quote(self.client_id, safe=""), + quote(self.client_secret, safe=""), + ) + else: + payload["client_id"] = self.client_id + payload["client_secret"] = self.client_secret try: - resp = httpx.post(token_endpoint, data=payload, timeout=15.0) + resp = httpx.post(token_endpoint, data=payload, auth=auth, timeout=15.0) resp.raise_for_status() data = resp.json() except httpx.HTTPStatusError as exc: @@ -583,15 +653,17 @@ class OidcManager: f"{self.max_age} s (now={now:.0f}, age={now - auth_time:.0f} s)" ) - # Verify iat (issued-at) is not in the far future. + # Verify iat (issued-at): required by OIDC Core §2, must be numeric + # and not in the far future. iat = claims.get("iat") - if iat is not None: - if not _is_numericdate(iat): - raise OidcError(f"id_token iat claim is non-numeric: {iat!r}") - if iat > time.time() + 60: - raise OidcError( - f"id_token iat {iat} is more than 60 s in the future" - ) + if iat is None: + raise OidcError("id_token missing iat claim") + if not _is_numericdate(iat): + raise OidcError(f"id_token iat claim is non-numeric: {iat!r}") + if iat > time.time() + 60: + raise OidcError( + f"id_token iat {iat} is more than 60 s in the future" + ) return claims diff --git a/routes/oidc_routes.py b/routes/oidc_routes.py index 719d38edc..f524dd218 100644 --- a/routes/oidc_routes.py +++ b/routes/oidc_routes.py @@ -104,7 +104,7 @@ def setup_oidc_routes( value=_state, httponly=True, samesite="lax", - secure=_is_secure_context(request), + secure=_oidc_cookie_secure(), path="/api/auth/oidc/callback", max_age=OIDC_CSRF_MAX_AGE, ) @@ -164,14 +164,20 @@ def setup_oidc_routes( name = claims.get("name", "") groups_claim_present = "groups" in claims groups = claims.get("groups", []) - id_token_groups_valid = groups_claim_present and isinstance(groups, list) + # Authoritative group evidence = a well-formed (list) groups claim + # from the verified id_token or the sub-bound UserInfo response. + # A missing or malformed claim is not evidence of membership loss. + groups_evidence_valid = groups_claim_present and isinstance(groups, list) # Validate claim types before use — malformed IdP claims must not # cause 500s or unsafe persistence operations. - if not isinstance(sub, str) or not sub.strip(): + # The sub is an opaque identifier (OIDC Core §8): it is validated + # for type and bounds but never normalized — trimming whitespace + # could collapse two distinct verified subjects into one local + # account and hand one subject the other's session and privileges. + if not isinstance(sub, str) or not sub: logger.error("OIDC id_token sub is not a non-empty string: %r", sub) return _build_oidc_error_redirect("oidc_failed") - sub = sub.strip() if len(sub) > 512: logger.error("OIDC id_token sub exceeds maximum length") return _build_oidc_error_redirect("oidc_failed") @@ -201,14 +207,7 @@ def setup_oidc_routes( clean_groups.append(normalized_group) groups = clean_groups - # Was UserInfo successfully fetched? When UserInfo is unavailable - # and the id_token does not carry a groups claim, we do not have - # authoritative group-membership evidence — existing admins must - # not be demoted based on missing data. userinfo_available = claims.pop("_userinfo_available", False) - # A malformed non-list groups claim is not authoritative evidence and - # must not demote an existing administrator. - id_token_has_groups = id_token_groups_valid # Determine admin status from IdP group membership. # OIDC_ADMIN_GROUPS is a comma-separated list; the user gets @@ -244,19 +243,20 @@ def setup_oidc_routes( # Otherwise the bootstrap (or manual grant) would be undone # on the next login. if admin_groups: - # Only sync admin status when we have authoritative group - # evidence. UserInfo is the primary source for groups; - # if it's unavailable and the id_token didn't carry a - # groups claim, skip the sync — a transient provider - # failure must not silently demote an existing admin. - if userinfo_available or id_token_has_groups: + # Only sync admin status when a trusted source (verified + # id_token or sub-bound UserInfo) supplied a well-formed + # groups claim. A missing or malformed claim — e.g. a + # transient provider failure or a scope/configuration + # change — must not silently demote an existing admin. + if groups_evidence_valid: logger.info("OIDC login for existing user '%s' (admin=%s)", username, is_admin) auth_manager.set_oidc_user_admin(username, is_admin) else: logger.info( "OIDC login for existing user '%s' — skipping admin sync " - "(UserInfo unavailable and id_token has no groups claim)", + "(no valid groups claim; userinfo_available=%s)", username, + userinfo_available, ) else: logger.info("OIDC login for existing user '%s'", username) @@ -285,17 +285,12 @@ def setup_oidc_routes( logger.error("Failed to create OIDC session for '%s'", username) return _build_oidc_error_redirect("oidc_failed") - # Default secure=true for OIDC flows (SSO implies a real deployment). - # Fall back to SECURE_COOKIES env var if explicitly set, then request - # scheme detection. - secure_val = _is_secure_context(request) - cookie_kwargs = dict( key=SESSION_COOKIE, value=token, httponly=True, samesite="lax", - secure=secure_val, + secure=_oidc_cookie_secure(), path="/", max_age=60 * 60 * 24 * 7, # 7 days ) @@ -309,22 +304,25 @@ def setup_oidc_routes( return router -def _is_secure_context(request: Request) -> bool: - """Determine whether the session cookie should have the Secure flag. +def _oidc_cookie_secure() -> bool: + """Determine whether OIDC cookies get the Secure flag. - Uses the SECURE_COOKIES env var if explicitly true/false; otherwise - derives from the request scheme (https → secure, http → not). + OIDC cookies are Secure by default: SSO implies a real deployment + behind TLS, and deriving the flag from SECURE_COOKIES (which the + bundled Compose files default to false) or the request scheme (which + is http behind a TLS-terminating proxy) would silently issue bearer + session cookies eligible for cleartext transmission. + + The only opt-out is the explicit development override + OIDC_ALLOW_INSECURE_COOKIES=true, for plain-HTTP local testing. + Nothing else — not SECURE_COOKIES, not the request scheme — can + downgrade OIDC cookies. """ - explicit = os.getenv("SECURE_COOKIES", "").strip().lower() - if explicit in ("true", "1", "yes"): - return True - if explicit in ("false", "0", "no"): + 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." + ) return False - # Not explicitly set — derive from the request. Forwarded headers are - # trusted only when the deployment explicitly opts into proxy headers; - # otherwise a client cannot influence the cookie policy via a header. - forwarded = "" - if os.getenv("TRUST_PROXY_HEADERS", "").strip().lower() in ("true", "1", "yes"): - forwarded = request.headers.get("x-forwarded-proto", "") - scheme = forwarded or request.url.scheme or "http" - return scheme == "https" + return True diff --git a/tests/test_oidc_manager.py b/tests/test_oidc_manager.py index 2011fd1dd..edea2d9b4 100644 --- a/tests/test_oidc_manager.py +++ b/tests/test_oidc_manager.py @@ -268,6 +268,10 @@ class TestAuthorizationUrl: parsed = parse_qs(urlparse(url).query) assert parsed.get("state") == [state] assert f"nonce={nonce}" in url + # PKCE (RFC 7636) — S256 challenge must always be sent + assert parsed.get("code_challenge_method") == ["S256"] + challenge = parsed.get("code_challenge", [""])[0] + assert len(challenge) == 43 # unpadded base64url SHA-256 # State is now a Fernet-encrypted token (base64, variable length) assert len(state) > 60 # Fernet tokens are always >60 chars assert len(nonce) == 64 # nonce is still 32 hex bytes @@ -293,6 +297,33 @@ class TestAuthorizationUrl: assert decoded is not None assert decoded["nonce"] == nonce assert decoded["redirect_uri"] == "https://app.example.com/callback" + # The PKCE verifier rides in the encrypted state and must S256-hash + # to the code_challenge sent in the authorization URL. + import base64 + import hashlib + from urllib.parse import urlparse, parse_qs + challenge = parse_qs(urlparse(url).query)["code_challenge"][0] + verifier = decoded["code_verifier"] + assert 43 <= len(verifier) <= 128 # RFC 7636 §4.1 bounds + expected = ( + base64.urlsafe_b64encode(hashlib.sha256(verifier.encode()).digest()) + .rstrip(b"=") + .decode() + ) + assert challenge == expected + + def test_state_without_code_verifier_rejected(self): + """Legacy/forged state payloads lacking a PKCE verifier are invalid.""" + import core.oidc as mod + + fernet = mod._get_state_fernet() + payload = json.dumps({ + "nonce": "n" * 64, + "redirect_uri": "https://app.example.com/callback", + "created": time.time(), + }) + state = fernet.encrypt(payload.encode()).decode() + assert mod._decode_state(state) is None class TestExchangeCode: @@ -324,7 +355,7 @@ class TestExchangeCode: url, state, gen_nonce = mgr.get_authorization_url("https://app.example.com/callback") # Override: build our own state with the nonce that matches the id_token - state = mod._encode_state(nonce, "https://app.example.com/callback") + state = mod._encode_state(nonce, "https://app.example.com/callback", "test-code-verifier") # Token exchange — mock first the JWKS fetch, then the token POST mock_post.return_value = _mock_token_response(id_token) @@ -376,7 +407,7 @@ class TestExchangeCode: ) # Build an already-expired state token - token = mod._encode_state("nonce", "https://app.example.com/callback") + token = mod._encode_state("nonce", "https://app.example.com/callback", "test-code-verifier") # Decode to verify it's valid, then re-encode with old timestamp fernet = mod._get_state_fernet() expired_data = json.dumps({ @@ -405,7 +436,7 @@ class TestExchangeCode: client_secret=FAKE_CLIENT_SECRET, ) - state = mod._encode_state("nonce", "https://app.example.com/callback") + state = mod._encode_state("nonce", "https://app.example.com/callback", "test-code-verifier") # Token response without id_token mock_post.return_value = _FakeResponse(200, {"access_token": "fake"}) @@ -429,7 +460,7 @@ class TestExchangeCode: client_secret=FAKE_CLIENT_SECRET, ) - state = mod._encode_state("nonce", "https://app.example.com/callback") + state = mod._encode_state("nonce", "https://app.example.com/callback", "test-code-verifier") mock_post.return_value = _FakeResponse(400, {"error": "invalid_grant"}) with pytest.raises(mod.OidcError): @@ -453,7 +484,7 @@ class TestExchangeCode: client_secret=FAKE_CLIENT_SECRET, ) - state = mod._encode_state(nonce, "https://app.example.com/callback") + state = mod._encode_state(nonce, "https://app.example.com/callback", "test-code-verifier") mock_post.return_value = _mock_token_response(id_token) mock_get.reset_mock() mock_get.side_effect = [ @@ -481,7 +512,7 @@ class TestExchangeCode: client_secret=FAKE_CLIENT_SECRET, ) - state = mod._encode_state(nonce, "https://app.example.com/callback") + state = mod._encode_state(nonce, "https://app.example.com/callback", "test-code-verifier") mock_post.return_value = _mock_token_response(id_token) mock_get.reset_mock() mock_get.side_effect = [ @@ -510,7 +541,7 @@ class TestExchangeCode: client_secret=FAKE_CLIENT_SECRET, ) - state = mod._encode_state(nonce, "https://app.example.com/callback") + state = mod._encode_state(nonce, "https://app.example.com/callback", "test-code-verifier") mock_post.return_value = _mock_token_response(id_token) mock_get.reset_mock() mock_get.side_effect = [ @@ -551,7 +582,7 @@ class TestExchangeCode: client_secret=FAKE_CLIENT_SECRET, ) - state = mod._encode_state(nonce, "https://app.example.com/callback") + state = mod._encode_state(nonce, "https://app.example.com/callback", "test-code-verifier") mock_post.return_value = _mock_token_response(id_token) mock_get.reset_mock() mock_get.side_effect = [ @@ -580,7 +611,7 @@ class TestExchangeCode: client_secret=FAKE_CLIENT_SECRET, ) - state = mod._encode_state(nonce, "https://app.example.com/callback") + state = mod._encode_state(nonce, "https://app.example.com/callback", "test-code-verifier") mock_post.return_value = _mock_token_response(id_token) mock_get.reset_mock() mock_get.side_effect = [ @@ -612,7 +643,7 @@ class TestExchangeCode: client_secret=FAKE_CLIENT_SECRET, ) - state = mod._encode_state(nonce, "https://app.example.com/callback") + state = mod._encode_state(nonce, "https://app.example.com/callback", "test-code-verifier") mock_post.return_value = _mock_token_response(id_token) mock_get.reset_mock() mock_get.side_effect = [ @@ -640,7 +671,7 @@ class TestExchangeCode: client_secret=FAKE_CLIENT_SECRET, ) - state = mod._encode_state(nonce, "https://app.example.com/callback") + state = mod._encode_state(nonce, "https://app.example.com/callback", "test-code-verifier") mock_post.return_value = _mock_token_response(id_token) mock_get.reset_mock() mock_get.side_effect = [ @@ -670,7 +701,7 @@ class TestExchangeCode: ) # State carries a different nonce than the id_token - state = mod._encode_state(different_nonce, "https://app.example.com/callback") + state = mod._encode_state(different_nonce, "https://app.example.com/callback", "test-code-verifier") mock_post.return_value = _mock_token_response(id_token) mock_get.reset_mock() mock_get.side_effect = [ @@ -701,7 +732,7 @@ class TestUserInfoProtection: client_secret=FAKE_CLIENT_SECRET, ) - state = mod._encode_state(nonce, "https://app.example.com/callback") + state = mod._encode_state(nonce, "https://app.example.com/callback", "test-code-verifier") mock_post.return_value = _mock_token_response(id_token) # UserInfo returns a different sub @@ -733,7 +764,7 @@ class TestUserInfoProtection: client_secret=FAKE_CLIENT_SECRET, ) - state = mod._encode_state(nonce, "https://app.example.com/callback") + state = mod._encode_state(nonce, "https://app.example.com/callback", "test-code-verifier") mock_post.return_value = _mock_token_response(id_token) # UserInfo matches sub, adds extra profile data @@ -807,7 +838,7 @@ class TestJwksCache: client_secret=FAKE_CLIENT_SECRET, ) - state = mod._encode_state(nonce, "https://app.example.com/callback") + state = mod._encode_state(nonce, "https://app.example.com/callback", "test-code-verifier") # First exchange — cache is populated mock_post.return_value = _mock_token_response(id_token) @@ -821,7 +852,7 @@ class TestJwksCache: # Second exchange with same kid — cached, no extra JWKS fetch. # But userinfo still tries to call GET on the userinfo endpoint # (which fails gracefully — logged as a warning, not a crash). - state2 = mod._encode_state(nonce, "https://app.example.com/callback") + state2 = mod._encode_state(nonce, "https://app.example.com/callback", "test-code-verifier") mock_post.return_value = _mock_token_response(id_token) mock_get.reset_mock() # Provide a userinfo mock so it doesn't count as a real failure @@ -855,7 +886,7 @@ class TestJwksCache: client_secret=FAKE_CLIENT_SECRET, ) - state = mod._encode_state(nonce, "https://app.example.com/callback") + state = mod._encode_state(nonce, "https://app.example.com/callback", "test-code-verifier") mock_post.return_value = _mock_token_response(id_token) # Simulate a network failure on the JWKS fetch inside _verify_id_token @@ -889,7 +920,7 @@ class TestJwksCache: client_secret=FAKE_CLIENT_SECRET, ) - state = mod._encode_state(nonce, "https://app.example.com/callback") + state = mod._encode_state(nonce, "https://app.example.com/callback", "test-code-verifier") mock_post.return_value = _mock_token_response(id_token) # Simulate a bad JSON response from the JWKS endpoint @@ -922,7 +953,7 @@ class TestJwksCache: client_secret=FAKE_CLIENT_SECRET, ) - state = mod._encode_state(nonce, "https://app.example.com/callback") + state = mod._encode_state(nonce, "https://app.example.com/callback", "test-code-verifier") mock_post.return_value = _mock_token_response(id_token) # Simulate HTTP 500 from the JWKS endpoint @@ -958,7 +989,7 @@ class TestStateKeyPersistence: assert not key_file.exists() # Worker A: encode state (this must create the shared key) - state_a = mod._encode_state("nonce-abc", "https://app.example.com/callback") + state_a = mod._encode_state("nonce-abc", "https://app.example.com/callback", "test-code-verifier") assert key_file.exists(), "Shared app key must be created on first state encode" assert key_file.stat().st_size > 0 @@ -988,7 +1019,7 @@ class TestStateKeyPersistence: # Simulate worker A: encode state (creates key file atomically) monkeypatch.setattr(mod, "_state_fernet", None) monkeypatch.setattr(ss, "_fernet", None) - state_a = mod._encode_state("nonce-a", "https://cb1.example.com/") + state_a = mod._encode_state("nonce-a", "https://cb1.example.com/", "test-code-verifier") assert key_file.exists() key_bytes_a = key_file.read_bytes() @@ -1000,7 +1031,7 @@ class TestStateKeyPersistence: assert decoded_b["nonce"] == "nonce-a" # Worker B encodes its own state — must use the same key - state_b = mod._encode_state("nonce-b", "https://cb2.example.com/") + state_b = mod._encode_state("nonce-b", "https://cb2.example.com/", "test-code-verifier") # After B's encode, the file must still contain worker A's key assert key_file.read_bytes() == key_bytes_a, \ "Worker B must not overwrite the key file created by worker A" @@ -1039,7 +1070,7 @@ class TestJwksCooldown: # Step 1: do one successful exchange with "test-key-1" so the # JWKS cache is populated and _fetch_jwks() returns cached data. - state = mod._encode_state(nonce, "https://app.example.com/callback") + state = mod._encode_state(nonce, "https://app.example.com/callback", "test-code-verifier") mock_post.return_value = _mock_token_response(id_token_known_kid) mock_get.reset_mock() mock_get.side_effect = [ @@ -1079,7 +1110,7 @@ class TestJwksCooldown: ).decode() # First unknown-kid attempt: JWKS refresh FAILS → cooldown set - state1 = mod._encode_state(nonce, "https://app.example.com/callback") + state1 = mod._encode_state(nonce, "https://app.example.com/callback", "test-code-verifier") mock_post.return_value = _mock_token_response(id_token_unknown_kid) mock_get.reset_mock() mock_get.side_effect = [ @@ -1094,7 +1125,7 @@ class TestJwksCooldown: # Second unknown-kid attempt: cooldown still active → throttled, # _refresh_jwks must NOT be called. The exchange will fail # because the key for "test-key-2" is not in the stale cache. - state2 = mod._encode_state(nonce, "https://app.example.com/callback") + state2 = mod._encode_state(nonce, "https://app.example.com/callback", "test-code-verifier") mock_post.return_value = _mock_token_response(id_token_unknown_kid) mock_get.reset_mock() # If _refresh_jwks were called, it would hit this side_effect. @@ -1128,7 +1159,7 @@ class TestRedirectUriBinding: ) # State encodes "https://original.example.com/callback" - state = mod._encode_state("nonce", "https://original.example.com/callback") + state = mod._encode_state("nonce", "https://original.example.com/callback", "test-code-verifier") # But the callback derives a different redirect_uri with pytest.raises(mod.OidcError, match="redirect_uri mismatch"): @@ -1157,7 +1188,7 @@ class TestRedirectUriBinding: client_secret=FAKE_CLIENT_SECRET, ) - state = mod._encode_state(nonce, stored_uri) + state = mod._encode_state(nonce, stored_uri, "test-code-verifier") mock_post.return_value = _mock_token_response(id_token) mock_get.reset_mock() mock_get.side_effect = [ @@ -1206,7 +1237,7 @@ class TestUserinfoEndpointMissing: # _mock_token_response already includes an access_token. mock_post.return_value = _mock_token_response(id_token) - state = mod._encode_state(nonce, "https://app.example.com/callback") + state = mod._encode_state(nonce, "https://app.example.com/callback", "test-code-verifier") claims = mgr.exchange_code("code", state, "https://app.example.com/callback") assert claims["sub"] == "user-no-ui" @@ -1240,7 +1271,7 @@ class TestUserinfoEndpointMissing: mock_post.return_value = _mock_token_response(id_token) - state = mod._encode_state(nonce, "https://app.example.com/callback") + state = mod._encode_state(nonce, "https://app.example.com/callback", "test-code-verifier") claims = mgr.exchange_code("code", state, "https://app.example.com/callback") assert claims["sub"] == "user-with-ui" @@ -1389,7 +1420,7 @@ def _exchange_with_userinfo(userinfo): mgr = _new_security_test_manager() nonce = "u" * 64 - state = mod._encode_state(nonce, "https://app.example.com/callback") + state = mod._encode_state(nonce, "https://app.example.com/callback", "test-code-verifier") mgr._token_request = MagicMock(return_value={ "access_token": "access-token", "id_token": "unused-in-this-unit-test", @@ -1476,10 +1507,13 @@ class TestNumericDateClaimValidation: with pytest.raises(mod.OidcError): mgr._verify_id_token(token, "f" * 64) - def test_iat_none_accepted(self): + def test_iat_missing_rejected(self): + # OIDC Core §2: iat is REQUIRED — a token without it must not verify. + import core.oidc as mod mgr = _new_security_test_manager() token = _make_claim_test_token("user123", "g" * 64, iat=None) - mgr._verify_id_token(token, "g" * 64) + with pytest.raises(mod.OidcError, match="missing iat"): + mgr._verify_id_token(token, "g" * 64) def test_iat_valid_accepted(self): mgr = _new_security_test_manager() @@ -1489,6 +1523,120 @@ class TestNumericDateClaimValidation: mgr._verify_id_token(token, "h" * 64) +def _make_manager(discovery_doc=None): + """Build an OidcManager against a mocked discovery endpoint.""" + import core.oidc as mod + jwt_jwks, _ = _make_test_jwks_and_key() + with patch.object(mod.httpx, "get") as mock_get: + mock_get.side_effect = [ + _FakeResponse(200, discovery_doc or DISCOVERY_DOC), + _mock_jwks_response(jwt_jwks), + ] + return mod.OidcManager( + issuer=FAKE_ISSUER, + client_id=FAKE_CLIENT_ID, + client_secret=FAKE_CLIENT_SECRET, + ) + + +class TestPkceTokenRequest: + def test_code_verifier_sent_to_token_endpoint(self): + """The verifier recovered from state must be POSTed to the token + endpoint and must hash to the challenge from the auth URL.""" + import base64 + import hashlib + from urllib.parse import urlparse, parse_qs + import core.oidc as mod + + jwt_jwks, _ = _make_test_jwks_and_key() + mgr = _make_manager() + url, state, nonce = mgr.get_authorization_url("https://app.example.com/callback") + challenge = parse_qs(urlparse(url).query)["code_challenge"][0] + id_token = _make_id_token("user123", nonce) + + with patch.object(mod.httpx, "get") as mock_get, \ + patch.object(mod.httpx, "post") as mock_post: + mock_get.side_effect = [_mock_jwks_response(jwt_jwks)] + mock_post.return_value = _mock_token_response(id_token) + mgr.exchange_code("auth_code_xyz", state, "https://app.example.com/callback") + + posted = mock_post.call_args.kwargs["data"] + verifier = posted["code_verifier"] + expected = ( + base64.urlsafe_b64encode(hashlib.sha256(verifier.encode()).digest()) + .rstrip(b"=") + .decode() + ) + assert challenge == expected + + +class TestTokenEndpointAuth: + def _exchange(self, discovery_doc): + """Run a full exchange and return the httpx.post call kwargs.""" + import core.oidc as mod + jwt_jwks, _ = _make_test_jwks_and_key() + mgr = _make_manager(discovery_doc) + _, state, nonce = mgr.get_authorization_url("https://app.example.com/callback") + id_token = _make_id_token("user123", nonce) + with patch.object(mod.httpx, "get") as mock_get, \ + patch.object(mod.httpx, "post") as mock_post: + mock_get.side_effect = [_mock_jwks_response(jwt_jwks)] + mock_post.return_value = _mock_token_response(id_token) + mgr.exchange_code("code", state, "https://app.example.com/callback") + return mock_post.call_args.kwargs + + def test_default_uses_client_secret_basic(self): + """No token_endpoint_auth_methods_supported in discovery → the OIDC + default client_secret_basic: HTTP Basic auth, no secret in the body.""" + kwargs = self._exchange(DISCOVERY_DOC) + assert kwargs["auth"] == (FAKE_CLIENT_ID, FAKE_CLIENT_SECRET) + assert "client_secret" not in kwargs["data"] + assert "client_id" not in kwargs["data"] + + def test_basic_preferred_when_advertised(self): + doc = dict(DISCOVERY_DOC) + doc["token_endpoint_auth_methods_supported"] = [ + "client_secret_post", "client_secret_basic", + ] + kwargs = self._exchange(doc) + assert kwargs["auth"] == (FAKE_CLIENT_ID, FAKE_CLIENT_SECRET) + assert "client_secret" not in kwargs["data"] + + def test_post_fallback_when_basic_unsupported(self): + doc = dict(DISCOVERY_DOC) + doc["token_endpoint_auth_methods_supported"] = ["client_secret_post"] + kwargs = self._exchange(doc) + assert kwargs["auth"] is None + assert kwargs["data"]["client_secret"] == FAKE_CLIENT_SECRET + assert kwargs["data"]["client_id"] == FAKE_CLIENT_ID + + +class TestHttpsEnforcement: + def test_http_issuer_rejected(self): + import core.oidc as mod + with patch.object(mod.httpx, "get") as mock_get: + mock_get.return_value = _mock_discovery_response() + with pytest.raises(mod.OidcError, match="issuer must use HTTPS"): + mod.OidcManager( + issuer="http://idp.example.com", + client_id=FAKE_CLIENT_ID, + client_secret=FAKE_CLIENT_SECRET, + ) + + def test_http_authorization_endpoint_rejected(self): + import core.oidc as mod + doc = dict(DISCOVERY_DOC) + doc["authorization_endpoint"] = "http://idp.example.com/authorize" + with patch.object(mod.httpx, "get") as mock_get: + mock_get.return_value = _FakeResponse(200, doc) + with pytest.raises(mod.OidcError, match="authorization_endpoint must use HTTPS"): + mod.OidcManager( + issuer=FAKE_ISSUER, + client_id=FAKE_CLIENT_ID, + client_secret=FAKE_CLIENT_SECRET, + ) + + class TestMaxAgeConfiguration: def test_max_age_added_to_auth_url(self): mgr = _new_security_test_manager(max_age=3600) diff --git a/tests/test_oidc_routes.py b/tests/test_oidc_routes.py index 4dfe96426..f722ffb9e 100644 --- a/tests/test_oidc_routes.py +++ b/tests/test_oidc_routes.py @@ -954,6 +954,197 @@ class TestAdminDemotionProtection: # An existing admin must not be demoted on missing evidence. auth.set_oidc_user_admin.assert_not_called() + def _run_admin_sync_callback(self, claims, monkeypatch): + monkeypatch.setenv("OIDC_ADMIN_GROUPS", "odysseus-admins") + mgr = MagicMock() + mgr.configured = True + mgr.redirect_uri_override = None + mgr.issuer = "https://idp.example.com" + mgr.exchange_code.return_value = claims + + auth = MagicMock() + auth.get_user_by_oidc.return_value = "existing-admin" + auth.create_session_trusted.return_value = "token" + + router = _setup_oidc_routes(auth, mgr) + ep = _get_endpoint(router, "/api/auth/oidc/callback") + import asyncio + asyncio.run( + ep( + _fake_request_with_params( + {"code": "code", "state": "state"}, + cookies={"odysseus_oidc_csrf": "state"}, + ), + SimpleNamespace( + set_cookie=MagicMock(), + delete_cookie=MagicMock(), + status_code=200, + headers={}, + ), + ) + ) + return auth + + def test_userinfo_available_without_groups_preserves_admin(self, monkeypatch): + """UserInfo fetched but carrying no groups claim is NOT evidence of + membership loss — availability alone must not drive a demotion.""" + auth = self._run_admin_sync_callback({ + "sub": "existing-admin", + "email": "admin@example.com", + "_userinfo_available": True, + # no "groups" key anywhere + }, monkeypatch) + auth.set_oidc_user_admin.assert_not_called() + + def test_malformed_groups_claim_preserves_admin(self, monkeypatch): + """A non-list groups value is not valid evidence and must not + demote an existing administrator.""" + auth = self._run_admin_sync_callback({ + "sub": "existing-admin", + "email": "admin@example.com", + "_userinfo_available": True, + "groups": "odysseus-admins", # malformed: string, not list + }, monkeypatch) + auth.set_oidc_user_admin.assert_not_called() + + +class TestSubjectIdentifierPreservation: + """Regression: OIDC subs are opaque — whitespace-bearing subjects must + not be normalized into a different subject's account.""" + + def _run_callback_with_sub(self, sub): + mgr = MagicMock() + mgr.configured = True + mgr.redirect_uri_override = None + mgr.issuer = "https://idp.example.com" + mgr.exchange_code.return_value = { + "sub": sub, + "email": "alice@example.com", + "preferred_username": "alice", + } + + auth = MagicMock() + auth.check_oidc_totp.return_value = False + auth.get_user_by_oidc.return_value = None + auth.create_user_oidc.return_value = "alice" + auth.create_session_trusted.return_value = "token" + + router = _setup_oidc_routes(auth, mgr) + ep = _get_endpoint(router, "/api/auth/oidc/callback") + import asyncio + asyncio.run( + ep( + _fake_request_with_params( + {"code": "code", "state": "state"}, + cookies={"odysseus_oidc_csrf": "state"}, + ), + SimpleNamespace( + set_cookie=MagicMock(), + delete_cookie=MagicMock(), + status_code=200, + headers={}, + ), + ) + ) + return auth + + def test_whitespace_bearing_sub_preserved_exactly(self): + sub = " user123 " + auth = self._run_callback_with_sub(sub) + auth.get_user_by_oidc.assert_called_once_with(sub, "https://idp.example.com") + auth.create_user_oidc.assert_called_once_with( + "alice", sub, "https://idp.example.com", + email="alice@example.com", is_admin=False, + ) + + def test_distinct_whitespace_subs_lookup_distinctly(self): + """'user123' and ' user123 ' must hit the account store as two + different keys, never collapsing into one local account.""" + auth_a = self._run_callback_with_sub("user123") + auth_b = self._run_callback_with_sub(" user123 ") + (sub_a, _), _ = auth_a.get_user_by_oidc.call_args + (sub_b, _), _ = auth_b.get_user_by_oidc.call_args + assert sub_a != sub_b + + def test_whitespace_only_sub_rejected(self): + auth = self._run_callback_with_sub(" ") + # " " is a technically valid opaque sub per spec; ensure it is + # either used exactly or rejected — never trimmed to empty and + # never persisted as a different identifier. + if auth.create_user_oidc.called: + args, kwargs = auth.create_user_oidc.call_args + assert args[1] == " " + + +class TestOidcCookieSecurity: + """Regression: OIDC cookies must be Secure by default, regardless of + SECURE_COOKIES (which the bundled Compose files default to false).""" + + def _login_cookie_kwargs(self): + mgr = MagicMock() + mgr.configured = True + mgr.redirect_uri_override = None + mgr.get_authorization_url.return_value = ( + "https://idp.example.com/authorize?state=abc", "abc", "def", + ) + router = _setup_oidc_routes(MagicMock(), mgr) + ep = _get_endpoint(router, "/api/auth/oidc/login") + import asyncio + with patch("fastapi.responses.RedirectResponse.set_cookie") as sc: + asyncio.run(ep(_fake_request())) + return sc.call_args.kwargs + + def test_csrf_cookie_secure_despite_secure_cookies_false(self, monkeypatch): + monkeypatch.setenv("SECURE_COOKIES", "false") + monkeypatch.delenv("OIDC_ALLOW_INSECURE_COOKIES", raising=False) + assert self._login_cookie_kwargs()["secure"] is True + + def test_csrf_cookie_secure_on_http_request(self, monkeypatch): + """Plain-http request scheme must not downgrade the cookie.""" + monkeypatch.delenv("SECURE_COOKIES", raising=False) + monkeypatch.delenv("OIDC_ALLOW_INSECURE_COOKIES", raising=False) + assert self._login_cookie_kwargs()["secure"] is True + + def test_explicit_dev_override_allows_insecure(self, monkeypatch): + monkeypatch.setenv("OIDC_ALLOW_INSECURE_COOKIES", "true") + assert self._login_cookie_kwargs()["secure"] is False + + def test_session_cookie_secure_despite_secure_cookies_false(self, monkeypatch): + monkeypatch.setenv("SECURE_COOKIES", "false") + monkeypatch.delenv("OIDC_ALLOW_INSECURE_COOKIES", raising=False) + + mgr = MagicMock() + mgr.configured = True + mgr.redirect_uri_override = None + mgr.issuer = "https://idp.example.com" + mgr.exchange_code.return_value = { + "sub": "user123", "email": "alice@example.com", + } + auth = MagicMock() + auth.check_oidc_totp.return_value = False + auth.get_user_by_oidc.return_value = "alice" + auth.create_session_trusted.return_value = "token" + + router = _setup_oidc_routes(auth, mgr) + ep = _get_endpoint(router, "/api/auth/oidc/callback") + set_cookie = MagicMock() + import asyncio + asyncio.run( + ep( + _fake_request_with_params( + {"code": "code", "state": "state"}, + cookies={"odysseus_oidc_csrf": "state"}, + ), + SimpleNamespace( + set_cookie=set_cookie, + delete_cookie=MagicMock(), + status_code=200, + headers={}, + ), + ) + ) + assert set_cookie.call_args.kwargs["secure"] is True + class TestOidcCallbackSecurityFailures: def _run_callback(self, auth, mgr, params, cookies=None): diff --git a/tests/test_session_cross_worker.py b/tests/test_session_cross_worker.py new file mode 100644 index 000000000..48ffb8d58 --- /dev/null +++ b/tests/test_session_cross_worker.py @@ -0,0 +1,84 @@ +"""Regression: sessions issued by one uvicorn worker must validate on another. + +The OIDC callback (or a password login) can run on worker A while the +browser's next request lands on worker B. Each worker loads sessions.json +only at startup, so without the read-through reload in +AuthManager._reload_sessions_if_changed the new token would be rejected +and a successful login would immediately become a logged-out session. +""" + +import importlib +import sys +import types +from pathlib import Path + +from tests.helpers.import_state import clear_module + + +def _real_core_package(): + root = Path(__file__).resolve().parent.parent + core_path = str(root / "core") + core = sys.modules.get("core") + if core is None: + core = types.ModuleType("core") + sys.modules["core"] = core + core.__path__ = [core_path] + clear_module("core.auth") + return core + + +def _auth_module(): + _real_core_package() + return importlib.import_module("core.auth") + + +def _two_workers(tmp_path): + """Build two AuthManager instances over the same data directory, + simulating two uvicorn worker processes.""" + 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") + worker_a = auth_mod.AuthManager(auth_path) + assert worker_a.create_user("alice", "password-1", is_admin=False) + worker_b = auth_mod.AuthManager(auth_path) # boots after user exists + return worker_a, worker_b + + +class TestCrossWorkerSessions: + def test_session_issued_on_other_worker_validates(self, tmp_path): + worker_a, worker_b = _two_workers(tmp_path) + token = worker_a.create_session_trusted("alice") + assert token is not None + # Worker B has never seen this token in memory — it must read + # through to sessions.json and accept it. + assert worker_b.validate_token(token) is True + assert worker_b.get_username_for_token(token) == "alice" + + def test_unknown_token_still_rejected(self, tmp_path): + worker_a, worker_b = _two_workers(tmp_path) + worker_a.create_session_trusted("alice") + assert worker_b.validate_token("f" * 64) is False + assert worker_b.get_username_for_token("f" * 64) is None + + def test_expired_session_from_other_worker_rejected(self, tmp_path): + auth_mod = _auth_module() + worker_a, worker_b = _two_workers(tmp_path) + token = worker_a.create_session_trusted("alice") + # Force the persisted expiry into the past, as another worker + # would see it after the TTL elapsed. + with worker_a._sessions_lock: + worker_a._sessions[token]["expiry"] = 1.0 + worker_a._save_sessions() + assert worker_b.validate_token(token) is False + assert worker_b.get_username_for_token(token) is None + + def test_reload_is_additive_not_destructive(self, tmp_path): + """A reload must never drop tokens this worker already holds in + memory (e.g. one issued moments ago, racing its own save).""" + worker_a, worker_b = _two_workers(tmp_path) + token_b = worker_b.create_session_trusted("alice") + token_a = worker_a.create_session_trusted("alice") + # 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