fix: simplify checker for access to namespaces

Author: erikgb

cmd/gitops-server/cmd/cmd.go

@@ -244,7 +244,7 @@ func runCmd(cmd *cobra.Command, args []string) error {
 
 	fetcher := fetcher.NewSingleClusterFetcher(cl)
 
-	clustersManager := clustersmngr.NewClustersManager([]clustersmngr.ClusterFetcher{fetcher}, nsaccess.NewChecker(nsaccess.DefautltWegoAppRules), log)
+	clustersManager := clustersmngr.NewClustersManager([]clustersmngr.ClusterFetcher{fetcher}, nsaccess.NewChecker(), log)
 	clustersManager.Start(ctx)
 
 	healthChecker := health.NewHealthChecker()

core/clustersmngr/factory_test.go

@@ -268,7 +268,7 @@ func TestUpdateUserNamespacesFailsToConnect(t *testing.T) {
 	ctx, cancel := context.WithCancel(context.Background())
 	defer cancel()
 
-	nsChecker := nsaccess.NewChecker(nil)
+	nsChecker := nsaccess.NewChecker()
 	clustersFetcher := new(clustersmngrfakes.FakeClusterFetcher)
 
 	clustersManager := clustersmngr.NewClustersManager([]clustersmngr.ClusterFetcher{clustersFetcher}, nsChecker, logger)
@@ -302,7 +302,7 @@ func TestGetClusters(t *testing.T) {
 	ctx, cancel := context.WithCancel(context.Background())
 	defer cancel()
 
-	nsChecker := nsaccess.NewChecker(nil)
+	nsChecker := nsaccess.NewChecker()
 	clustersFetcher := new(clustersmngrfakes.FakeClusterFetcher)
 
 	clustersManager := clustersmngr.NewClustersManager([]clustersmngr.ClusterFetcher{clustersFetcher}, nsChecker, logger)

core/nsaccess/nsaccess.go

@@ -6,48 +6,12 @@ import (
 
 	authorizationv1 "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"
-	"k8s.io/apimachinery/pkg/util/sets"
 	typedauth "k8s.io/client-go/kubernetes/typed/authorization/v1"
 )
 
 //go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 -generate
 
-// DefautltWegoAppRules is the minimun set of permissions a user will need to use the wego-app in a given namespace
-var DefautltWegoAppRules = []rbacv1.PolicyRule{
-	{
-		APIGroups: []string{""},
-		Resources: []string{"pods", "secrets"},
-		Verbs:     []string{"get", "list"},
-	},
-	{
-		APIGroups: []string{""},
-		Resources: []string{"events"},
-		Verbs:     []string{"get", "list", "watch"},
-	},
-	{
-		APIGroups: []string{"apps"},
-		Resources: []string{"deployments", "replicasets"},
-		Verbs:     []string{"get", "list"},
-	},
-	{
-		APIGroups: []string{"kustomize.toolkit.fluxcd.io"},
-		Resources: []string{"kustomizations"},
-		Verbs:     []string{"get", "list"},
-	},
-	{
-		APIGroups: []string{"helm.toolkit.fluxcd.io"},
-		Resources: []string{"helmreleases"},
-		Verbs:     []string{"get", "list"},
-	},
-	{
-		APIGroups: []string{"source.toolkit.fluxcd.io"},
-		Resources: []string{"buckets", "helmcharts", "helmrepositories", "gitrepositories", "ocirepositories"},
-		Verbs:     []string{"get", "list"},
-	},
-}
-
 // Checker contains methods for validing user access to Kubernetes namespaces, based on a set of PolicyRules
 //
 //counterfeiter:generate . Checker
@@ -56,19 +20,25 @@ type Checker interface {
 	FilterAccessibleNamespaces(ctx context.Context, auth typedauth.AuthorizationV1Interface, namespaces []corev1.Namespace) ([]corev1.Namespace, error)
 }
 
-type simpleChecker struct {
-	rules []rbacv1.PolicyRule
-}
+type simpleChecker struct{}
 
-func NewChecker(rules []rbacv1.PolicyRule) Checker {
-	return simpleChecker{rules: rules}
+func NewChecker() Checker {
+	return simpleChecker{}
 }
 
 func (sc simpleChecker) FilterAccessibleNamespaces(ctx context.Context, auth typedauth.AuthorizationV1Interface, namespaces []corev1.Namespace) ([]corev1.Namespace, error) {
-	result := []corev1.Namespace{}
+	accessToAllNamespace, err := hasAccessToAllNamespaces(ctx, auth)
+	if err != nil {
+		return nil, err
+	}
+
+	if accessToAllNamespace {
+		return namespaces, nil
+	}
 
+	var result []corev1.Namespace
 	for _, ns := range namespaces {
-		ok, err := userCanUseNamespace(ctx, auth, ns, sc.rules)
+		ok, err := hasAccessToNamespace(ctx, auth, ns)
 		if err != nil {
 			return nil, fmt.Errorf("user namespace access: %w", err)
 		}
@@ -81,163 +51,35 @@ func (sc simpleChecker) FilterAccessibleNamespaces(ctx context.Context, auth typ
 	return result, nil
 }
 
-func userCanUseNamespace(ctx context.Context, auth typedauth.AuthorizationV1Interface, ns corev1.Namespace, rules []rbacv1.PolicyRule) (bool, error) {
-	sar := &authorizationv1.SelfSubjectRulesReview{
-		Spec: authorizationv1.SelfSubjectRulesReviewSpec{
-			Namespace: ns.Name,
+func hasAccessToNamespace(ctx context.Context, auth typedauth.AuthorizationV1Interface, ns corev1.Namespace) (bool, error) {
+	ssar := &authorizationv1.SelfSubjectAccessReview{
+		Spec: authorizationv1.SelfSubjectAccessReviewSpec{
+			ResourceAttributes: &authorizationv1.ResourceAttributes{
+				Verb:      "get",
+				Resource:  "configmaps",
+				Namespace: ns.Name,
+			},
 		},
 	}
-
-	authRes, err := auth.SelfSubjectRulesReviews().Create(ctx, sar, metav1.CreateOptions{})
+	res, err := auth.SelfSubjectAccessReviews().Create(ctx, ssar, metav1.CreateOptions{})
 	if err != nil {
 		return false, err
 	}
-
-	return hasAllRules(authRes.Status, rules), nil
+	return res.Status.Allowed, nil
 }
 
-var allK8sVerbs = []string{"create", "get", "list", "watch", "patch", "delete", "deletecollection"}
-
-func allResources(rules []rbacv1.PolicyRule) []string {
-	resources := sets.NewString()
-	for _, r := range rules {
-		resources.Insert(r.Resources...)
-	}
-
-	return resources.List()
-}
-
-func allAPIGroups(rules []rbacv1.PolicyRule) []string {
-	apiGroups := sets.NewString()
-	for _, r := range rules {
-		apiGroups.Insert(r.APIGroups...)
-	}
-
-	return apiGroups.List()
-}
-
-// hasAll rules determines if a set of SubjectRulesReview rules match a minimum set of policy rules
-//
-// We need to understand the "sum" of all the rules for a role.
-// Convert to a hash lookup to make it easier to tell what a user can do.
-// Looks like { "apps": { "deployments": { get: true, list: true } } }
-//
-// We handle wildcards by expanding them out to all the types of resources/APIGroups
-// that are relevant to the provided rules.
-// This creates slightly nonsensical sets sometimes, for example given:
-//
-// PolicyRules:
-//
-//	{
-//		APIGroups: []string{""},
-//		Resources: []string{"secrets"},
-//		Verbs:     []string{"get"},
-//	},
-//
-//	{
-//		APIGroups: []string{"apps"},
-//		Resources: []string{"deployment"}",
-//		Verbs:     []string{"get"},
-//	},
-//
-// SubjectRulesReviewStatus:
-// ["*", "*", "get"]
-//
-// We get:
-//
-//	{
-//		"": {
-//			"secrets": { get: true }
-//			"deployments": { get: true }
-//		},
-//		"apps": {
-//			"secrets": { get: true }
-//			"deployments", "secrets": { get: true }
-//		}
-//	}
-//
-// Secrets don't exist in "apps" according to k8s,
-// but the "index checker" that checks this struct does not mind.
-func hasAllRules(status authorizationv1.SubjectRulesReviewStatus, rules []rbacv1.PolicyRule) bool {
-	derivedAccess := map[string]map[string]map[string]bool{}
-
-	allResourcesInRules := allResources(rules)
-	allAPIGroupsInRules := allAPIGroups(rules)
-
-	for _, statusRule := range status.ResourceRules {
-		apiGroups := statusRule.APIGroups
-		if containsWildcard(apiGroups) {
-			apiGroups = allAPIGroupsInRules
-		}
-
-		for _, apiGroup := range apiGroups {
-			if _, ok := derivedAccess[apiGroup]; !ok {
-				derivedAccess[apiGroup] = map[string]map[string]bool{}
-			}
-
-			resources := statusRule.Resources
-			if containsWildcard(resources) {
-				resources = allResourcesInRules
-			}
-
-			for _, resource := range resources {
-				if _, ok := derivedAccess[apiGroup][resource]; !ok {
-					derivedAccess[apiGroup][resource] = map[string]bool{}
-				}
-
-				verbs := statusRule.Verbs
-				if containsWildcard(verbs) {
-					verbs = allK8sVerbs
-				}
-
-				for _, v := range verbs {
-					derivedAccess[apiGroup][resource][v] = true
-				}
-			}
-		}
-	}
-
-	hasAccess := true
-
-Rules:
-	for _, rule := range rules {
-		for _, apiGroup := range rule.APIGroups {
-			g, ok := derivedAccess[apiGroup]
-
-			if !ok {
-				hasAccess = false
-				break Rules
-			}
-			for _, resource := range rule.Resources {
-				r, ok := g[resource]
-				if !ok {
-					// A resource is not present for this apiGroup.
-					hasAccess = false
-					break Rules
-				}
-
-				for _, verb := range rule.Verbs {
-					_, ok := r[verb]
-					if !ok {
-						// A verb is not present for this resource,
-						// no need to check the rest of the verbs.
-						hasAccess = false
-						break Rules
-					}
-				}
-			}
-		}
+func hasAccessToAllNamespaces(ctx context.Context, auth typedauth.AuthorizationV1Interface) (bool, error) {
+	ssar := &authorizationv1.SelfSubjectAccessReview{
+		Spec: authorizationv1.SelfSubjectAccessReviewSpec{
+			ResourceAttributes: &authorizationv1.ResourceAttributes{
+				Verb:     "list",
+				Resource: "namespaces",
+			},
+		},
 	}
-
-	return hasAccess
-}
-
-func containsWildcard(permissions []string) bool {
-	for _, p := range permissions {
-		if p == "*" {
-			return true
-		}
+	res, err := auth.SelfSubjectAccessReviews().Create(ctx, ssar, metav1.CreateOptions{})
+	if err != nil {
+		return false, err
 	}
-
-	return false
+	return res.Status.Allowed, nil
 }