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 }