From 45dec5ab4b4734060d4d8175e1d78bffdba63514 Mon Sep 17 00:00:00 2001 From: Andy Bonventre Date: Thu, 9 Jul 2026 14:00:38 -0400 Subject: [PATCH 1/5] fix(credentialsource): bound token-exchange cache TTL and evict on upstream 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. --- AGENTS.md | 9 + CHANGELOG.md | 4 + credentialsource/tokenexchange.go | 106 +++++++-- credentialsource/tokenexchange_test.go | 290 +++++++++++++++++++++++ docs/content/guides/06-token-exchange.md | 13 +- docs/token-exchange-endpoint.md | 11 +- gatekeeper_tokenexchange.go | 4 + gatekeeper_tokenexchange_test.go | 106 +++++++++ proxy/intercept_test.go | 81 +++++++ proxy/proxy.go | 44 ++++ proxy/relay.go | 2 + 11 files changed, 643 insertions(+), 27 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index d2f6710..5c85650 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -102,6 +102,15 @@ go vet ./... go build -o gatekeeper ./cmd/gatekeeper/ ``` +## Testing + +Always work test-first. Write the test, run it and confirm it fails, then write the implementation and confirm it passes. This holds even for small bug fixes where the fix looks obvious — a test written after the fix can pass without ever having demonstrated the bug. The failing run is the evidence that the test is testing something. + +- **Confirm the test fails for the right reason.** Prefer a failure that demonstrates the defect over one that is merely a compile error. If asserting against a not-yet-existing constant would only produce `undefined: X`, pin the expected value in the test instead, so it compiles and fails on behavior. A red state of `cached TTL = 7h56m12s, want <= 1m0s` names the bug; `undefined: maxTokenTTL` does not. +- **If you wrote the fix first, back it out.** `git checkout -- `, write the test, watch it fail, then re-apply. +- **When a clean red is impossible** — adding a new struct field or method makes the compile error unavoidable — mutation-check afterward: neuter the implementation (make the new method a no-op), confirm the test fails, restore. This catches tests that pass vacuously. +- **Regression tests should encode the incident.** Name the real values from the bug report in the test, so the failure output reads like the original symptom. + ## Code Style - Follow standard Go conventions and `go fmt` formatting diff --git a/CHANGELOG.md b/CHANGELOG.md index df4c343..9bc9d53 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,10 @@ Gatekeeper is pre-1.0. The configuration schema and credential source interface ## Unreleased +### Fixed + +- **Token-exchange credentials no longer serve a rotated token until its advertised expiry** — `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 that was 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. Two changes bound this. First, cache TTL is now capped 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. Second, a `401` or `403` from the destination now drops the cache entry for that `(subject, actor)` so the next request exchanges afresh; the failed request is **not** retried (its body is already consumed, and requests like a git push are not idempotent). Because gatekeeper sees only a status code — a GitHub `403` covers "re-authorize the app", secondary rate limits, and plain permission denials alike — evictions are rate-limited to one per key per 10 seconds, so a client looping on a failing request cannot drive one STS exchange per request. An `Invalidate` hook on `proxy.CredentialHeader` carries this signal; it is nil for credentials with no cache behind them. Note that `expires_in` values above the cap no longer reduce STS request volume ([#39](https://github.com/majorcontext/gatekeeper/pull/39)) + ### Added - **`process` credential source** — run a host command (`sh -c`) and use its stdout as the credential value; any helper that prints a credential works (OS keychain CLIs, `pass`, 1Password's `op`, AWS `credential_process` helpers). Implements `RefreshingSource`: when the output is AWS `credential_process`-format JSON (exact-case `Version`/`AccessKeyId`/`Expiration` keys, so unrelated JSON can't hijack the schedule), the credential refreshes on the embedded `Expiration`, and already-expired output fails the fetch (engaging retry backoff) instead of installing credentials that would 401; other output reports a configurable `ttl` (default 5m; gatekeeper re-fetches at the standard 75%-of-TTL schedule). Header-invalid control characters are stripped from the output, with a warning (count only, never the value) when non-whitespace control bytes were present; stderr is included (truncated) in fetch errors for diagnosability, stdout never; the command string is config-owned and must not be accepted from untrusted config by embedding operators ([#38](https://github.com/majorcontext/gatekeeper/pull/38)) diff --git a/credentialsource/tokenexchange.go b/credentialsource/tokenexchange.go index 057fa23..a497b12 100644 --- a/credentialsource/tokenexchange.go +++ b/credentialsource/tokenexchange.go @@ -34,7 +34,8 @@ type TokenExchangeResponse struct { } // TokenExchangeSource exchanges a subject token for an access token via -// RFC 8693. It caches tokens per subject with TTL from the STS response. +// RFC 8693. It caches tokens per subject with TTL from the STS response, +// capped at maxTokenTTL. type TokenExchangeSource struct { endpoint string clientID string @@ -44,9 +45,19 @@ type TokenExchangeSource struct { actorTokenType string client *http.Client + // invalidateCooldown bounds how often a given key may be evicted by + // Invalidate. Overridden in tests. + invalidateCooldown time.Duration + mu sync.Mutex cache map[tokenCacheKey]cachedToken - sf singleflight.Group + // cacheGen increments on every Invalidate. A Resolve that captured an + // older generation must not write its result: the exchange may have been + // issued before the upstream credential rotated, so caching it would + // re-stale the entry the invalidation just cleared. + cacheGen uint64 + lastInvalidated map[tokenCacheKey]time.Time + sf singleflight.Group } type tokenCacheKey struct { @@ -70,14 +81,16 @@ func NewTokenExchangeSource(cfg TokenExchangeConfig) *TokenExchangeSource { actorTokenType = "urn:ietf:params:oauth:token-type:access_token" } return &TokenExchangeSource{ - endpoint: cfg.Endpoint, - clientID: cfg.ClientID, - clientSecret: cfg.ClientSecret, - resource: cfg.Resource, - subjectTokenType: subjectTokenType, - actorTokenType: actorTokenType, - client: &http.Client{Timeout: 30 * time.Second}, - cache: make(map[tokenCacheKey]cachedToken), + endpoint: cfg.Endpoint, + clientID: cfg.ClientID, + clientSecret: cfg.ClientSecret, + resource: cfg.Resource, + subjectTokenType: subjectTokenType, + actorTokenType: actorTokenType, + client: &http.Client{Timeout: 30 * time.Second}, + cache: make(map[tokenCacheKey]cachedToken), + lastInvalidated: make(map[tokenCacheKey]time.Time), + invalidateCooldown: defaultInvalidateCooldown, } } @@ -130,10 +143,62 @@ func (s *TokenExchangeSource) Exchange(ctx context.Context, subjectToken, actorT return &result, nil } -const defaultTokenTTL = 5 * time.Minute +// maxTokenTTL caps how long an exchanged token is cached, regardless of the +// expires_in the STS advertises. A long expires_in only means the token may +// live that long, not that it stays valid: the upstream credential behind the +// exchange can be revoked or rotated at any time, and gatekeeper has no way to +// learn of it. Capping bounds how long a stale credential keeps being injected +// after such a change. Resolve is singleflighted, so the extra STS calls are +// coalesced and cheap. +const maxTokenTTL = time.Minute + +// defaultInvalidateCooldown bounds how often one key may force a re-exchange. +// +// Invalidate's trigger is an upstream rejection, which gatekeeper can only see +// as a status code — a GitHub 403 means "re-authorize the app", but equally +// "secondary rate limit" or "no write access to this repo". A client looping on +// a request that always fails would otherwise drive one STS exchange per +// request, so evictions for a given key are rate-limited. The cost is bounded +// recovery latency: at worst one cooldown passes before a genuinely rotated +// credential is picked up. +const defaultInvalidateCooldown = 10 * time.Second + +// Invalidate drops the cached token for the given subject and actor, so the +// next Resolve performs a fresh exchange. Callers invoke it when the +// destination rejects an injected credential, which usually means the upstream +// credential behind the exchange was rotated or re-authorized and the cached +// token predates that change. +// +// Evictions are rate-limited per key (see defaultInvalidateCooldown); calls +// within the cooldown are no-ops. Invalidate is safe to call when no entry is +// cached — it still bars any in-flight exchange from caching a result that +// predates it. +func (s *TokenExchangeSource) Invalidate(subjectToken, actorToken string) { + ck := tokenCacheKey{subject: subjectToken, actor: actorToken} + + s.mu.Lock() + defer s.mu.Unlock() + + now := time.Now() + if last, ok := s.lastInvalidated[ck]; ok && now.Sub(last) < s.invalidateCooldown { + return + } + s.lastInvalidated[ck] = now + + delete(s.cache, ck) + s.cacheGen++ + + // Bound the bookkeeping map: entries past their cooldown carry no meaning. + for k, t := range s.lastInvalidated { + if now.Sub(t) >= s.invalidateCooldown { + delete(s.lastInvalidated, k) + } + } +} // Resolve returns a credential for the given subject, using the cache when -// possible. Concurrent requests for the same subject are coalesced into a +// possible. Cache entries live for the STS-advertised expires_in, capped at +// maxTokenTTL. Concurrent requests for the same subject are coalesced into a // single STS call via singleflight. When actorToken is non-empty, it is // forwarded to the STS as the RFC 8693 actor_token parameter and included // in the cache key. When requestID is non-empty, it is forwarded as @@ -156,6 +221,7 @@ func (s *TokenExchangeSource) Resolve(ctx context.Context, subjectToken, actorTo s.mu.Unlock() return cached.accessToken, nil } + gen := s.cacheGen s.mu.Unlock() // WithoutCancel strips both cancellation and deadline from the parent. @@ -171,8 +237,8 @@ func (s *TokenExchangeSource) Resolve(ctx context.Context, subjectToken, actorTo } ttl := time.Duration(result.ExpiresIn) * time.Second - if ttl <= 0 { - ttl = defaultTokenTTL + if ttl <= 0 || ttl > maxTokenTTL { + ttl = maxTokenTTL } s.mu.Lock() @@ -182,9 +248,15 @@ func (s *TokenExchangeSource) Resolve(ctx context.Context, subjectToken, actorTo delete(s.cache, k) } } - s.cache[ck] = cachedToken{ - accessToken: result.AccessToken, - expiresAt: now.Add(ttl), + // Only cache when no Invalidate ran while the exchange was in flight. + // Otherwise this token may predate the rotation that prompted the + // invalidation, and writing it would re-stale the entry that was just + // cleared. The caller still gets this token; only the caching is skipped. + if s.cacheGen == gen { + s.cache[ck] = cachedToken{ + accessToken: result.AccessToken, + expiresAt: now.Add(ttl), + } } s.mu.Unlock() diff --git a/credentialsource/tokenexchange_test.go b/credentialsource/tokenexchange_test.go index 11dce55..5923cb5 100644 --- a/credentialsource/tokenexchange_test.go +++ b/credentialsource/tokenexchange_test.go @@ -189,6 +189,296 @@ func TestTokenExchange_CacheExpiry(t *testing.T) { } } +// wantMaxTTL is the ceiling the token cache must respect. Asserted independently +// of the production constant so that raising the cap is a deliberate, visible change. +const wantMaxTTL = time.Minute + +// cachedTTL returns the remaining lifetime of the cache entry for subject/actor. +func cachedTTL(t *testing.T, src *TokenExchangeSource, subject, actor string) time.Duration { + t.Helper() + src.mu.Lock() + defer src.mu.Unlock() + entry, ok := src.cache[tokenCacheKey{subject: subject, actor: actor}] + if !ok { + t.Fatalf("no cache entry for subject %q actor %q", subject, actor) + } + return time.Until(entry.expiresAt) +} + +// The STS may advertise a very long expires_in — Neptune returns the remaining +// lifetime of the underlying GitHub token, which can be hours. That token can be +// revoked or rotated upstream at any moment, so caching it for the full window +// means gatekeeper keeps injecting a stale credential (and the destination keeps +// returning 403) long after the user has reconnected their account. +func TestTokenExchange_CacheTTLCappedBelowAdvertisedExpiry(t *testing.T) { + // Observed in production: expires_in of ~8 hours. + const advertisedExpiresIn = 28573 + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + json.NewEncoder(w).Encode(map[string]any{ + "access_token": "gho_stale", + "token_type": "Bearer", + "expires_in": advertisedExpiresIn, + }) + })) + defer srv.Close() + + src := NewTokenExchangeSource(TokenExchangeConfig{ + Endpoint: srv.URL, + ClientID: "gk", + ClientSecret: "secret", + }) + + if _, err := src.Resolve(context.Background(), "usr_abc", "", ""); err != nil { + t.Fatalf("Resolve: %v", err) + } + + // However long the STS claims, a rotated credential must not stay cached + // for more than the cap. + if ttl := cachedTTL(t, src, "usr_abc", ""); ttl > wantMaxTTL { + t.Errorf("cached TTL = %v, want <= %v (STS advertised %ds)", ttl, wantMaxTTL, advertisedExpiresIn) + } +} + +// A short expires_in must still be honored — the cap is a ceiling, not a floor. +func TestTokenExchange_CacheTTLHonorsShortExpiry(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + json.NewEncoder(w).Encode(map[string]any{ + "access_token": "gho_short", + "token_type": "Bearer", + "expires_in": 5, + }) + })) + defer srv.Close() + + src := NewTokenExchangeSource(TokenExchangeConfig{ + Endpoint: srv.URL, + ClientID: "gk", + ClientSecret: "secret", + }) + + if _, err := src.Resolve(context.Background(), "usr_abc", "", ""); err != nil { + t.Fatalf("Resolve: %v", err) + } + + if ttl := cachedTTL(t, src, "usr_abc", ""); ttl > 5*time.Second { + t.Errorf("cached TTL = %v, want <= 5s (the advertised expires_in)", ttl) + } +} + +// An STS that omits expires_in must not produce a zero-TTL (uncacheable) or +// unbounded entry. +func TestTokenExchange_CacheTTLMissingExpiry(t *testing.T) { + var callCount atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + callCount.Add(1) + w.Header().Set("Content-Type", "application/json") + json.NewEncoder(w).Encode(map[string]any{ + "access_token": "gho_no_expiry", + "token_type": "Bearer", + }) + })) + defer srv.Close() + + src := NewTokenExchangeSource(TokenExchangeConfig{ + Endpoint: srv.URL, + ClientID: "gk", + ClientSecret: "secret", + }) + + if _, err := src.Resolve(context.Background(), "usr_abc", "", ""); err != nil { + t.Fatalf("Resolve: %v", err) + } + if _, err := src.Resolve(context.Background(), "usr_abc", "", ""); err != nil { + t.Fatalf("second Resolve: %v", err) + } + + if n := callCount.Load(); n != 1 { + t.Errorf("STS calls = %d, want 1 (missing expires_in should still cache)", n) + } + if ttl := cachedTTL(t, src, "usr_abc", ""); ttl <= 0 || ttl > wantMaxTTL { + t.Errorf("cached TTL = %v, want in (0, %v]", ttl, wantMaxTTL) + } +} + +// rotatingSTS returns a server handing out token_v1, token_v2, ... one per call. +func rotatingSTS(t *testing.T, calls *atomic.Int32) *httptest.Server { + t.Helper() + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + n := calls.Add(1) + w.Header().Set("Content-Type", "application/json") + json.NewEncoder(w).Encode(map[string]any{ + "access_token": fmt.Sprintf("token_v%d", n), + "token_type": "Bearer", + "expires_in": 3600, + }) + })) + t.Cleanup(srv.Close) + return srv +} + +// When the destination rejects an injected credential, the cached token is the +// prime suspect: the upstream credential behind the exchange was rotated or +// re-authorized, and the cache is still serving the pre-rotation token. Dropping +// the entry lets the next request pick up the fresh one. +func TestTokenExchange_InvalidateForcesReExchange(t *testing.T) { + var calls atomic.Int32 + srv := rotatingSTS(t, &calls) + + src := NewTokenExchangeSource(TokenExchangeConfig{ + Endpoint: srv.URL, + ClientID: "gk", + ClientSecret: "secret", + }) + + token1, err := src.Resolve(context.Background(), "usr_abc", "ak", "") + if err != nil { + t.Fatalf("Resolve: %v", err) + } + if token1 != "token_v1" { + t.Fatalf("token1 = %q, want token_v1", token1) + } + + src.Invalidate("usr_abc", "ak") + + token2, err := src.Resolve(context.Background(), "usr_abc", "ak", "") + if err != nil { + t.Fatalf("Resolve after Invalidate: %v", err) + } + if token2 != "token_v2" { + t.Errorf("token2 = %q, want token_v2 (Invalidate should force re-exchange)", token2) + } + if n := calls.Load(); n != 2 { + t.Errorf("STS calls = %d, want 2", n) + } +} + +// Invalidate must only drop the entry it names — one caller's 403 must not +// evict every other subject's token. +func TestTokenExchange_InvalidateScopedToKey(t *testing.T) { + var calls atomic.Int32 + srv := rotatingSTS(t, &calls) + + src := NewTokenExchangeSource(TokenExchangeConfig{ + Endpoint: srv.URL, + ClientID: "gk", + ClientSecret: "secret", + }) + + if _, err := src.Resolve(context.Background(), "alice", "", ""); err != nil { + t.Fatalf("Resolve alice: %v", err) + } + bob1, err := src.Resolve(context.Background(), "bob", "", "") + if err != nil { + t.Fatalf("Resolve bob: %v", err) + } + + src.Invalidate("alice", "") + + bob2, err := src.Resolve(context.Background(), "bob", "", "") + if err != nil { + t.Fatalf("Resolve bob after invalidating alice: %v", err) + } + if bob2 != bob1 { + t.Errorf("bob's token = %q, want %q (unchanged — only alice was invalidated)", bob2, bob1) + } + if n := calls.Load(); n != 2 { + t.Errorf("STS calls = %d, want 2 (bob should still be cached)", n) + } +} + +// Gatekeeper cannot tell a "re-authorize" 403 from a rate-limit or +// permission-denied 403 — the request logger only records the status code. So a +// client looping on a URL that always 403s must not turn into one STS exchange +// per request. Invalidate is rate-limited per key. +func TestTokenExchange_InvalidateCooldown(t *testing.T) { + var calls atomic.Int32 + srv := rotatingSTS(t, &calls) + + src := NewTokenExchangeSource(TokenExchangeConfig{ + Endpoint: srv.URL, + ClientID: "gk", + ClientSecret: "secret", + }) + src.invalidateCooldown = time.Hour // no second eviction should get through + + if _, err := src.Resolve(context.Background(), "usr_abc", "", ""); err != nil { + t.Fatalf("Resolve: %v", err) + } + + // First 403: evicts, so the next request re-exchanges. + src.Invalidate("usr_abc", "") + if _, err := src.Resolve(context.Background(), "usr_abc", "", ""); err != nil { + t.Fatalf("Resolve: %v", err) + } + if n := calls.Load(); n != 2 { + t.Fatalf("STS calls = %d, want 2 after first Invalidate", n) + } + + // A burst of further 403s within the cooldown must not each force an exchange. + for range 20 { + src.Invalidate("usr_abc", "") + if _, err := src.Resolve(context.Background(), "usr_abc", "", ""); err != nil { + t.Fatalf("Resolve: %v", err) + } + } + if n := calls.Load(); n != 2 { + t.Errorf("STS calls = %d, want 2 (cooldown should suppress the burst)", n) + } +} + +// An Invalidate racing an in-flight Exchange must not be undone by that +// exchange writing its (pre-invalidation) result into the cache. +func TestTokenExchange_InvalidateDuringInflightResolve(t *testing.T) { + var calls atomic.Int32 + entered := make(chan struct{}) + release := make(chan struct{}) + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + n := calls.Add(1) + if n == 1 { + close(entered) + <-release + } + w.Header().Set("Content-Type", "application/json") + json.NewEncoder(w).Encode(map[string]any{ + "access_token": fmt.Sprintf("token_v%d", n), + "token_type": "Bearer", + "expires_in": 3600, + }) + })) + defer srv.Close() + + src := NewTokenExchangeSource(TokenExchangeConfig{ + Endpoint: srv.URL, + ClientID: "gk", + ClientSecret: "secret", + }) + + done := make(chan struct{}) + go func() { + defer close(done) + if _, err := src.Resolve(context.Background(), "usr_abc", "", ""); err != nil { + t.Errorf("Resolve: %v", err) + } + }() + + <-entered + src.Invalidate("usr_abc", "") // lands while the exchange is in flight + close(release) + <-done + + // The in-flight result predates the invalidation, so it must not be cached. + src.mu.Lock() + _, cached := src.cache[tokenCacheKey{subject: "usr_abc"}] + src.mu.Unlock() + if cached { + t.Error("in-flight exchange re-cached a token across an Invalidate") + } +} + func TestTokenExchange_STSError(t *testing.T) { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusBadRequest) diff --git a/docs/content/guides/06-token-exchange.md b/docs/content/guides/06-token-exchange.md index 7f69583..fcdc4dd 100644 --- a/docs/content/guides/06-token-exchange.md +++ b/docs/content/guides/06-token-exchange.md @@ -129,13 +129,16 @@ When `actor_token_from` is configured on any credential, gatekeeper requires all Gatekeeper caches tokens per `(subject_token, actor_token)` pair: -- If `expires_in` is returned by the STS, the token is cached until expiry. -- If `expires_in` is `0` or omitted, a default TTL of 5 minutes is applied. +- If `expires_in` is returned by the STS, the token is cached until expiry, **capped at 1 minute**. +- If `expires_in` is `0` or omitted, the cap is used. - Concurrent requests for the same subject are coalesced into a single STS call via singleflight. - Expired entries are evicted lazily on the next exchange. - There is no proactive refresh. When a cached token expires, the next request triggers a new exchange. +- When the destination rejects an injected credential with `401` or `403`, the cache entry is dropped so the next request exchanges afresh. The failed request is **not** retried. Evictions are rate-limited to one per key per 10 seconds. -For high-throughput scenarios, set `expires_in` to a reasonable TTL (e.g., `3600` for one hour) to avoid per-request STS calls. +The cap exists because a long `expires_in` only means the token *may* live that long, not that it stays valid. The upstream credential behind the exchange can be revoked, rotated, or re-authorized at any moment, and gatekeeper has no way to learn of it. Honoring a multi-hour `expires_in` meant a rotated credential kept being injected — and kept being rejected — for hours. + +A consequence: `expires_in` values above the cap no longer reduce STS request volume. Sizing the STS for roughly one exchange per subject per minute is the safe assumption. ## STS Endpoint Requirements @@ -168,7 +171,7 @@ grant_type=urn%3Aietf%3Aparams%3Aoauth%3Agrant-type%3Atoken-exchange&subject_tok | `access_token` | string | Yes | The token gatekeeper injects upstream | | `issued_token_type` | string | No | Token type URI of the issued token | | `token_type` | string | No | Informational; gatekeeper uses its own prefix config | -| `expires_in` | int | No | TTL in seconds. Defaults to 300 if omitted | +| `expires_in` | int | No | TTL in seconds, capped at 60. Defaults to the cap if omitted | `access_token` must be non-empty. Gatekeeper treats an empty value as an error. @@ -194,7 +197,7 @@ Gatekeeper treats any non-200 status as a failure and returns HTTP 502 to the cl - [ ] Read `resource` if present -- the target API - [ ] Look up or mint an access token for the given subject and resource - [ ] Return JSON with a non-empty `access_token` -- [ ] Set `expires_in` to enable caching +- [ ] Set `expires_in` to enable caching (values above 60s are capped) - [ ] Return non-200 for invalid/expired/unknown subjects - [ ] Handle concurrent requests (idempotency or internal locking) - [ ] *(Optional)* Validate `actor_token` against `subject_token` to prevent impersonation diff --git a/docs/token-exchange-endpoint.md b/docs/token-exchange-endpoint.md index 9c22afb..1af222f 100644 --- a/docs/token-exchange-endpoint.md +++ b/docs/token-exchange-endpoint.md @@ -64,7 +64,7 @@ A JSON response per [RFC 8693 §2.2.1](https://datatracker.ietf.org/doc/html/rfc | `access_token` | string | **Yes** | The token gatekeeper injects into the upstream request | | `issued_token_type` | string | No | Token type URI of the issued token | | `token_type` | string | No | How the token should be used (informational; gatekeeper uses its own prefix config) | -| `expires_in` | int | No | TTL in seconds. Gatekeeper caches the token per subject until expiry. If omitted, the token is not cached (re-exchanged on every request). | +| `expires_in` | int | No | TTL in seconds. Gatekeeper caches the token per subject until expiry, capped at 1 minute. If omitted, the cap is used. | **Important:** `access_token` must be non-empty. Gatekeeper treats an empty `access_token` as an error. @@ -87,11 +87,12 @@ Use standard OAuth error responses for debugging clarity: Gatekeeper caches tokens per `(subject_token, actor_token)` pair within each credential source instance: -- If `expires_in` is provided, the token is cached until expiry. No refresh is attempted — when the cache entry expires, the next request triggers a new exchange. -- If `expires_in` is `0` or omitted, a default TTL of 5 minutes is applied. +- If `expires_in` is provided, the token is cached until expiry, capped at 1 minute. No refresh is attempted — when the cache entry expires, the next request triggers a new exchange. +- If `expires_in` is `0` or omitted, the cap is used. - There is no proactive refresh or sliding window. Expired entries are replaced on the next request. +- A `401` or `403` from the destination drops the cache entry, so the next request exchanges afresh. Evictions are rate-limited to one per key per 10 seconds. -For high-throughput scenarios, set `expires_in` to a reasonable TTL (e.g., 3600 for one hour) to avoid per-request STS calls. +The cap bounds how long a rotated or revoked upstream credential keeps being injected. Because of it, an `expires_in` above 60 seconds does not reduce STS request volume — size the endpoint for roughly one exchange per subject per minute. ## Gatekeeper Configuration Reference @@ -213,7 +214,7 @@ By default, `actor_token_type` is `urn:ietf:params:oauth:token-type:access_token - [ ] Read `resource` if present — this identifies the target API the token will be used against - [ ] Look up or mint an access token for the given subject and resource - [ ] Return a JSON response with at minimum `access_token` (non-empty string) -- [ ] Set `expires_in` to enable client-side caching and reduce request volume +- [ ] Set `expires_in` to enable client-side caching (gatekeeper caps it at 60s) - [ ] Return non-200 with an error body for invalid/expired/unknown subjects - [ ] Handle concurrent requests for the same subject (idempotency or internal locking) - [ ] *(Optional)* Validate `actor_token` against `subject_token` to prevent impersonation (see [Preventing Subject Impersonation](#preventing-subject-impersonation)) diff --git a/gatekeeper_tokenexchange.go b/gatekeeper_tokenexchange.go index 013023e..af94fe8 100644 --- a/gatekeeper_tokenexchange.go +++ b/gatekeeper_tokenexchange.go @@ -77,6 +77,10 @@ func newTokenExchangeResolver(cfg tokenExchangeResolverConfig) proxy.CredentialR Name: header, Value: value, Grant: cfg.Grant, + // The destination rejecting this token means the credential behind + // the exchange was rotated or re-authorized; drop the cached copy so + // the next request exchanges afresh. The source rate-limits this. + Invalidate: func() { src.Invalidate(subject, actorToken) }, }}, nil } } diff --git a/gatekeeper_tokenexchange_test.go b/gatekeeper_tokenexchange_test.go index 524d0f0..54b3da4 100644 --- a/gatekeeper_tokenexchange_test.go +++ b/gatekeeper_tokenexchange_test.go @@ -58,6 +58,112 @@ func TestNewTokenExchangeResolver(t *testing.T) { } } +// Regression: a user's GitHub OAuth token went stale, so the STS handed +// gatekeeper an unauthorized token; gatekeeper cached it for the full +// expires_in (~8h, GitHub's remaining token lifetime). The user reconnected +// their GitHub account and the STS began returning a working token, but the +// proxy kept injecting the cached pre-reconnect one, and every push got a 403 +// from github.com on /info/refs until the process was restarted. +// +// Invoking the credential's Invalidate hook — as the proxy now does on a 401 or +// 403 from the destination — must drop the stale entry so the next request +// carries the reconnected token. +func TestNewTokenExchangeResolver_InvalidateOnAuthFailureRecovers(t *testing.T) { + var exchanges atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + n := exchanges.Add(1) + token := "gho_stale_pre_reconnect" + if n > 1 { + token = "gho_fresh_post_reconnect" + } + w.Header().Set("Content-Type", "application/json") + json.NewEncoder(w).Encode(map[string]any{ + "access_token": token, + "token_type": "Bearer", + "expires_in": 28573, // what Neptune's STS returned in the incident + }) + })) + defer srv.Close() + + // The production config from the incident report. + resolver := newTokenExchangeResolver(tokenExchangeResolverConfig{ + Endpoint: srv.URL, + ClientID: "gk", + ClientSecret: "secret", + SubjectFrom: "proxy-auth", + ActorTokenFrom: "proxy-auth-password", + Grant: "github-user", + Header: "Authorization", + Prefix: "x-access-token", + Format: "basic", + }) + + newReq := func() *http.Request { + req := httptest.NewRequest("GET", "https://github.com/meetneptune/web.git/info/refs", nil) + req.SetBasicAuth("usr_abc", "ak_run_token") + req.Header.Set("Proxy-Authorization", req.Header.Get("Authorization")) + req.Header.Del("Authorization") + return req + } + + req := newReq() + creds, err := resolver(context.Background(), req, req, "github.com") + if err != nil { + t.Fatalf("resolver: %v", err) + } + if len(creds) != 1 { + t.Fatalf("got %d creds, want 1", len(creds)) + } + if !strings.Contains(decodeBasic(t, creds[0].Value), "gho_stale_pre_reconnect") { + t.Fatalf("first credential = %q, want the stale token", creds[0].Value) + } + + // Without invalidation the stale token is served for the full ~8h TTL. + req = newReq() + again, err := resolver(context.Background(), req, req, "github.com") + if err != nil { + t.Fatalf("resolver: %v", err) + } + if again[0].Value != creds[0].Value { + t.Fatalf("expected the cached token on a repeat request") + } + if n := exchanges.Load(); n != 1 { + t.Fatalf("STS exchanges = %d, want 1 (second request should be cached)", n) + } + + // github.com answers /info/refs with 403; the proxy calls Invalidate. + if creds[0].Invalidate == nil { + t.Fatal("token-exchange credential has no Invalidate hook") + } + creds[0].Invalidate() + + req = newReq() + recovered, err := resolver(context.Background(), req, req, "github.com") + if err != nil { + t.Fatalf("resolver after invalidate: %v", err) + } + if got := decodeBasic(t, recovered[0].Value); !strings.Contains(got, "gho_fresh_post_reconnect") { + t.Errorf("credential after invalidate = %q, want the reconnected token", got) + } + if n := exchanges.Load(); n != 2 { + t.Errorf("STS exchanges = %d, want 2", n) + } +} + +// decodeBasic returns the decoded user:pass of a "Basic " header value. +func decodeBasic(t *testing.T, headerValue string) string { + t.Helper() + encoded, ok := strings.CutPrefix(headerValue, "Basic ") + if !ok { + t.Fatalf("header value %q is not Basic-encoded", headerValue) + } + decoded, err := base64.StdEncoding.DecodeString(encoded) + if err != nil { + t.Fatalf("decoding %q: %v", encoded, err) + } + return string(decoded) +} + func TestNewTokenExchangeResolver_NoSubjectHeader(t *testing.T) { resolver := newTokenExchangeResolver(tokenExchangeResolverConfig{ Endpoint: "http://unused", diff --git a/proxy/intercept_test.go b/proxy/intercept_test.go index 4017b17..b3f8c48 100644 --- a/proxy/intercept_test.go +++ b/proxy/intercept_test.go @@ -3,6 +3,7 @@ package proxy import ( "bufio" "bytes" + "context" "crypto/tls" "crypto/x509" "fmt" @@ -124,6 +125,86 @@ func TestIntercept_CredentialInjection(t *testing.T) { } } +// A destination that rejects an injected credential is the only signal +// gatekeeper gets that a cached token has gone stale (the upstream credential +// behind it was rotated or re-authorized). Without this, the proxy keeps +// injecting the dead token until its cache entry expires on its own. +func TestIntercept_UpstreamAuthFailureInvalidatesCredential(t *testing.T) { + tests := []struct { + name string + status int + wantInvalidated bool + }{ + {"unauthorized", http.StatusUnauthorized, true}, + {"forbidden", http.StatusForbidden, true}, + {"success", http.StatusOK, false}, + {"server error", http.StatusInternalServerError, false}, + {"not found", http.StatusNotFound, false}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + setup := newInterceptTestSetup(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(tc.status) + w.Write([]byte("body")) + })) + + var invalidated atomic.Int32 + setup.Proxy.SetCredentialResolver(setup.BackendHost, func(ctx context.Context, proxyReq, innerReq *http.Request, host string) ([]credentialHeader, error) { + return []credentialHeader{{ + Name: "Authorization", + Value: "Bearer stale-token", + Grant: "github-user", + Invalidate: func() { invalidated.Add(1) }, + }}, nil + }) + + // /info/refs is the first request of the git push protocol, and where + // the reported 403 surfaced. + resp, err := setup.Client.Get(setup.Backend.URL + "/meetneptune/web.git/info/refs") + if err != nil { + t.Fatalf("request: %v", err) + } + defer resp.Body.Close() + io.ReadAll(resp.Body) + + if resp.StatusCode != tc.status { + t.Fatalf("status = %d, want %d", resp.StatusCode, tc.status) + } + + got := invalidated.Load() + want := int32(0) + if tc.wantInvalidated { + want = 1 + } + if got != want { + t.Errorf("Invalidate called %d times, want %d (upstream returned %d)", got, want, tc.status) + } + }) + } +} + +// A credential with no Invalidate hook (a static header, say) must not crash +// the response path when the upstream rejects it. +func TestIntercept_UpstreamAuthFailureWithoutInvalidateHook(t *testing.T) { + setup := newInterceptTestSetup(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusForbidden) + })) + + setup.Proxy.SetCredentialWithGrant(setup.BackendHost, "Authorization", "Bearer static", "static-grant") + + resp, err := setup.Client.Get(setup.Backend.URL + "/x") + if err != nil { + t.Fatalf("request: %v", err) + } + defer resp.Body.Close() + io.ReadAll(resp.Body) + + if resp.StatusCode != http.StatusForbidden { + t.Errorf("status = %d, want 403", resp.StatusCode) + } +} + func TestIntercept_CredentialInjectionCanonicalLog(t *testing.T) { setup := newInterceptTestSetup(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Write([]byte("ok")) diff --git a/proxy/proxy.go b/proxy/proxy.go index 2a2c7eb..55f3e2d 100644 --- a/proxy/proxy.go +++ b/proxy/proxy.go @@ -516,6 +516,15 @@ type credentialHeader struct { Name string // Header name (e.g., "Authorization", "x-api-key") Value string // Header value (e.g., "Bearer token", "sk-ant-...") Grant string // Grant name for logging (e.g., "github", "anthropic") + + // Invalidate, when non-nil, drops whatever cached state produced Value so + // the next request re-resolves it. The proxy calls it when the destination + // rejects the credential (401/403), which usually means the credential was + // rotated or re-authorized upstream and the cached copy is stale. Sources + // are expected to rate-limit their own evictions: a 403 does not reliably + // distinguish a stale credential from an authorized-but-forbidden request. + // Nil for credentials with no cache behind them (e.g. static headers). + Invalidate func() } // extraHeader holds an additional header to inject for a host. @@ -1237,6 +1246,32 @@ func (p *Proxy) getCredentialsForRequest(ctxReq, innerReq *http.Request, host st return p.getCredentials(host), nil } +// invalidateCredentialsOnAuthFailure drops the cached state behind each +// injected credential when the destination rejected it. A 401 or 403 is the +// only signal gatekeeper gets that a credential resolved from a cache has gone +// stale — the upstream credential behind it was rotated or re-authorized while +// the cache entry was still live. Without this, the proxy keeps injecting the +// dead credential until the entry expires on its own, which can be hours. +// +// This is deliberately evict-only: the failed request is not retried. Its body +// has already been consumed by the time the response arrives, and the requests +// that surface this (a git push, say) are not idempotent. The next request +// re-resolves and succeeds. +// +// Statuses other than 401/403 are left alone; a 5xx says nothing about the +// credential. Sources rate-limit their own evictions, since a 403 also covers +// rate limits and genuinely-forbidden requests. +func invalidateCredentialsOnAuthFailure(creds []credentialHeader, statusCode int) { + if statusCode != http.StatusUnauthorized && statusCode != http.StatusForbidden { + return + } + for _, cred := range creds { + if cred.Invalidate != nil { + cred.Invalidate() + } + } +} + // getExtraHeadersForRequest returns extra headers for a host, checking // RunContextData first, then falling back to the proxy's own map. func (p *Proxy) getExtraHeadersForRequest(r *http.Request, host string) []extraHeader { @@ -1912,6 +1947,8 @@ func (p *Proxy) handleHTTP(w http.ResponseWriter, r *http.Request) { } defer resp.Body.Close() + invalidateCredentialsOnAuthFailure(creds, resp.StatusCode) + logData.StatusCode = resp.StatusCode logData.ResponseHeaders = resp.Header.Clone() logData.ResponseSize = resp.ContentLength @@ -2360,6 +2397,13 @@ func (p *Proxy) handleConnectWithInterception(w http.ResponseWriter, r *http.Req ModifyResponse: func(resp *http.Response) error { req := resp.Request + // The destination rejected us: if the credential came from a cache, + // drop it so the next request re-resolves rather than replaying a + // credential the destination has already refused. + if creds, ok := req.Context().Value(interceptCredsKey{}).([]credentialHeader); ok { + invalidateCredentialsOnAuthFailure(creds, resp.StatusCode) + } + // Track LLM policy denials for the canonical log line. var llmDenied bool var llmDenyReason string diff --git a/proxy/relay.go b/proxy/relay.go index c2140c5..85c1439 100644 --- a/proxy/relay.go +++ b/proxy/relay.go @@ -138,6 +138,8 @@ func (p *Proxy) handleRelay(w http.ResponseWriter, r *http.Request) { } defer resp.Body.Close() + invalidateCredentialsOnAuthFailure(creds, resp.StatusCode) + // Copy response headers for key, values := range resp.Header { for _, value := range values { From 5e1230e151de1d9928cba1a7bd0b27b88aeaa918 Mon Sep 17 00:00:00 2001 From: Andy Bonventre Date: Thu, 9 Jul 2026 14:02:01 -0400 Subject: [PATCH 2/5] docs(changelog): cut v0.15.0 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. --- CHANGELOG.md | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9bc9d53..c2ae835 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,16 +4,16 @@ Gatekeeper is a standalone credential-injecting TLS-intercepting proxy. It trans Gatekeeper is pre-1.0. The configuration schema and credential source interface may change between minor versions. -## Unreleased - -### Fixed - -- **Token-exchange credentials no longer serve a rotated token until its advertised expiry** — `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 that was 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. Two changes bound this. First, cache TTL is now capped 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. Second, a `401` or `403` from the destination now drops the cache entry for that `(subject, actor)` so the next request exchanges afresh; the failed request is **not** retried (its body is already consumed, and requests like a git push are not idempotent). Because gatekeeper sees only a status code — a GitHub `403` covers "re-authorize the app", secondary rate limits, and plain permission denials alike — evictions are rate-limited to one per key per 10 seconds, so a client looping on a failing request cannot drive one STS exchange per request. An `Invalidate` hook on `proxy.CredentialHeader` carries this signal; it is nil for credentials with no cache behind them. Note that `expires_in` values above the cap no longer reduce STS request volume ([#39](https://github.com/majorcontext/gatekeeper/pull/39)) +## v0.15.0 — 2026-07-09 ### Added - **`process` credential source** — run a host command (`sh -c`) and use its stdout as the credential value; any helper that prints a credential works (OS keychain CLIs, `pass`, 1Password's `op`, AWS `credential_process` helpers). Implements `RefreshingSource`: when the output is AWS `credential_process`-format JSON (exact-case `Version`/`AccessKeyId`/`Expiration` keys, so unrelated JSON can't hijack the schedule), the credential refreshes on the embedded `Expiration`, and already-expired output fails the fetch (engaging retry backoff) instead of installing credentials that would 401; other output reports a configurable `ttl` (default 5m; gatekeeper re-fetches at the standard 75%-of-TTL schedule). Header-invalid control characters are stripped from the output, with a warning (count only, never the value) when non-whitespace control bytes were present; stderr is included (truncated) in fetch errors for diagnosability, stdout never; the command string is config-owned and must not be accepted from untrusted config by embedding operators ([#38](https://github.com/majorcontext/gatekeeper/pull/38)) +### Fixed + +- **Token-exchange credentials no longer serve a rotated token until its advertised expiry** — `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 that was 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. Two changes bound this. First, cache TTL is now capped 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. Second, a `401` or `403` from the destination now drops the cache entry for that `(subject, actor)` so the next request exchanges afresh; the failed request is **not** retried (its body is already consumed, and requests like a git push are not idempotent). Because gatekeeper sees only a status code — a GitHub `403` covers "re-authorize the app", secondary rate limits, and plain permission denials alike — evictions are rate-limited to one per key per 10 seconds, so a client looping on a failing request cannot drive one STS exchange per request. An `Invalidate` hook on `proxy.CredentialHeader` carries this signal; it is nil for credentials with no cache behind them. Note that `expires_in` values above the cap no longer reduce STS request volume ([#39](https://github.com/majorcontext/gatekeeper/pull/39)) + ## v0.14.1 — 2026-06-23 ### Fixed From 3ede3f63ea27e1b75ba010a24a71ce7d6e8682b0 Mon Sep 17 00:00:00 2001 From: Andy Bonventre Date: Thu, 9 Jul 2026 14:08:49 -0400 Subject: [PATCH 3/5] fix(proxy): invalidate only the credential that was actually injected 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. --- CHANGELOG.md | 2 +- proxy/intercept_test.go | 77 +++++++++++++++++++++++++++++++++++++++++ proxy/proxy.go | 29 ++++++++++++---- proxy/relay.go | 2 +- 4 files changed, 102 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c2ae835..f843d36 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,7 +12,7 @@ Gatekeeper is pre-1.0. The configuration schema and credential source interface ### Fixed -- **Token-exchange credentials no longer serve a rotated token until its advertised expiry** — `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 that was 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. Two changes bound this. First, cache TTL is now capped 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. Second, a `401` or `403` from the destination now drops the cache entry for that `(subject, actor)` so the next request exchanges afresh; the failed request is **not** retried (its body is already consumed, and requests like a git push are not idempotent). Because gatekeeper sees only a status code — a GitHub `403` covers "re-authorize the app", secondary rate limits, and plain permission denials alike — evictions are rate-limited to one per key per 10 seconds, so a client looping on a failing request cannot drive one STS exchange per request. An `Invalidate` hook on `proxy.CredentialHeader` carries this signal; it is nil for credentials with no cache behind them. Note that `expires_in` values above the cap no longer reduce STS request volume ([#39](https://github.com/majorcontext/gatekeeper/pull/39)) +- **Token-exchange credentials no longer serve a rotated token until its advertised expiry** — `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 that was 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. Two changes bound this. First, cache TTL is now capped 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. Second, a `401` or `403` from the destination now drops the cache entry for that `(subject, actor)` so the next request exchanges afresh; the failed request is **not** retried (its body is already consumed, and requests like a git push are not idempotent). Because gatekeeper sees only a status code — a GitHub `403` covers "re-authorize the app", secondary rate limits, and plain permission denials alike — evictions are rate-limited to one per key per 10 seconds, so a client looping on a failing request cannot drive one STS exchange per request. An `Invalidate` hook on `proxy.CredentialHeader` carries this signal; it is nil for credentials with no cache behind them, and only the credential actually injected into the rejected request is evicted — when several credentials for a host share a header name, the ones that lost de-duplication keep their cache entries. Note that `expires_in` values above the cap no longer reduce STS request volume ([#39](https://github.com/majorcontext/gatekeeper/pull/39)) ## v0.14.1 — 2026-06-23 diff --git a/proxy/intercept_test.go b/proxy/intercept_test.go index b3f8c48..86d0a9f 100644 --- a/proxy/intercept_test.go +++ b/proxy/intercept_test.go @@ -184,6 +184,83 @@ func TestIntercept_UpstreamAuthFailureInvalidatesCredential(t *testing.T) { } } +// Only the credential actually placed on the wire may be invalidated. When two +// credentials for a host share a header name, injectCredentials picks one +// winner via its byHeader de-dup — evicting the loser's cache entry would drop +// a credential that had nothing to do with this request, and (because eviction +// is cooldown-gated per key) could suppress the loser's own legitimate eviction +// later. +func TestIntercept_AuthFailureInvalidatesOnlyInjectedCredential(t *testing.T) { + setup := newInterceptTestSetup(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusForbidden) + })) + + var winnerInvalidated, loserInvalidated atomic.Int32 + setup.Proxy.SetCredentialResolver(setup.BackendHost, func(ctx context.Context, proxyReq, innerReq *http.Request, host string) ([]credentialHeader, error) { + return []credentialHeader{ + // Same header name: the "claude" grant loses to the other. + {Name: "Authorization", Value: "Bearer claude", Grant: "claude", + Invalidate: func() { loserInvalidated.Add(1) }}, + {Name: "Authorization", Value: "Bearer github", Grant: "github-user", + Invalidate: func() { winnerInvalidated.Add(1) }}, + }, nil + }) + + resp, err := setup.Client.Get(setup.Backend.URL + "/info/refs") + if err != nil { + t.Fatalf("request: %v", err) + } + defer resp.Body.Close() + io.ReadAll(resp.Body) + + if got := winnerInvalidated.Load(); got != 1 { + t.Errorf("injected credential Invalidate called %d times, want 1", got) + } + if got := loserInvalidated.Load(); got != 0 { + t.Errorf("uninjected credential Invalidate called %d times, want 0", got) + } +} + +// When credentials use distinct header names and the client sends a placeholder +// selecting one of them, only the selected one is injected — and only it may be +// invalidated. +func TestIntercept_AuthFailureInvalidatesOnlyPlaceholderSelectedCredential(t *testing.T) { + setup := newInterceptTestSetup(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusUnauthorized) + })) + + var selected, unselected atomic.Int32 + setup.Proxy.SetCredentialResolver(setup.BackendHost, func(ctx context.Context, proxyReq, innerReq *http.Request, host string) ([]credentialHeader, error) { + return []credentialHeader{ + {Name: "Authorization", Value: "Bearer real", Grant: "a", + Invalidate: func() { selected.Add(1) }}, + {Name: "X-Api-Key", Value: "real-key", Grant: "b", + Invalidate: func() { unselected.Add(1) }}, + }, nil + }) + + req, err := http.NewRequest("GET", setup.Backend.URL+"/x", nil) + if err != nil { + t.Fatal(err) + } + // A placeholder for one credential selects it; the other is not injected. + req.Header.Set("Authorization", "placeholder") + + resp, err := setup.Client.Do(req) + if err != nil { + t.Fatalf("request: %v", err) + } + defer resp.Body.Close() + io.ReadAll(resp.Body) + + if got := selected.Load(); got != 1 { + t.Errorf("selected credential Invalidate called %d times, want 1", got) + } + if got := unselected.Load(); got != 0 { + t.Errorf("unselected credential Invalidate called %d times, want 0", got) + } +} + // A credential with no Invalidate hook (a static header, say) must not crash // the response path when the upstream rejects it. func TestIntercept_UpstreamAuthFailureWithoutInvalidateHook(t *testing.T) { diff --git a/proxy/proxy.go b/proxy/proxy.go index 55f3e2d..f1ae16d 100644 --- a/proxy/proxy.go +++ b/proxy/proxy.go @@ -1071,6 +1071,12 @@ func (p *Proxy) getCredentialResolver(host string) CredentialResolver { type credentialInjectionResult struct { InjectedHeaders map[string]bool // Lower-cased header names that were injected Grants []string // Grant names of injected credentials + // Injected holds the credentials actually placed on the request. This is + // narrower than InjectedHeaders: when several credentials share a header + // name, only the one that won de-duplication appears here. Consumers acting + // on a credential's identity (invalidation) must use this, not the header + // name set. + Injected []credentialHeader } // injectCredentials replaces credential headers in the request. For each @@ -1090,6 +1096,7 @@ func injectCredentials(req *http.Request, creds []credentialHeader, host, method injected := make(map[string]bool, len(creds)) var grants []string + var injectedCreds []credentialHeader // First pass: inject credentials where the client sent a matching // placeholder header. This lets the client choose which credential @@ -1098,6 +1105,7 @@ func injectCredentials(req *http.Request, creds []credentialHeader, host, method if req.Header.Get(c.Name) != "" { req.Header.Set(c.Name, c.Value) injected[strings.ToLower(c.Name)] = true + injectedCreds = append(injectedCreds, c) if c.Grant != "" { grants = append(grants, c.Grant) } @@ -1127,6 +1135,7 @@ func injectCredentials(req *http.Request, creds []credentialHeader, host, method for _, c := range byHeader { req.Header.Set(c.Name, c.Value) injected[strings.ToLower(c.Name)] = true + injectedCreds = append(injectedCreds, c) if c.Grant != "" { grants = append(grants, c.Grant) } @@ -1141,7 +1150,7 @@ func injectCredentials(req *http.Request, creds []credentialHeader, host, method } } - return credentialInjectionResult{InjectedHeaders: injected, Grants: grants} + return credentialInjectionResult{InjectedHeaders: injected, Grants: grants, Injected: injectedCreds} } // mergeExtraHeaders injects extra headers into a request. If the request @@ -1247,7 +1256,14 @@ func (p *Proxy) getCredentialsForRequest(ctxReq, innerReq *http.Request, host st } // invalidateCredentialsOnAuthFailure drops the cached state behind each -// injected credential when the destination rejected it. A 401 or 403 is the +// credential that was injected into the rejected request. Pass +// credentialInjectionResult.Injected, never the full candidate list for the +// host: only one credential wins when several share a header name, and evicting +// the losers would drop cache entries that had no part in this request — and, +// since sources rate-limit evictions per key, could suppress a loser's own +// legitimate eviction later. +// +// A 401 or 403 is the // only signal gatekeeper gets that a credential resolved from a cache has gone // stale — the upstream credential behind it was rotated or re-authorized while // the cache entry was still live. Without this, the proxy keeps injecting the @@ -1947,7 +1963,7 @@ func (p *Proxy) handleHTTP(w http.ResponseWriter, r *http.Request) { } defer resp.Body.Close() - invalidateCredentialsOnAuthFailure(creds, resp.StatusCode) + invalidateCredentialsOnAuthFailure(credResult.Injected, resp.StatusCode) logData.StatusCode = resp.StatusCode logData.ResponseHeaders = resp.Header.Clone() @@ -2399,9 +2415,10 @@ func (p *Proxy) handleConnectWithInterception(w http.ResponseWriter, r *http.Req // The destination rejected us: if the credential came from a cache, // drop it so the next request re-resolves rather than replaying a - // credential the destination has already refused. - if creds, ok := req.Context().Value(interceptCredsKey{}).([]credentialHeader); ok { - invalidateCredentialsOnAuthFailure(creds, resp.StatusCode) + // credential the destination has already refused. Rewrite stored the + // injection result, so this evicts only what was actually sent. + if cr, ok := req.Context().Value(interceptCredResultKey{}).(credentialInjectionResult); ok { + invalidateCredentialsOnAuthFailure(cr.Injected, resp.StatusCode) } // Track LLM policy denials for the canonical log line. diff --git a/proxy/relay.go b/proxy/relay.go index 85c1439..54bd434 100644 --- a/proxy/relay.go +++ b/proxy/relay.go @@ -138,7 +138,7 @@ func (p *Proxy) handleRelay(w http.ResponseWriter, r *http.Request) { } defer resp.Body.Close() - invalidateCredentialsOnAuthFailure(creds, resp.StatusCode) + invalidateCredentialsOnAuthFailure(credResult.Injected, resp.StatusCode) // Copy response headers for key, values := range resp.Header { From e0036a8af14ccb9300c718301c408a075d431aa8 Mon Sep 17 00:00:00 2001 From: Andy Bonventre Date: Thu, 9 Jul 2026 14:22:01 -0400 Subject: [PATCH 4/5] fix(proxy): record only the on-the-wire credential as injected 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. --- proxy/intercept_test.go | 47 +++++++++++++++++++++++++++++++++++++++++ proxy/proxy.go | 24 ++++++++++++++------- 2 files changed, 63 insertions(+), 8 deletions(-) diff --git a/proxy/intercept_test.go b/proxy/intercept_test.go index 86d0a9f..f834c7f 100644 --- a/proxy/intercept_test.go +++ b/proxy/intercept_test.go @@ -261,6 +261,53 @@ func TestIntercept_AuthFailureInvalidatesOnlyPlaceholderSelectedCredential(t *te } } +// Placeholder selection combined with a shared header name: injectCredentials' +// first pass re-reads the header after a prior iteration overwrote it, so every +// same-named credential passes its "client sent this header" check and the last +// one in slice order lands on the wire. Only that last writer — the credential +// the destination actually rejected — may be evicted. +// +// (The wire-selection and grant-logging consequences of that re-read are +// pre-existing and deliberately left alone here; see the injectCredentials +// follow-up. This pins the invalidation behavior only.) +func TestIntercept_AuthFailureInvalidatesOnlyWireCredentialWithPlaceholder(t *testing.T) { + setup := newInterceptTestSetup(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusForbidden) + })) + + var firstInvalidated, lastInvalidated atomic.Int32 + setup.Proxy.SetCredentialResolver(setup.BackendHost, func(ctx context.Context, proxyReq, innerReq *http.Request, host string) ([]credentialHeader, error) { + return []credentialHeader{ + {Name: "Authorization", Value: "Bearer claude", Grant: "claude", + Invalidate: func() { firstInvalidated.Add(1) }}, + {Name: "Authorization", Value: "Bearer github", Grant: "github-user", + Invalidate: func() { lastInvalidated.Add(1) }}, + }, nil + }) + + req, err := http.NewRequest("GET", setup.Backend.URL+"/info/refs", nil) + if err != nil { + t.Fatal(err) + } + req.Header.Set("Authorization", "placeholder") + + resp, err := setup.Client.Do(req) + if err != nil { + t.Fatalf("request: %v", err) + } + defer resp.Body.Close() + io.ReadAll(resp.Body) + + // "Bearer github" is what reached the destination, so it is the credential + // the 403 refers to. + if got := lastInvalidated.Load(); got != 1 { + t.Errorf("on-the-wire credential Invalidate called %d times, want 1", got) + } + if got := firstInvalidated.Load(); got != 0 { + t.Errorf("overwritten credential Invalidate called %d times, want 0", got) + } +} + // A credential with no Invalidate hook (a static header, say) must not crash // the response path when the upstream rejects it. func TestIntercept_UpstreamAuthFailureWithoutInvalidateHook(t *testing.T) { diff --git a/proxy/proxy.go b/proxy/proxy.go index f1ae16d..70da72f 100644 --- a/proxy/proxy.go +++ b/proxy/proxy.go @@ -1071,11 +1071,12 @@ func (p *Proxy) getCredentialResolver(host string) CredentialResolver { type credentialInjectionResult struct { InjectedHeaders map[string]bool // Lower-cased header names that were injected Grants []string // Grant names of injected credentials - // Injected holds the credentials actually placed on the request. This is - // narrower than InjectedHeaders: when several credentials share a header - // name, only the one that won de-duplication appears here. Consumers acting - // on a credential's identity (invalidation) must use this, not the header - // name set. + // Injected holds the credentials whose values are actually on the request, + // one per header name. This is narrower than InjectedHeaders: when several + // credentials share a header name, only the one whose value survived — the + // de-duplication winner, or the last writer when a client placeholder + // matched several — appears here. Consumers acting on a credential's + // identity (invalidation) must use this, not the header name set. Injected []credentialHeader } @@ -1096,7 +1097,10 @@ func injectCredentials(req *http.Request, creds []credentialHeader, host, method injected := make(map[string]bool, len(creds)) var grants []string - var injectedCreds []credentialHeader + // onWire tracks, per lower-cased header name, the credential whose value the + // request actually carries. Writing the same header twice leaves only the + // last writer on the wire, so only it may be treated as injected. + onWire := make(map[string]credentialHeader, len(creds)) // First pass: inject credentials where the client sent a matching // placeholder header. This lets the client choose which credential @@ -1105,7 +1109,7 @@ func injectCredentials(req *http.Request, creds []credentialHeader, host, method if req.Header.Get(c.Name) != "" { req.Header.Set(c.Name, c.Value) injected[strings.ToLower(c.Name)] = true - injectedCreds = append(injectedCreds, c) + onWire[strings.ToLower(c.Name)] = c if c.Grant != "" { grants = append(grants, c.Grant) } @@ -1135,7 +1139,7 @@ func injectCredentials(req *http.Request, creds []credentialHeader, host, method for _, c := range byHeader { req.Header.Set(c.Name, c.Value) injected[strings.ToLower(c.Name)] = true - injectedCreds = append(injectedCreds, c) + onWire[strings.ToLower(c.Name)] = c if c.Grant != "" { grants = append(grants, c.Grant) } @@ -1150,6 +1154,10 @@ func injectCredentials(req *http.Request, creds []credentialHeader, host, method } } + injectedCreds := make([]credentialHeader, 0, len(onWire)) + for _, c := range onWire { + injectedCreds = append(injectedCreds, c) + } return credentialInjectionResult{InjectedHeaders: injected, Grants: grants, Injected: injectedCreds} } From 26800682fad1a872c9a1c739920874c2d1a68e33 Mon Sep 17 00:00:00 2001 From: Andy Bonventre Date: Thu, 9 Jul 2026 14:22:57 -0400 Subject: [PATCH 5/5] docs(agents): require resolving review threads once addressed --- AGENTS.md | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/AGENTS.md b/AGENTS.md index 5c85650..97f36f9 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -144,3 +144,24 @@ This module (`github.com/majorcontext/gatekeeper`) was extracted from moat's `in - Use `gh pr create` with default flags only (no `--base`, `--head`, etc.) - If `gh pr create` fails, report the error to the operator immediately - Do not attempt to work around failures by adding flags or changing configuration + +## Responding to Review Feedback + +- **Resolve a review thread once its finding is addressed.** Reply explaining what changed (or why nothing did), then resolve it. A thread left open after the fix has landed reads as unaddressed. +- Resolving requires GraphQL — `gh pr review` cannot do it: + +```bash +# List threads and their IDs +gh api graphql -f query='{repository(owner:"majorcontext",name:"gatekeeper"){ + pullRequest(number:NN){reviewThreads(first:20){nodes{id isResolved}}}}}' + +# Reply to a thread, then resolve it +gh api graphql -f query='mutation($tid:ID!,$body:String!){ + addPullRequestReviewThreadReply(input:{pullRequestReviewThreadId:$tid,body:$body}){comment{url}}}' \ + -f tid=THREAD_ID -f body='...' +gh api graphql -f query='mutation($tid:ID!){ + resolveReviewThread(input:{threadId:$tid}){thread{isResolved}}}' -f tid=THREAD_ID +``` + +- Verify a reviewer's claim before acting on it, and say so if the suggested fix is wrong. A reviewer can correctly identify a bug while proposing a remedy that does not fix it. +- Do not silently expand a PR's scope to fix pre-existing bugs a review surfaces. Note them, and ask whether to fold them in or track separately.