Clean up escalation error parsing and fix tests

Pass in the clusterextension to PreAuthorize instead of the user.Info
since we need the extension to create the clusterextension/finalizer

Signed-off-by: Tayler Geiger <[email protected]>

Author: trgeiger

internal/operator-controller/applier/helm.go

@@ -19,7 +19,6 @@ import (
 	rbacv1 "k8s.io/api/rbac/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	apimachyaml "k8s.io/apimachinery/pkg/util/yaml"
-	"k8s.io/apiserver/pkg/authentication/user"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/log"
 
@@ -100,8 +99,7 @@ func (h *Helm) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExte
 			return nil, "", fmt.Errorf("failed to get release state using client-only dry-run: %w", err)
 		}
 
-		ceServiceAccount := user.DefaultInfo{Name: fmt.Sprintf("system:serviceaccount:%s:%s", ext.Spec.Namespace, ext.Spec.ServiceAccount.Name)}
-		missingRules, err := h.PreAuthorizer.PreAuthorize(ctx, &ceServiceAccount, strings.NewReader(tmplRel.Manifest))
+		missingRules, err := h.PreAuthorizer.PreAuthorize(ctx, ext, strings.NewReader(tmplRel.Manifest))
 
 		var preAuthErrors []error
 		if len(missingRules) > 0 {

internal/operator-controller/applier/helm_test.go

@@ -17,7 +17,6 @@ import (
 	"helm.sh/helm/v3/pkg/storage/driver"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apiserver/pkg/authentication/user"
 	featuregatetesting "k8s.io/component-base/featuregate/testing"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
@@ -39,7 +38,7 @@ type noOpPreAuthorizer struct{}
 
 func (p *noOpPreAuthorizer) PreAuthorize(
 	ctx context.Context,
-	manifestManager user.Info,
+	ext *ocv1.ClusterExtension,
 	manifestReader io.Reader,
 ) ([]authorization.ScopedPolicyRules, error) {
 	// No-op: always return an empty map and no error

internal/operator-controller/authorization/rbac.go

@@ -6,11 +6,11 @@ import (
 	"fmt"
 	"io"
 	"maps"
-	"reflect"
 	"regexp"
 	"sort"
 	"strings"
 
+	ocv1 "github.com/operator-framework/operator-controller/api/v1"
 	corev1 "k8s.io/api/core/v1"
 	rbacv1 "k8s.io/api/rbac/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
@@ -34,7 +34,7 @@ import (
 )
 
 type PreAuthorizer interface {
-	PreAuthorize(ctx context.Context, manifestManager user.Info, manifestReader io.Reader) ([]ScopedPolicyRules, error)
+	PreAuthorize(ctx context.Context, ext *ocv1.ClusterExtension, manifestReader io.Reader) ([]ScopedPolicyRules, error)
 }
 
 type ScopedPolicyRules struct {
@@ -69,13 +69,14 @@ func NewRBACPreAuthorizer(cl client.Client) PreAuthorizer {
 //     completes successfully but identifies missing rules, then a nil error is returned along with
 //     the list (or slice) of missing rules. Note that in some cases the error may encapsulate multiple
 //     evaluation failures
-func (a *rbacPreAuthorizer) PreAuthorize(ctx context.Context, manifestManager user.Info, manifestReader io.Reader) ([]ScopedPolicyRules, error) {
+func (a *rbacPreAuthorizer) PreAuthorize(ctx context.Context, ext *ocv1.ClusterExtension, manifestReader io.Reader) ([]ScopedPolicyRules, error) {
 	var allMissingPolicyRules = []ScopedPolicyRules{}
 	dm, err := a.decodeManifest(manifestReader)
 	if err != nil {
 		return nil, err
 	}
-	attributesRecords := dm.asAuthorizationAttributesRecordsForUser(manifestManager)
+	manifestManager := &user.DefaultInfo{Name: fmt.Sprintf("system:serviceaccount:%s:%s", ext.Spec.Namespace, ext.Spec.ServiceAccount.Name)}
+	attributesRecords := dm.asAuthorizationAttributesRecordsForUser(manifestManager, ext)
 
 	var preAuthEvaluationErrors []error
 	missingRules, err := a.authorizeAttributesRecords(ctx, attributesRecords)
@@ -92,14 +93,8 @@ func (a *rbacPreAuthorizer) PreAuthorize(ctx context.Context, manifestManager us
 
 	for _, obj := range dm.rbacObjects() {
 		if err := ec.checkEscalation(ctx, manifestManager, obj); err != nil {
-			missingEscalationRules, namespace := parseEscalationErrorForMissingRules(err)
-			// Check if we already have these escalation PolicyRules, if so don't append
-			for i, rule := range missingEscalationRules {
-				previousRule := missingRules[namespace][len(missingRules[namespace])-len(missingEscalationRules)+i]
-				if !arePolicyRulesEqual(previousRule, rule) {
-					missingRules[namespace] = append(missingRules[namespace], missingEscalationRules...)
-				}
-			}
+			missingEscalationRules, err := parseEscalationErrorForMissingRules(err)
+			missingRules[obj.GetNamespace()] = append(missingRules[obj.GetNamespace()], missingEscalationRules...)
 			preAuthEvaluationErrors = append(preAuthEvaluationErrors, err)
 		}
 	}
@@ -112,7 +107,20 @@ func (a *rbacPreAuthorizer) PreAuthorize(ctx context.Context, manifestManager us
 		if compactMissingRules, err := validation.CompactRules(nsMissingRules); err == nil {
 			missingRules[ns] = compactMissingRules
 		}
-		sortableRules := rbacv1helpers.SortableRuleSlice(missingRules[ns])
+
+		missingRulesWithDeduplicatedVerbs := []rbacv1.PolicyRule{}
+		for _, rule := range missingRules[ns] {
+			verbSet := sets.New[string](rule.Verbs...)
+			if verbSet.Has("*") {
+				rule.Verbs = []string{"*"}
+			} else {
+				rule.Verbs = sets.List(verbSet)
+			}
+			missingRulesWithDeduplicatedVerbs = append(missingRulesWithDeduplicatedVerbs, rule)
+		}
+
+		sortableRules := rbacv1helpers.SortableRuleSlice(missingRulesWithDeduplicatedVerbs)
+
 		sort.Sort(sortableRules)
 		allMissingPolicyRules = append(allMissingPolicyRules, ScopedPolicyRules{Namespace: ns, MissingRules: sortableRules})
 	}
@@ -284,7 +292,7 @@ func (dm *decodedManifest) rbacObjects() []client.Object {
 	return objects
 }
 
-func (dm *decodedManifest) asAuthorizationAttributesRecordsForUser(manifestManager user.Info) []authorizer.AttributesRecord {
+func (dm *decodedManifest) asAuthorizationAttributesRecordsForUser(manifestManager user.Info, ext *ocv1.ClusterExtension) []authorizer.AttributesRecord {
 	var attributeRecords []authorizer.AttributesRecord
 
 	// Here we are splitting collection verbs based on required scope
@@ -338,6 +346,18 @@ func (dm *decodedManifest) asAuthorizationAttributesRecordsForUser(manifestManag
 				Verb:            v,
 			})
 		}
+
+		for _, verb := range []string{"update", "patch"} {
+			attributeRecords = append(attributeRecords, authorizer.AttributesRecord{
+				User:            manifestManager,
+				Name:            ext.Name,
+				APIGroup:        ext.GroupVersionKind().Group,
+				APIVersion:      ext.GroupVersionKind().Version,
+				Resource:        "clusterextensions/finalizers",
+				ResourceRequest: true,
+				Verb:            verb,
+			})
+		}
 	}
 	return attributeRecords
 }
@@ -518,17 +538,14 @@ var fullAuthority = []rbacv1.PolicyRule{
 	{Verbs: []string{"*"}, NonResourceURLs: []string{"*"}},
 }
 
-func parseEscalationErrorForMissingRules(ecError error) ([]rbacv1.PolicyRule, string) {
-	// Regex to capture namespace and serviceaccount
-	userRegex := regexp.MustCompile(`system:serviceaccount:(?P<Namespace>[^:]+):(?P<ServiceAccount>[^"]+)`)
-	// Regex to capture missing permissions
+func parseEscalationErrorForMissingRules(ecError error) ([]rbacv1.PolicyRule, error) {
+	errRegex := regexp.MustCompile(`(?s)^(user \".*\" \(groups=.*\) is attempting to grant RBAC permissions not currently held):.*?$`)
 	permRegex := regexp.MustCompile(`{APIGroups:\[("[^"]*")\], Resources:\[("[^"]*")\], Verbs:\[("[^"]*")\]}`)
 
-	userMatches := userRegex.FindStringSubmatch(ecError.Error())
-	namespace := userMatches[1]
+	errMatches := errRegex.FindAllStringSubmatch(ecError.Error(), -1)
 
 	// Extract permissions
-	var permissions []rbacv1.PolicyRule
+	permissions := []rbacv1.PolicyRule{}
 	permMatches := permRegex.FindAllStringSubmatch(ecError.Error(), -1)
 	for _, match := range permMatches {
 		permissions = append(permissions, rbacv1.PolicyRule{
@@ -538,20 +555,7 @@ func parseEscalationErrorForMissingRules(ecError error) ([]rbacv1.PolicyRule, st
 		})
 	}
 
-	return permissions, namespace
-}
-
-func arePolicyRulesEqual(rule1 rbacv1.PolicyRule, rule2 rbacv1.PolicyRule) bool {
-	sort.Strings(rule1.Verbs)
-	sort.Strings(rule2.Verbs)
-	sort.Strings(rule1.APIGroups)
-	sort.Strings(rule2.APIGroups)
-	sort.Strings(rule1.Resources)
-	sort.Strings(rule2.Resources)
-	sort.Strings(rule1.ResourceNames)
-	sort.Strings(rule2.ResourceNames)
-
-	return reflect.DeepEqual(rule1, rule2)
+	return permissions, errors.New(errMatches[0][1])
 }
 
 func hasAggregationRule(clusterRole *rbacv1.ClusterRole) bool {

internal/operator-controller/authorization/rbac_test.go

@@ -2,17 +2,16 @@ package authorization
 
 import (
 	"context"
-	"fmt"
 	"strings"
 	"testing"
 
+	ocv1 "github.com/operator-framework/operator-controller/api/v1"
 	"github.com/stretchr/testify/require"
 	corev1 "k8s.io/api/core/v1"
 	rbacv1 "k8s.io/api/rbac/v1"
 	"k8s.io/apimachinery/pkg/api/meta/testrestmapper"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
-	"k8s.io/apiserver/pkg/authentication/user"
 	featuregatetesting "k8s.io/component-base/featuregate/testing"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
@@ -127,9 +126,17 @@ subjects:
   namespace: a-test-namespace
   `
 
-	saName             = "test-serviceaccount"
-	ns                 = "test-namespace"
-	testServiceAccount = user.DefaultInfo{Name: fmt.Sprintf("system:serviceaccount:%s:%s", ns, saName)}
+	saName                  = "test-serviceaccount"
+	ns                      = "test-namespace"
+	exampleClusterExtension = ocv1.ClusterExtension{
+		ObjectMeta: metav1.ObjectMeta{Name: "test-cluster-extension"},
+		Spec: ocv1.ClusterExtensionSpec{
+			Namespace: ns,
+			ServiceAccount: ocv1.ServiceAccountReference{
+				Name: saName,
+			},
+		},
+	}
 
 	objects = []client.Object{
 		&corev1.Namespace{
@@ -194,7 +201,7 @@ subjects:
 		Rules: []rbacv1.PolicyRule{
 			{
 				APIGroups: []string{"*"},
-				Resources: []string{"serviceaccounts", "services"},
+				Resources: []string{"serviceaccounts", "services", "clusterextensions/finalizers"},
 				Verbs:     []string{"*"},
 			},
 			{
@@ -221,7 +228,7 @@ func TestPreAuthorize_Success(t *testing.T) {
 		featuregatetesting.SetFeatureGateDuringTest(t, features.OperatorControllerFeatureGate, features.PreflightPermissions, true)
 		fakeClient := setupFakeClient(privilegedClusterRole)
 		preAuth := NewRBACPreAuthorizer(fakeClient)
-		missingRules, err := preAuth.PreAuthorize(context.TODO(), &testServiceAccount, strings.NewReader(testManifest))
+		missingRules, err := preAuth.PreAuthorize(context.TODO(), &exampleClusterExtension, strings.NewReader(testManifest))
 		require.NoError(t, err)
 		require.Equal(t, []ScopedPolicyRules{}, missingRules)
 	})
@@ -232,7 +239,7 @@ func TestPreAuthorize_Failure(t *testing.T) {
 		featuregatetesting.SetFeatureGateDuringTest(t, features.OperatorControllerFeatureGate, features.PreflightPermissions, true)
 		fakeClient := setupFakeClient(limitedClusterRole)
 		preAuth := NewRBACPreAuthorizer(fakeClient)
-		missingRules, err := preAuth.PreAuthorize(context.TODO(), &testServiceAccount, strings.NewReader(testManifest))
+		missingRules, err := preAuth.PreAuthorize(context.TODO(), &exampleClusterExtension, strings.NewReader(testManifest))
 		require.Error(t, err)
 		require.NotEqual(t, []ScopedPolicyRules{}, missingRules)
 	})
@@ -243,7 +250,7 @@ func TestPreAuthorizeMultiNamespace_Failure(t *testing.T) {
 		featuregatetesting.SetFeatureGateDuringTest(t, features.OperatorControllerFeatureGate, features.PreflightPermissions, true)
 		fakeClient := setupFakeClient(limitedClusterRole)
 		preAuth := NewRBACPreAuthorizer(fakeClient)
-		missingRules, err := preAuth.PreAuthorize(context.TODO(), &testServiceAccount, strings.NewReader(testManifestMultiNamespace))
+		missingRules, err := preAuth.PreAuthorize(context.TODO(), &exampleClusterExtension, strings.NewReader(testManifestMultiNamespace))
 		require.Error(t, err)
 		require.NotEqual(t, []ScopedPolicyRules{}, missingRules)
 	})
@@ -254,7 +261,7 @@ func TestPreAuthorize_CheckEscalation(t *testing.T) {
 		featuregatetesting.SetFeatureGateDuringTest(t, features.OperatorControllerFeatureGate, features.PreflightPermissions, true)
 		fakeClient := setupFakeClient(escalatingClusterRole)
 		preAuth := NewRBACPreAuthorizer(fakeClient)
-		missingRules, err := preAuth.PreAuthorize(context.TODO(), &testServiceAccount, strings.NewReader(testManifest))
+		missingRules, err := preAuth.PreAuthorize(context.TODO(), &exampleClusterExtension, strings.NewReader(testManifest))
 		require.NoError(t, err)
 		require.Equal(t, []ScopedPolicyRules{}, missingRules)
 	})