fix(auth): portal + CLI SSO — authorization-code + PKCE, default over implicit (1.9.4) - #107
Open
123andy wants to merge 1 commit into
Open
fix(auth): portal + CLI SSO — authorization-code + PKCE, default over implicit (1.9.4)#107123andy wants to merge 1 commit into
123andy wants to merge 1 commit into
Conversation
… implicit (1.9.4)
Both of CCAG's own sign-in paths sent the browser to the IdP with
response_type=id_token (the OAuth implicit flow) and read the token
straight out of the URL:
- Portal SSO: build_provider_info hard-coded the implicit authorize URL;
the SPA's checkSsoCallback() parsed #id_token= and stored the raw
external token as its own bearer credential.
- CLI login (apiKeyHelper): same shape, redirect_uri=/auth/cli/callback,
client-side JS extracted the fragment and POSTed it to
/auth/cli/complete.
Implicit was removed from the OAuth spec entirely in OAuth 2.1 — the
identity token travels in the URL, which means browser history, Referer
headers, and any extension with tab-reading access can see it.
The fix: authorization-code + PKCE (S256), server-side exchange — the
code/verifier/token never touch browser JS.
- src/auth/pkce.rs (new): RFC 7636 verifier/challenge generation.
- src/auth/oidc.rs: discover_endpoints() now also resolves token_endpoint
(previously only authorization_endpoint); exchange_code_for_id_token()
does the back-channel POST to the IdP (public client, no secret — same
trust model any other consumer of this OIDC client already uses).
IdpConfig gains flow_type.
- src/api/sso_login.rs (new): portal login state (PKCE verifier + IdP
details) keyed by a random state, stored in proxy_settings — DB-backed
because a multi-replica deployment can have the callback land on a
different pod than the one that issued the login_url. New
GET /auth/sso/callback redeems the code, validates via the existing
MultiIdpValidator, issues CCAG's own session token, and redirects to
/portal#session_token=... — the external id_token itself never reaches
the browser.
- src/api/cli_auth.rs: same idea for the CLI flow. The stored session
value needed an explicit {"status": pending|complete} shape — a bare
non-empty verifier string would otherwise be indistinguishable from a
completed token if /auth/cli/poll landed mid-flow (regression test
included for exactly this). cli_callback() becomes a real server-side
handler for the code path (no client-side JS at all); the old
extract-and-POST page is kept as a fallback for any IdP still
explicitly configured for implicit.
- static/index.html: checkSsoCallback() reads session_token= (ours)
first, falls back to id_token= (legacy implicit, still supported
per-IdP).
Nothing breaks by default. flow_type defaults to authorization_code for
new env-configured IdPs (OIDC_FLOW_TYPE to override); existing
DB-configured IdPs keep whatever flow_type they already have (schema
default device_code falls through to today's implicit behavior,
unchanged) until an admin explicitly switches it via the portal Flow
Type dropdown. The fix activates per-IdP, not repo-wide.
base64 moved from dev-dependencies to a real dependency (pkce.rs needs
it at runtime, not just in tests).
Patch bumps the gateway and CLI to 1.9.4.
Written and tested with Claude Code — build/clippy/fmt clean, 524/525
lib tests pass (10 new: PKCE generation, endpoint discovery, code
exchange, and pending-vs-complete state discrimination for the CLI poll
race). The 1 failure (budget::notifications::tests::deliver_routes_to_webhook)
is a pre-existing, unrelated timing flake on this machine (duration_ms
truncates to whole milliseconds and can read 0 on a fast loopback mock)
— confirmed it fails identically on a clean checkout of main, nothing to
do with this change. Live end-to-end verified against a real Keycloak
IdP (both the portal and CLI flows) on a separate staging deployment
before porting this fix upstream.
🤖 Generated with Claude Code (https://claude.com/claude-code)
Co-Authored-By: Claude O4.8
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
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.
Both of CCAG's own sign-in paths sent the browser to the IdP with
response_type=id_token(the OAuth implicit flow) and read the token straight out of the URL:
build_provider_infohard-coded the implicit authorize URL; the SPA'scheckSsoCallback()parsed#id_token=and stored the raw external token as its ownbearer credential.
apiKeyHelper): same shape,redirect_uri=/auth/cli/callback,client-side JS extracted the fragment and POSTed it to
/auth/cli/complete.Implicit was removed from the OAuth spec entirely in OAuth 2.1 — the identity token
travels in the URL, which means browser history,
Refererheaders, and any extensionwith tab-reading access can see it.
The fix
Authorization-code + PKCE (S256), server-side exchange — the code/verifier/token never
touch browser JS:
src/auth/pkce.rs(new): RFC 7636 verifier/challenge generation.src/auth/oidc.rs:discover_endpoints()now also resolvestoken_endpoint(previously only
authorization_endpoint);exchange_code_for_id_token()does theback-channel POST to the IdP (public client, no secret — same trust model any other
consumer of this OIDC client already uses).
IdpConfiggainsflow_type.src/api/sso_login.rs(new): portal login state (PKCE verifier + IdP details)keyed by a random
state, stored inproxy_settings— DB-backed because amulti-replica deployment can have the callback land on a different pod than the one
that issued the
login_url. NewGET /auth/sso/callbackredeems the code, validatesvia the existing
MultiIdpValidator, issues CCAG's own session token, and redirectsto
/portal#session_token=...— the external id_token itself never reaches thebrowser.
src/api/cli_auth.rs: same idea for the CLI flow. The stored session value neededan explicit
{"status": pending|complete}shape — a bare non-empty verifier stringwould otherwise be indistinguishable from a completed token if
/auth/cli/polllandedmid-flow (regression test included for exactly this).
cli_callback()becomes a realserver-side handler for the code path (no client-side JS at all); the old
extract-and-POST page is kept as a fallback for any IdP still explicitly configured
for implicit.
static/index.html:checkSsoCallback()readssession_token=(ours) first,falls back to
id_token=(legacy implicit, still supported per-IdP).Nothing breaks by default.
flow_typedefaults toauthorization_codefor newenv-configured IdPs (
OIDC_FLOW_TYPEto override); existing DB-configured IdPs keepwhatever
flow_typethey already have (schema defaultdevice_codefalls through totoday's implicit behavior, unchanged) until an admin explicitly switches it via the
portal's Flow Type dropdown. The fix activates per-IdP, not repo-wide.
base64moved fromdev-dependenciesto a real dependency (pkce.rsneeds it atruntime, not just in tests). Patch bumps the gateway and CLI to
1.9.4.Testing
Written and tested with Claude Code — build/clippy/fmt clean, 524/525 lib tests pass
(10 new: PKCE generation, endpoint discovery, code exchange, and — the one I was most
worried about getting subtly wrong — pending-vs-complete state discrimination for the CLI
poll race). The 1 failure
(
budget::notifications::tests::deliver_routes_to_webhook) is a pre-existing, unrelatedtiming flake on this machine (
duration_mstruncates to whole milliseconds and can read0 on a fast loopback mock) — confirmed it fails identically on a clean checkout of
main,nothing to do with this change.
Also live end-to-end verified against a real Keycloak IdP — both the portal and CLI
apiKeyHelperflows — on a separate staging deployment before porting this fix upstream.🤖 Generated with Claude Code