feat(auth): add generic OpenID Connect (OIDC) single sign-on#3508
feat(auth): add generic OpenID Connect (OIDC) single sign-on#3508holden093 wants to merge 30 commits into
Conversation
github-actions
Bot
added
needs work
vdmkenny
left a comment
vdmkenny
left a comment
There was a problem hiding this comment.
First-pass review (auth flow only; not exhaustive). Up front on process: the project's current focus is bug-fixing and code cleanup, so large new features carry a high bar, and AI-generated ones especially. A feature of this size and sensitivity (authentication) needs sign-off from multiple maintainers before it can be considered, so treat this as a direction/quality pass rather than a path to near-term merge.
Credit where due: using authlib for discovery + JWKS verification instead of hand-rolling, verifying iss/aud/exp/nonce, server-side state CSRF, passwordless OIDC users that can't be password-authenticated, and (sub, issuer) identity keying with suffixed username collisions (no account takeover) are all the right calls. OIDC_ENABLED defaulting off is correct.
Issues to address:
-
audrejects valid multi-audience tokens.core/oidc.py:if claims.get("aud") != self.client_id. Per the OIDC specaudmay be a JSON array. A token withaud: ["odysseus"](or client_id plus extras) is wrongly rejected. Normalize to a list and check membership (and, whenaudhas multiple values, validateazp == client_id). -
redirect_uriis derived fromrequest.base_url(Host header). Built from the inbound request in both/loginand/callback. Behind a proxy this depends on trusted forwarded headers, and a spoofed Host can poison the value. IdP allow-listing limits the blast radius, but please support a configurable fixed public base /OIDC_REDIRECT_URIand stop trusting the Host header for this. -
Possible zero-admin lockout. Admin is derived solely from
OIDC_ADMIN_GROUPSintersecting thegroupsclaim, re-synced every login, with no fallback. In an OIDC-only deployment withOIDC_ADMIN_GROUPSunset (or after a group is removed), there is no admin at all. Needs a documented bootstrap path (e.g. keep a password admin, or first-OIDC-user-is-admin guard) so an install can't end up unadministrable. -
JWKS fetched on every login.
_verify_id_tokendoeshttpx.get(jwks_uri)per call. Cache the key set (with refresh on unknownkid) so sign-in does not depend on a live IdP round-trip each time and to avoid the added latency / failure surface. -
Signing alg not pinned.
jwt.decode(id_token, key_set)has no algorithm allow-list. authlib + a typed RSA key set mitigates alg-confusion, but pin the expected algs explicitly (e.g. from the discovery doc'sid_token_signing_alg_values_supported, restricted to RS256/ES256) rather than relying on key typing. -
In-memory
statestore breaks multi-worker._state_storeis a module-level dict, so underuvicorn --workers >1(or multiple processes) the callback can land on a worker without the state and fail intermittently. Either document single-worker-only or back it with a shared store.
I did not deep-review the 3 test files or the app.py/UI wiring in this pass, and CI's Python tests have not reported yet on this branch. Not a merge recommendation either way.
|
Cross-linking prior art for maintainer context: this is the third iteration of this feature. The author's earlier copies #3487 and #3301 were both closed (superseded by this PR), and the tracking issue is #806 (already linked above). No other OIDC/SSO PRs or issues exist. Noting it so the multi-maintainer review can see the lineage; the review feedback to act on is in this PR's first-pass review above. |
|
All six review items addressed in d53cdc8:
Tests: 105 passing (63 OIDC + 42 regression), 0 failures. Ready for re-review. |
There was a problem hiding this comment.
Re-review of d53cdc8. The six items from the previous pass are addressed: aud arrays + azp, JWKS caching, algorithm pinning, OIDC_REDIRECT_URI, Fernet stateless state, and the first-user-admin bootstrap. Linking on (sub, issuer) rather than email is the right call and avoids email-based account takeover. Thank you for the thorough rework.
This pass surfaced five issues that need to be resolved before this can move forward. The first is a functional lockout; the rest are security-relevant for an authentication feature, so I am not treating any of them as optional.
1. Bootstrap admin is demoted on the second login (zero-admin lockout).
routes/oidc_routes.py runs set_oidc_user_admin(username, is_admin) on every login for an existing user, and is_admin is computed solely from OIDC_ADMIN_GROUPS (lines 136-143). When that env var is unset (the exact scenario the bootstrap exists for):
- First login:
create_user_oidcpromotes the first user to admin. - Second login: the user is found,
is_adminisFalse, soset_oidc_user_admin(user, False)demotes them. The install now has no admin.
This nullifies fix #6. Only sync admin from groups when group-based management is actually configured: skip set_oidc_user_admin entirely when OIDC_ADMIN_GROUPS is unset/empty, so bootstrap and manually-granted admin persist.
2. state is not bound to the browser session (login CSRF).
The state token is Fernet-encrypted but carries no binding to the browser that initiated the flow, and nothing is stored client-side at /login. An attacker can obtain an authorization code for their own identity and hand the victim a crafted /callback?code=...&state=... URL; the victim's browser redeems it and is silently signed in as the attacker. The nonce only prevents id_token replay, not this. Bind state to a value set in an HttpOnly cookie at /login and verify it matches at /callback (the standard OIDC state-as-CSRF-token pattern).
3. Unknown-kid JWKS refresh is unauthenticated and attacker-triggerable.
_verify_id_token calls _refresh_jwks() (a live outbound fetch to the IdP) whenever the token header carries a kid not in cache. The callback is unauthenticated, so an attacker can send tokens with random kid values and drive one outbound IdP request per call. Add a refresh cooldown / minimum interval so a bad kid cannot cause unbounded outbound fetches.
4. SECURE_COOKIES defaults to false.
The session cookie issued after OIDC login is set with secure defaulting off, so it can traverse plaintext HTTP. SSO implies a real deployment; an auth session cookie should not default to being transmittable in cleartext. Default it to true, or derive secure from the request scheme, and document the expectation.
5. Dead validation-shaped code in the verifier.
core/oidc.py lines 349-363 decode header_claims with all claims marked non-essential and then discard the result, swallowing every exception with pass. In a token-verification routine, code that looks like validation but does nothing is a hazard for the next reader. Remove the block; the real verification at lines 377-434 stands on its own.
Also rebase onto dev (currently BEHIND) so this is evaluated against current state. Given this is authentication and a sizeable feature, it will also need a second maintainer to sign off before merge.
|
All five second-pass items addressed in ae7910f and rebased onto latest
Tests: 100 passing (66 OIDC + 34 regression), 0 failures. Rebased onto latest |
|
Third pass on ae7910f. I verified all five second-pass items in the code, not just the summary:
That is solid work across three rounds, thank you for the careful reworks. One thing before this goes further: have you re-run the full login flow end to end against a live IdP on this exact commit, not just the unit suite? The 100 tests mock the HTTP and JWKS layers, and the changes since your last live test (the CSRF cookie with its To be clear on process, this is not an approval. This is a large, AI-assisted feature in the authentication and trust boundary, so per the project's direction it needs sign-off from a second maintainer, and whether generic OIDC SSO is in scope right now is pewd's call, not mine. My passes have been quality and security review to get the implementation into a defensible state; the merge decision sits above me. |
There was a problem hiding this comment.
Pull request overview
Adds generic OpenID Connect (OIDC) single sign-on to the existing auth system, enabling users to log in via a standard OIDC provider while preserving current password-based authentication and introducing passwordless OIDC user accounts.
Changes:
- Introduces an
OidcManager(discovery, authorization URL generation, code exchange, JWT/JWKS verification) and new/api/auth/oidc/*routes for config/login/callback. - Extends
AuthManagerwith OIDC-linked user creation/lookup, password rejection for OIDC users, and IdP-group-driven admin syncing. - Updates UI to display an SSO button on the login page and hide password/2FA settings for OIDC users; wires new env vars through compose +
.env.example.
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_oidc_routes.py | Adds route-level tests for OIDC config/login/callback flows and error paths. |
| tests/test_oidc_manager.py | Adds unit tests for discovery, state handling, token exchange, id_token verification, and JWKS caching. |
| tests/test_oidc_auth.py | Adds AuthManager tests for OIDC user creation, collisions, password rejection, and admin syncing. |
| static/login.html | Adds frontend OIDC config fetch + “Sign in with …” button section and styling. |
| static/js/settings.js | Hides password/2FA cards for OIDC users based on auth/status response. |
| static/index.html | Adds an id hook for the password card so it can be hidden for OIDC users. |
| routes/oidc_routes.py | Implements /api/auth/oidc/config, /login, /callback endpoints. |
| routes/auth_routes.py | Includes is_oidc in /api/auth/status payload for the settings UI. |
| requirements.txt | Adds authlib dependency for OIDC/JWT/JWKS handling. |
| docker-compose.yml | Wires OIDC env vars into the default container runtime configuration. |
| docker-compose.gpu-nvidia.yml | Wires OIDC env vars into NVIDIA GPU compose profile. |
| docker-compose.gpu-amd.yml | Wires OIDC env vars into AMD GPU compose profile. |
| core/oidc.py | Adds the core OIDC client: discovery, state, token exchange, id_token verification, JWKS caching. |
| core/auth.py | Adds OIDC user support (create/lookup/admin sync) and blocks password flows for OIDC users. |
| app.py | Initializes and registers OIDC routes and exempts OIDC endpoints from auth middleware. |
| .env.example | Documents OIDC configuration variables and behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Live end-to-end test completed on ae7910f against both Authentik and Keycloak:
Thank you for the thorough three-pass review — the careful feedback made this a much stronger implementation. Ready for the second maintainer sign-off whenever pewd or another maintainer has bandwidth. |
Findings
ValidationRan backend compile, |
|
@RaresKeY both findings addressed in 23f1a49:
103 tests passing (69 OIDC + 34 regression), 0 failures. |
|
✅ Works with Keycloak Environment
Setup
Tested
Configuration |
|
Apologies — I overlooked two OIDC env vars in the compose files:
Both are now added to docker-compose.yml, docker-compose.gpu-nvidia.yml, |
|
I rechecked this at current head The latest issue called out in the thread was real: Stepping back, I do not think generic OIDC should be treated as priority work for the current stabilization push. It is useful for self-hosted/team deployments, but it is an enhancement rather than a stabilization bug fix, and the scope/implications are multifold if accepted. This touches more than the login button: login now has an IdP redirect path, users are created from @vdmkenny @alteixeira20 can you weigh in on tracking/routing? My suggestion is to keep SSO/OIDC on a roadmap track rather than pushing it through as stabilization work: a parent tracker for future SSO/OIDC, with this PR linked as a child issue/subissue/work item so the author has somewhere durable to track the current implementation and future follow-ups, and maintainers can revisit it when SSO is actually in scope. If this PR stays active or becomes the basis for that future work, there are a few current-head issues I would keep on the child issue before another approval pass:
For PR readiness, I would also like screenshots/short clips because this changes the login page and account/settings behavior: OIDC disabled, OIDC enabled desktop/mobile, local-user settings, OIDC-user settings, and at least one callback failure state without exposing secrets. Follow-up/maintenance if this lands: provider setup docs for Keycloak and Authentik, a tested provider/version matrix, reverse-proxy/cookie/group-claim/clock-skew/JWKS troubleshooting, an RP-initiated logout decision, an Authlib/Joserfc upgrade watch, and ideally an OpenID Foundation RP conformance pass for the basic code-flow profile. Authlib itself is not the issue; using a real OIDC/JWT library is the right direction. The maintenance point is that Validation run locally:
Not run: full test suite, live browser screenshots, live Keycloak/Authentik flow on this exact head, or the OpenID Foundation RP conformance suite. |
|
@RaresKeY I'll try to take a closer look tomorrow and get back to you! |
|
@RaresKeY @vdmkenny all three items from the last review pass are addressed: 1. Discovery issuer mismatch now fails closed
Test: 2. Multi-audience tokens without
|
… to compose files Both vars were documented in .env.example and read by the application code, but never forwarded to the container by any docker-compose file. OIDC_REDIRECT_URI is essential for deployments behind a reverse proxy to avoid deriving the wrong scheme from the inbound request.
…-audience azp, OIDC 2FA guards 1. Discovery issuer mismatch now raises OidcError instead of logging a warning (OIDC Discovery §1.1 requires mismatch abort). 2. Multi-audience ID tokens without azp are now rejected (OIDC Core §2 requires azp when aud has multiple values). 3. /change-password, /2fa/setup, /2fa/confirm, and /2fa/disable now reject OIDC users with a clear message. The frontend already hides these cards, but the backend must also enforce the policy. 113 passing (76 OIDC + 37 regression), 0 failures.
…ed redirect_uri 1. JWKS fetch/parse errors now wrapped as OidcError instead of escaping as raw exceptions. Transient network failures, HTTP errors, and bad JSON responses from the JWKS endpoint all produce a controlled OidcError, which the callback route already redirects to /login?error=oidc_failed instead of a 500. 2. exchange_code() now extracts the stored redirect_uri from the Fernet-encrypted state token and rejects any callback-derived redirect_uri that differs. The token exchange POSTs the stored value — the one the IdP saw in the authorization request — removing the last callback dependence on request-derived URI behaviour. Tests: 5 new (3 JWKS error wrapping + 2 redirect_uri binding) Total: 130 passing (81 OIDC + 49 regression), 0 failures.
Wrap oidc_manager.exchange_code() in asyncio.to_thread() so slow provider I/O (token exchange, JWKS refresh, UserInfo) doesn't block the async worker's event loop. Moves import asyncio to top level. Add regression test proving a 0.3s blocking exchange completes quickly without blocking concurrent async work.
…s workers Two fixes from review feedback: 1. (security/availability) Record _last_jwks_refresh timestamp BEFORE calling _refresh_jwks() so a failed JWKS fetch is also throttled by the 60-second cooldown. Previously the timestamp was only set after success, so an outage or key rotation would retry the IdP on every login attempt. 2. (auth) Use the shared persistent app key from secret_storage (_get_fernet) for OIDC state encryption instead of reading data/.app_key directly and falling back to a per-process key. This guarantees all uvicorn workers share the same Fernet key, even on a fresh data directory — worker A's state is always decryptable by worker B on the callback. Add regression tests for both fixes.
…nst silent demotion Three fixes from second review pass: 1. (security) Serialize the first-OIDC-user admin bootstrap inside _config_lock. Previously the check and username collision resolution happened outside the lock, so two concurrent first-login callbacks could both observe an empty user map and both persist as admin. Now idempotent lookup, bootstrap decision, and collision resolution are all inside one critical section. 2. (auth) Make data/.app_key creation atomic via O_EXCL open so two racing workers on a fresh deployment cannot generate different keys. The loser reads the winner's key, guaranteeing every worker shares the same Fernet key for OIDC state encryption. 3. (auth) Track whether UserInfo was successfully fetched (_userinfo_available flag in claims). The callback now skips admin group sync for existing users when UserInfo is unavailable AND the id_token lacks a groups claim — a transient provider failure no longer silently demotes existing OIDC admins. When UserInfo succeeds or the id_token carries groups, admin status syncs as before. Regression tests: concurrent admin bootstrap, key-creation race, UserInfo-unavailable preserves admin, UserInfo-available demotes, id_token groups authoritative without UserInfo.
…omic key creation
- Add fcntl.flock inter-process file lock shared by setup() and
create_user_oidc() so multi-worker first-admin bootstrap is
serialised across processes, not just threads within one worker.
Both methods reload auth.json inside the lock so the loser sees
the winner's write.
- _fetch_userinfo() now returns None (not {}) when discovery has
no userinfo_endpoint, and exchange_code() only sets
_userinfo_available=True when a live endpoint was reached.
Prevents the callback from treating 'no endpoint' as
authoritative group non-membership evidence.
- Rewrite _load_or_create_key() to write the Fernet key to a temp
file, fsync, then atomically os.link() into place. No reader
ever sees the final path before the complete key bytes are
available — a racing worker either sees no file or a complete
one, never an empty/partial file.
105 tests pass (97 existing + 8 new regressions covering the
three fixes).
Co-Authored-By: Kevin <holden093@users.noreply.github.com>
fcntl.flock serialises across processes (uvicorn workers) but does NOT
block threads within the same process — two threads calling
flock(LOCK_EX) on the same file both succeed immediately. This caused a
rare race in concurrent first-admin bootstrap where both
create_user_oidc() and setup() could enter the critical section
simultaneously, resulting in a lost write and zero admins.
Add a module-level threading.Lock acquired before the file lock so the
critical section is serialised across both threads and workers.
Fixes the flaky test:
TestInterprocessFirstAdminSerialisation::test_two_managers_single_first_admin
("Expected exactly 1 admin after concurrent bootstrap; found 0")
1. Serialize set_oidc_user_admin() across workers via _interprocess_auth_lock() with reload-recheck-save, preventing stale in-memory snapshots from overwriting users concurrently created by another worker. 2. Accept single-element aud arrays without azp (OIDC Core §2 only requires azp for multi-audience tokens). Normalize aud to a list and only enforce azp when len > 1. 3. Add threading.Lock around _get_fernet() key creation to prevent two threads in the same process from racing on the shared temp-file path and destroying each other's work. 4. Guard import fcntl with try/except so AuthManager imports on native Windows. _interprocess_auth_lock degrades to intra-process-only when fcntl is unavailable. Tests: +4 regression tests (108 total, 0 failures).
create_user() (password-user creation) now takes _interprocess_auth_lock with reload-before-save, preventing a concurrent set_oidc_user_admin() from losing a newly-created password user. Introduces _create_user_locked() internal helper so setup() (which already holds the inter-process lock) can create users without a nested fcntl.flock deadlock. Also changed _auth_intraprocess_lock to threading.RLock to support safe nesting patterns across mutation methods. Regression: test_create_user_survives_concurrent_set_oidc_user_admin (109 total, 0 failures).
The reserved-username guard was only in create_user(), but setup() now calls _create_user_locked() directly (to avoid nested flock deadlock). Move the check into the shared helper so it applies regardless of which path creates the user. Fixes CI failure in test_setup_rejects_reserved_admin_username.
All auth.json mutation methods now acquire _interprocess_auth_lock (flock-based) + _config_lock with reload-before-save, matching the pattern already used by create_user / create_user_oidc / set_oidc_user_admin. This prevents stale-write clobbers when an OIDC callback races a concurrent set_privileges, delete_user, rename_user, set_admin, change_password, or TOTP mutation from another uvicorn worker. Also: - Normalize setup() username (strip + lower) to match every other user-creation path, preventing "Alice" vs "alice" lockout. - Normalize in _create_user_locked() as defense-in-depth. - Remove dead _setup_lock — its serialisation was subsumed by _interprocess_auth_lock + _config_lock. - Move pre-lock validation inside the critical section for change_password, totp_generate_secret, and totp_confirm_enable. - Re-read backup codes from disk inside the lock in totp_verify() to prevent dual-consumption across workers. Co-Authored-By: antigravity <agy@antigravity>
RaresKeY
left a comment
RaresKeY
left a comment
There was a problem hiding this comment.
Thanks for the update. I checked the latest head; several earlier OIDC fixes are in place, but I found two remaining issues that should be addressed before merge.
Findings
-
Problem:
exchange_code()treats any non-NoneUserInfo response as available, rejects only when a UserInfosubis present and different from the verified ID-tokensub, and then merges non-identity claims such asgroupsinto the verified claim set. The callback uses those merged groups plus_userinfo_availableto create or sync OIDC admin status whenOIDC_ADMIN_GROUPSis configured. If UserInfo omitssub, the group data is never bound back to the authenticated subject before it can drive local admin state. -
Impact: A malformed or buggy configured UserInfo endpoint can grant or revoke Odysseus admin based on group data that was not tied to the ID-token subject. A focused no-network probe returned
groups: ["odysseus-admins"]and_userinfo_available: truefrom a UserInfo response with nosub, which would be treated as authoritative group evidence by the callback. -
Ask: Please require successful UserInfo to include
subequal to the verified ID-tokensub, or at minimum refuse to merge/use authorization-relevant claims such asgroupsunless that binding is present. Add a regression for UserInfo returning groups but no subject. -
Location:
core/oidc.py:279
-
Problem: The login page reads
/login?error=...and displays a human-readable OIDC callback error, but normal initialization then callssetMode("setup")orsetMode("login"), andsetMode()unconditionally hides the error element. As a result, messages foroidc_failed,oidc_csrf,oidc_denied,oidc_invalid, andoidc_configdisappear during page load. -
Impact: Users returned from a provider denial, CSRF failure, invalid callback, or code-exchange/config failure land on a blank login form instead of seeing why SSO failed. That makes real sign-in failures look like a silent redirect loop.
-
Ask: Please preserve an already-displayed OIDC callback error when initializing mode, or show the callback error after mode selection. A lightweight regression for
/login?error=oidc_failedstaying visible after auth-status initialization would cover this. -
Location:
static/login.html:371
Validation
- Visible GitHub checks are passing on the reviewed head, apart from the skipped Trivy SARIF upload job.
- Ran focused no-network validation for the UserInfo subject-binding issue, and focused OIDC route/auth tests passed in the isolated runner.
- Not run: full local pytest with dependencies installed, live browser/app flow, live IdP validation, Docker compose config/build/startup, OpenID Foundation RP conformance, or native Windows startup.
Fixes 11 security/robustness issues identified across a 4-model review (deepseek, gpt-5.5, claude-fable-5, gpt-5.6-sol) of the OIDC SSO implementation: UserInfo & claims integrity: - Require non-empty matching sub before trusting UserInfo (P2) - Reject non-dict/malformed UserInfo responses as unavailable - Validate NumericDate strictly: reject bool, NaN, Inf - Add iat future-token verification (60s tolerance) - Initialize userinfo safely before try block Callback hardening: - Clear CSRF cookie on ALL 8 failure branches + 503 unconfigured - Echo state parameter on IdP error redirects (OIDC Core §3.1.2.6) - Check create_session_trusted() return value before setting cookie Defense-in-depth: - OIDC_MAX_AGE env var with auth_time verification (+60s skew) - AuthManager.check_oidc_totp() with disk-reloaded TOTP enforcement - SameSite=Lax proxy documentation in .env.example UI fix: - Preserve OIDC callback error after setMode() initialization Tests: 164 passed (123 OIDC + 41 regression), +21 new regression tests
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
Verify set_oidc_user_admin() refuses to demote the sole admin when an IdP group change would otherwise lock out all admin access.
|
@RaresKeY — both items from your last review are addressed, plus additional hardening surfaced by a new review strategy I put together for this branchs security assessment. Your P2 — UserInfo sub binding: Your P3 — Login-page error clearing: Additional hardening from the same review pass:
Tests: 165 passed, 0 failures. Ready for your next pass. |
RaresKeY
left a comment
RaresKeY
left a comment
There was a problem hiding this comment.
I reviewed the current head and found five issues that should be addressed:
-
Problem: The callback validates that sub is nonempty and then calls strip before using it as the issuer/subject account-link key. OIDC subjects are opaque identifiers, so this normalization can collapse distinct verified subjects into one local account.
-
Impact: A distinct authenticated subject can receive the local account and session of the subject with the trimmed identifier, including its administrator privileges.
-
Ask: Preserve the exact verified subject for persistence and lookup. Validate nonemptiness and bounds without normalizing the identifier, and add a regression for distinct whitespace-bearing subjects.
-
Location: routes/oidc_routes.py lines 171-174, 240, and 282-286.
-
Problem: A bound UserInfo response is considered available even when it omits groups or provides a malformed value. That availability flag alone causes the callback to synchronize an existing OIDC admin to non-admin when no group matches.
-
Impact: A normal provider or scope configuration change can unexpectedly demote an administrator and remove that user's administrative access, despite the response containing no valid group evidence for the transition.
-
Ask: Change admin state only when a trusted source supplied a valid groups claim; otherwise retain the current state or fail the login explicitly. Add regressions for missing and malformed UserInfo groups.
-
Location: core/oidc.py lines 351-388; routes/oidc_routes.py lines 204-211 and 246-254.
-
Problem: Discovery accepts an HTTP issuer and authorization endpoint while only enforcing HTTPS for back-channel endpoints, and the authorization request contains state and nonce but no PKCE challenge.
-
Impact: With an accepted insecure authorization endpoint, an active network observer can arrange a substituted authorization code for the observed state and switch the resulting session identity.
-
Ask: Require HTTPS for the issuer and authorization endpoint, implement S256 PKCE with a verifier bound to the encrypted callback state, and cover rejected HTTP discovery plus challenge/verifier handling.
-
Location: core/oidc.py lines 171-218 and 257-285.
-
Problem: The OIDC login and callback derive the CSRF/session cookie Secure attribute from SECURE_COOKIES, while the bundled Compose files default that variable to false. This silently forces both OIDC cookies insecure even in a public TLS-terminating proxy deployment.
-
Impact: A stock external OIDC deployment can issue bearer session and login-CSRF cookies that are eligible for transmission over HTTP if that origin is reachable, exposing an authenticated session or weakening login-CSRF protection.
-
Ask: Make OIDC cookies Secure by default and reject or explicitly document an HTTP-only development override. Ensure the proxy-supported configuration cannot silently force them insecure, and add coverage for the Compose default and proxied HTTPS path.
-
Location: routes/oidc_routes.py lines 102-110 and 288-330; docker-compose.yml lines 39-48 and matching GPU Compose files.
-
Problem: The OIDC callback can run on one worker, but the browser session it creates is retained only in that process in-memory session map. A following request handled by another worker cannot validate the token.
-
Impact: In a normal multi-worker, non-sticky deployment, successful OIDC login can immediately become a logged-out session on the next request.
-
Ask: Use a shared or reloadable session store for this flow, or explicitly constrain the deployment to a compatible single-worker/sticky-session model and add a two-worker regression.
-
Location: core/auth.py lines 123-140 and 854-926; routes/oidc_routes.py lines 282-307.
Validation
- The completed current-head GitHub checks are successful; the Trivy SARIF upload is skipped.
- Focused OIDC route/auth/session tests passed in the secretless runner. The manager-inclusive suite is blocked by the runner image missing declared authlib, not by an assertion failure.
- No live IdP, browser/UI, multi-worker deployment, Docker Compose runtime, or OpenID Foundation RP-conformance run was available. The vulnerability-detector lane is degraded because Podman and Docker are unavailable.
|
Follow-up validation on unchanged head
|
…ssions Addresses RaresKeY's review (5 findings) and follow-up Basic RP validation comment on PR odysseus-dev#3508: - Add PKCE (RFC 7636, S256): code_challenge in the authorization request, verifier carried in the Fernet-encrypted state, and code_verifier sent to the token endpoint. - Require the iat claim in id_tokens (OIDC Core §2); tokens without iat are now rejected. - Prefer client_secret_basic at the token endpoint per discovery (OIDC default), falling back to client_secret_post only when the provider excludes basic. - Require HTTPS for the issuer and authorization endpoint, not just the back-channel endpoints. - Preserve OIDC subs exactly (no strip) so distinct whitespace-bearing subjects can never collapse into one local account; same for the UserInfo sub-binding comparison. - Sync admin state only on a well-formed groups claim; UserInfo availability alone (or a malformed groups value) no longer demotes an existing admin. - OIDC session/CSRF cookies are Secure by default regardless of SECURE_COOKIES; explicit OIDC_ALLOW_INSECURE_COOKIES=true is the only (documented, dev-only) opt-out. - Make sessions issued by one uvicorn worker validate on others via an mtime-gated read-through reload of sessions.json. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GRiLb12nnLnBnYsg14oSWd
… cookie policy 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, odysseus-dev#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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GRiLb12nnLnBnYsg14oSWd
|
@RaresKeY Thanks for the thorough review and the follow-up Basic RP validation — all findings are addressed in Follow-up comment (Basic RP validation):
Review findings:
Proactive hardening beyond the findings (
Validation:
|
Summary
Adds generic OpenID Connect (OIDC) single sign-on as an alternative login
method. Users can sign in via any standard OIDC provider (Authentik,
Keycloak, Authelia, Google, etc.) while existing password authentication
remains fully functional. New OIDC users are auto-created on first login,
and admin status can be driven by IdP group membership.
Uses
authlibfor provider discovery (.well-known), JWT/JWKSsignature verification, and the authorization code flow — avoiding the
security risk of hand-rolling OIDC logic.
Built with AI assistance (see the
Co-Authored-Bytrailer).Target branch
dev, notmain.Linked Issue
Closes #806
Type of Change
Checklist
devrequirements.txtand wired corresponding env vars throughdocker-compose.yml.How to Test
Run the unit tests:
85 tests covering OidcManager (mocked HTTP + real RSA JWT signing), OIDC route handlers (login redirect, callback success/failure, error paths), AuthManager OIDC methods (user creation, lookup, idempotency, username collision, password rejection), and admin group mapping (promotion, demotion, no-op).
Verify existing auth tests pass:
Verify the app starts without OIDC configured (default):
End-to-end with a real OIDC provider:
OIDC_ENABLED=true,OIDC_ISSUER,OIDC_CLIENT_ID,OIDC_CLIENT_SECRETin.envdocker compose up -d --buildhttp://localhost:7000/login— a "Sign in with …" button appears below the password formVisual / UI changes
Login page — SSO button below the password form when OIDC is configured, with a divider. Styled to match the existing login form using theme variables.
Settings → Account — "Change Password" and "Two-Factor Authentication" cards hidden for OIDC users (they have no password and authenticate through the IdP).
Configuration reference
OIDC_ENABLEDfalseOIDC_ISSUEROIDC_CLIENT_IDOIDC_CLIENT_SECRETOIDC_SCOPESopenid profile emailOIDC_ADMIN_GROUPS🤖 Generated with Claude Code