refactor(auth): use go-oidc IDTokenVerifier instead of hand-rolled JWKS+JWT - #36
Merged
Conversation
…KS+JWT The previous internal/auth/jwt.go reimplemented what github.com/coreos/go-oidc already provides for free, and not particularly well: - hand-rolled JWKS fetcher with a 15-minute TTL and a kid lookup that stayed silently stuck across many cycles when the IdP rotated (the production symptom that prompted PR #35 — login broken for >2 days) - hand-rolled jwkToRSAPublicKey via big.Int because the chain assumed RSA only (`token.Method.(*jwt.SigningMethodRSA)` hardcoded) - hand-rolled iss / aud / exp checks where aud was assumed to be a string (multi-valued audiences would crash on .(string)) - no nonce, no nbf, no at_hash Replace with: keySet := oidc.NewRemoteKeySet(ctx, jwksURL) verifier := oidc.NewVerifier(issuer, keySet, &oidc.Config{ClientID: audience}) // … idToken, err := verifier.Verify(ctx, tokenString) RemoteKeySet handles the cache, including refresh-on-unknown-kid out of the box (the very bug PR #35 patched manually). IDTokenVerifier handles signature, iss, aud (string or array), exp, nbf — and accepts any algo advertised in the JWKS, not just RS256. Public surface preserved: - NewJWTValidator(jwksURL, issuer, audience, usernameClaim) — same args - (v *JWTValidator) Validate(token) (username, claims, error) — same shape - ValidateToken(token) (username, error) — kept for callers - SanitizeUsername(string) — unchanged Stats: - jwt.go: 269 -> 89 lines (-180, -67%) - golang-jwt/jwt/v5 dependency dropped - coreos/go-oidc/v3 added - net: ~30 lines of refactor logic vs ~270 lines of reimplemented standard
This was referenced Jun 4, 2026
AYDEV-FR
added a commit
that referenced
this pull request
Jun 4, 2026
Phase B of the auth-code-rationalisation pair. Continues the work of PR #36 (jwt.go -> go-oidc IDTokenVerifier) on the discovery/code-exchange side, replacing 386 lines of hand-rolled HTTP+url.Values plumbing with the canonical golang.org/x/oauth2 and github.com/coreos/go-oidc APIs. What's gone: - OIDCDiscovery struct + discoverOIDC HTTP fetch of the well-known doc. oidc.NewProvider does this and validates the document for us. - discoverOIDCWithRetry's hand-rolled retry. We keep the same retry policy (5 attempts, exponential backoff capped at 4 s) but wrap oidc.NewProvider instead of the raw HTTP call. - Dual-discovery dance for split-horizon issuers (fetch public, fall back to URL substitution). The new code uses oidc.InsecureIssuerURLContext to tell go-oidc which issuer to expect in tokens, and just substitutes the public base on the AuthorizationEndpoint we got from discovery. Single code path. - OIDCTokenResponse struct + exchangeCode's manual POST form-encode. oauth2.Config.Exchange does it, and oauth2.Token.Extra("id_token") pulls the OIDC-specific extra. - Login's hand-built query-string. oauth2.Config.AuthCodeURL(state) returns the full URL. Public surface preserved: - NewOIDCHandler(*config.Config) (*OIDCHandler, error) — same signature, same retry budget at boot, same "fatal if discovery never recovers" semantics (cmd/api/main.go logs Fatal then exits, k8s restarts). - (*OIDCHandler).Login / Callback / Logout fiber handlers — same shape, same returnUrl-via-hash-fragment redirect, same open-redirect guard on the returnUrl. State management unchanged (in-memory map + 5-min cleanup goroutine). Signed-cookie state would survive pod restarts but is a separate behavioural change; out of scope here. Stats: - oidc.go: 386 -> 252 lines (-134, -35%) - Together with PR #36: 655 -> 337 (-318, -48% of pre-refactor auth code) - golang.org/x/oauth2 promoted from indirect to direct (was already in go.sum thanks to other transitive deps).
AYDEV-FR
added a commit
that referenced
this pull request
Jun 4, 2026
Phase B of the auth-code-rationalisation pair. Continues the work of PR #36 (jwt.go -> go-oidc IDTokenVerifier) on the discovery/code-exchange side, replacing 386 lines of hand-rolled HTTP+url.Values plumbing with the canonical golang.org/x/oauth2 and github.com/coreos/go-oidc APIs. What's gone: - OIDCDiscovery struct + discoverOIDC HTTP fetch of the well-known doc. oidc.NewProvider does this and validates the document for us. - discoverOIDCWithRetry's hand-rolled retry. We keep the same retry policy (5 attempts, exponential backoff capped at 4 s) but wrap oidc.NewProvider instead of the raw HTTP call. - Dual-discovery dance for split-horizon issuers (fetch public, fall back to URL substitution). The new code uses oidc.InsecureIssuerURLContext to tell go-oidc which issuer to expect in tokens, and just substitutes the public base on the AuthorizationEndpoint we got from discovery. Single code path. - OIDCTokenResponse struct + exchangeCode's manual POST form-encode. oauth2.Config.Exchange does it, and oauth2.Token.Extra("id_token") pulls the OIDC-specific extra. - Login's hand-built query-string. oauth2.Config.AuthCodeURL(state) returns the full URL. Public surface preserved: - NewOIDCHandler(*config.Config) (*OIDCHandler, error) — same signature, same retry budget at boot, same "fatal if discovery never recovers" semantics (cmd/api/main.go logs Fatal then exits, k8s restarts). - (*OIDCHandler).Login / Callback / Logout fiber handlers — same shape, same returnUrl-via-hash-fragment redirect, same open-redirect guard on the returnUrl. State management unchanged (in-memory map + 5-min cleanup goroutine). Signed-cookie state would survive pod restarts but is a separate behavioural change; out of scope here. Stats: - oidc.go: 386 -> 252 lines (-134, -35%) - Together with PR #36: 655 -> 337 (-318, -48% of pre-refactor auth code) - golang.org/x/oauth2 promoted from indirect to direct (was already in go.sum thanks to other transitive deps).
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
1 participant
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.
Replaces the hand-rolled JWT validator with the canonical
github.com/coreos/go-oidcstack. Supersedes PR #35 — the bug it patched (kid-miss not triggering refresh) is fixed implicitly becauseRemoteKeySetdoes it natively.What was hand-rolled and is gone
jwkToRSAPublicKeyviabig.Intbecause the chain hardcodedtoken.Method.(*jwt.SigningMethodRSA)— any non-RSA IdP would have been rejected.iss/aud/expchecks whereaudwas assumed to be a string (multi-valued audiences crashed on.(string)).nbf, no nonce, noat_hash.What replaces it
RemoteKeySethandles the cache, refresh-on-unknown-kid (the PR fix(auth): refetch JWKS on unknown kid (rate-limited) #35 bug), key rotation transparently.IDTokenVerifierhandles signature + iss + aud (string or array) + exp + nbf, with whatever algorithm the JWKS advertises (not just RS256).NewRemoteKeySettakes the JWKS URL directly, decoupled from the expected issuer the tokens carry.Public surface preserved
NewJWTValidator(jwksURL, issuer, audience, usernameClaim)(*JWTValidator).Validate(token) → (username, claims, error)(*JWTValidator).ValidateToken(token) → (username, error)SanitizeUsername(string)No caller in
cmd/api,internal/handlers,internal/auth/middleware.goorinternal/controller/naming.goneeds to change.Stats
jwt.golinescoreos/go-oidc/v3 v3.18.0golang-jwt/jwt/v5Build green, existing
admin_test.gostill passes (it doesn't go through JWT validation).