[IAmJSD]

Root cause

go-oidc v3.18.0's RemoteKeySet.verify refetches the JWKS every time a token fails to verify against its cached keys, and it has no cooldown. cli#1386 bounded our own verifier rebuilds to one per 10s, but each bad token still cost one JWKS fetch inside go-oidc. A spammer could proxy requests 1:1 to the provider's JWKS endpoint by repeating an unknown kid, or by reusing a published kid with a forged signature.

Fix

internal/server/scheduledauth/kidguard.go wraps the go-oidc RemoteKeySet in a guardedKeySet:

• Negative cache. A jwksObserver RoundTripper sits on top of the existing hardened pkg/httpclient transport. It does not replace it. The observer numbers each JWKS fetch as it starts and records the key set from every successful response. If go-oidc rejects a token, and a fetch that started after that token arrived still cannot verify it, the token's kid goes into the negative cache for negativeKidTTL. Later tokens with that kid are rejected without calling go-oidc, so they trigger no fetch.
• Fail closed. A cache hit only ever rejects a token.
• No poisoning of live keys. A cached kid never blocks a token that the latest fetched JWKS verifies. Forged tokens that reuse a published kid therefore cannot lock out genuine tokens with that kid. A rotated key is also accepted as soon as any refresh picks it up, even while its kid is still cached.
• Bounded rotation delay. negativeKidTTL is equal to the existing minRebuildInterval (10s), so a freshly rotated kid is never rejected for longer than the existing recovery bound. Rejection can only happen if the attacker sent that kid before it was published and no other refresh has happened since.
• What gets cached. Failed fetches cache nothing. Neither do fetches that were already in flight when the token arrived, which is the go-oidc stale-inflight case that the rebuild retry exists for.
• Bounded size. The cache holds at most 1024 entries, keyed by SHA-256 of the kid, so an attacker-sized kid costs 32 bytes. When the cache is full, expired entries are swept. If it is still full, the new kid is not stored and behaves as it did before this change (fail closed, one fetch).
• Concurrency. The observer uses atomics and a CAS so an older fetch never overwrites a newer snapshot. The cache uses a mutex. The guard state lives on the Validator, so it survives verifier rebuilds.

validator.go now builds both the initial verifier and the rebuilt verifier through one newVerifier() helper. The unused keySet field is removed.

Closes #75

Verification

All of this is local unit/fixture evidence against an httptest JWKS server. No real JWKS endpoint was called. Tests ran with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null because this machine's default AWS profile is invalid. That has nothing to do with this package.

Fail before. I ran the new tests against main's validator.go, without kidguard.go:

--- FAIL: TestValidate_OIDC_RepeatedUnknownKidSkipsRefetch
    20 tokens with the same unknown kid made 21 JWKS fetches; want 1
--- FAIL: TestValidate_OIDC_ForgedPublishedKidDoesNotBlockLegit
    10 forged tokens with a published kid made 11 JWKS fetches; want 1

Pass after. On this branch, go test -race -count=5 ./internal/server/scheduledauth/ passes, and so does go test -count=1 ./internal/server/ ./internal/mocks/.

New tests in kidguard_test.go:

• Repeated unknown kid: one fetch for 20 tokens.
• Forged published kid: one fetch for 10 forged tokens, and legit tokens with that kid still pass.
• TTL expiry: the cached kid is rejected with no fetch at TTL−1s and accepted at TTL.
• Bypass once published: a cached kid that a later refresh published is accepted inside its TTL.
• Failed refresh: nothing is cached and each token still refetches.
• Concurrency under -race: 16 workers × 25 rounds of mixed valid and unknown-kid tokens. Valid tokens always pass and there are at most 6 fetches.
• Cache bound and expiry sweep.

Mutation probes. Each probe was applied to kidguard.go, the package tests were run, and the file was restored:

Mutation	Failing tests
Remove the "fetch started after arrival" check	FailedRefreshDoesNotCacheKid, the existing KeyRotation_StaleInflight
Remove the "latest JWKS verifies" bypass	NegativeKidBypassedOncePublished, ForgedPublishedKidDoesNotBlockLegit, IsSignatureError_MatchesGoOIDCContract
Entries never expire	NegativeKidExpiresAfterTTL, the existing RotationRecoversAfterInterval
No size cap	NegativeKidCache_BoundedAndExpiring
Guard disabled	RepeatedUnknownKid, ExpiresAfterTTL, ForgedPublishedKid, NegativeKidConcurrent, RebuildRateLimited

Existing test change. TestVerifyRetry_RebuildRateLimited asserted at least 2 JWKS fetches. It now asserts exactly 1: the rebuild retry and the second token are answered from the negative cache, so they no longer fetch. This is the intended effect, and the new assertion is stricter.

Static checks:

• go build ./... and go vet ./internal/server/... pass.
• golangci-lint v2.10.1 reports 0 issues on the package.
• gocyclo -over 10 is silent.
• gosec v2.28.0 with the hook's excludes reports 0 issues.
• git-secrets scan of the changed files, in an isolated temp repo, is clean.
• scripts/test-git-secrets-allowlist.sh reports 42 PASS.

