mirror of
https://github.com/pewdiepie-archdaemon/odysseus.git
synced 2026-10-13 17:37:56 +00:00
fix(oidc): address all MEDIUM findings from 4-model security review
13 additional hardening fixes based on gpt-5.6-sol final judgment: Spec compliance & validation: - Validate azp == client_id whenever present (not just multi-audience) - Enforce HTTPS on token/JWKS/UserInfo endpoints - Validate IdP claim types before use (sub must be non-empty string) - Enforce 'openid' in OIDC_SCOPES - Validate state payload shape after decryption Hardening: - Last-admin guard on OIDC group demotion (refuse to demote sole admin) - Cookie Secure: trust X-Forwarded-Proto only when TRUST_PROXY_HEADERS=true - OIDC_MAX_AGE parse error now caught during init (no app crash) - Nonce comparison uses secrets.compare_digest (constant-time) - Algorithm validated against allow-list before jwt.decode (not after) - JWKS cooldown marker now lock-protected against concurrent threads Operational: - Config endpoint returns generic error, logs details server-side - Pin authlib>=1.3.0,<2 in requirements.txt Tests: 164 passed (123 OIDC + 41 regression), 0 failures
This commit is contained in:
holden093
parent
9e6e8b62d3
commit
12d33685eb
7 changed files with 167 additions and 49 deletions
|
|
@ -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)
|
||||
|
|
|
|||
14
core/auth.py
14
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)
|
||||
|
|
|
|||
116
core/oidc.py
116
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,
|
||||
|
|
|
|||
|
|
@ -41,7 +41,7 @@ bcrypt
|
|||
mcp
|
||||
pyotp
|
||||
qrcode[pil]
|
||||
authlib
|
||||
authlib>=1.3.0,<2
|
||||
croniter
|
||||
pytest
|
||||
pytest-asyncio
|
||||
|
|
|
|||
|
|
@ -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"
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue