diff --git a/internal/server/scheduledauth/validator.go b/internal/server/scheduledauth/validator.go index f9ec30fbe..0e036f9c3 100644 --- a/internal/server/scheduledauth/validator.go +++ b/internal/server/scheduledauth/validator.go @@ -11,6 +11,7 @@ import ( "net/http" "net/url" "strings" + "sync" "time" "github.com/coreos/go-oidc/v3/oidc" @@ -61,15 +62,17 @@ type Config struct { // Validator authenticates inbound requests against the configured mode. type Validator struct { - verifier *oidc.IDTokenVerifier - keySet *oidc.RemoteKeySet - audiences map[string]struct{} - subjects map[string]struct{} - now func() time.Time - mode Mode - jwksURL string - bearer []byte - skew time.Duration + mode Mode + verifier *oidc.IDTokenVerifier // nil unless mode == ModeOIDC; backed by go-oidc's single-flight RemoteKeySet + verMu sync.RWMutex // guards verifier and lastRebuild during rotation-recovery rebuilds + lastRebuild time.Time // last verifier rebuild; rate-limits attacker-driven JWKS refetches + jwksURL string // remembered for Warmup and rotation rebuild; empty unless mode == ModeOIDC + issuer string // OIDC issuer URL; empty unless mode == ModeOIDC; needed to rebuild verifier + audiences map[string]struct{} + subjects map[string]struct{} + skew time.Duration + bearer []byte + now func() time.Time // pluggable for tests } // claims captures the timestamp claims we need beyond what go-oidc's @@ -163,8 +166,9 @@ func configureOIDC(v *Validator, cfg Config) (*Validator, error) { v.audiences = auds v.subjects = subs v.jwksURL = cfg.JWKSURL - v.keySet = oidc.NewRemoteKeySet(context.Background(), cfg.JWKSURL) - v.verifier = oidc.NewVerifier(cfg.Issuer, v.keySet, &oidc.Config{ + v.issuer = cfg.Issuer + keySet := oidc.NewRemoteKeySet(context.Background(), cfg.JWKSURL) + v.verifier = oidc.NewVerifier(cfg.Issuer, keySet, &oidc.Config{ // Pin to RS256. Google's tokens are RS256; rejecting anything // else closes the alg=none / alg=HS256 confusion family. SupportedSigningAlgs: []string{string(oidc.RS256)}, @@ -337,7 +341,14 @@ func (v *Validator) validateOIDC(ctx context.Context, authz string) error { // Verify signature, issuer, and algorithm via go-oidc. SkipClientIDCheck // and SkipExpiryCheck are enabled because we apply our own multi-aud // and skew-tolerant expiry checks below. - idToken, err := v.verifier.Verify(ctx, rawToken) + // + // verifyWithRotationRetry handles the go-oidc stale-inflight race: during + // key rotation, go-oidc's RemoteKeySet may cache a completed but + // not-yet-cleaned-up inflight request. A second goroutine joining that + // inflight receives pre-rotation keys, causing a spurious signature + // failure. The retry detects this class of error and rebuilds the + // RemoteKeySet from scratch (no stale inflight), then retries once. + idToken, err := v.verifyWithRotationRetry(ctx, rawToken) if err != nil { return fmt.Errorf("%w: %w", ErrUnauthorized, err) } @@ -372,6 +383,85 @@ func (v *Validator) validateOIDC(ctx context.Context, authz string) error { return nil } +// minRebuildInterval rate-limits verifier rebuilds. Without it, every +// bad-signature token would trigger a keyset rebuild plus an outbound JWKS +// GET on the retry, letting an unauthenticated attacker amplify garbage +// tokens into requests against the provider's JWKS endpoint. 10s bounds +// attacker-driven fetches to ~6/min while a genuine key rotation still +// recovers within one interval (providers publish the new key well before +// signing with it, so a >10s outage window here is not a realistic +// rotation pattern - and go-oidc's own refresh-on-unknown-kid still runs +// on every request regardless of this limit). +const minRebuildInterval = 10 * time.Second + +// verifyWithRotationRetry calls Verify on the current verifier. If the call +// fails with a signature error (unknown kid after key rotation, or stale +// inflight keyset) it rebuilds the RemoteKeySet and IDTokenVerifier from +// scratch to bypass go-oidc's cached inflight, then retries exactly once. +// +// Fail-closed constraints: +// - Retry only on signature errors (error contains "failed to verify signature"). +// aud/iss/exp/nbf/sub failures are returned immediately without retry. +// - At most one rebuild per rotation event: if another goroutine has already +// rebuilt the verifier (pointer changed under the write lock), we skip the +// rebuild and retry with the new verifier directly. +// - Rebuilds are rate-limited to one per minRebuildInterval; within the +// window the ORIGINAL signature error is returned without a retry +// (fail closed), preventing bad-token spam from amplifying into +// unbounded JWKS fetches. +// - No sleep; the retry itself re-fetching fresh keys is the synchronization. +// - Rebuild is logged once per rotation (not per failing request). +func (v *Validator) verifyWithRotationRetry(ctx context.Context, rawToken string) (*oidc.IDToken, error) { + v.verMu.RLock() + ver := v.verifier + v.verMu.RUnlock() + + idToken, err := ver.Verify(ctx, rawToken) + if err == nil || !isSignatureError(err) { + return idToken, err + } + + // Signature failure - possibly a stale inflight keyset from go-oidc's + // single-flight cache. Rebuild the RemoteKeySet (starts empty, no cached + // inflight) so the retry always performs a fresh JWKS fetch. The check + // v.verifier == ver prevents duplicate rebuilds when multiple goroutines + // hit the same rotation at once: only the first write-lock holder rebuilds; + // the rest find the pointer already updated and reuse the new verifier. + v.verMu.Lock() + if v.verifier == ver { + if !v.lastRebuild.IsZero() && v.now().Sub(v.lastRebuild) < minRebuildInterval { + // Rebuild budget exhausted: fail closed with the original + // signature error. No retry - a retry against the same verifier + // would still trigger go-oidc's internal refetch and reopen the + // amplification vector this limit exists to close. + v.verMu.Unlock() + return nil, err + } + log.Printf("scheduledauth: oidc signature verification failed; rebuilding key set for rotation retry") + newKS := oidc.NewRemoteKeySet(context.Background(), v.jwksURL) + v.verifier = oidc.NewVerifier(v.issuer, newKS, &oidc.Config{ + SupportedSigningAlgs: []string{string(oidc.RS256)}, + SkipClientIDCheck: true, + SkipExpiryCheck: true, + }) + v.lastRebuild = v.now() + } + newVer := v.verifier + v.verMu.Unlock() + + return newVer.Verify(ctx, rawToken) +} + +// isSignatureError reports whether err is a go-oidc signature verification +// failure (unknown kid, key mismatch, or stale inflight result). It does NOT +// match aud/iss/exp/nbf/sub claim failures, which must never trigger a retry. +// The sentinel string "failed to verify signature" is the exact prefix emitted +// by (*oidc.IDTokenVerifier).Verify when (*RemoteKeySet).VerifySignature +// returns an error (see go-oidc/v3 verify.go). +func isSignatureError(err error) bool { + return strings.Contains(err.Error(), "failed to verify signature") +} + // extractBearerToken pulls the JWT out of an `Authorization: Bearer ` // header. Returns ErrUnauthorized for any shape that isn't a non-empty // bearer token. diff --git a/internal/server/scheduledauth/validator_test.go b/internal/server/scheduledauth/validator_test.go index 65c0fa0ff..0ac64cdb1 100644 --- a/internal/server/scheduledauth/validator_test.go +++ b/internal/server/scheduledauth/validator_test.go @@ -12,6 +12,7 @@ import ( "log" "net/http" "net/http/httptest" + "runtime" "strings" "sync" "sync/atomic" @@ -399,10 +400,17 @@ func TestValidate_OIDC_AudienceListClaim(t *testing.T) { } func TestValidate_OIDC_KeyRotation_RefreshOnUnknownKid(t *testing.T) { - // 1. Server starts with key A. Token signed by A → validates. + // 1. Server starts with key A. Token signed by A validates. // 2. Provider rotates: server now publishes key B. Token signed by B // arrives with a previously-unseen kid. The validator MUST refresh // the JWKS and accept the new token. + // + // The verifyWithRotationRetry path in validateOIDC makes this + // deterministic: when the cached (or in-flight) JWKS lacks kid-B, the + // first Verify returns a signature error, the verifier is rebuilt with a + // fresh RemoteKeySet, and the retry fetches the rotated JWKS. No goroutine + // timing is relied upon - see also TestValidate_OIDC_KeyRotation_StaleInflight + // for a direct exercise of the stale-inflight path. keyA := newTestKey(t, "kid-A") keyB := newTestKey(t, "kid-B") srv := newJWKSServer(t, jwks(keyA)) @@ -416,27 +424,14 @@ func TestValidate_OIDC_KeyRotation_RefreshOnUnknownKid(t *testing.T) { t.Fatalf("kid A: %v", err) } - // Sign tokB before issuing the swap so that the RSA operation - // (CPU-bound, ~1 ms) gives the go-oidc cleanup goroutine time to - // set inflight=nil under the mutex. Without this, fast loopback - // HTTP on Linux CI completes the swap POST without a goroutine - // switch, leaving the stale inflight visible to the next Validate - // call and causing it to reuse the pre-rotation key set. - tokB := signToken(t, keyB, baseClaims(time.Now(), - testSchedulerSubject, - "https://api.example.com", - "https://accounts.example.com")) - // Swap the JWKS to publish kid B. // // Both the request build and the response status are checked: if the - // /swap handler 5xx's (or -- more subtly -- returns a non-200 because - // the body short-read), the JWKS would silently NOT update. The test - // would then fail later at "unknown kid" instead of pointing at the - // real cause. Surfacing the swap failure here keeps the diagnostic - // chain short. + // /swap handler 5xx's (or returns non-200 due to a body short-read), + // the JWKS would silently NOT update and the test would fail later at + // "unknown kid" instead of pointing at the real cause. jwksB := jwks(keyB) - swap, err := http.NewRequestWithContext(context.Background(), http.MethodPost, srv.URL+"/swap", strings.NewReader(string(jwksB))) + swap, err := http.NewRequestWithContext(context.Background(), http.MethodPost, srv.URL+"/swap", bytes.NewReader(jwksB)) if err != nil { t.Fatalf("build swap request: %v", err) } @@ -452,11 +447,238 @@ func TestValidate_OIDC_KeyRotation_RefreshOnUnknownKid(t *testing.T) { } resp.Body.Close() + tokB := signToken(t, keyB, baseClaims(time.Now(), + testSchedulerSubject, + "https://api.example.com", + "https://accounts.example.com")) if err := v.Validate(context.Background(), "Bearer "+tokB); err != nil { t.Fatalf("kid B (post-rotation): %v", err) } } +func TestValidate_OIDC_KeyRotation_StaleInflight(t *testing.T) { + // Directly exercise the stale-inflight race diagnosed in issue #1381. + // + // go-oidc's RemoteKeySet uses a single goroutine ("inflight") to fetch + // JWKS. The goroutine calls inflight.done(keys) to unblock callers, then + // must re-acquire the mutex to set inflight=nil. In the window between + // done() and inflight=nil, a new caller joins the same inflight and + // receives the pre-rotation keys, causing a spurious signature failure. + // + // Setup: a JWKS server that holds its first response (simulating the + // inflight window). While the first fetch is blocked, the JWKS rotates + // to key B. A second validation (tokB, signed with key B) joins the + // blocked inflight and will receive stale key-A keys. + // + // Pre-fix behavior: tokB fails with "failed to verify id token signature". + // Post-fix behavior: the retry rebuilds the RemoteKeySet (fresh, no stale + // inflight), fetches the rotated JWKS, and verifies tokB successfully. + keyA := newTestKey(t, "kid-A") + keyB := newTestKey(t, "kid-B") + + var ( + fetchCount atomic.Int64 + firstArrived = make(chan struct{}) + release = make(chan struct{}) + jwksA = jwks(keyA) + jwksB = jwks(keyB) + ) + + // The JWKS server is deterministic by request number: + // request 1: serves keyA JWKS (held until release) - simulates the + // inflight completing with pre-rotation keys + // request 2+: serves keyB JWKS - the post-rotation state that the + // retry must pick up + mux := http.NewServeMux() + mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { + n := fetchCount.Add(1) + var body []byte + if n == 1 { + close(firstArrived) // signal: first JWKS request is in progress + <-release // hold until the test releases (stale-inflight window) + body = jwksA + } else { + body = jwksB + } + w.Header().Set("Content-Type", "application/jwk-set+json") + _, _ = w.Write(body) + }) + ts := httptest.NewServer(mux) + t.Cleanup(ts.Close) + + v := newOIDCValidator(t, ts.URL) + + tokA := signToken(t, keyA, baseClaims(time.Now(), + testSchedulerSubject, "https://api.example.com", "https://accounts.example.com")) + tokB := signToken(t, keyB, baseClaims(time.Now(), + testSchedulerSubject, "https://api.example.com", "https://accounts.example.com")) + + // Start tokA validation in background; it triggers the first (held) JWKS fetch. + errA := make(chan error, 1) + go func() { errA <- v.Validate(context.Background(), "Bearer "+tokA) }() + + // Wait for the first JWKS fetch to start (inflight#1 is now active). + <-firstArrived + + // Start tokB validation while inflight#1 is blocked. go-oidc deduplicates + // concurrent fetches via single-flight, so tokB joins the same inflight + // and will receive key-A keys when the inflight is released. + // NOTE: the inflight join is best-effort (Gosched below, no hard sync + // hook into go-oidc internals) - this test exercises the retry only when + // the join wins; either way tokB must succeed. The error-string contract + // the retry depends on is pinned deterministically by + // TestIsSignatureError_MatchesGoOIDCContract. + errB := make(chan error, 1) + go func() { errB <- v.Validate(context.Background(), "Bearer "+tokB) }() + + // Yield the scheduler a few times to let the tokB goroutine progress + // into keysFromRemote and join the active inflight before we release it. + for i := 0; i < 20; i++ { + runtime.Gosched() + } + + // Release the first fetch. The inflight returns key-A keys. + // tokA verifies (kid-A present). tokB fails (kid-B absent in key-A set) + // and the validator's retry rebuilds the RemoteKeySet, fetches the + // rotated JWKS (request 2, returns key-B), and accepts tokB. + close(release) + + if err := <-errA; err != nil { + t.Errorf("tokA: expected success, got %v", err) + } + if err := <-errB; err != nil { + t.Errorf("tokB (stale-inflight retry): expected success, got %v", err) + } +} + +func TestVerifyRetry_RebuildRateLimited(t *testing.T) { + // Amplification-DoS guard: sequential bad-signature tokens must trigger + // at most ONE verifier rebuild per minRebuildInterval. Without the rate + // limit, every garbage token would rebuild the keyset and fire an extra + // outbound JWKS GET on the retry. + // + // Expected JWKS fetch budget with the limit in place (server always + // serves keyA; both tokens carry unknown kid-X): + // token 1: fetch#1 (lazy initial) -> sig fail -> rebuild -> fetch#2 + // (retry on fresh keyset) -> sig fail -> 401 + // token 2: cached keys present -> kid miss -> fetch#3 (go-oidc's own + // refresh-on-unknown-kid, unavoidable) -> sig fail -> + // rate-limited: NO rebuild, NO retry -> 401 + // Total: exactly 3 fetches. Without the rate limit token 2 would add a + // rebuild + retry fetch (4+). + keyA := newTestKey(t, "kid-A") + keyX := newTestKey(t, "kid-X") // never published + srv := newJWKSServer(t, jwks(keyA)) + v := newOIDCValidator(t, srv.URL) + + // Freeze the clock so the second token is deterministically within + // minRebuildInterval of the first rebuild. + t0 := time.Now() + v.now = func() time.Time { return t0 } + + for i := 1; i <= 2; i++ { + tok := signToken(t, keyX, baseClaims(t0, + testSchedulerSubject, "https://api.example.com", "https://accounts.example.com")) + if err := v.Validate(context.Background(), "Bearer "+tok); !errors.Is(err, ErrUnauthorized) { + t.Fatalf("bad token %d: expected ErrUnauthorized, got: %v", i, err) + } + } + + if hits := srv.hits.Load(); hits != 3 { + t.Fatalf("expected exactly 3 JWKS fetches (one rebuild) for 2 sequential bad tokens, got %d", hits) + } +} + +func TestVerifyRetry_RotationRecoversAfterInterval(t *testing.T) { + // The rebuild rate limit must not permanently wedge the validator: once + // minRebuildInterval elapses, a genuine rotation (still-stale cached + // keys) must trigger a fresh rebuild and succeed. + // + // The server simulates a slow-to-converge JWKS CDN: fetches 1-3 return + // the pre-rotation keyA document, fetch 4+ return the rotated keyB. + // Sequence (all fetches sequential, so counts are deterministic): + // t0: Validate(tokB) -> fetch#1 (A) fail -> rebuild#1 -> + // fetch#2 (A) fail -> 401; lastRebuild = t0 + // t0+11s: Validate(tokB) -> cached A, kid miss -> fetch#3 (A) fail -> + // interval elapsed -> rebuild#2 -> fetch#4 (B) -> success + keyA := newTestKey(t, "kid-A") + keyB := newTestKey(t, "kid-B") + jwksA := jwks(keyA) + jwksB := jwks(keyB) + + var fetchCount atomic.Int64 + mux := http.NewServeMux() + mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { + n := fetchCount.Add(1) + body := jwksA + if n >= 4 { + body = jwksB + } + w.Header().Set("Content-Type", "application/jwk-set+json") + _, _ = w.Write(body) + }) + ts := httptest.NewServer(mux) + t.Cleanup(ts.Close) + + v := newOIDCValidator(t, ts.URL) + + // Injectable clock (same pattern as the skew tests): start at t0, then + // jump past minRebuildInterval. No sleeping. + t0 := time.Now() + current := t0 + v.now = func() time.Time { return current } + + tokB := signToken(t, keyB, baseClaims(t0, + testSchedulerSubject, "https://api.example.com", "https://accounts.example.com")) + + if err := v.Validate(context.Background(), "Bearer "+tokB); !errors.Is(err, ErrUnauthorized) { + t.Fatalf("t0: expected ErrUnauthorized while JWKS is stale, got: %v", err) + } + + current = t0.Add(minRebuildInterval + time.Second) + if err := v.Validate(context.Background(), "Bearer "+tokB); err != nil { + t.Fatalf("post-interval: expected rotation to recover via rebuild, got: %v", err) + } + if hits := fetchCount.Load(); hits != 4 { + t.Fatalf("expected exactly 4 JWKS fetches (2 rebuilds, 1 interval apart), got %d", hits) + } +} + +func TestIsSignatureError_MatchesGoOIDCContract(t *testing.T) { + // Guard: the retry path keys off go-oidc's literal error string + // "failed to verify signature" (verify.go in go-oidc/v3). If a future + // go-oidc bump rewords it, this test fails loudly instead of the retry + // being silently disabled. It drives REAL tokens through the actual + // verifier rather than asserting against a hand-built error. + key := newTestKey(t, "kid-1") + imposter := newTestKey(t, "kid-1") // same kid, different key -> pure signature failure + srv := newJWKSServer(t, jwks(key)) + v := newOIDCValidator(t, srv.URL) + + badSig := signToken(t, imposter, baseClaims(time.Now(), + testSchedulerSubject, "https://api.example.com", "https://accounts.example.com")) + _, err := v.verifier.Verify(context.Background(), badSig) + if err == nil { + t.Fatalf("expected signature verification to fail") + } + if !isSignatureError(err) { + t.Fatalf("isSignatureError = false for a real go-oidc signature failure: %v\n"+ + "go-oidc likely reworded its error string - update isSignatureError to match", err) + } + + // Negative: a claim failure (wrong issuer) must NOT be classified as a + // signature error, otherwise iss failures would start triggering retries. + wrongIss := signToken(t, key, baseClaims(time.Now(), + testSchedulerSubject, "https://api.example.com", "https://attacker-iss.example.com")) + _, err = v.verifier.Verify(context.Background(), wrongIss) + if err == nil { + t.Fatalf("expected issuer verification to fail") + } + if isSignatureError(err) { + t.Fatalf("isSignatureError = true for an issuer failure; retry must not fire on claim errors: %v", err) + } +} + func TestValidate_OIDC_SingleFlight_StampedeProtection(t *testing.T) { // 50 concurrent verifications of fresh tokens after a cold start // should result in at most a small handful of JWKS fetches (the