diff --git a/cmd/thv-operator/api/v1alpha1/mcpexternalauthconfig_types.go b/cmd/thv-operator/api/v1alpha1/mcpexternalauthconfig_types.go index acf7dd0ee4..66ff4b8ef3 100644 --- a/cmd/thv-operator/api/v1alpha1/mcpexternalauthconfig_types.go +++ b/cmd/thv-operator/api/v1alpha1/mcpexternalauthconfig_types.go @@ -227,6 +227,16 @@ type EmbeddedAuthServerConfig struct { // +optional Storage *AuthServerStorageConfig `json:"storage,omitempty"` + // DisableUpstreamTokenInjection prevents the embedded auth server from injecting + // upstream IdP tokens into requests forwarded to the backend MCP server. + // When true, the embedded auth server still handles OAuth flows for clients + // but does not swap ToolHive JWTs for upstream tokens on outgoing requests. + // This is useful when the backend MCP server does not require authentication + // (e.g., public documentation servers) but you still want client authentication. + // +kubebuilder:default=false + // +optional + DisableUpstreamTokenInjection bool `json:"disableUpstreamTokenInjection,omitempty"` + // AllowedAudiences is the list of valid resource URIs that tokens can be issued for. // For an embedded auth server, this can be determined by the servers (MCP or vMCP) it serves. diff --git a/cmd/thv-operator/controllers/mcpremoteproxy_controller_test.go b/cmd/thv-operator/controllers/mcpremoteproxy_controller_test.go index b833347395..5a15ed4ef5 100644 --- a/cmd/thv-operator/controllers/mcpremoteproxy_controller_test.go +++ b/cmd/thv-operator/controllers/mcpremoteproxy_controller_test.go @@ -89,8 +89,20 @@ func TestMCPRemoteProxyValidateSpec(t *testing.T) { expectError: true, errContains: "remote URL must not be empty", }, - // Note: "missing OIDC config" test removed - OIDCConfig is now a required value type - // with kubebuilder:validation:Required, so the API server prevents resources without it + { + name: "valid spec without OIDC config (anonymous access)", + proxy: &mcpv1alpha1.MCPRemoteProxy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "anon-proxy", + Namespace: "default", + }, + Spec: mcpv1alpha1.MCPRemoteProxySpec{ + RemoteURL: "https://docs.example.com/mcp", + ProxyPort: 8080, + }, + }, + expectError: false, + }, { name: "with valid external auth config", proxy: &mcpv1alpha1.MCPRemoteProxy{ diff --git a/cmd/thv-operator/controllers/mcpremoteproxy_deployment.go b/cmd/thv-operator/controllers/mcpremoteproxy_deployment.go index bca8face85..f180b29e97 100644 --- a/cmd/thv-operator/controllers/mcpremoteproxy_deployment.go +++ b/cmd/thv-operator/controllers/mcpremoteproxy_deployment.go @@ -179,39 +179,8 @@ func (r *MCPRemoteProxyReconciler) buildEnvVarsForProxy( env = append(env, otelEnvVars...) } - // Add token exchange environment variables - // Note: Embedded auth server env vars are added separately in deploymentForMCPRemoteProxy - // to avoid duplicate API calls. - if proxy.Spec.ExternalAuthConfigRef != nil { - tokenExchangeEnvVars, err := ctrlutil.GenerateTokenExchangeEnvVars( - ctx, - r.Client, - proxy.Namespace, - proxy.Spec.ExternalAuthConfigRef, - ctrlutil.GetExternalAuthConfigByName, - ) - if err != nil { - ctxLogger := log.FromContext(ctx) - ctxLogger.Error(err, "Failed to generate token exchange environment variables") - } else { - env = append(env, tokenExchangeEnvVars...) - } - - // Add bearer token environment variables - bearerTokenEnvVars, err := ctrlutil.GenerateBearerTokenEnvVar( - ctx, - r.Client, - proxy.Namespace, - proxy.Spec.ExternalAuthConfigRef, - ctrlutil.GetExternalAuthConfigByName, - ) - if err != nil { - ctxLogger := log.FromContext(ctx) - ctxLogger.Error(err, "Failed to generate bearer token environment variables") - } else { - env = append(env, bearerTokenEnvVars...) - } - } + // Add token exchange and bearer token environment variables + env = append(env, r.buildExternalAuthEnvVars(ctx, proxy)...) // Add OIDC client secret environment variable if using inline config with secretRef env = append(env, r.buildOIDCClientSecretEnvVars(ctx, proxy)...) @@ -265,6 +234,40 @@ func (r *MCPRemoteProxyReconciler) buildOIDCClientSecretEnvVars( return []corev1.EnvVar{*oidcClientSecretEnvVar} } +// buildExternalAuthEnvVars builds environment variables for external auth (token exchange and bearer token). +// Note: Embedded auth server env vars are added separately in deploymentForMCPRemoteProxy +// to avoid duplicate API calls. +func (r *MCPRemoteProxyReconciler) buildExternalAuthEnvVars( + ctx context.Context, proxy *mcpv1alpha1.MCPRemoteProxy, +) []corev1.EnvVar { + if proxy.Spec.ExternalAuthConfigRef == nil { + return nil + } + + ctxLogger := log.FromContext(ctx) + var env []corev1.EnvVar + + tokenExchangeEnvVars, err := ctrlutil.GenerateTokenExchangeEnvVars( + ctx, r.Client, proxy.Namespace, proxy.Spec.ExternalAuthConfigRef, ctrlutil.GetExternalAuthConfigByName, + ) + if err != nil { + ctxLogger.Error(err, "Failed to generate token exchange environment variables") + } else { + env = append(env, tokenExchangeEnvVars...) + } + + bearerTokenEnvVars, err := ctrlutil.GenerateBearerTokenEnvVar( + ctx, r.Client, proxy.Namespace, proxy.Spec.ExternalAuthConfigRef, ctrlutil.GetExternalAuthConfigByName, + ) + if err != nil { + ctxLogger.Error(err, "Failed to generate bearer token environment variables") + } else { + env = append(env, bearerTokenEnvVars...) + } + + return env +} + // buildHeaderForwardSecretEnvVars builds environment variables for header forward secrets. // Each secret is mounted as an env var using Kubernetes SecretKeyRef, with a name following // the TOOLHIVE_SECRET_ pattern expected by the secrets.EnvironmentProvider. diff --git a/cmd/thv-operator/controllers/mcpremoteproxy_reconciler_test.go b/cmd/thv-operator/controllers/mcpremoteproxy_reconciler_test.go index 2244704240..fce83db436 100644 --- a/cmd/thv-operator/controllers/mcpremoteproxy_reconciler_test.go +++ b/cmd/thv-operator/controllers/mcpremoteproxy_reconciler_test.go @@ -50,6 +50,50 @@ func TestMCPRemoteProxyFullReconciliation(t *testing.T) { secret *corev1.Secret validateResult func(*testing.T, *mcpv1alpha1.MCPRemoteProxy, client.Client) }{ + { + name: "anonymous proxy without OIDC config", + proxy: &mcpv1alpha1.MCPRemoteProxy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "anon-proxy", + Namespace: "default", + }, + Spec: mcpv1alpha1.MCPRemoteProxySpec{ + RemoteURL: "https://docs.example.com/mcp", + ProxyPort: 8080, + Transport: "streamable-http", + }, + }, + validateResult: func(t *testing.T, proxy *mcpv1alpha1.MCPRemoteProxy, c client.Client) { + t.Helper() + + // Verify RunConfig ConfigMap created with no OIDC config + cm := &corev1.ConfigMap{} + err := c.Get(context.TODO(), types.NamespacedName{ + Name: fmt.Sprintf("%s-runconfig", proxy.Name), + Namespace: proxy.Namespace, + }, cm) + assert.NoError(t, err, "RunConfig ConfigMap should be created") + assert.Contains(t, cm.Data, "runconfig.json") + // OIDC config should not be present in the RunConfig + assert.NotContains(t, cm.Data["runconfig.json"], "oidc_issuer") + + // Verify Deployment created + dep := &appsv1.Deployment{} + err = c.Get(context.TODO(), types.NamespacedName{ + Name: proxy.Name, + Namespace: proxy.Namespace, + }, dep) + assert.NoError(t, err, "Deployment should be created") + + // Verify Service created + svc := &corev1.Service{} + err = c.Get(context.TODO(), types.NamespacedName{ + Name: createProxyServiceName(proxy.Name), + Namespace: proxy.Namespace, + }, svc) + assert.NoError(t, err, "Service should be created") + }, + }, { name: "basic proxy with inline OIDC", proxy: &mcpv1alpha1.MCPRemoteProxy{ diff --git a/cmd/thv-operator/controllers/mcpremoteproxy_runconfig.go b/cmd/thv-operator/controllers/mcpremoteproxy_runconfig.go index 34cd5f2fb5..ae9a64cb1f 100644 --- a/cmd/thv-operator/controllers/mcpremoteproxy_runconfig.go +++ b/cmd/thv-operator/controllers/mcpremoteproxy_runconfig.go @@ -124,15 +124,15 @@ func (r *MCPRemoteProxyReconciler) createRunConfigFromMCPRemoteProxy( return nil, fmt.Errorf("failed to process AuthzConfig: %w", err) } - // Add OIDC configuration (required for proxy mode) - // Supports both legacy inline OIDCConfig and new MCPOIDCConfigRef paths + // Add OIDC configuration + // Supports both legacy inline OIDCConfig and new MCPOIDCConfigRef paths. + // Returns nil when neither is set (anonymous access). resolvedOIDCConfig, err := r.resolveAndAddOIDCConfig(apiCtx, proxy, &options) if err != nil { return nil, err } - // Add external auth configuration if specified (updated call) - // Will fail if embedded auth server is used without OIDC config or resourceUrl + // Add external auth configuration if specified if err := ctrlutil.AddExternalAuthConfigOptions( apiCtx, r.Client, proxy.Namespace, proxy.Name, proxy.Spec.ExternalAuthConfigRef, resolvedOIDCConfig, &options, @@ -179,11 +179,17 @@ func (r *MCPRemoteProxyReconciler) createRunConfigFromMCPRemoteProxy( // resolveAndAddOIDCConfig resolves OIDC configuration from either the shared MCPOIDCConfigRef // or the legacy inline OIDCConfig, adds the appropriate runner options, and returns the resolved config. +// Returns nil when neither OIDCConfig nor OIDCConfigRef is set (anonymous access). func (r *MCPRemoteProxyReconciler) resolveAndAddOIDCConfig( ctx context.Context, proxy *mcpv1alpha1.MCPRemoteProxy, options *[]runner.RunConfigBuilderOption, ) (*oidc.OIDCConfig, error) { + // No OIDC config = anonymous access (no auth middleware) + if proxy.Spec.OIDCConfig == nil && proxy.Spec.OIDCConfigRef == nil { + return nil, nil + } + resolver := oidc.NewResolver(r.Client) if proxy.Spec.OIDCConfigRef != nil { diff --git a/cmd/thv-operator/pkg/controllerutil/authserver.go b/cmd/thv-operator/pkg/controllerutil/authserver.go index e3057082b4..31425a8186 100644 --- a/cmd/thv-operator/pkg/controllerutil/authserver.go +++ b/cmd/thv-operator/pkg/controllerutil/authserver.go @@ -484,6 +484,9 @@ func BuildAuthServerRunConfig( } config.Storage = storageCfg + // Wire through upstream token injection flag + config.DisableUpstreamTokenInjection = authConfig.DisableUpstreamTokenInjection + return config, nil } diff --git a/cmd/thv-operator/pkg/controllerutil/authserver_test.go b/cmd/thv-operator/pkg/controllerutil/authserver_test.go index 07d0b53616..b0bbe9ca49 100644 --- a/cmd/thv-operator/pkg/controllerutil/authserver_test.go +++ b/cmd/thv-operator/pkg/controllerutil/authserver_test.go @@ -967,6 +967,45 @@ func TestBuildAuthServerRunConfig(t *testing.T) { assert.Equal(t, UpstreamClientSecretEnvVar+"_GITHUB", github.OAuth2Config.ClientSecretEnvVar) }, }, + { + name: "DisableUpstreamTokenInjection is wired through", + authConfig: &mcpv1alpha1.EmbeddedAuthServerConfig{ + Issuer: "https://auth.example.com", + SigningKeySecretRefs: []mcpv1alpha1.SecretKeyRef{ + {Name: "signing-key", Key: "private.pem"}, + }, + HMACSecretRefs: []mcpv1alpha1.SecretKeyRef{ + {Name: "hmac-secret", Key: "hmac"}, + }, + DisableUpstreamTokenInjection: true, + }, + allowedAudiences: defaultAudiences, + scopesSupported: defaultScopes, + checkFunc: func(t *testing.T, config *authserver.RunConfig) { + t.Helper() + assert.True(t, config.DisableUpstreamTokenInjection, + "DisableUpstreamTokenInjection should be wired from CRD to RunConfig") + }, + }, + { + name: "DisableUpstreamTokenInjection defaults to false", + authConfig: &mcpv1alpha1.EmbeddedAuthServerConfig{ + Issuer: "https://auth.example.com", + SigningKeySecretRefs: []mcpv1alpha1.SecretKeyRef{ + {Name: "signing-key", Key: "private.pem"}, + }, + HMACSecretRefs: []mcpv1alpha1.SecretKeyRef{ + {Name: "hmac-secret", Key: "hmac"}, + }, + }, + allowedAudiences: defaultAudiences, + scopesSupported: defaultScopes, + checkFunc: func(t *testing.T, config *authserver.RunConfig) { + t.Helper() + assert.False(t, config.DisableUpstreamTokenInjection, + "DisableUpstreamTokenInjection should default to false") + }, + }, } for _, tt := range tests { diff --git a/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_mcpexternalauthconfigs.yaml b/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_mcpexternalauthconfigs.yaml index 4f80856ae6..266ccfe8a9 100644 --- a/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_mcpexternalauthconfigs.yaml +++ b/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_mcpexternalauthconfigs.yaml @@ -201,6 +201,16 @@ spec: Must be a valid HTTPS URL (or HTTP for localhost) without query, fragment, or trailing slash. pattern: ^https?://[^\s?#]+[^/\s?#]$ type: string + disableUpstreamTokenInjection: + default: false + description: |- + DisableUpstreamTokenInjection prevents the embedded auth server from injecting + upstream IdP tokens into requests forwarded to the backend MCP server. + When true, the embedded auth server still handles OAuth flows for clients + but does not swap ToolHive JWTs for upstream tokens on outgoing requests. + This is useful when the backend MCP server does not require authentication + (e.g., public documentation servers) but you still want client authentication. + type: boolean hmacSecretRefs: description: |- HMACSecretRefs references Kubernetes Secrets containing symmetric secrets for signing diff --git a/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_virtualmcpservers.yaml b/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_virtualmcpservers.yaml index 213aa451e9..02f2e1de53 100644 --- a/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_virtualmcpservers.yaml +++ b/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_virtualmcpservers.yaml @@ -86,6 +86,16 @@ spec: Must be a valid HTTPS URL (or HTTP for localhost) without query, fragment, or trailing slash. pattern: ^https?://[^\s?#]+[^/\s?#]$ type: string + disableUpstreamTokenInjection: + default: false + description: |- + DisableUpstreamTokenInjection prevents the embedded auth server from injecting + upstream IdP tokens into requests forwarded to the backend MCP server. + When true, the embedded auth server still handles OAuth flows for clients + but does not swap ToolHive JWTs for upstream tokens on outgoing requests. + This is useful when the backend MCP server does not require authentication + (e.g., public documentation servers) but you still want client authentication. + type: boolean hmacSecretRefs: description: |- HMACSecretRefs references Kubernetes Secrets containing symmetric secrets for signing diff --git a/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_mcpexternalauthconfigs.yaml b/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_mcpexternalauthconfigs.yaml index 61a556bb59..d1f3b36177 100644 --- a/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_mcpexternalauthconfigs.yaml +++ b/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_mcpexternalauthconfigs.yaml @@ -204,6 +204,16 @@ spec: Must be a valid HTTPS URL (or HTTP for localhost) without query, fragment, or trailing slash. pattern: ^https?://[^\s?#]+[^/\s?#]$ type: string + disableUpstreamTokenInjection: + default: false + description: |- + DisableUpstreamTokenInjection prevents the embedded auth server from injecting + upstream IdP tokens into requests forwarded to the backend MCP server. + When true, the embedded auth server still handles OAuth flows for clients + but does not swap ToolHive JWTs for upstream tokens on outgoing requests. + This is useful when the backend MCP server does not require authentication + (e.g., public documentation servers) but you still want client authentication. + type: boolean hmacSecretRefs: description: |- HMACSecretRefs references Kubernetes Secrets containing symmetric secrets for signing diff --git a/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_virtualmcpservers.yaml b/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_virtualmcpservers.yaml index a3a775f477..ddbb15c8da 100644 --- a/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_virtualmcpservers.yaml +++ b/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_virtualmcpservers.yaml @@ -89,6 +89,16 @@ spec: Must be a valid HTTPS URL (or HTTP for localhost) without query, fragment, or trailing slash. pattern: ^https?://[^\s?#]+[^/\s?#]$ type: string + disableUpstreamTokenInjection: + default: false + description: |- + DisableUpstreamTokenInjection prevents the embedded auth server from injecting + upstream IdP tokens into requests forwarded to the backend MCP server. + When true, the embedded auth server still handles OAuth flows for clients + but does not swap ToolHive JWTs for upstream tokens on outgoing requests. + This is useful when the backend MCP server does not require authentication + (e.g., public documentation servers) but you still want client authentication. + type: boolean hmacSecretRefs: description: |- HMACSecretRefs references Kubernetes Secrets containing symmetric secrets for signing diff --git a/docs/operator/crd-api.md b/docs/operator/crd-api.md index dd79a4f147..f7cb584f70 100644 --- a/docs/operator/crd-api.md +++ b/docs/operator/crd-api.md @@ -989,6 +989,7 @@ _Appears in:_ | `tokenLifespans` _[api.v1alpha1.TokenLifespanConfig](#apiv1alpha1tokenlifespanconfig)_ | TokenLifespans configures the duration that various tokens are valid.
If not specified, defaults are applied (access: 1h, refresh: 7d, authCode: 10m). | | Optional: \{\}
| | `upstreamProviders` _[api.v1alpha1.UpstreamProviderConfig](#apiv1alpha1upstreamproviderconfig) array_ | UpstreamProviders configures connections to upstream Identity Providers.
The embedded auth server delegates authentication to these providers.
MCPServer and MCPRemoteProxy support a single upstream; VirtualMCPServer supports multiple. | | MinItems: 1
Required: \{\}
| | `storage` _[api.v1alpha1.AuthServerStorageConfig](#apiv1alpha1authserverstorageconfig)_ | Storage configures the storage backend for the embedded auth server.
If not specified, defaults to in-memory storage. | | Optional: \{\}
| +| `disableUpstreamTokenInjection` _boolean_ | DisableUpstreamTokenInjection prevents the embedded auth server from injecting
upstream IdP tokens into requests forwarded to the backend MCP server.
When true, the embedded auth server still handles OAuth flows for clients
but does not swap ToolHive JWTs for upstream tokens on outgoing requests.
This is useful when the backend MCP server does not require authentication
(e.g., public documentation servers) but you still want client authentication. | false | Optional: \{\}
| #### api.v1alpha1.EmbeddingResourceOverrides diff --git a/docs/server/docs.go b/docs/server/docs.go index df70eb0529..69d4a6a4a9 100644 --- a/docs/server/docs.go +++ b/docs/server/docs.go @@ -495,6 +495,10 @@ const docTemplate = `{ "description": "AuthorizationEndpointBaseURL overrides the base URL used for the authorization_endpoint\nin the OAuth discovery document. When set, the discovery document will advertise\n` + "`" + `{authorization_endpoint_base_url}/oauth/authorize` + "`" + ` instead of ` + "`" + `{issuer}/oauth/authorize` + "`" + `.\nAll other endpoints remain derived from the issuer.", "type": "string" }, + "disable_upstream_token_injection": { + "description": "DisableUpstreamTokenInjection prevents the upstream swap middleware from being added.\nWhen true, the embedded auth server handles OAuth flows for clients but does not\ninject upstream IdP tokens into requests forwarded to the backend MCP server.", + "type": "boolean" + }, "hmac_secret_files": { "description": "HMACSecretFiles contains file paths to HMAC secrets for signing authorization codes\nand refresh tokens (opaque tokens).\nFirst file is the current secret (must be at least 32 bytes), subsequent files\nare for rotation/verification of existing tokens.\nIf empty, an ephemeral secret will be auto-generated (development only).", "items": { diff --git a/docs/server/swagger.json b/docs/server/swagger.json index 4008970275..99173fe99b 100644 --- a/docs/server/swagger.json +++ b/docs/server/swagger.json @@ -488,6 +488,10 @@ "description": "AuthorizationEndpointBaseURL overrides the base URL used for the authorization_endpoint\nin the OAuth discovery document. When set, the discovery document will advertise\n`{authorization_endpoint_base_url}/oauth/authorize` instead of `{issuer}/oauth/authorize`.\nAll other endpoints remain derived from the issuer.", "type": "string" }, + "disable_upstream_token_injection": { + "description": "DisableUpstreamTokenInjection prevents the upstream swap middleware from being added.\nWhen true, the embedded auth server handles OAuth flows for clients but does not\ninject upstream IdP tokens into requests forwarded to the backend MCP server.", + "type": "boolean" + }, "hmac_secret_files": { "description": "HMACSecretFiles contains file paths to HMAC secrets for signing authorization codes\nand refresh tokens (opaque tokens).\nFirst file is the current secret (must be at least 32 bytes), subsequent files\nare for rotation/verification of existing tokens.\nIf empty, an ephemeral secret will be auto-generated (development only).", "items": { diff --git a/docs/server/swagger.yaml b/docs/server/swagger.yaml index ac859bc102..7229c2e055 100644 --- a/docs/server/swagger.yaml +++ b/docs/server/swagger.yaml @@ -487,6 +487,12 @@ components: `{authorization_endpoint_base_url}/oauth/authorize` instead of `{issuer}/oauth/authorize`. All other endpoints remain derived from the issuer. type: string + disable_upstream_token_injection: + description: |- + DisableUpstreamTokenInjection prevents the upstream swap middleware from being added. + When true, the embedded auth server handles OAuth flows for clients but does not + inject upstream IdP tokens into requests forwarded to the backend MCP server. + type: boolean hmac_secret_files: description: |- HMACSecretFiles contains file paths to HMAC secrets for signing authorization codes diff --git a/pkg/authserver/config.go b/pkg/authserver/config.go index a19a1cc45b..260764c791 100644 --- a/pkg/authserver/config.go +++ b/pkg/authserver/config.go @@ -79,6 +79,12 @@ type RunConfig struct { // Storage configures the storage backend for the auth server. // If nil, defaults to in-memory storage. Storage *storage.RunConfig `json:"storage,omitempty" yaml:"storage,omitempty"` + + // DisableUpstreamTokenInjection prevents the upstream swap middleware from being added. + // When true, the embedded auth server handles OAuth flows for clients but does not + // inject upstream IdP tokens into requests forwarded to the backend MCP server. + //nolint:lll // field tags require full JSON+YAML names + DisableUpstreamTokenInjection bool `json:"disable_upstream_token_injection,omitempty" yaml:"disable_upstream_token_injection,omitempty"` } // SigningKeyRunConfig configures where to load signing keys from. diff --git a/pkg/runner/middleware.go b/pkg/runner/middleware.go index be9dd33506..81c4a438c9 100644 --- a/pkg/runner/middleware.go +++ b/pkg/runner/middleware.go @@ -5,6 +5,7 @@ package runner import ( "fmt" + "net/http" "github.com/stacklok/toolhive/pkg/audit" "github.com/stacklok/toolhive/pkg/auth" @@ -45,6 +46,7 @@ func GetSupportedMiddlewareFactories() map[string]types.MiddlewareFactory { headerfwd.HeaderForwardMiddlewareName: headerfwd.CreateMiddleware, validating.MiddlewareType: validating.CreateMiddleware, mutating.MiddlewareType: mutating.CreateMiddleware, + stripAuthMiddlewareType: createStripAuthMiddleware, } } @@ -330,8 +332,9 @@ func addUsageMetricsMiddleware(middlewares []types.MiddlewareConfig, configDisab // addUpstreamSwapMiddleware adds upstream swap middleware if the embedded auth server is configured. // This middleware exchanges ToolHive JWTs for upstream IdP tokens. -// The middleware is only added when EmbeddedAuthServerConfig is set; if UpstreamSwapConfig -// is nil, default configuration values are used. +// The middleware is only added when EmbeddedAuthServerConfig is set and +// DisableUpstreamTokenInjection is false. If UpstreamSwapConfig is nil, +// default configuration values are used. func addUpstreamSwapMiddleware( middlewares []types.MiddlewareConfig, config *RunConfig, @@ -341,6 +344,12 @@ func addUpstreamSwapMiddleware( return middlewares, nil } + // When upstream token injection is disabled, strip the Authorization header + // so the client's ToolHive JWT doesn't leak to the upstream server. + if config.EmbeddedAuthServerConfig.DisableUpstreamTokenInjection { + return addAuthHeaderStripMiddleware(middlewares) + } + // Use provided config or defaults upstreamSwapConfig := config.UpstreamSwapConfig if upstreamSwapConfig == nil { @@ -396,6 +405,45 @@ func injectUpstreamProviderIfNeeded( return cedar.InjectUpstreamProvider(authzCfg, providerName) } +// stripAuthMiddlewareType is the type identifier for the auth header stripping middleware. +const stripAuthMiddlewareType = "strip-auth" + +// addAuthHeaderStripMiddleware adds a middleware that removes the Authorization header +// before forwarding to the upstream. This prevents the client's ToolHive JWT from +// leaking to upstream servers that don't expect it. +func addAuthHeaderStripMiddleware( + middlewares []types.MiddlewareConfig, +) ([]types.MiddlewareConfig, error) { + mwConfig, err := types.NewMiddlewareConfig(stripAuthMiddlewareType, struct{}{}) + if err != nil { + return nil, fmt.Errorf("failed to create strip-auth middleware config: %w", err) + } + return append(middlewares, *mwConfig), nil +} + +// createStripAuthMiddleware is the factory function for the auth header stripping middleware. +func createStripAuthMiddleware(_ *types.MiddlewareConfig, runner types.MiddlewareRunner) error { + mw := &stripAuthMiddleware{} + runner.AddMiddleware(stripAuthMiddlewareType, mw) + return nil +} + +// stripAuthMiddleware removes the Authorization header from requests. +type stripAuthMiddleware struct{} + +// Handler returns the middleware function. +func (*stripAuthMiddleware) Handler() types.MiddlewareFunction { + return func(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + r.Header.Del("Authorization") + next.ServeHTTP(w, r) + }) + } +} + +// Close cleans up resources. +func (*stripAuthMiddleware) Close() error { return nil } + // addAWSStsMiddleware adds AWS STS middleware if configured. // Returns an error if AWSStsConfig is set but RemoteURL is empty, because // SigV4 signing is only meaningful for remote MCP servers. diff --git a/pkg/runner/middleware_test.go b/pkg/runner/middleware_test.go index cf604da16c..8892e6140e 100644 --- a/pkg/runner/middleware_test.go +++ b/pkg/runner/middleware_test.go @@ -271,6 +271,7 @@ func TestAddUpstreamSwapMiddleware(t *testing.T) { name string config *RunConfig wantAppended bool + wantType string // expected middleware type when appended }{ { name: "nil EmbeddedAuthServerConfig returns input unchanged", @@ -284,6 +285,17 @@ func TestAddUpstreamSwapMiddleware(t *testing.T) { UpstreamSwapConfig: nil, }, wantAppended: true, + wantType: upstreamswap.MiddlewareType, + }, + { + name: "DisableUpstreamTokenInjection adds strip-auth middleware instead", + config: func() *RunConfig { + cfg := createMinimalAuthServerConfig() + cfg.DisableUpstreamTokenInjection = true + return &RunConfig{EmbeddedAuthServerConfig: cfg} + }(), + wantAppended: true, + wantType: stripAuthMiddlewareType, }, { name: "EmbeddedAuthServerConfig set with explicit UpstreamSwapConfig uses provided config", @@ -294,6 +306,7 @@ func TestAddUpstreamSwapMiddleware(t *testing.T) { }, }, wantAppended: true, + wantType: upstreamswap.MiddlewareType, }, { name: "EmbeddedAuthServerConfig with custom header strategy config", @@ -305,6 +318,7 @@ func TestAddUpstreamSwapMiddleware(t *testing.T) { }, }, wantAppended: true, + wantType: upstreamswap.MiddlewareType, }, } @@ -324,20 +338,20 @@ func TestAddUpstreamSwapMiddleware(t *testing.T) { // Should have one additional entry. require.Len(t, got, len(initial)+1) added := got[len(got)-1] - assert.Equal(t, upstreamswap.MiddlewareType, added.Type) - - // Verify serialized params contain the expected config. - var params upstreamswap.MiddlewareParams - require.NoError(t, json.Unmarshal(added.Parameters, ¶ms)) + assert.Equal(t, tt.wantType, added.Type) - if tt.config.UpstreamSwapConfig != nil { - // Should use the provided config - require.NotNil(t, params.Config) - assert.Equal(t, tt.config.UpstreamSwapConfig.HeaderStrategy, params.Config.HeaderStrategy) - assert.Equal(t, tt.config.UpstreamSwapConfig.CustomHeaderName, params.Config.CustomHeaderName) - } else { - // Should use defaults (empty config is valid) - require.NotNil(t, params.Config) + // For upstreamswap type, verify serialized params + if tt.wantType == upstreamswap.MiddlewareType { + var params upstreamswap.MiddlewareParams + require.NoError(t, json.Unmarshal(added.Parameters, ¶ms)) + + if tt.config.UpstreamSwapConfig != nil { + require.NotNil(t, params.Config) + assert.Equal(t, tt.config.UpstreamSwapConfig.HeaderStrategy, params.Config.HeaderStrategy) + assert.Equal(t, tt.config.UpstreamSwapConfig.CustomHeaderName, params.Config.CustomHeaderName) + } else { + require.NotNil(t, params.Config) + } } }) } @@ -350,6 +364,7 @@ func TestPopulateMiddlewareConfigs_UpstreamSwap(t *testing.T) { name string config *RunConfig wantUpstreamSwap bool + wantStripAuth bool wantHeaderStrategy string }{ { @@ -362,6 +377,16 @@ func TestPopulateMiddlewareConfigs_UpstreamSwap(t *testing.T) { config: &RunConfig{EmbeddedAuthServerConfig: nil}, wantUpstreamSwap: false, }, + { + name: "DisableUpstreamTokenInjection adds strip-auth instead of upstream-swap", + config: func() *RunConfig { + cfg := createMinimalAuthServerConfig() + cfg.DisableUpstreamTokenInjection = true + return &RunConfig{EmbeddedAuthServerConfig: cfg} + }(), + wantUpstreamSwap: false, + wantStripAuth: true, + }, { name: "explicit UpstreamSwapConfig is used", config: &RunConfig{ @@ -382,20 +407,25 @@ func TestPopulateMiddlewareConfigs_UpstreamSwap(t *testing.T) { err := PopulateMiddlewareConfigs(tt.config) require.NoError(t, err) - var found bool + var foundSwap bool + var foundStrip bool var foundConfig *types.MiddlewareConfig for i, mw := range tt.config.MiddlewareConfigs { if mw.Type == upstreamswap.MiddlewareType { - found = true + foundSwap = true foundConfig = &tt.config.MiddlewareConfigs[i] - break + } + if mw.Type == stripAuthMiddlewareType { + foundStrip = true } } - assert.Equal(t, tt.wantUpstreamSwap, found, + assert.Equal(t, tt.wantUpstreamSwap, foundSwap, "upstream-swap middleware presence mismatch") + assert.Equal(t, tt.wantStripAuth, foundStrip, + "strip-auth middleware presence mismatch") // Verify config values if we expect the middleware and have specific expectations - if found && tt.wantHeaderStrategy != "" { + if foundSwap && tt.wantHeaderStrategy != "" { var params upstreamswap.MiddlewareParams require.NoError(t, json.Unmarshal(foundConfig.Parameters, ¶ms)) require.NotNil(t, params.Config) diff --git a/pkg/transport/proxy/transparent/transparent_proxy.go b/pkg/transport/proxy/transparent/transparent_proxy.go index f5d9599b9a..c1b07010fe 100644 --- a/pkg/transport/proxy/transparent/transparent_proxy.go +++ b/pkg/transport/proxy/transparent/transparent_proxy.go @@ -522,6 +522,14 @@ func (t *tracingTransport) RoundTrip(req *http.Request) (*http.Response, error) req.Host = req.URL.Host } + slog.Debug("outbound request to upstream", + "method", req.Method, + "url", req.URL.String(), + "host", req.Host, + "accept", req.Header.Get("Accept"), + "content_type", req.Header.Get("Content-Type"), + ) + reqBody := readRequestBody(req) // thv proxy does not provide the transport type, so we need to detect it from the request @@ -611,6 +619,13 @@ func (t *tracingTransport) RoundTrip(req *http.Request) (*http.Response, error) return nil, err } + slog.Debug("upstream response received", + "status", resp.StatusCode, + "url", req.URL.String(), + "content_type", resp.Header.Get("Content-Type"), + "mcp_session_id", resp.Header.Get("Mcp-Session-Id"), + ) + // Check for 401 Unauthorized response (bearer token authentication failure) if resp.StatusCode == http.StatusUnauthorized { //nolint:gosec // G706: logging target URI from config @@ -1006,7 +1021,15 @@ func (p *TransparentProxy) Start(ctx context.Context) error { FlushInterval: -1, Rewrite: func(pr *httputil.ProxyRequest) { pr.SetURL(targetURL) - pr.SetXForwarded() + + // Only set X-Forwarded-* headers for local backends. + // For remote upstreams, these headers leak the proxy's hostname + // (X-Forwarded-Host) to third-party servers, which can cause + // 307 redirect loops when the upstream uses that header to + // construct redirect URLs pointing back to the proxy. + if !p.isRemote { + pr.SetXForwarded() + } // Route to the originating backend pod when session metadata contains backend_url. // Falls back to static targetURL when the session doesn't exist or has no backend_url.