From 12ab842cd1606ae92d96c1b39609030336635932 Mon Sep 17 00:00:00 2001 From: Per Goncalves da Silva Date: Mon, 12 Jan 2026 14:29:20 +0100 Subject: [PATCH] Add preflight checks to Boxcutter applier Signed-off-by: Per Goncalves da Silva --- cmd/operator-controller/main.go | 8 +- .../operator-controller/applier/boxcutter.go | 103 +++++++-- .../applier/boxcutter_test.go | 197 ++++++++++++++++++ internal/operator-controller/applier/helm.go | 70 +++++-- .../operator-controller/applier/helm_test.go | 101 +++++++-- .../operator-controller/authorization/rbac.go | 31 +-- .../authorization/rbac_test.go | 38 +--- 7 files changed, 436 insertions(+), 112 deletions(-) diff --git a/cmd/operator-controller/main.go b/cmd/operator-controller/main.go index 2d78edda4f..f02818f1d5 100644 --- a/cmd/operator-controller/main.go +++ b/cmd/operator-controller/main.go @@ -598,7 +598,12 @@ func (c *boxcutterReconcilerConfigurator) Configure(ceReconciler *controllers.Cl return err } - // TODO: add support for preflight checks + // 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()) + } + // TODO: better scheme handling - which types do we want to support? _ = apiextensionsv1.AddToScheme(c.mgr.GetScheme()) rg := &applier.SimpleRevisionGenerator{ @@ -610,6 +615,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()} diff --git a/internal/operator-controller/applier/boxcutter.go b/internal/operator-controller/applier/boxcutter.go index 63578e9cb8..7c1a78e57a 100644 --- a/internal/operator-controller/applier/boxcutter.go +++ b/internal/operator-controller/applier/boxcutter.go @@ -5,6 +5,7 @@ import ( "context" "errors" "fmt" + "io" "io/fs" "maps" "slices" @@ -16,6 +17,8 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apiserver/pkg/authentication/user" + "k8s.io/apiserver/pkg/authorization/authorizer" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/apiutil" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" @@ -25,6 +28,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" ) @@ -279,28 +283,25 @@ type Boxcutter struct { Scheme *runtime.Scheme RevisionGenerator ClusterExtensionRevisionGenerator Preflights []Preflight + PreAuthorizer authorization.PreAuthorizer FieldOwner string } -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) +func (bc *Boxcutter) createOrUpdateRevisionWithPreAuthorization(ctx context.Context, ext *ocv1.ClusterExtension, rev *ocv1.ClusterExtensionRevision) error { + if rev.GetObjectKind().GroupVersionKind().Empty() { + gvk, err := apiutil.GVKForObject(rev, bc.Scheme) if err != nil { return err } - obj.GetObjectKind().SetGroupVersionKind(gvk) + rev.GetObjectKind().SetGroupVersionKind(gvk) } - return bc.Client.Patch(ctx, obj, client.Apply, client.FieldOwner(bc.FieldOwner), client.ForceOwnership) + + // Run auth preflight checks + if err := bc.runPreAuthorizationChecks(ctx, ext, rev); err != nil { + return err + } + + return bc.Client.Patch(ctx, rev, client.Apply, client.FieldOwner(bc.FieldOwner), client.ForceOwnership) } func (bc *Boxcutter) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) error { @@ -329,7 +330,7 @@ func (bc *Boxcutter) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.Clust desiredRevision.Spec.Revision = currentRevision.Spec.Revision desiredRevision.Name = currentRevision.Name - err := bc.createOrUpdate(ctx, desiredRevision) + err := bc.createOrUpdateRevisionWithPreAuthorization(ctx, ext, desiredRevision) switch { case apierrors.IsInvalid(err): // We could not update the current revision due to trying to update an immutable field. @@ -344,7 +345,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 @@ -379,7 +380,7 @@ func (bc *Boxcutter) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.Clust return fmt.Errorf("garbage collecting old revisions: %w", err) } - if err := bc.createOrUpdate(ctx, desiredRevision); err != nil { + if err := bc.createOrUpdateRevisionWithPreAuthorization(ctx, ext, desiredRevision); err != nil { return fmt.Errorf("creating new Revision: %w", err) } } @@ -387,6 +388,32 @@ func (bc *Boxcutter) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.Clust return 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 the revision's resources +func (bc *Boxcutter) runPreAuthorizationChecks(ctx context.Context, ext *ocv1.ClusterExtension, rev *ocv1.ClusterExtensionRevision) error { + if bc.PreAuthorizer == nil { + return nil + } + + // collect the revision manifests + manifestReader, err := revisionManifestReader(rev) + if err != nil { + return err + } + + // extract user to check permissions against + manifestManager := getUserInfo(ext) + + // collect additional permissions required by OLM to manage the bundle manifests + managementPerms := slices.Concat( + clusterExtensionManagementPermissions(manifestManager, ext), + clusterExtensionRevisionManagementPermissions(manifestManager, rev), + ) + + // run preauthorization check + return formatPreAuthorizerOutput(bc.PreAuthorizer.PreAuthorize(ctx, manifestManager, manifestReader, managementPerms...)) +} + // 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 { @@ -445,3 +472,43 @@ 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 +} + +// revisionManifestReader 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 +} + +func clusterExtensionRevisionManagementPermissions(manifestManager user.Info, rev *ocv1.ClusterExtensionRevision) []authorizer.AttributesRecord { + return []authorizer.AttributesRecord{ + { + User: manifestManager, + Name: rev.Name, + APIGroup: ocv1.GroupVersion.Group, + APIVersion: ocv1.GroupVersion.Version, + Resource: "clusterextensionrevisions/finalizers", + ResourceRequest: true, + Verb: "update", + }, + } +} diff --git a/internal/operator-controller/applier/boxcutter_test.go b/internal/operator-controller/applier/boxcutter_test.go index 081747285c..f3c0532a2d 100644 --- a/internal/operator-controller/applier/boxcutter_test.go +++ b/internal/operator-controller/applier/boxcutter_test.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "io" "io/fs" "strings" "testing" @@ -16,11 +17,14 @@ import ( "helm.sh/helm/v3/pkg/storage/driver" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" + rbacv1 "k8s.io/api/rbac/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/validation/field" + "k8s.io/apiserver/pkg/authentication/user" + "k8s.io/apiserver/pkg/authorization/authorizer" k8scheme "k8s.io/client-go/kubernetes/scheme" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" @@ -28,6 +32,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" ) @@ -948,6 +953,198 @@ 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", + }, + Spec: ocv1.ClusterExtensionSpec{ + Namespace: "test-namespace", + ServiceAccount: ocv1.ServiceAccountReference{ + Name: "test-sa", + }, + }, + } + fakeClient := fake.NewClientBuilder().WithScheme(testScheme).Build() + dummyGenerator := &mockBundleRevisionBuilder{ + makeRevisionFunc: func(ctx context.Context, bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotation map[string]string) (*ocv1.ClusterExtensionRevision, error) { + return &ocv1.ClusterExtensionRevision{ + Spec: ocv1.ClusterExtensionRevisionSpec{ + Phases: []ocv1.ClusterExtensionRevisionPhase{ + { + Name: "some-phase", + Objects: []ocv1.ClusterExtensionRevisionObject{ + { + Object: unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "v1", + "kind": "ConfigMap", + "data": map[string]string{ + "test-data": "test-data", + }, + }, + }, + }, + }, + }, + }, + }, + }, nil + }, + } + dummyBundleFs := fstest.MapFS{} + revisionAnnotations := map[string]string{} + + for _, tc := range []struct { + name string + preAuthorizer func(t *testing.T) authorization.PreAuthorizer + validate func(t *testing.T, err error) + }{ + { + name: "preauthorizer called with correct parameters", + preAuthorizer: func(t *testing.T) authorization.PreAuthorizer { + return &mockPreAuthorizer{ + fn: func(ctx context.Context, manifestManager user.Info, reader io.Reader, additionalRequiredPerms ...authorizer.AttributesRecord) ([]authorization.ScopedPolicyRules, error) { + t.Log("has correct manifestManager") + require.Equal(t, "system:serviceaccount:test-namespace:test-sa", manifestManager.GetName()) + require.Empty(t, manifestManager.GetUID()) + require.Nil(t, manifestManager.GetExtra()) + require.Empty(t, manifestManager.GetGroups()) + + t.Log("has correct additional permissions") + require.Len(t, additionalRequiredPerms, 2) + require.Contains(t, additionalRequiredPerms, authorizer.AttributesRecord{ + User: manifestManager, + Name: "test-ext", + APIGroup: "olm.operatorframework.io", + APIVersion: "v1", + Resource: "clusterextensions/finalizers", + ResourceRequest: true, + Verb: "update", + }) + require.Contains(t, additionalRequiredPerms, authorizer.AttributesRecord{ + User: manifestManager, + Name: "test-ext-1", + APIGroup: "olm.operatorframework.io", + APIVersion: "v1", + Resource: "clusterextensionrevisions/finalizers", + ResourceRequest: true, + Verb: "update", + }) + + t.Log("has correct manifest reader") + manifests, err := io.ReadAll(reader) + require.NoError(t, err) + require.Equal(t, "---\napiVersion: v1\ndata:\n test-data: test-data\nkind: ConfigMap\n\n", string(manifests)) + return nil, nil + }, + } + }, + }, { + name: "preauthorizer errors are returned", + preAuthorizer: func(t *testing.T) authorization.PreAuthorizer { + return &mockPreAuthorizer{ + fn: func(ctx context.Context, manifestManager user.Info, reader io.Reader, additionalRequiredPerms ...authorizer.AttributesRecord) ([]authorization.ScopedPolicyRules, error) { + return nil, errors.New("test error") + }, + } + }, + validate: func(t *testing.T, err error) { + require.Error(t, err) + require.Contains(t, err.Error(), "pre-authorization failed") + require.Contains(t, err.Error(), "authorization evaluation error: test error") + }, + }, { + name: "preauthorizer missing permissions are returned as an error", + preAuthorizer: func(t *testing.T) authorization.PreAuthorizer { + return &mockPreAuthorizer{ + fn: func(ctx context.Context, manifestManager user.Info, reader io.Reader, additionalRequiredPerms ...authorizer.AttributesRecord) ([]authorization.ScopedPolicyRules, error) { + return []authorization.ScopedPolicyRules{ + { + Namespace: "", + MissingRules: []rbacv1.PolicyRule{ + { + APIGroups: []string{""}, + Resources: []string{"pods"}, + Verbs: []string{"get", "list", "watch"}, + }, + }, + }, + }, nil + }, + } + }, + validate: func(t *testing.T, err error) { + require.Error(t, err) + require.Contains(t, err.Error(), "pre-authorization failed") + require.Contains(t, err.Error(), "service account requires the following permissions") + require.Contains(t, err.Error(), "Resources:[pods]") + require.Contains(t, err.Error(), "Verbs:[get,list,watch]") + }, + }, { + name: "preauthorizer missing permissions and errors are combined and returned as an error", + preAuthorizer: func(t *testing.T) authorization.PreAuthorizer { + return &mockPreAuthorizer{ + fn: func(ctx context.Context, manifestManager user.Info, reader io.Reader, additionalRequiredPerms ...authorizer.AttributesRecord) ([]authorization.ScopedPolicyRules, error) { + return []authorization.ScopedPolicyRules{ + { + Namespace: "", + MissingRules: []rbacv1.PolicyRule{ + { + APIGroups: []string{""}, + Resources: []string{"pods"}, + Verbs: []string{"get", "list", "watch"}, + }, + }, + }, + }, errors.New("test error") + }, + } + }, + validate: func(t *testing.T, err error) { + require.Error(t, err) + require.Contains(t, err.Error(), "pre-authorization failed") + require.Contains(t, err.Error(), "service account requires the following permissions") + require.Contains(t, err.Error(), "Resources:[pods]") + require.Contains(t, err.Error(), "Verbs:[get,list,watch]") + require.Contains(t, err.Error(), "authorization evaluation error: test error") + }, + }, { + name: "successful call to preauthorizer does not block applier", + preAuthorizer: func(t *testing.T) authorization.PreAuthorizer { + return &mockPreAuthorizer{ + fn: func(ctx context.Context, manifestManager user.Info, reader io.Reader, additionalRequiredPerms ...authorizer.AttributesRecord) ([]authorization.ScopedPolicyRules, error) { + return nil, nil + }, + } + }, + validate: func(t *testing.T, err error) { + require.NoError(t, err) + }, + }, + } { + t.Run(tc.name, func(t *testing.T) { + boxcutter := &applier.Boxcutter{ + Client: fakeClient, + Scheme: testScheme, + FieldOwner: "test-owner", + RevisionGenerator: dummyGenerator, + PreAuthorizer: tc.preAuthorizer(t), + } + err := boxcutter.Apply(t.Context(), dummyBundleFs, ext, nil, revisionAnnotations) + if tc.validate != nil { + tc.validate(t, err) + } + }) + } +} + func TestBoxcutterStorageMigrator(t *testing.T) { t.Run("creates revision", func(t *testing.T) { testScheme := runtime.NewScheme() diff --git a/internal/operator-controller/applier/helm.go b/internal/operator-controller/applier/helm.go index 22ed096fe1..eda9031ec6 100644 --- a/internal/operator-controller/applier/helm.go +++ b/internal/operator-controller/applier/helm.go @@ -19,6 +19,8 @@ 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" + "k8s.io/apiserver/pkg/authorization/authorizer" "k8s.io/klog/v2" "sigs.k8s.io/controller-runtime/pkg/client" @@ -77,29 +79,8 @@ 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 + manifestManager := getUserInfo(ext) + return formatPreAuthorizerOutput(h.PreAuthorizer.PreAuthorize(ctx, manifestManager, strings.NewReader(tmplRel.Manifest), clusterExtensionManagementPermissions(manifestManager, ext)...)) } 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 +309,46 @@ 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 +} + +func getUserInfo(ext *ocv1.ClusterExtension) user.Info { + return &user.DefaultInfo{Name: fmt.Sprintf("system:serviceaccount:%s:%s", ext.Spec.Namespace, ext.Spec.ServiceAccount.Name)} +} + +func clusterExtensionManagementPermissions(manifestManager user.Info, ext *ocv1.ClusterExtension) []authorizer.AttributesRecord { + return []authorizer.AttributesRecord{ + { + User: manifestManager, + Name: ext.Name, + APIGroup: ocv1.GroupVersion.Group, + APIVersion: ocv1.GroupVersion.Version, + Resource: "clusterextensions/finalizers", + ResourceRequest: true, + Verb: "update", + }, + } +} diff --git a/internal/operator-controller/applier/helm_test.go b/internal/operator-controller/applier/helm_test.go index 8d07de9123..10563c0ea7 100644 --- a/internal/operator-controller/applier/helm_test.go +++ b/internal/operator-controller/applier/helm_test.go @@ -15,6 +15,9 @@ import ( "helm.sh/helm/v3/pkg/release" "helm.sh/helm/v3/pkg/storage/driver" rbacv1 "k8s.io/api/rbac/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apiserver/pkg/authentication/user" + "k8s.io/apiserver/pkg/authorization/authorizer" "sigs.k8s.io/controller-runtime/pkg/client" helmclient "github.com/operator-framework/helm-operator-plugins/pkg/client" @@ -70,16 +73,11 @@ type mockPreflight struct { } type mockPreAuthorizer struct { - missingRules []authorization.ScopedPolicyRules - returnError error + fn func(context.Context, user.Info, io.Reader, ...authorizer.AttributesRecord) ([]authorization.ScopedPolicyRules, error) } -func (p *mockPreAuthorizer) PreAuthorize( - ctx context.Context, - ext *ocv1.ClusterExtension, - manifestReader io.Reader, -) ([]authorization.ScopedPolicyRules, error) { - return p.missingRules, p.returnError +func (p *mockPreAuthorizer) PreAuthorize(ctx context.Context, manifestManager user.Info, manifestReader io.Reader, additionalRequiredPerms ...authorizer.AttributesRecord) ([]authorization.ScopedPolicyRules, error) { + return p.fn(ctx, manifestManager, manifestReader, additionalRequiredPerms...) } func (mp *mockPreflight) Install(context.Context, []client.Object) error { @@ -193,7 +191,17 @@ metadata: spec: clusterIP: 0.0.0.0` - testCE = &ocv1.ClusterExtension{} + testCE = &ocv1.ClusterExtension{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-ext", + }, + Spec: ocv1.ClusterExtensionSpec{ + Namespace: "test-namespace", + ServiceAccount: ocv1.ServiceAccountReference{ + Name: "test-sa", + }, + }, + } testObjectLabels = map[string]string{"object": "label"} testStorageLabels = map[string]string{"storage": "label"} errPreAuth = errors.New("problem running preauthorization") @@ -345,6 +353,47 @@ func TestApply_Installation(t *testing.T) { } func TestApply_InstallationWithPreflightPermissionsEnabled(t *testing.T) { + t.Run("preauthorizer called with correct parameters", func(t *testing.T) { + mockAcg := &mockActionGetter{ + getClientErr: driver.ErrReleaseNotFound, + installErr: errors.New("failed installing chart"), + desiredRel: &release.Release{ + Info: &release.Info{Status: release.StatusDeployed}, + Manifest: validManifest, + }, + } + mockPf := &mockPreflight{installErr: errors.New("failed during install pre-flight check")} + helmApplier := applier.Helm{ + ActionClientGetter: mockAcg, + Preflights: []applier.Preflight{mockPf}, + PreAuthorizer: &mockPreAuthorizer{ + fn: func(ctx context.Context, manifestManager user.Info, reader io.Reader, additionalRequiredPerms ...authorizer.AttributesRecord) ([]authorization.ScopedPolicyRules, error) { + require.Equal(t, "system:serviceaccount:test-namespace:test-sa", manifestManager.GetName()) + require.Empty(t, manifestManager.GetUID()) + require.Nil(t, manifestManager.GetExtra()) + require.Empty(t, manifestManager.GetGroups()) + + require.Len(t, additionalRequiredPerms, 1) + require.Contains(t, additionalRequiredPerms, authorizer.AttributesRecord{ + User: manifestManager, + Name: "test-ext", + APIGroup: "olm.operatorframework.io", + APIVersion: "v1", + Resource: "clusterextensions/finalizers", + ResourceRequest: true, + Verb: "update", + }) + return nil, nil + }, + }, + HelmChartProvider: DummyHelmChartProvider, + HelmReleaseToObjectsConverter: mockHelmReleaseToObjectsConverter{}, + } + + _, _, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels) + require.Error(t, err) + }) + t.Run("fails during dry-run installation", func(t *testing.T) { mockAcg := &mockActionGetter{ getClientErr: driver.ErrReleaseNotFound, @@ -373,9 +422,13 @@ func TestApply_InstallationWithPreflightPermissionsEnabled(t *testing.T) { } mockPf := &mockPreflight{installErr: errors.New("failed during install pre-flight check")} helmApplier := applier.Helm{ - ActionClientGetter: mockAcg, - Preflights: []applier.Preflight{mockPf}, - PreAuthorizer: &mockPreAuthorizer{nil, nil}, + ActionClientGetter: mockAcg, + Preflights: []applier.Preflight{mockPf}, + PreAuthorizer: &mockPreAuthorizer{ + fn: func(ctx context.Context, manifestManager user.Info, reader io.Reader, additionalRequiredPerms ...authorizer.AttributesRecord) ([]authorization.ScopedPolicyRules, error) { + return nil, nil + }, + }, HelmChartProvider: DummyHelmChartProvider, HelmReleaseToObjectsConverter: mockHelmReleaseToObjectsConverter{}, } @@ -397,8 +450,12 @@ func TestApply_InstallationWithPreflightPermissionsEnabled(t *testing.T) { } helmApplier := applier.Helm{ ActionClientGetter: mockAcg, - PreAuthorizer: &mockPreAuthorizer{nil, errPreAuth}, - HelmChartProvider: DummyHelmChartProvider, + PreAuthorizer: &mockPreAuthorizer{ + fn: func(ctx context.Context, manifestManager user.Info, reader io.Reader, additionalRequiredPerms ...authorizer.AttributesRecord) ([]authorization.ScopedPolicyRules, error) { + return nil, errPreAuth + }, + }, + HelmChartProvider: DummyHelmChartProvider, } // Use a ClusterExtension with valid Spec fields. validCE := &ocv1.ClusterExtension{ @@ -426,8 +483,12 @@ func TestApply_InstallationWithPreflightPermissionsEnabled(t *testing.T) { } helmApplier := applier.Helm{ ActionClientGetter: mockAcg, - PreAuthorizer: &mockPreAuthorizer{missingRBAC, nil}, - HelmChartProvider: DummyHelmChartProvider, + PreAuthorizer: &mockPreAuthorizer{ + fn: func(ctx context.Context, manifestManager user.Info, reader io.Reader, additionalRequiredPerms ...authorizer.AttributesRecord) ([]authorization.ScopedPolicyRules, error) { + return missingRBAC, nil + }, + }, + HelmChartProvider: DummyHelmChartProvider, } // Use a ClusterExtension with valid Spec fields. validCE := &ocv1.ClusterExtension{ @@ -454,8 +515,12 @@ func TestApply_InstallationWithPreflightPermissionsEnabled(t *testing.T) { }, } helmApplier := applier.Helm{ - ActionClientGetter: mockAcg, - PreAuthorizer: &mockPreAuthorizer{nil, nil}, + ActionClientGetter: mockAcg, + PreAuthorizer: &mockPreAuthorizer{ + fn: func(ctx context.Context, manifestManager user.Info, reader io.Reader, additionalRequiredPerms ...authorizer.AttributesRecord) ([]authorization.ScopedPolicyRules, error) { + return nil, nil + }, + }, HelmChartProvider: DummyHelmChartProvider, HelmReleaseToObjectsConverter: mockHelmReleaseToObjectsConverter{}, Manager: &mockManagedContentCacheManager{ diff --git a/internal/operator-controller/authorization/rbac.go b/internal/operator-controller/authorization/rbac.go index 357268615c..be0a428ef7 100644 --- a/internal/operator-controller/authorization/rbac.go +++ b/internal/operator-controller/authorization/rbac.go @@ -31,12 +31,10 @@ import ( "k8s.io/kubernetes/pkg/registry/rbac/validation" rbac "k8s.io/kubernetes/plugin/pkg/auth/authorizer/rbac" "sigs.k8s.io/controller-runtime/pkg/client" - - ocv1 "github.com/operator-framework/operator-controller/api/v1" ) type PreAuthorizer interface { - PreAuthorize(ctx context.Context, ext *ocv1.ClusterExtension, manifestReader io.Reader) ([]ScopedPolicyRules, error) + PreAuthorize(ctx context.Context, manifestManager user.Info, manifestReader io.Reader, additionalRequiredPerms ...authorizer.AttributesRecord) ([]ScopedPolicyRules, error) } type ScopedPolicyRules struct { @@ -68,9 +66,10 @@ func NewRBACPreAuthorizer(cl client.Client) PreAuthorizer { } } -// PreAuthorize validates whether the current user/request satisfies the necessary permissions -// as defined by the RBAC policy. It examines the user’s roles, resource identifiers, and -// the intended action to determine if the operation is allowed. +// PreAuthorize validates whether the manifestManager satisfies the necessary permissions +// as defined by the RBAC policy. It examines the manifestManager’s roles, resource identifiers, and +// the intended action to determine if the operation is allowed. Optional additional required permissions are also evaluated +// against the manifestManager. // // Return Value: // - nil: indicates that the authorization check passed and the operation is permitted. @@ -79,13 +78,13 @@ 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, ext *ocv1.ClusterExtension, manifestReader io.Reader) ([]ScopedPolicyRules, error) { +func (a *rbacPreAuthorizer) PreAuthorize(ctx context.Context, manifestManager user.Info, manifestReader io.Reader, additionalRequiredPerms ...authorizer.AttributesRecord) ([]ScopedPolicyRules, error) { dm, err := a.decodeManifest(manifestReader) if err != nil { 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 := append(dm.asAuthorizationAttributesRecordsForUser(manifestManager), additionalRequiredPerms...) var preAuthEvaluationErrors []error missingRules, err := a.authorizeAttributesRecords(ctx, attributesRecords) @@ -316,7 +315,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) []authorizer.AttributesRecord { var attributeRecords []authorizer.AttributesRecord for gvr, keys := range dm.gvrs { @@ -363,18 +362,6 @@ func (dm *decodedManifest) asAuthorizationAttributesRecordsForUser(manifestManag Verb: v, }) } - - for _, verb := range []string{"update"} { - 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 } diff --git a/internal/operator-controller/authorization/rbac_test.go b/internal/operator-controller/authorization/rbac_test.go index 8e3d47687d..6f9e90c9dd 100644 --- a/internal/operator-controller/authorization/rbac_test.go +++ b/internal/operator-controller/authorization/rbac_test.go @@ -18,8 +18,6 @@ import ( "k8s.io/kubernetes/pkg/registry/rbac/validation" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" - - ocv1 "github.com/operator-framework/operator-controller/api/v1" ) var ( @@ -129,17 +127,9 @@ subjects: namespace: a-test-namespace ` - 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, - }, - }, - } + saName = "test-serviceaccount" + ns = "test-namespace" + testUser = &user.DefaultInfo{Name: fmt.Sprintf("system:serviceaccount:%s:%s", ns, saName)} objects = []client.Object{ &corev1.Namespace{ @@ -204,7 +194,7 @@ subjects: Rules: []rbacv1.PolicyRule{ { APIGroups: []string{"*"}, - Resources: []string{"serviceaccounts", "services", "clusterextensions/finalizers"}, + Resources: []string{"serviceaccounts", "services"}, Verbs: []string{"*"}, }, { @@ -236,12 +226,6 @@ subjects: APIGroups: []string{"rbac.authorization.k8s.io"}, Resources: []string{"roles"}, ResourceNames: []string(nil), - NonResourceURLs: []string(nil)}, - { - Verbs: []string{"update"}, - APIGroups: []string{""}, - Resources: []string{"clusterextensions/finalizers"}, - ResourceNames: []string{"test-cluster-extension"}, NonResourceURLs: []string(nil), }, }, @@ -310,12 +294,6 @@ subjects: APIGroups: []string{"rbac.authorization.k8s.io"}, Resources: []string{"roles"}, ResourceNames: []string(nil), - NonResourceURLs: []string(nil)}, - { - Verbs: []string{"update"}, - APIGroups: []string{""}, - Resources: []string{"clusterextensions/finalizers"}, - ResourceNames: []string{"test-cluster-extension"}, NonResourceURLs: []string(nil), }, }, @@ -418,7 +396,7 @@ 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) - missingRules, err := preAuth.PreAuthorize(context.TODO(), &exampleClusterExtension, strings.NewReader(testManifest)) + missingRules, err := preAuth.PreAuthorize(context.TODO(), testUser, strings.NewReader(testManifest)) require.NoError(t, err) require.Equal(t, []ScopedPolicyRules{}, missingRules) }) @@ -428,7 +406,7 @@ 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) - missingRules, err := preAuth.PreAuthorize(context.TODO(), &exampleClusterExtension, strings.NewReader(testManifest)) + missingRules, err := preAuth.PreAuthorize(context.TODO(), testUser, strings.NewReader(testManifest)) require.NoError(t, err) require.Equal(t, expectedSingleNamespaceMissingRules, missingRules) }) @@ -438,7 +416,7 @@ 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) - missingRules, err := preAuth.PreAuthorize(context.TODO(), &exampleClusterExtension, strings.NewReader(testManifestMultiNamespace)) + missingRules, err := preAuth.PreAuthorize(context.TODO(), testUser, strings.NewReader(testManifestMultiNamespace)) require.NoError(t, err) require.Equal(t, expectedMultiNamespaceMissingRules, missingRules) }) @@ -448,7 +426,7 @@ 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) - missingRules, err := preAuth.PreAuthorize(context.TODO(), &exampleClusterExtension, strings.NewReader(testManifest)) + missingRules, err := preAuth.PreAuthorize(context.TODO(), testUser, strings.NewReader(testManifest)) require.NoError(t, err) require.Equal(t, []ScopedPolicyRules{}, missingRules) })