From a152b696e55a04d6b13616e51141b7c0be66f0d5 Mon Sep 17 00:00:00 2001 From: Andy Bonventre Date: Thu, 9 Jul 2026 14:52:08 -0400 Subject: [PATCH] fix(proxy): select one credential per header name, independent of order injectCredentials' placeholder pass tested req.Header.Get(c.Name) after earlier iterations had already written that header. When several credentials for a host shared a header name and the client sent a placeholder for it, every same-named credential passed the "client sent this header" check. Three consequences: - The last credential in config order silently won the wire, regardless of which grant the client's placeholder meant to select. - Grants recorded every same-named credential as injected, so the canonical log line named credentials that never left the proxy. - Injected, which drives cache invalidation on a 401/403, listed credentials the destination never saw. Sample the client's headers once, before any injection, and choose exactly one credential per header name. Both selection paths now break ties by grant rather than by position: auto-injection prefers a non-"claude" grant, and placeholder selection prefers "claude". The claude grant is Claude Code's OAuth flow, so it must not be injected transparently, but a client that explicitly sends the header is asking for exactly it -- which the old code could not reliably honor. TestIntercept_AuthFailureInvalidatesOnlyWireCredentialWithPlaceholder previously pinned the last-writer behavior; it now asserts the tie-break. Fixes #40. --- CHANGELOG.md | 6 + proxy/inject_credentials_test.go | 212 +++++++++++++++++++++++++++++++ proxy/intercept_test.go | 29 ++--- proxy/proxy.go | 157 +++++++++++++---------- 4 files changed, 321 insertions(+), 83 deletions(-) create mode 100644 proxy/inject_credentials_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index f843d36..7ac77e0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,12 @@ 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 + +- **A client placeholder no longer selects the wrong credential, and the canonical log line no longer over-reports grants** — `injectCredentials`' placeholder pass tested `req.Header.Get(c.Name)` *after* earlier iterations had already written that header, so when several credentials for a host shared a header name and the client sent a placeholder for it, every same-named credential passed the "client sent this" check. Three consequences: the last credential in config order silently won the wire regardless of which grant the placeholder meant to select; `Grants` recorded every same-named credential as injected, so audit logs named credentials that never left the proxy; and `Injected` (used for cache invalidation since v0.15.0) listed credentials the destination never saw. Client-sent headers are now sampled once, before any injection, and exactly one credential is chosen per header name by a tie-break that does not depend on config order. **Behavior change:** the two selection paths break the tie in opposite directions — auto-injection still prefers a non-`claude` grant, while placeholder selection now prefers `claude`, since the claude grant is Claude Code's OAuth flow and a client that explicitly sends the header is asking for exactly it. Previously a placeholder could never reliably select it ([#40](https://github.com/majorcontext/gatekeeper/issues/40)) + ## v0.15.0 — 2026-07-09 ### Added diff --git a/proxy/inject_credentials_test.go b/proxy/inject_credentials_test.go new file mode 100644 index 0000000..c32400c --- /dev/null +++ b/proxy/inject_credentials_test.go @@ -0,0 +1,212 @@ +package proxy + +import ( + "net/http" + "slices" + "sort" + "testing" +) + +// grantsOf returns the grants injectCredentials recorded, sorted for comparison. +func grantsOf(res credentialInjectionResult) []string { + g := slices.Clone(res.Grants) + sort.Strings(g) + return g +} + +// injectedGrantsOf returns the grants of the credentials actually on the wire. +func injectedGrantsOf(res credentialInjectionResult) []string { + var g []string + for _, c := range res.Injected { + g = append(g, c.Grant) + } + sort.Strings(g) + return g +} + +func newInjectReq(t *testing.T, clientHeaders map[string]string) *http.Request { + t.Helper() + req, err := http.NewRequest("GET", "https://example.test/x", nil) + if err != nil { + t.Fatal(err) + } + for k, v := range clientHeaders { + req.Header.Set(k, v) + } + return req +} + +// When several credentials share a header name and the client sends a +// placeholder for it, exactly one credential may be injected. The "claude" +// grant wins the placeholder path: it is OAuth, and per injectCredentials' +// contract it should only be injected when Claude Code explicitly asks for it +// by sending the header. +func TestInjectCredentials_PlaceholderSameHeaderPrefersClaude(t *testing.T) { + creds := []credentialHeader{ + {Name: "Authorization", Value: "Bearer other", Grant: "other"}, + {Name: "Authorization", Value: "Bearer claude", Grant: "claude"}, + } + req := newInjectReq(t, map[string]string{"Authorization": "placeholder"}) + + res := injectCredentials(req, creds, "example.test", "GET", "/x") + + if got := req.Header.Get("Authorization"); got != "Bearer claude" { + t.Errorf("wire Authorization = %q, want %q", got, "Bearer claude") + } + if got := grantsOf(res); !slices.Equal(got, []string{"claude"}) { + t.Errorf("Grants = %v, want [claude] (only the injected credential)", got) + } + if got := injectedGrantsOf(res); !slices.Equal(got, []string{"claude"}) { + t.Errorf("Injected = %v, want [claude]", got) + } +} + +// Slice order must not decide the winner. Same as above with the credentials +// reversed: claude still wins. +func TestInjectCredentials_PlaceholderWinnerIndependentOfOrder(t *testing.T) { + creds := []credentialHeader{ + {Name: "Authorization", Value: "Bearer claude", Grant: "claude"}, + {Name: "Authorization", Value: "Bearer other", Grant: "other"}, + } + req := newInjectReq(t, map[string]string{"Authorization": "placeholder"}) + + res := injectCredentials(req, creds, "example.test", "GET", "/x") + + if got := req.Header.Get("Authorization"); got != "Bearer claude" { + t.Errorf("wire Authorization = %q, want %q", got, "Bearer claude") + } + if got := grantsOf(res); !slices.Equal(got, []string{"claude"}) { + t.Errorf("Grants = %v, want [claude]", got) + } +} + +// The first pass must test what the *client* sent, not what an earlier +// iteration wrote. A credential whose header the client never sent must not be +// injected just because a same-named credential was injected before it. +func TestInjectCredentials_PlaceholderDoesNotSelectUnsentHeaders(t *testing.T) { + creds := []credentialHeader{ + {Name: "Authorization", Value: "Bearer auth", Grant: "auth"}, + {Name: "X-Api-Key", Value: "real-key", Grant: "apikey"}, + } + req := newInjectReq(t, map[string]string{"Authorization": "placeholder"}) + + res := injectCredentials(req, creds, "example.test", "GET", "/x") + + if got := req.Header.Get("Authorization"); got != "Bearer auth" { + t.Errorf("wire Authorization = %q, want %q", got, "Bearer auth") + } + if got := req.Header.Get("X-Api-Key"); got != "" { + t.Errorf("X-Api-Key = %q, want empty (client never sent it)", got) + } + if got := grantsOf(res); !slices.Equal(got, []string{"auth"}) { + t.Errorf("Grants = %v, want [auth]", got) + } + if res.InjectedHeaders["x-api-key"] { + t.Error("InjectedHeaders marks x-api-key injected, but it was not") + } +} + +// Unchanged behavior: with no placeholder, auto-injection prefers the +// non-claude grant on a shared header name. +func TestInjectCredentials_AutoInjectPrefersNonClaude(t *testing.T) { + creds := []credentialHeader{ + {Name: "Authorization", Value: "Bearer claude", Grant: "claude"}, + {Name: "Authorization", Value: "Bearer other", Grant: "other"}, + } + req := newInjectReq(t, nil) + + res := injectCredentials(req, creds, "example.test", "GET", "/x") + + if got := req.Header.Get("Authorization"); got != "Bearer other" { + t.Errorf("wire Authorization = %q, want %q", got, "Bearer other") + } + if got := grantsOf(res); !slices.Equal(got, []string{"other"}) { + t.Errorf("Grants = %v, want [other] (claude must not be reported)", got) + } + if got := injectedGrantsOf(res); !slices.Equal(got, []string{"other"}) { + t.Errorf("Injected = %v, want [other]", got) + } +} + +// Unchanged behavior: distinct header names are all auto-injected. +func TestInjectCredentials_AutoInjectDistinctHeaders(t *testing.T) { + creds := []credentialHeader{ + {Name: "Authorization", Value: "Bearer a", Grant: "a"}, + {Name: "X-Api-Key", Value: "k", Grant: "b"}, + } + req := newInjectReq(t, nil) + + res := injectCredentials(req, creds, "example.test", "GET", "/x") + + if got := req.Header.Get("Authorization"); got != "Bearer a" { + t.Errorf("Authorization = %q, want %q", got, "Bearer a") + } + if got := req.Header.Get("X-Api-Key"); got != "k" { + t.Errorf("X-Api-Key = %q, want %q", got, "k") + } + if got := grantsOf(res); !slices.Equal(got, []string{"a", "b"}) { + t.Errorf("Grants = %v, want [a b]", got) + } +} + +// A placeholder on one header selects that credential; credentials for other +// headers are not auto-injected in the same request (existing behavior: the +// auto pass only runs when no placeholder matched). +func TestInjectCredentials_PlaceholderSuppressesAutoInject(t *testing.T) { + creds := []credentialHeader{ + {Name: "Authorization", Value: "Bearer a", Grant: "a"}, + {Name: "X-Api-Key", Value: "k", Grant: "b"}, + } + req := newInjectReq(t, map[string]string{"X-Api-Key": "placeholder"}) + + res := injectCredentials(req, creds, "example.test", "GET", "/x") + + if got := req.Header.Get("X-Api-Key"); got != "k" { + t.Errorf("X-Api-Key = %q, want %q", got, "k") + } + if got := req.Header.Get("Authorization"); got != "" { + t.Errorf("Authorization = %q, want empty", got) + } + if got := grantsOf(res); !slices.Equal(got, []string{"b"}) { + t.Errorf("Grants = %v, want [b]", got) + } +} + +// Grants must never name a credential that did not reach the wire: the +// canonical log line is an audit record. +func TestInjectCredentials_GrantsNeverOverReport(t *testing.T) { + creds := []credentialHeader{ + {Name: "Authorization", Value: "Bearer claude", Grant: "claude"}, + {Name: "Authorization", Value: "Bearer other", Grant: "other"}, + {Name: "Authorization", Value: "Bearer third", Grant: "third"}, + } + + for _, tc := range []struct { + name string + clientHdrs map[string]string + wantGrants []string + }{ + {"placeholder", map[string]string{"Authorization": "ph"}, []string{"claude"}}, + {"auto", nil, []string{"other"}}, + } { + t.Run(tc.name, func(t *testing.T) { + req := newInjectReq(t, tc.clientHdrs) + res := injectCredentials(req, creds, "example.test", "GET", "/x") + + if got := grantsOf(res); !slices.Equal(got, tc.wantGrants) { + t.Errorf("Grants = %v, want %v", got, tc.wantGrants) + } + if len(res.Injected) != 1 { + t.Errorf("Injected has %d creds, want 1", len(res.Injected)) + } + }) + } +} + +func TestInjectCredentials_Empty(t *testing.T) { + req := newInjectReq(t, nil) + res := injectCredentials(req, nil, "example.test", "GET", "/x") + if len(res.Injected) != 0 || len(res.Grants) != 0 || len(res.InjectedHeaders) != 0 { + t.Errorf("empty creds should produce an empty result, got %+v", res) + } +} diff --git a/proxy/intercept_test.go b/proxy/intercept_test.go index f834c7f..28d5f86 100644 --- a/proxy/intercept_test.go +++ b/proxy/intercept_test.go @@ -261,27 +261,24 @@ 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.) +// Placeholder selection combined with a shared header name: exactly one +// credential reaches the destination, so exactly one may be evicted on a 403. +// The credential that lost the tie was never sent, and evicting it would drop a +// cache entry the destination never saw — and, because eviction is +// cooldown-gated per key, could suppress its own legitimate eviction later. 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 + var claudeInvalidated, loserInvalidated atomic.Int32 setup.Proxy.SetCredentialResolver(setup.BackendHost, func(ctx context.Context, proxyReq, innerReq *http.Request, host string) ([]credentialHeader, error) { return []credentialHeader{ + // A client-sent placeholder selects the claude grant. {Name: "Authorization", Value: "Bearer claude", Grant: "claude", - Invalidate: func() { firstInvalidated.Add(1) }}, + Invalidate: func() { claudeInvalidated.Add(1) }}, {Name: "Authorization", Value: "Bearer github", Grant: "github-user", - Invalidate: func() { lastInvalidated.Add(1) }}, + Invalidate: func() { loserInvalidated.Add(1) }}, }, nil }) @@ -298,13 +295,13 @@ func TestIntercept_AuthFailureInvalidatesOnlyWireCredentialWithPlaceholder(t *te defer resp.Body.Close() io.ReadAll(resp.Body) - // "Bearer github" is what reached the destination, so it is the credential + // "Bearer claude" is what reached the destination, so it is the credential // the 403 refers to. - if got := lastInvalidated.Load(); got != 1 { + if got := claudeInvalidated.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) + if got := loserInvalidated.Load(); got != 0 { + t.Errorf("credential that lost the tie Invalidate called %d times, want 0", got) } } diff --git a/proxy/proxy.go b/proxy/proxy.go index 70da72f..c335bb2 100644 --- a/proxy/proxy.go +++ b/proxy/proxy.go @@ -1073,91 +1073,114 @@ type credentialInjectionResult struct { 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. + // credentials share a header name, only the one that won the tie 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 -// credential, if the client already sent that header (e.g., a placeholder), -// the proxy replaces it with the real value. When no placeholder matches, -// credentials are injected unconditionally for transparent auth. If multiple -// credentials share the same header name and no placeholder matched, the -// "claude" grant is skipped in favor of the other — claude uses OAuth and -// should only be injected when Claude Code explicitly sends a placeholder. -// When credentials have different header names, all are auto-injected. -// Returns a credentialInjectionResult with the set of lower-cased header -// names that were injected and the grant names used. +// injectCredentials replaces credential headers in the request. +// +// A credential is selected in one of two ways: +// +// - Placeholder selection. If the client already sent a credential's header +// (any non-empty value), that credential is chosen and its real value +// replaces what the client sent. This lets a client pick which grant to use +// when several target the same host. +// - Auto-injection. If the client sent none of the credentials' headers, all +// of them are injected unconditionally, for transparent auth. +// +// Exactly one credential is injected per header name. When several share a +// header name the tie is broken by grant, and the two paths break it in +// opposite directions: auto-injection prefers a non-"claude" grant, while +// placeholder selection prefers "claude". The claude grant is Claude Code's +// OAuth flow, so it must not be injected transparently, but a client that +// explicitly sends the header is asking for exactly it. The winner does not +// depend on the order credentials appear in. +// +// Client-sent headers are sampled once, before anything is written. Testing the +// request as it is mutated would let a credential injected by an earlier +// iteration look like a placeholder the client sent, selecting several +// same-named credentials at once. +// +// Returns a credentialInjectionResult naming the headers injected, the +// credentials whose values reached the wire, and their grants. Grants and +// Injected describe only what was sent — never a credential that lost the tie. func injectCredentials(req *http.Request, creds []credentialHeader, host, method, path string) credentialInjectionResult { if len(creds) == 0 { return credentialInjectionResult{} } - 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 - // to use when multiple grants target the same host. + // Sample the client's headers before any injection mutates them. + clientSent := make(map[string]bool, len(creds)) for _, c := range creds { 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) - } - slog.Debug("credential injected", - "subsystem", "proxy", - "action", "inject", - "grant", c.Grant, - "host", host, - "header", c.Name, - "method", method, - "path", path) - } - } - - // If no placeholder matched, inject unconditionally for transparent auth. - // When multiple credentials share the same header name, prefer the - // non-"claude" grant — the claude grant is for Claude Code's OAuth flow - // and should only be injected when explicitly requested via placeholder. - if len(injected) == 0 { - byHeader := make(map[string]credentialHeader, len(creds)) - for _, c := range creds { + clientSent[strings.ToLower(c.Name)] = true + } + } + + // selectWinners maps each eligible header name to the index of the single + // credential that will be injected for it. Iterating creds in order makes + // the choice independent of map iteration order. + selectWinners := func(eligible func(headerKey string) bool, preferClaude bool) map[string]int { + winners := make(map[string]int, len(creds)) + for i, c := range creds { key := strings.ToLower(c.Name) - if existing, ok := byHeader[key]; !ok || existing.Grant == "claude" { - byHeader[key] = c + if !eligible(key) { + continue } - } - 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) + j, seen := winners[key] + if !seen { + winners[key] = i + continue + } + incumbent := creds[j].Grant == "claude" + challenger := c.Grant == "claude" + if preferClaude && challenger && !incumbent { + winners[key] = i + } else if !preferClaude && incumbent && !challenger { + winners[key] = i } - slog.Debug("credential auto-injected", - "subsystem", "proxy", - "action", "inject-auto", - "grant", c.Grant, - "host", host, - "header", c.Name, - "method", method, - "path", path) } + return winners } - injectedCreds := make([]credentialHeader, 0, len(onWire)) - for _, c := range onWire { + winners := selectWinners(func(key string) bool { return clientSent[key] }, true) + autoInjected := len(winners) == 0 + if autoInjected { + winners = selectWinners(func(string) bool { return true }, false) + } + + injected := make(map[string]bool, len(winners)) + grants := make([]string, 0, len(winners)) + injectedCreds := make([]credentialHeader, 0, len(winners)) + + msg, action := "credential injected", "inject" + if autoInjected { + msg, action = "credential auto-injected", "inject-auto" + } + + for i, c := range creds { + key := strings.ToLower(c.Name) + if w, ok := winners[key]; !ok || w != i { + continue + } + req.Header.Set(c.Name, c.Value) + injected[key] = true injectedCreds = append(injectedCreds, c) + if c.Grant != "" { + grants = append(grants, c.Grant) + } + slog.Debug(msg, + "subsystem", "proxy", + "action", action, + "grant", c.Grant, + "host", host, + "header", c.Name, + "method", method, + "path", path) } + return credentialInjectionResult{InjectedHeaders: injected, Grants: grants, Injected: injectedCreds} }