Merge pull request #28304 from ericchiang/fix-rbac-non-resource-url-rule-evaluation

Automatic merge from submit-queue

rbac authorizer: cleanups to rule evaluation for non-resource URLs

An few oversights in the RBAC authorizer. Fixes #28291 and permits non-resource URLs to use stars in the path. E.g. ("/apis/*").

cc @liggitt @kubernetes/sig-auth

pkg/apis/rbac/validation/policy_comparator.go

@@ -16,7 +16,11 @@ limitations under the License.
 
 package validation
 
-import "k8s.io/kubernetes/pkg/apis/rbac"
+import (
+	"strings"
+
+	"k8s.io/kubernetes/pkg/apis/rbac"
+)
 
 // Covers determines whether or not the ownerRules cover the servantRules in terms of allowed actions.
 // It returns whether or not the ownerRules cover and a list of the rules that the ownerRules do not cover.
@@ -69,9 +73,11 @@ func breakdownRule(rule rbac.PolicyRule) []rbac.PolicyRule {
 		}
 	}
 
-	// Non-resource URLs are unique because they don't combine with other policy rule fields.
+	// Non-resource URLs are unique because they only combine with verbs.
 	for _, nonResourceURL := range rule.NonResourceURLs {
-		subrules = append(subrules, rbac.PolicyRule{NonResourceURLs: []string{nonResourceURL}})
+		for _, verb := range rule.Verbs {
+			subrules = append(subrules, rbac.PolicyRule{NonResourceURLs: []string{nonResourceURL}, Verbs: []string{verb}})
+		}
 	}
 
 	return subrules
@@ -99,6 +105,29 @@ func hasAll(set, contains []string) bool {
 	return true
 }
 
