Skip to content

Commit 900c076

Browse files
author
Kubernetes Submit Queue
authored
Merge pull request kubernetes#53722 from deads2k/rbac-01-allow-star
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. allow */subresource in rbac policy rules xref kubernetes#29698 xref kubernetes#38756 xref kubernetes#49504 xref kubernetes#38810 Allow `*/subresource` format in RBAC policy rules to support polymorphic subresources like `*/scale` for HPA. @DirectXMan12 fyi ```release-note RBAC PolicyRules now allow resource=`*/<subresource>` to cover `any-resource/<subresource>`. For example, `*/scale` covers `replicationcontroller/scale`. ```
2 parents d196a4a + e8a703b commit 900c076

File tree

26 files changed

+205
-49
lines changed

26 files changed

+205
-49
lines changed

Diff for: ‎api/openapi-spec/swagger.json

+3-3
Original file line numberDiff line numberDiff line change
@@ -64461,7 +64461,7 @@
6446164461
}
6446264462
},
6446364463
"resources": {
64464-
"description": "Resources is a list of resources this rule applies to. ResourceAll represents all resources. \"*\" means all.",
64464+
"description": "Resources is a list of resources this rule applies to. \"*\" means all in the specified apiGroups.\n \"*/foo\" represents the subresource 'foo' for all resources in the specified apiGroups.",
6446564465
"type": "array",
6446664466
"items": {
6446764467
"type": "string"
@@ -64812,7 +64812,7 @@
6481264812
}
6481364813
},
6481464814
"resources": {
64815-
"description": "Resources is a list of resources this rule applies to. ResourceAll represents all resources. \"*\" means all.",
64815+
"description": "Resources is a list of resources this rule applies to. \"*\" means all in the specified apiGroups.\n \"*/foo\" represents the subresource 'foo' for all resources in the specified apiGroups.",
6481664816
"type": "array",
6481764817
"items": {
6481864818
"type": "string"
@@ -73180,7 +73180,7 @@
7318073180
}
7318173181
},
7318273182
"resources": {
73183-
"description": "Resources is a list of resources this rule applies to. ResourceAll represents all resources.",
73183+
"description": "Resources is a list of resources this rule applies to. '*' represents all resources in the specified apiGroups. '*/foo' represents the subresource 'foo' for all resources in the specified apiGroups.",
7318473184
"type": "array",
7318573185
"items": {
7318673186
"type": "string"

Diff for: ‎api/swagger-spec/authorization.k8s.io_v1.json

+1-1
Original file line numberDiff line numberDiff line change
@@ -785,7 +785,7 @@
785785
"items": {
786786
"type": "string"
787787
},
788-
"description": "Resources is a list of resources this rule applies to. ResourceAll represents all resources. \"*\" means all."
788+
"description": "Resources is a list of resources this rule applies to. \"*\" means all in the specified apiGroups.\n \"*/foo\" represents the subresource 'foo' for all resources in the specified apiGroups."
789789
},
790790
"resourceNames": {
791791
"type": "array",

Diff for: ‎api/swagger-spec/authorization.k8s.io_v1beta1.json

+1-1
Original file line numberDiff line numberDiff line change
@@ -785,7 +785,7 @@
785785
"items": {
786786
"type": "string"
787787
},
788-
"description": "Resources is a list of resources this rule applies to. ResourceAll represents all resources. \"*\" means all."
788+
"description": "Resources is a list of resources this rule applies to. \"*\" means all in the specified apiGroups.\n \"*/foo\" represents the subresource 'foo' for all resources in the specified apiGroups."
789789
},
790790
"resourceNames": {
791791
"type": "array",

Diff for: ‎api/swagger-spec/rbac.authorization.k8s.io_v1beta1.json

+1-1
Original file line numberDiff line numberDiff line change
@@ -3818,7 +3818,7 @@
38183818
"items": {
38193819
"type": "string"
38203820
},
3821-
"description": "Resources is a list of resources this rule applies to. ResourceAll represents all resources."
3821+
"description": "Resources is a list of resources this rule applies to. '*' represents all resources in the specified apiGroups. '*/foo' represents the subresource 'foo' for all resources in the specified apiGroups."
38223822
},
38233823
"resourceNames": {
38243824
"type": "array",

Diff for: ‎docs/api-reference/authorization.k8s.io/v1/definitions.html

+2-1
Original file line numberDiff line numberDiff line change
@@ -1432,7 +1432,8 @@ <h3 id="_v1_resourcerule">v1.ResourceRule</h3>
14321432
</tr>
14331433
<tr>
14341434
<td class="tableblock halign-left valign-top"><p class="tableblock">resources</p></td>
1435-
<td class="tableblock halign-left valign-top"><p class="tableblock">Resources is a list of resources this rule applies to. ResourceAll represents all resources. "*" means all.</p></td>
1435+
<td class="tableblock halign-left valign-top"><p class="tableblock">Resources is a list of resources this rule applies to. "<strong>" means all in the specified apiGroups.<br>
1436+
"</strong>/foo" represents the subresource <em>foo</em> for all resources in the specified apiGroups.</p></td>
14361437
<td class="tableblock halign-left valign-top"><p class="tableblock">false</p></td>
14371438
<td class="tableblock halign-left valign-top"><p class="tableblock">string array</p></td>
14381439
<td class="tableblock halign-left valign-top"></td>

Diff for: ‎docs/api-reference/authorization.k8s.io/v1beta1/definitions.html

+2-1
Original file line numberDiff line numberDiff line change
@@ -1452,7 +1452,8 @@ <h3 id="_v1beta1_resourcerule">v1beta1.ResourceRule</h3>
14521452
</tr>
14531453
<tr>
14541454
<td class="tableblock halign-left valign-top"><p class="tableblock">resources</p></td>
1455-
<td class="tableblock halign-left valign-top"><p class="tableblock">Resources is a list of resources this rule applies to. ResourceAll represents all resources. "*" means all.</p></td>
1455+
<td class="tableblock halign-left valign-top"><p class="tableblock">Resources is a list of resources this rule applies to. "<strong>" means all in the specified apiGroups.<br>
1456+
"</strong>/foo" represents the subresource <em>foo</em> for all resources in the specified apiGroups.</p></td>
14561457
<td class="tableblock halign-left valign-top"><p class="tableblock">false</p></td>
14571458
<td class="tableblock halign-left valign-top"><p class="tableblock">string array</p></td>
14581459
<td class="tableblock halign-left valign-top"></td>

Diff for: ‎docs/api-reference/rbac.authorization.k8s.io/v1beta1/definitions.html

+1-1
Original file line numberDiff line numberDiff line change
@@ -1682,7 +1682,7 @@ <h3 id="_v1beta1_policyrule">v1beta1.PolicyRule</h3>
16821682
</tr>
16831683
<tr>
16841684
<td class="tableblock halign-left valign-top"><p class="tableblock">resources</p></td>
1685-
<td class="tableblock halign-left valign-top"><p class="tableblock">Resources is a list of resources this rule applies to. ResourceAll represents all resources.</p></td>
1685+
<td class="tableblock halign-left valign-top"><p class="tableblock">Resources is a list of resources this rule applies to. <em><strong></em> represents all resources in the specified apiGroups. <em></strong>/foo</em> represents the subresource <em>foo</em> for all resources in the specified apiGroups.</p></td>
16861686
<td class="tableblock halign-left valign-top"><p class="tableblock">false</p></td>
16871687
<td class="tableblock halign-left valign-top"><p class="tableblock">string array</p></td>
16881688
<td class="tableblock halign-left valign-top"></td>

Diff for: ‎pkg/apis/authorization/types.go

+2-1
Original file line numberDiff line numberDiff line change
@@ -205,7 +205,8 @@ type ResourceRule struct {
205205
// APIGroups is the name of the APIGroup that contains the resources. If multiple API groups are specified, any action requested against one of
206206
// the enumerated resources in any API group will be allowed. "*" means all.
207207
APIGroups []string
208-
// Resources is a list of resources this rule applies to. ResourceAll represents all resources. "*" means all.
208+
// Resources is a list of resources this rule applies to. "*" means all in the specified apiGroups.
209+
// "*/foo" represents the subresource 'foo' for all resources in the specified apiGroups.
209210
Resources []string
210211
// ResourceNames is an optional white list of names that the rule applies to. An empty set means that everything is allowed. "*" means all.
211212
ResourceNames []string

Diff for: ‎pkg/apis/rbac/helpers.go

+17-2
Original file line numberDiff line numberDiff line change
@@ -55,14 +55,29 @@ func APIGroupMatches(rule *PolicyRule, requestedGroup string) bool {
5555
return false
5656
}
5757

58-
func ResourceMatches(rule *PolicyRule, requestedResource string) bool {
58+
func ResourceMatches(rule *PolicyRule, combinedRequestedResource, requestedSubresource string) bool {
5959
for _, ruleResource := range rule.Resources {
60+
// if everything is allowed, we match
6061
if ruleResource == ResourceAll {
6162
return true
6263
}
63-
if ruleResource == requestedResource {
64+
// if we have an exact match, we match
65+
if ruleResource == combinedRequestedResource {
6466
return true
6567
}
68+
69+
// We can also match a */subresource.
70+
// if there isn't a subresource, then continue
71+
if len(requestedSubresource) == 0 {
72+
continue
73+
}
74+
// if the rule isn't in the format */subresource, then we don't match, continue
75+
if len(ruleResource) == len(requestedSubresource)+2 &&
76+
strings.HasPrefix(ruleResource, "*/") &&
77+
strings.HasSuffix(ruleResource, requestedSubresource) {
78+
return true
79+
80+
}
6681
}
6782

6883
return false

Diff for: ‎pkg/apis/rbac/helpers_test.go

+105
Original file line numberDiff line numberDiff line change
@@ -70,3 +70,108 @@ func TestHelpersRoundTrip(t *testing.T) {
7070
}
7171
}
7272
}
73+
74+
func TestResourceMatches(t *testing.T) {
75+
tests := []struct {
76+
name string
77+
ruleResources []string
78+
combinedRequestedResource string
79+
requestedSubresource string
80+
expected bool
81+
}{
82+
{
83+
name: "all matches 01",
84+
ruleResources: []string{"*"},
85+
combinedRequestedResource: "foo",
86+
expected: true,
87+
},
88+
{
89+
name: "checks all rules",
90+
ruleResources: []string{"doesn't match", "*"},
91+
combinedRequestedResource: "foo",
92+
expected: true,
93+
},
94+
{
95+
name: "matches exact rule",
96+
ruleResources: []string{"foo/bar"},
97+
combinedRequestedResource: "foo/bar",
98+
requestedSubresource: "bar",
99+
expected: true,
100+
},
101+
{
102+
name: "matches exact rule 02",
103+
ruleResources: []string{"foo/bar"},
104+
combinedRequestedResource: "foo",
105+
expected: false,
106+
},
107+
{
108+
name: "matches subresource",
109+
ruleResources: []string{"*/scale"},
110+
combinedRequestedResource: "foo/scale",
111+
requestedSubresource: "scale",
112+
expected: true,
113+
},
114+
{
115+
name: "doesn't match partial subresource hit",
116+
ruleResources: []string{"foo/bar", "*/other"},
117+
combinedRequestedResource: "foo/other/segment",
118+
requestedSubresource: "other/segment",
119+
expected: false,
120+
},
121+
{
122+
name: "matches subresource with multiple slashes",
123+
ruleResources: []string{"*/other/segment"},
124+
combinedRequestedResource: "foo/other/segment",
125+
requestedSubresource: "other/segment",
126+
expected: true,
127+
},
128+
{
129+
name: "doesn't fail on empty",
130+
ruleResources: []string{""},
131+
combinedRequestedResource: "foo/other/segment",
132+
requestedSubresource: "other/segment",
133+
expected: false,
134+
},
135+
{
136+
name: "doesn't fail on slash",
137+
ruleResources: []string{"/"},
138+
combinedRequestedResource: "foo/other/segment",
139+
requestedSubresource: "other/segment",
140+
expected: false,
141+
},
142+
{
143+
name: "doesn't fail on missing subresource",
144+
ruleResources: []string{"*/"},
145+
combinedRequestedResource: "foo/other/segment",
146+
requestedSubresource: "other/segment",
147+
expected: false,
148+
},
149+
{
150+
name: "doesn't match on not star",
151+
ruleResources: []string{"*something/other/segment"},
152+
combinedRequestedResource: "foo/other/segment",
153+
requestedSubresource: "other/segment",
154+
expected: false,
155+
},
156+
{
157+
name: "doesn't match on something else",
158+
ruleResources: []string{"something/other/segment"},
159+
combinedRequestedResource: "foo/other/segment",
160+
requestedSubresource: "other/segment",
161+
expected: false,
162+
},
163+
}
164+
165+
for _, tc := range tests {
166+
t.Run(tc.name, func(t *testing.T) {
167+
rule := &rbac.PolicyRule{
168+
Resources: tc.ruleResources,
169+
}
170+
actual := rbac.ResourceMatches(rule, tc.combinedRequestedResource, tc.requestedSubresource)
171+
if tc.expected != actual {
172+
t.Errorf("expected %v, got %v", tc.expected, actual)
173+
}
174+
175+
})
176+
}
177+
}

Diff for: ‎pkg/apis/rbac/types.go

+2-1
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,8 @@ type PolicyRule struct {
4848
// APIGroups is the name of the APIGroup that contains the resources.
4949
// If multiple API groups are specified, any action requested against one of the enumerated resources in any API group will be allowed.
5050
APIGroups []string
51-
// Resources is a list of resources this rule applies to. ResourceAll represents all resources.
51+
// Resources is a list of resources this rule applies to. '*' represents all resources in the specified apiGroups.
52+
// '*/foo' represents the subresource 'foo' for all resources in the specified apiGroups.
5253
Resources []string
5354
// ResourceNames is an optional white list of names that the rule applies to. An empty set means that everything is allowed.
5455
ResourceNames []string

Diff for: ‎pkg/registry/rbac/validation/policy_comparator.go

+26-1
Original file line numberDiff line numberDiff line change
@@ -105,6 +105,31 @@ func hasAll(set, contains []string) bool {
105105
return true
106106
}
107107

108+
func resourceCoversAll(setResources, coversResources []string) bool {
109+
// if we have a star or an exact match on all resources, then we match
110+
if has(setResources, rbac.ResourceAll) || hasAll(setResources, coversResources) {
111+
return true
112+
}
113+
114+
for _, path := range coversResources {
115+
// if we have an exact match, then we match.
116+
if has(setResources, path) {
117+
continue
118+
}
119+
// if we're not a subresource, then we definitely don't match. fail.
120+
if !strings.Contains(path, "/") {
121+
return false
122+
}
123+
tokens := strings.SplitN(path, "/", 2)
124+
resourceToCheck := "*/" + tokens[1]
125+
if !has(setResources, resourceToCheck) {
126+
return false
127+
}
128+
}
129+
130+
return true
131+
}
132+
108133
func nonResourceURLsCoversAll(set, covers []string) bool {
109134
for _, path := range covers {
110135
covered := false
@@ -133,7 +158,7 @@ func nonResourceURLCovers(ownerPath, subPath string) bool {
133158
func ruleCovers(ownerRule, subRule rbac.PolicyRule) bool {
134159
verbMatches := has(ownerRule.Verbs, rbac.VerbAll) || hasAll(ownerRule.Verbs, subRule.Verbs)
135160
groupMatches := has(ownerRule.APIGroups, rbac.APIGroupAll) || hasAll(ownerRule.APIGroups, subRule.APIGroups)
136-
resourceMatches := has(ownerRule.Resources, rbac.ResourceAll) || hasAll(ownerRule.Resources, subRule.Resources)
161+
resourceMatches := resourceCoversAll(ownerRule.Resources, subRule.Resources)
137162
nonResourceURLMatches := nonResourceURLsCoversAll(ownerRule.NonResourceURLs, subRule.NonResourceURLs)
138163

139164
resourceNameMatches := false

Diff for: ‎pkg/registry/rbac/validation/policy_comparator_test.go

+14
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,20 @@ func TestCoversExactMatch(t *testing.T) {
4545
}.test(t)
4646
}
4747

48+
func TestCoversSubresourceWildcard(t *testing.T) {
49+
escalationTest{
50+
ownerRules: []rbac.PolicyRule{
51+
{APIGroups: []string{"v1"}, Verbs: []string{"get"}, Resources: []string{"*/scale"}},
52+
},
53+
servantRules: []rbac.PolicyRule{
54+
{APIGroups: []string{"v1"}, Verbs: []string{"get"}, Resources: []string{"foo/scale"}},
55+
},
56+
57+
expectedCovered: true,
58+
expectedUncoveredRules: []rbac.PolicyRule{},
59+
}.test(t)
60+
}
61+
4862
func TestCoversMultipleRulesCoveringSingleRule(t *testing.T) {
4963
escalationTest{
5064
ownerRules: []rbac.PolicyRule{

Diff for: ‎plugin/pkg/auth/authorizer/rbac/bootstrappolicy/controller_policy.go

+1-4
Original file line numberDiff line numberDiff line change
@@ -156,10 +156,7 @@ func buildControllerRoles() ([]rbac.ClusterRole, []rbac.ClusterRoleBinding) {
156156
Rules: []rbac.PolicyRule{
157157
rbac.NewRule("get", "list", "watch").Groups(autoscalingGroup).Resources("horizontalpodautoscalers").RuleOrDie(),
158158
rbac.NewRule("update").Groups(autoscalingGroup).Resources("horizontalpodautoscalers/status").RuleOrDie(),
159-
rbac.NewRule("get", "update").Groups(legacyGroup).Resources("replicationcontrollers/scale").RuleOrDie(),
160-
// TODO this should be removable when the HPA contoller is fixed
161-
rbac.NewRule("get", "update").Groups(extensionsGroup).Resources("replicationcontrollers/scale").RuleOrDie(),
162-
rbac.NewRule("get", "update").Groups(extensionsGroup, appsGroup).Resources("deployments/scale", "replicasets/scale").RuleOrDie(),
159+
rbac.NewRule("get", "update").Groups("*").Resources("*/scale").RuleOrDie(),
163160
rbac.NewRule("list").Groups(legacyGroup).Resources("pods").RuleOrDie(),
164161
// TODO: restrict this to the appropriate namespace
165162
rbac.NewRule("get").Groups(legacyGroup).Resources("services/proxy").Names("https:heapster:", "http:heapster:").RuleOrDie(),

Diff for: ‎plugin/pkg/auth/authorizer/rbac/bootstrappolicy/testdata/controller-roles.yaml

+2-18
Original file line numberDiff line numberDiff line change
@@ -445,25 +445,9 @@ items:
445445
verbs:
446446
- update
447447
- apiGroups:
448-
- ""
449-
resources:
450-
- replicationcontrollers/scale
451-
verbs:
452-
- get
453-
- update
454-
- apiGroups:
455-
- extensions
456-
resources:
457-
- replicationcontrollers/scale
458-
verbs:
459-
- get
460-
- update
461-
- apiGroups:
462-
- apps
463-
- extensions
448+
- '*'
464449
resources:
465-
- deployments/scale
466-
- replicasets/scale
450+
- '*/scale'
467451
verbs:
468452
- get
469453
- update

Diff for: ‎plugin/pkg/auth/authorizer/rbac/rbac.go

+3-3
Original file line numberDiff line numberDiff line change
@@ -174,14 +174,14 @@ func RulesAllow(requestAttributes authorizer.Attributes, rules ...rbac.PolicyRul
174174

175175
func RuleAllows(requestAttributes authorizer.Attributes, rule *rbac.PolicyRule) bool {
176176
if requestAttributes.IsResourceRequest() {
177-
resource := requestAttributes.GetResource()
177+
combinedResource := requestAttributes.GetResource()
178178
if len(requestAttributes.GetSubresource()) > 0 {
179-
resource = requestAttributes.GetResource() + "/" + requestAttributes.GetSubresource()
179+
combinedResource = requestAttributes.GetResource() + "/" + requestAttributes.GetSubresource()
180180
}
181181

182182
return rbac.VerbMatches(rule, requestAttributes.GetVerb()) &&
183183
rbac.APIGroupMatches(rule, requestAttributes.GetAPIGroup()) &&
184-
rbac.ResourceMatches(rule, resource) &&
184+
rbac.ResourceMatches(rule, combinedResource, requestAttributes.GetSubresource()) &&
185185
rbac.ResourceNameMatches(rule, requestAttributes.GetName())
186186
}
187187

Diff for: ‎plugin/pkg/auth/authorizer/rbac/rbac_test.go

+7-1
Original file line numberDiff line numberDiff line change
@@ -224,13 +224,19 @@ func TestAuthorizer(t *testing.T) {
224224
{
225225
// test subresource resolution
226226
clusterRoles: []*rbac.ClusterRole{
227-
newClusterRole("admin", newRule("*", "*", "pods/status", "*")),
227+
newClusterRole("admin",
228+
newRule("*", "*", "pods/status", "*"),
229+
newRule("*", "*", "*/scale", "*"),
230+
),
228231
},
229232
roleBindings: []*rbac.RoleBinding{
230233
newRoleBinding("ns1", "admin", bindToClusterRole, "User:admin", "Group:admins"),
231234
},
232235
shouldPass: []authorizer.Attributes{
233236
&defaultAttributes{"admin", "", "get", "pods", "status", "ns1", ""},
237+
&defaultAttributes{"admin", "", "get", "pods", "scale", "ns1", ""},
238+
&defaultAttributes{"admin", "", "get", "deployments", "scale", "ns1", ""},
239+
&defaultAttributes{"admin", "", "get", "anything", "scale", "ns1", ""},
234240
},
235241
shouldFail: []authorizer.Attributes{
236242
&defaultAttributes{"admin", "", "get", "pods", "", "ns1", ""},

Diff for: ‎staging/src/k8s.io/api/authorization/v1/generated.proto

+2-1
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)