From d9589dae04c343c69a8084b90d6ec7959cd4f3f8 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Tue, 22 Sep 2026 18:13:16 +0300 Subject: [PATCH] fix(security): compare the admin API key in constant time Go's `==` on strings returns at the first differing byte, so the time a request takes to be rejected leaks a prefix of the configured admin key to anyone who can measure it. The server-edition OIDC provider already uses crypto/subtle for its nonce check; the main auth paths were the outlier. Add auth.ConstantTimeEqual (subtle.ConstantTimeCompare on []byte) and route all four admin-key comparison sites through it: - internal/httpapi/server.go authenticateExplicitToken (X-API-Key, ?apikey=) - internal/httpapi/server.go authenticateBearer - internal/server/server.go mcpAuthMiddleware (the /mcp surface) - internal/server/server.go pprof gate The helper rejects an empty candidate or secret itself. That is load-bearing: subtle.ConstantTimeCompare([]byte(""), []byte("")) returns 1, so a bare swap that dropped the surrounding `token != ""` / `cfg.APIKey != ""` guards would make an absent credential authenticate as admin. Also delete the time-based fallback in both copies of generateAPIKey (internal/config, cmd/mcpproxy-tray). That branch was unreachable, not a live vulnerability: crypto/rand.Read is documented since Go 1.24 as never returning an error, terminating the process on a source failure instead, and this module is go 1.26.0. It is removed because nothing in the code said so. Key shape is unchanged (32 random bytes, 64 hex chars) and the signature stays `generateAPIKey() string`, so EnsureAPIKey and its callers are untouched. No behavior change: same accept/reject verdict at every site, agent-token prefix routing still runs ahead of the admin compare, Spec 107 credential precedence untouched. Tests: an AST guard that fails on any `==`/`!=` against .APIKey outside an emptiness guard (it listed all four sites before this change), the ConstantTimeEqual contract including both-empty, a behavior table over all three REST credential sources, and a generateAPIKey shape lock. Co-Authored-By: Claude Opus 5 --- cmd/mcpproxy-tray/main.go | 14 +- internal/auth/compare.go | 28 +++ internal/auth/compare_test.go | 55 ++++++ internal/config/api_key_generation_test.go | 43 +++++ internal/config/config.go | 14 +- .../admin_key_compare_behavior_test.go | 176 ++++++++++++++++++ .../constant_time_key_compare_guard_test.go | 120 ++++++++++++ internal/httpapi/server.go | 8 +- internal/server/server.go | 6 +- 9 files changed, 450 insertions(+), 14 deletions(-) create mode 100644 internal/auth/compare.go create mode 100644 internal/auth/compare_test.go create mode 100644 internal/config/api_key_generation_test.go create mode 100644 internal/httpapi/admin_key_compare_behavior_test.go create mode 100644 internal/httpapi/constant_time_key_compare_guard_test.go diff --git a/cmd/mcpproxy-tray/main.go b/cmd/mcpproxy-tray/main.go index feb123d93..458037dd0 100644 --- a/cmd/mcpproxy-tray/main.go +++ b/cmd/mcpproxy-tray/main.go @@ -82,13 +82,17 @@ func getLogDir() string { return fallback } -// generateAPIKey creates a cryptographically secure random API key +// generateAPIKey creates a cryptographically secure random API key: 32 random +// bytes (256 bits), hex-encoded to 64 characters. +// +// crypto/rand.Read is documented since Go 1.24 as never returning an error and +// always filling b entirely; on a failure of the system source it terminates +// the process (runtime fatal, not a recoverable panic). The former +// time-based fallback branch here was therefore unreachable, and is gone — +// nothing can observe a predictable key from this function. func generateAPIKey() string { bytes := make([]byte, 32) // 32 bytes = 256 bits - if _, err := rand.Read(bytes); err != nil { - // Fallback to less secure method if crypto/rand fails - return fmt.Sprintf("tray_%d", time.Now().UnixNano()) - } + _, _ = rand.Read(bytes) // documented never to fail; see above return hex.EncodeToString(bytes) } diff --git a/internal/auth/compare.go b/internal/auth/compare.go new file mode 100644 index 000000000..56a4df58e --- /dev/null +++ b/internal/auth/compare.go @@ -0,0 +1,28 @@ +package auth + +import "crypto/subtle" + +// ConstantTimeEqual reports whether candidate equals secret, in time that does +// not depend on how many leading bytes they share. +// +// Go's `==` on strings returns at the first differing byte, so using it to +// check a presented credential against a configured one leaks a prefix of the +// secret to a caller who can measure response time. Every comparison that +// decides access should go through this helper instead. +// +// An empty candidate or secret is never a match. This matters: subtle's +// ConstantTimeCompare returns 1 for two empty slices, so a bare swap of +// `token == cfg.APIKey` for subtle.ConstantTimeCompare would make an absent +// credential authenticate as admin wherever the caller's `!= ""` guard was +// dropped. Folding the emptiness check in here means call sites cannot make +// that mistake. +// +// The length of the two values is still compared in variable time (a length +// mismatch returns immediately). That is the standard subtle.ConstantTimeCompare +// contract: credential length is not the secret, its contents are. +func ConstantTimeEqual(candidate, secret string) bool { + if candidate == "" || secret == "" { + return false + } + return subtle.ConstantTimeCompare([]byte(candidate), []byte(secret)) == 1 +} diff --git a/internal/auth/compare_test.go b/internal/auth/compare_test.go new file mode 100644 index 000000000..9373f2f47 --- /dev/null +++ b/internal/auth/compare_test.go @@ -0,0 +1,55 @@ +package auth + +import "testing" + +// TestConstantTimeEqual pins the contract of the shared credential comparison +// helper: a timing-safe equality check that treats an empty credential as a +// non-match. +// +// The empty case is the important one. crypto/subtle.ConstantTimeCompare +// returns 1 for two empty slices, so a naive swap of `token == cfg.APIKey` +// for subtle.ConstantTimeCompare would turn an absent credential into an +// admin match wherever the caller's `!= ""` guard was dropped. The helper +// therefore rejects empty inputs itself. +func TestConstantTimeEqual(t *testing.T) { + tests := []struct { + name string + a string + b string + want bool + }{ + {"equal non-empty", "my-admin-key", "my-admin-key", true}, + {"equal long hex", "0123456789abcdef0123456789abcdef", "0123456789abcdef0123456789abcdef", true}, + {"differs same length", "my-admin-key", "my-admin-keY", false}, + {"differs in length", "my-admin-key", "my-admin-key-longer", false}, + {"empty candidate", "", "my-admin-key", false}, + {"empty secret", "my-admin-key", "", false}, + {"both empty must not match", "", "", false}, + {"prefix of secret", "my-admin", "my-admin-key", false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := ConstantTimeEqual(tt.a, tt.b); got != tt.want { + t.Errorf("ConstantTimeEqual(%q, %q) = %v, want %v", tt.a, tt.b, got, tt.want) + } + }) + } +} + +// TestConstantTimeEqual_Symmetric guards against an implementation that +// short-circuits on one argument only. +func TestConstantTimeEqual_Symmetric(t *testing.T) { + pairs := [][2]string{ + {"a", "b"}, + {"", "b"}, + {"", ""}, + {"same", "same"}, + {"short", "longer-value"}, + } + for _, p := range pairs { + if ConstantTimeEqual(p[0], p[1]) != ConstantTimeEqual(p[1], p[0]) { + t.Errorf("ConstantTimeEqual is not symmetric for (%q, %q)", p[0], p[1]) + } + } +} diff --git a/internal/config/api_key_generation_test.go b/internal/config/api_key_generation_test.go new file mode 100644 index 000000000..de3870145 --- /dev/null +++ b/internal/config/api_key_generation_test.go @@ -0,0 +1,43 @@ +package config + +import ( + "encoding/hex" + "regexp" + "testing" +) + +var hex64 = regexp.MustCompile(`^[0-9a-f]{64}$`) + +// TestGenerateAPIKey_Shape pins the generated key to 32 random bytes rendered +// as 64 lowercase hex characters, and pins out the time-based fallback the +// function used to carry (`mcpproxy_`). That branch was unreachable +// — crypto/rand.Read is documented never to return an error since Go 1.24 — +// but nothing in the code said so, and a reader could not tell a predictable +// key was impossible. The shape assertion keeps it impossible. +func TestGenerateAPIKey_Shape(t *testing.T) { + key := generateAPIKey() + + if !hex64.MatchString(key) { + t.Fatalf("generateAPIKey() = %q; want 64 lowercase hex characters", key) + } + raw, err := hex.DecodeString(key) + if err != nil { + t.Fatalf("generateAPIKey() = %q is not hex: %v", key, err) + } + if len(raw) != 32 { + t.Errorf("generateAPIKey() decodes to %d bytes; want 32 (256 bits)", len(raw)) + } +} + +// TestGenerateAPIKey_Unique is the cheap liveness check on the entropy source: +// a deterministic fallback would make successive keys equal (or near-equal). +func TestGenerateAPIKey_Unique(t *testing.T) { + seen := make(map[string]struct{}, 16) + for i := 0; i < 16; i++ { + key := generateAPIKey() + if _, dup := seen[key]; dup { + t.Fatalf("generateAPIKey() returned a duplicate key on iteration %d: %q", i, key) + } + seen[key] = struct{}{} + } +} diff --git a/internal/config/config.go b/internal/config/config.go index 2e0dbeffe..49c1ac3a6 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -1856,13 +1856,17 @@ func DefaultConfig() *Config { } } -// generateAPIKey creates a cryptographically secure random API key +// generateAPIKey creates a cryptographically secure random API key: 32 random +// bytes (256 bits), hex-encoded to 64 characters. +// +// crypto/rand.Read is documented since Go 1.24 as never returning an error and +// always filling b entirely; on a failure of the system source it terminates +// the process (runtime fatal, not a recoverable panic). The former +// time-based fallback branch here was therefore unreachable, and is gone — +// nothing can observe a predictable key from this function. func generateAPIKey() string { bytes := make([]byte, 32) // 32 bytes = 256 bits - if _, err := rand.Read(bytes); err != nil { - // Fallback to less secure method if crypto/rand fails - return fmt.Sprintf("mcpproxy_%d", time.Now().UnixNano()) - } + _, _ = rand.Read(bytes) // documented never to fail; see above return hex.EncodeToString(bytes) } diff --git a/internal/httpapi/admin_key_compare_behavior_test.go b/internal/httpapi/admin_key_compare_behavior_test.go new file mode 100644 index 000000000..b18eb33b7 --- /dev/null +++ b/internal/httpapi/admin_key_compare_behavior_test.go @@ -0,0 +1,176 @@ +package httpapi + +import ( + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/zap" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/auth" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" +) + +// Behavior lock for the switch from `token == cfg.APIKey` to +// auth.ConstantTimeEqual in authenticateExplicitToken and authenticateBearer. +// +// The compare is timing-safe now, but the accept/reject verdict must be +// byte-for-byte what it was. The case that matters most is the empty +// credential: subtle.ConstantTimeCompare([]byte(""), []byte("")) returns 1, +// so dropping the emptiness guard during the rewrite would turn an absent +// credential into an admin match. Each source is exercised with an empty +// value for exactly that reason. +func TestAdminKeyCompare_BehaviorPreservedAcrossSources(t *testing.T) { + const adminKey = "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef" + + // applySource installs a credential on the request the way a client would. + type sourceFn func(r *http.Request, token string) + + xAPIKey := func(r *http.Request, token string) { r.Header.Set("X-API-Key", token) } + bearer := func(r *http.Request, token string) { r.Header.Set("Authorization", "Bearer "+token) } + query := func(r *http.Request, token string) { + q := r.URL.Query() + q.Set("apikey", token) + r.URL.RawQuery = q.Encode() + } + + sources := []struct { + name string + apply sourceFn + }{ + {"X-API-Key", xAPIKey}, + {"Authorization Bearer", bearer}, + {"apikey query param", query}, + } + + tokens := []struct { + name string + token string + wantCode int + }{ + {"correct key", adminKey, http.StatusOK}, + {"wrong key, same length", "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdee", http.StatusUnauthorized}, + {"wrong key, shorter", "0123456789abcdef", http.StatusUnauthorized}, + {"wrong key, longer", adminKey + "ff", http.StatusUnauthorized}, + {"prefix of the key", adminKey[:32], http.StatusUnauthorized}, + {"empty value", "", http.StatusUnauthorized}, + } + + for _, src := range sources { + for _, tc := range tokens { + t.Run(src.name+"/"+tc.name, func(t *testing.T) { + srv := newAdminKeyTestServer(t, adminKey) + + var capturedCtx *auth.AuthContext + handler := srv.apiKeyAuthMiddleware()(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + capturedCtx = auth.AuthContextFromContext(r.Context()) + w.WriteHeader(http.StatusOK) + })) + + req := httptest.NewRequest("GET", "/test", nil) + src.apply(req, tc.token) + w := httptest.NewRecorder() + handler.ServeHTTP(w, req) + + assert.Equal(t, tc.wantCode, w.Code) + if tc.wantCode == http.StatusOK { + require.NotNil(t, capturedCtx, "AuthContext should be set for an accepted admin key") + assert.True(t, capturedCtx.IsAdmin(), "a correct admin key must yield admin context") + } else { + assert.Nil(t, capturedCtx, "a rejected credential must never reach the handler") + } + }) + } + } +} + +// TestAdminKeyCompare_AgentTokenPrefixStillPreemptsAdminCompare pins the +// ordering the rewrite had to leave alone: an mcp_agt_-prefixed token is +// routed to the agent-token path before the admin compare runs, so it never +// falls through to the admin branch — including when the agent token store is +// unconfigured, which is its own distinct rejection, not an admin miss. +func TestAdminKeyCompare_AgentTokenPrefixStillPreemptsAdminCompare(t *testing.T) { + const adminKey = "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef" + agentToken := auth.TokenPrefixStr + "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + + for _, source := range []string{"X-API-Key", "Bearer"} { + t.Run(source, func(t *testing.T) { + srv := newAdminKeyTestServer(t, adminKey) + + reached := false + handler := srv.apiKeyAuthMiddleware()(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + reached = true + w.WriteHeader(http.StatusOK) + })) + + req := httptest.NewRequest("GET", "/test", nil) + if source == "X-API-Key" { + req.Header.Set("X-API-Key", agentToken) + } else { + req.Header.Set("Authorization", "Bearer "+agentToken) + } + w := httptest.NewRecorder() + handler.ServeHTTP(w, req) + + assert.False(t, reached, "an agent token must not be admitted as admin") + assert.Equal(t, http.StatusUnauthorized, w.Code) + // The agent-token path (no token store configured here) says so + // explicitly; the admin path's message is "Invalid or missing API + // key". Seeing the former proves the prefix routing ran first. + assert.Contains(t, w.Body.String(), "Agent tokens are not configured", + "the mcp_agt_ prefix must route to the agent-token path, not the admin compare") + }) + } +} + +// TestAdminKeyCompare_EmptyConfiguredKeyAdmitsNobody covers the empty/empty +// case at middleware level, which the table above cannot reach because it +// always configures a real key. +// +// This is the failure mode a careless constant-time rewrite produces: +// subtle.ConstantTimeCompare of two empty slices returns 1, so an +// unconfigured server presented with an absent credential would hand out +// admin context. Two independent things prevent it — apiKeyAuthMiddleware +// rejects an empty cfg.APIKey before any compare runs, and ConstantTimeEqual +// rejects empty arguments — and this pins the outer one. +func TestAdminKeyCompare_EmptyConfiguredKeyAdmitsNobody(t *testing.T) { + presented := []struct { + name string + apply func(r *http.Request) + }{ + {"no credential", func(*http.Request) {}}, + {"empty X-API-Key", func(r *http.Request) { r.Header.Set("X-API-Key", "") }}, + {"empty Bearer", func(r *http.Request) { r.Header.Set("Authorization", "Bearer ") }}, + {"empty query apikey", func(r *http.Request) { r.URL.RawQuery = "apikey=" }}, + {"some key", func(r *http.Request) { r.Header.Set("X-API-Key", "anything") }}, + } + + for _, tc := range presented { + t.Run(tc.name, func(t *testing.T) { + srv := newAdminKeyTestServer(t, "") // no API key configured + + reached := false + handler := srv.apiKeyAuthMiddleware()(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + reached = true + w.WriteHeader(http.StatusOK) + })) + + req := httptest.NewRequest("GET", "/test", nil) + tc.apply(req) + w := httptest.NewRecorder() + handler.ServeHTTP(w, req) + + assert.False(t, reached, "an unconfigured API key must admit nobody") + assert.Equal(t, http.StatusUnauthorized, w.Code) + }) + } +} + +func newAdminKeyTestServer(t *testing.T, apiKey string) *Server { + t.Helper() + logger := zap.NewNop().Sugar() + ctrl := &testControllerWithConfig{cfg: &config.Config{APIKey: apiKey}} + return NewServer(ctrl, logger, nil) +} diff --git a/internal/httpapi/constant_time_key_compare_guard_test.go b/internal/httpapi/constant_time_key_compare_guard_test.go new file mode 100644 index 000000000..48a24cdcc --- /dev/null +++ b/internal/httpapi/constant_time_key_compare_guard_test.go @@ -0,0 +1,120 @@ +package httpapi + +import ( + "fmt" + "go/ast" + "go/parser" + "go/token" + "os" + "path/filepath" + "sort" + "strconv" + "strings" + "testing" +) + +// Guard: the global admin API key must never be compared with Go's `==`/`!=` +// string operators on an authentication path. +// +// `==` on strings short-circuits at the first differing byte, so the time a +// request takes to be rejected leaks a prefix of the configured key to anyone +// who can measure it. Every admin-key comparison that decides access must go +// through auth.ConstantTimeEqual (crypto/subtle.ConstantTimeCompare on +// []byte), which the server-edition OIDC provider already uses for its nonce +// check. +// +// The walk uses go/parser directly (precedent: +// internal/config/latent_symbols_guard_test.go), so build tags are irrelevant +// and the same verdict holds under `-tags server`. Emptiness guards +// (`cfg.APIKey == ""` / `!= ""`) are NOT violations: they are exactly the +// checks that must survive the rewrite, because +// subtle.ConstantTimeCompare([]byte(""), []byte("")) returns 1 and an absent +// credential would otherwise match. +var constantTimeGuardFiles = []string{ + "internal/httpapi/server.go", + "internal/server/server.go", +} + +func TestAdminKeyNeverComparedWithStringEquality(t *testing.T) { + root := constantTimeGuardRepoRoot(t) + fset := token.NewFileSet() + + var violations []string + for _, rel := range constantTimeGuardFiles { + path := filepath.Join(root, filepath.FromSlash(rel)) + f, err := parser.ParseFile(fset, path, nil, parser.SkipObjectResolution) + if err != nil { + t.Fatalf("parse %s: %v", rel, err) + } + ast.Inspect(f, func(n ast.Node) bool { + bin, ok := n.(*ast.BinaryExpr) + if !ok { + return true + } + if bin.Op != token.EQL && bin.Op != token.NEQ { + return true + } + if !isAPIKeySelector(bin.X) && !isAPIKeySelector(bin.Y) { + return true + } + // Emptiness guards are required, not forbidden. + if isEmptyStringLiteral(bin.X) || isEmptyStringLiteral(bin.Y) { + return true + } + pos := fset.Position(bin.Pos()) + violations = append(violations, fmt.Sprintf("%s:%d: %s %s %s", + rel, pos.Line, exprString(bin.X), bin.Op, exprString(bin.Y))) + return true + }) + } + + if len(violations) > 0 { + sort.Strings(violations) + t.Errorf("admin API key compared with string equality at %d site(s); use auth.ConstantTimeEqual instead:\n %s", + len(violations), strings.Join(violations, "\n ")) + } +} + +// isAPIKeySelector reports whether e is a selector expression whose final +// field is APIKey (cfg.APIKey, s.cfg.APIKey, ...). +func isAPIKeySelector(e ast.Expr) bool { + sel, ok := e.(*ast.SelectorExpr) + return ok && sel.Sel != nil && sel.Sel.Name == "APIKey" +} + +func isEmptyStringLiteral(e ast.Expr) bool { + lit, ok := e.(*ast.BasicLit) + if !ok || lit.Kind != token.STRING { + return false + } + v, err := strconv.Unquote(lit.Value) + return err == nil && v == "" +} + +// exprString renders the simple expression shapes this guard reports. +func exprString(e ast.Expr) string { + switch v := e.(type) { + case *ast.Ident: + return v.Name + case *ast.SelectorExpr: + return exprString(v.X) + "." + v.Sel.Name + case *ast.BasicLit: + return v.Value + case *ast.CallExpr: + return exprString(v.Fun) + "(...)" + default: + return fmt.Sprintf("%T", e) + } +} + +func constantTimeGuardRepoRoot(t *testing.T) string { + t.Helper() + root, err := filepath.Abs(filepath.Join("..", "..")) + if err != nil { + t.Fatalf("resolve repo root: %v", err) + } + if _, err := os.Stat(filepath.Join(root, "go.mod")); err != nil { + t.Fatalf("repo root %s has no go.mod: %v", root, err) + } + return root +} diff --git a/internal/httpapi/server.go b/internal/httpapi/server.go index 894093ae9..ef1c29d7a 100644 --- a/internal/httpapi/server.go +++ b/internal/httpapi/server.go @@ -563,7 +563,9 @@ func (s *Server) authenticateExplicitToken(w http.ResponseWriter, r *http.Reques s.handleAgentTokenAuth(w, r, next, token) return } - if token != "" && token == cfg.APIKey { + // Timing-safe compare. ConstantTimeEqual also rejects an empty token, so + // it subsumes the previous `token != ""` guard. + if auth.ConstantTimeEqual(token, cfg.APIKey) { s.logger.Debugw("TCP connection with valid API key", zap.String("path", r.URL.Path), zap.String("remote_addr", r.RemoteAddr)) @@ -590,7 +592,9 @@ func (s *Server) authenticateBearer(w http.ResponseWriter, r *http.Request, next s.handleAgentTokenAuth(w, r, next, token) return } - if token != "" && token == cfg.APIKey { + // Timing-safe compare. ConstantTimeEqual also rejects an empty token, so + // it subsumes the previous `token != ""` guard. + if auth.ConstantTimeEqual(token, cfg.APIKey) { s.logger.Debugw("TCP connection with valid API key", zap.String("path", r.URL.Path), zap.String("remote_addr", r.RemoteAddr)) diff --git a/internal/server/server.go b/internal/server/server.go index 1a9a776bd..134c664ae 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -542,7 +542,9 @@ func (s *Server) mcpAuthMiddleware(next http.Handler) http.Handler { // Check if it matches the global API key — treat as admin cfg := s.runtime.Config() - if cfg != nil && cfg.APIKey != "" && token == cfg.APIKey { + // Timing-safe compare; ConstantTimeEqual also rejects an empty key or + // token, subsuming the previous `cfg.APIKey != ""` guard. + if cfg != nil && auth.ConstantTimeEqual(token, cfg.APIKey) { ctx := auth.WithAuthContext(r.Context(), credentialKindContext(auth.AdminContext(), auth.CredentialKindAPIKey)) next.ServeHTTP(w, r.WithContext(ctx)) return @@ -3184,7 +3186,7 @@ func (s *Server) startCustomHTTPServer(ctx context.Context, streamableServer *se token = strings.TrimPrefix(h, "Bearer ") } } - if token != cfg.APIKey { + if !auth.ConstantTimeEqual(token, cfg.APIKey) { http.Error(w, "unauthorized", http.StatusUnauthorized) return }