diff --git a/.env.example b/.env.example index 063b9f039..36cba2f47 100644 --- a/.env.example +++ b/.env.example @@ -126,6 +126,10 @@ SEARXNG_INSTANCE=http://localhost:8080 # iframe/subresource callbacks. When deploying behind a reverse proxy, # use OIDC_REDIRECT_URI to ensure the callback origin matches the # browser-visible origin. +# SECURE_COOKIES=true should be set for HTTPS deployments. Set +# 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. # ============================================================ # ChromaDB (vector store) diff --git a/core/auth.py b/core/auth.py index 6cd822131..f3700693a 100644 --- a/core/auth.py +++ b/core/auth.py @@ -486,6 +486,20 @@ class AuthManager: return False # not an OIDC user (or removed) — don't touch if user.get("is_admin") == is_admin: return True # no change needed + # Refuse to demote the only remaining administrator. Group + # membership changes must not silently make the instance + # unadministrable; password admins count as recovery admins. + if user.get("is_admin") and not is_admin: + admin_count = sum( + 1 for data in self._config.get("users", {}).values() + if data.get("is_admin") + ) + if admin_count <= 1: + logger.warning( + "Refusing to demote last admin '%s' during OIDC sync", + username, + ) + return False self._config["users"][username]["is_admin"] = is_admin if is_admin: self._config["users"][username]["privileges"] = dict(ADMIN_PRIVILEGES) diff --git a/core/oidc.py b/core/oidc.py index 742c05bc9..c6c34ef2a 100644 --- a/core/oidc.py +++ b/core/oidc.py @@ -93,7 +93,19 @@ def _decode_state(state: str) -> Optional[Dict[str, Any]]: data = json.loads(plain) except Exception: return None - if time.time() - data.get("created", 0) > _STATE_TTL: + if not isinstance(data, dict): + return None + nonce = data.get("nonce") + redirect_uri = data.get("redirect_uri") + created = data.get("created") + if not isinstance(nonce, str) or not nonce: + return None + if not isinstance(redirect_uri, str): + return None + if not _is_numericdate(created): + return None + now = time.time() + if created > now + 60 or now - created > _STATE_TTL: return None return data @@ -192,6 +204,19 @@ 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"): + url = self._config.get(name) + if url and not isinstance(url, str): + raise OidcError(f"OIDC {name} must be a URL string") + if url and not url.startswith("https://"): + raise OidcError( + f"OIDC {name} must use HTTPS, got {url!r}. " + "Configure the IdP with TLS or set OIDC_ENABLED=false." + ) + # Pin signing algorithms to those the provider supports. # Restrict to RS256/ES256 to avoid algorithm confusion attacks; # HS256 and 'none' are never allowed. @@ -441,26 +466,45 @@ class OidcManager: from authlib.jose.errors import JoseError header = self._peek_jwt_header(id_token) + alg = header.get("alg", "") kid = header.get("kid", "") + # Reject disallowed algorithms before importing keys or verifying the + # signature. This keeps the JOSE policy independent of Authlib's + # decoded-claims header API. + if not alg or (self._allowed_algs and alg not in self._allowed_algs): + raise OidcError( + f"id_token signed with disallowed algorithm {alg!r} " + f"(allowed: {self._allowed_algs!r})" + ) + # Fetch or refresh JWKS jwks = self._fetch_jwks() # If the kid from the token header is unknown, refresh the cache. # Guarded by a cooldown so an attacker can't drive unbounded # outbound fetches by sending random kid values to the callback. - if kid and kid not in self._jwks_cache: - now = time.time() - last_refresh = getattr(self, "_last_jwks_refresh", 0) - if now - last_refresh >= 60: - logger.info("OIDC JWKS cache miss for kid=%r — refreshing", kid) - self._last_jwks_refresh = now # record attempt before refresh so failed fetches are also throttled - jwks = self._refresh_jwks() - else: - logger.warning( - "OIDC JWKS cache miss for kid=%r but refresh on cooldown " - "(%.0fs remaining)", kid, 60 - (now - last_refresh), - ) + refresh_jwks = False + if kid: + with self._jwks_cache_lock: + if kid not in self._jwks_cache: + now = time.time() + last_refresh = getattr(self, "_last_jwks_refresh", 0) + if now - last_refresh >= 60: + logger.info("OIDC JWKS cache miss for kid=%r — refreshing", kid) + # Record the attempt before I/O so failed refreshes + # are throttled too. The lock protects this marker + # against concurrent callback threads. + self._last_jwks_refresh = now + refresh_jwks = True + else: + logger.warning( + "OIDC JWKS cache miss for kid=%r but refresh on cooldown " + "(%.0fs remaining)", + kid, 60 - (now - last_refresh), + ) + if refresh_jwks: + jwks = self._refresh_jwks() # authlib needs a key set in the format it expects try: @@ -474,15 +518,6 @@ class OidcManager: except JoseError as exc: raise OidcError(f"id_token signature verification failed: {exc}") from exc - # Verify the algorithm is in our allow-list - claims_header = getattr(claims, "header", {}) if hasattr(claims, "header") else {} - alg = claims_header.get("alg", "") - if alg and self._allowed_algs and alg not in self._allowed_algs: - raise OidcError( - f"id_token signed with disallowed algorithm {alg!r} " - f"(allowed: {self._allowed_algs!r})" - ) - claims = dict(claims) # Manual claim validation — more explicit and version-agnostic @@ -504,24 +539,26 @@ class OidcManager: raise OidcError( f"id_token aud mismatch: client_id {self.client_id!r} not in aud {aud!r}" ) - if len(aud_list) > 1: - azp = claims.get("azp") - if not azp: - raise OidcError( - "id_token has multiple audiences but no azp claim " - "(required by OIDC Core 1.0 § 2)" - ) - if azp != self.client_id: - raise OidcError( - f"id_token azp mismatch: expected {self.client_id!r}, got {azp!r}" - ) + if len(aud_list) > 1 and not claims.get("azp"): + raise OidcError( + "id_token has multiple audiences but no azp claim " + "(required by OIDC Core 1.0 § 2)" + ) + # OIDC Core § 2: whenever azp is present, it must identify this RP, + # including single-audience tokens. + azp = claims.get("azp") + if azp is not None and azp != self.client_id: + raise OidcError( + f"id_token azp mismatch: expected {self.client_id!r}, got {azp!r}" + ) exp = claims.get("exp", 0) if not _is_numericdate(exp) or time.time() > exp: raise OidcError(f"id_token expired at {exp}") - # Verify nonce - if claims.get("nonce") != nonce: + # Verify nonce with constant-time comparison. + token_nonce = claims.get("nonce", "") + if not isinstance(token_nonce, str) or not secrets.compare_digest(token_nonce, nonce): raise OidcError("id_token nonce mismatch") # Verify auth_time when max_age was requested. @@ -568,7 +605,8 @@ class OidcManager: # Pad to a multiple of 4 (base64url) pad_len = (-len(parts[0])) % 4 padded = parts[0] + ("=" * pad_len) - return json.loads(base64.urlsafe_b64decode(padded)) + header = json.loads(base64.urlsafe_b64decode(padded)) + return header if isinstance(header, dict) else {} except Exception: pass return {} @@ -616,7 +654,10 @@ def init_oidc_manager() -> Optional[OidcManager]: client_id = os.getenv("OIDC_CLIENT_ID", "").strip() client_secret = os.getenv("OIDC_CLIENT_SECRET", "").strip() scopes = os.getenv("OIDC_SCOPES", "openid profile email").strip() - max_age = _parse_max_age() + scope_list = [s for s in scopes.split() if s] + if "openid" not in scope_list: + scope_list.insert(0, "openid") + scopes = " ".join(scope_list) if not issuer or not client_id or not client_secret: _oidc_init_error = ( @@ -627,6 +668,7 @@ def init_oidc_manager() -> Optional[OidcManager]: return None try: + max_age = _parse_max_age() _oidc_manager = OidcManager( issuer=issuer, client_id=client_id, diff --git a/requirements.txt b/requirements.txt index 4574b3c89..4c1ba6cf1 100644 --- a/requirements.txt +++ b/requirements.txt @@ -41,7 +41,7 @@ bcrypt mcp pyotp qrcode[pil] -authlib +authlib>=1.3.0,<2 croniter pytest pytest-asyncio diff --git a/routes/oidc_routes.py b/routes/oidc_routes.py index 352f7ec43..719d38edc 100644 --- a/routes/oidc_routes.py +++ b/routes/oidc_routes.py @@ -57,8 +57,10 @@ def setup_oidc_routes( """ from core.oidc import get_oidc_init_error if oidc_manager is None or not oidc_manager.configured: - error = get_oidc_init_error() - return {"enabled": False, "error": error or "OIDC not configured"} + init_error = get_oidc_init_error() + if init_error: + logger.warning("OIDC config endpoint: %s", init_error) + return {"enabled": False, "error": "OIDC not configured"} return { "enabled": True, "provider_name": oidc_manager.provider_name, @@ -160,18 +162,53 @@ def setup_oidc_routes( email = claims.get("email", "") preferred_username = claims.get("preferred_username", "") 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) + + # 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(): + 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") + if not isinstance(email, str) or len(email) > 512: + email = "" + if not isinstance(preferred_username, str) or len(preferred_username) > 256: + preferred_username = "" + if not isinstance(name, str) or len(name) > 512: + name = "" + if not isinstance(groups, list): + groups = [] + else: + clean_groups = [] + for group in groups: + if isinstance(group, str): + if len(group) <= 256: + clean_groups.append(group) + else: + logger.warning("OIDC groups claim element exceeds maximum length") + else: + logger.warning( + "OIDC groups claim contained non-string element %r — coerced", + group, + ) + normalized_group = str(group) + if len(normalized_group) <= 256: + 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) - id_token_has_groups = "groups" in claims - - if not sub: - logger.error("OIDC id_token missing sub claim") - return _build_oidc_error_redirect("oidc_failed") + # 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 @@ -283,6 +320,11 @@ def _is_secure_context(request: Request) -> bool: return True if explicit in ("false", "0", "no"): return False - # Not explicitly set — derive from the request - scheme = request.headers.get("x-forwarded-proto", "") or request.url.scheme or "http" + # 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" diff --git a/tests/test_oidc_auth.py b/tests/test_oidc_auth.py index cb821f424..8b7b9fc38 100644 --- a/tests/test_oidc_auth.py +++ b/tests/test_oidc_auth.py @@ -267,6 +267,9 @@ def test_set_oidc_user_admin_demotes(tmp_path): mgr = _make_manager(tmp_path) mgr.create_user_oidc("alice", sub="abc", issuer="https://idp.example.com", is_admin=True) + # A second administrator must exist before group sync can demote alice; + # otherwise the last-admin guard preserves recoverability. + mgr.create_user("recovery", "password123", is_admin=True) assert mgr.is_admin("alice") assert mgr.set_oidc_user_admin("alice", False) assert not mgr.is_admin("alice") @@ -605,8 +608,10 @@ class TestInterprocessFirstAdminSerialisation: assert alice == "alice" bob = mgr_a.create_user_oidc("bob", "sub-b", "https://idp.example.com") assert bob == "bob" - # Make alice an admin so the no-op short-circuit doesn't trigger + # Make both OIDC users admins so demoting alice does not trigger the + # last-admin guard, and the no-op short-circuit doesn't trigger. mgr_a._config["users"]["alice"]["is_admin"] = True + mgr_a._config["users"]["bob"]["is_admin"] = True mgr_a._save() # Manager B: loaded from disk but now we make its in-memory state @@ -713,12 +718,14 @@ class TestInterprocessFirstAdminSerialisation: auth_path = str(tmp_path / "auth.json") # Manager A: create an OIDC user (alice) then make her admin so the - # no-op short-circuit in set_oidc_user_admin doesn't trigger. + # no-op short-circuit in set_oidc_user_admin doesn't trigger. A + # recovery admin ensures concurrent demotion is permitted. mgr_a = AuthManager(auth_path) alice = mgr_a.create_user_oidc("alice", "sub-a", "https://idp.example.com") assert alice == "alice" mgr_a._config["users"]["alice"]["is_admin"] = True mgr_a._save() + assert mgr_a.create_user("recovery", "password123", is_admin=True) # Manager B: a separate instance with the same auth file. mgr_b = AuthManager(auth_path) diff --git a/tests/test_oidc_routes.py b/tests/test_oidc_routes.py index ca16aca4a..4dfe96426 100644 --- a/tests/test_oidc_routes.py +++ b/tests/test_oidc_routes.py @@ -12,6 +12,15 @@ from fastapi import APIRouter def _setup_oidc_routes(auth_manager, oidc_manager): """Import and call setup_oidc_routes, returning the router.""" + # MagicMock methods are truthy by default; explicitly model the normal + # OIDC-user case so the defense-in-depth TOTP refusal does not alter every + # unrelated route test. + if isinstance(auth_manager, MagicMock): + # Preserve an explicit test override (e.g. return_value=True in the + # refusal-path test), but make bare MagicMock auth managers behave + # like normal OIDC users. + if isinstance(auth_manager.check_oidc_totp.return_value, MagicMock): + auth_manager.check_oidc_totp.return_value = False from routes.oidc_routes import setup_oidc_routes return setup_oidc_routes(auth_manager, oidc_manager)