Coverage gap. Nothing was exercised against Google's real JWKS endpoint or a real Cloud Scheduler token.

Out of scope / notes

• Unique kids. An attacker who sends a different made-up kid on every token still costs one fetch per token: 1:1 and non-amplifying, the same as before. A per-kid cache cannot stop that. Stopping it needs a global refresh cooldown, which would delay a genuine immediate rotation (the existing KeyRotation_RefreshOnUnknownKid test asserts that this does not happen), or per-source rate limiting at the edge. Left for a maintainer decision.
• Wasted rebuilds. A negative-cache hit still surfaces as a go-oidc signature error, so it still drives the existing rate-limited verifier rebuild, at most once per 10s. That rebuild discards go-oidc's cached keys, so the next legit token makes one lazy fetch. This is bounded, and I left it alone to keep scope tight.
• Dropping the wrapper later. If go-oidc adds its own refresh cooldown, bump go-oidc and drop this wrapper. The file header says so.
• File length. validator.go was already over the 500-line guideline (543 lines). It is now 550, and the new logic lives in its own file.
• Possible conflict. Open PR fix: six small scheduled-auth, CI, OpenAPI and IaC fixes #1127 also edits configureOIDC in validator.go (https-only JWKS URL). The hunks look separate, but whichever PR merges second may need a trivial rebase.

Maintainer: please mirror the issue's triage labels onto this PR: impact/internal, triaged, priority/p3, severity/low, urgency/this-quarter, effort/s, type/security.

🤖 Generated with Claude Code

[coderabbitai]

Note

Currently processing new changes in this PR. This may take a few minutes, please wait...

⚙️ Run configuration

• Configuration used: Repository: LeanerCloud/cloud-commitments-platform/.coderabbit.yaml
• Review profile: CHILL
• Plan: Advanced
• Run ID: ed674706-621f-4525-acbc-480432f525b4

📥 Commits

Reviewing files that changed from the base of the PR and between ba38120 and a7405d1.

📒 Files selected for processing: 4

• internal/server/scheduledauth/kidguard.go
• internal/server/scheduledauth/kidguard_test.go
• internal/server/scheduledauth/validator.go
• internal/server/scheduledauth/validator_test.go

+------------------------------------------+
| Duck and cover! I'm reviewing your code. |
+------------------------------------------+
  \
   \     /)/)
        ( '')
        /  |
      o(___)

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{Comment @coderabbitai help to get the list of available commands.}

[cristim]

VERDICT: APPROVE a7405d1

Reviewed: full diff origin/main...a7405d17 (1 commit, 4 files, +558/-18). Rebase check: git merge-tree --write-tree origin/main a7405d17 against origin/main f4387e4 is clean. The PR body flags a possible textual conflict with #1127 in configureOIDC.

Issue #75 asks for an optional negative-kid cache so that repeated unknown-kid tokens short-circuit without a JWKS fetch, while a genuine rotation still refreshes once, failing closed throughout. kidguard.go implements exactly that. The arrival-sequence check keeps go-oidc's stale-inflight case uncached. The latest-snapshot bypass keeps forged tokens on a published kid from locking out genuine ones. TTL equals minRebuildInterval, and the cache is capped at 1024 hashed entries.

Findings (none blocking):

• design internal/server/scheduledauth/kidguard.go:199 - the protection only helps when the attacker repeats a kid. An attacker who sends a new kid on every token still gets 1:1 JWKS proxying. The PR body states this. A maintainer should decide whether about 250 lines is worth it against that threat model or whether edge rate limiting is the real fix. The change matches the issue's proposed design, so this does not block.
• nit kidguard.go:106 - the CAS ordering guard (cur.seq >= seq) is untested. Replacing it with an unconditional store survives the whole suite. The race is narrow, and its failure mode is a stale snapshot that rejects a just-rotated cached kid until TTL. That is fail-closed and bounded.
• nit kidguard.go:230 - the kid-less token branch (kid != "") is untested. Making the kid comparison strict survives the suite. Google tokens always carry a kid.
• nit kidguard.go:220 - the verifiesWith term in rememberIfConfirmed is untested (the mutant survives). It is nearly equivalent, because latestVerifies at :199 would let such a token through anyway.

Attacked:

