Skip to content

Commit 52c9149

Browse files
author
OpenShift Bot
authored
Merge pull request #13415 from enj/enj/i/covers_attribute_restrictions
Merged by openshift-bot
2 parents bd0bf10 + 8c66c8d commit 52c9149

File tree

6 files changed

+125
-28
lines changed

6 files changed

+125
-28
lines changed

‎pkg/authorization/authorizer/scope/converter.go

-1
Original file line numberDiff line numberDiff line change
@@ -177,7 +177,6 @@ func (userEvaluator) ResolveRules(scope, namespace string, clusterPolicyGetter c
177177
case UserAccessCheck:
178178
return []authorizationapi.PolicyRule{
179179
authorizationapi.NewRule("create").Groups(kauthorizationapi.GroupName).Resources("selfsubjectaccessreviews").RuleOrDie(),
180-
{Verbs: sets.NewString("create"), APIGroups: []string{authorizationapi.GroupName, authorizationapi.LegacyGroupName}, Resources: sets.NewString("subjectaccessreviews", "localsubjectaccessreviews"), AttributeRestrictions: &authorizationapi.IsPersonalSubjectAccessReview{}},
181180
authorizationapi.NewRule("create").Groups(authorizationapi.GroupName, authorizationapi.LegacyGroupName).Resources("selfsubjectrulesreviews").RuleOrDie(),
182181
}, nil
183182
case UserListScopedProjects:

‎pkg/authorization/authorizer/scope/converter_test.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -45,12 +45,12 @@ func TestUserEvaluator(t *testing.T) {
4545
{
4646
name: "access",
4747
scopes: []string{UserAccessCheck},
48-
numRules: 4,
48+
numRules: 3,
4949
},
5050
{
5151
name: "both",
5252
scopes: []string{UserInfo, UserAccessCheck},
53-
numRules: 5,
53+
numRules: 4,
5454
},
5555
{
5656
name: "list--scoped-projects",

‎pkg/authorization/rulevalidation/policy_comparator.go

+11-1
Original file line numberDiff line numberDiff line change
@@ -47,11 +47,16 @@ func Covers(ownerRules, servantRules []authorizationapi.PolicyRule) (bool, []aut
4747
func BreakdownRule(rule authorizationapi.PolicyRule) []authorizationapi.PolicyRule {
4848
subrules := []authorizationapi.PolicyRule{}
4949

50+
// A rule with an attribute restriction is ignored and thus is the same as having no rule at all
51+
if rule.AttributeRestrictions != nil {
52+
return subrules
53+
}
54+
5055
for _, group := range rule.APIGroups {
5156
subrules = append(subrules, breakdownRuleForGroup(group, rule)...)
5257
}
5358

54-
// if no groups are present, then the default group is assumed. Buidl the subrules, then strip the groups
59+
// if no groups are present, then the default group is assumed. Build the subrules, then strip the groups
5560
if len(rule.APIGroups) == 0 {
5661
for _, subrule := range breakdownRuleForGroup("", rule) {
5762
subrule.APIGroups = nil
@@ -91,6 +96,11 @@ func breakdownRuleForGroup(group string, rule authorizationapi.PolicyRule) []aut
9196
// ruleCovers determines whether the ownerRule (which may have multiple verbs, resources, and resourceNames) covers
9297
// the subrule (which may only contain at most one verb, resource, and resourceName)
9398
func ruleCovers(ownerRule, subrule authorizationapi.PolicyRule) bool {
99+
// A rule with an attribute restriction is ignored and thus it can never cover another rule
100+
if ownerRule.AttributeRestrictions != nil {
101+
return false
102+
}
103+
94104
allResources := authorizationapi.NormalizeResources(ownerRule.Resources)
95105

96106
ownerGroups := sets.NewString(ownerRule.APIGroups...)

‎pkg/authorization/rulevalidation/policy_comparator_test.go

+112
Original file line numberDiff line numberDiff line change
@@ -411,6 +411,118 @@ func TestMixedResourceNonResourceUncovered(t *testing.T) {
411411
}.test(t)
412412
}
413413

414+
func TestAttributeRestrictionsCovering(t *testing.T) {
415+
escalationTest{
416+
ownerRules: []authorizationapi.PolicyRule{
417+
{Verbs: sets.NewString("create"), Resources: sets.NewString("builds"), AttributeRestrictions: &authorizationapi.IsPersonalSubjectAccessReview{}},
418+
},
419+
servantRules: []authorizationapi.PolicyRule{
420+
{Verbs: sets.NewString("create"), Resources: sets.NewString("builds"), AttributeRestrictions: &authorizationapi.Role{}},
421+
},
422+
423+
expectedCovered: true,
424+
expectedUncoveredRules: []authorizationapi.PolicyRule{},
425+
}.test(t)
426+
escalationTest{
427+
ownerRules: []authorizationapi.PolicyRule{
428+
{Verbs: sets.NewString("create"), Resources: sets.NewString("builds")},
429+
},
430+
servantRules: []authorizationapi.PolicyRule{
431+
{Verbs: sets.NewString("create"), Resources: sets.NewString("builds"), AttributeRestrictions: &authorizationapi.Role{}},
432+
},
433+
434+
expectedCovered: true,
435+
expectedUncoveredRules: []authorizationapi.PolicyRule{},
436+
}.test(t)
437+
escalationTest{
438+
ownerRules: []authorizationapi.PolicyRule{
439+
{Verbs: sets.NewString("create"), Resources: sets.NewString("builds"), AttributeRestrictions: &authorizationapi.IsPersonalSubjectAccessReview{}},
440+
{Verbs: sets.NewString("update"), Resources: sets.NewString("builds"), AttributeRestrictions: &authorizationapi.Role{}},
441+
},
442+
servantRules: []authorizationapi.PolicyRule{},
443+
444+
expectedCovered: true,
445+
expectedUncoveredRules: []authorizationapi.PolicyRule{},
446+
}.test(t)
447+
escalationTest{
448+
ownerRules: []authorizationapi.PolicyRule{},
449+
servantRules: []authorizationapi.PolicyRule{
450+
{Verbs: sets.NewString("create"), Resources: sets.NewString("builds"), AttributeRestrictions: &authorizationapi.Role{}},
451+
{Verbs: sets.NewString("update"), Resources: sets.NewString("builds"), AttributeRestrictions: &authorizationapi.IsPersonalSubjectAccessReview{}},
452+
},
453+
454+
expectedCovered: true,
455+
expectedUncoveredRules: []authorizationapi.PolicyRule{},
456+
}.test(t)
457+
escalationTest{
458+
ownerRules: []authorizationapi.PolicyRule{
459+
{Verbs: sets.NewString("create"), Resources: sets.NewString("pods")},
460+
},
461+
servantRules: []authorizationapi.PolicyRule{
462+
{Verbs: sets.NewString("create"), Resources: sets.NewString("builds"), AttributeRestrictions: &authorizationapi.Role{}},
463+
{Verbs: sets.NewString("update"), Resources: sets.NewString("builds"), AttributeRestrictions: &authorizationapi.IsPersonalSubjectAccessReview{}},
464+
{Verbs: sets.NewString("delete"), Resources: sets.NewString("builds"), AttributeRestrictions: &authorizationapi.ClusterRole{}},
465+
{Verbs: sets.NewString("impersonate"), Resources: sets.NewString("builds"), AttributeRestrictions: &authorizationapi.ClusterPolicyBinding{}},
466+
},
467+
468+
expectedCovered: true,
469+
expectedUncoveredRules: []authorizationapi.PolicyRule{},
470+
}.test(t)
471+
escalationTest{
472+
ownerRules: []authorizationapi.PolicyRule{
473+
{Verbs: sets.NewString(authorizationapi.VerbAll), Resources: sets.NewString(authorizationapi.ResourceAll), AttributeRestrictions: &authorizationapi.Role{}},
474+
},
475+
servantRules: []authorizationapi.PolicyRule{
476+
{Verbs: sets.NewString("create"), Resources: sets.NewString("builds"), AttributeRestrictions: &authorizationapi.Role{}},
477+
{Verbs: sets.NewString("update"), Resources: sets.NewString("builds"), AttributeRestrictions: &authorizationapi.IsPersonalSubjectAccessReview{}},
478+
{Verbs: sets.NewString("delete"), Resources: sets.NewString("builds"), AttributeRestrictions: &authorizationapi.ClusterRole{}},
479+
{Verbs: sets.NewString("impersonate"), Resources: sets.NewString("builds"), AttributeRestrictions: &authorizationapi.ClusterPolicyBinding{}},
480+
},
481+
482+
expectedCovered: true,
483+
expectedUncoveredRules: []authorizationapi.PolicyRule{},
484+
}.test(t)
485+
escalationTest{
486+
ownerRules: []authorizationapi.PolicyRule{
487+
{Verbs: sets.NewString("create"), Resources: sets.NewString("builds"), AttributeRestrictions: &authorizationapi.IsPersonalSubjectAccessReview{}},
488+
},
489+
servantRules: []authorizationapi.PolicyRule{
490+
{Verbs: sets.NewString("create"), Resources: sets.NewString("builds")},
491+
},
492+
493+
expectedCovered: false,
494+
expectedUncoveredRules: []authorizationapi.PolicyRule{
495+
{Verbs: sets.NewString("create"), Resources: sets.NewString("builds")},
496+
},
497+
}.test(t)
498+
escalationTest{
499+
ownerRules: []authorizationapi.PolicyRule{
500+
{Verbs: sets.NewString("delete"), Resources: sets.NewString("builds"), AttributeRestrictions: &authorizationapi.Role{}},
501+
},
502+
servantRules: []authorizationapi.PolicyRule{
503+
{Verbs: sets.NewString("delete"), Resources: sets.NewString("builds")},
504+
},
505+
506+
expectedCovered: false,
507+
expectedUncoveredRules: []authorizationapi.PolicyRule{
508+
{Verbs: sets.NewString("delete"), Resources: sets.NewString("builds")},
509+
},
510+
}.test(t)
511+
escalationTest{
512+
ownerRules: []authorizationapi.PolicyRule{
513+
{Verbs: sets.NewString(authorizationapi.VerbAll), Resources: sets.NewString(authorizationapi.ResourceAll), AttributeRestrictions: &authorizationapi.IsPersonalSubjectAccessReview{}},
514+
},
515+
servantRules: []authorizationapi.PolicyRule{
516+
{Verbs: sets.NewString("delete"), Resources: sets.NewString("builds")},
517+
},
518+
519+
expectedCovered: false,
520+
expectedUncoveredRules: []authorizationapi.PolicyRule{
521+
{Verbs: sets.NewString("delete"), Resources: sets.NewString("builds")},
522+
},
523+
}.test(t)
524+
}
525+
414526
func (test escalationTest) test(t *testing.T) {
415527
actualCovered, actualUncoveredRules := Covers(test.ownerRules, test.servantRules)
416528

‎pkg/cmd/server/bootstrappolicy/policy.go

-2
Original file line numberDiff line numberDiff line change
@@ -476,7 +476,6 @@ func GetBootstrapClusterRoles() []authorizationapi.ClusterRole {
476476
authorizationapi.NewRule("list").Groups(storageGroup).Resources("storageclasses").RuleOrDie(),
477477
authorizationapi.NewRule("list", "watch").Groups(projectGroup, legacyProjectGroup).Resources("projects").RuleOrDie(),
478478
authorizationapi.NewRule("create").Groups(authzGroup, legacyAuthzGroup).Resources("selfsubjectrulesreviews").RuleOrDie(),
479-
{Verbs: sets.NewString("create"), APIGroups: []string{authzGroup, legacyAuthzGroup}, Resources: sets.NewString("subjectaccessreviews", "localsubjectaccessreviews"), AttributeRestrictions: &authorizationapi.IsPersonalSubjectAccessReview{}},
480479
authorizationapi.NewRule("create").Groups(kAuthzGroup).Resources("selfsubjectaccessreviews").RuleOrDie(),
481480
},
482481
},
@@ -489,7 +488,6 @@ func GetBootstrapClusterRoles() []authorizationapi.ClusterRole {
489488
},
490489
Rules: []authorizationapi.PolicyRule{
491490
authorizationapi.NewRule("create").Groups(authzGroup, legacyAuthzGroup).Resources("selfsubjectrulesreviews").RuleOrDie(),
492-
{Verbs: sets.NewString("create"), APIGroups: []string{authzGroup, legacyAuthzGroup}, Resources: sets.NewString("subjectaccessreviews", "localsubjectaccessreviews"), AttributeRestrictions: &authorizationapi.IsPersonalSubjectAccessReview{}},
493491
authorizationapi.NewRule("create").Groups(kAuthzGroup).Resources("selfsubjectaccessreviews").RuleOrDie(),
494492
},
495493
},

‎test/testdata/bootstrappolicy/bootstrap_cluster_roles.yaml

-22
Original file line numberDiff line numberDiff line change
@@ -1630,17 +1630,6 @@ items:
16301630
- selfsubjectrulesreviews
16311631
verbs:
16321632
- create
1633-
- apiGroups:
1634-
- authorization.openshift.io
1635-
- ""
1636-
attributeRestrictions:
1637-
apiVersion: authorization.openshift.io/v1
1638-
kind: IsPersonalSubjectAccessReview
1639-
resources:
1640-
- localsubjectaccessreviews
1641-
- subjectaccessreviews
1642-
verbs:
1643-
- create
16441633
- apiGroups:
16451634
- authorization.k8s.io
16461635
attributeRestrictions: null
@@ -1664,17 +1653,6 @@ items:
16641653
- selfsubjectrulesreviews
16651654
verbs:
16661655
- create
1667-
- apiGroups:
1668-
- authorization.openshift.io
1669-
- ""
1670-
attributeRestrictions:
1671-
apiVersion: authorization.openshift.io/v1
1672-
kind: IsPersonalSubjectAccessReview
1673-
resources:
1674-
- localsubjectaccessreviews
1675-
- subjectaccessreviews
1676-
verbs:
1677-
- create
16781656
- apiGroups:
16791657
- authorization.k8s.io
16801658
attributeRestrictions: null

0 commit comments

Comments
 (0)