From 74c93c73063a6693d7bfd3e1299669b235120a2b Mon Sep 17 00:00:00 2001 From: Tayler Geiger Date: Thu, 13 Feb 2025 10:13:39 -0600 Subject: [PATCH] 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" +}