From 8eb4f3e5cb786c47b137c6650c2c530718bc7906 Mon Sep 17 00:00:00 2001 From: Steve Kuznetsov Date: Mon, 18 Sep 2023 12:18:48 -0400 Subject: [PATCH] pkg/controller: label RBAC with content hash (#3034) When a CSV is processed, it is assumed that the InstallPlan has already run, or that a user that's creating a CSV as their entrypoint into the system has otherwise met all the preconditions for the CSV to exist. As part of validating these preconditions, the CSV logic today uses cluster-scoped listers for all RBAC resources. sets up an in-memory authorizer for these, and queries the CSV install strategie's permissions against those. We would like to restrict the amount of memory OLM uses, and part of that is not caching the world. For the above approach to work, all RBAC objects fulfilling CSV permission preconditions would need to be labelled. In the case that a user is creating a CSV manually, this will not be the case. We can use the SubjectAccessReview API to check for the presence of permissions without caching the world, but since a PolicyRule has slices of verbs, resources, subjects, etc and the SAR endpoint accepts but one of each, there will be (in the general case) a combinatorical explosion of calls to issue enough SARs to cover the full set of permissions. Therefore, we need to limit the amount of times we take that action. A simple optimization is to check for permissions created directly by OLM, as that's by far the most common entrypoint into the system (a user creates a Subscription, that triggers an InstallPlan, which creates the RBAC). As OLM chose to name the RBAC objects with random strings of characters, it's not possible to look at a list of permissions in a CSV and know which resources OLM would have created. Therefore, this PR adds a label to all relevant RBAC resources with the hash of their content. We already have the name of the CSV, but since CSV content is ostensibly mutable, this is not enough. Signed-off-by: Steve Kuznetsov --- pkg/controller/operators/catalog/operator.go | 29 ++++++++-- pkg/controller/operators/labeller/filters.go | 16 ++++++ pkg/controller/operators/labeller/rbac.go | 56 ++++++++++++++++++++ pkg/controller/operators/olm/operator.go | 33 ++++++++++-- pkg/controller/registry/resolver/rbac.go | 54 +++++++++++++++++++ pkg/package-server/provider/registry_test.go | 3 +- 6 files changed, 183 insertions(+), 8 deletions(-) create mode 100644 pkg/controller/operators/labeller/rbac.go diff --git a/pkg/controller/operators/catalog/operator.go b/pkg/controller/operators/catalog/operator.go index 5b056e58ea..3a9e8d82e2 100644 --- a/pkg/controller/operators/catalog/operator.go +++ b/pkg/controller/operators/catalog/operator.go @@ -108,7 +108,6 @@ type Operator struct { client versioned.Interface dynamicClient dynamic.Interface lister operatorlister.OperatorLister - k8sLabelQueueSets map[schema.GroupVersionResource]workqueue.RateLimitingInterface catsrcQueueSet *queueinformer.ResourceQueueSet subQueueSet *queueinformer.ResourceQueueSet ipQueueSet *queueinformer.ResourceQueueSet @@ -202,7 +201,6 @@ func NewOperator(ctx context.Context, kubeconfigPath string, clock utilclock.Clo lister: lister, namespace: operatorNamespace, recorder: eventRecorder, - k8sLabelQueueSets: map[schema.GroupVersionResource]workqueue.RateLimitingInterface{}, catsrcQueueSet: queueinformer.NewEmptyResourceQueueSet(), subQueueSet: queueinformer.NewEmptyResourceQueueSet(), ipQueueSet: queueinformer.NewEmptyResourceQueueSet(), @@ -386,11 +384,12 @@ func NewOperator(ctx context.Context, kubeconfigPath string, clock utilclock.Clo if canFilter { return nil } - op.k8sLabelQueueSets[gvr] = workqueue.NewRateLimitingQueueWithConfig(workqueue.DefaultControllerRateLimiter(), workqueue.RateLimitingQueueConfig{ + queue := workqueue.NewRateLimitingQueueWithConfig(workqueue.DefaultControllerRateLimiter(), workqueue.RateLimitingQueueConfig{ Name: gvr.String(), }) queueInformer, err := queueinformer.NewQueueInformer( ctx, + queueinformer.WithQueue(queue), queueinformer.WithLogger(op.logger), queueinformer.WithInformer(informer), queueinformer.WithSyncer(sync.ToSyncer()), @@ -416,6 +415,18 @@ func NewOperator(ctx context.Context, kubeconfigPath string, clock utilclock.Clo )); err != nil { return nil, err } + if err := labelObjects(rolesgvk, roleInformer.Informer(), labeller.ContentHashLabeler[*rbacv1.Role, *rbacv1applyconfigurations.RoleApplyConfiguration]( + ctx, op.logger, labeller.ContentHashFilter, + func(role *rbacv1.Role) (string, error) { + return resolver.PolicyRuleHashLabelValue(role.Rules) + }, + rbacv1applyconfigurations.Role, + func(namespace string, ctx context.Context, cfg *rbacv1applyconfigurations.RoleApplyConfiguration, opts metav1.ApplyOptions) (*rbacv1.Role, error) { + return op.opClient.KubernetesInterface().RbacV1().Roles(namespace).Apply(ctx, cfg, opts) + }, + )); err != nil { + return nil, err + } // Wire RoleBindings roleBindingInformer := k8sInformerFactory.Rbac().V1().RoleBindings() @@ -432,6 +443,18 @@ func NewOperator(ctx context.Context, kubeconfigPath string, clock utilclock.Clo )); err != nil { return nil, err } + if err := labelObjects(rolebindingsgvk, roleBindingInformer.Informer(), labeller.ContentHashLabeler[*rbacv1.RoleBinding, *rbacv1applyconfigurations.RoleBindingApplyConfiguration]( + ctx, op.logger, labeller.ContentHashFilter, + func(roleBinding *rbacv1.RoleBinding) (string, error) { + return resolver.RoleReferenceAndSubjectHashLabelValue(roleBinding.RoleRef, roleBinding.Subjects) + }, + rbacv1applyconfigurations.RoleBinding, + func(namespace string, ctx context.Context, cfg *rbacv1applyconfigurations.RoleBindingApplyConfiguration, opts metav1.ApplyOptions) (*rbacv1.RoleBinding, error) { + return op.opClient.KubernetesInterface().RbacV1().RoleBindings(namespace).Apply(ctx, cfg, opts) + }, + )); err != nil { + return nil, err + } // Wire ServiceAccounts serviceAccountInformer := k8sInformerFactory.Core().V1().ServiceAccounts() diff --git a/pkg/controller/operators/labeller/filters.go b/pkg/controller/operators/labeller/filters.go index 18dc334b01..4bdbfece31 100644 --- a/pkg/controller/operators/labeller/filters.go +++ b/pkg/controller/operators/labeller/filters.go @@ -21,6 +21,10 @@ import ( "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/internal/alongside" ) +func ContentHashFilter(object metav1.Object) bool { + return HasOLMOwnerRef(object) && !hasHashLabel(object) +} + func Filter(gvr schema.GroupVersionResource) func(metav1.Object) bool { if f, ok := filters[gvr]; ok { return f @@ -80,6 +84,18 @@ func Validate(ctx context.Context, logger *logrus.Logger, metadataClient metadat allFilters[batchv1.SchemeGroupVersion.WithResource("jobs")] = JobFilter(func(namespace, name string) (metav1.Object, error) { return metadataClient.Resource(corev1.SchemeGroupVersion.WithResource("configmaps")).Namespace(namespace).Get(ctx, name, metav1.GetOptions{}) }) + + for _, gvr := range []schema.GroupVersionResource{ + rbacv1.SchemeGroupVersion.WithResource("roles"), + rbacv1.SchemeGroupVersion.WithResource("rolebindings"), + rbacv1.SchemeGroupVersion.WithResource("clusterroles"), + rbacv1.SchemeGroupVersion.WithResource("clusterrolebindings"), + } { + previous := allFilters[gvr] + allFilters[gvr] = func(object metav1.Object) bool { + return previous != nil && previous(object) && ContentHashFilter(object) + } + } for gvr, filter := range allFilters { gvr, filter := gvr, filter g.Go(func() error { diff --git a/pkg/controller/operators/labeller/rbac.go b/pkg/controller/operators/labeller/rbac.go new file mode 100644 index 0000000000..6e64791ca7 --- /dev/null +++ b/pkg/controller/operators/labeller/rbac.go @@ -0,0 +1,56 @@ +package labeller + +import ( + "context" + "fmt" + + "github.com/operator-framework/api/pkg/operators/v1alpha1" + "github.com/operator-framework/operator-lifecycle-manager/pkg/controller/registry/resolver" + "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/ownerutil" + "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/queueinformer" + "github.com/sirupsen/logrus" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func hasHashLabel(obj metav1.Object) bool { + _, ok := obj.GetLabels()[resolver.ContentHashLabelKey] + return ok +} + +func ContentHashLabeler[T metav1.Object, A ApplyConfig[A]]( + ctx context.Context, + logger *logrus.Logger, + check func(metav1.Object) bool, + hasher func(object T) (string, error), + applyConfigFor func(name, namespace string) A, + apply func(namespace string, ctx context.Context, cfg A, opts metav1.ApplyOptions) (T, error), +) queueinformer.LegacySyncHandler { + return func(obj interface{}) error { + cast, ok := obj.(T) + if !ok { + err := fmt.Errorf("wrong type %T, expected %T: %#v", obj, new(T), obj) + logger.WithError(err).Error("casting failed") + return fmt.Errorf("casting failed: %w", err) + } + + if _, _, ok := ownerutil.GetOwnerByKindLabel(cast, v1alpha1.ClusterServiceVersionKind); !ok { + return nil + } + + if !check(cast) || hasHashLabel(cast) { + return nil + } + + hash, err := hasher(cast) + if err != nil { + return fmt.Errorf("failed to calculate hash: %w", err) + } + + cfg := applyConfigFor(cast.GetName(), cast.GetNamespace()) + cfg.WithLabels(map[string]string{ + resolver.ContentHashLabelKey: hash, + }) + _, err = apply(cast.GetNamespace(), ctx, cfg, metav1.ApplyOptions{}) + return err + } +} diff --git a/pkg/controller/operators/olm/operator.go b/pkg/controller/operators/olm/operator.go index f7afa1ed6d..9855e8d106 100644 --- a/pkg/controller/operators/olm/operator.go +++ b/pkg/controller/operators/olm/operator.go @@ -78,7 +78,6 @@ type Operator struct { opClient operatorclient.ClientInterface client versioned.Interface lister operatorlister.OperatorLister - k8sLabelQueueSets map[schema.GroupVersionResource]workqueue.RateLimitingInterface protectedCopiedCSVNamespaces map[string]struct{} copiedCSVLister metadatalister.Lister ogQueueSet *queueinformer.ResourceQueueSet @@ -162,7 +161,6 @@ func newOperatorWithConfig(ctx context.Context, config *operatorConfig) (*Operat resolver: config.strategyResolver, apiReconciler: config.apiReconciler, lister: lister, - k8sLabelQueueSets: map[schema.GroupVersionResource]workqueue.RateLimitingInterface{}, recorder: eventRecorder, apiLabeler: config.apiLabeler, csvIndexers: map[string]cache.Indexer{}, @@ -453,11 +451,12 @@ func newOperatorWithConfig(ctx context.Context, config *operatorConfig) (*Operat if canFilter { return nil } - op.k8sLabelQueueSets[gvr] = workqueue.NewRateLimitingQueueWithConfig(workqueue.DefaultControllerRateLimiter(), workqueue.RateLimitingQueueConfig{ + queue := workqueue.NewRateLimitingQueueWithConfig(workqueue.DefaultControllerRateLimiter(), workqueue.RateLimitingQueueConfig{ Name: gvr.String(), }) queueInformer, err := queueinformer.NewQueueInformer( ctx, + queueinformer.WithQueue(queue), queueinformer.WithLogger(op.logger), queueinformer.WithInformer(informer), queueinformer.WithSyncer(sync.ToSyncer()), @@ -557,6 +556,20 @@ func newOperatorWithConfig(ctx context.Context, config *operatorConfig) (*Operat )); err != nil { return nil, err } + if err := labelObjects(clusterrolesgvk, clusterRoleInformer.Informer(), labeller.ContentHashLabeler[*rbacv1.ClusterRole, *rbacv1applyconfigurations.ClusterRoleApplyConfiguration]( + ctx, op.logger, labeller.ContentHashFilter, + func(clusterRole *rbacv1.ClusterRole) (string, error) { + return resolver.PolicyRuleHashLabelValue(clusterRole.Rules) + }, + func(name, _ string) *rbacv1applyconfigurations.ClusterRoleApplyConfiguration { + return rbacv1applyconfigurations.ClusterRole(name) + }, + func(_ string, ctx context.Context, cfg *rbacv1applyconfigurations.ClusterRoleApplyConfiguration, opts metav1.ApplyOptions) (*rbacv1.ClusterRole, error) { + return op.opClient.KubernetesInterface().RbacV1().ClusterRoles().Apply(ctx, cfg, opts) + }, + )); err != nil { + return nil, err + } clusterRoleBindingInformer := k8sInformerFactory.Rbac().V1().ClusterRoleBindings() informersByNamespace[metav1.NamespaceAll].ClusterRoleBindingInformer = clusterRoleBindingInformer @@ -586,6 +599,20 @@ func newOperatorWithConfig(ctx context.Context, config *operatorConfig) (*Operat )); err != nil { return nil, err } + if err := labelObjects(clusterrolebindingssgvk, clusterRoleBindingInformer.Informer(), labeller.ContentHashLabeler[*rbacv1.ClusterRoleBinding, *rbacv1applyconfigurations.ClusterRoleBindingApplyConfiguration]( + ctx, op.logger, labeller.ContentHashFilter, + func(clusterRoleBinding *rbacv1.ClusterRoleBinding) (string, error) { + return resolver.RoleReferenceAndSubjectHashLabelValue(clusterRoleBinding.RoleRef, clusterRoleBinding.Subjects) + }, + func(name, _ string) *rbacv1applyconfigurations.ClusterRoleBindingApplyConfiguration { + return rbacv1applyconfigurations.ClusterRoleBinding(name) + }, + func(_ string, ctx context.Context, cfg *rbacv1applyconfigurations.ClusterRoleBindingApplyConfiguration, opts metav1.ApplyOptions) (*rbacv1.ClusterRoleBinding, error) { + return op.opClient.KubernetesInterface().RbacV1().ClusterRoleBindings().Apply(ctx, cfg, opts) + }, + )); err != nil { + return nil, err + } // register namespace queueinformer namespaceInformer := k8sInformerFactory.Core().V1().Namespaces() diff --git a/pkg/controller/registry/resolver/rbac.go b/pkg/controller/registry/resolver/rbac.go index 45012df0e4..f0079e5016 100644 --- a/pkg/controller/registry/resolver/rbac.go +++ b/pkg/controller/registry/resolver/rbac.go @@ -1,8 +1,11 @@ package resolver import ( + "crypto/sha256" + "encoding/json" "fmt" "hash/fnv" + "math/big" corev1 "k8s.io/api/core/v1" rbacv1 "k8s.io/api/rbac/v1" @@ -62,6 +65,37 @@ func (o *OperatorPermissions) AddClusterRoleBinding(clusterRoleBinding *rbacv1.C o.ClusterRoleBindings = append(o.ClusterRoleBindings, clusterRoleBinding) } +const ContentHashLabelKey = "olm.permissions.hash" + +func PolicyRuleHashLabelValue(rules []rbacv1.PolicyRule) (string, error) { + raw, err := json.Marshal(rules) + if err != nil { + return "", err + } + return toBase62(sha256.Sum224(raw)), nil +} + +func RoleReferenceAndSubjectHashLabelValue(roleRef rbacv1.RoleRef, subjects []rbacv1.Subject) (string, error) { + var container = struct { + RoleRef rbacv1.RoleRef + Subjects []rbacv1.Subject + }{ + RoleRef: roleRef, + Subjects: subjects, + } + raw, err := json.Marshal(&container) + if err != nil { + return "", err + } + return toBase62(sha256.Sum224(raw)), nil +} + +func toBase62(hash [28]byte) string { + var i big.Int + i.SetBytes(hash[:]) + return i.Text(62) +} + func RBACForClusterServiceVersion(csv *v1alpha1.ClusterServiceVersion) (map[string]*OperatorPermissions, error) { permissions := map[string]*OperatorPermissions{} @@ -100,6 +134,11 @@ func RBACForClusterServiceVersion(csv *v1alpha1.ClusterServiceVersion) (map[stri }, Rules: permission.Rules, } + hash, err := PolicyRuleHashLabelValue(permission.Rules) + if err != nil { + return nil, fmt.Errorf("failed to hash permission rules: %w", err) + } + role.Labels[ContentHashLabelKey] = hash permissions[permission.ServiceAccountName].AddRole(role) // Create RoleBinding @@ -120,6 +159,11 @@ func RBACForClusterServiceVersion(csv *v1alpha1.ClusterServiceVersion) (map[stri Namespace: csv.GetNamespace(), }}, } + hash, err = RoleReferenceAndSubjectHashLabelValue(roleBinding.RoleRef, roleBinding.Subjects) + if err != nil { + return nil, fmt.Errorf("failed to hash binding content: %w", err) + } + roleBinding.Labels[ContentHashLabelKey] = hash permissions[permission.ServiceAccountName].AddRoleBinding(roleBinding) } @@ -142,6 +186,11 @@ func RBACForClusterServiceVersion(csv *v1alpha1.ClusterServiceVersion) (map[stri }, Rules: permission.Rules, } + hash, err := PolicyRuleHashLabelValue(permission.Rules) + if err != nil { + return nil, fmt.Errorf("failed to hash permission rules: %w", err) + } + role.Labels[ContentHashLabelKey] = hash permissions[permission.ServiceAccountName].AddClusterRole(role) // Create ClusterRoleBinding @@ -162,6 +211,11 @@ func RBACForClusterServiceVersion(csv *v1alpha1.ClusterServiceVersion) (map[stri Namespace: csv.GetNamespace(), }}, } + hash, err = RoleReferenceAndSubjectHashLabelValue(roleBinding.RoleRef, roleBinding.Subjects) + if err != nil { + return nil, fmt.Errorf("failed to hash binding content: %w", err) + } + roleBinding.Labels[ContentHashLabelKey] = hash permissions[permission.ServiceAccountName].AddClusterRoleBinding(roleBinding) } return permissions, nil diff --git a/pkg/package-server/provider/registry_test.go b/pkg/package-server/provider/registry_test.go index 2f0b5277f1..beeb82a747 100644 --- a/pkg/package-server/provider/registry_test.go +++ b/pkg/package-server/provider/registry_test.go @@ -787,7 +787,7 @@ func TestRegistryProviderList(t *testing.T) { globalNS: "ns", requestNamespace: "wisconsin", expectedErr: "", - expected: &operators.PackageManifestList{Items: []operators.PackageManifest{}}, + expected: &operators.PackageManifestList{}, }, { name: "PackagesFound", @@ -1230,7 +1230,6 @@ func TestRegistryProviderList(t *testing.T) { } else { require.Nil(t, err) } - require.Equal(t, len(test.expected.Items), len(packageManifestList.Items)) require.ElementsMatch(t, test.expected.Items, packageManifestList.Items) })