From 87ae6beab20df398b66726406bf7002c5785721a Mon Sep 17 00:00:00 2001 From: Stanislav Laznicka Date: Wed, 20 Apr 2022 14:51:29 +0200 Subject: [PATCH] address fixmes and todos; add godocs --- .../podsecurity_label_sync_controller.go | 25 ++++- pkg/psalabelsyncer/sccrolecache.go | 105 ++++++++++++------ pkg/psalabelsyncer/scctopsamapping.go | 44 ++++---- 3 files changed, 114 insertions(+), 60 deletions(-) diff --git a/pkg/psalabelsyncer/podsecurity_label_sync_controller.go b/pkg/psalabelsyncer/podsecurity_label_sync_controller.go index 06a69404d..86b6da561 100644 --- a/pkg/psalabelsyncer/podsecurity_label_sync_controller.go +++ b/pkg/psalabelsyncer/podsecurity_label_sync_controller.go @@ -33,6 +33,10 @@ const ( labelSyncControlLabel = "security.openshift.io/scc.podSecurityLabelSync" ) +// PodSecurityAdmissionLabelSynchronizationController watches over namespaces labelled with +// "security.openshift.io/scc.podSecurityLabelSync: true" and configures the PodSecurity +// admission namespace label to match the user account privileges in terms of being able +// to use SCCs type PodSecurityAdmissionLabelSynchronizationController struct { namespaceClient corev1client.NamespaceInterface @@ -109,7 +113,7 @@ func NewPodSecurityAdmissionLabelSynchronizationController( rbacInformers.ClusterRoles().Informer(), ). WithFilteredEventsInformersQueueKeysFunc( - nsToQueueKey, + nameToKey, func(obj interface{}) bool { ns, ok := obj.(*corev1.Namespace) if !ok { @@ -118,7 +122,6 @@ func NewPodSecurityAdmissionLabelSynchronizationController( return false } // the SCC mapping requires the annotation - // FIXME: make the mapping not panic but error out instead if ns.Annotations == nil || len(ns.Annotations[securityv1.UIDRangeAnnotation]) == 0 { return false } @@ -158,14 +161,14 @@ func (c *PodSecurityAdmissionLabelSynchronizationController) sync(ctx context.Co return fmt.Errorf(errFmt, qKey, err) } - if err := c.syncNamespace(ctx, ns); err != nil { + if err := c.syncNamespace(ctx, controllerContext, ns); err != nil { return fmt.Errorf(errFmt, qKey, err) } return nil } -func (c *PodSecurityAdmissionLabelSynchronizationController) syncNamespace(ctx context.Context, ns *corev1.Namespace) error { +func (c *PodSecurityAdmissionLabelSynchronizationController) syncNamespace(ctx context.Context, controllerContext factory.SyncContext, ns *corev1.Namespace) error { // We cannot safely determine the SCC level for an NS until it gets the UID annotation. // No need to care about re-queueing the key, we should get the NS once it is updated // with the annotation. @@ -194,7 +197,10 @@ func (c *PodSecurityAdmissionLabelSynchronizationController) syncNamespace(ctx c // TODO: the SCC was removed in the meantime and synced in the cache? return err } - sccPSaLevel := convertSCCToPSALevel(ns, scc) + sccPSaLevel, err := convertSCCToPSALevel(ns, scc) + if err != nil { + return err + } if sccPSaLevel > currentNSLevel { currentNSLevel = sccPSaLevel @@ -226,7 +232,8 @@ func (c *PodSecurityAdmissionLabelSynchronizationController) syncNamespace(ctx c return nil } -func nsToQueueKey(obj runtime.Object) []string { +// nameToKey turns a meta object into a key by using the object's name. +func nameToKey(obj runtime.Object) []string { metaObj, ok := obj.(metav1.ObjectMetaAccessor) if !ok { return factory.DefaultQueueKeysFunc(obj) @@ -234,6 +241,9 @@ func nsToQueueKey(obj runtime.Object) []string { return []string{metaObj.GetObjectMeta().GetName()} } +// queueKeysForObj returns slice with: +// - a namespace name for a namespaced object +// - all cluster namespaces names for cluster-wide objects func (c *PodSecurityAdmissionLabelSynchronizationController) queueKeysForObj(obj runtime.Object) []string { metaObj, ok := obj.(metav1.ObjectMetaAccessor) if !ok { @@ -248,6 +258,7 @@ func (c *PodSecurityAdmissionLabelSynchronizationController) queueKeysForObj(obj return c.allWatchedNamespacesAsQueueKeys(obj) } +// allWatchedNamespacesAsQueueKeys returns all namespace names slice irregardles of the retrieved object. func (c *PodSecurityAdmissionLabelSynchronizationController) allWatchedNamespacesAsQueueKeys(_ runtime.Object) []string { namespaces, err := c.namespaceLister.List(c.nsLabelSelector) if err != nil && !apierrors.IsNotFound(err) { @@ -262,6 +273,8 @@ func (c *PodSecurityAdmissionLabelSynchronizationController) allWatchedNamespace return qKeys } +// controlledNamespacesLabelSelector returns label selector to be used with the +// PodSecurityAdmissionLabelSynchronizationController. func controlledNamespacesLabelSelector() (labels.Selector, error) { labelRequirement, err := labels.NewRequirement(labelSyncControlLabel, selection.NotEquals, []string{"false"}) if err != nil { diff --git a/pkg/psalabelsyncer/sccrolecache.go b/pkg/psalabelsyncer/sccrolecache.go index 8da631e8d..4b864739b 100644 --- a/pkg/psalabelsyncer/sccrolecache.go +++ b/pkg/psalabelsyncer/sccrolecache.go @@ -7,6 +7,7 @@ import ( corev1 "k8s.io/api/core/v1" rbacv1 "k8s.io/api/rbac/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apiserver/pkg/authentication/serviceaccount" @@ -15,6 +16,7 @@ import ( rbacv1informers "k8s.io/client-go/informers/rbac/v1" rbacv1listers "k8s.io/client-go/listers/rbac/v1" "k8s.io/client-go/tools/cache" + "k8s.io/client-go/util/retry" "k8s.io/klog/v2" "k8s.io/kubernetes/plugin/pkg/auth/authorizer/rbac" @@ -23,8 +25,12 @@ import ( securityv1listers "github.com/openshift/client-go/security/listers/security/v1" ) +// The index name to be used along with the BySAIndexKeys indexing function const BySAIndexName = "ByServiceAccount" +// SAToSCCCache is a construct that caches roles and rolebindings +// (and their cluster variants) and based on that and on SCCs present in the cluster +// it allows retrieving a set of SCCs for a given ServiceAccount type SAToSCCCache struct { roleLister rbacv1listers.RoleLister clusterRoleLister rbacv1listers.ClusterRoleLister @@ -75,18 +81,21 @@ func (r *roleBindingObj) Subjects() []rbacv1.Subject { return r.roleBinding.Subjects } -func (r *roleBindingObj) AppliesToNS(ns string) bool { +func (r *roleBindingObj) Namespace() string { if r.clusterRoleBinding != nil { - return true + return "" } - return ns == r.roleBinding.Namespace + return r.roleBinding.Namespace } -func (r *roleBindingObj) Namespace() string { +// AppliesToNS returns true if: +// - the wrapped object is a cluster role binding +// - the namespace of a wrapped rolebinding matches the namespace supplied in `ns` +func (r *roleBindingObj) AppliesToNS(ns string) bool { if r.clusterRoleBinding != nil { - return "" + return true } - return r.roleBinding.Namespace + return ns == r.roleBinding.Namespace } // roleObj helps to handle roles and clusterroles in a generic manner @@ -133,6 +142,11 @@ func (r *roleObj) Namespace() string { return r.role.Namespace } +// BySAIndexKeys is a cache.IndexFunc indexing function that shall be used on +// rolebinding and clusterrolebinding informer caches. +// It retrieves the subjects of the incoming object and if there are SA, SA groups +// or the system:authenticated group subjects, these will all be returned as a slice +// of strings to create an index for the SA or SA group. func BySAIndexKeys(obj interface{}) ([]string, error) { roleBinding, err := newRoleBindingObj(obj) if err != nil { @@ -163,17 +177,13 @@ func NewSAToSCCCache(rbacInformers rbacv1informers.Interface, sccInfomer securit sccLister: sccInfomer.Lister(), - // TODO: do I need these? - rolesSynced: rbacInformers.Roles().Informer().HasSynced, - roleBindingsSynced: rbacInformers.RoleBindings().Informer().HasSynced, - usefulRoles: make(map[string][]string), } } // SCCsFor returns a slice of all the SCCs that a given service account can use -// to run pods in its namespace -// It expects the serviceAccount name in the system:serviceaccount:: form +// to run pods in its namespace. +// It expects the serviceAccount name in the system:serviceaccount:: form. func (c *SAToSCCCache) SCCsFor(serviceAccount *corev1.ServiceAccount) (sets.String, error) { saUserInfo := serviceaccount.UserInfo( serviceAccount.Namespace, @@ -212,7 +222,6 @@ func (c *SAToSCCCache) SCCsFor(serviceAccount *corev1.ServiceAccount) (sets.Stri } } - // TODO: (idea): determine, ahead of time, which SCCs are allowed for all authenticated or for all SAs? for _, o := range objs { rb, err := newRoleBindingObj(o) if err != nil { @@ -242,24 +251,39 @@ func (c *SAToSCCCache) SCCsFor(serviceAccount *corev1.ServiceAccount) (sets.Stri return allowedSCCs, nil } -func (c *SAToSCCCache) GetRoleFromRoleRef(ns string, roleRef rbacv1.RoleRef) (*roleObj, error) { +// getRoleFromRoleRef tries to retrieve the role or clusterrole from roleRef. +func (c *SAToSCCCache) getRoleFromRoleRef(ns string, roleRef rbacv1.RoleRef) (*roleObj, error) { var role interface{} - var err error + var retryErr error switch kind := roleRef.Kind; kind { - case "Role": // FIXME: add retries (retry.WithBackoff) on NotFound - role, err = c.roleLister.Roles(ns).Get(roleRef.Name) + case "Role": + retryErr = retry.OnError(retry.DefaultBackoff, apierrors.IsNotFound, func() error { + var err error + role, err = c.roleLister.Roles(ns).Get(roleRef.Name) + return err + }) + case "ClusterRole": - role, err = c.clusterRoleLister.Get(roleRef.Name) + retryErr = retry.OnError(retry.DefaultBackoff, apierrors.IsNotFound, func() error { + var err error + role, err = c.clusterRoleLister.Get(roleRef.Name) + return err + }) + default: return nil, fmt.Errorf("unknown kind in roleRef: %s", kind) } - if err != nil { - return nil, err + + if retryErr != nil { + return nil, retryErr } return newRoleObj(role) } +// IsRoleBindingRelevant returns true if the cluster/rolebinding supplied binds +// to a Role that provides access to at least one of the SCCs available in the +// cluster. func (c *SAToSCCCache) IsRoleBindingRelevant(obj interface{}) bool { rb, err := newRoleBindingObj(obj) if err != nil { @@ -267,19 +291,23 @@ func (c *SAToSCCCache) IsRoleBindingRelevant(obj interface{}) bool { return false } - role, err := c.GetRoleFromRoleRef(rb.Namespace(), rb.RoleRef()) + role, err := c.getRoleFromRoleRef(rb.Namespace(), rb.RoleRef()) if err != nil { klog.Infof("failed to retrieve a role for a rolebinding ref: %v", err) return false } - // TODO: actually cache the relevant rolebindings and relevant roles - // or maybe only the roles and update cached roles on a role update? return c.IsRoleInvolvesSCCs(role, false) } +// IsRoleInvolvesSCCs returns true if the role supplied in obj provides access +// to at least one of the SCCs present in the cluster. +// Set isRoleUpdate to true if instead of using the cached role the supplied object +// should be used to update the role in the cache as well. func (c *SAToSCCCache) IsRoleInvolvesSCCs(obj interface{}, isRoleUpdate bool) bool { - c.usefulRolesLock.Lock() // TODO: comment this stuff + // lock the roles cache to make sure the state of the role that's later retrieved here + // is written properly + c.usefulRolesLock.Lock() defer c.usefulRolesLock.Unlock() role, err := newRoleObj(obj) @@ -288,23 +316,31 @@ func (c *SAToSCCCache) IsRoleInvolvesSCCs(obj interface{}, isRoleUpdate bool) bo return false } - sccs, err := c.sccLister.List(labels.Everything()) // TODO: this should probably requeue, right? - if err != nil { - klog.Warning("failed to list SCCs: %v", err) - return false - } - if isRoleUpdate { + sccs, err := c.sccLister.List(labels.Everything()) + if err != nil { + klog.Warning("failed to list SCCs: %v", err) + return false + } + c.syncRoleCache(role.Namespace(), role.Name(), role.Rules(), sccs) } return len(c.usefulRoles[fmt.Sprintf("%s/%s", role.Namespace(), role.Name())]) != 0 } +// ReinitializeRoleCache clears the current cache of roles that provide access +// to SCCs and reinitializes it by pulling all cluster-/roles and SCCs and +// reevaluating them. +// This should be used rather rarely as there are many computations involved +// when assessing all the present cluster-/role rules. func (c *SAToSCCCache) ReinitializeRoleCache() error { - c.usefulRolesLock.Lock() // TODO: comment this stuff + // rewriting the whole cache, lock is needed + c.usefulRolesLock.Lock() defer c.usefulRolesLock.Unlock() + c.usefulRoles = map[string][]string{} + roles, err := c.roleLister.List(labels.Everything()) if err != nil { return fmt.Errorf("failed to initialize role cache: %w", err) @@ -331,6 +367,9 @@ func (c *SAToSCCCache) ReinitializeRoleCache() error { return nil } +// syncRoleCache will write the current mapping of "role->SCCs it allows" to the cache. +// It expects the c.usefulRolesLock to be already locked as even the wrapping context +// handling roles and SCCs likely requires synchronization. func (c *SAToSCCCache) syncRoleCache(roleNS, roleName string, rules []rbacv1.PolicyRule, sccs []*securityv1.SecurityContextConstraints) { if c.usefulRolesLock.TryLock() { c.usefulRolesLock.Unlock() @@ -340,12 +379,12 @@ func (c *SAToSCCCache) syncRoleCache(roleNS, roleName string, rules []rbacv1.Pol dummyUserInfo := &user.DefaultInfo{ Name: "dummyUser", } - if allowedSCCs := SCCsAllowedByPolicyRules("", dummyUserInfo, sccs, rules); len(allowedSCCs) > 0 { + if allowedSCCs := sccsAllowedByPolicyRules("", dummyUserInfo, sccs, rules); len(allowedSCCs) > 0 { c.usefulRoles[fmt.Sprintf("%s/%s", roleNS, roleName)] = allowedSCCs } } -func SCCsAllowedByPolicyRules(nsName string, saUserInfo user.Info, sccs []*securityv1.SecurityContextConstraints, rules []rbacv1.PolicyRule) []string { +func sccsAllowedByPolicyRules(nsName string, saUserInfo user.Info, sccs []*securityv1.SecurityContextConstraints, rules []rbacv1.PolicyRule) []string { ar := authorizer.AttributesRecord{ User: saUserInfo, APIGroup: securityv1.GroupName, diff --git a/pkg/psalabelsyncer/scctopsamapping.go b/pkg/psalabelsyncer/scctopsamapping.go index f8b717d90..7b304e13b 100644 --- a/pkg/psalabelsyncer/scctopsamapping.go +++ b/pkg/psalabelsyncer/scctopsamapping.go @@ -12,7 +12,8 @@ import ( ) const ( - restricted uint8 = iota + unknown uint8 = iota + restricted baseline privileged ) @@ -30,7 +31,7 @@ func internalRestrictivnessToPSaLevel(restr uint8) psapi.Level { } } -func convertSCCToPSALevel(namespace *corev1.Namespace, scc *securityv1.SecurityContextConstraints) uint8 { +func convertSCCToPSALevel(namespace *corev1.Namespace, scc *securityv1.SecurityContextConstraints) (uint8, error) { sccRestrictivness := make([]uint8, 0, 10) sccRestrictivness = append(sccRestrictivness, convert_hostDir(scc.AllowHostDirVolumePlugin, scc.Volumes), @@ -40,11 +41,16 @@ func convertSCCToPSALevel(namespace *corev1.Namespace, scc *securityv1.SecurityC convert_allowPrivilegedContainer(scc.AllowPrivilegedContainer), convert_allowedCapabilities(scc.AllowedCapabilities, scc.RequiredDropCapabilities), convert_unsafeSysctls(scc.AllowedUnsafeSysctls), - convert_runAsUserOrDie(namespace, &scc.RunAsUser), convert_volumes(scc.Volumes), convert_seLinuxOptions(&scc.SELinuxContext), ) + if restrictivness, err := convert_runAsUserOrDie(namespace, &scc.RunAsUser); err != nil { + return privileged, fmt.Errorf("failed to convert SCC %q in namespace %q: %w", scc.Name, namespace.Name, err) + } else { + sccRestrictivness = append(sccRestrictivness, restrictivness) + } + // scc.ForbiddenSysctls <-- only restricts the current allowed set, unused for conversion // scc.AllowedFlexVolumes <-- only restricts the current allowed set, unused for conversion @@ -59,14 +65,14 @@ func convertSCCToPSALevel(namespace *corev1.Namespace, scc *securityv1.SecurityC var restrictiveness = restricted for _, r := range sccRestrictivness { if r == privileged { - return privileged + return privileged, nil } if r > restrictiveness { restrictiveness = r } } - return restrictiveness + return restrictiveness, nil } func convert_hostDir(allowHostDirVolumePlugin bool, volumes []securityv1.FSType) uint8 { @@ -129,9 +135,6 @@ func convert_allowPrivilegedContainer(allowPrivilegedContainer bool) uint8 { return restricted } -// FIXME: -// this single conversion will make all SCCs be evaluated as `baseline` at best -// since we don't currently have a way to require dropping ALL caps in OCP func convert_allowedCapabilities(sccAllowedCapabilities, requiredDropCapabilities []corev1.Capability) uint8 { // upstream: check_capabilities_{baseline,restricted} // baseline allows: `baseline_capabilities_allowed_1_0` @@ -204,52 +207,51 @@ func convert_unsafeSysctls(allowedUnsafeSysctls []string) uint8 { return restricted } -// INTERESTING: this function makes the whole conversion NS-dependant func convert_runAsUserOrDie( namespace *corev1.Namespace, runAsUser *securityv1.RunAsUserStrategyOptions, -) uint8 { +) (uint8, error) { // upstream: check_runAsUser // restricted requires: non-zero, undefined switch runAsUser.Type { case securityv1.RunAsUserStrategyMustRunAsNonRoot: - return restricted + return restricted, nil case securityv1.RunAsUserStrategyMustRunAs: // RunAsUserStrategyMustRunAs requires the UID to be set if runAsUser.UID != nil && *runAsUser.UID > 0 { - return restricted + return restricted, nil } - return baseline + return baseline, nil case securityv1.RunAsUserStrategyMustRunAsRange: if runAsUser.UIDRangeMin == nil || runAsUser.UIDRangeMax == nil { annotationVal, ok := namespace.Annotations[securityv1.UIDRangeAnnotation] if !ok || len(annotationVal) == 0 { - panic(fmt.Errorf("the namespace %q has no valid %q label or the label's missing value, even though the SCC requires it", namespace, securityv1.UIDRangeAnnotation)) + return unknown, fmt.Errorf("the namespace %q has no valid %q label or the label's missing value, even though the SCC requires it", namespace, securityv1.UIDRangeAnnotation) } uidBlock, err := uid.ParseBlock(annotationVal) if err != nil { - panic(fmt.Errorf("failed to parse uid block for the %q namespace: %v", namespace, err)) + return unknown, fmt.Errorf("failed to parse uid block for the %q namespace: %v", namespace, err) } if uidBlock.Start > 0 { // we only care about the beginning of the block, no need to check for valid blocks here - return restricted + return restricted, nil } - return baseline + return baseline, nil } // we only care about the beginning of the block, no need to check for valid blocks here if *runAsUser.UIDRangeMin > 0 { - return restricted + return restricted, nil } - return baseline + return baseline, nil case securityv1.RunAsUserStrategyRunAsAny: - return baseline + return baseline, nil default: - panic(fmt.Errorf("unknown strategy: %s", runAsUser.Type)) + return unknown, fmt.Errorf("unknown strategy: %s", runAsUser.Type) } }