Skip to content

🐛 make create verb a fixed namespaced collection verb for preflight checks#2587

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
openshift-merge-bot merged 1 commit into from
Mar 30, 2026
Merged

🐛 make create verb a fixed namespaced collection verb for preflight checks #2587

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion cmd/operator-controller/main.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -743,7 +743,6 @@ func (c *helmReconcilerConfigurator) Configure(ceReconciler *controllers.Cluster
c.mgr.GetClient(),
// Additional verbs / bundle manifest that are expected by the content manager to watch those resources
authorization.WithClusterCollectionVerbs("list", "watch"),
authorization.WithNamespacedCollectionVerbs("create"),
)
}

Expand Down
27 changes: 13 additions & 14 deletions internal/operator-controller/authorization/rbac.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -57,8 +57,16 @@ type ScopedPolicyRules struct {
MissingRules []rbacv1.PolicyRule
}

// objectVerbs are the verbs checked for each specific resource (by name) in the manifest.
// These verbs operate on existing resource instances and can be restricted by resourceNames in RBAC rules.
var objectVerbs = []string{"get", "patch", "update", "delete"}

// namespacedCollectionVerbs are the verbs checked once per unique namespace across all resources in a GVR.
// These verbs operate at the resource type level (collection) and cannot be restricted by resourceNames.
// The "create" verb is included here because it operates on the collection level - you can't specify
// resourceNames for create operations since the resource doesn't exist yet.
var namespacedCollectionVerbs = []string{"create"}

type RBACPreAuthorizerOption func(*rbacPreAuthorizer)

// WithClusterCollectionVerbs configures cluster-scoped collection verbs (e.g. list, watch)
Expand All@@ -69,20 +77,11 @@ func WithClusterCollectionVerbs(verbs ...string) RBACPreAuthorizerOption {
}
}

// WithNamespacedCollectionVerbs configures namespaced collection verbs (e.g. create)
// that are checked for each unique namespace across all objects in a GVR.
func WithNamespacedCollectionVerbs(verbs ...string) RBACPreAuthorizerOption {
return func(a *rbacPreAuthorizer) {
a.namespacedCollectionVerbs = slices.Clone(verbs)
}
}

type rbacPreAuthorizer struct {
authorizer authorizer.Authorizer
ruleResolver validation.AuthorizationRuleResolver
restMapper meta.RESTMapper
clusterCollectionVerbs []string
namespacedCollectionVerbs []string
authorizer authorizer.Authorizer
ruleResolver validation.AuthorizationRuleResolver
restMapper meta.RESTMapper
clusterCollectionVerbs []string
}

