ASI-07: Add mTLS and HMAC request signing for agent↔gateway communication#4679
Conversation
…munication - Add TLS server support: --tls-cert, --tls-key, --tls-ca flags and MCP_GATEWAY_TLS_CERT/MCP_GATEWAY_TLS_KEY/MCP_GATEWAY_CA_CERT env vars. Gateway starts plain HTTP if no certs are configured (backward compatible). - Add mTLS client cert verification when --tls-ca / MCP_GATEWAY_CA_CERT is set. - Add HMAC-SHA256 request signing middleware: --hmac-secret flag and MCP_GATEWAY_HMAC_SECRET env var. Each request must carry X-MCP-Timestamp, X-MCP-Nonce, and X-MCP-Signature headers. Enforces 30-second timestamp window and per-nonce replay protection. - Middleware is backward compatible: disabled when no secret is configured. - Comprehensive tests for TLS loading, mTLS handshake, HMAC validation, replay protection, and all rejection paths. Agent-Logs-Url: https://github.com/github/gh-aw-mcpg/sessions/be0d13b4-c2b0-49cd-9f9b-0f0680c26634 Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Agent-Logs-Url: https://github.com/github/gh-aw-mcpg/sessions/be0d13b4-c2b0-49cd-9f9b-0f0680c26634 Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds opt-in transport- and message-level protections for agent↔gateway traffic (ASI-07), introducing TLS (including mTLS) at the listener level and HMAC request signing with replay protection in the HTTP middleware stack.
Changes:
- Add CLI/env flags for TLS cert/key/CA and an HMAC secret.
- Add TLS config loader and tests covering one-way TLS + mTLS handshake/error paths.
- Add HMAC middleware with timestamp/nonce/signature validation and extensive unit tests; plumb it into routed + unified server creation.
Show a summary per file
| File | Description |
|---|---|
| internal/cmd/root.go | Switches server startup to httpServer.Serve(listener) and optionally wraps listener with TLS; passes HMAC secret into server constructors. |
| internal/cmd/flags_tls.go | Adds TLS/HMAC CLI flags with env var defaults. |
| internal/server/gateway_tls.go | Implements LoadGatewayTLS to load server cert/key and optionally require/verify client certs using a CA. |
| internal/server/gateway_tls_test.go | Tests TLS config loading + live mTLS handshake and error paths. |
| internal/server/hmac.go | Adds HMAC request-signing middleware, nonce cache, and helper for conditional enablement. |
| internal/server/hmac_test.go | Adds unit tests for HMAC validation scenarios and nonce cache concurrency. |
| internal/server/http_helpers.go | Extends wrapWithMiddleware to include HMAC in the middleware stack. |
| internal/server/transport.go | Extends unified server creation signature to accept HMAC secret and wires it into middleware wrapping. |
| internal/server/routed.go | Extends routed server creation signature to accept HMAC secret and wires it into middleware wrapping. |
| internal/server/*_test.go (multiple) | Updates existing tests to match new function signatures (adds hmacSecret argument). |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 18/18 changed files
- Comments generated: 7
| if tlsCertPath != "" && tlsKeyPath != "" { | ||
| tlsCfg, err = server.LoadGatewayTLS(tlsCertPath, tlsKeyPath, tlsCAPath) | ||
| if err != nil { | ||
| _ = listener.Close() | ||
| return fmt.Errorf("failed to configure TLS: %w", err) | ||
| } | ||
| listener = tls.NewListener(listener, tlsCfg) | ||
| scheme := "https" | ||
| if tlsCAPath != "" { | ||
| scheme = "https (mTLS)" | ||
| } | ||
| logger.StartupInfo("TLS enabled: cert=%s, key=%s, ca=%s — listening on %s://%s", tlsCertPath, tlsKeyPath, tlsCAPath, scheme, listenAddr) | ||
| } else { | ||
| logger.StartupInfo("TLS not configured — listening on http://%s (set --tls-cert/--tls-key to enable)", listenAddr) | ||
| } |
There was a problem hiding this comment.
TLS enablement is treated as “on” only when both --tls-cert and --tls-key are set; if a user accidentally provides only one (or sets --tls-ca without cert/key), the server silently falls back to plain HTTP. This is risky because it can lead to unintended plaintext deployments. Consider returning an error when any TLS-related flag is set without the complete required set (cert+key, and optional CA).
| // Build net.Listener — optionally wrapping with TLS (ASI-07 Phase 1). | ||
| // Plain HTTP is still used when no TLS certificate is configured (backward compatible). | ||
| listener, err := net.Listen("tcp", listenAddr) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to listen on %s: %w", listenAddr, err) | ||
| } | ||
| var tlsCfg *tls.Config | ||
| if tlsCertPath != "" && tlsKeyPath != "" { | ||
| tlsCfg, err = server.LoadGatewayTLS(tlsCertPath, tlsKeyPath, tlsCAPath) | ||
| if err != nil { | ||
| _ = listener.Close() | ||
| return fmt.Errorf("failed to configure TLS: %w", err) | ||
| } | ||
| listener = tls.NewListener(listener, tlsCfg) | ||
| scheme := "https" | ||
| if tlsCAPath != "" { | ||
| scheme = "https (mTLS)" | ||
| } | ||
| logger.StartupInfo("TLS enabled: cert=%s, key=%s, ca=%s — listening on %s://%s", tlsCertPath, tlsKeyPath, tlsCAPath, scheme, listenAddr) | ||
| } else { | ||
| logger.StartupInfo("TLS not configured — listening on http://%s (set --tls-cert/--tls-key to enable)", listenAddr) | ||
| } | ||
| if hmacSecret != "" { | ||
| logger.StartupInfo("HMAC request signing enabled (ASI-07)") | ||
| } | ||
|
|
||
| // Start HTTP server in background | ||
| go func() { | ||
| if err := httpServer.ListenAndServe(); err != nil && err != http.ErrServerClosed { | ||
| if err := httpServer.Serve(listener); err != nil && err != http.ErrServerClosed { |
There was a problem hiding this comment.
When TLS is enabled via --tls-cert/--tls-key, the server will speak HTTPS on the listener, but writeGatewayConfigToStdout still emits http://... URLs. This will cause clients that consume the emitted config to attempt plaintext connections and fail. Consider threading the effective scheme (http/https) into the config writer (or deriving it from the TLS flags) so the emitted URLs match the actual listener.
| // If apiKey is provided, all requests except /health require authentication (spec 7.1) | ||
| func CreateHTTPServerForMCP(addr string, unifiedServer *UnifiedServer, apiKey string) *http.Server { | ||
| logTransport.Printf("Creating HTTP server for MCP: addr=%s, auth_enabled=%v", addr, apiKey != "") | ||
| // If hmacSecret is provided, each request must carry a valid HMAC-SHA256 signature (ASI-07) |
There was a problem hiding this comment.
The doc comment says “each request must carry a valid HMAC… signature”, but the HMAC middleware is only applied via wrapWithMiddleware to the /mcp handlers; the common endpoints registered by registerCommonEndpoints (notably /close) are not HMAC-protected. Either tighten the implementation so HMAC is enforced on sensitive common endpoints too, or narrow the wording to “each MCP request” to avoid a misleading security guarantee.
| // If hmacSecret is provided, each request must carry a valid HMAC-SHA256 signature (ASI-07) | |
| // If hmacSecret is provided, each MCP request must carry a valid HMAC-SHA256 signature (ASI-07) |
| // Multiple routes from the same Authorization header share a session | ||
| // If apiKey is provided, all requests except /health require authentication (spec 7.1) | ||
| func CreateHTTPServerForRoutedMode(addr string, unifiedServer *UnifiedServer, apiKey string) *http.Server { | ||
| // If hmacSecret is provided, each request must carry a valid HMAC-SHA256 signature (ASI-07) |
There was a problem hiding this comment.
This comment promises “each request must carry a valid HMAC… signature”, but HMAC is only applied to the /mcp/<server> handlers via wrapWithMiddleware; endpoints like /health and /close are registered separately and are not HMAC-protected. Either apply HMAC to the common endpoints as well (at least /close), or narrow the wording to avoid overstating the protection.
| // If hmacSecret is provided, each request must carry a valid HMAC-SHA256 signature (ASI-07) | |
| // If hmacSecret is provided, routed /mcp/<server> requests must carry a valid | |
| // HMAC-SHA256 signature (ASI-07); common endpoints are registered separately. |
| // Apply auth middleware if API key is configured (spec 7.1) | ||
| authedHandler := applyAuthIfConfigured(apiKey, shutdownHandler.ServeHTTP) | ||
|
|
||
| // Apply HMAC signature verification if secret is configured (ASI-07) | ||
| hmacHandler := applyHMACIfConfigured(hmacSecret, authedHandler) | ||
|
|
||
| // Wrap with OTEL tracing span (outermost, so it covers auth + shutdown + logging) | ||
| tracingHandler := WithOTELTracing(authedHandler, logTag) | ||
| tracingHandler := WithOTELTracing(hmacHandler, logTag) | ||
|
|
There was a problem hiding this comment.
wrapWithMiddleware currently wraps the authed handler with applyHMACIfConfigured, which means HMAC validation runs before API key auth. This contradicts the comment (“auth → HMAC”) and the PR description (“after API key auth”), and it also makes unauthenticated requests pay the HMAC/body-read cost. Swap the wrapping order so that API key auth executes before HMAC validation (i.e., have auth middleware wrap the HMAC-wrapped handler).
See below for a potential fix:
// Apply HMAC signature verification if secret is configured (ASI-07)
hmacHandler := applyHMACIfConfigured(hmacSecret, shutdownHandler.ServeHTTP)
// Apply auth middleware if API key is configured (spec 7.1)
authedHandler := applyAuthIfConfigured(apiKey, hmacHandler)
// Wrap with OTEL tracing span (outermost, so it covers auth + HMAC + shutdown + logging)
tracingHandler := WithOTELTracing(authedHandler, logTag)
| // Validate nonce uniqueness (replay protection) | ||
| if !cache.checkAndSet(nonce) { | ||
| logHMAC.Printf("HMAC rejected: replay detected nonce=%s, remote=%s", nonce, r.RemoteAddr) | ||
| rejectRequest(w, r, http.StatusUnauthorized, "unauthorized", "HMAC nonce already used (replay detected)", "auth", "hmac_validation_failed", "replay_detected") | ||
| return | ||
| } |
There was a problem hiding this comment.
The nonce is recorded (checkAndSet) before the signature is verified. This lets any caller (even with an invalid signature) consume nonces and grow the cache, creating an easy DoS vector and potentially causing legitimate requests to be rejected if they reuse a nonce. Only mark the nonce as “seen” after the HMAC signature has been validated successfully (and consider rejecting obviously malformed nonces before any caching).
| if tlsCAPath != "" { | ||
| scheme = "https (mTLS)" | ||
| } | ||
| logger.StartupInfo("TLS enabled: cert=%s, key=%s, ca=%s — listening on %s://%s", tlsCertPath, tlsKeyPath, tlsCAPath, scheme, listenAddr) |
There was a problem hiding this comment.
The startup log builds scheme as "https (mTLS)" and then formats it as %s://%s, which produces an invalid URL-like string (https (mTLS)://...). Keep the scheme as https for the URL and append an “mTLS enabled” note separately so logs remain parseable/copy-pastable.
| if tlsCAPath != "" { | |
| scheme = "https (mTLS)" | |
| } | |
| logger.StartupInfo("TLS enabled: cert=%s, key=%s, ca=%s — listening on %s://%s", tlsCertPath, tlsKeyPath, tlsCAPath, scheme, listenAddr) | |
| tlsModeNote := "" | |
| if tlsCAPath != "" { | |
| tlsModeNote = " (mTLS enabled)" | |
| } | |
| logger.StartupInfo("TLS enabled: cert=%s, key=%s, ca=%s — listening on %s://%s%s", tlsCertPath, tlsKeyPath, tlsCAPath, scheme, listenAddr, tlsModeNote) |
|
@copilot address this review feedback #4679 (review) |
- Fix nonce DoS: record nonce only after successful signature verification (invalid-sig requests can no longer poison the replay-protection cache); add read-only seenNonce fast-reject before body read - Fix middleware ordering: API key auth now runs before HMAC so unauthenticated requests are rejected cheaply without paying body-read overhead - Fix log message: https (mTLS):// → https:// with separate mTLS note - Add partial-TLS config validation: error when only one of --tls-cert/--tls-key is provided, or --tls-ca is set without cert+key - Thread tlsEnabled into writeGatewayConfig so emitted URLs use https:// when TLS is active - Narrow doc comments on CreateHTTPServerForMCP/ForRoutedMode to accurately state that HMAC applies to /mcp handlers, not common endpoints like /health and /close - Add tests: TestHMACMiddleware_InvalidSigDoesNotPoisonNonceCache, TestNonceCache_SeenNonce, TestWriteGatewayConfig_TLSScheme Agent-Logs-Url: https://github.com/github/gh-aw-mcpg/sessions/07fc0aad-bc5a-4216-9f18-ed549f648a83 Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Addressed all 7 items from the review in commit
Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@copilot make sure that these options are off by default to remain backward compatible |
All four security options are already off by default and the implementation is fully backward compatible:
No flags set = identical behaviour to before this PR. |
Per OWASP Agentic Top 10 ASI-07, the MCP gateway communicated over plain HTTP with only an API key — no transport encryption, no message signing, no replay protection. This adds opt-in mTLS and HMAC-SHA256 request signing.
Phase 1: mTLS
New flags/env vars:
--tls-cert/MCP_GATEWAY_TLS_CERT,--tls-key/MCP_GATEWAY_TLS_KEY,--tls-ca/MCP_GATEWAY_CA_CERT.internal/server/gateway_tls.go—LoadGatewayTLS(cert, key, ca): loads PEM files, setstls.RequireAndVerifyClientCertwhencais non-empty (mTLS)internal/cmd/root.go— wraps the TCP listener with TLS post-bind; plain HTTP remains default (no certs = no change in behaviour)--tls-certand--tls-keymust both be provided together;--tls-carequires cert+key to also be set — prevents silent plaintext fallback from incomplete configwriteGatewayConfignow receives atlsEnabledflag so emitted server URLs usehttps://when TLS is activePhase 2: HMAC Request Signing + Replay Protection
New flag/env var:
--hmac-secret/MCP_GATEWAY_HMAC_SECRET.internal/server/hmac.go—hmacMiddlewareinjected intowrapWithMiddleware(both routed and unified modes); API key auth runs before HMAC so unauthenticated requests are rejected before paying the body-read costX-MCP-Timestamp— must be within ±30 s of server clockX-MCP-Nonce— must not have been seen before (in-process cache, 60 s TTL)X-MCP-Signature—HMAC-SHA256(secret, "timestamp\nnonce\npath\nhex(sha256(body))")seenNoncepre-check provides fast replay rejection before the body-read step/mcphandlers only; common endpoints (/health,/close) are not HMAC-protectedBoth features are opt-in and backward compatible — all flags default to empty/disabled, so omitting them leaves plain-HTTP + API-key behaviour completely unchanged.
Test coverage
gateway_tls_test.go: server-only TLS, mTLS config, live mTLS handshake (proper CA + client cert withExtKeyUsageClientAuth), error pathshmac_test.go: valid signature, missing headers, stale/future timestamps, replay detection, wrong secret, nonce cache concurrency, nonce cache DoS prevention (TestHMACMiddleware_InvalidSigDoesNotPoisonNonceCache),seenNonceread-only pre-check (TestNonceCache_SeenNonce)stdout_config_test.go:TestWriteGatewayConfig_TLSSchemeverifieshttp://vshttps://URL emission based on TLS state