From 8c5b4e2760895499de9f5ff97de3798eaef2b389 Mon Sep 17 00:00:00 2001 From: Jonathan Crowther Date: Mon, 27 Nov 2023 20:32:54 -0500 Subject: [PATCH] Address comments --- docs.md | 7 +- go.mod | 4 +- go.sum | 4 + pkg/resources/common/common.go | 28 ++++ .../v3/globalrole/GlobalRole.md | 7 +- .../v3/globalrole/setup_test.go | 4 + .../v3/globalrole/validator.go | 63 +++++++-- .../v3/globalrole/validator_test.go | 61 ++++++++- .../v3/globalrolebinding/setup_test.go | 15 ++- .../v3/globalrolebinding/validator.go | 46 +++++-- .../v3/globalrolebinding/validator_test.go | 125 +++++++++++++++++- .../v1/role/validator.go | 38 +----- .../v1/role/validator_test.go | 68 ++++++++-- .../v1/rolebinding/validator.go | 42 +----- .../v1/rolebinding/validator_test.go | 69 ++++++++-- pkg/server/handlers.go | 4 +- 16 files changed, 448 insertions(+), 137 deletions(-) diff --git a/docs.md b/docs.md index 67c56714a..0f3d6d138 100644 --- a/docs.md +++ b/docs.md @@ -120,11 +120,12 @@ On create or update, the following checks take place: #### Escalation Prevention -Users can only change GlobalRoles with rights less than or equal to those they currently possess. This is to prevent privilege escalation. This includes the rules in the RoleTemplates referred to in `inheritedClusterRoles`. + Escalation checks are bypassed if a user has the `escalate` verb on the GlobalRole that they are attempting to update. This can also be given through a wildcard permission (i.e. the `*` verb also gives `escalate`). -This escalation check is bypassed if a user has the `escalate` verb on the GlobalRole that they are attempting to update. This can also be given through a wildcard permission (i.e. the `*` verb also gives `escalate`). +Users can only change GlobalRoles with rights less than or equal to those they currently possess. This is to prevent privilege escalation. This includes the rules in the RoleTemplates referred to in `inheritedClusterRoles`. -Users can only have rules in the `NamespacedRules` field with rights less than or equal to those they currently possess. +Users can only have rules in the `NamespacedRules` field with rights less than or equal to those they currently possess. This works on a per namespace basis, meaning that the user must have the permission +in the namespace specified. The `Rules` field apply to every namespace, which means a user can create `NamespacedRules` in any namespace that are equal to or less than the `Rules` they currently possess. #### Builtin Validation diff --git a/go.mod b/go.mod index 089dbedc0..097a9e7fc 100644 --- a/go.mod +++ b/go.mod @@ -5,7 +5,7 @@ go 1.20 // on release remove this wrangler replace and use the latest tag replace github.com/rancher/wrangler v1.1.1 => github.com/rancher/wrangler v1.1.1-0.20230831050635-df1bd5aae9df -replace github.com/rancher/rancher/pkg/apis => /Users/jcrowther/git/rancher/pkg/apis +replace github.com/rancher/rancher/pkg/apis => github.com/JonCrowther/rancher/pkg/apis v0.0.0-20231114011923-3284ace2d964 replace ( k8s.io/api => k8s.io/api v0.27.4 @@ -41,6 +41,7 @@ require ( github.com/evanphx/json-patch v5.6.0+incompatible github.com/golang/mock v1.6.0 github.com/gorilla/mux v1.8.0 + github.com/hashicorp/go-multierror v1.0.0 github.com/rancher/dynamiclistener v0.3.5 github.com/rancher/lasso v0.0.0-20230830164424-d684fdeb6f29 github.com/rancher/lasso/controller-runtime v0.0.0-20230830164424-d684fdeb6f29 @@ -92,6 +93,7 @@ require ( github.com/google/go-cmp v0.5.9 // indirect github.com/google/gofuzz v1.2.0 // indirect github.com/google/uuid v1.3.1 // indirect + github.com/hashicorp/errwrap v1.0.0 // indirect github.com/huandu/xstrings v1.3.3 // indirect github.com/imdario/mergo v0.3.16 // indirect github.com/josharian/intern v1.0.0 // indirect diff --git a/go.sum b/go.sum index 31e4fc767..920b70334 100644 --- a/go.sum +++ b/go.sum @@ -47,6 +47,8 @@ dmitri.shuralyov.com/gpu/mtl v0.0.0-20190408044501-666a987793e9/go.mod h1:H6x//7 github.com/Azure/go-ansiterm v0.0.0-20210617225240-d185dfc1b5a1/go.mod h1:xomTg63KZ2rFqZQzSB4Vz2SUXa1BpHTVz9L5PTmPC4E= github.com/BurntSushi/toml v0.3.1/go.mod h1:xHWCNGjB5oqiDr8zfno3MHue2Ht5sIBksp03qcyfWMU= github.com/BurntSushi/xgb v0.0.0-20160522181843-27f122750802/go.mod h1:IVnqGOEym/WlBOVXweHU+Q+/VP0lqqI8lqeDx9IjBqo= +github.com/JonCrowther/rancher/pkg/apis v0.0.0-20231114011923-3284ace2d964 h1:ELebAgwhbeYZiffxmZvgu9LSJrQz7QkhvTOmJ7m0ijA= +github.com/JonCrowther/rancher/pkg/apis v0.0.0-20231114011923-3284ace2d964/go.mod h1:NVTx5M0/hU0kdLN4PdL6oONMCtxKiMNOr7d8cAccSgs= github.com/MakeNowJust/heredoc v1.0.0 h1:cXCdzVdstXyiTqTvfqk9SDHpKNjxuom+DOlyEeQ4pzQ= github.com/MakeNowJust/heredoc v1.0.0/go.mod h1:mG5amYoWBHf8vpLOuehzbGGw0EHxpZZ6lCpQ4fNJ8LE= github.com/Masterminds/goutils v1.1.1 h1:5nUrii3FMTL5diU80unEVvNevw1nH4+ZV4DSLVJLSYI= @@ -345,10 +347,12 @@ github.com/grpc-ecosystem/grpc-gateway v1.16.0/go.mod h1:BDjrQk3hbvj6Nolgz8mAMFb github.com/grpc-ecosystem/grpc-gateway/v2 v2.7.0/go.mod h1:hgWBS7lorOAVIJEQMi4ZsPv9hVvWI6+ch50m39Pf2Ks= github.com/hashicorp/consul/api v1.1.0/go.mod h1:VmuI/Lkw1nC05EYQWNKwWGbkg+FbDBtguAZLlVdkD9Q= github.com/hashicorp/consul/sdk v0.1.1/go.mod h1:VKf9jXwCTEY1QZP2MOLRhb5i/I/ssyNV1vwHyQBF0x8= +github.com/hashicorp/errwrap v1.0.0 h1:hLrqtEDnRye3+sgx6z4qVLNuviH3MR5aQ0ykNJa/UYA= github.com/hashicorp/errwrap v1.0.0/go.mod h1:YH+1FKiLXxHSkmPseP+kNlulaMuP3n2brvKWEqk/Jc4= github.com/hashicorp/go-cleanhttp v0.5.1/go.mod h1:JpRdi6/HCYpAwUzNwuwqhbovhLtngrth3wmdIIUrZ80= github.com/hashicorp/go-immutable-radix v1.0.0/go.mod h1:0y9vanUI8NX6FsYoO3zeMjhV/C5i9g4Q3DwcSNZ4P60= github.com/hashicorp/go-msgpack v0.5.3/go.mod h1:ahLV/dePpqEmjfWmKiqvPkv/twdG7iPBM1vqhUKIvfM= +github.com/hashicorp/go-multierror v1.0.0 h1:iVjPR7a6H0tWELX5NxNe7bYopibicUzc7uPribsnS6o= github.com/hashicorp/go-multierror v1.0.0/go.mod h1:dHtQlpGsu+cZNNAkkCN/P3hoUDHhCYQXV3UM06sGGrk= github.com/hashicorp/go-rootcerts v1.0.0/go.mod h1:K6zTfqpRlCUIjkwsN4Z+hiSfzSTQa6eBIzfwKfwNnHU= github.com/hashicorp/go-sockaddr v1.0.0/go.mod h1:7Xibr9yA9JjQq1JpNB2Vw7kxv8xerXegt+ozgdvDeDU= diff --git a/pkg/resources/common/common.go b/pkg/resources/common/common.go index 90e7a5a24..7f2aacfa7 100644 --- a/pkg/resources/common/common.go +++ b/pkg/resources/common/common.go @@ -14,3 +14,31 @@ func ConvertAuthnExtras(extra map[string]authnv1.ExtraValue) map[string]authzv1. } return result } + +// ValidateLabel checks if a user is removing or modifying a label and returns an error. If the label is being added return nil. +func IsModifyingLabel(old, new map[string]string, label string) bool { + var oldValue, newValue string + var oldLabelExists, newLabelExists bool + if old == nil { + oldLabelExists = false + } else { + oldValue, oldLabelExists = old[label] + } + if new == nil { + newLabelExists = false + } else { + newValue, newLabelExists = new[label] + } + + if !oldLabelExists { + return false + } + if oldLabelExists && !newLabelExists { + return true + } + if oldValue != newValue { + return true + } + + return false +} diff --git a/pkg/resources/management.cattle.io/v3/globalrole/GlobalRole.md b/pkg/resources/management.cattle.io/v3/globalrole/GlobalRole.md index 078971ff3..0f6f4911c 100644 --- a/pkg/resources/management.cattle.io/v3/globalrole/GlobalRole.md +++ b/pkg/resources/management.cattle.io/v3/globalrole/GlobalRole.md @@ -10,11 +10,12 @@ On create or update, the following checks take place: ### Escalation Prevention -Users can only change GlobalRoles with rights less than or equal to those they currently possess. This is to prevent privilege escalation. This includes the rules in the RoleTemplates referred to in `inheritedClusterRoles`. + Escalation checks are bypassed if a user has the `escalate` verb on the GlobalRole that they are attempting to update. This can also be given through a wildcard permission (i.e. the `*` verb also gives `escalate`). -This escalation check is bypassed if a user has the `escalate` verb on the GlobalRole that they are attempting to update. This can also be given through a wildcard permission (i.e. the `*` verb also gives `escalate`). +Users can only change GlobalRoles with rights less than or equal to those they currently possess. This is to prevent privilege escalation. This includes the rules in the RoleTemplates referred to in `inheritedClusterRoles`. -Users can only have rules in the `NamespacedRules` field with rights less than or equal to those they currently possess. +Users can only have rules in the `NamespacedRules` field with rights less than or equal to those they currently possess. This works on a per namespace basis, meaning that the user must have the permission +in the namespace specified. The `Rules` field apply to every namespace, which means a user can create `NamespacedRules` in any namespace that are equal to or less than the `Rules` they currently possess. ### Builtin Validation diff --git a/pkg/resources/management.cattle.io/v3/globalrole/setup_test.go b/pkg/resources/management.cattle.io/v3/globalrole/setup_test.go index 937a7e564..51a8ee384 100644 --- a/pkg/resources/management.cattle.io/v3/globalrole/setup_test.go +++ b/pkg/resources/management.cattle.io/v3/globalrole/setup_test.go @@ -15,6 +15,7 @@ import ( "github.com/stretchr/testify/assert" admissionv1 "k8s.io/api/admission/v1" v1authentication "k8s.io/api/authentication/v1" + corev1 "k8s.io/api/core/v1" v1 "k8s.io/api/rbac/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -172,6 +173,7 @@ type testState struct { rtCacheMock *fake.MockNonNamespacedCacheInterface[*v3.RoleTemplate] grCacheMock *fake.MockNonNamespacedCacheInterface[*v3.GlobalRole] grbCacheMock *fake.MockNonNamespacedCacheInterface[*v3.GlobalRoleBinding] + nsCacheMock *fake.MockNonNamespacedCacheInterface[*corev1.Namespace] sarMock *k8fake.FakeSubjectAccessReviews resolver validation.AuthorizationRuleResolver } @@ -259,6 +261,7 @@ func newDefaultState(t *testing.T) testState { rtCacheMock := fake.NewMockNonNamespacedCacheInterface[*v3.RoleTemplate](ctrl) grCacheMock := fake.NewMockNonNamespacedCacheInterface[*v3.GlobalRole](ctrl) grbCacheMock := fake.NewMockNonNamespacedCacheInterface[*v3.GlobalRoleBinding](ctrl) + nsCacheMock := fake.NewMockNonNamespacedCacheInterface[*corev1.Namespace](ctrl) grbs := []*v3.GlobalRoleBinding{&baseGRB} grbCacheMock.EXPECT().GetByIndex(gomock.Any(), resolvers.GetUserKey(testUser, "")).Return(grbs, nil).AnyTimes() grbCacheMock.EXPECT().GetByIndex(gomock.Any(), resolvers.GetUserKey(adminUser, "")).Return(grbs, nil).AnyTimes() @@ -273,6 +276,7 @@ func newDefaultState(t *testing.T) testState { rtCacheMock: rtCacheMock, grCacheMock: grCacheMock, grbCacheMock: grbCacheMock, + nsCacheMock: nsCacheMock, sarMock: fakeSAR, resolver: resolver, } diff --git a/pkg/resources/management.cattle.io/v3/globalrole/validator.go b/pkg/resources/management.cattle.io/v3/globalrole/validator.go index cad47f47d..918daa128 100644 --- a/pkg/resources/management.cattle.io/v3/globalrole/validator.go +++ b/pkg/resources/management.cattle.io/v3/globalrole/validator.go @@ -6,15 +6,18 @@ import ( "fmt" "reflect" + "github.com/hashicorp/go-multierror" v3 "github.com/rancher/rancher/pkg/apis/management.cattle.io/v3" "github.com/rancher/webhook/pkg/admission" "github.com/rancher/webhook/pkg/auth" objectsv3 "github.com/rancher/webhook/pkg/generated/objects/management.cattle.io/v3" "github.com/rancher/webhook/pkg/resolvers" "github.com/rancher/webhook/pkg/resources/common" + corev1controller "github.com/rancher/wrangler/pkg/generated/controllers/core/v1" "github.com/sirupsen/logrus" admissionv1 "k8s.io/api/admission/v1" admissionregistrationv1 "k8s.io/api/admissionregistration/v1" + v1 "k8s.io/api/rbac/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/util/validation/field" @@ -36,12 +39,13 @@ const ( // NewValidator returns a new validator used for validation globalRoles. func NewValidator(ruleResolver validation.AuthorizationRuleResolver, grbResolver *resolvers.GRBClusterRuleResolver, - sar authorizationv1.SubjectAccessReviewInterface) *Validator { + sar authorizationv1.SubjectAccessReviewInterface, nsCache corev1controller.NamespaceCache) *Validator { return &Validator{ admitter: admitter{ resolver: ruleResolver, grbResolver: grbResolver, sar: sar, + namespaces: nsCache, }, } } @@ -75,6 +79,7 @@ type admitter struct { resolver validation.AuthorizationRuleResolver grbResolver *resolvers.GRBClusterRuleResolver sar authorizationv1.SubjectAccessReviewInterface + namespaces corev1controller.NamespaceCache } // Admit is the entrypoint for the validator. Admit will return an error if it's unable to process the request. @@ -124,6 +129,7 @@ func (a *admitter) Admit(request *admission.Request) (*admissionv1.AdmissionResp hasEscalate, escErr := auth.RequestUserHasVerb(request, gvr, a.sar, escalateVerb, newGR.Name, "") if escErr != nil { logrus.Warnf("Failed to check for the 'escalate' verb on GlobalRoles: %v", escErr) + hasEscalate = false } // if we have the escalate verb, no need to check global/cluster/namespaced permissions @@ -136,25 +142,32 @@ func (a *admitter) Admit(request *admission.Request) (*admissionv1.AdmissionResp if err != nil { return nil, fmt.Errorf("unable to resolve rules for new global role: %w", err) } + + // collect all escalation errors for the user + var allErrors []error err = auth.ConfirmNoEscalation(request, clusterRules, "", a.grbResolver) if err != nil { - return admission.ResponseFailedEscalation(err.Error()), nil + allErrors = append(allErrors, err) } globalRules := a.grbResolver.GlobalRoleResolver.GlobalRulesFromRole(newGR) + err = validateRules(globalRules) + if err != nil { + allErrors = append(allErrors, err) + } err = auth.ConfirmNoEscalation(request, globalRules, "", a.resolver) if err != nil { - return admission.ResponseFailedEscalation(err.Error()), nil + allErrors = append(allErrors, err) } - // check for escalation in each namespace of NamespacedRules - for namespace, rules := range newGR.NamespacedRules { - err = auth.ConfirmNoEscalation(request, rules, namespace, a.resolver) - if err != nil { - return admission.ResponseFailedEscalation(err.Error()), nil - } + errs := a.validateNamespacedRules(request, newGR.NamespacedRules) + if errs != nil { + allErrors = append(allErrors, errs...) } + if allErrors != nil { + return admission.ResponseFailedEscalation(fmt.Sprintf("errors due to escalation: %v", allErrors)), nil + } return admission.ResponseAllowed(), nil } @@ -256,3 +269,35 @@ func (a *admitter) validateUpdateFields(oldRole, newRole *v3.GlobalRole, fldPath } return nil } + +func (a *admitter) validateNamespacedRules(request *admission.Request, rules map[string][]v1.PolicyRule) []error { + var returnErrs []error + // check for escalation in each namespace of the GlobalRole's NamespacedRules + for namespace, rules := range rules { + // check that namespace exists + _, err := a.namespaces.Get(namespace) + if err != nil { + returnErrs = append(returnErrs, fmt.Errorf("error getting namespace %v: %w", namespace, err)) + continue + } + err = validateRules(rules) + if err != nil { + returnErrs = append(returnErrs, err) + } + err = auth.ConfirmNoEscalation(request, rules, namespace, a.resolver) + if err != nil { + returnErrs = append(returnErrs, err) + } + } + return returnErrs +} + +func validateRules(rules []v1.PolicyRule) error { + var err error + for _, rule := range rules { + if len(rule.Verbs) == 0 { + err = multierror.Append(err, fmt.Errorf("rule %v must have at least 1 verb", rule)) + } + } + return err +} diff --git a/pkg/resources/management.cattle.io/v3/globalrole/validator_test.go b/pkg/resources/management.cattle.io/v3/globalrole/validator_test.go index 559fd24ff..801d58975 100644 --- a/pkg/resources/management.cattle.io/v3/globalrole/validator_test.go +++ b/pkg/resources/management.cattle.io/v3/globalrole/validator_test.go @@ -5,6 +5,7 @@ import ( "testing" "time" + "github.com/golang/mock/gomock" v3 "github.com/rancher/rancher/pkg/apis/management.cattle.io/v3" "github.com/rancher/webhook/pkg/resources/management.cattle.io/v3/globalrole" "github.com/stretchr/testify/assert" @@ -557,6 +558,10 @@ func TestAdmit(t *testing.T) { } return baseGR }, + stateSetup: func(state testState) { + state.nsCacheMock.EXPECT().Get(gomock.Any()).Return(nil, nil).AnyTimes() + setSarResponse(false, nil, testUser, newDefaultGR().Name, state.sarMock) + }, }, allowed: false, }, @@ -573,6 +578,7 @@ func TestAdmit(t *testing.T) { return baseGR }, stateSetup: func(state testState) { + state.nsCacheMock.EXPECT().Get(gomock.Any()).Return(nil, nil).AnyTimes() setSarResponse(true, nil, testUser, newDefaultGR().Name, state.sarMock) }, }, @@ -589,6 +595,30 @@ func TestAdmit(t *testing.T) { } return baseGR }, + stateSetup: func(state testState) { + state.nsCacheMock.EXPECT().Get(gomock.Any()).Return(nil, nil).AnyTimes() + setSarResponse(false, nil, testUser, newDefaultGR().Name, state.sarMock) + }, + }, + + allowed: true, + }, + { + name: "creating with multiple allowed NamespacedRules, multiple namespaces", + args: args{ + username: adminUser, + newGR: func() *v3.GlobalRole { + baseGR := newDefaultGR() + baseGR.NamespacedRules = map[string][]v1.PolicyRule{ + "ns1": {ruleReadPods}, + "ns2": {ruleWriteNodes, ruleReadPods}, + } + return baseGR + }, + stateSetup: func(state testState) { + state.nsCacheMock.EXPECT().Get(gomock.Any()).Return(nil, nil).AnyTimes() + setSarResponse(false, nil, testUser, newDefaultGR().Name, state.sarMock) + }, }, allowed: true, }, @@ -603,6 +633,10 @@ func TestAdmit(t *testing.T) { } return baseGR }, + stateSetup: func(state testState) { + state.nsCacheMock.EXPECT().Get(gomock.Any()).Return(nil, nil).AnyTimes() + setSarResponse(false, nil, testUser, newDefaultGR().Name, state.sarMock) + }, }, allowed: true, }, @@ -624,6 +658,10 @@ func TestAdmit(t *testing.T) { } return baseGR }, + stateSetup: func(state testState) { + state.nsCacheMock.EXPECT().Get(gomock.Any()).Return(nil, nil).AnyTimes() + setSarResponse(false, nil, testUser, newDefaultGR().Name, state.sarMock) + }, }, allowed: false, }, @@ -646,11 +684,30 @@ func TestAdmit(t *testing.T) { return baseGR }, stateSetup: func(state testState) { + state.nsCacheMock.EXPECT().Get(gomock.Any()).Return(nil, nil).AnyTimes() setSarResponse(true, nil, testUser, newDefaultGR().Name, state.sarMock) }, }, allowed: true, }, + { + name: "error getting namespace in NamespacedRules", + args: args{ + username: adminUser, + newGR: func() *v3.GlobalRole { + baseGR := newDefaultGR() + baseGR.NamespacedRules = map[string][]v1.PolicyRule{ + "ns1": {ruleReadPods}, + } + return baseGR + }, + stateSetup: func(ts testState) { + ts.nsCacheMock.EXPECT().Get("ns1").Return(nil, fmt.Errorf("error")).AnyTimes() + setSarResponse(false, nil, testUser, newDefaultGR().Name, ts.sarMock) + }, + }, + allowed: false, + }, } for _, test := range tests { @@ -662,7 +719,7 @@ func TestAdmit(t *testing.T) { test.args.stateSetup(state) } grbResolver := state.createBaseGRBResolver() - admitters := globalrole.NewValidator(state.resolver, grbResolver, state.sarMock).Admitters() + admitters := globalrole.NewValidator(state.resolver, grbResolver, state.sarMock, state.nsCacheMock).Admitters() assert.Len(t, admitters, 1) req := createGRRequest(t, test) @@ -680,7 +737,7 @@ func TestAdmit(t *testing.T) { func Test_UnexpectedErrors(t *testing.T) { t.Parallel() resolver, _ := validation.NewTestRuleResolver(nil, nil, nil, nil) - validator := globalrole.NewValidator(resolver, nil, nil) + validator := globalrole.NewValidator(resolver, nil, nil, nil) admitters := validator.Admitters() require.Len(t, admitters, 1, "wanted only one admitter") test := testCase{ diff --git a/pkg/resources/management.cattle.io/v3/globalrolebinding/setup_test.go b/pkg/resources/management.cattle.io/v3/globalrolebinding/setup_test.go index 9de61fee4..62ab32c08 100644 --- a/pkg/resources/management.cattle.io/v3/globalrolebinding/setup_test.go +++ b/pkg/resources/management.cattle.io/v3/globalrolebinding/setup_test.go @@ -14,6 +14,7 @@ import ( "github.com/stretchr/testify/assert" v1 "k8s.io/api/admission/v1" v1authentication "k8s.io/api/authentication/v1" + corev1 "k8s.io/api/core/v1" rbacv1 "k8s.io/api/rbac/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -36,17 +37,19 @@ const ( ) type testCase struct { - wantGRB func() *v3.GlobalRoleBinding - name string - args args - wantError bool - allowed bool + wantGRB func() *v3.GlobalRoleBinding + name string + args args + wantError bool + allowed bool + respMessageContains []string } type testState struct { rtCacheMock *fake.MockNonNamespacedCacheInterface[*v3.RoleTemplate] grCacheMock *fake.MockNonNamespacedCacheInterface[*v3.GlobalRole] grbCacheMock *fake.MockNonNamespacedCacheInterface[*v3.GlobalRoleBinding] + nsCacheMock *fake.MockNonNamespacedCacheInterface[*corev1.Namespace] sarMock *k8fake.FakeSubjectAccessReviews resolver validation.AuthorizationRuleResolver } @@ -313,6 +316,7 @@ func newDefaultState(t *testing.T) testState { rtCacheMock := fake.NewMockNonNamespacedCacheInterface[*v3.RoleTemplate](ctrl) grCacheMock := fake.NewMockNonNamespacedCacheInterface[*v3.GlobalRole](ctrl) grbCacheMock := fake.NewMockNonNamespacedCacheInterface[*v3.GlobalRoleBinding](ctrl) + nsCacheMock := fake.NewMockNonNamespacedCacheInterface[*corev1.Namespace](ctrl) grbs := []*v3.GlobalRoleBinding{&baseGRB} grbCacheMock.EXPECT().GetByIndex(gomock.Any(), resolvers.GetUserKey(testUser, "")).Return(grbs, nil).AnyTimes() @@ -331,6 +335,7 @@ func newDefaultState(t *testing.T) testState { rtCacheMock: rtCacheMock, grCacheMock: grCacheMock, grbCacheMock: grbCacheMock, + nsCacheMock: nsCacheMock, sarMock: fakeSAR, resolver: resolver, } diff --git a/pkg/resources/management.cattle.io/v3/globalrolebinding/validator.go b/pkg/resources/management.cattle.io/v3/globalrolebinding/validator.go index 2bb7e7fdf..4fec231f9 100644 --- a/pkg/resources/management.cattle.io/v3/globalrolebinding/validator.go +++ b/pkg/resources/management.cattle.io/v3/globalrolebinding/validator.go @@ -11,9 +11,11 @@ import ( "github.com/rancher/webhook/pkg/auth" objectsv3 "github.com/rancher/webhook/pkg/generated/objects/management.cattle.io/v3" "github.com/rancher/webhook/pkg/resolvers" + corev1controller "github.com/rancher/wrangler/pkg/generated/controllers/core/v1" "github.com/sirupsen/logrus" admissionv1 "k8s.io/api/admission/v1" admissionregistrationv1 "k8s.io/api/admissionregistration/v1" + v1 "k8s.io/api/rbac/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/util/validation/field" @@ -39,12 +41,13 @@ const bindVerb = "bind" // NewValidator returns a new validator for GlobalRoleBindings. func NewValidator(resolver rbacvalidation.AuthorizationRuleResolver, grbResolver *resolvers.GRBClusterRuleResolver, - sar authorizationv1.SubjectAccessReviewInterface) *Validator { + sar authorizationv1.SubjectAccessReviewInterface, nsCache corev1controller.NamespaceCache) *Validator { return &Validator{ admitter: admitter{ resolver: resolver, grbResolver: grbResolver, sar: sar, + namespaces: nsCache, }, } } @@ -78,6 +81,7 @@ type admitter struct { resolver rbacvalidation.AuthorizationRuleResolver grbResolver *resolvers.GRBClusterRuleResolver sar authorizationv1.SubjectAccessReviewInterface + namespaces corev1controller.NamespaceCache } // Admit handles the webhook admission request sent to this webhook. @@ -126,6 +130,7 @@ func (a *admitter) Admit(request *admission.Request) (*admissionv1.AdmissionResp hasBind, bindErr := auth.RequestUserHasVerb(request, globalRoleGvr, a.sar, bindVerb, globalRole.Name, "") if bindErr != nil { logrus.Warnf("Failed to check for the 'bind' verb on GlobalRoleBinding: %v", bindErr) + hasBind = false } // if we have the bind verb, no need to check global/cluster/namespaced permisions @@ -141,24 +146,29 @@ func (a *admitter) Admit(request *admission.Request) (*admissionv1.AdmissionResp } return nil, fmt.Errorf("unable to get global rules from role %s: %w", globalRole.Name, err) } + + // collect all escalation errors for the user + var allErrors []error err = auth.ConfirmNoEscalation(request, clusterRules, "", a.grbResolver) if err != nil { - return admission.ResponseFailedEscalation(err.Error()), nil + allErrors = append(allErrors, err) } rules := a.grbResolver.GlobalRoleResolver.GlobalRulesFromRole(globalRole) err = auth.ConfirmNoEscalation(request, rules, "", a.resolver) if err != nil { - return admission.ResponseFailedEscalation(err.Error()), nil + allErrors = append(allErrors, err) } - // check for escalation in each namespace of the GlobalRole's NamespacedRules - for namespace, rules := range globalRole.NamespacedRules { - err = auth.ConfirmNoEscalation(request, rules, namespace, a.resolver) - if err != nil { - return admission.ResponseFailedEscalation(err.Error()), nil - } + errs := a.validateNamespacedRules(request, globalRole.NamespacedRules) + if errs != nil { + allErrors = append(allErrors, errs...) } + + if allErrors != nil { + return admission.ResponseFailedEscalation(fmt.Sprintf("errors due to escalation: %v", allErrors)), nil + } + return admission.ResponseAllowed(), nil } @@ -213,3 +223,21 @@ func (a *admitter) validateGlobalRole(globalRole *v3.GlobalRole, fieldPath *fiel } return nil } + +func (a *admitter) validateNamespacedRules(request *admission.Request, rules map[string][]v1.PolicyRule) []error { + var returnErrs []error + // check for escalation in each namespace of the GlobalRole's NamespacedRules + for namespace, rules := range rules { + // check that namespace exists + _, err := a.namespaces.Get(namespace) + if err != nil { + returnErrs = append(returnErrs, fmt.Errorf("error getting namespace %v: %w", namespace, err)) + continue + } + err = auth.ConfirmNoEscalation(request, rules, namespace, a.resolver) + if err != nil { + returnErrs = append(returnErrs, err) + } + } + return returnErrs +} diff --git a/pkg/resources/management.cattle.io/v3/globalrolebinding/validator_test.go b/pkg/resources/management.cattle.io/v3/globalrolebinding/validator_test.go index ef7ee776b..4d0d8d4ef 100644 --- a/pkg/resources/management.cattle.io/v3/globalrolebinding/validator_test.go +++ b/pkg/resources/management.cattle.io/v3/globalrolebinding/validator_test.go @@ -5,6 +5,7 @@ import ( "testing" "time" + "github.com/golang/mock/gomock" v3 "github.com/rancher/rancher/pkg/apis/management.cattle.io/v3" "github.com/rancher/webhook/pkg/auth" "github.com/rancher/webhook/pkg/resolvers" @@ -13,6 +14,7 @@ import ( "github.com/stretchr/testify/require" v1 "k8s.io/api/admission/v1" authorizationv1 "k8s.io/api/authorization/v1" + rbacv1 "k8s.io/api/rbac/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -389,7 +391,7 @@ func TestAdmit(t *testing.T) { allowed: false, }, { - name: "rules in GR NamespacedRules allowed", + name: "create rules in GR NamespacedRules allowed", args: args{ username: adminUser, newGRB: func() *v3.GlobalRoleBinding { @@ -400,13 +402,41 @@ func TestAdmit(t *testing.T) { }, stateSetup: func(ts testState) { ts.grCacheMock.EXPECT().Get(namespacedRulesGR.Name).Return(&namespacedRulesGR, nil) + ts.nsCacheMock.EXPECT().Get(gomock.Any()).Return(nil, nil).AnyTimes() setSarResponse(false, nil, adminUser, namespacedRulesGR.Name, ts.sarMock) }, }, allowed: true, }, { - name: "escalation in GR NamespacedRules", + name: "create rules in GR NamespacedRules allowed, multiple namespaces", + args: args{ + // adminUser has global rules that apply across all namespaces + username: adminUser, + newGRB: func() *v3.GlobalRoleBinding { + baseGRB := newDefaultGRB() + baseGRB.UserName = adminUser + baseGRB.GlobalRoleName = namespacedRulesGR.Name + return baseGRB + }, + stateSetup: func(ts testState) { + gr := namespacedRulesGR.DeepCopy() + gr.NamespacedRules["ns2"] = []rbacv1.PolicyRule{ + { + APIGroups: []string{""}, + Resources: []string{"pods"}, + Verbs: []string{"*"}, + }, + } + ts.grCacheMock.EXPECT().Get(namespacedRulesGR.Name).Return(gr, nil) + ts.nsCacheMock.EXPECT().Get(gomock.Any()).Return(nil, nil).AnyTimes() + setSarResponse(false, nil, adminUser, namespacedRulesGR.Name, ts.sarMock) + }, + }, + allowed: true, + }, + { + name: "create escalation in GR NamespacedRules", args: args{ newGRB: func() *v3.GlobalRoleBinding { return &v3.GlobalRoleBinding{ @@ -419,13 +449,14 @@ func TestAdmit(t *testing.T) { }, stateSetup: func(ts testState) { ts.grCacheMock.EXPECT().Get(namespacedRulesGR.Name).Return(&namespacedRulesGR, nil) + ts.nsCacheMock.EXPECT().Get(gomock.Any()).Return(nil, nil).AnyTimes() setSarResponse(false, nil, testUser, namespacedRulesGR.Name, ts.sarMock) }, }, allowed: false, }, { - name: "escalation in GR NamespacedRules, bind allowed", + name: "create escalation in GR NamespacedRules, bind allowed", args: args{ newGRB: func() *v3.GlobalRoleBinding { return &v3.GlobalRoleBinding{ @@ -438,11 +469,90 @@ func TestAdmit(t *testing.T) { }, stateSetup: func(ts testState) { ts.grCacheMock.EXPECT().Get(namespacedRulesGR.Name).Return(&namespacedRulesGR, nil) + ts.nsCacheMock.EXPECT().Get(gomock.Any()).Return(nil, nil).AnyTimes() setSarResponse(true, nil, testUser, namespacedRulesGR.Name, ts.sarMock) }, }, allowed: true, }, + { + name: "error getting namespace in NamespacedRules", + args: args{ + username: adminUser, + newGRB: func() *v3.GlobalRoleBinding { + baseGRB := newDefaultGRB() + baseGRB.UserName = adminUser + baseGRB.GlobalRoleName = namespacedRulesGR.Name + return baseGRB + }, + stateSetup: func(ts testState) { + ts.grCacheMock.EXPECT().Get(namespacedRulesGR.Name).Return(&namespacedRulesGR, nil) + ts.nsCacheMock.EXPECT().Get("ns1").Return(nil, fmt.Errorf("error")).AnyTimes() + setSarResponse(false, nil, testUser, namespacedRulesGR.Name, ts.sarMock) + }, + }, + allowed: false, + }, + { + name: "multiple escalation failures all get logged", + args: args{ + newGRB: func() *v3.GlobalRoleBinding { + baseGRB := newDefaultGRB() + baseGRB.GlobalRoleName = "test-grb" + return baseGRB + }, + stateSetup: func(ts testState) { + // the GlobalRole has an escalation in ClusterRule, GlobalRule and NamespacedRule + // this test checks that the response message informs the user of all 3 + gr := v3.GlobalRole{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-gr", + }, + Rules: []rbacv1.PolicyRule{ + { + APIGroups: []string{""}, + Resources: []string{"global"}, + Verbs: []string{"*"}, + }, + }, + InheritedClusterRoles: []string{ + "test-rt", + }, + NamespacedRules: map[string][]rbacv1.PolicyRule{ + "ns1": { + { + APIGroups: []string{""}, + Resources: []string{"namespaced"}, + Verbs: []string{"*"}, + }, + }, + }, + } + rt := v3.RoleTemplate{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-rt", + }, + Rules: []rbacv1.PolicyRule{ + { + APIGroups: []string{""}, + Resources: []string{"cluster"}, + Verbs: []string{"*"}, + }, + }, + } + ts.grCacheMock.EXPECT().Get("test-grb").Return(&gr, nil) + ts.nsCacheMock.EXPECT().Get(gomock.Any()).Return(nil, nil).AnyTimes() + ts.rtCacheMock.EXPECT().Get("test-rt").Return(&rt, nil).AnyTimes() + setSarResponse(false, nil, testUser, namespacedRulesGR.Name, ts.sarMock) + }, + }, + allowed: false, + respMessageContains: []string{ + "global", + "cluster", + "namespaced", + }, + }, { name: "not found error resolving rules", args: args{ @@ -715,7 +825,7 @@ func TestAdmit(t *testing.T) { } grResolver := auth.NewGlobalRoleResolver(auth.NewRoleTemplateResolver(state.rtCacheMock, nil), state.grCacheMock) grbResolver := resolvers.NewGRBClusterRuleResolver(state.grbCacheMock, grResolver) - admitters := globalrolebinding.NewValidator(state.resolver, grbResolver, state.sarMock).Admitters() + admitters := globalrolebinding.NewValidator(state.resolver, grbResolver, state.sarMock, state.nsCacheMock).Admitters() require.Len(t, admitters, 1) req := createGRBRequest(t, test) @@ -727,6 +837,11 @@ func TestAdmit(t *testing.T) { } require.NoError(t, err) require.Equalf(t, test.allowed, response.Allowed, "Response was incorrectly validated wanted response.Allowed = '%v' got '%v' message=%+v", test.allowed, response.Allowed, response.Result) + if test.respMessageContains != nil { + for _, c := range test.respMessageContains { + require.Contains(t, response.Result.Message, c) + } + } }) } } @@ -736,7 +851,7 @@ func Test_UnexpectedErrors(t *testing.T) { state := newDefaultState(t) grResolver := auth.NewGlobalRoleResolver(auth.NewRoleTemplateResolver(state.rtCacheMock, nil), state.grCacheMock) grbResolver := resolvers.NewGRBClusterRuleResolver(state.grbCacheMock, grResolver) - validator := globalrolebinding.NewValidator(state.resolver, grbResolver, state.sarMock) + validator := globalrolebinding.NewValidator(state.resolver, grbResolver, state.sarMock, state.nsCacheMock) admitters := validator.Admitters() require.Len(t, admitters, 1, "wanted only one admitter") admitter := admitters[0] diff --git a/pkg/resources/rbac.authorization.k8s.io/v1/role/validator.go b/pkg/resources/rbac.authorization.k8s.io/v1/role/validator.go index e4d4db221..63a4ea416 100644 --- a/pkg/resources/rbac.authorization.k8s.io/v1/role/validator.go +++ b/pkg/resources/rbac.authorization.k8s.io/v1/role/validator.go @@ -5,12 +5,11 @@ import ( "github.com/rancher/webhook/pkg/admission" objectsv1 "github.com/rancher/webhook/pkg/generated/objects/rbac.authorization.k8s.io/v1" + "github.com/rancher/webhook/pkg/resources/common" admissionv1 "k8s.io/api/admission/v1" admissionregistrationv1 "k8s.io/api/admissionregistration/v1" - v1 "k8s.io/api/rbac/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime/schema" - "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/utils/trace" ) @@ -78,40 +77,9 @@ func (a *admitter) Admit(request *admission.Request) (*admissionv1.AdmissionResp return nil, err } - fldPath := field.NewPath("role") - fieldErr := validateGROwnerLabel(oldRole, newRole, fldPath) - if fieldErr != nil { - return admission.ResponseBadRequest(fieldErr.Error()), nil + if common.IsModifyingLabel(oldRole.Labels, newRole.Labels, grOwnerLabel) { + return admission.ResponseBadRequest(fmt.Sprintf("cannot modify or remove label %s", grOwnerLabel)), nil } return admission.ResponseAllowed(), nil } - -// validateGROwnerLabel checks that a user isn't adding, removing or modifying the grOwnerLabel. -// Users can add the label if it didn't already exist. -func validateGROwnerLabel(old, new *v1.Role, fldPath *field.Path) *field.Error { - var oldOwnerLabelExists, newOwnerLabelExists bool - var oldOwner, newOwner string - if old.Labels == nil { - oldOwnerLabelExists = false - } else { - oldOwner, oldOwnerLabelExists = old.Labels[grOwnerLabel] - } - if new.Labels == nil { - newOwnerLabelExists = false - } else { - newOwner, newOwnerLabelExists = new.Labels[grOwnerLabel] - } - - if !oldOwnerLabelExists { - return nil - } - if oldOwnerLabelExists && !newOwnerLabelExists { - return field.Forbidden(fldPath, fmt.Sprintf("cannot remove label %s", grOwnerLabel)) - } - if oldOwner != newOwner { - return field.Forbidden(fldPath, fmt.Sprintf("cannot modify label %s", grOwnerLabel)) - } - - return nil -} diff --git a/pkg/resources/rbac.authorization.k8s.io/v1/role/validator_test.go b/pkg/resources/rbac.authorization.k8s.io/v1/role/validator_test.go index 0c5ad3e9a..87937df2d 100644 --- a/pkg/resources/rbac.authorization.k8s.io/v1/role/validator_test.go +++ b/pkg/resources/rbac.authorization.k8s.io/v1/role/validator_test.go @@ -26,6 +26,11 @@ func TestAdmit(t *testing.T) { Name: "default", }, } + emptyRole := &v1.Role{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{}, + }, + } roleWithOwnerLabel := &v1.Role{ ObjectMeta: metav1.ObjectMeta{ Name: "default", @@ -34,11 +39,18 @@ func TestAdmit(t *testing.T) { }, }, } + roleWithNewOwnerLabel := &v1.Role{ + ObjectMeta: metav1.ObjectMeta{ + Name: "default", + Labels: map[string]string{ + grOwnerLabel: "new-owner", + }, + }, + } type args struct { oldRole *v1.Role newRole *v1.Role - op admissionv1.Operation } type test struct { name string @@ -48,42 +60,74 @@ func TestAdmit(t *testing.T) { } tests := []test{ { - name: "create allowed", + name: "updating with 2 nil labels maps allowed", args: args{ oldRole: defaultRole.DeepCopy(), - op: admissionv1.Create, + newRole: defaultRole.DeepCopy(), }, allowed: true, }, { - name: "create with gr-owner label allowed", + name: "updating with 2 empty labels maps allowed", args: args{ - oldRole: roleWithOwnerLabel.DeepCopy(), - op: admissionv1.Create, + oldRole: emptyRole.DeepCopy(), + newRole: emptyRole.DeepCopy(), }, allowed: true, }, { - name: "update gr-owner label not allowed", + name: "updating labels other than grOwner allowed", args: args{ oldRole: roleWithOwnerLabel.DeepCopy(), newRole: &v1.Role{ ObjectMeta: metav1.ObjectMeta{ + Name: "default", Labels: map[string]string{ - grOwnerLabel: "new-owner", + "new-label": "test-value", + grOwnerLabel: "gr-owner", }, }, }, - op: admissionv1.Update, + }, + allowed: true, + }, + { + name: "adding gr-owner label allowed", + args: args{ + oldRole: defaultRole.DeepCopy(), + newRole: roleWithNewOwnerLabel.DeepCopy(), + }, + allowed: true, + }, + { + name: "adding gr-owner label to empty labels map allowed", + args: args{ + oldRole: emptyRole.DeepCopy(), + newRole: roleWithNewOwnerLabel.DeepCopy(), + }, + allowed: true, + }, + { + name: "modifying gr-owner label not allowed", + args: args{ + oldRole: roleWithOwnerLabel.DeepCopy(), + newRole: roleWithNewOwnerLabel.DeepCopy(), }, allowed: false, }, { - name: "remove gr-owner label not allowed", + name: "removing gr-owner label not allowed", args: args{ oldRole: roleWithOwnerLabel.DeepCopy(), newRole: defaultRole.DeepCopy(), - op: admissionv1.Update, + }, + allowed: false, + }, + { + name: "replacing labels with empty map not allowed", + args: args{ + oldRole: roleWithOwnerLabel.DeepCopy(), + newRole: emptyRole.DeepCopy(), }, allowed: false, }, @@ -101,7 +145,7 @@ func TestAdmit(t *testing.T) { Resource: gvr, RequestKind: &gvk, RequestResource: &gvr, - Operation: test.args.op, + Operation: admissionv1.Update, Object: runtime.RawExtension{}, OldObject: runtime.RawExtension{}, }, diff --git a/pkg/resources/rbac.authorization.k8s.io/v1/rolebinding/validator.go b/pkg/resources/rbac.authorization.k8s.io/v1/rolebinding/validator.go index b2f110538..78de4e449 100644 --- a/pkg/resources/rbac.authorization.k8s.io/v1/rolebinding/validator.go +++ b/pkg/resources/rbac.authorization.k8s.io/v1/rolebinding/validator.go @@ -5,12 +5,11 @@ import ( "github.com/rancher/webhook/pkg/admission" objectsv1 "github.com/rancher/webhook/pkg/generated/objects/rbac.authorization.k8s.io/v1" + "github.com/rancher/webhook/pkg/resources/common" admissionv1 "k8s.io/api/admission/v1" admissionregistrationv1 "k8s.io/api/admissionregistration/v1" - v1 "k8s.io/api/rbac/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime/schema" - "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/utils/trace" ) @@ -42,16 +41,13 @@ func (v *Validator) GVR() schema.GroupVersionResource { // Operations returns list of operations handled by this validator. func (v *Validator) Operations() []admissionregistrationv1.OperationType { return []admissionregistrationv1.OperationType{ - // admissionv1.Create, TODO Not sure if needed admissionregistrationv1.Update, } } // ValidatingWebhook returns the ValidatingWebhook used for this CRD. func (v *Validator) ValidatingWebhook(clientConfig admissionregistrationv1.WebhookClientConfig) []admissionregistrationv1.ValidatingWebhook { - webhook := admission.NewDefaultValidatingWebhook(v, clientConfig, admissionregistrationv1.NamespacedScope, v.Operations()) - webhook.ObjectSelector = &metav1.LabelSelector{ MatchExpressions: []metav1.LabelSelectorRequirement{ { @@ -82,41 +78,9 @@ func (a *admitter) Admit(request *admission.Request) (*admissionv1.AdmissionResp return nil, err } - fldPath := field.NewPath("rolebinding") - - fieldErr := validateGRBOwnerLabel(oldRoleBinding, newRoleBinding, fldPath) - if fieldErr != nil { - return admission.ResponseBadRequest(fieldErr.Error()), nil + if common.IsModifyingLabel(oldRoleBinding.Labels, newRoleBinding.Labels, grbOwnerLabel) { + return admission.ResponseBadRequest(fmt.Sprintf("cannot modify or remove label %s", grbOwnerLabel)), nil } return admission.ResponseAllowed(), nil } - -// validateGRBOwnerLabel checks that a user isn't removing or modifying the grbOwnerLabel. -// Users can add the label if it didn't already exist. -func validateGRBOwnerLabel(old, new *v1.RoleBinding, fldPath *field.Path) *field.Error { - var oldOwnerLabelExists, newOwnerLabelExists bool - var oldOwner, newOwner string - if old.Labels == nil { - oldOwnerLabelExists = false - } else { - oldOwner, oldOwnerLabelExists = old.Labels[grbOwnerLabel] - } - if new.Labels == nil { - newOwnerLabelExists = false - } else { - newOwner, newOwnerLabelExists = new.Labels[grbOwnerLabel] - } - - if !oldOwnerLabelExists { - return nil - } - if oldOwnerLabelExists && !newOwnerLabelExists { - return field.Forbidden(fldPath, fmt.Sprintf("cannot remove label %s", grbOwnerLabel)) - } - if oldOwner != newOwner { - return field.Forbidden(fldPath, fmt.Sprintf("cannot modify label %s", grbOwnerLabel)) - } - - return nil -} diff --git a/pkg/resources/rbac.authorization.k8s.io/v1/rolebinding/validator_test.go b/pkg/resources/rbac.authorization.k8s.io/v1/rolebinding/validator_test.go index 0575df02a..0241354d9 100644 --- a/pkg/resources/rbac.authorization.k8s.io/v1/rolebinding/validator_test.go +++ b/pkg/resources/rbac.authorization.k8s.io/v1/rolebinding/validator_test.go @@ -26,6 +26,12 @@ func TestAdmit(t *testing.T) { Name: "default", }, } + emptyLabelsRoleBinding := &v1.RoleBinding{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{}, + Name: "default", + }, + } roleBindingWithOwnerLabel := &v1.RoleBinding{ ObjectMeta: metav1.ObjectMeta{ Name: "default", @@ -34,11 +40,18 @@ func TestAdmit(t *testing.T) { }, }, } + roleBindingWithNewOwnerLabel := &v1.RoleBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: "default", + Labels: map[string]string{ + grbOwnerLabel: "new-owner", + }, + }, + } type args struct { oldRB *v1.RoleBinding newRB *v1.RoleBinding - op admissionv1.Operation } type test struct { name string @@ -48,42 +61,74 @@ func TestAdmit(t *testing.T) { } tests := []test{ { - name: "create allowed", + name: "updating with 2 nil labels maps allowed", args: args{ oldRB: defaultRoleBinding.DeepCopy(), - op: admissionv1.Create, + newRB: defaultRoleBinding.DeepCopy(), }, allowed: true, }, { - name: "create with grb-owner label allowed", + name: "updating with 2 empty labels maps allowed", args: args{ - oldRB: roleBindingWithOwnerLabel.DeepCopy(), - op: admissionv1.Create, + oldRB: emptyLabelsRoleBinding.DeepCopy(), + newRB: emptyLabelsRoleBinding.DeepCopy(), }, allowed: true, }, { - name: "update grb-owner label not allowed", + name: "updating labels other than grbOwner allowed", args: args{ oldRB: roleBindingWithOwnerLabel.DeepCopy(), newRB: &v1.RoleBinding{ ObjectMeta: metav1.ObjectMeta{ + Name: "default", Labels: map[string]string{ - grbOwnerLabel: "new-owner", + "new-label": "test-value", + grbOwnerLabel: "grb-owner", }, }, }, - op: admissionv1.Update, + }, + allowed: true, + }, + { + name: "adding grb-owner label allowed", + args: args{ + oldRB: defaultRoleBinding.DeepCopy(), + newRB: roleBindingWithOwnerLabel.DeepCopy(), + }, + allowed: true, + }, + { + name: "adding grb-owner label to empty labels map allowed", + args: args{ + oldRB: emptyLabelsRoleBinding.DeepCopy(), + newRB: roleBindingWithOwnerLabel.DeepCopy(), + }, + allowed: true, + }, + { + name: "modifying grb-owner label not allowed", + args: args{ + oldRB: roleBindingWithOwnerLabel.DeepCopy(), + newRB: roleBindingWithNewOwnerLabel.DeepCopy(), }, allowed: false, }, { - name: "remove grb-owner label not allowed", + name: "removing grb-owner label not allowed", args: args{ oldRB: roleBindingWithOwnerLabel.DeepCopy(), newRB: defaultRoleBinding.DeepCopy(), - op: admissionv1.Update, + }, + allowed: false, + }, + { + name: "replacing labels with empty map not allowed", + args: args{ + oldRB: roleBindingWithOwnerLabel.DeepCopy(), + newRB: emptyLabelsRoleBinding.DeepCopy(), }, allowed: false, }, @@ -101,7 +146,7 @@ func TestAdmit(t *testing.T) { Resource: gvr, RequestKind: &gvk, RequestResource: &gvr, - Operation: test.args.op, + Operation: admissionv1.Update, Object: runtime.RawExtension{}, OldObject: runtime.RawExtension{}, }, diff --git a/pkg/server/handlers.go b/pkg/server/handlers.go index 2d4c14afa..4725baecc 100644 --- a/pkg/server/handlers.go +++ b/pkg/server/handlers.go @@ -38,8 +38,8 @@ func Validation(clients *clients.Clients) ([]admission.ValidatingAdmissionHandle prtbResolver := resolvers.NewPRTBRuleResolver(clients.Management.ProjectRoleTemplateBinding().Cache(), clients.RoleTemplateResolver) grbResolver := resolvers.NewGRBClusterRuleResolver(clients.Management.GlobalRoleBinding().Cache(), clients.GlobalRoleResolver) psact := podsecurityadmissionconfigurationtemplate.NewValidator(clients.Management.Cluster().Cache(), clients.Provisioning.Cluster().Cache()) - globalRoles := globalrole.NewValidator(clients.DefaultResolver, grbResolver, clients.K8s.AuthorizationV1().SubjectAccessReviews()) - globalRoleBindings := globalrolebinding.NewValidator(clients.DefaultResolver, grbResolver, clients.K8s.AuthorizationV1().SubjectAccessReviews()) + globalRoles := globalrole.NewValidator(clients.DefaultResolver, grbResolver, clients.K8s.AuthorizationV1().SubjectAccessReviews(), clients.Core.Namespace().Cache()) + globalRoleBindings := globalrolebinding.NewValidator(clients.DefaultResolver, grbResolver, clients.K8s.AuthorizationV1().SubjectAccessReviews(), clients.Core.Namespace().Cache()) prtbs := projectroletemplatebinding.NewValidator(prtbResolver, crtbResolver, clients.DefaultResolver, clients.RoleTemplateResolver, clients.Management.Cluster().Cache(), clients.Management.Project().Cache()) crtbs := clusterroletemplatebinding.NewValidator(crtbResolver, clients.DefaultResolver, clients.RoleTemplateResolver, clients.Management.GlobalRoleBinding().Cache(), clients.Management.Cluster().Cache()) roleTemplates := roletemplate.NewValidator(clients.DefaultResolver, clients.RoleTemplateResolver, clients.K8s.AuthorizationV1().SubjectAccessReviews(), clients.Management.GlobalRole().Cache())