refactor(auth): canonical go-oidc + oauth2 OIDC flow + CTF hardening - #39
Merged
Conversation
Four targeted fixes layered on top of the phase-A/B/C refactor. Every one uses an existing library helper — nothing hand-rolled. 1. Open-redirect via protocol-relative returnUrl (HIGH) The previous check `strings.HasPrefix(returnURL, "/")` accepted `//evil.com/x`, which c.Redirect then turns into a cross-origin redirect through the browser's protocol-relative URL rules. Add safeRelativePath: leading "/" plus url.Parse-confirmed empty scheme + empty host + nil userinfo. Covers //, /\\, http://, https://, javascript:, data:, userinfo tricks. Applied in both Login (before we sign the state) and Callback (before we honour the unwrapped blob). Test matrix in oidc_test.go locks the cases. 2. OIDC nonce (MEDIUM) The flow had no nonce, so a stolen id_token issued for our client_id could be replayed through /auth/callback. Add a 32-byte crypto/rand nonce baked into the (signed) state blob, passed to the IdP via oidc.Nonce(n) on AuthCodeURL, and checked against idToken.Nonce on return. 3. Server-side id_token verification (MEDIUM) We used to hand the raw id_token straight to the browser, trusting that the request-time JWT validator would catch a bad signature on the *next* call. Now Verify() runs at callback time — signature, iss, aud, exp, nbf, plus at_hash binding the id_token to the access_token it came with. Forged / unsigned tokens never reach the browser. 4. PKCE S256 (LOW) Adds the OAuth 2.1-recommended PKCE leg even though we're a confidential client. The code verifier is baked into the signed state, the S256 challenge is sent in AuthCodeURL via oauth2.S256ChallengeOption, and the verifier is passed to Exchange via oauth2.VerifierOption. An attacker who snags the auth code on the redirect leg can no longer redeem it. No new dependencies — all four leverage the go-oidc / oauth2 we already pulled in phases A/B. Public surface unchanged.
There was a problem hiding this comment.
Pull request overview
Hardens the OAuth2/OIDC Authorization Code flow in internal/auth to better withstand common redirect/state probing and token replay attempts, while keeping the flow stateless via a signed state blob.
Changes:
- Adds
safeRelativePathvalidation forreturnUrland applies it in both/auth/loginand/auth/callback. - Introduces OIDC nonce + PKCE S256, storing nonce/verifier in the signed state and enforcing them during callback.
- Verifies
id_tokenserver-side during callback before redirecting back to the SPA; adds tests for return URL validation.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| internal/auth/oidc.go | Adds nonce + PKCE + callback-time ID token verification, and hardens returnUrl handling. |
| internal/auth/oidc_test.go | Adds a test matrix for safeRelativePath to prevent open-redirect regressions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+273
to
+275
| // Verify the id_token server-side (signature, iss, aud, exp, nbf, at_hash) | ||
| // before handing it to the browser. Catches forged / unsigned id_tokens | ||
| // here instead of trusting them as far as the next API call. |
Two follow-ups from the inline review: 1. at_hash binding was claimed but never executed The Verify() comment said it checks at_hash, but go-oidc/v3's Verify only does signature + iss + aud + exp + nbf. at_hash needs an explicit idToken.VerifyAccessToken(accessToken) call. Add it: when the IdP set the claim (Dex does), confirm the id_token was issued together with the access_token we received; skip with a warn when the claim is absent (the spec allows that). 2. Smuggled fragment in returnUrl broke the SPA hand-off safeRelativePath accepts "/foo#section" (the path is fine, the fragment is just metadata). On callback we then build "/foo#section#token=..." — two hashes, which the SPA's consumeHashToken() doesn't parse (it expects location.hash to start with "#token="). Strip the user-supplied fragment before appending the token hash. stripFragment helper + test cases pinning the behaviour.
Follow-up to the Copilot review. The previous fix had two helpers parsing the same URL twice: - safeRelativePath: url.Parse(s), reject if scheme/host/user set - stripFragment: strings.Cut(s, "#"), throw away the tail stripFragment was the embarrassing one — net/url already exposes url.URL.Fragment = "" + url.URL.String(), no string fiddling required. Collapse both helpers into a single sanitizeRelativePath(s) (string, bool) that returns the canonical fragment-stripped form alongside the safety verdict. One Parse, one set of checks, one rebuild. Test suite merges the two matrices into a single table — including fragment-stripping cases that the previous split missed (e.g. "/?x=1#frag" must come back as "/?x=1", not "" + stray strings.Cut output).
Phase D — the last chunk of hand-rolled auth code goes away. zitadel/oidc's RelyingParty owns the whole OAuth2/OIDC flow: discovery, state cookie (signed + encrypted), PKCE S256, nonce, code exchange, id_token verification (signature, iss, aud, exp, nbf, at_hash). Everything we implemented + hardened across phases A–C and #39 is now provided by the library, with a single import. What's left in oidc.go is the dploy-specific glue we still own: - split-horizon AuthURL substitution (in-cluster discovery, public-host browser redirect) - sanitizeRelativePath for returnUrl (open-redirect guard + #fragment strip) - retry-on-discovery at boot - Fiber adapter via fiber/middleware/adaptor - the "#token=..." hand-off the SPA expects What's gone: - Manual state map / cookie / encoding - Manual PKCE generation + Exchange-time VerifierOption wiring - Manual nonce generation + idToken.Nonce check - Manual idToken.VerifyAccessToken for at_hash - Manual id_token Verify call + error mapping - stateBlob struct, securecookie.SecureCookie setup, consumeState helper Stats: - oidc.go: 305 -> 184 lines (-121, -40%) - Whole auth package: 655 -> 269 lines (-386, -59% of pre-refactor) - Direct deps net: +zitadel/oidc/v3 - Direct deps gone: jwt.go's golang-jwt was already dropped in phase A
AYDEV-FR
changed the title
Comment on lines
+157
to
+158
| stateFn := func() string { return returnURL } | ||
| return adaptor.HTTPHandler(rp.AuthURLHandler(stateFn, h.rp))(c) |
Comment on lines
171
to
174
| returnURL := "/" | ||
| if stateData != nil && strings.HasPrefix(stateData.ReturnURL, "/") { | ||
| returnURL = stateData.ReturnURL | ||
| if clean, ok := sanitizeRelativePath(state); ok { | ||
| returnURL = clean | ||
| } |
Comment on lines
+62
to
+67
| relyingParty, err := newRelyingPartyWithRetry(context.Background(), | ||
| cfg.OIDCIssuer, cfg.OIDCClientID, cfg.OIDCClientSecret, cfg.OIDCRedirectURL, | ||
| []string{oidc.ScopeOpenID, "email", "profile"}, | ||
| rp.WithCookieHandler(cookieHandler), | ||
| rp.WithPKCE(cookieHandler), | ||
| ) |
Three follow-ups from the new inline comments. 1. CRITICAL — split-horizon broke after the zitadel/oidc switch zitadel/oidc validates that the discovery doc's `issuer` matches the one we passed to NewRelyingPartyOIDC. Dex always advertises its configured (public) issuer regardless of which URL the request came through, so the previous call — passing the *internal* issuer — would have failed at boot with ErrIssuerInvalid. Fix: pass the public issuer as `issuer` and use rp.WithCustomDiscoveryUrl(internalURL) to fetch discovery via the in-cluster endpoint. The AuthURL rebasing on OAuthConfig() still applies (endpoints are request-host-derived by Dex), but TokenURL + JWKS stay internal as intended. 2. HIGH — state = sanitized returnUrl was predictable For the vast majority of callers returnUrl is just "/", which means `state` was guessable. An attacker triggering /auth/login on a victim's browser then driving them to /auth/callback with the attacker's own code+state=/ would have bypassed CSRF / session-fixation protection: the cookie-state vs query-state check trivially matches when both are "/". Fix: state is now "<16-byte nonce>:<returnUrl>". The nonce makes the state unguessable per login attempt; returnUrl piggybacks for free. The callback splits on the first ":" to recover the returnUrl (and re-sanitises it, defense in depth). 3. MEDIUM — process-random cookie keys broke multi-replica + rolling updates The previous code minted per-process hash/block keys, so a callback landing on a different pod than the one that started the login would fail to decrypt the state cookie. Fine for the current single-replica cluster, broken for any rolling update or replicaCount > 1. Fix: add OIDC_COOKIE_HASH_KEY / OIDC_COOKIE_BLOCK_KEY env vars (loaded via config), with a warn-and-random fallback when unset. Chart pipes them through the existing Secret, so values become `auth.oidcCookieHashKey` / `auth.oidcCookieBlockKey`. Documented in the values comment.
The split-horizon issuer support (in-cluster discovery URL + public AuthURL rebase) was already gated on OIDCPublicIssuer being set and distinct from OIDCIssuer, but the code structure read like the default path. Reword the two comment blocks to say "this is optional, no-op when you use a single URL" and update the chart values.yaml so operators know oidcPublicIssuer is only for split-horizon and can be left empty for the common single-URL setup. No behaviour change.
Three things I caught re-reading the diff one more time: 1. AuthURL rebase was silently no-op when Dex's discovery returned an AuthURL whose base didn't match OIDCIssuer (config drift, port mismatch, etc.). strings.Replace just returned the input unchanged and we logged "rebased to public" anyway, so the browser would redirect to the in-cluster URL it can't reach. Now we Contains-check first and fail boot with a clear error. 2. Cookie key warn was inconsistent — only HASH absence triggered the "process-random, breaks across replicas" warning, BLOCK absence was silent. If you set HASH but forgot BLOCK, you'd ship to prod with one stable key and one random one. Symmetrical warn now. 3. The "<nonce>:<returnUrl>" state wire format had no test. Extracted decodeStateReturnURL (consumed by Callback, mirrors Login's encoding) and pinned it with 11 cases: empty / malformed / path-with-colon / fragment-strip / the three classic open-redirect tricks injected via the returnUrl half, plus a round-trip check that nonce never leaks into the redirect.
Comment on lines
+59
to
+69
| hashKey := []byte(cfg.OIDCCookieHashKey) | ||
| blockKey := []byte(cfg.OIDCCookieBlockKey) | ||
| if len(hashKey) == 0 { | ||
| hashKey = securecookie.GenerateRandomKey(64) | ||
| logger.Warn("OIDC_COOKIE_HASH_KEY not set; using a process-random key — logins will break across pod restarts or replicas") | ||
| } | ||
| if len(blockKey) == 0 { | ||
| blockKey = securecookie.GenerateRandomKey(32) | ||
| logger.Warn("OIDC_COOKIE_BLOCK_KEY not set; using a process-random key — logins will break across pod restarts or replicas") | ||
| } | ||
| cookieHandler := httphelper.NewCookieHandler(hashKey, blockKey, cookieOpts...) |
Comment on lines
+87
to
+93
| # Cookie keys for the zitadel/oidc state + PKCE cookies. Leave empty for | ||
| # single-replica dev (API will mint per-process random keys + log a warn). | ||
| # MUST be set to stable secrets when running with replicaCount > 1 or | ||
| # tolerating rolling updates without breaking in-flight logins. | ||
| # Generate with: head -c 64 /dev/urandom | base64 | ||
| oidcCookieHashKey: "" | ||
| oidcCookieBlockKey: "" |
Comment on lines
+21
to
+24
| {{/* Cookie keys for the zitadel/oidc state + PKCE cookies. When unset the | ||
| API generates per-process random keys (logs a warning) — fine for a | ||
| single replica; mandatory for replicaCount > 1 or rolling-update | ||
| resilience. Generate with `head -c 64 /dev/urandom | base64`. */}} |
Comment on lines
+23
to
+29
| // Cookie keys for the OIDC state / PKCE cookies (zitadel/oidc CookieHandler). | ||
| // Set both to stable random secrets (k8s Secret, env, etc.) when running | ||
| // with replicaCount > 1 or you'll get login failures on rolling updates, | ||
| // since requests can land on a pod that can't decrypt cookies set by | ||
| // another. Empty values fall back to process-random keys with a warn. | ||
| OIDCCookieHashKey string // 64 bytes recommended (HMAC key) | ||
| OIDCCookieBlockKey string // 32 bytes (AES key) |
Drop zitadel/oidc, gorilla/securecookie and the derived-cookie-keys plumbing. The OIDC RP is now built from the two libraries every other Go project reaches for (coreos/go-oidc + golang.org/x/oauth2) and follows the official go-oidc README example: three short-lived HttpOnly, SameSite=Lax cookies carrying state / PKCE verifier / returnUrl across the login bounce, plus a state-mismatch check on the callback. What this removes: - the zitadel/oidc RelyingParty + CookieHandler dance and its custom discovery URL plumbing - the OIDC_COOKIE_HASH_KEY / OIDC_COOKIE_BLOCK_KEY env vars (and the matching Helm values, Secret entries, config fields, base64-decode helper, length validation, and tests) - the "<nonce>:<returnUrl>" state encoding (returnUrl now lives in its own cookie, no encoding gymnastics) - the fiber/middleware/adaptor bridge — handlers are native Fiber What this keeps: - split-horizon issuer support via oidc.InsecureIssuerURLContext + AuthURL rebase (gated on OIDCPublicIssuer, no-op when single URL) - the discovery retry-on-boot for Cilium-style network-identity races - sanitizeRelativePath + open-redirect tests - ID token verification (signature, iss, aud, exp, nbf) via the Provider's IDTokenVerifier CSRF protection comes from the state match (cookie vs query); replay protection from PKCE S256; cookie tampering surface is bounded by HttpOnly + Secure + SameSite=Lax + a /auth path scope. No shared key between replicas needed because the flow cookies travel with the user end-to-end. Net diff vs the previous PR head: ~170 fewer lines of auth code, two fewer direct deps, zero new operator config knobs.
State + PKCE cover CSRF and code-replay, but the OIDC spec recommends nonce as the third leg: it binds the *ID token* (not just the code) to this specific login attempt. The attack it closes is the one where both the auth code and the PKCE verifier leak together — the IdP would issue a token carrying the attacker's nonce, not ours. Mint a 16-byte random nonce per Login, ship it to the IdP via oidc.Nonce(...) on AuthCodeURL, store it alongside state/verifier in an HttpOnly cookie, then compare against idToken.Nonce in Callback with subtle.ConstantTimeCompare. Strict mode: a missing nonce cookie fails the flow with "missing or expired login session" same as missing state/verifier.
Comment on lines
232
to
+236
| func (h *OIDCHandler) Callback(c *fiber.Ctx) error { | ||
| if errorParam := c.Query("error"); errorParam != "" { | ||
| errorDesc := c.Query("error_description", errorParam) | ||
| return c.Status(fiber.StatusBadRequest).JSON(fiber.Map{"error": errorDesc}) | ||
| } | ||
| code := c.Query("code") | ||
| state := c.Query("state") | ||
| if code == "" || state == "" { | ||
| return c.Status(fiber.StatusBadRequest).JSON(fiber.Map{"error": "missing code or state parameter"}) | ||
| state := c.Cookies(cookieState) | ||
| verifier := c.Cookies(cookieVerifier) | ||
| nonce := c.Cookies(cookieNonce) | ||
| returnURL := c.Cookies(cookieReturn) |
Comment on lines
251
to
254
| token, err := h.oauth2Config.Exchange(c.Context(), c.Query("code"), oauth2.VerifierOption(verifier)) | ||
| if err != nil { | ||
| return c.Status(fiber.StatusInternalServerError).JSON(fiber.Map{ | ||
| "error": fmt.Sprintf("failed to exchange code: %v", err), | ||
| }) | ||
| return c.Status(fiber.StatusBadGateway).JSON(fiber.Map{"error": "token exchange failed: " + err.Error()}) | ||
| } |
Copilot round #4 caught two diagnostic gaps in the new go-oidc-style callback: 1. The IdP signals failure (user cancelled, consent denied, scope rejected, …) by redirecting back with ?error=<code>& error_description=<text> per OAuth 2.0 §4.1.2.1, NOT with a code. Previous code fell through to state validation and returned a "state mismatch" or "token exchange failed" error that hid the actual IdP reason. Now we short-circuit on ?error= with a 400 that carries through the IdP's error code and description. 2. An empty ?code= (IdP misbehaving, or someone hitting /auth/callback directly) reached oauth2.Exchange and surfaced as a 502 from the token endpoint. Now it's caught up front with a clean 400 "missing authorization code". Both checks happen after the one-shot cookie clear, so a failed flow can't be replayed by re-hitting the callback URL.
| if subtle.ConstantTimeCompare([]byte(idToken.Nonce), []byte(nonce)) != 1 { | ||
| return c.Status(fiber.StatusBadGateway).JSON(fiber.Map{"error": "nonce mismatch"}) | ||
| } | ||
|
|
Comment on lines
+268
to
271
| token, err := h.oauth2Config.Exchange(c.Context(), code, oauth2.VerifierOption(verifier)) | ||
| if err != nil { | ||
| return c.Status(fiber.StatusInternalServerError).JSON(fiber.Map{ | ||
| "error": fmt.Sprintf("failed to exchange code: %v", err), | ||
| }) | ||
| return c.Status(fiber.StatusBadGateway).JSON(fiber.Map{"error": "token exchange failed: " + err.Error()}) | ||
| } |
Comment on lines
+276
to
+279
| idToken, err := h.verifier.Verify(c.Context(), rawIDToken) | ||
| if err != nil { | ||
| return c.Status(fiber.StatusBadGateway).JSON(fiber.Map{"error": "id_token verification failed: " + err.Error()}) | ||
| } |
AYDEV-FR
changed the title
oauth2 and go-oidc error strings embed the token endpoint and issuer URLs — internal cluster topology a CTF player shouldn't get for free. Log the detail, return a stable generic message to the browser. Addresses Copilot review round 5 (1/3 and 2/3); 3/3 (at_hash) is resolved in the PR description instead: the access token is discarded, so the at_hash binding has nothing to protect.
CI lint has been red on every branch since the action's 'latest'
moved to golangci-lint v2, which rejects the v1 config schema (and
the last v1 release can't even run against Go 1.26). Migrated with
'golangci-lint migrate', then tuned:
- exhaustive: default-signifies-exhaustive (all flagged switches
already had an explicit default)
- drop goconst (pure noise: "true", "error", k8s condition strings)
- govet: disable shadow ('if err :=' inside an err scope is idiomatic)
- gocritic: disable hugeParam (controller-runtime passes specs by value)
- gochecknoinits excluded for api/ and cmd/ (kubebuilder scheme
registration pattern), gocyclo excluded for Reconcile loops
- errcheck back to defaults (check-blank flagged deliberate discards)
Code fixes: gofmt x4, misspell x4, QF1008 embedded selector, named
results on JWTValidator.Validate, nolint on the scaffolded
scheme.Builder. golangci-lint v2.12.2 now reports 0 issues.
Found by deploying the PR build to the CTF cluster: the split-horizon AuthURL rebase required the discovery doc's AuthURL to be based on the internal issuer URL, and returned an error otherwise — crashlooping the pod. Dex doesn't derive endpoints from the request Host (the old comment claimed it does): it bakes its configured issuer into every endpoint, so fetching discovery via the internal URL still yields public endpoints, and there is nothing to rebase. AuthURL already on the public base -> leave it (the common Dex case); on the internal base -> rebase; neither -> warn and keep it, instead of refusing to boot a config the IdP considers valid.
Comment on lines
+295
to
297
| if subtle.ConstantTimeCompare([]byte(idToken.Nonce), []byte(nonce)) != 1 { | ||
| return c.Status(fiber.StatusBadGateway).JSON(fiber.Map{"error": "nonce mismatch"}) | ||
| } |
Comment on lines
+157
to
167
| func sanitizeRelativePath(s string) (string, bool) { | ||
| if !strings.HasPrefix(s, "/") || strings.HasPrefix(s, "//") || strings.HasPrefix(s, "/\\") { | ||
| return "", false | ||
| } | ||
| u, err := url.Parse(s) | ||
| if err != nil || u.Scheme != "" || u.Host != "" || u.User != nil { | ||
| return "", false | ||
| } | ||
| u.Fragment = "" | ||
| return u.String(), true | ||
| } |
Comment on lines
+24
to
+28
| {"//evil.com/x", "", false}, // protocol-relative URL | ||
| {"/\\evil.com/x", "", false}, // backslash-prefixed (some browsers) | ||
| {"http://evil.com/x", "", false}, // absolute URL | ||
| {"https://evil.com/x", "", false}, // absolute URL https | ||
| {"javascript:alert(1)", "", false}, |
…ings Address the latest Copilot review on PR #39: - sanitizeRelativePath: reject a backslash in the decoded path. A percent-encoded backslash ("/%5cevil.com") survived the literal "/\\" prefix check, and user agents that normalize "\" to "/" would read the decoded "//evil.com" as a protocol-relative redirect. - Callback: return 400 (not 502) on nonce mismatch — a client/session issue or attack, not an upstream failure, mirroring the state path. - Fix the package comment: the flow now carries four flow cookies (state/verifier/nonce/returnUrl), not three. - Pin the bypass with percent-encoded backslash test cases. Claude-Session: https://claude.ai/code/session_01M4Dd5oASBGHwr6ts6wTnYn
AYDEV-FR
added a commit
that referenced
this pull request
Jun 20, 2026
Two follow-ups from the inline review: 1. at_hash binding was claimed but never executed The Verify() comment said it checks at_hash, but go-oidc/v3's Verify only does signature + iss + aud + exp + nbf. at_hash needs an explicit idToken.VerifyAccessToken(accessToken) call. Add it: when the IdP set the claim (Dex does), confirm the id_token was issued together with the access_token we received; skip with a warn when the claim is absent (the spec allows that). 2. Smuggled fragment in returnUrl broke the SPA hand-off safeRelativePath accepts "/foo#section" (the path is fine, the fragment is just metadata). On callback we then build "/foo#section#token=..." — two hashes, which the SPA's consumeHashToken() doesn't parse (it expects location.hash to start with "#token="). Strip the user-supplied fragment before appending the token hash. stripFragment helper + test cases pinning the behaviour.
AYDEV-FR
added a commit
that referenced
this pull request
Jun 20, 2026
Phase D — the last chunk of hand-rolled auth code goes away. zitadel/oidc's RelyingParty owns the whole OAuth2/OIDC flow: discovery, state cookie (signed + encrypted), PKCE S256, nonce, code exchange, id_token verification (signature, iss, aud, exp, nbf, at_hash). Everything we implemented + hardened across phases A–C and #39 is now provided by the library, with a single import. What's left in oidc.go is the dploy-specific glue we still own: - split-horizon AuthURL substitution (in-cluster discovery, public-host browser redirect) - sanitizeRelativePath for returnUrl (open-redirect guard + #fragment strip) - retry-on-discovery at boot - Fiber adapter via fiber/middleware/adaptor - the "#token=..." hand-off the SPA expects What's gone: - Manual state map / cookie / encoding - Manual PKCE generation + Exchange-time VerifierOption wiring - Manual nonce generation + idToken.Nonce check - Manual idToken.VerifyAccessToken for at_hash - Manual id_token Verify call + error mapping - stateBlob struct, securecookie.SecureCookie setup, consumeState helper Stats: - oidc.go: 305 -> 184 lines (-121, -40%) - Whole auth package: 655 -> 269 lines (-386, -59% of pre-refactor) - Direct deps net: +zitadel/oidc/v3 - Direct deps gone: jwt.go's golang-jwt was already dropped in phase A
AYDEV-FR
added a commit
that referenced
this pull request
Jun 20, 2026
Three follow-ups from the new inline comments. 1. CRITICAL — split-horizon broke after the zitadel/oidc switch zitadel/oidc validates that the discovery doc's `issuer` matches the one we passed to NewRelyingPartyOIDC. Dex always advertises its configured (public) issuer regardless of which URL the request came through, so the previous call — passing the *internal* issuer — would have failed at boot with ErrIssuerInvalid. Fix: pass the public issuer as `issuer` and use rp.WithCustomDiscoveryUrl(internalURL) to fetch discovery via the in-cluster endpoint. The AuthURL rebasing on OAuthConfig() still applies (endpoints are request-host-derived by Dex), but TokenURL + JWKS stay internal as intended. 2. HIGH — state = sanitized returnUrl was predictable For the vast majority of callers returnUrl is just "/", which means `state` was guessable. An attacker triggering /auth/login on a victim's browser then driving them to /auth/callback with the attacker's own code+state=/ would have bypassed CSRF / session-fixation protection: the cookie-state vs query-state check trivially matches when both are "/". Fix: state is now "<16-byte nonce>:<returnUrl>". The nonce makes the state unguessable per login attempt; returnUrl piggybacks for free. The callback splits on the first ":" to recover the returnUrl (and re-sanitises it, defense in depth). 3. MEDIUM — process-random cookie keys broke multi-replica + rolling updates The previous code minted per-process hash/block keys, so a callback landing on a different pod than the one that started the login would fail to decrypt the state cookie. Fine for the current single-replica cluster, broken for any rolling update or replicaCount > 1. Fix: add OIDC_COOKIE_HASH_KEY / OIDC_COOKIE_BLOCK_KEY env vars (loaded via config), with a warn-and-random fallback when unset. Chart pipes them through the existing Secret, so values become `auth.oidcCookieHashKey` / `auth.oidcCookieBlockKey`. Documented in the values comment.
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.
OIDC login rewritten onto the canonical Go RP stack —
coreos/go-oidc/v3for discovery + ID-token verification,golang.org/x/oauth2for the Authorization Code + PKCE flow — following the official go-oidc README example. No framework, no hand-rolled crypto, no cookie-key management. The flow state (state, PKCE verifier, nonce, returnUrl) rides in four short-lived HttpOnly cookies (Path=/auth,SameSite=Lax,Secureon HTTPS, 10 min, cleared one-shot at callback).Hardening, layered on the rewrite
1. Open-redirect via protocol-relative
returnUrl(HIGH)The previous check
strings.HasPrefix(returnURL, "/")accepted//evil.com/x, whichc.Redirectthen turns into a cross-origin redirect through the browser's protocol-relative URL rules.sanitizeRelativePath: leading/plusurl.Parse-confirmed empty scheme + empty host + nil userinfo, fragment stripped. Covers//,/\\,http://,https://,javascript:,data:, userinfo tricks. Applied inLogin(before the cookie is set) and re-applied inCallback(defense-in-depth). 15-case test matrix inoidc_test.golocks the surface.2. CSRF state + OIDC nonce (MEDIUM)
Random
state(24 B) checked cookie-vs-query withsubtle.ConstantTimeCompare. Randomnonce(16 B) sent viaoidc.Nonce(...)and checked againstidToken.Nonceon return, so anid_tokenminted for someone else's login attempt can't be spliced into ours.3. Server-side id_token verification (MEDIUM)
We used to hand the raw
id_tokenstraight to the browser, trusting the request-time JWT validator to catch a bad signature on the next call. NowVerify()runs at callback time — signature,iss,aud,exp,nbf. Forged / unsigned tokens never reach the browser. (We don't verifyat_hash: the access token is discarded, only the id_token is used, so there's nothing for the binding to protect.)4. PKCE S256 (LOW)
OAuth 2.1-recommended PKCE even though we're a confidential client:
oauth2.GenerateVerifier()in a cookie, S256 challenge onAuthCodeURL, verifier passed toExchange. An attacker who snags the auth code on the redirect leg can no longer redeem it.5. Clean failure paths
IdP errors (
?error=per OAuth 2.0 §4.1.2.1) and a missingcodeget explicit 400s instead of falling through to misleading errors. Exchange/verification failure details are logged server-side only — the browser gets a stable, generic message (no internal IdP endpoints leaked).What we still rely on
exp./api/run/...requiresAuthorization: Bearerand CORS doesn't include credentials, so no browser-based CSRF surface today.Also in this PR
latestmoved to v2) + repo-wide lint fixes.