fix(credentialsource): bound token-exchange cache TTL and evict on upstream 401/403#39
Conversation
…stream 401/403 TokenExchangeSource.Resolve cached each exchanged token for the full expires_in returned by the STS, with no invalidation path. When an STS reports the remaining lifetime of the underlying credential (e.g. a GitHub user-to-server token, ~8h), a credential revoked, rotated, or re-authorized upstream kept being injected for the rest of that window. Every request failed with a 403 from the destination, and the only remediation was restarting the process to flush the in-memory cache. Cap the cache TTL at 1 minute regardless of the advertised expires_in. A long expires_in only means the token may live that long, not that it stays valid, and Resolve is singleflighted so the extra exchanges coalesce. Additionally, drop the cache entry when the destination rejects the credential with 401 or 403, so the next request exchanges afresh. The failed request is not retried: its body is already consumed by the time the response arrives, and the requests that surface this (a git push) are not idempotent. Gatekeeper sees only a status code -- a GitHub 403 covers "re-authorize the app", secondary rate limits, and plain permission denials alike -- so evictions are rate-limited to one per key per 10 seconds, preventing a client looping on a failing request from driving one STS exchange per request. An Invalidate hook on proxy.CredentialHeader carries the signal from the response path back to the source; it is nil for credentials with no cache behind them. Resolve now guards its cache write with a generation counter, mirroring NeonResolver: without it, an exchange already in flight when the 403 lands would write its pre-rotation token back into the cache and undo the eviction. Note that expires_in values above the cap no longer reduce STS request volume.
📦 Previous Review (superseded by newer review)Claude finished @andybons's task in 3m 36s —— View job Code review completeTodo list
SummaryThis is a well-scoped, carefully reasoned fix. The core mechanism holds up:
Finding (posted inline on
|
The unreleased section carries the process credential source (a feature) alongside this fix, so the pre-1.0 bump from v0.14.1 is a minor one.
📦 Previous Review (superseded by newer review)Claude finished @andybons's task in 2m 55s —— View job Claude finished reviewing this PRTodo list
SummaryI read through the full diff ( Specific things I checked and verified correct:
One judgment call worth being aware of (not a defect, already called out in the PR description):
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
invalidateCredentialsOnAuthFailure iterated every candidate credential for the host, not just the one placed on the wire. injectCredentials can leave candidates unused -- when several share a header name only the de-dup winner is sent, and a client-sent placeholder selects one grant over another -- so a 401/403 caused by the injected credential evicted unrelated cache entries too. Because sources rate-limit evictions per key, that could also suppress a loser's own legitimate eviction later. Record the credentials actually injected in credentialInjectionResult and invalidate only those. Gating on InjectedHeaders would not suffice: that map is keyed by header name, so two credentials sharing a name both match it. The intercept path reads the injection result from the context key Rewrite already stores, rather than the raw candidate list. Reported by claude[bot] on #39.
📦 Previous Review (superseded by newer review)Claude finished @andybons's task in 3m 30s —— View job Re-review of PR #39 (after commit
|
injectCredentials' first pass tests req.Header.Get(c.Name) after earlier iterations may have already written that header. When two credentials share a header name and the client sends a placeholder, every one of them passes the check, so all were appended to Injected -- reintroducing over-invalidation through the placeholder branch rather than the auto-inject branch fixed in 3ede3f6. Track the last writer per header name instead of appending every match, so Injected names exactly the credentials whose values reached the destination. Those are the ones a 401/403 refers to. The re-read also lets the last credential in slice order win the wire regardless of which grant the client's placeholder meant to select, and over-reports Grants in the canonical log line. Both predate this branch and are left untouched here; tracked separately. Reported by claude[bot] on #39.
|
Claude finished @andybons's task in 3m 17s —— View job Review of PR #39Todo list
SummaryLatest commit The over-invalidation bug via the placeholder branch is fixed. The remaining piece — which credential wins the wire when a placeholder makes two same-header credentials match — is pre-existing, not introduced by this PR. I diffed Rest of the diff (re-reviewed in full against
No new findings. No inline comments to add. Not verified: |
|
Claude finished @andybons's task in 2m 5s —— View job Reviewing PR #39Todo list
Re-review after
|
1 participant
Summary
TokenExchangeSource.Resolvecached each exchanged token for the fullexpires_inreturned by the STS, with no invalidation path. When the STS reports the remaining lifetime of the underlying credential — Neptune's returns the GitHub user-to-server token's, ~8h — a credential that was revoked, rotated, or re-authorized upstream kept being injected for the rest of that window. Every push failed with a403from GitHub on/info/refs, and the only remediation was restarting the process to flush the in-memory cache.Two changes bound this.
Cap the cache TTL at 1 minute, regardless of the advertised
expires_in. A longexpires_inonly means the token may live that long, not that it stays valid.Resolveis singleflighted, so the extra exchanges coalesce.Evict on upstream rejection. A
401or403from the destination drops the cache entry for that(subject, actor), so the next request exchanges afresh. A newInvalidatehook onproxy.CredentialHeadercarries the signal from the response path back to the source; it's nil for credentials with no cache behind them (static headers). Wired into all three credential paths: intercepted CONNECT (ModifyResponse), plain HTTP, and the relay.Judgment calls worth reviewing
Evict, don't retry. Postgres can retry because
connectWithRetryreplays a handshake. Here the request body is already consumed bycaptureBodyby the time the response arrives, and a git push isn't idempotent. The push that hits the stale token still fails; the next one succeeds, instead of failing for eight hours.Evictions are rate-limited to one per key per 10 seconds. Gatekeeper's request logger only ever sees
status=403— GitHub's "re-authorize the GitHub App" text never reaches it. So a 403 is indistinguishable from a secondary rate limit or a push to a repo the user simply can't write. Without a cooldown, any client looping on a request that always 403s drives one STS exchange per request, and it's attacker-triggerable. The cost is bounded recovery latency: worst case one cooldown before a genuinely rotated credential is picked up.Generation-counter guard on the cache write, mirroring
NeonResolver. Without it, an exchange already in flight when the 403 lands writes its pre-rotation token straight back into the cache and undoes the eviction.Operational consequence
An
expires_inabove 60s no longer reduces STS request volume. Size the STS endpoint for roughly one exchange per subject per minute. This is the change most likely to surprise someone; docs call it out explicitly.Testing
Written test-first; each test was confirmed to fail before the fix landed. The TTL-cap red state read
cached TTL = 7h56m12s, want <= 1m0s— the reportedexpires_in: 28573, reproduced.credentialsource: TTL capped below advertised expiry, short expiry still honored (the cap is a ceiling, not a floor), missingexpires_inhandled,Invalidateforces re-exchange, is scoped to its own key, respects the cooldown, and survives a race against an in-flightResolve.proxy: table test over 200/401/403/404/500 asserting the hook fires on exactly 401 and 403, plus a nil-hook case.gatekeeper: end-to-end regression reproducing the incident, using the production config from the report. Mutation-checked by neuteringInvalidate— it fails withcredential after invalidate = "x-access-token:gho_stale_pre_reconnect", so it isn't passing vacuously.go vet ./...clean,go test -race ./...green.Also in this PR
docs/token-exchange-endpoint.mdanddocs/content/guides/06-token-exchange.mddescribed cache-until-expiry with a 5-minute default; one line was already wrong before this change. Both now state the cap, the eviction rule, and the STS-volume consequence.AGENTS.mdgains a Testing section recording the test-first workflow this repo expects.🤖 Generated with Claude Code