Switch to pointer to policy rule, visit and short circuit during authorization

pkg/apis/rbac/helpers.go

@@ -29,7 +29,7 @@ func RoleRefGroupKind(roleRef RoleRef) schema.GroupKind {
 	return schema.GroupKind{Group: roleRef.APIGroup, Kind: roleRef.Kind}
 }
 
-func VerbMatches(rule PolicyRule, requestedVerb string) bool {
+func VerbMatches(rule *PolicyRule, requestedVerb string) bool {
 	for _, ruleVerb := range rule.Verbs {
 		if ruleVerb == VerbAll {
 			return true
@@ -42,7 +42,7 @@ func VerbMatches(rule PolicyRule, requestedVerb string) bool {
 	return false
 }
 
-func APIGroupMatches(rule PolicyRule, requestedGroup string) bool {
+func APIGroupMatches(rule *PolicyRule, requestedGroup string) bool {
 	for _, ruleGroup := range rule.APIGroups {
 		if ruleGroup == APIGroupAll {
 			return true
@@ -55,7 +55,7 @@ func APIGroupMatches(rule PolicyRule, requestedGroup string) bool {
 	return false
 }
 
-func ResourceMatches(rule PolicyRule, requestedResource string) bool {
+func ResourceMatches(rule *PolicyRule, requestedResource string) bool {
 	for _, ruleResource := range rule.Resources {
 		if ruleResource == ResourceAll {
 			return true
@@ -68,7 +68,7 @@ func ResourceMatches(rule PolicyRule, requestedResource string) bool {
 	return false
 }
 
-func ResourceNameMatches(rule PolicyRule, requestedName string) bool {
+func ResourceNameMatches(rule *PolicyRule, requestedName string) bool {
 	if len(rule.ResourceNames) == 0 {
 		return true
 	}
@@ -82,7 +82,7 @@ func ResourceNameMatches(rule PolicyRule, requestedName string) bool {
 	return false
 }
 
-func NonResourceURLMatches(rule PolicyRule, requestedURL string) bool {
+func NonResourceURLMatches(rule *PolicyRule, requestedURL string) bool {
 	for _, ruleURL := range rule.NonResourceURLs {
 		if ruleURL == NonResourceAll {
 			return true

pkg/registry/rbac/validation/rule.go

@@ -39,6 +39,10 @@ type AuthorizationRuleResolver interface {
 	// PolicyRules may not be complete, but it contains all retrievable rules.  This is done because policy rules are purely additive and policy determinations
 	// can be made on the basis of those rules that are found.
 	RulesFor(user user.Info, namespace string) ([]rbac.PolicyRule, error)
+
+	// VisitRulesFor invokes visitor() with each rule that applies to a given user in a given namespace, and each error encountered resolving those rules.
+	// If visitor() returns false, visiting is short-circuited.
+	VisitRulesFor(user user.Info, namespace string, visitor func(rule *rbac.PolicyRule, err error) bool)
 }
 
 // ConfirmNoEscalation determines if the roles for a given user in a given namespace encompass the provided role.
@@ -93,46 +97,76 @@ type ClusterRoleBindingLister interface {
 }
 
 func (r *DefaultRuleResolver) RulesFor(user user.Info, namespace string) ([]rbac.PolicyRule, error) {
-	policyRules := []rbac.PolicyRule{}
-	errorlist := []error{}
+	visitor := &ruleAccumulator{}
+	r.VisitRulesFor(user, namespace, visitor.visit)
+	return visitor.rules, utilerrors.NewAggregate(visitor.errors)
+}
 
-	if clusterRoleBindings, err := r.clusterRoleBindingLister.ListClusterRoleBindings(); err != nil {
-		errorlist = append(errorlist, err)
+type ruleAccumulator struct {
+	rules  []rbac.PolicyRule
+	errors []error
+}
+
+func (r *ruleAccumulator) visit(rule *rbac.PolicyRule, err error) bool {
+	if rule != nil {
+		r.rules = append(r.rules, *rule)
+	}
+	if err != nil {
+		r.errors = append(r.errors, err)
+	}
+	return true
+}
 
+func (r *DefaultRuleResolver) VisitRulesFor(user user.Info, namespace string, visitor func(rule *rbac.PolicyRule, err error) bool) {
+	if clusterRoleBindings, err := r.clusterRoleBindingLister.ListClusterRoleBindings(); err != nil {
+		if !visitor(nil, err) {
+			return
+		}
 	} else {
 		for _, clusterRoleBinding := range clusterRoleBindings {
 			if !appliesTo(user, clusterRoleBinding.Subjects, "") {
 				continue
 			}
 			rules, err := r.GetRoleReferenceRules(clusterRoleBinding.RoleRef, "")
 			if err != nil {
-				errorlist = append(errorlist, err)
+				if !visitor(nil, err) {
+					return
+				}
 				continue
 			}
-			policyRules = append(policyRules, rules...)
+			for i := range rules {
+				if !visitor(&rules[i], nil) {
+					return
+				}
+			}
 		}
 	}
 
 	if len(namespace) > 0 {
 		if roleBindings, err := r.roleBindingLister.ListRoleBindings(namespace); err != nil {
-			errorlist = append(errorlist, err)
-
+			if !visitor(nil, err) {
+				return
+			}
 		} else {
 			for _, roleBinding := range roleBindings {
 				if !appliesTo(user, roleBinding.Subjects, namespace) {
 					continue
 				}
 				rules, err := r.GetRoleReferenceRules(roleBinding.RoleRef, namespace)
 				if err != nil {
-					errorlist = append(errorlist, err)
+					if !visitor(nil, err) {
+						return
+					}
 					continue
 				}
-				policyRules = append(policyRules, rules...)
+				for i := range rules {
+					if !visitor(&rules[i], nil) {
+						return
+					}
+				}
 			}
 		}
 	}
-
-	return policyRules, utilerrors.NewAggregate(errorlist)
 }
 
 // GetRoleReferenceRules attempts to resolve the RoleBinding or ClusterRoleBinding.

pkg/registry/rbac/validation/rule_test.go

@@ -128,7 +128,7 @@ func TestDefaultRuleResolver(t *testing.T) {
 			StaticRoles:    staticRoles1,
 			user:           &user.DefaultInfo{Name: "foobar"},
 			namespace:      "namespace2",
-			effectiveRules: []rbac.PolicyRule{},
+			effectiveRules: nil,
 		},
 		{
 			StaticRoles: staticRoles1,
@@ -139,7 +139,7 @@ func TestDefaultRuleResolver(t *testing.T) {
 		{
 			StaticRoles:    staticRoles1,
 			user:           &user.DefaultInfo{},
-			effectiveRules: []rbac.PolicyRule{},
+			effectiveRules: nil,
 		},
 	}
 

plugin/pkg/auth/authorizer/rbac/BUILD

@@ -36,6 +36,7 @@ go_test(
     deps = [
         "//pkg/apis/rbac:go_default_library",
         "//pkg/registry/rbac/validation:go_default_library",
+        "//plugin/pkg/auth/authorizer/rbac/bootstrappolicy:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
         "//vendor/k8s.io/apiserver/pkg/authentication/user:go_default_library",
         "//vendor/k8s.io/apiserver/pkg/authorization/authorizer:go_default_library",

plugin/pkg/auth/authorizer/rbac/rbac.go

@@ -24,6 +24,7 @@ import (
 
 	"bytes"
 
+	utilerrors "k8s.io/apimachinery/pkg/util/errors"
 	"k8s.io/apiserver/pkg/authentication/user"
 	"k8s.io/apiserver/pkg/authorization/authorizer"
 	"k8s.io/kubernetes/pkg/apis/rbac"
@@ -36,15 +37,41 @@ type RequestToRuleMapper interface {
 	// supplied, you do not have to fail the request.  If you cannot, you should indicate the error along
 	// with your denial.
 	RulesFor(subject user.Info, namespace string) ([]rbac.PolicyRule, error)
+
+	// VisitRulesFor invokes visitor() with each rule that applies to a given user in a given namespace,
+	// and each error encountered resolving those rules. Rule may be nil if err is non-nil.
+	// If visitor() returns false, visiting is short-circuited.
+	VisitRulesFor(user user.Info, namespace string, visitor func(rule *rbac.PolicyRule, err error) bool)
 }
 
 type RBACAuthorizer struct {
 	authorizationRuleResolver RequestToRuleMapper
 }
 
+// authorizingVisitor short-circuits once allowed, and collects any resolution errors encountered
+type authorizingVisitor struct {
+	requestAttributes authorizer.Attributes
+
+	allowed bool
+	errors  []error
+}
+
+func (v *authorizingVisitor) visit(rule *rbac.PolicyRule, err error) bool {
+	if rule != nil && RuleAllows(v.requestAttributes, rule) {
+		v.allowed = true
+		return false
+	}
+	if err != nil {
+		v.errors = append(v.errors, err)
+	}
+	return true
+}
+
 func (r *RBACAuthorizer) Authorize(requestAttributes authorizer.Attributes) (bool, string, error) {
-	rules, ruleResolutionError := r.authorizationRuleResolver.RulesFor(requestAttributes.GetUser(), requestAttributes.GetNamespace())
-	if RulesAllow(requestAttributes, rules...) {
+	ruleCheckingVisitor := &authorizingVisitor{requestAttributes: requestAttributes}
+
+	r.authorizationRuleResolver.VisitRulesFor(requestAttributes.GetUser(), requestAttributes.GetNamespace(), ruleCheckingVisitor.visit)
+	if ruleCheckingVisitor.allowed {
 		return true, "", nil
 	}
 
@@ -88,8 +115,8 @@ func (r *RBACAuthorizer) Authorize(requestAttributes authorizer.Attributes) (boo
 	}
 
 	reason := ""
-	if ruleResolutionError != nil {
-		reason = fmt.Sprintf("%v", ruleResolutionError)
+	if len(ruleCheckingVisitor.errors) > 0 {
+		reason = fmt.Sprintf("%v", utilerrors.NewAggregate(ruleCheckingVisitor.errors))
 	}
 	return false, reason, nil
 }
@@ -104,16 +131,16 @@ func New(roles rbacregistryvalidation.RoleGetter, roleBindings rbacregistryvalid
 }
 
 func RulesAllow(requestAttributes authorizer.Attributes, rules ...rbac.PolicyRule) bool {
-	for _, rule := range rules {
-		if RuleAllows(requestAttributes, rule) {
+	for i := range rules {
+		if RuleAllows(requestAttributes, &rules[i]) {
 			return true
 		}
 	}
 
 	return false
 }
 
-func RuleAllows(requestAttributes authorizer.Attributes, rule rbac.PolicyRule) bool {
+func RuleAllows(requestAttributes authorizer.Attributes, rule *rbac.PolicyRule) bool {
 	if requestAttributes.IsResourceRequest() {
 		resource := requestAttributes.GetResource()
 		if len(requestAttributes.GetSubresource()) > 0 {

plugin/pkg/auth/authorizer/rbac/rbac_test.go

@@ -26,6 +26,7 @@ import (
 	"k8s.io/apiserver/pkg/authorization/authorizer"
 	"k8s.io/kubernetes/pkg/apis/rbac"
 	rbacregistryvalidation "k8s.io/kubernetes/pkg/registry/rbac/validation"
+	"k8s.io/kubernetes/plugin/pkg/auth/authorizer/rbac/bootstrappolicy"
 )
 
 func newRule(verbs, apiGroups, resources, nonResourceURLs string) rbac.PolicyRule {
@@ -381,7 +382,7 @@ func TestRuleMatches(t *testing.T) {
 	}
 	for _, tc := range tests {
 		for request, expected := range tc.requestsToExpected {
-			if e, a := expected, RuleAllows(request, tc.rule); e != a {
+			if e, a := expected, RuleAllows(request, &tc.rule); e != a {
 				t.Errorf("%q: expected %v, got %v for %v", tc.name, e, a, request)
 			}
 		}
@@ -432,3 +433,84 @@ func (r *requestAttributeBuilder) URL(url string) *requestAttributeBuilder {
 func (r *requestAttributeBuilder) New() authorizer.AttributesRecord {
 	return r.request
 }
+
+func BenchmarkAuthorize(b *testing.B) {
+	bootstrapRoles := []rbac.ClusterRole{}
+	bootstrapRoles = append(bootstrapRoles, bootstrappolicy.ControllerRoles()...)
+	bootstrapRoles = append(bootstrapRoles, bootstrappolicy.ClusterRoles()...)
+
+	bootstrapBindings := []rbac.ClusterRoleBinding{}
+	bootstrapBindings = append(bootstrapBindings, bootstrappolicy.ClusterRoleBindings()...)
+	bootstrapBindings = append(bootstrapBindings, bootstrappolicy.ControllerRoleBindings()...)
+
+	clusterRoles := []*rbac.ClusterRole{}
+	for i := range bootstrapRoles {
+		clusterRoles = append(clusterRoles, &bootstrapRoles[i])
+	}
+	clusterRoleBindings := []*rbac.ClusterRoleBinding{}
+	for i := range bootstrapBindings {
+		clusterRoleBindings = append(clusterRoleBindings, &bootstrapBindings[i])
+	}
+
+	_, resolver := rbacregistryvalidation.NewTestRuleResolver(nil, nil, clusterRoles, clusterRoleBindings)
+
+	authz := New(resolver, resolver, resolver, resolver)
+
+	nodeUser := &user.DefaultInfo{Name: "system:node:node1", Groups: []string{"system:nodes", "system:authenticated"}}
+	requests := []struct {
+		name  string
+		attrs authorizer.Attributes
+	}{
+		{
+			"allow list pods",
+			authorizer.AttributesRecord{
+				ResourceRequest: true,
+				User:            nodeUser,
+				Verb:            "list",
+				Resource:        "pods",
+				Subresource:     "",
+				Name:            "",
+				Namespace:       "",
+				APIGroup:        "",
+				APIVersion:      "v1",
+			},
+		},
+		{
+			"allow update pods/status",
+			authorizer.AttributesRecord{
+				ResourceRequest: true,
+				User:            nodeUser,
+				Verb:            "update",
+				Resource:        "pods",
+				Subresource:     "status",
+				Name:            "mypods",
+				Namespace:       "myns",
+				APIGroup:        "",
+				APIVersion:      "v1",
+			},
+		},
+		{
+			"forbid educate dolphins",
+			authorizer.AttributesRecord{
+				ResourceRequest: true,
+				User:            nodeUser,
+				Verb:            "educate",
+				Resource:        "dolphins",
+				Subresource:     "",
+				Name:            "",
+				Namespace:       "",
+				APIGroup:        "",
+				APIVersion:      "v1",
+			},
+		},
+	}
+
+	b.ResetTimer()
+	for _, request := range requests {
+		b.Run(request.name, func(b *testing.B) {
+			for i := 0; i < b.N; i++ {
+				authz.Authorize(request.attrs)
+			}
+		})
+	}
+}