From d2a0c4860f0bbc45eb76e0a026676fc90676de60 Mon Sep 17 00:00:00 2001 From: Lukasz Raczylo Date: Sat, 22 Aug 2026 13:58:48 +0100 Subject: [PATCH] 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) + } +}