diff --git a/cmd/thv-operator/api/v1beta1/mcpoidcconfig_types.go b/cmd/thv-operator/api/v1beta1/mcpoidcconfig_types.go index 2b9eb79630..61c63724e2 100644 --- a/cmd/thv-operator/api/v1beta1/mcpoidcconfig_types.go +++ b/cmd/thv-operator/api/v1beta1/mcpoidcconfig_types.go @@ -141,7 +141,7 @@ type InlineOIDCSharedConfig struct { // +optional ProtectedResourceAllowPrivateIP bool `json:"protectedResourceAllowPrivateIP"` - // InsecureAllowHTTP allows HTTP (non-HTTPS) OIDC issuers for development/testing. + // InsecureAllowHTTP allows HTTP (non-HTTPS) OIDC issuer and JWKS URLs for development/testing. // WARNING: This is insecure and should NEVER be used in production. // +kubebuilder:default=false // +optional diff --git a/cmd/thv-operator/controllers/mcpoidcconfig_controller.go b/cmd/thv-operator/controllers/mcpoidcconfig_controller.go index 1826b7278a..3dc65cb9a5 100644 --- a/cmd/thv-operator/controllers/mcpoidcconfig_controller.go +++ b/cmd/thv-operator/controllers/mcpoidcconfig_controller.go @@ -21,6 +21,7 @@ import ( mcpv1beta1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1beta1" ctrlutil "github.com/stacklok/toolhive/cmd/thv-operator/pkg/controllerutil" + "github.com/stacklok/toolhive/cmd/thv-operator/pkg/validation" ) const ( @@ -87,7 +88,7 @@ func (r *MCPOIDCConfigReconciler) Reconcile(ctx context.Context, req ctrl.Reques } // Validate spec configuration early - if err := oidcConfig.Validate(); err != nil { + if err := validateOIDCConfigSpec(oidcConfig); err != nil { logger.Error(err, "MCPOIDCConfig spec validation failed") return r.handleValidationFailure(ctx, oidcConfig, err) } @@ -135,6 +136,26 @@ func (r *MCPOIDCConfigReconciler) Reconcile(ctx context.Context, req ctrl.Reques return ctrl.Result{}, nil } +// validateOIDCConfigSpec validates the MCPOIDCConfig spec: type/config +// consistency via Validate(), then issuer and JWKS URL checks for inline +// configs. The URL checks run here rather than inside Validate() because the +// validation package imports the v1beta1 API package (for example +// validation.ValidateCABundleSource), so calling it from the types package +// would create an import cycle. This mirrors the MCPRemoteProxy controller, +// which applies validation.ValidateRemoteURL in its validateSpec step. +func validateOIDCConfigSpec(oidcConfig *mcpv1beta1.MCPOIDCConfig) error { + if err := oidcConfig.Validate(); err != nil { + return err + } + if oidcConfig.Spec.Type != mcpv1beta1.MCPOIDCConfigTypeInline || oidcConfig.Spec.Inline == nil { + return nil + } + if err := validation.ValidateOIDCIssuerURL(oidcConfig.Spec.Inline.Issuer, oidcConfig.Spec.Inline.InsecureAllowHTTP); err != nil { + return err + } + return validation.ValidateJWKSURL(oidcConfig.Spec.Inline.JWKSURL, oidcConfig.Spec.Inline.InsecureAllowHTTP) +} + // handleValidationFailure records the Valid=False condition for a spec that // failed validation and emits a one-shot Warning event on the transition into // the invalid state. It never requeues — the user must fix the spec. diff --git a/cmd/thv-operator/controllers/mcpoidcconfig_controller_test.go b/cmd/thv-operator/controllers/mcpoidcconfig_controller_test.go index 213d6a32aa..be7b5c0d33 100644 --- a/cmd/thv-operator/controllers/mcpoidcconfig_controller_test.go +++ b/cmd/thv-operator/controllers/mcpoidcconfig_controller_test.go @@ -513,6 +513,211 @@ func TestMCPOIDCConfig_Validate(t *testing.T) { } } +func TestValidateOIDCConfigSpec(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + config *mcpv1beta1.MCPOIDCConfig + expectError bool + }{ + { + name: "valid inline config with HTTPS issuer and JWKS URL", + config: &mcpv1beta1.MCPOIDCConfig{ + Spec: mcpv1beta1.MCPOIDCConfigSpec{ + Type: mcpv1beta1.MCPOIDCConfigTypeInline, + Inline: &mcpv1beta1.InlineOIDCSharedConfig{ + Issuer: "https://accounts.google.com", + JWKSURL: "https://www.googleapis.com/oauth2/v3/certs", + ClientID: "test-client", + }, + }, + }, + expectError: false, + }, + { + name: "valid inline config with empty JWKS URL (auto-discovery)", + config: &mcpv1beta1.MCPOIDCConfig{ + Spec: mcpv1beta1.MCPOIDCConfigSpec{ + Type: mcpv1beta1.MCPOIDCConfigTypeInline, + Inline: &mcpv1beta1.InlineOIDCSharedConfig{ + Issuer: "https://accounts.google.com", + ClientID: "test-client", + }, + }, + }, + expectError: false, + }, + { + name: "valid inline config with HTTP issuer when insecureAllowHTTP is set", + config: &mcpv1beta1.MCPOIDCConfig{ + Spec: mcpv1beta1.MCPOIDCConfigSpec{ + Type: mcpv1beta1.MCPOIDCConfigTypeInline, + Inline: &mcpv1beta1.InlineOIDCSharedConfig{ + Issuer: "http://issuer.dev.local", + ClientID: "test-client", + InsecureAllowHTTP: true, + }, + }, + }, + expectError: false, + }, + { + name: "invalid inline config with HTTP issuer without insecureAllowHTTP", + config: &mcpv1beta1.MCPOIDCConfig{ + Spec: mcpv1beta1.MCPOIDCConfigSpec{ + Type: mcpv1beta1.MCPOIDCConfigTypeInline, + Inline: &mcpv1beta1.InlineOIDCSharedConfig{ + Issuer: "http://issuer.example.com", + ClientID: "test-client", + }, + }, + }, + expectError: true, + }, + { + name: "invalid inline config with malformed issuer", + config: &mcpv1beta1.MCPOIDCConfig{ + Spec: mcpv1beta1.MCPOIDCConfigSpec{ + Type: mcpv1beta1.MCPOIDCConfigTypeInline, + Inline: &mcpv1beta1.InlineOIDCSharedConfig{ + Issuer: "not-a-url", + ClientID: "test-client", + }, + }, + }, + expectError: true, + }, + { + name: "invalid inline config with issuer using unsupported scheme", + config: &mcpv1beta1.MCPOIDCConfig{ + Spec: mcpv1beta1.MCPOIDCConfigSpec{ + Type: mcpv1beta1.MCPOIDCConfigTypeInline, + Inline: &mcpv1beta1.InlineOIDCSharedConfig{ + Issuer: "ftp://issuer.example.com", + ClientID: "test-client", + }, + }, + }, + expectError: true, + }, + { + name: "invalid inline config with non-HTTPS JWKS URL", + config: &mcpv1beta1.MCPOIDCConfig{ + Spec: mcpv1beta1.MCPOIDCConfigSpec{ + Type: mcpv1beta1.MCPOIDCConfigTypeInline, + Inline: &mcpv1beta1.InlineOIDCSharedConfig{ + Issuer: "https://accounts.google.com", + JWKSURL: "http://www.googleapis.com/oauth2/v3/certs", + ClientID: "test-client", + }, + }, + }, + expectError: true, + }, + { + name: "valid inline config with HTTP JWKS URL and insecureAllowHTTP", + config: &mcpv1beta1.MCPOIDCConfig{ + Spec: mcpv1beta1.MCPOIDCConfigSpec{ + Type: mcpv1beta1.MCPOIDCConfigTypeInline, + Inline: &mcpv1beta1.InlineOIDCSharedConfig{ + Issuer: "http://keycloak:8080/realms/toolhive", + JWKSURL: "http://keycloak:8080/realms/toolhive/protocol/openid-connect/certs", + ClientID: "test-client", + InsecureAllowHTTP: true, + }, + }, + }, + expectError: false, + }, + { + name: "kubernetesServiceAccount config skips URL validation", + config: &mcpv1beta1.MCPOIDCConfig{ + Spec: mcpv1beta1.MCPOIDCConfigSpec{ + Type: mcpv1beta1.MCPOIDCConfigTypeKubernetesServiceAccount, + KubernetesServiceAccount: &mcpv1beta1.KubernetesServiceAccountOIDCConfig{ + ServiceAccount: "test-sa", + // Would fail the URL checks if they applied to this type + Issuer: "not-a-url", + }, + }, + }, + expectError: false, + }, + { + name: "type consistency failure is still reported", + config: &mcpv1beta1.MCPOIDCConfig{ + Spec: mcpv1beta1.MCPOIDCConfigSpec{ + Type: mcpv1beta1.MCPOIDCConfigTypeInline, + // Missing Inline config + }, + }, + expectError: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + err := validateOIDCConfigSpec(tt.config) + + if tt.expectError { + assert.Error(t, err) + } else { + assert.NoError(t, err) + } + }) + } +} + +func TestMCPOIDCConfigReconciler_URLValidationFailureSetsCondition(t *testing.T) { + t.Parallel() + + ctx := t.Context() + + // Invalid config: HTTP issuer without insecureAllowHTTP + oidcConfig := &mcpv1beta1.MCPOIDCConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "insecure-issuer-config", + Namespace: "default", + Finalizers: []string{OIDCConfigFinalizerName}, + Generation: 1, + }, + Spec: mcpv1beta1.MCPOIDCConfigSpec{ + Type: mcpv1beta1.MCPOIDCConfigTypeInline, + Inline: &mcpv1beta1.InlineOIDCSharedConfig{ + Issuer: "http://issuer.example.com", + ClientID: "test-client", + }, + }, + } + + r, fakeClient := newTestMCPOIDCConfigReconciler(t, oidcConfig) + + req := reconcile.Request{ + NamespacedName: types.NamespacedName{ + Name: oidcConfig.Name, + Namespace: oidcConfig.Namespace, + }, + } + + // Reconcile should not return error (validation failures are not requeued) + _, err := r.Reconcile(ctx, req) + require.NoError(t, err) + + var updatedConfig mcpv1beta1.MCPOIDCConfig + err = fakeClient.Get(ctx, req.NamespacedName, &updatedConfig) + require.NoError(t, err) + + cond := meta.FindStatusCondition(updatedConfig.Status.Conditions, mcpv1beta1.ConditionTypeOIDCConfigValid) + require.NotNil(t, cond, "Should have a Valid condition") + assert.Equal(t, metav1.ConditionFalse, cond.Status, "Valid condition should be False") + assert.Equal(t, mcpv1beta1.ConditionReasonOIDCConfigInvalid, cond.Reason) + assert.Contains(t, cond.Message, "http://issuer.example.com", + "Message should surface the offending issuer URL") +} + // TestMCPOIDCConfigReconciler_ReconcileKeepsExistingForeignCondition verifies // that when the controller observes a foreign-owned condition already on the // object and then writes its own Valid=True, it folds its condition into the diff --git a/cmd/thv-operator/pkg/validation/url_validation.go b/cmd/thv-operator/pkg/validation/url_validation.go index 6d69d2d9d7..cf060f993b 100644 --- a/cmd/thv-operator/pkg/validation/url_validation.go +++ b/cmd/thv-operator/pkg/validation/url_validation.go @@ -116,9 +116,13 @@ func validateHostNotInternal(host string) error { // 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 { +// HTTPS. If allowInsecure is true, HTTP is permitted (for development/testing +// only), matching ValidateOIDCIssuerURL. An empty rawURL is allowed because +// JWKS discovery can determine the endpoint automatically. +// +// Schemes other than http and https are always rejected, regardless of +// allowInsecure. +func ValidateJWKSURL(rawURL string, allowInsecure bool) error { if rawURL == "" { return nil } @@ -128,8 +132,15 @@ func ValidateJWKSURL(rawURL string) error { 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.Scheme == schemeHTTP && !allowInsecure { + return fmt.Errorf( + "JWKS URL %q uses HTTP scheme, which is insecure; "+ + "use HTTPS or set insecureAllowHTTP: true for development only", rawURL, + ) + } + + if u.Scheme != schemeHTTP && u.Scheme != schemeHTTPS { + return fmt.Errorf("JWKS URL %q has unsupported scheme %q; must be http or https", rawURL, u.Scheme) } if u.Host == "" { diff --git a/cmd/thv-operator/pkg/validation/url_validation_test.go b/cmd/thv-operator/pkg/validation/url_validation_test.go index ab72dfacdf..230920ddf1 100644 --- a/cmd/thv-operator/pkg/validation/url_validation_test.go +++ b/cmd/thv-operator/pkg/validation/url_validation_test.go @@ -236,10 +236,11 @@ func TestValidateJWKSURL(t *testing.T) { t.Parallel() tests := []struct { - name string - rawURL string - wantErr bool - errContains string + name string + rawURL string + allowInsecure bool + wantErr bool + errContains string }{ { name: "empty URL allowed", @@ -255,13 +256,26 @@ func TestValidateJWKSURL(t *testing.T) { name: "http rejected", rawURL: "http://jwks.example.com", wantErr: true, - errContains: "HTTPS", + errContains: "insecureAllowHTTP", + }, + { + name: "http allowed with insecureAllowHTTP", + rawURL: "http://jwks.example.com", + allowInsecure: true, + wantErr: false, }, { name: "unsupported scheme", rawURL: "ftp://jwks.example.com", wantErr: true, - errContains: "HTTPS", + errContains: "unsupported scheme", + }, + { + name: "unsupported scheme rejected even with insecureAllowHTTP", + rawURL: "ftp://jwks.example.com", + allowInsecure: true, + wantErr: true, + errContains: "unsupported scheme", }, { name: "missing host", @@ -275,7 +289,7 @@ func TestValidateJWKSURL(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() - err := validation.ValidateJWKSURL(tt.rawURL) + err := validation.ValidateJWKSURL(tt.rawURL, tt.allowInsecure) if tt.wantErr { require.Error(t, err) diff --git a/cmd/thv-operator/test-integration/mcp-remote-proxy/mcpremoteproxy_authserverref_integration_test.go b/cmd/thv-operator/test-integration/mcp-remote-proxy/mcpremoteproxy_authserverref_integration_test.go index a8cbf1ace0..13526fc771 100644 --- a/cmd/thv-operator/test-integration/mcp-remote-proxy/mcpremoteproxy_authserverref_integration_test.go +++ b/cmd/thv-operator/test-integration/mcp-remote-proxy/mcpremoteproxy_authserverref_integration_test.go @@ -288,7 +288,8 @@ func newMCPOIDCConfig(name, namespace string) *mcpv1beta1.MCPOIDCConfig { Spec: mcpv1beta1.MCPOIDCConfigSpec{ Type: mcpv1beta1.MCPOIDCConfigTypeInline, Inline: &mcpv1beta1.InlineOIDCSharedConfig{ - Issuer: "http://localhost:9090", + Issuer: "http://localhost:9090", + InsecureAllowHTTP: true, }, }, } diff --git a/cmd/thv-operator/test-integration/mcp-remote-proxy/mcpremoteproxy_cabundle_integration_test.go b/cmd/thv-operator/test-integration/mcp-remote-proxy/mcpremoteproxy_cabundle_integration_test.go index 5d8828d19d..c425d97636 100644 --- a/cmd/thv-operator/test-integration/mcp-remote-proxy/mcpremoteproxy_cabundle_integration_test.go +++ b/cmd/thv-operator/test-integration/mcp-remote-proxy/mcpremoteproxy_cabundle_integration_test.go @@ -25,7 +25,8 @@ func newMCPOIDCConfigWithCABundle(name, namespace, cmName, key string) *mcpv1bet Spec: mcpv1beta1.MCPOIDCConfigSpec{ Type: mcpv1beta1.MCPOIDCConfigTypeInline, Inline: &mcpv1beta1.InlineOIDCSharedConfig{ - Issuer: "http://localhost:9090", + Issuer: "http://localhost:9090", + InsecureAllowHTTP: true, CABundleRef: &mcpv1beta1.CABundleSource{ ConfigMapRef: &corev1.ConfigMapKeySelector{ LocalObjectReference: corev1.LocalObjectReference{Name: cmName}, diff --git a/cmd/thv-operator/test-integration/mcp-server/mcpserver_authserverref_integration_test.go b/cmd/thv-operator/test-integration/mcp-server/mcpserver_authserverref_integration_test.go index 4fe4672ee1..7acb73208b 100644 --- a/cmd/thv-operator/test-integration/mcp-server/mcpserver_authserverref_integration_test.go +++ b/cmd/thv-operator/test-integration/mcp-server/mcpserver_authserverref_integration_test.go @@ -42,7 +42,8 @@ var _ = Describe("MCPServer AuthServerRef Integration Tests", func() { Spec: mcpv1beta1.MCPOIDCConfigSpec{ Type: mcpv1beta1.MCPOIDCConfigTypeInline, Inline: &mcpv1beta1.InlineOIDCSharedConfig{ - Issuer: "http://localhost:9090", + Issuer: "http://localhost:9090", + InsecureAllowHTTP: true, }, }, } @@ -154,7 +155,8 @@ var _ = Describe("MCPServer AuthServerRef Integration Tests", func() { Spec: mcpv1beta1.MCPOIDCConfigSpec{ Type: mcpv1beta1.MCPOIDCConfigTypeInline, Inline: &mcpv1beta1.InlineOIDCSharedConfig{ - Issuer: "http://localhost:9090", + Issuer: "http://localhost:9090", + InsecureAllowHTTP: true, }, }, } @@ -340,7 +342,8 @@ var _ = Describe("MCPServer AuthServerRef Integration Tests", func() { Spec: mcpv1beta1.MCPOIDCConfigSpec{ Type: mcpv1beta1.MCPOIDCConfigTypeInline, Inline: &mcpv1beta1.InlineOIDCSharedConfig{ - Issuer: "http://localhost:9090", + Issuer: "http://localhost:9090", + InsecureAllowHTTP: true, }, }, } diff --git a/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_mcpoidcconfigs.yaml b/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_mcpoidcconfigs.yaml index b30459b312..a0da4c26a8 100644 --- a/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_mcpoidcconfigs.yaml +++ b/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_mcpoidcconfigs.yaml @@ -117,7 +117,7 @@ spec: insecureAllowHTTP: default: false description: |- - InsecureAllowHTTP allows HTTP (non-HTTPS) OIDC issuers for development/testing. + InsecureAllowHTTP allows HTTP (non-HTTPS) OIDC issuer and JWKS URLs for development/testing. WARNING: This is insecure and should NEVER be used in production. type: boolean introspectionUrl: @@ -382,7 +382,7 @@ spec: insecureAllowHTTP: default: false description: |- - InsecureAllowHTTP allows HTTP (non-HTTPS) OIDC issuers for development/testing. + InsecureAllowHTTP allows HTTP (non-HTTPS) OIDC issuer and JWKS URLs for development/testing. WARNING: This is insecure and should NEVER be used in production. type: boolean introspectionUrl: diff --git a/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_mcpoidcconfigs.yaml b/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_mcpoidcconfigs.yaml index ed087f74c5..c04a73286f 100644 --- a/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_mcpoidcconfigs.yaml +++ b/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_mcpoidcconfigs.yaml @@ -120,7 +120,7 @@ spec: insecureAllowHTTP: default: false description: |- - InsecureAllowHTTP allows HTTP (non-HTTPS) OIDC issuers for development/testing. + InsecureAllowHTTP allows HTTP (non-HTTPS) OIDC issuer and JWKS URLs for development/testing. WARNING: This is insecure and should NEVER be used in production. type: boolean introspectionUrl: @@ -385,7 +385,7 @@ spec: insecureAllowHTTP: default: false description: |- - InsecureAllowHTTP allows HTTP (non-HTTPS) OIDC issuers for development/testing. + InsecureAllowHTTP allows HTTP (non-HTTPS) OIDC issuer and JWKS URLs for development/testing. WARNING: This is insecure and should NEVER be used in production. type: boolean introspectionUrl: diff --git a/deploy/keycloak/mcpserver-with-auth.yaml b/deploy/keycloak/mcpserver-with-auth.yaml index 85ecf97a3a..f2bdc8110b 100644 --- a/deploy/keycloak/mcpserver-with-auth.yaml +++ b/deploy/keycloak/mcpserver-with-auth.yaml @@ -8,6 +8,8 @@ spec: inline: # Keycloak issuer URL for the toolhive realm issuer: http://keycloak:8080/realms/toolhive + # Dev Keycloak has no TLS, so the HTTP issuer and JWKS URLs need this opt-in. + insecureAllowHTTP: true # Explicit JWKS URL to avoid OIDC discovery issues jwksUrl: http://keycloak-dev-service.keycloak.svc.cluster.local:8080/realms/toolhive/protocol/openid-connect/certs # Optional: Allow private IP addresses for development diff --git a/docs/operator/crd-api.md b/docs/operator/crd-api.md index 5b32a3cf92..070bc9c8f9 100644 --- a/docs/operator/crd-api.md +++ b/docs/operator/crd-api.md @@ -2304,7 +2304,7 @@ _Appears in:_ | `jwksAuthTokenPath` _string_ | JWKSAuthTokenPath is the path to file containing bearer token for JWKS/OIDC requests | | Optional: \{\}
| | `jwksAllowPrivateIP` _boolean_ | JWKSAllowPrivateIP allows JWKS/OIDC endpoints on private IP addresses.
Note: at runtime, if either JWKSAllowPrivateIP or ProtectedResourceAllowPrivateIP
is true, private IPs are allowed for all OIDC HTTP requests (JWKS, discovery, introspection). | false | Optional: \{\}
| | `protectedResourceAllowPrivateIP` _boolean_ | ProtectedResourceAllowPrivateIP allows protected resource endpoint on private IP addresses.
Note: at runtime, if either ProtectedResourceAllowPrivateIP or JWKSAllowPrivateIP
is true, private IPs are allowed for all OIDC HTTP requests (JWKS, discovery, introspection). | false | Optional: \{\}
| -| `insecureAllowHTTP` _boolean_ | InsecureAllowHTTP allows HTTP (non-HTTPS) OIDC issuers for development/testing.
WARNING: This is insecure and should NEVER be used in production. | false | Optional: \{\}
| +| `insecureAllowHTTP` _boolean_ | InsecureAllowHTTP allows HTTP (non-HTTPS) OIDC issuer and JWKS URLs for development/testing.
WARNING: This is insecure and should NEVER be used in production. | false | Optional: \{\}
| #### api.v1beta1.KubernetesServiceAccountOIDCConfig