fix(auth): reject duplicate and empty user names

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.
This commit is contained in:
2026-08-22 15:26:32 +01:00
parent 800f63ac53
commit d2a0c4860f
3 changed files with 220 additions and 0 deletions
+44
View File
@@ -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)
}