Skip to content

Commit 7d08e2e

Browse files
jhrozekclaude
andauthored
Unify subject provider defaulting, hard-error xaa (#5708)
* Unify subject provider defaulting, hard-error xaa The YAML and operator paths each defaulted a backend's SubjectProviderName to the first configured upstream, but drifted apart: the YAML path was missing aws_sts entirely (#5687), so an aws_sts backend with an unset field failed at request time instead of being defaulted. Separately, both paths silently picked upstream[0] for xaa even with multiple upstreams configured, a security-relevant footgun since sending the wrong subject token to Step A yields a confusing IdP error or a token minted for the wrong subject (#5697). Extract a shared authtypes.DefaultSubjectProviderName helper used by both the YAML (pkg/vmcp/config/defaults.go) and operator (cmd/thv-operator/controllers/virtualmcpserver_controller.go) paths, covering token_exchange, aws_sts, and xaa. xaa now hard-errors on ambiguous multi-upstream configs on both paths; on the operator side this surfaces as a non-fatal per-backend AuthConfigError condition rather than a Reconcile failure. Fixes #5687, #5697 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Deep-copy backend strategy, correct xaa risk framing DefaultSubjectProviderName shallow-copied each strategy's sub-config by hand, leaving slice fields (Scopes, RoleMappings) aliased with the caller's original struct. Use the generated DeepCopy() instead, which removes the aliasing risk and lets the doc comment drop its "never mutated" hedge. The doc comment also framed the wrong-subject-token risk as specific to xaa, which isn't accurate: token_exchange and aws_sts read from the same UpstreamTokens map and are exposed to the identical risk. xaa hard-errors first purely because it has no existing deployments to break; hard-erroring the other two would be a breaking change needing a deprecation path, tracked separately in #5697. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Exclude backends with failed auth from served config A backend whose auth-strategy injection failed (e.g. an ambiguous xaa config) was omitted from OutgoingAuthConfig.Backends, but still fully served in the ConfigMap's static backend list. At runtime, ResolveForBackend falls through to Default for any backend absent from that map, so the backend ended up authenticated with a different, valid-looking strategy instead of being rejected - exactly the wrong-identity outcome this defaulting work exists to prevent, just relocated to a different code path (CWE-285/863). Track failed backend names via backendsWithFailedAuth and exclude them from convertBackendsToStaticBackends's output entirely, mirroring the existing k8s.go precedent of dropping a backend outright when its auth config can't be resolved, rather than serving it with a fallback strategy. A backend can accumulate an auth error from one path (e.g. a broken discovered ExternalAuthConfigRef) while still resolving a valid strategy from another (e.g. an inline override) - discoverExternalAuthConfigs records the discovered-path error before checking whether an inline override makes it irrelevant. backendsWithFailedAuth only excludes a backend if it also lacks a successfully-resolved strategy, so a backend with valid resolved auth is never dropped just because an unrelated path also produced an error for the same name. Also reworded the injectSubjectProviderIfNeeded doc comment: it named pkg/runner/middleware.go's sibling implementation as if the duplication were a settled non-issue, when in fact two of the three call sites operate on the identical UpstreamRunConfig type and are a legitimate extraction candidate for a future, separate change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
1 parent c46b038 commit 7d08e2e

9 files changed

Lines changed: 1008 additions & 225 deletions

File tree

cmd/thv-operator/controllers/virtualmcpserver_controller.go

Lines changed: 80 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,10 @@ const (
6161
authContextDefault = "default"
6262
authContextBackendPrefix = "backend:"
6363
authContextDiscoveredPrefix = "discovered:"
64+
65+
// authReasonAmbiguousSubjectProvider is the condition Reason surfaced when
66+
// injectSubjectProviderIfNeeded returns authtypes.ErrAmbiguousSubjectProvider.
67+
authReasonAmbiguousSubjectProvider = "AmbiguousSubjectProvider"
6468
)
6569

6670
// AuthConfigError represents a single auth config conversion failure.
@@ -106,6 +110,16 @@ func authConfigErrorReason(authErr *AuthConfigError) string {
106110
return "ConversionFailed"
107111
}
108112

113+
// subjectProviderErrorReason maps an error returned by injectSubjectProviderIfNeeded
114+
// to a condition Reason. Falls back to "" so the caller's AuthConfigError.Reason
115+
// stays empty and authConfigErrorReason applies its default ("ConversionFailed").
116+
func subjectProviderErrorReason(err error) string {
117+
if stderrors.Is(err, authtypes.ErrAmbiguousSubjectProvider) {
118+
return authReasonAmbiguousSubjectProvider
119+
}
120+
return ""
121+
}
122+
109123
// VirtualMCPServerReconciler reconciles a VirtualMCPServer object
110124
//
111125
// Resource Cleanup Strategy:
@@ -2399,12 +2413,24 @@ func (r *VirtualMCPServerReconciler) discoverExternalAuthConfigs(
23992413
continue
24002414
}
24012415

2402-
// Only add if not already overridden in inline config
2403-
if vmcp.Spec.OutgoingAuth == nil || vmcp.Spec.OutgoingAuth.Backends == nil {
2404-
outgoing.Backends[workloadInfo.Name] = injectSubjectProviderIfNeeded(strategy, vmcp.Spec.AuthServerConfig)
2405-
} else if _, exists := vmcp.Spec.OutgoingAuth.Backends[workloadInfo.Name]; !exists {
2406-
// Only add discovered config if not explicitly overridden
2407-
outgoing.Backends[workloadInfo.Name] = injectSubjectProviderIfNeeded(strategy, vmcp.Spec.AuthServerConfig)
2416+
// Only add if not already overridden in inline config.
2417+
shouldAssign := true
2418+
if vmcp.Spec.OutgoingAuth != nil && vmcp.Spec.OutgoingAuth.Backends != nil {
2419+
_, exists := vmcp.Spec.OutgoingAuth.Backends[workloadInfo.Name]
2420+
shouldAssign = !exists
2421+
}
2422+
if shouldAssign {
2423+
injected, err := injectSubjectProviderIfNeeded(strategy, vmcp.Spec.AuthServerConfig)
2424+
if err != nil {
2425+
authErrors = append(authErrors, AuthConfigError{
2426+
Context: fmt.Sprintf("%s%s", authContextDiscoveredPrefix, workloadInfo.Name),
2427+
BackendName: workloadInfo.Name,
2428+
Error: fmt.Errorf("failed to inject subject provider name: %w", err),
2429+
Reason: subjectProviderErrorReason(err),
2430+
})
2431+
} else {
2432+
outgoing.Backends[workloadInfo.Name] = injected
2433+
}
24082434
}
24092435
}
24102436

@@ -2483,8 +2509,15 @@ func (r *VirtualMCPServerReconciler) buildOutgoingAuthConfig(
24832509
Error: fmt.Errorf("failed to convert default auth config: %w", err),
24842510
Reason: mirroredReasonFromError(err),
24852511
})
2512+
} else if injected, injectErr := injectSubjectProviderIfNeeded(defaultStrategy, vmcp.Spec.AuthServerConfig); injectErr != nil {
2513+
allAuthErrors = append(allAuthErrors, AuthConfigError{
2514+
Context: authContextDefault,
2515+
BackendName: "",
2516+
Error: fmt.Errorf("failed to inject subject provider name: %w", injectErr),
2517+
Reason: subjectProviderErrorReason(injectErr),
2518+
})
24862519
} else {
2487-
outgoing.Default = injectSubjectProviderIfNeeded(defaultStrategy, vmcp.Spec.AuthServerConfig)
2520+
outgoing.Default = injected
24882521
}
24892522
}
24902523

