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} }