Skip to content

Commit 010f7fc

Browse files
aron-muonclaude
andcommitted
Fix transparent proxy for remote MCP servers behind redirects
Three issues prevented MCPRemoteProxy from connecting to third-party upstream MCP servers that use HTTP redirects: 1. X-Forwarded-Host leaked the proxy's hostname to the upstream. The upstream used it to construct 307 redirect URLs pointing back to the proxy, creating a redirect loop. Fix: skip SetXForwarded() for remote upstreams (isRemote == true). 2. Go's http.Transport.RoundTrip does not follow redirects, but httputil.ReverseProxy uses Transport directly. Upstream 307/308 redirects (e.g. HTTPS→HTTP scheme changes, path canonicalization) were returned to the MCP client which cannot follow them through the proxy. Fix: add forwardFollowingRedirects that transparently follows up to 10 redirects, preserving method and body for 307/308 (RFC 7538). 3. When disableUpstreamTokenInjection is true, the client's ToolHive JWT was still forwarded to the upstream in the Authorization header. Fix: add strip-auth middleware that removes the Authorization header before forwarding. Also adds debug logging for outbound request headers and upstream response status codes to aid diagnosis of remote proxy issues. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent bc36676 commit 010f7fc

File tree

6 files changed

+196
-31
lines changed

6 files changed

+196
-31
lines changed

