Conversation
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 <noreply@anthropic.com>
Deploying mcpproxy-docs with
|
| Latest commit: |
d9589da
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://bff54196.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://fix-constant-time-key-compar.mcpproxy-docs.pages.dev |
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
📦 Build ArtifactsWorkflow Run: View Run Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 35745844810 --repo smart-mcp-proxy/mcpproxy-go
|
|
Second-lens review (SEC-05): no changes applied. Re-verified the one open finding from the zcode review round:
Confirmed this is accurate but not a current gap: a repo-wide grep for Considered widening the guard to walk the whole repo, but that's not the small/low-risk change it looks like: two legitimate non-attacker-facing
A repo-wide walk would false-positive on both immediately, so doing this safely means classifying "attacker input vs. secret" comparisons apart from "trusted config vs. trusted config" ones across the codebase (or hand-maintaining an allowlist) — real design work, not a mechanical extension. Alias-tracking ( No behavior or test changes in this pass. |
This branch has not been deployed
2 participants
What
Compare the global admin API key in constant time, and delete two copies of an unreachable fallback in
generateAPIKey.auth.ConstantTimeEqual(internal/auth/compare.go) wrapscrypto/subtle.ConstantTimeCompareon[]byteand, deliberately, returnsfalsewhen either side is empty.internal/httpapi/server.go—authenticateExplicitToken(X-API-Key /?apikey=)internal/httpapi/server.go—authenticateBearerinternal/server/server.go—mcpAuthMiddleware(the/mcpsurface)internal/server/server.go— the pprof gategenerateAPIKeyininternal/config/config.goandcmd/mcpproxy-tray/main.goloses itsfmt.Sprintf("mcpproxy_%d"/"tray_%d", time.Now().UnixNano())fallback. Output shape is unchanged (32 random bytes, 64 hex chars).Why
==on strings returns at the first differing byte, so rejection time leaks a prefix of the configured key to anyone who can measure it. The server-edition OIDC provider already usessubtle.ConstantTimeCompare(internal/serveredition/auth/oidc_provider.go); the main auth paths were the outlier.The brief named two sites; there were four.
/mcpand the pprof gate carry the same weakness, so they are fixed here rather than left for a follow-up — it is the same one-line change.The deleted
generateAPIKeyfallback was unreachable, not a live vulnerability.crypto/rand.Readis documented since Go 1.24 as never returning an error and always filling the buffer; on a failure of the system source it terminates the process with a runtime fatal rather than returning. This module isgo 1.26.0, so that branch could not execute and no predictable key was ever observable from it. It is removed because nothing in the code said so and a reader could not tell.generateAPIKey() stringkeeps its signature — returning an error would ripple intoEnsureAPIKey()and its two callers for no gain.The empty-string rejection in the helper is load-bearing:
subtle.ConstantTimeCompare([]byte(""), []byte(""))returns 1, so a bare swap that dropped the surroundingtoken != ""/cfg.APIKey != ""guards would make an absent credential authenticate as admin. Folding the check into the helper means call sites cannot make that mistake.No behavior change: same accept/reject verdict at every site, agent-token (
mcp_agt_) prefix routing still runs ahead of the admin compare, and Spec 107 credential precedence is untouched.How verified
internal/httpapi/constant_time_key_compare_guard_test.gois an AST guard (precedent:internal/config/latent_symbols_guard_test.go) that fails on any==/!=against.APIKeyoutside an emptiness guard — it listed all four sites before the fix.internal/auth/compare_test.gofailed to compile against the missing helper.go test -race ./internal/auth/... ./internal/httpapi/... ./internal/config/...and the server-edition race run with theunit-tests.ymlskip regex overserveredition,config,oauth,server,httpapi,storage— all green.--build-tags server) clean for the touched files.X-API-Key,Authorization: Bearerand?apikey=, plus the pprof gate and a/mcpinitializewith the admin key.codex exec --model gpt-5.6-sol.🤖 Generated with Claude Code