Skip to content

Commit d99daea

Browse files
openshift-merge-robotopenshift-merge-robot
authored
Merge pull request #514 from njhale/non-resource-perms
fix(requirements): add support for non resource url rules
2 parents 4fc5b9f + ad76da4 commit d99daea

File tree

8 files changed

+416
-36
lines changed

8 files changed

+416
-36
lines changed

‎deploy/chart/templates/0000_30_02-clusterserviceversion.crd.yaml

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -653,8 +653,6 @@ spec:
653653
- create
654654
- update
655655
- patch
656-
- put
657-
- post
658656
- delete
659657
- deletecollection
660658
- initialize
@@ -674,6 +672,8 @@ spec:
674672
type: array
675673
items:
676674
type: object
675+
required:
676+
- verbs
677677
description: a rule required by the service account
678678
properties:
679679
apiGroups:
@@ -689,6 +689,10 @@ spec:
689689
type: array
690690
items:
691691
type: string
692+
nonResourceURLs:
693+
type: array
694+
items:
695+
type: string
692696
verbs:
693697
type: array
694698
items:

‎pkg/controller/install/attributes_util.go

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -9,10 +9,12 @@ import (
99
"k8s.io/apiserver/pkg/authorization/authorizer"
1010
)
1111

12+
// toAttributesSet converts the given user, namespace, and PolicyRule into a set of Attributes expected. This is useful for checking
13+
// if a composed set of Roles/RoleBindings satisfies a PolicyRule.
1214
func toAttributesSet(user user.Info, namespace string, rule rbacv1.PolicyRule) []authorizer.Attributes {
1315
set := map[authorizer.AttributesRecord]struct{}{}
1416

15-
// add empty string for empty groups, resources, and resource names
17+
// add empty string for empty groups, resources, resource names, and non resource urls
1618
groups := rule.APIGroups
1719
if len(groups) == 0 {
1820
groups = make([]string, 1)
@@ -25,12 +27,18 @@ func toAttributesSet(user user.Info, namespace string, rule rbacv1.PolicyRule) [
2527
if len(names) == 0 {
2628
names = make([]string, 1)
2729
}
30+
nonResourceURLs := rule.NonResourceURLs
31+
if len(nonResourceURLs) == 0 {
32+
nonResourceURLs = make([]string, 1)
33+
}
2834

2935
for _, verb := range rule.Verbs {
3036
for _, group := range groups {
3137
for _, resource := range resources {
3238
for _, name := range names {
33-
set[attributesRecord(user, namespace, verb, group, resource, name)] = struct{}{}
39+
for _, nonResourceURL := range nonResourceURLs {
40+
set[attributesRecord(user, namespace, verb, group, resource, name, nonResourceURL)] = struct{}{}
41+
}
3442
}
3543
}
3644
}
@@ -48,15 +56,17 @@ func toAttributesSet(user user.Info, namespace string, rule rbacv1.PolicyRule) [
4856
}
4957

5058
// attribute creates a new AttributesRecord with the given info. Currently RBAC authz only looks at user, verb, apiGroup, resource, and name.
51-
func attributesRecord(user user.Info, namespace, verb, apiGroup, resource, name string) authorizer.AttributesRecord {
59+
func attributesRecord(user user.Info, namespace, verb, apiGroup, resource, name, path string) authorizer.AttributesRecord {
60+
resourceRequest := path == ""
5261
return authorizer.AttributesRecord{
5362
User: user,
5463
Verb: verb,
5564
Namespace: namespace,
5665
APIGroup: apiGroup,
5766
Resource: resource,
5867
Name: name,
59-
ResourceRequest: true,
68+
ResourceRequest: resourceRequest,
69+
Path: path,
6070
}
6171
}
6272

‎pkg/controller/install/attributes_util_test.go

Lines changed: 40 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,17 @@ func TestToAttributeSet(t *testing.T) {
2929
Resources: []string{"*"},
3030
},
3131
expectedAttributes: map[authorizer.AttributesRecord]struct{}{
32-
attributesRecord(user, namespace, "*", "*", "*", ""): {},
32+
attributesRecord(user, namespace, "*", "*", "*", "", ""): {},
33+
},
34+
},
35+
{
36+
description: "SimpleNonResourceRule",
37+
rule: rbacv1.PolicyRule{
38+
Verbs: []string{"*"},
39+
NonResourceURLs: []string{"/api"},
40+
},
41+
expectedAttributes: map[authorizer.AttributesRecord]struct{}{
42+
attributesRecord(user, namespace, "*", "", "", "", "/api"): {},
3343
},
3444
},
3545
{
@@ -40,8 +50,8 @@ func TestToAttributeSet(t *testing.T) {
4050
Resources: []string{"*"},
4151
},
4252
expectedAttributes: map[authorizer.AttributesRecord]struct{}{
43-
attributesRecord(user, namespace, "create", "*", "*", ""): {},
44-
attributesRecord(user, namespace, "delete", "*", "*", ""): {},
53+
attributesRecord(user, namespace, "create", "*", "*", "", ""): {},
54+
attributesRecord(user, namespace, "delete", "*", "*", "", ""): {},
4555
},
4656
},
4757
{
@@ -51,10 +61,21 @@ func TestToAttributeSet(t *testing.T) {
5161
Resources: []string{"donuts", "coffee"},
5262
},
5363
expectedAttributes: map[authorizer.AttributesRecord]struct{}{
54-
attributesRecord(user, namespace, "get", "", "donuts", ""): {},
55-
attributesRecord(user, namespace, "update", "", "donuts", ""): {},
56-
attributesRecord(user, namespace, "get", "", "coffee", ""): {},
57-
attributesRecord(user, namespace, "update", "", "coffee", ""): {},
64+
attributesRecord(user, namespace, "get", "", "donuts", "", ""): {},
65+
attributesRecord(user, namespace, "update", "", "donuts", "", ""): {},
66+
attributesRecord(user, namespace, "get", "", "coffee", "", ""): {},
67+
attributesRecord(user, namespace, "update", "", "coffee", "", ""): {},
68+
},
69+
},
70+
{
71+
description: "MultipleNonResourceURLs",
72+
rule: rbacv1.PolicyRule{
73+
Verbs: []string{"*"},
74+
NonResourceURLs: []string{"/capybaras", "/caviidaes"},
75+
},
76+
expectedAttributes: map[authorizer.AttributesRecord]struct{}{
77+
attributesRecord(user, namespace, "*", "", "", "", "/capybaras"): {},
78+
attributesRecord(user, namespace, "*", "", "", "", "/caviidaes"): {},
5879
},
5980
},
6081
{
@@ -65,10 +86,10 @@ func TestToAttributeSet(t *testing.T) {
6586
ResourceNames: []string{"nyc"},
6687
},
6788
expectedAttributes: map[authorizer.AttributesRecord]struct{}{
68-
attributesRecord(user, namespace, "get", "", "donuts", "nyc"): {},
69-
attributesRecord(user, namespace, "update", "", "donuts", "nyc"): {},
70-
attributesRecord(user, namespace, "get", "", "coffee", "nyc"): {},
71-
attributesRecord(user, namespace, "update", "", "coffee", "nyc"): {},
89+
attributesRecord(user, namespace, "get", "", "donuts", "nyc", ""): {},
90+
attributesRecord(user, namespace, "update", "", "donuts", "nyc", ""): {},
91+
attributesRecord(user, namespace, "get", "", "coffee", "nyc", ""): {},
92+
attributesRecord(user, namespace, "update", "", "coffee", "nyc", ""): {},
7293
},
7394
},
7495
{
@@ -80,14 +101,14 @@ func TestToAttributeSet(t *testing.T) {
80101
ResourceNames: []string{"nyc"},
81102
},
82103
expectedAttributes: map[authorizer.AttributesRecord]struct{}{
83-
attributesRecord(user, namespace, "get", "apps.coreos.com", "donuts", "nyc"): {},
84-
attributesRecord(user, namespace, "update", "apps.coreos.com", "donuts", "nyc"): {},
85-
attributesRecord(user, namespace, "get", "apps.coreos.com", "coffee", "nyc"): {},
86-
attributesRecord(user, namespace, "update", "apps.coreos.com", "coffee", "nyc"): {},
87-
attributesRecord(user, namespace, "get", "apps.redhat.com", "donuts", "nyc"): {},
88-
attributesRecord(user, namespace, "update", "apps.redhat.com", "donuts", "nyc"): {},
89-
attributesRecord(user, namespace, "get", "apps.redhat.com", "coffee", "nyc"): {},
90-
attributesRecord(user, namespace, "update", "apps.redhat.com", "coffee", "nyc"): {},
104+
attributesRecord(user, namespace, "get", "apps.coreos.com", "donuts", "nyc", ""): {},
105+
attributesRecord(user, namespace, "update", "apps.coreos.com", "donuts", "nyc", ""): {},
106+
attributesRecord(user, namespace, "get", "apps.coreos.com", "coffee", "nyc", ""): {},
107+
attributesRecord(user, namespace, "update", "apps.coreos.com", "coffee", "nyc", ""): {},
108+
attributesRecord(user, namespace, "get", "apps.redhat.com", "donuts", "nyc", ""): {},
109+
attributesRecord(user, namespace, "update", "apps.redhat.com", "donuts", "nyc", ""): {},
110+
attributesRecord(user, namespace, "get", "apps.redhat.com", "coffee", "nyc", ""): {},
111+
attributesRecord(user, namespace, "update", "apps.redhat.com", "coffee", "nyc", ""): {},
91112
},
92113
},
93114
{

‎pkg/controller/install/rule_checker.go

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
package install
22

33
import (
4+
"fmt"
5+
46
corev1 "k8s.io/api/core/v1"
57
rbacv1 "k8s.io/api/rbac/v1"
68
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -42,6 +44,12 @@ func NewCSVRuleChecker(roleLister crbacv1.RoleLister, roleBindingLister crbacv1.
4244

4345
// RuleSatisfied returns true if a ServiceAccount is authorized to perform all actions described by a PolicyRule in a namespace
4446
func (c *CSVRuleChecker) RuleSatisfied(sa *corev1.ServiceAccount, namespace string, rule rbacv1.PolicyRule) (bool, error) {
47+
// check if the rule is valid
48+
err := ruleValid(rule)
49+
if err != nil {
50+
return false, fmt.Errorf("rule invalid: %s", err.Error())
51+
}
52+
4553
// get attributes set for the given Role and ServiceAccount
4654
user := toDefaultInfo(sa)
4755
attributesSet := toAttributesSet(user, namespace, rule)
@@ -155,3 +163,16 @@ func (c *CSVRuleChecker) hasOwnerConflicts(ownerRefs []metav1.OwnerReference) bo
155163

156164
return conflicts
157165
}
166+
167+
func ruleValid(rule rbacv1.PolicyRule) error {
168+
if len(rule.Verbs) == 0 {
169+
return fmt.Errorf("policy rule must have at least one verb")
170+
}
171+
172+
resourceCount := len(rule.APIGroups) + len(rule.Resources) + len(rule.ResourceNames)
173+
if resourceCount > 0 && len(rule.NonResourceURLs) > 0 {
174+
return fmt.Errorf("rule cannot apply to both regular resources and non-resource URLs")
175+
}
176+
177+
return nil
178+
}

‎pkg/controller/operators/olm/operator_test.go

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import (
1313
"github.com/stretchr/testify/require"
1414
appsv1 "k8s.io/api/apps/v1"
1515
"k8s.io/api/core/v1"
16+
rbacv1 "k8s.io/api/rbac/v1"
1617
"k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1"
1718
apiextensionsfake "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset/fake"
1819
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -393,6 +394,50 @@ func TestTransitionCSV(t *testing.T) {
393394
},
394395
},
395396
},
397+
{
398+
name: "SingleCSVPendingToFailed/BadStrategyPermissions",
399+
initial: initial{
400+
csvs: []runtime.Object{
401+
csv("csv1",
402+
namespace,
403+
"",
404+
installStrategy("csv1-dep1",
405+
nil,
406+
[]install.StrategyDeploymentPermissions{
407+
{
408+
ServiceAccountName: "sa",
409+
Rules: []rbacv1.PolicyRule{
410+
{
411+
Verbs: []string{"*"},
412+
Resources: []string{"*"},
413+
NonResourceURLs: []string{"/osb"},
414+
},
415+
},
416+
},
417+
}),
418+
[]*v1beta1.CustomResourceDefinition{crd("c1", "v1")},
419+
[]*v1beta1.CustomResourceDefinition{},
420+
v1alpha1.CSVPhasePending,
421+
),
422+
},
423+
crds: []runtime.Object{
424+
crd("c1", "v1"),
425+
},
426+
objs: []runtime.Object{
427+
&v1.ServiceAccount{
428+
ObjectMeta: metav1.ObjectMeta{
429+
Name: "sa",
430+
Namespace: namespace,
431+
},
432+
},
433+
},
434+
},
435+
expected: expected{
436+
csvStates: map[string]csvState{
437+
"csv1": {exists: true, phase: v1alpha1.CSVPhaseFailed},
438+
},
439+
},
440+
},
396441
{
397442
name: "SingleCSVPendingToPending/CRD",
398443
initial: initial{

‎pkg/controller/operators/olm/requirements.go

Lines changed: 54 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -30,11 +30,35 @@ func (a *Operator) requirementStatus(strategyDetailsDeployment *install.Strategy
3030
if err != nil {
3131
status.Status = v1alpha1.RequirementStatusReasonNotPresent
3232
met = false
33-
} else {
33+
statuses = append(statuses, status)
34+
continue
35+
}
36+
37+
if crd.Spec.Version == r.Version {
3438
status.Status = v1alpha1.RequirementStatusReasonPresent
3539
status.UUID = string(crd.GetUID())
40+
statuses = append(statuses, status)
41+
continue
42+
}
43+
44+
served := false
45+
for _, version := range crd.Spec.Versions {
46+
if version.Name == r.Version {
47+
if version.Served {
48+
status.Status = v1alpha1.RequirementStatusReasonPresent
49+
status.UUID = string(crd.GetUID())
50+
statuses = append(statuses, status)
51+
served = true
52+
}
53+
break
54+
}
55+
}
56+
57+
if !served {
58+
status.Status = v1alpha1.RequirementStatusReasonNotPresent
59+
met = false
60+
statuses = append(statuses, status)
3661
}
37-
statuses = append(statuses, status)
3862
}
3963

4064
// Check for required API services
@@ -106,11 +130,11 @@ func (a *Operator) requirementStatus(strategyDetailsDeployment *install.Strategy
106130
}
107131

108132
// permissionStatus checks whether the given CSV's RBAC requirements are met in its namespace
109-
func (a *Operator) permissionStatus(strategyDetailsDeployment *install.StrategyDetailsDeployment, ruleChecker install.RuleChecker, csvNamespace string) (bool, []v1alpha1.RequirementStatus) {
133+
func (a *Operator) permissionStatus(strategyDetailsDeployment *install.StrategyDetailsDeployment, ruleChecker install.RuleChecker, csvNamespace string) (bool, []v1alpha1.RequirementStatus, error) {
110134
statusesSet := map[string]v1alpha1.RequirementStatus{}
111135
met := true
112136

113-
checkPermissions := func(permissions []install.StrategyDeploymentPermissions, namespace string) {
137+
checkPermissions := func(permissions []install.StrategyDeploymentPermissions, namespace string) error {
114138
for _, perm := range permissions {
115139
saName := perm.ServiceAccountName
116140
log.Debugf("perm.ServiceAccountName: %s", saName)
@@ -153,10 +177,19 @@ func (a *Operator) permissionStatus(strategyDetailsDeployment *install.StrategyD
153177
status.Dependents = append(status.Dependents, dependent)
154178
continue
155179
}
156-
dependent.Message = fmt.Sprintf("rule raw:%s", marshalled)
180+
181+
var scope string
182+
if namespace == metav1.NamespaceAll {
183+
scope = "cluster"
184+
} else {
185+
scope = "namespaced"
186+
}
187+
dependent.Message = fmt.Sprintf("%s rule:%s", scope, marshalled)
157188

158189
satisfied, err := ruleChecker.RuleSatisfied(sa, namespace, rule)
159-
if err != nil || !satisfied {
190+
if err != nil {
191+
return err
192+
} else if !satisfied {
160193
met = false
161194
dependent.Status = v1alpha1.DependentStatusReasonNotSatisfied
162195
status.Status = v1alpha1.RequirementStatusReasonPresentNotSatisfied
@@ -169,18 +202,26 @@ func (a *Operator) permissionStatus(strategyDetailsDeployment *install.StrategyD
169202

170203
statusesSet[saName] = status
171204
}
205+
206+
return nil
172207
}
173208

174-
checkPermissions(strategyDetailsDeployment.Permissions, csvNamespace)
175-
checkPermissions(strategyDetailsDeployment.ClusterPermissions, metav1.NamespaceAll)
209+
err := checkPermissions(strategyDetailsDeployment.Permissions, csvNamespace)
210+
if err != nil {
211+
return false, nil, err
212+
}
213+
err = checkPermissions(strategyDetailsDeployment.ClusterPermissions, metav1.NamespaceAll)
214+
if err != nil {
215+
return false, nil, err
216+
}
176217

177218
statuses := []v1alpha1.RequirementStatus{}
178219
for key, status := range statusesSet {
179220
log.Debugf("appending permission status: %s", key)
180221
statuses = append(statuses, status)
181222
}
182223

183-
return met, statuses
224+
return met, statuses, nil
184225
}
185226

186227
// requirementAndPermissionStatus returns the aggregate requirement and permissions statuses for the given CSV
@@ -201,7 +242,10 @@ func (a *Operator) requirementAndPermissionStatus(csv *v1alpha1.ClusterServiceVe
201242
reqMet, reqStatuses := a.requirementStatus(strategyDetailsDeployment, csv.GetAllCRDDescriptions(), csv.GetOwnedAPIServiceDescriptions(), csv.GetRequiredAPIServiceDescriptions())
202243

203244
ruleChecker := install.NewCSVRuleChecker(a.roleLister, a.roleBindingLister, a.clusterRoleLister, a.clusterRoleBindingLister, csv)
204-
permMet, permStatuses := a.permissionStatus(strategyDetailsDeployment, ruleChecker, csv.GetNamespace())
245+
permMet, permStatuses, err := a.permissionStatus(strategyDetailsDeployment, ruleChecker, csv.GetNamespace())
246+
if err != nil {
247+
return false, nil, err
248+
}
205249

206250
// Aggregate requirement and permissions statuses
207251
statuses := append(reqStatuses, permStatuses...)

0 commit comments

Comments
 (0)