Fix Cedar upstream-claim evaluation on VirtualMCPServer

cmd/thv-operator/api/v1beta1/virtualmcpserver_types.go

@@ -346,6 +346,13 @@ 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"
+
 	// ConditionReasonVirtualMCPServerTelemetryConfigRefValid indicates the referenced MCPTelemetryConfig is valid
 	ConditionReasonVirtualMCPServerTelemetryConfigRefValid = "TelemetryConfigRefValid"
 

cmd/thv-operator/controllers/virtualmcpserver_controller.go

@@ -352,23 +352,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(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 +498,45 @@ func (*VirtualMCPServerReconciler) validateAuthServerConfig(
 	return nil
 }
 
+// validateAuthzUpstreamAvailable ensures that when authorization policies are
+// configured via IncomingAuth.AuthzConfig, an upstream IDP is available to
+// source claims from. Without an upstream, Cedar evaluates claim references
+// (e.g. principal.claim_department) against the ToolHive-issued AS token,
+// whose claim namespace (sub, aud, tsid) can overlap upstream claims and
+// silently authorize against the wrong identity.
+//
+// This is a belt-and-suspenders check that complements the converter's
+// PrimaryUpstreamProvider population: when the user omits AuthServerConfig or
+// leaves UpstreamProviders empty but still sets AuthzConfig, surface a
+// misconfiguration status condition instead of silently deploying.
+func (*VirtualMCPServerReconciler) validateAuthzUpstreamAvailable(
+	vmcp *mcpv1beta1.VirtualMCPServer,
+	statusManager virtualmcpserverstatus.StatusManager,
+) error {
+	if vmcp.Spec.IncomingAuth == nil || vmcp.Spec.IncomingAuth.AuthzConfig == nil {
+		return nil
+	}
+	if vmcp.Spec.AuthServerConfig != nil && len(vmcp.Spec.AuthServerConfig.UpstreamProviders) > 0 {
+		return nil
+	}
+
+	message := "spec.incomingAuth.authzConfig is set but no upstream IDP is configured: " +
+		"Cedar policies referencing principal.claim_* would evaluate against the " +
+		"ToolHive-issued token instead of the upstream IDP token. Configure " +
+		"spec.authServerConfig.upstreamProviders with at least one upstream IDP."
+
+	statusManager.SetPhase(mcpv1beta1.VirtualMCPServerPhaseFailed)
+	statusManager.SetMessage(message)
+	statusManager.SetAuthServerConfigValidatedCondition(
+		mcpv1beta1.ConditionReasonAuthzRequiresUpstream,
+		message,
+		metav1.ConditionFalse,
+	)
+	statusManager.SetObservedGeneration(vmcp.Generation)
+
+	return fmt.Errorf("%s", message)
+}
+
 // 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.

cmd/thv-operator/controllers/virtualmcpserver_controller_test.go

@@ -3476,3 +3476,150 @@ func TestDiscoveredRBACRulesIncludeMCPServerEntries(t *testing.T) {
 	}
 	assert.True(t, foundMCPServerEntries, "vmcpDiscoveredRBACRules should include mcpserverentries")
 }
+
+// TestVirtualMCPServerValidateAuthzUpstreamAvailable verifies that the
+// validator surfaces a misconfiguration when AuthzConfig is set but no
+// upstream IDP is available to source claims from. Without this check,
+// Cedar silently evaluates policies against the ToolHive-issued AS token,
+// whose claim namespace (sub, aud, tsid) can overlap upstream claims and
+// authorize against the wrong identity.
+func TestVirtualMCPServerValidateAuthzUpstreamAvailable(t *testing.T) {
+	t.Parallel()
+
+	inlineAuthzRef := &mcpv1beta1.AuthzConfigRef{
+		Type: "inline",
+		Inline: &mcpv1beta1.InlineAuthzConfig{
+			Policies: []string{`permit(principal, action, resource);`},
+		},
+	}
+
+	tests := []struct {
+		name             string
+		incomingAuth     *mcpv1beta1.IncomingAuthConfig
+		authServerConfig *mcpv1beta1.EmbeddedAuthServerConfig
+		expectError      bool
+		expectedReason   string
+	}{
+		{
+			name:         "no incoming auth is valid",
+			incomingAuth: nil,
+		},
+		{
+			name: "incoming auth without authz is valid",
+			incomingAuth: &mcpv1beta1.IncomingAuthConfig{
+				Type: "anonymous",
+			},
+		},
+		{
+			name: "authz with nil auth server config is invalid",
+			incomingAuth: &mcpv1beta1.IncomingAuthConfig{
+				Type:        "oidc",
+				AuthzConfig: inlineAuthzRef,
+			},
+			authServerConfig: nil,
+			expectError:      true,
+			expectedReason:   mcpv1beta1.ConditionReasonAuthzRequiresUpstream,
+		},
+		{
+			name: "anonymous incoming auth with authz and no upstream is invalid",
+			incomingAuth: &mcpv1beta1.IncomingAuthConfig{
+				Type:        "anonymous",
+				AuthzConfig: inlineAuthzRef,
+			},
+			authServerConfig: nil,
+			expectError:      true,
+			expectedReason:   mcpv1beta1.ConditionReasonAuthzRequiresUpstream,
+		},
+		{
+			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,
+		},
+		{
+			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},
+				},
+			},
+		},
+		{
+			name: "authz with multiple upstreams 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},
+					{Name: "entra", Type: mcpv1beta1.UpstreamProviderTypeOIDC},
+				},
+			},
+		},
+	}
+
+	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(vmcp, statusManager)
+			_ = statusManager.UpdateStatus(context.Background(), &vmcp.Status)
+
+			if tt.expectError {
+				require.Error(t, err)
+				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)
+				// The validator does not set a positive condition when valid;
+				// it only surfaces the negative case. Make sure it did not
+				// spuriously mark the server failed.
+				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)
+					}
+				}
+			}
+		})
+	}
+}

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