Merge pull request #58531 from liggitt/rbac-reason

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 {

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)
+		}
 	}
 }

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.