Skip to content

Commit 117be04

Browse files
tgrunnagleclaude
andauthored
Default redirectUri on upstream providers as documented in CRD (#4905)
* Default redirectUri on upstream providers to {resourceUrl}/oauth/callback The MCPExternalAuthConfig CRD documents that redirectUri defaults to {resourceUrl}/oauth/callback when not specified, but the operator was passing the empty string through to the proxy runner, which then rejected it with "redirect_uri is required". Pass resourceURL through BuildAuthServerRunConfig to buildUpstreamRunConfig so empty RedirectURI fields on both OIDC and OAuth2 upstream providers are resolved to the documented default before generating the runconfig. This applies to both MCPServer and VirtualMCPServer code paths. Fixes #4874 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Add unit test for deriveResourceURL in converter_test.go Address code review feedback: add TestDeriveResourceURL table-driven test matching the convention established by TestDeriveAllowedAudiences and TestDeriveScopesSupported. Covers nil IncomingAuth, nil OIDC, empty Resource, and populated Resource cases. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent f3c9f24 commit 117be04

4 files changed

Lines changed: 233 additions & 10 deletions

File tree

cmd/thv-operator/pkg/controllerutil/authserver.go

Lines changed: 29 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -403,6 +403,7 @@ func AddEmbeddedAuthServerConfigOptions(
403403
embeddedConfig, err := BuildAuthServerRunConfig(
404404
namespace, mcpServerName, authServerConfig,
405405
[]string{oidcConfig.ResourceURL}, oidcConfig.Scopes,
406+
oidcConfig.ResourceURL,
406407
)
407408
if err != nil {
408409
return fmt.Errorf("failed to build embedded auth server config: %w", err)
@@ -453,15 +454,19 @@ func validateOIDCConfigForEmbeddedAuthServer(oidcConfig *oidc.OIDCConfig) error
453454
// BuildAuthServerRunConfig converts CRD EmbeddedAuthServerConfig to authserver.RunConfig.
454455
// The RunConfig is serializable and contains file paths for secrets (not the secrets themselves).
455456
//
456-
// AllowedAudiences and ScopesSupported are caller-provided because different controllers
457-
// derive them from different sources (MCPServer uses oidcConfig.ResourceURL/Scopes;
457+
// AllowedAudiences, ScopesSupported, and resourceURL are caller-provided because different
458+
// controllers derive them from different sources (MCPServer uses oidcConfig.ResourceURL/Scopes;
458459
// VirtualMCPServer derives from the resolved vmcp Config).
460+
//
461+
// resourceURL is used to default the RedirectURI on upstream providers when not explicitly set.
462+
// The default is {resourceURL}/oauth/callback as documented in the MCPExternalAuthConfig CRD.
459463
func BuildAuthServerRunConfig(
460464
namespace string,
461465
name string,
462466
authConfig *mcpv1alpha1.EmbeddedAuthServerConfig,
463467
allowedAudiences []string,
464468
scopesSupported []string,
469+
resourceURL string,
465470
) (*authserver.RunConfig, error) {
466471
config := &authserver.RunConfig{
467472
SchemaVersion: authserver.CurrentSchemaVersion,
@@ -506,7 +511,7 @@ func BuildAuthServerRunConfig(
506511
bindings := buildUpstreamSecretBindings(authConfig.UpstreamProviders)
507512
config.Upstreams = make([]authserver.UpstreamRunConfig, 0, len(bindings))
508513
for _, b := range bindings {
509-
config.Upstreams = append(config.Upstreams, *buildUpstreamRunConfig(b.Provider, b.EnvVarName))
514+
config.Upstreams = append(config.Upstreams, *buildUpstreamRunConfig(b.Provider, b.EnvVarName, resourceURL))
510515
}
511516

512517
// Build storage configuration
@@ -632,12 +637,21 @@ func resolveSentinelAddrs(
632637
return []string{dnsName}, nil
633638
}
634639

640+
// defaultRedirectURI returns the default redirect URI for an upstream provider
641+
// when one is not explicitly configured. The default is {resourceURL}/oauth/callback
642+
// as documented in the MCPExternalAuthConfig CRD.
643+
func defaultRedirectURI(resourceURL string) string {
644+
return strings.TrimRight(resourceURL, "/") + "/oauth/callback"
645+
}
646+
635647
// buildUpstreamRunConfig converts CRD UpstreamProviderConfig to authserver.UpstreamRunConfig.
636648
// The envVarName is computed by buildUpstreamSecretBindings to keep Pod env
637-
// and runtime config in sync.
649+
// and runtime config in sync. When a provider's RedirectURI is empty, it is
650+
// defaulted to {resourceURL}/oauth/callback.
638651
func buildUpstreamRunConfig(
639652
provider *mcpv1alpha1.UpstreamProviderConfig,
640653
envVarName string,
654+
resourceURL string,
641655
) *authserver.UpstreamRunConfig {
642656
config := &authserver.UpstreamRunConfig{
643657
Name: provider.Name,
@@ -647,10 +661,14 @@ func buildUpstreamRunConfig(
647661
switch provider.Type {
648662
case mcpv1alpha1.UpstreamProviderTypeOIDC:
649663
if provider.OIDCConfig != nil {
664+
redirectURI := provider.OIDCConfig.RedirectURI
665+
if redirectURI == "" && resourceURL != "" {
666+
redirectURI = defaultRedirectURI(resourceURL)
667+
}
650668
config.OIDCConfig = &authserver.OIDCUpstreamRunConfig{
651669
IssuerURL: provider.OIDCConfig.IssuerURL,
652670
ClientID: provider.OIDCConfig.ClientID,
653-
RedirectURI: provider.OIDCConfig.RedirectURI,
671+
RedirectURI: redirectURI,
654672
Scopes: provider.OIDCConfig.Scopes,
655673
AdditionalAuthorizationParams: provider.OIDCConfig.AdditionalAuthorizationParams,
656674
}
@@ -664,11 +682,15 @@ func buildUpstreamRunConfig(
664682
}
665683
case mcpv1alpha1.UpstreamProviderTypeOAuth2:
666684
if provider.OAuth2Config != nil {
685+
redirectURI := provider.OAuth2Config.RedirectURI
686+
if redirectURI == "" && resourceURL != "" {
687+
redirectURI = defaultRedirectURI(resourceURL)
688+
}
667689
config.OAuth2Config = &authserver.OAuth2UpstreamRunConfig{
668690
AuthorizationEndpoint: provider.OAuth2Config.AuthorizationEndpoint,
669691
TokenEndpoint: provider.OAuth2Config.TokenEndpoint,
670692
ClientID: provider.OAuth2Config.ClientID,
671-
RedirectURI: provider.OAuth2Config.RedirectURI,
693+
RedirectURI: redirectURI,
672694
Scopes: provider.OAuth2Config.Scopes,
673695
AdditionalAuthorizationParams: provider.OAuth2Config.AdditionalAuthorizationParams,
674696
}
@@ -799,6 +821,7 @@ func AddAuthServerRefOptions(
799821
embeddedConfig, err := BuildAuthServerRunConfig(
800822
namespace, mcpServerName, authServerConfig,
801823
[]string{oidcConfig.ResourceURL}, oidcConfig.Scopes,
824+
oidcConfig.ResourceURL,
802825
)
803826
if err != nil {
804827
return fmt.Errorf("failed to build embedded auth server config: %w", err)

cmd/thv-operator/pkg/controllerutil/authserver_test.go

Lines changed: 139 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -690,11 +690,14 @@ func TestBuildAuthServerRunConfig(t *testing.T) {
690690
defaultAudiences := []string{"http://test-server.default.svc.cluster.local:8080"}
691691
defaultScopes := []string{"openid", "offline_access"}
692692

693+
defaultResourceURL := "http://test-server.default.svc.cluster.local:8080"
694+
693695
tests := []struct {
694696
name string
695697
authConfig *mcpv1alpha1.EmbeddedAuthServerConfig
696698
allowedAudiences []string
697699
scopesSupported []string
700+
resourceURL string
698701
checkFunc func(t *testing.T, config *authserver.RunConfig)
699702
}{
700703
{
@@ -774,7 +777,8 @@ func TestBuildAuthServerRunConfig(t *testing.T) {
774777
},
775778
},
776779
{
777-
name: "with OIDC upstream provider",
780+
name: "with OIDC upstream provider",
781+
resourceURL: defaultResourceURL,
778782
authConfig: &mcpv1alpha1.EmbeddedAuthServerConfig{
779783
Issuer: "https://auth.example.com",
780784
SigningKeySecretRefs: []mcpv1alpha1.SecretKeyRef{
@@ -811,7 +815,8 @@ func TestBuildAuthServerRunConfig(t *testing.T) {
811815
},
812816
},
813817
{
814-
name: "with OAuth2 upstream provider with userinfo config",
818+
name: "with OAuth2 upstream provider with userinfo config",
819+
resourceURL: defaultResourceURL,
815820
authConfig: &mcpv1alpha1.EmbeddedAuthServerConfig{
816821
Issuer: "https://auth.example.com",
817822
SigningKeySecretRefs: []mcpv1alpha1.SecretKeyRef{
@@ -904,7 +909,8 @@ func TestBuildAuthServerRunConfig(t *testing.T) {
904909
},
905910
},
906911
{
907-
name: "with multiple upstream providers all are included",
912+
name: "with multiple upstream providers all are included",
913+
resourceURL: defaultResourceURL,
908914
authConfig: &mcpv1alpha1.EmbeddedAuthServerConfig{
909915
Issuer: "https://auth.example.com",
910916
SigningKeySecretRefs: []mcpv1alpha1.SecretKeyRef{
@@ -1041,13 +1047,141 @@ func TestBuildAuthServerRunConfig(t *testing.T) {
10411047
upstream.OAuth2Config.AdditionalAuthorizationParams)
10421048
},
10431049
},
1050+
{
1051+
name: "OIDC upstream with empty redirectUri defaults to resourceURL/oauth/callback",
1052+
resourceURL: "https://mcp.example.com",
1053+
authConfig: &mcpv1alpha1.EmbeddedAuthServerConfig{
1054+
Issuer: "https://auth.example.com",
1055+
SigningKeySecretRefs: []mcpv1alpha1.SecretKeyRef{
1056+
{Name: "signing-key", Key: "private.pem"},
1057+
},
1058+
HMACSecretRefs: []mcpv1alpha1.SecretKeyRef{
1059+
{Name: "hmac-secret", Key: "hmac"},
1060+
},
1061+
UpstreamProviders: []mcpv1alpha1.UpstreamProviderConfig{
1062+
{
1063+
Name: "okta",
1064+
Type: mcpv1alpha1.UpstreamProviderTypeOIDC,
1065+
OIDCConfig: &mcpv1alpha1.OIDCUpstreamConfig{
1066+
IssuerURL: "https://okta.example.com",
1067+
ClientID: "client-id",
1068+
// RedirectURI intentionally omitted
1069+
},
1070+
},
1071+
},
1072+
},
1073+
allowedAudiences: defaultAudiences,
1074+
scopesSupported: defaultScopes,
1075+
checkFunc: func(t *testing.T, config *authserver.RunConfig) {
1076+
t.Helper()
1077+
require.Len(t, config.Upstreams, 1)
1078+
require.NotNil(t, config.Upstreams[0].OIDCConfig)
1079+
assert.Equal(t, "https://mcp.example.com/oauth/callback", config.Upstreams[0].OIDCConfig.RedirectURI)
1080+
},
1081+
},
1082+
{
1083+
name: "OAuth2 upstream with empty redirectUri defaults to resourceURL/oauth/callback",
1084+
resourceURL: "https://mcp.example.com",
1085+
authConfig: &mcpv1alpha1.EmbeddedAuthServerConfig{
1086+
Issuer: "https://auth.example.com",
1087+
SigningKeySecretRefs: []mcpv1alpha1.SecretKeyRef{
1088+
{Name: "signing-key", Key: "private.pem"},
1089+
},
1090+
HMACSecretRefs: []mcpv1alpha1.SecretKeyRef{
1091+
{Name: "hmac-secret", Key: "hmac"},
1092+
},
1093+
UpstreamProviders: []mcpv1alpha1.UpstreamProviderConfig{
1094+
{
1095+
Name: "github",
1096+
Type: mcpv1alpha1.UpstreamProviderTypeOAuth2,
1097+
OAuth2Config: &mcpv1alpha1.OAuth2UpstreamConfig{
1098+
AuthorizationEndpoint: "https://github.com/login/oauth/authorize",
1099+
TokenEndpoint: "https://github.com/login/oauth/access_token",
1100+
ClientID: "client-id",
1101+
// RedirectURI intentionally omitted
1102+
},
1103+
},
1104+
},
1105+
},
1106+
allowedAudiences: defaultAudiences,
1107+
scopesSupported: defaultScopes,
1108+
checkFunc: func(t *testing.T, config *authserver.RunConfig) {
1109+
t.Helper()
1110+
require.Len(t, config.Upstreams, 1)
1111+
require.NotNil(t, config.Upstreams[0].OAuth2Config)
1112+
assert.Equal(t, "https://mcp.example.com/oauth/callback", config.Upstreams[0].OAuth2Config.RedirectURI)
1113+
},
1114+
},
1115+
{
1116+
name: "explicit redirectUri is preserved when resourceURL is also set",
1117+
resourceURL: "https://mcp.example.com",
1118+
authConfig: &mcpv1alpha1.EmbeddedAuthServerConfig{
1119+
Issuer: "https://auth.example.com",
1120+
SigningKeySecretRefs: []mcpv1alpha1.SecretKeyRef{
1121+
{Name: "signing-key", Key: "private.pem"},
1122+
},
1123+
HMACSecretRefs: []mcpv1alpha1.SecretKeyRef{
1124+
{Name: "hmac-secret", Key: "hmac"},
1125+
},
1126+
UpstreamProviders: []mcpv1alpha1.UpstreamProviderConfig{
1127+
{
1128+
Name: "okta",
1129+
Type: mcpv1alpha1.UpstreamProviderTypeOIDC,
1130+
OIDCConfig: &mcpv1alpha1.OIDCUpstreamConfig{
1131+
IssuerURL: "https://okta.example.com",
1132+
ClientID: "client-id",
1133+
RedirectURI: "https://custom.example.com/callback",
1134+
},
1135+
},
1136+
},
1137+
},
1138+
allowedAudiences: defaultAudiences,
1139+
scopesSupported: defaultScopes,
1140+
checkFunc: func(t *testing.T, config *authserver.RunConfig) {
1141+
t.Helper()
1142+
require.Len(t, config.Upstreams, 1)
1143+
require.NotNil(t, config.Upstreams[0].OIDCConfig)
1144+
assert.Equal(t, "https://custom.example.com/callback", config.Upstreams[0].OIDCConfig.RedirectURI)
1145+
},
1146+
},
1147+
{
1148+
name: "resourceURL with trailing slash produces correct default redirectUri",
1149+
resourceURL: "https://mcp.example.com/",
1150+
authConfig: &mcpv1alpha1.EmbeddedAuthServerConfig{
1151+
Issuer: "https://auth.example.com",
1152+
SigningKeySecretRefs: []mcpv1alpha1.SecretKeyRef{
1153+
{Name: "signing-key", Key: "private.pem"},
1154+
},
1155+
HMACSecretRefs: []mcpv1alpha1.SecretKeyRef{
1156+
{Name: "hmac-secret", Key: "hmac"},
1157+
},
1158+
UpstreamProviders: []mcpv1alpha1.UpstreamProviderConfig{
1159+
{
1160+
Name: "okta",
1161+
Type: mcpv1alpha1.UpstreamProviderTypeOIDC,
1162+
OIDCConfig: &mcpv1alpha1.OIDCUpstreamConfig{
1163+
IssuerURL: "https://okta.example.com",
1164+
ClientID: "client-id",
1165+
},
1166+
},
1167+
},
1168+
},
1169+
allowedAudiences: defaultAudiences,
1170+
scopesSupported: defaultScopes,
1171+
checkFunc: func(t *testing.T, config *authserver.RunConfig) {
1172+
t.Helper()
1173+
require.Len(t, config.Upstreams, 1)
1174+
require.NotNil(t, config.Upstreams[0].OIDCConfig)
1175+
assert.Equal(t, "https://mcp.example.com/oauth/callback", config.Upstreams[0].OIDCConfig.RedirectURI)
1176+
},
1177+
},
10441178
}
10451179

10461180
for _, tt := range tests {
10471181
t.Run(tt.name, func(t *testing.T) {
10481182
t.Parallel()
10491183

1050-
config, err := BuildAuthServerRunConfig("default", "test-server", tt.authConfig, tt.allowedAudiences, tt.scopesSupported)
1184+
config, err := BuildAuthServerRunConfig("default", "test-server", tt.authConfig, tt.allowedAudiences, tt.scopesSupported, tt.resourceURL)
10511185

10521186
require.NoError(t, err)
10531187
require.NotNil(t, config)
@@ -1634,6 +1768,7 @@ func TestBuildAuthServerRunConfig_WithRedisStorage(t *testing.T) {
16341768
"default", "my-mcp-server", authConfig,
16351769
[]string{"http://test-server.default.svc.cluster.local:8080"},
16361770
[]string{"openid"},
1771+
"http://test-server.default.svc.cluster.local:8080",
16371772
)
16381773

16391774
require.NoError(t, err)

cmd/thv-operator/pkg/vmcpconfig/converter.go

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -318,6 +318,7 @@ func (*Converter) convertAuthServerConfig(
318318
vmcp.Spec.AuthServerConfig,
319319
deriveAllowedAudiences(config),
320320
deriveScopesSupported(config),
321+
deriveResourceURL(config),
321322
)
322323
}
323324

@@ -344,6 +345,16 @@ func deriveAllowedAudiences(config *vmcpconfig.Config) []string {
344345
return []string{resource}
345346
}
346347

348+
// deriveResourceURL returns the resource URL from the resolved incoming OIDC config.
349+
// Returns empty string when OIDC is not configured or Resource is empty.
350+
// Used to default upstream provider RedirectURIs to {resourceURL}/oauth/callback.
351+
func deriveResourceURL(config *vmcpconfig.Config) string {
352+
if config.IncomingAuth == nil || config.IncomingAuth.OIDC == nil {
353+
return ""
354+
}
355+
return config.IncomingAuth.OIDC.Resource
356+
}
357+
347358
// deriveScopesSupported returns the scopes from the resolved incoming OIDC config.
348359
// Returns nil when OIDC is not configured or scopes are empty, which causes the
349360
// auth server to use its default scopes (["openid", "profile", "email", "offline_access"]).

cmd/thv-operator/pkg/vmcpconfig/converter_test.go

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1710,6 +1710,60 @@ func TestDeriveScopesSupported(t *testing.T) {
17101710
}
17111711
}
17121712

1713+
func TestDeriveResourceURL(t *testing.T) {
1714+
t.Parallel()
1715+
1716+
tests := []struct {
1717+
name string
1718+
config *vmcpconfig.Config
1719+
expected string
1720+
}{
1721+
{
1722+
name: "nil IncomingAuth returns empty",
1723+
config: &vmcpconfig.Config{},
1724+
expected: "",
1725+
},
1726+
{
1727+
name: "nil OIDC returns empty",
1728+
config: &vmcpconfig.Config{
1729+
IncomingAuth: &vmcpconfig.IncomingAuthConfig{Type: "oidc"},
1730+
},
1731+
expected: "",
1732+
},
1733+
{
1734+
name: "empty Resource returns empty",
1735+
config: &vmcpconfig.Config{
1736+
IncomingAuth: &vmcpconfig.IncomingAuthConfig{
1737+
Type: "oidc",
1738+
OIDC: &vmcpconfig.OIDCConfig{},
1739+
},
1740+
},
1741+
expected: "",
1742+
},
1743+
{
1744+
name: "populated Resource is returned",
1745+
config: &vmcpconfig.Config{
1746+
IncomingAuth: &vmcpconfig.IncomingAuthConfig{
1747+
Type: "oidc",
1748+
OIDC: &vmcpconfig.OIDCConfig{
1749+
Resource: "https://resource.example.com",
1750+
},
1751+
},
1752+
},
1753+
expected: "https://resource.example.com",
1754+
},
1755+
}
1756+
1757+
for _, tt := range tests {
1758+
t.Run(tt.name, func(t *testing.T) {
1759+
t.Parallel()
1760+
1761+
result := deriveResourceURL(tt.config)
1762+
assert.Equal(t, tt.expected, result)
1763+
})
1764+
}
1765+
}
1766+
17131767
// TestConvert_AuthServerConfigIntegration is an integration-level test that exercises the
17141768
// full Convert() path with an AuthServerConfig set on the VirtualMCPServer. It verifies that
17151769
// the returned RunConfig has the correct Issuer, Upstreams, and AllowedAudiences derived

0 commit comments

Comments
 (0)