Phase 2a: embedded OIDC provider (authorize/token/userinfo/jwks/discovery) - #148
Open
ClaydeCode wants to merge 7 commits into
Open
Phase 2a: embedded OIDC provider (authorize/token/userinfo/jwks/discovery)#148ClaydeCode wants to merge 7 commits into
ClaydeCode wants to merge 7 commits into
Conversation
ClaydeCode
force-pushed
the
feature/oidc-phase2a-provider
branch
3 times, most recently
from
July 11, 2026 18:43
7d3a362 to
be5293b
Compare
Contributor
Author
|
Security-hardening round from the OIDC review (RFC 9700), commit
Deferred (low, noted for later): per-client/IP rate-limit buckets — the coarse global 30/min guard stays. Tests: 20/20 in 🤖 Generated with Claude Code |
… passed spike Five endpoints under /public/oidc (discovery, authorize, token, userinfo, jwks) on Authlib's framework-agnostic server classes: authorization-code + PKCE + refresh rotation, RS256 id_tokens signed with a key persisted in kv_store. First-party app clients only — no dynamic registration, no consent UI, no third-party exposure. The session is the existing terminal cookie: /authorize resolves it to the terminal's user (phase-1 users table), so every paired browser signs into an app with zero clicks. Anonymous browsers are redirected to the terminal UI with an oidc_rd return URL until the phase-3 login page exists. Authlib's core is synchronous; endpoints run it in asyncio.to_thread and the storage hooks bridge back to the main loop's psycopg pool via run_coroutine_threadsafe — single pool, unlike the spike's sync-psycopg sidestep. Hardening over the spike (per PoC verdict findings): access/refresh tokens and authorization codes stored as SHA-256 digests; scope saved from request.scope (client-allowed) not the raw request; null claims stripped from id_tokens (strict clients reject nonce:null); client_secret_basic AND _post accepted (Immich uses _post); token endpoint rate-limited in-process; authlib pinned <1.8 (grant param becomes mandatory there); FORWARDED_ALLOW_IPS=* so uvicorn trusts Traefik's X-Forwarded-Proto — Authlib refuses flows it sees as plain http. Client secrets deliberately stay plaintext: compose templates re-render them at every startup and they live in app compose envs on the same disk anyway. Phase 2a of the multi-user identity rollout; client registration at app install follows in phase 2b. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
user_sub columns are BIGINT FKs; ShardUser carries the numeric id and the OIDC sub claim is its stringified form. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…s now) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…mic code redemption, reuse family revocation Applies the three findings from the 2026-07-11 security review (RFC 9700): - PKCE accepts S256 only. Authlib's default also allows 'plain', where the challenge travels as the cleartext verifier — useless against the authorization-request interception PKCE exists for. Discovery no longer advertises plain. - Authorization codes are consumed atomically (single UPDATE ... RETURNING on a redeemed flag), closing the TOCTOU race where two concurrent /token requests could both redeem one code. Any redemption attempt burns the code, including ones that later fail PKCE. Redeemed rows are kept until expiry: reuse of a redeemed code signals interception and revokes every token issued to that (client, user) grant. Side effect: the nonce-replay window now actually spans the code lifetime. - Replay of a rotated-out refresh token revokes the whole token family, so a thief who rotates first no longer keeps a live token while the legit client is silently rejected. Per-client rate-limit buckets (finding 4) deferred — the global in-process guard stays; noted as follow-up when it matters. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ClaydeCode
force-pushed
the
feature/oidc-phase2a-provider
branch
from
July 27, 2026 11:14
be5293b to
ba1ea67
Compare
…rusting proxy headers Authlib refuses OAuth flows on plain http, and the URL Starlette sees inside the container is http — Traefik terminates TLS. The previous fix set FORWARDED_ALLOW_IPS=* so uvicorn's ProxyHeadersMiddleware would take the scheme from X-Forwarded-Proto. That trusts those headers from every peer, not just Traefik, and every installed app shares the portal network and can reach shard_core directly. The middleware also rewrites request.client from X-Forwarded-For, and /internal/call_peer identifies the calling app by request.client.host to decide which subdomain the shard's identity key signs a peer request for — so the wildcard turns a direct-reach path into app impersonation (see issue #188 for the underlying weakness). Narrowing the value to Traefik's address is not available: uvicorn reads the variable once at process start, core compose starts traefik only after shard_core is healthy, container IPs are dynamic, a hostname never matches (the middleware compares against the peer IP), and the portal subnet contains every app container. The OAuth2Request is built by hand here, so the URL handed to Authlib does not have to come from the request. Rebuild it from the default identity's domain — the same source the issuer already uses — and drop the env var. The provider no longer reads X-Forwarded-* at all, and request.client keeps meaning the real socket peer. This also fixes the anonymous-authorize return leg. Traefik strips the /core/ prefix, so str(request.url) lacked it and the oidc_rd URL pointed at https://<domain>/public/oidc/authorize, which matches the catch-all web-terminal router rather than shard_core — the browser would never get back to the authorize endpoint after pairing. Reverting _public_url in that line fails the extended test with the unprefixed URL. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The embedded IdP is a new public surface on every shard. Ship it dark and turn
it on per shard, the same way pause_enabled rolls out: the controller writes an
override into the shard's core .env on every converge
(freeshard-controller#415), so the compose file needs an interpolation slot for
it. FREESHARD_OIDC__ENABLED=${OIDC_ENABLED:-false} — the :-false guard keeps a
shard whose .env omits the key on the default, and an empty string from a bare
${OIDC_ENABLED} would fail Settings() at boot.
Gate with a router-level dependency rather than by skipping the include_router
call: the public router is built at module import, so an import-time check
freezes the first test's config for the whole session and no config_override
could reach it. The dependency answers 404, not 403 — a shard that is not in the
rollout should look like it has no IdP at all, including from discovery.
Enabled in tests/config.toml so the existing suite keeps exercising the provider;
the new test flips it off and asserts all five endpoints answer 404.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Grants were keyed (client_id, user_sub) only, so they recorded which app and which person but not which device. Deleting a terminal therefore cascaded nothing: an un-paired device kept working access tokens for an hour and could keep minting new ones for thirty days. Records terminal_id and an opaque per-authorization sid on oidc_codes and oidc_tokens, carries both from /authorize through the code exchange and across refresh rotation, emits sid in the id_token, and revokes a terminal's grants when it is un-paired. The binding is taken from the credential rather than request.user, because on the token endpoint the user is rebuilt from the stored grant and has no terminal attached. terminal_id is ON DELETE SET NULL rather than CASCADE on purpose. Cascading would delete the rows, and a deleted row is indistinguishable from an unknown token, which loses both the ability to deny a presented token and the refresh-reuse detection that get_token_by_refresh_hash depends on. Un-pair therefore revokes first and lets the FK null out afterwards, in one transaction. sid is a fresh random value, not the terminal id: it goes to app containers in the id_token and they have no business learning shard-internal identifiers. This is the plumbing, not the feature. Revoking our own tokens does not end the session inside an app that minted its own at callback time -- that needs back-channel logout delivery (#192) and the serving barrier (#193). Also adds backchannel_logout_uri to oidc_clients so phase 2b can declare it at install time. Refs #189
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Phase 2a of the multi-user identity rollout: the embedded OIDC provider, productionized from the passed PoC (spike
spike/oidc-provider-poc, verdict 2026-06-12). Stacked on the phase-1 substrate PR.Five endpoints under
/public/oidc:/.well-known/openid-configuration,/authorize,/token,/userinfo,/jwks. Authorization-code + PKCE + refresh rotation, RS256 id_tokens. Deliberately not built: dynamic client registration, consent UI, third-party clients, logout channels — first-party app clients only.How it works
>=1.7.2,<1.8— 1.8 makes thegrantparam mandatory; pinned rather than chased tonight)./authorizeresolves theauthorizationJWT → terminal →usersrow (phase 1). Thesubclaim is the stringified numeric user id. A paired browser signs into an app with zero clicks; an anonymous browser is 302'd to the terminal UI with anoidc_rdreturn URL (login page lands in phase 3).asyncio.to_thread, storage hooks bridge back to the main loop's psycopg pool viarun_coroutine_threadsafe— one pool, no sidestep (unlike the spike's sync-psycopg shortcut).usersrows; when members arrive (phase 4) nothing here changes except the grant check in/authorize.Hardening vs. the spike (full list from the PoC verdict)
request.scope(client-allowed), not raw request — scope-escalation regression covered by test.nonce: null).client_secret_basicandclient_secret_postaccepted (Immich sends_post).kv_store.ENV FORWARDED_ALLOW_IPS=*in the Dockerfile:fastapi runalready enables uvicorn's proxy headers, but forwarded headers from the in-network Traefik must be trusted or Authlib seeshttp://and refuses flows (InsecureTransportError, hit in the PoC).Tests
17 integration tests against the real app + Postgres: discovery, JWKS stability, confidential + public (PKCE-only) flows, id_token claim correctness incl. nonce-absence, userinfo, refresh rotation (old access revoked, rotated refresh unusable), open-redirect rejection, wrong secret / unknown / expired / reused codes, anonymous redirect, scope narrowing, hashed-at-rest assertion, rate limit.
Not in this PR
{{ oidc.* }}template vars (phase 2b, next PR)Recommended reading order
migrations/shard-core-0003-oidc.sql— schemashard_core/database/oidc.py— storage (conn-first, hashed lookups)shard_core/service/oidc_provider.py— Authlib models, grants, key mgmt, sync→async bridgeshard_core/web/public/oidc.py— FastAPI adapter, lazy init, session resolution, rate limitshard_core/web/public/__init__.py,Dockerfile,pyproject.toml— wiringtests/test_oidc.py🤖 Generated with Claude Code
Test status
Full suite: 190 passed, 1 failed —
test_app_lifecycle.py::test_app_starts_and_stops, which fails identically on cleanmainon this machine (pre-existing, container-start timing), not a regression from this branch.Rebased on the reworked phase-1 branch:
user_subcolumns are BIGINT FKs,ShardUsercarries the numeric id.