From b4fb25261e4d266aca5f0bc19c75b003b531f35d Mon Sep 17 00:00:00 2001 From: Jordan Liggitt Date: Fri, 19 Jan 2018 13:17:32 -0500 Subject: [PATCH] return reason for allowed rbac authorizations includes the binding, role, and subject that allowed a request so audit can make use of it --- pkg/registry/rbac/validation/rule.go | 81 ++++++++++++++++++----- pkg/registry/rbac/validation/rule_test.go | 13 +++- plugin/pkg/auth/authorizer/rbac/rbac.go | 8 ++- 3 files changed, 83 insertions(+), 19 deletions(-) diff --git a/pkg/registry/rbac/validation/rule.go b/pkg/registry/rbac/validation/rule.go index db65ed4508..e80382e6d3 100644 --- a/pkg/registry/rbac/validation/rule.go +++ b/pkg/registry/rbac/validation/rule.go @@ -42,7 +42,7 @@ type AuthorizationRuleResolver interface { // 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) + VisitRulesFor(user user.Info, namespace string, visitor func(source fmt.Stringer, rule *rbac.PolicyRule, err error) bool) } // ConfirmNoEscalation determines if the roles for a given user in a given namespace encompass the provided role. @@ -107,7 +107,7 @@ type ruleAccumulator struct { errors []error } -func (r *ruleAccumulator) visit(rule *rbac.PolicyRule, err error) bool { +func (r *ruleAccumulator) visit(source fmt.Stringer, rule *rbac.PolicyRule, err error) bool { if rule != nil { r.rules = append(r.rules, *rule) } @@ -117,25 +117,69 @@ func (r *ruleAccumulator) visit(rule *rbac.PolicyRule, err error) bool { return true } -func (r *DefaultRuleResolver) VisitRulesFor(user user.Info, namespace string, visitor func(rule *rbac.PolicyRule, err error) bool) { +func describeSubject(s *rbac.Subject, bindingNamespace string) string { + switch s.Kind { + case rbac.ServiceAccountKind: + if len(s.Namespace) > 0 { + return fmt.Sprintf("%s %q", s.Kind, s.Name+"/"+s.Namespace) + } + return fmt.Sprintf("%s %q", s.Kind, s.Name+"/"+bindingNamespace) + default: + return fmt.Sprintf("%s %q", s.Kind, s.Name) + } +} + +type clusterRoleBindingDescriber struct { + binding *rbac.ClusterRoleBinding + subject *rbac.Subject +} + +func (d *clusterRoleBindingDescriber) String() string { + return fmt.Sprintf("ClusterRoleBinding %q of %s %q to %s", + d.binding.Name, + d.binding.RoleRef.Kind, + d.binding.RoleRef.Name, + describeSubject(d.subject, ""), + ) +} + +type roleBindingDescriber struct { + binding *rbac.RoleBinding + subject *rbac.Subject +} + +func (d *roleBindingDescriber) String() string { + return fmt.Sprintf("RoleBinding %q of %s %q to %s", + d.binding.Name+"/"+d.binding.Namespace, + d.binding.RoleRef.Kind, + d.binding.RoleRef.Name, + describeSubject(d.subject, d.binding.Namespace), + ) +} + +func (r *DefaultRuleResolver) VisitRulesFor(user user.Info, namespace string, visitor func(source fmt.Stringer, rule *rbac.PolicyRule, err error) bool) { if clusterRoleBindings, err := r.clusterRoleBindingLister.ListClusterRoleBindings(); err != nil { - if !visitor(nil, err) { + if !visitor(nil, nil, err) { return } } else { + sourceDescriber := &clusterRoleBindingDescriber{} for _, clusterRoleBinding := range clusterRoleBindings { - if !appliesTo(user, clusterRoleBinding.Subjects, "") { + subjectIndex, applies := appliesTo(user, clusterRoleBinding.Subjects, "") + if !applies { continue } rules, err := r.GetRoleReferenceRules(clusterRoleBinding.RoleRef, "") if err != nil { - if !visitor(nil, err) { + if !visitor(nil, nil, err) { return } continue } + sourceDescriber.binding = clusterRoleBinding + sourceDescriber.subject = &clusterRoleBinding.Subjects[subjectIndex] for i := range rules { - if !visitor(&rules[i], nil) { + if !visitor(sourceDescriber, &rules[i], nil) { return } } @@ -144,23 +188,27 @@ func (r *DefaultRuleResolver) VisitRulesFor(user user.Info, namespace string, vi if len(namespace) > 0 { if roleBindings, err := r.roleBindingLister.ListRoleBindings(namespace); err != nil { - if !visitor(nil, err) { + if !visitor(nil, nil, err) { return } } else { + sourceDescriber := &roleBindingDescriber{} for _, roleBinding := range roleBindings { - if !appliesTo(user, roleBinding.Subjects, namespace) { + subjectIndex, applies := appliesTo(user, roleBinding.Subjects, namespace) + if !applies { continue } rules, err := r.GetRoleReferenceRules(roleBinding.RoleRef, namespace) if err != nil { - if !visitor(nil, err) { + if !visitor(nil, nil, err) { return } continue } + sourceDescriber.binding = roleBinding + sourceDescriber.subject = &roleBinding.Subjects[subjectIndex] for i := range rules { - if !visitor(&rules[i], nil) { + if !visitor(sourceDescriber, &rules[i], nil) { return } } @@ -190,13 +238,16 @@ func (r *DefaultRuleResolver) GetRoleReferenceRules(roleRef rbac.RoleRef, bindin return nil, fmt.Errorf("unsupported role reference kind: %q", kind) } } -func appliesTo(user user.Info, bindingSubjects []rbac.Subject, namespace string) bool { - for _, bindingSubject := range bindingSubjects { + +// appliesTo returns whether any of the bindingSubjects applies to the specified subject, +// and if true, the index of the first subject that applies +func appliesTo(user user.Info, bindingSubjects []rbac.Subject, namespace string) (int, bool) { + for i, bindingSubject := range bindingSubjects { if appliesToUser(user, bindingSubject, namespace) { - return true + return i, true } } - return false + return 0, false } func appliesToUser(user user.Info, subject rbac.Subject, namespace string) bool { diff --git a/pkg/registry/rbac/validation/rule_test.go b/pkg/registry/rbac/validation/rule_test.go index b0a4d5bb1c..1a176126bd 100644 --- a/pkg/registry/rbac/validation/rule_test.go +++ b/pkg/registry/rbac/validation/rule_test.go @@ -168,6 +168,7 @@ func TestAppliesTo(t *testing.T) { user user.Info namespace string appliesTo bool + index int testCase string }{ { @@ -176,6 +177,7 @@ func TestAppliesTo(t *testing.T) { }, user: &user.DefaultInfo{Name: "foobar"}, appliesTo: true, + index: 0, testCase: "single subject that matches username", }, { @@ -185,6 +187,7 @@ func TestAppliesTo(t *testing.T) { }, user: &user.DefaultInfo{Name: "foobar"}, appliesTo: true, + index: 1, testCase: "multiple subjects, one that matches username", }, { @@ -203,6 +206,7 @@ func TestAppliesTo(t *testing.T) { }, user: &user.DefaultInfo{Name: "zimzam", Groups: []string{"foobar"}}, appliesTo: true, + index: 1, testCase: "multiple subjects, one that match group", }, { @@ -213,6 +217,7 @@ func TestAppliesTo(t *testing.T) { user: &user.DefaultInfo{Name: "zimzam", Groups: []string{"foobar"}}, namespace: "namespace1", appliesTo: true, + index: 1, testCase: "multiple subjects, one that match group, should ignore namespace", }, { @@ -224,6 +229,7 @@ func TestAppliesTo(t *testing.T) { user: &user.DefaultInfo{Name: "system:serviceaccount:kube-system:default"}, namespace: "default", appliesTo: true, + index: 2, testCase: "multiple subjects with a service account that matches", }, { @@ -243,6 +249,7 @@ func TestAppliesTo(t *testing.T) { user: &user.DefaultInfo{Name: "foobar", Groups: []string{user.AllAuthenticated}}, namespace: "default", appliesTo: true, + index: 0, testCase: "binding to all authenticated and unauthenticated subjects matches authenticated user", }, { @@ -253,14 +260,18 @@ func TestAppliesTo(t *testing.T) { user: &user.DefaultInfo{Name: "system:anonymous", Groups: []string{user.AllUnauthenticated}}, namespace: "default", appliesTo: true, + index: 1, testCase: "binding to all authenticated and unauthenticated subjects matches anonymous user", }, } for _, tc := range tests { - got := appliesTo(tc.user, tc.subjects, tc.namespace) + gotIndex, got := appliesTo(tc.user, tc.subjects, tc.namespace) if got != tc.appliesTo { t.Errorf("case %q want appliesTo=%t, got appliesTo=%t", tc.testCase, tc.appliesTo, got) } + if gotIndex != tc.index { + t.Errorf("case %q want index %d, got %d", tc.testCase, tc.index, gotIndex) + } } } diff --git a/plugin/pkg/auth/authorizer/rbac/rbac.go b/plugin/pkg/auth/authorizer/rbac/rbac.go index 1f507eb0ed..bb714a01a0 100644 --- a/plugin/pkg/auth/authorizer/rbac/rbac.go +++ b/plugin/pkg/auth/authorizer/rbac/rbac.go @@ -43,7 +43,7 @@ type RequestToRuleMapper interface { // 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) + VisitRulesFor(user user.Info, namespace string, visitor func(source fmt.Stringer, rule *rbac.PolicyRule, err error) bool) } type RBACAuthorizer struct { @@ -55,12 +55,14 @@ type authorizingVisitor struct { requestAttributes authorizer.Attributes allowed bool + reason string errors []error } -func (v *authorizingVisitor) visit(rule *rbac.PolicyRule, err error) bool { +func (v *authorizingVisitor) visit(source fmt.Stringer, rule *rbac.PolicyRule, err error) bool { if rule != nil && RuleAllows(v.requestAttributes, rule) { v.allowed = true + v.reason = fmt.Sprintf("allowed by %s", source.String()) return false } if err != nil { @@ -74,7 +76,7 @@ func (r *RBACAuthorizer) Authorize(requestAttributes authorizer.Attributes) (aut r.authorizationRuleResolver.VisitRulesFor(requestAttributes.GetUser(), requestAttributes.GetNamespace(), ruleCheckingVisitor.visit) if ruleCheckingVisitor.allowed { - return authorizer.DecisionAllow, "", nil + return authorizer.DecisionAllow, ruleCheckingVisitor.reason, nil } // Build a detailed log of the denial.