func NewRBACPreAuthorizer(cl client.Client, opts ...RBACPreAuthorizerOption) PreAuthorizer {
Expand All@@ -104,7 +103,7 @@ func (a *rbacPreAuthorizer) PreAuthorize(ctx context.Context, user user.Info, ma
}

// derive manifest related attributes records
attributesRecords := dm.asAuthorizationAttributesRecordsForUser(user, a.clusterCollectionVerbs, a.namespacedCollectionVerbs)
attributesRecords := dm.asAuthorizationAttributesRecordsForUser(user, a.clusterCollectionVerbs, namespacedCollectionVerbs)

// append additional required perms
for _, fn := range additionalRequiredPerms {
Expand Down
144 changes: 18 additions & 126 deletions internal/operator-controller/authorization/rbac_test.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -396,7 +396,7 @@ func setupFakeClient(role client.Object) client.Client {
func TestPreAuthorize_Success(t *testing.T) {
t.Run("preauthorize succeeds with no missing rbac rules", func(t *testing.T) {
fakeClient := setupFakeClient(privilegedClusterRole)
preAuth := NewRBACPreAuthorizer(fakeClient, WithClusterCollectionVerbs("list", "watch"), WithNamespacedCollectionVerbs("create"))
preAuth := NewRBACPreAuthorizer(fakeClient, WithClusterCollectionVerbs("list", "watch"))
missingRules, err := preAuth.PreAuthorize(context.TODO(), testUser, strings.NewReader(testManifest))
require.NoError(t, err)
require.Equal(t, []ScopedPolicyRules{}, missingRules)
Expand All@@ -406,7 +406,7 @@ func TestPreAuthorize_Success(t *testing.T) {
func TestPreAuthorize_MissingRBAC(t *testing.T) {
t.Run("preauthorize fails and finds missing rbac rules", func(t *testing.T) {
fakeClient := setupFakeClient(limitedClusterRole)
preAuth := NewRBACPreAuthorizer(fakeClient, WithClusterCollectionVerbs("list", "watch"), WithNamespacedCollectionVerbs("create"))
preAuth := NewRBACPreAuthorizer(fakeClient, WithClusterCollectionVerbs("list", "watch"))
missingRules, err := preAuth.PreAuthorize(context.TODO(), testUser, strings.NewReader(testManifest))
require.NoError(t, err)
require.Equal(t, expectedSingleNamespaceMissingRules, missingRules)
Expand All@@ -416,7 +416,7 @@ func TestPreAuthorize_MissingRBAC(t *testing.T) {
func TestPreAuthorizeMultiNamespace_MissingRBAC(t *testing.T) {
t.Run("preauthorize fails and finds missing rbac rules in multiple namespaces", func(t *testing.T) {
fakeClient := setupFakeClient(limitedClusterRole)
preAuth := NewRBACPreAuthorizer(fakeClient, WithClusterCollectionVerbs("list", "watch"), WithNamespacedCollectionVerbs("create"))
preAuth := NewRBACPreAuthorizer(fakeClient, WithClusterCollectionVerbs("list", "watch"))
missingRules, err := preAuth.PreAuthorize(context.TODO(), testUser, strings.NewReader(testManifestMultiNamespace))
require.NoError(t, err)
require.Equal(t, expectedMultiNamespaceMissingRules, missingRules)
Expand All@@ -426,7 +426,7 @@ func TestPreAuthorizeMultiNamespace_MissingRBAC(t *testing.T) {
func TestPreAuthorize_CheckEscalation(t *testing.T) {
t.Run("preauthorize succeeds with no missing rbac rules", func(t *testing.T) {
fakeClient := setupFakeClient(escalatingClusterRole)
preAuth := NewRBACPreAuthorizer(fakeClient, WithClusterCollectionVerbs("list", "watch"), WithNamespacedCollectionVerbs("create"))
preAuth := NewRBACPreAuthorizer(fakeClient, WithClusterCollectionVerbs("list", "watch"))
missingRules, err := preAuth.PreAuthorize(context.TODO(), testUser, strings.NewReader(testManifest))
require.NoError(t, err)
require.Equal(t, []ScopedPolicyRules{}, missingRules)
Expand All@@ -436,7 +436,7 @@ func TestPreAuthorize_CheckEscalation(t *testing.T) {
func TestPreAuthorize_AdditionalRequiredPerms_MissingRBAC(t *testing.T) {
t.Run("preauthorize fails and finds missing rbac rules coming from the additional required permissions", func(t *testing.T) {
fakeClient := setupFakeClient(escalatingClusterRole)
preAuth := NewRBACPreAuthorizer(fakeClient, WithClusterCollectionVerbs("list", "watch"), WithNamespacedCollectionVerbs("create"))
preAuth := NewRBACPreAuthorizer(fakeClient, WithClusterCollectionVerbs("list", "watch"))
missingRules, err := preAuth.PreAuthorize(context.TODO(), testUser, strings.NewReader(testManifest), func(user user.Info) []authorizer.AttributesRecord {
return []authorizer.AttributesRecord{
{
Expand DownExpand Up@@ -514,7 +514,7 @@ func TestPreAuthorize_WithClusterCollectionVerbs(t *testing.T) {

t.Run("no cluster collection verbs option omits cluster-scoped collection rules", func(t *testing.T) {
fakeClient := setupFakeClient(limitedClusterRole)
preAuth := NewRBACPreAuthorizer(fakeClient, WithNamespacedCollectionVerbs("create"))
preAuth := NewRBACPreAuthorizer(fakeClient)
missingRules, err := preAuth.PreAuthorize(context.TODO(), testUser, strings.NewReader(testManifest))
require.NoError(t, err)
// With no cluster collection verbs, there should be no cluster-scoped (namespace="") missing rules
Expand All@@ -523,7 +523,7 @@ func TestPreAuthorize_WithClusterCollectionVerbs(t *testing.T) {

t.Run("cluster verbs option only checks those verbs at cluster scope", func(t *testing.T) {
fakeClient := setupFakeClient(limitedClusterRole)
preAuth := NewRBACPreAuthorizer(fakeClient, WithClusterCollectionVerbs("get", "patch", "update"), WithNamespacedCollectionVerbs("create"))
preAuth := NewRBACPreAuthorizer(fakeClient, WithClusterCollectionVerbs("get", "patch", "update"))
missingRules, err := preAuth.PreAuthorize(context.TODO(), testUser, strings.NewReader(testManifest))
require.NoError(t, err)
require.Equal(t, []ScopedPolicyRules{
Expand DownExpand Up@@ -557,139 +557,31 @@ func TestPreAuthorize_WithClusterCollectionVerbs(t *testing.T) {

t.Run("privileged user with no cluster collection verbs succeeds", func(t *testing.T) {
fakeClient := setupFakeClient(privilegedClusterRole)
preAuth := NewRBACPreAuthorizer(fakeClient, WithNamespacedCollectionVerbs("create"))
preAuth := NewRBACPreAuthorizer(fakeClient)
missingRules, err := preAuth.PreAuthorize(context.TODO(), testUser, strings.NewReader(testManifest))
require.NoError(t, err)
require.Equal(t, []ScopedPolicyRules{}, missingRules)
})
}

func TestPreAuthorize_WithNamespacedCollectionVerbs(t *testing.T) {
// expectedClusterMissingRules are the missing rules expected at cluster scope
// when cluster collection verbs are configured as "list", "watch".
expectedClusterMissingRules := ScopedPolicyRules{
Namespace: "",
MissingRules: []rbacv1.PolicyRule{
{
Verbs: []string{"list", "watch"},
APIGroups: []string{""},
Resources: []string{"services"},
ResourceNames: []string(nil),
NonResourceURLs: []string(nil)},
{
Verbs: []string{"list", "watch"},
APIGroups: []string{"rbac.authorization.k8s.io"},
Resources: []string{"rolebindings"},
ResourceNames: []string(nil),
NonResourceURLs: []string(nil)},
{
Verbs: []string{"list", "watch"},
APIGroups: []string{"rbac.authorization.k8s.io"},
Resources: []string{"roles"},
ResourceNames: []string(nil),
NonResourceURLs: []string(nil),
},
},
}
func TestPreAuthorize_NamespacedCollectionVerbs(t *testing.T) {
// With namespacedCollectionVerbs now being a fixed variable (containing "create"),
// this test verifies that "create" permissions are always checked at the namespace level.

t.Run("no namespaced collection verbs option omits namespaced collection rules", func(t *testing.T) {
t.Run("create verb is always checked as namespaced collection verb", func(t *testing.T) {
fakeClient := setupFakeClient(limitedClusterRole)
preAuth := NewRBACPreAuthorizer(fakeClient, WithClusterCollectionVerbs("list", "watch"))
missingRules, err := preAuth.PreAuthorize(context.TODO(), testUser, strings.NewReader(testManifest))
require.NoError(t, err)
// Without namespaced collection verbs, no "create" rules from collection verbs should appear,
// but object verbs (get, patch, update, delete) and escalation checks still apply
require.Equal(t, []ScopedPolicyRules{
expectedClusterMissingRules,
{
Namespace: "test-namespace",
MissingRules: []rbacv1.PolicyRule{
{
Verbs: []string{"create"},
APIGroups: []string{"*"},
Resources: []string{"certificates"}},
{
Verbs: []string{"delete", "get", "patch", "update"},
APIGroups: []string{""},
Resources: []string{"services"},
ResourceNames: []string{"test-service"}},
{
Verbs: []string{"delete", "get", "patch", "update"},
APIGroups: []string{"rbac.authorization.k8s.io"},
Resources: []string{"rolebindings"},
ResourceNames: []string{"test-extension-binding"}},
{
Verbs: []string{"delete", "get", "patch", "update"},
APIGroups: []string{"rbac.authorization.k8s.io"},
Resources: []string{"roles"},
ResourceNames: []string{"test-extension-role"}},
{
Verbs: []string{"watch"},
APIGroups: []string{"*"},
Resources: []string{"serviceaccounts"},
},
},
},
}, missingRules)
})

t.Run("namespaced collection verbs option checks those verbs per namespace", func(t *testing.T) {
fakeClient := setupFakeClient(limitedClusterRole)
preAuth := NewRBACPreAuthorizer(fakeClient, WithClusterCollectionVerbs("list", "watch"), WithNamespacedCollectionVerbs("create", "deletecollection"))
missingRules, err := preAuth.PreAuthorize(context.TODO(), testUser, strings.NewReader(testManifest))
require.NoError(t, err)
// Should have cluster-scoped missing rules plus namespaced rules with both create and deletecollection.
// Note: "certificates" with apiGroup "*" comes from the escalation check on the Role, not
// from namespaced collection verbs, so it only has "create".
require.Equal(t, []ScopedPolicyRules{
expectedClusterMissingRules,
{
Namespace: "test-namespace",
MissingRules: []rbacv1.PolicyRule{
{
Verbs: []string{"create", "deletecollection"},
APIGroups: []string{""},
Resources: []string{"services"}},
{
Verbs: []string{"create", "deletecollection"},
APIGroups: []string{"rbac.authorization.k8s.io"},
Resources: []string{"rolebindings"}},
{
Verbs: []string{"create", "deletecollection"},
APIGroups: []string{"rbac.authorization.k8s.io"},
Resources: []string{"roles"}},
{
Verbs: []string{"create"},
APIGroups: []string{"*"},
Resources: []string{"certificates"}},
{
Verbs: []string{"delete", "get", "patch", "update"},
APIGroups: []string{""},
Resources: []string{"services"},
ResourceNames: []string{"test-service"}},
{
Verbs: []string{"delete", "get", "patch", "update"},
APIGroups: []string{"rbac.authorization.k8s.io"},
Resources: []string{"rolebindings"},
ResourceNames: []string{"test-extension-binding"}},
{
Verbs: []string{"delete", "get", "patch", "update"},
APIGroups: []string{"rbac.authorization.k8s.io"},
Resources: []string{"roles"},
ResourceNames: []string{"test-extension-role"}},
{
Verbs: []string{"watch"},
APIGroups: []string{"*"},
Resources: []string{"serviceaccounts"},
},
},
},
}, missingRules)
// The "create" verb should always appear in missing rules because it's part of the
// namespacedCollectionVerbs. This test verifies expectedSingleNamespaceMissingRules
// which includes "create" verbs for namespace-scoped resources.
require.Equal(t, expectedSingleNamespaceMissingRules, missingRules)
})

t.Run("privileged user with custom namespaced collection verbs succeeds", func(t *testing.T) {
t.Run("privileged user with namespaced collection verbs succeeds", func(t *testing.T) {
fakeClient := setupFakeClient(privilegedClusterRole)
preAuth := NewRBACPreAuthorizer(fakeClient, WithNamespacedCollectionVerbs("create", "deletecollection"))
preAuth := NewRBACPreAuthorizer(fakeClient)
missingRules, err := preAuth.PreAuthorize(context.TODO(), testUser, strings.NewReader(testManifest))
require.NoError(t, err)
require.Equal(t, []ScopedPolicyRules{}, missingRules)
Expand Down
35 changes: 35 additions & 0 deletions test/e2e/features/install.feature
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -535,3 +535,38 @@ Feature: Install ClusterExtension
nodeSelector:
kubernetes.io/os: linux
"""

@BoxcutterRuntime
@PreflightPermissions
Scenario: Boxcutter preflight check detects missing CREATE permissions
Given ServiceAccount "olm-sa" without create permissions is available in ${TEST_NAMESPACE}
And ClusterExtension is applied
"""
apiVersion: olm.operatorframework.io/v1
kind: ClusterExtension
metadata:
name: ${NAME}
spec:
namespace: ${TEST_NAMESPACE}
serviceAccount:
name: olm-sa
source:
sourceType: Catalog
catalog:
packageName: test
selector:
matchLabels:
"olm.operatorframework.io/metadata.name": test-catalog
"""
And ClusterExtension reports Progressing as True with Reason Retrying and Message includes:
"""
pre-authorization failed: service account requires the following permissions to manage cluster extension
"""
And ClusterExtension reports Progressing as True with Reason Retrying and Message includes:
"""
Verbs:[create]
"""
When ServiceAccount "olm-sa" with needed permissions is available in ${TEST_NAMESPACE}
Then ClusterExtension is available
And ClusterExtension reports Progressing as True with Reason Succeeded
And ClusterExtension reports Installed as True
24 changes: 24 additions & 0 deletions test/e2e/steps/steps.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -34,6 +34,7 @@ import (
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/util/sets"
k8sresource "k8s.io/cli-runtime/pkg/resource"
"k8s.io/component-base/featuregate"
"k8s.io/utils/ptr"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/yaml"
Expand DownExpand Up@@ -120,6 +121,7 @@ func RegisterSteps(sc *godog.ScenarioContext) {
sc.Step(`^(?i)ServiceAccount "([^"]*)" with permissions to install extensions is available in "([^"]*)" namespace$`, ServiceAccountWithNeededPermissionsIsAvailableInGivenNamespace)
sc.Step(`^(?i)ServiceAccount "([^"]*)" with needed permissions is available in test namespace$`, ServiceAccountWithNeededPermissionsIsAvailableInTestNamespace)
sc.Step(`^(?i)ServiceAccount "([^"]*)" with needed permissions is available in \${TEST_NAMESPACE}$`, ServiceAccountWithNeededPermissionsIsAvailableInTestNamespace)
sc.Step(`^(?i)ServiceAccount "([^"]*)" without create permissions is available in \${TEST_NAMESPACE}$`, ServiceAccountWithoutCreatePermissionsIsAvailableInTestNamespace)
sc.Step(`^(?i)ServiceAccount "([^"]*)" is available in \${TEST_NAMESPACE}$`, ServiceAccountIsAvailableInNamespace)
sc.Step(`^(?i)ServiceAccount "([^"]*)" in test namespace is cluster admin$`, ServiceAccountWithClusterAdminPermissionsIsAvailableInNamespace)
sc.Step(`^(?i)ServiceAccount "([^"]+)" in test namespace has permissions to fetch "([^"]+)" metrics$`, ServiceAccountWithFetchMetricsPermissions)
Expand DownExpand Up@@ -418,6 +420,11 @@ type msgMatchFn func(string) bool

func alwaysMatch(_ string) bool { return true }

func isFeatureGateEnabled(feature featuregate.Feature) bool {
enabled, found := featureGates[feature]
return enabled && found
}
Comment thread
kuiwang02 marked this conversation as resolved.

func messageComparison(ctx context.Context, msg *godog.DocString) msgMatchFn {
msgCmp := alwaysMatch
if msg != nil {
Expand DownExpand Up@@ -877,6 +884,23 @@ func ServiceAccountWithNeededPermissionsIsAvailableInTestNamespace(ctx context.C
return applyPermissionsToServiceAccount(ctx, serviceAccount, rbacTemplate)
}

// ServiceAccountWithoutCreatePermissionsIsAvailableInTestNamespace creates a ServiceAccount with permissions that
// intentionally exclude the "create" verb to test preflight permission validation for Boxcutter applier.
// This is used to verify that the preflight check properly detects missing CREATE permissions.
// Note: This function requires both @BoxcutterRuntime and @PreflightPermissions tags.
func ServiceAccountWithoutCreatePermissionsIsAvailableInTestNamespace(ctx context.Context, serviceAccount string) error {
// This test is only valid with Boxcutter runtime enabled
if !isFeatureGateEnabled(features.BoxcutterRuntime) {
return fmt.Errorf("this step requires BoxcutterRuntime feature gate to be enabled")
}
Comment thread
kuiwang02 marked this conversation as resolved.
// It also requires preflight permissions checks to be enabled
if !isFeatureGateEnabled(features.PreflightPermissions) {
return fmt.Errorf("this step requires PreflightPermissions feature gate to be enabled")
}
rbacTemplate := fmt.Sprintf("%s-boxcutter-no-create-rbac-template.yaml", serviceAccount)
return applyPermissionsToServiceAccount(ctx, serviceAccount, rbacTemplate)
}

// ServiceAccountWithNeededPermissionsIsAvailableInGivenNamespace creates a ServiceAccount and enables creation of any cluster extension on behalf of this account.
func ServiceAccountWithNeededPermissionsIsAvailableInGivenNamespace(ctx context.Context, serviceAccount string, ns string) error {
sc := scenarioCtx(ctx)
Expand Down
Loading
Loading