fix(scheduledauth): retry once on unknown-kid signature failure; deterministic rotation test (closes #1381) - #1386
Conversation
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 6 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughOIDC validation now retries once after signature failures caused by stale in-flight JWKS data. The validator synchronizes and rate-limits verifier rebuilds, while tests cover concurrent rotation, retry guards, interval recovery, and error classification. ChangesOIDC key-rotation retry
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Validator
participant IDTokenVerifier
participant RemoteKeySet
participant JWKS_Server
Validator->>IDTokenVerifier: Verify token
IDTokenVerifier->>RemoteKeySet: Fetch signing keys
RemoteKeySet->>JWKS_Server: Request JWKS
IDTokenVerifier-->>Validator: Return signature error
Validator->>RemoteKeySet: Rebuild key set
RemoteKeySet->>JWKS_Server: Fetch current JWKS
Validator->>IDTokenVerifier: Retry verification once
Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/server/scheduledauth/validator_test.go`:
- Around line 529-533: Make the tokB concurrency test deterministically
synchronize with the stale-inflight window instead of relying on the
runtime.Gosched loop. Update the tokB fetch path around keysFromRemote to expose
an observable entry signal, or use an existing rebuild/generation counter, wait
for that signal before releasing the blocked request, and assert the
rotation-retry path was exercised rather than the ordinary unknown-kid refresh
path.
In `@internal/server/scheduledauth/validator.go`:
- Around line 404-429: Update the retry logic around Verify and isSignatureError
so only the terminal stale-key/signature-mismatch error triggers RemoteKeySet
rebuilding; propagate JWKS fetch, cancellation, and other verification errors
without retrying. In the v.verifier rebuild path, retain the concurrent pointer
guard and add a cooldown or generation guard to prevent sequential invalid
tokens from repeatedly creating fresh key sets and fetching JWKS.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0fc43525-4e16-4974-9c4a-57bde1c2aa63
📒 Files selected for processing (2)
internal/server/scheduledauth/validator.gointernal/server/scheduledauth/validator_test.go
|
DoS fix landed (commit 176a772): Root cause: every bad-signature token entered the rebuild path, firing a fresh JWKS GET on the retry. An unauthenticated attacker could amplify garbage tokens into ~unbounded requests against the OIDC provider's JWKS endpoint. Fix: Fetch-count proof (
Rotation still recovers ( Error-string contract pinned ( Gate results: @coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== Current PR diff summary =='
git diff --stat HEAD^ HEAD || true
git diff -- internal/server/scheduledauth/validator.go internal/server/scheduledauth/validator_test.go || true
echo '== validator.go outline =='
ast-grep outline internal/server/scheduledauth/validator.go --items all --type function,struct,method --view expanded || true
echo '== validator_test.go rotation/rate-limit test locations =='
rg -n -C 5 'VerifyRetry|KeyRotation|RebuildRateLimited|RotationRecovers|IsSignatureError|minRebuildInterval|lastRebuild|verifyWithRotationRetry' \
internal/server/scheduledauth/validator.go internal/server/scheduledauth/validator_test.go || true
echo '== Relevant module dependency version =='
rg -n -C 2 'coreos/go-oidc|go-oidc' go.mod go.sum || trueLength of output: 24458 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== Validator construction and OIDC configuration =='
sed -n '95,185p' internal/server/scheduledauth/validator.go
echo '== Complete rotation retry implementation =='
sed -n '386,470p' internal/server/scheduledauth/validator.go
echo '== Complete stale-inflight and throttle tests =='
sed -n '459,715p' internal/server/scheduledauth/validator_test.go
echo '== Test JWKS helpers and server state =='
rg -n -C 8 'func newJWKSServer|type .*JWKS|hits|func newOIDCValidator|func signToken|func baseClaims' \
internal/server/scheduledauth/validator_test.go
echo '== Locally cached go-oidc RemoteKeySet implementation (if available) =='
modcache="$(go env GOMODCACHE 2>/dev/null || true)"
if [ -n "$modcache" ] && [ -d "$modcache" ]; then
fd -a 'remote_key_set\.go|remote.*key.*set.*\.go' "$modcache/github.com/coreos" 2>/dev/null | head -20 | while read -r f; do
echo "--- $f"
rg -n -C 6 'func \(.*RemoteKeySet.*\) VerifySignature|keysFromRemote|inflight|nextUpdate' "$f" || true
done
else
echo 'Go module cache unavailable.'
fiLength of output: 25860
Please make the test assert the intended path—e.g., instrument the JWKS handler/client so the test can prove no second fetch occurs before releasing request 1, then require the retry’s post-release fetch—or avoid claiming this regression test is deterministic. The rebuild rate-limit itself looks correctly fail-closed: within the interval it returns the original signature error before constructing a new verifier or retrying. 🐇🔍 |
176a772 to
1efb040
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Rebased onto main (post-#1364, Lint now green on main); should be fully green on this head. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
…rministic rotation test (closes #1381) go-oidc's RemoteKeySet caches an in-flight JWKS fetch via a single goroutine. The goroutine calls inflight.done(keys) to unblock callers, then must re-acquire the mutex to set inflight=nil. A second goroutine arriving in this window joins the stale inflight, receives pre-rotation keys, and returns "failed to verify id token signature" for a valid post-rotation token. In production this causes a transient 401 for a brief window during key rotation. The test relied on goroutine-scheduling timing (RSA-sign delay) to avoid the race, which failed under CI load (run 29515635173/904e9939e). Fix (fail-closed, two layers): 1. Validator resilience: verifyWithRotationRetry() detects "failed to verify signature" errors from go-oidc and rebuilds the RemoteKeySet from scratch (no cached inflight), then retries exactly once. Non-signature failures (aud/iss/exp/nbf/sub) are returned immediately without retry. Only one rebuild fires per rotation event: a pointer-equality check prevents duplicate rebuilds under concurrent load. 2. Test determinism: TestValidate_OIDC_KeyRotation_RefreshOnUnknownKid loses the goroutine-timing comment/ordering trick; the retry handles any scheduling outcome. New TestValidate_OIDC_KeyRotation_StaleInflight directly replicates the race: a controlled JWKS server holds its first response (keyA) while the JWKS rotates to keyB; tokB joins the stale inflight, fails, and the retry fetches keyB successfully. Gates: go build ./... (0), go vet (0), -race -count=10 (520/520), -run RefreshOnUnknownKid -race -count=50 (50/50), -run StaleInflight -race -count=50 (50/50), gocyclo -over 10 (0).
…ification DoS Sequential badly-signed tokens each triggered a keyset rebuild plus an extra outbound JWKS GET on the retry path, letting an unauthenticated attacker amplify garbage tokens into ~unbounded requests against the OIDC provider's JWKS endpoint. Fix: add `lastRebuild time.Time` guarded by verMu. Inside the rebuild block, if `time.Since(lastRebuild) < minRebuildInterval (10s)`, skip the rebuild and return the original signature error immediately (fail closed, no retry). After a successful rebuild, record lastRebuild = now(). The existing pointer-equality guard still deduplicates concurrent goroutines hitting the same rotation. 10s bounds attacker-driven JWKS fetches to ~6/min while genuine key rotations still recover within one interval (providers publish the new key before signing with it; go-oidc's own refresh-on-unknown-kid path is unaffected). Also remove the now-unused `v.keySet` struct field; the verifier holds its own internal reference and nothing else read the field. Tests added (injectable clock, no sleeps): - TestVerifyRetry_RebuildRateLimited: 2 sequential bad-sig tokens -> exactly 3 JWKS fetches (not 4+), both return ErrUnauthorized - TestVerifyRetry_RotationRecoversAfterInterval: advance injected clock past interval, genuine rotation still recovers via rebuild#2 - TestIsSignatureError_MatchesGoOIDCContract: drives real tokens through the verifier and asserts isSignatureError matches go-oidc v3.18.0's wording; fails CI loudly if a future bump rewords the sentinel string
1efb040 to
4ed5999
Compare
|
Rebased onto current main (head Conflicts resolved (2 files):
Lint-gate cleanup: main's Gate exit codes (final rebased head):
@coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== Diff for reviewed files =='
git diff --unified=80 HEAD^ HEAD -- \
internal/server/scheduledauth/validator.go \
internal/server/scheduledauth/validator_test.go || true
echo '== Validator outline =='
ast-grep outline internal/server/scheduledauth/validator.go \
--items all --type struct,function,method --view expanded || true
echo '== Test outline =='
ast-grep outline internal/server/scheduledauth/validator_test.go \
--items all --type function,struct --view expanded || true
echo '== Retry implementation and synchronization-related test code =='
rg -n -C 12 \
'verifyWithRotationRetry|isSignatureError|minRebuildInterval|lastRebuild|TestValidate_OIDC_KeyRotation_StaleInflight|runtime\.Gosched|inflight|release|fetch|hits|started|joined' \
internal/server/scheduledauth/validator.go \
internal/server/scheduledauth/validator_test.go || true
echo '== Relevant precise file ranges =='
sed -n '95,190p' internal/server/scheduledauth/validator.go
sed -n '380,480p' internal/server/scheduledauth/validator.go
sed -n '440,760p' internal/server/scheduledauth/validator_test.goLength of output: 122296
Consequently, if The rebase conflict resolution itself looks sound: the |
|
Merged to main (closes #1381): fixes both the flaky rotation test AND the underlying production race - go-oidc's stale-inflight window during JWKS key rotation caused spurious signature failures (transient 401s in prod, CI flake in test). Fix: retry-once on unknown-kid signature failure by rebuilding the verifier, guarded by a 10s rebuild rate-limit (fail-closed) so bad-token spam cannot amplify JWKS fetches (Fable-caught DoS). Deterministic StaleInflight regression test + error-string contract test pinning go-oidc v3.18.0. Residual 1-fetch-per-token (go-oidc spec) tracked in #1387. This retires the #1381 flake exception - main's Unit Tests are now deterministically green. |
Root cause
go-oidc's
RemoteKeySetuses a single goroutine ("inflight") to deduplicate concurrent JWKS fetches. The goroutine callsinflight.done(keys)to unblock all waiters, then must re-acquire an internal mutex to setinflight = nil. A second goroutine arriving in this window joins the stale inflight and receives pre-rotation keys, returning"failed to verify id token signature"for a valid post-rotation token.Production impact: transient 401 errors during key rotation until the next successful validation re-fetches JWKS.
Test impact:
TestValidate_OIDC_KeyRotation_RefreshOnUnknownKiddodged the race via goroutine-scheduling timing (RSA-sign delay between request and swap), which failed under CI load (run 29515635173 on commit 904e993).Fix (two layers)
1. Validator resilience (
validator.go)verifyWithRotationRetry()is called fromvalidateOIDCin place of a barev.verifier.Verify. When go-oidc returns an error containing"failed to verify signature"it rebuildsRemoteKeySetandIDTokenVerifierfrom scratch (no cached inflight), then retries exactly once.Fail-closed constraints enforced:
aud/iss/exp/nbf/subfailures return immediately without retry.v.verifier == verunder write lock) prevents concurrent goroutines from spawning duplicate rebuilds.2. Test determinism (
validator_test.go)TestValidate_OIDC_KeyRotation_RefreshOnUnknownKid- timing comment and goroutine-scheduling trick removed; retry handles any scheduler outcome.TestValidate_OIDC_KeyRotation_StaleInflight(new) - directly replicates the race using a controlled JWKS server: holds the first response (keyA) until after a second validation has started, forcing it to join the stale inflight. tokA verifies; tokB fails on keyA → retry rebuilds verifier → fetches keyB → succeeds. Fails on pre-fix code, passes on post-fix.Gate results
go build ./...go vet ./internal/server/scheduledauth/...-race -count=10(whole package, 52 tests)-run RefreshOnUnknownKid -race -count=50-run StaleInflight -race -count=50gocyclo -over 10on touched filesSummary by CodeRabbit