diff --git a/cmd/thv-operator/api/v1beta1/virtualmcpserver_types.go b/cmd/thv-operator/api/v1beta1/virtualmcpserver_types.go index 6a9f72b3d9..e14f00df5d 100644 --- a/cmd/thv-operator/api/v1beta1/virtualmcpserver_types.go +++ b/cmd/thv-operator/api/v1beta1/virtualmcpserver_types.go @@ -280,6 +280,12 @@ const ( // ConditionTypeAuthServerConfigValidated indicates whether the AuthServerConfig has been validated ConditionTypeAuthServerConfigValidated = "AuthServerConfigValidated" + // ConditionTypeAuthzUpstreamSelectionWarning is an advisory condition set to True when + // multiple AuthServerConfig.UpstreamProviders are configured alongside AuthzConfig. + // Only the first upstream is authoritative for Cedar claim resolution; this warns the + // operator that the auto-selection has taken effect and names the selected upstream. + ConditionTypeAuthzUpstreamSelectionWarning = "AuthzUpstreamSelectionWarning" + // ConditionTypeVirtualMCPServerTelemetryConfigRefValidated indicates whether the TelemetryConfigRef is valid ConditionTypeVirtualMCPServerTelemetryConfigRefValidated = "TelemetryConfigRefValidated" ) @@ -346,6 +352,18 @@ const ( // ConditionReasonAuthServerConfigInvalid indicates the AuthServerConfig is invalid ConditionReasonAuthServerConfigInvalid = "AuthServerConfigInvalid" + // ConditionReasonAuthzRequiresUpstream indicates that authorization policies are + // configured but no upstream IDP is available to source claims from. Without an + // upstream, Cedar evaluates against the ToolHive-issued AS token, whose claim + // namespace (sub, aud, tsid) can overlap upstream claims and silently authorize + // against the wrong identity. + ConditionReasonAuthzRequiresUpstream = "AuthzRequiresUpstream" + + // ConditionReasonAuthzUpstreamAutoSelected is set when authorization is configured + // alongside multiple upstream providers and the first upstream has been chosen as + // the Cedar claim source. The advisory message names the selected upstream. + ConditionReasonAuthzUpstreamAutoSelected = "AuthzUpstreamAutoSelected" + // ConditionReasonVirtualMCPServerTelemetryConfigRefValid indicates the referenced MCPTelemetryConfig is valid ConditionReasonVirtualMCPServerTelemetryConfigRefValid = "TelemetryConfigRefValid" diff --git a/cmd/thv-operator/controllers/virtualmcpserver_controller.go b/cmd/thv-operator/controllers/virtualmcpserver_controller.go index 3baf42eed9..981a06e9f1 100644 --- a/cmd/thv-operator/controllers/virtualmcpserver_controller.go +++ b/cmd/thv-operator/controllers/virtualmcpserver_controller.go @@ -298,8 +298,10 @@ func (r *VirtualMCPServerReconciler) applyStatusUpdates( return nil } -// runValidations runs all pre-reconciliation validations (PodTemplateSpec, GroupRef, -// CompositeToolRefs, EmbeddingServerRef, AuthServerConfig). +// runValidations runs all pre-reconciliation validations in order: schema-level +// spec validation, PodTemplateSpec, GroupRef, CompositeToolRefs, EmbeddingServerRef, +// auth-related checks (inline AuthServerConfig + AuthzConfig/upstream coherence, +// delegated to runAuthValidations), and the advisory SessionStorage warning. // Returns (true, nil) to continue reconciliation. // Returns (false, nil) for spec validation errors that should NOT trigger requeue // (user must fix the spec; next reconciliation is triggered by spec changes). @@ -352,23 +354,53 @@ func (r *VirtualMCPServerReconciler) runValidations( } } + // Validate auth-related spec fields (AuthServerConfig + AuthzConfig coherence). + if ok := r.runAuthValidations(ctx, vmcp, statusManager); !ok { + return false, nil + } + + // Advisory: warn when replicas > 1 but session storage is not Redis-backed. + r.validateSessionStorageForReplicas(vmcp, statusManager) + + return true, nil +} + +// runAuthValidations runs the auth-related spec validations: the inline +// AuthServerConfig (when specified) and the AuthzConfig/upstream coherence +// check. Returns false when a validation fails and the caller should stop +// reconciliation (user must fix the spec); true to continue. +func (r *VirtualMCPServerReconciler) runAuthValidations( + ctx context.Context, + vmcp *mcpv1beta1.VirtualMCPServer, + statusManager virtualmcpserverstatus.StatusManager, +) bool { + ctxLogger := log.FromContext(ctx) + // Validate inline AuthServerConfig (when specified). if vmcp.Spec.AuthServerConfig != nil { if err := r.validateAuthServerConfig(vmcp, statusManager); err != nil { if applyErr := r.applyStatusUpdates(ctx, vmcp, statusManager); applyErr != nil { ctxLogger.Error(applyErr, "Failed to apply status updates after AuthServerConfig validation error") } - return false, nil + return false } } else { // Remove stale condition if AuthServerConfig was previously set then removed. statusManager.RemoveConditionsWithPrefix(mcpv1beta1.ConditionTypeAuthServerConfigValidated, []string{}) } - // Advisory: warn when replicas > 1 but session storage is not Redis-backed. - r.validateSessionStorageForReplicas(vmcp, statusManager) + // Validate that authz policies have an upstream IDP available to source + // claims from. Runs after the AuthServerConfig branch so it can set the + // AuthServerConfigValidated condition without being clobbered by the + // RemoveConditionsWithPrefix call above when AuthServerConfig is nil. + if err := r.validateAuthzUpstreamAvailable(ctx, vmcp, statusManager); err != nil { + if applyErr := r.applyStatusUpdates(ctx, vmcp, statusManager); applyErr != nil { + ctxLogger.Error(applyErr, "Failed to apply status updates after AuthzUpstreamAvailable validation error") + } + return false + } - return true, nil + return true } // validateSessionStorageForReplicas emits a SessionStorageWarning condition when @@ -468,6 +500,98 @@ func (*VirtualMCPServerReconciler) validateAuthServerConfig( return nil } +// validateAuthzUpstreamAvailable ensures that when authorization policies are +// configured via IncomingAuth.AuthzConfig AND an embedded AuthServer is in use, +// at least one upstream IDP is declared so Cedar evaluates claim references +// (e.g. principal.claim_department) against the upstream token rather than the +// ToolHive-issued AS token — whose claim namespace (sub, aud, tsid) can overlap +// upstream claims and silently authorize against the wrong identity. +// +// Direct-IdP incoming auth (clients present an already-validated IdP token, no +// embedded AS) is legitimate: Cedar evaluates against the identity's claims via +// the default branch and no upstream is needed. The validator ignores that case. +// +// When multiple upstream providers are declared alongside AuthzConfig, only the +// first one is authoritative for Cedar. Surface an advisory +// AuthzUpstreamSelectionWarning condition naming the selected provider so the +// operator can reorder or prune the list if the auto-selection is wrong. +func (*VirtualMCPServerReconciler) validateAuthzUpstreamAvailable( + ctx context.Context, + vmcp *mcpv1beta1.VirtualMCPServer, + statusManager virtualmcpserverstatus.StatusManager, +) error { + // No authz configured, or no incoming auth at all: nothing to check and + // no advisory to maintain. Remove any stale condition from a previous + // multi-upstream configuration. + if vmcp.Spec.IncomingAuth == nil || vmcp.Spec.IncomingAuth.AuthzConfig == nil { + statusManager.RemoveConditionsWithPrefix(mcpv1beta1.ConditionTypeAuthzUpstreamSelectionWarning, []string{}) + return nil + } + + // Direct-IdP flow: no embedded AS. Cedar evaluates against identity.Claims + // populated by incoming OIDC middleware from the IdP token. No upstream + // needed; nothing to warn about. Remove any stale condition. + if vmcp.Spec.AuthServerConfig == nil { + statusManager.RemoveConditionsWithPrefix(mcpv1beta1.ConditionTypeAuthzUpstreamSelectionWarning, []string{}) + return nil + } + + // Embedded AS configured but no upstreams: this is the misconfiguration + // that silently evaluates policies against the AS-issued token. + if len(vmcp.Spec.AuthServerConfig.UpstreamProviders) == 0 { + statusManager.RemoveConditionsWithPrefix(mcpv1beta1.ConditionTypeAuthzUpstreamSelectionWarning, []string{}) + + // User-facing message includes full remediation guidance and ends with + // a period, matching other validator messages. The returned error uses + // a trimmed form without trailing punctuation to satisfy staticcheck. + message := "spec.authServerConfig is set but has no upstream providers, and " + + "spec.incomingAuth.authzConfig references claims. Cedar would evaluate " + + "against the ToolHive-issued AS token rather than the upstream IDP token. " + + "Configure spec.authServerConfig.upstreamProviders with at least one " + + "upstream IDP, or remove authServerConfig if clients will present IdP " + + "tokens directly." + + ctxLogger := log.FromContext(ctx) + ctxLogger.Info("authz configured without an upstream IDP; rejecting VirtualMCPServer", + "name", vmcp.Name, + "namespace", vmcp.Namespace, + "reason", mcpv1beta1.ConditionReasonAuthzRequiresUpstream, + ) + + statusManager.SetPhase(mcpv1beta1.VirtualMCPServerPhaseFailed) + statusManager.SetMessage(message) + statusManager.SetAuthServerConfigValidatedCondition( + mcpv1beta1.ConditionReasonAuthzRequiresUpstream, + message, + metav1.ConditionFalse, + ) + statusManager.SetObservedGeneration(vmcp.Generation) + return stderrors.New("authz configured without an upstream IDP") + } + + // Valid configuration. When multiple upstreams are declared, surface an + // advisory naming the auto-selected upstream; otherwise ensure any stale + // warning is cleared. + if len(vmcp.Spec.AuthServerConfig.UpstreamProviders) > 1 { + selected := vmcp.Spec.AuthServerConfig.UpstreamProviders[0].Name + statusManager.SetCondition( + mcpv1beta1.ConditionTypeAuthzUpstreamSelectionWarning, + mcpv1beta1.ConditionReasonAuthzUpstreamAutoSelected, + fmt.Sprintf( + "multiple upstreamProviders configured; Cedar policies will evaluate "+ + "claims from the first upstream (%q). If another upstream should be "+ + "authoritative, remove or reorder the list.", + selected, + ), + metav1.ConditionTrue, + ) + } else { + statusManager.RemoveConditionsWithPrefix(mcpv1beta1.ConditionTypeAuthzUpstreamSelectionWarning, []string{}) + } + + return nil +} + // handleSpecValidationError checks whether err is a SpecValidationError (user must fix the spec). // If so, it applies the already-set status conditions and returns nil (no requeue). // Otherwise it returns the original error unchanged for normal requeue handling. diff --git a/cmd/thv-operator/controllers/virtualmcpserver_controller_test.go b/cmd/thv-operator/controllers/virtualmcpserver_controller_test.go index 97626acbfa..2f79a9d235 100644 --- a/cmd/thv-operator/controllers/virtualmcpserver_controller_test.go +++ b/cmd/thv-operator/controllers/virtualmcpserver_controller_test.go @@ -3476,3 +3476,250 @@ func TestDiscoveredRBACRulesIncludeMCPServerEntries(t *testing.T) { } assert.True(t, foundMCPServerEntries, "vmcpDiscoveredRBACRules should include mcpserverentries") } + +// TestVirtualMCPServerValidateAuthzUpstreamAvailable verifies that the +// validator fires only when the embedded AuthServer is configured without any +// upstream providers alongside AuthzConfig. Direct-IdP flows (clients present +// an already-validated IdP token) leave AuthServerConfig nil and are valid — +// Cedar evaluates against the identity's claims via the default branch. +// +// The validator also emits an advisory AuthzUpstreamSelectionWarning condition +// when multiple upstreams are declared, naming the auto-selected provider. +func TestVirtualMCPServerValidateAuthzUpstreamAvailable(t *testing.T) { + t.Parallel() + + inlineAuthzRef := &mcpv1beta1.AuthzConfigRef{ + Type: "inline", + Inline: &mcpv1beta1.InlineAuthzConfig{ + Policies: []string{`permit(principal, action, resource);`}, + }, + } + + // warningExpectation captures the expected state of the advisory + // AuthzUpstreamSelectionWarning condition after validation. When + // expectPresent is false the condition must not appear in status at + // all — the advisory only applies to the narrow multi-upstream slice. + type warningExpectation struct { + expectPresent bool + status metav1.ConditionStatus + reason string + messageSubstr string // empty when we don't care about the message + } + + tests := []struct { + name string + incomingAuth *mcpv1beta1.IncomingAuthConfig + authServerConfig *mcpv1beta1.EmbeddedAuthServerConfig + expectError bool + expectedReason string + expectedWarning warningExpectation + }{ + { + name: "no incoming auth is valid", + incomingAuth: nil, + expectedWarning: warningExpectation{expectPresent: false}, + }, + { + name: "incoming auth without authz is valid", + incomingAuth: &mcpv1beta1.IncomingAuthConfig{ + Type: "anonymous", + }, + expectedWarning: warningExpectation{expectPresent: false}, + }, + { + name: "authz with nil auth server config is valid (direct IdP flow)", + incomingAuth: &mcpv1beta1.IncomingAuthConfig{ + Type: "oidc", + AuthzConfig: inlineAuthzRef, + }, + authServerConfig: nil, + expectError: false, + expectedWarning: warningExpectation{expectPresent: false}, + }, + { + name: "authz with empty upstream providers is invalid", + incomingAuth: &mcpv1beta1.IncomingAuthConfig{ + Type: "oidc", + AuthzConfig: inlineAuthzRef, + }, + authServerConfig: &mcpv1beta1.EmbeddedAuthServerConfig{ + Issuer: "https://authserver.example.com", + UpstreamProviders: []mcpv1beta1.UpstreamProviderConfig{}, + }, + expectError: true, + expectedReason: mcpv1beta1.ConditionReasonAuthzRequiresUpstream, + expectedWarning: warningExpectation{expectPresent: false}, + }, + { + name: "authz with single upstream is valid", + incomingAuth: &mcpv1beta1.IncomingAuthConfig{ + Type: "oidc", + AuthzConfig: inlineAuthzRef, + }, + authServerConfig: &mcpv1beta1.EmbeddedAuthServerConfig{ + Issuer: "https://authserver.example.com", + UpstreamProviders: []mcpv1beta1.UpstreamProviderConfig{ + {Name: "okta", Type: mcpv1beta1.UpstreamProviderTypeOIDC}, + }, + }, + expectedWarning: warningExpectation{expectPresent: false}, + }, + { + name: "authz with multiple upstreams emits advisory warning", + incomingAuth: &mcpv1beta1.IncomingAuthConfig{ + Type: "oidc", + AuthzConfig: inlineAuthzRef, + }, + authServerConfig: &mcpv1beta1.EmbeddedAuthServerConfig{ + Issuer: "https://authserver.example.com", + UpstreamProviders: []mcpv1beta1.UpstreamProviderConfig{ + {Name: "okta", Type: mcpv1beta1.UpstreamProviderTypeOIDC}, + {Name: "entra", Type: mcpv1beta1.UpstreamProviderTypeOIDC}, + }, + }, + expectedWarning: warningExpectation{ + expectPresent: true, + status: metav1.ConditionTrue, + reason: mcpv1beta1.ConditionReasonAuthzUpstreamAutoSelected, + messageSubstr: `"okta"`, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + vmcp := &mcpv1beta1.VirtualMCPServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: testVmcpName, + Namespace: "default", + Generation: 1, + }, + Spec: mcpv1beta1.VirtualMCPServerSpec{ + GroupRef: &mcpv1beta1.MCPGroupRef{Name: testGroupName}, + IncomingAuth: tt.incomingAuth, + AuthServerConfig: tt.authServerConfig, + }, + } + + r := &VirtualMCPServerReconciler{} + statusManager := virtualmcpserverstatus.NewStatusManager(vmcp) + err := r.validateAuthzUpstreamAvailable(t.Context(), vmcp, statusManager) + + if tt.expectError { + require.Error(t, err) + // Error path writes phase, message, and the AuthServerConfigValidated + // condition — UpdateStatus must report a change. + assert.True(t, statusManager.UpdateStatus(t.Context(), &vmcp.Status)) + assert.Equal(t, mcpv1beta1.VirtualMCPServerPhaseFailed, vmcp.Status.Phase) + assert.NotEmpty(t, vmcp.Status.Message) + + found := false + for _, cond := range vmcp.Status.Conditions { + if cond.Type == mcpv1beta1.ConditionTypeAuthServerConfigValidated { + found = true + assert.Equal(t, metav1.ConditionFalse, cond.Status) + assert.Equal(t, tt.expectedReason, cond.Reason) + } + } + assert.True(t, found, "AuthServerConfigValidated condition should be set to False") + } else { + require.NoError(t, err) + // Positive path: apply any pending status changes (only the + // multi-upstream case emits the advisory; other valid paths + // leave the collector unchanged). + _ = statusManager.UpdateStatus(t.Context(), &vmcp.Status) + assert.NotEqual(t, mcpv1beta1.VirtualMCPServerPhaseFailed, vmcp.Status.Phase) + for _, cond := range vmcp.Status.Conditions { + if cond.Type == mcpv1beta1.ConditionTypeAuthServerConfigValidated { + assert.NotEqual(t, mcpv1beta1.ConditionReasonAuthzRequiresUpstream, cond.Reason) + } + } + } + + // The advisory AuthzUpstreamSelectionWarning condition should only + // appear on the narrow multi-upstream path. Every other path must + // leave it absent so kubectl describe stays clean. + var warning *metav1.Condition + for i := range vmcp.Status.Conditions { + if vmcp.Status.Conditions[i].Type == mcpv1beta1.ConditionTypeAuthzUpstreamSelectionWarning { + warning = &vmcp.Status.Conditions[i] + break + } + } + if !tt.expectedWarning.expectPresent { + assert.Nil(t, warning, "AuthzUpstreamSelectionWarning condition should not be present") + return + } + require.NotNil(t, warning, "AuthzUpstreamSelectionWarning condition should be present") + assert.Equal(t, tt.expectedWarning.status, warning.Status) + assert.Equal(t, tt.expectedWarning.reason, warning.Reason) + if tt.expectedWarning.messageSubstr != "" { + assert.Contains(t, warning.Message, tt.expectedWarning.messageSubstr) + } + }) + } +} + +// TestVirtualMCPServerValidateAuthzUpstreamAvailable_ClearsStaleWarning verifies +// the transition case: a VMCP that was previously multi-upstream (advisory True +// on its status) is reconfigured to a single upstream, and the stale advisory +// condition must be removed after the next validation pass. +func TestVirtualMCPServerValidateAuthzUpstreamAvailable_ClearsStaleWarning(t *testing.T) { + t.Parallel() + + inlineAuthzRef := &mcpv1beta1.AuthzConfigRef{ + Type: "inline", + Inline: &mcpv1beta1.InlineAuthzConfig{ + Policies: []string{`permit(principal, action, resource);`}, + }, + } + + vmcp := &mcpv1beta1.VirtualMCPServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: testVmcpName, + Namespace: "default", + Generation: 2, + }, + Spec: mcpv1beta1.VirtualMCPServerSpec{ + GroupRef: &mcpv1beta1.MCPGroupRef{Name: testGroupName}, + IncomingAuth: &mcpv1beta1.IncomingAuthConfig{ + Type: "oidc", + AuthzConfig: inlineAuthzRef, + }, + // Single upstream now — the advisory should be cleared. + AuthServerConfig: &mcpv1beta1.EmbeddedAuthServerConfig{ + Issuer: "https://authserver.example.com", + UpstreamProviders: []mcpv1beta1.UpstreamProviderConfig{ + {Name: "okta", Type: mcpv1beta1.UpstreamProviderTypeOIDC}, + }, + }, + }, + Status: mcpv1beta1.VirtualMCPServerStatus{ + // Simulate a stale True advisory from a previous multi-upstream + // reconciliation. + Conditions: []metav1.Condition{ + { + Type: mcpv1beta1.ConditionTypeAuthzUpstreamSelectionWarning, + Status: metav1.ConditionTrue, + Reason: mcpv1beta1.ConditionReasonAuthzUpstreamAutoSelected, + Message: `multiple upstreamProviders configured; Cedar policies will evaluate claims from the first upstream ("okta").`, + }, + }, + }, + } + + r := &VirtualMCPServerReconciler{} + statusManager := virtualmcpserverstatus.NewStatusManager(vmcp) + require.NoError(t, r.validateAuthzUpstreamAvailable(t.Context(), vmcp, statusManager)) + + // Applying the status should remove the stale condition. + assert.True(t, statusManager.UpdateStatus(t.Context(), &vmcp.Status), + "UpdateStatus must report a change because a stale condition was removed") + + for _, cond := range vmcp.Status.Conditions { + assert.NotEqual(t, mcpv1beta1.ConditionTypeAuthzUpstreamSelectionWarning, cond.Type, + "stale AuthzUpstreamSelectionWarning condition should have been removed") + } +} diff --git a/cmd/thv-operator/pkg/vmcpconfig/converter.go b/cmd/thv-operator/pkg/vmcpconfig/converter.go index 18473d11f5..cd31af6d69 100644 --- a/cmd/thv-operator/pkg/vmcpconfig/converter.go +++ b/cmd/thv-operator/pkg/vmcpconfig/converter.go @@ -190,6 +190,19 @@ func (c *Converter) convertIncomingAuth( incoming.Authz.Policies = vmcp.Spec.IncomingAuth.AuthzConfig.Inline.Policies } // TODO: Load policies from ConfigMap if Type is "configMap" + + // When an embedded auth server with upstream providers is configured, Cedar + // policies must evaluate claims from the upstream IDP token rather than the + // ToolHive-issued AS token. Mirrors injectSubjectProviderIfNeeded in + // virtualmcpserver_controller.go (outgoing auth) and + // injectUpstreamProviderIfNeeded in pkg/runner/middleware.go (thv run path). + // Leaving PrimaryUpstreamProvider empty (no embedded AS or no upstreams) lets + // Cedar fall back to claims from the ToolHive-issued token. + if vmcp.Spec.AuthServerConfig != nil && len(vmcp.Spec.AuthServerConfig.UpstreamProviders) > 0 { + incoming.Authz.PrimaryUpstreamProvider = authserver.ResolveUpstreamName( + vmcp.Spec.AuthServerConfig.UpstreamProviders[0].Name, + ) + } } return incoming, nil diff --git a/cmd/thv-operator/pkg/vmcpconfig/converter_test.go b/cmd/thv-operator/pkg/vmcpconfig/converter_test.go index f0f47d245b..cee72256af 100644 --- a/cmd/thv-operator/pkg/vmcpconfig/converter_test.go +++ b/cmd/thv-operator/pkg/vmcpconfig/converter_test.go @@ -1893,3 +1893,131 @@ func TestConverter_TelemetryConfigRef(t *testing.T) { assert.True(t, config.Telemetry.TracingEnabled, "Tracing should be enabled from MCPTelemetryConfig") assert.True(t, config.Telemetry.MetricsEnabled, "Metrics should be enabled from MCPTelemetryConfig") } + +// TestConvertIncomingAuth_PrimaryUpstreamProvider verifies that convertIncomingAuth +// propagates the first configured upstream provider name into AuthzConfig so Cedar +// evaluates claims from the upstream IDP token rather than the ToolHive-issued +// AS token. Without this, policies referencing upstream claims (e.g. "department") +// fail at runtime because Cedar reads the wrong token. +func TestConvertIncomingAuth_PrimaryUpstreamProvider(t *testing.T) { + t.Parallel() + + inlineAuthzRef := &mcpv1beta1.AuthzConfigRef{ + Type: "inline", + Inline: &mcpv1beta1.InlineAuthzConfig{ + Policies: []string{`permit(principal, action, resource);`}, + }, + } + + tests := []struct { + name string + authServerConfig *mcpv1beta1.EmbeddedAuthServerConfig + authzConfig *mcpv1beta1.AuthzConfigRef + expectAuthzNil bool + expectedProvider string + }{ + { + name: "no auth server leaves provider unset", + authServerConfig: nil, + authzConfig: inlineAuthzRef, + expectedProvider: "", + }, + { + name: "auth server with empty upstream list leaves provider unset", + authServerConfig: &mcpv1beta1.EmbeddedAuthServerConfig{ + Issuer: "https://authserver.example.com", + UpstreamProviders: []mcpv1beta1.UpstreamProviderConfig{}, + }, + authzConfig: inlineAuthzRef, + expectedProvider: "", + }, + { + name: "single named upstream becomes primary", + authServerConfig: &mcpv1beta1.EmbeddedAuthServerConfig{ + Issuer: "https://authserver.example.com", + UpstreamProviders: []mcpv1beta1.UpstreamProviderConfig{ + {Name: "okta", Type: mcpv1beta1.UpstreamProviderTypeOIDC}, + }, + }, + authzConfig: inlineAuthzRef, + expectedProvider: "okta", + }, + { + name: "empty upstream name resolves to default", + authServerConfig: &mcpv1beta1.EmbeddedAuthServerConfig{ + Issuer: "https://authserver.example.com", + UpstreamProviders: []mcpv1beta1.UpstreamProviderConfig{ + {Name: "", Type: mcpv1beta1.UpstreamProviderTypeOIDC}, + }, + }, + authzConfig: inlineAuthzRef, + expectedProvider: "default", + }, + { + name: "first upstream wins with multiple providers", + authServerConfig: &mcpv1beta1.EmbeddedAuthServerConfig{ + Issuer: "https://authserver.example.com", + UpstreamProviders: []mcpv1beta1.UpstreamProviderConfig{ + {Name: "okta", Type: mcpv1beta1.UpstreamProviderTypeOIDC}, + {Name: "github", Type: mcpv1beta1.UpstreamProviderTypeOAuth2}, + {Name: "google", Type: mcpv1beta1.UpstreamProviderTypeOIDC}, + }, + }, + authzConfig: inlineAuthzRef, + expectedProvider: "okta", + }, + { + name: "no authz config leaves Authz nil without panic", + authServerConfig: &mcpv1beta1.EmbeddedAuthServerConfig{ + Issuer: "https://authserver.example.com", + UpstreamProviders: []mcpv1beta1.UpstreamProviderConfig{ + {Name: "okta", Type: mcpv1beta1.UpstreamProviderTypeOIDC}, + }, + }, + authzConfig: nil, + expectAuthzNil: true, + }, + { + // Direct-IdP flow with anonymous incoming auth: neither the embedded + // AS nor authz is configured. Converter must not panic and must leave + // Authz unset. + name: "both auth server and authz nil leaves Authz nil without panic", + authServerConfig: nil, + authzConfig: nil, + expectAuthzNil: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + converter := newTestConverter(t, newNoOpMockResolver(t)) + + vmcp := &mcpv1beta1.VirtualMCPServer{ + ObjectMeta: metav1.ObjectMeta{Name: "test-vmcp", Namespace: "default"}, + Spec: mcpv1beta1.VirtualMCPServerSpec{ + GroupRef: &mcpv1beta1.MCPGroupRef{Name: "test-group"}, + IncomingAuth: &mcpv1beta1.IncomingAuthConfig{ + Type: "anonymous", + AuthzConfig: tt.authzConfig, + }, + AuthServerConfig: tt.authServerConfig, + }, + } + + ctx := log.IntoContext(t.Context(), logr.Discard()) + incoming, err := converter.convertIncomingAuth(ctx, vmcp) + require.NoError(t, err) + require.NotNil(t, incoming) + + if tt.expectAuthzNil { + assert.Nil(t, incoming.Authz) + return + } + + require.NotNil(t, incoming.Authz) + assert.Equal(t, tt.expectedProvider, incoming.Authz.PrimaryUpstreamProvider) + }) + } +} diff --git a/pkg/authz/integration_test.go b/pkg/authz/integration_test.go index 96ea1438ea..79ca874820 100644 --- a/pkg/authz/integration_test.go +++ b/pkg/authz/integration_test.go @@ -1070,3 +1070,120 @@ func TestIntegrationUpstreamProviderGroupAuth(t *testing.T) { }) } } + +// TestIntegrationPrimaryUpstreamProviderClaimAttributeAccess verifies the +// behavioral effect of the VirtualMCPServer operator converter fix in +// stacklok/toolhive#4997: when PrimaryUpstreamProvider is populated, Cedar +// reads scalar JWT claim attributes (e.g. `email`) from the upstream IDP token +// rather than the ToolHive-issued AS token. +// +// Prior to the fix the operator left PrimaryUpstreamProvider empty, so Cedar +// evaluated policies against the AS-issued token's claims — which do not carry +// upstream-provider attributes such as `email`. Policies referencing those +// attributes (via `has` or `==`) failed with Cedar's +// "does not have the attribute" error. This test pins the two branches of +// cedar.Authorizer.resolveClaims (see core.go:421) against the same identity +// and policy set, demonstrating that only the upstream-provider branch admits +// the reproducer policy from #4997. +func TestIntegrationPrimaryUpstreamProviderClaimAttributeAccess(t *testing.T) { + t.Parallel() + + const providerName = "okta" + + // AS-issued token claims: realistic ToolHive-AS fields, deliberately + // without `email`. This is what Cedar sees when PrimaryUpstreamProvider + // is empty (the pre-fix behavior). + directClaims := jwt.MapClaims{ + "sub": "thv-as|alice", + "aud": "toolhive-vmcp", + "iss": "https://thv-as.example.com/", + "tsid": "sess-abc123", + } + + // Upstream IDP token: carries the `email` claim that the policy inspects. + upstreamToken := makeUnsignedJWT(t, jwt.MapClaims{ + "sub": "okta|alice", + "email": "alice@example.com", + }) + + // Using has-attribute rather than equality so failure uniquely signals + // "the claim source Cedar read from lacked this attribute", which is + // exactly the #4997 regression surface. + policies := []string{ + `permit(principal, action == Action::"call_tool", resource) when { principal has claim_email };`, + } + + tests := []struct { + name string + primaryUpstreamProvider string + expectAllowed bool + }{ + { + // With the #4997 fix: converter populates PrimaryUpstreamProvider, + // Cedar reads the upstream token which has `email`, policy permits. + name: "upstream_provider_set_reads_upstream_claim_and_permits", + primaryUpstreamProvider: providerName, + expectAllowed: true, + }, + { + // Pre-fix behavior: PrimaryUpstreamProvider empty, Cedar falls + // back to direct claims which lack `email`, policy denies with + // Cedar's "does not have the attribute claim_email" error. + name: "upstream_provider_empty_falls_back_to_direct_claims_and_denies", + primaryUpstreamProvider: "", + expectAllowed: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + authorizer, err := cedar.NewCedarAuthorizer(cedar.ConfigOptions{ + Policies: policies, + EntitiesJSON: "[]", + PrimaryUpstreamProvider: tt.primaryUpstreamProvider, + }, "") + require.NoError(t, err) + + params, err := json.Marshal(map[string]interface{}{"name": "deploy", "arguments": map[string]interface{}{}}) + require.NoError(t, err) + callReq, err := jsonrpc2.NewCall(jsonrpc2.Int64ID(1), string(mcp.MethodToolsCall), json.RawMessage(params)) + require.NoError(t, err) + reqJSON, err := jsonrpc2.EncodeMessage(callReq) + require.NoError(t, err) + + httpReq, err := http.NewRequest(http.MethodPost, "/messages", bytes.NewBuffer(reqJSON)) + require.NoError(t, err) + httpReq.Header.Set("Content-Type", "application/json") + + identity := &auth.Identity{ + PrincipalInfo: auth.PrincipalInfo{ + Subject: directClaims["sub"].(string), + Claims: directClaims, + }, + UpstreamTokens: map[string]string{providerName: upstreamToken}, + } + httpReq = httpReq.WithContext(auth.WithIdentity(httpReq.Context(), identity)) + + rr := httptest.NewRecorder() + var handlerCalled bool + mockHandler := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + handlerCalled = true + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{"result": "ok"}`)) + }) + + middleware := mcpparser.ParsingMiddleware(Middleware(authorizer, mockHandler, nil)) + middleware.ServeHTTP(rr, httpReq) + + if tt.expectAllowed { + assert.Equal(t, http.StatusOK, rr.Code) + assert.True(t, handlerCalled) + } else { + assert.Equal(t, http.StatusForbidden, rr.Code) + assert.False(t, handlerCalled) + } + }) + } +}