From addc4b166c3b21b3ea94a1d50d579a84b86fde5f Mon Sep 17 00:00:00 2001 From: Eric Chiang Date: Thu, 30 Jun 2016 13:34:22 -0700 Subject: [PATCH] rbac authorizer: support non-resource urls with stars ("/apis/*") --- pkg/apis/rbac/validation/policy_comparator.go | 31 ++++++++++++++- .../rbac/validation/policy_comparator_test.go | 38 +++++++++++++++++-- pkg/apis/rbac/validation/validation_test.go | 25 ++++++++++++ plugin/pkg/auth/authorizer/rbac/rbac_test.go | 11 ++++++ 4 files changed, 99 insertions(+), 6 deletions(-) diff --git a/pkg/apis/rbac/validation/policy_comparator.go b/pkg/apis/rbac/validation/policy_comparator.go index 756407a3795..53dcba5bc60 100644 --- a/pkg/apis/rbac/validation/policy_comparator.go +++ b/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. @@ -101,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 { @@ -108,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 diff --git a/pkg/apis/rbac/validation/policy_comparator_test.go b/pkg/apis/rbac/validation/policy_comparator_test.go index f029d77d59c..85e51237aab 100644 --- a/pkg/apis/rbac/validation/policy_comparator_test.go +++ b/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, diff --git a/pkg/apis/rbac/validation/validation_test.go b/pkg/apis/rbac/validation/validation_test.go index 599bf9cef74..1325bef99e3 100644 --- a/pkg/apis/rbac/validation/validation_test.go +++ b/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) + } + } +} diff --git a/plugin/pkg/auth/authorizer/rbac/rbac_test.go b/plugin/pkg/auth/authorizer/rbac/rbac_test.go index e4111f1668a..ec327a7a756 100644 --- a/plugin/pkg/auth/authorizer/rbac/rbac_test.go +++ b/plugin/pkg/auth/authorizer/rbac/rbac_test.go @@ -150,10 +150,12 @@ func TestAuthorizer(t *testing.T) { 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"}, @@ -162,6 +164,11 @@ func TestAuthorizer(t *testing.T) { 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 @@ -173,6 +180,10 @@ func TestAuthorizer(t *testing.T) { 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"}, }, }, }