feat(auth): add self-service OIDC console login - #1854
Conversation
Roadmap item "OIDC/SAML for console/admin" — a scoped MVP slice, not the full item. The gateway previously only verified externally-issued JWTs (shared-secret HMAC); this is the first thing that mints its own tokens. - GET /v1/auth/oidc/login, POST /v1/oidc/link/start, GET /v1/auth/oidc/callback (openidconnect crate handles PKCE/discovery/JWKS/ID-token verification). - Self-service identity linking only: an authenticated caller links their own OIDC identity to their tenant (oidc_identities, UNIQUE(issuer, subject)). No auto-provisioning — an unrecognized identity at login fails closed. - Entirely gated on AEGIS_OIDC_* + AEGIS_JWT_SECRET; unset means every route returns 501, no other behavior changes. - ui-next: a "Sign in with SSO" / "Link SSO identity" section in Settings, and an OidcCallbackHandler that picks up the #access_token=/#error= fragment on load. Security review (security-auditor agent) caught a CRITICAL cross-tenant identity-linking vulnerability before merge: the flow-state cookie carrying `linking_tenant_id` was unsigned, so a caller could forge it to bind their own IdP identity to a victim tenant despite link/start's own bearer-token gate — that gate only controlled what the server wrote, not what the client presented back at the callback. Fixed by HMAC-tagging the cookie with the same AEGIS_JWT_SECRET used to mint the JWT, with a regression test proving a forged cookie is now rejected. Also fixed a related non-atomic check-then-insert race in link_oidc_identity (INSERT now carries the uniqueness guarantee directly; the SELECT only runs on conflict). The mock IdP's RSA keypair used in tests is generated at test-run time (rsa + rand dev-dependencies) rather than a committed PEM fixture, so there's no private-key-shaped string literal in source history at all. Known, documented limitations (not silently omitted): the minted JWT carries only tenant authority with no per-SSO-user attribution or revocation; the pre-existing raw `Bearer tenant_<id>` fallback still bypasses OIDC unless AEGIS_JWT_REQUIRED=true (now warned about at startup); single IdP per gateway; no Secure cookie flag (matches the existing aegis_csrf precedent, documented as worth revisiting).
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
|
Warning Review limit reached
Next review available in: 39 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (22)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements a beta OIDC console login feature, adding backend support for OIDC discovery, PKCE-based authorization flows, and database-backed identity linking (for both SQLite and Postgres), alongside frontend integration in the console UI. Feedback on these changes highlights two main improvement opportunities: configuring a timeout on the HTTP client used for OIDC discovery to prevent gateway startup from blocking indefinitely on network hangs, and using TextDecoder instead of atob directly in the frontend to robustly decode JWT payloads containing multi-byte UTF-8 characters.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| let http_client = openidconnect::reqwest::Client::builder() | ||
| .redirect(openidconnect::reqwest::redirect::Policy::none()) | ||
| .build() | ||
| .map_err(|e| OidcDiscoveryError::Discovery(e.to_string()))?; |
There was a problem hiding this comment.
The reqwest::Client is built without a timeout. Since OIDC discovery is performed during gateway startup in main, any network hang or firewall packet drop when connecting to the IdP will cause the entire gateway startup to block indefinitely. This can lead to deployment failures and liveness/readiness probe timeouts. Configuring a reasonable timeout on the HTTP client ensures that discovery failures fail fast and allow the gateway to start up with OIDC disabled as intended.
| let http_client = openidconnect::reqwest::Client::builder() | |
| .redirect(openidconnect::reqwest::redirect::Policy::none()) | |
| .build() | |
| .map_err(|e| OidcDiscoveryError::Discovery(e.to_string()))?; | |
| let http_client = openidconnect::reqwest::Client::builder() | |
| .redirect(openidconnect::reqwest::redirect::Policy::none()) | |
| .timeout(std::time::Duration::from_secs(10)) | |
| .build() | |
| .map_err(|e| OidcDiscoveryError::Discovery(e.to_string()))?; |
| const payload = token.split(".")[1]; | ||
| if (!payload) return undefined; | ||
| const base64 = payload.replace(/-/g, "+").replace(/_/g, "/"); | ||
| const json = atob(base64); |
There was a problem hiding this comment.
atob decodes base64 strings into a binary string using Latin-1 encoding, which does not correctly handle multi-byte UTF-8 characters. If any claim in the JWT (even if not tenant_id itself) contains non-ASCII characters, JSON.parse(atob(base64)) can fail with a syntax error or corrupt the characters, causing the decoding to fail and return undefined. Using TextDecoder ensures robust UTF-8 decoding in modern browsers.
const json = new TextDecoder().decode(
Uint8Array.from(atob(base64), (c) => c.charCodeAt(0))
);
Summary
Picks up the roadmap item "OIDC/SAML for console/admin" — a scoped MVP slice (self-service OIDC login/linking), not the full item (no SAML, no multi-IdP, no per-SSO-user attribution/revocation yet — all called out explicitly below and in the docs, not silently omitted).
Supersedes #1853, which is closed: its history contained a since-removed test-only RSA private-key PEM fixture that gitleaks correctly flags on every commit in that branch's range (even after a later commit removed it — gitleaks scans the full commit history, not just the final diff). This PR is a clean squash of that branch's final state onto a fresh branch, so the flagged string never appears in this branch's history at all.
The gateway previously only verified externally-issued JWTs (shared-secret HMAC via
AEGIS_JWT_SECRET) — this is the first thing in the gateway that mints its own tokens.GET /v1/auth/oidc/login(plain login),POST /v1/oidc/link/start(authenticated; starts a "link my identity to my tenant" flow),GET /v1/auth/oidc/callback(IdP redirect target). PKCE, discovery, JWKS caching, and ID-token signature/issuer/audience/expiry/nonce verification are all handled by theopenidconnectcrate, not hand-rolled.oidc_identitiestable (UNIQUE(issuer, subject)) maps an external identity to exactly one tenant, populated only via the self-service link flow. An unrecognized identity at plain login fails closed (#error=identity_not_linked), it never creates or guesses a tenant.AEGIS_OIDC_ISSUER_URL/CLIENT_ID/CLIENT_SECRET/REDIRECT_URL/UI_REDIRECT_URLand every route returns 501 — no other gateway behavior changes.OidcCallbackHandlerthat picks up the#access_token=/#error=URL fragment on load and populates the store.Security review caught and fixed a CRITICAL bug before merge
Because this is new authentication surface, I ran a security-auditor review before finalizing. It found a critical cross-tenant identity-linking vulnerability: the flow-state cookie carrying
linking_tenant_id(which tenant a verified identity gets bound to) was plain base64 JSON with no signature. An attacker could forge the cookie with an arbitrary victimlinking_tenant_id, complete a normal OIDC login for their own identity, and bind their own account to the victim's tenant —link/start's bearer-token gate only controlled what the server wrote into the cookie, not what the client presented back at the callback.Fix: the flow-state cookie is now HMAC-tagged with the same
AEGIS_JWT_SECRETused to mint the JWT (OidcFlowState::encode/decodenow require the key; a cookie that doesn't verify is treated as absent, not partially trusted). Added a regression test (callback_rejects_a_flow_state_cookie_not_signed_by_the_gateway) that forges a cookie with a different key and asserts the identity is never linked.Also fixed, from the same review:
link_oidc_identity— theINSERTnow carries the uniqueness guarantee directly (oidc_identities.UNIQUE(issuer, subject)); theSELECTonly runs to classify an actual conflict.AEGIS_JWT_REQUIRED=truemeans the pre-existing rawBearer tenant_<id>fallback still bypasses OIDC entirely.Secureattribute (matches the existingaegis_csrfprecedent; deviating would break the documented plain-HTTP local/CI dev flow).Test plan
cargo check/cargo fmt --check/cargo clippy --workspace --all-targets -D warnings— cleancargo test --workspace -- --test-threads=1— all passoidc_identities(link/lookup round-trip, idempotent re-link to same tenant, rejected re-link to a different tenant, tenant-scoped listing)bun run build/bunx tsc --noEmit/bun run lint/bun test(ui-next) — all pass, including new unit tests for the callback-fragment parser