Address comments

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
 

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

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=

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
+}

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
 

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,
 	}

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
+}

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{