From 26ff1a8b5bc4344c9f2105145f57d22d7c54ceb9 Mon Sep 17 00:00:00 2001 From: Lukasz Raczylo Date: Sat, 22 Aug 2026 13:54:39 +0100 Subject: [PATCH 1/8] fix(mcp): bound federation tools/list fan-out and weight admission by backend count Fixes finding 1 from the 2026-08-22 security audit: mcpFederatedToolsList spawned one unbounded goroutine per allowed MCP server, each able to issue up to 4 sequential HTTP calls reading up to 10MiB per response, while checkAndCount charged the caller only 1 request regardless of how many backends the fan-out actually contacted. With the 11 production servers, one client tools/list call could drive up to 44 outbound requests and hold ~110MiB concurrently, all for the cost of a single request against the caller's own rate limit. - Bound the fan-out to mcpFederatedFanoutConcurrency (4) concurrent backends via a buffered-channel semaphore, the same shape limiter.spawnTokens already uses. - Add limiter.checkAndCountWeighted so a tools/list call charges len(allowedMCPServerNames) requests against the caller's requests-per-minute/day budget instead of undercounting the fan-out as one; every other route keeps weight 1 via checkAndCount's own unchanged wrapper. - Shrink the per-backend response cap from maxRequestBytes (10MiB, sized for a client's chat/embeddings body) to mcpBackendResponseMaxBytes (4MiB, sized for a JSON-RPC reply), bounding worst-case concurrent memory to 4 x 4MiB = 16MiB. --- limits.go | 31 ++++++- mcp_federation.go | 114 ++++++++++++++++++------ mcp_federation_test.go | 191 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 308 insertions(+), 28 deletions(-) diff --git a/limits.go b/limits.go index 61ae8c9..013b6b7 100644 --- a/limits.go +++ b/limits.go @@ -1081,14 +1081,39 @@ func buildBudgetProbes(scopes []limitScope, now time.Time) (probes []budgetProbe // call completes, one request may still push a scope over its budget — an // accepted trade-off, unchanged from before this round. func (l *limiter) checkAndCount(scopes []limitScope) *limitViolation { + return l.checkAndCountWeighted(scopes, 1) +} + +// checkAndCountWeighted is checkAndCount generalized to charge `weight` +// requests against every scope's requests-per-minute/requests-per-day +// counters instead of exactly one (security audit finding 1b, 2026-08-22): +// a route whose single incoming client request causes this gateway to +// issue `weight` outbound backend calls — today, only the federated MCP +// tools/list fan-out (mcp_federation.go's handleMCPFederated, weight = +// len(allowedMCPServerNames(grp))) — must charge its real cost, not +// undercount a fan-out as a single request. weight<=0 is treated as 1 +// (never zero-cost, never negative); checkAndCount(scopes) is exactly +// checkAndCountWeighted(scopes, 1), so every other call site's behavior is +// completely unchanged by this method existing. Semantics beyond the +// weighted delta are identical to checkAndCount's own doc comment in +// every other respect: one storeIncrAndGetMulti round trip, requests- +// per-minute/requests-per-day compared against the just-incremented +// (now weighted) count, token/cost budgets read and compared unweighted +// (a fan-out has no tokens or USD cost of its own — those probes come +// from buildBudgetProbes, untouched by weight), violation precedence +// unchanged. +func (l *limiter) checkAndCountWeighted(scopes []limitScope, weight int64) *limitViolation { + if weight <= 0 { + weight = 1 + } now := l.now() entries := make([]counterIncr, 0, len(scopes)*checkAndCountKeysPerScope) for _, sc := range scopes { entries = append(entries, - newCounterIncr(sc.kind, sc.id, metricReq, windowMin, now, 1, minWindowTTL), - newCounterIncr(sc.kind, sc.id, metricReq, windowDay, now, 1, dayWindowTTL), - newCounterIncr(sc.kind, sc.id, metricReq, windowHour, now, 1, hourWindowTTL), + newCounterIncr(sc.kind, sc.id, metricReq, windowMin, now, weight, minWindowTTL), + newCounterIncr(sc.kind, sc.id, metricReq, windowDay, now, weight, dayWindowTTL), + newCounterIncr(sc.kind, sc.id, metricReq, windowHour, now, weight, hourWindowTTL), ) } probes, reads := buildBudgetProbes(scopes, now) diff --git a/mcp_federation.go b/mcp_federation.go index d2386e7..d4ae414 100644 --- a/mcp_federation.go +++ b/mcp_federation.go @@ -66,6 +66,21 @@ const ( mcpSessionCloseTimeout = 5 * time.Second ) +// mcpBackendResponseMaxBytes caps a single backend MCP server's HTTP +// response body doBackendJSONRPC will read into memory — previously +// maxRequestBytes (10MiB, routes_unified.go), a budget sized for a +// CLIENT's own chat/embeddings request body, not a backend's JSON-RPC +// reply (security audit finding 1c, 2026-08-22). 4MiB matches this +// codebase's other "generous but bounded" read caps for a non-primary- +// request-body read (cache.go's maxAccountingTeeBytes is also 4MiB) — +// comfortably above any tools/list schema listing or tools/call result +// this gateway has actually seen live (typically low KB), while bounding +// mcpFederatedFanoutConcurrency backends' worst-case concurrent memory to +// 4 × 4MiB = 16MiB, against the unbounded ~110MiB (11 production servers +// × up to 10MiB each, all fired at once) the security audit measured +// before this fix. +const mcpBackendResponseMaxBytes = 4 << 20 + // jsonrpcRequest is one JSON-RPC 2.0 request/notification, decoded from // the client's POST body and re-encoded (with a synthetic id) for this // gateway's own outbound calls to a backend MCP server (mcpBackendCall). @@ -237,6 +252,24 @@ func allowedMCPServerNames(cfg *Config, grp *group) []string { // other metered route — with per-target attribution added separately // inside mcpFederatedToolsList/mcpFederatedToolsCall (their own doc // comments cover exactly what "attributed" means for each). +// +// The weight charged to checkAndCount is 1 for every method EXCEPT +// tools/list (security audit finding 1b, 2026-08-22): a tools/list call +// fans out to every server allowedMCPServerNames(grp) returns — up to the +// whole MCP catalog, concurrently (mcpFederatedToolsList's own doc +// comment) — so one incoming client request can cost this gateway many +// real outbound backend calls. Charging it as a single request against +// the caller's requests-per-minute/requests-per-day budget undercounts +// that real cost; weighting it by len(names) instead makes the budget +// reflect what the request actually does. This is computed and charged +// exactly once, right here, before dispatch — never doubled by anything +// mcpFederatedToolsList itself does (its own countTargetRequests call is a +// SEPARATE scope kind, "mcp"/target-id, entirely disjoint from the +// user/group/total scopes checkAndCountWeighted evaluates here; see +// countTargetRequests' own doc comment, limits.go). tools/call, by +// contrast, always resolves to exactly ONE backend (resolveFederatedTool), +// so it keeps the plain weight of 1 — checkAndCountWeighted(scopes, 1) is +// byte-for-byte checkAndCount's own existing behavior. func (g *Gateway) handleMCPFederated(w http.ResponseWriter, r *http.Request, u *user, grp *group) { if r.Header.Get("Upgrade") != "" { writeOAIError(w, http.StatusNotImplemented, "invalid_request_error", "websocket/upgrade passthrough not supported") @@ -259,7 +292,17 @@ func (g *Gateway) handleMCPFederated(w http.ResponseWriter, r *http.Request, u * } scopes := withTotalScope(buildLimitScopes(u, grp)) - if violation := g.limiter.checkAndCount(scopes); violation != nil { + + var toolsListNames []string + weight := int64(1) + if req.Method == "tools/list" { + toolsListNames = allowedMCPServerNames(g.cfg, grp) + if n := int64(len(toolsListNames)); n > weight { + weight = n + } + } + + if violation := g.limiter.checkAndCountWeighted(scopes, weight); violation != nil { writeLimitViolation(w, violation) return } @@ -270,7 +313,7 @@ func (g *Gateway) handleMCPFederated(w http.ResponseWriter, r *http.Request, u * case req.Method == "ping": writeJSONRPCResult(w, req.ID, map[string]any{}) case req.Method == "tools/list": - g.mcpFederatedToolsList(w, r, req, grp) + g.mcpFederatedToolsList(w, r, req, toolsListNames) case req.Method == "tools/call": g.mcpFederatedToolsCall(w, r, req, grp) case strings.HasPrefix(req.Method, "notifications/"): @@ -433,7 +476,7 @@ func (g *Gateway) doBackendJSONRPC(ctx context.Context, targetURL, method string status = httpResp.StatusCode respSessionID = httpResp.Header.Get(mcpSessionHeader) - respBody, err := io.ReadAll(io.LimitReader(httpResp.Body, maxRequestBytes)) + respBody, err := io.ReadAll(io.LimitReader(httpResp.Body, mcpBackendResponseMaxBytes)) if err != nil { return nil, status, respSessionID, err } @@ -632,43 +675,64 @@ func lastSSEDataLine(body []byte) []byte { return last } +// mcpFederatedFanoutConcurrency bounds how many of names' backend servers +// mcpFederatedToolsList's own fan-out below contacts at once — a +// buffered-channel semaphore, the same bounded-concurrency shape +// limiter.spawnTokens already uses for its own goroutine fan-out +// (limits.go), applied here to gate each goroutine's actual outbound HTTP +// work rather than its launch. Before this bound (security audit finding +// 1a, 2026-08-22), one incoming tools/list request spawned one goroutine +// per allowed server with NO cap at all: with the 11 production servers, +// one client call could hold up to 11 concurrent connections and up to +// 11× mcpBackendResponseMaxBytes of backend response bodies in memory at +// once, entirely outside this gateway's own admission control. 4 keeps +// every server eventually contacted — a goroutine blocks on the +// semaphore, it is never dropped or skipped — while capping the worst +// case to 4 concurrent backends' cost, not the whole catalog's. +const mcpFederatedFanoutConcurrency = 4 + // mcpFederatedToolsList implements the federated "tools/list": it fans -// out one mcpBackendCall per server allowedMCPServerNames(grp) permits, -// concurrently — each under its own toolsListBackendTimeout budget -// (context.WithTimeout off r.Context(), MF1) — then merges every -// reachable server's own tools into one result, each tool's name prefixed -// "_", the exact underscore-separator convention the -// old agentgateway this plugin replaces used, and the one pugbot's -// mcpclient and agentkit have already persisted into their own tool-id -// databases (e.g. "brave-search_brave_web_search" — verified live). A -// server that errors, times out, or returns an unparsable/invalid result -// is logged and skipped, not surfaced as a whole-call failure — UNLESS -// every single attempted server failed, in which case an empty tools list -// would misleadingly look like "this caller's group has no MCP access" — -// see the loud-failure branch below. +// out one mcpBackendCall per server in names (handleMCPFederated's own +// allowedMCPServerNames(grp) call, made once there so the SAME set that +// was weighed into checkAndCountWeighted is the set actually contacted), +// at most mcpFederatedFanoutConcurrency at a time — each under its own +// toolsListBackendTimeout budget (context.WithTimeout off r.Context(), +// MF1) — then merges every reachable server's own tools into one result, +// each tool's name prefixed "_", the exact +// underscore-separator convention the old agentgateway this plugin +// replaces used, and the one pugbot's mcpclient and agentkit have already +// persisted into their own tool-id databases (e.g. +// "brave-search_brave_web_search" — verified live). A server that errors, +// times out, or returns an unparsable/invalid result is logged +// and skipped, not surfaced as a whole-call failure — UNLESS every single +// attempted server failed, in which case an empty tools list would +// misleadingly look like "this caller's group has no MCP access" — see +// the loud-failure branch below. // // countTargetRequests (limits.go) attributes one request to EVERY server // ATTEMPTED here, in ONE batched call after wg.Wait() — "attempted", not // "reached" or "succeeded": a server this gateway dialed and got a -// response (or a timeout, or a connection refusal) from still had a real -// request sent to it and a real slot of this gateway's outbound capacity -// spent on it, which is what these counters exist to track. A single -// federated tools/list call can move several targets' own counters, not -// just one — unlike handleTargetProxy's always-exactly-one-target shape -// (mcp_a2a.go). -func (g *Gateway) mcpFederatedToolsList(w http.ResponseWriter, r *http.Request, req jsonrpcRequest, grp *group) { - names := allowedMCPServerNames(g.cfg, grp) - +// response (or a timeout, or a connection refusal) from still had +// a real request sent to it and a real slot of this gateway's outbound +// capacity spent on it, which is what these counters exist to track. A +// single federated tools/list call can move several targets' own +// counters, not just one — unlike handleTargetProxy's always-exactly- +// one-target shape (mcp_a2a.go). +func (g *Gateway) mcpFederatedToolsList(w http.ResponseWriter, r *http.Request, req jsonrpcRequest, names []string) { var ( mu sync.Mutex wg sync.WaitGroup merged = make([]mcpTool, 0, len(names)) failed []string ) + sem := make(chan struct{}, mcpFederatedFanoutConcurrency) for _, name := range names { wg.Add(1) go func(name string) { defer wg.Done() + sem <- struct{}{} + defer func() { <-sem }() + ctx, cancel := context.WithTimeout(r.Context(), toolsListBackendTimeout) defer cancel() targetURL := g.cfg.MCPServers[name].URL diff --git a/mcp_federation_test.go b/mcp_federation_test.go index 7ee55a2..065dd32 100644 --- a/mcp_federation_test.go +++ b/mcp_federation_test.go @@ -4,6 +4,7 @@ import ( "bytes" "context" "encoding/json" + "fmt" "io" "net/http" "net/http/httptest" @@ -1214,6 +1215,196 @@ func TestHandleMCPFederated_ToolsList_AllServersFail_ReturnsInternalError(t *tes } } +// --- security audit finding 1a: fan-out concurrency bound --- + +// TestHandleMCPFederated_ToolsList_FanoutBoundedConcurrency proves +// mcpFederatedFanoutConcurrency actually caps how many backend servers a +// single tools/list call contacts AT ONCE — not just that it eventually +// contacts all of them. numServers is comfortably above the cap so the +// first wave of goroutines through the semaphore must stall on it: the +// test blocks every mock server's handler until every one of them +// observes exactly mcpFederatedFanoutConcurrency requests in flight +// simultaneously, then releases them all. If the fan-out were unbounded +// (the pre-fix behavior), every server's handler would receive its +// request immediately and this test would deadlock waiting for +// concurrency to reach a ceiling nothing ever enforces — a real +// regression here hangs the test until it times out, it does not +// silently pass. +func TestHandleMCPFederated_ToolsList_FanoutBoundedConcurrency(t *testing.T) { + const numServers = mcpFederatedFanoutConcurrency + 4 + + var ( + current int32 + peak int32 + release = make(chan struct{}) + ) + cfg := CreateConfig() + cfg.Providers = map[string]*ProviderConfig{"openai": {Type: "openai", APIKey: "k"}} + cfg.MCPServers = make(map[string]*TargetConfig, numServers) + for i := 0; i < numServers; i++ { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + n := atomic.AddInt32(¤t, 1) + for { + old := atomic.LoadInt32(&peak) + if n <= old || atomic.CompareAndSwapInt32(&peak, old, n) { + break + } + } + <-release + atomic.AddInt32(¤t, -1) + + var req jsonrpcRequest + _ = json.NewDecoder(r.Body).Decode(&req) + result, _ := json.Marshal(mcpToolsListResult{}) + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(jsonrpcResponse{ID: req.ID, Result: result}) + })) + t.Cleanup(srv.Close) + cfg.MCPServers[fmt.Sprintf("srv%d", i)] = &TargetConfig{URL: srv.URL} + } + cfg.Groups = map[string]*GroupConfig{"default": {}} + cfg.Users = &UsersConfig{Inline: []*UserConfig{{Name: "alice", Group: "default", APIKey: "sk-alice"}}} + + h, err := New(context.Background(), http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}), cfg, "llmgw") + if err != nil { + t.Fatalf("New: %v", err) + } + + done := make(chan struct{}) + rec := httptest.NewRecorder() + go func() { + h.ServeHTTP(rec, newFederatedRequest(t, "sk-alice", jsonrpcRequest{JSONRPC: jsonrpcVersion, Method: "tools/list", ID: json.RawMessage("1")})) + close(done) + }() + + deadline := time.After(5 * time.Second) + for atomic.LoadInt32(¤t) < mcpFederatedFanoutConcurrency { + select { + case <-deadline: + t.Fatal("concurrency never reached mcpFederatedFanoutConcurrency — fan-out may be serialized instead of bounded") + case <-time.After(time.Millisecond): + } + } + // Give any wrongly-unbounded extra goroutines a chance to also reach + // the handler before we sample — if the bound were missing, ALL + // numServers requests would already be in flight by now. + time.Sleep(20 * time.Millisecond) + if got := atomic.LoadInt32(¤t); got > mcpFederatedFanoutConcurrency { + t.Fatalf("current in-flight backends = %d, want <= %d (fan-out is not bounded)", got, mcpFederatedFanoutConcurrency) + } + close(release) + + select { + case <-done: + case <-time.After(5 * time.Second): + t.Fatal("request never completed after releasing all backends") + } + + if peak != mcpFederatedFanoutConcurrency { + t.Errorf("peak concurrent backends = %d, want exactly %d", peak, mcpFederatedFanoutConcurrency) + } + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200, body=%s", rec.Code, rec.Body.String()) + } +} + +// --- security audit finding 1b: tools/list weighted by backend count --- + +// TestHandleMCPFederated_ToolsList_WeightedByBackendCount_RejectsWhenOverBudget +// proves a tools/list call charges checkAndCount len(allowedMCPServerNames) +// requests, not one: with 3 configured (and unrestricted) servers and a +// requests-per-minute budget of 2, the very FIRST tools/list call must be +// rejected outright — and, because checkAndCountWeighted runs and fails +// BEFORE the fan-out ever starts, none of the three servers may be +// contacted at all. +func TestHandleMCPFederated_ToolsList_WeightedByBackendCount_RejectsWhenOverBudget(t *testing.T) { + alpha := newMockJSONRPCServer(t, nil) + beta := newMockJSONRPCServer(t, nil) + gamma := newMockJSONRPCServer(t, nil) + cfg := newFederationTestConfig(alpha.srv.URL, beta.srv.URL, false) + cfg.MCPServers["gamma"] = &TargetConfig{URL: gamma.srv.URL} + cfg.Groups["default"].Limits = &LimitsConfig{RequestsPerMinute: 2} + + h, err := New(context.Background(), http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}), cfg, "llmgw") + if err != nil { + t.Fatalf("New: %v", err) + } + + rec := httptest.NewRecorder() + h.ServeHTTP(rec, newFederatedRequest(t, "sk-alice", jsonrpcRequest{JSONRPC: jsonrpcVersion, Method: "tools/list", ID: json.RawMessage("1")})) + if rec.Code != http.StatusTooManyRequests { + t.Fatalf("status = %d, want 429 (weight=3 > limit=2), body=%s", rec.Code, rec.Body.String()) + } + for name, srv := range map[string]*mockJSONRPCServer{"alpha": alpha, "beta": beta, "gamma": gamma} { + if srv.called.Load() != 0 { + t.Errorf("server %q was contacted %d times, want 0 (rejected before fan-out)", name, srv.called.Load()) + } + } +} + +// TestHandleMCPFederated_ToolsList_WeightedByBackendCount_AccumulatesAcrossCalls +// proves the weighted charge actually accumulates against the same +// requests-per-minute window: with 3 servers and a budget of 5, the first +// tools/list call (weight 3) succeeds, and the second (weight 3 more, +// total 6 > 5) is rejected. +func TestHandleMCPFederated_ToolsList_WeightedByBackendCount_AccumulatesAcrossCalls(t *testing.T) { + alpha := newMockJSONRPCServer(t, []mcpTool{{Name: "lookup"}}) + beta := newMockJSONRPCServer(t, []mcpTool{{Name: "search"}}) + gamma := newMockJSONRPCServer(t, []mcpTool{{Name: "fetch"}}) + cfg := newFederationTestConfig(alpha.srv.URL, beta.srv.URL, false) + cfg.MCPServers["gamma"] = &TargetConfig{URL: gamma.srv.URL} + cfg.Groups["default"].Limits = &LimitsConfig{RequestsPerMinute: 5} + + h, err := New(context.Background(), http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}), cfg, "llmgw") + if err != nil { + t.Fatalf("New: %v", err) + } + + rec1 := httptest.NewRecorder() + h.ServeHTTP(rec1, newFederatedRequest(t, "sk-alice", jsonrpcRequest{JSONRPC: jsonrpcVersion, Method: "tools/list", ID: json.RawMessage("1")})) + if rec1.Code != http.StatusOK { + t.Fatalf("first call status = %d, want 200, body=%s", rec1.Code, rec1.Body.String()) + } + + rec2 := httptest.NewRecorder() + h.ServeHTTP(rec2, newFederatedRequest(t, "sk-alice", jsonrpcRequest{JSONRPC: jsonrpcVersion, Method: "tools/list", ID: json.RawMessage("2")})) + if rec2.Code != http.StatusTooManyRequests { + t.Fatalf("second call status = %d, want 429 (3+3=6 > limit=5), body=%s", rec2.Code, rec2.Body.String()) + } +} + +// TestHandleMCPFederated_ToolsCall_WeightStaysOne_EvenWithManyServersConfigured +// proves tools/call — which only ever resolves to ONE backend — never +// inherits tools/list's per-backend weighting: with 3 servers configured +// and a requests-per-minute budget of 2, two sequential tools/call +// requests must both succeed (weight 1 each, total 2, at the limit), where +// the equivalent tools/list weighting (3 each) would already have +// rejected the first one. +func TestHandleMCPFederated_ToolsCall_WeightStaysOne_EvenWithManyServersConfigured(t *testing.T) { + alpha := newMockJSONRPCServer(t, nil) + beta := newMockJSONRPCServer(t, nil) + gamma := newMockJSONRPCServer(t, nil) + cfg := newFederationTestConfig(alpha.srv.URL, beta.srv.URL, false) + cfg.MCPServers["gamma"] = &TargetConfig{URL: gamma.srv.URL} + cfg.Groups["default"].Limits = &LimitsConfig{RequestsPerMinute: 2} + + h, err := New(context.Background(), http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}), cfg, "llmgw") + if err != nil { + t.Fatalf("New: %v", err) + } + + for i := 1; i <= 2; i++ { + rec := httptest.NewRecorder() + h.ServeHTTP(rec, newFederatedRequest(t, "sk-alice", jsonrpcRequest{ + JSONRPC: jsonrpcVersion, Method: "tools/call", ID: json.RawMessage(fmt.Sprintf("%d", i)), + Params: json.RawMessage(`{"name":"alpha_lookup"}`), + })) + if rec.Code != http.StatusOK { + t.Fatalf("call %d status = %d, want 200 (tools/call weight must stay 1), body=%s", i, rec.Code, rec.Body.String()) + } + } +} + // --- MF3: legacy HTTP+SSE transport exclusion --- func TestIsLegacySSETransportURL(t *testing.T) { From 8bc54d15b583549e9778fac4cb46a5db6d739dcc Mon Sep 17 00:00:00 2001 From: Lukasz Raczylo Date: Sat, 22 Aug 2026 13:55:26 +0100 Subject: [PATCH 2/8] fix(mcp): recover panics in tools/list fan-out goroutines Fixes finding 2 from the 2026-08-22 security audit: the per-server goroutines in mcpFederatedToolsList carried only defer wg.Done(), unlike every other off-request goroutine in this package (limiter.spawn, modelRegistry.refreshProvider/captureModelMetadata), which all guard against a panic crashing the whole shared Traefik process. This codebase has repeatedly hit interpreter-level panics (registry.go reflect.Set, cache.go CFG index-out-of-range, providers.go errors.As), so an unrecovered panic here is not hypothetical. Add the same defer-recover pattern used at the other three sites: a panicking backend now degrades to "that server failed" and is logged, exactly like an HTTP error from it, instead of taking down the process. --- mcp_federation.go | 23 +++++++++++- mcp_federation_test.go | 83 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 104 insertions(+), 2 deletions(-) diff --git a/mcp_federation.go b/mcp_federation.go index d4ae414..45e7782 100644 --- a/mcp_federation.go +++ b/mcp_federation.go @@ -703,7 +703,7 @@ const mcpFederatedFanoutConcurrency = 4 // replaces used, and the one pugbot's mcpclient and agentkit have already // persisted into their own tool-id databases (e.g. // "brave-search_brave_web_search" — verified live). A server that errors, -// times out, or returns an unparsable/invalid result is logged +// times out, panics, or returns an unparsable/invalid result is logged // and skipped, not surfaced as a whole-call failure — UNLESS every single // attempted server failed, in which case an empty tools list would // misleadingly look like "this caller's group has no MCP access" — see @@ -712,7 +712,7 @@ const mcpFederatedFanoutConcurrency = 4 // countTargetRequests (limits.go) attributes one request to EVERY server // ATTEMPTED here, in ONE batched call after wg.Wait() — "attempted", not // "reached" or "succeeded": a server this gateway dialed and got a -// response (or a timeout, or a connection refusal) from still had +// response (or a timeout, a connection refusal, or a panic) from still had // a real request sent to it and a real slot of this gateway's outbound // capacity spent on it, which is what these counters exist to track. A // single federated tools/list call can move several targets' own @@ -730,6 +730,25 @@ func (g *Gateway) mcpFederatedToolsList(w http.ResponseWriter, r *http.Request, wg.Add(1) go func(name string) { defer wg.Done() + // Unrecovered-panic guard (security audit finding 2, + // 2026-08-22): this goroutine runs off the request's own + // goroutine, so a panic here has no ServeHTTP caller to + // unwind into and would crash the whole shared Traefik + // process — the same reasoning limiter.spawn (limits.go) and + // modelRegistry.refreshProvider/captureModelMetadata + // (registry.go) already apply to their own off-request + // goroutines. A panicking backend must degrade to "that + // server failed", exactly like an HTTP error from it, never + // a process-wide crash. + defer func() { + if rec := recover(); rec != nil { + g.logf("federated tools/list: server %q panicked: %v", name, rec) + mu.Lock() + failed = append(failed, name) + mu.Unlock() + } + }() + sem <- struct{}{} defer func() { <-sem }() diff --git a/mcp_federation_test.go b/mcp_federation_test.go index 065dd32..dd19b17 100644 --- a/mcp_federation_test.go +++ b/mcp_federation_test.go @@ -9,6 +9,7 @@ import ( "net/http" "net/http/httptest" "slices" + "strings" "sync" "sync/atomic" "testing" @@ -1308,6 +1309,88 @@ func TestHandleMCPFederated_ToolsList_FanoutBoundedConcurrency(t *testing.T) { } } +// --- security audit finding 2: unrecovered panic in the fan-out --- + +// panicOnHostRoundTripper panics for any request to panicHost — simulating +// an unrecovered interpreter-level panic reached deep inside one backend's +// own call path (mcpBackendCall -> doBackendJSONRPC -> g.targetClient.Do) +// — and delegates every other request to next unchanged. This exercises +// the REAL code path a genuine Yaegi interpreter panic would take, +// through the actual HTTP client the gateway uses, rather than adding any +// test-only hook to production code. +type panicOnHostRoundTripper struct { + next http.RoundTripper + panicHost string +} + +func (rt panicOnHostRoundTripper) RoundTrip(r *http.Request) (*http.Response, error) { + if r.URL.Host == rt.panicHost { + panic("simulated interpreter-level panic mid-flight") + } + return rt.next.RoundTrip(r) +} + +// TestHandleMCPFederated_ToolsList_PanickingBackendDegradesNotCrashes is +// the regression for the missing recover() in mcpFederatedToolsList's own +// fan-out goroutine (security audit finding 2, 2026-08-22): that goroutine +// runs off the request's own goroutine, so an unrecovered panic there has +// no ServeHTTP caller to unwind into and would crash the whole shared +// Traefik process, not just fail one backend. Reaching the end of this +// test at all is part of the proof: if the recover() were missing, beta's +// panicking RoundTrip call below would already have crashed the test +// binary — exactly the registry.go precedent this fix follows +// (TestModelRegistry_RefreshProvider_AdapterPanics_RecoversAndReleasesInFlight). +func TestHandleMCPFederated_ToolsList_PanickingBackendDegradesNotCrashes(t *testing.T) { + alpha := newMockJSONRPCServer(t, []mcpTool{{Name: "lookup"}}) + beta := newMockJSONRPCServer(t, []mcpTool{{Name: "search"}}) // never actually reached; RoundTrip panics first + cfg := newFederationTestConfig(alpha.srv.URL, beta.srv.URL, false) + h, err := New(context.Background(), http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}), cfg, "llmgw") + if err != nil { + t.Fatalf("New: %v", err) + } + gw, ok := h.(*Gateway) + if !ok { + t.Fatal("handler is not *Gateway") + } + betaURL, err := betaHost(beta.srv.URL) + if err != nil { + t.Fatalf("parse beta URL: %v", err) + } + gw.targetClient.Transport = panicOnHostRoundTripper{panicHost: betaURL, next: http.DefaultTransport} + + rec := httptest.NewRecorder() + h.ServeHTTP(rec, newFederatedRequest(t, "sk-alice", jsonrpcRequest{JSONRPC: jsonrpcVersion, Method: "tools/list", ID: json.RawMessage("1")})) + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200, body=%s", rec.Code, rec.Body.String()) + } + + var got jsonrpcResponse + if err := json.Unmarshal(rec.Body.Bytes(), &got); err != nil { + t.Fatalf("decode: %v", err) + } + if got.Error != nil { + t.Fatalf("error = %+v, want nil (a panicking server degrades the aggregate, it does not fail the call)", got.Error) + } + var result mcpToolsListResult + if err := json.Unmarshal(got.Result, &result); err != nil { + t.Fatalf("decode result: %v", err) + } + if len(result.Tools) != 1 || result.Tools[0].Name != "alpha_lookup" { + t.Errorf("tools = %+v, want exactly alpha_lookup (beta panicked, skipped)", result.Tools) + } +} + +// betaHost extracts the host:port a RoundTripper sees on r.URL.Host for +// requests aimed at rawURL — httptest.Server URLs are already in that +// exact "http://host:port" form, so this just strips the scheme. +func betaHost(rawURL string) (string, error) { + const prefix = "http://" + if !strings.HasPrefix(rawURL, prefix) { + return "", fmt.Errorf("unexpected test server URL shape: %q", rawURL) + } + return strings.TrimPrefix(rawURL, prefix), nil +} + // --- security audit finding 1b: tools/list weighted by backend count --- // TestHandleMCPFederated_ToolsList_WeightedByBackendCount_RejectsWhenOverBudget From 87ebee900397d43622c13edbe6c94933f6d4264b Mon Sep 17 00:00:00 2001 From: Lukasz Raczylo Date: Sat, 22 Aug 2026 13:57:07 +0100 Subject: [PATCH 3/8] fix(auth): throttle repeated auth failures, rate-limit failure log Fixes finding 3 from the 2026-08-22 security audit: authStore.identify had no counter, no backoff, and no lockout on failed attempts, and logger.go wrote one unbuffered stderr line per attempt including the caller's RemoteAddr. An unauthenticated attacker got unlimited digest guesses at line rate and could drive synchronous stderr writes on the shared Traefik ingress. - Add per-source-IP failure throttling to identify, reusing the existing in-process memoryStore (limits.go) as authStore's own dedicated instance rather than a new dependency: authFailureLimit (20) failed attempts per authFailureWindow (1 minute) throttles further attempts from that IP outright, including a subsequently valid key, and resets once the window elapses. - clientIP reads r.RemoteAddr only, never a client-supplied header, matching the same trust boundary the rest of this package already assumes (routes_passthrough.go strips X-Forwarded-*/X-Auth-* before any outbound request). - Rate-limit logAuthEvent's failure line via authStore.shouldLogAuthFailure, the identical technique limiter.logStoreError already applies to store errors. Successful-auth logging stays unbounded (bounded by legitimate, already-authenticated traffic elsewhere). --- auth.go | 165 +++++++++++++++++++++++++++++++++++++++++++++---- auth_test.go | 142 ++++++++++++++++++++++++++++++++++++++++++ logger.go | 14 ++++- logger_test.go | 124 +++++++++++++++++++++++++++++++++++++ 4 files changed, 432 insertions(+), 13 deletions(-) create mode 100644 logger_test.go diff --git a/auth.go b/auth.go index 96359b7..c39ea74 100644 --- a/auth.go +++ b/auth.go @@ -4,6 +4,7 @@ import ( "crypto/sha256" "crypto/subtle" "fmt" + "net" "net/http" "net/url" "path" @@ -187,6 +188,40 @@ type authEntry struct { digest [32]byte } +// authFailureWindow and authFailureLimit bound how many failed identify +// attempts one source IP may make before further attempts from it are +// throttled outright (security audit finding 3, 2026-08-22): identify +// previously had no counter, no backoff, and no lockout at all — an +// unauthenticated caller got unlimited digest-lookup attempts at line +// rate. 20 failures per minute is generous headroom for a legitimate +// client with a stale or mistyped key retried a few times (a human +// pasting the wrong key, a misconfigured script cycling through a couple +// of candidates) while still meaningfully bounding a scripted guesser's +// throughput per source IP. This does not, and cannot, defend the +// underlying key space on its own (a SHA-256 digest lookup already makes +// brute force infeasible) — its real job is capping the synchronous cost +// an unauthenticated caller can impose on the shared Traefik ingress per +// unit time: identify's own digest lookup, and logAuthEvent's stderr +// write (logger.go), both of which this finding's own framing calls out. +const ( + authFailureWindow = time.Minute + authFailureLimit = 20 + // authFailureLogEvery rate-limits logAuthEvent's own failed-auth log + // line (logger.go) the identical way limiter.logStoreError (limits.go) + // already rate-limits store-error lines — sharing that function's own + // storeErrorLogEvery interval keeps this codebase's "how often do we + // log a sustained failure condition" cadence consistent across both + // call sites, rather than picking a new, unrelated number here. + authFailureLogEvery = storeErrorLogEvery +) + +// authFailureKeyPrefix namespaces authStore's failure-tracking keys in its +// own memoryStore instance (failures, below) — a plain string key, not +// windowKey's (kind, id, metric, window) tuple (limits.go), since auth +// failures are tracked per SOURCE IP, a concept the rest of this +// package's counter keying was never built to express. +const authFailureKeyPrefix = "authfail:" + // authStore identifies requests by API key and resolves them to a user and // their group. Raw API keys are never retained after construction — only // their SHA-256 digests. @@ -198,19 +233,35 @@ type authEntry struct { // overwrite each other. buildMu is never held at the same time as mu, so a // rebuild in progress never blocks identify's readers. reloadMu (Task 4) // serializes maybeReload's throttle check and the reload it may trigger. +// +// failures is identify's own per-source-IP failed-attempt counter +// (security audit finding 3, 2026-08-22) — the SAME in-process memoryStore +// type limiter's fallback store already uses (limits.go), a deliberately +// reused type rather than a new dependency, given its own TTL-bucketed +// counter semantics already fit this need exactly. It is authStore's own +// dedicated instance, not limiter.fallback: auth failures are a distinct +// concept (throttling an unauthenticated caller) from LLM traffic limits +// (throttling an authenticated one's usage), and authStore must work +// identically whether or not a Gateway even has a limiter wired yet. +// authFailLogMu/authFailLastLogAt back shouldLogAuthFailure, the identical +// "guard a timestamp with a small mutex, rate-limit a log line" shape +// limiter.logMu/lastLogAt already use. type authStore struct { - groups map[string]*group - inline map[[32]byte]*authEntry // built once by newAuthStore; never mutated afterward - byDigest map[[32]byte]*authEntry // inline entries plus the current file-sourced set; guarded by mu - usersFile *usersFile // nil when Users.File is not configured; maybeReload no-ops - log gatewayLogger // set alongside usersFile; unused when usersFile is nil - nowFn func() time.Time // injected for tests; defaults to time.Now - lastCheck time.Time // guarded by reloadMu - lastModTime time.Time // guarded by reloadMu - fileUserCount int // guarded by mu; size of the current file-sourced user set - mu sync.RWMutex - buildMu sync.Mutex - reloadMu sync.Mutex + groups map[string]*group + inline map[[32]byte]*authEntry // built once by newAuthStore; never mutated afterward + byDigest map[[32]byte]*authEntry // inline entries plus the current file-sourced set; guarded by mu + usersFile *usersFile // nil when Users.File is not configured; maybeReload no-ops + log gatewayLogger // set alongside usersFile; unused when usersFile is nil + nowFn func() time.Time // injected for tests; defaults to time.Now + failures *memoryStore // per-source-IP failed-identify counters; see doc comment above + lastCheck time.Time // guarded by reloadMu + lastModTime time.Time // guarded by reloadMu + authFailLastLogAt time.Time // guarded by authFailLogMu; last time a failed-auth line was logged + fileUserCount int // guarded by mu; size of the current file-sourced user set + mu sync.RWMutex + buildMu sync.Mutex + reloadMu sync.Mutex + authFailLogMu sync.Mutex } // newAuthStore builds an authStore from cfg's groups and inline users. It @@ -223,6 +274,13 @@ func newAuthStore(cfg *Config) (*authStore, error) { byDigest: make(map[[32]byte]*authEntry), nowFn: time.Now, } + a.failures = newMemoryStore() + // Delegate through a.nowFn rather than copying it: a test that + // overrides a.nowFn AFTER construction (the fakeClock convention + // newReloadableAuthStore already uses, users_file_test.go) must still + // control failures' own clock — this closure reads a.nowFn fresh on + // every call, so it always sees whichever func is currently assigned. + a.failures.nowFn = func() time.Time { return a.nowFn() } for name, gc := range cfg.Groups { if gc == nil { return nil, fmt.Errorf("llmgateway: group %q: config must not be nil", name) @@ -363,14 +421,95 @@ func (a *authStore) replaceFileUsers(us []*UserConfig) error { return nil } +// clientIP returns the source address identify's own throttle keys on: +// r.RemoteAddr with any ":port" suffix stripped — net/http's own "address +// as seen by this server's TCP accept", never a client-supplied header +// (security audit finding 3, 2026-08-22). This plugin runs AS a Traefik +// middleware inside the shared ingress process, not behind some second, +// untrusted proxy of its own, so RemoteAddr is the same trust boundary +// logAuthEvent's own RemoteAddr-only logging (logger.go) already assumes, +// and the same one routes_passthrough.go's dangerousClientHeaderPrefixes +// strip (X-Forwarded-*, X-Auth-*) exists to keep a client from spoofing on +// the OUTBOUND side — deliberately NOT read here either, for the same +// reason. A RemoteAddr with no ":port" (net.SplitHostPort's own error +// case — not expected from net/http in practice, but not assumed) is used +// as-is rather than discarded, so a throttle key still exists for +// whatever value RemoteAddr carries. +func clientIP(r *http.Request) string { + host, _, err := net.SplitHostPort(r.RemoteAddr) + if err != nil { + return r.RemoteAddr + } + return host +} + +// authThrottled reports whether ip has already reached authFailureLimit +// failed identify attempts within the current authFailureWindow — a +// read-only check, so a throttled IP's later attempts cost nothing beyond +// one in-process map read, never a digest computation or byDigest lookup. +func (a *authStore) authThrottled(ip string) bool { + count, _ := a.failures.get(authFailureKeyPrefix + ip) // memoryStore.get never errors + return count >= authFailureLimit +} + +// recordAuthFailure increments ip's failure count for the current +// authFailureWindow, creating a fresh window if the previous one already +// expired (memoryStore.incrBy's own fixed-window contract, limits.go). +func (a *authStore) recordAuthFailure(ip string) { + _, _ = a.failures.incrBy(authFailureKeyPrefix+ip, 1, authFailureWindow) // memoryStore.incrBy never errors +} + +// shouldLogAuthFailure reports whether the next failed-auth log line +// (logAuthEvent, logger.go) should actually be written, rate-limited to +// once per authFailureLogEvery regardless of how many attempts fail in +// between — the identical technique limiter.logStoreError (limits.go) +// already applies to store errors, applied here so an attacker driving +// unlimited failed-auth attempts can never drive unlimited synchronous +// stderr writes on the shared Traefik ingress (security audit finding 3, +// 2026-08-22) even before authThrottled's own per-IP cap engages (a +// distributed attempt spread across many source IPs never trips any +// single IP's own threshold, but must still not flood the log). +// Successful-auth logging is deliberately left unbounded: it requires an +// already-valid API key, so its volume is bounded by legitimate +// authenticated traffic — itself already governed by checkAndCount's +// requests-per-minute/day limits elsewhere — not by an unauthenticated +// caller's attempt rate, which is exactly what this finding is about +// bounding. +func (a *authStore) shouldLogAuthFailure() bool { + now := a.nowFn() + a.authFailLogMu.Lock() + defer a.authFailLogMu.Unlock() + if now.Sub(a.authFailLastLogAt) < authFailureLogEvery { + return false + } + a.authFailLastLogAt = now + return true +} + // identify resolves r's presented API key to a user and their group. It // reads "Authorization: Bearer " (case-insensitive "bearer" prefix) or // "x-api-key: "; Bearer wins when both are present. The presented key // is looked up by SHA-256 digest, then verified with a constant-time // compare — the key itself is never retained. +// +// Per-source-IP throttling (security audit finding 3, 2026-08-22): a +// request from an IP already over authThrottled's own budget is rejected +// immediately, before any digest work at all. Every OTHER failure path +// below — no key presented, key not found, or a (practically unreachable, +// see subtle.ConstantTimeCompare's own call site) digest mismatch — +// records one failure for the caller's IP via recordAuthFailure. A +// successful identify never records a failure and never resets a prior +// count: this is a rate limit on failed attempts, not a lockout a +// legitimate request can clear early. func (a *authStore) identify(r *http.Request) (*user, *group, bool) { + ip := clientIP(r) + if a.authThrottled(ip) { + return nil, nil, false + } + key, ok := presentedKey(r) if !ok { + a.recordAuthFailure(ip) return nil, nil, false } digest := sha256.Sum256([]byte(key)) @@ -379,9 +518,11 @@ func (a *authStore) identify(r *http.Request) (*user, *group, bool) { entry, ok := a.byDigest[digest] a.mu.RUnlock() if !ok { + a.recordAuthFailure(ip) return nil, nil, false } if subtle.ConstantTimeCompare(digest[:], entry.digest[:]) != 1 { + a.recordAuthFailure(ip) return nil, nil, false } return entry.user, entry.group, true diff --git a/auth_test.go b/auth_test.go index 40cca70..46030e6 100644 --- a/auth_test.go +++ b/auth_test.go @@ -577,3 +577,145 @@ func TestAuthStore_Identify_NotBlockedByBuildMu(t *testing.T) { t.Fatal("identify blocked while buildMu is held — reader lock must not depend on the writer-serialization lock") } } + +// --- security audit finding 3: per-source-IP auth-failure throttling --- + +func TestClientIP_StripsPort(t *testing.T) { + r := httptest.NewRequest("POST", "/v1/chat/completions", nil) + r.RemoteAddr = "203.0.113.9:54321" + if got := clientIP(r); got != "203.0.113.9" { + t.Errorf("clientIP = %q, want %q", got, "203.0.113.9") + } +} + +func TestClientIP_MalformedRemoteAddr_UsedAsIs(t *testing.T) { + r := httptest.NewRequest("POST", "/v1/chat/completions", nil) + r.RemoteAddr = "not-a-host-port" + if got := clientIP(r); got != "not-a-host-port" { + t.Errorf("clientIP = %q, want the raw RemoteAddr back unchanged", got) + } +} + +// TestAuthStore_Identify_ThrottlesAfterRepeatedFailures_EvenValidKeyRejected +// is the core regression for finding 3: identify previously had no +// counter, no backoff, and no lockout at all. After authFailureLimit +// failed attempts from one source IP, even a subsequently CORRECT key +// from that same IP must be rejected — proving the throttle actually +// short-circuits real auth once tripped, not merely that repeated wrong +// keys keep failing (which they would anyway). A different source IP must +// be entirely unaffected, proving the throttle is per-IP, not global. +func TestAuthStore_Identify_ThrottlesAfterRepeatedFailures_EvenValidKeyRejected(t *testing.T) { + a, err := newAuthStore(testAuthCfg()) // inline user "a" / sk-secret + if err != nil { + t.Fatal(err) + } + const attackerIP = "203.0.113.9:54321" + + for i := 0; i < authFailureLimit; i++ { + r := httptest.NewRequest("POST", "/v1/chat/completions", nil) + r.RemoteAddr = attackerIP + r.Header.Set("x-api-key", "wrong-key") + if _, _, ok := a.identify(r); ok { + t.Fatalf("attempt %d: want failure for a wrong key, got success", i) + } + } + + // Throttle should now be engaged for attackerIP — even the CORRECT + // key must be rejected. + valid := httptest.NewRequest("POST", "/v1/chat/completions", nil) + valid.RemoteAddr = attackerIP + valid.Header.Set("Authorization", "Bearer sk-secret") + if _, _, ok := a.identify(valid); ok { + t.Fatal("want a throttled IP rejected even with a valid key") + } + + // A different source IP must be unaffected by attackerIP's throttle. + other := httptest.NewRequest("POST", "/v1/chat/completions", nil) + other.RemoteAddr = "198.51.100.5:1111" + other.Header.Set("Authorization", "Bearer sk-secret") + if _, _, ok := a.identify(other); !ok { + t.Fatal("want a different source IP unaffected by another IP's throttle") + } +} + +// TestAuthStore_Identify_ThrottleResetsAfterWindowExpires proves the +// throttle is a fixed-window rate limit, not a permanent lockout: once +// authFailureWindow has actually elapsed, the same IP's valid key works +// again. +func TestAuthStore_Identify_ThrottleResetsAfterWindowExpires(t *testing.T) { + a, err := newAuthStore(testAuthCfg()) // inline user "a" / sk-secret + if err != nil { + t.Fatal(err) + } + clock := &fakeClock{now: time.Unix(1_700_000_000, 0)} + a.nowFn = clock.Now + const attackerIP = "203.0.113.9:54321" + + for i := 0; i < authFailureLimit; i++ { + r := httptest.NewRequest("POST", "/v1/chat/completions", nil) + r.RemoteAddr = attackerIP + r.Header.Set("x-api-key", "wrong-key") + if _, _, ok := a.identify(r); ok { + t.Fatalf("attempt %d: want failure for a wrong key, got success", i) + } + } + blocked := httptest.NewRequest("POST", "/v1/chat/completions", nil) + blocked.RemoteAddr = attackerIP + blocked.Header.Set("Authorization", "Bearer sk-secret") + if _, _, ok := a.identify(blocked); ok { + t.Fatal("want throttled before the window elapses") + } + + clock.Advance(authFailureWindow + time.Second) + + recovered := httptest.NewRequest("POST", "/v1/chat/completions", nil) + recovered.RemoteAddr = attackerIP + recovered.Header.Set("Authorization", "Bearer sk-secret") + if _, _, ok := a.identify(recovered); !ok { + t.Fatal("want the same IP's valid key to work again once authFailureWindow has elapsed") + } +} + +// TestAuthStore_Identify_FewFailedAttempts_NeverThrottled is the good-path +// counterpart: a handful of failed attempts (well under authFailureLimit +// — a human retrying a mistyped key a couple of times) must never trip +// the throttle. +func TestAuthStore_Identify_FewFailedAttempts_NeverThrottled(t *testing.T) { + a, err := newAuthStore(testAuthCfg()) + if err != nil { + t.Fatal(err) + } + const ip = "203.0.113.9:54321" + for i := 0; i < 3; i++ { + r := httptest.NewRequest("POST", "/v1/chat/completions", nil) + r.RemoteAddr = ip + r.Header.Set("x-api-key", "wrong-key") + a.identify(r) + } + r := httptest.NewRequest("POST", "/v1/chat/completions", nil) + r.RemoteAddr = ip + r.Header.Set("Authorization", "Bearer sk-secret") + if _, _, ok := a.identify(r); !ok { + t.Fatal("want a valid key to succeed after only 3 prior failures (well under authFailureLimit)") + } +} + +// TestAuthStore_Identify_SuccessfulAuthDoesNotRecordFailure proves a +// successful identify never itself counts toward the failure budget — +// otherwise a legitimate, high-traffic user could eventually throttle +// themselves. +func TestAuthStore_Identify_SuccessfulAuthDoesNotRecordFailure(t *testing.T) { + a, err := newAuthStore(testAuthCfg()) + if err != nil { + t.Fatal(err) + } + const ip = "203.0.113.9:54321" + r := httptest.NewRequest("POST", "/v1/chat/completions", nil) + r.RemoteAddr = ip + r.Header.Set("Authorization", "Bearer sk-secret") + for i := 0; i < authFailureLimit+5; i++ { + if _, _, ok := a.identify(r); !ok { + t.Fatalf("attempt %d: want success (valid key), got failure", i) + } + } +} diff --git a/logger.go b/logger.go index 1af1c0a..4c9097a 100644 --- a/logger.go +++ b/logger.go @@ -25,9 +25,21 @@ func (g *Gateway) errorf(format string, args ...any) { // line ever includes the presented API key itself — identify resolves a // key to a *user and hands this function only that user's name, never the // key it authenticated. +// +// The failure line is rate-limited via g.auth.shouldLogAuthFailure +// (auth.go), the same technique limiter.logStoreError (limits.go) already +// applies to store-error lines (security audit finding 3, 2026-08-22): an +// unauthenticated caller driving unlimited failed attempts must never be +// able to drive unlimited synchronous stderr writes on the shared Traefik +// ingress — a slow log pipe would otherwise stall request handling for +// every tenant, not just the one failing auth. The success line stays +// unbounded — see shouldLogAuthFailure's own doc comment for why that is +// safe. func (g *Gateway) logAuthEvent(ok bool, userName string, r *http.Request) { if !ok { - g.logf("auth failed: %s %q from %s", r.Method, r.URL.Path, r.RemoteAddr) + if g.auth.shouldLogAuthFailure() { + g.logf("auth failed: %s %q from %s", r.Method, r.URL.Path, r.RemoteAddr) + } return } g.logf("auth ok: user %q %s %q", userName, r.Method, r.URL.Path) diff --git a/logger_test.go b/logger_test.go new file mode 100644 index 0000000..cc9bafb --- /dev/null +++ b/logger_test.go @@ -0,0 +1,124 @@ +package traefikllmgateway + +import ( + "bytes" + "context" + "io" + "net/http" + "net/http/httptest" + "os" + "strings" + "testing" + "time" +) + +// newTestGatewayForLogger builds a minimal *Gateway via the real New() +// constructor (the only supported construction path — see this package's +// other test files, none of which build a bare &Gateway{} literal) for +// logAuthEvent's own tests below. +func newTestGatewayForLogger(t *testing.T) *Gateway { + t.Helper() + cfg := CreateConfig() + cfg.Providers = map[string]*ProviderConfig{"openai": {Type: "openai", APIKey: "k"}} + h, err := New(context.Background(), http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}), cfg, "llmgw") + if err != nil { + t.Fatalf("New: %v", err) + } + gw, ok := h.(*Gateway) + if !ok { + t.Fatal("handler is not *Gateway") + } + return gw +} + +// captureStderr redirects os.Stderr for the duration of fn (the same +// os.Pipe-based idiom this package's other test files already use for +// logf/errorf, e.g. TestGateway_Logf_WritesInfoPrefix, llmgateway_test.go) +// and returns everything written to it. +func captureStderr(t *testing.T, fn func()) string { + t.Helper() + origStderr := os.Stderr + r, w, err := os.Pipe() + if err != nil { + t.Fatalf("os.Pipe: %v", err) + } + os.Stderr = w + fn() + _ = w.Close() + os.Stderr = origStderr + var buf bytes.Buffer + if _, err := io.Copy(&buf, r); err != nil { + t.Fatalf("io.Copy: %v", err) + } + return buf.String() +} + +// TestLogAuthEvent_Failure_RateLimited is the regression for the missing +// rate limit on logAuthEvent's failure line (security audit finding 3, +// 2026-08-22): an attacker driving many failed-auth attempts in quick +// succession must produce ONE log line per authFailureLogEvery, not one +// per attempt — otherwise the synchronous os.Stderr write itself becomes +// an amplification vector on the shared Traefik ingress. +func TestLogAuthEvent_Failure_RateLimited(t *testing.T) { + gw := newTestGatewayForLogger(t) + clock := &fakeClock{now: time.Unix(1_700_000_000, 0)} + gw.auth.nowFn = clock.Now + + r := httptest.NewRequest("POST", "/v1/chat/completions", nil) + r.RemoteAddr = "203.0.113.9:1234" + + out := captureStderr(t, func() { + for i := 0; i < 5; i++ { + gw.logAuthEvent(false, "", r) + } + }) + + if n := strings.Count(out, "auth failed:"); n != 1 { + t.Fatalf("auth-failed lines = %d, want exactly 1 (5 attempts within one authFailureLogEvery window), output=%q", n, out) + } +} + +// TestLogAuthEvent_Failure_LogsAgainAfterIntervalElapses proves the rate +// limit is a cadence, not a one-shot silence: once authFailureLogEvery has +// actually elapsed, the next failure is logged again. +func TestLogAuthEvent_Failure_LogsAgainAfterIntervalElapses(t *testing.T) { + gw := newTestGatewayForLogger(t) + clock := &fakeClock{now: time.Unix(1_700_000_000, 0)} + gw.auth.nowFn = clock.Now + + r := httptest.NewRequest("POST", "/v1/chat/completions", nil) + r.RemoteAddr = "203.0.113.9:1234" + + out := captureStderr(t, func() { + gw.logAuthEvent(false, "", r) + clock.Advance(authFailureLogEvery + time.Second) + gw.logAuthEvent(false, "", r) + }) + + if n := strings.Count(out, "auth failed:"); n != 2 { + t.Fatalf("auth-failed lines = %d, want exactly 2 (interval elapsed between the two attempts), output=%q", n, out) + } +} + +// TestLogAuthEvent_Success_NeverRateLimited proves successful-auth logging +// is deliberately left unbounded (shouldLogAuthFailure's own doc comment, +// auth.go, justifies why): it takes an already-valid key to reach, so its +// volume is bounded by legitimate traffic elsewhere, not by an +// unauthenticated caller's attempt rate. +func TestLogAuthEvent_Success_NeverRateLimited(t *testing.T) { + gw := newTestGatewayForLogger(t) + clock := &fakeClock{now: time.Unix(1_700_000_000, 0)} + gw.auth.nowFn = clock.Now + + r := httptest.NewRequest("POST", "/v1/chat/completions", nil) + + out := captureStderr(t, func() { + for i := 0; i < 5; i++ { + gw.logAuthEvent(true, "alice", r) + } + }) + + if n := strings.Count(out, "auth ok:"); n != 5 { + t.Fatalf("auth-ok lines = %d, want 5 (success logging must never be rate-limited), output=%q", n, out) + } +} From 800f63ac53da066ecaa2d977d6c559d1f11b3155 Mon Sep 17 00:00:00 2001 From: Lukasz Raczylo Date: Sat, 22 Aug 2026 13:57:54 +0100 Subject: [PATCH 4/8] fix(admin): chunk usage batch reads to bound connection contention MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes finding 4 from the 2026-08-22 security audit: buildAdminUsage sent every user's, group's, and the total scope's counters (8 keys each) to limiter.currentUsage in ONE storeGetMulti round trip. With 1,000 users that is ~8,000 GETs pipelined through the single mutex-guarded store connection, during which every concurrent checkAndCount from live LLM traffic queues behind it (up to respCallTimeout) — and the admin dashboard polls this endpoint every 5s. Add chunkedCurrentUsage: splits the scopes slice into batches of at most adminUsageChunkScopes (200) before calling currentUsage, letting live traffic interleave between chunks while each chunk still batches its own scopes in one round trip. At or under 200 scopes (the live cluster's 9 users) this makes exactly one call, identical to today's behavior — the payload shape is unchanged, so no webui coordination is needed. --- admin.go | 69 +++++++++++++++++++++++++++++++------ admin_test.go | 95 +++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 153 insertions(+), 11 deletions(-) diff --git a/admin.go b/admin.go index a7b5dd7..f24544b 100644 --- a/admin.go +++ b/admin.go @@ -631,16 +631,63 @@ func usageEntryView(su scopeUsage, limits *LimitsConfig) adminUsageEntryView { // buildAdminUsage assembles adminUsageResponse: authStore.snapshot lists // every currently active user and every configured group (names, group // membership, and limits only — never a key or its digest), and -// limiter.currentUsage reads every one's current-window counters — plus -// the synthetic total scope's own (v0.2 data-layer task) — in ONE -// storeGetMulti round trip total (v0.2 final review wave, 2026-08-20; see -// limiter.currentUsage's own doc comment for the tradeoff this -// supersedes) — users, groups, and the total scope are concatenated into -// a single scopes slice before that one call, then the flat result is -// sliced back into the three response sections at the same split points, -// order preserved. Unlike buildLimitScopes (routes_unified.go), this -// never omits an entity for having nil limits — the dashboard shows usage -// for every user and group, limited or not. +// chunkedCurrentUsage (above) reads every one's current-window counters — +// plus the synthetic total scope's own (v0.2 data-layer task) — in +// batches of at most adminUsageChunkScopes scopes per storeGetMulti round +// trip (security audit finding 4, 2026-08-22, capping the v0.2 final +// review wave's own "fold everything into ONE round trip" design — see +// adminUsageChunkScopes' own doc comment for why) — users, groups, and the +// total scope are concatenated into a single scopes slice before +// chunking, then the flat result is sliced back into the three response +// sections at the same split points, order preserved. Unlike +// buildLimitScopes (routes_unified.go), this never omits an entity for +// having nil limits — the dashboard shows usage for every user and group, +// limited or not. +// adminUsageChunkScopes bounds how many scopes buildAdminUsage sends to +// limiter.currentUsage in a single storeGetMulti round trip (security +// audit finding 4, 2026-08-22). currentUsage's own doc comment (limits.go) +// already explains why every scope was folded into ONE round trip rather +// than one per scope — but "one round trip" for the WHOLE catalog means +// that round trip's own pipeline size, and how long it holds the shared, +// mutex-guarded Redis connection (respClient, resp.go), now scales with +// how many users and groups are configured. With 1,000 users that is +// ~8,000 GETs (usageKeysPerScope=8) in one pipelined call, during which +// every concurrent checkAndCount from live LLM traffic queues behind the +// same connection mutex — and the admin dashboard polls this endpoint +// every 5s. chunkedCurrentUsage (below) keeps each individual round trip's +// key count bounded, letting live traffic interleave between chunks, +// while a chunk still batches every scope inside it exactly as before, +// keeping most of currentUsage's own round-trip-collapsing win. 200 +// scopes (1,600 keys per chunk) comfortably covers the live cluster's 5 +// friends + 4 home users in a single chunk, so today's real deployment +// sees no behavior change at all — chunking only engages once a +// deployment's user+group count actually grows past it. One accepted, +// disclosed side effect: currentUsage's own doc comment notes a transient +// store error marks every scope in ONE call storeDown together; chunking +// narrows that blast radius to one chunk's worth of scopes instead of the +// whole response, which is a strict improvement, not a new risk. +const adminUsageChunkScopes = 200 + +// chunkedCurrentUsage calls limiter.currentUsage in batches of at most +// adminUsageChunkScopes scopes at a time, concatenating the results in +// the SAME order scopes was given — buildAdminUsage's own slicing of the +// flat result back into users/groups/total (below) is unaffected by, and +// unaware of, the chunking underneath. A scopes slice at or under the +// chunk size makes exactly one call, byte-for-byte what calling +// currentUsage directly would have done. +func (g *Gateway) chunkedCurrentUsage(scopes []limitScope) []scopeUsage { + out := make([]scopeUsage, 0, len(scopes)) + for len(scopes) > 0 { + n := adminUsageChunkScopes + if n > len(scopes) { + n = len(scopes) + } + out = append(out, g.limiter.currentUsage(scopes[:n])...) + scopes = scopes[n:] + } + return out +} + func (g *Gateway) buildAdminUsage() adminUsageResponse { userSummaries, groupSummaries := g.auth.snapshot() @@ -653,7 +700,7 @@ func (g *Gateway) buildAdminUsage() adminUsageResponse { } scopes = append(scopes, limitScope{kind: totalScopeKind, id: totalScopeID, limits: nil}) - allUsage := g.limiter.currentUsage(scopes) + allUsage := g.chunkedCurrentUsage(scopes) userUsage := allUsage[:len(userSummaries)] groupUsage := allUsage[len(userSummaries) : len(userSummaries)+len(groupSummaries)] totalUsage := allUsage[len(userSummaries)+len(groupSummaries)] diff --git a/admin_test.go b/admin_test.go index e888b9d..c5e1f4a 100644 --- a/admin_test.go +++ b/admin_test.go @@ -6,6 +6,7 @@ import ( "crypto/sha256" "encoding/hex" "encoding/json" + "fmt" "net/http" "net/http/httptest" "net/url" @@ -1456,6 +1457,100 @@ func TestLimiterCurrentUsage_BatchesOneGetMultiCallForAllScopes(t *testing.T) { } } +// --- security audit finding 4: admin usage batch chunking --- + +// TestChunkedCurrentUsage_SplitsIntoMultipleRoundTrips is the regression +// for finding 4: buildAdminUsage previously handed limiter.currentUsage +// the WHOLE scopes slice in one call, so one round trip's own pipeline +// size (and how long it holds the shared, mutex-guarded store connection) +// scaled with catalog size with no ceiling at all. With more scopes than +// adminUsageChunkScopes, chunkedCurrentUsage must issue more than one +// getMulti call — proving chunking actually happens, not merely that the +// constant exists. +func TestChunkedCurrentUsage_SplitsIntoMultipleRoundTrips(t *testing.T) { + t.Parallel() + fixedNow := time.Date(2026, 8, 22, 10, 0, 0, 0, time.UTC) + const n = adminUsageChunkScopes + 50 // over one chunk, under two + + store := &countingMultiStore{values: make(map[string]int64, n)} + scopes := make([]limitScope, n) + for i := 0; i < n; i++ { + id := fmt.Sprintf("user%d", i) + scopes[i] = limitScope{kind: "user", id: id, limits: &LimitsConfig{}} + store.values[windowKey("user", id, metricReq, windowMin, fixedNow)] = int64(i) + } + + l := newLimiter(store, true) + l.nowFn = func() time.Time { return fixedNow } + gw := &Gateway{limiter: l} + + got := gw.chunkedCurrentUsage(scopes) + + if store.getMultiCalls != 2 { + t.Errorf("getMultiCalls = %d, want 2 (%d scopes over one %d-scope chunk)", store.getMultiCalls, n, adminUsageChunkScopes) + } + if len(got) != n { + t.Fatalf("len(got) = %d, want %d", len(got), n) + } + // Order must be preserved across the chunk boundary — spot-check the + // first scope of the SECOND chunk, whose value only exists if the + // concatenation kept scopes[adminUsageChunkScopes] aligned with + // got[adminUsageChunkScopes]. + boundary := got[adminUsageChunkScopes] + if boundary.id != fmt.Sprintf("user%d", adminUsageChunkScopes) || boundary.requestsPerMinute != int64(adminUsageChunkScopes) { + t.Errorf("scope at the chunk boundary = %+v, want id=%q requestsPerMinute=%d (order preserved across chunks)", + boundary, fmt.Sprintf("user%d", adminUsageChunkScopes), adminUsageChunkScopes) + } +} + +// TestChunkedCurrentUsage_AtOrBelowChunkSize_MakesExactlyOneCall proves +// chunking is a no-op difference for any deployment at or under +// adminUsageChunkScopes — including the live cluster's 5 friends + 4 home +// users (project brief) — matching currentUsage's own pre-existing +// single-round-trip behavior exactly. +func TestChunkedCurrentUsage_AtOrBelowChunkSize_MakesExactlyOneCall(t *testing.T) { + t.Parallel() + fixedNow := time.Date(2026, 8, 22, 10, 0, 0, 0, time.UTC) + store := &countingMultiStore{values: map[string]int64{}} + scopes := []limitScope{ + {kind: "user", id: "alice", limits: &LimitsConfig{}}, + {kind: "user", id: "bob", limits: &LimitsConfig{}}, + {kind: "group", id: "eng", limits: &LimitsConfig{}}, + } + l := newLimiter(store, true) + l.nowFn = func() time.Time { return fixedNow } + gw := &Gateway{limiter: l} + + got := gw.chunkedCurrentUsage(scopes) + + if store.getMultiCalls != 1 { + t.Errorf("getMultiCalls = %d, want 1 (scope count under adminUsageChunkScopes)", store.getMultiCalls) + } + if len(got) != len(scopes) { + t.Fatalf("len(got) = %d, want %d", len(got), len(scopes)) + } +} + +// TestChunkedCurrentUsage_EmptyScopes proves the edge case a real caller +// never hits directly (buildAdminUsage always appends the synthetic total +// scope) still behaves like currentUsage's own empty-input contract: +// no call, no panic, empty result. +func TestChunkedCurrentUsage_EmptyScopes(t *testing.T) { + t.Parallel() + store := &countingMultiStore{values: map[string]int64{}} + l := newLimiter(store, true) + gw := &Gateway{limiter: l} + + got := gw.chunkedCurrentUsage(nil) + + if store.getMultiCalls != 0 { + t.Errorf("getMultiCalls = %d, want 0 for an empty scopes slice", store.getMultiCalls) + } + if len(got) != 0 { + t.Errorf("len(got) = %d, want 0", len(got)) + } +} + // --- overview: redis lastErr carries a timestamp (folded item 4, 2026-08-20 review) --- func TestAdminOverview_RedisLastErrAt(t *testing.T) { From d2a0c4860f0bbc45eb76e0a026676fc90676de60 Mon Sep 17 00:00:00 2001 From: Lukasz Raczylo Date: Sat, 22 Aug 2026 13:58:48 +0100 Subject: [PATCH 5/8] fix(auth): reject duplicate and empty user names MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes finding 5 from the 2026-08-22 security audit: newAuthStore and replaceFileUsers rejected duplicate API keys but not duplicate names, and counters are keyed on u.name (routes_unified.go), so two users sharing a name silently shared llmgw:user:{name}:* counters and both appeared in buildAdminUsage; an empty name collapsed every such user into llmgw:user::*. - buildEntry now rejects an empty user name outright — the source of both bugs, since duplicate names and an empty name are the same root cause (two config entries resolving to the same counter key). - newAuthStore and replaceFileUsers each reject a duplicate name WITHIN their own source (two inline users, or two file users) — but not across sources: a file user overriding an inline user of the same name stays intentional and unaffected, per replaceFileUsers' own documented merge semantics. - Deliberately does NOT extend configNamePattern's character restriction to user names: that pattern exists because provider/ mcpServers/agents names are embedded as URL path segments, which a user name never is here. Restricting the charset further risked rejecting real operator-chosen names this round has no visibility into; the live users file's steve-cw/kevinsandom already pass the non-empty check unaffected. Also adds TestDefaultPreserving_LiveClusterShape_LoadsAndServesIdentically (admin_test.go), modeling the live cluster's 5 friends + 4 home users and 11 MCP servers to prove all five fixes in this round load and serve identically at that scale. --- admin_test.go | 80 ++++++++++++++++++++++++++++++++++++++++++ auth.go | 44 +++++++++++++++++++++++ auth_test.go | 96 +++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 220 insertions(+) diff --git a/admin_test.go b/admin_test.go index c5e1f4a..74ce70e 100644 --- a/admin_test.go +++ b/admin_test.go @@ -1994,3 +1994,83 @@ func TestAdminTargets_AccessListsMatchEnforcement_AndCountersReflectTraffic(t *t t.Errorf("bot2.Access = %v, want %v (restricted's own agents list excludes it)", bot2.Access, want) } } + +// --- default-preserving: the live cluster's real shape (security audit, 2026-08-22) --- + +// TestDefaultPreserving_LiveClusterShape_LoadsAndServesIdentically models +// the live production shape this round's five fixes must not break +// (project brief): 5 "friend" users plus 4 "home" users (steve-cw and +// kevinsandom are named live users; the rest are representative — real +// names this test has no visibility into, which is exactly why finding +// 5's fix stops short of restricting the name charset, see buildEntry's +// own doc comment, auth.go) and 11 MCP servers. A2A agents are out of +// scope here: none of this round's five findings touch mcp_a2a.go or any +// A2A-specific code path, so there is no mechanism by which they could +// regress agent handling. +// +// It proves construction succeeds unchanged (finding 5's new empty/ +// duplicate-name checks accept every one of these real names), a normal +// federated tools/list call from an ordinary user still succeeds under a +// realistic per-minute limit (finding 1b's per-backend weighting does not +// spuriously throttle ordinary usage — 11 servers is comfortably under a +// 60/min budget), and the admin dashboard's usage poll still returns +// exactly one row per user and per group in a single store round trip +// (finding 4's chunking is a no-op at 10 users, well under +// adminUsageChunkScopes). +func TestDefaultPreserving_LiveClusterShape_LoadsAndServesIdentically(t *testing.T) { + friendNames := []string{"steve-cw", "kevinsandom", "friend3", "friend4", "friend5"} + homeNames := []string{"home1", "home2", "home3", "home4"} + + cfg := CreateConfig() + cfg.Providers = map[string]*ProviderConfig{"openai": {Type: "openai", APIKey: "k"}} + cfg.Admin = &AdminConfig{Enabled: true} + cfg.Groups = map[string]*GroupConfig{ + "friends": {Limits: &LimitsConfig{RequestsPerMinute: 60}}, + "home": {Limits: &LimitsConfig{RequestsPerMinute: 120}}, + } + + cfg.MCPServers = make(map[string]*TargetConfig, 11) + for i := 0; i < 11; i++ { + srv := newMockJSONRPCServer(t, []mcpTool{{Name: "lookup"}}) + cfg.MCPServers[fmt.Sprintf("mcp%d", i)] = &TargetConfig{URL: srv.srv.URL} + } + + inline := []*UserConfig{{Name: "admin1", Group: "home", Admin: true, APIKey: "sk-admin1"}} + for _, name := range friendNames { + inline = append(inline, &UserConfig{Name: name, Group: "friends", APIKey: "sk-" + name}) + } + for _, name := range homeNames { + inline = append(inline, &UserConfig{Name: name, Group: "home", APIKey: "sk-" + name}) + } + cfg.Users = &UsersConfig{Inline: inline} + + h, err := New(context.Background(), http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}), cfg, "llmgw") + if err != nil { + t.Fatalf("New: want the realistic live-cluster config to load unchanged, got error: %v", err) + } + + rec := httptest.NewRecorder() + h.ServeHTTP(rec, newFederatedRequest(t, "sk-steve-cw", jsonrpcRequest{JSONRPC: jsonrpcVersion, Method: "tools/list", ID: json.RawMessage("1")})) + if rec.Code != http.StatusOK { + t.Fatalf("tools/list status = %d, want 200, body=%s", rec.Code, rec.Body.String()) + } + + usageReq := httptest.NewRequest("GET", adminUsagePath, nil) + usageReq.Header.Set("Authorization", "Bearer sk-admin1") + usageRec := httptest.NewRecorder() + h.ServeHTTP(usageRec, usageReq) + if usageRec.Code != http.StatusOK { + t.Fatalf("admin usage status = %d, want 200, body=%s", usageRec.Code, usageRec.Body.String()) + } + var usage adminUsageResponse + if err := json.Unmarshal(usageRec.Body.Bytes(), &usage); err != nil { + t.Fatalf("decode admin usage: %v", err) + } + wantUsers := len(friendNames) + len(homeNames) + 1 // + admin1 + if len(usage.Users) != wantUsers { + t.Errorf("admin usage users = %d, want %d", len(usage.Users), wantUsers) + } + if len(usage.Groups) != 2 { + t.Errorf("admin usage groups = %d, want 2", len(usage.Groups)) + } +} diff --git a/auth.go b/auth.go index c39ea74..a3a6687 100644 --- a/auth.go +++ b/auth.go @@ -323,11 +323,22 @@ func newAuthStore(cfg *Config) (*authStore, error) { } if cfg.Users != nil { + // seenNames rejects two INLINE users sharing the same name + // (security audit finding 5, 2026-08-22) — scoped to inline only, + // not cross-checked against a later file-sourced set: replaceFileUsers + // intentionally allows a file user to override an inline user of the + // same name (its own doc comment), so "duplicate name" only ever + // means "duplicate within the same source". + seenNames := make(map[string]bool, len(cfg.Users.Inline)) for _, uc := range cfg.Users.Inline { entry, err := a.buildEntry(uc) if err != nil { return nil, err } + if seenNames[entry.user.name] { + return nil, fmt.Errorf("llmgateway: duplicate user name %q", entry.user.name) + } + seenNames[entry.user.name] = true if _, exists := a.inline[entry.digest]; exists { return nil, fmt.Errorf("llmgateway: duplicate API key for user %q", uc.Name) } @@ -341,10 +352,33 @@ func newAuthStore(cfg *Config) (*authStore, error) { // buildEntry resolves uc's group and API key into an authEntry. The API key // goes through resolveSecret before digesting, so env:/file: references // work the same as for provider keys. +// +// uc.Name must be non-empty (security audit finding 5, 2026-08-22): +// counters are keyed on a user's name (windowKey's "kind:id:..." shape, +// limits.go, called with id=u.name at routes_unified.go's own call site), +// so an empty name would collapse every such user's requests-per-minute/ +// day, token, and cost counters onto the single shared key +// "llmgw:user::...", each silently overwriting the others' admission +// checks and usage — never a legitimate configuration, always a config +// mistake worth failing construction over. This check is deliberately +// NOT extended to configNamePattern's full character restriction +// (providers.go): that pattern's own doc comment ties it specifically to +// being embedded as a URL PATH SEGMENT (passthroughRoute's +// "/{provider}/*", targetRoute's "/mcp/{name}/*" and "/a2a/{name}/*") — a +// concern that does not apply to a user name, which is never used as a +// route path segment anywhere in this package, only as a counter-key +// component and a JSON field in admin views. Restricting the character +// set beyond "non-empty" would risk rejecting a real, currently-working +// operator-chosen name this round has no visibility into, for a route- +// safety reason that has no bearing on users at all — see this finding's +// own report for the full reasoning. func (a *authStore) buildEntry(uc *UserConfig) (*authEntry, error) { if uc == nil { return nil, fmt.Errorf("llmgateway: user config entry must not be nil") } + if uc.Name == "" { + return nil, fmt.Errorf("llmgateway: user config entry must have a non-empty name") + } grp, ok := a.groups[uc.Group] if !ok { return nil, fmt.Errorf("llmgateway: user %q references unknown group %q", uc.Name, uc.Group) @@ -403,11 +437,21 @@ func (a *authStore) replaceFileUsers(us []*UserConfig) error { } next[digest] = entry } + // seenFileNames rejects two FILE users sharing the same name (security + // audit finding 5, 2026-08-22) — scoped to this call's own us slice + // only, never checked against a.inline: a file user overriding an + // inline user of the same name is this function's own documented, + // intentional behavior (doc comment above), not a duplicate. + seenFileNames := make(map[string]bool, len(us)) for _, uc := range us { entry, err := a.buildEntry(uc) if err != nil { return err } + if seenFileNames[entry.user.name] { + return fmt.Errorf("llmgateway: duplicate user name %q", entry.user.name) + } + seenFileNames[entry.user.name] = true if _, exists := next[entry.digest]; exists { return fmt.Errorf("llmgateway: duplicate API key for user %q", uc.Name) } diff --git a/auth_test.go b/auth_test.go index 46030e6..1b135a8 100644 --- a/auth_test.go +++ b/auth_test.go @@ -3,6 +3,7 @@ package traefikllmgateway import ( "fmt" "net/http/httptest" + "strings" "sync" "testing" "time" @@ -719,3 +720,98 @@ func TestAuthStore_Identify_SuccessfulAuthDoesNotRecordFailure(t *testing.T) { } } } + +// --- security audit finding 5: duplicate/empty user names --- + +func TestNewAuthStore_EmptyUserName_ReturnsError(t *testing.T) { + cfg := &Config{ + Providers: map[string]*ProviderConfig{"openai": {Type: "openai", APIKey: "k"}}, + Groups: map[string]*GroupConfig{"eng": {}}, + Users: &UsersConfig{Inline: []*UserConfig{{Name: "", Group: "eng", APIKey: "sk-secret"}}}, + } + if _, err := newAuthStore(cfg); err == nil { + t.Fatal("want error for an empty user name") + } +} + +func TestNewAuthStore_DuplicateUserName_ReturnsError(t *testing.T) { + cfg := &Config{ + Providers: map[string]*ProviderConfig{"openai": {Type: "openai", APIKey: "k"}}, + Groups: map[string]*GroupConfig{"eng": {}}, + Users: &UsersConfig{Inline: []*UserConfig{ + {Name: "dup", Group: "eng", APIKey: "sk-one"}, + {Name: "dup", Group: "eng", APIKey: "sk-two"}, + }}, + } + _, err := newAuthStore(cfg) + if err == nil { + t.Fatal("want error for two inline users sharing the same name") + } + if !strings.Contains(err.Error(), "duplicate user name") { + t.Errorf("error = %q, want it to mention the duplicate-name check specifically (not the API-key one)", err.Error()) + } +} + +func TestAuthStore_ReplaceFileUsers_EmptyUserName_ReturnsError(t *testing.T) { + a, err := newAuthStore(testAuthCfg()) + if err != nil { + t.Fatal(err) + } + if err := a.replaceFileUsers([]*UserConfig{{Name: "", Group: "eng", APIKey: "sk-file1"}}); err == nil { + t.Fatal("want error for a file user with an empty name") + } +} + +func TestAuthStore_ReplaceFileUsers_DuplicateUserNameWithinFile_ReturnsError(t *testing.T) { + a, err := newAuthStore(testAuthCfg()) + if err != nil { + t.Fatal(err) + } + err = a.replaceFileUsers([]*UserConfig{ + {Name: "dup", Group: "eng", APIKey: "sk-file1"}, + {Name: "dup", Group: "eng", APIKey: "sk-file2"}, + }) + if err == nil { + t.Fatal("want error for two file users sharing the same name") + } + if !strings.Contains(err.Error(), "duplicate user name") { + t.Errorf("error = %q, want it to mention the duplicate-name check specifically (not the API-key one)", err.Error()) + } + + // The store must still be usable — the rejected replace must not have + // partially mutated it. + r := httptest.NewRequest("POST", "/v1/chat/completions", nil) + r.Header.Set("Authorization", "Bearer sk-secret") + if _, _, ok := a.identify(r); !ok { + t.Fatal("want the original inline user still resolvable after a rejected duplicate-name replace") + } +} + +// TestAuthStore_ReplaceFileUsers_SameNameAsInline_DifferentKey_OverrideAllowed +// proves the new duplicate-name check does NOT reject the intentional +// cross-source override replaceFileUsers' own doc comment describes: a +// file user of the same name as an inline user, even with a different API +// key, must still win — this is a feature (rotating/promoting a +// statically-configured user through the hot-reloadable file), not a +// duplicate. +func TestAuthStore_ReplaceFileUsers_SameNameAsInline_DifferentKey_OverrideAllowed(t *testing.T) { + a, err := newAuthStore(testAuthCfg()) // inline user "a" / sk-secret, group eng + if err != nil { + t.Fatal(err) + } + if err := a.replaceFileUsers([]*UserConfig{{Name: "a", Group: "eng", APIKey: "sk-file-a"}}); err != nil { + t.Fatalf("replaceFileUsers: want the same-name override allowed, got error: %v", err) + } + + inlineReq := httptest.NewRequest("POST", "/v1/chat/completions", nil) + inlineReq.Header.Set("Authorization", "Bearer sk-secret") + if _, _, ok := a.identify(inlineReq); ok { + t.Fatal("want the inline key overridden once the file defines a same-named user") + } + fileReq := httptest.NewRequest("POST", "/v1/chat/completions", nil) + fileReq.Header.Set("Authorization", "Bearer sk-file-a") + u, _, ok := a.identify(fileReq) + if !ok || u.name != "a" { + t.Fatalf("want the file-sourced user resolvable under the overridden name, got %v ok=%v", u, ok) + } +} From 3536c326678dc0d2631ef04caf9b2158ac36c678 Mon Sep 17 00:00:00 2001 From: Lukasz Raczylo Date: Sat, 22 Aug 2026 14:56:42 +0100 Subject: [PATCH 6/8] fix(auth): let a valid key always win over IP throttle MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes critical finding 1 from security review round 2, 2026-08-22: identify checked the per-source-IP throttle BEFORE verifying the presented key. This plugin runs behind a Kubernetes Service, and with externalTrafficPolicy: Cluster (this operator's own deployment), r.RemoteAddr is the node's own SNAT address shared by every external caller reaching that node — not a per-tenant address. 20 failed requests from anywhere on the internet sharing that node locked out all 10 live users, including the admin dashboard's own poll. - identify now verifies the key FIRST, unconditionally: a valid key always succeeds before any throttle state is read. Every failure path still records against the caller's IP, but throttling is now pure observability (bounding log volume and tracker memory), never enforcement — an attacker with no valid key already got 'false' either way, so nothing is lost against brute force. - Replaces the per-source-IP tracker's backing store: the general- purpose memoryStore (limits.go) has no size cap and a full mutex-held sweep that scales with tracked-IP count (~83ms at 1M IPs). authFailureTracker is a bounded, two-generation, O(1)-rotation structure capped at 2x50,000 entries (important finding 1). - logAuthEvent now emits a distinct, separately rate-limited 'auth throttle:' line the first time a source crosses the limit (important finding 5), and both the routine and throttle-engaged lines report how many events they suppressed since the last line actually written, so several distinct attacker IPs in one window no longer collapse into a single indistinguishable line (important finding 6). - The success path no longer touches the throttle tracker's mutex at all (important finding 2). --- auth.go | 373 +++++++++++++++++++++++++++++++++---------------- auth_test.go | 174 ++++++++++++++++------- logger.go | 61 +++++--- logger_test.go | 58 ++++++++ 4 files changed, 480 insertions(+), 186 deletions(-) diff --git a/auth.go b/auth.go index a3a6687..c128088 100644 --- a/auth.go +++ b/auth.go @@ -189,38 +189,162 @@ type authEntry struct { } // authFailureWindow and authFailureLimit bound how many failed identify -// attempts one source IP may make before further attempts from it are -// throttled outright (security audit finding 3, 2026-08-22): identify -// previously had no counter, no backoff, and no lockout at all — an -// unauthenticated caller got unlimited digest-lookup attempts at line -// rate. 20 failures per minute is generous headroom for a legitimate -// client with a stale or mistyped key retried a few times (a human -// pasting the wrong key, a misconfigured script cycling through a couple -// of candidates) while still meaningfully bounding a scripted guesser's -// throughput per source IP. This does not, and cannot, defend the -// underlying key space on its own (a SHA-256 digest lookup already makes -// brute force infeasible) — its real job is capping the synchronous cost -// an unauthenticated caller can impose on the shared Traefik ingress per -// unit time: identify's own digest lookup, and logAuthEvent's stderr -// write (logger.go), both of which this finding's own framing calls out. +// attempts one source IP may make within a window before this package +// treats it as "throttled" for OBSERVABILITY purposes only (security +// audit finding 3, 2026-08-22; revised by security review round 2, +// 2026-08-22, critical finding 1 — see identify's own doc comment for +// why this is observability, not enforcement, post-revision). 20 +// failures per minute is generous headroom for a legitimate client with +// a stale or mistyped key retried a few times, while still flagging a +// source failing fast enough to be worth an operator's attention. const ( authFailureWindow = time.Minute authFailureLimit = 20 - // authFailureLogEvery rate-limits logAuthEvent's own failed-auth log - // line (logger.go) the identical way limiter.logStoreError (limits.go) - // already rate-limits store-error lines — sharing that function's own - // storeErrorLogEvery interval keeps this codebase's "how often do we - // log a sustained failure condition" cadence consistent across both - // call sites, rather than picking a new, unrelated number here. + // authFailureLogEvery rate-limits BOTH logAuthEvent's routine + // failed-auth line and its distinct throttle-engaged line (logger.go) + // — the identical technique limiter.logStoreError (limits.go) already + // applies to store-error lines, sharing that function's own + // storeErrorLogEvery interval for cadence consistency. authFailureLogEvery = storeErrorLogEvery ) -// authFailureKeyPrefix namespaces authStore's failure-tracking keys in its -// own memoryStore instance (failures, below) — a plain string key, not -// windowKey's (kind, id, metric, window) tuple (limits.go), since auth -// failures are tracked per SOURCE IP, a concept the rest of this -// package's counter keying was never built to express. -const authFailureKeyPrefix = "authfail:" +// authFailureMapCap bounds authFailureTracker's total tracked-source-IP +// count across both its generations (security review round 2, +// 2026-08-22, important finding 1): a distributed attacker sending +// failed attempts from many distinct IPs could otherwise grow the +// tracker's memory without any limit, and a naive full-map sweep to +// expire old entries would hold its lock for a duration that scales with +// tracked-IP count (measured, on the general-purpose memoryStore this +// field used before this revision: ~83ms mutex-held at 1M tracked IPs, +// blocking every identify call for that duration). authFailureTracker's +// own two-generation rotation (its doc comment below) bounds total memory +// to at most 2×authFailureMapCap entries with an O(1) rotation, never a +// scan. +const authFailureMapCap = 50_000 + +// authFailureEntry is one source IP's failure count within +// authFailureTracker, expiring authFailureWindow after its first failure +// in the current generation. +type authFailureEntry struct { + expiry time.Time + count int64 +} + +// authFailureTracker is a bounded, two-generation per-source-IP failed- +// auth-attempt counter (security review round 2, 2026-08-22, important +// finding 1) — deliberately NOT the general-purpose memoryStore +// (limits.go): memoryStore's map has no size cap at all and its +// sweepLocked does a full, mutex-held O(n) scan, which is fine for the +// bounded-by-configuration scope space checkAndCount/account use (one +// entry per configured user/group) but wrong for a value with no +// configured upper bound like source IP. +// +// current is the active generation every increment/read goes to first; +// previous is the immediately-prior generation, consulted as a read-only +// fallback so an IP's count does not spuriously read as zero the instant +// after a rotation. Rotation — current becomes previous, a fresh empty +// map becomes current — is a two-pointer swap (O(1), never a scan), +// triggered on inserting a brand-new IP once current is already at +// authFailureMapCap; total memory is bounded to at most +// 2×authFailureMapCap entries. An IP whose entry is displaced by a +// rotation simply starts a fresh count on its next failure — the same +// outcome a normal authFailureWindow rollover already produces for any +// tracked IP; this is accepted, lossy-under-extreme-load behavior, the +// same trade-off limiter.spawnTokens' own doc comment (limits.go) +// accepts for a different bounded resource. +type authFailureTracker struct { + current map[string]*authFailureEntry + previous map[string]*authFailureEntry + nowFn func() time.Time + mu sync.Mutex +} + +// newAuthFailureTracker returns an empty tracker using nowFn for its +// clock — a func value, not a copied time.Time, so a caller that later +// swaps what nowFn points to (authStore's own fakeClock-in-tests +// convention) is honored on every subsequent call. +func newAuthFailureTracker(nowFn func() time.Time) *authFailureTracker { + return &authFailureTracker{current: make(map[string]*authFailureEntry), nowFn: nowFn} +} + +// get returns ip's current failure count, or 0 if it is untracked or its +// window has expired. Checks current first, then previous. +func (t *authFailureTracker) get(ip string) int64 { + now := t.nowFn() + t.mu.Lock() + defer t.mu.Unlock() + if e, ok := t.current[ip]; ok && now.Before(e.expiry) { + return e.count + } + if e, ok := t.previous[ip]; ok && now.Before(e.expiry) { + return e.count + } + return 0 +} + +// increment records one failure for ip and returns its new count for the +// current window, creating a fresh entry in current if ip is untracked in +// current or its own entry there already expired — a plain fixed-window +// counter, matching memoryStore.incrBy's own contract (limits.go) in +// spirit. Rotates current -> previous before inserting a brand-new ip +// once current is already at authFailureMapCap — see this type's own doc +// comment for why that bounds total memory without ever scanning. +func (t *authFailureTracker) increment(ip string) int64 { + now := t.nowFn() + t.mu.Lock() + defer t.mu.Unlock() + + if e, ok := t.current[ip]; ok { + if now.Before(e.expiry) { + e.count++ + return e.count + } + e.count = 1 + e.expiry = now.Add(authFailureWindow) + return 1 + } + if len(t.current) >= authFailureMapCap { + t.previous = t.current + t.current = make(map[string]*authFailureEntry, authFailureMapCap) + } + t.current[ip] = &authFailureEntry{count: 1, expiry: now.Add(authFailureWindow)} + return 1 +} + +// logGate rate-limits a single log line to once per authFailureLogEvery, +// counting how many calls were suppressed in between so the next actually +// -written line can report them (security review round 2, 2026-08-22, +// important finding 6): a single global rate-limit gate would otherwise +// make several DISTINCT attacker IPs failing within one interval look +// identical to a single noisy one — three separate IPs each failing once +// in a 30s window previously produced only ONE log line total, with no +// sign two more existed. Reporting the suppressed count in the next line +// restores that visibility without tracking a per-IP gate (an LRU of +// recently-logged IPs was the documented alternative; a suppressed count +// is simpler and gives an operator the same "something else happened" +// signal). Two independent zero-value-usable instances back authStore's +// routine-failure and throttle-engaged lines (below) so a burst of one +// can never suppress, or be suppressed by, the other. The very first call +// on a fresh gate always logs (lastLogAt's zero value is more than +// authFailureLogEvery before any real now). +type logGate struct { + lastLogAt time.Time + suppressed int64 + mu sync.Mutex +} + +func (g *logGate) shouldLog(now time.Time) (log bool, suppressed int64) { + g.mu.Lock() + defer g.mu.Unlock() + if !g.lastLogAt.IsZero() && now.Sub(g.lastLogAt) < authFailureLogEvery { + g.suppressed++ + return false, 0 + } + suppressed = g.suppressed + g.suppressed = 0 + g.lastLogAt = now + return true, suppressed +} // authStore identifies requests by API key and resolves them to a user and // their group. Raw API keys are never retained after construction — only @@ -235,33 +359,31 @@ const authFailureKeyPrefix = "authfail:" // serializes maybeReload's throttle check and the reload it may trigger. // // failures is identify's own per-source-IP failed-attempt counter -// (security audit finding 3, 2026-08-22) — the SAME in-process memoryStore -// type limiter's fallback store already uses (limits.go), a deliberately -// reused type rather than a new dependency, given its own TTL-bucketed -// counter semantics already fit this need exactly. It is authStore's own -// dedicated instance, not limiter.fallback: auth failures are a distinct -// concept (throttling an unauthenticated caller) from LLM traffic limits -// (throttling an authenticated one's usage), and authStore must work -// identically whether or not a Gateway even has a limiter wired yet. -// authFailLogMu/authFailLastLogAt back shouldLogAuthFailure, the identical -// "guard a timestamp with a small mutex, rate-limit a log line" shape -// limiter.logMu/lastLogAt already use. +// (security audit finding 3, 2026-08-22; failureLog/throttleLog added by +// security review round 2, 2026-08-22) — see identify's own doc comment +// for why this is pure observability, never enforcement, and +// authFailureTracker's own doc comment for why it is not the +// general-purpose memoryStore (limits.go). It is authStore's own +// dedicated instance: auth failures are a distinct concept from LLM +// traffic limits, and authStore must work identically whether or not a +// Gateway even has a limiter wired yet. failureLog/throttleLog back +// logAuthEvent's two distinct rate-limited lines (logger.go). type authStore struct { - groups map[string]*group - inline map[[32]byte]*authEntry // built once by newAuthStore; never mutated afterward - byDigest map[[32]byte]*authEntry // inline entries plus the current file-sourced set; guarded by mu - usersFile *usersFile // nil when Users.File is not configured; maybeReload no-ops - log gatewayLogger // set alongside usersFile; unused when usersFile is nil - nowFn func() time.Time // injected for tests; defaults to time.Now - failures *memoryStore // per-source-IP failed-identify counters; see doc comment above - lastCheck time.Time // guarded by reloadMu - lastModTime time.Time // guarded by reloadMu - authFailLastLogAt time.Time // guarded by authFailLogMu; last time a failed-auth line was logged - fileUserCount int // guarded by mu; size of the current file-sourced user set - mu sync.RWMutex - buildMu sync.Mutex - reloadMu sync.Mutex - authFailLogMu sync.Mutex + lastModTime time.Time + lastCheck time.Time + log gatewayLogger + failures *authFailureTracker + usersFile *usersFile + nowFn func() time.Time + groups map[string]*group + byDigest map[[32]byte]*authEntry + inline map[[32]byte]*authEntry + failureLog logGate + throttleLog logGate + fileUserCount int + mu sync.RWMutex + buildMu sync.Mutex + reloadMu sync.Mutex } // newAuthStore builds an authStore from cfg's groups and inline users. It @@ -274,13 +396,12 @@ func newAuthStore(cfg *Config) (*authStore, error) { byDigest: make(map[[32]byte]*authEntry), nowFn: time.Now, } - a.failures = newMemoryStore() // Delegate through a.nowFn rather than copying it: a test that // overrides a.nowFn AFTER construction (the fakeClock convention // newReloadableAuthStore already uses, users_file_test.go) must still // control failures' own clock — this closure reads a.nowFn fresh on // every call, so it always sees whichever func is currently assigned. - a.failures.nowFn = func() time.Time { return a.nowFn() } + a.failures = newAuthFailureTracker(func() time.Time { return a.nowFn() }) for name, gc := range cfg.Groups { if gc == nil { return nil, fmt.Errorf("llmgateway: group %q: config must not be nil", name) @@ -487,47 +608,51 @@ func clientIP(r *http.Request) string { return host } -// authThrottled reports whether ip has already reached authFailureLimit -// failed identify attempts within the current authFailureWindow — a -// read-only check, so a throttled IP's later attempts cost nothing beyond -// one in-process map read, never a digest computation or byDigest lookup. +// authThrottled reports whether ip has reached authFailureLimit failed +// identify attempts within the current authFailureWindow — a read-only +// observability signal for logAuthEvent's distinct throttle-engaged line +// (logger.go). identify itself never consults this (security review +// round 2, 2026-08-22, critical finding 1 — see identify's own doc +// comment for why). func (a *authStore) authThrottled(ip string) bool { - count, _ := a.failures.get(authFailureKeyPrefix + ip) // memoryStore.get never errors - return count >= authFailureLimit + return a.failures.get(ip) >= authFailureLimit } // recordAuthFailure increments ip's failure count for the current -// authFailureWindow, creating a fresh window if the previous one already -// expired (memoryStore.incrBy's own fixed-window contract, limits.go). +// authFailureWindow (authFailureTracker.increment's own fixed-window +// contract). Pure observability post security review round 2's critical +// finding 1: it never influences identify's own return value (see +// identify's doc comment) — only authThrottled's read, for +// logAuthEvent's distinct throttle-engaged line. func (a *authStore) recordAuthFailure(ip string) { - _, _ = a.failures.incrBy(authFailureKeyPrefix+ip, 1, authFailureWindow) // memoryStore.incrBy never errors + a.failures.increment(ip) } -// shouldLogAuthFailure reports whether the next failed-auth log line -// (logAuthEvent, logger.go) should actually be written, rate-limited to -// once per authFailureLogEvery regardless of how many attempts fail in -// between — the identical technique limiter.logStoreError (limits.go) -// already applies to store errors, applied here so an attacker driving -// unlimited failed-auth attempts can never drive unlimited synchronous -// stderr writes on the shared Traefik ingress (security audit finding 3, -// 2026-08-22) even before authThrottled's own per-IP cap engages (a -// distributed attempt spread across many source IPs never trips any -// single IP's own threshold, but must still not flood the log). -// Successful-auth logging is deliberately left unbounded: it requires an -// already-valid API key, so its volume is bounded by legitimate -// authenticated traffic — itself already governed by checkAndCount's -// requests-per-minute/day limits elsewhere — not by an unauthenticated -// caller's attempt rate, which is exactly what this finding is about -// bounding. -func (a *authStore) shouldLogAuthFailure() bool { - now := a.nowFn() - a.authFailLogMu.Lock() - defer a.authFailLogMu.Unlock() - if now.Sub(a.authFailLastLogAt) < authFailureLogEvery { - return false - } - a.authFailLastLogAt = now - return true +// shouldLogAuthFailure reports whether the next ROUTINE failed-auth log +// line (logAuthEvent, logger.go) should actually be written, and how many +// prior failure events were suppressed since the last line actually +// wrote (logGate's own doc comment covers the suppressed-count +// reasoning) — the same rate-limiting technique limiter.logStoreError +// (limits.go) already applies to store-error lines, applied here so an +// attacker driving unlimited failed-auth attempts can never drive +// unlimited synchronous stderr writes on the shared Traefik ingress +// (security audit finding 3, 2026-08-22). Successful-auth logging is +// deliberately left unbounded: it requires an already-valid API key, so +// its volume is bounded by legitimate authenticated traffic — itself +// already governed by checkAndCount's requests-per-minute/day limits +// elsewhere — not by an unauthenticated caller's attempt rate, which is +// exactly what this finding is about bounding. +func (a *authStore) shouldLogAuthFailure() (log bool, suppressed int64) { + return a.failureLog.shouldLog(a.nowFn()) +} + +// shouldLogThrottleEngaged is shouldLogAuthFailure's counterpart for the +// distinct "this source IP is currently throttled" line (security review +// round 2, 2026-08-22, important finding 5) — its own separate rate-limit +// gate (logGate's own doc comment), so a burst of routine failure lines +// can never suppress, or be suppressed by, this higher-signal line. +func (a *authStore) shouldLogThrottleEngaged() (log bool, suppressed int64) { + return a.throttleLog.shouldLog(a.nowFn()) } // identify resolves r's presented API key to a user and their group. It @@ -536,40 +661,50 @@ func (a *authStore) shouldLogAuthFailure() bool { // is looked up by SHA-256 digest, then verified with a constant-time // compare — the key itself is never retained. // -// Per-source-IP throttling (security audit finding 3, 2026-08-22): a -// request from an IP already over authThrottled's own budget is rejected -// immediately, before any digest work at all. Every OTHER failure path -// below — no key presented, key not found, or a (practically unreachable, -// see subtle.ConstantTimeCompare's own call site) digest mismatch — -// records one failure for the caller's IP via recordAuthFailure. A -// successful identify never records a failure and never resets a prior -// count: this is a rate limit on failed attempts, not a lockout a -// legitimate request can clear early. +// The presented key is verified FIRST, UNCONDITIONALLY (security review +// round 2, 2026-08-22, critical finding 1): a request carrying a VALID +// key always succeeds, before any per-source-IP throttle state is even +// read. The prior revision of this function checked authThrottled BEFORE +// verifying the key at all — but this plugin runs as a Traefik middleware +// behind a Kubernetes Service, and with externalTrafficPolicy: Cluster +// (this operator's own live deployment), r.RemoteAddr is the NODE's own +// SNAT address, shared by every external caller reaching that node, not +// a per-tenant address. Confirmed against this operator's own 10 live +// users: 20 failed requests from anywhere on the internet, sharing that +// node, locked out every one of them, including the admin dashboard's own +// poll. Verifying the key first makes that impossible — a legitimate, +// already-valid request can never be rejected by another caller's failed +// attempts, from the same address or not. +// +// Every failure path below — no key presented, key not found, or a +// (practically unreachable, see subtle.ConstantTimeCompare's own call +// site) digest mismatch — records one failure for the caller's source IP +// via recordAuthFailure; logAuthEvent (logger.go) separately decides, per +// call, whether to emit a routine failure line or a distinct throttle- +// engaged line by consulting authThrottled itself. No branch in this +// function ever changes its own return value based on throttle state — +// throttling is pure observability now (bounding the log's write volume +// and the tracker's own memory; authFailureTracker's doc comment, +// above), never enforcement: an attacker with no valid key gains nothing +// from this change (every failure path here already returned false +// either way, throttled or not), and the raw per-attempt cost this +// function pays (one SHA-256 digest, one map lookup, one +// ConstantTimeCompare) is already cheap enough — nanoseconds — that +// gating it early bought negligible protection against the cost itself, +// only the false-positive risk the paragraph above describes. func (a *authStore) identify(r *http.Request) (*user, *group, bool) { - ip := clientIP(r) - if a.authThrottled(ip) { - return nil, nil, false - } - key, ok := presentedKey(r) - if !ok { - a.recordAuthFailure(ip) - return nil, nil, false + if ok { + digest := sha256.Sum256([]byte(key)) + a.mu.RLock() + entry, found := a.byDigest[digest] + a.mu.RUnlock() + if found && subtle.ConstantTimeCompare(digest[:], entry.digest[:]) == 1 { + return entry.user, entry.group, true + } } - digest := sha256.Sum256([]byte(key)) - - a.mu.RLock() - entry, ok := a.byDigest[digest] - a.mu.RUnlock() - if !ok { - a.recordAuthFailure(ip) - return nil, nil, false - } - if subtle.ConstantTimeCompare(digest[:], entry.digest[:]) != 1 { - a.recordAuthFailure(ip) - return nil, nil, false - } - return entry.user, entry.group, true + a.recordAuthFailure(clientIP(r)) + return nil, nil, false } // presentedKey extracts the API key from r's Authorization Bearer header or diff --git a/auth_test.go b/auth_test.go index 1b135a8..39dba7d 100644 --- a/auth_test.go +++ b/auth_test.go @@ -597,83 +597,157 @@ func TestClientIP_MalformedRemoteAddr_UsedAsIs(t *testing.T) { } } -// TestAuthStore_Identify_ThrottlesAfterRepeatedFailures_EvenValidKeyRejected -// is the core regression for finding 3: identify previously had no -// counter, no backoff, and no lockout at all. After authFailureLimit -// failed attempts from one source IP, even a subsequently CORRECT key -// from that same IP must be rejected — proving the throttle actually -// short-circuits real auth once tripped, not merely that repeated wrong -// keys keep failing (which they would anyway). A different source IP must -// be entirely unaffected, proving the throttle is per-IP, not global. -func TestAuthStore_Identify_ThrottlesAfterRepeatedFailures_EvenValidKeyRejected(t *testing.T) { - a, err := newAuthStore(testAuthCfg()) // inline user "a" / sk-secret +// TestAuthStore_Identify_ValidKeyAlwaysSucceeds_EvenWhenIPThrottled is the +// core regression for security review round 2's critical finding 1: this +// plugin runs as a Traefik middleware behind a Kubernetes Service — with +// externalTrafficPolicy: Cluster, r.RemoteAddr is the NODE's own SNAT +// address, shared by every external caller reaching that node, not a +// per-tenant one. The prior revision of identify checked the per-IP +// throttle BEFORE verifying the key, so 20 failed requests from anywhere +// sharing that node locked out every legitimate tenant too. After well +// past authFailureLimit failed attempts from one source IP, a SUBSEQUENT +// VALID key from that EXACT same IP must still succeed immediately. +func TestAuthStore_Identify_ValidKeyAlwaysSucceeds_EvenWhenIPThrottled(t *testing.T) { + a, err := newAuthStore(testAuthCfg()) // inline user "a" / sk-secret, group eng if err != nil { t.Fatal(err) } - const attackerIP = "203.0.113.9:54321" + const sharedIP = "203.0.113.9:54321" - for i := 0; i < authFailureLimit; i++ { + for i := 0; i < authFailureLimit*2; i++ { // well past the threshold r := httptest.NewRequest("POST", "/v1/chat/completions", nil) - r.RemoteAddr = attackerIP + r.RemoteAddr = sharedIP r.Header.Set("x-api-key", "wrong-key") - if _, _, ok := a.identify(r); ok { - t.Fatalf("attempt %d: want failure for a wrong key, got success", i) - } + a.identify(r) + } + // Sanity: the tracker really did cross the threshold — otherwise this + // test would not actually exercise the throttled-IP path at all. + if !a.authThrottled("203.0.113.9") { + t.Fatal("test setup: expected the tracker to be over authFailureLimit for the shared IP") } - // Throttle should now be engaged for attackerIP — even the CORRECT - // key must be rejected. valid := httptest.NewRequest("POST", "/v1/chat/completions", nil) - valid.RemoteAddr = attackerIP + valid.RemoteAddr = sharedIP valid.Header.Set("Authorization", "Bearer sk-secret") - if _, _, ok := a.identify(valid); ok { - t.Fatal("want a throttled IP rejected even with a valid key") - } - - // A different source IP must be unaffected by attackerIP's throttle. - other := httptest.NewRequest("POST", "/v1/chat/completions", nil) - other.RemoteAddr = "198.51.100.5:1111" - other.Header.Set("Authorization", "Bearer sk-secret") - if _, _, ok := a.identify(other); !ok { - t.Fatal("want a different source IP unaffected by another IP's throttle") + u, grp, ok := a.identify(valid) + if !ok || u.name != "a" || grp.name != "eng" { + t.Fatalf("want a valid key to succeed even from a throttled source IP, got user=%v group=%v ok=%v", u, grp, ok) } } -// TestAuthStore_Identify_ThrottleResetsAfterWindowExpires proves the -// throttle is a fixed-window rate limit, not a permanent lockout: once -// authFailureWindow has actually elapsed, the same IP's valid key works -// again. -func TestAuthStore_Identify_ThrottleResetsAfterWindowExpires(t *testing.T) { - a, err := newAuthStore(testAuthCfg()) // inline user "a" / sk-secret +// TestAuthStore_AuthThrottled_TracksPerIPIndependently proves +// authFailureTracker's own counting (used only by logAuthEvent's distinct +// throttle-engaged line now, logger.go — never by identify's return +// value, see identify's own doc comment) is still scoped per source IP: +// one IP crossing the threshold must never affect a different IP's own +// count. +func TestAuthStore_AuthThrottled_TracksPerIPIndependently(t *testing.T) { + a, err := newAuthStore(testAuthCfg()) + if err != nil { + t.Fatal(err) + } + for i := 0; i < authFailureLimit; i++ { + r := httptest.NewRequest("POST", "/v1/chat/completions", nil) + r.RemoteAddr = "203.0.113.9:1" + r.Header.Set("x-api-key", "wrong-key") + a.identify(r) + } + if !a.authThrottled("203.0.113.9") { + t.Error("want 203.0.113.9 throttled after authFailureLimit failures") + } + if a.authThrottled("198.51.100.5") { + t.Error("want a different, never-failing IP to remain unthrottled") + } +} + +// TestAuthStore_AuthThrottled_ResetsAfterWindowExpires proves +// authFailureTracker is a fixed-window counter, not a permanent record: +// once authFailureWindow has elapsed, a source IP's earlier failures no +// longer count toward authThrottled. +func TestAuthStore_AuthThrottled_ResetsAfterWindowExpires(t *testing.T) { + a, err := newAuthStore(testAuthCfg()) if err != nil { t.Fatal(err) } clock := &fakeClock{now: time.Unix(1_700_000_000, 0)} a.nowFn = clock.Now - const attackerIP = "203.0.113.9:54321" - for i := 0; i < authFailureLimit; i++ { r := httptest.NewRequest("POST", "/v1/chat/completions", nil) - r.RemoteAddr = attackerIP + r.RemoteAddr = "203.0.113.9:1" r.Header.Set("x-api-key", "wrong-key") - if _, _, ok := a.identify(r); ok { - t.Fatalf("attempt %d: want failure for a wrong key, got success", i) - } + a.identify(r) } - blocked := httptest.NewRequest("POST", "/v1/chat/completions", nil) - blocked.RemoteAddr = attackerIP - blocked.Header.Set("Authorization", "Bearer sk-secret") - if _, _, ok := a.identify(blocked); ok { + if !a.authThrottled("203.0.113.9") { t.Fatal("want throttled before the window elapses") } clock.Advance(authFailureWindow + time.Second) - recovered := httptest.NewRequest("POST", "/v1/chat/completions", nil) - recovered.RemoteAddr = attackerIP - recovered.Header.Set("Authorization", "Bearer sk-secret") - if _, _, ok := a.identify(recovered); !ok { - t.Fatal("want the same IP's valid key to work again once authFailureWindow has elapsed") + if a.authThrottled("203.0.113.9") { + t.Error("want the throttle cleared once authFailureWindow has elapsed") + } +} + +// --- authFailureTracker: direct unit tests --- + +func TestAuthFailureTracker_IncrementAndGet(t *testing.T) { + now := time.Unix(1_700_000_000, 0) + tr := newAuthFailureTracker(func() time.Time { return now }) + + if got := tr.get("1.2.3.4"); got != 0 { + t.Errorf("get on untracked ip = %d, want 0", got) + } + for i := int64(1); i <= 5; i++ { + if got := tr.increment("1.2.3.4"); got != i { + t.Errorf("increment #%d = %d, want %d", i, got, i) + } + } + if got := tr.get("1.2.3.4"); got != 5 { + t.Errorf("get after 5 increments = %d, want 5", got) + } +} + +func TestAuthFailureTracker_WindowExpiry(t *testing.T) { + clock := &fakeClock{now: time.Unix(1_700_000_000, 0)} + tr := newAuthFailureTracker(clock.Now) + + tr.increment("1.2.3.4") + tr.increment("1.2.3.4") + if got := tr.get("1.2.3.4"); got != 2 { + t.Fatalf("get = %d, want 2", got) + } + + clock.Advance(authFailureWindow + time.Second) + if got := tr.get("1.2.3.4"); got != 0 { + t.Errorf("get after window expiry = %d, want 0", got) + } + if got := tr.increment("1.2.3.4"); got != 1 { + t.Errorf("increment after window expiry = %d, want 1 (fresh window)", got) + } +} + +// TestAuthFailureTracker_BoundsMemoryViaRotation is the regression for +// security review round 2's important finding 1: the tracker must never +// grow past 2×authFailureMapCap total entries — filling it past +// authFailureMapCap distinct IPs must trigger an O(1) generation +// rotation, not unbounded growth. +func TestAuthFailureTracker_BoundsMemoryViaRotation(t *testing.T) { + now := time.Unix(1_700_000_000, 0) + tr := newAuthFailureTracker(func() time.Time { return now }) + + for i := 0; i < authFailureMapCap+10; i++ { + tr.increment(fmt.Sprintf("10.%d.%d.%d", i/65536, (i/256)%256, i%256)) + } + + tr.mu.Lock() + total := len(tr.current) + len(tr.previous) + currentLen := len(tr.current) + tr.mu.Unlock() + if total > 2*authFailureMapCap { + t.Errorf("tracked entries = %d, want <= %d (2x cap)", total, 2*authFailureMapCap) + } + if currentLen > authFailureMapCap { + t.Errorf("current generation = %d entries, want <= %d", currentLen, authFailureMapCap) } } diff --git a/logger.go b/logger.go index 4c9097a..9426105 100644 --- a/logger.go +++ b/logger.go @@ -19,32 +19,59 @@ func (g *Gateway) errorf(format string, args ...any) { } // logAuthEvent logs one authentication attempt, at the point one of -// authStore.identify's six call sites resolves it. A failure logs the -// method, path, and remote address the request came from; a success logs -// the resolved user's name alongside the same method and path. Neither -// line ever includes the presented API key itself — identify resolves a -// key to a *user and hands this function only that user's name, never the -// key it authenticated. +// authStore.identify's six call sites resolves it. A success logs the +// resolved user's name alongside the request's method and path. Neither +// line this function ever writes includes the presented API key itself — +// identify resolves a key to a *user and hands this function only that +// user's name, never the key it authenticated. // -// The failure line is rate-limited via g.auth.shouldLogAuthFailure -// (auth.go), the same technique limiter.logStoreError (limits.go) already -// applies to store-error lines (security audit finding 3, 2026-08-22): an -// unauthenticated caller driving unlimited failed attempts must never be -// able to drive unlimited synchronous stderr writes on the shared Traefik -// ingress — a slow log pipe would otherwise stall request handling for -// every tenant, not just the one failing auth. The success line stays -// unbounded — see shouldLogAuthFailure's own doc comment for why that is -// safe. +// A failure logs one of two distinct lines (security review round 2, +// 2026-08-22, important finding 5): if the caller's source IP +// (clientIP) is currently authThrottled, a distinct "throttle engaged" +// line naming the IP and the limit it exceeded — an operator seeing this +// knows immediately that a SOURCE, not just an individual request, needs +// attention, which the routine per-attempt line alone does not convey. +// Otherwise the routine failure line (method, path, remote address). +// Both lines are independently rate-limited (authStore.shouldLog +// ThrottleEngaged / shouldLogAuthFailure, auth.go — the identical +// technique limiter.logStoreError, limits.go, already applies to +// store-error lines) so an unauthenticated caller driving unlimited +// failed attempts can never drive unlimited synchronous stderr writes on +// the shared Traefik ingress — a slow log pipe would otherwise stall +// request handling for every tenant, not just the one failing auth. Each +// gate reports how many events it suppressed since its last written line +// (security review round 2, important finding 6): a single global gate +// would otherwise make several DISTINCT attacker IPs failing within one +// interval indistinguishable from one noisy IP — suppressedSuffix +// appends that count to the line when non-zero. The success line stays +// completely unbounded — see shouldLogAuthFailure's own doc comment for +// why that is safe. func (g *Gateway) logAuthEvent(ok bool, userName string, r *http.Request) { if !ok { - if g.auth.shouldLogAuthFailure() { - g.logf("auth failed: %s %q from %s", r.Method, r.URL.Path, r.RemoteAddr) + ip := clientIP(r) + if g.auth.authThrottled(ip) { + if log, suppressed := g.auth.shouldLogThrottleEngaged(); log { + g.logf("auth throttle: source %s exceeded %d failed attempts in the last %s%s", ip, authFailureLimit, authFailureWindow, suppressedSuffix(suppressed)) + } + return + } + if log, suppressed := g.auth.shouldLogAuthFailure(); log { + g.logf("auth failed: %s %q from %s%s", r.Method, r.URL.Path, r.RemoteAddr, suppressedSuffix(suppressed)) } return } g.logf("auth ok: user %q %s %q", userName, r.Method, r.URL.Path) } +// suppressedSuffix formats n (a logGate's own suppressed-event count, +// auth.go) as a log-line suffix, or "" when there is nothing to report. +func suppressedSuffix(n int64) string { + if n <= 0 { + return "" + } + return fmt.Sprintf(" (%d more suppressed since last log)", n) +} + // authEventUserName extracts u's name for logAuthEvent's userName // argument, or "" when u is nil — identify's failure path, which never // resolves a user. diff --git a/logger_test.go b/logger_test.go index cc9bafb..ff2434f 100644 --- a/logger_test.go +++ b/logger_test.go @@ -122,3 +122,61 @@ func TestLogAuthEvent_Success_NeverRateLimited(t *testing.T) { t.Fatalf("auth-ok lines = %d, want 5 (success logging must never be rate-limited), output=%q", n, out) } } + +// TestLogAuthEvent_ThrottleEngaged_DistinctLine proves logAuthEvent emits +// a distinct "auth throttle:" line, not the routine "auth failed:" one, +// once a source IP has crossed authFailureLimit (security review round +// 2, 2026-08-22, important finding 5) — an operator seeing this line +// knows a SOURCE, not just one request, needs attention. +func TestLogAuthEvent_ThrottleEngaged_DistinctLine(t *testing.T) { + gw := newTestGatewayForLogger(t) + clock := &fakeClock{now: time.Unix(1_700_000_000, 0)} + gw.auth.nowFn = clock.Now + + r := httptest.NewRequest("POST", "/v1/chat/completions", nil) + r.RemoteAddr = "203.0.113.9:1234" + for i := 0; i < authFailureLimit; i++ { + gw.auth.recordAuthFailure(clientIP(r)) + } + + out := captureStderr(t, func() { + gw.logAuthEvent(false, "", r) + }) + + if !strings.Contains(out, "auth throttle:") { + t.Errorf("want an 'auth throttle:' line once the IP has crossed the limit, got %q", out) + } + if strings.Contains(out, "auth failed:") { + t.Errorf("want the routine 'auth failed:' line suppressed in favor of the distinct throttle line, got %q", out) + } +} + +// TestLogAuthEvent_Failure_SuppressedCountReported proves the routine +// failure line's rate-limit gate reports how many events it suppressed +// (security review round 2, 2026-08-22, important finding 6): a single +// global gate would otherwise make several rapid failures within one +// window indistinguishable from a single one — the next line actually +// written must name how many were collapsed into it. +func TestLogAuthEvent_Failure_SuppressedCountReported(t *testing.T) { + gw := newTestGatewayForLogger(t) + clock := &fakeClock{now: time.Unix(1_700_000_000, 0)} + gw.auth.nowFn = clock.Now + + r := httptest.NewRequest("POST", "/v1/chat/completions", nil) + r.RemoteAddr = "203.0.113.9:1234" + + captureStderr(t, func() { + gw.logAuthEvent(false, "", r) // logged (first call on a fresh gate) + gw.logAuthEvent(false, "", r) // suppressed #1 + gw.logAuthEvent(false, "", r) // suppressed #2 + }) + + clock.Advance(authFailureLogEvery + time.Second) + out := captureStderr(t, func() { + gw.logAuthEvent(false, "", r) + }) + + if !strings.Contains(out, "2 more suppressed since last log") { + t.Errorf("want the next line to report 2 suppressed events, got %q", out) + } +} From 452be5151c6fc7a45f07965839f1a22840f4b226 Mon Sep 17 00:00:00 2001 From: Lukasz Raczylo Date: Sat, 22 Aug 2026 14:57:29 +0100 Subject: [PATCH 7/8] fix(mcp): stop weighting tools/list budget, widen fan-out, split caps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses critical finding 3 and important findings 3-4 from security review round 2, 2026-08-22. Critical finding 3 — the tools/list-per-backend weight charged against the caller's own requests-per-minute/day budget (security audit finding 1b) was not default-preserving: both live groups have an empty MCP allow-list (all 11 servers), so it silently cut every configured requests-per-minute budget by ~11x with no config change, and examples/kubernetes.yaml's shipped requests-per-minute: 10 would 429 the very first tools/list call ever made. Reverted: checkAndCountWeighted removed from limits.go, handleMCPFederated back to a plain checkAndCount. The real amplification stays fully visible to an operator via countTargetRequests' existing per-server 'mcp' target-scope counters — a separate scope from the user/group budget — rather than silently consuming tenant quota. The fan-out semaphore (below) is the actual protection against the amplification finding 1 described. Important finding 3 — the fan-out semaphore (4 concurrent backends) tripled worst-case latency (907ms measured at 300ms/server, was ~300ms) and pushed the hung-backend worst case to ceil(11/4)x20s=60s, past the ~30s timeout many MCP clients use. Raised to 8, and every backend in one tools/list call now shares ONE overall toolsListBackendTimeout deadline (fanoutCtx) instead of each getting a fresh one from its own staggered start — restoring this function's original 'bounds the whole call regardless of server count' invariant even with bounded concurrency. Important finding 4 — the 4MiB response cap (sized for the tools/list fan-out's memory-amplification risk) silently applied to tools/call too, where exactly one backend is ever contacted and a legitimate result (an image, an extracted document) can be much larger. tools/call now uses mcpBackendCallResponseMaxBytes (10MiB, the pre-finding-1c budget); tools/list keeps the 4MiB cap. A response that hits either cap is detected via a cap+1 read (never silently truncated into a confusing parse failure) and reported to the client as 'response too large'. --- admin_test.go | 55 ++++---- limits.go | 31 +---- mcp_federation.go | 220 +++++++++++++++++++++---------- mcp_federation_test.go | 284 +++++++++++++++++++++++++++++------------ 4 files changed, 389 insertions(+), 201 deletions(-) diff --git a/admin_test.go b/admin_test.go index 74ce70e..1645e99 100644 --- a/admin_test.go +++ b/admin_test.go @@ -1998,34 +1998,39 @@ func TestAdminTargets_AccessListsMatchEnforcement_AndCountersReflectTraffic(t *t // --- default-preserving: the live cluster's real shape (security audit, 2026-08-22) --- // TestDefaultPreserving_LiveClusterShape_LoadsAndServesIdentically models -// the live production shape this round's five fixes must not break -// (project brief): 5 "friend" users plus 4 "home" users (steve-cw and -// kevinsandom are named live users; the rest are representative — real -// names this test has no visibility into, which is exactly why finding -// 5's fix stops short of restricting the name charset, see buildEntry's -// own doc comment, auth.go) and 11 MCP servers. A2A agents are out of -// scope here: none of this round's five findings touch mcp_a2a.go or any -// A2A-specific code path, so there is no mechanism by which they could -// regress agent handling. +// the live production shape this round's fixes must not break (project +// brief): 5 "friend" users plus 4 "home" users (steve-cw and kevinsandom +// are named live users; the rest are representative — real names this +// test has no visibility into, which is exactly why finding 5's fix stops +// short of restricting the name charset, see buildEntry's own doc +// comment, auth.go) and 11 MCP servers. A2A agents are out of scope here: +// none of this round's fixes touch mcp_a2a.go or any A2A-specific code +// path, so there is no mechanism by which they could regress agent +// handling. // // It proves construction succeeds unchanged (finding 5's new empty/ -// duplicate-name checks accept every one of these real names), a normal -// federated tools/list call from an ordinary user still succeeds under a -// realistic per-minute limit (finding 1b's per-backend weighting does not -// spuriously throttle ordinary usage — 11 servers is comfortably under a -// 60/min budget), and the admin dashboard's usage poll still returns -// exactly one row per user and per group in a single store round trip -// (finding 4's chunking is a no-op at 10 users, well under -// adminUsageChunkScopes). +// duplicate-name checks accept every one of these real names), a +// friends-group user's configured requests-per-minute budget still buys +// EXACTLY that many tools/list calls against all 11 servers — not +// budget/11 — regressing security review round 2's critical finding 3 +// (weighting tools/list by backend count was reverted specifically +// because it was not default-preserving at this exact shape: both live +// groups have an empty MCP allow-list, i.e. all 11 servers, so a naive +// per-backend weight would have silently cut every configured +// requests-per-minute budget by ~11x here), and the admin dashboard's +// usage poll still returns exactly one row per user and per group in a +// single store round trip (finding 4's chunking is a no-op at 10 users, +// well under adminUsageChunkScopes). func TestDefaultPreserving_LiveClusterShape_LoadsAndServesIdentically(t *testing.T) { friendNames := []string{"steve-cw", "kevinsandom", "friend3", "friend4", "friend5"} homeNames := []string{"home1", "home2", "home3", "home4"} + const friendsRPM = 60 cfg := CreateConfig() cfg.Providers = map[string]*ProviderConfig{"openai": {Type: "openai", APIKey: "k"}} cfg.Admin = &AdminConfig{Enabled: true} cfg.Groups = map[string]*GroupConfig{ - "friends": {Limits: &LimitsConfig{RequestsPerMinute: 60}}, + "friends": {Limits: &LimitsConfig{RequestsPerMinute: friendsRPM}}, "home": {Limits: &LimitsConfig{RequestsPerMinute: 120}}, } @@ -2049,10 +2054,16 @@ func TestDefaultPreserving_LiveClusterShape_LoadsAndServesIdentically(t *testing t.Fatalf("New: want the realistic live-cluster config to load unchanged, got error: %v", err) } - rec := httptest.NewRecorder() - h.ServeHTTP(rec, newFederatedRequest(t, "sk-steve-cw", jsonrpcRequest{JSONRPC: jsonrpcVersion, Method: "tools/list", ID: json.RawMessage("1")})) - if rec.Code != http.StatusOK { - t.Fatalf("tools/list status = %d, want 200, body=%s", rec.Code, rec.Body.String()) + // Drives friendsRPM calls, not just one (security review round 2: + // a single-call assertion cannot distinguish "budget buys N calls" + // from "budget buys N/11 calls" — exactly the blind spot that let the + // weighted-charge regression ship undetected). + for i := 1; i <= friendsRPM; i++ { + rec := httptest.NewRecorder() + h.ServeHTTP(rec, newFederatedRequest(t, "sk-steve-cw", jsonrpcRequest{JSONRPC: jsonrpcVersion, Method: "tools/list", ID: json.RawMessage(fmt.Sprintf("%d", i))})) + if rec.Code != http.StatusOK { + t.Fatalf("tools/list call %d/%d status = %d, want 200 (budget must buy all %d calls against 11 servers), body=%s", i, friendsRPM, rec.Code, friendsRPM, rec.Body.String()) + } } usageReq := httptest.NewRequest("GET", adminUsagePath, nil) diff --git a/limits.go b/limits.go index 013b6b7..61ae8c9 100644 --- a/limits.go +++ b/limits.go @@ -1081,39 +1081,14 @@ func buildBudgetProbes(scopes []limitScope, now time.Time) (probes []budgetProbe // call completes, one request may still push a scope over its budget — an // accepted trade-off, unchanged from before this round. func (l *limiter) checkAndCount(scopes []limitScope) *limitViolation { - return l.checkAndCountWeighted(scopes, 1) -} - -// checkAndCountWeighted is checkAndCount generalized to charge `weight` -// requests against every scope's requests-per-minute/requests-per-day -// counters instead of exactly one (security audit finding 1b, 2026-08-22): -// a route whose single incoming client request causes this gateway to -// issue `weight` outbound backend calls — today, only the federated MCP -// tools/list fan-out (mcp_federation.go's handleMCPFederated, weight = -// len(allowedMCPServerNames(grp))) — must charge its real cost, not -// undercount a fan-out as a single request. weight<=0 is treated as 1 -// (never zero-cost, never negative); checkAndCount(scopes) is exactly -// checkAndCountWeighted(scopes, 1), so every other call site's behavior is -// completely unchanged by this method existing. Semantics beyond the -// weighted delta are identical to checkAndCount's own doc comment in -// every other respect: one storeIncrAndGetMulti round trip, requests- -// per-minute/requests-per-day compared against the just-incremented -// (now weighted) count, token/cost budgets read and compared unweighted -// (a fan-out has no tokens or USD cost of its own — those probes come -// from buildBudgetProbes, untouched by weight), violation precedence -// unchanged. -func (l *limiter) checkAndCountWeighted(scopes []limitScope, weight int64) *limitViolation { - if weight <= 0 { - weight = 1 - } now := l.now() entries := make([]counterIncr, 0, len(scopes)*checkAndCountKeysPerScope) for _, sc := range scopes { entries = append(entries, - newCounterIncr(sc.kind, sc.id, metricReq, windowMin, now, weight, minWindowTTL), - newCounterIncr(sc.kind, sc.id, metricReq, windowDay, now, weight, dayWindowTTL), - newCounterIncr(sc.kind, sc.id, metricReq, windowHour, now, weight, hourWindowTTL), + newCounterIncr(sc.kind, sc.id, metricReq, windowMin, now, 1, minWindowTTL), + newCounterIncr(sc.kind, sc.id, metricReq, windowDay, now, 1, dayWindowTTL), + newCounterIncr(sc.kind, sc.id, metricReq, windowHour, now, 1, hourWindowTTL), ) } probes, reads := buildBudgetProbes(scopes, now) diff --git a/mcp_federation.go b/mcp_federation.go index 45e7782..25e3a6d 100644 --- a/mcp_federation.go +++ b/mcp_federation.go @@ -67,20 +67,51 @@ const ( ) // mcpBackendResponseMaxBytes caps a single backend MCP server's HTTP -// response body doBackendJSONRPC will read into memory — previously -// maxRequestBytes (10MiB, routes_unified.go), a budget sized for a -// CLIENT's own chat/embeddings request body, not a backend's JSON-RPC -// reply (security audit finding 1c, 2026-08-22). 4MiB matches this -// codebase's other "generous but bounded" read caps for a non-primary- -// request-body read (cache.go's maxAccountingTeeBytes is also 4MiB) — -// comfortably above any tools/list schema listing or tools/call result -// this gateway has actually seen live (typically low KB), while bounding -// mcpFederatedFanoutConcurrency backends' worst-case concurrent memory to -// 4 × 4MiB = 16MiB, against the unbounded ~110MiB (11 production servers -// × up to 10MiB each, all fired at once) the security audit measured -// before this fix. +// response body for the tools/list FAN-OUT path only (mcpFederatedToolsList) +// — previously maxRequestBytes (10MiB, routes_unified.go), a budget sized +// for a CLIENT's own chat/embeddings request body, not a backend's +// JSON-RPC reply (security audit finding 1c, 2026-08-22). 4MiB matches +// this codebase's other "generous but bounded" read caps for a non- +// primary-request-body read (cache.go's maxAccountingTeeBytes is also +// 4MiB) — measured at 547KiB for a real tools/list response describing +// 20 fat tools (security review round 2, 2026-08-22), comfortably below +// this cap — while bounding mcpFederatedFanoutConcurrency backends' +// worst-case concurrent memory to a small multiple of 4MiB, against the +// unbounded ~110MiB (11 production servers × up to 10MiB each, all fired +// at once) the security audit measured before this fix. This cap is +// deliberately NOT used for tools/call — see mcpBackendCallResponseMaxBytes, +// whose own doc comment covers why the fan-out memory argument does not +// apply there. const mcpBackendResponseMaxBytes = 4 << 20 +// mcpBackendCallResponseMaxBytes caps a single backend's response for the +// tools/call path (mcpFederatedToolsCall) — kept at maxRequestBytes +// (10MiB), the SAME budget this file used everywhere before finding 1c, +// deliberately NOT shrunk to mcpBackendResponseMaxBytes (security review +// round 2, 2026-08-22, important finding 4): tools/call always resolves +// to exactly ONE backend (resolveFederatedTool), so the fan-out memory- +// amplification argument that justifies the fan-out path's smaller cap +// does not hold here at all — a single tool result can legitimately be +// much larger than a tools/list schema listing (an extracted document, an +// image or other blob a tool returns), and silently truncating one into +// a generic parse failure would be a real, avoidable regression for any +// tool whose legitimate results sit between 4MiB and 10MiB. +const mcpBackendCallResponseMaxBytes = maxRequestBytes + +// mcpResponseTooLargeMarker is a fixed, unique substring doBackendJSONRPC +// embeds in its own error message when a backend response hits its +// call's maxBytes cap (security review round 2, 2026-08-22, important +// finding 4) — detected via strings.Contains, deliberately NOT an +// errors.Is-checked sentinel. matchesSentinel's own doc comment +// (limits.go) proves errors.Is safe under Yaegi only for chains reaching +// a call site this package's own yaegi-check harness actually exercises +// at runtime under the real interpreter — federation's tools/call path +// is not one of those exercised paths today, so a plain string check +// trades a little idiom for zero interpreter risk here, rather than +// relying on reasoning about compiled-vs-interpreted-origin values that +// nothing in the gate actually verifies for this specific path. +const mcpResponseTooLargeMarker = "mcp backend response too large" + // jsonrpcRequest is one JSON-RPC 2.0 request/notification, decoded from // the client's POST body and re-encoded (with a synthetic id) for this // gateway's own outbound calls to a backend MCP server (mcpBackendCall). @@ -249,27 +280,32 @@ func allowedMCPServerNames(cfg *Config, grp *group) []string { // // checkAndCount runs once per incoming request, before method dispatch — // covering the caller's own user/group/total scopes exactly like every -// other metered route — with per-target attribution added separately +// other metered route, charging the plain weight of one request +// regardless of method — with per-target attribution added separately // inside mcpFederatedToolsList/mcpFederatedToolsCall (their own doc // comments cover exactly what "attributed" means for each). // -// The weight charged to checkAndCount is 1 for every method EXCEPT -// tools/list (security audit finding 1b, 2026-08-22): a tools/list call -// fans out to every server allowedMCPServerNames(grp) returns — up to the -// whole MCP catalog, concurrently (mcpFederatedToolsList's own doc -// comment) — so one incoming client request can cost this gateway many -// real outbound backend calls. Charging it as a single request against -// the caller's requests-per-minute/requests-per-day budget undercounts -// that real cost; weighting it by len(names) instead makes the budget -// reflect what the request actually does. This is computed and charged -// exactly once, right here, before dispatch — never doubled by anything -// mcpFederatedToolsList itself does (its own countTargetRequests call is a -// SEPARATE scope kind, "mcp"/target-id, entirely disjoint from the -// user/group/total scopes checkAndCountWeighted evaluates here; see -// countTargetRequests' own doc comment, limits.go). tools/call, by -// contrast, always resolves to exactly ONE backend (resolveFederatedTool), -// so it keeps the plain weight of 1 — checkAndCountWeighted(scopes, 1) is -// byte-for-byte checkAndCount's own existing behavior. +// tools/list's fan-out is deliberately NOT charged against the caller's +// own requests-per-minute/requests-per-day budget (security review round +// 2, 2026-08-22, critical finding 3 — reverting security audit finding +// 1b's weighted-charge attempt): weighting checkAndCount by the number of +// allowed MCP servers is not default-preserving — on this operator's own +// live fleet, both configured groups have an empty MCP allow-list (every +// server allowed), so EVERY tools/list call would have weighed 11, +// silently turning a configured requests-per-minute: 60 into an effective +// budget of 5 successful tools/list calls per minute, and +// examples/kubernetes.yaml's shipped requests-per-minute: 10 would have +// 429'd the very FIRST tools/list call ever made — with no config change +// on the operator's part. The fan-out's real amplification is still +// fully visible to an operator without silently consuming tenant quota: +// mcpFederatedToolsList's own countTargetRequests call attributes one +// request to EVERY server actually attempted, at the "mcp"/target-id +// scope (limits.go) — a SEPARATE scope kind from the user/group/total +// scopes checkAndCount evaluates here — so the dashboard's targets view +// already shows the true per-server cost. The fan-out's outbound HTTP/ +// memory cost is bounded instead by mcpFederatedFanoutConcurrency's +// semaphore (mcpFederatedToolsList's own doc comment), which is the +// actual protection against the amplification finding 1 described. func (g *Gateway) handleMCPFederated(w http.ResponseWriter, r *http.Request, u *user, grp *group) { if r.Header.Get("Upgrade") != "" { writeOAIError(w, http.StatusNotImplemented, "invalid_request_error", "websocket/upgrade passthrough not supported") @@ -292,17 +328,7 @@ func (g *Gateway) handleMCPFederated(w http.ResponseWriter, r *http.Request, u * } scopes := withTotalScope(buildLimitScopes(u, grp)) - - var toolsListNames []string - weight := int64(1) - if req.Method == "tools/list" { - toolsListNames = allowedMCPServerNames(g.cfg, grp) - if n := int64(len(toolsListNames)); n > weight { - weight = n - } - } - - if violation := g.limiter.checkAndCountWeighted(scopes, weight); violation != nil { + if violation := g.limiter.checkAndCount(scopes); violation != nil { writeLimitViolation(w, violation) return } @@ -313,7 +339,7 @@ func (g *Gateway) handleMCPFederated(w http.ResponseWriter, r *http.Request, u * case req.Method == "ping": writeJSONRPCResult(w, req.ID, map[string]any{}) case req.Method == "tools/list": - g.mcpFederatedToolsList(w, r, req, toolsListNames) + g.mcpFederatedToolsList(w, r, req, allowedMCPServerNames(g.cfg, grp)) case req.Method == "tools/call": g.mcpFederatedToolsCall(w, r, req, grp) case strings.HasPrefix(req.Method, "notifications/"): @@ -413,8 +439,16 @@ type mcpToolsListResult struct { // doBackendJSONRPC issues one JSON-RPC 2.0 POST to targetURL — an MCP // server's own configured base URL, the same single endpoint // handleTargetProxy reverse-proxies every method to — carrying sessionID -// as the Mcp-Session-Id request header when non-empty. It is the one place -// that actually builds and sends an HTTP request for this file; every +// as the Mcp-Session-Id request header when non-empty, and reading at +// most maxBytes of the response body (mcpBackendResponseMaxBytes for the +// tools/list fan-out, mcpBackendCallResponseMaxBytes for tools/call — +// their own doc comments cover why the two differ; security review round +// 2, 2026-08-22, important finding 4). A response body that hits maxBytes +// is reported with mcpResponseTooLargeMarker in its message, not silently truncated into +// whatever partial (and likely invalid) JSON happened to fit. +// +// It is the one place that actually builds and sends an HTTP request for +// this file; every // other function in the handshake-fallback machinery (mcpBackendCall, // mcpBackendHandshake, mcpBackendCloseSession's own DELETE aside) goes // through this. @@ -439,7 +473,7 @@ type mcpToolsListResult struct { // that WAS valid JSON but not a real JSON-RPC response (see // parseBackendJSONRPC's own validity check) would have silently produced // an all-zero-fields response otherwise. -func (g *Gateway) doBackendJSONRPC(ctx context.Context, targetURL, method string, params any, sessionID string) (resp *jsonrpcResponse, status int, respSessionID string, err error) { +func (g *Gateway) doBackendJSONRPC(ctx context.Context, targetURL, method string, params any, sessionID string, maxBytes int64) (resp *jsonrpcResponse, status int, respSessionID string, err error) { paramsRaw, err := json.Marshal(params) if err != nil { return nil, 0, "", err @@ -476,10 +510,17 @@ func (g *Gateway) doBackendJSONRPC(ctx context.Context, targetURL, method string status = httpResp.StatusCode respSessionID = httpResp.Header.Get(mcpSessionHeader) - respBody, err := io.ReadAll(io.LimitReader(httpResp.Body, mcpBackendResponseMaxBytes)) + // Reads one byte past maxBytes so a response that hits the cap is + // DETECTABLE (len(respBody) > maxBytes) rather than silently truncated + // and handed to parseBackendJSONRPC to fail confusingly (security + // review round 2, 2026-08-22, important finding 4). + respBody, err := io.ReadAll(io.LimitReader(httpResp.Body, maxBytes+1)) if err != nil { return nil, status, respSessionID, err } + if int64(len(respBody)) > maxBytes { + return nil, status, respSessionID, fmt.Errorf("%s: exceeds %d bytes", mcpResponseTooLargeMarker, maxBytes) + } if status < 200 || status >= 300 { return nil, status, respSessionID, fmt.Errorf("upstream returned HTTP %d", status) } @@ -500,13 +541,13 @@ func (g *Gateway) doBackendJSONRPC(ctx context.Context, targetURL, method string // the initialize call itself failed outright (network error, non-2xx, a // JSON-RPC error response to initialize itself) — there is no retry of a // failed handshake. -func (g *Gateway) mcpBackendHandshake(ctx context.Context, targetURL string) (sessionID string, err error) { +func (g *Gateway) mcpBackendHandshake(ctx context.Context, targetURL string, maxBytes int64) (sessionID string, err error) { initParams := map[string]any{ "protocolVersion": defaultMCPProtocolVersion, "capabilities": map[string]any{}, "clientInfo": map[string]any{"name": "traefik-llmgateway", "version": pluginVersion}, } - resp, _, respSessionID, err := g.doBackendJSONRPC(ctx, targetURL, "initialize", initParams, "") + resp, _, respSessionID, err := g.doBackendJSONRPC(ctx, targetURL, "initialize", initParams, "", maxBytes) if err != nil { return "", err } @@ -567,9 +608,14 @@ func (g *Gateway) mcpBackendCloseSession(ctx context.Context, targetURL, session // except the close itself — see mcpSessionCloseTimeout's own doc // comment); callers derive it from context.WithTimeout(r.Context(), ...) // with toolsListBackendTimeout or toolsCallBackendTimeout (MF1, review -// round 2, 2026-08-21). -func (g *Gateway) mcpBackendCall(ctx context.Context, targetURL, method string, params any) (*jsonrpcResponse, error) { - resp, status, _, err := g.doBackendJSONRPC(ctx, targetURL, method, params, "") +// round 2, 2026-08-21). maxBytes is doBackendJSONRPC's own response-size +// cap, passed straight through to every doBackendJSONRPC call this +// function makes (bare attempt, handshake, retry) — callers pass +// mcpBackendResponseMaxBytes or mcpBackendCallResponseMaxBytes depending +// on which path they are (security review round 2, 2026-08-22, important +// finding 4). +func (g *Gateway) mcpBackendCall(ctx context.Context, targetURL, method string, params any, maxBytes int64) (*jsonrpcResponse, error) { + resp, status, _, err := g.doBackendJSONRPC(ctx, targetURL, method, params, "", maxBytes) if err == nil && resp.Error == nil { return resp, nil } @@ -579,7 +625,7 @@ func (g *Gateway) mcpBackendCall(ctx context.Context, targetURL, method string, return nil, err } - sessionID, initErr := g.mcpBackendHandshake(ctx, targetURL) + sessionID, initErr := g.mcpBackendHandshake(ctx, targetURL, maxBytes) if initErr != nil { // The handshake itself failed: surface the bare attempt's own // outcome when it has one (a JSON-RPC error response is more @@ -600,7 +646,7 @@ func (g *Gateway) mcpBackendCall(ctx context.Context, targetURL, method string, }() } - retryResp, _, _, retryErr := g.doBackendJSONRPC(ctx, targetURL, method, params, sessionID) + retryResp, _, _, retryErr := g.doBackendJSONRPC(ctx, targetURL, method, params, sessionID, maxBytes) if retryErr != nil { return nil, retryErr } @@ -685,19 +731,28 @@ func lastSSEDataLine(body []byte) []byte { // per allowed server with NO cap at all: with the 11 production servers, // one client call could hold up to 11 concurrent connections and up to // 11× mcpBackendResponseMaxBytes of backend response bodies in memory at -// once, entirely outside this gateway's own admission control. 4 keeps -// every server eventually contacted — a goroutine blocks on the -// semaphore, it is never dropped or skipped — while capping the worst -// case to 4 concurrent backends' cost, not the whole catalog's. -const mcpFederatedFanoutConcurrency = 4 +// once, entirely outside this gateway's own admission control. Every +// server is eventually contacted regardless of this bound — a goroutine +// blocks on the semaphore, it is never dropped or skipped. 8 (raised from +// an initial 4, security review round 2, 2026-08-22, important finding +// 3): 4 measurably tripled worst-case wall-clock latency for an +// unrestricted group against all 11 servers (907ms measured at 300ms/ +// server, versus ~300ms pre-bound) and, against hung backends, pushed the +// worst case to ceil(11/4)×toolsListBackendTimeout = 60s — well past the +// ~30s timeout many real MCP clients use, holding the ServeHTTP goroutine +// the whole time. 8 nearly halves both: ceil(11/8)=2 batches. The fanoutCtx +// deadline below (not this constant alone) is what actually re-bounds the +// worst case back down near toolsListBackendTimeout regardless of +// concurrency or backend count — see its own doc comment. +const mcpFederatedFanoutConcurrency = 8 // mcpFederatedToolsList implements the federated "tools/list": it fans // out one mcpBackendCall per server in names (handleMCPFederated's own -// allowedMCPServerNames(grp) call, made once there so the SAME set that -// was weighed into checkAndCountWeighted is the set actually contacted), -// at most mcpFederatedFanoutConcurrency at a time — each under its own -// toolsListBackendTimeout budget (context.WithTimeout off r.Context(), -// MF1) — then merges every reachable server's own tools into one result, +// allowedMCPServerNames(grp) call, made once there so the exact set +// contacted is always the one the client actually asked to reach), at +// most mcpFederatedFanoutConcurrency at a time — every backend sharing +// ONE overall toolsListBackendTimeout deadline (fanoutCtx, below) — then +// merges every reachable server's own tools into one result, // each tool's name prefixed "_", the exact // underscore-separator convention the old agentgateway this plugin // replaces used, and the one pugbot's mcpclient and agentkit have already @@ -725,6 +780,24 @@ func (g *Gateway) mcpFederatedToolsList(w http.ResponseWriter, r *http.Request, merged = make([]mcpTool, 0, len(names)) failed []string ) + // fanoutCtx bounds the WHOLE fan-out to toolsListBackendTimeout from + // when THIS call started — not each individual backend's own start + // (security review round 2, 2026-08-22, important finding 3). With + // mcpFederatedFanoutConcurrency backends running at once, a later + // batch's goroutines only begin once an earlier one's semaphore slot + // frees; giving each one its own FRESH toolsListBackendTimeout budget + // (this function's pre-bound design, and its own initial post- + // semaphore revision) let total wall-clock grow with + // ceil(len(names)/mcpFederatedFanoutConcurrency) batches. Every + // backend below shares this ONE already-ticking context instead — + // context.WithTimeout always resolves to the earlier of a parent's + // existing deadline and its own duration (proven by + // TestHandleMCPFederated_ToolsList_SlowBackend_BoundedByRequestContext), + // so deriving fanoutCtx from r.Context() here preserves that same + // property for the request's own deadline, on top of this one. + fanoutCtx, fanoutCancel := context.WithTimeout(r.Context(), toolsListBackendTimeout) + defer fanoutCancel() + sem := make(chan struct{}, mcpFederatedFanoutConcurrency) for _, name := range names { wg.Add(1) @@ -752,10 +825,8 @@ func (g *Gateway) mcpFederatedToolsList(w http.ResponseWriter, r *http.Request, sem <- struct{}{} defer func() { <-sem }() - ctx, cancel := context.WithTimeout(r.Context(), toolsListBackendTimeout) - defer cancel() targetURL := g.cfg.MCPServers[name].URL - resp, err := g.mcpBackendCall(ctx, targetURL, "tools/list", struct{}{}) + resp, err := g.mcpBackendCall(fanoutCtx, targetURL, "tools/list", struct{}{}, mcpBackendResponseMaxBytes) mu.Lock() defer mu.Unlock() @@ -867,11 +938,24 @@ func (g *Gateway) mcpFederatedToolsCall(w http.ResponseWriter, r *http.Request, ctx, cancel := context.WithTimeout(r.Context(), toolsCallBackendTimeout) defer cancel() targetURL := g.cfg.MCPServers[serverName].URL - resp, err := g.mcpBackendCall(ctx, targetURL, "tools/call", mcpToolCallParams{Name: toolName, Arguments: params.Arguments}) + // mcpBackendCallResponseMaxBytes (10MiB), not the fan-out's smaller + // mcpBackendResponseMaxBytes (security review round 2, 2026-08-22, + // important finding 4): tools/call always resolves to exactly ONE + // backend, so the fan-out memory-amplification argument does not + // apply here — see mcpBackendCallResponseMaxBytes' own doc comment. + resp, err := g.mcpBackendCall(ctx, targetURL, "tools/call", mcpToolCallParams{Name: toolName, Arguments: params.Arguments}, mcpBackendCallResponseMaxBytes) g.limiter.countTargetRequest(targetKindMCP, serverName) if err != nil { g.logf("federated tools/call: server %q: %v", serverName, err) - writeJSONRPCErrorResponse(w, req.ID, jsonrpcInternalError, "upstream error") + // A response that hit mcpBackendCallResponseMaxBytes gets its own + // specific message, not the generic "upstream error" a truncated- + // then-unparsable body would otherwise produce (security review + // round 2, 2026-08-22, important finding 4). + if strings.Contains(err.Error(), mcpResponseTooLargeMarker) { + writeJSONRPCErrorResponse(w, req.ID, jsonrpcInternalError, "response too large") + } else { + writeJSONRPCErrorResponse(w, req.ID, jsonrpcInternalError, "upstream error") + } return } diff --git a/mcp_federation_test.go b/mcp_federation_test.go index dd19b17..5bf0582 100644 --- a/mcp_federation_test.go +++ b/mcp_federation_test.go @@ -1031,7 +1031,7 @@ func TestMcpBackendCall_HandshakeFallbackTrigger(t *testing.T) { t.Fatal("handler is not *Gateway") } - _, _ = gw.mcpBackendCall(context.Background(), srv.URL, "tools/list", struct{}{}) + _, _ = gw.mcpBackendCall(context.Background(), srv.URL, "tools/list", struct{}{}, mcpBackendResponseMaxBytes) gotCalls := callCount.Load() if tc.wantHandshake && gotCalls < 2 { @@ -1060,12 +1060,134 @@ func TestMcpBackendCall_NetworkFailure_NeverAttemptsHandshake(t *testing.T) { t.Fatal("handler is not *Gateway") } - _, err = gw.mcpBackendCall(context.Background(), "http://127.0.0.1:1", "tools/list", struct{}{}) + _, err = gw.mcpBackendCall(context.Background(), "http://127.0.0.1:1", "tools/list", struct{}{}, mcpBackendResponseMaxBytes) if err == nil { t.Fatal("want an error (nothing listening on 127.0.0.1:1)") } } +// --- security review round 2, important finding 4: dual response caps --- + +// TestMcpBackendCall_ResponseExceedsCap_ReturnsDistinctTooLargeError is a +// fast, small-scale unit test of doBackendJSONRPC's own maxBytes +// mechanism, using an artificially tiny cap so the test needs no +// multi-megabyte payload: a backend response over the cap must be +// reported with mcpResponseTooLargeMarker in its message — +// distinguishable via strings.Contains from an ordinary network/parse +// failure — never silently truncated into a confusing parse error. +func TestMcpBackendCall_ResponseExceedsCap_ReturnsDistinctTooLargeError(t *testing.T) { + const tinyCap = 64 + big := strings.Repeat("x", tinyCap*4) // well past tinyCap once JSON-encoded + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + var req jsonrpcRequest + _ = json.NewDecoder(r.Body).Decode(&req) + result, _ := json.Marshal(map[string]string{"data": big}) + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(jsonrpcResponse{ID: req.ID, Result: result}) + })) + defer srv.Close() + + cfg := newFederationTestConfig(srv.URL, "http://beta.invalid", false) + h, err := New(context.Background(), http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}), cfg, "llmgw") + if err != nil { + t.Fatalf("New: %v", err) + } + gw, ok := h.(*Gateway) + if !ok { + t.Fatal("handler is not *Gateway") + } + + _, err = gw.mcpBackendCall(context.Background(), srv.URL, "tools/list", struct{}{}, tinyCap) + if err == nil { + t.Fatal("want an error for a response exceeding the cap") + } + if !strings.Contains(err.Error(), mcpResponseTooLargeMarker) { + t.Errorf("err = %v, want it to contain %q", err, mcpResponseTooLargeMarker) + } +} + +// TestMcpBackendCall_ResponseAtOrUnderCap_Succeeds proves the cap+1 read +// technique does not false-positive: a response exactly AT the cap must +// still succeed, not be mistaken for oversized. +func TestMcpBackendCall_ResponseAtOrUnderCap_Succeeds(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + var req jsonrpcRequest + _ = json.NewDecoder(r.Body).Decode(&req) + result, _ := json.Marshal(map[string]bool{"ok": true}) + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(jsonrpcResponse{ID: req.ID, Result: result}) + })) + defer srv.Close() + + cfg := newFederationTestConfig(srv.URL, "http://beta.invalid", false) + h, err := New(context.Background(), http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}), cfg, "llmgw") + if err != nil { + t.Fatalf("New: %v", err) + } + gw, ok := h.(*Gateway) + if !ok { + t.Fatal("handler is not *Gateway") + } + + // A generous cap comfortably above this tiny response — must succeed + // cleanly, not be flagged as too-large. + resp, err := gw.mcpBackendCall(context.Background(), srv.URL, "tools/list", struct{}{}, 4096) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if resp.Error != nil { + t.Fatalf("resp.Error = %+v, want nil", resp.Error) + } +} + +// TestHandleMCPFederated_ToolsCall_ResponseBetween4And10MiB_Succeeds is +// the end-to-end regression for important finding 4: a real tool result +// between mcpBackendResponseMaxBytes (4MiB, the fan-out-only cap) and +// mcpBackendCallResponseMaxBytes (10MiB, tools/call's own cap) — legal +// under the pre-finding-1c budget, and a realistic size for an image or +// extracted-document tool result — must succeed on the tools/call path, +// proving that path was never shrunk to the fan-out's smaller cap. +func TestHandleMCPFederated_ToolsCall_ResponseBetween4And10MiB_Succeeds(t *testing.T) { + const payloadBytes = 6 << 20 // 6MiB: over the 4MiB fan-out cap, under the 10MiB tools/call cap + big := strings.Repeat("y", payloadBytes) + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + var req jsonrpcRequest + _ = json.NewDecoder(r.Body).Decode(&req) + if req.Method == "tools/call" { + result, _ := json.Marshal(map[string]string{"data": big}) + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(jsonrpcResponse{ID: req.ID, Result: result}) + return + } + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(jsonrpcResponse{ID: req.ID, Result: json.RawMessage(`{}`)}) + })) + defer srv.Close() + + cfg := newFederationTestConfig(srv.URL, "http://beta.invalid", false) + h, err := New(context.Background(), http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}), cfg, "llmgw") + if err != nil { + t.Fatalf("New: %v", err) + } + + rec := httptest.NewRecorder() + h.ServeHTTP(rec, newFederatedRequest(t, "sk-alice", jsonrpcRequest{ + JSONRPC: jsonrpcVersion, Method: "tools/call", ID: json.RawMessage("1"), + Params: json.RawMessage(`{"name":"alpha_lookup"}`), + })) + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200, body=%s", rec.Code, rec.Body.String()) + } + var got jsonrpcResponse + if err := json.Unmarshal(rec.Body.Bytes(), &got); err != nil { + t.Fatalf("decode: %v", err) + } + if got.Error != nil { + t.Fatalf("error = %+v, want nil (a 6MiB result must succeed on the tools/call path)", got.Error) + } +} + // TestHandleMCPFederated_ToolsList_HandshakeFallback_SucceedsAfterSessionRetry // is MF3's core proof: a server that rejects a bare tools/list call // (readitall/fetch's real, live-probed behavior — sessionRequiredMockServer's @@ -1391,100 +1513,96 @@ func betaHost(rawURL string) (string, error) { return strings.TrimPrefix(rawURL, prefix), nil } -// --- security audit finding 1b: tools/list weighted by backend count --- +// --- security review round 2, critical finding 3: tools/list is NOT weighted --- -// TestHandleMCPFederated_ToolsList_WeightedByBackendCount_RejectsWhenOverBudget -// proves a tools/list call charges checkAndCount len(allowedMCPServerNames) -// requests, not one: with 3 configured (and unrestricted) servers and a -// requests-per-minute budget of 2, the very FIRST tools/list call must be -// rejected outright — and, because checkAndCountWeighted runs and fails -// BEFORE the fan-out ever starts, none of the three servers may be -// contacted at all. -func TestHandleMCPFederated_ToolsList_WeightedByBackendCount_RejectsWhenOverBudget(t *testing.T) { - alpha := newMockJSONRPCServer(t, nil) - beta := newMockJSONRPCServer(t, nil) - gamma := newMockJSONRPCServer(t, nil) - cfg := newFederationTestConfig(alpha.srv.URL, beta.srv.URL, false) - cfg.MCPServers["gamma"] = &TargetConfig{URL: gamma.srv.URL} - cfg.Groups["default"].Limits = &LimitsConfig{RequestsPerMinute: 2} +// TestHandleMCPFederated_ToolsList_NotWeightedByBackendCount_BudgetBuysNCalls +// is the regression for security review round 2's critical finding 3: +// weighting checkAndCount by the number of allowed MCP servers (security +// audit finding 1b's original fix) was not default-preserving — on a +// config where the MCP allow-list is unrestricted (every group on the +// live fleet), it silently turned a configured requests-per-minute budget +// into budget/serverCount successful tools/list calls, with NO config +// change on the operator's part (11 servers, requests-per-minute: 60 -> +// only 5 calls/minute actually succeeded; examples/kubernetes.yaml's +// shipped requests-per-minute: 10 would 429 the very FIRST call ever +// made). With numServers (11, matching the live fleet) configured and a +// requests-per-minute budget of exactly rpmBudget, all rpmBudget calls +// must succeed and the next one must be rejected — proving the charge is +// exactly 1 per call, regardless of how many backends that call fans out +// to. +func TestHandleMCPFederated_ToolsList_NotWeightedByBackendCount_BudgetBuysNCalls(t *testing.T) { + const numServers = 11 // matches the live fleet's own MCP server count + const rpmBudget = 5 + + cfg := CreateConfig() + cfg.Providers = map[string]*ProviderConfig{"openai": {Type: "openai", APIKey: "k"}} + cfg.MCPServers = make(map[string]*TargetConfig, numServers) + for i := 0; i < numServers; i++ { + srv := newMockJSONRPCServer(t, []mcpTool{{Name: "lookup"}}) + cfg.MCPServers[fmt.Sprintf("srv%d", i)] = &TargetConfig{URL: srv.srv.URL} + } + cfg.Groups = map[string]*GroupConfig{"default": {Limits: &LimitsConfig{RequestsPerMinute: rpmBudget}}} + cfg.Users = &UsersConfig{Inline: []*UserConfig{{Name: "alice", Group: "default", APIKey: "sk-alice"}}} h, err := New(context.Background(), http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}), cfg, "llmgw") if err != nil { t.Fatalf("New: %v", err) } + for i := 1; i <= rpmBudget; i++ { + rec := httptest.NewRecorder() + h.ServeHTTP(rec, newFederatedRequest(t, "sk-alice", jsonrpcRequest{JSONRPC: jsonrpcVersion, Method: "tools/list", ID: json.RawMessage(fmt.Sprintf("%d", i))})) + if rec.Code != http.StatusOK { + t.Fatalf("call %d/%d status = %d, want 200 (budget must buy exactly rpmBudget calls, not rpmBudget/numServers), body=%s", i, rpmBudget, rec.Code, rec.Body.String()) + } + } + + rec := httptest.NewRecorder() + h.ServeHTTP(rec, newFederatedRequest(t, "sk-alice", jsonrpcRequest{JSONRPC: jsonrpcVersion, Method: "tools/list", ID: json.RawMessage(`"over"`)})) + if rec.Code != http.StatusTooManyRequests { + t.Fatalf("call %d status = %d, want 429 (budget exhausted), body=%s", rpmBudget+1, rec.Code, rec.Body.String()) + } +} + +// TestHandleMCPFederated_ToolsList_AmplificationVisibleAtTargetScopeOnly +// proves the fan-out's real per-server cost is still fully visible to an +// operator without silently consuming tenant quota (security review round +// 2, 2026-08-22, critical finding 3's own ruling): after ONE tools/list +// call against 2 servers, the user's own requests-per-minute counter +// reads exactly 1 (never weighted), while EACH server's own "mcp" +// target-scope counter (countTargetRequests, limits.go) reads exactly 1 +// — the real amplification, attributed where an operator can see it. +func TestHandleMCPFederated_ToolsList_AmplificationVisibleAtTargetScopeOnly(t *testing.T) { + alpha := newMockJSONRPCServer(t, []mcpTool{{Name: "lookup"}}) + beta := newMockJSONRPCServer(t, []mcpTool{{Name: "search"}}) + cfg := newFederationTestConfig(alpha.srv.URL, beta.srv.URL, false) + h, err := New(context.Background(), http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}), cfg, "llmgw") + if err != nil { + t.Fatalf("New: %v", err) + } + gw, ok := h.(*Gateway) + if !ok { + t.Fatal("handler is not *Gateway") + } + rec := httptest.NewRecorder() h.ServeHTTP(rec, newFederatedRequest(t, "sk-alice", jsonrpcRequest{JSONRPC: jsonrpcVersion, Method: "tools/list", ID: json.RawMessage("1")})) - if rec.Code != http.StatusTooManyRequests { - t.Fatalf("status = %d, want 429 (weight=3 > limit=2), body=%s", rec.Code, rec.Body.String()) - } - for name, srv := range map[string]*mockJSONRPCServer{"alpha": alpha, "beta": beta, "gamma": gamma} { - if srv.called.Load() != 0 { - t.Errorf("server %q was contacted %d times, want 0 (rejected before fan-out)", name, srv.called.Load()) - } - } -} - -// TestHandleMCPFederated_ToolsList_WeightedByBackendCount_AccumulatesAcrossCalls -// proves the weighted charge actually accumulates against the same -// requests-per-minute window: with 3 servers and a budget of 5, the first -// tools/list call (weight 3) succeeds, and the second (weight 3 more, -// total 6 > 5) is rejected. -func TestHandleMCPFederated_ToolsList_WeightedByBackendCount_AccumulatesAcrossCalls(t *testing.T) { - alpha := newMockJSONRPCServer(t, []mcpTool{{Name: "lookup"}}) - beta := newMockJSONRPCServer(t, []mcpTool{{Name: "search"}}) - gamma := newMockJSONRPCServer(t, []mcpTool{{Name: "fetch"}}) - cfg := newFederationTestConfig(alpha.srv.URL, beta.srv.URL, false) - cfg.MCPServers["gamma"] = &TargetConfig{URL: gamma.srv.URL} - cfg.Groups["default"].Limits = &LimitsConfig{RequestsPerMinute: 5} - - h, err := New(context.Background(), http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}), cfg, "llmgw") - if err != nil { - t.Fatalf("New: %v", err) + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200, body=%s", rec.Code, rec.Body.String()) } - rec1 := httptest.NewRecorder() - h.ServeHTTP(rec1, newFederatedRequest(t, "sk-alice", jsonrpcRequest{JSONRPC: jsonrpcVersion, Method: "tools/list", ID: json.RawMessage("1")})) - if rec1.Code != http.StatusOK { - t.Fatalf("first call status = %d, want 200, body=%s", rec1.Code, rec1.Body.String()) + now := time.Now() + userReq, ok := gw.limiter.getCounter("user", "alice", metricReq, windowDay, now) + if !ok || userReq != 1 { + t.Errorf("user req:day counter = %d (ok=%v), want exactly 1 (never weighted by backend count)", userReq, ok) } - - rec2 := httptest.NewRecorder() - h.ServeHTTP(rec2, newFederatedRequest(t, "sk-alice", jsonrpcRequest{JSONRPC: jsonrpcVersion, Method: "tools/list", ID: json.RawMessage("2")})) - if rec2.Code != http.StatusTooManyRequests { - t.Fatalf("second call status = %d, want 429 (3+3=6 > limit=5), body=%s", rec2.Code, rec2.Body.String()) + alphaReq, ok := gw.limiter.getCounter(targetKindMCP, "alpha", metricReq, windowDay, now) + if !ok || alphaReq != 1 { + t.Errorf("mcp/alpha req:day counter = %d (ok=%v), want 1", alphaReq, ok) } -} - -// TestHandleMCPFederated_ToolsCall_WeightStaysOne_EvenWithManyServersConfigured -// proves tools/call — which only ever resolves to ONE backend — never -// inherits tools/list's per-backend weighting: with 3 servers configured -// and a requests-per-minute budget of 2, two sequential tools/call -// requests must both succeed (weight 1 each, total 2, at the limit), where -// the equivalent tools/list weighting (3 each) would already have -// rejected the first one. -func TestHandleMCPFederated_ToolsCall_WeightStaysOne_EvenWithManyServersConfigured(t *testing.T) { - alpha := newMockJSONRPCServer(t, nil) - beta := newMockJSONRPCServer(t, nil) - gamma := newMockJSONRPCServer(t, nil) - cfg := newFederationTestConfig(alpha.srv.URL, beta.srv.URL, false) - cfg.MCPServers["gamma"] = &TargetConfig{URL: gamma.srv.URL} - cfg.Groups["default"].Limits = &LimitsConfig{RequestsPerMinute: 2} - - h, err := New(context.Background(), http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}), cfg, "llmgw") - if err != nil { - t.Fatalf("New: %v", err) - } - - for i := 1; i <= 2; i++ { - rec := httptest.NewRecorder() - h.ServeHTTP(rec, newFederatedRequest(t, "sk-alice", jsonrpcRequest{ - JSONRPC: jsonrpcVersion, Method: "tools/call", ID: json.RawMessage(fmt.Sprintf("%d", i)), - Params: json.RawMessage(`{"name":"alpha_lookup"}`), - })) - if rec.Code != http.StatusOK { - t.Fatalf("call %d status = %d, want 200 (tools/call weight must stay 1), body=%s", i, rec.Code, rec.Body.String()) - } + betaReq, ok := gw.limiter.getCounter(targetKindMCP, "beta", metricReq, windowDay, now) + if !ok || betaReq != 1 { + t.Errorf("mcp/beta req:day counter = %d (ok=%v), want 1", betaReq, ok) } } From de4219242646f13c047bbaa319e67f33dab865c0 Mon Sep 17 00:00:00 2001 From: Lukasz Raczylo Date: Sat, 22 Aug 2026 15:38:35 +0100 Subject: [PATCH 8/8] refactor(mcp): restore errors.Is for the oversize-response sentinel Federation's tools/call oversize branch matched a string marker with strings.Contains instead of wrapping a sentinel. The stated reason was that no gate exercised this path under Yaegi, so errors.Is was unproven here. The caution was misplaced. errors.New returns a compiled *errors.errorString and fmt.Errorf a compiled *fmt.wrapError, so declaring the sentinel variable in interpreted code leaves the entire chain errors.Is walks compiled. Neither documented trap applies: the matchesSentinel trip-wire covers a plugin-declared error TYPE, and the comma-ok trap needs a plugin-declared interface. retry.go has matched errRequestBuildFailed this way through a multi-%w wrap since v0.2.0. The string check was also strictly worse: err.Error() is itself an interpreted-to-compiled dispatch, so it verified nothing extra, and matching on text that partly comes from the backend admits false positives a sentinel cannot have. Rather than settle this by argument, tools/yaegi-check now drives POST /mcp tools/call against a backend whose response exceeds the cap, and asserts the interpreted plugin reports "response too large". Breaking the match makes the gate fail with "upstream error", so the probe has teeth. This closes the objection permanently instead of answering it in a comment. Also in doBackendJSONRPC, check the HTTP status before the size cap. A backend failing with a large HTML error page now reports "upstream returned HTTP 500", which is actionable, rather than sending the operator after a payload-size problem that does not exist. Two comment corrections carried forward from review: the fan-out semaphore is not the same shape as limiter.spawnTokens (spawnTokens drops work via a default case, this one blocks and must never skip a server), and the shared fan-out deadline gives later batches the remaining budget, not a fresh one, so uniformly slow backends yield a partial list. --- mcp_federation.go | 80 +++++++++++++++++++--------- mcp_federation_test.go | 11 ++-- tools/yaegi-check/main.go | 106 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 169 insertions(+), 28 deletions(-) diff --git a/mcp_federation.go b/mcp_federation.go index 25e3a6d..37dae6c 100644 --- a/mcp_federation.go +++ b/mcp_federation.go @@ -4,6 +4,7 @@ import ( "bytes" "context" "encoding/json" + "errors" "fmt" "io" "net/http" @@ -98,19 +99,32 @@ const mcpBackendResponseMaxBytes = 4 << 20 // tool whose legitimate results sit between 4MiB and 10MiB. const mcpBackendCallResponseMaxBytes = maxRequestBytes -// mcpResponseTooLargeMarker is a fixed, unique substring doBackendJSONRPC -// embeds in its own error message when a backend response hits its -// call's maxBytes cap (security review round 2, 2026-08-22, important -// finding 4) — detected via strings.Contains, deliberately NOT an -// errors.Is-checked sentinel. matchesSentinel's own doc comment -// (limits.go) proves errors.Is safe under Yaegi only for chains reaching -// a call site this package's own yaegi-check harness actually exercises -// at runtime under the real interpreter — federation's tools/call path -// is not one of those exercised paths today, so a plain string check -// trades a little idiom for zero interpreter risk here, rather than -// relying on reasoning about compiled-vs-interpreted-origin values that -// nothing in the gate actually verifies for this specific path. -const mcpResponseTooLargeMarker = "mcp backend response too large" +// errMCPResponseTooLarge is the sentinel doBackendJSONRPC wraps when a +// backend response hits its call's maxBytes cap (security review round 2, +// 2026-08-22, important finding 4), letting the tools/call handler report +// "response too large" instead of a generic "upstream error". +// +// errors.Is is the correct match here, and it is Yaegi-safe. An earlier +// revision used a string marker plus strings.Contains out of caution; a +// harness on yaegi v0.16.1 (the version tools/yaegi-check pins, same +// stdlib.Symbols set) interpreting this shape found errors.Is correct +// through shallow, deep, and multi-%w wrapping, and correctly false for +// an unrelated error. The reason is that declaring the VARIABLE in +// interpreted code does not make its VALUE interpreted: errors.New +// returns a compiled *errors.errorString and fmt.Errorf a compiled +// *fmt.wrapError, so the whole chain errors.Is walks is compiled. This +// is the same shape retry.go:isTransient has run in production since +// v0.2.0, and errRequestBuildFailed (providers.go) is already matched +// this way through a multi-%w wrap. +// +// Neither documented Yaegi trap applies: matchesSentinel's TRIP-WIRE +// (limits.go) is about a plugin-declared error TYPE, which errors.New +// does not create, and the comma-ok trap needs a plugin-declared +// interface. A string check was also strictly worse than it looked — +// err.Error() is itself an interpreted-to-compiled method dispatch, so +// it verified nothing extra, and matching on text that partly comes +// from the backend admits false positives a sentinel cannot have. +var errMCPResponseTooLarge = errors.New("mcp backend response too large") // jsonrpcRequest is one JSON-RPC 2.0 request/notification, decoded from // the client's POST body and re-encoded (with a synthetic id) for this @@ -444,8 +458,8 @@ type mcpToolsListResult struct { // tools/list fan-out, mcpBackendCallResponseMaxBytes for tools/call — // their own doc comments cover why the two differ; security review round // 2, 2026-08-22, important finding 4). A response body that hits maxBytes -// is reported with mcpResponseTooLargeMarker in its message, not silently truncated into -// whatever partial (and likely invalid) JSON happened to fit. +// is reported by wrapping errMCPResponseTooLarge, not silently truncated +// into whatever partial (and likely invalid) JSON happened to fit. // // It is the one place that actually builds and sends an HTTP request for // this file; every @@ -518,12 +532,18 @@ func (g *Gateway) doBackendJSONRPC(ctx context.Context, targetURL, method string if err != nil { return nil, status, respSessionID, err } - if int64(len(respBody)) > maxBytes { - return nil, status, respSessionID, fmt.Errorf("%s: exceeds %d bytes", mcpResponseTooLargeMarker, maxBytes) - } + // Status is checked BEFORE the size cap: a backend that fails with a + // large HTML error page is diagnosed as "upstream returned HTTP 500", + // which is actionable, rather than as "too large", which sends the + // operator hunting a payload-size problem that does not exist. An + // oversized SUCCESS response still reports the cap, which is the case + // the cap exists for. if status < 200 || status >= 300 { return nil, status, respSessionID, fmt.Errorf("upstream returned HTTP %d", status) } + if int64(len(respBody)) > maxBytes { + return nil, status, respSessionID, fmt.Errorf("%w: exceeds %d bytes", errMCPResponseTooLarge, maxBytes) + } parsed, err := parseBackendJSONRPC(httpResp.Header.Get("Content-Type"), respBody) if err != nil { @@ -723,10 +743,12 @@ func lastSSEDataLine(body []byte) []byte { // mcpFederatedFanoutConcurrency bounds how many of names' backend servers // mcpFederatedToolsList's own fan-out below contacts at once — a -// buffered-channel semaphore, the same bounded-concurrency shape -// limiter.spawnTokens already uses for its own goroutine fan-out -// (limits.go), applied here to gate each goroutine's actual outbound HTTP -// work rather than its launch. Before this bound (security audit finding +// buffered-channel semaphore gating each goroutine's actual outbound HTTP +// work rather than its launch. It resembles limiter.spawnTokens +// (limits.go) only in using a buffered channel: spawnTokens is a +// non-blocking select with a `default:` that DROPS the work, whereas this +// one blocks until a slot frees and must never skip a server. Do not +// treat the two as interchangeable. Before this bound (security audit finding // 1a, 2026-08-22), one incoming tools/list request spawned one goroutine // per allowed server with NO cap at all: with the 11 production servers, // one client call could hold up to 11 concurrent connections and up to @@ -795,6 +817,18 @@ func (g *Gateway) mcpFederatedToolsList(w http.ResponseWriter, r *http.Request, // TestHandleMCPFederated_ToolsList_SlowBackend_BoundedByRequestContext), // so deriving fanoutCtx from r.Context() here preserves that same // property for the request's own deadline, on top of this one. + // + // The accepted trade: a later batch inherits the REMAINING budget, not + // a fresh one, so if every backend is slow enough to consume most of + // toolsListBackendTimeout on its own, servers in later batches are cut + // off and their tools are missing from the merged list. Measured: 11 + // servers at 11s each returns 8 of 11 tools after 20s, where a per- + // backend budget returned 11 of 11 after 11s. This is deliberate — + // the alternative is a worst case of ceil(11/8) x 20s = 60s for a + // single tools/list — and it degrades to a partial list plus a logged + // failure line per starved server, never a hung request. Real + // tools/list calls answer in well under a second (at 300ms/server all + // 11 return), so this path needs a pathological backend to trigger. fanoutCtx, fanoutCancel := context.WithTimeout(r.Context(), toolsListBackendTimeout) defer fanoutCancel() @@ -951,7 +985,7 @@ func (g *Gateway) mcpFederatedToolsCall(w http.ResponseWriter, r *http.Request, // specific message, not the generic "upstream error" a truncated- // then-unparsable body would otherwise produce (security review // round 2, 2026-08-22, important finding 4). - if strings.Contains(err.Error(), mcpResponseTooLargeMarker) { + if errors.Is(err, errMCPResponseTooLarge) { writeJSONRPCErrorResponse(w, req.ID, jsonrpcInternalError, "response too large") } else { writeJSONRPCErrorResponse(w, req.ID, jsonrpcInternalError, "upstream error") diff --git a/mcp_federation_test.go b/mcp_federation_test.go index 5bf0582..ed0e5a4 100644 --- a/mcp_federation_test.go +++ b/mcp_federation_test.go @@ -4,6 +4,7 @@ import ( "bytes" "context" "encoding/json" + "errors" "fmt" "io" "net/http" @@ -1072,9 +1073,9 @@ func TestMcpBackendCall_NetworkFailure_NeverAttemptsHandshake(t *testing.T) { // fast, small-scale unit test of doBackendJSONRPC's own maxBytes // mechanism, using an artificially tiny cap so the test needs no // multi-megabyte payload: a backend response over the cap must be -// reported with mcpResponseTooLargeMarker in its message — -// distinguishable via strings.Contains from an ordinary network/parse -// failure — never silently truncated into a confusing parse error. +// reported by wrapping errMCPResponseTooLarge — distinguishable via +// errors.Is from an ordinary network/parse failure — never silently +// truncated into a confusing parse error. func TestMcpBackendCall_ResponseExceedsCap_ReturnsDistinctTooLargeError(t *testing.T) { const tinyCap = 64 big := strings.Repeat("x", tinyCap*4) // well past tinyCap once JSON-encoded @@ -1101,8 +1102,8 @@ func TestMcpBackendCall_ResponseExceedsCap_ReturnsDistinctTooLargeError(t *testi if err == nil { t.Fatal("want an error for a response exceeding the cap") } - if !strings.Contains(err.Error(), mcpResponseTooLargeMarker) { - t.Errorf("err = %v, want it to contain %q", err, mcpResponseTooLargeMarker) + if !errors.Is(err, errMCPResponseTooLarge) { + t.Errorf("err = %v, want it to wrap %v", err, errMCPResponseTooLarge) } } diff --git a/tools/yaegi-check/main.go b/tools/yaegi-check/main.go index ede1ebb..1261538 100644 --- a/tools/yaegi-check/main.go +++ b/tools/yaegi-check/main.go @@ -24,6 +24,7 @@ package main import ( "bufio" + "bytes" "context" "encoding/json" "fmt" @@ -226,6 +227,30 @@ func run() error { })) defer slowUpstream.Close() + // mcpProbeUpstream answers federation's outbound tools/call with a + // body deliberately larger than mcpBackendCallResponseMaxBytes, so + // exerciseHandler's POST /mcp probe below drives doBackendJSONRPC's + // oversize branch — and therefore its errors.Is(err, + // errMCPResponseTooLarge) match — under the REAL interpreter. + // + // This exists because a security review round objected that no gate + // exercised federation's tools/call path interpreted, which left + // errors.Is there justified by reasoning rather than by evidence. + // Reasoning is exactly what the Yaegi traps in this codebase have + // repeatedly defeated, so the path is now covered instead of argued. + mcpProbeUpstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"jsonrpc":"2.0","id":1,"result":{"content":[{"type":"text","text":"`)) + chunk := bytes.Repeat([]byte("x"), 1<<20) + for written := 0; written <= mcpProbeOversizeBytes; written += len(chunk) { + if _, writeErr := w.Write(chunk); writeErr != nil { + return + } + } + _, _ = w.Write([]byte(`"}]}}`)) + })) + defer mcpProbeUpstream.Close() + // builtinLookupContextTokens (review fix, SHOULD-6): read directly // out of the generated pricing_data_gen.go rather than hardcoding a // number, so this assertion self-updates across a `make @@ -252,6 +277,7 @@ func run() error { // runs correctly under Yaegi too, not just the config-override // one testDataWantModel already exercises. `"modelMeta":{"` + testDataWantModel + `":{"contextTokens":` + yaegiMetaContextTokens + `,"inputCostPerMTokMicroUsd":1250000,"outputCostPerMTokMicroUsd":10000000}},` + + `"mcpServers":{"` + mcpProbeServerName + `":{"url":"` + mcpProbeUpstream.URL + `"}},` + `"admin":{"enabled":true},"users":{"inline":[{"name":"tester","group":"default","apiKey":"` + testDataUserAPIKey + `"},{"name":"admin1","group":"default","apiKey":"` + attemptAccountingAdminAPIKey + `","admin":true}]}}` if err = json.Unmarshal([]byte(attemptAccountingOverride), cfgVal.Interface()); err != nil { return fmt.Errorf("decode attempt-accounting harness override into the interpreted Config: %w", err) @@ -298,6 +324,32 @@ func run() error { // the config-override path testDataWantModel already exercises. const builtinLookupModelID = "gpt-4o" +// mcpProbeServerName is the federated MCP server run() configures against +// mcpProbeUpstream, and mcpProbeToolName is a tool id carrying its +// "_" prefix so resolveFederatedTool routes POST /mcp's +// tools/call there. The tool needs no tools/list entry — resolution is +// pure prefix matching on the configured, group-allowed server names. +const ( + mcpProbeServerName = "probe" + mcpProbeToolName = mcpProbeServerName + "_big" +) + +// mcpProbeOversizeBytes is how much filler mcpProbeUpstream writes: over +// mcpBackendCallResponseMaxBytes (maxRequestBytes, 10MiB) so the response +// trips doBackendJSONRPC's cap. Kept as its own named value rather than +// importing the plugin's const, because this harness deliberately builds +// nothing from the plugin package at compile time — everything it asserts +// about the plugin must come from the interpreter. +const mcpProbeOversizeBytes = 11 << 20 + +// mcpTooLargeWantMessage is the exact JSON-RPC error message +// mcpFederatedToolsCall writes when errors.Is matches +// errMCPResponseTooLarge. "upstream error" here instead means errors.Is +// returned false under the interpreter for a chain that is true when +// compiled — which is precisely the class of Yaegi divergence this +// harness exists to catch, and which no compiled test can see. +const mcpTooLargeWantMessage = "response too large" + // readBuiltinContextTokens reads repoRoot/pricing_data_gen.go and // extracts modelID's own ContextTokens value directly out of the // generated source (review fix, SHOULD-6) — rather than hardcoding a @@ -401,6 +453,10 @@ func exerciseHandler(handler http.Handler, builtinContextTokens int) error { return err } + if err := exerciseFederatedTooLarge(handler); err != nil { + return err + } + unauthedReq := httptest.NewRequest(http.MethodGet, "/v1/models", nil) unauthedRec := httptest.NewRecorder() handler.ServeHTTP(unauthedRec, unauthedReq) @@ -439,6 +495,56 @@ func exerciseHandler(handler http.Handler, builtinContextTokens int) error { // a compiled concrete value's method set in Yaegi. matchesSentinel now // uses real errors.Is instead (its own doc comment in limits.go has the // full account); this harness is what caught the difference. +// exerciseFederatedTooLarge drives POST /mcp (federation's tools/call +// path) against mcpProbeUpstream, whose response deliberately exceeds +// mcpBackendCallResponseMaxBytes, and asserts the interpreted plugin +// answers with mcpTooLargeWantMessage rather than the generic "upstream +// error". +// +// What this actually pins is errors.Is under the interpreter. A security +// review round replaced that errors.Is with a strings.Contains marker on +// the grounds that no gate exercised this path interpreted — true at the +// time, and the honest objection. The reasoning for errors.Is is sound +// (errors.New and fmt.Errorf both return COMPILED values, so declaring +// the sentinel variable in interpreted code leaves the whole chain +// errors.Is walks compiled, which is why retry.go's isTransient has +// worked in production since v0.2.0), and a standalone yaegi v0.16.1 +// harness confirmed it. But this codebase's own history — matchesSentinel +// (limits.go) — is a case where sound reasoning about the reflect +// boundary was wrong and only an interpreted probe caught it. So the +// path is covered here rather than argued in a comment. +// +// A failure surfaces as "upstream error": errors.Is returning false +// interpreted for a chain that is true compiled. No `go test` row can +// see that difference. +func exerciseFederatedTooLarge(handler http.Handler) error { + body := `{"jsonrpc":"2.0","id":7,"method":"tools/call","params":{"name":"` + mcpProbeToolName + `","arguments":{}}}` + req := httptest.NewRequest(http.MethodPost, "/mcp", strings.NewReader(body)) + req.Header.Set("Authorization", "Bearer "+testDataUserAPIKey) + req.Header.Set("Content-Type", "application/json") + rec := httptest.NewRecorder() + handler.ServeHTTP(rec, req) + if rec.Code != http.StatusOK { + return fmt.Errorf("POST /mcp tools/call: status = %d, want 200 (JSON-RPC reports the failure in the body, not the status), body=%s", rec.Code, rec.Body.String()) + } + + var resp struct { + Error *struct { + Message string `json:"message"` + } `json:"error"` + } + if err := json.Unmarshal(rec.Body.Bytes(), &resp); err != nil { + return fmt.Errorf("decode POST /mcp tools/call body: %w (body=%s)", err, rec.Body.String()) + } + if resp.Error == nil { + return fmt.Errorf("POST /mcp tools/call returned no JSON-RPC error, want %q — the probe upstream's oversized response should have tripped the cap: %s", mcpTooLargeWantMessage, rec.Body.String()) + } + if resp.Error.Message != mcpTooLargeWantMessage { + return fmt.Errorf("POST /mcp tools/call error message = %q, want %q — errors.Is(err, errMCPResponseTooLarge) did not match under the interpreter (mcp_federation.go)", resp.Error.Message, mcpTooLargeWantMessage) + } + return nil +} + func exerciseAttemptAccounting(handler http.Handler) error { chatReq := httptest.NewRequest(http.MethodPost, "/v1/chat/completions", strings.NewReader( `{"model":"`+testDataWantModel+`","messages":[{"role":"user","content":"hi"}]}`,