From 9344c6856fdf19c70fd342a444931d0666e64749 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Wed, 22 Apr 2026 12:30:18 +0100 Subject: [PATCH 1/4] Populate Cedar PrimaryUpstreamProvider for vMCP VirtualMCPServer's operator converter never set AuthzConfig.PrimaryUpstreamProvider, so Cedar policies that referenced upstream claims (e.g. principal.claim_department) failed at runtime. Cedar evaluated against the ToolHive-issued AS token rather than the upstream IDP token, and the claim was missing. Derive the field from Spec.AuthServerConfig.UpstreamProviders[0].Name in convertIncomingAuth when an embedded auth server with upstream providers is configured. Mirrors injectSubjectProviderIfNeeded in virtualmcpserver_controller.go (outgoing auth) and injectUpstreamProviderIfNeeded in pkg/runner/middleware.go (thv run path). Leaves the field empty when no embedded AS or no upstreams so Cedar correctly falls back to ToolHive-issued claims in those modes. Fixes #4997 --- cmd/thv-operator/pkg/vmcpconfig/converter.go | 13 ++ .../pkg/vmcpconfig/converter_test.go | 119 ++++++++++++++++++ 2 files changed, 132 insertions(+) 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..53c2823175 100644 --- a/cmd/thv-operator/pkg/vmcpconfig/converter_test.go +++ b/cmd/thv-operator/pkg/vmcpconfig/converter_test.go @@ -1893,3 +1893,122 @@ 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, + }, + } + + 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(context.Background(), 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) + }) + } +} From ff5e912732d3d18fe1fe363b1e2dc262936cfe2b Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Wed, 22 Apr 2026 12:58:30 +0100 Subject: [PATCH 2/4] Reject VirtualMCPServer authz without an upstream IDP When IncomingAuth.AuthzConfig is set but no upstream IDP is configured, Cedar silently evaluates policies against the ToolHive-issued AS token. That token's claim namespace (sub, aud, tsid) can overlap upstream claims and authorize against the wrong identity, so the misconfig must be surfaced rather than deployed. Add validateAuthzUpstreamAvailable to the VirtualMCPServer reconciler chain. When AuthzConfig is set but AuthServerConfig is nil or UpstreamProviders is empty, mark the server Failed and set AuthServerConfigValidated=False with reason AuthzRequiresUpstream. The user-facing message points at spec.authServerConfig.upstreamProviders, which is where the fix belongs. Extract runAuthValidations from runValidations so the auth-related checks live together and gocyclo stays happy. No behavior change in the moved block. Belt-and-suspenders companion to the converter fix in the previous commit: the converter wires the provider name when upstreams exist; this validator makes the absence of upstreams an explicit failure. Refs #4997 --- .../api/v1beta1/virtualmcpserver_types.go | 7 + .../virtualmcpserver_controller.go | 77 ++++++++- .../virtualmcpserver_controller_test.go | 147 ++++++++++++++++++ 3 files changed, 227 insertions(+), 4 deletions(-) diff --git a/cmd/thv-operator/api/v1beta1/virtualmcpserver_types.go b/cmd/thv-operator/api/v1beta1/virtualmcpserver_types.go index 6a9f72b3d9..33eccc8a2e 100644 --- a/cmd/thv-operator/api/v1beta1/virtualmcpserver_types.go +++ b/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" diff --git a/cmd/thv-operator/controllers/virtualmcpserver_controller.go b/cmd/thv-operator/controllers/virtualmcpserver_controller.go index 3baf42eed9..ca0726059e 100644 --- a/cmd/thv-operator/controllers/virtualmcpserver_controller.go +++ b/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. diff --git a/cmd/thv-operator/controllers/virtualmcpserver_controller_test.go b/cmd/thv-operator/controllers/virtualmcpserver_controller_test.go index 97626acbfa..2ebfba8533 100644 --- a/cmd/thv-operator/controllers/virtualmcpserver_controller_test.go +++ b/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) + } + } + } + }) + } +} From 1e3ffad269b72b60797593af1239877ccd64c837 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Wed, 22 Apr 2026 13:49:40 +0100 Subject: [PATCH 3/4] Verify Cedar reads upstream claims via PrimaryUpstreamProvider MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Behavioral regression guard for the converter fix in the preceding commit. With the same identity, upstream token, and Cedar policy, flip PrimaryUpstreamProvider between "okta" and "" and observe the outcome change from permit to deny through the real middleware stack. This pins the runtime contract below the operator-layer fix: when the converter populates PrimaryUpstreamProvider, Cedar resolves principal.claim_* from the upstream IDP token; when it is empty, Cedar falls back to the AS-issued token's claims (which do not carry upstream profile attributes). Uses has-attribute rather than equality so a failure uniquely signals "the claim source Cedar read from lacked this attribute" — the exact #4997 regression shape. Refs #4997 --- pkg/authz/integration_test.go | 117 ++++++++++++++++++++++++++++++++++ 1 file changed, 117 insertions(+) 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) + } + }) + } +} From 1b042372f1073ed461d8e1b550aeab76c0b6a984 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Wed, 22 Apr 2026 17:11:31 +0100 Subject: [PATCH 4/4] Address review feedback on #5002 Key change: narrow validateAuthzUpstreamAvailable to only reject when an embedded auth server is configured but has no upstream providers. Previously the validator rejected any authz configuration without an embedded auth server, which false-positived direct-IdP deployments where the client presents an Okta/Entra/etc token directly and Cedar evaluates against identity.Claims via the default branch. Thanks to Trey for catching this regression during review. Additional review feedback addressed: - Advisory status condition AuthzUpstreamSelectionWarning surfaces which upstream was auto-selected when multiple are configured (F2). Condition is set only on the applicable path; stale conditions are removed via RemoveConditionsWithPrefix on non-applicable paths so normal VMCPs do not carry a False/NotApplicable advisory row. - Use errors.New instead of fmt.Errorf("%s", msg) (F3). - Emit a WARN-equivalent log (logr.Info at V=0, matching the file idiom) when rejecting so operators have a grep-able signal (F5). - Update runValidations doc comment after the earlier runAuthValidations extraction (F7). - Assert UpdateStatus bool return in error-path test subtests (F9). - Add double-nil (AuthServerConfig nil and AuthzConfig nil) converter subtest (F12). - Switch new tests to t.Context() (F14). Refs #4997 --- .../api/v1beta1/virtualmcpserver_types.go | 11 ++ .../virtualmcpserver_controller.go | 107 +++++++++--- .../virtualmcpserver_controller_test.go | 156 ++++++++++++++---- .../pkg/vmcpconfig/converter_test.go | 11 +- 4 files changed, 230 insertions(+), 55 deletions(-) diff --git a/cmd/thv-operator/api/v1beta1/virtualmcpserver_types.go b/cmd/thv-operator/api/v1beta1/virtualmcpserver_types.go index 33eccc8a2e..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" ) @@ -353,6 +359,11 @@ const ( // 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 ca0726059e..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). @@ -391,7 +393,7 @@ func (r *VirtualMCPServerReconciler) runAuthValidations( // 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 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") } @@ -499,42 +501,95 @@ func (*VirtualMCPServerReconciler) validateAuthServerConfig( } // 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. +// 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. // -// 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. +// 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 } - if vmcp.Spec.AuthServerConfig != nil && len(vmcp.Spec.AuthServerConfig.UpstreamProviders) > 0 { + + // 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 } - 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." + // 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{}) - statusManager.SetPhase(mcpv1beta1.VirtualMCPServerPhaseFailed) - statusManager.SetMessage(message) - statusManager.SetAuthServerConfigValidatedCondition( - mcpv1beta1.ConditionReasonAuthzRequiresUpstream, - message, - metav1.ConditionFalse, - ) - statusManager.SetObservedGeneration(vmcp.Generation) + // 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." - return fmt.Errorf("%s", message) + 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). diff --git a/cmd/thv-operator/controllers/virtualmcpserver_controller_test.go b/cmd/thv-operator/controllers/virtualmcpserver_controller_test.go index 2ebfba8533..2f79a9d235 100644 --- a/cmd/thv-operator/controllers/virtualmcpserver_controller_test.go +++ b/cmd/thv-operator/controllers/virtualmcpserver_controller_test.go @@ -3478,11 +3478,13 @@ func TestDiscoveredRBACRulesIncludeMCPServerEntries(t *testing.T) { } // 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. +// 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() @@ -3493,42 +3495,46 @@ func TestVirtualMCPServerValidateAuthzUpstreamAvailable(t *testing.T) { }, } + // 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, + 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 invalid", + name: "authz with nil auth server config is valid (direct IdP flow)", 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, + expectError: false, + expectedWarning: warningExpectation{expectPresent: false}, }, { name: "authz with empty upstream providers is invalid", @@ -3540,8 +3546,9 @@ func TestVirtualMCPServerValidateAuthzUpstreamAvailable(t *testing.T) { Issuer: "https://authserver.example.com", UpstreamProviders: []mcpv1beta1.UpstreamProviderConfig{}, }, - expectError: true, - expectedReason: mcpv1beta1.ConditionReasonAuthzRequiresUpstream, + expectError: true, + expectedReason: mcpv1beta1.ConditionReasonAuthzRequiresUpstream, + expectedWarning: warningExpectation{expectPresent: false}, }, { name: "authz with single upstream is valid", @@ -3555,9 +3562,10 @@ func TestVirtualMCPServerValidateAuthzUpstreamAvailable(t *testing.T) { {Name: "okta", Type: mcpv1beta1.UpstreamProviderTypeOIDC}, }, }, + expectedWarning: warningExpectation{expectPresent: false}, }, { - name: "authz with multiple upstreams is valid", + name: "authz with multiple upstreams emits advisory warning", incomingAuth: &mcpv1beta1.IncomingAuthConfig{ Type: "oidc", AuthzConfig: inlineAuthzRef, @@ -3569,6 +3577,12 @@ func TestVirtualMCPServerValidateAuthzUpstreamAvailable(t *testing.T) { {Name: "entra", Type: mcpv1beta1.UpstreamProviderTypeOIDC}, }, }, + expectedWarning: warningExpectation{ + expectPresent: true, + status: metav1.ConditionTrue, + reason: mcpv1beta1.ConditionReasonAuthzUpstreamAutoSelected, + messageSubstr: `"okta"`, + }, }, } @@ -3591,11 +3605,13 @@ func TestVirtualMCPServerValidateAuthzUpstreamAvailable(t *testing.T) { r := &VirtualMCPServerReconciler{} statusManager := virtualmcpserverstatus.NewStatusManager(vmcp) - err := r.validateAuthzUpstreamAvailable(vmcp, statusManager) - _ = statusManager.UpdateStatus(context.Background(), &vmcp.Status) + 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) @@ -3610,9 +3626,10 @@ func TestVirtualMCPServerValidateAuthzUpstreamAvailable(t *testing.T) { 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. + // 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 { @@ -3620,6 +3637,89 @@ func TestVirtualMCPServerValidateAuthzUpstreamAvailable(t *testing.T) { } } } + + // 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_test.go b/cmd/thv-operator/pkg/vmcpconfig/converter_test.go index 53c2823175..cee72256af 100644 --- a/cmd/thv-operator/pkg/vmcpconfig/converter_test.go +++ b/cmd/thv-operator/pkg/vmcpconfig/converter_test.go @@ -1977,6 +1977,15 @@ func TestConvertIncomingAuth_PrimaryUpstreamProvider(t *testing.T) { 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 { @@ -1997,7 +2006,7 @@ func TestConvertIncomingAuth_PrimaryUpstreamProvider(t *testing.T) { }, } - ctx := log.IntoContext(context.Background(), logr.Discard()) + ctx := log.IntoContext(t.Context(), logr.Discard()) incoming, err := converter.convertIncomingAuth(ctx, vmcp) require.NoError(t, err) require.NotNil(t, incoming)