From 67360883bc801b0f1334b146e8dce6f282e50e7e Mon Sep 17 00:00:00 2001 From: Jordan Liggitt Date: Thu, 13 Apr 2017 10:33:28 -0400 Subject: [PATCH] Switch to pointer to policy rule, visit and short circuit during authorization --- pkg/apis/rbac/helpers.go | 10 +-- pkg/registry/rbac/validation/rule.go | 58 ++++++++++--- pkg/registry/rbac/validation/rule_test.go | 4 +- plugin/pkg/auth/authorizer/rbac/BUILD | 1 + plugin/pkg/auth/authorizer/rbac/rbac.go | 41 +++++++-- plugin/pkg/auth/authorizer/rbac/rbac_test.go | 84 ++++++++++++++++++- .../k8s.io/client-go/pkg/apis/rbac/helpers.go | 10 +-- 7 files changed, 176 insertions(+), 32 deletions(-) diff --git a/pkg/apis/rbac/helpers.go b/pkg/apis/rbac/helpers.go index 8b4677bb869..830e227fd74 100644 --- a/pkg/apis/rbac/helpers.go +++ b/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 diff --git a/pkg/registry/rbac/validation/rule.go b/pkg/registry/rbac/validation/rule.go index 131c1820bcd..db65ed4508d 100644 --- a/pkg/registry/rbac/validation/rule.go +++ b/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,12 +97,31 @@ 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) +} +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 { - errorlist = append(errorlist, err) - + if !visitor(nil, err) { + return + } } else { for _, clusterRoleBinding := range clusterRoleBindings { if !appliesTo(user, clusterRoleBinding.Subjects, "") { @@ -106,17 +129,24 @@ func (r *DefaultRuleResolver) RulesFor(user user.Info, namespace string) ([]rbac } 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) { @@ -124,15 +154,19 @@ func (r *DefaultRuleResolver) RulesFor(user user.Info, namespace string) ([]rbac } 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. diff --git a/pkg/registry/rbac/validation/rule_test.go b/pkg/registry/rbac/validation/rule_test.go index fb45667b9cf..b0a4d5bb1cf 100644 --- a/pkg/registry/rbac/validation/rule_test.go +++ b/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, }, } diff --git a/plugin/pkg/auth/authorizer/rbac/BUILD b/plugin/pkg/auth/authorizer/rbac/BUILD index fdd92b754b0..598ff353545 100644 --- a/plugin/pkg/auth/authorizer/rbac/BUILD +++ b/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", diff --git a/plugin/pkg/auth/authorizer/rbac/rbac.go b/plugin/pkg/auth/authorizer/rbac/rbac.go index 655f3d1cb7f..7851cf2a388 100644 --- a/plugin/pkg/auth/authorizer/rbac/rbac.go +++ b/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,8 +131,8 @@ 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 } } @@ -113,7 +140,7 @@ func RulesAllow(requestAttributes authorizer.Attributes, rules ...rbac.PolicyRul 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 { diff --git a/plugin/pkg/auth/authorizer/rbac/rbac_test.go b/plugin/pkg/auth/authorizer/rbac/rbac_test.go index c4dd0ed0b34..16a21241ce8 100644 --- a/plugin/pkg/auth/authorizer/rbac/rbac_test.go +++ b/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) + } + }) + } +} diff --git a/staging/src/k8s.io/client-go/pkg/apis/rbac/helpers.go b/staging/src/k8s.io/client-go/pkg/apis/rbac/helpers.go index a2ec9e8a53c..7007a509a9b 100644 --- a/staging/src/k8s.io/client-go/pkg/apis/rbac/helpers.go +++ b/staging/src/k8s.io/client-go/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