Add preflight checks to Boxcutter applier

Signed-off-by: Per Goncalves da Silva <pegoncal@redhat.com>

Author: Per Goncalves da Silva

cmd/operator-controller/main.go

@@ -598,6 +598,12 @@ func (c *boxcutterReconcilerConfigurator) Configure(ceReconciler *controllers.Cl
 		return err
 	}
 
+	// determine if PreAuthorizer should be enabled based on feature gate
+	var preAuth authorization.PreAuthorizer
+	if features.OperatorControllerFeatureGate.Enabled(features.PreflightPermissions) {
+		preAuth = authorization.NewRBACPreAuthorizer(c.mgr.GetClient(), authorization.WithClusterExtensionRevisionPerms())
+	}
+
 	// TODO: add support for preflight checks
 	// TODO: better scheme handling - which types do we want to support?
 	_ = apiextensionsv1.AddToScheme(c.mgr.GetScheme())
@@ -610,6 +616,7 @@ func (c *boxcutterReconcilerConfigurator) Configure(ceReconciler *controllers.Cl
 		Scheme:            c.mgr.GetScheme(),
 		RevisionGenerator: rg,
 		Preflights:        c.preflights,
+		PreAuthorizer:     preAuth,
 		FieldOwner:        fmt.Sprintf("%s/clusterextension-controller", fieldOwnerPrefix),
 	}
 	revisionStatesGetter := &controllers.BoxcutterRevisionStatesGetter{Reader: c.mgr.GetClient()}

internal/operator-controller/applier/boxcutter.go

@@ -5,6 +5,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"io"
 	"io/fs"
 	"maps"
 	"slices"
@@ -26,6 +27,7 @@ import (
 	helmclient "github.com/operator-framework/helm-operator-plugins/pkg/client"
 
 	ocv1 "github.com/operator-framework/operator-controller/api/v1"
+	"github.com/operator-framework/operator-controller/internal/operator-controller/authorization"
 	"github.com/operator-framework/operator-controller/internal/operator-controller/labels"
 	"github.com/operator-framework/operator-controller/internal/shared/util/cache"
 )
@@ -280,23 +282,14 @@ type Boxcutter struct {
 	Scheme            *runtime.Scheme
 	RevisionGenerator ClusterExtensionRevisionGenerator
 	Preflights        []Preflight
+	PreAuthorizer     authorization.PreAuthorizer
 	FieldOwner        string
 }
 
 func (bc *Boxcutter) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (bool, string, error) {
 	return bc.apply(ctx, contentFS, ext, objectLabels, revisionAnnotations)
 }
 
