diff --git a/cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go b/cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go index 2170dce581..77a4aadc51 100644 --- a/cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go +++ b/cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go @@ -279,6 +279,21 @@ 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 instead of swapping ToolHive JWTs for upstream tokens the proxy STRIPS + // the client's credential headers (Authorization, Cookie, Proxy-Authorization) + // after validating the JWT — the backend receives an unauthenticated request. + // Use headerForward to attach static credentials (e.g. an API key) if the + // backend needs them. Cannot be combined with token exchange or AWS STS, + // which would re-add credentials after the strip. + // 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"` + // BaselineClientScopes is a baseline set of OAuth 2.0 scopes guaranteed to be // included in every client registration. The embedded auth server unions these // scopes into the registered set returned by RFC 7591 Dynamic Client diff --git a/cmd/thv-operator/pkg/controllerutil/authserver.go b/cmd/thv-operator/pkg/controllerutil/authserver.go index 84d4ed979d..4d13355308 100644 --- a/cmd/thv-operator/pkg/controllerutil/authserver.go +++ b/cmd/thv-operator/pkg/controllerutil/authserver.go @@ -582,6 +582,9 @@ func BuildAuthServerRunConfig( } config.Storage = storageCfg + // Wire through upstream token injection flag + config.DisableUpstreamTokenInjection = authConfig.DisableUpstreamTokenInjection + // Build CIMD configuration. CacheFallbackTTL is passed as-is (string); // resolveCIMDConfig in the runner parses it to time.Duration at startup. if authConfig.CIMD != nil && authConfig.CIMD.Enabled { diff --git a/cmd/thv-operator/pkg/controllerutil/authserver_test.go b/cmd/thv-operator/pkg/controllerutil/authserver_test.go index bec4c98179..5f7434b921 100644 --- a/cmd/thv-operator/pkg/controllerutil/authserver_test.go +++ b/cmd/thv-operator/pkg/controllerutil/authserver_test.go @@ -1538,6 +1538,45 @@ func TestBuildAuthServerRunConfig(t *testing.T) { "IdentityFromToken must be nil when not configured") }, }, + { + name: "DisableUpstreamTokenInjection is wired through", + authConfig: &mcpv1beta1.EmbeddedAuthServerConfig{ + Issuer: "https://auth.example.com", + SigningKeySecretRefs: []mcpv1beta1.SecretKeyRef{ + {Name: "signing-key", Key: "private.pem"}, + }, + HMACSecretRefs: []mcpv1beta1.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: &mcpv1beta1.EmbeddedAuthServerConfig{ + Issuer: "https://auth.example.com", + SigningKeySecretRefs: []mcpv1beta1.SecretKeyRef{ + {Name: "signing-key", Key: "private.pem"}, + }, + HMACSecretRefs: []mcpv1beta1.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 0239662a04..1c005d6a00 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 @@ -269,6 +269,21 @@ spec: required: - enabled type: object + 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 instead of swapping ToolHive JWTs for upstream tokens the proxy STRIPS + the client's credential headers (Authorization, Cookie, Proxy-Authorization) + after validating the JWT — the backend receives an unauthenticated request. + Use headerForward to attach static credentials (e.g. an API key) if the + backend needs them. Cannot be combined with token exchange or AWS STS, + which would re-add credentials after the strip. + 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 @@ -1578,6 +1593,21 @@ spec: required: - enabled type: object + 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 instead of swapping ToolHive JWTs for upstream tokens the proxy STRIPS + the client's credential headers (Authorization, Cookie, Proxy-Authorization) + after validating the JWT — the backend receives an unauthenticated request. + Use headerForward to attach static credentials (e.g. an API key) if the + backend needs them. Cannot be combined with token exchange or AWS STS, + which would re-add credentials after the strip. + 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 d8b82f4951..178d3b60c6 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 @@ -142,6 +142,21 @@ spec: required: - enabled type: object + 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 instead of swapping ToolHive JWTs for upstream tokens the proxy STRIPS + the client's credential headers (Authorization, Cookie, Proxy-Authorization) + after validating the JWT — the backend receives an unauthenticated request. + Use headerForward to attach static credentials (e.g. an API key) if the + backend needs them. Cannot be combined with token exchange or AWS STS, + which would re-add credentials after the strip. + 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 @@ -3122,6 +3137,21 @@ spec: required: - enabled type: object + 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 instead of swapping ToolHive JWTs for upstream tokens the proxy STRIPS + the client's credential headers (Authorization, Cookie, Proxy-Authorization) + after validating the JWT — the backend receives an unauthenticated request. + Use headerForward to attach static credentials (e.g. an API key) if the + backend needs them. Cannot be combined with token exchange or AWS STS, + which would re-add credentials after the strip. + 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 b032071c98..db58942062 100644 --- a/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_mcpexternalauthconfigs.yaml +++ b/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_mcpexternalauthconfigs.yaml @@ -272,6 +272,21 @@ spec: required: - enabled type: object + 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 instead of swapping ToolHive JWTs for upstream tokens the proxy STRIPS + the client's credential headers (Authorization, Cookie, Proxy-Authorization) + after validating the JWT — the backend receives an unauthenticated request. + Use headerForward to attach static credentials (e.g. an API key) if the + backend needs them. Cannot be combined with token exchange or AWS STS, + which would re-add credentials after the strip. + 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 @@ -1581,6 +1596,21 @@ spec: required: - enabled type: object + 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 instead of swapping ToolHive JWTs for upstream tokens the proxy STRIPS + the client's credential headers (Authorization, Cookie, Proxy-Authorization) + after validating the JWT — the backend receives an unauthenticated request. + Use headerForward to attach static credentials (e.g. an API key) if the + backend needs them. Cannot be combined with token exchange or AWS STS, + which would re-add credentials after the strip. + 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 6755543c62..59ab53de26 100644 --- a/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_virtualmcpservers.yaml +++ b/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_virtualmcpservers.yaml @@ -145,6 +145,21 @@ spec: required: - enabled type: object + 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 instead of swapping ToolHive JWTs for upstream tokens the proxy STRIPS + the client's credential headers (Authorization, Cookie, Proxy-Authorization) + after validating the JWT — the backend receives an unauthenticated request. + Use headerForward to attach static credentials (e.g. an API key) if the + backend needs them. Cannot be combined with token exchange or AWS STS, + which would re-add credentials after the strip. + 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 @@ -3125,6 +3140,21 @@ spec: required: - enabled type: object + 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 instead of swapping ToolHive JWTs for upstream tokens the proxy STRIPS + the client's credential headers (Authorization, Cookie, Proxy-Authorization) + after validating the JWT — the backend receives an unauthenticated request. + Use headerForward to attach static credentials (e.g. an API key) if the + backend needs them. Cannot be combined with token exchange or AWS STS, + which would re-add credentials after the strip. + 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 1795b0fe19..70ad8496bf 100644 --- a/docs/operator/crd-api.md +++ b/docs/operator/crd-api.md @@ -1236,6 +1236,7 @@ _Appears in:_ | `upstreamProviders` _[api.v1beta1.UpstreamProviderConfig](#apiv1beta1upstreamproviderconfig) 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: \{\}
| | `primaryUpstreamProvider` _string_ | PrimaryUpstreamProvider names the upstream IDP whose access token Cedar
should read claims from when authorising a request. Must match the name
of one of the entries in UpstreamProviders. When empty, the controller
auto-selects the first entry of UpstreamProviders.
Only meaningful on VirtualMCPServer, where multiple upstream providers
can be configured and Cedar needs to pick which token's claims to
evaluate. The VirtualMCPServer controller validates this field against
UpstreamProviders at admission and rejects unresolvable values.
On MCPServer and MCPRemoteProxy this field is structurally present (the
EmbeddedAuthServerConfig struct is shared) but has no runtime effect:
those CRDs are restricted to a single upstream so there is no choice to
make. Setting it on those CRDs is silently ignored. | | MaxLength: 63
MinLength: 1
Pattern: `^[a-z0-9]([a-z0-9-]*[a-z0-9])?$`
Optional: \{\}
| | `storage` _[api.v1beta1.AuthServerStorageConfig](#apiv1beta1authserverstorageconfig)_ | 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 instead of swapping ToolHive JWTs for upstream tokens the proxy STRIPS
the client's credential headers (Authorization, Cookie, Proxy-Authorization)
after validating the JWT — the backend receives an unauthenticated request.
Use headerForward to attach static credentials (e.g. an API key) if the
backend needs them. Cannot be combined with token exchange or AWS STS,
which would re-add credentials after the strip.
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: \{\}
| | `baselineClientScopes` _string array_ | BaselineClientScopes is a baseline set of OAuth 2.0 scopes guaranteed to be
included in every client registration. The embedded auth server unions these
scopes into the registered set returned by RFC 7591 Dynamic Client
Registration, so a client that narrows the `scope` field at /oauth/register
can still request the baseline scopes at /oauth/authorize. All values must
be present in the upstream-derived scopesSupported set; the auth server
fails to start if any value is missing.
Security: every client registered via /oauth/register will gain the
ability to request these scopes at /oauth/authorize, regardless of what
the client itself requested. Keep the baseline narrow (typically
"openid" and "offline_access"). Adding a privileged scope here — e.g.
"admin:read" — would grant it to every DCR-registered client, including
public clients like Claude Code, Cursor, and VS Code.
When cimd.enabled is true, every dynamically resolved CIMD client will
also gain the ability to request these scopes, including third-party
clients resolved from arbitrary HTTPS URLs. | | MaxItems: 10
items:MinLength: 1
items:Pattern: `^[\x21\x23-\x5B\x5D-\x7E]+$`
Optional: \{\}
| | `cimd` _[api.v1beta1.EmbeddedAuthServerCIMDConfig](#apiv1beta1embeddedauthservercimdconfig)_ | CIMD configures Client ID Metadata Document support. When omitted, CIMD is disabled. | | Optional: \{\}
| diff --git a/docs/server/docs.go b/docs/server/docs.go index ace8208c63..a7dcdadfdb 100644 --- a/docs/server/docs.go +++ b/docs/server/docs.go @@ -539,6 +539,10 @@ const docTemplate = `{ "cimd": { "$ref": "#/components/schemas/github_com_stacklok_toolhive_pkg_authserver.CIMDRunConfig" }, + "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 instead of\ninjecting upstream IdP tokens the proxy strips the client's credential headers\n(Authorization, Cookie, Proxy-Authorization) after the JWT is validated — the\nbackend receives an unauthenticated request. Incompatible with token exchange\nand AWS STS, which would re-add credentials after the strip.", + "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 9b4116b313..224bd6faa3 100644 --- a/docs/server/swagger.json +++ b/docs/server/swagger.json @@ -532,6 +532,10 @@ "cimd": { "$ref": "#/components/schemas/github_com_stacklok_toolhive_pkg_authserver.CIMDRunConfig" }, + "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 instead of\ninjecting upstream IdP tokens the proxy strips the client's credential headers\n(Authorization, Cookie, Proxy-Authorization) after the JWT is validated — the\nbackend receives an unauthenticated request. Incompatible with token exchange\nand AWS STS, which would re-add credentials after the strip.", + "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 771abb7baf..f95340c432 100644 --- a/docs/server/swagger.yaml +++ b/docs/server/swagger.yaml @@ -598,6 +598,15 @@ components: uniqueItems: false cimd: $ref: '#/components/schemas/github_com_stacklok_toolhive_pkg_authserver.CIMDRunConfig' + 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 instead of + injecting upstream IdP tokens the proxy strips the client's credential headers + (Authorization, Cookie, Proxy-Authorization) after the JWT is validated — the + backend receives an unauthenticated request. Incompatible with token exchange + and AWS STS, which would re-add credentials after the strip. + 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 b5b3bc8634..a59ee2b931 100644 --- a/pkg/authserver/config.go +++ b/pkg/authserver/config.go @@ -91,6 +91,15 @@ type RunConfig struct { // 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 instead of + // injecting upstream IdP tokens the proxy strips the client's credential headers + // (Authorization, Cookie, Proxy-Authorization) after the JWT is validated — the + // backend receives an unauthenticated request. Incompatible with token exchange + // and AWS STS, which would re-add credentials after the strip. + //nolint:lll // field tags require full JSON+YAML names + DisableUpstreamTokenInjection bool `json:"disable_upstream_token_injection,omitempty" yaml:"disable_upstream_token_injection,omitempty"` + // CIMD controls client_id metadata document support. When enabled, the // embedded authorization server accepts HTTPS URLs as client_id values // and resolves them via the CIMD protocol instead of requiring DCR. diff --git a/pkg/runner/middleware.go b/pkg/runner/middleware.go index fdb415dc07..223d6524d3 100644 --- a/pkg/runner/middleware.go +++ b/pkg/runner/middleware.go @@ -47,6 +47,7 @@ func GetSupportedMiddlewareFactories() map[string]types.MiddlewareFactory { audit.MiddlewareType: audit.CreateMiddleware, recovery.MiddlewareType: recovery.CreateMiddleware, headerfwd.HeaderForwardMiddlewareName: headerfwd.CreateMiddleware, + headerfwd.StripAuthMiddlewareName: headerfwd.CreateStripAuthMiddleware, validating.MiddlewareType: validating.CreateMiddleware, mutating.MiddlewareType: mutating.CreateMiddleware, } @@ -364,8 +365,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, @@ -375,6 +377,29 @@ func addUpstreamSwapMiddleware( return middlewares, nil } + // When upstream token injection is disabled, strip the client's credential + // headers (Authorization, Cookie, Proxy-Authorization) so they never reach + // the upstream server. Two ordering invariants apply, pinned by + // TestPopulateMiddlewareConfigs_StripAuthOrdering: + // - strip-auth is appended after the auth middleware, so the client JWT + // is fully validated (and the identity stored in the request context + // for authz/audit) before the header is removed; + // - token-injecting middlewares (token exchange, AWS STS) run closer to + // the backend and would re-add an Authorization header after the + // strip, silently defeating the flag — that contradiction is rejected + // here instead. + if config.EmbeddedAuthServerConfig.DisableUpstreamTokenInjection { + if config.TokenExchangeConfig != nil { + return nil, fmt.Errorf("disableUpstreamTokenInjection cannot be combined with token exchange: " + + "token exchange would re-add an Authorization header after strip-auth removes it") + } + if config.AWSStsConfig != nil { + return nil, fmt.Errorf("disableUpstreamTokenInjection cannot be combined with AWS STS: " + + "SigV4 signing would re-add credentials after strip-auth removes them") + } + return addAuthHeaderStripMiddleware(middlewares) + } + // Use provided config or defaults upstreamSwapConfig := config.UpstreamSwapConfig if upstreamSwapConfig == nil { @@ -430,6 +455,21 @@ func injectUpstreamProviderIfNeeded( return cedar.InjectUpstreamProvider(authzCfg, providerName) } +// addAuthHeaderStripMiddleware adds the strip-auth middleware +// (pkg/transport/middleware), which removes the client's credential headers +// before forwarding to the upstream. This prevents the client's ToolHive JWT, +// cookies, and proxy credentials from leaking to upstream servers that don't +// expect them. +func addAuthHeaderStripMiddleware( + middlewares []types.MiddlewareConfig, +) ([]types.MiddlewareConfig, error) { + mwConfig, err := types.NewMiddlewareConfig(headerfwd.StripAuthMiddlewareName, struct{}{}) + if err != nil { + return nil, fmt.Errorf("failed to create strip-auth middleware config: %w", err) + } + return append(middlewares, *mwConfig), 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 fbc4c0d100..30f2deaced 100644 --- a/pkg/runner/middleware_test.go +++ b/pkg/runner/middleware_test.go @@ -26,6 +26,7 @@ import ( "github.com/stacklok/toolhive/pkg/authz/authorizers/cedar" "github.com/stacklok/toolhive/pkg/bodylimit" "github.com/stacklok/toolhive/pkg/mcp" + "github.com/stacklok/toolhive/pkg/oauthproto/tokenexchange" "github.com/stacklok/toolhive/pkg/ratelimit" "github.com/stacklok/toolhive/pkg/recovery" "github.com/stacklok/toolhive/pkg/telemetry" @@ -309,6 +310,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", @@ -322,6 +324,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: headerfwd.StripAuthMiddlewareName, }, { name: "EmbeddedAuthServerConfig set with explicit UpstreamSwapConfig uses provided config", @@ -332,6 +345,7 @@ func TestAddUpstreamSwapMiddleware(t *testing.T) { }, }, wantAppended: true, + wantType: upstreamswap.MiddlewareType, }, { name: "EmbeddedAuthServerConfig with custom header strategy config", @@ -343,6 +357,7 @@ func TestAddUpstreamSwapMiddleware(t *testing.T) { }, }, wantAppended: true, + wantType: upstreamswap.MiddlewareType, }, } @@ -362,20 +377,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) + assert.Equal(t, tt.wantType, added.Type) - // Verify serialized params contain the expected config. - var params upstreamswap.MiddlewareParams - require.NoError(t, json.Unmarshal(added.Parameters, ¶ms)) - - 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) + } } }) } @@ -388,6 +403,7 @@ func TestPopulateMiddlewareConfigs_UpstreamSwap(t *testing.T) { name string config *RunConfig wantUpstreamSwap bool + wantStripAuth bool wantHeaderStrategy string }{ { @@ -400,6 +416,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{ @@ -420,20 +446,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 == headerfwd.StripAuthMiddlewareName { + 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) @@ -1066,3 +1097,71 @@ func TestPopulateMiddlewareConfigs_FullCoverage(t *testing.T) { assert.True(t, typeIndex[authz.MiddlewareType]) assert.True(t, typeIndex[audit.MiddlewareType]) } + +// TestPopulateMiddlewareConfigs_StripAuthOrdering pins the ordering invariant +// for strip-auth: the auth middleware must precede it in the chain so the +// client JWT is fully validated (and the identity stored in the request +// context for authz/audit) before the Authorization header is removed. +func TestPopulateMiddlewareConfigs_StripAuthOrdering(t *testing.T) { + t.Parallel() + + authServerCfg := createMinimalAuthServerConfig() + authServerCfg.DisableUpstreamTokenInjection = true + config := &RunConfig{EmbeddedAuthServerConfig: authServerCfg} + + require.NoError(t, PopulateMiddlewareConfigs(config)) + + authIdx, stripIdx := -1, -1 + for i, mw := range config.MiddlewareConfigs { + switch mw.Type { + case auth.MiddlewareType: + authIdx = i + case headerfwd.StripAuthMiddlewareName: + stripIdx = i + } + } + require.GreaterOrEqual(t, authIdx, 0, "auth middleware must be present") + require.GreaterOrEqual(t, stripIdx, 0, "strip-auth middleware must be present") + assert.Less(t, authIdx, stripIdx, + "auth must validate the client JWT before strip-auth removes the Authorization header") +} + +// TestPopulateMiddlewareConfigs_StripAuthConflicts verifies that +// DisableUpstreamTokenInjection is rejected when combined with middlewares +// that would re-add credentials after the strip (token exchange, AWS STS), +// instead of silently defeating the flag at runtime. +func TestPopulateMiddlewareConfigs_StripAuthConflicts(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + mutate func(*RunConfig) + wantErr string + }{ + { + name: "token exchange combination is rejected", + mutate: func(c *RunConfig) { c.TokenExchangeConfig = &tokenexchange.Config{} }, + wantErr: "token exchange", + }, + { + name: "AWS STS combination is rejected", + mutate: func(c *RunConfig) { c.AWSStsConfig = &awssts.Config{}; c.RemoteURL = "https://example.com" }, + wantErr: "AWS STS", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + authServerCfg := createMinimalAuthServerConfig() + authServerCfg.DisableUpstreamTokenInjection = true + config := &RunConfig{EmbeddedAuthServerConfig: authServerCfg} + tt.mutate(config) + + err := PopulateMiddlewareConfigs(config) + require.ErrorContains(t, err, "disableUpstreamTokenInjection cannot be combined") + require.ErrorContains(t, err, tt.wantErr) + }) + } +} diff --git a/pkg/transport/middleware/strip_auth.go b/pkg/transport/middleware/strip_auth.go new file mode 100644 index 0000000000..ee7ec866f6 --- /dev/null +++ b/pkg/transport/middleware/strip_auth.go @@ -0,0 +1,52 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package middleware + +import ( + "net/http" + + "github.com/stacklok/toolhive/pkg/transport/types" +) + +// StripAuthMiddlewareName is the type constant for the credential stripping middleware. +const StripAuthMiddlewareName = "strip-auth" + +// clientCredentialHeaders are the request headers removed by the strip-auth +// middleware before a request is forwarded to the backend. Authorization +// carries the ToolHive JWT; Cookie and Proxy-Authorization are the other +// headers a client can use to carry credentials. The set mirrors the headers +// net/http refuses to copy across cross-host redirects. +var clientCredentialHeaders = []string{"Authorization", "Cookie", "Proxy-Authorization"} + +// StripAuthMiddleware removes client credential headers from requests so they +// never reach the backend. It is used when clients are authenticated by the +// proxy but the backend itself is public (DisableUpstreamTokenInjection): by +// the time this middleware runs, the auth middleware has already validated +// the client JWT and stored the identity in the request context, so the +// backend receives an unauthenticated request. Credentials injected by +// middlewares that run closer to the backend (e.g. header-forward) are +// unaffected. +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) { + for _, h := range clientCredentialHeaders { + r.Header.Del(h) + } + next.ServeHTTP(w, r) + }) + } +} + +// Close cleans up resources. The middleware holds none. +func (*StripAuthMiddleware) Close() error { return nil } + +// CreateStripAuthMiddleware is the factory function for the strip-auth +// middleware. It takes no parameters. +func CreateStripAuthMiddleware(config *types.MiddlewareConfig, runner types.MiddlewareRunner) error { + runner.AddMiddleware(config.Type, &StripAuthMiddleware{}) + return nil +} diff --git a/pkg/transport/middleware/strip_auth_test.go b/pkg/transport/middleware/strip_auth_test.go new file mode 100644 index 0000000000..634a856cf7 --- /dev/null +++ b/pkg/transport/middleware/strip_auth_test.go @@ -0,0 +1,64 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package middleware + +import ( + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/mock/gomock" + + "github.com/stacklok/toolhive/pkg/transport/types" + typesmocks "github.com/stacklok/toolhive/pkg/transport/types/mocks" +) + +// TestStripAuthMiddleware covers the strip-auth middleware end to end: the +// factory registers it on the runner under the config type, the handler +// removes every client credential header (Authorization, Cookie, +// Proxy-Authorization) while passing unrelated headers through, and Close is +// a no-op. +func TestStripAuthMiddleware(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + var registered types.Middleware + mockRunner := typesmocks.NewMockMiddlewareRunner(ctrl) + mockRunner.EXPECT().AddMiddleware(StripAuthMiddlewareName, gomock.Any()).Do(func(_ string, mw types.Middleware) { + registered = mw + }) + + mwConfig, err := types.NewMiddlewareConfig(StripAuthMiddlewareName, struct{}{}) + require.NoError(t, err) + require.NoError(t, CreateStripAuthMiddleware(mwConfig, mockRunner)) + require.NotNil(t, registered) + + var captured *http.Request + next := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + captured = r + w.WriteHeader(http.StatusNoContent) + }) + + req := httptest.NewRequest(http.MethodPost, "/mcp", nil) + req.Header.Set("Authorization", "Bearer toolhive-jwt") + req.Header.Set("Cookie", "session=abc") + req.Header.Set("Proxy-Authorization", "Basic Zm9vOmJhcg==") + req.Header.Set("X-Custom", "kept") + rec := httptest.NewRecorder() + + registered.Handler()(next).ServeHTTP(rec, req) + + require.NotNil(t, captured) + for _, h := range clientCredentialHeaders { + assert.Empty(t, captured.Header.Get(h), "%s must be stripped before reaching the backend", h) + } + assert.Equal(t, "kept", captured.Header.Get("X-Custom"), "unrelated headers must pass through") + assert.Equal(t, http.StatusNoContent, rec.Code) + + assert.NoError(t, registered.Close()) +} diff --git a/pkg/transport/proxy/transparent/transparent_proxy.go b/pkg/transport/proxy/transparent/transparent_proxy.go index 7452a74246..1cad66e6a2 100644 --- a/pkg/transport/proxy/transparent/transparent_proxy.go +++ b/pkg/transport/proxy/transparent/transparent_proxy.go @@ -566,6 +566,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, err := readRequestBody(req) if err != nil { // Oversized request body (chunked / no Content-Length) tripped the @@ -649,6 +657,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 @@ -1045,6 +1060,20 @@ func (p *TransparentProxy) modifyResponse(resp *http.Response) error { return p.responseProcessor.ProcessResponse(resp) } +// setXForwardedHeaders populates the standard X-Forwarded-* headers on the +// outbound request. For remote upstreams X-Forwarded-Host is removed: a +// third-party server may use it to construct redirect URLs pointing back at +// the proxy, producing 307 redirect loops. X-Forwarded-For and +// X-Forwarded-Proto are kept so remote backends can still log the client IP +// and scheme. Client-supplied X-Forwarded-* values never pass through either +// way — httputil strips them from the outbound request before Rewrite runs. +func (p *TransparentProxy) setXForwardedHeaders(pr *httputil.ProxyRequest) { + pr.SetXForwarded() + if p.isRemote { + pr.Out.Header.Del("X-Forwarded-Host") + } +} + // Start starts the transparent proxy. // nolint:gocyclo // This function handles multiple startup scenarios and is complex by design func (p *TransparentProxy) Start(ctx context.Context) error { @@ -1067,7 +1096,7 @@ func (p *TransparentProxy) Start(ctx context.Context) error { FlushInterval: -1, Rewrite: func(pr *httputil.ProxyRequest) { pr.SetURL(targetURL) - pr.SetXForwarded() + p.setXForwardedHeaders(pr) // 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. diff --git a/pkg/transport/proxy/transparent/xforwarded_test.go b/pkg/transport/proxy/transparent/xforwarded_test.go new file mode 100644 index 0000000000..358b23192f --- /dev/null +++ b/pkg/transport/proxy/transparent/xforwarded_test.go @@ -0,0 +1,58 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package transparent + +import ( + "net/http" + "net/http/httptest" + "net/http/httputil" + "testing" + + "github.com/stretchr/testify/assert" +) + +// TestSetXForwardedHeaders verifies the remote/local split for X-Forwarded-* +// headers: a remote upstream must not receive X-Forwarded-Host (third-party +// servers can echo it into redirect URLs, creating 307 loops back to the +// proxy), while X-Forwarded-For and X-Forwarded-Proto are always set so +// backends can still see the client IP and scheme. Local backends receive +// the full set. +func TestSetXForwardedHeaders(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + isRemote bool + wantHost bool + }{ + {name: "local backend keeps X-Forwarded-Host", isRemote: false, wantHost: true}, + {name: "remote upstream omits X-Forwarded-Host", isRemote: true, wantHost: false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + p := NewTransparentProxy( + "127.0.0.1", 0, "", nil, nil, nil, false, + tt.isRemote, "streamable-http", nil, nil, "", false, + ) + + in := httptest.NewRequest(http.MethodPost, "http://proxy.example.com/mcp", nil) + pr := &httputil.ProxyRequest{In: in, Out: in.Clone(in.Context())} + + p.setXForwardedHeaders(pr) + + if tt.wantHost { + assert.Equal(t, "proxy.example.com", pr.Out.Header.Get("X-Forwarded-Host")) + } else { + assert.Empty(t, pr.Out.Header.Get("X-Forwarded-Host"), + "remote upstreams must not see the proxy hostname") + } + assert.NotEmpty(t, pr.Out.Header.Get("X-Forwarded-For"), + "client IP must be forwarded for both local and remote backends") + assert.Equal(t, "http", pr.Out.Header.Get("X-Forwarded-Proto")) + }) + } +}