+func nonResourceURLsCoversAll(set, covers []string) bool {
+	for _, path := range covers {
+		covered := false
+		for _, owner := range set {
+			if nonResourceURLCovers(owner, path) {
+				covered = true
+				break
+			}
+		}
+		if !covered {
+			return false
+		}
+	}
+	return true
+}
+
+func nonResourceURLCovers(ownerPath, subPath string) bool {
+	if ownerPath == subPath {
+		return true
+	}
+	return strings.HasSuffix(ownerPath, "*") && strings.HasPrefix(subPath, strings.TrimRight(ownerPath, "*"))
+}
+
 // ruleCovers determines whether the ownerRule (which may have multiple verbs, resources, and resourceNames) covers
 // the subrule (which may only contain at most one verb, resource, and resourceName)
 func ruleCovers(ownerRule, subRule rbac.PolicyRule) bool {
@@ -106,7 +135,7 @@ func ruleCovers(ownerRule, subRule rbac.PolicyRule) bool {
 	verbMatches := has(ownerRule.Verbs, rbac.VerbAll) || hasAll(ownerRule.Verbs, subRule.Verbs)
 	groupMatches := has(ownerRule.APIGroups, rbac.APIGroupAll) || hasAll(ownerRule.APIGroups, subRule.APIGroups)
 	resourceMatches := has(ownerRule.Resources, rbac.ResourceAll) || hasAll(ownerRule.Resources, subRule.Resources)
-	nonResourceURLMatches := has(ownerRule.NonResourceURLs, rbac.NonResourceAll) || hasAll(ownerRule.NonResourceURLs, subRule.NonResourceURLs)
+	nonResourceURLMatches := nonResourceURLsCoversAll(ownerRule.NonResourceURLs, subRule.NonResourceURLs)
 
 	resourceNameMatches := false
 

pkg/apis/rbac/validation/policy_comparator_test.go

@@ -284,10 +284,10 @@ func TestCoversEnumerationNotCoveringResourceNameEmpty(t *testing.T) {
 func TestCoversNonResourceURLs(t *testing.T) {
 	escalationTest{
 		ownerRules: []rbac.PolicyRule{
-			{NonResourceURLs: []string{"/apis"}},
+			{NonResourceURLs: []string{"/apis"}, Verbs: []string{"*"}},
 		},
 		servantRules: []rbac.PolicyRule{
-			{NonResourceURLs: []string{"/apis"}},
+			{NonResourceURLs: []string{"/apis"}, Verbs: []string{"*"}},
 		},
 
 		expectedCovered:        true,
@@ -298,10 +298,40 @@ func TestCoversNonResourceURLs(t *testing.T) {
 func TestCoversNonResourceURLsStar(t *testing.T) {
 	escalationTest{
 		ownerRules: []rbac.PolicyRule{
-			{NonResourceURLs: []string{"*"}},
+			{NonResourceURLs: []string{"*"}, Verbs: []string{"*"}},
 		},
 		servantRules: []rbac.PolicyRule{
-			{NonResourceURLs: []string{"/apis", "/apis/v1", "/"}},
+			{NonResourceURLs: []string{"/apis", "/apis/v1", "/"}, Verbs: []string{"*"}},
+		},
+
+		expectedCovered:        true,
+		expectedUncoveredRules: []rbac.PolicyRule{},
+	}.test(t)
+}
+
+func TestCoversNonResourceURLsStarAfterPrefixDoesntCover(t *testing.T) {
+	escalationTest{
+		ownerRules: []rbac.PolicyRule{
+			{NonResourceURLs: []string{"/apis/*"}, Verbs: []string{"*"}},
+		},
+		servantRules: []rbac.PolicyRule{
+			{NonResourceURLs: []string{"/apis", "/apis/v1"}, Verbs: []string{"get"}},
+		},
+
+		expectedCovered: false,
+		expectedUncoveredRules: []rbac.PolicyRule{
+			{NonResourceURLs: []string{"/apis"}, Verbs: []string{"get"}},
+		},
+	}.test(t)
+}
+
+func TestCoversNonResourceURLsStarAfterPrefix(t *testing.T) {
+	escalationTest{
+		ownerRules: []rbac.PolicyRule{
+			{NonResourceURLs: []string{"/apis/*"}, Verbs: []string{"*"}},
+		},
+		servantRules: []rbac.PolicyRule{
+			{NonResourceURLs: []string{"/apis/v1/foo", "/apis/v1"}, Verbs: []string{"get"}},
 		},
 
 		expectedCovered:        true,
@@ -333,7 +363,7 @@ func TestCoversNonResourceURLsWithOtherFieldsFailure(t *testing.T) {
 		},
 
 		expectedCovered:        false,
-		expectedUncoveredRules: []rbac.PolicyRule{{NonResourceURLs: []string{"/apis"}}},
+		expectedUncoveredRules: []rbac.PolicyRule{{NonResourceURLs: []string{"/apis"}, Verbs: []string{"get"}}},
 	}.test(t)
 }
 

pkg/apis/rbac/validation/validation_test.go

@@ -219,3 +219,28 @@ func TestValidateRole(t *testing.T) {
 		}
 	}
 }
+
+func TestNonResourceURLCovers(t *testing.T) {
+	tests := []struct {
+		owner     string
+		requested string
+		want      bool
+	}{
+		{"*", "/api", true},
+		{"/api", "/api", true},
+		{"/apis", "/api", false},
+		{"/api/v1", "/api", false},
+		{"/api/v1", "/api/v1", true},
+		{"/api/*", "/api/v1", true},
+		{"/api/*", "/api", false},
+		{"/api/*/*", "/api/v1", false},
+		{"/*/v1/*", "/api/v1", false},
+	}
+
+	for _, tc := range tests {
+		got := nonResourceURLCovers(tc.owner, tc.requested)
+		if got != tc.want {
+			t.Errorf("nonResourceURLCovers(%q, %q): want=(%t), got=(%t)", tc.owner, tc.requested, tc.want, got)
+		}
+	}
+}

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

@@ -58,6 +58,7 @@ func (r *RBACAuthorizer) Authorize(attr authorizer.Attributes) error {
 		}
 	} else {
 		requestedRule = rbac.PolicyRule{
+			Verbs:           []string{attr.GetVerb()},
 			NonResourceURLs: []string{attr.GetPath()},
 		}
 	}

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

@@ -25,13 +25,15 @@ import (
 	"k8s.io/kubernetes/pkg/apis/rbac"
 	"k8s.io/kubernetes/pkg/apis/rbac/validation"
 	"k8s.io/kubernetes/pkg/auth/authorizer"
+	"k8s.io/kubernetes/pkg/auth/user"
 )
 
-func newRule(verbs, apiGroups, resources string) rbac.PolicyRule {
+func newRule(verbs, apiGroups, resources, nonResourceURLs string) rbac.PolicyRule {
 	return rbac.PolicyRule{
-		Verbs:     strings.Split(verbs, ","),
-		APIGroups: strings.Split(apiGroups, ","),
-		Resources: strings.Split(resources, ","),
+		Verbs:           strings.Split(verbs, ","),
+		APIGroups:       strings.Split(apiGroups, ","),
+		Resources:       strings.Split(resources, ","),
+		NonResourceURLs: strings.Split(nonResourceURLs, ","),
 	}
 }
 
@@ -48,6 +50,23 @@ const (
 	bindToClusterRole uint16 = 0x1
 )
 
+func newClusterRoleBinding(roleName string, subjects ...string) rbac.ClusterRoleBinding {
+	r := rbac.ClusterRoleBinding{
+		ObjectMeta: api.ObjectMeta{},
+		RoleRef: api.ObjectReference{
+			Kind: "ClusterRole", // ClusterRoleBindings can only refer to ClusterRole
+			Name: roleName,
+		},
+	}
+
+	r.Subjects = make([]rbac.Subject, len(subjects))
+	for i, subject := range subjects {
+		split := strings.SplitN(subject, ":", 2)
+		r.Subjects[i].Kind, r.Subjects[i].Name = split[0], split[1]
+	}
+	return r
+}
+
 func newRoleBinding(namespace, roleName string, bindType uint16, subjects ...string) rbac.RoleBinding {
 	r := rbac.RoleBinding{ObjectMeta: api.ObjectMeta{Namespace: namespace}}
 
@@ -107,7 +126,7 @@ func TestAuthorizer(t *testing.T) {
 	}{
 		{
 			clusterRoles: []rbac.ClusterRole{
-				newClusterRole("admin", newRule("*", "*", "*")),
+				newClusterRole("admin", newRule("*", "*", "*", "*")),
 			},
 			roleBindings: []rbac.RoleBinding{
 				newRoleBinding("ns1", "admin", bindToClusterRole, "User:admin", "Group:admins"),
@@ -126,6 +145,47 @@ func TestAuthorizer(t *testing.T) {
 				&defaultAttributes{"admin", "admins", "GET", "Nodes", "", ""},
 			},
 		},
+		{
+			// Non-resource-url tests
+			clusterRoles: []rbac.ClusterRole{
+				newClusterRole("non-resource-url-getter", newRule("get", "", "", "/apis")),
+				newClusterRole("non-resource-url", newRule("*", "", "", "/apis")),
+				newClusterRole("non-resource-url-prefix", newRule("get", "", "", "/apis/*")),
+			},
+			clusterRoleBindings: []rbac.ClusterRoleBinding{
+				newClusterRoleBinding("non-resource-url-getter", "User:foo", "Group:bar"),
+				newClusterRoleBinding("non-resource-url", "User:admin", "Group:admin"),
+				newClusterRoleBinding("non-resource-url-prefix", "User:prefixed", "Group:prefixed"),
+			},
+			shouldPass: []authorizer.Attributes{
+				authorizer.AttributesRecord{User: &user.DefaultInfo{Name: "foo"}, Verb: "get", Path: "/apis"},
+				authorizer.AttributesRecord{User: &user.DefaultInfo{Groups: []string{"bar"}}, Verb: "get", Path: "/apis"},
+				authorizer.AttributesRecord{User: &user.DefaultInfo{Name: "admin"}, Verb: "get", Path: "/apis"},
+				authorizer.AttributesRecord{User: &user.DefaultInfo{Groups: []string{"admin"}}, Verb: "get", Path: "/apis"},
+				authorizer.AttributesRecord{User: &user.DefaultInfo{Name: "admin"}, Verb: "watch", Path: "/apis"},
+				authorizer.AttributesRecord{User: &user.DefaultInfo{Groups: []string{"admin"}}, Verb: "watch", Path: "/apis"},
+
+				authorizer.AttributesRecord{User: &user.DefaultInfo{Name: "prefixed"}, Verb: "get", Path: "/apis/v1"},
+				authorizer.AttributesRecord{User: &user.DefaultInfo{Groups: []string{"prefixed"}}, Verb: "get", Path: "/apis/v1"},
+				authorizer.AttributesRecord{User: &user.DefaultInfo{Name: "prefixed"}, Verb: "get", Path: "/apis/v1/foobar"},
+				authorizer.AttributesRecord{User: &user.DefaultInfo{Groups: []string{"prefixed"}}, Verb: "get", Path: "/apis/v1/foorbar"},
+			},
+			shouldFail: []authorizer.Attributes{
+				// wrong verb
+				authorizer.AttributesRecord{User: &user.DefaultInfo{Name: "foo"}, Verb: "watch", Path: "/apis"},
+				authorizer.AttributesRecord{User: &user.DefaultInfo{Groups: []string{"bar"}}, Verb: "watch", Path: "/apis"},
+
+				// wrong path
+				authorizer.AttributesRecord{User: &user.DefaultInfo{Name: "foo"}, Verb: "get", Path: "/api/v1"},
+				authorizer.AttributesRecord{User: &user.DefaultInfo{Groups: []string{"bar"}}, Verb: "get", Path: "/api/v1"},
+				authorizer.AttributesRecord{User: &user.DefaultInfo{Name: "admin"}, Verb: "get", Path: "/api/v1"},
+				authorizer.AttributesRecord{User: &user.DefaultInfo{Groups: []string{"admin"}}, Verb: "get", Path: "/api/v1"},
+
+				// not covered by prefix
+				authorizer.AttributesRecord{User: &user.DefaultInfo{Name: "prefixed"}, Verb: "get", Path: "/api/v1"},
+				authorizer.AttributesRecord{User: &user.DefaultInfo{Groups: []string{"prefixed"}}, Verb: "get", Path: "/api/v1"},
+			},
+		},
 	}
 	for i, tt := range tests {
 		ruleResolver := validation.NewTestRuleResolver(tt.roles, tt.roleBindings, tt.clusterRoles, tt.clusterRoleBindings)