fix(security): compare the admin API key in constant time

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)
 }
 

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
+}

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])
+		}
+	}
+}

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_<unixnano>`). 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{}{}
+	}
+}

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)
 }
 

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)
+}