@@ -2509,8 +2542,15 @@ func (r *VirtualMCPServerReconciler) buildOutgoingAuthConfig(
25092542
Error: fmt.Errorf("failed to convert backend auth config: %w", err),
25102543
Reason: mirroredReasonFromError(err),
25112544
})
2545+
} else if injected, injectErr := injectSubjectProviderIfNeeded(strategy, vmcp.Spec.AuthServerConfig); injectErr != nil {
2546+
allAuthErrors = append(allAuthErrors, AuthConfigError{
2547+
Context: fmt.Sprintf("%s%s", authContextBackendPrefix, backendName),
2548+
BackendName: backendName,
2549+
Error: fmt.Errorf("failed to inject subject provider name: %w", injectErr),
2550+
Reason: subjectProviderErrorReason(injectErr),
2551+
})
25122552
} else {
2513-
outgoing.Backends[backendName] = injectSubjectProviderIfNeeded(strategy, vmcp.Spec.AuthServerConfig)
2553+
outgoing.Backends[backendName] = injected
25142554
}
25152555
}
25162556
}
@@ -2520,59 +2560,33 @@ func (r *VirtualMCPServerReconciler) buildOutgoingAuthConfig(
25202560

25212561
// injectSubjectProviderIfNeeded auto-populates the upstream provider name on
25222562
// token_exchange, aws_sts, and xaa strategies when the field is empty and an
2523-
// embedded auth server is configured on the VirtualMCPServer.
2524-
// All three strategies use SubjectProviderName for the same concept: which
2525-
// upstream provider's token to pull from Identity.UpstreamTokens. Mirrors
2526-
// injectUpstreamProviderIfNeeded in pkg/runner/middleware.go, which does the
2527-
// same for Cedar's PrimaryUpstreamProvider, and InjectSubjectProviderNames in
2528-
// pkg/vmcp/config/defaults.go, which applies the same defaulting at serve time.
2529-
// Returns strategy unchanged when it is nil, not an applicable strategy type,
2530-
// already has the provider name set, or no embedded auth server is configured.
2563+
// embedded auth server is configured on the VirtualMCPServer. All three
2564+
// strategies use SubjectProviderName for the same concept: which upstream
2565+
// provider's token to pull from Identity.UpstreamTokens.
2566+
//
2567+
// The same first-upstream-name extraction appears in pkg/runner/middleware.go
2568+
// and pkg/vmcp/config/defaults.go, which share the authserver.UpstreamRunConfig
2569+
// type and are candidates for a shared authserver helper (tracked as follow-up
2570+
// work). This function operates on the CRD-specific EmbeddedAuthServerConfig
2571+
// type and is intentionally kept separate.
2572+
//
2573+
// Delegates the actual defaulting to authtypes.DefaultSubjectProviderName.
2574+
// Returns strategy unchanged when it is nil or no embedded auth server is
2575+
// configured. Can return authtypes.ErrAmbiguousSubjectProvider when strategy
2576+
// is xaa, SubjectProviderName is empty, and more than one upstream provider
2577+
// is configured.
25312578
func injectSubjectProviderIfNeeded(
25322579
strategy *authtypes.BackendAuthStrategy,
25332580
embeddedCfg *mcpv1beta1.EmbeddedAuthServerConfig,
2534-
) *authtypes.BackendAuthStrategy {
2581+
) (*authtypes.BackendAuthStrategy, error) {
25352582
if strategy == nil || embeddedCfg == nil {
2536-
return strategy
2537-
}
2538-
2539-
switch strategy.Type {
2540-
case authtypes.StrategyTypeTokenExchange:
2541-
if strategy.TokenExchange == nil || strategy.TokenExchange.SubjectProviderName != "" {
2542-
return strategy
2543-
}
2544-
providerName := resolveFirstUpstreamProvider(embeddedCfg)
2545-
copied := *strategy
2546-
teCopied := *strategy.TokenExchange
2547-
teCopied.SubjectProviderName = providerName
2548-
copied.TokenExchange = &teCopied
2549-
return &copied
2550-
2551-
case authtypes.StrategyTypeAwsSts:
2552-
if strategy.AwsSts == nil || strategy.AwsSts.SubjectProviderName != "" {
2553-
return strategy
2554-
}
2555-
providerName := resolveFirstUpstreamProvider(embeddedCfg)
2556-
copied := *strategy
2557-
stsCopied := *strategy.AwsSts
2558-
stsCopied.SubjectProviderName = providerName
2559-
copied.AwsSts = &stsCopied
2560-
return &copied
2561-
2562-
case authtypes.StrategyTypeXAA:
2563-
if strategy.XAA == nil || strategy.XAA.SubjectProviderName != "" {
2564-
return strategy
2565-
}
2566-
providerName := resolveFirstUpstreamProvider(embeddedCfg)
2567-
copied := *strategy
2568-
xaaCopied := *strategy.XAA
2569-
xaaCopied.SubjectProviderName = providerName
2570-
copied.XAA = &xaaCopied
2571-
return &copied
2572-
2573-
default:
2574-
return strategy
2583+
return strategy, nil
25752584
}
2585+
return authtypes.DefaultSubjectProviderName(
2586+
strategy,
2587+
resolveFirstUpstreamProvider(embeddedCfg),
2588+
len(embeddedCfg.UpstreamProviders) > 1,
2589+
)
25762590
}
25772591

25782592
// resolveFirstUpstreamProvider returns the resolved name of the first upstream
@@ -2590,15 +2604,25 @@ func resolveFirstUpstreamProvider(embeddedCfg *mcpv1beta1.EmbeddedAuthServerConf
25902604
// Preserves metadata and uses transport types from workload Specs.
25912605
// Logs warnings when backends are skipped due to missing URL or transport information.
25922606
// caBundlePathMap maps backend names to their CA bundle mount paths (populated for MCPServerEntry backends).
2607+
// excludedBackends names backends whose outgoing auth strategy failed to build (see
2608+
// backendsWithFailedAuth); they are dropped from the served set entirely rather than
2609+
// left to fall through to the Default strategy at runtime.
25932610
func convertBackendsToStaticBackends(
25942611
ctx context.Context,
25952612
backends []vmcptypes.Backend,
25962613
transportMap map[string]string,
25972614
caBundlePathMap map[string]string,
2615+
excludedBackends map[string]struct{},
25982616
) []vmcpconfig.StaticBackendConfig {
25992617
logger := log.FromContext(ctx)
26002618
static := make([]vmcpconfig.StaticBackendConfig, 0, len(backends))
26012619
for _, backend := range backends {
2620+
if _, excluded := excludedBackends[backend.Name]; excluded {
2621+
logger.V(1).Info("Skipping backend with failed outgoing auth configuration",
2622+
"backend", backend.Name)
2623+
continue
2624+
}
2625+
26022626
if backend.BaseURL == "" {
26032627
logger.V(1).Info("Skipping backend without URL in static mode",
26042628
"backend", backend.Name)

0 commit comments

Comments
 (0)