cmd/thv-operator/controllers/mcpremoteproxy_controller.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -444,6 +444,9 @@ func (*MCPRemoteProxyReconciler) validateOIDCIssuerURL(proxy *mcpv1alpha1.MCPRem
444444
// validateJWKSURL validates the JWKS URL scheme in the OIDC config.
445445
func (*MCPRemoteProxyReconciler) validateJWKSURL(proxy *mcpv1alpha1.MCPRemoteProxy) error {
446446
oidcConfig := proxy.Spec.OIDCConfig
447+
if oidcConfig == nil {
448+
return nil
449+
}
447450

448451
switch oidcConfig.Type {
449452
case mcpv1alpha1.OIDCConfigTypeInline:

cmd/thv-operator/controllers/mcpremoteproxy_reconciler_test.go

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -916,7 +916,7 @@ func TestValidateSpecConfigurationConditions(t *testing.T) {
916916
ObjectMeta: metav1.ObjectMeta{Name: "invalid-cedar-proxy", Namespace: "default"},
917917
Spec: mcpv1alpha1.MCPRemoteProxySpec{
918918
RemoteURL: "https://mcp.example.com",
919-
OIDCConfig: mcpv1alpha1.OIDCConfigRef{
919+
OIDCConfig: &mcpv1alpha1.OIDCConfigRef{
920920
Type: mcpv1alpha1.OIDCConfigTypeInline,
921921
Inline: &mcpv1alpha1.InlineOIDCConfig{
922922
Issuer: "https://auth.example.com",
@@ -942,7 +942,7 @@ func TestValidateSpecConfigurationConditions(t *testing.T) {
942942
ObjectMeta: metav1.ObjectMeta{Name: "missing-configmap-proxy", Namespace: "default"},
943943
Spec: mcpv1alpha1.MCPRemoteProxySpec{
944944
RemoteURL: "https://mcp.example.com",
945-
OIDCConfig: mcpv1alpha1.OIDCConfigRef{
945+
OIDCConfig: &mcpv1alpha1.OIDCConfigRef{
946946
Type: mcpv1alpha1.OIDCConfigTypeInline,
947947
Inline: &mcpv1alpha1.InlineOIDCConfig{
948948
Issuer: "https://auth.example.com",
@@ -968,7 +968,7 @@ func TestValidateSpecConfigurationConditions(t *testing.T) {
968968
ObjectMeta: metav1.ObjectMeta{Name: "missing-header-secret-proxy", Namespace: "default"},
969969
Spec: mcpv1alpha1.MCPRemoteProxySpec{
970970
RemoteURL: "https://mcp.example.com",
971-
OIDCConfig: mcpv1alpha1.OIDCConfigRef{
971+
OIDCConfig: &mcpv1alpha1.OIDCConfigRef{
972972
Type: mcpv1alpha1.OIDCConfigTypeInline,
973973
Inline: &mcpv1alpha1.InlineOIDCConfig{
974974
Issuer: "https://auth.example.com",
@@ -999,7 +999,7 @@ func TestValidateSpecConfigurationConditions(t *testing.T) {
999999
ObjectMeta: metav1.ObjectMeta{Name: "bad-scheme-proxy", Namespace: "default"},
10001000
Spec: mcpv1alpha1.MCPRemoteProxySpec{
10011001
RemoteURL: "ftp://bad-scheme.example.com",
1002-
OIDCConfig: mcpv1alpha1.OIDCConfigRef{
1002+
OIDCConfig: &mcpv1alpha1.OIDCConfigRef{
10031003
Type: mcpv1alpha1.OIDCConfigTypeInline,
10041004
Inline: &mcpv1alpha1.InlineOIDCConfig{
10051005
Issuer: "https://auth.example.com",
@@ -1019,7 +1019,7 @@ func TestValidateSpecConfigurationConditions(t *testing.T) {
10191019
ObjectMeta: metav1.ObjectMeta{Name: "http-jwks-proxy", Namespace: "default"},
10201020
Spec: mcpv1alpha1.MCPRemoteProxySpec{
10211021
RemoteURL: "https://mcp.example.com",
1022-
OIDCConfig: mcpv1alpha1.OIDCConfigRef{
1022+
OIDCConfig: &mcpv1alpha1.OIDCConfigRef{
10231023
Type: mcpv1alpha1.OIDCConfigTypeInline,
10241024
Inline: &mcpv1alpha1.InlineOIDCConfig{
10251025
Issuer: "https://auth.example.com",

cmd/thv-operator/test-integration/mcp-remote-proxy/remoteproxy_helpers.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -152,7 +152,7 @@ func (rb *RemoteProxyBuilder) WithRemoteURL(url string) *RemoteProxyBuilder {
152152
func (rb *RemoteProxyBuilder) WithInlineOIDCConfigAndJWKS(
153153
issuer, audience, jwksURL string,
154154
) *RemoteProxyBuilder {
155-
rb.proxy.Spec.OIDCConfig = mcpv1alpha1.OIDCConfigRef{
155+
rb.proxy.Spec.OIDCConfig = &mcpv1alpha1.OIDCConfigRef{
156156
Type: mcpv1alpha1.OIDCConfigTypeInline,
157157
Inline: &mcpv1alpha1.InlineOIDCConfig{
158158
Issuer: issuer,

pkg/runner/middleware.go

Lines changed: 44 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ package runner
55

66
import (
77
"fmt"
8+
"net/http"
89

910
"github.com/stacklok/toolhive/pkg/audit"
1011
"github.com/stacklok/toolhive/pkg/auth"
@@ -37,6 +38,7 @@ func GetSupportedMiddlewareFactories() map[string]types.MiddlewareFactory {
3738
audit.MiddlewareType: audit.CreateMiddleware,
3839
recovery.MiddlewareType: recovery.CreateMiddleware,
3940
headerfwd.HeaderForwardMiddlewareName: headerfwd.CreateMiddleware,
41+
stripAuthMiddlewareType: createStripAuthMiddleware,
4042
}
4143
}
4244

@@ -263,9 +265,10 @@ func addUpstreamSwapMiddleware(
263265
return middlewares, nil
264266
}
265267

266-
// Skip upstream token injection if explicitly disabled
268+
// When upstream token injection is disabled, strip the Authorization header
269+
// so the client's ToolHive JWT doesn't leak to the upstream server.
267270
if config.EmbeddedAuthServerConfig.DisableUpstreamTokenInjection {
268-
return middlewares, nil
271+
return addAuthHeaderStripMiddleware(middlewares)
269272
}
270273

271274
// Use provided config or defaults
@@ -287,6 +290,45 @@ func addUpstreamSwapMiddleware(
287290
return append(middlewares, *upstreamSwapMwConfig), nil
288291
}
289292

293+
// stripAuthMiddlewareType is the type identifier for the auth header stripping middleware.
294+
const stripAuthMiddlewareType = "strip-auth"
295+
296+
// addAuthHeaderStripMiddleware adds a middleware that removes the Authorization header
297+
// before forwarding to the upstream. This prevents the client's ToolHive JWT from
298+
// leaking to upstream servers that don't expect it.
299+
func addAuthHeaderStripMiddleware(
300+
middlewares []types.MiddlewareConfig,
301+
) ([]types.MiddlewareConfig, error) {
302+
mwConfig, err := types.NewMiddlewareConfig(stripAuthMiddlewareType, struct{}{})
303+
if err != nil {
304+
return nil, fmt.Errorf("failed to create strip-auth middleware config: %w", err)
305+
}
306+
return append(middlewares, *mwConfig), nil
307+
}
308+
309+
// createStripAuthMiddleware is the factory function for the auth header stripping middleware.
310+
func createStripAuthMiddleware(_ *types.MiddlewareConfig, runner types.MiddlewareRunner) error {
311+
mw := &stripAuthMiddleware{}
312+
runner.AddMiddleware(stripAuthMiddlewareType, mw)
313+
return nil
314+
}
315+
316+
// stripAuthMiddleware removes the Authorization header from requests.
317+
type stripAuthMiddleware struct{}
318+
319+
// Handler returns the middleware function.
320+
func (*stripAuthMiddleware) Handler() types.MiddlewareFunction {
321+
return func(next http.Handler) http.Handler {
322+
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
323+
r.Header.Del("Authorization")
324+
next.ServeHTTP(w, r)
325+
})
326+
}
327+
}
328+
329+
// Close cleans up resources.
330+
func (*stripAuthMiddleware) Close() error { return nil }
331+
290332
// addAWSStsMiddleware adds AWS STS middleware if configured.
291333
// Returns an error if AWSStsConfig is set but RemoteURL is empty, because
292334
// SigV4 signing is only meaningful for remote MCP servers.

pkg/runner/middleware_test.go

Lines changed: 33 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -256,6 +256,7 @@ func TestAddUpstreamSwapMiddleware(t *testing.T) {
256256
name string
257257
config *RunConfig
258258
wantAppended bool
259+
wantType string // expected middleware type when appended
259260
}{
260261
{
261262
name: "nil EmbeddedAuthServerConfig returns input unchanged",
@@ -269,15 +270,17 @@ func TestAddUpstreamSwapMiddleware(t *testing.T) {
269270
UpstreamSwapConfig: nil,
270271
},
271272
wantAppended: true,
273+
wantType: upstreamswap.MiddlewareType,
272274
},
273275
{
274-
name: "EmbeddedAuthServerConfig with DisableUpstreamTokenInjection skips middleware",
276+
name: "DisableUpstreamTokenInjection adds strip-auth middleware instead",
275277
config: func() *RunConfig {
276278
cfg := createMinimalAuthServerConfig()
277279
cfg.DisableUpstreamTokenInjection = true
278280
return &RunConfig{EmbeddedAuthServerConfig: cfg}
279281
}(),
280-
wantAppended: false,
282+
wantAppended: true,
283+
wantType: stripAuthMiddlewareType,
281284
},
282285
{
283286
name: "EmbeddedAuthServerConfig set with explicit UpstreamSwapConfig uses provided config",
@@ -288,6 +291,7 @@ func TestAddUpstreamSwapMiddleware(t *testing.T) {
288291
},
289292
},
290293
wantAppended: true,
294+
wantType: upstreamswap.MiddlewareType,
291295
},
292296
{
293297
name: "EmbeddedAuthServerConfig with custom header strategy config",
@@ -299,6 +303,7 @@ func TestAddUpstreamSwapMiddleware(t *testing.T) {
299303
},
300304
},
301305
wantAppended: true,
306+
wantType: upstreamswap.MiddlewareType,
302307
},
303308
}
304309

@@ -318,20 +323,20 @@ func TestAddUpstreamSwapMiddleware(t *testing.T) {
318323
// Should have one additional entry.
319324
require.Len(t, got, len(initial)+1)
320325
added := got[len(got)-1]
321-
assert.Equal(t, upstreamswap.MiddlewareType, added.Type)
322-
323-
// Verify serialized params contain the expected config.
324-
var params upstreamswap.MiddlewareParams
325-
require.NoError(t, json.Unmarshal(added.Parameters, &params))
326+
assert.Equal(t, tt.wantType, added.Type)
326327

327-
if tt.config.UpstreamSwapConfig != nil {
328-
// Should use the provided config
329-
require.NotNil(t, params.Config)
330-
assert.Equal(t, tt.config.UpstreamSwapConfig.HeaderStrategy, params.Config.HeaderStrategy)
331-
assert.Equal(t, tt.config.UpstreamSwapConfig.CustomHeaderName, params.Config.CustomHeaderName)
332-
} else {
333-
// Should use defaults (empty config is valid)
334-
require.NotNil(t, params.Config)
328+
// For upstreamswap type, verify serialized params
329+
if tt.wantType == upstreamswap.MiddlewareType {
330+
var params upstreamswap.MiddlewareParams
331+
require.NoError(t, json.Unmarshal(added.Parameters, &params))
332+
333+
if tt.config.UpstreamSwapConfig != nil {
334+
require.NotNil(t, params.Config)
335+
assert.Equal(t, tt.config.UpstreamSwapConfig.HeaderStrategy, params.Config.HeaderStrategy)
336+
assert.Equal(t, tt.config.UpstreamSwapConfig.CustomHeaderName, params.Config.CustomHeaderName)
337+
} else {
338+
require.NotNil(t, params.Config)
339+
}
335340
}
336341
})
337342
}
@@ -344,6 +349,7 @@ func TestPopulateMiddlewareConfigs_UpstreamSwap(t *testing.T) {
344349
name string
345350
config *RunConfig
346351
wantUpstreamSwap bool
352+
wantStripAuth bool
347353
wantHeaderStrategy string
348354
}{
349355
{
@@ -357,13 +363,14 @@ func TestPopulateMiddlewareConfigs_UpstreamSwap(t *testing.T) {
357363
wantUpstreamSwap: false,
358364
},
359365
{
360-
name: "DisableUpstreamTokenInjection omits upstream-swap",
366+
name: "DisableUpstreamTokenInjection adds strip-auth instead of upstream-swap",
361367
config: func() *RunConfig {
362368
cfg := createMinimalAuthServerConfig()
363369
cfg.DisableUpstreamTokenInjection = true
364370
return &RunConfig{EmbeddedAuthServerConfig: cfg}
365371
}(),
366372
wantUpstreamSwap: false,
373+
wantStripAuth: true,
367374
},
368375
{
369376
name: "explicit UpstreamSwapConfig is used",
@@ -385,20 +392,25 @@ func TestPopulateMiddlewareConfigs_UpstreamSwap(t *testing.T) {
385392
err := PopulateMiddlewareConfigs(tt.config)
386393
require.NoError(t, err)
387394

388-
var found bool
395+
var foundSwap bool
396+
var foundStrip bool
389397
var foundConfig *types.MiddlewareConfig
390398
for i, mw := range tt.config.MiddlewareConfigs {
391399
if mw.Type == upstreamswap.MiddlewareType {
392-
found = true
400+
foundSwap = true
393401
foundConfig = &tt.config.MiddlewareConfigs[i]
394-
break
402+
}
403+
if mw.Type == stripAuthMiddlewareType {
404+
foundStrip = true
395405
}
396406
}
397-
assert.Equal(t, tt.wantUpstreamSwap, found,
407+
assert.Equal(t, tt.wantUpstreamSwap, foundSwap,
398408
"upstream-swap middleware presence mismatch")
409+
assert.Equal(t, tt.wantStripAuth, foundStrip,
410+
"strip-auth middleware presence mismatch")
399411

400412
// Verify config values if we expect the middleware and have specific expectations
401-
if found && tt.wantHeaderStrategy != "" {
413+
if foundSwap && tt.wantHeaderStrategy != "" {
402414
var params upstreamswap.MiddlewareParams
403415
require.NoError(t, json.Unmarshal(foundConfig.Parameters, &params))
404416
require.NotNil(t, params.Config)

0 commit comments

Comments
 (0)