diff --git a/cmd/thv-operator/api/v1alpha1/mcpremoteproxy_types.go b/cmd/thv-operator/api/v1alpha1/mcpremoteproxy_types.go index d2c000ab2c..11cdc20513 100644 --- a/cmd/thv-operator/api/v1alpha1/mcpremoteproxy_types.go +++ b/cmd/thv-operator/api/v1alpha1/mcpremoteproxy_types.go @@ -268,6 +268,21 @@ const ( // ConditionReasonOIDCIssuerInvalid indicates the OIDC issuer URL is malformed ConditionReasonOIDCIssuerInvalid = "OIDCIssuerInvalid" + + // ConditionReasonAuthzPolicySyntaxInvalid indicates an inline Cedar policy has a syntax error + ConditionReasonAuthzPolicySyntaxInvalid = "AuthzPolicySyntaxInvalid" + + // ConditionReasonAuthzConfigMapNotFound indicates the referenced authz ConfigMap was not found + ConditionReasonAuthzConfigMapNotFound = "AuthzConfigMapNotFound" + + // ConditionReasonHeaderSecretNotFound indicates a referenced header Secret was not found + ConditionReasonHeaderSecretNotFound = "HeaderSecretNotFound" + + // ConditionReasonRemoteURLInvalid indicates the remoteURL is malformed or has an invalid scheme + ConditionReasonRemoteURLInvalid = "RemoteURLInvalid" + + // ConditionReasonJWKSURLInvalid indicates the JWKS URL is malformed or has an invalid scheme + ConditionReasonJWKSURLInvalid = "JWKSURLInvalid" ) //+kubebuilder:object:root=true diff --git a/cmd/thv-operator/controllers/mcpremoteproxy_controller.go b/cmd/thv-operator/controllers/mcpremoteproxy_controller.go index cc4e1fb9bf..a098dd02d0 100644 --- a/cmd/thv-operator/controllers/mcpremoteproxy_controller.go +++ b/cmd/thv-operator/controllers/mcpremoteproxy_controller.go @@ -7,6 +7,7 @@ package controllers import ( "context" + stderrors "errors" "fmt" "maps" "reflect" @@ -335,29 +336,49 @@ func (r *MCPRemoteProxyReconciler) ensureServiceURL(ctx context.Context, proxy * // validateSpec validates the MCPRemoteProxy spec func (r *MCPRemoteProxyReconciler) validateSpec(ctx context.Context, proxy *mcpv1alpha1.MCPRemoteProxy) error { - if proxy.Spec.RemoteURL == "" { - return fmt.Errorf("remoteURL is required") - } - // Validate external auth config if referenced if proxy.Spec.ExternalAuthConfigRef != nil { externalAuthConfig, err := ctrlutil.GetExternalAuthConfigForMCPRemoteProxy(ctx, r.Client, proxy) if err != nil { - return fmt.Errorf("failed to validate external auth config: %w", err) + return r.failValidation(proxy, + mcpv1alpha1.ConditionReasonMCPRemoteProxyExternalAuthConfigFetchError, + fmt.Errorf("failed to validate external auth config: %w", err), + ) } if externalAuthConfig == nil { - return fmt.Errorf("referenced MCPExternalAuthConfig %s not found", proxy.Spec.ExternalAuthConfigRef.Name) + return r.failValidation(proxy, + mcpv1alpha1.ConditionReasonMCPRemoteProxyExternalAuthConfigNotFound, + fmt.Errorf("referenced MCPExternalAuthConfig %s not found", proxy.Spec.ExternalAuthConfigRef.Name), + ) } } + // Validate remote URL format (also rejects empty URLs) + if err := validation.ValidateRemoteURL(proxy.Spec.RemoteURL); err != nil { + return r.failValidation(proxy, mcpv1alpha1.ConditionReasonRemoteURLInvalid, err) + } + // Validate OIDC issuer URL scheme if err := r.validateOIDCIssuerURL(proxy); err != nil { reason := mcpv1alpha1.ConditionReasonOIDCIssuerInvalid if strings.Contains(err.Error(), "HTTP scheme") { reason = mcpv1alpha1.ConditionReasonOIDCIssuerInsecure } - r.recordValidationEvent(proxy, reason, err.Error()) - setConfigurationInvalidCondition(proxy, reason, err.Error()) + return r.failValidation(proxy, reason, err) + } + + // Validate JWKS URL format + if err := r.validateJWKSURL(proxy); err != nil { + return r.failValidation(proxy, mcpv1alpha1.ConditionReasonJWKSURLInvalid, err) + } + + // Validate inline Cedar policy syntax + if err := r.validateAuthzPolicySyntax(proxy); err != nil { + return r.failValidation(proxy, mcpv1alpha1.ConditionReasonAuthzPolicySyntaxInvalid, err) + } + + // Validate Kubernetes resource references (ConfigMaps, Secrets) + if err := r.validateK8sRefs(ctx, proxy); err != nil { return err } @@ -373,6 +394,14 @@ func (r *MCPRemoteProxyReconciler) validateSpec(ctx context.Context, proxy *mcpv return nil } +// failValidation records a validation event, sets the ConfigurationValid condition to False, +// and returns the error. This consolidates the repeated validate → event → condition → return pattern. +func (r *MCPRemoteProxyReconciler) failValidation(proxy *mcpv1alpha1.MCPRemoteProxy, reason string, err error) error { + r.recordValidationEvent(proxy, reason, err.Error()) + setConfigurationInvalidCondition(proxy, reason, err.Error()) + return err +} + // recordValidationEvent emits a Warning event for a validation failure. func (r *MCPRemoteProxyReconciler) recordValidationEvent(proxy *mcpv1alpha1.MCPRemoteProxy, reason, message string) { if r.Recorder != nil { @@ -409,6 +438,117 @@ func (*MCPRemoteProxyReconciler) validateOIDCIssuerURL(proxy *mcpv1alpha1.MCPRem return nil } +// validateJWKSURL validates the JWKS URL scheme in the OIDC config. +func (*MCPRemoteProxyReconciler) validateJWKSURL(proxy *mcpv1alpha1.MCPRemoteProxy) error { + oidcConfig := proxy.Spec.OIDCConfig + + switch oidcConfig.Type { + case mcpv1alpha1.OIDCConfigTypeInline: + if oidcConfig.Inline != nil { + return validation.ValidateJWKSURL(oidcConfig.Inline.JWKSURL) + } + case mcpv1alpha1.OIDCConfigTypeKubernetes: + if oidcConfig.Kubernetes != nil { + return validation.ValidateJWKSURL(oidcConfig.Kubernetes.JWKSURL) + } + } + return nil +} + +// validateAuthzPolicySyntax validates inline Cedar authorization policy syntax. +func (*MCPRemoteProxyReconciler) validateAuthzPolicySyntax( + proxy *mcpv1alpha1.MCPRemoteProxy, +) error { + if proxy.Spec.AuthzConfig == nil || + proxy.Spec.AuthzConfig.Type != mcpv1alpha1.AuthzConfigTypeInline || + proxy.Spec.AuthzConfig.Inline == nil { + return nil + } + return validation.ValidateCedarPolicies(proxy.Spec.AuthzConfig.Inline.Policies) +} + +// validateK8sRefs validates that referenced ConfigMaps and Secrets exist. +func (r *MCPRemoteProxyReconciler) validateK8sRefs( + ctx context.Context, proxy *mcpv1alpha1.MCPRemoteProxy, +) error { + // Check authz ConfigMap reference + if proxy.Spec.AuthzConfig != nil && + proxy.Spec.AuthzConfig.Type == mcpv1alpha1.AuthzConfigTypeConfigMap && + proxy.Spec.AuthzConfig.ConfigMap != nil { + cm := &corev1.ConfigMap{} + cmName := proxy.Spec.AuthzConfig.ConfigMap.Name + err := r.Get(ctx, types.NamespacedName{ + Name: cmName, Namespace: proxy.Namespace, + }, cm) + if err != nil { + if errors.IsNotFound(err) { + msg := fmt.Sprintf( + "authorization ConfigMap %q not found in namespace %q", + cmName, proxy.Namespace, + ) + r.recordValidationEvent( + proxy, + mcpv1alpha1.ConditionReasonAuthzConfigMapNotFound, + msg, + ) + setConfigurationInvalidCondition( + proxy, + mcpv1alpha1.ConditionReasonAuthzConfigMapNotFound, + msg, + ) + return stderrors.New(msg) + } + ctxLogger := log.FromContext(ctx) + ctxLogger.Error(err, "Failed to fetch authorization ConfigMap", "name", cmName, "namespace", proxy.Namespace) + genericMsg := fmt.Sprintf("failed to fetch authorization ConfigMap %q", cmName) + r.recordValidationEvent(proxy, mcpv1alpha1.ConditionReasonAuthzConfigMapNotFound, genericMsg) + setConfigurationInvalidCondition(proxy, mcpv1alpha1.ConditionReasonAuthzConfigMapNotFound, genericMsg) + return stderrors.New(genericMsg) + } + } + + // Check header Secret references + if proxy.Spec.HeaderForward != nil { + for _, headerRef := range proxy.Spec.HeaderForward.AddHeadersFromSecret { + if headerRef.ValueSecretRef == nil { + continue + } + secret := &corev1.Secret{} + secretName := headerRef.ValueSecretRef.Name + err := r.Get(ctx, types.NamespacedName{ + Name: secretName, Namespace: proxy.Namespace, + }, secret) + if err != nil { + if errors.IsNotFound(err) { + msg := fmt.Sprintf( + "secret %q referenced for header %q not found in namespace %q", + secretName, headerRef.HeaderName, proxy.Namespace, + ) + r.recordValidationEvent( + proxy, + mcpv1alpha1.ConditionReasonHeaderSecretNotFound, + msg, + ) + setConfigurationInvalidCondition( + proxy, + mcpv1alpha1.ConditionReasonHeaderSecretNotFound, + msg, + ) + return stderrors.New(msg) + } + ctxLogger := log.FromContext(ctx) + ctxLogger.Error(err, "Failed to fetch secret", "name", secretName, "namespace", proxy.Namespace) + genericMsg := fmt.Sprintf("failed to fetch secret %q for header %q", secretName, headerRef.HeaderName) + r.recordValidationEvent(proxy, mcpv1alpha1.ConditionReasonHeaderSecretNotFound, genericMsg) + setConfigurationInvalidCondition(proxy, mcpv1alpha1.ConditionReasonHeaderSecretNotFound, genericMsg) + return stderrors.New(genericMsg) + } + } + } + + return nil +} + // handleToolConfig handles MCPToolConfig reference for an MCPRemoteProxy func (r *MCPRemoteProxyReconciler) handleToolConfig(ctx context.Context, proxy *mcpv1alpha1.MCPRemoteProxy) error { ctxLogger := log.FromContext(ctx) diff --git a/cmd/thv-operator/controllers/mcpremoteproxy_controller_test.go b/cmd/thv-operator/controllers/mcpremoteproxy_controller_test.go index e58c331896..7691be4b79 100644 --- a/cmd/thv-operator/controllers/mcpremoteproxy_controller_test.go +++ b/cmd/thv-operator/controllers/mcpremoteproxy_controller_test.go @@ -87,7 +87,7 @@ func TestMCPRemoteProxyValidateSpec(t *testing.T) { }, }, expectError: true, - errContains: "remoteURL is required", + errContains: "remote URL must not be empty", }, // Note: "missing OIDC config" test removed - OIDCConfig is now a required value type // with kubebuilder:validation:Required, so the API server prevents resources without it diff --git a/cmd/thv-operator/controllers/mcpremoteproxy_reconciler_test.go b/cmd/thv-operator/controllers/mcpremoteproxy_reconciler_test.go index 3f5a9f4d82..cfd6c4ed0e 100644 --- a/cmd/thv-operator/controllers/mcpremoteproxy_reconciler_test.go +++ b/cmd/thv-operator/controllers/mcpremoteproxy_reconciler_test.go @@ -866,6 +866,130 @@ func TestValidateSpecConfigurationConditions(t *testing.T) { expectCondition: mcpv1alpha1.ConditionReasonConfigurationValid, conditionStatus: metav1.ConditionTrue, }, + { + name: "invalid Cedar policy syntax is rejected", + proxy: &mcpv1alpha1.MCPRemoteProxy{ + ObjectMeta: metav1.ObjectMeta{Name: "invalid-cedar-proxy", Namespace: "default"}, + Spec: mcpv1alpha1.MCPRemoteProxySpec{ + RemoteURL: "https://mcp.example.com", + OIDCConfig: mcpv1alpha1.OIDCConfigRef{ + Type: mcpv1alpha1.OIDCConfigTypeInline, + Inline: &mcpv1alpha1.InlineOIDCConfig{ + Issuer: "https://auth.example.com", + Audience: "test", + }, + }, + AuthzConfig: &mcpv1alpha1.AuthzConfigRef{ + Type: mcpv1alpha1.AuthzConfigTypeInline, + Inline: &mcpv1alpha1.InlineAuthzConfig{ + Policies: []string{"not valid cedar"}, + }, + }, + }, + }, + expectError: true, + errContains: "invalid syntax", + expectCondition: mcpv1alpha1.ConditionReasonAuthzPolicySyntaxInvalid, + conditionStatus: metav1.ConditionFalse, + }, + { + name: "referenced authz ConfigMap not found is rejected", + proxy: &mcpv1alpha1.MCPRemoteProxy{ + ObjectMeta: metav1.ObjectMeta{Name: "missing-configmap-proxy", Namespace: "default"}, + Spec: mcpv1alpha1.MCPRemoteProxySpec{ + RemoteURL: "https://mcp.example.com", + OIDCConfig: mcpv1alpha1.OIDCConfigRef{ + Type: mcpv1alpha1.OIDCConfigTypeInline, + Inline: &mcpv1alpha1.InlineOIDCConfig{ + Issuer: "https://auth.example.com", + Audience: "test", + }, + }, + AuthzConfig: &mcpv1alpha1.AuthzConfigRef{ + Type: mcpv1alpha1.AuthzConfigTypeConfigMap, + ConfigMap: &mcpv1alpha1.ConfigMapAuthzRef{ + Name: "does-not-exist", + }, + }, + }, + }, + expectError: true, + errContains: "not found", + expectCondition: mcpv1alpha1.ConditionReasonAuthzConfigMapNotFound, + conditionStatus: metav1.ConditionFalse, + }, + { + name: "referenced header secret not found is rejected", + proxy: &mcpv1alpha1.MCPRemoteProxy{ + ObjectMeta: metav1.ObjectMeta{Name: "missing-header-secret-proxy", Namespace: "default"}, + Spec: mcpv1alpha1.MCPRemoteProxySpec{ + RemoteURL: "https://mcp.example.com", + OIDCConfig: mcpv1alpha1.OIDCConfigRef{ + Type: mcpv1alpha1.OIDCConfigTypeInline, + Inline: &mcpv1alpha1.InlineOIDCConfig{ + Issuer: "https://auth.example.com", + Audience: "test", + }, + }, + HeaderForward: &mcpv1alpha1.HeaderForwardConfig{ + AddHeadersFromSecret: []mcpv1alpha1.HeaderFromSecret{ + { + HeaderName: "X-API-Key", + ValueSecretRef: &mcpv1alpha1.SecretKeyRef{ + Name: "missing-secret", + Key: "api-key", + }, + }, + }, + }, + }, + }, + expectError: true, + errContains: "not found", + expectCondition: mcpv1alpha1.ConditionReasonHeaderSecretNotFound, + conditionStatus: metav1.ConditionFalse, + }, + { + name: "malformed remote URL is rejected", + proxy: &mcpv1alpha1.MCPRemoteProxy{ + ObjectMeta: metav1.ObjectMeta{Name: "bad-scheme-proxy", Namespace: "default"}, + Spec: mcpv1alpha1.MCPRemoteProxySpec{ + RemoteURL: "ftp://bad-scheme.example.com", + OIDCConfig: mcpv1alpha1.OIDCConfigRef{ + Type: mcpv1alpha1.OIDCConfigTypeInline, + Inline: &mcpv1alpha1.InlineOIDCConfig{ + Issuer: "https://auth.example.com", + Audience: "test", + }, + }, + }, + }, + expectError: true, + errContains: "scheme", + expectCondition: mcpv1alpha1.ConditionReasonRemoteURLInvalid, + conditionStatus: metav1.ConditionFalse, + }, + { + name: "HTTP JWKS URL is rejected", + proxy: &mcpv1alpha1.MCPRemoteProxy{ + ObjectMeta: metav1.ObjectMeta{Name: "http-jwks-proxy", Namespace: "default"}, + Spec: mcpv1alpha1.MCPRemoteProxySpec{ + RemoteURL: "https://mcp.example.com", + OIDCConfig: mcpv1alpha1.OIDCConfigRef{ + Type: mcpv1alpha1.OIDCConfigTypeInline, + Inline: &mcpv1alpha1.InlineOIDCConfig{ + Issuer: "https://auth.example.com", + Audience: "test", + JWKSURL: "http://jwks.example.com", + }, + }, + }, + }, + expectError: true, + errContains: "HTTPS", + expectCondition: mcpv1alpha1.ConditionReasonJWKSURLInvalid, + conditionStatus: metav1.ConditionFalse, + }, } for _, tt := range tests { diff --git a/cmd/thv-operator/pkg/validation/cedar_validation.go b/cmd/thv-operator/pkg/validation/cedar_validation.go new file mode 100644 index 0000000000..f0f7781473 --- /dev/null +++ b/cmd/thv-operator/pkg/validation/cedar_validation.go @@ -0,0 +1,23 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package validation + +import ( + "fmt" + + cedar "github.com/cedar-policy/cedar-go" +) + +// ValidateCedarPolicies validates the syntax of each Cedar policy string in the +// provided slice. It returns an error for the first policy that fails to parse, +// or nil if all policies are valid (including when the slice is empty or nil). +func ValidateCedarPolicies(policies []string) error { + for i, policy := range policies { + var p cedar.Policy + if err := p.UnmarshalCedar([]byte(policy)); err != nil { + return fmt.Errorf("cedar policy at index %d has invalid syntax: %w", i, err) + } + } + return nil +} diff --git a/cmd/thv-operator/pkg/validation/cedar_validation_test.go b/cmd/thv-operator/pkg/validation/cedar_validation_test.go new file mode 100644 index 0000000000..4a3cf9776b --- /dev/null +++ b/cmd/thv-operator/pkg/validation/cedar_validation_test.go @@ -0,0 +1,76 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package validation_test + +import ( + "testing" + + "github.com/stretchr/testify/require" + + "github.com/stacklok/toolhive/cmd/thv-operator/pkg/validation" +) + +func TestValidateCedarPolicies(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + policies []string + wantErr bool + errContains string + }{ + { + name: "nil policies", + policies: nil, + wantErr: false, + }, + { + name: "empty policies", + policies: []string{}, + wantErr: false, + }, + { + name: "valid permit policy", + policies: []string{"permit (principal, action, resource);"}, + wantErr: false, + }, + { + name: "valid forbid policy", + policies: []string{"forbid (principal, action, resource);"}, + wantErr: false, + }, + { + name: "invalid syntax", + policies: []string{"not valid cedar"}, + wantErr: true, + errContains: "invalid syntax", + }, + { + name: "mixed valid and invalid", + policies: []string{ + "permit (principal, action, resource);", + "bad policy", + }, + wantErr: true, + errContains: "index 1", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + err := validation.ValidateCedarPolicies(tt.policies) + + if tt.wantErr { + require.Error(t, err) + if tt.errContains != "" { + require.Contains(t, err.Error(), tt.errContains) + } + } else { + require.NoError(t, err) + } + }) + } +} diff --git a/cmd/thv-operator/pkg/validation/oidc_validation.go b/cmd/thv-operator/pkg/validation/oidc_validation.go index 6e6e55e827..246ea5d92e 100644 --- a/cmd/thv-operator/pkg/validation/oidc_validation.go +++ b/cmd/thv-operator/pkg/validation/oidc_validation.go @@ -70,14 +70,14 @@ func ValidateOIDCIssuerURL(issuer string, allowInsecure bool) error { return fmt.Errorf("OIDC issuer URL %q is malformed: missing scheme or host", issuer) } - if u.Scheme == "http" && !allowInsecure { + if u.Scheme == schemeHTTP && !allowInsecure { return fmt.Errorf( "OIDC issuer URL %q uses HTTP scheme, which is insecure; "+ "use HTTPS or set insecureAllowHTTP: true for development only", issuer, ) } - if u.Scheme != "http" && u.Scheme != "https" { + if u.Scheme != schemeHTTP && u.Scheme != schemeHTTPS { return fmt.Errorf("OIDC issuer URL %q has unsupported scheme %q; must be http or https", issuer, u.Scheme) } diff --git a/cmd/thv-operator/pkg/validation/url_validation.go b/cmd/thv-operator/pkg/validation/url_validation.go new file mode 100644 index 0000000000..d972252489 --- /dev/null +++ b/cmd/thv-operator/pkg/validation/url_validation.go @@ -0,0 +1,62 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package validation + +import ( + "fmt" + "net/url" +) + +const ( + schemeHTTP = "http" + schemeHTTPS = "https" +) + +// ValidateRemoteURL validates that rawURL is a well-formed HTTP or HTTPS URL +// with a non-empty host. No network calls are made. +func ValidateRemoteURL(rawURL string) error { + if rawURL == "" { + return fmt.Errorf("remote URL must not be empty") + } + + u, err := url.Parse(rawURL) + if err != nil { + return fmt.Errorf("remote URL is invalid: %w", err) + } + + if u.Scheme != schemeHTTP && u.Scheme != schemeHTTPS { + return fmt.Errorf("remote URL must use http or https scheme, got %q", u.Scheme) + } + + if u.Host == "" { + return fmt.Errorf("remote URL must have a valid host") + } + + return nil +} + +// ValidateJWKSURL validates that rawURL, if non-empty, is a well-formed HTTPS +// URL with a non-empty host. JWKS endpoints serve key material and must use +// HTTPS. An empty rawURL is allowed because JWKS discovery can determine the +// endpoint automatically. +func ValidateJWKSURL(rawURL string) error { + if rawURL == "" { + return nil + } + + u, err := url.Parse(rawURL) + if err != nil { + return fmt.Errorf("JWKS URL is invalid: %w", err) + } + + if u.Scheme != schemeHTTPS { + return fmt.Errorf("JWKS URL must use HTTPS scheme, got %q", u.Scheme) + } + + if u.Host == "" { + return fmt.Errorf("JWKS URL must have a valid host") + } + + return nil +} diff --git a/cmd/thv-operator/pkg/validation/url_validation_test.go b/cmd/thv-operator/pkg/validation/url_validation_test.go new file mode 100644 index 0000000000..ec7fb01c02 --- /dev/null +++ b/cmd/thv-operator/pkg/validation/url_validation_test.go @@ -0,0 +1,132 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package validation_test + +import ( + "testing" + + "github.com/stretchr/testify/require" + + "github.com/stacklok/toolhive/cmd/thv-operator/pkg/validation" +) + +func TestValidateRemoteURL(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + rawURL string + wantErr bool + errContains string + }{ + { + name: "valid https URL", + rawURL: "https://mcp.example.com", + wantErr: false, + }, + { + name: "valid http URL", + rawURL: "http://mcp.example.com", + wantErr: false, + }, + { + name: "empty URL", + rawURL: "", + wantErr: true, + errContains: "empty", + }, + { + name: "no scheme", + rawURL: "mcp.example.com", + wantErr: true, + errContains: "scheme", + }, + { + name: "unsupported scheme", + rawURL: "ftp://mcp.example.com", + wantErr: true, + errContains: "scheme", + }, + { + name: "missing host", + rawURL: "https://", + wantErr: true, + errContains: "host", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + err := validation.ValidateRemoteURL(tt.rawURL) + + if tt.wantErr { + require.Error(t, err) + if tt.errContains != "" { + require.Contains(t, err.Error(), tt.errContains) + } + } else { + require.NoError(t, err) + } + }) + } +} + +func TestValidateJWKSURL(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + rawURL string + wantErr bool + errContains string + }{ + { + name: "empty URL allowed", + rawURL: "", + wantErr: false, + }, + { + name: "valid https URL", + rawURL: "https://jwks.example.com/.well-known/jwks.json", + wantErr: false, + }, + { + name: "http rejected", + rawURL: "http://jwks.example.com", + wantErr: true, + errContains: "HTTPS", + }, + { + name: "unsupported scheme", + rawURL: "ftp://jwks.example.com", + wantErr: true, + errContains: "HTTPS", + }, + { + name: "missing host", + rawURL: "https://", + wantErr: true, + errContains: "host", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + err := validation.ValidateJWKSURL(tt.rawURL) + + if tt.wantErr { + require.Error(t, err) + if tt.errContains != "" { + require.Contains(t, err.Error(), tt.errContains) + } + } else { + require.NoError(t, err) + } + }) + } +} diff --git a/cmd/thv-operator/test-integration/mcp-remote-proxy/mcpremoteproxy_validation_integration_test.go b/cmd/thv-operator/test-integration/mcp-remote-proxy/mcpremoteproxy_validation_integration_test.go index c08386fa65..21e87a81a9 100644 --- a/cmd/thv-operator/test-integration/mcp-remote-proxy/mcpremoteproxy_validation_integration_test.go +++ b/cmd/thv-operator/test-integration/mcp-remote-proxy/mcpremoteproxy_validation_integration_test.go @@ -83,6 +83,101 @@ var _ = Describe("MCPRemoteProxy Configuration Validation", Label("k8s", "remote }) }) + Context("Remote URL Format Validation", func() { + It("should reject creation when remote URL has invalid scheme via CRD validation", func() { + By("attempting to create an MCPRemoteProxy with ftp:// remote URL") + proxy := proxyHelper.NewRemoteProxyBuilder("test-bad-url"). + WithRemoteURL("ftp://bad-scheme.example.com"). + Build() + + By("verifying the API server rejects the resource") + err := k8sClient.Create(testCtx, proxy) + Expect(err).To(HaveOccurred(), "expected CRD validation to reject ftp:// URL") + Expect(err.Error()).To(ContainSubstring("remoteURL")) + }) + }) + + Context("JWKS URL Validation", func() { + It("should set ConfigurationValid=False when JWKS URL uses HTTP", func() { + By("creating an MCPRemoteProxy with HTTP JWKS URL") + proxy := proxyHelper.NewRemoteProxyBuilder("test-http-jwks"). + WithInlineOIDCConfigAndJWKS( + "https://auth.example.com", "test-audience", + "http://jwks.example.com/.well-known/jwks.json", + ). + Create(proxyHelper) + + By("waiting for the proxy to reach Failed phase") + statusHelper.WaitForPhase(proxy.Name, mcpv1alpha1.MCPRemoteProxyPhaseFailed, MediumTimeout) + + By("verifying the ConfigurationValid condition") + statusHelper.WaitForConditionReason( + proxy.Name, + mcpv1alpha1.ConditionTypeConfigurationValid, + mcpv1alpha1.ConditionReasonJWKSURLInvalid, + MediumTimeout, + ) + }) + }) + + Context("Cedar Policy Syntax Validation", func() { + It("should set ConfigurationValid=False when Cedar policy has invalid syntax", func() { + By("creating an MCPRemoteProxy with invalid Cedar policy") + proxy := proxyHelper.NewRemoteProxyBuilder("test-bad-cedar"). + WithInlineAuthzConfig([]string{"not valid cedar policy syntax"}). + Create(proxyHelper) + + By("waiting for the proxy to reach Failed phase") + statusHelper.WaitForPhase(proxy.Name, mcpv1alpha1.MCPRemoteProxyPhaseFailed, MediumTimeout) + + By("verifying the ConfigurationValid condition") + statusHelper.WaitForConditionReason( + proxy.Name, + mcpv1alpha1.ConditionTypeConfigurationValid, + mcpv1alpha1.ConditionReasonAuthzPolicySyntaxInvalid, + MediumTimeout, + ) + }) + }) + + Context("ConfigMap and Secret Reference Validation", func() { + It("should set ConfigurationValid=False when authz ConfigMap does not exist", func() { + By("creating an MCPRemoteProxy with missing authz ConfigMap reference") + proxy := proxyHelper.NewRemoteProxyBuilder("test-missing-cm"). + WithAuthzConfigMapRef("does-not-exist", ""). + Create(proxyHelper) + + By("waiting for the proxy to reach Failed phase") + statusHelper.WaitForPhase(proxy.Name, mcpv1alpha1.MCPRemoteProxyPhaseFailed, MediumTimeout) + + By("verifying the ConfigurationValid condition") + statusHelper.WaitForConditionReason( + proxy.Name, + mcpv1alpha1.ConditionTypeConfigurationValid, + mcpv1alpha1.ConditionReasonAuthzConfigMapNotFound, + MediumTimeout, + ) + }) + + It("should set ConfigurationValid=False when header Secret does not exist", func() { + By("creating an MCPRemoteProxy with missing header Secret reference") + proxy := proxyHelper.NewRemoteProxyBuilder("test-missing-secret"). + WithHeaderFromSecret("X-API-Key", "missing-secret", "api-key"). + Create(proxyHelper) + + By("waiting for the proxy to reach Failed phase") + statusHelper.WaitForPhase(proxy.Name, mcpv1alpha1.MCPRemoteProxyPhaseFailed, MediumTimeout) + + By("verifying the ConfigurationValid condition") + statusHelper.WaitForConditionReason( + proxy.Name, + mcpv1alpha1.ConditionTypeConfigurationValid, + mcpv1alpha1.ConditionReasonHeaderSecretNotFound, + MediumTimeout, + ) + }) + }) + Context("Kubernetes Events", func() { It("should emit a Warning event when OIDC issuer uses HTTP", func() { By("creating an MCPRemoteProxy with HTTP OIDC issuer") @@ -115,6 +210,90 @@ var _ = Describe("MCPRemoteProxy Configuration Validation", Label("k8s", "remote "expected a Warning event with reason OIDCIssuerInsecure") }) + It("should emit a Warning event when Cedar policy has invalid syntax", func() { + By("creating an MCPRemoteProxy with invalid Cedar policy") + proxy := proxyHelper.NewRemoteProxyBuilder("test-event-bad-cedar"). + WithInlineAuthzConfig([]string{"not valid cedar"}). + Create(proxyHelper) + + By("waiting for the proxy to reach Failed phase") + statusHelper.WaitForPhase(proxy.Name, mcpv1alpha1.MCPRemoteProxyPhaseFailed, MediumTimeout) + + By("verifying a Warning event was emitted with AuthzPolicySyntaxInvalid reason") + Eventually(func() bool { + eventList := &corev1.EventList{} + err := k8sClient.List(testCtx, eventList, client.InNamespace(testNamespace)) + if err != nil { + return false + } + for _, event := range eventList.Items { + if event.InvolvedObject.Name == proxy.Name && + event.Type == corev1.EventTypeWarning && + event.Reason == mcpv1alpha1.ConditionReasonAuthzPolicySyntaxInvalid { + return true + } + } + return false + }, MediumTimeout, DefaultPollingInterval).Should(BeTrue(), + "expected a Warning event with reason AuthzPolicySyntaxInvalid") + }) + + It("should emit a Warning event when authz ConfigMap is not found", func() { + By("creating an MCPRemoteProxy with missing authz ConfigMap") + proxy := proxyHelper.NewRemoteProxyBuilder("test-event-missing-cm"). + WithAuthzConfigMapRef("nonexistent-cm", ""). + Create(proxyHelper) + + By("waiting for the proxy to reach Failed phase") + statusHelper.WaitForPhase(proxy.Name, mcpv1alpha1.MCPRemoteProxyPhaseFailed, MediumTimeout) + + By("verifying a Warning event was emitted") + Eventually(func() bool { + eventList := &corev1.EventList{} + err := k8sClient.List(testCtx, eventList, client.InNamespace(testNamespace)) + if err != nil { + return false + } + for _, event := range eventList.Items { + if event.InvolvedObject.Name == proxy.Name && + event.Type == corev1.EventTypeWarning && + event.Reason == mcpv1alpha1.ConditionReasonAuthzConfigMapNotFound { + return true + } + } + return false + }, MediumTimeout, DefaultPollingInterval).Should(BeTrue(), + "expected a Warning event with reason AuthzConfigMapNotFound") + }) + + It("should emit a Warning event when header Secret is not found", func() { + By("creating an MCPRemoteProxy with missing header Secret") + proxy := proxyHelper.NewRemoteProxyBuilder("test-event-missing-secret"). + WithHeaderFromSecret("X-API-Key", "nonexistent-secret", "key"). + Create(proxyHelper) + + By("waiting for the proxy to reach Failed phase") + statusHelper.WaitForPhase(proxy.Name, mcpv1alpha1.MCPRemoteProxyPhaseFailed, MediumTimeout) + + By("verifying a Warning event was emitted") + Eventually(func() bool { + eventList := &corev1.EventList{} + err := k8sClient.List(testCtx, eventList, client.InNamespace(testNamespace)) + if err != nil { + return false + } + for _, event := range eventList.Items { + if event.InvolvedObject.Name == proxy.Name && + event.Type == corev1.EventTypeWarning && + event.Reason == mcpv1alpha1.ConditionReasonHeaderSecretNotFound { + return true + } + } + return false + }, MediumTimeout, DefaultPollingInterval).Should(BeTrue(), + "expected a Warning event with reason HeaderSecretNotFound") + }) + It("should not emit a Warning event when OIDC issuer uses HTTPS", func() { By("creating an MCPRemoteProxy with HTTPS OIDC issuer") proxy := proxyHelper.NewRemoteProxyBuilder("test-event-https-oidc"). diff --git a/cmd/thv-operator/test-integration/mcp-remote-proxy/remoteproxy_helpers.go b/cmd/thv-operator/test-integration/mcp-remote-proxy/remoteproxy_helpers.go index c08c39327f..2300693758 100644 --- a/cmd/thv-operator/test-integration/mcp-remote-proxy/remoteproxy_helpers.go +++ b/cmd/thv-operator/test-integration/mcp-remote-proxy/remoteproxy_helpers.go @@ -142,6 +142,74 @@ func (rb *RemoteProxyBuilder) WithInlineOIDCConfig(issuer, audience string, inse return rb } +// WithRemoteURL overrides the default remote URL +func (rb *RemoteProxyBuilder) WithRemoteURL(url string) *RemoteProxyBuilder { + rb.proxy.Spec.RemoteURL = url + return rb +} + +// WithInlineOIDCConfigAndJWKS sets an inline OIDC config with a custom JWKS URL +func (rb *RemoteProxyBuilder) WithInlineOIDCConfigAndJWKS( + issuer, audience, jwksURL string, +) *RemoteProxyBuilder { + rb.proxy.Spec.OIDCConfig = mcpv1alpha1.OIDCConfigRef{ + Type: mcpv1alpha1.OIDCConfigTypeInline, + Inline: &mcpv1alpha1.InlineOIDCConfig{ + Issuer: issuer, + Audience: audience, + JWKSURL: jwksURL, + }, + } + return rb +} + +// WithInlineAuthzConfig sets an inline authz config with Cedar policies +func (rb *RemoteProxyBuilder) WithInlineAuthzConfig(policies []string) *RemoteProxyBuilder { + rb.proxy.Spec.AuthzConfig = &mcpv1alpha1.AuthzConfigRef{ + Type: mcpv1alpha1.AuthzConfigTypeInline, + Inline: &mcpv1alpha1.InlineAuthzConfig{ + Policies: policies, + }, + } + return rb +} + +// WithAuthzConfigMapRef sets an authz config referencing a ConfigMap +func (rb *RemoteProxyBuilder) WithAuthzConfigMapRef(name, key string) *RemoteProxyBuilder { + rb.proxy.Spec.AuthzConfig = &mcpv1alpha1.AuthzConfigRef{ + Type: mcpv1alpha1.AuthzConfigTypeConfigMap, + ConfigMap: &mcpv1alpha1.ConfigMapAuthzRef{ + Name: name, + Key: key, + }, + } + return rb +} + +// WithHeaderFromSecret sets a header forward config that references a secret +func (rb *RemoteProxyBuilder) WithHeaderFromSecret( + headerName, secretName, secretKey string, +) *RemoteProxyBuilder { + rb.proxy.Spec.HeaderForward = &mcpv1alpha1.HeaderForwardConfig{ + AddHeadersFromSecret: []mcpv1alpha1.HeaderFromSecret{ + { + HeaderName: headerName, + ValueSecretRef: &mcpv1alpha1.SecretKeyRef{ + Name: secretName, + Key: secretKey, + }, + }, + }, + } + return rb +} + +// Build returns a deep copy of the MCPRemoteProxy without creating it in the cluster. +// Use this when testing CRD-level validation that rejects the resource at creation time. +func (rb *RemoteProxyBuilder) Build() *mcpv1alpha1.MCPRemoteProxy { + return rb.proxy.DeepCopy() +} + // Create builds and creates the MCPRemoteProxy in the cluster func (rb *RemoteProxyBuilder) Create(h *MCPRemoteProxyTestHelper) *mcpv1alpha1.MCPRemoteProxy { proxy := rb.proxy.DeepCopy()