From 3ee156da693f1792b982c8c4edddfcac2e8c9e76 Mon Sep 17 00:00:00 2001 From: Per Goncalves da Silva Date: Wed, 17 Sep 2025 16:28:08 +0200 Subject: [PATCH 1/3] Add webhook rule checker Signed-off-by: Per Goncalves da Silva --- .../render/registryv1/validators/validator.go | 42 +++ .../registryv1/validators/validator_test.go | 310 ++++++++++++++++++ 2 files changed, 352 insertions(+) diff --git a/internal/operator-controller/rukpak/render/registryv1/validators/validator.go b/internal/operator-controller/rukpak/render/registryv1/validators/validator.go index 61d0aad7c..63e8bbc4a 100644 --- a/internal/operator-controller/rukpak/render/registryv1/validators/validator.go +++ b/internal/operator-controller/rukpak/render/registryv1/validators/validator.go @@ -264,3 +264,45 @@ func CheckWebhookNameIsDNS1123SubDomain(rv1 *bundle.RegistryV1) []error { } return errs } + +// unsupportedWebhookRuleAPIGroups contain the API groups that are unsupported for webhook configuration rules in OLMv1 +var unsupportedWebhookRuleAPIGroups = sets.New("olm.operatorframework.io", "*") + +// unsupportedAdmissionRegistrationResources contain the resources that are unsupported for webhook configuration rules +// for the admissionregistration.k8s.io api group +var unsupportedAdmissionRegistrationResources = sets.New( + "*", + "mutatingwebhookconfiguration", + "mutatingwebhookconfigurations", + "validatingwebhookconfiguration", + "validatingwebhookconfigurations", +) + +// CheckWebhookRules ensures webhook rules do not reference forbidden API groups or resources in line with OLMv0 behavior +// See https://github.com/operator-framework/operator-lifecycle-manager/blob/ccf0c4c91f1e7673e87f3a18947f9a1f88d48438/pkg/controller/install/webhook.go#L19 +// for more details +func CheckWebhookRules(rv1 *bundle.RegistryV1) []error { + var errs []error + for _, wh := range rv1.CSV.Spec.WebhookDefinitions { + // Rules are not used for conversion webhooks + if wh.Type == v1alpha1.ConversionWebhook { + continue + } + webhookName := wh.GenerateName + for _, rule := range wh.Rules { + for _, apiGroup := range rule.APIGroups { + if unsupportedWebhookRuleAPIGroups.Has(apiGroup) { + errs = append(errs, fmt.Errorf("webhook %q contains forbidden rule: admission webhook rules cannot reference API group %q", webhookName, apiGroup)) + } + if apiGroup == "admissionregistration.k8s.io" { + for _, resource := range rule.Resources { + if unsupportedAdmissionRegistrationResources.Has(strings.ToLower(resource)) { + errs = append(errs, fmt.Errorf("webhook %q contains forbidden rule: admission webhook rules cannot reference resource %q for API group %q", webhookName, resource, apiGroup)) + } + } + } + } + } + } + return errs +} diff --git a/internal/operator-controller/rukpak/render/registryv1/validators/validator_test.go b/internal/operator-controller/rukpak/render/registryv1/validators/validator_test.go index 6c1d7491b..57c010d65 100644 --- a/internal/operator-controller/rukpak/render/registryv1/validators/validator_test.go +++ b/internal/operator-controller/rukpak/render/registryv1/validators/validator_test.go @@ -5,6 +5,7 @@ import ( "testing" "github.com/stretchr/testify/require" + admissionregistrationv1 "k8s.io/api/admissionregistration/v1" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -338,6 +339,315 @@ func Test_CheckWebhookSupport(t *testing.T) { } } +func Test_CheckWebhookRules(t *testing.T) { + for _, tc := range []struct { + name string + bundle *bundle.RegistryV1 + expectedErrs []error + }{ + { + name: "accepts bundles with webhook definitions without rules", + bundle: &bundle.RegistryV1{ + CSV: MakeCSV( + WithInstallModeSupportFor(v1alpha1.InstallModeTypeAllNamespaces), + WithWebhookDefinitions( + v1alpha1.WebhookDescription{ + Type: v1alpha1.ValidatingAdmissionWebhook, + }, + v1alpha1.WebhookDescription{ + Type: v1alpha1.MutatingAdmissionWebhook, + }, + ), + ), + }, + }, + { + name: "accepts bundles with webhook definitions with supported rules", + bundle: &bundle.RegistryV1{ + CSV: MakeCSV( + WithInstallModeSupportFor(v1alpha1.InstallModeTypeAllNamespaces), + WithWebhookDefinitions( + v1alpha1.WebhookDescription{ + Type: v1alpha1.ValidatingAdmissionWebhook, + Rules: []admissionregistrationv1.RuleWithOperations{ + { + Rule: admissionregistrationv1.Rule{ + APIGroups: []string{"appsv1"}, + Resources: []string{"deployments", "replicasets", "statefulsets"}, + }, + }, + }, + }, + v1alpha1.WebhookDescription{ + Type: v1alpha1.MutatingAdmissionWebhook, + Rules: []admissionregistrationv1.RuleWithOperations{ + { + Rule: admissionregistrationv1.Rule{ + APIGroups: []string{""}, + Resources: []string{"services"}, + }, + }, + }, + }, + ), + ), + }, + }, + { + name: "reject bundles with webhook definitions with rules containing '*' api group", + bundle: &bundle.RegistryV1{ + CSV: MakeCSV( + WithInstallModeSupportFor(v1alpha1.InstallModeTypeAllNamespaces), + WithWebhookDefinitions( + v1alpha1.WebhookDescription{ + Type: v1alpha1.ValidatingAdmissionWebhook, + GenerateName: "webhook-z", + Rules: []admissionregistrationv1.RuleWithOperations{ + { + Rule: admissionregistrationv1.Rule{ + APIGroups: []string{"*"}, + }, + }, + }, + }, + v1alpha1.WebhookDescription{ + Type: v1alpha1.MutatingAdmissionWebhook, + GenerateName: "webhook-a", + Rules: []admissionregistrationv1.RuleWithOperations{ + { + Rule: admissionregistrationv1.Rule{ + APIGroups: []string{"*"}, + }, + }, + }, + }, + ), + ), + }, + expectedErrs: []error{ + errors.New("webhook \"webhook-z\" contains forbidden rule: admission webhook rules cannot reference API group \"*\""), + errors.New("webhook \"webhook-a\" contains forbidden rule: admission webhook rules cannot reference API group \"*\""), + }, + }, + { + name: "reject bundles with webhook definitions with rules containing 'olm.operatorframework.io' api group", + bundle: &bundle.RegistryV1{ + CSV: MakeCSV( + WithInstallModeSupportFor(v1alpha1.InstallModeTypeAllNamespaces), + WithWebhookDefinitions( + v1alpha1.WebhookDescription{ + Type: v1alpha1.ValidatingAdmissionWebhook, + GenerateName: "webhook-z", + Rules: []admissionregistrationv1.RuleWithOperations{ + { + Rule: admissionregistrationv1.Rule{ + APIGroups: []string{"olm.operatorframework.io"}, + }, + }, + }, + }, + v1alpha1.WebhookDescription{ + Type: v1alpha1.MutatingAdmissionWebhook, + GenerateName: "webhook-a", + Rules: []admissionregistrationv1.RuleWithOperations{ + { + Rule: admissionregistrationv1.Rule{ + APIGroups: []string{"olm.operatorframework.io"}, + }, + }, + }, + }, + ), + ), + }, + expectedErrs: []error{ + errors.New("webhook \"webhook-z\" contains forbidden rule: admission webhook rules cannot reference API group \"olm.operatorframework.io\""), + errors.New("webhook \"webhook-a\" contains forbidden rule: admission webhook rules cannot reference API group \"olm.operatorframework.io\""), + }, + }, + { + name: "reject bundles with webhook definitions with rules containing 'admissionregistration.k8s.io' api group and '*' resource", + bundle: &bundle.RegistryV1{ + CSV: MakeCSV( + WithInstallModeSupportFor(v1alpha1.InstallModeTypeAllNamespaces), + WithWebhookDefinitions( + v1alpha1.WebhookDescription{ + Type: v1alpha1.ValidatingAdmissionWebhook, + GenerateName: "webhook-a", + Rules: []admissionregistrationv1.RuleWithOperations{ + { + Rule: admissionregistrationv1.Rule{ + APIGroups: []string{"admissionregistration.k8s.io"}, + Resources: []string{"*"}, + }, + }, + }, + }, + ), + ), + }, + expectedErrs: []error{ + errors.New("webhook \"webhook-a\" contains forbidden rule: admission webhook rules cannot reference resource \"*\" for API group \"admissionregistration.k8s.io\""), + }, + }, + { + name: "reject bundles with webhook definitions with rules containing 'admissionregistration.k8s.io' api group and 'MutatingWebhookConfiguration' resource", + bundle: &bundle.RegistryV1{ + CSV: MakeCSV( + WithInstallModeSupportFor(v1alpha1.InstallModeTypeAllNamespaces), + WithWebhookDefinitions( + v1alpha1.WebhookDescription{ + Type: v1alpha1.ValidatingAdmissionWebhook, + GenerateName: "webhook-a", + Rules: []admissionregistrationv1.RuleWithOperations{ + { + Rule: admissionregistrationv1.Rule{ + APIGroups: []string{"admissionregistration.k8s.io"}, + Resources: []string{"MutatingWebhookConfiguration"}, + }, + }, + }, + }, + ), + ), + }, + expectedErrs: []error{ + errors.New("webhook \"webhook-a\" contains forbidden rule: admission webhook rules cannot reference resource \"MutatingWebhookConfiguration\" for API group \"admissionregistration.k8s.io\""), + }, + }, + { + name: "reject bundles with webhook definitions with rules containing 'admissionregistration.k8s.io' api group and 'mutatingwebhookconfiguration' resource", + bundle: &bundle.RegistryV1{ + CSV: MakeCSV( + WithInstallModeSupportFor(v1alpha1.InstallModeTypeAllNamespaces), + WithWebhookDefinitions( + v1alpha1.WebhookDescription{ + Type: v1alpha1.ValidatingAdmissionWebhook, + GenerateName: "webhook-a", + Rules: []admissionregistrationv1.RuleWithOperations{ + { + Rule: admissionregistrationv1.Rule{ + APIGroups: []string{"admissionregistration.k8s.io"}, + Resources: []string{"mutatingwebhookconfiguration"}, + }, + }, + }, + }, + ), + ), + }, + expectedErrs: []error{ + errors.New("webhook \"webhook-a\" contains forbidden rule: admission webhook rules cannot reference resource \"mutatingwebhookconfiguration\" for API group \"admissionregistration.k8s.io\""), + }, + }, + { + name: "reject bundles with webhook definitions with rules containing 'admissionregistration.k8s.io' api group and 'mutatingwebhookconfigurations' resource", + bundle: &bundle.RegistryV1{ + CSV: MakeCSV( + WithInstallModeSupportFor(v1alpha1.InstallModeTypeAllNamespaces), + WithWebhookDefinitions( + v1alpha1.WebhookDescription{ + Type: v1alpha1.ValidatingAdmissionWebhook, + GenerateName: "webhook-a", + Rules: []admissionregistrationv1.RuleWithOperations{ + { + Rule: admissionregistrationv1.Rule{ + APIGroups: []string{"admissionregistration.k8s.io"}, + Resources: []string{"mutatingwebhookconfigurations"}, + }, + }, + }, + }, + ), + ), + }, + expectedErrs: []error{ + errors.New("webhook \"webhook-a\" contains forbidden rule: admission webhook rules cannot reference resource \"mutatingwebhookconfigurations\" for API group \"admissionregistration.k8s.io\""), + }, + }, + { + name: "reject bundles with webhook definitions with rules containing 'admissionregistration.k8s.io' api group and 'ValidatingWebhookConfiguration' resource", + bundle: &bundle.RegistryV1{ + CSV: MakeCSV( + WithInstallModeSupportFor(v1alpha1.InstallModeTypeAllNamespaces), + WithWebhookDefinitions( + v1alpha1.WebhookDescription{ + Type: v1alpha1.ValidatingAdmissionWebhook, + GenerateName: "webhook-a", + Rules: []admissionregistrationv1.RuleWithOperations{ + { + Rule: admissionregistrationv1.Rule{ + APIGroups: []string{"admissionregistration.k8s.io"}, + Resources: []string{"ValidatingWebhookConfiguration"}, + }, + }, + }, + }, + ), + ), + }, + expectedErrs: []error{ + errors.New("webhook \"webhook-a\" contains forbidden rule: admission webhook rules cannot reference resource \"ValidatingWebhookConfiguration\" for API group \"admissionregistration.k8s.io\""), + }, + }, + { + name: "reject bundles with webhook definitions with rules containing 'admissionregistration.k8s.io' api group and 'validatingwebhookconfiguration' resource", + bundle: &bundle.RegistryV1{ + CSV: MakeCSV( + WithInstallModeSupportFor(v1alpha1.InstallModeTypeAllNamespaces), + WithWebhookDefinitions( + v1alpha1.WebhookDescription{ + Type: v1alpha1.ValidatingAdmissionWebhook, + GenerateName: "webhook-a", + Rules: []admissionregistrationv1.RuleWithOperations{ + { + Rule: admissionregistrationv1.Rule{ + APIGroups: []string{"admissionregistration.k8s.io"}, + Resources: []string{"validatingwebhookconfiguration"}, + }, + }, + }, + }, + ), + ), + }, + expectedErrs: []error{ + errors.New("webhook \"webhook-a\" contains forbidden rule: admission webhook rules cannot reference resource \"validatingwebhookconfiguration\" for API group \"admissionregistration.k8s.io\""), + }, + }, + { + name: "reject bundles with webhook definitions with rules containing 'admissionregistration.k8s.io' api group and 'validatingwebhookconfigurations' resource", + bundle: &bundle.RegistryV1{ + CSV: MakeCSV( + WithInstallModeSupportFor(v1alpha1.InstallModeTypeAllNamespaces), + WithWebhookDefinitions( + v1alpha1.WebhookDescription{ + Type: v1alpha1.ValidatingAdmissionWebhook, + GenerateName: "webhook-a", + Rules: []admissionregistrationv1.RuleWithOperations{ + { + Rule: admissionregistrationv1.Rule{ + APIGroups: []string{"admissionregistration.k8s.io"}, + Resources: []string{"validatingwebhookconfigurations"}, + }, + }, + }, + }, + ), + ), + }, + expectedErrs: []error{ + errors.New("webhook \"webhook-a\" contains forbidden rule: admission webhook rules cannot reference resource \"validatingwebhookconfigurations\" for API group \"admissionregistration.k8s.io\""), + }, + }, + } { + t.Run(tc.name, func(t *testing.T) { + errs := validators.CheckWebhookRules(tc.bundle) + require.Equal(t, tc.expectedErrs, errs) + }) + } +} + func Test_CheckWebhookDeploymentReferentialIntegrity(t *testing.T) { for _, tc := range []struct { name string From 5d5a7b601a3248634e53841091c6940ec157d7b2 Mon Sep 17 00:00:00 2001 From: Per Goncalves da Silva Date: Fri, 19 Sep 2025 09:48:43 +0200 Subject: [PATCH 2/3] fix ups Signed-off-by: Per Goncalves da Silva --- .../render/registryv1/validators/validator.go | 22 ++++++++++++++----- 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/internal/operator-controller/rukpak/render/registryv1/validators/validator.go b/internal/operator-controller/rukpak/render/registryv1/validators/validator.go index 63e8bbc4a..f11c55890 100644 --- a/internal/operator-controller/rukpak/render/registryv1/validators/validator.go +++ b/internal/operator-controller/rukpak/render/registryv1/validators/validator.go @@ -265,12 +265,12 @@ func CheckWebhookNameIsDNS1123SubDomain(rv1 *bundle.RegistryV1) []error { return errs } -// unsupportedWebhookRuleAPIGroups contain the API groups that are unsupported for webhook configuration rules in OLMv1 -var unsupportedWebhookRuleAPIGroups = sets.New("olm.operatorframework.io", "*") +// forbiddenWebhookRuleAPIGroups contain the API groups that are forbidden for webhook configuration rules in OLMv1 +var forbiddenWebhookRuleAPIGroups = sets.New("olm.operatorframework.io", "*") -// unsupportedAdmissionRegistrationResources contain the resources that are unsupported for webhook configuration rules +// forbiddenAdmissionRegistrationResources contain the resources that are forbidden for webhook configuration rules // for the admissionregistration.k8s.io api group -var unsupportedAdmissionRegistrationResources = sets.New( +var forbiddenAdmissionRegistrationResources = sets.New( "*", "mutatingwebhookconfiguration", "mutatingwebhookconfigurations", @@ -279,6 +279,16 @@ var unsupportedAdmissionRegistrationResources = sets.New( ) // CheckWebhookRules ensures webhook rules do not reference forbidden API groups or resources in line with OLMv0 behavior +// The following are forbidden, rules targeting: +// - all API groups (i.e. '*') +// - OLMv1 API group (i.e. 'olm.operatorframework.io') +// - all resources under the 'admissionregistration.k8s.io' API group +// - the 'ValidatingWebhookConfiguration' resource under the 'admissionregistration.k8s.io' API group +// - the 'MutatingWebhookConfiguration' resource under the 'admissionregistration.k8s.io' API group +// +// These boundaries attempt to reduce the blast radius of faulty webhooks and avoid deadlocks preventing the user +// from deleting OLMv1 resources installing and managing the faulty webhook, or deleting faulty admission webhook +// configurations. // See https://github.com/operator-framework/operator-lifecycle-manager/blob/ccf0c4c91f1e7673e87f3a18947f9a1f88d48438/pkg/controller/install/webhook.go#L19 // for more details func CheckWebhookRules(rv1 *bundle.RegistryV1) []error { @@ -291,12 +301,12 @@ func CheckWebhookRules(rv1 *bundle.RegistryV1) []error { webhookName := wh.GenerateName for _, rule := range wh.Rules { for _, apiGroup := range rule.APIGroups { - if unsupportedWebhookRuleAPIGroups.Has(apiGroup) { + if forbiddenWebhookRuleAPIGroups.Has(apiGroup) { errs = append(errs, fmt.Errorf("webhook %q contains forbidden rule: admission webhook rules cannot reference API group %q", webhookName, apiGroup)) } if apiGroup == "admissionregistration.k8s.io" { for _, resource := range rule.Resources { - if unsupportedAdmissionRegistrationResources.Has(strings.ToLower(resource)) { + if forbiddenAdmissionRegistrationResources.Has(strings.ToLower(resource)) { errs = append(errs, fmt.Errorf("webhook %q contains forbidden rule: admission webhook rules cannot reference resource %q for API group %q", webhookName, resource, apiGroup)) } } From d42452a172959bf6bca46c8c7c25fb81cee6d623 Mon Sep 17 00:00:00 2001 From: Per Goncalves da Silva Date: Wed, 17 Sep 2025 16:29:12 +0200 Subject: [PATCH 3/3] Add webhook rule checker to validator Signed-off-by: Per Goncalves da Silva --- .../operator-controller/rukpak/render/registryv1/registryv1.go | 1 + .../rukpak/render/registryv1/registryv1_test.go | 1 + 2 files changed, 2 insertions(+) diff --git a/internal/operator-controller/rukpak/render/registryv1/registryv1.go b/internal/operator-controller/rukpak/render/registryv1/registryv1.go index 6621a6ca4..fc38d884b 100644 --- a/internal/operator-controller/rukpak/render/registryv1/registryv1.go +++ b/internal/operator-controller/rukpak/render/registryv1/registryv1.go @@ -27,6 +27,7 @@ var BundleValidator = render.BundleValidator{ validators.CheckWebhookNameIsDNS1123SubDomain, validators.CheckConversionWebhookCRDReferenceUniqueness, validators.CheckConversionWebhooksReferenceOwnedCRDs, + validators.CheckWebhookRules, } // ResourceGenerators a slice of ResourceGenerators required to generate plain resource manifests for diff --git a/internal/operator-controller/rukpak/render/registryv1/registryv1_test.go b/internal/operator-controller/rukpak/render/registryv1/registryv1_test.go index c75f1d602..7eacaff73 100644 --- a/internal/operator-controller/rukpak/render/registryv1/registryv1_test.go +++ b/internal/operator-controller/rukpak/render/registryv1/registryv1_test.go @@ -31,6 +31,7 @@ func Test_BundleValidatorHasAllValidationFns(t *testing.T) { validators.CheckWebhookNameIsDNS1123SubDomain, validators.CheckConversionWebhookCRDReferenceUniqueness, validators.CheckConversionWebhooksReferenceOwnedCRDs, + validators.CheckWebhookRules, } actualValidationFns := registryv1.BundleValidator