diff --git a/AGENTS.md b/AGENTS.md index d2f6710..97f36f9 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 @@ -135,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. diff --git a/CHANGELOG.md b/CHANGELOG.md index df4c343..f843d36 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,12 +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 +## 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, 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 ### Fixed 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..f834c7f 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,210 @@ 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) + } + }) + } +} + +// 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) + } +} + +// 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) { + 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..70da72f 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. @@ -1062,6 +1071,13 @@ 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 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 } // injectCredentials replaces credential headers in the request. For each @@ -1081,6 +1097,10 @@ func injectCredentials(req *http.Request, creds []credentialHeader, host, method injected := make(map[string]bool, len(creds)) var grants []string + // 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 @@ -1089,6 +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 + onWire[strings.ToLower(c.Name)] = c if c.Grant != "" { grants = append(grants, c.Grant) } @@ -1118,6 +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 + onWire[strings.ToLower(c.Name)] = c if c.Grant != "" { grants = append(grants, c.Grant) } @@ -1132,7 +1154,11 @@ func injectCredentials(req *http.Request, creds []credentialHeader, host, method } } - return credentialInjectionResult{InjectedHeaders: injected, Grants: grants} + injectedCreds := make([]credentialHeader, 0, len(onWire)) + for _, c := range onWire { + injectedCreds = append(injectedCreds, c) + } + return credentialInjectionResult{InjectedHeaders: injected, Grants: grants, Injected: injectedCreds} } // mergeExtraHeaders injects extra headers into a request. If the request @@ -1237,6 +1263,39 @@ func (p *Proxy) getCredentialsForRequest(ctxReq, innerReq *http.Request, host st return p.getCredentials(host), nil } +// invalidateCredentialsOnAuthFailure drops the cached state behind each +// 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 +// 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 +1971,8 @@ func (p *Proxy) handleHTTP(w http.ResponseWriter, r *http.Request) { } defer resp.Body.Close() + invalidateCredentialsOnAuthFailure(credResult.Injected, resp.StatusCode) + logData.StatusCode = resp.StatusCode logData.ResponseHeaders = resp.Header.Clone() logData.ResponseSize = resp.ContentLength @@ -2360,6 +2421,14 @@ 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. 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. var llmDenied bool var llmDenyReason string diff --git a/proxy/relay.go b/proxy/relay.go index c2140c5..54bd434 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(credResult.Injected, resp.StatusCode) + // Copy response headers for key, values := range resp.Header { for _, value := range values {