-func (bc *Boxcutter) getObjects(rev *ocv1.ClusterExtensionRevision) []client.Object {
-	var objs []client.Object
-	for _, phase := range rev.Spec.Phases {
-		for _, phaseObject := range phase.Objects {
-			objs = append(objs, &phaseObject.Object)
-		}
-	}
-	return objs
-}
-
 func (bc *Boxcutter) createOrUpdate(ctx context.Context, obj client.Object) error {
 	if obj.GetObjectKind().GroupVersionKind().Empty() {
 		gvk, err := apiutil.GVKForObject(obj, bc.Scheme)
@@ -315,6 +308,11 @@ func (bc *Boxcutter) apply(ctx context.Context, contentFS fs.FS, ext *ocv1.Clust
 		return false, "", err
 	}
 
+	// Run auth preflight checks
+	if err := bc.runPreAuthorizationChecks(ctx, ext, desiredRevision); err != nil {
+		return false, "", err
+	}
+
 	if err := controllerutil.SetControllerReference(ext, desiredRevision, bc.Scheme); err != nil {
 		return false, "", fmt.Errorf("set ownerref: %w", err)
 	}
@@ -349,7 +347,7 @@ func (bc *Boxcutter) apply(ctx context.Context, contentFS fs.FS, ext *ocv1.Clust
 	}
 
 	// Preflights
-	plainObjs := bc.getObjects(desiredRevision)
+	plainObjs := getObjects(desiredRevision)
 	for _, preflight := range bc.Preflights {
 		if shouldSkipPreflight(ctx, preflight, ext, state) {
 			continue
@@ -411,6 +409,20 @@ func (bc *Boxcutter) apply(ctx context.Context, contentFS fs.FS, ext *ocv1.Clust
 	return true, "", nil
 }
 
+// runPreAuthorizationChecks runs PreAuthorization checks if the PreAuthorizer is set. An error will be returned if
+// the ClusterExtension service account does not have the necessary permissions to manage to the revision's resources
+func (bc *Boxcutter) runPreAuthorizationChecks(ctx context.Context, ext *ocv1.ClusterExtension, rev *ocv1.ClusterExtensionRevision) error {
+	if bc.PreAuthorizer == nil {
+		return nil
+	}
+
+	manifestReader, err := revisionManifestReader(rev)
+	if err != nil {
+		return err
+	}
+	return formatPreAuthorizerOutput(bc.PreAuthorizer.PreAuthorize(ctx, ext, manifestReader))
+}
+
 // garbageCollectOldRevisions deletes archived revisions beyond ClusterExtensionRevisionRetentionLimit.
 // Active revisions are never deleted. revisionList must be sorted oldest to newest.
 func (bc *Boxcutter) garbageCollectOldRevisions(ctx context.Context, revisionList []ocv1.ClusterExtensionRevision) error {
@@ -469,3 +481,29 @@ func splitManifestDocuments(file string) []string {
 	}
 	return docs
 }
+
+// getObjects returns a slice of all objects in the revision
+func getObjects(rev *ocv1.ClusterExtensionRevision) []client.Object {
+	var objs []client.Object
+	for _, phase := range rev.Spec.Phases {
+		for _, phaseObject := range phase.Objects {
+			objs = append(objs, &phaseObject.Object)
+		}
+	}
+	return objs
+}
+
+// revisionMenifestReader returns an io.Reader containing all manifests in the revision
+func revisionManifestReader(rev *ocv1.ClusterExtensionRevision) (io.Reader, error) {
+	var manifestBuilder strings.Builder
+	for _, obj := range getObjects(rev) {
+		objBytes, err := yaml.Marshal(obj)
+		if err != nil {
+			return nil, fmt.Errorf("error generating revision manifest: %w", err)
+		}
+		manifestBuilder.WriteString("---\n")
+		manifestBuilder.WriteString(string(objBytes))
+		manifestBuilder.WriteString("\n")
+	}
+	return strings.NewReader(manifestBuilder.String()), nil
+}

internal/operator-controller/applier/boxcutter_test.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"io"
 	"io/fs"
 	"strings"
 	"testing"
@@ -28,6 +29,7 @@ import (
 
 	ocv1 "github.com/operator-framework/operator-controller/api/v1"
 	"github.com/operator-framework/operator-controller/internal/operator-controller/applier"
+	"github.com/operator-framework/operator-controller/internal/operator-controller/authorization"
 	"github.com/operator-framework/operator-controller/internal/operator-controller/labels"
 )
 
@@ -952,6 +954,44 @@ func TestBoxcutter_Apply(t *testing.T) {
 	}
 }
 
+func Test_PreAuthorizer_Integration(t *testing.T) {
+	testScheme := runtime.NewScheme()
+	require.NoError(t, ocv1.AddToScheme(testScheme))
+
+	// This is the revision that the mock builder will produce by default.
+	// We calculate its hash to use in the tests.
+	ext := &ocv1.ClusterExtension{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "test-ext",
+			UID:  "test-uid",
+		},
+	}
+	fakeClient := fake.NewClientBuilder().WithScheme(testScheme).Build()
+
+	boxcutter := &applier.Boxcutter{
+		Client:     fakeClient,
+		Scheme:     testScheme,
+		FieldOwner: "test-owner",
+		RevisionGenerator: &mockBundleRevisionBuilder{
+			makeRevisionFunc: func(ctx context.Context, bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotation map[string]string) (*ocv1.ClusterExtensionRevision, error) {
+				return &ocv1.ClusterExtensionRevision{}, nil
+			},
+		},
+		PreAuthorizer: &fakePreAuthorizer{
+			err: errors.New("test error"),
+		},
+	}
+
+	dummyBundleFs := fstest.MapFS{}
+	revisionAnnotations := map[string]string{}
+	installSucceeded, installStatus, err := boxcutter.Apply(t.Context(), dummyBundleFs, ext, nil, revisionAnnotations)
+
+	require.False(t, installSucceeded)
+	require.Empty(t, installStatus)
+	require.Error(t, err)
+	require.Equal(t, "pre-authorization failed: authorization evaluation error: test error", err.Error())
+}
+
 func TestBoxcutterStorageMigrator(t *testing.T) {
 	t.Run("creates revision", func(t *testing.T) {
 		testScheme := runtime.NewScheme()
@@ -1130,3 +1170,13 @@ func (s *statusWriterMock) Patch(ctx context.Context, obj client.Object, patch c
 func (s *statusWriterMock) Create(ctx context.Context, obj client.Object, subResource client.Object, opts ...client.SubResourceCreateOption) error {
 	return nil
 }
+
+var _ authorization.PreAuthorizer = &fakePreAuthorizer{}
+
+type fakePreAuthorizer struct {
+	err error
+}
+
+func (f fakePreAuthorizer) PreAuthorize(_ context.Context, _ *ocv1.ClusterExtension, _ io.Reader) ([]authorization.ScopedPolicyRules, error) {
+	return nil, f.err
+}

internal/operator-controller/applier/helm.go

@@ -77,29 +77,7 @@ func (h *Helm) runPreAuthorizationChecks(ctx context.Context, ext *ocv1.ClusterE
 		return fmt.Errorf("error rendering content for pre-authorization checks: %w", err)
 	}
 
-	missingRules, authErr := h.PreAuthorizer.PreAuthorize(ctx, ext, strings.NewReader(tmplRel.Manifest))
-
-	var preAuthErrors []error
-
-	if len(missingRules) > 0 {
-		var missingRuleDescriptions []string
-		for _, policyRules := range missingRules {
-			for _, rule := range policyRules.MissingRules {
-				missingRuleDescriptions = append(missingRuleDescriptions, ruleDescription(policyRules.Namespace, rule))
-			}
-		}
-		slices.Sort(missingRuleDescriptions)
-		// This phrase is explicitly checked by external testing
-		preAuthErrors = append(preAuthErrors, fmt.Errorf("service account requires the following permissions to manage cluster extension:\n  %s", strings.Join(missingRuleDescriptions, "\n  ")))
-	}
-	if authErr != nil {
-		preAuthErrors = append(preAuthErrors, fmt.Errorf("authorization evaluation error: %w", authErr))
-	}
-	if len(preAuthErrors) > 0 {
-		// This phrase is explicitly checked by external testing
-		return fmt.Errorf("pre-authorization failed: %v", errors.Join(preAuthErrors...))
-	}
-	return nil
+	return formatPreAuthorizerOutput(h.PreAuthorizer.PreAuthorize(ctx, ext, strings.NewReader(tmplRel.Manifest)))
 }
 
 func (h *Helm) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExtension, objectLabels map[string]string, storageLabels map[string]string) (bool, string, error) {
@@ -328,3 +306,28 @@ func ruleDescription(ns string, rule rbacv1.PolicyRule) string {
 	}
 	return sb.String()
 }
+
+// formatPreAuthorizerOutput formats the output of PreAuthorizer.PreAuthorize calls into a consistent and deterministic error.
+// If the call returns no missing rules, and no error, nil is returned.
+func formatPreAuthorizerOutput(missingRules []authorization.ScopedPolicyRules, authErr error) error {
+	var preAuthErrors []error
+	if len(missingRules) > 0 {
+		var missingRuleDescriptions []string
+		for _, policyRules := range missingRules {
+			for _, rule := range policyRules.MissingRules {
+				missingRuleDescriptions = append(missingRuleDescriptions, ruleDescription(policyRules.Namespace, rule))
+			}
+		}
+		slices.Sort(missingRuleDescriptions)
+		// This phrase is explicitly checked by external testing
+		preAuthErrors = append(preAuthErrors, fmt.Errorf("service account requires the following permissions to manage cluster extension:\n  %s", strings.Join(missingRuleDescriptions, "\n  ")))
+	}
+	if authErr != nil {
+		preAuthErrors = append(preAuthErrors, fmt.Errorf("authorization evaluation error: %w", authErr))
+	}
+	if len(preAuthErrors) > 0 {
+		// This phrase is explicitly checked by external testing
+		return fmt.Errorf("pre-authorization failed: %v", errors.Join(preAuthErrors...))
+	}
+	return nil
+}

internal/operator-controller/authorization/rbac.go

@@ -44,6 +44,17 @@ type ScopedPolicyRules struct {
 	MissingRules []rbacv1.PolicyRule
 }
 
+// Option configures the behavior of the RBACPreAuthorizer
+type Option func(preAuthorizer *rbacPreAuthorizer)
+
+// WithClusterExtensionRevisionPerms configures the RBACPreAuthorizer to also require
+// permissions required against ClusterExtensionRevisions
+func WithClusterExtensionRevisionPerms() Option {
+	return func(preAuthorizer *rbacPreAuthorizer) {
+		preAuthorizer.includeClusterExtensionRevisionPerms = true
+	}
+}
+
 var objectVerbs = []string{"get", "patch", "update", "delete"}
 
 // Here we are splitting collection verbs based on required scope
@@ -55,17 +66,25 @@ var namespacedCollectionVerbs = []string{"create"}
 var clusterCollectionVerbs = []string{"list", "watch"}
 
 type rbacPreAuthorizer struct {
-	authorizer   authorizer.Authorizer
-	ruleResolver validation.AuthorizationRuleResolver
-	restMapper   meta.RESTMapper
+	authorizer                           authorizer.Authorizer
+	ruleResolver                         validation.AuthorizationRuleResolver
+	restMapper                           meta.RESTMapper
+	includeClusterExtensionRevisionPerms bool
+}
+
+func (a *rbacPreAuthorizer) applyOpts(opts []Option) *rbacPreAuthorizer {
+	for _, applyOption := range opts {
+		applyOption(a)
+	}
+	return a
 }
 
-func NewRBACPreAuthorizer(cl client.Client) PreAuthorizer {
-	return &rbacPreAuthorizer{
+func NewRBACPreAuthorizer(cl client.Client, opts ...Option) PreAuthorizer {
+	return (&rbacPreAuthorizer{
 		authorizer:   newRBACAuthorizer(cl),
 		ruleResolver: newRBACRulesResolver(cl),
 		restMapper:   cl.RESTMapper(),
-	}
+	}).applyOpts(opts)
 }
 
 // PreAuthorize validates whether the current user/request satisfies the necessary permissions
@@ -85,7 +104,7 @@ func (a *rbacPreAuthorizer) PreAuthorize(ctx context.Context, ext *ocv1.ClusterE
 		return nil, err
 	}
 	manifestManager := &user.DefaultInfo{Name: fmt.Sprintf("system:serviceaccount:%s:%s", ext.Spec.Namespace, ext.Spec.ServiceAccount.Name)}
-	attributesRecords := dm.asAuthorizationAttributesRecordsForUser(manifestManager, ext)
+	attributesRecords := dm.asAuthorizationAttributesRecordsForUser(manifestManager, ext, a.includeClusterExtensionRevisionPerms)
 
 	var preAuthEvaluationErrors []error
 	missingRules, err := a.authorizeAttributesRecords(ctx, attributesRecords)
@@ -316,7 +335,7 @@ func (dm *decodedManifest) rbacObjects() []client.Object {
 	return objects
 }
 
-func (dm *decodedManifest) asAuthorizationAttributesRecordsForUser(manifestManager user.Info, ext *ocv1.ClusterExtension) []authorizer.AttributesRecord {
+func (dm *decodedManifest) asAuthorizationAttributesRecordsForUser(manifestManager user.Info, ext *ocv1.ClusterExtension, includeClusterExtensionRevisionPerms bool) []authorizer.AttributesRecord {
 	var attributeRecords []authorizer.AttributesRecord
 
 	for gvr, keys := range dm.gvrs {
@@ -374,6 +393,16 @@ func (dm *decodedManifest) asAuthorizationAttributesRecordsForUser(manifestManag
 				ResourceRequest: true,
 				Verb:            verb,
 			})
+			if includeClusterExtensionRevisionPerms {
+				attributeRecords = append(attributeRecords, authorizer.AttributesRecord{
+					User:            manifestManager,
+					APIGroup:        ext.GroupVersionKind().Group,
+					APIVersion:      ext.GroupVersionKind().Version,
+					Resource:        "clusterextensionrevisions/finalizers",
+					ResourceRequest: true,
+					Verb:            verb,
+				})
+			}
 		}
 	}
 	return attributeRecords