From 348e9af298ca7b91ed951f97fc6cc984c37763d1 Mon Sep 17 00:00:00 2001 From: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com> Date: Sun, 10 May 2026 14:28:49 +0100 Subject: [PATCH 1/7] Add wirefmt package and HeaderForwardConfig types The wirefmt package centralizes the env-var encoding shared between the operator (which emits TOOLHIVE_HEADER_FORWARD_ manifests) and the vMCP runtime (which parses them). HeaderForwardConfig and the Backend / BackendTarget fields carry per-backend header forwarding state through the vMCP domain types. --- pkg/vmcp/headerforward/wirefmt/wirefmt.go | 92 ++++++++++++ .../headerforward/wirefmt/wirefmt_test.go | 132 ++++++++++++++++++ pkg/vmcp/registry.go | 1 + pkg/vmcp/types.go | 31 ++++ 4 files changed, 256 insertions(+) create mode 100644 pkg/vmcp/headerforward/wirefmt/wirefmt.go create mode 100644 pkg/vmcp/headerforward/wirefmt/wirefmt_test.go diff --git a/pkg/vmcp/headerforward/wirefmt/wirefmt.go b/pkg/vmcp/headerforward/wirefmt/wirefmt.go new file mode 100644 index 0000000000..3d5a3bbf2e --- /dev/null +++ b/pkg/vmcp/headerforward/wirefmt/wirefmt.go @@ -0,0 +1,92 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +// Package wirefmt is the shared wire-format contract for headerForward +// data shipped from the operator to the vMCP runtime via pod env vars. +// +// Both the operator (producer) and the vMCP runtime (consumer) import this +// package: the operator emits TOOLHIVE_HEADER_FORWARD_ +// env vars and the runtime parses them at startup. Living in pkg/ rather +// than under cmd/thv-operator preserves one-way Go layering — the +// operator binary depends on pkg/ helpers, never the reverse. +package wirefmt + +import ( + "fmt" + "regexp" + "strings" +) + +// ManifestEnvVarPrefix is the prefix the operator uses when emitting one +// JSON-encoded manifest env var per backend on a workload pod for +// MCPServerEntry / MCPRemoteProxy headerForward configuration. +// +// The full env var name is "TOOLHIVE_HEADER_FORWARD_"; +// the value is JSON-encoded vmcp.HeaderForwardConfig with original header +// names preserved (uppercased/sanitized normalization can't recover hyphens +// or original casing on the receive side, so we carry the literal names in +// the JSON value instead). Plaintext header values appear inline; secret- +// backed entries carry only the secret identifier — the actual Secret +// value rides a sibling TOOLHIVE_SECRET_HEADER_FORWARD__ env var +// emitted via valueFrom.secretKeyRef and resolved at request time by +// secrets.EnvironmentProvider. +const ManifestEnvVarPrefix = "TOOLHIVE_HEADER_FORWARD_" + +// envVarSanitizer matches any character outside the env-var-safe set +// [A-Z0-9_]. Used by NormalizeForEnvVar to collapse non-conforming chars. +var envVarSanitizer = regexp.MustCompile(`[^A-Z0-9_]`) + +// NormalizeForEnvVar applies the canonical sanitization used in every +// header-forward env-var name: uppercase, hyphens to underscores, anything +// outside [A-Z0-9_] also to underscore. The secret-ref and the manifest +// emitters MUST share this function so a header that round-trips through +// one branch never collides with the other. +// +// Collision domain: this normalization is one-way and lossy. Two distinct +// header names that differ only in non-[A-Z0-9_-] characters will collapse +// to the same normalized form (e.g. "X-A.B" and "X-A_B" both become +// "X_A_B"). For backend (owner) names this is mitigated by Kubernetes +// DNS-1123 subdomain validation, which forbids underscores in resource +// names — two K8s resources cannot present a colliding pair. Header names +// don't have that guard, so callers iterating env vars should log when +// they see the same normalized owner key twice; readers that decode a +// single-backend manifest are not affected because the JSON value carries +// the literal user-authored header names verbatim. +func NormalizeForEnvVar(s string) string { + upper := strings.ToUpper(strings.ReplaceAll(s, "-", "_")) + return envVarSanitizer.ReplaceAllString(upper, "_") +} + +// SecretEnvVarName generates the environment variable name for a header +// forward secret. The generated name follows the TOOLHIVE_SECRET_ +// pattern expected by secrets.EnvironmentProvider. +// +// Parameters: +// - ownerName: the resource owning the header forwarding configuration +// (an MCPRemoteProxy or an MCPServerEntry). +// - headerName: the HTTP header name (e.g. "X-API-Key"). +// +// Returns the full env var name (e.g. "TOOLHIVE_SECRET_HEADER_FORWARD_X_API_KEY_MY_OWNER") +// and the secret identifier (the env var name with the leading +// "TOOLHIVE_SECRET_" stripped, used by secrets.EnvironmentProvider lookups). +func SecretEnvVarName(ownerName, headerName string) (envVarName, secretIdentifier string) { + sanitizedHeader := NormalizeForEnvVar(headerName) + sanitizedOwner := NormalizeForEnvVar(ownerName) + secretIdentifier = fmt.Sprintf("HEADER_FORWARD_%s_%s", sanitizedHeader, sanitizedOwner) + envVarName = fmt.Sprintf("TOOLHIVE_SECRET_%s", secretIdentifier) + return envVarName, secretIdentifier +} + +// ManifestEnvVarName generates the environment variable name for the +// JSON-encoded headerForward manifest emitted per backend on a workload +// pod. One env var per backend; the JSON value carries every configured +// header (plaintext values inline, secret entries by identifier). +// +// Returns the full env var name (e.g. "TOOLHIVE_HEADER_FORWARD_MY_OWNER") +// and the normalized owner segment ("MY_OWNER") used when iterating env +// vars matching ManifestEnvVarPrefix. +func ManifestEnvVarName(ownerName string) (envVarName, normalizedOwner string) { + normalizedOwner = NormalizeForEnvVar(ownerName) + envVarName = ManifestEnvVarPrefix + normalizedOwner + return envVarName, normalizedOwner +} diff --git a/pkg/vmcp/headerforward/wirefmt/wirefmt_test.go b/pkg/vmcp/headerforward/wirefmt/wirefmt_test.go new file mode 100644 index 0000000000..54e095a354 --- /dev/null +++ b/pkg/vmcp/headerforward/wirefmt/wirefmt_test.go @@ -0,0 +1,132 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package wirefmt + +import ( + "regexp" + "testing" + + "github.com/stretchr/testify/assert" +) + +// envVarPattern is the character set every env var name produced by this +// package must conform to. The runtime's secrets.EnvironmentProvider lookups +// expect [A-Z0-9_], so every name we emit MUST round-trip cleanly. +var envVarPattern = regexp.MustCompile(`^[A-Z0-9_]+$`) + +func TestNormalizeForEnvVar(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + in string + want string + }{ + {"hyphens become underscores", "my-proxy", "MY_PROXY"}, + {"already uppercase preserved", "FOO_BAR", "FOO_BAR"}, + {"non-alnum collapses to underscore", "proxy.name@123", "PROXY_NAME_123"}, + {"mixed case lowercased to upper", "GitHub-Copilot", "GITHUB_COPILOT"}, + {"empty stays empty", "", ""}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + got := NormalizeForEnvVar(tt.in) + assert.Equal(t, tt.want, got) + }) + } +} + +func TestSecretEnvVarName(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + ownerName string + headerName string + expectedEnvVarName string + expectedSecretIdentifier string + }{ + { + name: "simple names", + ownerName: "my-proxy", + headerName: "X-API-Key", + expectedEnvVarName: "TOOLHIVE_SECRET_HEADER_FORWARD_X_API_KEY_MY_PROXY", + expectedSecretIdentifier: "HEADER_FORWARD_X_API_KEY_MY_PROXY", + }, + { + name: "lowercase header upcases", + ownerName: "test-proxy", + headerName: "authorization", + expectedEnvVarName: "TOOLHIVE_SECRET_HEADER_FORWARD_AUTHORIZATION_TEST_PROXY", + expectedSecretIdentifier: "HEADER_FORWARD_AUTHORIZATION_TEST_PROXY", + }, + { + name: "multiple hyphens", + ownerName: "my-remote-proxy", + headerName: "X-Custom-Header", + expectedEnvVarName: "TOOLHIVE_SECRET_HEADER_FORWARD_X_CUSTOM_HEADER_MY_REMOTE_PROXY", + expectedSecretIdentifier: "HEADER_FORWARD_X_CUSTOM_HEADER_MY_REMOTE_PROXY", + }, + { + name: "special characters in owner name", + ownerName: "proxy.name@123", + headerName: "X-Token", + expectedEnvVarName: "TOOLHIVE_SECRET_HEADER_FORWARD_X_TOKEN_PROXY_NAME_123", + expectedSecretIdentifier: "HEADER_FORWARD_X_TOKEN_PROXY_NAME_123", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + envVarName, secretIdentifier := SecretEnvVarName(tt.ownerName, tt.headerName) + + assert.Equal(t, tt.expectedEnvVarName, envVarName) + assert.Equal(t, tt.expectedSecretIdentifier, secretIdentifier) + + assert.Regexp(t, envVarPattern, envVarName) + assert.Regexp(t, envVarPattern, secretIdentifier) + assert.Equal(t, "TOOLHIVE_SECRET_"+secretIdentifier, envVarName, + "envVarName must equal TOOLHIVE_SECRET_ + secretIdentifier") + }) + } +} + +func TestManifestEnvVarName(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + ownerName string + expectedEnvVarName string + expectedNormalizedOwn string + }{ + { + name: "hyphen owner", + ownerName: "github-copilot", + expectedEnvVarName: "TOOLHIVE_HEADER_FORWARD_GITHUB_COPILOT", + expectedNormalizedOwn: "GITHUB_COPILOT", + }, + { + name: "already uppercase", + ownerName: "STRIPE", + expectedEnvVarName: "TOOLHIVE_HEADER_FORWARD_STRIPE", + expectedNormalizedOwn: "STRIPE", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + envVarName, normalizedOwner := ManifestEnvVarName(tt.ownerName) + assert.Equal(t, tt.expectedEnvVarName, envVarName) + assert.Equal(t, tt.expectedNormalizedOwn, normalizedOwner) + assert.Regexp(t, envVarPattern, envVarName) + assert.Equal(t, ManifestEnvVarPrefix+normalizedOwner, envVarName, + "envVarName must equal ManifestEnvVarPrefix + normalizedOwner") + }) + } +} diff --git a/pkg/vmcp/registry.go b/pkg/vmcp/registry.go index ba9f2c6b81..ccce6e321c 100644 --- a/pkg/vmcp/registry.go +++ b/pkg/vmcp/registry.go @@ -373,6 +373,7 @@ func BackendToTarget(backend *Backend) *BackendTarget { AuthConfig: backend.AuthConfig, SessionAffinity: false, // TODO: Add session affinity support in future phases HealthStatus: backend.HealthStatus, + HeaderForward: backend.HeaderForward, Metadata: backend.Metadata, } } diff --git a/pkg/vmcp/types.go b/pkg/vmcp/types.go index 025fd3d746..8867da1131 100644 --- a/pkg/vmcp/types.go +++ b/pkg/vmcp/types.go @@ -15,6 +15,27 @@ import ( // This file contains shared domain types used across multiple vmcp subpackages. // Following DDD principles, these are core domain concepts that cross bounded contexts. +// HeaderForwardConfig configures HTTP headers injected into outbound requests +// from the vMCP runtime to a static backend. AddPlaintextHeaders carries literal +// values; AddHeadersFromSecret maps header names to secret identifiers resolved +// via secrets.EnvironmentProvider at request time (env var TOOLHIVE_SECRET_). +// +// Secret values MUST NOT appear in this struct — only identifiers. The operator +// injects actual Secret values into the vMCP pod as env vars via +// valueFrom.secretKeyRef at Deployment rendering time. Plaintext values are +// also injected via env vars by the operator (literal values, not secret refs) +// and ingested at vMCP startup; the type is therefore purely a runtime +// representation and is not part of any CRD schema. +type HeaderForwardConfig struct { + // AddPlaintextHeaders is a map of canonical HTTP header name to literal value. + AddPlaintextHeaders map[string]string `json:"addPlaintextHeaders,omitempty" yaml:"addPlaintextHeaders,omitempty"` + + // AddHeadersFromSecret maps canonical HTTP header name to secret identifier. + // The vMCP runtime resolves each identifier via secrets.EnvironmentProvider, + // which reads TOOLHIVE_SECRET_ from the pod environment. + AddHeadersFromSecret map[string]string `json:"addHeadersFromSecret,omitempty" yaml:"addHeadersFromSecret,omitempty"` +} + // BackendTarget identifies a specific backend workload and provides // the information needed to forward requests to it. type BackendTarget struct { @@ -78,6 +99,11 @@ type BackendTarget struct { // HealthStatus indicates the current health of the backend. HealthStatus BackendHealthStatus + // HeaderForward carries per-backend HTTP header injection configuration + // applied by the vMCP client to every outbound request targeting this backend + // (list, call, health-check). Nil when no headers are configured. + HeaderForward *HeaderForwardConfig + // Metadata stores additional backend-specific information. Metadata map[string]string } @@ -328,6 +354,11 @@ type Backend struct { // +optional AuthConfigRef string + // HeaderForward carries per-backend HTTP header injection configuration. + // Only populated for entry-type backends whose MCPServerEntry declares + // spec.headerForward. Nil when the entry has no header forwarding configured. + HeaderForward *HeaderForwardConfig + // Metadata stores additional backend information. Metadata map[string]string } From 1ebcf3d000e1a1614ce5af8c315ccf32f11c8660 Mon Sep 17 00:00:00 2001 From: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com> Date: Sun, 10 May 2026 14:29:13 +0100 Subject: [PATCH 2/7] Adopt wirefmt in MCPRemoteProxy controller Replace the local SecretEnvVarName helpers with the shared wirefmt encoder so the operator and vMCP runtime stay in lockstep on env-var naming. --- cmd/thv-operator/controllers/mcpremoteproxy_deployment.go | 3 ++- cmd/thv-operator/controllers/mcpremoteproxy_runconfig.go | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/cmd/thv-operator/controllers/mcpremoteproxy_deployment.go b/cmd/thv-operator/controllers/mcpremoteproxy_deployment.go index bff48386a3..591cdc04a1 100644 --- a/cmd/thv-operator/controllers/mcpremoteproxy_deployment.go +++ b/cmd/thv-operator/controllers/mcpremoteproxy_deployment.go @@ -18,6 +18,7 @@ import ( ctrlutil "github.com/stacklok/toolhive/cmd/thv-operator/pkg/controllerutil" "github.com/stacklok/toolhive/cmd/thv-operator/pkg/runconfig/configmap/checksum" "github.com/stacklok/toolhive/pkg/container/kubernetes" + "github.com/stacklok/toolhive/pkg/vmcp/headerforward/wirefmt" ) // deploymentForMCPRemoteProxy returns a MCPRemoteProxy Deployment object @@ -278,7 +279,7 @@ func buildHeaderForwardSecretEnvVars(proxy *mcpv1beta1.MCPRemoteProxy) []corev1. } // Generate env var name following the TOOLHIVE_SECRET_ pattern - envVarName, _ := ctrlutil.GenerateHeaderForwardSecretEnvVarName(proxy.Name, headerSecret.HeaderName) + envVarName, _ := wirefmt.SecretEnvVarName(proxy.Name, headerSecret.HeaderName) envVars = append(envVars, corev1.EnvVar{ Name: envVarName, diff --git a/cmd/thv-operator/controllers/mcpremoteproxy_runconfig.go b/cmd/thv-operator/controllers/mcpremoteproxy_runconfig.go index 65cf8cb81f..0059404fb1 100644 --- a/cmd/thv-operator/controllers/mcpremoteproxy_runconfig.go +++ b/cmd/thv-operator/controllers/mcpremoteproxy_runconfig.go @@ -21,6 +21,7 @@ import ( "github.com/stacklok/toolhive/cmd/thv-operator/pkg/runconfig/configmap/checksum" "github.com/stacklok/toolhive/pkg/runner" transporttypes "github.com/stacklok/toolhive/pkg/transport/types" + "github.com/stacklok/toolhive/pkg/vmcp/headerforward/wirefmt" ) // ensureRunConfigConfigMap ensures the RunConfig ConfigMap exists and is up to date for MCPRemoteProxy @@ -291,7 +292,7 @@ func addHeaderForwardConfigOptions(proxy *mcpv1beta1.MCPRemoteProxy, options *[] continue } // Get the secret identifier (not the full env var name) - _, secretIdentifier := ctrlutil.GenerateHeaderForwardSecretEnvVarName(proxy.Name, headerSecret.HeaderName) + _, secretIdentifier := wirefmt.SecretEnvVarName(proxy.Name, headerSecret.HeaderName) headerSecrets[headerSecret.HeaderName] = secretIdentifier } *options = append(*options, runner.WithHeaderForwardSecrets(headerSecrets)) From 91ad467c38dbe39db89fc8bbdb235060e632d013 Mon Sep 17 00:00:00 2001 From: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com> Date: Sun, 10 May 2026 14:29:05 +0100 Subject: [PATCH 3/7] Refactor externalauth helpers in operator controllerutil Surfaced while wiring headerForward through the MCPRemoteProxy and MCPServerEntry controllers. Tightens the helper contracts so callers in the new code paths share the same lookup signature. --- .../pkg/controllerutil/externalauth.go | 31 ++------- .../pkg/controllerutil/externalauth_test.go | 68 +------------------ 2 files changed, 6 insertions(+), 93 deletions(-) diff --git a/cmd/thv-operator/pkg/controllerutil/externalauth.go b/cmd/thv-operator/pkg/controllerutil/externalauth.go index 576b38014f..0412f7d3b3 100644 --- a/cmd/thv-operator/pkg/controllerutil/externalauth.go +++ b/cmd/thv-operator/pkg/controllerutil/externalauth.go @@ -44,30 +44,7 @@ func GenerateUniqueHeaderInjectionEnvVarName(configName string) string { return fmt.Sprintf("TOOLHIVE_HEADER_INJECTION_VALUE_%s", sanitized) } -// GenerateHeaderForwardSecretEnvVarName generates the environment variable name for a header forward secret. -// The generated name follows the TOOLHIVE_SECRET_ pattern expected by the EnvironmentProvider. -// -// Parameters: -// - proxyName: The name of the MCPRemoteProxy resource -// - headerName: The HTTP header name (e.g., "X-API-Key") -// -// Returns the full environment variable name (e.g., "TOOLHIVE_SECRET_HEADER_FORWARD_X_API_KEY_MY_PROXY") -// and the secret identifier portion (e.g., "HEADER_FORWARD_X_API_KEY_MY_PROXY") for use in RunConfig. -func GenerateHeaderForwardSecretEnvVarName(proxyName, headerName string) (envVarName, secretIdentifier string) { - // Sanitize header name for use in env var (uppercase, replace hyphens with underscore) - sanitizedHeader := strings.ToUpper(strings.ReplaceAll(headerName, "-", "_")) - sanitizedHeader = envVarSanitizer.ReplaceAllString(sanitizedHeader, "_") - - // Sanitize proxy name for use in env var - sanitizedProxy := strings.ToUpper(strings.ReplaceAll(proxyName, "-", "_")) - sanitizedProxy = envVarSanitizer.ReplaceAllString(sanitizedProxy, "_") - - // Build the secret identifier (what gets stored in RunConfig.AddHeadersFromSecret) - secretIdentifier = fmt.Sprintf("HEADER_FORWARD_%s_%s", sanitizedHeader, sanitizedProxy) - - // Build the full env var name (TOOLHIVE_SECRET_ prefix + identifier) - // This follows the pattern expected by secrets.EnvironmentProvider - envVarName = fmt.Sprintf("TOOLHIVE_SECRET_%s", secretIdentifier) - - return envVarName, secretIdentifier -} +// Header-forward env-var helpers (constants + name generators + the shared +// header-name normalizer) moved to pkg/vmcp/headerforward/wirefmt so the +// runtime can consume them without inverting Go layering. Operator code +// imports them from there. diff --git a/cmd/thv-operator/pkg/controllerutil/externalauth_test.go b/cmd/thv-operator/pkg/controllerutil/externalauth_test.go index 7cffbeabb5..638ba82dce 100644 --- a/cmd/thv-operator/pkg/controllerutil/externalauth_test.go +++ b/cmd/thv-operator/pkg/controllerutil/externalauth_test.go @@ -104,69 +104,5 @@ func TestGenerateUniqueHeaderInjectionEnvVarName(t *testing.T) { } } -// TestGenerateHeaderForwardSecretEnvVarName tests the GenerateHeaderForwardSecretEnvVarName function -func TestGenerateHeaderForwardSecretEnvVarName(t *testing.T) { - t.Parallel() - - tests := []struct { - name string - proxyName string - headerName string - expectedEnvVarName string - expectedSecretIdentifier string - }{ - { - name: "simple names", - proxyName: "my-proxy", - headerName: "X-API-Key", - expectedEnvVarName: "TOOLHIVE_SECRET_HEADER_FORWARD_X_API_KEY_MY_PROXY", - expectedSecretIdentifier: "HEADER_FORWARD_X_API_KEY_MY_PROXY", - }, - { - name: "lowercase header", - proxyName: "test-proxy", - headerName: "authorization", - expectedEnvVarName: "TOOLHIVE_SECRET_HEADER_FORWARD_AUTHORIZATION_TEST_PROXY", - expectedSecretIdentifier: "HEADER_FORWARD_AUTHORIZATION_TEST_PROXY", - }, - { - name: "multiple hyphens", - proxyName: "my-remote-proxy", - headerName: "X-Custom-Header", - expectedEnvVarName: "TOOLHIVE_SECRET_HEADER_FORWARD_X_CUSTOM_HEADER_MY_REMOTE_PROXY", - expectedSecretIdentifier: "HEADER_FORWARD_X_CUSTOM_HEADER_MY_REMOTE_PROXY", - }, - { - name: "special characters in proxy name", - proxyName: "proxy.name@123", - headerName: "X-Token", - expectedEnvVarName: "TOOLHIVE_SECRET_HEADER_FORWARD_X_TOKEN_PROXY_NAME_123", - expectedSecretIdentifier: "HEADER_FORWARD_X_TOKEN_PROXY_NAME_123", - }, - } - - envVarPattern := regexp.MustCompile(`^[A-Z0-9_]+$`) - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - envVarName, secretIdentifier := GenerateHeaderForwardSecretEnvVarName(tt.proxyName, tt.headerName) - - // Verify expected values - assert.Equal(t, tt.expectedEnvVarName, envVarName, "envVarName should match expected") - assert.Equal(t, tt.expectedSecretIdentifier, secretIdentifier, "secretIdentifier should match expected") - - // Verify env var name starts with TOOLHIVE_SECRET_ prefix - assert.True(t, regexp.MustCompile("^TOOLHIVE_SECRET_").MatchString(envVarName), - "envVarName should start with TOOLHIVE_SECRET_ prefix") - - // Verify env var name is valid - assert.Regexp(t, envVarPattern, envVarName, "envVarName should be a valid environment variable name") - assert.Regexp(t, envVarPattern, secretIdentifier, "secretIdentifier should be a valid identifier") - - // Verify relationship: envVarName = "TOOLHIVE_SECRET_" + secretIdentifier - assert.Equal(t, "TOOLHIVE_SECRET_"+secretIdentifier, envVarName, - "envVarName should equal TOOLHIVE_SECRET_ prefix + secretIdentifier") - }) - } -} +// Tests for the header-forward env var generators moved with the +// implementation to pkg/vmcp/headerforward/wirefmt — see wirefmt_test.go. From 70fc08271386c305c62a108b975b0dbeb0359c64 Mon Sep 17 00:00:00 2001 From: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com> Date: Sun, 10 May 2026 14:29:24 +0100 Subject: [PATCH 4/7] Validate headerForward Secret refs on MCPServerEntry The headerForward field already exists on the MCPServerEntry CRD; this commit adds the reconciler validation that walks spec.headerForward.addHeadersFromSecret, confirms each referenced Secret exists in the namespace, and surfaces the result as a HeaderSecretRefsValidated status condition. Mirrors the validation MCPRemoteProxy already performs for its header Secret refs. --- .../api/v1beta1/mcpserverentry_types.go | 13 ++ .../controllers/mcpserverentry_controller.go | 161 ++++++++++++++++++ .../mcpserverentry_controller_test.go | 150 ++++++++++++++++ 3 files changed, 324 insertions(+) diff --git a/cmd/thv-operator/api/v1beta1/mcpserverentry_types.go b/cmd/thv-operator/api/v1beta1/mcpserverentry_types.go index 6c5043d6e7..04c989230b 100644 --- a/cmd/thv-operator/api/v1beta1/mcpserverentry_types.go +++ b/cmd/thv-operator/api/v1beta1/mcpserverentry_types.go @@ -101,6 +101,11 @@ const ( // ConditionTypeMCPServerEntryRemoteURLValidated indicates whether the RemoteURL passes // format and SSRF safety checks. ConditionTypeMCPServerEntryRemoteURLValidated = "RemoteURLValidated" + + // ConditionTypeMCPServerEntryHeaderSecretRefsValidated indicates whether every Kubernetes + // Secret referenced by spec.headerForward.addHeadersFromSecret exists. Absent when the + // entry declares no header Secret refs. + ConditionTypeMCPServerEntryHeaderSecretRefsValidated = "HeaderSecretRefsValidated" ) // Condition reasons for MCPServerEntry. @@ -146,6 +151,14 @@ const ( // ConditionReasonMCPServerEntryRemoteURLInvalid indicates the RemoteURL is malformed or // targets a blocked internal/metadata endpoint. ConditionReasonMCPServerEntryRemoteURLInvalid = ConditionReasonRemoteURLInvalid + + // ConditionReasonMCPServerEntryHeaderSecretsValid indicates every referenced header Secret exists. + ConditionReasonMCPServerEntryHeaderSecretsValid = "HeaderSecretsValid" + + // ConditionReasonMCPServerEntryHeaderSecretNotFound indicates a Secret referenced by + // spec.headerForward.addHeadersFromSecret was not found in the entry's namespace. + // Reuses the string value used by MCPRemoteProxy for parity. + ConditionReasonMCPServerEntryHeaderSecretNotFound = ConditionReasonHeaderSecretNotFound ) //+kubebuilder:object:root=true diff --git a/cmd/thv-operator/controllers/mcpserverentry_controller.go b/cmd/thv-operator/controllers/mcpserverentry_controller.go index c34281470f..0ad7a1c5c5 100644 --- a/cmd/thv-operator/controllers/mcpserverentry_controller.go +++ b/cmd/thv-operator/controllers/mcpserverentry_controller.go @@ -6,6 +6,7 @@ package controllers import ( "context" "fmt" + "strings" "time" corev1 "k8s.io/api/core/v1" @@ -32,6 +33,10 @@ const ( // mcpServerEntryCABundleRefField is the field index key for CABundleRef ConfigMap lookups. mcpServerEntryCABundleRefField = "spec.caBundleRef.configMapRef.name" + + // mcpServerEntryHeaderSecretRefField is the field index key for + // spec.headerForward.addHeadersFromSecret[*].valueSecretRef.name lookups. + mcpServerEntryHeaderSecretRefField = "spec.headerForward.addHeadersFromSecret.valueSecretRef.name" ) // MCPServerEntryReconciler reconciles a MCPServerEntry object. @@ -46,6 +51,7 @@ type MCPServerEntryReconciler struct { // +kubebuilder:rbac:groups=toolhive.stacklok.dev,resources=mcpgroups,verbs=get;list;watch // +kubebuilder:rbac:groups=toolhive.stacklok.dev,resources=mcpexternalauthconfigs,verbs=get;list;watch // +kubebuilder:rbac:groups="",resources=configmaps,verbs=get;list;watch +// +kubebuilder:rbac:groups="",resources=secrets,verbs=get;list;watch // Reconcile validates referenced resources and updates status conditions. func (r *MCPServerEntryReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { @@ -85,6 +91,12 @@ func (r *MCPServerEntryReconciler) Reconcile(ctx context.Context, req ctrl.Reque } allValid = valid && allValid + valid, err = r.validateHeaderForwardSecretRefs(ctx, entry) + if err != nil { + return ctrl.Result{}, err + } + allValid = valid && allValid + // Compute overall phase and Valid condition r.updateOverallStatus(entry, allValid) @@ -137,6 +149,38 @@ func (r *MCPServerEntryReconciler) SetupWithManager(mgr ctrl.Manager) error { mcpServerEntryCABundleRefField, err) } + // Set up field index for headerForward Secret refs. Used by the Secret + // watch below so a Secret create/update/delete reconciles every entry that + // references it — without this index we would have to list all entries on + // every Secret event. + if err := mgr.GetFieldIndexer().IndexField( + context.Background(), + &mcpv1beta1.MCPServerEntry{}, + mcpServerEntryHeaderSecretRefField, + func(obj client.Object) []string { + entry := obj.(*mcpv1beta1.MCPServerEntry) + if entry.Spec.HeaderForward == nil { + return nil + } + seen := make(map[string]struct{}, len(entry.Spec.HeaderForward.AddHeadersFromSecret)) + names := make([]string, 0, len(entry.Spec.HeaderForward.AddHeadersFromSecret)) + for _, ref := range entry.Spec.HeaderForward.AddHeadersFromSecret { + if ref.ValueSecretRef == nil { + continue + } + if _, ok := seen[ref.ValueSecretRef.Name]; ok { + continue + } + seen[ref.ValueSecretRef.Name] = struct{}{} + names = append(names, ref.ValueSecretRef.Name) + } + return names + }, + ); err != nil { + return fmt.Errorf("unable to create field index for MCPServerEntry %s: %w", + mcpServerEntryHeaderSecretRefField, err) + } + return ctrl.NewControllerManagedBy(mgr). For(&mcpv1beta1.MCPServerEntry{}). Watches( @@ -151,6 +195,10 @@ func (r *MCPServerEntryReconciler) SetupWithManager(mgr ctrl.Manager) error { &corev1.ConfigMap{}, handler.EnqueueRequestsFromMapFunc(r.findEntriesForConfigMap), ). + Watches( + &corev1.Secret{}, + handler.EnqueueRequestsFromMapFunc(r.findEntriesForHeaderSecret), + ). Complete(r) } @@ -300,6 +348,81 @@ func (r *MCPServerEntryReconciler) validateCABundleRef( return true, nil } +// validateHeaderForwardSecretRefs checks that every Secret referenced by +// spec.headerForward.addHeadersFromSecret exists AND that the named key +// inside it is present in the entry's namespace. Skipped (condition removed) +// when no Secret-backed headers are declared. +// +// All ref-level failures are aggregated into one condition message — a user +// fixing two missing secrets at once should see both surface together rather +// than one-per-reconcile. +// +// Returns (valid, error) where a non-nil error means a transient API +// failure that should be requeued. Key-not-found and Secret-not-found are +// surfaced via the condition (valid=false, err=nil). +func (r *MCPServerEntryReconciler) validateHeaderForwardSecretRefs( + ctx context.Context, + entry *mcpv1beta1.MCPServerEntry, +) (bool, error) { + ctxLogger := log.FromContext(ctx) + + if entry.Spec.HeaderForward == nil || len(entry.Spec.HeaderForward.AddHeadersFromSecret) == 0 { + meta.RemoveStatusCondition(&entry.Status.Conditions, + mcpv1beta1.ConditionTypeMCPServerEntryHeaderSecretRefsValidated) + return true, nil + } + + var failures []string + for _, ref := range entry.Spec.HeaderForward.AddHeadersFromSecret { + if ref.ValueSecretRef == nil { + continue + } + secret := &corev1.Secret{} + secretKey := types.NamespacedName{ + Namespace: entry.Namespace, + Name: ref.ValueSecretRef.Name, + } + if err := r.Get(ctx, secretKey, secret); err != nil { + if errors.IsNotFound(err) { + failures = append(failures, fmt.Sprintf( + "Secret %q referenced for header %q not found", + ref.ValueSecretRef.Name, ref.HeaderName)) + continue + } + // Transient API failure (rate-limit, network, etc.) — requeue + // rather than condition-flip. The next reconcile will re-validate. + ctxLogger.Error(err, "Failed to get referenced header Secret", + "secret", ref.ValueSecretRef.Name, "header", ref.HeaderName) + return false, err + } + if _, ok := secret.Data[ref.ValueSecretRef.Key]; !ok { + failures = append(failures, fmt.Sprintf( + "Secret %q has no key %q referenced for header %q", + ref.ValueSecretRef.Name, ref.ValueSecretRef.Key, ref.HeaderName)) + } + } + + if len(failures) > 0 { + meta.SetStatusCondition(&entry.Status.Conditions, metav1.Condition{ + Type: mcpv1beta1.ConditionTypeMCPServerEntryHeaderSecretRefsValidated, + Status: metav1.ConditionFalse, + Reason: mcpv1beta1.ConditionReasonMCPServerEntryHeaderSecretNotFound, + Message: strings.Join(failures, "; "), + ObservedGeneration: entry.Generation, + }) + return false, nil + } + + meta.SetStatusCondition(&entry.Status.Conditions, metav1.Condition{ + Type: mcpv1beta1.ConditionTypeMCPServerEntryHeaderSecretRefsValidated, + Status: metav1.ConditionTrue, + Reason: mcpv1beta1.ConditionReasonMCPServerEntryHeaderSecretsValid, + Message: "All referenced header Secrets exist with required keys", + ObservedGeneration: entry.Generation, + }) + return true, nil +} + // validateRemoteURL checks that the RemoteURL is well-formed and does not target // a blocked internal or metadata endpoint (SSRF protection). func (*MCPServerEntryReconciler) validateRemoteURL( @@ -421,6 +544,44 @@ func (r *MCPServerEntryReconciler) findEntriesForGroup( return requests } +// findEntriesForHeaderSecret maps Secret changes to MCPServerEntry reconcile +// requests for entries that reference the Secret in +// spec.headerForward.addHeadersFromSecret. This ensures that creating the +// referenced Secret (or rotating its contents) flips the validation condition +// from false → true within one reconcile, instead of waiting for the resync. +func (r *MCPServerEntryReconciler) findEntriesForHeaderSecret( + ctx context.Context, + obj client.Object, +) []reconcile.Request { + ctxLogger := log.FromContext(ctx) + + secret, ok := obj.(*corev1.Secret) + if !ok { + ctxLogger.Error(nil, "Object is not a Secret", "object", obj.GetName()) + return nil + } + + entryList := &mcpv1beta1.MCPServerEntryList{} + if err := r.List(ctx, entryList, + client.InNamespace(secret.Namespace), + client.MatchingFields{mcpServerEntryHeaderSecretRefField: secret.Name}, + ); err != nil { + ctxLogger.Error(err, "Failed to list MCPServerEntries for header Secret change") + return nil + } + + requests := make([]reconcile.Request, len(entryList.Items)) + for i, entry := range entryList.Items { + requests[i] = reconcile.Request{ + NamespacedName: types.NamespacedName{ + Namespace: entry.Namespace, + Name: entry.Name, + }, + } + } + return requests +} + // findEntriesForConfigMap maps ConfigMap changes to MCPServerEntry reconcile requests // for entries that reference the ConfigMap as a CA bundle. func (r *MCPServerEntryReconciler) findEntriesForConfigMap( diff --git a/cmd/thv-operator/controllers/mcpserverentry_controller_test.go b/cmd/thv-operator/controllers/mcpserverentry_controller_test.go index 97b01014fd..42d0e7c48c 100644 --- a/cmd/thv-operator/controllers/mcpserverentry_controller_test.go +++ b/cmd/thv-operator/controllers/mcpserverentry_controller_test.go @@ -137,6 +137,10 @@ func TestMCPServerEntryReconciler_Reconcile(t *testing.T) { status metav1.ConditionStatus reason string } + // extraCheck runs additional assertions on the post-reconcile entry + // state — used when matching condition reasons isn't enough (e.g. + // asserting message content for aggregated failures). + extraCheck func(t *testing.T, entry *mcpv1beta1.MCPServerEntry) }{ { name: "happy path - all refs valid", @@ -330,6 +334,149 @@ func TestMCPServerEntryReconciler_Reconcile(t *testing.T) { {mcpv1beta1.ConditionTypeMCPServerEntryValid, metav1.ConditionTrue, mcpv1beta1.ConditionReasonMCPServerEntryValid}, }, }, + { + name: "headerForward plaintext only - no secret validation needed", + entry: func() *mcpv1beta1.MCPServerEntry { + e := newMCPServerEntry(testEntryGroupRef, nil, nil) + e.Spec.HeaderForward = &mcpv1beta1.HeaderForwardConfig{ + AddPlaintextHeaders: map[string]string{"X-MCP-Toolsets": "projects,issues"}, + } + return e + }(), + objects: []client.Object{newMCPGroup(mcpv1beta1.MCPGroupPhaseReady)}, + wantPhase: mcpv1beta1.MCPServerEntryPhaseValid, + conditions: []struct { + condType string + status metav1.ConditionStatus + reason string + }{ + {mcpv1beta1.ConditionTypeMCPServerEntryValid, metav1.ConditionTrue, mcpv1beta1.ConditionReasonMCPServerEntryValid}, + }, + }, + { + name: "headerForward secret ref exists", + entry: func() *mcpv1beta1.MCPServerEntry { + e := newMCPServerEntry(testEntryGroupRef, nil, nil) + e.Spec.HeaderForward = &mcpv1beta1.HeaderForwardConfig{ + AddHeadersFromSecret: []mcpv1beta1.HeaderFromSecret{ + { + HeaderName: "X-API-Key", + ValueSecretRef: &mcpv1beta1.SecretKeyRef{Name: "api-key-secret", Key: "token"}, + }, + }, + } + return e + }(), + objects: []client.Object{ + newMCPGroup(mcpv1beta1.MCPGroupPhaseReady), + &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: "api-key-secret", Namespace: testEntryNS}, + Data: map[string][]byte{"token": []byte("secret-value")}, + }, + }, + wantPhase: mcpv1beta1.MCPServerEntryPhaseValid, + conditions: []struct { + condType string + status metav1.ConditionStatus + reason string + }{ + {mcpv1beta1.ConditionTypeMCPServerEntryHeaderSecretRefsValidated, metav1.ConditionTrue, mcpv1beta1.ConditionReasonMCPServerEntryHeaderSecretsValid}, + {mcpv1beta1.ConditionTypeMCPServerEntryValid, metav1.ConditionTrue, mcpv1beta1.ConditionReasonMCPServerEntryValid}, + }, + }, + { + name: "headerForward secret ref missing flips entry to Failed", + entry: func() *mcpv1beta1.MCPServerEntry { + e := newMCPServerEntry(testEntryGroupRef, nil, nil) + e.Spec.HeaderForward = &mcpv1beta1.HeaderForwardConfig{ + AddHeadersFromSecret: []mcpv1beta1.HeaderFromSecret{ + { + HeaderName: "X-API-Key", + ValueSecretRef: &mcpv1beta1.SecretKeyRef{Name: "missing-secret", Key: "token"}, + }, + }, + } + return e + }(), + objects: []client.Object{newMCPGroup(mcpv1beta1.MCPGroupPhaseReady)}, + wantPhase: mcpv1beta1.MCPServerEntryPhaseFailed, + conditions: []struct { + condType string + status metav1.ConditionStatus + reason string + }{ + {mcpv1beta1.ConditionTypeMCPServerEntryHeaderSecretRefsValidated, metav1.ConditionFalse, mcpv1beta1.ConditionReasonMCPServerEntryHeaderSecretNotFound}, + {mcpv1beta1.ConditionTypeMCPServerEntryValid, metav1.ConditionFalse, mcpv1beta1.ConditionReasonMCPServerEntryInvalid}, + }, + }, + { + name: "headerForward secret exists but key is missing flips entry to Failed", + entry: func() *mcpv1beta1.MCPServerEntry { + e := newMCPServerEntry(testEntryGroupRef, nil, nil) + e.Spec.HeaderForward = &mcpv1beta1.HeaderForwardConfig{ + AddHeadersFromSecret: []mcpv1beta1.HeaderFromSecret{ + { + HeaderName: "X-API-Key", + ValueSecretRef: &mcpv1beta1.SecretKeyRef{Name: "api-key-secret", Key: "absent-key"}, + }, + }, + } + return e + }(), + objects: []client.Object{ + newMCPGroup(mcpv1beta1.MCPGroupPhaseReady), + &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: "api-key-secret", Namespace: testEntryNS}, + Data: map[string][]byte{"token": []byte("secret-value")}, + }, + }, + wantPhase: mcpv1beta1.MCPServerEntryPhaseFailed, + conditions: []struct { + condType string + status metav1.ConditionStatus + reason string + }{ + {mcpv1beta1.ConditionTypeMCPServerEntryHeaderSecretRefsValidated, metav1.ConditionFalse, mcpv1beta1.ConditionReasonMCPServerEntryHeaderSecretNotFound}, + {mcpv1beta1.ConditionTypeMCPServerEntryValid, metav1.ConditionFalse, mcpv1beta1.ConditionReasonMCPServerEntryInvalid}, + }, + }, + { + name: "headerForward multiple secret refs failures are aggregated", + entry: func() *mcpv1beta1.MCPServerEntry { + e := newMCPServerEntry(testEntryGroupRef, nil, nil) + e.Spec.HeaderForward = &mcpv1beta1.HeaderForwardConfig{ + AddHeadersFromSecret: []mcpv1beta1.HeaderFromSecret{ + { + HeaderName: "X-API-Key", + ValueSecretRef: &mcpv1beta1.SecretKeyRef{Name: "missing-1", Key: "token"}, + }, + { + HeaderName: "X-Other", + ValueSecretRef: &mcpv1beta1.SecretKeyRef{Name: "missing-2", Key: "value"}, + }, + }, + } + return e + }(), + objects: []client.Object{newMCPGroup(mcpv1beta1.MCPGroupPhaseReady)}, + wantPhase: mcpv1beta1.MCPServerEntryPhaseFailed, + conditions: []struct { + condType string + status metav1.ConditionStatus + reason string + }{ + {mcpv1beta1.ConditionTypeMCPServerEntryHeaderSecretRefsValidated, metav1.ConditionFalse, mcpv1beta1.ConditionReasonMCPServerEntryHeaderSecretNotFound}, + {mcpv1beta1.ConditionTypeMCPServerEntryValid, metav1.ConditionFalse, mcpv1beta1.ConditionReasonMCPServerEntryInvalid}, + }, + extraCheck: func(t *testing.T, entry *mcpv1beta1.MCPServerEntry) { + t.Helper() + cond := meta.FindStatusCondition(entry.Status.Conditions, + mcpv1beta1.ConditionTypeMCPServerEntryHeaderSecretRefsValidated) + require.NotNil(t, cond) + assert.Contains(t, cond.Message, "missing-1") + assert.Contains(t, cond.Message, "missing-2") + }, + }, } for _, tt := range tests { @@ -388,6 +535,9 @@ func TestMCPServerEntryReconciler_Reconcile(t *testing.T) { for _, c := range tt.conditions { assertCondition(t, updatedEntry.Status.Conditions, c.condType, c.status, c.reason) } + if tt.extraCheck != nil { + tt.extraCheck(t, &updatedEntry) + } }) } } From a5bd56d1529e43f6345fcbe3df73ded9003d5cf6 Mon Sep 17 00:00:00 2001 From: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com> Date: Sun, 10 May 2026 14:29:35 +0100 Subject: [PATCH 5/7] Emit headerForward env vars from VirtualMCPServer deployment The VirtualMCPServer reconciler now renders the entry-side headerForward manifest into the vMCP pod env via the wirefmt encoding. Plaintext values land directly; Secret-backed values become valueFrom.secretKeyRef so the runtime never sees raw secret material in CRD or pod spec. --- .../virtualmcpserver_deployment.go | 159 ++++++++++ .../virtualmcpserver_deployment_test.go | 299 ++++++++++++++++++ 2 files changed, 458 insertions(+) diff --git a/cmd/thv-operator/controllers/virtualmcpserver_deployment.go b/cmd/thv-operator/controllers/virtualmcpserver_deployment.go index ea476512b2..6f93594319 100644 --- a/cmd/thv-operator/controllers/virtualmcpserver_deployment.go +++ b/cmd/thv-operator/controllers/virtualmcpserver_deployment.go @@ -28,7 +28,9 @@ import ( ctrlutil "github.com/stacklok/toolhive/cmd/thv-operator/pkg/controllerutil" "github.com/stacklok/toolhive/cmd/thv-operator/pkg/runconfig/configmap/checksum" "github.com/stacklok/toolhive/pkg/container/kubernetes" + vmcptypes "github.com/stacklok/toolhive/pkg/vmcp" vmcpconfig "github.com/stacklok/toolhive/pkg/vmcp/config" + "github.com/stacklok/toolhive/pkg/vmcp/headerforward/wirefmt" "github.com/stacklok/toolhive/pkg/vmcp/workloads" ) @@ -358,6 +360,17 @@ func (r *VirtualMCPServerReconciler) buildEnvVarsForVmcp( // Mount outgoing auth secrets env = append(env, r.buildOutgoingAuthEnvVars(ctx, vmcp, typedWorkloads)...) + // Mount per-(entry, header) env vars for MCPServerEntry headerForward. + // Plaintext entries are emitted as literal-value env vars; secret entries + // as valueFrom.secretKeyRef. The vMCP runtime walks the well-known prefixes + // at startup to reconstruct the per-backend HeaderForwardConfig in static + // mode (issue #4996). + headerEnv, err := r.buildHeaderForwardEnvVarsForEntries(ctx, vmcp.Namespace, typedWorkloads) + if err != nil { + return nil, fmt.Errorf("failed to build header forward env vars: %w", err) + } + env = append(env, headerEnv...) + // Always mount HMAC secret for session token binding. env = append(env, r.buildHMACSecretEnvVar(vmcp)) @@ -493,6 +506,152 @@ func (r *VirtualMCPServerReconciler) buildOutgoingAuthEnvVars( return env } +// buildHeaderForwardEnvVarsForEntries iterates entry-type workloads and emits +// header-forward env vars on the vMCP pod for each entry that declares +// spec.headerForward. +// +// One JSON-encoded manifest env var per backend carries the full +// HeaderForwardConfig with original header names preserved +// (TOOLHIVE_HEADER_FORWARD_). Plaintext header values +// appear inline in the JSON; secret-backed entries carry only the secret +// identifier — the Secret value rides a sibling +// TOOLHIVE_SECRET_HEADER_FORWARD_
_ env var emitted via +// valueFrom.secretKeyRef so the value never enters the operator's view of +// the world and resolves at request time inside resolveHeaderForward via +// secrets.EnvironmentProvider. +// +// Scoping by entry name prevents collisions when multiple entries in the +// group declare the same header name. +// +// Returns (nil, nil) when no entries declare any HeaderForward — the common +// case, producing no env-var churn on the Deployment spec. +func (r *VirtualMCPServerReconciler) buildHeaderForwardEnvVarsForEntries( + ctx context.Context, + namespace string, + typedWorkloads []workloads.TypedWorkload, +) ([]corev1.EnvVar, error) { + // Early return if no MCPServerEntry workloads to avoid unnecessary API calls. + hasEntries := false + for _, workload := range typedWorkloads { + if workload.Type == workloads.WorkloadTypeMCPServerEntry { + hasEntries = true + break + } + } + if !hasEntries { + return nil, nil + } + + entryMap, err := r.listMCPServerEntriesAsMap(ctx, namespace) + if err != nil { + return nil, fmt.Errorf("failed to list MCPServerEntries: %w", err) + } + + var envVars []corev1.EnvVar + for _, workload := range typedWorkloads { + if workload.Type != workloads.WorkloadTypeMCPServerEntry { + continue + } + entry, found := entryMap[workload.Name] + if !found || entry.Spec.HeaderForward == nil { + continue + } + + manifest, err := buildHeaderForwardManifestForEntry(entry) + if err != nil { + return nil, fmt.Errorf("failed to build header-forward manifest for entry %q: %w", entry.Name, err) + } + if manifest != "" { + manifestVarName, _ := wirefmt.ManifestEnvVarName(entry.Name) + envVars = append(envVars, corev1.EnvVar{ + Name: manifestVarName, + Value: manifest, + }) + } + + // Secret-backed headers — emit valueFrom.secretKeyRef env vars so + // the resolved Secret value never enters the operator's view of the + // world. AddHeadersFromSecret is a slice in the v1beta1 API so the + // order is already deterministic from the user's manifest. + for _, ref := range entry.Spec.HeaderForward.AddHeadersFromSecret { + if ref.ValueSecretRef == nil { + continue + } + envVarName, _ := wirefmt.SecretEnvVarName(entry.Name, ref.HeaderName) + envVars = append(envVars, corev1.EnvVar{ + Name: envVarName, + ValueFrom: &corev1.EnvVarSource{ + SecretKeyRef: &corev1.SecretKeySelector{ + LocalObjectReference: corev1.LocalObjectReference{ + Name: ref.ValueSecretRef.Name, + }, + Key: ref.ValueSecretRef.Key, + }, + }, + }) + } + } + + // Sort the resulting env vars by Name. The Kubernetes informer cache + // returns items in non-deterministic order (Go map iteration), so + // without sorting the env vars appear in a different sequence on each + // reconcile. reflect.DeepEqual in containerNeedsUpdate is order- + // sensitive, so non-deterministic ordering causes a continuous + // deployment update loop. Mirrors the pattern in + // discoverExternalAuthConfigSecrets / discoverInlineExternalAuthConfigSecrets. + sort.Slice(envVars, func(i, j int) bool { + return envVars[i].Name < envVars[j].Name + }) + + return envVars, nil +} + +// buildHeaderForwardManifestForEntry builds the JSON-encoded manifest for +// a single MCPServerEntry. AddHeadersFromSecret values carry the secret +// identifier (HEADER_FORWARD__) the runtime hands to +// secrets.EnvironmentProvider at request time; the resolved Secret value +// itself never appears in the JSON. +// +// Returns the empty string when the entry has no plaintext or +// secret-backed entries — the caller skips emitting the env var in that +// case to keep Deployment specs minimal. +func buildHeaderForwardManifestForEntry(entry *mcpv1beta1.MCPServerEntry) (string, error) { + if entry == nil || entry.Spec.HeaderForward == nil { + return "", nil + } + src := entry.Spec.HeaderForward + + // Build vmcp.HeaderForwardConfig directly so the JSON shape is the + // runtime contract — there is no intermediate operator-side type to + // drift away from it. Header keys preserve original casing. + manifest := vmcptypes.HeaderForwardConfig{ + AddPlaintextHeaders: src.AddPlaintextHeaders, + } + for _, ref := range src.AddHeadersFromSecret { + if ref.ValueSecretRef == nil { + continue + } + _, identifier := wirefmt.SecretEnvVarName(entry.Name, ref.HeaderName) + if manifest.AddHeadersFromSecret == nil { + manifest.AddHeadersFromSecret = make(map[string]string) + } + manifest.AddHeadersFromSecret[ref.HeaderName] = identifier + } + + if len(manifest.AddPlaintextHeaders) == 0 && len(manifest.AddHeadersFromSecret) == 0 { + return "", nil + } + + // json.Marshal sorts map keys alphabetically by default, giving + // deterministic Deployment-spec rendering across reconciles regardless + // of Go's randomized map iteration. + out, err := json.Marshal(manifest) + if err != nil { + return "", fmt.Errorf("marshal manifest: %w", err) + } + return string(out), nil +} + // discoverExternalAuthConfigSecrets discovers ExternalAuthConfigs from workloads in the group // and returns environment variables for their client secrets. This is used for discovered mode. func (r *VirtualMCPServerReconciler) discoverExternalAuthConfigSecrets( diff --git a/cmd/thv-operator/controllers/virtualmcpserver_deployment_test.go b/cmd/thv-operator/controllers/virtualmcpserver_deployment_test.go index 6f1a792abc..95047a5ce5 100644 --- a/cmd/thv-operator/controllers/virtualmcpserver_deployment_test.go +++ b/cmd/thv-operator/controllers/virtualmcpserver_deployment_test.go @@ -1121,3 +1121,302 @@ func TestImagePullSecretsHash(t *testing.T) { assert.NotEqual(t, a, b) }) } + +// TestBuildHeaderForwardEnvVarsForEntries verifies the operator emits one env +// var per (entry, header) declared on an MCPServerEntry.spec.headerForward, +// using literal values for plaintext and valueFrom.secretKeyRef for +// secret-backed headers. The map iteration is sorted for determinism so that +// two reconciles with the same input produce byte-identical Deployment specs. +func TestBuildHeaderForwardEnvVarsForEntries(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + entries []mcpv1beta1.MCPServerEntry + workloads []workloads.TypedWorkload + validate func(t *testing.T, env []corev1.EnvVar) + }{ + { + name: "no MCPServerEntry workloads yields no env vars", + entries: nil, + workloads: []workloads.TypedWorkload{ + {Name: "server1", Type: workloads.WorkloadTypeMCPServer}, + }, + validate: func(t *testing.T, env []corev1.EnvVar) { + t.Helper() + assert.Empty(t, env) + }, + }, + { + name: "entry without headerForward yields no env vars", + entries: []mcpv1beta1.MCPServerEntry{ + { + ObjectMeta: metav1.ObjectMeta{Name: "entry-noop", Namespace: "default"}, + Spec: mcpv1beta1.MCPServerEntrySpec{ + RemoteURL: "https://mcp.example.com", + Transport: "streamable-http", + GroupRef: &mcpv1beta1.MCPGroupRef{Name: "test-group"}, + }, + }, + }, + workloads: []workloads.TypedWorkload{ + {Name: "entry-noop", Type: workloads.WorkloadTypeMCPServerEntry}, + }, + validate: func(t *testing.T, env []corev1.EnvVar) { + t.Helper() + assert.Empty(t, env) + }, + }, + { + name: "entry with plaintext headers emits one JSON manifest env var preserving original casing", + entries: []mcpv1beta1.MCPServerEntry{ + { + ObjectMeta: metav1.ObjectMeta{Name: "github-copilot", Namespace: "default"}, + Spec: mcpv1beta1.MCPServerEntrySpec{ + RemoteURL: "https://api.githubcopilot.com/mcp/", + Transport: "streamable-http", + GroupRef: &mcpv1beta1.MCPGroupRef{Name: "test-group"}, + HeaderForward: &mcpv1beta1.HeaderForwardConfig{ + AddPlaintextHeaders: map[string]string{ + "X-MCP-Toolsets": "projects,issues,pull_requests", + "X-Trace-Id": "abc123", + }, + }, + }, + }, + }, + workloads: []workloads.TypedWorkload{ + {Name: "github-copilot", Type: workloads.WorkloadTypeMCPServerEntry}, + }, + validate: func(t *testing.T, env []corev1.EnvVar) { + t.Helper() + require.Len(t, env, 1) + assert.Equal(t, "TOOLHIVE_HEADER_FORWARD_GITHUB_COPILOT", env[0].Name) + assert.Nil(t, env[0].ValueFrom) + // json.Marshal sorts map keys alphabetically. + assert.JSONEq(t, + `{"addPlaintextHeaders":{"X-MCP-Toolsets":"projects,issues,pull_requests","X-Trace-Id":"abc123"}}`, + env[0].Value, + ) + }, + }, + { + name: "entry with secret-backed headers emits both manifest and valueFrom.secretKeyRef env var", + entries: []mcpv1beta1.MCPServerEntry{ + { + ObjectMeta: metav1.ObjectMeta{Name: "stripe", Namespace: "default"}, + Spec: mcpv1beta1.MCPServerEntrySpec{ + RemoteURL: "https://api.stripe.example/mcp/", + Transport: "streamable-http", + GroupRef: &mcpv1beta1.MCPGroupRef{Name: "test-group"}, + HeaderForward: &mcpv1beta1.HeaderForwardConfig{ + AddHeadersFromSecret: []mcpv1beta1.HeaderFromSecret{ + { + HeaderName: "X-API-Key", + ValueSecretRef: &mcpv1beta1.SecretKeyRef{ + Name: "stripe-key", + Key: "token", + }, + }, + }, + }, + }, + }, + }, + workloads: []workloads.TypedWorkload{ + {Name: "stripe", Type: workloads.WorkloadTypeMCPServerEntry}, + }, + validate: func(t *testing.T, env []corev1.EnvVar) { + t.Helper() + require.Len(t, env, 2) + + assert.Equal(t, "TOOLHIVE_HEADER_FORWARD_STRIPE", env[0].Name) + assert.JSONEq(t, + `{"addHeadersFromSecret":{"X-API-Key":"HEADER_FORWARD_X_API_KEY_STRIPE"}}`, + env[0].Value, + ) + + assert.Equal(t, "TOOLHIVE_SECRET_HEADER_FORWARD_X_API_KEY_STRIPE", env[1].Name) + assert.Empty(t, env[1].Value) + require.NotNil(t, env[1].ValueFrom) + require.NotNil(t, env[1].ValueFrom.SecretKeyRef) + assert.Equal(t, "stripe-key", env[1].ValueFrom.SecretKeyRef.Name) + assert.Equal(t, "token", env[1].ValueFrom.SecretKeyRef.Key) + }, + }, + { + name: "mixed plaintext + secret across multiple entries are scoped per entry", + entries: []mcpv1beta1.MCPServerEntry{ + { + ObjectMeta: metav1.ObjectMeta{Name: "alpha", Namespace: "default"}, + Spec: mcpv1beta1.MCPServerEntrySpec{ + RemoteURL: "https://alpha.example/mcp/", + Transport: "streamable-http", + GroupRef: &mcpv1beta1.MCPGroupRef{Name: "test-group"}, + HeaderForward: &mcpv1beta1.HeaderForwardConfig{ + AddPlaintextHeaders: map[string]string{"X-Trace": "alpha-trace"}, + AddHeadersFromSecret: []mcpv1beta1.HeaderFromSecret{ + {HeaderName: "X-Token", ValueSecretRef: &mcpv1beta1.SecretKeyRef{Name: "alpha-secret", Key: "tok"}}, + }, + }, + }, + }, + { + ObjectMeta: metav1.ObjectMeta{Name: "beta", Namespace: "default"}, + Spec: mcpv1beta1.MCPServerEntrySpec{ + RemoteURL: "https://beta.example/mcp/", + Transport: "streamable-http", + GroupRef: &mcpv1beta1.MCPGroupRef{Name: "test-group"}, + HeaderForward: &mcpv1beta1.HeaderForwardConfig{ + AddPlaintextHeaders: map[string]string{"X-Trace": "beta-trace"}, + }, + }, + }, + }, + workloads: []workloads.TypedWorkload{ + {Name: "alpha", Type: workloads.WorkloadTypeMCPServerEntry}, + {Name: "beta", Type: workloads.WorkloadTypeMCPServerEntry}, + }, + validate: func(t *testing.T, env []corev1.EnvVar) { + t.Helper() + require.Len(t, env, 3) + + // After sort.Slice by Name: + // TOOLHIVE_HEADER_FORWARD_ALPHA + // TOOLHIVE_HEADER_FORWARD_BETA + // TOOLHIVE_SECRET_HEADER_FORWARD_X_TOKEN_ALPHA + assert.Equal(t, "TOOLHIVE_HEADER_FORWARD_ALPHA", env[0].Name) + assert.JSONEq(t, + `{"addPlaintextHeaders":{"X-Trace":"alpha-trace"},"addHeadersFromSecret":{"X-Token":"HEADER_FORWARD_X_TOKEN_ALPHA"}}`, + env[0].Value, + ) + + assert.Equal(t, "TOOLHIVE_HEADER_FORWARD_BETA", env[1].Name) + assert.JSONEq(t, + `{"addPlaintextHeaders":{"X-Trace":"beta-trace"}}`, + env[1].Value, + ) + + assert.Equal(t, "TOOLHIVE_SECRET_HEADER_FORWARD_X_TOKEN_ALPHA", env[2].Name) + require.NotNil(t, env[2].ValueFrom) + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + scheme := runtime.NewScheme() + require.NoError(t, mcpv1beta1.AddToScheme(scheme)) + require.NoError(t, corev1.AddToScheme(scheme)) + + objs := make([]client.Object, 0, len(tt.entries)) + for i := range tt.entries { + objs = append(objs, &tt.entries[i]) + } + + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(objs...). + Build() + + r := &VirtualMCPServerReconciler{ + Client: fakeClient, + Scheme: scheme, + } + + env, err := r.buildHeaderForwardEnvVarsForEntries(t.Context(), "default", tt.workloads) + require.NoError(t, err) + tt.validate(t, env) + }) + } +} + +// TestBuildHeaderForwardEnvVarsForEntries_ShuffledInputDeterministic verifies +// that the env-var emission is byte-identical regardless of the order +// typedWorkloads arrives in. The function sorts internally; this test pins +// that contract so a future refactor that drops the sort is caught +// immediately. Together with the comment block in the function, this +// closes the deployment-update-loop hazard from informer-cache ordering. +func TestBuildHeaderForwardEnvVarsForEntries_ShuffledInputDeterministic(t *testing.T) { + t.Parallel() + + entries := []mcpv1beta1.MCPServerEntry{ + { + ObjectMeta: metav1.ObjectMeta{Name: "alpha", Namespace: "default"}, + Spec: mcpv1beta1.MCPServerEntrySpec{ + RemoteURL: "https://alpha.example/", + Transport: "streamable-http", + GroupRef: &mcpv1beta1.MCPGroupRef{Name: "g"}, + HeaderForward: &mcpv1beta1.HeaderForwardConfig{ + AddPlaintextHeaders: map[string]string{"X-Trace": "alpha-trace"}, + AddHeadersFromSecret: []mcpv1beta1.HeaderFromSecret{ + {HeaderName: "X-Token", ValueSecretRef: &mcpv1beta1.SecretKeyRef{Name: "alpha-secret", Key: "tok"}}, + }, + }, + }, + }, + { + ObjectMeta: metav1.ObjectMeta{Name: "beta", Namespace: "default"}, + Spec: mcpv1beta1.MCPServerEntrySpec{ + RemoteURL: "https://beta.example/", + Transport: "streamable-http", + GroupRef: &mcpv1beta1.MCPGroupRef{Name: "g"}, + HeaderForward: &mcpv1beta1.HeaderForwardConfig{ + AddPlaintextHeaders: map[string]string{"X-Trace": "beta-trace"}, + }, + }, + }, + { + ObjectMeta: metav1.ObjectMeta{Name: "gamma", Namespace: "default"}, + Spec: mcpv1beta1.MCPServerEntrySpec{ + RemoteURL: "https://gamma.example/", + Transport: "streamable-http", + GroupRef: &mcpv1beta1.MCPGroupRef{Name: "g"}, + HeaderForward: &mcpv1beta1.HeaderForwardConfig{ + AddHeadersFromSecret: []mcpv1beta1.HeaderFromSecret{ + {HeaderName: "X-Key", ValueSecretRef: &mcpv1beta1.SecretKeyRef{Name: "gamma-secret", Key: "key"}}, + }, + }, + }, + }, + } + + // Two workload orderings — natural and reversed. + natural := []workloads.TypedWorkload{ + {Name: "alpha", Type: workloads.WorkloadTypeMCPServerEntry}, + {Name: "beta", Type: workloads.WorkloadTypeMCPServerEntry}, + {Name: "gamma", Type: workloads.WorkloadTypeMCPServerEntry}, + } + reversed := []workloads.TypedWorkload{ + {Name: "gamma", Type: workloads.WorkloadTypeMCPServerEntry}, + {Name: "beta", Type: workloads.WorkloadTypeMCPServerEntry}, + {Name: "alpha", Type: workloads.WorkloadTypeMCPServerEntry}, + } + + scheme := runtime.NewScheme() + require.NoError(t, mcpv1beta1.AddToScheme(scheme)) + require.NoError(t, corev1.AddToScheme(scheme)) + + objs := make([]client.Object, 0, len(entries)) + for i := range entries { + objs = append(objs, &entries[i]) + } + + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(objs...). + Build() + + r := &VirtualMCPServerReconciler{Client: fakeClient, Scheme: scheme} + + envNatural, err := r.buildHeaderForwardEnvVarsForEntries(t.Context(), "default", natural) + require.NoError(t, err) + + envReversed, err := r.buildHeaderForwardEnvVarsForEntries(t.Context(), "default", reversed) + require.NoError(t, err) + + assert.Equal(t, envNatural, envReversed, + "buildHeaderForwardEnvVarsForEntries must produce byte-identical output regardless of input workload order") +} From 6b4b7439c6667c280d560b9109946de8151821db Mon Sep 17 00:00:00 2001 From: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com> Date: Sun, 10 May 2026 14:29:48 +0100 Subject: [PATCH 6/7] Apply headerForward in vMCP client The HTTP client decorator injects per-backend headers (plaintext and Secret-resolved) on every outbound request: list, call, and health checks. Secret identifiers are resolved through the standard EnvironmentProvider, so the client never holds raw secret values. --- pkg/vmcp/client/client.go | 17 +- pkg/vmcp/client/header_forward.go | 160 ++++++++++++ .../client/header_forward_integration_test.go | 134 ++++++++++ pkg/vmcp/client/header_forward_test.go | 243 ++++++++++++++++++ 4 files changed, 553 insertions(+), 1 deletion(-) create mode 100644 pkg/vmcp/client/header_forward.go create mode 100644 pkg/vmcp/client/header_forward_integration_test.go create mode 100644 pkg/vmcp/client/header_forward_test.go diff --git a/pkg/vmcp/client/client.go b/pkg/vmcp/client/client.go index d84018856a..c72cf11cb9 100644 --- a/pkg/vmcp/client/client.go +++ b/pkg/vmcp/client/client.go @@ -27,6 +27,7 @@ import ( "go.opentelemetry.io/otel/propagation" "github.com/stacklok/toolhive/pkg/auth" + "github.com/stacklok/toolhive/pkg/secrets" "github.com/stacklok/toolhive/pkg/versions" "github.com/stacklok/toolhive/pkg/vmcp" vmcpauth "github.com/stacklok/toolhive/pkg/vmcp/auth" @@ -64,6 +65,11 @@ type httpBackendClient struct { // registry manages authentication strategies for outgoing requests to backend MCP servers. // Must not be nil - use UnauthenticatedStrategy for no authentication. registry vmcpauth.OutgoingAuthRegistry + + // secretsProvider resolves TOOLHIVE_SECRET_ env vars at client-creation + // time for per-backend header-forward injection. Nil when no backends declare + // headerForward.AddHeadersFromSecret — plaintext-only backends do not require it. + secretsProvider secrets.Provider } // NewHTTPBackendClient creates a new HTTP-based backend client. @@ -80,7 +86,8 @@ func NewHTTPBackendClient(registry vmcpauth.OutgoingAuthRegistry) (vmcp.BackendC } c := &httpBackendClient{ - registry: registry, + registry: registry, + secretsProvider: secrets.NewEnvironmentProvider(), } c.clientFactory = c.defaultClientFactory return c, nil @@ -296,6 +303,14 @@ func (h *httpBackendClient) defaultClientFactory(ctx context.Context, target *vm target: target, } + // Inject per-backend HTTP headers from MCPServerEntry.spec.headerForward. + // Resolves plaintext + secret-backed headers once here; auth (inner) always + // wins over user-supplied headers because it runs after this tripper. + baseTransport, err = buildHeaderForwardTripper(ctx, baseTransport, target.HeaderForward, h.secretsProvider, target.WorkloadID) + if err != nil { + return nil, fmt.Errorf("failed to build header-forward transport: %w", err) + } + // Extract identity and health-check marker from context and propagate them to backend // requests. The health-check marker must be carried through to the DELETE request that // mcp-go emits when closing a streamable-HTTP session: mcp-go creates that request with diff --git a/pkg/vmcp/client/header_forward.go b/pkg/vmcp/client/header_forward.go new file mode 100644 index 0000000000..1708742dd8 --- /dev/null +++ b/pkg/vmcp/client/header_forward.go @@ -0,0 +1,160 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package client + +import ( + "context" + "errors" + "fmt" + "log/slog" + "net/http" + + "github.com/stacklok/toolhive/pkg/secrets" + "github.com/stacklok/toolhive/pkg/transport/middleware" + "github.com/stacklok/toolhive/pkg/vmcp" +) + +// headerForwardRoundTripper injects a pre-resolved set of HTTP headers onto every +// outbound request to a specific backend. Headers are resolved once at client +// creation time (plaintext verbatim + secret identifiers looked up via +// secrets.EnvironmentProvider) and stored in an immutable map. The request is +// cloned before mutation so callers retain an untouched request. +// +// This tripper applies to health checks, listTools/listResources/listPrompts, and +// tool/resource/prompt invocations — every HTTP call made by the vMCP backend +// client passes through the same transport chain. +type headerForwardRoundTripper struct { + base http.RoundTripper + headers http.Header // canonicalized header name → single value +} + +// RoundTrip injects the pre-resolved headers onto a clone of the request and +// delegates to the wrapped transport. Headers already present on the request +// are left untouched so inner transports (auth, identity, trace) can always +// override user-supplied values for the same name. +func (h *headerForwardRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) { + if len(h.headers) == 0 { + return h.base.RoundTrip(req) + } + reqCopy := req.Clone(req.Context()) + if reqCopy.Header == nil { + reqCopy.Header = make(http.Header) + } + for name, values := range h.headers { + if len(values) == 0 { + continue + } + // Do not clobber headers already set by earlier (outer) tripper stages. + // The vMCP runtime is authoritative for identity/trace/auth headers. + if reqCopy.Header.Get(name) != "" { + continue + } + reqCopy.Header.Set(name, values[0]) + } + return h.base.RoundTrip(reqCopy) +} + +// buildHeaderForwardTripper constructs a headerForwardRoundTripper for the +// backend's pre-resolved HeaderForwardConfig. Returns base unchanged when no +// header injection is configured or the effective header set is empty. +// +// Fails loudly (constructor validation, per go-style.md) when a secret identifier +// cannot be resolved through the provider, so a misconfigured backend surfaces +// at pod startup — not as a silent missing-header on every request. +// +// Restricted header names (matching pkg/transport/middleware.RestrictedHeaders) +// are rejected to prevent Host, Content-Length, Authorization, hop-by-hop, and +// X-Forwarded-* spoofing via user-supplied config. +func buildHeaderForwardTripper( + ctx context.Context, + base http.RoundTripper, + cfg *vmcp.HeaderForwardConfig, + provider secrets.Provider, + backendName string, +) (http.RoundTripper, error) { + if cfg == nil { + return base, nil + } + + headers, err := resolveHeaderForward(ctx, cfg, provider, backendName) + if err != nil { + return nil, err + } + if len(headers) == 0 { + return base, nil + } + return &headerForwardRoundTripper{base: base, headers: headers}, nil +} + +// resolveHeaderForward merges plaintext headers with secret-resolved headers into +// a single canonicalized http.Header. Plaintext entries are copied verbatim; +// secret identifiers are looked up via the provider (TOOLHIVE_SECRET_). +// +// Returns an error if any header name is in middleware.RestrictedHeaders or if +// any referenced secret identifier is not present in the environment. +func resolveHeaderForward( + ctx context.Context, + cfg *vmcp.HeaderForwardConfig, + provider secrets.Provider, + backendName string, +) (http.Header, error) { + if cfg == nil { + return nil, nil + } + + size := len(cfg.AddPlaintextHeaders) + len(cfg.AddHeadersFromSecret) + if size == 0 { + return nil, nil + } + out := make(http.Header, size) + + for name, value := range cfg.AddPlaintextHeaders { + canonical := http.CanonicalHeaderKey(name) + if middleware.RestrictedHeaders[canonical] { + return nil, fmt.Errorf( + "backend %q: header %q is restricted and cannot be forwarded", + backendName, canonical, + ) + } + out.Set(canonical, value) + } + + if provider == nil && len(cfg.AddHeadersFromSecret) > 0 { + return nil, fmt.Errorf( + "backend %q: secret provider required to resolve %d header secret(s)", + backendName, len(cfg.AddHeadersFromSecret), + ) + } + + for name, identifier := range cfg.AddHeadersFromSecret { + canonical := http.CanonicalHeaderKey(name) + if middleware.RestrictedHeaders[canonical] { + return nil, fmt.Errorf( + "backend %q: header %q is restricted and cannot be forwarded", + backendName, canonical, + ) + } + value, err := provider.GetSecret(ctx, identifier) + if err != nil { + // Do not include the identifier or value in any user-facing error; + // the log is the only place we record the identifier, not the value. + slog.Error("failed to resolve header-forward secret", + "backend", backendName, "header", canonical, "identifier", identifier, + "error", err) + if errors.Is(err, secrets.ErrSecretNotFound) { + return nil, fmt.Errorf( + "backend %q: header %q secret not found in pod environment", + backendName, canonical, + ) + } + return nil, fmt.Errorf( + "backend %q: failed to resolve header %q from secret provider: %w", + backendName, canonical, err, + ) + } + out.Set(canonical, value) + } + + return out, nil +} diff --git a/pkg/vmcp/client/header_forward_integration_test.go b/pkg/vmcp/client/header_forward_integration_test.go new file mode 100644 index 0000000000..45dacc88ba --- /dev/null +++ b/pkg/vmcp/client/header_forward_integration_test.go @@ -0,0 +1,134 @@ +// SPDX-FileCopyrightText: Copyright 2026 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package client_test + +import ( + "context" + "encoding/json" + "io" + "net/http" + "net/http/httptest" + "strings" + "sync" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/stacklok/toolhive/pkg/secrets" + "github.com/stacklok/toolhive/pkg/vmcp" + "github.com/stacklok/toolhive/pkg/vmcp/auth" + "github.com/stacklok/toolhive/pkg/vmcp/auth/strategies" + vmcpclient "github.com/stacklok/toolhive/pkg/vmcp/client" +) + +// TestHeaderForward_EndToEnd_ThroughHTTPBackendClient drives the full +// NewHTTPBackendClient → defaultClientFactory wiring. It verifies that: +// +// 1. The plaintext header reaches the backend +// 2. A secret-backed header is resolved through the EnvironmentProvider +// using the TOOLHIVE_SECRET_ convention and reaches the backend +// +// Anything below this is exercised by the round-tripper unit tests, but only +// an end-to-end test confirms the *factory wiring itself* (provider lookup, +// transport stacking, ctx threading) is correct in production code paths. +func TestHeaderForward_EndToEnd_ThroughHTTPBackendClient(t *testing.T) { + // t.Setenv forbids t.Parallel — fine here, this test is fast. + const ( + identifier = "HEADER_FORWARD_X_API_KEY_BACKEND_E2E" + envVarName = secrets.EnvVarPrefix + identifier + secretVal = "super-secret-resolved-from-env" + ) + t.Setenv(envVarName, secretVal) + + captured := newCapturingMCPServer(t) + t.Cleanup(captured.server.Close) + + registry := auth.NewDefaultOutgoingAuthRegistry() + require.NoError(t, registry.RegisterStrategy("unauthenticated", &strategies.UnauthenticatedStrategy{})) + + backendClient, err := vmcpclient.NewHTTPBackendClient(registry) + require.NoError(t, err) + + target := &vmcp.BackendTarget{ + WorkloadID: "backend-e2e", + WorkloadName: "Backend E2E", + BaseURL: captured.server.URL, + TransportType: "streamable-http", + HeaderForward: &vmcp.HeaderForwardConfig{ + AddPlaintextHeaders: map[string]string{ + "X-Tenant": "acme", + }, + AddHeadersFromSecret: map[string]string{ + "X-API-Key": identifier, + }, + }, + } + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + // CallTool drives the full Initialize → tools/list flow through the + // streamable-HTTP transport. We don't care about the call result — only + // that the request reached the test server with the configured headers. + _, _ = backendClient.CallTool(ctx, target, "anything", map[string]any{}, nil) + + captured.mu.Lock() + defer captured.mu.Unlock() + require.NotEmpty(t, captured.headers, "test server received no requests") + assert.Equal(t, "acme", captured.headers.Get("X-Tenant"), + "plaintext header must be forwarded by the e2e factory wiring") + assert.Equal(t, secretVal, captured.headers.Get("X-Api-Key"), + "secret-backed header must be resolved via EnvironmentProvider and forwarded") +} + +// capturingMCPServer is a minimal HTTP server that records the headers of +// every inbound request and returns a stub MCP initialize response. It does +// not implement the full MCP protocol — just enough for the streamable-HTTP +// client to send at least one request whose headers we can inspect. +type capturingMCPServer struct { + server *httptest.Server + mu sync.Mutex + headers http.Header +} + +func newCapturingMCPServer(t *testing.T) *capturingMCPServer { + t.Helper() + c := &capturingMCPServer{} + c.server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + c.mu.Lock() + // Only capture the FIRST request — that's the one carrying the headers + // resolved at client-construction time. Subsequent requests on the same + // connection would see the same headers but recording every one makes + // the test read confusing. + if c.headers == nil { + c.headers = r.Header.Clone() + } + c.mu.Unlock() + + // Drain the body so the underlying transport can reuse the connection. + _, _ = io.Copy(io.Discard, r.Body) + _ = r.Body.Close() + + // Return a minimal JSON-RPC error response so the client unwinds + // cleanly. We don't need a full Initialize because we already have + // what we need (the headers) before the client gives up. + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + _ = json.NewEncoder(w).Encode(map[string]any{ + "jsonrpc": "2.0", + "id": 1, + "error": map[string]any{ + "code": -32601, + "message": "method not implemented in test server", + }, + }) + })) + // Sanity check — the test server URL must be plain HTTP (no TLS) so the + // transport doesn't try to verify a self-signed cert against system roots. + require.True(t, strings.HasPrefix(c.server.URL, "http://"), + "capturingMCPServer must serve plain HTTP") + return c +} diff --git a/pkg/vmcp/client/header_forward_test.go b/pkg/vmcp/client/header_forward_test.go new file mode 100644 index 0000000000..eddbfd21c9 --- /dev/null +++ b/pkg/vmcp/client/header_forward_test.go @@ -0,0 +1,243 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package client + +import ( + "context" + "errors" + "io" + "net/http" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/stacklok/toolhive/pkg/secrets" + "github.com/stacklok/toolhive/pkg/transport/middleware" + "github.com/stacklok/toolhive/pkg/vmcp" +) + +// mockProvider is an in-memory stand-in for secrets.Provider so tests don't rely +// on process env. It only implements GetSecret; other methods return errors. +// +// errors maps an identifier to a per-key error to inject — used to verify that +// resolveHeaderForward propagates non-NotFound errors (transient backend +// failures, permission errors, etc.) without translating them to NotFound. +type mockProvider struct { + values map[string]string + errors map[string]error +} + +func (m *mockProvider) GetSecret(_ context.Context, name string) (string, error) { + if m.errors != nil { + if err, ok := m.errors[name]; ok { + return "", err + } + } + if v, ok := m.values[name]; ok { + return v, nil + } + return "", secrets.ErrSecretNotFound +} + +// mockProvider needs to satisfy secrets.Provider; the other methods are unused. +func (*mockProvider) SetSecret(_ context.Context, _, _ string) error { return nil } +func (*mockProvider) DeleteSecret(_ context.Context, _ string) error { return nil } +func (*mockProvider) ListSecrets(_ context.Context) ([]secrets.SecretDescription, error) { + return nil, nil +} +func (*mockProvider) DeleteSecrets(_ context.Context, _ []string) error { return nil } +func (*mockProvider) Cleanup() error { return nil } +func (*mockProvider) Capabilities() secrets.ProviderCapabilities { + return secrets.ProviderCapabilities{CanRead: true} +} + +// captureTripper records the headers seen on the last request that reached it. +type captureTripper struct { + lastHeaders http.Header +} + +func (c *captureTripper) RoundTrip(req *http.Request) (*http.Response, error) { + c.lastHeaders = req.Header.Clone() + return &http.Response{ + StatusCode: http.StatusOK, + Body: io.NopCloser(strings.NewReader("")), + Header: make(http.Header), + }, nil +} + +func TestHeaderForwardRoundTripper_InjectsPlaintext(t *testing.T) { + t.Parallel() + + base := &captureTripper{} + rt := &headerForwardRoundTripper{ + base: base, + headers: http.Header{ + "X-Mcp-Toolsets": []string{"projects,issues"}, + "X-Tenant": []string{"acme"}, + }, + } + + req, err := http.NewRequest(http.MethodGet, "https://example.com", http.NoBody) + require.NoError(t, err) + + _, err = rt.RoundTrip(req) + require.NoError(t, err) + + assert.Equal(t, "projects,issues", base.lastHeaders.Get("X-Mcp-Toolsets")) + assert.Equal(t, "acme", base.lastHeaders.Get("X-Tenant")) + // Original request must remain unmutated. + assert.Empty(t, req.Header.Get("X-Mcp-Toolsets")) +} + +func TestHeaderForwardRoundTripper_NoHeadersIsNoOp(t *testing.T) { + t.Parallel() + + base := &captureTripper{} + rt := &headerForwardRoundTripper{base: base, headers: nil} + + req, err := http.NewRequest(http.MethodGet, "https://example.com", http.NoBody) + require.NoError(t, err) + + _, err = rt.RoundTrip(req) + require.NoError(t, err) + assert.Empty(t, base.lastHeaders.Get("X-Mcp-Toolsets")) +} + +func TestHeaderForwardRoundTripper_DoesNotClobberExistingHeader(t *testing.T) { + t.Parallel() + + base := &captureTripper{} + rt := &headerForwardRoundTripper{ + base: base, + headers: http.Header{ + "Authorization": []string{"Bearer user-provided"}, + }, + } + + req, err := http.NewRequest(http.MethodGet, "https://example.com", http.NoBody) + require.NoError(t, err) + // Pre-set the header as an outer (identity/trace/auth) tripper would. + req.Header.Set("Authorization", "Bearer real-auth") + + _, err = rt.RoundTrip(req) + require.NoError(t, err) + + assert.Equal(t, "Bearer real-auth", base.lastHeaders.Get("Authorization"), + "user-supplied header-forward value must not clobber an already-set header") +} + +func TestResolveHeaderForward_PlaintextAndSecretMerged(t *testing.T) { + t.Parallel() + + cfg := &vmcp.HeaderForwardConfig{ + AddPlaintextHeaders: map[string]string{ + "X-Tenant": "acme", + }, + AddHeadersFromSecret: map[string]string{ + "X-API-Key": "HEADER_FORWARD_X_API_KEY_BACKEND_A", + }, + } + provider := &mockProvider{values: map[string]string{ + "HEADER_FORWARD_X_API_KEY_BACKEND_A": "resolved-from-env", + }} + + hdr, err := resolveHeaderForward(t.Context(), cfg, provider, "backend-a") + require.NoError(t, err) + assert.Equal(t, "acme", hdr.Get("X-Tenant")) + assert.Equal(t, "resolved-from-env", hdr.Get("X-Api-Key")) +} + +func TestResolveHeaderForward_SecretNotFoundReturnsError(t *testing.T) { + t.Parallel() + + cfg := &vmcp.HeaderForwardConfig{ + AddHeadersFromSecret: map[string]string{ + "X-API-Key": "HEADER_FORWARD_X_API_KEY_BACKEND_A", + }, + } + provider := &mockProvider{values: map[string]string{}} + + hdr, err := resolveHeaderForward(t.Context(), cfg, provider, "backend-a") + require.Error(t, err) + assert.Nil(t, hdr) + // Error must not leak the identifier or the backend's private values. + assert.NotContains(t, err.Error(), "HEADER_FORWARD_X_API_KEY_BACKEND_A") +} + +// TestResolveHeaderForward_RestrictedHeaderRejected iterates the full +// middleware.RestrictedHeaders set and verifies every entry is rejected via +// both the plaintext and secret-backed paths. Hardcoding a subset would let a +// future addition to RestrictedHeaders silently fall out of coverage. +func TestResolveHeaderForward_RestrictedHeaderRejected(t *testing.T) { + t.Parallel() + + require.NotEmpty(t, middleware.RestrictedHeaders, "guard against empty map") + + for header := range middleware.RestrictedHeaders { + t.Run("plaintext_"+header, func(t *testing.T) { + t.Parallel() + cfg := &vmcp.HeaderForwardConfig{ + AddPlaintextHeaders: map[string]string{header: "x"}, + } + _, err := resolveHeaderForward(t.Context(), cfg, &mockProvider{}, "backend-x") + require.Error(t, err) + assert.Contains(t, err.Error(), "restricted") + }) + + t.Run("secret_"+header, func(t *testing.T) { + t.Parallel() + cfg := &vmcp.HeaderForwardConfig{ + AddHeadersFromSecret: map[string]string{header: "HEADER_FORWARD_RESTRICTED"}, + } + _, err := resolveHeaderForward(t.Context(), cfg, &mockProvider{}, "backend-x") + require.Error(t, err) + assert.Contains(t, err.Error(), "restricted") + }) + } +} + +// TestResolveHeaderForward_NonNotFoundProviderError verifies that a transient +// secret-provider failure (e.g. permission denied, timeout) is propagated with +// its original chain intact — distinct from the user-facing "not found" +// message produced for ErrSecretNotFound. +func TestResolveHeaderForward_NonNotFoundProviderError(t *testing.T) { + t.Parallel() + + wantErr := errors.New("permission denied calling secrets backend") + cfg := &vmcp.HeaderForwardConfig{ + AddHeadersFromSecret: map[string]string{ + "X-API-Key": "HEADER_FORWARD_X_API_KEY_BACKEND_A", + }, + } + provider := &mockProvider{ + errors: map[string]error{ + "HEADER_FORWARD_X_API_KEY_BACKEND_A": wantErr, + }, + } + + hdr, err := resolveHeaderForward(t.Context(), cfg, provider, "backend-a") + require.Error(t, err) + assert.Nil(t, hdr) + require.ErrorIs(t, err, wantErr, + "non-NotFound provider errors must be wrapped with %%w so callers can errors.Is them") + assert.NotContains(t, err.Error(), "not found", + "only ErrSecretNotFound should produce the user-facing 'not found' message") +} + +func TestResolveHeaderForward_NilCfgReturnsNil(t *testing.T) { + t.Parallel() + hdr, err := resolveHeaderForward(t.Context(), nil, nil, "x") + require.NoError(t, err) + assert.Nil(t, hdr) +} + +func TestBuildHeaderForwardTripper_NilCfgReturnsBase(t *testing.T) { + t.Parallel() + base := &captureTripper{} + got, err := buildHeaderForwardTripper(t.Context(), base, nil, nil, "x") + require.NoError(t, err) + assert.Same(t, base, got, "nil cfg must pass base through untouched") +} From c328915d174489a2b19ce0036178800c37b9f314 Mon Sep 17 00:00:00 2001 From: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com> Date: Sun, 10 May 2026 14:30:02 +0100 Subject: [PATCH 7/7] Thread per-backend headerForward through aggregator, workloads, and CLI The static-mode discoverer now keys headerForward by normalized backend name and stamps each Backend with its config at discovery time. The Kubernetes workload discoverer surfaces the same field on managed entries, and the health monitor forwards it through to client calls. vMCP startup ingests the operator-emitted TOOLHIVE_HEADER_FORWARD_* env vars and routes the resulting per-backend map through serve into the discoverer. --- pkg/vmcp/aggregator/discoverer.go | 28 +++- pkg/vmcp/aggregator/discoverer_test.go | 4 + pkg/vmcp/cli/header_forward_env.go | 74 ++++++++++ pkg/vmcp/cli/header_forward_env_test.go | 151 +++++++++++++++++++++ pkg/vmcp/cli/serve.go | 11 ++ pkg/vmcp/health/monitor.go | 8 +- pkg/vmcp/workloads/k8s.go | 38 ++++++ pkg/vmcp/workloads/k8s_test.go | 173 ++++++++++++++++++++++++ 8 files changed, 481 insertions(+), 6 deletions(-) create mode 100644 pkg/vmcp/cli/header_forward_env.go create mode 100644 pkg/vmcp/cli/header_forward_env_test.go diff --git a/pkg/vmcp/aggregator/discoverer.go b/pkg/vmcp/aggregator/discoverer.go index 4a26b9937a..d50f409ed7 100644 --- a/pkg/vmcp/aggregator/discoverer.go +++ b/pkg/vmcp/aggregator/discoverer.go @@ -21,6 +21,7 @@ import ( "github.com/stacklok/toolhive/pkg/groups" "github.com/stacklok/toolhive/pkg/vmcp" "github.com/stacklok/toolhive/pkg/vmcp/config" + "github.com/stacklok/toolhive/pkg/vmcp/headerforward/wirefmt" "github.com/stacklok/toolhive/pkg/vmcp/workloads" workloadsmgr "github.com/stacklok/toolhive/pkg/workloads" ) @@ -33,6 +34,15 @@ type backendDiscoverer struct { authConfig *config.OutgoingAuthConfig staticBackends []config.StaticBackendConfig // Pre-configured backends for static mode groupRef string // Group reference for static mode metadata + + // headerForwardByBackend is keyed by the NORMALIZED backend name (the + // suffix the operator emits in TOOLHIVE_HEADER_FORWARD_). The + // canonical backend name from the static config is normalized on + // lookup via wirefmt.NormalizeForEnvVar so the keying matches + // the operator-side env-var encoding. Nil/empty when no entry declared + // headerForward. Only populated in static mode — dynamic mode reads + // headerForward directly from the MCPServerEntry CRD. + headerForwardByBackend map[string]*vmcp.HeaderForwardConfig } // NewUnifiedBackendDiscoverer creates a unified backend discoverer that works with both @@ -55,17 +65,24 @@ func NewUnifiedBackendDiscoverer( // NewUnifiedBackendDiscovererWithStaticBackends creates a backend discoverer for static mode // with pre-configured backends, eliminating the need for K8s API access. +// +// headerForwardByBackend carries per-backend HeaderForwardConfig (keyed by +// backend name) constructed at startup from the operator-emitted env vars. +// Pass nil when no entry in the group declares headerForward — the discoverer +// will simply leave Backend.HeaderForward nil for every backend. func NewUnifiedBackendDiscovererWithStaticBackends( staticBackends []config.StaticBackendConfig, authConfig *config.OutgoingAuthConfig, groupRef string, + headerForwardByBackend map[string]*vmcp.HeaderForwardConfig, ) BackendDiscoverer { return &backendDiscoverer{ - workloadsManager: nil, // Not needed in static mode - groupsManager: nil, // Not needed in static mode - authConfig: authConfig, - staticBackends: staticBackends, - groupRef: groupRef, + workloadsManager: nil, // Not needed in static mode + groupsManager: nil, // Not needed in static mode + authConfig: authConfig, + staticBackends: staticBackends, + groupRef: groupRef, + headerForwardByBackend: headerForwardByBackend, } } @@ -272,6 +289,7 @@ func (d *backendDiscoverer) discoverFromStaticConfig() []vmcp.Backend { Type: vmcp.BackendType(staticBackend.Type), CABundlePath: staticBackend.CABundlePath, HealthStatus: vmcp.BackendHealthy, // Assume healthy, actual health check happens later + HeaderForward: d.headerForwardByBackend[wirefmt.NormalizeForEnvVar(staticBackend.Name)], Metadata: staticBackend.Metadata, } diff --git a/pkg/vmcp/aggregator/discoverer_test.go b/pkg/vmcp/aggregator/discoverer_test.go index 6321662fa5..1e3bc50369 100644 --- a/pkg/vmcp/aggregator/discoverer_test.go +++ b/pkg/vmcp/aggregator/discoverer_test.go @@ -1186,6 +1186,7 @@ func TestStaticBackendDiscoverer_EmptyBackendList(t *testing.T) { []config.StaticBackendConfig{}, // Empty slice, not nil nil, // No auth config "test-group", + nil, // No headerForward map ) // This should return empty list without panicking @@ -1281,6 +1282,7 @@ func TestStaticBackendDiscoverer_MetadataGroupOverride(t *testing.T) { tt.staticBackends, nil, // No auth config needed for this test tt.groupRef, + nil, // No headerForward map for this test ) backends, err := discoverer.Discover(ctx, tt.groupRef) @@ -1400,6 +1402,7 @@ func TestStaticBackendDiscoverer_EntryBackendFields(t *testing.T) { tt.staticBackends, nil, // No auth config needed for this test tt.groupRef, + nil, // No headerForward map for this test ) backends, err := discoverer.Discover(ctx, tt.groupRef) @@ -1466,6 +1469,7 @@ func TestBackendDiscoverer_Discover_DeterministicOrdering(t *testing.T) { tc.staticBackends, nil, // No auth config needed for this test "test-group", + nil, // No headerForward map for this test ) backends, err := discoverer.Discover(ctx, "test-group") diff --git a/pkg/vmcp/cli/header_forward_env.go b/pkg/vmcp/cli/header_forward_env.go new file mode 100644 index 0000000000..1e5a84a285 --- /dev/null +++ b/pkg/vmcp/cli/header_forward_env.go @@ -0,0 +1,74 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package cli + +import ( + "encoding/json" + "log/slog" + "strings" + + "github.com/stacklok/toolhive/pkg/vmcp" + "github.com/stacklok/toolhive/pkg/vmcp/headerforward/wirefmt" +) + +// readHeaderForwardFromEnv reconstructs the per-backend +// vmcp.HeaderForwardConfig map by walking environment variables emitted by +// the operator on the vMCP pod. +// +// The operator emits one JSON-encoded manifest env var per backend named +// "TOOLHIVE_HEADER_FORWARD_". The JSON value carries +// every configured header for that backend with original (un-normalized) +// names preserved: +// +// { +// "addPlaintextHeaders": {"X-MCP-Toolsets":"projects,issues"}, +// "addHeadersFromSecret": {"X-Api-Key":"HEADER_FORWARD_X_API_KEY_"} +// } +// +// AddHeadersFromSecret values are secret IDENTIFIERS, not values. Secret +// values resolve later inside resolveHeaderForward via +// secrets.EnvironmentProvider, which reads TOOLHIVE_SECRET_ +// env vars (delivered separately via valueFrom.secretKeyRef so the value +// never enters the operator's view of the world). +// +// The map key is the normalized entry segment from the env-var suffix — +// the SAME value the operator's GenerateHeaderForwardManifestEnvVarName +// produces for that backend's name. Callers look up by passing the +// original backend name through ctrlutil.NormalizeHeaderForEnvVar before +// indexing. +func readHeaderForwardFromEnv(envEntries []string) map[string]*vmcp.HeaderForwardConfig { + out := make(map[string]*vmcp.HeaderForwardConfig) + for _, entry := range envEntries { + name, value, ok := strings.Cut(entry, "=") + if !ok || !strings.HasPrefix(name, wirefmt.ManifestEnvVarPrefix) { + continue + } + ownerSegment := strings.TrimPrefix(name, wirefmt.ManifestEnvVarPrefix) + + var cfg vmcp.HeaderForwardConfig + if err := json.Unmarshal([]byte(value), &cfg); err != nil { + // A malformed manifest is a programmer error in the operator; + // log and skip rather than fail the whole startup. The backend + // will simply have no headerForward attached. + slog.Warn("invalid headerForward manifest from env, skipping", + "envvar", name, "error", err) + continue + } + // wirefmt.NormalizeForEnvVar maps two distinct entry names with the + // same uppercased+sanitized form to the same env-var suffix (e.g. + // "github-copilot" and "github_copilot"). DNS-1123 forbids + // underscores in entry names, so this collision is unreachable in + // production today — but a future relaxation, a migration that + // allows mixed casing, or a third-party producing manifests can all + // land us here. Surface the collision loudly: the second config + // silently overwriting the first would mask a misconfiguration that + // is otherwise extremely hard to debug. + if _, dup := out[ownerSegment]; dup { + slog.Warn("duplicate headerForward manifest env var; later value overrides earlier", + "envvar", name, "ownerSegment", ownerSegment) + } + out[ownerSegment] = &cfg + } + return out +} diff --git a/pkg/vmcp/cli/header_forward_env_test.go b/pkg/vmcp/cli/header_forward_env_test.go new file mode 100644 index 0000000000..4683c08335 --- /dev/null +++ b/pkg/vmcp/cli/header_forward_env_test.go @@ -0,0 +1,151 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package cli + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/stacklok/toolhive/pkg/vmcp" +) + +// TestReadHeaderForwardFromEnv covers reconstructing the per-backend +// HeaderForwardConfig from the JSON-encoded TOOLHIVE_HEADER_FORWARD_ +// env vars the operator emits on the vMCP pod. Original header casing / +// punctuation is preserved by the JSON value, so the runtime reconstructs +// "X-MCP-Toolsets" rather than "X_MCP_TOOLSETS". +// +// The map is keyed by the normalized entry segment from the env-var +// suffix; the discoverer normalizes Backend.Name through +// ctrlutil.NormalizeHeaderForEnvVar before indexing. +func TestReadHeaderForwardFromEnv(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + env []string + validate func(t *testing.T, m map[string]*vmcp.HeaderForwardConfig) + }{ + { + name: "no env vars yields empty map", + env: []string{"HOME=/root", "PATH=/bin"}, + validate: func(t *testing.T, m map[string]*vmcp.HeaderForwardConfig) { + t.Helper() + assert.Empty(t, m) + }, + }, + { + name: "plaintext header decodes preserving original casing and hyphens", + env: []string{ + `TOOLHIVE_HEADER_FORWARD_GITHUB_COPILOT={"addPlaintextHeaders":{"X-MCP-Toolsets":"projects,issues"}}`, + "PATH=/bin", + }, + validate: func(t *testing.T, m map[string]*vmcp.HeaderForwardConfig) { + t.Helper() + cfg := m["GITHUB_COPILOT"] + require.NotNil(t, cfg) + assert.Equal(t, map[string]string{"X-MCP-Toolsets": "projects,issues"}, cfg.AddPlaintextHeaders) + assert.Empty(t, cfg.AddHeadersFromSecret) + }, + }, + { + name: "secret header decodes to identifier (value not in manifest)", + env: []string{ + `TOOLHIVE_HEADER_FORWARD_STRIPE={"addHeadersFromSecret":{"X-Api-Key":"HEADER_FORWARD_X_API_KEY_STRIPE"}}`, + // The secret-value env var carries the resolved value via + // secretKeyRef; the walker must NOT treat it as a manifest. + "TOOLHIVE_SECRET_HEADER_FORWARD_X_API_KEY_STRIPE=resolved-secret-value", + }, + validate: func(t *testing.T, m map[string]*vmcp.HeaderForwardConfig) { + t.Helper() + assert.NotContains(t, m, "X_API_KEY_STRIPE", "secret env var must not be parsed as a manifest") + cfg := m["STRIPE"] + require.NotNil(t, cfg) + assert.Empty(t, cfg.AddPlaintextHeaders) + assert.Equal(t, + map[string]string{"X-Api-Key": "HEADER_FORWARD_X_API_KEY_STRIPE"}, + cfg.AddHeadersFromSecret, + ) + }, + }, + { + name: "malformed manifest is skipped without failing other backends", + env: []string{ + `TOOLHIVE_HEADER_FORWARD_BROKEN=not-json`, + `TOOLHIVE_HEADER_FORWARD_DEMO={"addPlaintextHeaders":{"X-Trace":"ok"}}`, + }, + validate: func(t *testing.T, m map[string]*vmcp.HeaderForwardConfig) { + t.Helper() + assert.NotContains(t, m, "BROKEN") + cfg := m["DEMO"] + require.NotNil(t, cfg) + assert.Equal(t, map[string]string{"X-Trace": "ok"}, cfg.AddPlaintextHeaders) + }, + }, + { + name: "mixed plaintext + secret in one manifest scoped per backend", + env: []string{ + `TOOLHIVE_HEADER_FORWARD_ALPHA={"addPlaintextHeaders":{"X-Trace":"alpha-trace"},"addHeadersFromSecret":{"X-Token":"HEADER_FORWARD_X_TOKEN_ALPHA"}}`, + `TOOLHIVE_HEADER_FORWARD_BETA={"addPlaintextHeaders":{"X-Trace":"beta-trace"}}`, + }, + validate: func(t *testing.T, m map[string]*vmcp.HeaderForwardConfig) { + t.Helper() + + alpha := m["ALPHA"] + require.NotNil(t, alpha) + assert.Equal(t, map[string]string{"X-Trace": "alpha-trace"}, alpha.AddPlaintextHeaders) + assert.Equal(t, + map[string]string{"X-Token": "HEADER_FORWARD_X_TOKEN_ALPHA"}, + alpha.AddHeadersFromSecret, + ) + + beta := m["BETA"] + require.NotNil(t, beta) + assert.Equal(t, map[string]string{"X-Trace": "beta-trace"}, beta.AddPlaintextHeaders) + assert.Empty(t, beta.AddHeadersFromSecret) + }, + }, + { + name: "malformed env entry without '=' is skipped", + env: []string{ + "NO_EQUALS_HERE", + `TOOLHIVE_HEADER_FORWARD_DEMO={"addPlaintextHeaders":{"X-Trace":"ok"}}`, + }, + validate: func(t *testing.T, m map[string]*vmcp.HeaderForwardConfig) { + t.Helper() + cfg := m["DEMO"] + require.NotNil(t, cfg) + assert.Equal(t, map[string]string{"X-Trace": "ok"}, cfg.AddPlaintextHeaders) + }, + }, + { + // DNS-1123 forbids underscores in MCPServerEntry names today, so + // real entries with the same NormalizeForEnvVar output cannot + // coexist. We pass two manifests that share the same suffix + // directly to verify the runtime keeps "last wins" behavior and + // surfaces a warning rather than silently dropping the first. + name: "duplicate manifest suffix retains last value", + env: []string{ + `TOOLHIVE_HEADER_FORWARD_DUP={"addPlaintextHeaders":{"X-Trace":"first"}}`, + `TOOLHIVE_HEADER_FORWARD_DUP={"addPlaintextHeaders":{"X-Trace":"second"}}`, + }, + validate: func(t *testing.T, m map[string]*vmcp.HeaderForwardConfig) { + t.Helper() + cfg := m["DUP"] + require.NotNil(t, cfg) + assert.Equal(t, map[string]string{"X-Trace": "second"}, cfg.AddPlaintextHeaders, + "duplicate manifest must keep the later value (last-write-wins)") + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + tt.validate(t, readHeaderForwardFromEnv(tt.env)) + }) + } +} diff --git a/pkg/vmcp/cli/serve.go b/pkg/vmcp/cli/serve.go index ffb501115c..727146f399 100644 --- a/pkg/vmcp/cli/serve.go +++ b/pkg/vmcp/cli/serve.go @@ -566,10 +566,21 @@ func discoverBackends( if len(cfg.Backends) > 0 { // Static mode: use pre-configured backends from config. slog.Info(fmt.Sprintf("Static mode: using %d pre-configured backends", len(cfg.Backends))) + + // Reconstruct per-backend HeaderForwardConfig from env vars the + // operator emitted on this pod. Plaintext header values are inline + // in the JSON manifest; secret-backed headers carry only identifiers + // here and resolve later via secrets.EnvironmentProvider at request + // time. Map keys are the normalized entry segment from the env-var + // suffix; the discoverer normalizes Backend.Name through + // ctrlutil.NormalizeHeaderForEnvVar to look up the matching entry. + // Returns an empty map when no entry in the group declared + // headerForward — the common case. discoverer = aggregator.NewUnifiedBackendDiscovererWithStaticBackends( cfg.Backends, cfg.OutgoingAuth, cfg.Group, + readHeaderForwardFromEnv(os.Environ()), ) } else { // Dynamic mode: discover backends at runtime from the active workload manager (K8s or local). diff --git a/pkg/vmcp/health/monitor.go b/pkg/vmcp/health/monitor.go index 38eafef800..82a6c72fac 100644 --- a/pkg/vmcp/health/monitor.go +++ b/pkg/vmcp/health/monitor.go @@ -427,13 +427,19 @@ func (m *Monitor) performHealthCheck(ctx context.Context, backend *vmcp.Backend) return } - // Create BackendTarget from Backend + // Create BackendTarget from Backend. Carry CA bundle and header-forward config + // so health checks hit the backend with the same TLS trust and header injection + // as list/call requests — otherwise a healthy-to-the-monitor backend could fail + // for real traffic (or vice versa). target := &vmcp.BackendTarget{ WorkloadID: backend.ID, WorkloadName: backend.Name, BaseURL: backend.BaseURL, TransportType: backend.TransportType, + CABundlePath: backend.CABundlePath, + CABundleData: backend.CABundleData, AuthConfig: backend.AuthConfig, + HeaderForward: backend.HeaderForward, HealthStatus: vmcp.BackendUnknown, // Status is determined by the health check Metadata: backend.Metadata, } diff --git a/pkg/vmcp/workloads/k8s.go b/pkg/vmcp/workloads/k8s.go index 7a8ffa557e..94b92eacc1 100644 --- a/pkg/vmcp/workloads/k8s.go +++ b/pkg/vmcp/workloads/k8s.go @@ -22,6 +22,7 @@ import ( transporttypes "github.com/stacklok/toolhive/pkg/transport/types" "github.com/stacklok/toolhive/pkg/vmcp" "github.com/stacklok/toolhive/pkg/vmcp/auth/converters" + "github.com/stacklok/toolhive/pkg/vmcp/headerforward/wirefmt" "github.com/stacklok/toolhive/pkg/workloads/types" ) @@ -582,9 +583,46 @@ func (d *k8sDiscoverer) mcpServerEntryToBackend(ctx context.Context, entry *mcpv return nil } + // Per-backend HTTP header injection. Mirrors the static-mode operator + // path in cmd/thv-operator/controllers/virtualmcpserver_deployment.go::buildHeaderForwardManifestForEntry: + // plaintext values verbatim, secret refs translated to identifiers + // (HEADER_FORWARD__) the round tripper resolves at request + // time via secrets.EnvironmentProvider. + backend.HeaderForward = headerForwardFromEntry(entry) + return backend } +// headerForwardFromEntry projects MCPServerEntry.spec.headerForward onto the +// runtime *vmcp.HeaderForwardConfig the round tripper consumes. Returns nil +// when the entry has no headerForward configured. Secret values never +// appear here — only identifiers, which are dereferenced at request time. +func headerForwardFromEntry(entry *mcpv1beta1.MCPServerEntry) *vmcp.HeaderForwardConfig { + if entry == nil || entry.Spec.HeaderForward == nil { + return nil + } + src := entry.Spec.HeaderForward + + cfg := &vmcp.HeaderForwardConfig{ + AddPlaintextHeaders: src.AddPlaintextHeaders, + } + for _, ref := range src.AddHeadersFromSecret { + if ref.ValueSecretRef == nil { + continue + } + _, identifier := wirefmt.SecretEnvVarName(entry.Name, ref.HeaderName) + if cfg.AddHeadersFromSecret == nil { + cfg.AddHeadersFromSecret = make(map[string]string) + } + cfg.AddHeadersFromSecret[ref.HeaderName] = identifier + } + + if len(cfg.AddPlaintextHeaders) == 0 && len(cfg.AddHeadersFromSecret) == 0 { + return nil + } + return cfg +} + // mapMCPServerEntryPhaseToHealth converts a MCPServerEntryPhase to a backend health status. func mapMCPServerEntryPhaseToHealth(phase mcpv1beta1.MCPServerEntryPhase) vmcp.BackendHealthStatus { switch phase { diff --git a/pkg/vmcp/workloads/k8s_test.go b/pkg/vmcp/workloads/k8s_test.go index edae76e868..5c32660044 100644 --- a/pkg/vmcp/workloads/k8s_test.go +++ b/pkg/vmcp/workloads/k8s_test.go @@ -5,6 +5,7 @@ package workloads import ( "context" + "encoding/json" "testing" "github.com/stretchr/testify/assert" @@ -1829,3 +1830,175 @@ func TestFetchCABundleData(t *testing.T) { }) } } + +// TestHeaderForwardFromEntry covers the dynamic-mode projection of +// MCPServerEntry.spec.headerForward into the runtime *vmcp.HeaderForwardConfig. +// The shape MUST match what the operator emits in the static-mode JSON +// manifest env var so both code paths feed equivalent data into the round +// tripper. +func TestHeaderForwardFromEntry(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + entry *mcpv1beta1.MCPServerEntry + want *vmcp.HeaderForwardConfig + }{ + { + name: "nil entry yields nil", + entry: nil, + want: nil, + }, + { + name: "entry without headerForward yields nil", + entry: &mcpv1beta1.MCPServerEntry{ + ObjectMeta: metav1.ObjectMeta{Name: "demo", Namespace: testNamespace}, + Spec: mcpv1beta1.MCPServerEntrySpec{RemoteURL: "https://x"}, + }, + want: nil, + }, + { + name: "plaintext only — header names preserved verbatim", + entry: &mcpv1beta1.MCPServerEntry{ + ObjectMeta: metav1.ObjectMeta{Name: "github-copilot", Namespace: testNamespace}, + Spec: mcpv1beta1.MCPServerEntrySpec{ + HeaderForward: &mcpv1beta1.HeaderForwardConfig{ + AddPlaintextHeaders: map[string]string{ + "X-MCP-Toolsets": "projects,issues", + "X-Trace-Id": "kind-test", + }, + }, + }, + }, + want: &vmcp.HeaderForwardConfig{ + AddPlaintextHeaders: map[string]string{ + "X-MCP-Toolsets": "projects,issues", + "X-Trace-Id": "kind-test", + }, + }, + }, + { + name: "secret only — values become identifiers", + entry: &mcpv1beta1.MCPServerEntry{ + ObjectMeta: metav1.ObjectMeta{Name: "stripe", Namespace: testNamespace}, + Spec: mcpv1beta1.MCPServerEntrySpec{ + HeaderForward: &mcpv1beta1.HeaderForwardConfig{ + AddHeadersFromSecret: []mcpv1beta1.HeaderFromSecret{ + { + HeaderName: "X-API-Key", + ValueSecretRef: &mcpv1beta1.SecretKeyRef{ + Name: "stripe-key", + Key: "token", + }, + }, + }, + }, + }, + }, + want: &vmcp.HeaderForwardConfig{ + AddHeadersFromSecret: map[string]string{ + "X-API-Key": "HEADER_FORWARD_X_API_KEY_STRIPE", + }, + }, + }, + { + name: "mixed plaintext + secret", + entry: &mcpv1beta1.MCPServerEntry{ + ObjectMeta: metav1.ObjectMeta{Name: "alpha", Namespace: testNamespace}, + Spec: mcpv1beta1.MCPServerEntrySpec{ + HeaderForward: &mcpv1beta1.HeaderForwardConfig{ + AddPlaintextHeaders: map[string]string{"X-Trace": "alpha-trace"}, + AddHeadersFromSecret: []mcpv1beta1.HeaderFromSecret{ + { + HeaderName: "X-Token", + ValueSecretRef: &mcpv1beta1.SecretKeyRef{ + Name: "alpha-secret", + Key: "tok", + }, + }, + }, + }, + }, + }, + want: &vmcp.HeaderForwardConfig{ + AddPlaintextHeaders: map[string]string{"X-Trace": "alpha-trace"}, + AddHeadersFromSecret: map[string]string{ + "X-Token": "HEADER_FORWARD_X_TOKEN_ALPHA", + }, + }, + }, + { + name: "secret ref with nil ValueSecretRef is skipped — empty config returns nil", + entry: &mcpv1beta1.MCPServerEntry{ + ObjectMeta: metav1.ObjectMeta{Name: "ghost", Namespace: testNamespace}, + Spec: mcpv1beta1.MCPServerEntrySpec{ + HeaderForward: &mcpv1beta1.HeaderForwardConfig{ + AddHeadersFromSecret: []mcpv1beta1.HeaderFromSecret{ + {HeaderName: "X-Stray", ValueSecretRef: nil}, + }, + }, + }, + }, + want: nil, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + got := headerForwardFromEntry(tt.entry) + assert.Equal(t, tt.want, got) + }) + } +} + +// TestHeaderForwardParity_StaticVsDynamic pins that the dynamic-mode +// projection produces a runtime-equivalent shape to what the operator's +// static-mode JSON manifest serializes. If either path drifts, the round +// tripper sees a different Backend.HeaderForward depending on mode — +// which is exactly the bug pattern PR #5239 fixes for static mode and +// this finding (F2) extends to dynamic mode. +// +// The test compares the JSON serialization of the dynamic-mode result +// to a hand-written manifest that mirrors what +// virtualmcpserver_deployment.go::buildHeaderForwardManifestForEntry +// would emit for the same entry. The two are required to be byte-equal +// after json.Marshal (which sorts map keys). +func TestHeaderForwardParity_StaticVsDynamic(t *testing.T) { + t.Parallel() + + entry := &mcpv1beta1.MCPServerEntry{ + ObjectMeta: metav1.ObjectMeta{Name: "github-copilot-fake", Namespace: testNamespace}, + Spec: mcpv1beta1.MCPServerEntrySpec{ + RemoteURL: "https://api.example/mcp", + Transport: "streamable-http", + GroupRef: &mcpv1beta1.MCPGroupRef{Name: "g"}, + HeaderForward: &mcpv1beta1.HeaderForwardConfig{ + AddPlaintextHeaders: map[string]string{ + "X-MCP-Toolsets": "projects,issues,pull_requests", + "X-Trace-Id": "kind-test", + }, + AddHeadersFromSecret: []mcpv1beta1.HeaderFromSecret{ + { + HeaderName: "X-Api-Key", + ValueSecretRef: &mcpv1beta1.SecretKeyRef{ + Name: "test-secret", + Key: "token", + }, + }, + }, + }, + }, + } + + dynamic := headerForwardFromEntry(entry) + require.NotNil(t, dynamic) + + dynamicJSON, err := json.Marshal(dynamic) + require.NoError(t, err) + + const wantStaticManifestJSON = `{"addPlaintextHeaders":{"X-MCP-Toolsets":"projects,issues,pull_requests","X-Trace-Id":"kind-test"},"addHeadersFromSecret":{"X-Api-Key":"HEADER_FORWARD_X_API_KEY_GITHUB_COPILOT_FAKE"}}` + + assert.JSONEq(t, wantStaticManifestJSON, string(dynamicJSON), + "dynamic-mode HeaderForward must marshal to the same shape as the static-mode operator manifest") +}