Skip to content

Phase 2a: embedded OIDC provider (authorize/token/userinfo/jwks/discovery) - #148

Open
ClaydeCode wants to merge 12 commits into
mainfrom
feature/oidc-phase2a-provider
Open

Phase 2a: embedded OIDC provider (authorize/token/userinfo/jwks/discovery)#148
ClaydeCode wants to merge 12 commits into
mainfrom
feature/oidc-phase2a-provider

Conversation

@ClaydeCode

@ClaydeCode ClaydeCode commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

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

  • Built on Authlib's framework-agnostic server classes (>=1.7.2,<1.8 — 1.8 makes the grant param mandatory; pinned rather than chased tonight).
  • Session = the existing terminal cookie. /authorize resolves the authorization JWT → terminal → users row (phase 1). The sub claim 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 an oidc_rd return URL (login page lands in phase 3).
  • Authlib's core is sync: request handling runs in asyncio.to_thread, storage hooks bridge back to the main loop's psycopg pool via run_coroutine_threadsafe — one pool, no sidestep (unlike the spike's sync-psycopg shortcut).
  • Multi-user-ready: the provider only knows users rows; when members arrive (phase 4) nothing here changes except the grant check in /authorize.

Hardening vs. the spike (full list from the PoC verdict)

  • Access/refresh tokens and authorization codes stored as SHA-256 digests — a DB read can no longer replay sessions. Client secrets stay plaintext deliberately: compose templates re-render them at every startup, and they sit in the app's compose env on the same disk anyway.
  • Scope stored from request.scope (client-allowed), not raw request — scope-escalation regression covered by test.
  • Null claims stripped from id_tokens (strict clients like Immich's openid-client v6 reject nonce: null).
  • Both client_secret_basic and client_secret_post accepted (Immich sends _post).
  • Token endpoint rate-limited (in-process, 30/min) → 429.
  • RS256 key generated once, persisted in kv_store.
  • ENV FORWARDED_ALLOW_IPS=* in the Dockerfile: fastapi run already enables uvicorn's proxy headers, but forwarded headers from the in-network Traefik must be trusted or Authlib sees http:// 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

  • Client registration at app install + {{ oidc.* }} template vars (phase 2b, next PR)
  • Login page / credentials (phase 3), members + grants (phase 4)
  • Cleanup job for expired codes/tokens (tables stay tiny at household scale; can ride along with phase 3)

Recommended reading order

  1. migrations/shard-core-0004-oidc.sql — schema
  2. shard_core/database/oidc.py — storage (conn-first, hashed lookups)
  3. shard_core/service/oidc_provider.py — Authlib models, grants, key mgmt, sync→async bridge
  4. shard_core/web/public/oidc.py — FastAPI adapter, lazy init, session resolution, rate limit
  5. shard_core/web/public/__init__.py, Dockerfile, pyproject.toml — wiring
  6. tests/test_oidc.py

🤖 Generated with Claude Code

Test status

Full suite: 336 passed, 0 failed, rebased onto main at 02bd02d.

An earlier revision of this section reported test_app_lifecycle.py::test_app_starts_and_stops failing. That was never a regression from this branch and is now fixed on main: the mock apps published host port 80, which collided with whatever already held it on the test machine. #209 removed the binding.


Rebased on the reworked phase-1 branch: user_sub columns are BIGINT FKs, ShardUser carries the numeric id.

@ClaydeCode

Copy link
Copy Markdown
Contributor Author

Security-hardening round from the OIDC review (RFC 9700), commit be5293b:

  • PKCE: S256 only. Authlib's default also accepted plain (challenge = cleartext verifier — no interception protection). Dropped from validation and discovery.
  • Authorization codes: atomic single-use. Redemption is now one UPDATE … RETURNING on a redeemed flag — the SELECT-then-DELETE TOCTOU where two concurrent /token calls could both redeem a code is gone. Any redemption attempt burns the code (even one that fails PKCE), and reuse of a redeemed code revokes every token of that (client, user) grant. Redeemed rows persist until expiry, which also makes the nonce-replay window real.
  • Refresh-token reuse detection. Replaying a rotated-out refresh token now revokes the whole token family — previously the thief who rotated first kept a live token while the legit client got a silent invalid_grant.

Deferred (low, noted for later): per-client/IP rate-limit buckets — the coarse global 30/min guard stays.

Tests: 20/20 in tests/test_oidc.py, incl. new coverage for plain-PKCE rejection, burn-on-failed-redemption, code-reuse revocation, and family revocation on refresh replay.

🤖 Generated with Claude Code

ClaydeCode and others added 11 commits August 25, 2026 09:58
Nothing referenced it: no test installs it by name, and its only trace
was the `# todo: test_large_app_does_not_start` comment removed here. Its
app_meta shape duplicated filebrowser's, so it added nothing to the
corpus that test_all_mock_app_store_metas_migrate walks either.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AHmisdKK8deh7iEd3Shn2Q
The fixtures were checked in as zips, so the compose templates and
app_meta files they carry could not be read or diffed without unpacking
them first. Four of the loose copies sitting next to the zips had already
drifted from the zip that actually ran: they lacked the `version: '3.5'`
line, and always_on's named the `shard` network where the live one names
`portal`.

The zips are now unpacked into the repo and a session-scoped fixture
packs them into a temporary directory at test start, removing it at
session end. The unpacked files come from the zips themselves, so what
the suite installs is byte-identical to before, bar the port change
below.

Dropping `ports: - 80:80` from every template while the files were open:
nothing consumed the host binding. Traefik reaches an app over the shared
portal network at `http://<container>:<port>`, and no app in the
app-repository publishes a host port. The binding only ever collided with
whatever already held port 80 on the machine, which made
test_app_starts_and_stops unpassable on a host running its own reverse
proxy. pause_cycle already carried no ports block.

immich's zip contains a .env that .gitignore's `.env` rule would have
kept out of the repo, so the built zip would have silently lost it — hence
the negation. build_mock_app_store_zips asserts every fixture file is
git-tracked, so the next such file fails the run instead of quietly
producing a different app in CI than on a developer machine.

test_all_mock_app_store_metas_migrate now reads the app_meta.json files
directly rather than unpacking each zip for one entry.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AHmisdKK8deh7iEd3Shn2Q
…e-fixture

Unpack the mock app store fixtures and build the zips at test time
… 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>
…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
`shard-core-0003-app-status-message` landed on main with #177, so this
file's 0003 was taken. Renumbered and rechained onto it.

Worth recording how this would have failed unnoticed: both files declared
`-- depends: shard-core-0002-users`, so yoyo would have read them as
siblings and applied both without error. Git reported no conflict either,
because the filenames differ. Nothing in CI looks at migration numbering,
so the repo would simply have grown two migrations called 0003.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016kZu32ajDb9YSVvcpLvSDX
upsert_client resolves ON CONFLICT (app_name) and deliberately preserves
the existing client_id and client_secret, so its RETURNING row is the only
place the caller can learn which credentials are actually in the database.
Returning the locally built dict instead handed back the freshly generated
pair, which on the conflict path was never written.

No caller outside the tests exists yet on this branch, so nothing was
broken. It would have been the moment app install started registering
clients: the compose template would render credentials the token endpoint
does not know, and the app's login would fail until the row was rewritten.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AHmisdKK8deh7iEd3Shn2Q
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants