auth: OIDC support for osctrl-api + React SPA#830
Open
alvarofraguas wants to merge 12 commits into
Open
Conversation
Scaffolds the protocol-agnostic federated-identity surface for
osctrl-api. No concrete provider yet — that lands on the next branch
(auth/01-oidc-provider).
== types.go ==
Defines:
- Provider interface: Type / LoginURL / HandleCallback. Context-aware
on every method so a slow IdP cannot wedge a handler.
- ResolvedIdentity: protocol-neutral output of HandleCallback. Subject
is the stable identifier; PreferredUsername is the display/match
field; Email is informational (mutable, spoofable — threat T24);
Groups carries IdP group memberships for the RequiredGroups gate.
- State: per-login data the caller must round-trip from LoginURL to
HandleCallback. Contains EnvUUID (locks to one env — threat T18),
Nonce (256-bit cryptorandom), and PKCE Verifier.
- Type constants: TypeOIDC ("oidc") + reserved TypeSAML ("saml").
== state.go ==
State-as-JWT cookie implementation. HS256-signed with the api's
existing JWT secret; audience-scoped to "osctrl-auth-state" so a
state JWT cannot be used as a user-auth JWT and vice versa
(threat T19). Cookie attributes:
- Name: osctrl_auth_state
- Path: /api/v1/auth/ (scoped so the cookie doesn't leak to
other endpoints)
- HttpOnly + Secure + SameSite=Lax
- MaxAge: 10 minutes
Verification enforces in order: cookie present, HS256 pinned
(WithValidMethods), signature valid, iss == osctrl-api, aud ==
osctrl-auth-state, exp/nbf valid, EnvUUID + Nonce non-empty.
Single ErrStateInvalid for all failure modes — external observers
cannot distinguish "tampered" from "expired" from "wrong audience"
(threat T31).
NewNonce returns 256-bit cryptorandom base64url-encoded values.
ClearStateCookie deletes the cookie immediately after a successful
parse so a callback URL cannot be replayed (threat T9).
== state_test.go ==
15 tests covering each threat vector in scope for this commit:
TestNonceUnique — sanity on cryptorandom (T7)
TestStateRoundTripHappy — canonical good path
TestStateMissing — no cookie → ErrStateMissing (T6)
TestStateEmptyValue — defensive: empty cookie value
TestStateSignatureTampered — flipped sig byte → ErrStateInvalid (T31)
TestStateExpired — past-exp token rejected (T9)
TestStateWrongAudience — user-auth aud rejected (T19)
TestStateWrongIssuer — wrong iss rejected (defense in depth)
TestStateWrongSecret — attacker-signed rejected (T27)
TestStateAlgNoneRejected — alg=none rejected (alg-confusion)
TestIssueRejectsEmptyEnv — empty EnvUUID refused (T18 groundwork)
TestIssueRejectsEmptyNonce — empty Nonce refused
TestCookieAttributes — HttpOnly+Secure+SameSite+Path (T11/T12)
TestClearStateCookie — path matches for browser-side delete
TestStateIsZero — IsZero helper
All 15 pass. `go vet ./pkg/auth/...` clean. `go build ./...` clean.
Lifts the OIDC protocol code from cmd/admin/oidc.go into a reusable
pkg/auth/oidc package implementing the auth.Provider interface from
auth/00-shared-pkg. Legacy admin is refactored to consume the shared
package; behavior is preserved (backwards-compat audit in spec).
== pkg/auth/oidc/ ==
config.go — Config struct decoupled from YAML/CLI bindings, with
Validate() and effective*() helpers. New field
LegacyPermissiveUsername for the backwards-compat shim.
claims.go — pickUsername (claim selection), sanitizeUsername
(strict regex), hasRequiredGroup (group gate with
malformed-claim defense).
provider.go — NewOIDCProvider, LoginURL, HandleCallback. Implements
the auth.Provider interface (compile-time check).
HandleCallback runs the full 10-step verification
chain documented inline.
Test coverage (28 oidc subtests; 15 pkg/auth subtests = 43 total):
TestT1ForgedSignature — sig mint-by-attacker rejected
TestT2WrongIssuer — iss claim mismatch rejected
TestT3WrongAudience — aud claim mismatch rejected
TestT4Expired — exp in the past rejected
TestNonceMismatch — id_token nonce != state nonce
TestT10PKCEVerifierMissing — PKCE-on + no verifier rejected
TestT17RequiredGroupMissing — gate denies wrong group
TestT17GroupsClaimMalformed — string-shaped claim denies
TestRequiredGroupSatisfied — happy-path group OK
TestT18StateEnvMismatch — cross-env state replay rejected
TestT23UsernameInjection (6 sub) — SQL/newline/NUL/etc usernames rejected
TestIdPErrorParam — IdP error_description NOT leaked
TestMissingCode — empty code rejected
TestLegacyPermissiveUsername(2) — shim accepts email-format,
still rejects empty/whitespace
TestHandleCallbackHappy — full successful exchange
TestNewOIDCProviderDiscoveryFails — bad discovery rejected at init
TestLoginURLValidatesState — empty State.EnvUUID/Nonce rejected
TestLoginURLPKCERequired — verifier-missing rejected at LoginURL
+ claim/config unit tests
The in-process fakeIdP test harness boots a real httptest.Server that
serves discovery + JWKS + token endpoints. id_tokens are
RS256-signed with an in-memory RSA key, so the signature/iss/aud/exp
checks all exercise the genuine go-oidc.Verifier path, not a mock.
== cmd/admin/oidc.go refactor ==
The file shrinks from 360 LoC to ~180 LoC by replacing the inline
protocol implementation with calls into pkg/auth/oidc. Preserved:
- Route names (/login, /oidc/callback)
- YAML config field names
- Session-cookie + session-manager flow
- JIT-provisioning policy (zero permissions)
- Audit-log behavior
State-cookie storage switches from gorilla/securecookie to
pkg/auth.IssueStateCookie (HMAC JWT signed with the existing
flagParams.Admin.SessionKey). State cookie name changes from
"osctrl-admin-oidc-state" to "osctrl_auth_state" — operators
mid-OIDC-login during a deploy will need one retry. Documented in
the spec's "accepted minor break" section.
Username validation: pre-refactor cmd/admin did no character-class
check, so deployments where IdPs emit email-format
preferred_username values rely on permissive acceptance. The
Config.LegacyPermissiveUsername flag set by cmd/admin's
fromYAMLConfig preserves that behavior. New callers (cmd/api in a
later branch) leave the flag false and get the strict default.
== cmd/admin/main.go ==
Drops the no-longer-needed `var oidcRT *oidcRuntime` global and
updates initOIDC's call site to match the new (error)-only return.
== Verified ==
- go build ./... clean
- go vet ./... clean
- go test ./... all 16 packages green including the new auth tests
- Backwards-compat audit documented in spec § "Backwards compatibility
for cmd/admin"
Closes hardening item 5A from the auth spec's pentest plan.
Before: LoginURL passed state.EnvUUID as the OAuth2 `state` parameter,
and HandleCallback compared the returned URL state against
state.EnvUUID. Env identifiers are operator-visible (URLs, logs, UI)
and in many deployments are short, guessable strings ("admin",
"prod", "default"). An attacker who knows or guesses the target's
EnvUUID could craft a callback URL whose state parameter survives
step 2 of the verification chain. The defense was relying entirely
on the cookie's HMAC signature for CSRF protection — a single
mistake in cookie handling would have opened a CSRF hole.
After: LoginURL passes state.Nonce — the 256-bit cryptorandom value
already minted for the id_token nonce — as the OAuth2 state param.
HandleCallback checks `url.state == cookie.Nonce`. The value is now
unguessable, so step 2 alone gates CSRF whether or not the cookie's
HMAC happens to verify. Cookie HMAC remains the second layer.
State.EnvUUID stays in the cookie shape (LoginURL still rejects an
empty one) but becomes informational at the protocol layer. Callers
that want env-scoping above the protocol use it out-of-band:
cmd/admin sets it to "admin"; cmd/api will set it to a global
sentinel.
Test changes:
- TestT18StateEnvMismatch repurposed as TestT6CSRFStateTampering
with a more accurate threat label: env-replay isn't a threat at
this layer; CSRF/state-tampering is.
- TestLoginURLValidatesState now asserts state=<nonce> in the
authorize URL AND asserts that state=<env-uuid> does NOT appear.
- fakeCallback's stateParam argument documented as the nonce.
- TestMissingCode's state param updated so the test reaches step 3
(missing code) rather than failing at step 2 (state mismatch).
All 28 oidc subtests pass. `go build ./...` and `go vet ./...` clean.
Bug found during Auth0 manual smoke: operators configuring
--oidc-username-claim with a non-standard name (nickname, upn,
login, etc.) silently got the subject claim back instead. The old
switch statement only covered preferred_username/email/sub; any
other configured name fell through to subject — which on Auth0 is
"auth0|hex…" containing the pipe character the strict username
regex rejects.
After: pickUsername now consults a generic raw claim map after the
typed-struct fast path. Any string-valued claim wins; non-string
values (arrays, objects, missing) fall back to subject. The typed
struct fast path stays for the three known claims since it avoids
a map lookup in the hot path.
== Files ==
pkg/auth/oidc/claims.go
* pickUsername(c, raw, claim) signature; reads raw[claim] for
claim names not in the typed struct
pkg/auth/oidc/provider.go
* HandleCallback decodes raw map ONCE (was duplicated — earlier
decode for ResolvedIdentity.Raw moved up so pickUsername can
reuse it)
pkg/auth/oidc/claims_test.go
+ TestPickUsernameCustomClaim covers nickname/upn/missing/
non-string/nil-map paths
== Why this matters ==
Auth0's default sub format (auth0|hex) means operators MUST set a
non-default username claim — and the only practical one in the
profile scope is `nickname`. Without this fix the manual smoke test
against Auth0 silently fails 100% of the time, regardless of how the
operator configures their app.
The fix also unblocks Entra ID (which uses `upn`) and any custom
claim shape operators have set up via Auth0 Actions or Keycloak
mappers.
== Verified ==
- go build ./... clean
- go test ./pkg/auth/... -count=1 — all pass (includes the new
3-subtest TestPickUsernameCustomClaim covering nickname, upn,
missing claim, non-string value, nil raw map)
Adds federated authentication to osctrl-api as an additive surface
alongside the existing password flow. Lands the implementation for
the "OIDC is global, not per env" pivot — no env_auth_providers
table, no per-env IdP config, just one deployment-wide IdP gated by
--oidc-enabled.
== Why global ==
osctrl-api is a single API surface fronting all envs. The user JWT
issued by /login/{env} is env-scoped at issue time (CheckPermissions
runs against the env in the URL), but federated identity is not —
the same Keycloak user maps to the same AdminUser row regardless of
which env tab they were viewing. Modeling OIDC per-env added a DB
table, an AES-256-GCM secret-box, and an env-aware route shape, all
for zero operational benefit (no jmpsec deployment runs distinct
IdPs per env). Spec § "OIDC is global" documents the choice.
== New handler files ==
cmd/api/handlers/auth_methods.go — GET /api/v1/auth/methods
cmd/api/handlers/auth_oidc.go — InitOIDC + login + callback
cmd/api/handlers/auth_resolve.go — protocol-neutral user resolve
cmd/api/handlers/auth_jwt.go — session cookie issuance shared
between password + OIDC paths
The auth_resolve and auth_jwt helpers are protocol-neutral by design
so the eventual SAML provider can reuse them verbatim.
== Wire-level behavior ==
AuthMethodsHandler returns {methods:[...]} unauthenticated, leaking
nothing about the IdP. SPA polls this on the login screen to decide
whether to render the SSO button.
OIDCLoginHandler issues the state-JWT cookie (audience
"osctrl-auth-state" segregates from user JWTs) and 302-redirects to
the IdP authorize endpoint with state=Nonce nonce=Nonce — the same
unguessable 256-bit value carried twice via different protocol
parameters (the 5A invariant from pkg/auth/oidc/provider.go).
OIDCCallbackHandler parses the state cookie, clears it
(single-use, T9), runs the 10-step provider verification, resolves
the identity to an AdminUser (existing row or JIT per
--oidc-jit-provision), mints user JWT + CSRF cookies via the shared
userJWTSessionTokens helper, and 302-redirects to "/". Every
failure path also 302s to "/" — the SPA shows the login screen and
the server-side log records WHY (timing-oracle defense, T31).
== Config + flag changes ==
pkg/config/types.go — YAMLConfigurationOIDC.Enabled bool
pkg/config/flags.go — --oidc-enabled flag (OIDC_ENABLED env)
initOIDCFlags wired into InitAPIFlags
(it was already wired into InitAdminFlags)
cmd/api/main.go — OIDC: &config.YAMLConfigurationOIDC{} in
flagParams init
InitOIDC called before handler init when
--oidc-enabled, fatal on failure
WithJWTSecret + WithOIDC passed to
CreateHandlersApi
New route block under "UNAUTHENTICATED":
GET /api/v1/auth/methods (pre-auth limit)
GET /api/v1/auth/oidc/login (login limit)
GET /api/v1/auth/oidc/callback (pre-auth)
OIDC routes registered ONLY when enabled
osctrl-admin is untouched: it uses --auth=oidc (a separate mode), and
ignores the Enabled flag.
== Username sanitization ==
cmd/api callers leave LegacyPermissiveUsername=false (the strict
regex applies). Greenfield surface — no pre-existing email-format
usernames to preserve. Operators with email-shaped usernames must
configure --oidc-username-claim sub or similar.
== Tests added (13 new subtests in cmd/api/handlers) ==
auth_methods_test.go:
TestAuthMethodsPasswordOnly — default branch shape
TestAuthMethodsWithOIDC — OIDC appended when enabled
TestAuthMethodsContentType — JSON content-type
TestAuthMethodResponseJSONShape — JSON field names locked
auth_oidc_test.go (against in-process fake IdP):
TestOIDCLoginRedirectsToIdP — 302 + state=nonce invariant
TestOIDCLoginPKCEIncluded — code_challenge present
TestOIDCLoginNoProvider — 503 when InitOIDC skipped
TestOIDCLoginNoJWTSecret — 500 when secret empty
TestOIDCCallbackNoProvider — 503 fail-closed
TestOIDCCallbackNoJWTSecret — 500 when secret empty
TestOIDCCallbackMissingStateCookie — 302 to /
TestOIDCCallbackTamperedStateCookie — 302 to /, no leak
TestOIDCCallbackForeignAudience — wrong-aud JWT rejected
Happy-path callback (real DB-backed user resolution) is deferred —
pkg/auth/oidc/provider_test.go covers the 10-step verification chain
against an in-process fake IdP; replicating that here would
duplicate the harness without adding signal.
== Verified ==
- go build ./... clean
- go vet ./... clean
- go test ./... — all 16 packages green
- 13 new subtests pass in cmd/api/handlers
- 28 subtests in pkg/auth/oidc still pass (5A invariant preserved)
Adds a Federated-login surface to the React SPA, completing the SPA
side of the OIDC track. The button appears below the existing
password form, separated by an "or" divider, when the API advertises
an oidc method via /api/v1/auth/methods. Deployments without an
IdP configured see no visual change.
== Wire-level behavior ==
src/api/client.ts:
- AuthMethod type mirroring the API's response shape.
- listAuthMethods() — direct fetch (NOT apiFetch) so the call can
never trigger the 401-redirect loop on the login page itself.
Returns [] on shape mismatch / network error; throws on non-2xx.
src/features/login/LoginPage.tsx:
- useQuery against listAuthMethods alongside the existing env
list query. staleTime=5min, retry=1, fail-soft.
- oidcMethod = find(type==='oidc'). If undefined, no surface
renders.
- Native <a href={oidcMethod.loginUrl}> rather than a Button. The
href MUST be a full-page navigation: the IdP flow goes browser →
/api/v1/auth/oidc/login (302) → IdP authorize (302) → IdP login
page → IdP callback (302) → /api/v1/auth/oidc/callback (302) →
"/". fetch/XHR-driven navigation would break the cross-origin
302 chain.
- Styled to match Button variant="secondary" size="lg" so the
button rhythm stays consistent with the primary submit.
- The divider uses --border + uppercase "or" microcopy in
mono-tabular, matching the brand guide.
== Tests added (3 subtests, src/features/login/LoginPage.test.tsx) ==
hides the SSO button when only password method is advertised
— most common deployment state; the unreachable button would
have been a regression.
renders the SSO button when oidc method is advertised
— asserts the anchor and verifies href matches the API's
loginUrl verbatim. Guards against drift between the SPA's URL
constant and the API's route registration.
hides the SSO button when methods endpoint errors
— fail-soft: password form must still work even if the methods
query throws.
The tests pin behavior at the boundary the user sees (DOM presence
+ href value). They don't reach into the React Query cache or
inspect internal state — the test is meaningful regardless of how
the component fetches.
== Verified ==
- npm run check (tsc --noEmit) — clean
- npm test — 95 tests across 20 files pass (was 92/19)
Next: auth/05 (Keycloak compose stack + Kali pentest pass).
AdminUser gains an AuthSource string field. JIT-provisioning via the
OIDC callback sets it to "oidc"; everything else leaves it empty
(the password / default path).
The field flows through projectAdminUserView → AdminUserView.AuthSource
(JSON tag auth_source) and is consumed by the SPA Users page, which
renders a small "OIDC" badge alongside the existing admin / service
labels. Operators can now distinguish at a glance which rows came
from federated login.
The auth flow itself doesn't gate on this field — an OIDC user with
a password set later (via the user-management API) can log in either
way by design. AuthSource is purely informational.
== Files ==
pkg/users/users.go — new AuthSource string column (GORM
AutoMigrate adds it on next startup)
pkg/types/types.go — AdminUserView.AuthSource with json tag
cmd/api/handlers/users.go — projectAdminUserView passes it through
cmd/api/handlers/auth_resolve.go — sets u.AuthSource = "oidc" on the
JIT path BEFORE Create()
frontend/src/api/types.ts — optional auth_source on AdminUser
frontend/src/features/users/UsersPage.tsx — renders an OIDC pill
when u.auth_source === 'oidc'
== Backwards compatibility ==
Existing rows have an empty AuthSource. Older /users responses (from
osctrl-api ≤ this commit) will simply omit the field — the SPA
treats absence as "not OIDC" → no badge. Newer SPA against older API
behaves identically.
== Verified ==
- go build + go vet clean
- frontend npm run check + npm test (99 tests) green
Fixes the "click Continue with SSO and it silently logs me back in"
bug. Three-tier teardown on POST /api/v1/logout:
1) Server-issued Set-Cookie headers expire both osctrl_token AND
osctrl_csrf (Max-Age=-1). The osctrl_token cookie is HttpOnly,
so JS-only logout could not clear it — previously a click on
"Sign out" cleared the in-memory CSRF + the osctrl_csrf cookie
but left osctrl_token sitting in the browser. On the next
"Continue with SSO" the still-valid osctrl_token rode the
top-level GET (SameSite=Lax) and the SPA treated the operator
as authenticated without going near Keycloak.
2) DB-level APIToken revocation via users.ClearToken. The auth
middleware (handlerAuthCheck) already enforces an APIToken-match
check against the stored row, so once we zero the column any
cached copy of the JWT — including the same token in another
browser tab — fails the next auth check with 401. Server-side
revocation is the load-bearing half: cookie expiry alone is
client-honor-system, APIToken="" makes the token a no-op even
if the bytes are replayed.
3) IdP end-session navigation. The response includes
idp_logout_url pulled from the discovery doc's
end_session_endpoint claim. The SPA navigates the browser to
${url}?post_logout_redirect_uri=${origin}/login. Keycloak
terminates its KEYCLOAK_IDENTITY cookie and bounces back to
/login — the next SSO click now actually prompts for
credentials instead of silently re-auth'ing.
Idempotent: a logout from an expired or anonymous session still
emits the cookie-clearing headers + the IdP URL. The endpoint is
NOT behind handlerAuthCheck for exactly this reason — logging out
from an expired session should not require fresh auth.
== Backend files ==
pkg/auth/oidc/provider.go
+ Provider.EndSessionURL() reads end_session_endpoint out of
the cached discovery doc; "" when unset. Memory read only,
no network call.
cmd/api/handlers/auth_logout.go (new)
+ LogoutResponse { idp_logout_url }
+ LogoutHandler: ClearToken via JWT cookie, Set-Cookie max-age=-1
for both cookies, returns IdP end-session URL.
cmd/api/main.go
+ POST /api/v1/logout under preAuthRateLimit (60/min/IP).
== Frontend files ==
src/api/client.ts
+ LogoutResponse type.
* logout() now async; POSTs /api/v1/logout, then navigates
to idp_logout_url with post_logout_redirect_uri pointing
back at /login. Falls back to /login on no-IdP or network
failure. window.location.href rather than router.navigate
because the post-logout URL is cross-origin to Keycloak.
src/components/chrome/UserMenu.tsx
* handleLogout() now just calls logout() (which owns the
navigation). Drops the unused router.navigate that was
racing with window.location.href.
== Keycloak realm config ==
(Out of repo, applied via the admin API at 192.168.31.239 against
the osctrl-api client in the osctrl realm.)
post.logout.redirect.uris adds:
http://192.168.31.239:8088/login
http://localhost:8088/login
http://192.168.31.239:8088/
http://localhost:8088/
== Verified ==
- go build clean
- frontend npm run check + npm test (104 tests) green
Fixes Keycloak 26's "Missing parameters: id_token_hint" error on
logout. Keycloak's RP-initiated logout requires EITHER id_token_hint
OR client_id alongside post_logout_redirect_uri — passing only the
redirect URI yields a Keycloak-side error page and the user is left
stuck mid-flow.
We choose the client_id path so we don't have to persist raw
id_tokens client-side (they carry email + arbitrary claims and are
privacy-sensitive). The client_id is not a secret — it already
appears in every authorize URL the SPA renders — so exposing it via
the /logout response is just plumbing.
== Wire change ==
LogoutResponse gains idp_client_id. The api sets it from the cached
oidcClientID package-global (mirrors cfg.ClientID, set during
InitOIDC). The SPA reads it from the same response and appends to
the end-session URL via URLSearchParams (cleaner than the prior
string-concat — handles encoding correctly).
The SPA falls back to omitting client_id when the api response
doesn't include it (older build, partial config). Keycloak will
then show its error page and the operator can navigate manually —
no silent breakage.
== Files ==
cmd/api/handlers/auth_oidc.go
+ oidcClientID package-global, set in InitOIDC
cmd/api/handlers/auth_logout.go
+ LogoutResponse.IdPClientID
+ populated from oidcClientID
frontend/src/api/client.ts
+ LogoutResponse.idp_client_id
* logout() builds the IdP URL with URLSearchParams and includes
client_id when present
Okta requires id_token_hint when chaining post_logout_redirect_uri
on /v1/logout — without it, the IdP returns "Missing parameter:
id_token_hint" and the operator gets stuck on the Okta error page.
Keycloak accepts either id_token_hint OR client_id as the hint;
we now send both and let the IdP pick whichever it knows.
== Implementation ==
pkg/auth/types.go
+ ResolvedIdentity.IDToken — the raw, already-verified id_token
string. Documented as logout-hint-only; not an auth credential.
pkg/auth/oidc/provider.go
* HandleCallback returns rawIDToken into ResolvedIdentity.IDToken
after the 10-step verification chain. Already verified (sig +
iss + aud + exp + nbf + nonce) by this point.
cmd/api/handlers/auth_oidc.go
* OIDCCallbackHandler now sets an HttpOnly osctrl_id_token cookie
after the user JWT is issued. Path=/api/v1/auth/ scope matches
the state cookie. Max-Age 8h — long enough for typical session
lifetimes, short enough to mostly age out before any leak.
cmd/api/handlers/auth_logout.go
+ LogoutResponse.IdPIDTokenHint json:idp_id_token_hint
* LogoutHandler reads the osctrl_id_token cookie, clears it with
Set-Cookie Max-Age=-1, and includes the value in the response.
The cookie value is the same id_token the IdP issued, so
handing it back to the browser is no information leak — we're
just routing it through the SPA so the IdP can verify session
ownership.
frontend/src/api/client.ts
+ LogoutResponse.idp_id_token_hint
* logout() reads the hint and appends id_token_hint=… to the IdP
URL alongside post_logout_redirect_uri + client_id. Whichever
param the IdP recognizes will work.
== Verified ==
- go build ./... clean
- npm run check + npm test (104 tests) green
This unblocks Okta integration (next: configure cmd/api with Okta
OIDC env vars + run the same probe set against the dev-* tenant).
Pentest finding: POST /api/v1/logout with no cookies still returned
LogoutResponse{IdPLogoutURL, IdPClientID, IdPIDTokenHint} populated
from oidcProvider/oidcClientID, leaking the OIDC tenant URL and
client_id to any drive-by curl. A reconnaissance attacker now gets
the bits they need to register their own client against the same
tenant or to fingerprint the IdP.
The endpoint is unauthenticated by design (idempotent logout — a user
with an expired token can still log out without re-authenticating
first) so we can't simply gate the route. Instead, the handler now
gates only the IdP-detail fields on whether the caller presented a
valid JWT, while preserving the idempotent cookie-clearing behavior.
Anonymous /logout: cookie-clearing Set-Cookie headers + empty
LogoutResponse{} body.
Authenticated /logout: cookie-clearing + IdP fields populated as
before, so the SPA can drive the IdP-side session termination.
Tests:
- TestLogoutAnonymousHidesIdPMetadata stands up a fake IdP, sets
oidcClientID, then hits /logout with no cookies and asserts all
three IdP fields are empty in the response body.
- TestLogoutClearsCookiesEvenWhenAnonymous pins the idempotent
contract: cookie-purge still happens for anonymous callers.
Pentest finding: pkg/auth/oidc set the OAuth2 `state` query param and the OIDC `nonce` query param to the SAME 256-bit random value (the single State.Nonce field), collapsing two protocol-level defenses onto one leak surface. A `state` value disclosed via a Referer header, access log, or proxy buffer simultaneously compromised the CSRF defense (matched against the cookie nonce on callback) AND the id_token replay defense (matched against the `nonce` claim). Fix: add State.OAuthState — a second 256-bit cryptorandom value, independent of State.Nonce. OIDC.LoginURL sends: state = OAuthState nonce = Nonce HandleCallback checks the URL `state` param against state.OAuthState (CSRF) and the id_token's `nonce` claim against state.Nonce (replay). A leak of either now compromises only one defense; the other still holds. SAML follows the same shape: RelayState carries OAuthState. State cookie format: a new `os` claim carries OAuthState; older cookies without it transparently fall back to using Nonce as OAuthState during the 10-minute StateCookieTTL, so in-flight logins don't break at upgrade time. Defense-in-depth guards: - IssueStateCookie refuses State with OAuthState == Nonce. - OIDC.LoginURL refuses State with OAuthState == Nonce. The whole point of the split is independence; allow same-value reuse and a future regression silently re-introduces the bug. Tests: - TestIssueRejectsEmptyOAuthState, TestIssueRejectsAliasedNonceAndOAuthState pin the cookie-issuer invariants. - TestParseLegacyCookieWithoutOAuthState pins the backward-compat fallback at the JWT-claim level. - pkg/auth/oidc fakeCallback sites updated to pass OAuthState as the URL `state` param; goodState() carries both values. - TestOIDCLoginRedirectsToIdP now asserts state != nonce (was: state == nonce, the inverted "5A invariant"). Callers updated: - cmd/api/handlers/auth_oidc.go mints OAuthState alongside Nonce before issuing the state cookie. - cmd/admin/oidc.go same.
6 tasks
javuto
added
osctrl-api
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
2 participants
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.
Summary
osctrl-apiusing thepkg/auth/oidcprovider from auth: lift OIDC into reusable pkg/auth provider package #829GET /api/v1/auth/methods,GET /auth/oidc/login,GET /auth/oidc/callback,POST /api/v1/logoutauth_source: oidc)/api/v1/logoutclears the DB token and returns IdP end-session URL (id_token_hintfor Okta,client_idfor Keycloak)stateandnonceare independent values (OIDC defense-in-depth — state for CSRF, nonce for replay)/logoutno longer leaks IdP tenant URL or client_idDepends on
Test plan
nicknameclaim,id_token_hintlogoutgo test ./...passesnpm run typecheck && npm run build)