From 88996a3fb1d2d03017a01f15610c724a8b45c3b4 Mon Sep 17 00:00:00 2001 From: Tayler Geiger Date: Thu, 13 Feb 2025 10:13:39 -0600 Subject: [PATCH 1/5] Add client-only helm dry-run to helm applier Adds a check that makes sure the installer service account has the necessary permissions to manage all the resources in the ClusterExtension payload and returns errors detailing all the missing permissions. This prevents a hung server-connected dry-run getting caught on individual missing get permissions as well returns many other missing required permissions needed for the actual installation or upgrade. Signed-off-by: Tayler Geiger --- cmd/operator-controller/main.go | 3 + internal/operator-controller/applier/helm.go | 64 ++++++++++ .../authorization/authorization.go | 112 ++++++++++++++++++ 3 files changed, 179 insertions(+) create mode 100644 internal/operator-controller/authorization/authorization.go diff --git a/cmd/operator-controller/main.go b/cmd/operator-controller/main.go index 2a46afc6d..eeb694df4 100644 --- a/cmd/operator-controller/main.go +++ b/cmd/operator-controller/main.go @@ -407,9 +407,12 @@ func run() error { crdupgradesafety.NewPreflight(aeClient.CustomResourceDefinitions()), } + acm := applier.NewAuthClientMapper(clientRestConfigMapper, mgr.GetConfig()) + helmApplier := &applier.Helm{ ActionClientGetter: acg, Preflights: preflights, + AuthClientMapper: acm, } cm := contentmanager.NewManager(clientRestConfigMapper, mgr.GetConfig(), mgr.GetRESTMapper()) diff --git a/internal/operator-controller/applier/helm.go b/internal/operator-controller/applier/helm.go index 76df085cb..c91d6297c 100644 --- a/internal/operator-controller/applier/helm.go +++ b/internal/operator-controller/applier/helm.go @@ -18,12 +18,15 @@ import ( corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" apimachyaml "k8s.io/apimachinery/pkg/util/yaml" + "k8s.io/client-go/rest" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/log" helmclient "github.com/operator-framework/helm-operator-plugins/pkg/client" + authorizationv1client "k8s.io/client-go/kubernetes/typed/authorization/v1" 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/features" "github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/convert" "github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/preflights/crdupgradesafety" @@ -53,9 +56,38 @@ type Preflight interface { Upgrade(context.Context, *release.Release) error } +type RestConfigMapper func(context.Context, client.Object, *rest.Config) (*rest.Config, error) + +type AuthClientMapper struct { + rcm RestConfigMapper + baseCfg *rest.Config +} + +func (acm *AuthClientMapper) GetAuthenticationClient(ctx context.Context, ext *ocv1.ClusterExtension) (*authorizationv1client.AuthorizationV1Client, error) { + authcfg, err := acm.rcm(ctx, ext, acm.baseCfg) + if err != nil { + return nil, err + } + + authclient, err := authorizationv1client.NewForConfig(authcfg) + if err != nil { + return nil, err + } + + return authclient, nil +} + type Helm struct { ActionClientGetter helmclient.ActionClientGetter Preflights []Preflight + AuthClientMapper AuthClientMapper +} + +func NewAuthClientMapper(rcm RestConfigMapper, baseCfg *rest.Config) AuthClientMapper { + return AuthClientMapper{ + rcm: rcm, + baseCfg: baseCfg, + } } // shouldSkipPreflight is a helper to determine if the preflight check is CRDUpgradeSafety AND @@ -79,7 +111,21 @@ func shouldSkipPreflight(ctx context.Context, preflight Preflight, ext *ocv1.Clu } func (h *Helm) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExtension, objectLabels map[string]string, storageLabels map[string]string) ([]client.Object, string, error) { + + if features.OperatorControllerFeatureGate.Enabled(features.PreflightPermissions) { + authclient, err := h.AuthClientMapper.GetAuthenticationClient(ctx, ext) + if err != nil { + return nil, "", err + } + + err = h.checkContentPermissions(ctx, contentFS, authclient, ext) + if err != nil { + return nil, "", err + } + } + chrt, err := convert.RegistryV1ToHelmChart(ctx, contentFS, ext.Spec.Namespace, []string{corev1.NamespaceAll}) + if err != nil { return nil, "", err } @@ -152,8 +198,26 @@ func (h *Helm) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExte return relObjects, state, nil } +// Check if RBAC allows the installer service account necessary permissions on the objects in the contentFS +func (h *Helm) checkContentPermissions(ctx context.Context, contentFS fs.FS, authcl *authorizationv1client.AuthorizationV1Client, ext *ocv1.ClusterExtension) error { + reg, err := convert.ParseFS(ctx, contentFS) + if err != nil { + return err + } + + plain, err := convert.Convert(reg, ext.Spec.Namespace, []string{corev1.NamespaceAll}) + if err != nil { + return err + } + + err = authorization.CheckObjectPermissions(ctx, authcl, plain.Objects, ext) + + return err +} + func (h *Helm) getReleaseState(cl helmclient.ActionInterface, ext *ocv1.ClusterExtension, chrt *chart.Chart, values chartutil.Values, post postrender.PostRenderer) (*release.Release, *release.Release, string, error) { currentRelease, err := cl.Get(ext.GetName()) + if errors.Is(err, driver.ErrReleaseNotFound) { desiredRelease, err := cl.Install(ext.GetName(), ext.Spec.Namespace, chrt, values, func(i *action.Install) error { i.DryRun = true diff --git a/internal/operator-controller/authorization/authorization.go b/internal/operator-controller/authorization/authorization.go new file mode 100644 index 000000000..d1d5d8592 --- /dev/null +++ b/internal/operator-controller/authorization/authorization.go @@ -0,0 +1,112 @@ +package authorization + +import ( + "context" + "errors" + "fmt" + "slices" + "strings" + + ocv1 "github.com/operator-framework/operator-controller/api/v1" + authv1 "k8s.io/api/authorization/v1" + rbacv1 "k8s.io/api/rbac/v1" + v1 "k8s.io/apimachinery/pkg/apis/meta/v1" + authorizationv1client "k8s.io/client-go/kubernetes/typed/authorization/v1" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +const ( + SelfSubjectRulesReview string = "SelfSubjectRulesReview" + SelfSubjectAccessReview string = "SelfSubjectAccessReview" +) + +func CheckObjectPermissions(ctx context.Context, authcl *authorizationv1client.AuthorizationV1Client, objects []client.Object, ext *ocv1.ClusterExtension) error { + ssrr := &authv1.SelfSubjectRulesReview{ + Spec: authv1.SelfSubjectRulesReviewSpec{ + Namespace: ext.Spec.Namespace, + }, + } + + ssrr, err := authcl.SelfSubjectRulesReviews().Create(ctx, ssrr, v1.CreateOptions{}) + if err != nil { + return err + } + + rules := []rbacv1.PolicyRule{} + for _, rule := range ssrr.Status.ResourceRules { + rules = append(rules, rbacv1.PolicyRule{ + Verbs: rule.Verbs, + APIGroups: rule.APIGroups, + Resources: rule.Resources, + ResourceNames: rule.ResourceNames, + }) + } + + for _, rule := range ssrr.Status.NonResourceRules { + rules = append(rules, rbacv1.PolicyRule{ + Verbs: rule.Verbs, + NonResourceURLs: rule.NonResourceURLs, + }) + } + + resAttrs := []authv1.ResourceAttributes{} + namespacedErrs := []error{} + clusterScopedErrs := []error{} + requiredVerbs := []string{"get", "create", "update", "list", "watch", "delete", "patch"} + + for _, o := range objects { + for _, verb := range requiredVerbs { + resAttrs = append(resAttrs, authv1.ResourceAttributes{ + Namespace: o.GetNamespace(), + Verb: verb, + Resource: sanitizeResourceName(o.GetObjectKind().GroupVersionKind().Kind), + Group: o.GetObjectKind().GroupVersionKind().Group, + Name: o.GetName(), + }) + } + } + + for _, resAttr := range resAttrs { + if !canI(resAttr, rules) { + if resAttr.Namespace != "" { + namespacedErrs = append(namespacedErrs, fmt.Errorf("cannot %s %s %s in namespace %s", + resAttr.Verb, + strings.TrimSuffix(resAttr.Resource, "s"), + resAttr.Name, + resAttr.Namespace)) + continue + } + clusterScopedErrs = append(clusterScopedErrs, fmt.Errorf("cannot %s %s %s", + resAttr.Verb, + strings.TrimSuffix(resAttr.Resource, "s"), + resAttr.Name)) + } + } + errs := append(namespacedErrs, clusterScopedErrs...) + if len(errs) > 0 { + errs = append([]error{fmt.Errorf("installer service account %s is missing required permissions", ext.Spec.ServiceAccount.Name)}, errs...) + } + + return errors.Join(errs...) + +} + +// Checks if the rules allow the verb on the GroupVersionKind in resAttr +func canI(resAttr authv1.ResourceAttributes, rules []rbacv1.PolicyRule) bool { + var canI bool + for _, rule := range rules { + if (slices.Contains(rule.APIGroups, resAttr.Group) || slices.Contains(rule.APIGroups, "*")) && + (slices.Contains(rule.Resources, resAttr.Resource) || slices.Contains(rule.Resources, "*")) && + (slices.Contains(rule.Verbs, resAttr.Verb) || slices.Contains(rule.Verbs, "*")) && + (slices.Contains(rule.ResourceNames, resAttr.Name) || len(rule.ResourceNames) == 0) { + canI = true + break + } + } + return canI +} + +// SelfSubjectRulesReview formats the resource names as lowercase and plural +func sanitizeResourceName(resourceName string) string { + return strings.ToLower(resourceName) + "s" +} From 4290449272866942bea08d16cae8ac96fac8879b Mon Sep 17 00:00:00 2001 From: Brett Tofel Date: Mon, 17 Feb 2025 16:35:15 -0500 Subject: [PATCH 2/5] Fix linter errors Signed-off-by: Brett Tofel --- internal/operator-controller/applier/helm.go | 3 +-- .../authorization/authorization.go | 20 +++++++++---------- 2 files changed, 11 insertions(+), 12 deletions(-) diff --git a/internal/operator-controller/applier/helm.go b/internal/operator-controller/applier/helm.go index c91d6297c..f21baa445 100644 --- a/internal/operator-controller/applier/helm.go +++ b/internal/operator-controller/applier/helm.go @@ -18,12 +18,12 @@ import ( corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" apimachyaml "k8s.io/apimachinery/pkg/util/yaml" + authorizationv1client "k8s.io/client-go/kubernetes/typed/authorization/v1" "k8s.io/client-go/rest" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/log" helmclient "github.com/operator-framework/helm-operator-plugins/pkg/client" - authorizationv1client "k8s.io/client-go/kubernetes/typed/authorization/v1" ocv1 "github.com/operator-framework/operator-controller/api/v1" "github.com/operator-framework/operator-controller/internal/operator-controller/authorization" @@ -111,7 +111,6 @@ func shouldSkipPreflight(ctx context.Context, preflight Preflight, ext *ocv1.Clu } func (h *Helm) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExtension, objectLabels map[string]string, storageLabels map[string]string) ([]client.Object, string, error) { - if features.OperatorControllerFeatureGate.Enabled(features.PreflightPermissions) { authclient, err := h.AuthClientMapper.GetAuthenticationClient(ctx, ext) if err != nil { diff --git a/internal/operator-controller/authorization/authorization.go b/internal/operator-controller/authorization/authorization.go index d1d5d8592..e6cb32465 100644 --- a/internal/operator-controller/authorization/authorization.go +++ b/internal/operator-controller/authorization/authorization.go @@ -7,12 +7,13 @@ import ( "slices" "strings" - ocv1 "github.com/operator-framework/operator-controller/api/v1" - authv1 "k8s.io/api/authorization/v1" + authorizationv1 "k8s.io/api/authorization/v1" rbacv1 "k8s.io/api/rbac/v1" - v1 "k8s.io/apimachinery/pkg/apis/meta/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" authorizationv1client "k8s.io/client-go/kubernetes/typed/authorization/v1" "sigs.k8s.io/controller-runtime/pkg/client" + + ocv1 "github.com/operator-framework/operator-controller/api/v1" ) const ( @@ -21,13 +22,13 @@ const ( ) func CheckObjectPermissions(ctx context.Context, authcl *authorizationv1client.AuthorizationV1Client, objects []client.Object, ext *ocv1.ClusterExtension) error { - ssrr := &authv1.SelfSubjectRulesReview{ - Spec: authv1.SelfSubjectRulesReviewSpec{ + ssrr := &authorizationv1.SelfSubjectRulesReview{ + Spec: authorizationv1.SelfSubjectRulesReviewSpec{ Namespace: ext.Spec.Namespace, }, } - ssrr, err := authcl.SelfSubjectRulesReviews().Create(ctx, ssrr, v1.CreateOptions{}) + ssrr, err := authcl.SelfSubjectRulesReviews().Create(ctx, ssrr, metav1.CreateOptions{}) if err != nil { return err } @@ -49,14 +50,14 @@ func CheckObjectPermissions(ctx context.Context, authcl *authorizationv1client.A }) } - resAttrs := []authv1.ResourceAttributes{} + resAttrs := []authorizationv1.ResourceAttributes{} namespacedErrs := []error{} clusterScopedErrs := []error{} requiredVerbs := []string{"get", "create", "update", "list", "watch", "delete", "patch"} for _, o := range objects { for _, verb := range requiredVerbs { - resAttrs = append(resAttrs, authv1.ResourceAttributes{ + resAttrs = append(resAttrs, authorizationv1.ResourceAttributes{ Namespace: o.GetNamespace(), Verb: verb, Resource: sanitizeResourceName(o.GetObjectKind().GroupVersionKind().Kind), @@ -88,11 +89,10 @@ func CheckObjectPermissions(ctx context.Context, authcl *authorizationv1client.A } return errors.Join(errs...) - } // Checks if the rules allow the verb on the GroupVersionKind in resAttr -func canI(resAttr authv1.ResourceAttributes, rules []rbacv1.PolicyRule) bool { +func canI(resAttr authorizationv1.ResourceAttributes, rules []rbacv1.PolicyRule) bool { var canI bool for _, rule := range rules { if (slices.Contains(rule.APIGroups, resAttr.Group) || slices.Contains(rule.APIGroups, "*")) && From 5b9fd7a795da562f16ad77d8a0b12324f6fed823 Mon Sep 17 00:00:00 2001 From: Brett Tofel Date: Wed, 19 Feb 2025 10:02:34 -0500 Subject: [PATCH 3/5] Change AuthorizationV1Client to interface for mocking Signed-off-by: Brett Tofel --- cmd/operator-controller/main.go | 6 + internal/operator-controller/applier/helm.go | 18 ++- .../operator-controller/applier/helm_test.go | 121 ++++++++++++++++-- .../authorization/authorization.go | 2 +- 4 files changed, 126 insertions(+), 21 deletions(-) diff --git a/cmd/operator-controller/main.go b/cmd/operator-controller/main.go index eeb694df4..35f2409f6 100644 --- a/cmd/operator-controller/main.go +++ b/cmd/operator-controller/main.go @@ -37,8 +37,10 @@ import ( k8slabels "k8s.io/apimachinery/pkg/labels" k8stypes "k8s.io/apimachinery/pkg/types" apimachineryrand "k8s.io/apimachinery/pkg/util/rand" + authorizationv1client "k8s.io/client-go/kubernetes/typed/authorization/v1" corev1client "k8s.io/client-go/kubernetes/typed/core/v1" _ "k8s.io/client-go/plugin/pkg/client/auth" + "k8s.io/client-go/rest" "k8s.io/klog/v2" "k8s.io/klog/v2/textlogger" "k8s.io/utils/ptr" @@ -408,6 +410,10 @@ func run() error { } acm := applier.NewAuthClientMapper(clientRestConfigMapper, mgr.GetConfig()) + acm.NewForConfig = func(cfg *rest.Config) (authorizationv1client.AuthorizationV1Interface, error) { + // *AuthorizationV1Client implements AuthorizationV1Interface + return authorizationv1client.NewForConfig(cfg) + } helmApplier := &applier.Helm{ ActionClientGetter: acg, diff --git a/internal/operator-controller/applier/helm.go b/internal/operator-controller/applier/helm.go index f21baa445..a43674087 100644 --- a/internal/operator-controller/applier/helm.go +++ b/internal/operator-controller/applier/helm.go @@ -58,23 +58,21 @@ type Preflight interface { type RestConfigMapper func(context.Context, client.Object, *rest.Config) (*rest.Config, error) +type NewForConfigFunc func(*rest.Config) (authorizationv1client.AuthorizationV1Interface, error) + type AuthClientMapper struct { - rcm RestConfigMapper - baseCfg *rest.Config + rcm RestConfigMapper + baseCfg *rest.Config + NewForConfig NewForConfigFunc } -func (acm *AuthClientMapper) GetAuthenticationClient(ctx context.Context, ext *ocv1.ClusterExtension) (*authorizationv1client.AuthorizationV1Client, error) { +func (acm *AuthClientMapper) GetAuthenticationClient(ctx context.Context, ext *ocv1.ClusterExtension) (authorizationv1client.AuthorizationV1Interface, error) { authcfg, err := acm.rcm(ctx, ext, acm.baseCfg) if err != nil { return nil, err } - authclient, err := authorizationv1client.NewForConfig(authcfg) - if err != nil { - return nil, err - } - - return authclient, nil + return acm.NewForConfig(authcfg) } type Helm struct { @@ -198,7 +196,7 @@ func (h *Helm) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExte } // Check if RBAC allows the installer service account necessary permissions on the objects in the contentFS -func (h *Helm) checkContentPermissions(ctx context.Context, contentFS fs.FS, authcl *authorizationv1client.AuthorizationV1Client, ext *ocv1.ClusterExtension) error { +func (h *Helm) checkContentPermissions(ctx context.Context, contentFS fs.FS, authcl authorizationv1client.AuthorizationV1Interface, ext *ocv1.ClusterExtension) error { reg, err := convert.ParseFS(ctx, contentFS) if err != nil { return err diff --git a/internal/operator-controller/applier/helm_test.go b/internal/operator-controller/applier/helm_test.go index 78d6629a5..5c3faedf5 100644 --- a/internal/operator-controller/applier/helm_test.go +++ b/internal/operator-controller/applier/helm_test.go @@ -13,6 +13,10 @@ import ( "helm.sh/helm/v3/pkg/chart" "helm.sh/helm/v3/pkg/release" "helm.sh/helm/v3/pkg/storage/driver" + authorizationv1 "k8s.io/api/authorization/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + authorizationv1client "k8s.io/client-go/kubernetes/typed/authorization/v1" + "k8s.io/client-go/rest" featuregatetesting "k8s.io/component-base/featuregate/testing" "sigs.k8s.io/controller-runtime/pkg/client" @@ -94,6 +98,83 @@ func (mag *mockActionGetter) Reconcile(rel *release.Release) error { return mag.reconcileErr } +// Helper returns an AuthorizationV1Interface where SSRR always passes. +func newPassingSSRRAuthClient() authorizationv1client.AuthorizationV1Interface { + return &mockAuthorizationV1Client{ + ssrrInterface: &mockSelfSubjectRulesReviewInterface{ + retSSRR: &authorizationv1.SelfSubjectRulesReview{ + Status: authorizationv1.SubjectRulesReviewStatus{ + ResourceRules: []authorizationv1.ResourceRule{{ + Verbs: []string{"*"}, + APIGroups: []string{"*"}, + Resources: []string{"*"}, + }}, + }, + }, + }, + } +} + +// Helper builds an AuthClientMapper with the passing SSRR +func newPassingAuthClientMapper() applier.AuthClientMapper { + fakeRestConfig := &rest.Config{Host: "fake-server"} + mockRCM := func(ctx context.Context, obj client.Object, cfg *rest.Config) (*rest.Config, error) { + return cfg, nil + } + acm := applier.NewAuthClientMapper(mockRCM, fakeRestConfig) + acm.NewForConfig = func(*rest.Config) (authorizationv1client.AuthorizationV1Interface, error) { + return newPassingSSRRAuthClient(), nil + } + return acm +} + +// Helper builds a Helm applier with passing SSRR +func buildHelmApplier(mockAcg *mockActionGetter, preflights []applier.Preflight) applier.Helm { + return applier.Helm{ + ActionClientGetter: mockAcg, + AuthClientMapper: newPassingAuthClientMapper(), + Preflights: preflights, + } +} + +type mockAuthorizationV1Client struct { + ssrrInterface authorizationv1client.SelfSubjectRulesReviewInterface +} + +func (m *mockAuthorizationV1Client) SelfSubjectRulesReviews() authorizationv1client.SelfSubjectRulesReviewInterface { + return m.ssrrInterface +} +func (m *mockAuthorizationV1Client) RESTClient() rest.Interface { + return nil +} + +// Mock for SelfSubjectRulesReviewInterface +type mockSelfSubjectRulesReviewInterface struct { + retSSRR *authorizationv1.SelfSubjectRulesReview + retErr error +} + +func (m *mockSelfSubjectRulesReviewInterface) Create( + ctx context.Context, + ssrr *authorizationv1.SelfSubjectRulesReview, + opts metav1.CreateOptions, +) (*authorizationv1.SelfSubjectRulesReview, error) { + // Return either a success or an error, depending on what you want in the test. + return m.retSSRR, m.retErr +} + +func (m *mockAuthorizationV1Client) LocalSubjectAccessReviews(namespace string) authorizationv1client.LocalSubjectAccessReviewInterface { + return nil +} + +func (m *mockAuthorizationV1Client) SelfSubjectAccessReviews() authorizationv1client.SelfSubjectAccessReviewInterface { + return nil +} + +func (m *mockAuthorizationV1Client) SubjectAccessReviews() authorizationv1client.SubjectAccessReviewInterface { + return nil +} + var ( // required for unmockable call to convert.RegistryV1ToHelmChart validFS = fstest.MapFS{ @@ -229,6 +310,7 @@ func TestApply_Installation(t *testing.T) { } func TestApply_InstallationWithPreflightPermissionsEnabled(t *testing.T) { + // Set feature gate ONCE at parent level featuregatetesting.SetFeatureGateDuringTest(t, features.OperatorControllerFeatureGate, features.PreflightPermissions, true) t.Run("fails during dry-run installation", func(t *testing.T) { @@ -236,9 +318,15 @@ func TestApply_InstallationWithPreflightPermissionsEnabled(t *testing.T) { getClientErr: driver.ErrReleaseNotFound, dryRunInstallErr: errors.New("failed attempting to dry-run install chart"), } - helmApplier := applier.Helm{ActionClientGetter: mockAcg} - - objs, state, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels) + helmApplier := buildHelmApplier(mockAcg, nil) + + objs, state, err := helmApplier.Apply( + context.TODO(), + validFS, + testCE, + testObjectLabels, + testStorageLabels, + ) require.Error(t, err) require.ErrorContains(t, err, "attempting to dry-run install chart") require.Nil(t, objs) @@ -251,7 +339,8 @@ func TestApply_InstallationWithPreflightPermissionsEnabled(t *testing.T) { installErr: errors.New("failed installing chart"), } mockPf := &mockPreflight{installErr: errors.New("failed during install pre-flight check")} - helmApplier := applier.Helm{ActionClientGetter: mockAcg, Preflights: []applier.Preflight{mockPf}} + + helmApplier := buildHelmApplier(mockAcg, []applier.Preflight{mockPf}) objs, state, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels) require.Error(t, err) @@ -265,9 +354,15 @@ func TestApply_InstallationWithPreflightPermissionsEnabled(t *testing.T) { getClientErr: driver.ErrReleaseNotFound, installErr: errors.New("failed installing chart"), } - helmApplier := applier.Helm{ActionClientGetter: mockAcg} - - objs, state, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels) + helmApplier := buildHelmApplier(mockAcg, nil) + + objs, state, err := helmApplier.Apply( + context.TODO(), + validFS, + testCE, + testObjectLabels, + testStorageLabels, + ) require.Error(t, err) require.ErrorContains(t, err, "installing chart") require.Equal(t, applier.StateNeedsInstall, state) @@ -282,9 +377,15 @@ func TestApply_InstallationWithPreflightPermissionsEnabled(t *testing.T) { Manifest: validManifest, }, } - helmApplier := applier.Helm{ActionClientGetter: mockAcg} - - objs, state, err := helmApplier.Apply(context.TODO(), validFS, testCE, testObjectLabels, testStorageLabels) + helmApplier := buildHelmApplier(mockAcg, nil) + + objs, state, err := helmApplier.Apply( + context.TODO(), + validFS, + testCE, + testObjectLabels, + testStorageLabels, + ) require.NoError(t, err) require.Equal(t, applier.StateNeedsInstall, state) require.NotNil(t, objs) diff --git a/internal/operator-controller/authorization/authorization.go b/internal/operator-controller/authorization/authorization.go index e6cb32465..9e848817b 100644 --- a/internal/operator-controller/authorization/authorization.go +++ b/internal/operator-controller/authorization/authorization.go @@ -21,7 +21,7 @@ const ( SelfSubjectAccessReview string = "SelfSubjectAccessReview" ) -func CheckObjectPermissions(ctx context.Context, authcl *authorizationv1client.AuthorizationV1Client, objects []client.Object, ext *ocv1.ClusterExtension) error { +func CheckObjectPermissions(ctx context.Context, authcl authorizationv1client.AuthorizationV1Interface, objects []client.Object, ext *ocv1.ClusterExtension) error { ssrr := &authorizationv1.SelfSubjectRulesReview{ Spec: authorizationv1.SelfSubjectRulesReviewSpec{ Namespace: ext.Spec.Namespace, From 7c9a05eb9f8c3a9be6e0bf8cf7a5beb09315774e Mon Sep 17 00:00:00 2001 From: Tayler Geiger Date: Wed, 19 Feb 2025 14:01:19 -0600 Subject: [PATCH 4/5] Clean up and organize authorization package Move all authorization-related code to the authorization package, rename anything using 'auth' to 'authorization' to avoid confusion with authentication. --- cmd/operator-controller/main.go | 9 +-- internal/operator-controller/applier/helm.go | 57 ++------------- .../operator-controller/applier/helm_test.go | 12 ++-- .../authorization/authorization.go | 72 +++++++++++++++---- 4 files changed, 77 insertions(+), 73 deletions(-) diff --git a/cmd/operator-controller/main.go b/cmd/operator-controller/main.go index 35f2409f6..2da4af083 100644 --- a/cmd/operator-controller/main.go +++ b/cmd/operator-controller/main.go @@ -61,6 +61,7 @@ import ( "github.com/operator-framework/operator-controller/internal/operator-controller/action" "github.com/operator-framework/operator-controller/internal/operator-controller/applier" "github.com/operator-framework/operator-controller/internal/operator-controller/authentication" + "github.com/operator-framework/operator-controller/internal/operator-controller/authorization" "github.com/operator-framework/operator-controller/internal/operator-controller/catalogmetadata/cache" catalogclient "github.com/operator-framework/operator-controller/internal/operator-controller/catalogmetadata/client" "github.com/operator-framework/operator-controller/internal/operator-controller/contentmanager" @@ -409,16 +410,16 @@ func run() error { crdupgradesafety.NewPreflight(aeClient.CustomResourceDefinitions()), } - acm := applier.NewAuthClientMapper(clientRestConfigMapper, mgr.GetConfig()) + acm := authorization.NewAuthorizationClientMapper(clientRestConfigMapper, mgr.GetConfig()) acm.NewForConfig = func(cfg *rest.Config) (authorizationv1client.AuthorizationV1Interface, error) { // *AuthorizationV1Client implements AuthorizationV1Interface return authorizationv1client.NewForConfig(cfg) } helmApplier := &applier.Helm{ - ActionClientGetter: acg, - Preflights: preflights, - AuthClientMapper: acm, + ActionClientGetter: acg, + Preflights: preflights, + AuthorizationClientMapper: acm, } cm := contentmanager.NewManager(clientRestConfigMapper, mgr.GetConfig(), mgr.GetRESTMapper()) diff --git a/internal/operator-controller/applier/helm.go b/internal/operator-controller/applier/helm.go index a43674087..7ce93d700 100644 --- a/internal/operator-controller/applier/helm.go +++ b/internal/operator-controller/applier/helm.go @@ -18,8 +18,6 @@ import ( corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" apimachyaml "k8s.io/apimachinery/pkg/util/yaml" - authorizationv1client "k8s.io/client-go/kubernetes/typed/authorization/v1" - "k8s.io/client-go/rest" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/log" @@ -56,36 +54,10 @@ type Preflight interface { Upgrade(context.Context, *release.Release) error } -type RestConfigMapper func(context.Context, client.Object, *rest.Config) (*rest.Config, error) - -type NewForConfigFunc func(*rest.Config) (authorizationv1client.AuthorizationV1Interface, error) - -type AuthClientMapper struct { - rcm RestConfigMapper - baseCfg *rest.Config - NewForConfig NewForConfigFunc -} - -func (acm *AuthClientMapper) GetAuthenticationClient(ctx context.Context, ext *ocv1.ClusterExtension) (authorizationv1client.AuthorizationV1Interface, error) { - authcfg, err := acm.rcm(ctx, ext, acm.baseCfg) - if err != nil { - return nil, err - } - - return acm.NewForConfig(authcfg) -} - type Helm struct { - ActionClientGetter helmclient.ActionClientGetter - Preflights []Preflight - AuthClientMapper AuthClientMapper -} - -func NewAuthClientMapper(rcm RestConfigMapper, baseCfg *rest.Config) AuthClientMapper { - return AuthClientMapper{ - rcm: rcm, - baseCfg: baseCfg, - } + ActionClientGetter helmclient.ActionClientGetter + Preflights []Preflight + AuthorizationClientMapper authorization.AuthorizationClientMapper } // shouldSkipPreflight is a helper to determine if the preflight check is CRDUpgradeSafety AND @@ -110,14 +82,14 @@ func shouldSkipPreflight(ctx context.Context, preflight Preflight, ext *ocv1.Clu func (h *Helm) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExtension, objectLabels map[string]string, storageLabels map[string]string) ([]client.Object, string, error) { if features.OperatorControllerFeatureGate.Enabled(features.PreflightPermissions) { - authclient, err := h.AuthClientMapper.GetAuthenticationClient(ctx, ext) + authclient, err := h.AuthorizationClientMapper.GetAuthorizationClient(ctx, ext) if err != nil { return nil, "", err } - err = h.checkContentPermissions(ctx, contentFS, authclient, ext) + err = h.AuthorizationClientMapper.CheckContentPermissions(ctx, contentFS, authclient, ext) if err != nil { - return nil, "", err + return nil, "", fmt.Errorf("failed checking content permissions: %w", err) } } @@ -195,23 +167,6 @@ func (h *Helm) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExte return relObjects, state, nil } -// Check if RBAC allows the installer service account necessary permissions on the objects in the contentFS -func (h *Helm) checkContentPermissions(ctx context.Context, contentFS fs.FS, authcl authorizationv1client.AuthorizationV1Interface, ext *ocv1.ClusterExtension) error { - reg, err := convert.ParseFS(ctx, contentFS) - if err != nil { - return err - } - - plain, err := convert.Convert(reg, ext.Spec.Namespace, []string{corev1.NamespaceAll}) - if err != nil { - return err - } - - err = authorization.CheckObjectPermissions(ctx, authcl, plain.Objects, ext) - - return err -} - func (h *Helm) getReleaseState(cl helmclient.ActionInterface, ext *ocv1.ClusterExtension, chrt *chart.Chart, values chartutil.Values, post postrender.PostRenderer) (*release.Release, *release.Release, string, error) { currentRelease, err := cl.Get(ext.GetName()) diff --git a/internal/operator-controller/applier/helm_test.go b/internal/operator-controller/applier/helm_test.go index 5c3faedf5..384ec7e60 100644 --- a/internal/operator-controller/applier/helm_test.go +++ b/internal/operator-controller/applier/helm_test.go @@ -115,13 +115,13 @@ func newPassingSSRRAuthClient() authorizationv1client.AuthorizationV1Interface { } } -// Helper builds an AuthClientMapper with the passing SSRR -func newPassingAuthClientMapper() applier.AuthClientMapper { +// Helper builds an AuthorizationClientMapper with the passing SSRR +func newPassingAuthorizationClientMapper() applier.AuthorizationClientMapper { fakeRestConfig := &rest.Config{Host: "fake-server"} mockRCM := func(ctx context.Context, obj client.Object, cfg *rest.Config) (*rest.Config, error) { return cfg, nil } - acm := applier.NewAuthClientMapper(mockRCM, fakeRestConfig) + acm := applier.NewAuthorizationClientMapper(mockRCM, fakeRestConfig) acm.NewForConfig = func(*rest.Config) (authorizationv1client.AuthorizationV1Interface, error) { return newPassingSSRRAuthClient(), nil } @@ -131,9 +131,9 @@ func newPassingAuthClientMapper() applier.AuthClientMapper { // Helper builds a Helm applier with passing SSRR func buildHelmApplier(mockAcg *mockActionGetter, preflights []applier.Preflight) applier.Helm { return applier.Helm{ - ActionClientGetter: mockAcg, - AuthClientMapper: newPassingAuthClientMapper(), - Preflights: preflights, + ActionClientGetter: mockAcg, + AuthorizationClientMapper: newPassingAuthorizationClientMapper(), + Preflights: preflights, } } diff --git a/internal/operator-controller/authorization/authorization.go b/internal/operator-controller/authorization/authorization.go index 9e848817b..3b6fe6ea4 100644 --- a/internal/operator-controller/authorization/authorization.go +++ b/internal/operator-controller/authorization/authorization.go @@ -4,16 +4,20 @@ import ( "context" "errors" "fmt" + "io/fs" "slices" "strings" - authorizationv1 "k8s.io/api/authorization/v1" + ocv1 "github.com/operator-framework/operator-controller/api/v1" + "github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/convert" + authv1 "k8s.io/api/authorization/v1" + corev1 "k8s.io/api/core/v1" rbacv1 "k8s.io/api/rbac/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + v1 "k8s.io/apimachinery/pkg/apis/meta/v1" authorizationv1client "k8s.io/client-go/kubernetes/typed/authorization/v1" + "k8s.io/client-go/rest" "sigs.k8s.io/controller-runtime/pkg/client" - - ocv1 "github.com/operator-framework/operator-controller/api/v1" ) const ( @@ -21,14 +25,58 @@ const ( SelfSubjectAccessReview string = "SelfSubjectAccessReview" ) -func CheckObjectPermissions(ctx context.Context, authcl authorizationv1client.AuthorizationV1Interface, objects []client.Object, ext *ocv1.ClusterExtension) error { - ssrr := &authorizationv1.SelfSubjectRulesReview{ - Spec: authorizationv1.SelfSubjectRulesReviewSpec{ +type RestConfigMapper func(context.Context, client.Object, *rest.Config) (*rest.Config, error) + +type NewForConfigFunc func(*rest.Config) (authorizationv1client.AuthorizationV1Interface, error) + +type AuthorizationClientMapper struct { + rcm RestConfigMapper + baseCfg *rest.Config + NewForConfig NewForConfigFunc +} + +func NewAuthorizationClientMapper(rcm RestConfigMapper, baseCfg *rest.Config) AuthorizationClientMapper { + return AuthorizationClientMapper{ + rcm: rcm, + baseCfg: baseCfg, + } +} + +func (acm *AuthorizationClientMapper) GetAuthorizationClient(ctx context.Context, ext *ocv1.ClusterExtension) (authorizationv1client.AuthorizationV1Interface, error) { + authcfg, err := acm.rcm(ctx, ext, acm.baseCfg) + if err != nil { + return nil, err + } + + return acm.NewForConfig(authcfg) +} + +// Check if RBAC allows the installer service account necessary permissions on the objects in the contentFS +func (acm *AuthorizationClientMapper) CheckContentPermissions(ctx context.Context, contentFS fs.FS, authcl authorizationv1client.AuthorizationV1Interface, ext *ocv1.ClusterExtension) error { + reg, err := convert.ParseFS(ctx, contentFS) + if err != nil { + return err + } + + plain, err := convert.Convert(reg, ext.Spec.Namespace, []string{corev1.NamespaceAll}) + if err != nil { + return err + } + + err = checkObjectPermissions(ctx, authcl, plain.Objects, ext) + + return err +} + +func checkObjectPermissions(ctx context.Context, authcl authorizationv1client.AuthorizationV1Interface, objects []client.Object, ext *ocv1.ClusterExtension) error { + ssrr := &authv1.SelfSubjectRulesReview{ + Spec: authv1.SelfSubjectRulesReviewSpec{ Namespace: ext.Spec.Namespace, }, } - ssrr, err := authcl.SelfSubjectRulesReviews().Create(ctx, ssrr, metav1.CreateOptions{}) + opts := v1.CreateOptions{} + ssrr, err := authcl.SelfSubjectRulesReviews().Create(ctx, ssrr, opts) if err != nil { return err } @@ -50,14 +98,14 @@ func CheckObjectPermissions(ctx context.Context, authcl authorizationv1client.Au }) } - resAttrs := []authorizationv1.ResourceAttributes{} namespacedErrs := []error{} clusterScopedErrs := []error{} requiredVerbs := []string{"get", "create", "update", "list", "watch", "delete", "patch"} + resAttrs := make([]authv1.ResourceAttributes, 0, len(requiredVerbs)*len(objects)) for _, o := range objects { for _, verb := range requiredVerbs { - resAttrs = append(resAttrs, authorizationv1.ResourceAttributes{ + resAttrs = append(resAttrs, authv1.ResourceAttributes{ Namespace: o.GetNamespace(), Verb: verb, Resource: sanitizeResourceName(o.GetObjectKind().GroupVersionKind().Kind), @@ -70,7 +118,7 @@ func CheckObjectPermissions(ctx context.Context, authcl authorizationv1client.Au for _, resAttr := range resAttrs { if !canI(resAttr, rules) { if resAttr.Namespace != "" { - namespacedErrs = append(namespacedErrs, fmt.Errorf("cannot %s %s %s in namespace %s", + namespacedErrs = append(namespacedErrs, fmt.Errorf("cannot %q %q %q in namespace %q", resAttr.Verb, strings.TrimSuffix(resAttr.Resource, "s"), resAttr.Name, @@ -92,7 +140,7 @@ func CheckObjectPermissions(ctx context.Context, authcl authorizationv1client.Au } // Checks if the rules allow the verb on the GroupVersionKind in resAttr -func canI(resAttr authorizationv1.ResourceAttributes, rules []rbacv1.PolicyRule) bool { +func canI(resAttr authv1.ResourceAttributes, rules []rbacv1.PolicyRule) bool { var canI bool for _, rule := range rules { if (slices.Contains(rule.APIGroups, resAttr.Group) || slices.Contains(rule.APIGroups, "*")) && From b632003cee7ba4e9394eb7fb528f338752270f62 Mon Sep 17 00:00:00 2001 From: Tayler Geiger Date: Wed, 26 Feb 2025 15:37:41 -0600 Subject: [PATCH 5/5] Add authorization client Adds authorization client to authorization package which helps with testing. Also makes errors more descriptive. Signed-off-by: Tayler Geiger Co-authored-by: Brett Tofel --- internal/operator-controller/applier/helm.go | 53 +++++++++++-------- .../operator-controller/applier/helm_test.go | 3 +- .../authorization/authorization.go | 46 ++++------------ .../authorization/client.go | 33 ++++++++++++ 4 files changed, 76 insertions(+), 59 deletions(-) create mode 100644 internal/operator-controller/authorization/client.go diff --git a/internal/operator-controller/applier/helm.go b/internal/operator-controller/applier/helm.go index 7ce93d700..c63c7ab03 100644 --- a/internal/operator-controller/applier/helm.go +++ b/internal/operator-controller/applier/helm.go @@ -82,13 +82,13 @@ func shouldSkipPreflight(ctx context.Context, preflight Preflight, ext *ocv1.Clu func (h *Helm) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExtension, objectLabels map[string]string, storageLabels map[string]string) ([]client.Object, string, error) { if features.OperatorControllerFeatureGate.Enabled(features.PreflightPermissions) { - authclient, err := h.AuthorizationClientMapper.GetAuthorizationClient(ctx, ext) + rawAuthClient, err := h.AuthorizationClientMapper.GetAuthorizationClient(ctx, ext) if err != nil { - return nil, "", err + return nil, "", fmt.Errorf("failed to get authorization client: %w", err) } - err = h.AuthorizationClientMapper.CheckContentPermissions(ctx, contentFS, authclient, ext) - if err != nil { + authClient := authorization.NewClient(rawAuthClient) + if err := h.checkContentPermissions(ctx, contentFS, authClient, ext); err != nil { return nil, "", fmt.Errorf("failed checking content permissions: %w", err) } } @@ -102,7 +102,7 @@ func (h *Helm) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExte ac, err := h.ActionClientGetter.ActionClientFor(ctx, ext) if err != nil { - return nil, "", err + return nil, "", fmt.Errorf("failed to get action client: %w", err) } post := &postrenderer{ @@ -120,14 +120,12 @@ func (h *Helm) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExte } switch state { case StateNeedsInstall: - err := preflight.Install(ctx, desiredRel) - if err != nil { - return nil, state, err + if err := preflight.Install(ctx, desiredRel); err != nil { + return nil, state, fmt.Errorf("preflight install check failed: %w", err) } case StateNeedsUpgrade: - err := preflight.Upgrade(ctx, desiredRel) - if err != nil { - return nil, state, err + if err := preflight.Upgrade(ctx, desiredRel); err != nil { + return nil, state, fmt.Errorf("preflight upgrade check failed: %w", err) } } } @@ -140,7 +138,7 @@ func (h *Helm) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExte return nil }, helmclient.AppendInstallPostRenderer(post)) if err != nil { - return nil, state, err + return nil, state, fmt.Errorf("failed to install release: %w", err) } case StateNeedsUpgrade: rel, err = ac.Upgrade(ext.GetName(), ext.Spec.Namespace, chrt, values, func(upgrade *action.Upgrade) error { @@ -149,11 +147,11 @@ func (h *Helm) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExte return nil }, helmclient.AppendUpgradePostRenderer(post)) if err != nil { - return nil, state, err + return nil, state, fmt.Errorf("failed to upgrade release: %w", err) } case StateUnchanged: if err := ac.Reconcile(rel); err != nil { - return nil, state, err + return nil, state, fmt.Errorf("failed to reconcile release: %w", err) } default: return nil, state, fmt.Errorf("unexpected release state %q", state) @@ -161,12 +159,27 @@ func (h *Helm) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExte relObjects, err := util.ManifestObjects(strings.NewReader(rel.Manifest), fmt.Sprintf("%s-release-manifest", rel.Name)) if err != nil { - return nil, state, err + return nil, state, fmt.Errorf("failed to convert manifest to objects: %w", err) } return relObjects, state, nil } +// Check if RBAC allows the installer service account necessary permissions on the objects in the contentFS +func (h *Helm) checkContentPermissions(ctx context.Context, contentFS fs.FS, authClient authorization.AuthorizationClient, ext *ocv1.ClusterExtension) error { + reg, err := convert.ParseFS(ctx, contentFS) + if err != nil { + return fmt.Errorf("failed to parse content FS: %w", err) + } + + plain, err := convert.Convert(reg, ext.Spec.Namespace, []string{corev1.NamespaceAll}) + if err != nil { + return fmt.Errorf("failed to convert registry: %w", err) + } + + return authClient.CheckContentPermissions(ctx, plain.Objects, ext) +} + func (h *Helm) getReleaseState(cl helmclient.ActionInterface, ext *ocv1.ClusterExtension, chrt *chart.Chart, values chartutil.Values, post postrender.PostRenderer) (*release.Release, *release.Release, string, error) { currentRelease, err := cl.Get(ext.GetName()) @@ -177,16 +190,12 @@ func (h *Helm) getReleaseState(cl helmclient.ActionInterface, ext *ocv1.ClusterE return nil }, helmclient.AppendInstallPostRenderer(post)) if err != nil { - if features.OperatorControllerFeatureGate.Enabled(features.PreflightPermissions) { - _ = struct{}{} // minimal no-op to satisfy linter - // probably need to break out this error as it's the one for helm dry-run as opposed to any returned later - } - return nil, nil, StateError, err + return nil, nil, StateError, fmt.Errorf("failed dry-run install: %w", err) } return nil, desiredRelease, StateNeedsInstall, nil } if err != nil { - return nil, nil, StateError, err + return nil, nil, StateError, fmt.Errorf("failed to get current release: %w", err) } desiredRelease, err := cl.Upgrade(ext.GetName(), ext.Spec.Namespace, chrt, values, func(upgrade *action.Upgrade) error { @@ -196,7 +205,7 @@ func (h *Helm) getReleaseState(cl helmclient.ActionInterface, ext *ocv1.ClusterE return nil }, helmclient.AppendUpgradePostRenderer(post)) if err != nil { - return currentRelease, nil, StateError, err + return currentRelease, nil, StateError, fmt.Errorf("failed dry-run upgrade: %w", err) } relState := StateUnchanged if desiredRelease.Manifest != currentRelease.Manifest || diff --git a/internal/operator-controller/applier/helm_test.go b/internal/operator-controller/applier/helm_test.go index 384ec7e60..73a15ff62 100644 --- a/internal/operator-controller/applier/helm_test.go +++ b/internal/operator-controller/applier/helm_test.go @@ -24,6 +24,7 @@ import ( v1 "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/features" ) @@ -121,7 +122,7 @@ func newPassingAuthorizationClientMapper() applier.AuthorizationClientMapper { mockRCM := func(ctx context.Context, obj client.Object, cfg *rest.Config) (*rest.Config, error) { return cfg, nil } - acm := applier.NewAuthorizationClientMapper(mockRCM, fakeRestConfig) + acm := authorization.NewAuthorizationClientMapper(mockRCM, fakeRestConfig) acm.NewForConfig = func(*rest.Config) (authorizationv1client.AuthorizationV1Interface, error) { return newPassingSSRRAuthClient(), nil } diff --git a/internal/operator-controller/authorization/authorization.go b/internal/operator-controller/authorization/authorization.go index 3b6fe6ea4..7b637ef10 100644 --- a/internal/operator-controller/authorization/authorization.go +++ b/internal/operator-controller/authorization/authorization.go @@ -4,14 +4,11 @@ import ( "context" "errors" "fmt" - "io/fs" "slices" "strings" ocv1 "github.com/operator-framework/operator-controller/api/v1" - "github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/convert" authv1 "k8s.io/api/authorization/v1" - corev1 "k8s.io/api/core/v1" rbacv1 "k8s.io/api/rbac/v1" v1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -20,15 +17,8 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" ) -const ( - SelfSubjectRulesReview string = "SelfSubjectRulesReview" - SelfSubjectAccessReview string = "SelfSubjectAccessReview" -) - type RestConfigMapper func(context.Context, client.Object, *rest.Config) (*rest.Config, error) -type NewForConfigFunc func(*rest.Config) (authorizationv1client.AuthorizationV1Interface, error) - type AuthorizationClientMapper struct { rcm RestConfigMapper baseCfg *rest.Config @@ -47,28 +37,11 @@ func (acm *AuthorizationClientMapper) GetAuthorizationClient(ctx context.Context if err != nil { return nil, err } - return acm.NewForConfig(authcfg) } -// Check if RBAC allows the installer service account necessary permissions on the objects in the contentFS -func (acm *AuthorizationClientMapper) CheckContentPermissions(ctx context.Context, contentFS fs.FS, authcl authorizationv1client.AuthorizationV1Interface, ext *ocv1.ClusterExtension) error { - reg, err := convert.ParseFS(ctx, contentFS) - if err != nil { - return err - } - - plain, err := convert.Convert(reg, ext.Spec.Namespace, []string{corev1.NamespaceAll}) - if err != nil { - return err - } - - err = checkObjectPermissions(ctx, authcl, plain.Objects, ext) - - return err -} - -func checkObjectPermissions(ctx context.Context, authcl authorizationv1client.AuthorizationV1Interface, objects []client.Object, ext *ocv1.ClusterExtension) error { +// CheckObjectPermissions verifies that the given objects have the required permissions. +func CheckObjectPermissions(ctx context.Context, authcl authorizationv1client.AuthorizationV1Interface, objects []client.Object, ext *ocv1.ClusterExtension) error { ssrr := &authv1.SelfSubjectRulesReview{ Spec: authv1.SelfSubjectRulesReviewSpec{ Namespace: ext.Spec.Namespace, @@ -78,7 +51,7 @@ func checkObjectPermissions(ctx context.Context, authcl authorizationv1client.Au opts := v1.CreateOptions{} ssrr, err := authcl.SelfSubjectRulesReviews().Create(ctx, ssrr, opts) if err != nil { - return err + return fmt.Errorf("failed to create SelfSubjectRulesReview: %w", err) } rules := []rbacv1.PolicyRule{} @@ -118,25 +91,26 @@ func checkObjectPermissions(ctx context.Context, authcl authorizationv1client.Au for _, resAttr := range resAttrs { if !canI(resAttr, rules) { if resAttr.Namespace != "" { - namespacedErrs = append(namespacedErrs, fmt.Errorf("cannot %q %q %q in namespace %q", + namespacedErrs = append(namespacedErrs, fmt.Errorf("cannot %s %q %q in namespace %q", resAttr.Verb, strings.TrimSuffix(resAttr.Resource, "s"), resAttr.Name, resAttr.Namespace)) continue } - clusterScopedErrs = append(clusterScopedErrs, fmt.Errorf("cannot %s %s %s", + clusterScopedErrs = append(clusterScopedErrs, fmt.Errorf("cannot %s %q %q", resAttr.Verb, strings.TrimSuffix(resAttr.Resource, "s"), resAttr.Name)) } } - errs := append(namespacedErrs, clusterScopedErrs...) - if len(errs) > 0 { - errs = append([]error{fmt.Errorf("installer service account %s is missing required permissions", ext.Spec.ServiceAccount.Name)}, errs...) + + allErrs := append(namespacedErrs, clusterScopedErrs...) + if len(allErrs) > 0 { + return fmt.Errorf("installer service account %q is missing required permissions: %w", ext.Spec.ServiceAccount.Name, errors.Join(allErrs...)) } - return errors.Join(errs...) + return nil } // Checks if the rules allow the verb on the GroupVersionKind in resAttr diff --git a/internal/operator-controller/authorization/client.go b/internal/operator-controller/authorization/client.go new file mode 100644 index 000000000..8a855b513 --- /dev/null +++ b/internal/operator-controller/authorization/client.go @@ -0,0 +1,33 @@ +package authorization + +import ( + "context" + + authorizationv1client "k8s.io/client-go/kubernetes/typed/authorization/v1" + "k8s.io/client-go/rest" + "sigs.k8s.io/controller-runtime/pkg/client" + + ocv1 "github.com/operator-framework/operator-controller/api/v1" +) + +type NewForConfigFunc func(*rest.Config) (authorizationv1client.AuthorizationV1Interface, error) + +// AuthorizationClient is an interface exposing only the needed functionality +type AuthorizationClient interface { + CheckContentPermissions(ctx context.Context, objects []client.Object, ext *ocv1.ClusterExtension) error +} + +// clientImpl wraps the underlying authorization client +type clientImpl struct { + authClient authorizationv1client.AuthorizationV1Interface +} + +// NewClient wraps an authorizationv1client.AuthorizationV1Interface +func NewClient(authClient authorizationv1client.AuthorizationV1Interface) AuthorizationClient { + return &clientImpl{authClient: authClient} +} + +// CheckContentPermissions is the public method that internally calls the generic check +func (c *clientImpl) CheckContentPermissions(ctx context.Context, objects []client.Object, ext *ocv1.ClusterExtension) error { + return CheckObjectPermissions(ctx, c.authClient, objects, ext) +}