Merge pull request operator-framework#514 from njhale/non-resource-perms

fix(requirements): add support for non resource url rules

deploy/chart/templates/0000_30_02-clusterserviceversion.crd.yaml

@@ -653,8 +653,6 @@ spec:
                                         - create
                                         - update
                                         - patch
-                                        - put
-                                        - post
                                         - delete
                                         - deletecollection
                                         - initialize
@@ -674,6 +672,8 @@ spec:
                               type: array
                               items:
                                 type: object
+                                required:
+                                - verbs
                                 description: a rule required by the service account
                                 properties:
                                   apiGroups:
@@ -689,6 +689,10 @@ spec:
                                     type: array
                                     items:
                                       type: string
+                                  nonResourceURLs:
+                                    type: array
+                                    items:
+                                      type: string
                                   verbs:
                                     type: array
                                     items:

pkg/controller/install/attributes_util.go

@@ -9,10 +9,12 @@ import (
 	"k8s.io/apiserver/pkg/authorization/authorizer"
 )
 
+// toAttributesSet converts the given user, namespace, and PolicyRule into a set of Attributes expected. This is useful for checking
+// if a composed set of Roles/RoleBindings satisfies a PolicyRule.
 func toAttributesSet(user user.Info, namespace string, rule rbacv1.PolicyRule) []authorizer.Attributes {
 	set := map[authorizer.AttributesRecord]struct{}{}
 
-	// add empty string for empty groups, resources, and resource names
+	// add empty string for empty groups, resources, resource names, and non resource urls
 	groups := rule.APIGroups
 	if len(groups) == 0 {
 		groups = make([]string, 1)
@@ -25,12 +27,18 @@ func toAttributesSet(user user.Info, namespace string, rule rbacv1.PolicyRule) [
 	if len(names) == 0 {
 		names = make([]string, 1)
 	}
+	nonResourceURLs := rule.NonResourceURLs
+	if len(nonResourceURLs) == 0 {
+		nonResourceURLs = make([]string, 1)
+	}
 
 	for _, verb := range rule.Verbs {
 		for _, group := range groups {
 			for _, resource := range resources {
 				for _, name := range names {
-					set[attributesRecord(user, namespace, verb, group, resource, name)] = struct{}{}
+					for _, nonResourceURL := range nonResourceURLs {
+						set[attributesRecord(user, namespace, verb, group, resource, name, nonResourceURL)] = struct{}{}
+					}
 				}
 			}
 		}
@@ -48,15 +56,17 @@ func toAttributesSet(user user.Info, namespace string, rule rbacv1.PolicyRule) [
 }
 
 // attribute creates a new AttributesRecord with the given info. Currently RBAC authz only looks at user, verb, apiGroup, resource, and name.
-func attributesRecord(user user.Info, namespace, verb, apiGroup, resource, name string) authorizer.AttributesRecord {
+func attributesRecord(user user.Info, namespace, verb, apiGroup, resource, name, path string) authorizer.AttributesRecord {
+	resourceRequest := path == ""
 	return authorizer.AttributesRecord{
 		User:            user,
 		Verb:            verb,
 		Namespace:       namespace,
 		APIGroup:        apiGroup,
 		Resource:        resource,
 		Name:            name,
-		ResourceRequest: true,
+		ResourceRequest: resourceRequest,
+		Path:            path,
 	}
 }
 

pkg/controller/install/attributes_util_test.go

@@ -29,7 +29,17 @@ func TestToAttributeSet(t *testing.T) {
 				Resources: []string{"*"},
 			},
 			expectedAttributes: map[authorizer.AttributesRecord]struct{}{
-				attributesRecord(user, namespace, "*", "*", "*", ""): {},
+				attributesRecord(user, namespace, "*", "*", "*", "", ""): {},
+			},
+		},
+		{
+			description: "SimpleNonResourceRule",
+			rule: rbacv1.PolicyRule{
+				Verbs:           []string{"*"},
+				NonResourceURLs: []string{"/api"},
+			},
+			expectedAttributes: map[authorizer.AttributesRecord]struct{}{
+				attributesRecord(user, namespace, "*", "", "", "", "/api"): {},
 			},
 		},
 		{
@@ -40,8 +50,8 @@ func TestToAttributeSet(t *testing.T) {
 				Resources: []string{"*"},
 			},
 			expectedAttributes: map[authorizer.AttributesRecord]struct{}{
-				attributesRecord(user, namespace, "create", "*", "*", ""): {},
-				attributesRecord(user, namespace, "delete", "*", "*", ""): {},
+				attributesRecord(user, namespace, "create", "*", "*", "", ""): {},
+				attributesRecord(user, namespace, "delete", "*", "*", "", ""): {},
 			},
 		},
 		{
@@ -51,10 +61,21 @@ func TestToAttributeSet(t *testing.T) {
 				Resources: []string{"donuts", "coffee"},
 			},
 			expectedAttributes: map[authorizer.AttributesRecord]struct{}{
-				attributesRecord(user, namespace, "get", "", "donuts", ""):    {},
-				attributesRecord(user, namespace, "update", "", "donuts", ""): {},
-				attributesRecord(user, namespace, "get", "", "coffee", ""):    {},
-				attributesRecord(user, namespace, "update", "", "coffee", ""): {},
+				attributesRecord(user, namespace, "get", "", "donuts", "", ""):    {},
+				attributesRecord(user, namespace, "update", "", "donuts", "", ""): {},
+				attributesRecord(user, namespace, "get", "", "coffee", "", ""):    {},
+				attributesRecord(user, namespace, "update", "", "coffee", "", ""): {},
+			},
+		},
+		{
+			description: "MultipleNonResourceURLs",
+			rule: rbacv1.PolicyRule{
+				Verbs:           []string{"*"},
+				NonResourceURLs: []string{"/capybaras", "/caviidaes"},
+			},
+			expectedAttributes: map[authorizer.AttributesRecord]struct{}{
+				attributesRecord(user, namespace, "*", "", "", "", "/capybaras"): {},
+				attributesRecord(user, namespace, "*", "", "", "", "/caviidaes"): {},
 			},
 		},
 		{
@@ -65,10 +86,10 @@ func TestToAttributeSet(t *testing.T) {
 				ResourceNames: []string{"nyc"},
 			},
 			expectedAttributes: map[authorizer.AttributesRecord]struct{}{
-				attributesRecord(user, namespace, "get", "", "donuts", "nyc"):    {},
-				attributesRecord(user, namespace, "update", "", "donuts", "nyc"): {},
-				attributesRecord(user, namespace, "get", "", "coffee", "nyc"):    {},
-				attributesRecord(user, namespace, "update", "", "coffee", "nyc"): {},
+				attributesRecord(user, namespace, "get", "", "donuts", "nyc", ""):    {},
+				attributesRecord(user, namespace, "update", "", "donuts", "nyc", ""): {},
+				attributesRecord(user, namespace, "get", "", "coffee", "nyc", ""):    {},
+				attributesRecord(user, namespace, "update", "", "coffee", "nyc", ""): {},
 			},
 		},
 		{
@@ -80,14 +101,14 @@ func TestToAttributeSet(t *testing.T) {
 				ResourceNames: []string{"nyc"},
 			},
 			expectedAttributes: map[authorizer.AttributesRecord]struct{}{
-				attributesRecord(user, namespace, "get", "apps.coreos.com", "donuts", "nyc"):    {},
-				attributesRecord(user, namespace, "update", "apps.coreos.com", "donuts", "nyc"): {},
-				attributesRecord(user, namespace, "get", "apps.coreos.com", "coffee", "nyc"):    {},
-				attributesRecord(user, namespace, "update", "apps.coreos.com", "coffee", "nyc"): {},
-				attributesRecord(user, namespace, "get", "apps.redhat.com", "donuts", "nyc"):    {},
-				attributesRecord(user, namespace, "update", "apps.redhat.com", "donuts", "nyc"): {},
-				attributesRecord(user, namespace, "get", "apps.redhat.com", "coffee", "nyc"):    {},
-				attributesRecord(user, namespace, "update", "apps.redhat.com", "coffee", "nyc"): {},
+				attributesRecord(user, namespace, "get", "apps.coreos.com", "donuts", "nyc", ""):    {},
+				attributesRecord(user, namespace, "update", "apps.coreos.com", "donuts", "nyc", ""): {},
+				attributesRecord(user, namespace, "get", "apps.coreos.com", "coffee", "nyc", ""):    {},
+				attributesRecord(user, namespace, "update", "apps.coreos.com", "coffee", "nyc", ""): {},
+				attributesRecord(user, namespace, "get", "apps.redhat.com", "donuts", "nyc", ""):    {},
+				attributesRecord(user, namespace, "update", "apps.redhat.com", "donuts", "nyc", ""): {},
+				attributesRecord(user, namespace, "get", "apps.redhat.com", "coffee", "nyc", ""):    {},
+				attributesRecord(user, namespace, "update", "apps.redhat.com", "coffee", "nyc", ""): {},
 			},
 		},
 		{

pkg/controller/install/rule_checker.go

@@ -1,6 +1,8 @@
 package install
 
 import (
+	"fmt"
+
 	corev1 "k8s.io/api/core/v1"
 	rbacv1 "k8s.io/api/rbac/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -42,6 +44,12 @@ func NewCSVRuleChecker(roleLister crbacv1.RoleLister, roleBindingLister crbacv1.
 
 // RuleSatisfied returns true if a ServiceAccount is authorized to perform all actions described by a PolicyRule in a namespace
 func (c *CSVRuleChecker) RuleSatisfied(sa *corev1.ServiceAccount, namespace string, rule rbacv1.PolicyRule) (bool, error) {
+	// check if the rule is valid
+	err := ruleValid(rule)
+	if err != nil {
+		return false, fmt.Errorf("rule invalid: %s", err.Error())
+	}
+
 	// get attributes set for the given Role and ServiceAccount
 	user := toDefaultInfo(sa)
 	attributesSet := toAttributesSet(user, namespace, rule)
@@ -155,3 +163,16 @@ func (c *CSVRuleChecker) hasOwnerConflicts(ownerRefs []metav1.OwnerReference) bo
 
 	return conflicts
 }
+
+func ruleValid(rule rbacv1.PolicyRule) error {
+	if len(rule.Verbs) == 0 {
+		return fmt.Errorf("policy rule must have at least one verb")
+	}
+
+	resourceCount := len(rule.APIGroups) + len(rule.Resources) + len(rule.ResourceNames)
+	if resourceCount > 0 && len(rule.NonResourceURLs) > 0 {
+		return fmt.Errorf("rule cannot apply to both regular resources and non-resource URLs")
+	}
+
+	return nil
+}

pkg/controller/operators/olm/operator_test.go

@@ -13,6 +13,7 @@ import (
 	"github.com/stretchr/testify/require"
 	appsv1 "k8s.io/api/apps/v1"
 	"k8s.io/api/core/v1"
+	rbacv1 "k8s.io/api/rbac/v1"
 	"k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1"
 	apiextensionsfake "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset/fake"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -393,6 +394,50 @@ func TestTransitionCSV(t *testing.T) {
 				},
 			},
 		},
+		{
+			name: "SingleCSVPendingToFailed/BadStrategyPermissions",
+			initial: initial{
+				csvs: []runtime.Object{
+					csv("csv1",
+						namespace,
+						"",
+						installStrategy("csv1-dep1",
+							nil,
+							[]install.StrategyDeploymentPermissions{
+								{
+									ServiceAccountName: "sa",
+									Rules: []rbacv1.PolicyRule{
+										{
+											Verbs:           []string{"*"},
+											Resources:       []string{"*"},
+											NonResourceURLs: []string{"/osb"},
+										},
+									},
+								},
+							}),
+						[]*v1beta1.CustomResourceDefinition{crd("c1", "v1")},
+						[]*v1beta1.CustomResourceDefinition{},
+						v1alpha1.CSVPhasePending,
+					),
+				},
+				crds: []runtime.Object{
+					crd("c1", "v1"),
+				},
+				objs: []runtime.Object{
+					&v1.ServiceAccount{
+						ObjectMeta: metav1.ObjectMeta{
+							Name:      "sa",
+							Namespace: namespace,
+						},
+					},
+				},
+			},
+			expected: expected{
+				csvStates: map[string]csvState{
+					"csv1": {exists: true, phase: v1alpha1.CSVPhaseFailed},
+				},
+			},
+		},
 		{
 			name: "SingleCSVPendingToPending/CRD",
 			initial: initial{

pkg/controller/operators/olm/requirements.go

@@ -30,11 +30,35 @@ func (a *Operator) requirementStatus(strategyDetailsDeployment *install.Strategy
 		if err != nil {
 			status.Status = v1alpha1.RequirementStatusReasonNotPresent
 			met = false
-		} else {
+			statuses = append(statuses, status)
+			continue
+		}
+
+		if crd.Spec.Version == r.Version {
 			status.Status = v1alpha1.RequirementStatusReasonPresent
 			status.UUID = string(crd.GetUID())
+			statuses = append(statuses, status)
+			continue
+		}
+
+		served := false
+		for _, version := range crd.Spec.Versions {
+			if version.Name == r.Version {
+				if version.Served {
+					status.Status = v1alpha1.RequirementStatusReasonPresent
+					status.UUID = string(crd.GetUID())
+					statuses = append(statuses, status)
+					served = true
+				}
+				break
+			}
+		}
+
+		if !served {
+			status.Status = v1alpha1.RequirementStatusReasonNotPresent
+			met = false
+			statuses = append(statuses, status)
 		}
-		statuses = append(statuses, status)
 	}
 
 	// Check for required API services
@@ -106,11 +130,11 @@ func (a *Operator) requirementStatus(strategyDetailsDeployment *install.Strategy
 }
 
 // permissionStatus checks whether the given CSV's RBAC requirements are met in its namespace
-func (a *Operator) permissionStatus(strategyDetailsDeployment *install.StrategyDetailsDeployment, ruleChecker install.RuleChecker, csvNamespace string) (bool, []v1alpha1.RequirementStatus) {
+func (a *Operator) permissionStatus(strategyDetailsDeployment *install.StrategyDetailsDeployment, ruleChecker install.RuleChecker, csvNamespace string) (bool, []v1alpha1.RequirementStatus, error) {
 	statusesSet := map[string]v1alpha1.RequirementStatus{}
 	met := true
 
-	checkPermissions := func(permissions []install.StrategyDeploymentPermissions, namespace string) {
+	checkPermissions := func(permissions []install.StrategyDeploymentPermissions, namespace string) error {
 		for _, perm := range permissions {
 			saName := perm.ServiceAccountName
 			log.Debugf("perm.ServiceAccountName: %s", saName)
@@ -153,10 +177,19 @@ func (a *Operator) permissionStatus(strategyDetailsDeployment *install.StrategyD
 					status.Dependents = append(status.Dependents, dependent)
 					continue
 				}
-				dependent.Message = fmt.Sprintf("rule raw:%s", marshalled)
+
+				var scope string
+				if namespace == metav1.NamespaceAll {
+					scope = "cluster"
+				} else {
+					scope = "namespaced"
+				}
+				dependent.Message = fmt.Sprintf("%s rule:%s", scope, marshalled)
 
 				satisfied, err := ruleChecker.RuleSatisfied(sa, namespace, rule)
-				if err != nil || !satisfied {
+				if err != nil {
+					return err
+				} else if !satisfied {
 					met = false
 					dependent.Status = v1alpha1.DependentStatusReasonNotSatisfied
 					status.Status = v1alpha1.RequirementStatusReasonPresentNotSatisfied
@@ -169,18 +202,26 @@ func (a *Operator) permissionStatus(strategyDetailsDeployment *install.StrategyD
 
 			statusesSet[saName] = status
 		}
+
+		return nil
 	}
 
-	checkPermissions(strategyDetailsDeployment.Permissions, csvNamespace)
-	checkPermissions(strategyDetailsDeployment.ClusterPermissions, metav1.NamespaceAll)
+	err := checkPermissions(strategyDetailsDeployment.Permissions, csvNamespace)
+	if err != nil {
+		return false, nil, err
+	}
+	err = checkPermissions(strategyDetailsDeployment.ClusterPermissions, metav1.NamespaceAll)
+	if err != nil {
+		return false, nil, err
+	}
 
 	statuses := []v1alpha1.RequirementStatus{}
 	for key, status := range statusesSet {
 		log.Debugf("appending permission status: %s", key)
 		statuses = append(statuses, status)
 	}
 
-	return met, statuses
+	return met, statuses, nil
 }
 
 // requirementAndPermissionStatus returns the aggregate requirement and permissions statuses for the given CSV
@@ -201,7 +242,10 @@ func (a *Operator) requirementAndPermissionStatus(csv *v1alpha1.ClusterServiceVe
 	reqMet, reqStatuses := a.requirementStatus(strategyDetailsDeployment, csv.GetAllCRDDescriptions(), csv.GetOwnedAPIServiceDescriptions(), csv.GetRequiredAPIServiceDescriptions())
 
 	ruleChecker := install.NewCSVRuleChecker(a.roleLister, a.roleBindingLister, a.clusterRoleLister, a.clusterRoleBindingLister, csv)
-	permMet, permStatuses := a.permissionStatus(strategyDetailsDeployment, ruleChecker, csv.GetNamespace())
+	permMet, permStatuses, err := a.permissionStatus(strategyDetailsDeployment, ruleChecker, csv.GetNamespace())
+	if err != nil {
+		return false, nil, err
+	}
 
 	// Aggregate requirement and permissions statuses
 	statuses := append(reqStatuses, permStatuses...)