• Mutation P1 (remove the arrival check): fails FailedRefreshDoesNotCacheKid and KeyRotation_StaleInflight.
• Mutation P2 (remove the latest-verifies bypass): fails ForgedPublishedKidDoesNotBlockLegit, NegativeKidBypassedOncePublished and IsSignatureError_MatchesGoOIDCContract.
• Mutation P3 (entries never expire): fails NegativeKidExpiresAfterTTL and RotationRecoversAfterInterval.
• Mutations P4, P5 and P6 survive, as described in the nits.
• Transport. pkg/httpclient.New() sets Transport, so the observer's base is non-nil. The observer wraps that transport and does not replace it, so the IMDS and timeout hardening stays. The body read happens inside RoundTrip, which Client.Timeout still bounds. go-oidc already reads the body unbounded, so buffering it adds no new exposure.
• Rotation. An attacker who pre-sends a not-yet-published kid can delay that kid by at most negativeKidTTL. Once any refresh publishes it, the latest-snapshot bypass admits it.
• Error path. A cached hit surfaces as "failed to verify signature", so it drives only the existing rate-limited rebuild, and the retry is answered from the cache with no fetch.
• Flakiness of the tightened TestVerifyRetry_RebuildRateLimited (hits == 1): stable over -race -count=10.

Local results (fresh git archive export): go build ./... ok; go vet ./internal/server/scheduledauth/ and go vet -tags integration ./internal/server/... ok; go test -race -count=10 ./internal/server/scheduledauth/ 650 passed; go test ./internal/server/ 569 passed. Nothing ran against Google's real JWKS endpoint.

[cristim]

Independent review 2 (claude-fleet, cold): APPROVE a7405d1

Findings (none blocking; a related note was shared privately with the maintainers per SECURITY.md):

• [non-blocking, test gap] internal/server/scheduledauth/kidguard.go:236 - Replacing the record CAS ordering guard (cur.seq >= seq) with false still passes every test. Without it, an older fetch that finishes late can overwrite a newer snapshot. The effect is in the safe direction (nothing cached, or a rotated kid held back for up to 10s), but a two-fetch out-of-order unit test on jwksObserver.record would pin it.
• [non-blocking, test gap] kidguard.go:212 - Dropping the status != 200 gate also passes, because the test's 503 body is not JSON. A non-200 response with a JSON body such as {"error":..} would unmarshal to an empty key set and cache kids from a failed fetch. The production code is correct; the test should serve a JSON error body.
• [non-blocking] kidguard.go:350 - The || verifiesWith(...) in rememberIfConfirmed changes no test result, and the cache-hit path's latestVerifies bypass already covers it. It only saves a map entry and could be deleted.

Against #75: go-oidc v3.18.0 verify calls keysFromRemote on every miss with no cooldown (checked in the module source). Wrapping the KeySet as the issue asks fixes the root cause. The residual 1:1 cost for unique kids is stated as out of scope.

Attacked:

• Cache behaviour:
  • A cache hit never refreshes the TTL (probe M2).
  • A hit can only reject, never accept, and acceptance still requires go-oidc's own verify.
  • Legitimate tokens bypass the cache through latestVerifies.
• Inflight timing: only a fetch whose RoundTrip begins after the token arrived counts as confirmation. A fetch that started earlier is never cached.
• Bounds and safety:
  • At most 1024 SHA-256 keys are held. The sweep runs only when the cache is full, after a network fetch.
  • Concurrency uses atomics plus a CAS on the snapshot and a mutex on the map.
  • httpclient.New returns a fresh client, so wrapping its Transport touches no shared state.
• Blast radius: the removed keySet field had no other users, and the PR merges cleanly with fix: six small scheduled-auth, CI, OpenAPI and IaC fixes #1127 on main.
• Mutation probes on kidguard.go, each restored and confirmed identical with cmp:
  • M1, seq<=arrival changed to <: FAILS.
  • M2, a cache hit refreshing the TTL: FAILS.
  • M3, the CAS ordering removed: passes (finding 1).
  • M4, the verifiesWith check in remember removed: passes (finding 3).
  • M5, the latestVerifies bypass removed: 3 tests FAIL.
  • M6, the TTL made inclusive: FAILS.
  • M7, non-200 responses recorded: passes (finding 2).

Verified locally, on a git archive export of HEAD with no cloud credentials:

• gofmt -l: clean. go build ./... and go vet ./internal/server/scheduledauth/: pass.
• go test -count=1 on scheduledauth, internal/server and internal/mocks: ok.
• Under the heavy lock, go test -race -count=10 ./internal/server/scheduledauth/: ok.
• On the merged tree (origin/main + PR), go test -race -count=3 on scheduledauth: ok.
• Fail-before, with the new tests on the base export:
  • RepeatedUnknownKid: 21 fetches, want 1.
  • ForgedPublishedKid: 11 fetches.
  • NegativeKidConcurrent: 15 fetches, limit 6.
  • ExpiresAfterTTL and RebuildRateLimited also FAIL.

Reviewed by an independent agent with no author context (second reviewer). Review only: nothing was pushed, and the GitHub review state is unchanged.

[cristim]

Local verification at a7405d17a26989c4319ffbbf3eb0da504e717b14 merged onto current main (56e226ac; merge-tree 89170139; macOS):

• go build ./..., go vet -tags integration ./... and go test -race -short ./... pass (full suite).
• Nothing was applied to any cloud.

[cristim]

Marked ready and merged by the owner's agent after 2 independent reviews and local verification (see the review and verification comments above, all at a7405d17a26989c4319ffbbf3eb0da504e717b14).