diff --git a/chart/templates/deployment.yaml b/chart/templates/deployment.yaml index 0f1e3ed..7437471 100644 --- a/chart/templates/deployment.yaml +++ b/chart/templates/deployment.yaml @@ -109,6 +109,14 @@ spec: - "--token-auth-only" - "true" {{- end }} + {{- if .Values.security.allowedHosts }} + - "--allowed-host" + - {{ join "," .Values.security.allowedHosts | quote }} + {{- end }} + {{- if .Values.security.trustedOrigins }} + - "--trusted-origin" + - {{ join "," .Values.security.trustedOrigins | quote }} + {{- end }} env: {{- if .Values.azure.subscriptionId }} - name: AZURE_SUBSCRIPTION_ID diff --git a/chart/values.yaml b/chart/values.yaml index e05583f..ddda9b1 100644 --- a/chart/values.yaml +++ b/chart/values.yaml @@ -75,6 +75,30 @@ oauth: # Requires azure.clientSecret (or azure.existingSecret with a client-secret key) to be set. oboEnabled: false +# HTTP transport security: DNS-rebinding / cross-origin protection. +# Applies to streamable-http and sse transports only; stdio is unaffected. +# +# The chart deployment hard-codes --host 0.0.0.0 so the listener is always +# routable inside the cluster. When app.transport is streamable-http or sse +# and oauth.enabled is false, the server REFUSES to start unless +# security.allowedHosts is set. Pick one of: +# (a) enable OAuth (oauth.enabled: true), or +# (b) declare an explicit trusted-host allowlist below. +# A default install with neither set will CrashLoopBackOff on purpose. +security: + # HTTP Host header values to accept on /mcp, /sse and /message. + # Entries may include a port (e.g. "aks-mcp.example.com:8000") to match + # only that port, or omit it to match any port for that hostname. + # Use the literal "*" only when an upstream reverse proxy validates Host + # on the operator's behalf. + # Example: ["aks-mcp.internal.example.com"] + allowedHosts: [] + # HTTP Origin header values to accept on browser-style cross-origin requests + # to /mcp, /sse and /message. Empty Origin (non-browser clients) always allowed. + # Use the literal "*" only to disable Origin enforcement entirely. + # Example: ["https://chat.internal.example.com"] + trustedOrigins: [] + # Application configuration config: # Enabled components (empty means all components enabled) diff --git a/cmd/aks-mcp/container_test.go b/cmd/aks-mcp/container_test.go index 09118cb..b8a6373 100644 --- a/cmd/aks-mcp/container_test.go +++ b/cmd/aks-mcp/container_test.go @@ -67,12 +67,21 @@ func TestContainerNetworkTransport(t *testing.T) { t.Skip("Docker image 'aks-mcp:test' not available, skipping network transport test") } - // Start container with default CMD (streamable-http transport) + // Start container with streamable-http transport. After the DNS-rebinding + // fail-closed validator landed, the default CMD (--host 0.0.0.0 with no + // auth and no --allowed-host) is intentionally rejected at startup, so + // this test supplies --allowed-host=localhost to satisfy the validator + // while still exercising the HTTP listener on localhost. ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) defer cancel() // Start container in background - cmd := exec.CommandContext(ctx, "docker", "run", "--rm", "-p", "8000:8000", "aks-mcp:test") + cmd := exec.CommandContext(ctx, "docker", "run", "--rm", "-p", "8000:8000", + "aks-mcp:test", + "--transport", "streamable-http", + "--host", "0.0.0.0", + "--allowed-host", "localhost", + ) // Start the container err := cmd.Start() diff --git a/internal/components/azapi/handler.go b/internal/components/azapi/handler.go index d0f5997..aa9d4e9 100644 --- a/internal/components/azapi/handler.go +++ b/internal/components/azapi/handler.go @@ -164,8 +164,11 @@ func AzApiHandler(azClient azcli.Client, cfg *config.ConfigData) func(ctx contex return mcp.NewToolResultError(errMsg), nil } - if result.Error != "" { - errMsg := fmt.Sprintf("Azure CLI command failed: %s", result.Error) + if result.ExitCode != 0 { + errMsg := fmt.Sprintf("Azure CLI command failed with exit code %d", result.ExitCode) + if result.Error != "" { + errMsg = fmt.Sprintf("%s: %s", errMsg, result.Error) + } logger.Errorf("AzApiHandler: %s", errMsg) if cfg.TelemetryService != nil { cfg.TelemetryService.TrackToolInvocation(ctx, req.Params.Name, sanitizeCliCommand(cliCommand), false) @@ -173,6 +176,10 @@ func AzApiHandler(azClient azcli.Client, cfg *config.ConfigData) func(ctx contex return mcp.NewToolResultError(errMsg), nil } + if result.Error != "" { + logger.Warnf("AzApiHandler: Azure CLI emitted stderr with successful exit code: %s", result.Error) + } + logger.Debugf("AzApiHandler: Command completed successfully") if cfg.TelemetryService != nil { diff --git a/internal/components/azapi/handler_test.go b/internal/components/azapi/handler_test.go index 652e316..5e579ea 100644 --- a/internal/components/azapi/handler_test.go +++ b/internal/components/azapi/handler_test.go @@ -23,8 +23,9 @@ func (m *mockAzClient) ExecuteCommand(ctx context.Context, command string) (*azc return m.executeFunc(ctx, command) } return &azcli.Result{ - Output: json.RawMessage("mock output"), - Error: "", + Output: json.RawMessage("mock output"), + ExitCode: 0, + Error: "", }, nil } @@ -51,8 +52,9 @@ func TestAzApiHandler_Success(t *testing.T) { t.Errorf("expected command 'az group list', got '%s'", command) } return &azcli.Result{ - Output: json.RawMessage(`[{"name":"rg1","location":"eastus"}]`), - Error: "", + Output: json.RawMessage(`[{"name":"rg1","location":"eastus"}]`), + ExitCode: 0, + Error: "", }, nil }, } @@ -185,8 +187,9 @@ func TestAzApiHandler_CommandError(t *testing.T) { mockClient := &mockAzClient{ executeFunc: func(ctx context.Context, command string) (*azcli.Result, error) { return &azcli.Result{ - Output: json.RawMessage(""), - Error: "command error: resource not found", + Output: json.RawMessage(""), + ExitCode: 2, + Error: "command error: resource not found", }, nil }, } @@ -213,6 +216,56 @@ func TestAzApiHandler_CommandError(t *testing.T) { if !result.IsError { t.Fatal("expected error result") } + + textContent, ok := result.Content[0].(mcp.TextContent) + if !ok { + t.Fatalf("expected TextContent, got %T", result.Content[0]) + } + + if !strings.Contains(textContent.Text, "exit code 2") { + t.Errorf("expected exit code in error message, got %s", textContent.Text) + } +} + +func TestAzApiHandler_WarningOnlyResult(t *testing.T) { + mockClient := &mockAzClient{ + executeFunc: func(ctx context.Context, command string) (*azcli.Result, error) { + return &azcli.Result{ + Output: json.RawMessage(`["aks-a","aks-b"]`), + ExitCode: 0, + Error: "WARNING: The behavior of this command has been altered by the following extension: aks-preview", + }, nil + }, + } + + cfg := newTestConfig(30) + handler := AzApiHandler(mockClient, cfg) + + req := mcp.CallToolRequest{ + Params: mcp.CallToolParams{ + Name: "call_az", + Arguments: map[string]interface{}{ + "cli_command": "az aks list --query [].name -o tsv", + }, + }, + } + + result, err := handler(context.Background(), req) + if err != nil { + t.Fatalf("expected no error, got %v", err) + } + if result.IsError { + t.Fatal("expected success result") + } + + textContent, ok := result.Content[0].(mcp.TextContent) + if !ok { + t.Fatalf("expected TextContent, got %T", result.Content[0]) + } + + if textContent.Text != `["aks-a","aks-b"]` { + t.Errorf("unexpected output: %s", textContent.Text) + } } func TestAzApiHandler_CustomTimeout(t *testing.T) { @@ -230,8 +283,9 @@ func TestAzApiHandler_CustomTimeout(t *testing.T) { } } return &azcli.Result{ - Output: json.RawMessage("success"), - Error: "", + Output: json.RawMessage("success"), + ExitCode: 0, + Error: "", }, nil }, } @@ -267,8 +321,9 @@ func TestAzApiHandler_NilTelemetryService(t *testing.T) { mockClient := &mockAzClient{ executeFunc: func(ctx context.Context, command string) (*azcli.Result, error) { return &azcli.Result{ - Output: json.RawMessage(`"ok"`), - Error: "", + Output: json.RawMessage(`"ok"`), + ExitCode: 0, + Error: "", }, nil }, } diff --git a/internal/config/config.go b/internal/config/config.go index 1764fdf..481d875 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -3,6 +3,7 @@ package config import ( "context" "fmt" + "net" "os" "regexp" "strings" @@ -83,6 +84,20 @@ type ConfigData struct { // DefaultAKSResourceID is the default AKS cluster resource ID used when aks_resource_id is not provided by the caller. // Set via --default-aks-resource-id flag or AZURE_AKS_RESOURCE_ID environment variable. DefaultAKSResourceID string + + // AllowedHosts is the set of HTTP Host header values (with or without port) + // that the streamable-http / sse transports will accept. Empty means + // loopback-only (the safe default). The literal "*" disables Host + // enforcement entirely — only use behind a reverse proxy that validates + // Host on the operator's behalf. + AllowedHosts []string + + // AllowedOrigins is the set of HTTP Origin header values that the + // streamable-http / sse transports will accept on browser-style cross + // origin requests. Empty Origin headers (non-browser callers) are always + // allowed. Empty list rejects every non-empty Origin. The literal "*" + // disables Origin enforcement entirely. + AllowedOrigins []string } // NewConfig creates and returns a new configuration instance @@ -100,6 +115,8 @@ func NewConfig() *ConfigData { LogLevel: "info", UseLegacyTools: os.Getenv("USE_LEGACY_TOOLS") == "true", TokenAuthOnly: false, + AllowedHosts: []string{}, + AllowedOrigins: []string{}, } } @@ -143,6 +160,13 @@ func (cfg *ConfigData) ParseFlags() { enabledComponents := flag.String("enabled-components", "", "Comma-separated list of enabled components (empty means all components enabled). Available: az_cli,monitor,fleet,network,compute,detectors,advisor,inspektorgadget,kubectl,helm,cilium,hubble") + // HTTP transport security: DNS-rebinding / cross-origin protections + // (apply to streamable-http and sse transports only). + allowedHosts := flag.String("allowed-host", "", + "Comma-separated list of HTTP Host header values to accept on /mcp, /sse and /message. Empty means loopback only (localhost, 127.0.0.1, [::1]). Use '*' as an escape valve when behind a trusted reverse proxy. Required for non-loopback bindings when OAuth is disabled.") + trustedOrigins := flag.String("trusted-origin", "", + "Comma-separated list of HTTP Origin header values to accept on browser-style cross-origin requests to /mcp, /sse and /message (e.g. https://chat.example.com). Empty Origin headers from non-browser clients are always allowed. Use '*' to disable Origin enforcement entirely.") + // Kubernetes namespaces configuration flag.StringVar(&cfg.AllowNamespaces, "allow-namespaces", "", "Comma-separated list of allowed Kubernetes namespaces (empty means all namespaces)") @@ -214,6 +238,29 @@ func (cfg *ConfigData) ParseFlags() { } } } + + // Parse HTTP transport allowlists. + cfg.AllowedHosts = splitAndTrim(*allowedHosts) + cfg.AllowedOrigins = splitAndTrim(*trustedOrigins) +} + +// splitAndTrim splits raw on commas, trims whitespace, and drops empty entries. +// Returns nil for empty input so a zero allowlist remains a zero allowlist. +func splitAndTrim(raw string) []string { + if raw == "" { + return nil + } + parts := strings.Split(raw, ",") + out := make([]string, 0, len(parts)) + for _, p := range parts { + if v := strings.TrimSpace(p); v != "" { + out = append(out, v) + } + } + if len(out) == 0 { + return nil + } + return out } // parseOAuthConfig parses OAuth-related command line arguments @@ -357,9 +404,53 @@ func (cfg *ConfigData) ValidateConfig() error { return fmt.Errorf("token-only authentication mode (--token-auth-only) requires unified tools and is not compatible with legacy tools (USE_LEGACY_TOOLS=true)") } + // Refuse to start a publicly reachable streamable-http / sse listener + // with no authentication and no explicit trusted-host allowlist. This + // is the DNS-rebinding posture: a browser-origin attacker can otherwise + // reach /mcp through the victim's local network. The operator must pick + // one of three safe combinations: + // (a) bind to loopback (default --host 127.0.0.1) + // (b) enable OAuth (--oauth-enabled) + // (c) declare an explicit trusted-host allowlist (--allowed-host) + if isHTTPTransport(cfg.Transport) && + !isLoopbackBindHost(cfg.Host) && + !cfg.OAuthConfig.Enabled && + len(cfg.AllowedHosts) == 0 { + return fmt.Errorf("transport %q bound to non-loopback host %q without OAuth and without --allowed-host: refusing to start to avoid DNS-rebinding exposure; choose one of: (a) bind --host 127.0.0.1, (b) --oauth-enabled, (c) --allowed-host=", + cfg.Transport, cfg.Host) + } + return nil } +// isHTTPTransport reports whether transport opens an HTTP listener that +// browsers can target. stdio is excluded because it has no HTTP attack surface. +func isHTTPTransport(transport string) bool { + switch transport { + case "streamable-http", "sse": + return true + } + return false +} + +// isLoopbackBindHost reports whether host (as configured via --host) only +// binds to a loopback interface and therefore cannot be reached by a remote +// browser-origin attacker. Used for startup-time safety validation. +// An empty host is treated as loopback because callers that never invoke +// ParseFlags (notably unit tests) leave Host at its zero value, and the +// production default applied by ParseFlags is 127.0.0.1 anyway. +// Any address in 127.0.0.0/8 or the IPv6 ::1 loopback is accepted, matching +// the kernel's notion of "loopback" and the runtime middleware's check. +func isLoopbackBindHost(host string) bool { + if host == "" || host == "localhost" { + return true + } + if ip := net.ParseIP(host); ip != nil && ip.IsLoopback() { + return true + } + return false +} + // InitializeTelemetry initializes the telemetry service func (cfg *ConfigData) InitializeTelemetry(ctx context.Context, serviceName, serviceVersion string) { // Create telemetry configuration diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 076360a..3b103a1 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -667,3 +667,69 @@ func TestValidateConfig_InvalidCombinations(t *testing.T) { }) } } + +func TestValidateConfig_PublicHTTPWithoutAuthOrAllowedHost(t *testing.T) { + tests := []struct { + name string + transport string + host string + wantErr bool + }{ + {name: "streamable-http on 0.0.0.0 without OAuth or allowed-host", transport: "streamable-http", host: "0.0.0.0", wantErr: true}, + {name: "sse on 0.0.0.0 without OAuth or allowed-host", transport: "sse", host: "0.0.0.0", wantErr: true}, + {name: "streamable-http on routable address without OAuth or allowed-host", transport: "streamable-http", host: "10.0.0.5", wantErr: true}, + {name: "streamable-http on loopback (127.0.0.1) is allowed", transport: "streamable-http", host: "127.0.0.1", wantErr: false}, + {name: "streamable-http on loopback (localhost) is allowed", transport: "streamable-http", host: "localhost", wantErr: false}, + {name: "streamable-http on loopback (::1) is allowed", transport: "streamable-http", host: "::1", wantErr: false}, + {name: "streamable-http on 127.0.0.0/8 address is allowed", transport: "streamable-http", host: "127.5.5.5", wantErr: false}, + {name: "sse on loopback is allowed", transport: "sse", host: "127.0.0.1", wantErr: false}, + {name: "stdio transport is never gated", transport: "stdio", host: "0.0.0.0", wantErr: false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cfg := NewConfig() + cfg.Transport = tt.transport + cfg.Host = tt.host + // OAuth disabled, no AllowedHosts. + + err := cfg.ValidateConfig() + if (err != nil) != tt.wantErr { + t.Fatalf("ValidateConfig() error = %v, wantErr %v", err, tt.wantErr) + } + if tt.wantErr { + if !contains(err.Error(), "DNS-rebinding") { + t.Errorf("expected DNS-rebinding rejection, got: %v", err) + } + } + }) + } +} + +func TestValidateConfig_PublicHTTPSafeCombinations(t *testing.T) { + // All three safe combinations for a public HTTP bind must pass. + tests := []struct { + name string + oauthEnabled bool + allowedHosts []string + }{ + {name: "OAuth enabled", oauthEnabled: true, allowedHosts: nil}, + {name: "Explicit allowed-host", oauthEnabled: false, allowedHosts: []string{"aks-mcp.example.com"}}, + {name: "Wildcard allowed-host (reverse-proxy escape valve)", oauthEnabled: false, allowedHosts: []string{"*"}}, + {name: "OAuth + explicit allowed-host", oauthEnabled: true, allowedHosts: []string{"aks-mcp.example.com"}}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cfg := NewConfig() + cfg.Transport = "streamable-http" + cfg.Host = "0.0.0.0" + cfg.OAuthConfig.Enabled = tt.oauthEnabled + cfg.AllowedHosts = tt.allowedHosts + + if err := cfg.ValidateConfig(); err != nil { + t.Fatalf("ValidateConfig() unexpected error: %v", err) + } + }) + } +} diff --git a/internal/server/httpsecurity/host_origin.go b/internal/server/httpsecurity/host_origin.go new file mode 100644 index 0000000..64bf24f --- /dev/null +++ b/internal/server/httpsecurity/host_origin.go @@ -0,0 +1,186 @@ +// Package httpsecurity provides HTTP middleware that guards MCP-bearing +// endpoints against browser-origin cross-origin abuse, in particular DNS +// rebinding attacks. +// +// This middleware is NOT an authentication mechanism. It only enforces that +// the HTTP Host header matches an explicitly trusted name (or a loopback +// default) and that any non-empty Origin header matches an explicitly trusted +// origin. Authentication is the job of the OAuth middleware in +// internal/auth/oauth. +package httpsecurity + +import ( + "encoding/json" + "net" + "net/http" + "strings" + + "github.com/Azure/aks-mcp/internal/logger" +) + +// Config holds the allowlists consulted by the middleware. A zero value falls +// back to the loopback-only Host policy and rejects every non-empty Origin. +type Config struct { + // AllowedHosts is the set of Host header values (with or without port) + // that the middleware will accept. When empty, the middleware defaults + // to allowing only loopback hosts (localhost, 127.0.0.1, [::1]). The + // literal "*" disables Host enforcement entirely; use it only when an + // upstream reverse proxy validates Host on the operator's behalf. + AllowedHosts []string + + // AllowedOrigins is the set of Origin header values (scheme://host[:port]) + // that the middleware will accept on cross-origin browser requests. + // Empty Origin headers (non-browser clients) are always allowed. When + // AllowedOrigins is empty every non-empty Origin is rejected. The + // literal "*" disables Origin enforcement entirely. + AllowedOrigins []string +} + +// NewMiddleware returns an http middleware that enforces cfg's Host and Origin +// policy before delegating to next. Rejected requests receive a 403 response +// with a JSON error body. +func NewMiddleware(cfg Config) func(http.Handler) http.Handler { + return func(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if !isHostAllowed(r.Host, cfg.AllowedHosts) { + writeForbidden(w, r, "host not allowed", r.Host, r.Header.Get("Origin")) + return + } + if !isOriginAllowed(r.Header.Get("Origin"), cfg.AllowedOrigins) { + writeForbidden(w, r, "origin not allowed", r.Host, r.Header.Get("Origin")) + return + } + next.ServeHTTP(w, r) + }) + } +} + +// isHostAllowed reports whether the request's Host header satisfies the +// configured allowlist. With an empty allowlist, only loopback names match. +// +// Allowlist entries may be written with or without a port. Each request is +// compared against both forms of the entry: an entry like +// "aks-mcp.example.com" matches the request regardless of port, while +// "aks-mcp.example.com:8000" matches only that port. +func isHostAllowed(rawHost string, allowed []string) bool { + rawHost = strings.TrimSpace(rawHost) + if rawHost == "" { + // A missing Host header is suspicious on its own — refuse it rather + // than fall through to the default-allow loopback branch. + return false + } + hostNoPort := strings.ToLower(stripPort(rawHost)) + hostFull := strings.ToLower(rawHost) + if hostNoPort == "" { + return false + } + + if len(allowed) == 0 { + return isLoopbackHost(hostNoPort) + } + + for _, candidate := range allowed { + c := strings.ToLower(strings.TrimSpace(candidate)) + if c == "" { + continue + } + if c == "*" { + return true + } + // Entry with explicit port: require exact match against full Host header. + // Entry without port: match against the port-stripped host. + if strings.Contains(c, ":") && !strings.HasPrefix(c, "[") { + // IPv4 or hostname with port (e.g. "example.com:8000"). + if c == hostFull { + return true + } + } else if strings.HasPrefix(c, "[") { + // Bracketed IPv6, with or without port (e.g. "[::1]" or "[::1]:8000"). + if strings.Contains(c, "]:") { + if c == hostFull { + return true + } + } else if stripPort(c) == hostNoPort { + return true + } + } else { + // Plain hostname or IPv4 with no port — compare host-only form. + if c == hostNoPort { + return true + } + } + } + return false +} + +// isOriginAllowed reports whether the request's Origin header satisfies the +// configured allowlist. An empty Origin is always allowed because non-browser +// clients do not send one. +func isOriginAllowed(rawOrigin string, allowed []string) bool { + origin := strings.ToLower(strings.TrimSpace(rawOrigin)) + if origin == "" { + return true + } + + for _, candidate := range allowed { + c := strings.ToLower(strings.TrimSpace(candidate)) + if c == "" { + continue + } + if c == "*" { + return true + } + if c == origin { + return true + } + } + return false +} + +// isLoopbackHost reports whether host (already lowercased, port-stripped) is +// one of the loopback names that the default-empty allowlist permits. +func isLoopbackHost(host string) bool { + switch host { + case "localhost", "127.0.0.1", "::1": + return true + } + // Any IPv4 address in 127.0.0.0/8 is loopback. + if ip := net.ParseIP(host); ip != nil && ip.IsLoopback() { + return true + } + return false +} + +// stripPort removes the optional ":port" suffix from a Host header value, +// handling bracketed IPv6 forms like "[::1]:8080" and "[::1]". +func stripPort(host string) string { + if host == "" { + return "" + } + if strings.HasPrefix(host, "[") { + // Bracketed IPv6 — find matching ']'. + if end := strings.IndexByte(host, ']'); end != -1 { + return host[1:end] + } + return host + } + if h, _, err := net.SplitHostPort(host); err == nil { + return h + } + return host +} + +// writeForbidden returns a 403 JSON error describing the rejection and emits a +// structured warning so operators can detect probing or misconfiguration. +func writeForbidden(w http.ResponseWriter, r *http.Request, reason, host, origin string) { + logger.Warnf( + "httpsecurity: rejected request: reason=%q remote=%s method=%s path=%s host=%q origin=%q", + reason, r.RemoteAddr, r.Method, r.URL.Path, host, origin, + ) + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusForbidden) + _ = json.NewEncoder(w).Encode(map[string]string{ + "error": "forbidden", + "error_description": "request rejected by host/origin policy: " + reason, + }) +} diff --git a/internal/server/httpsecurity/host_origin_test.go b/internal/server/httpsecurity/host_origin_test.go new file mode 100644 index 0000000..e8fcbba --- /dev/null +++ b/internal/server/httpsecurity/host_origin_test.go @@ -0,0 +1,196 @@ +package httpsecurity + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +func TestIsHostAllowed(t *testing.T) { + tests := []struct { + name string + host string + allowed []string + want bool + }{ + // Default (empty allowlist) — loopback only. + {"default allows localhost", "localhost", nil, true}, + {"default allows localhost with port", "localhost:8000", nil, true}, + {"default allows 127.0.0.1", "127.0.0.1", nil, true}, + {"default allows 127.0.0.1 with port", "127.0.0.1:8000", nil, true}, + {"default allows IPv6 loopback bracketed", "[::1]:8000", nil, true}, + {"default allows IPv6 loopback bare", "::1", nil, true}, + {"default allows 127.0.0.0/8", "127.5.5.5", nil, true}, + {"default rejects foreign host", "rebind.example.com", nil, false}, + {"default rejects 0.0.0.0", "0.0.0.0", nil, false}, + {"default rejects empty host", "", nil, false}, + + // Explicit allowlist. + {"explicit allows listed host", "aks-mcp.example.com", []string{"aks-mcp.example.com"}, true}, + {"explicit allows listed host case insensitive", "AKS-MCP.EXAMPLE.COM", []string{"aks-mcp.example.com"}, true}, + {"explicit allows listed host with port", "aks-mcp.example.com:8000", []string{"aks-mcp.example.com"}, true}, + {"explicit rejects unlisted loopback", "127.0.0.1", []string{"aks-mcp.example.com"}, false}, + {"explicit rejects unlisted host", "rebind.example.com", []string{"aks-mcp.example.com"}, false}, + {"explicit ignores empty entries", "aks-mcp.example.com", []string{"", "aks-mcp.example.com", ""}, true}, + + // Wildcard escape valve. + {"wildcard allows anything", "rebind.example.com", []string{"*"}, true}, + {"wildcard with other entries still allows anything", "rebind.example.com", []string{"aks-mcp.example.com", "*"}, true}, + + // Allowlist entries with explicit port: match only that port. + {"entry with port matches same port", "aks-mcp.example.com:8000", []string{"aks-mcp.example.com:8000"}, true}, + {"entry with port rejects different port", "aks-mcp.example.com:9000", []string{"aks-mcp.example.com:8000"}, false}, + {"entry with port rejects portless request", "aks-mcp.example.com", []string{"aks-mcp.example.com:8000"}, false}, + {"entry without port matches any port", "aks-mcp.example.com:9000", []string{"aks-mcp.example.com"}, true}, + {"entry with port case insensitive", "AKS-MCP.EXAMPLE.COM:8000", []string{"aks-mcp.example.com:8000"}, true}, + {"mixed entries (with and without port) both work", "aks-mcp.example.com:9000", []string{"other.example.com:8000", "aks-mcp.example.com"}, true}, + {"IPv6 bracketed entry with port matches", "[2001:db8::1]:8000", []string{"[2001:db8::1]:8000"}, true}, + {"IPv6 bracketed entry without port matches any port", "[2001:db8::1]:9000", []string{"[2001:db8::1]"}, true}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if got := isHostAllowed(tc.host, tc.allowed); got != tc.want { + t.Errorf("isHostAllowed(%q, %v) = %v, want %v", tc.host, tc.allowed, got, tc.want) + } + }) + } +} + +func TestIsOriginAllowed(t *testing.T) { + tests := []struct { + name string + origin string + allowed []string + want bool + }{ + // Empty Origin always allowed. + {"empty origin allowed (no allowlist)", "", nil, true}, + {"empty origin allowed (with allowlist)", "", []string{"http://localhost:8000"}, true}, + + // Empty allowlist rejects any non-empty Origin. + {"non-empty origin rejected with empty allowlist", "http://rebind.example.com:8806", nil, false}, + + // Explicit allowlist. + {"listed origin allowed", "http://localhost:8000", []string{"http://localhost:8000"}, true}, + {"listed origin case insensitive", "HTTP://LOCALHOST:8000", []string{"http://localhost:8000"}, true}, + {"unlisted origin rejected", "http://rebind.example.com", []string{"http://localhost:8000"}, false}, + + // Wildcard escape valve. + {"wildcard allows anything", "http://rebind.example.com", []string{"*"}, true}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if got := isOriginAllowed(tc.origin, tc.allowed); got != tc.want { + t.Errorf("isOriginAllowed(%q, %v) = %v, want %v", tc.origin, tc.allowed, got, tc.want) + } + }) + } +} + +func TestMiddleware(t *testing.T) { + pass := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte("ok")) + }) + + tests := []struct { + name string + cfg Config + requestHost string // value to set on r.Host + requestOrigin string // value to set as Origin header (empty = unset) + wantStatus int + wantBodyPrefix string // partial match on body + }{ + { + name: "loopback default allows curl-style request", + cfg: Config{}, + requestHost: "127.0.0.1:8000", + wantStatus: http.StatusOK, + }, + { + name: "loopback default rejects browser dns-rebind host", + cfg: Config{}, + requestHost: "rebind.example.com:8000", + requestOrigin: "http://rebind.example.com:8000", + wantStatus: http.StatusForbidden, + }, + { + name: "loopback host accepted but foreign origin rejected when allowlist empty", + cfg: Config{}, + requestHost: "127.0.0.1:8000", + requestOrigin: "http://rebind.example.com:8000", + wantStatus: http.StatusForbidden, + }, + { + name: "explicit allowlist accepts matching host and origin", + cfg: Config{AllowedHosts: []string{"aks-mcp.example.com"}, AllowedOrigins: []string{"https://chat.example.com"}}, + requestHost: "aks-mcp.example.com", + requestOrigin: "https://chat.example.com", + wantStatus: http.StatusOK, + }, + { + name: "explicit allowlist rejects loopback when not listed", + cfg: Config{AllowedHosts: []string{"aks-mcp.example.com"}}, + requestHost: "127.0.0.1:8000", + wantStatus: http.StatusForbidden, + }, + { + name: "wildcard origin allowlist passes any origin", + cfg: Config{AllowedOrigins: []string{"*"}}, + requestHost: "127.0.0.1:8000", + requestOrigin: "http://rebind.example.com", + wantStatus: http.StatusOK, + }, + { + name: "wildcard host allowlist passes any host", + cfg: Config{AllowedHosts: []string{"*"}}, + requestHost: "rebind.example.com", + wantStatus: http.StatusOK, + }, + { + name: "ipv6 loopback default allowed", + cfg: Config{}, + requestHost: "[::1]:8000", + requestOrigin: "", + wantStatus: http.StatusOK, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + mw := NewMiddleware(tc.cfg) + handler := mw(pass) + + req := httptest.NewRequest(http.MethodPost, "/mcp", strings.NewReader("")) + req.Host = tc.requestHost + if tc.requestOrigin != "" { + req.Header.Set("Origin", tc.requestOrigin) + } + + rr := httptest.NewRecorder() + handler.ServeHTTP(rr, req) + + if rr.Code != tc.wantStatus { + t.Fatalf("status = %d, want %d (body=%s)", rr.Code, tc.wantStatus, rr.Body.String()) + } + + if tc.wantStatus == http.StatusForbidden { + if ct := rr.Header().Get("Content-Type"); ct != "application/json" { + t.Errorf("Content-Type = %q, want application/json", ct) + } + var body map[string]string + if err := json.Unmarshal(rr.Body.Bytes(), &body); err != nil { + t.Fatalf("body is not valid JSON: %v (body=%s)", err, rr.Body.String()) + } + if body["error"] != "forbidden" { + t.Errorf("body.error = %q, want forbidden", body["error"]) + } + if !strings.Contains(body["error_description"], "host/origin policy") { + t.Errorf("body.error_description = %q, want substring 'host/origin policy'", body["error_description"]) + } + } + }) + } +} diff --git a/internal/server/server.go b/internal/server/server.go index b317fa3..9f163d0 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "fmt" + "net" "net/http" "os" "strings" @@ -27,6 +28,7 @@ import ( "github.com/Azure/aks-mcp/internal/k8s" "github.com/Azure/aks-mcp/internal/logger" "github.com/Azure/aks-mcp/internal/prompts" + "github.com/Azure/aks-mcp/internal/server/httpsecurity" "github.com/Azure/aks-mcp/internal/tools" "github.com/Azure/aks-mcp/internal/version" azapimcp "github.com/Azure/azure-api-mcp/pkg/azcli" @@ -304,17 +306,18 @@ func (s *Service) createCustomSSEServerWithHelp404(sseServer *server.SSEServer, } // Register SSE and Message handlers with authentication if enabled + secMW := s.buildHTTPSecurityMiddleware() if s.cfg.OAuthConfig.Enabled { if s.authMiddleware == nil { logger.Errorf("OAuth is enabled but auth middleware is not initialized - this indicates a bug in server initialization") } // Apply authentication middleware to SSE and Message endpoints - mux.Handle("/sse", s.authMiddleware.Middleware(sseServer.SSEHandler())) - mux.Handle("/message", s.authMiddleware.Middleware(sseServer.MessageHandler())) + mux.Handle("/sse", secMW(s.authMiddleware.Middleware(sseServer.SSEHandler()))) + mux.Handle("/message", secMW(s.authMiddleware.Middleware(sseServer.MessageHandler()))) } else { // Register without authentication - mux.Handle("/sse", sseServer.SSEHandler()) - mux.Handle("/message", sseServer.MessageHandler()) + mux.Handle("/sse", secMW(sseServer.SSEHandler())) + mux.Handle("/message", secMW(sseServer.MessageHandler())) } // Handle all other paths with a helpful 404 response @@ -422,16 +425,7 @@ func (s *Service) Run() error { // Update the mux to use the actual streamable server as the MCP handler if mux, ok := customServer.Handler.(*http.ServeMux); ok { - if s.cfg.OAuthConfig.Enabled { - if s.authMiddleware == nil { - logger.Errorf("OAuth is enabled but auth middleware is not initialized - this indicates a bug in server initialization") - } - // Apply authentication middleware to MCP endpoint - mux.Handle("/mcp", s.authMiddleware.Middleware(streamableServer)) - } else { - // Register without authentication - mux.Handle("/mcp", streamableServer) - } + s.installStreamableMCPHandler(mux, streamableServer) } logger.Infof("Streamable HTTP server listening on %s", addr) @@ -448,6 +442,86 @@ func (s *Service) Run() error { } } +// installStreamableMCPHandler mounts the streamable-http MCP handler on mux at +// /mcp, wrapping it in the host/origin security middleware and (when OAuth is +// enabled) the OAuth auth middleware. Extracted so that tests can exercise the +// security middleware without binding a TCP listener. +func (s *Service) installStreamableMCPHandler(mux *http.ServeMux, streamableServer http.Handler) { + secMW := s.buildHTTPSecurityMiddleware() + if s.cfg.OAuthConfig.Enabled { + if s.authMiddleware == nil { + logger.Errorf("OAuth is enabled but auth middleware is not initialized - this indicates a bug in server initialization") + } + // Apply authentication middleware to MCP endpoint + mux.Handle("/mcp", secMW(s.authMiddleware.Middleware(streamableServer))) + } else { + // Register without authentication + mux.Handle("/mcp", secMW(streamableServer)) + } +} + +// buildHTTPSecurityMiddleware constructs the host/origin middleware used by +// the streamable-http and sse transports. Defaults are chosen so that the +// gate prevents DNS rebinding without breaking common, demonstrably safe +// deployments: +// +// - OAuth enabled: a valid bearer token is already required for tool +// dispatch, and DNS rebinding cannot produce one. Applying the +// Host/Origin allowlist would only break legitimate ingress / reverse- +// proxy deployments that forward the public hostname, so both default +// to "*". Operators who want belt-and-suspenders enforcement on top of +// OAuth can still set --allowed-host / --trusted-origin to tighten. +// - OAuth disabled, loopback bind: the listener is unreachable from a +// remote network in the first place, so the Origin allowlist would only +// reject local browser MCP clients (MCP Inspector etc.) that send an +// Origin like http://localhost:6274. The Host gate keeps blocking the +// DNS-rebinding shape — a foreign Host header against the loopback +// listener — but Origin defaults to "*". +// - OAuth disabled, non-loopback bind: ValidateConfig already refused to +// start unless --allowed-host was set, so this path requires an explicit +// Host allowlist. Origin still defaults to reject-any when unset, since +// in this configuration there is no other authentication and the +// operator opted into a public-facing listener. +func (s *Service) buildHTTPSecurityMiddleware() func(http.Handler) http.Handler { + cfg := httpsecurity.Config{ + AllowedHosts: s.cfg.AllowedHosts, + AllowedOrigins: s.cfg.AllowedOrigins, + } + switch { + case s.cfg.OAuthConfig.Enabled: + if len(cfg.AllowedHosts) == 0 { + cfg.AllowedHosts = []string{"*"} + } + if len(cfg.AllowedOrigins) == 0 { + cfg.AllowedOrigins = []string{"*"} + } + case isLoopbackBindHost(s.cfg.Host): + // Host gate still defaults to loopback-only inside the middleware + // (cfg.AllowedHosts left empty), which is what we want — DNS-rebinding + // requests carry a foreign Host header and are still rejected. + // Origin, however, would otherwise reject MCP Inspector / browser + // MCP clients running on the same machine. + if len(cfg.AllowedOrigins) == 0 { + cfg.AllowedOrigins = []string{"*"} + } + } + return httpsecurity.NewMiddleware(cfg) +} + +// isLoopbackBindHost reports whether host (as configured via --host) only +// binds to a loopback interface. Mirrors internal/config.isLoopbackBindHost +// so the runtime middleware default can match the startup-time validator +// without exporting an internal helper. +func isLoopbackBindHost(host string) bool { + if host == "" || host == "localhost" { + return true + } + if ip := net.ParseIP(host); ip != nil && ip.IsLoopback() { + return true + } + return false +} + // registerAzureComponents registers all Azure tools (AKS operations, monitoring, fleet, network, compute, detectors, advisor) func (s *Service) registerAzureComponents() { logger.Infof("Registering Azure Components...") diff --git a/internal/server/server_test.go b/internal/server/server_test.go index 9968724..131d516 100644 --- a/internal/server/server_test.go +++ b/internal/server/server_test.go @@ -847,3 +847,254 @@ func TestJSONResponseFormat(t *testing.T) { }) } } + +// TestStreamableHTTPHostOriginMiddleware verifies the host/origin security +// middleware is mounted in front of /mcp on the streamable-http transport. +// A foreign Host (DNS-rebinding shape) must be rejected with 403 before +// the request reaches MCP session handling. A loopback Host (the default +// allowlist) must pass the middleware. +func TestStreamableHTTPHostOriginMiddleware(t *testing.T) { + cfg := createTestConfig("readonly", []string{}) + service := NewService(cfg) + if err := service.Initialize(); err != nil { + t.Fatalf("Failed to initialize service: %v", err) + } + + customServer := service.createCustomHTTPServerWithHelp404("127.0.0.1:8000") + mux, ok := customServer.Handler.(*http.ServeMux) + if !ok { + t.Fatalf("expected ServeMux, got %T", customServer.Handler) + } + + // Mount /mcp using the same path Run() takes, but with a sentinel + // downstream handler so we can distinguish "middleware passed through" + // from "middleware blocked". + sentinel := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusTeapot) + }) + service.installStreamableMCPHandler(mux, sentinel) + + cases := []struct { + name string + host string + origin string + wantStatus int + }{ + {name: "loopback host passes", host: "127.0.0.1:8000", wantStatus: http.StatusTeapot}, + {name: "foreign host rejected", host: "rebind.example.com:8000", wantStatus: http.StatusForbidden}, + // Origin enforcement on a loopback bind without OAuth is exercised by + // TestStreamableHTTPHostOriginMiddlewareLoopbackBindRelaxesOrigin and + // TestStreamableHTTPHostOriginMiddlewareNonLoopbackBindStillStrict. + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + req := httptest.NewRequest(http.MethodPost, "/mcp", strings.NewReader("")) + req.Host = tc.host + if tc.origin != "" { + req.Header.Set("Origin", tc.origin) + } + rr := httptest.NewRecorder() + customServer.Handler.ServeHTTP(rr, req) + if rr.Code != tc.wantStatus { + t.Fatalf("status = %d, want %d (body=%s)", rr.Code, tc.wantStatus, rr.Body.String()) + } + }) + } +} + +// TestSSEHostOriginMiddleware verifies the host/origin security middleware +// is mounted in front of /sse and /message on the SSE transport. +func TestSSEHostOriginMiddleware(t *testing.T) { + cfg := createTestConfig("readonly", []string{}) + service := NewService(cfg) + if err := service.Initialize(); err != nil { + t.Fatalf("Failed to initialize service: %v", err) + } + + sseServer := server.NewSSEServer(service.mcpServer) + customServer := service.createCustomSSEServerWithHelp404(sseServer, "127.0.0.1:8081") + + for _, path := range []string{"/sse", "/message"} { + t.Run(path+" foreign host rejected", func(t *testing.T) { + req := httptest.NewRequest(http.MethodGet, path, nil) + req.Host = "rebind.example.com:8081" + req.Header.Set("Origin", "http://rebind.example.com:8081") + rr := httptest.NewRecorder() + customServer.Handler.ServeHTTP(rr, req) + if rr.Code != http.StatusForbidden { + t.Fatalf("status = %d, want 403 (body=%s)", rr.Code, rr.Body.String()) + } + }) + } +} + +// TestStreamableHTTPHostOriginMiddlewareDefaultsWhenOAuthEnabled verifies +// that with OAuth enabled and no explicit Host/Origin allowlist, the +// middleware default is to allow any Host/Origin. A valid bearer token is +// already required for tool dispatch, so the Host/Origin gate would only +// surprise legitimate ingress / reverse-proxy deployments that forward the +// public hostname. +func TestStreamableHTTPHostOriginMiddlewareDefaultsWhenOAuthEnabled(t *testing.T) { + cfg := createTestConfig("readonly", []string{}) + cfg.OAuthConfig.Enabled = true + cfg.Host = "0.0.0.0" + service := NewService(cfg) + if service == nil { + t.Fatal("NewService returned nil") + } + // Don't call Initialize() — it would set up the real OAuth auth middleware, + // which is not what this test exercises. We want to isolate the + // host/origin gate behavior under OAuth-enabled defaults. + + secMW := service.buildHTTPSecurityMiddleware() + sentinel := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusTeapot) + }) + handler := secMW(sentinel) + + req := httptest.NewRequest(http.MethodPost, "/mcp", strings.NewReader("")) + req.Host = "aks-mcp.ingress.example.com" + req.Header.Set("Origin", "https://chat.example.com") + rr := httptest.NewRecorder() + handler.ServeHTTP(rr, req) + if rr.Code != http.StatusTeapot { + t.Fatalf("OAuth-enabled deployment was blocked by host/origin middleware (status=%d body=%s); "+ + "middleware should default to allow-any when OAuth is enabled", rr.Code, rr.Body.String()) + } +} + +// TestStreamableHTTPHostOriginMiddlewareExplicitAllowlistOverridesOAuthDefault +// verifies that an explicit --allowed-host still takes effect even when OAuth +// is enabled. OAuth-aware operators who want belt-and-suspenders Host +// enforcement can opt back in. +func TestStreamableHTTPHostOriginMiddlewareExplicitAllowlistOverridesOAuthDefault(t *testing.T) { + cfg := createTestConfig("readonly", []string{}) + cfg.OAuthConfig.Enabled = true + cfg.Host = "0.0.0.0" + cfg.AllowedHosts = []string{"aks-mcp.ingress.example.com"} + service := NewService(cfg) + if service == nil { + t.Fatal("NewService returned nil") + } + + secMW := service.buildHTTPSecurityMiddleware() + sentinel := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusTeapot) + }) + handler := secMW(sentinel) + + req := httptest.NewRequest(http.MethodPost, "/mcp", strings.NewReader("")) + req.Host = "rebind.example.com" + rr := httptest.NewRecorder() + handler.ServeHTTP(rr, req) + if rr.Code != http.StatusForbidden { + t.Fatalf("explicit allowlist should still reject foreign host even with OAuth enabled, got status %d (body=%s)", + rr.Code, rr.Body.String()) + } +} + +// TestStreamableHTTPHostOriginMiddlewareLoopbackBindRelaxesOrigin verifies +// that on a loopback bind without OAuth, a local browser MCP client whose +// Origin does not match the (empty) trusted-origins list is still accepted. +// The DNS-rebinding shape is still rejected because the Host header gate +// continues to require a loopback Host on the default policy. +func TestStreamableHTTPHostOriginMiddlewareLoopbackBindRelaxesOrigin(t *testing.T) { + cases := []struct { + name string + host string + }{ + {"127.0.0.1 bind", "127.0.0.1"}, + {"localhost bind", "localhost"}, + {"::1 bind", "::1"}, + {"empty bind (test default)", ""}, + {"127.0.0.0/8 bind", "127.5.5.5"}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + cfg := createTestConfig("readonly", []string{}) + cfg.OAuthConfig.Enabled = false + cfg.Host = tc.host + + service := NewService(cfg) + if service == nil { + t.Fatal("NewService returned nil") + } + + secMW := service.buildHTTPSecurityMiddleware() + sentinel := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusTeapot) + }) + handler := secMW(sentinel) + + // MCP Inspector / browser MCP clients hit loopback with an Origin + // from a different localhost port. The middleware should accept + // this even though AllowedOrigins is empty. + req := httptest.NewRequest(http.MethodPost, "/mcp", strings.NewReader("")) + req.Host = "127.0.0.1:8000" + req.Header.Set("Origin", "http://localhost:6274") + rr := httptest.NewRecorder() + handler.ServeHTTP(rr, req) + if rr.Code != http.StatusTeapot { + t.Fatalf("loopback bind without OAuth should accept browser Origin, got status %d (body=%s)", + rr.Code, rr.Body.String()) + } + + // DNS-rebinding shape (foreign Host on the loopback listener) must + // still be rejected. + req = httptest.NewRequest(http.MethodPost, "/mcp", strings.NewReader("")) + req.Host = "rebind.example.com:8000" + req.Header.Set("Origin", "http://rebind.example.com:8000") + rr = httptest.NewRecorder() + handler.ServeHTTP(rr, req) + if rr.Code != http.StatusForbidden { + t.Fatalf("loopback bind without OAuth must still reject foreign Host, got status %d (body=%s)", + rr.Code, rr.Body.String()) + } + }) + } +} + +// TestStreamableHTTPHostOriginMiddlewareNonLoopbackBindStillStrict verifies +// that with OAuth disabled and a non-loopback bind (the path that requires +// --allowed-host to start at all), the middleware still rejects unlisted +// Origins. The loopback Origin relaxation must not leak into this branch. +func TestStreamableHTTPHostOriginMiddlewareNonLoopbackBindStillStrict(t *testing.T) { + cfg := createTestConfig("readonly", []string{}) + cfg.OAuthConfig.Enabled = false + cfg.Host = "0.0.0.0" + cfg.AllowedHosts = []string{"aks-mcp.example.com"} + // AllowedOrigins intentionally left empty. + + service := NewService(cfg) + if service == nil { + t.Fatal("NewService returned nil") + } + + secMW := service.buildHTTPSecurityMiddleware() + sentinel := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusTeapot) + }) + handler := secMW(sentinel) + + req := httptest.NewRequest(http.MethodPost, "/mcp", strings.NewReader("")) + req.Host = "aks-mcp.example.com" + req.Header.Set("Origin", "http://rebind.example.com") + rr := httptest.NewRecorder() + handler.ServeHTTP(rr, req) + if rr.Code != http.StatusForbidden { + t.Fatalf("non-loopback bind without OAuth and empty Origin allowlist must reject foreign Origin, got status %d (body=%s)", + rr.Code, rr.Body.String()) + } + + // A non-browser caller (no Origin) is still allowed. + req = httptest.NewRequest(http.MethodPost, "/mcp", strings.NewReader("")) + req.Host = "aks-mcp.example.com" + rr = httptest.NewRecorder() + handler.ServeHTTP(rr, req) + if rr.Code != http.StatusTeapot { + t.Fatalf("non-browser caller with matching Host should pass, got status %d (body=%s)", + rr.Code, rr.Body.String()) + } +}