diff --git a/docs.md b/docs.md index 162574786..14d4f6c7c 100644 --- a/docs.md +++ b/docs.md @@ -122,7 +122,7 @@ On create or update, the following checks take place: 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`. -This escalation checking currently prevents service accounts from modifying GlobalRoles which include permissions on downstream clusters (such as Admin, Restricted Admin, or GlobalRoles which use the `inheritedClusterRoles` field). +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`). #### Builtin Validation @@ -140,12 +140,12 @@ Note: all checks are bypassed if the GlobalRoleBinding is being deleted, or if o Users can only create/update GlobalRoleBindings with rights less than or equal to those they currently possess. This is to prevent privilege escalation. -This escalation checking currently prevents service accounts from modifying GlobalRoleBindings which give access to GlobalRoles which include permissions on downstream clusters (such as Admin, Restricted Admin, or GlobalRoles which use the `inheritedClusterRoles` field). - #### Valid Global Role Reference GlobalRoleBindings must refer to a valid global role (i.e. an existing `GlobalRole` object in the `management.cattle.io/v3` apiGroup). +This escalation check is bypassed if a user has the `bind` verb on the GlobalRole that they are trying to bind to (through creating or updating a GlobalRoleBinding to that GlobalRole). This can also be given through a wildcard permission (i.e. the `*` verb also gives `bind`). + #### Invalid Fields - Update Users cannot update the following fields after creation: - `userName` diff --git a/pkg/auth/escalation.go b/pkg/auth/escalation.go index 42e504ada..50cf5cf34 100644 --- a/pkg/auth/escalation.go +++ b/pkg/auth/escalation.go @@ -24,8 +24,8 @@ const ( CreatorIDAnn = "field.cattle.io/creatorId" ) -// EscalationAuthorized checks if the user associated with the context is explicitly authorized to escalate the given GVR. -func EscalationAuthorized(request *admission.Request, gvr schema.GroupVersionResource, sar authorizationv1.SubjectAccessReviewInterface, namespace string) (bool, error) { +// RequestUserHasVerb checks if the user associated with the context has a given verb on a given gvr for a specified name/namespace +func RequestUserHasVerb(request *admission.Request, gvr schema.GroupVersionResource, sar authorizationv1.SubjectAccessReviewInterface, verb, name, namespace string) (bool, error) { extras := map[string]v1.ExtraValue{} for k, v := range request.UserInfo.Extra { extras[k] = v1.ExtraValue(v) @@ -34,11 +34,12 @@ func EscalationAuthorized(request *admission.Request, gvr schema.GroupVersionRes resp, err := sar.Create(request.Context, &v1.SubjectAccessReview{ Spec: v1.SubjectAccessReviewSpec{ ResourceAttributes: &v1.ResourceAttributes{ - Verb: "escalate", + Verb: verb, Namespace: namespace, Version: gvr.Version, Resource: gvr.Resource, Group: gvr.Group, + Name: name, }, User: request.UserInfo.Username, Groups: request.UserInfo.Groups, diff --git a/pkg/auth/escalation_test.go b/pkg/auth/escalation_test.go index ea1ab7df8..f56843054 100644 --- a/pkg/auth/escalation_test.go +++ b/pkg/auth/escalation_test.go @@ -166,7 +166,7 @@ func (e *EscalationSuite) TestConfirmNoEscalation() { } } -func (e *EscalationSuite) TestEscalationAuthorized() { +func (e *EscalationSuite) TestRequestUserHasVerb() { gvr := schema.GroupVersionResource{ Group: "management.cattle.io", Version: "v3", @@ -194,7 +194,8 @@ func (e *EscalationSuite) TestEscalationAuthorized() { spec.ResourceAttributes.Group == gvr.Group && spec.ResourceAttributes.Resource == gvr.Resource && spec.ResourceAttributes.Namespace == namespace && - spec.ResourceAttributes.Verb == "escalate" + spec.ResourceAttributes.Verb == "escalate" && + spec.ResourceAttributes.Name == "test-resource" return true, review, nil }) tests := []struct { @@ -225,7 +226,7 @@ func (e *EscalationSuite) TestEscalationAuthorized() { for i := range tests { test := tests[i] e.Run(test.name, func() { - got, err := auth.EscalationAuthorized(test.request, gvr, fakeSAR, namespace) + got, err := auth.RequestUserHasVerb(test.request, gvr, fakeSAR, "escalate", "test-resource", namespace) if test.wantErr { e.Error(err, "expected tests to have error.") } else { diff --git a/pkg/auth/globalrole.go b/pkg/auth/globalrole.go index 2141970b5..35084de30 100644 --- a/pkg/auth/globalrole.go +++ b/pkg/auth/globalrole.go @@ -16,7 +16,7 @@ type GlobalRoleResolver struct { const ownerRT = "cluster-owner" -var adminRoles = []string{"admin", "restricted-admin"} +var adminRoles = []string{"restricted-admin"} // NewRoleTemplateResolver creates a newly allocated RoleTemplateResolver from the provided caches func NewGlobalRoleResolver(roleTemplateResolver *RoleTemplateResolver, grCache controllerv3.GlobalRoleCache) *GlobalRoleResolver { @@ -46,8 +46,8 @@ func (g *GlobalRoleResolver) ClusterRulesFromRole(gr *v3.GlobalRole) ([]rbacv1.P if gr == nil { return nil, nil } - // admin and restricted admin are treated like they are owners of all downstream clusters - // but they don't get the same field because this would duplicate legacy logic + // restricted admin is treated like it is owner of all downstream clusters + // but it doesn't get the same field because this would duplicate legacy logic for _, name := range adminRoles { if gr.Name == name { templateRules, err := g.roleTemplateResolver.RulesFromTemplateName(ownerRT) diff --git a/pkg/auth/globalrole_test.go b/pkg/auth/globalrole_test.go index 58f87e5de..a9e167276 100644 --- a/pkg/auth/globalrole_test.go +++ b/pkg/auth/globalrole_test.go @@ -215,20 +215,6 @@ func TestClusterRulesFromRole(t *testing.T) { }, wantRules: append(append(noInheritRules, firstRTRules...), secondRTRules...), }, - { - name: "test admin gr", - globalRole: &v3.GlobalRole{ - ObjectMeta: metav1.ObjectMeta{ - Name: "admin", - }, - Rules: globalRules, - InheritedClusterRoles: []string{}, - }, - stateSetup: func(state testState) { - state.rtCacheMock.EXPECT().Get("cluster-owner").Return(adminRT, nil) - }, - wantRules: adminRTRules, - }, { name: "test restricted admin gr", globalRole: &v3.GlobalRole{ diff --git a/pkg/resolvers/grbClusterResolver.go b/pkg/resolvers/grbClusterResolver.go index ae5abc310..e40cabc8e 100644 --- a/pkg/resolvers/grbClusterResolver.go +++ b/pkg/resolvers/grbClusterResolver.go @@ -1,19 +1,13 @@ package resolvers import ( - "context" "fmt" - "time" apisv3 "github.com/rancher/rancher/pkg/apis/management.cattle.io/v3" "github.com/rancher/webhook/pkg/auth" v3 "github.com/rancher/webhook/pkg/generated/controllers/management.cattle.io/v3" - v1 "k8s.io/api/authorization/v1" rbacv1 "k8s.io/api/rbac/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apiserver/pkg/authentication/serviceaccount" "k8s.io/apiserver/pkg/authentication/user" - authorizationv1 "k8s.io/client-go/kubernetes/typed/authorization/v1" ) const ( @@ -21,37 +15,20 @@ const ( localCluster = "local" ) -var ( - exceptionServiceAccounts = []string{"rancher-backup", "fleet-agent"} - adminRules = []rbacv1.PolicyRule{ - { - APIGroups: []string{rbacv1.APIGroupAll}, - Resources: []string{rbacv1.ResourceAll}, - Verbs: []string{rbacv1.VerbAll}, - }, - { - NonResourceURLs: []string{rbacv1.NonResourceAll}, - Verbs: []string{rbacv1.VerbAll}, - }, - } -) - // GRBClusterRuleResolver implements the rbacv1.AuthorizationRuleResolver interface. Provides rule resolution // for the permissions a GRB gives that apply in a given cluster (or all clusters). type GRBClusterRuleResolver struct { GlobalRoleBindings v3.GlobalRoleBindingCache GlobalRoleResolver *auth.GlobalRoleResolver - sar authorizationv1.SubjectAccessReviewInterface } // New NewGRBClusterRuleResolver returns a new resolver for resolving rules given through GlobalRoleBindings // which apply to cluster(s). This function can only be called once for each unique instance of grbCache. -func NewGRBClusterRuleResolver(grbCache v3.GlobalRoleBindingCache, grResolver *auth.GlobalRoleResolver, sar authorizationv1.SubjectAccessReviewInterface) *GRBClusterRuleResolver { +func NewGRBClusterRuleResolver(grbCache v3.GlobalRoleBindingCache, grResolver *auth.GlobalRoleResolver) *GRBClusterRuleResolver { grbCache.AddIndexer(grbSubjectIndex, grbBySubject) return &GRBClusterRuleResolver{ GlobalRoleBindings: grbCache, GlobalRoleResolver: grResolver, - sar: sar, } } @@ -109,77 +86,6 @@ func (g *GRBClusterRuleResolver) VisitRulesFor(user user.Info, namespace string, return } } - // if we aren't an exception service account, no need to check sa-specific permissions - if !isExceptionServiceAccount(user) { - return - } - isAdmin, err := g.hasAdminPermissions(user) - if err != nil { - visitor(nil, nil, err) - return - } - if isAdmin { - // exception service accounts are considered to have full permissions for the purposes of the clusterRules - if !visitRules(nil, adminRules, nil, visitor) { - return - } - } -} - -// hasAdminPermissions checks if a given user is an admin -func (g *GRBClusterRuleResolver) hasAdminPermissions(user user.Info) (bool, error) { - extras := map[string]v1.ExtraValue{} - for extraKey, extraValue := range user.GetExtra() { - extras[extraKey] = v1.ExtraValue(extraValue) - } - ctx, cancel := context.WithTimeout(context.Background(), time.Second*5) - defer cancel() - resourceResponse, err := g.sar.Create(ctx, &v1.SubjectAccessReview{ - Spec: v1.SubjectAccessReviewSpec{ - ResourceAttributes: &v1.ResourceAttributes{ - Verb: rbacv1.VerbAll, - Group: rbacv1.APIGroupAll, - Version: rbacv1.APIGroupAll, - Resource: rbacv1.ResourceAll, - }, - User: user.GetName(), - Groups: user.GetGroups(), - UID: user.GetUID(), - Extra: extras, - }}, metav1.CreateOptions{}) - if err != nil { - return false, fmt.Errorf("unable to create sar for user %s: %w", user.GetName(), err) - } - if resourceResponse == nil { - return false, fmt.Errorf("no sar returned from create request for user %s", user.GetName()) - } - return resourceResponse.Status.Allowed, nil -} - -// isExceptionServiceAccount checks if the specified user is one of the rancher service accounts which need to interact -// with globalRoles but don't themselves have grb permissions -func isExceptionServiceAccount(user user.Info) bool { - isExceptionUser := false - _, saName, err := serviceaccount.SplitUsername(user.GetName()) - if err != nil { - // an error indicates that this wasn't a service account, so we can return early - return false - } - for _, exceptionUsername := range exceptionServiceAccounts { - if saName == exceptionUsername { - isExceptionUser = true - break - } - } - if !isExceptionUser { - return false - } - for _, group := range user.GetGroups() { - if group == serviceaccount.AllServiceAccountsGroup { - return true - } - } - return false } // grbBySubject indexes a GRB using the subject as the key. diff --git a/pkg/resolvers/grbClusterResolver_test.go b/pkg/resolvers/grbClusterResolver_test.go index 0ea6676f9..b8ac4c61d 100644 --- a/pkg/resolvers/grbClusterResolver_test.go +++ b/pkg/resolvers/grbClusterResolver_test.go @@ -9,30 +9,22 @@ import ( "github.com/rancher/webhook/pkg/auth" "github.com/rancher/wrangler/pkg/generic/fake" "github.com/stretchr/testify/suite" - v1 "k8s.io/api/authorization/v1" rbacv1 "k8s.io/api/rbac/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" "k8s.io/apiserver/pkg/authentication/user" - k8fake "k8s.io/client-go/kubernetes/typed/authorization/v1/fake" - k8testing "k8s.io/client-go/testing" ) type GRBClusterRuleResolverSuite struct { suite.Suite - userInfo user.Info - broUserInfo user.Info - broDefaultUserInfo user.Info - fleetUserInfo user.Info - otherSAUserInfo user.Info - groupGRB *v3.GlobalRoleBinding - errorGroupGRB *v3.GlobalRoleBinding - userGRB *v3.GlobalRoleBinding - errorUserGRB *v3.GlobalRoleBinding - globalUserRules []rbacv1.PolicyRule - globalGroupRules []rbacv1.PolicyRule - userClusterRules []rbacv1.PolicyRule - groupClusterRules []rbacv1.PolicyRule + userInfo user.Info + groupGRB *v3.GlobalRoleBinding + errorGroupGRB *v3.GlobalRoleBinding + userGRB *v3.GlobalRoleBinding + errorUserGRB *v3.GlobalRoleBinding + globalUserRules []rbacv1.PolicyRule + globalGroupRules []rbacv1.PolicyRule + userClusterRules []rbacv1.PolicyRule + groupClusterRules []rbacv1.PolicyRule } func TestGRBClusterRuleResolver(t *testing.T) { @@ -42,10 +34,6 @@ func TestGRBClusterRuleResolver(t *testing.T) { func (g *GRBClusterRuleResolverSuite) SetupSuite() { g.userInfo = NewUserInfo("test-user", "test-group") - g.broUserInfo = NewUserInfo("system:serviceaccount:cattle-resources-system:rancher-backup", "system:serviceaccounts") - g.broDefaultUserInfo = NewUserInfo("system:serviceaccount:default:rancher-backup", "system:serviceaccounts") - g.fleetUserInfo = NewUserInfo("system:serviceaccount:cattle-fleet-local-system:fleet-agent", "system:serviceaccounts") - g.otherSAUserInfo = NewUserInfo("system:serviceaccount:default:default", "system:serviceaccounts") g.groupGRB = &v3.GlobalRoleBinding{ ObjectMeta: metav1.ObjectMeta{ Name: "group-grb", @@ -110,12 +98,10 @@ func (g *GRBClusterRuleResolverSuite) TestGRBClusterRuleResolver() { grCache *fake.MockNonNamespacedCacheInterface[*v3.GlobalRole] grbCache *fake.MockNonNamespacedCacheInterface[*v3.GlobalRoleBinding] rtCache *fake.MockNonNamespacedCacheInterface[*v3.RoleTemplate] - sar *k8fake.FakeSubjectAccessReviews } tests := []struct { name string - userInfo user.Info setup func(state testState) namespace string wantRules []rbacv1.PolicyRule @@ -124,7 +110,6 @@ func (g *GRBClusterRuleResolverSuite) TestGRBClusterRuleResolver() { { name: "test rule resolution, valid + invalid user/group bindings", namespace: "", - userInfo: g.userInfo, setup: func(state testState) { state.grbCache.EXPECT().GetByIndex(grbSubjectIndex, GetUserKey(g.userInfo.GetName(), "")).Return([]*v3.GlobalRoleBinding{g.userGRB, g.errorUserGRB}, nil) state.grbCache.EXPECT().GetByIndex(grbSubjectIndex, GetGroupKey(g.userInfo.GetGroups()[0], "")).Return([]*v3.GlobalRoleBinding{g.groupGRB, g.errorGroupGRB}, nil) @@ -164,7 +149,6 @@ func (g *GRBClusterRuleResolverSuite) TestGRBClusterRuleResolver() { { name: "test state resolution group indexer error", namespace: "", - userInfo: g.userInfo, setup: func(state testState) { state.grbCache.EXPECT().GetByIndex(grbSubjectIndex, GetUserKey(g.userInfo.GetName(), "")).Return([]*v3.GlobalRoleBinding{g.userGRB, g.errorUserGRB}, nil) state.grbCache.EXPECT().GetByIndex(grbSubjectIndex, GetGroupKey(g.userInfo.GetGroups()[0], "")).Return(nil, fmt.Errorf("server unavailable")) @@ -190,7 +174,6 @@ func (g *GRBClusterRuleResolverSuite) TestGRBClusterRuleResolver() { { name: "test state resolution user indexer error", namespace: "", - userInfo: g.userInfo, setup: func(state testState) { state.grbCache.EXPECT().GetByIndex(grbSubjectIndex, GetUserKey(g.userInfo.GetName(), "")).Return(nil, fmt.Errorf("server unavailable")) state.grbCache.EXPECT().GetByIndex(grbSubjectIndex, GetGroupKey(g.userInfo.GetGroups()[0], "")).Return([]*v3.GlobalRoleBinding{g.groupGRB, g.errorGroupGRB}, nil) @@ -217,7 +200,6 @@ func (g *GRBClusterRuleResolverSuite) TestGRBClusterRuleResolver() { { name: "test state resolution local cluster", namespace: "local", - userInfo: g.userInfo, setup: func(state testState) { state.grbCache.EXPECT().GetByIndex(grbSubjectIndex, GetUserKey(g.userInfo.GetName(), "")).Return([]*v3.GlobalRoleBinding{g.userGRB, g.errorUserGRB}, nil) state.grbCache.EXPECT().GetByIndex(grbSubjectIndex, GetGroupKey(g.userInfo.GetGroups()[0], "")).Return([]*v3.GlobalRoleBinding{g.groupGRB, g.errorGroupGRB}, nil) @@ -246,7 +228,6 @@ func (g *GRBClusterRuleResolverSuite) TestGRBClusterRuleResolver() { { name: "test state resolution non-local cluster", namespace: "not-local", - userInfo: g.userInfo, setup: func(state testState) { state.grbCache.EXPECT().GetByIndex(grbSubjectIndex, GetUserKey(g.userInfo.GetName(), "")).Return([]*v3.GlobalRoleBinding{g.userGRB}, nil) state.grbCache.EXPECT().GetByIndex(grbSubjectIndex, GetGroupKey(g.userInfo.GetGroups()[0], "")).Return([]*v3.GlobalRoleBinding{g.groupGRB}, nil) @@ -286,7 +267,6 @@ func (g *GRBClusterRuleResolverSuite) TestGRBClusterRuleResolver() { { name: "test state resolution no error, no namespace", namespace: "", - userInfo: g.userInfo, setup: func(state testState) { state.grbCache.EXPECT().GetByIndex(grbSubjectIndex, GetUserKey(g.userInfo.GetName(), "")).Return([]*v3.GlobalRoleBinding{g.userGRB}, nil) state.grbCache.EXPECT().GetByIndex(grbSubjectIndex, GetGroupKey(g.userInfo.GetGroups()[0], "")).Return([]*v3.GlobalRoleBinding{g.groupGRB}, nil) @@ -323,276 +303,6 @@ func (g *GRBClusterRuleResolverSuite) TestGRBClusterRuleResolver() { wantRules: append(g.userClusterRules, g.groupClusterRules...), wantError: false, }, - { - name: "test backup-restore service account", - namespace: "", - userInfo: g.broUserInfo, - setup: func(state testState) { - state.grbCache.EXPECT().GetByIndex(grbSubjectIndex, GetUserKey(g.broUserInfo.GetName(), "")).Return([]*v3.GlobalRoleBinding{}, nil) - state.grbCache.EXPECT().GetByIndex(grbSubjectIndex, GetGroupKey(g.broUserInfo.GetGroups()[0], "")).Return([]*v3.GlobalRoleBinding{}, nil) - state.sar.Fake.AddReactor("create", "subjectaccessreviews", func(action k8testing.Action) (handled bool, ret runtime.Object, err error) { - createAction := action.(k8testing.CreateActionImpl) - review := createAction.GetObject().(*v1.SubjectAccessReview) - spec := review.Spec - attributes := spec.ResourceAttributes - isAdminPerm := attributes != nil && attributes.Group == rbacv1.APIGroupAll && - attributes.Resource == rbacv1.ResourceAll && attributes.Verb == rbacv1.VerbAll && - attributes.Version == rbacv1.APIGroupAll - if isAdminPerm && spec.User == g.broUserInfo.GetName() && len(spec.Groups) == 1 && spec.Groups[0] == g.broUserInfo.GetGroups()[0] { - review.Status.Allowed = true - return true, review, nil - } - return false, nil, nil - }) - - }, - - wantRules: adminRules, - wantError: false, - }, - { - name: "test backup-restore service account, non-admin", - namespace: "", - userInfo: g.broUserInfo, - setup: func(state testState) { - state.grbCache.EXPECT().GetByIndex(grbSubjectIndex, GetUserKey(g.broUserInfo.GetName(), "")).Return([]*v3.GlobalRoleBinding{}, nil) - state.grbCache.EXPECT().GetByIndex(grbSubjectIndex, GetGroupKey(g.broUserInfo.GetGroups()[0], "")).Return([]*v3.GlobalRoleBinding{}, nil) - state.sar.Fake.AddReactor("create", "subjectaccessreviews", func(action k8testing.Action) (handled bool, ret runtime.Object, err error) { - createAction := action.(k8testing.CreateActionImpl) - review := createAction.GetObject().(*v1.SubjectAccessReview) - spec := review.Spec - attributes := spec.ResourceAttributes - isAdminPerm := attributes != nil && attributes.Group == rbacv1.APIGroupAll && - attributes.Resource == rbacv1.ResourceAll && attributes.Verb == rbacv1.VerbAll && - attributes.Version == rbacv1.APIGroupAll - if isAdminPerm && spec.User == g.broUserInfo.GetName() && len(spec.Groups) == 1 && spec.Groups[0] == g.broUserInfo.GetGroups()[0] { - review.Status.Allowed = false - return true, review, nil - } - return false, nil, nil - }) - - }, - - wantRules: []rbacv1.PolicyRule{}, - wantError: false, - }, - { - name: "test backup-restore service account, error when evaluting SAR", - namespace: "", - userInfo: g.broUserInfo, - setup: func(state testState) { - state.grbCache.EXPECT().GetByIndex(grbSubjectIndex, GetUserKey(g.broUserInfo.GetName(), "")).Return([]*v3.GlobalRoleBinding{}, nil) - state.grbCache.EXPECT().GetByIndex(grbSubjectIndex, GetGroupKey(g.broUserInfo.GetGroups()[0], "")).Return([]*v3.GlobalRoleBinding{}, nil) - state.sar.Fake.AddReactor("create", "subjectaccessreviews", func(action k8testing.Action) (handled bool, ret runtime.Object, err error) { - createAction := action.(k8testing.CreateActionImpl) - review := createAction.GetObject().(*v1.SubjectAccessReview) - spec := review.Spec - attributes := spec.ResourceAttributes - isAdminPerm := attributes != nil && attributes.Group == rbacv1.APIGroupAll && - attributes.Resource == rbacv1.ResourceAll && attributes.Verb == rbacv1.VerbAll && - attributes.Version == rbacv1.APIGroupAll - if isAdminPerm && spec.User == g.broUserInfo.GetName() && len(spec.Groups) == 1 && spec.Groups[0] == g.broUserInfo.GetGroups()[0] { - return true, review, fmt.Errorf("unable to process request") - } - return false, nil, nil - }) - - }, - wantRules: []rbacv1.PolicyRule{}, - wantError: true, - }, - { - name: "test backup-restore service account, nil SAR result", - namespace: "", - userInfo: g.broUserInfo, - setup: func(state testState) { - state.grbCache.EXPECT().GetByIndex(grbSubjectIndex, GetUserKey(g.broUserInfo.GetName(), "")).Return([]*v3.GlobalRoleBinding{}, nil) - state.grbCache.EXPECT().GetByIndex(grbSubjectIndex, GetGroupKey(g.broUserInfo.GetGroups()[0], "")).Return([]*v3.GlobalRoleBinding{}, nil) - state.sar.Fake.AddReactor("create", "subjectaccessreviews", func(action k8testing.Action) (handled bool, ret runtime.Object, err error) { - createAction := action.(k8testing.CreateActionImpl) - review := createAction.GetObject().(*v1.SubjectAccessReview) - spec := review.Spec - attributes := spec.ResourceAttributes - isAdminPerm := attributes != nil && attributes.Group == rbacv1.APIGroupAll && - attributes.Resource == rbacv1.ResourceAll && attributes.Verb == rbacv1.VerbAll && - attributes.Version == rbacv1.APIGroupAll - if isAdminPerm && spec.User == g.broUserInfo.GetName() && len(spec.Groups) == 1 && spec.Groups[0] == g.broUserInfo.GetGroups()[0] { - return true, nil, nil - } - return false, nil, nil - }) - - }, - wantRules: []rbacv1.PolicyRule{}, - wantError: true, - }, - { - name: "test fleet service account", - namespace: "", - userInfo: g.fleetUserInfo, - setup: func(state testState) { - state.grbCache.EXPECT().GetByIndex(grbSubjectIndex, GetUserKey(g.fleetUserInfo.GetName(), "")).Return([]*v3.GlobalRoleBinding{}, nil) - state.grbCache.EXPECT().GetByIndex(grbSubjectIndex, GetGroupKey(g.fleetUserInfo.GetGroups()[0], "")).Return([]*v3.GlobalRoleBinding{}, nil) - state.sar.Fake.AddReactor("create", "subjectaccessreviews", func(action k8testing.Action) (handled bool, ret runtime.Object, err error) { - createAction := action.(k8testing.CreateActionImpl) - review := createAction.GetObject().(*v1.SubjectAccessReview) - spec := review.Spec - attributes := spec.ResourceAttributes - isAdminPerm := attributes != nil && attributes.Group == rbacv1.APIGroupAll && - attributes.Resource == rbacv1.ResourceAll && attributes.Verb == rbacv1.VerbAll && - attributes.Version == rbacv1.APIGroupAll - if isAdminPerm && spec.User == g.fleetUserInfo.GetName() && len(spec.Groups) == 1 && spec.Groups[0] == g.fleetUserInfo.GetGroups()[0] { - review.Status.Allowed = true - return true, review, nil - } - return false, nil, nil - }) - - }, - - wantRules: adminRules, - wantError: false, - }, - { - name: "test fleet service account, non-admin permissions", - namespace: "", - userInfo: g.fleetUserInfo, - setup: func(state testState) { - state.grbCache.EXPECT().GetByIndex(grbSubjectIndex, GetUserKey(g.fleetUserInfo.GetName(), "")).Return([]*v3.GlobalRoleBinding{}, nil) - state.grbCache.EXPECT().GetByIndex(grbSubjectIndex, GetGroupKey(g.fleetUserInfo.GetGroups()[0], "")).Return([]*v3.GlobalRoleBinding{}, nil) - state.sar.Fake.AddReactor("create", "subjectaccessreviews", func(action k8testing.Action) (handled bool, ret runtime.Object, err error) { - createAction := action.(k8testing.CreateActionImpl) - review := createAction.GetObject().(*v1.SubjectAccessReview) - spec := review.Spec - attributes := spec.ResourceAttributes - isAdminPerm := attributes != nil && attributes.Group == rbacv1.APIGroupAll && - attributes.Resource == rbacv1.ResourceAll && attributes.Verb == rbacv1.VerbAll && - attributes.Version == rbacv1.APIGroupAll - if isAdminPerm && spec.User == g.fleetUserInfo.GetName() && len(spec.Groups) == 1 && spec.Groups[0] == g.fleetUserInfo.GetGroups()[0] { - review.Status.Allowed = false - return true, review, nil - } - return false, nil, nil - }) - - }, - - wantRules: []rbacv1.PolicyRule{}, - wantError: false, - }, - { - name: "test fleet service account, error when processing SAR", - namespace: "", - userInfo: g.fleetUserInfo, - setup: func(state testState) { - state.grbCache.EXPECT().GetByIndex(grbSubjectIndex, GetUserKey(g.fleetUserInfo.GetName(), "")).Return([]*v3.GlobalRoleBinding{}, nil) - state.grbCache.EXPECT().GetByIndex(grbSubjectIndex, GetGroupKey(g.fleetUserInfo.GetGroups()[0], "")).Return([]*v3.GlobalRoleBinding{}, nil) - state.sar.Fake.AddReactor("create", "subjectaccessreviews", func(action k8testing.Action) (handled bool, ret runtime.Object, err error) { - createAction := action.(k8testing.CreateActionImpl) - review := createAction.GetObject().(*v1.SubjectAccessReview) - spec := review.Spec - attributes := spec.ResourceAttributes - isAdminPerm := attributes != nil && attributes.Group == rbacv1.APIGroupAll && - attributes.Resource == rbacv1.ResourceAll && attributes.Verb == rbacv1.VerbAll && - attributes.Version == rbacv1.APIGroupAll - if isAdminPerm && spec.User == g.fleetUserInfo.GetName() && len(spec.Groups) == 1 && spec.Groups[0] == g.fleetUserInfo.GetGroups()[0] { - return true, review, fmt.Errorf("unable to process sar") - } - return false, nil, nil - }) - - }, - - wantRules: []rbacv1.PolicyRule{}, - wantError: true, - }, - { - name: "test fleet service account, nil SAR result", - namespace: "", - userInfo: g.fleetUserInfo, - setup: func(state testState) { - state.grbCache.EXPECT().GetByIndex(grbSubjectIndex, GetUserKey(g.fleetUserInfo.GetName(), "")).Return([]*v3.GlobalRoleBinding{}, nil) - state.grbCache.EXPECT().GetByIndex(grbSubjectIndex, GetGroupKey(g.fleetUserInfo.GetGroups()[0], "")).Return([]*v3.GlobalRoleBinding{}, nil) - state.sar.Fake.AddReactor("create", "subjectaccessreviews", func(action k8testing.Action) (handled bool, ret runtime.Object, err error) { - createAction := action.(k8testing.CreateActionImpl) - review := createAction.GetObject().(*v1.SubjectAccessReview) - spec := review.Spec - attributes := spec.ResourceAttributes - isAdminPerm := attributes != nil && attributes.Group == rbacv1.APIGroupAll && - attributes.Resource == rbacv1.ResourceAll && attributes.Verb == rbacv1.VerbAll && - attributes.Version == rbacv1.APIGroupAll - if isAdminPerm && spec.User == g.fleetUserInfo.GetName() && len(spec.Groups) == 1 && spec.Groups[0] == g.fleetUserInfo.GetGroups()[0] { - return true, nil, nil - } - return false, nil, nil - }) - - }, - - wantRules: []rbacv1.PolicyRule{}, - wantError: true, - }, - - { - name: "test bro default service account", - namespace: "", - userInfo: g.broDefaultUserInfo, - setup: func(state testState) { - state.grbCache.EXPECT().GetByIndex(grbSubjectIndex, GetUserKey(g.broDefaultUserInfo.GetName(), "")).Return([]*v3.GlobalRoleBinding{}, nil) - state.grbCache.EXPECT().GetByIndex(grbSubjectIndex, GetGroupKey(g.broDefaultUserInfo.GetGroups()[0], "")).Return([]*v3.GlobalRoleBinding{}, nil) - state.sar.Fake.AddReactor("create", "subjectaccessreviews", func(action k8testing.Action) (handled bool, ret runtime.Object, err error) { - createAction := action.(k8testing.CreateActionImpl) - review := createAction.GetObject().(*v1.SubjectAccessReview) - spec := review.Spec - attributes := spec.ResourceAttributes - isAdminPerm := attributes != nil && attributes.Group == rbacv1.APIGroupAll && - attributes.Resource == rbacv1.ResourceAll && attributes.Verb == rbacv1.VerbAll && - attributes.Version == rbacv1.APIGroupAll - if isAdminPerm && spec.User == g.broDefaultUserInfo.GetName() && len(spec.Groups) == 1 && spec.Groups[0] == g.broDefaultUserInfo.GetGroups()[0] { - review.Status.Allowed = true - return true, review, nil - } - return false, nil, nil - }) - - }, - - wantRules: adminRules, - wantError: false, - }, - - { - name: "test other service account", - namespace: "", - userInfo: g.otherSAUserInfo, - setup: func(state testState) { - state.grbCache.EXPECT().GetByIndex(grbSubjectIndex, GetUserKey(g.otherSAUserInfo.GetName(), "")).Return([]*v3.GlobalRoleBinding{}, nil) - state.grbCache.EXPECT().GetByIndex(grbSubjectIndex, GetGroupKey(g.otherSAUserInfo.GetGroups()[0], "")).Return([]*v3.GlobalRoleBinding{}, nil) - }, - wantRules: []rbacv1.PolicyRule{}, - wantError: false, - }, - { - name: "test backup-restore not in sa group", - namespace: "", - userInfo: NewUserInfo(g.broUserInfo.GetName()), - setup: func(state testState) { - state.grbCache.EXPECT().GetByIndex(grbSubjectIndex, GetUserKey(g.broUserInfo.GetName(), "")).Return([]*v3.GlobalRoleBinding{}, nil) - }, - wantRules: []rbacv1.PolicyRule{}, - wantError: false, - }, - { - name: "test fleet not in sa group", - namespace: "", - userInfo: NewUserInfo(g.fleetUserInfo.GetName()), - setup: func(state testState) { - state.grbCache.EXPECT().GetByIndex(grbSubjectIndex, GetUserKey(g.fleetUserInfo.GetName(), "")).Return([]*v3.GlobalRoleBinding{}, nil) - }, - wantRules: []rbacv1.PolicyRule{}, - wantError: false, - }, } for _, test := range tests { @@ -604,14 +314,10 @@ func (g *GRBClusterRuleResolverSuite) TestGRBClusterRuleResolver() { rtCache := fake.NewMockNonNamespacedCacheInterface[*v3.RoleTemplate](ctrl) grCache := fake.NewMockNonNamespacedCacheInterface[*v3.GlobalRole](ctrl) - k8Fake := &k8testing.Fake{} - fakeSAR := &k8fake.FakeSubjectAccessReviews{Fake: &k8fake.FakeAuthorizationV1{Fake: k8Fake}} - state := testState{ grCache: grCache, grbCache: grbCache, rtCache: rtCache, - sar: fakeSAR, } if test.setup != nil { @@ -619,9 +325,9 @@ func (g *GRBClusterRuleResolverSuite) TestGRBClusterRuleResolver() { } grResolver := auth.NewGlobalRoleResolver(auth.NewRoleTemplateResolver(state.rtCache, nil), state.grCache) - grbResolver := NewGRBClusterRuleResolver(state.grbCache, grResolver, state.sar) + grbResolver := NewGRBClusterRuleResolver(state.grbCache, grResolver) - rules, err := grbResolver.RulesFor(test.userInfo, test.namespace) + rules, err := grbResolver.RulesFor(g.userInfo, test.namespace) g.Require().Len(rules, len(test.wantRules)) for _, rule := range test.wantRules { g.Require().Contains(rules, rule) diff --git a/pkg/resolvers/resolvers_test.go b/pkg/resolvers/resolvers_test.go index 9a8a4de0e..570c69e57 100644 --- a/pkg/resolvers/resolvers_test.go +++ b/pkg/resolvers/resolvers_test.go @@ -54,9 +54,6 @@ func NewUserInfo(username string, groups ...string) *user.DefaultInfo { return &user.DefaultInfo{ Name: username, Groups: groups, - Extra: map[string][]string{ - "test": {"test"}, - }, } } diff --git a/pkg/resources/management.cattle.io/v3/globalrole/GlobalRole.md b/pkg/resources/management.cattle.io/v3/globalrole/GlobalRole.md index 84d7114b5..da579cacd 100644 --- a/pkg/resources/management.cattle.io/v3/globalrole/GlobalRole.md +++ b/pkg/resources/management.cattle.io/v3/globalrole/GlobalRole.md @@ -12,7 +12,7 @@ On create or update, the following checks take place: 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`. -This escalation checking currently prevents service accounts from modifying GlobalRoles which include permissions on downstream clusters (such as Admin, Restricted Admin, or GlobalRoles which use the `inheritedClusterRoles` field). +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`). ### 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 17ea2d4ad..a07fa449c 100644 --- a/pkg/resources/management.cattle.io/v3/globalrole/setup_test.go +++ b/pkg/resources/management.cattle.io/v3/globalrole/setup_test.go @@ -18,6 +18,8 @@ import ( v1 "k8s.io/api/rbac/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + k8fake "k8s.io/client-go/kubernetes/typed/authorization/v1/fake" + k8testing "k8s.io/client-go/testing" "k8s.io/kubernetes/pkg/registry/rbac/validation" ) @@ -170,6 +172,7 @@ type testState struct { rtCacheMock *fake.MockNonNamespacedCacheInterface[*v3.RoleTemplate] grCacheMock *fake.MockNonNamespacedCacheInterface[*v3.GlobalRole] grbCacheMock *fake.MockNonNamespacedCacheInterface[*v3.GlobalRoleBinding] + sarMock *k8fake.FakeSubjectAccessReviews resolver validation.AuthorizationRuleResolver } @@ -194,7 +197,7 @@ func createGRRequest(t *testing.T, test testCase) *admission.Request { RequestKind: &gvk, RequestResource: &gvr, Operation: admissionv1.Create, - UserInfo: v1authentication.UserInfo{Username: username, UID: ""}, + UserInfo: v1authentication.UserInfo{Username: username, UID: "", Extra: map[string]v1authentication.ExtraValue{"test": []string{"test"}}}, Object: runtime.RawExtension{}, OldObject: runtime.RawExtension{}, }, @@ -262,17 +265,20 @@ func newDefaultState(t *testing.T) testState { grbCacheMock.EXPECT().AddIndexer(gomock.Any(), gomock.Any()).AnyTimes() grCacheMock.EXPECT().Get(baseGR.Name).Return(&baseGR, nil).AnyTimes() rtCacheMock.EXPECT().Get(baseRT.Name).Return(&baseRT, nil).AnyTimes() + k8Fake := &k8testing.Fake{} + fakeSAR := &k8fake.FakeSubjectAccessReviews{Fake: &k8fake.FakeAuthorizationV1{Fake: k8Fake}} resolver, _ := validation.NewTestRuleResolver(nil, nil, clusterRoles, clusterRoleBindings) return testState{ rtCacheMock: rtCacheMock, grCacheMock: grCacheMock, grbCacheMock: grbCacheMock, + sarMock: fakeSAR, resolver: resolver, } } func (m *testState) createBaseGRBResolver() *resolvers.GRBClusterRuleResolver { grResolver := auth.NewGlobalRoleResolver(auth.NewRoleTemplateResolver(m.rtCacheMock, nil), m.grCacheMock) - return resolvers.NewGRBClusterRuleResolver(m.grbCacheMock, grResolver, nil) + return resolvers.NewGRBClusterRuleResolver(m.grbCacheMock, grResolver) } diff --git a/pkg/resources/management.cattle.io/v3/globalrole/validator.go b/pkg/resources/management.cattle.io/v3/globalrole/validator.go index 87310eaf8..d6f30700e 100644 --- a/pkg/resources/management.cattle.io/v3/globalrole/validator.go +++ b/pkg/resources/management.cattle.io/v3/globalrole/validator.go @@ -12,13 +12,14 @@ import ( 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" - "golang.org/x/exp/slices" + "github.com/sirupsen/logrus" admissionv1 "k8s.io/api/admission/v1" admissionregistrationv1 "k8s.io/api/admissionregistration/v1" rbacv1 "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" + authorizationv1 "k8s.io/client-go/kubernetes/typed/authorization/v1" "k8s.io/kubernetes/pkg/registry/rbac/validation" "k8s.io/utils/trace" ) @@ -29,14 +30,19 @@ var gvr = schema.GroupVersionResource{ Resource: "globalroles", } -const roleTemplateClusterContext = "cluster" +const ( + roleTemplateClusterContext = "cluster" + escalateVerb = "escalate" +) // NewValidator returns a new validator used for validation globalRoles. -func NewValidator(ruleResolver validation.AuthorizationRuleResolver, grbResolver *resolvers.GRBClusterRuleResolver) *Validator { +func NewValidator(ruleResolver validation.AuthorizationRuleResolver, grbResolver *resolvers.GRBClusterRuleResolver, + sar authorizationv1.SubjectAccessReviewInterface) *Validator { return &Validator{ admitter: admitter{ resolver: ruleResolver, grbResolver: grbResolver, + sar: sar, }, } } @@ -69,6 +75,7 @@ func (v *Validator) Admitters() []admission.Admitter { type admitter struct { resolver validation.AuthorizationRuleResolver grbResolver *resolvers.GRBClusterRuleResolver + sar authorizationv1.SubjectAccessReviewInterface } // Admit is the entrypoint for the validator. Admit will return an error if it's unable to process the request. @@ -93,11 +100,6 @@ func (a *admitter) Admit(request *admission.Request) (*admissionv1.AdmissionResp // This is needed to supported the deletion of old GlobalRoles that would not pass the update check that verifies all rules have verbs. return admission.ResponseAllowed(), nil } - if isMetaOnlyChange(oldGR, newGR) { - // if this change only affects metadata, don't validate any further - // this allows users with the appropriate permissions to manage labels/annotations/finalizers - return admission.ResponseAllowed(), nil - } fieldErr := a.validateUpdateFields(oldGR, newGR, fldPath) if fieldErr != nil { return admission.ResponseBadRequest(fieldErr.Error()), nil @@ -124,14 +126,20 @@ 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) } - err = auth.ConfirmNoEscalation(request, clusterRules, "", a.grbResolver) + + hasEscalate, err := a.isRulesAllowed(request, clusterRules, newGR.Name, a.grbResolver) if err != nil { return admission.ResponseFailedEscalation(err.Error()), nil } + if hasEscalate { + // if we have the escalate verb, no need to check global permissions + return admission.ResponseAllowed(), nil + } rules := a.grbResolver.GlobalRoleResolver.GlobalRulesFromRole(newGR) - - err = auth.ConfirmNoEscalation(request, rules, "", a.resolver) + // don't need to check if this request was allowed due to escalation since this is the last permission check + // if others are added in the future a short-circuit here will be needed like for the clusterRules + _, err = a.isRulesAllowed(request, rules, newGR.Name, a.resolver) if err != nil { return admission.ResponseFailedEscalation(err.Error()), nil } @@ -139,6 +147,25 @@ func (a *admitter) Admit(request *admission.Request) (*admissionv1.AdmissionResp return admission.ResponseAllowed(), nil } +// isRulesAllowed checks if the use of requested rules are allowed by the givenResolver for a given request/user +// returns an error if the user failed an escalation check, and nil if the request was allowed. Also returns a bool +// indicating if the allow was due to the user having the escalate verb +func (a *admitter) isRulesAllowed(request *admission.Request, rules []rbacv1.PolicyRule, grName string, resolver validation.AuthorizationRuleResolver) (bool, error) { + err := auth.ConfirmNoEscalation(request, rules, "", resolver) + if err != nil { + hasEscalate, escErr := auth.RequestUserHasVerb(request, gvr, a.sar, escalateVerb, grName, "") + if escErr != nil { + logrus.Warnf("Failed to check for the 'escalate' verb on GlobalRoles: %v", escErr) + return false, err + } + if hasEscalate { + return true, nil + } + return false, err + } + return false, nil +} + // validateDelete checks if a global role can be deleted and returns the appropriate response. func validateDelete(oldRole *v3.GlobalRole, fldPath *field.Path) (*admissionv1.AdmissionResponse, error) { if oldRole.Builtin { @@ -221,71 +248,19 @@ func (a *admitter) validateUpdateFields(oldRole, newRole *v3.GlobalRole, fldPath // ignore changes to meta data and newUserDefault origDefault := oldRole.NewUserDefault + origObjMeta := oldRole.ObjectMeta + origTypeMeta := oldRole.TypeMeta defer func() { oldRole.NewUserDefault = origDefault + oldRole.ObjectMeta = origObjMeta + oldRole.TypeMeta = origTypeMeta }() oldRole.NewUserDefault = newRole.NewUserDefault + oldRole.ObjectMeta = newRole.ObjectMeta + oldRole.TypeMeta = newRole.TypeMeta - if !grContentEqual(oldRole, newRole) { + if !reflect.DeepEqual(oldRole, newRole) { return field.Forbidden(fldPath, "updates to builtIn GlobalRoles for fields other than 'newUserDefault' are forbidden") } return nil } - -// isMetaOnlyChange checks if old and new are deep equal in all fields except metadata and typemeta. Will return false on a -// non-effectual change. -func isMetaOnlyChange(oldGR *v3.GlobalRole, newGR *v3.GlobalRole) bool { - // if the metadata between old/new hasn't changed, then this isn't a metadata only change - if grMetaEqual(oldGR, newGR) { - return false - } - - return grContentEqual(oldGR, newGR) -} - -func grMetaEqual(oldGR *v3.GlobalRole, newGR *v3.GlobalRole) bool { - if oldGR == newGR { - // same pointer or both objects are nil - return true - } - if oldGR == nil || newGR == nil { - // one object is nil and the other is not. - return false - } - return reflect.DeepEqual(oldGR.ObjectMeta, newGR.ObjectMeta) -} - -// grContentEqual checks if two globalRoles have equivalent content (all fields excluding metadata && typeMeta). -// this function is used instead of reflect.DeepEqual since it is less costly and faster. -// this function ignores the typeMeta field of the object. -func grContentEqual(oldGR *v3.GlobalRole, newGR *v3.GlobalRole) bool { - if oldGR == newGR { - // same pointer or both objects are nil - return true - } - if oldGR == nil || newGR == nil { - // one object is nil and the other is not. - return false - } - - return oldGR.DisplayName == newGR.DisplayName && - oldGR.Description == newGR.Description && - oldGR.NewUserDefault == newGR.NewUserDefault && - oldGR.Builtin == newGR.Builtin && - slices.Equal(oldGR.InheritedClusterRoles, newGR.InheritedClusterRoles) && - policyRulesEqual(oldGR.Rules, newGR.Rules) -} - -// policyRulesEqual checks for equivalence between two list of policy rules. -// This function considers both empty list and nil list as equivalent. -func policyRulesEqual(rules1, rules2 []rbacv1.PolicyRule) bool { - return slices.EqualFunc(rules1, rules2, rulesAreEqual) -} - -func rulesAreEqual(rule1, rule2 rbacv1.PolicyRule) bool { - return slices.Equal(rule1.Verbs, rule2.Verbs) && - slices.Equal(rule1.APIGroups, rule2.APIGroups) && - slices.Equal(rule1.Resources, rule2.Resources) && - slices.Equal(rule1.ResourceNames, rule2.ResourceNames) && - slices.Equal(rule1.NonResourceURLs, rule2.NonResourceURLs) -} 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 8d64e9327..72fcd2d43 100644 --- a/pkg/resources/management.cattle.io/v3/globalrole/validator_test.go +++ b/pkg/resources/management.cattle.io/v3/globalrole/validator_test.go @@ -1,6 +1,7 @@ package globalrole_test import ( + "fmt" "testing" "time" @@ -9,11 +10,14 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" admissionv1 "k8s.io/api/admission/v1" + authorizationv1 "k8s.io/api/authorization/v1" v1 "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" "k8s.io/apimachinery/pkg/runtime/schema" + k8fake "k8s.io/client-go/kubernetes/typed/authorization/v1/fake" + k8testing "k8s.io/client-go/testing" "k8s.io/kubernetes/pkg/registry/rbac/validation" ) @@ -164,58 +168,49 @@ func TestAdmit(t *testing.T) { allowed: true, }, { - name: "escalation in global + cluster rules, and invalid RT, but only meta changed", + name: "creating with equal privilege level", // User attempts to create a globalrole with rules equal to one they hold. args: args{ + username: testUser, newGR: func() *v3.GlobalRole { - gr := newDefaultGR() - gr.Labels = map[string]string{ - "new-label": "just-added", - } - gr.Rules = []v1.PolicyRule{ruleAdmin} - return gr - }, - oldGR: func() *v3.GlobalRole { - gr := newDefaultGR() - gr.Rules = []v1.PolicyRule{ruleAdmin} - return gr + baseGR := newDefaultGR() + baseGR.Rules = readPodsCR.Rules + return baseGR }, }, allowed: true, }, { - name: "escalation in global + cluster rules, and invalid RT, but metadata and field changed", + name: "creation with privilege escalation", // User attempts to create a globalrole with more rules than the ones they hold. args: args{ + username: testUser, newGR: func() *v3.GlobalRole { - gr := newDefaultGR() - gr.Labels = map[string]string{ - "new-label": "just-added", - } - gr.Description = "new desc" - gr.Rules = []v1.PolicyRule{ruleAdmin} - return gr + baseGR := newDefaultGR() + baseGR.Rules = adminCR.Rules + return baseGR }, - oldGR: func() *v3.GlobalRole { - gr := newDefaultGR() - gr.Rules = []v1.PolicyRule{ruleAdmin} - return gr + stateSetup: func(state testState) { + setSarResponse(false, nil, testUser, newDefaultGR().Name, state.sarMock) }, }, allowed: false, }, { - name: "creating with equal privilege level", // User attempts to create a globalrole with rules equal to one they hold. + name: "creation with privilege escalation, escalate allowed", args: args{ username: testUser, newGR: func() *v3.GlobalRole { baseGR := newDefaultGR() - baseGR.Rules = readPodsCR.Rules + baseGR.Rules = adminCR.Rules return baseGR }, + stateSetup: func(state testState) { + setSarResponse(true, nil, testUser, newDefaultGR().Name, state.sarMock) + }, }, allowed: true, }, { - name: "creation with privilege escalation", // User attempts to create a globalrole with more rules than the ones they hold. + name: "creation with privilege escalation, escalate check failed", args: args{ username: testUser, newGR: func() *v3.GlobalRole { @@ -223,6 +218,9 @@ func TestAdmit(t *testing.T) { baseGR.Rules = adminCR.Rules return baseGR }, + stateSetup: func(state testState) { + setSarResponse(false, fmt.Errorf("server not available"), testUser, newDefaultGR().Name, state.sarMock) + }, }, allowed: false, }, @@ -240,9 +238,32 @@ func TestAdmit(t *testing.T) { baseGR.Rules = append(baseGR.Rules, ruleReadPods, ruleWriteNodes) return baseGR }, + stateSetup: func(state testState) { + setSarResponse(false, nil, testUser, newDefaultGR().Name, state.sarMock) + }, }, allowed: false, }, + { + name: "update with privilege escalation, escalate allowed", + args: args{ + username: testUser, + oldGR: func() *v3.GlobalRole { + baseGR := newDefaultGR() + baseGR.Rules = readPodsCR.Rules + return baseGR + }, + newGR: func() *v3.GlobalRole { + baseGR := newDefaultGR() + baseGR.Rules = append(baseGR.Rules, ruleReadPods, ruleWriteNodes) + return baseGR + }, + stateSetup: func(state testState) { + setSarResponse(true, nil, testUser, newDefaultGR().Name, state.sarMock) + }, + }, + allowed: true, + }, { name: "escalation in Cluster Rules", // User attempts to create a global with a cluster rules it does not currently have args: args{ @@ -254,6 +275,39 @@ func TestAdmit(t *testing.T) { }, stateSetup: func(state testState) { state.rtCacheMock.EXPECT().Get(roleTemplate.Name).Return(&roleTemplate, nil).AnyTimes() + setSarResponse(false, nil, testUser, newDefaultGR().Name, state.sarMock) + }, + }, + allowed: false, + }, + { + name: "escalation in Cluster Rules, escalate allowed", + args: args{ + username: testUser, + newGR: func() *v3.GlobalRole { + gr := newDefaultGR() + gr.InheritedClusterRoles = []string{roleTemplate.Name} + return gr + }, + stateSetup: func(state testState) { + state.rtCacheMock.EXPECT().Get(roleTemplate.Name).Return(&roleTemplate, nil).AnyTimes() + setSarResponse(true, nil, testUser, newDefaultGR().Name, state.sarMock) + }, + }, + allowed: true, + }, + { + name: "escalation in Cluster Rules, escalate check failed", + args: args{ + username: testUser, + newGR: func() *v3.GlobalRole { + gr := newDefaultGR() + gr.InheritedClusterRoles = []string{roleTemplate.Name} + return gr + }, + stateSetup: func(state testState) { + state.rtCacheMock.EXPECT().Get(roleTemplate.Name).Return(&roleTemplate, nil).AnyTimes() + setSarResponse(false, fmt.Errorf("server not available"), testUser, newDefaultGR().Name, state.sarMock) }, }, allowed: false, @@ -282,6 +336,7 @@ func TestAdmit(t *testing.T) { Context: "cluster", }, nil) state.rtCacheMock.EXPECT().Get("error").Return(nil, errServer) + setSarResponse(false, nil, testUser, newDefaultGR().Name, state.sarMock) }, }, wantErr: true, @@ -501,7 +556,7 @@ func TestAdmit(t *testing.T) { test.args.stateSetup(state) } grbResolver := state.createBaseGRBResolver() - admitters := globalrole.NewValidator(state.resolver, grbResolver).Admitters() + admitters := globalrole.NewValidator(state.resolver, grbResolver, state.sarMock).Admitters() assert.Len(t, admitters, 1) req := createGRRequest(t, test) @@ -519,7 +574,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) + validator := globalrole.NewValidator(resolver, nil, nil) admitters := validator.Admitters() require.Len(t, admitters, 1, "wanted only one admitter") test := testCase{ @@ -538,3 +593,20 @@ func Test_UnexpectedErrors(t *testing.T) { _, err = admitters[0].Admit(req) require.Error(t, err, "Admit should fail on unhandled operations") } + +func setSarResponse(allowed bool, testErr error, targetUser string, targetGrName string, sarMock *k8fake.FakeSubjectAccessReviews) { + sarMock.Fake.AddReactor("create", "subjectaccessreviews", func(action k8testing.Action) (handled bool, ret runtime.Object, err error) { + createAction := action.(k8testing.CreateActionImpl) + review := createAction.GetObject().(*authorizationv1.SubjectAccessReview) + spec := review.Spec + + isForGRGVR := spec.ResourceAttributes.Group == "management.cattle.io" && spec.ResourceAttributes.Version == "v3" && + spec.ResourceAttributes.Resource == "globalroles" + if spec.User == targetUser && spec.ResourceAttributes.Verb == "escalate" && + spec.ResourceAttributes.Namespace == "" && spec.ResourceAttributes.Name == targetGrName && isForGRGVR { + review.Status.Allowed = allowed + return true, review, testErr + } + return false, nil, nil + }) +} diff --git a/pkg/resources/management.cattle.io/v3/globalrolebinding/GlobalRoleBinding.md b/pkg/resources/management.cattle.io/v3/globalrolebinding/GlobalRoleBinding.md index a67a3bcbf..020d90eb4 100644 --- a/pkg/resources/management.cattle.io/v3/globalrolebinding/GlobalRoleBinding.md +++ b/pkg/resources/management.cattle.io/v3/globalrolebinding/GlobalRoleBinding.md @@ -6,12 +6,12 @@ Note: all checks are bypassed if the GlobalRoleBinding is being deleted, or if o Users can only create/update GlobalRoleBindings with rights less than or equal to those they currently possess. This is to prevent privilege escalation. -This escalation checking currently prevents service accounts from modifying GlobalRoleBindings which give access to GlobalRoles which include permissions on downstream clusters (such as Admin, Restricted Admin, or GlobalRoles which use the `inheritedClusterRoles` field). - ### Valid Global Role Reference GlobalRoleBindings must refer to a valid global role (i.e. an existing `GlobalRole` object in the `management.cattle.io/v3` apiGroup). +This escalation check is bypassed if a user has the `bind` verb on the GlobalRole that they are trying to bind to (through creating or updating a GlobalRoleBinding to that GlobalRole). This can also be given through a wildcard permission (i.e. the `*` verb also gives `bind`). + ### Invalid Fields - Update Users cannot update the following fields after creation: - `userName` 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 9031d6cd1..11970c8cf 100644 --- a/pkg/resources/management.cattle.io/v3/globalrolebinding/setup_test.go +++ b/pkg/resources/management.cattle.io/v3/globalrolebinding/setup_test.go @@ -19,6 +19,8 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" + k8fake "k8s.io/client-go/kubernetes/typed/authorization/v1/fake" + k8testing "k8s.io/client-go/testing" "k8s.io/kubernetes/pkg/registry/rbac/validation" ) @@ -45,6 +47,7 @@ type testState struct { rtCacheMock *fake.MockNonNamespacedCacheInterface[*v3.RoleTemplate] grCacheMock *fake.MockNonNamespacedCacheInterface[*v3.GlobalRole] grbCacheMock *fake.MockNonNamespacedCacheInterface[*v3.GlobalRoleBinding] + sarMock *k8fake.FakeSubjectAccessReviews resolver validation.AuthorizationRuleResolver } @@ -256,7 +259,7 @@ func createGRBRequest(t *testing.T, test testCase) *admission.Request { RequestKind: &gvk, RequestResource: &gvr, Operation: v1.Create, - UserInfo: v1authentication.UserInfo{Username: username, UID: ""}, + UserInfo: v1authentication.UserInfo{Username: username, UID: "", Extra: map[string]v1authentication.ExtraValue{"test": []string{"test"}}}, Object: runtime.RawExtension{}, OldObject: runtime.RawExtension{}, }, @@ -306,11 +309,15 @@ func newDefaultState(t *testing.T) testState { grCacheMock.EXPECT().Get(notFoundName).Return(nil, newNotFound(notFoundName)).AnyTimes() grCacheMock.EXPECT().Get("").Return(nil, newNotFound("")).AnyTimes() rtCacheMock.EXPECT().Get(baseRT.Name).Return(&baseRT, nil).AnyTimes() + k8Fake := &k8testing.Fake{} + fakeSAR := &k8fake.FakeSubjectAccessReviews{Fake: &k8fake.FakeAuthorizationV1{Fake: k8Fake}} + resolver, _ := validation.NewTestRuleResolver(nil, nil, clusterRoles, clusterRoleBindings) return testState{ rtCacheMock: rtCacheMock, grCacheMock: grCacheMock, grbCacheMock: grbCacheMock, + 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 508719356..7f66c2650 100644 --- a/pkg/resources/management.cattle.io/v3/globalrolebinding/validator.go +++ b/pkg/resources/management.cattle.io/v3/globalrolebinding/validator.go @@ -4,7 +4,6 @@ package globalrolebinding import ( "errors" "fmt" - "reflect" "strings" v3 "github.com/rancher/rancher/pkg/apis/management.cattle.io/v3" @@ -12,28 +11,41 @@ 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" + "github.com/sirupsen/logrus" admissionv1 "k8s.io/api/admission/v1" admissionregistrationv1 "k8s.io/api/admissionregistration/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/schema" "k8s.io/apimachinery/pkg/util/validation/field" + authorizationv1 "k8s.io/client-go/kubernetes/typed/authorization/v1" rbacvalidation "k8s.io/kubernetes/pkg/registry/rbac/validation" "k8s.io/utils/trace" ) -var gvr = schema.GroupVersionResource{ - Group: "management.cattle.io", - Version: "v3", - Resource: "globalrolebindings", -} +var ( + gvr = schema.GroupVersionResource{ + Group: "management.cattle.io", + Version: "v3", + Resource: "globalrolebindings", + } + globalRoleGvr = schema.GroupVersionResource{ + Group: "management.cattle.io", + Version: "v3", + Resource: "globalroles", + } +) + +const bindVerb = "bind" // NewValidator returns a new validator for GlobalRoleBindings. -func NewValidator(resolver rbacvalidation.AuthorizationRuleResolver, grbResolver *resolvers.GRBClusterRuleResolver) *Validator { +func NewValidator(resolver rbacvalidation.AuthorizationRuleResolver, grbResolver *resolvers.GRBClusterRuleResolver, + sar authorizationv1.SubjectAccessReviewInterface) *Validator { return &Validator{ admitter: admitter{ resolver: resolver, grbResolver: grbResolver, + sar: sar, }, } } @@ -66,6 +78,7 @@ func (v *Validator) Admitters() []admission.Admitter { type admitter struct { resolver rbacvalidation.AuthorizationRuleResolver grbResolver *resolvers.GRBClusterRuleResolver + sar authorizationv1.SubjectAccessReviewInterface } // Admit handles the webhook admission request sent to this webhook. @@ -78,7 +91,8 @@ func (a *admitter) Admit(request *admission.Request) (*admissionv1.AdmissionResp return nil, fmt.Errorf("failed to get %s from request: %w", gvr.Resource, err) } - if canSkipValidation(request.Operation, oldGRB, newGRB) { + // if the grb is being deleted don't enforce integrity checks + if request.Operation == admissionv1.Update && newGRB.DeletionTimestamp != nil { return admission.ResponseAllowed(), nil } @@ -117,19 +131,44 @@ 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) } - err = auth.ConfirmNoEscalation(request, clusterRules, "", a.grbResolver) + hasBind, err := a.isRulesAllowed(request, clusterRules, globalRole.Name, a.grbResolver) if err != nil { return admission.ResponseFailedEscalation(err.Error()), nil } + if hasBind { + // user has the bind verb, no need to check global permissions + return admission.ResponseAllowed(), nil + } rules := a.grbResolver.GlobalRoleResolver.GlobalRulesFromRole(globalRole) - err = auth.ConfirmNoEscalation(request, rules, "", a.resolver) + _, err = a.isRulesAllowed(request, rules, globalRole.Name, a.resolver) + // don't need to check if this request was allowed due to the bind verb since this is the last permission check + // if others are added in the future a short-circuit here will be needed like for the clusterRules if err != nil { return admission.ResponseFailedEscalation(err.Error()), nil } return admission.ResponseAllowed(), nil } +// isRulesAllowed checks if the use of requested rules are allowed by the givenResolver for a given request/user +// returns an error if the user failed an escalation check, and nil if the request was allowed. Also returns a bool +// indicating if the allow was due to the user having the bind verb +func (a *admitter) isRulesAllowed(request *admission.Request, rules []rbacv1.PolicyRule, grName string, resolver rbacvalidation.AuthorizationRuleResolver) (bool, error) { + err := auth.ConfirmNoEscalation(request, rules, "", resolver) + if err != nil { + hasBind, bindErr := auth.RequestUserHasVerb(request, globalRoleGvr, a.sar, bindVerb, grName, "") + if bindErr != nil { + logrus.Warnf("Failed to check for the 'bind' verb on GlobalRoles: %v", bindErr) + return false, err + } + if hasBind { + return true, nil + } + return false, err + } + return false, nil +} + // validUpdateFields checks if the fields being changed are valid update fields. func validateUpdateFields(oldBinding, newBinding *v3.GlobalRoleBinding, fldPath *field.Path) error { var err error @@ -181,31 +220,3 @@ func (a *admitter) validateGlobalRole(globalRole *v3.GlobalRole, fieldPath *fiel } return nil } - -// canSkipValidation checks if validation matches one of two scenarios which allow us to skip validation. -// 1. Object is scheduled for deletion (deletion timestamp set) -// 2. Object is a metadata only change. (this allows users with the appropriate permissions to manage labels/annotations/finalizers) -// deletionIsScheduled returns true if the GlobalRoleBinding is being deleted. -func canSkipValidation(op admissionv1.Operation, oldGRB, newGRB *v3.GlobalRoleBinding) bool { - return op == admissionv1.Update && (newGRB.DeletionTimestamp != nil || isMetaOnlyChange(oldGRB, newGRB)) -} - -// isMetaOnlyChange checks if old and new are deep equal in all fields except metadata. Will return false on a -// non-effectual change. -func isMetaOnlyChange(oldGRB *v3.GlobalRoleBinding, newGRB *v3.GlobalRoleBinding) bool { - oldMeta := oldGRB.ObjectMeta - newMeta := newGRB.ObjectMeta - - // if the metadata between old/new hasn't changed, then this isn't a metadata only change - if reflect.DeepEqual(oldMeta, newMeta) { - // checking equality of global role bindings with reflect.DeepEqual() can be very expensive from a cpu perspective - return false - } - - oldGRB.ObjectMeta = metav1.ObjectMeta{} - newGRB.ObjectMeta = metav1.ObjectMeta{} - result := reflect.DeepEqual(oldGRB, newGRB) - oldGRB.ObjectMeta = oldMeta - newGRB.ObjectMeta = newMeta - return result -} 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 af223c7af..c2259cedc 100644 --- a/pkg/resources/management.cattle.io/v3/globalrolebinding/validator_test.go +++ b/pkg/resources/management.cattle.io/v3/globalrolebinding/validator_test.go @@ -1,6 +1,7 @@ package globalrolebinding_test import ( + "fmt" "testing" "time" @@ -11,10 +12,13 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" v1 "k8s.io/api/admission/v1" + authorizationv1 "k8s.io/api/authorization/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" + k8fake "k8s.io/client-go/kubernetes/typed/authorization/v1/fake" + k8testing "k8s.io/client-go/testing" ) func TestAdmit(t *testing.T) { @@ -170,23 +174,6 @@ func TestAdmit(t *testing.T) { }, allowed: true, }, - { - name: "update meta-only changed", - args: args{ - newGRB: func() *v3.GlobalRoleBinding { - gr := newDefaultGRB() - gr.Labels = map[string]string{"updated": "yes"} - gr.GlobalRoleName = errName - return gr - }, - oldGRB: func() *v3.GlobalRoleBinding { - gr := newDefaultGRB() - gr.GlobalRoleName = errName - return gr - }, - }, - allowed: true, - }, // Start privilege test { name: "base test valid privileges", @@ -199,6 +186,9 @@ func TestAdmit(t *testing.T) { return baseGRB }, oldGRB: func() *v3.GlobalRoleBinding { return nil }, + stateSetup: func(ts testState) { + setSarResponse(false, nil, adminUser, adminGR.Name, ts.sarMock) + }, }, allowed: true, }, @@ -213,6 +203,9 @@ func TestAdmit(t *testing.T) { return baseGRB }, oldGRB: func() *v3.GlobalRoleBinding { return nil }, + stateSetup: func(ts testState) { + setSarResponse(false, nil, noPrivUser, baseGR.Name, ts.sarMock) + }, }, allowed: true, }, @@ -227,9 +220,29 @@ func TestAdmit(t *testing.T) { return baseGRB }, oldGRB: func() *v3.GlobalRoleBinding { return nil }, + stateSetup: func(ts testState) { + setSarResponse(false, nil, testUser, adminGR.Name, ts.sarMock) + }, }, allowed: false, }, + { + name: "privilege escalation other user, bind allowed", + args: args{ + username: testUser, + newGRB: func() *v3.GlobalRoleBinding { + baseGRB := newDefaultGRB() + baseGRB.UserName = noPrivUser + baseGRB.GlobalRoleName = adminGR.Name + return baseGRB + }, + oldGRB: func() *v3.GlobalRoleBinding { return nil }, + stateSetup: func(ts testState) { + setSarResponse(true, nil, testUser, adminGR.Name, ts.sarMock) + }, + }, + allowed: true, + }, { name: "privilege escalation self", args: args{ @@ -241,11 +254,48 @@ func TestAdmit(t *testing.T) { return baseGRB }, oldGRB: func() *v3.GlobalRoleBinding { return nil }, + stateSetup: func(ts testState) { + setSarResponse(false, nil, testUser, adminGR.Name, ts.sarMock) + }, + }, + allowed: false, + }, + { + name: "privilege escalation self, bind allowed", + args: args{ + username: testUser, + newGRB: func() *v3.GlobalRoleBinding { + baseGRB := newDefaultGRB() + baseGRB.UserName = testUser + baseGRB.GlobalRoleName = adminGR.Name + return baseGRB + }, + oldGRB: func() *v3.GlobalRoleBinding { return nil }, + stateSetup: func(ts testState) { + setSarResponse(true, nil, testUser, adminGR.Name, ts.sarMock) + }, + }, + allowed: true, + }, + { + name: "correct user privileges are evaluated", // test that the privileges evaluated are those of the user in the request not the user being bound. + args: args{ + username: testUser, + newGRB: func() *v3.GlobalRoleBinding { + baseGRB := newDefaultGRB() + baseGRB.UserName = adminUser + baseGRB.GlobalRoleName = adminGR.Name + return baseGRB + }, + oldGRB: func() *v3.GlobalRoleBinding { return nil }, + stateSetup: func(ts testState) { + setSarResponse(false, nil, testUser, adminGR.Name, ts.sarMock) + }, }, allowed: false, }, { - name: "correct user privileges are evaluated.", // test that the privileges evaluated are those of the user in the request not the user being bound. + name: "correct user privileges are evaluated, bind allowed", args: args{ username: testUser, newGRB: func() *v3.GlobalRoleBinding { @@ -255,6 +305,26 @@ func TestAdmit(t *testing.T) { return baseGRB }, oldGRB: func() *v3.GlobalRoleBinding { return nil }, + stateSetup: func(ts testState) { + setSarResponse(true, nil, testUser, adminGR.Name, ts.sarMock) + }, + }, + allowed: true, + }, + { + name: "correct user privileges are evaluated, bind error", + args: args{ + username: testUser, + newGRB: func() *v3.GlobalRoleBinding { + baseGRB := newDefaultGRB() + baseGRB.UserName = adminUser + baseGRB.GlobalRoleName = adminGR.Name + return baseGRB + }, + oldGRB: func() *v3.GlobalRoleBinding { return nil }, + stateSetup: func(ts testState) { + setSarResponse(false, fmt.Errorf("server not available"), testUser, adminGR.Name, ts.sarMock) + }, }, allowed: false, }, @@ -273,6 +343,47 @@ func TestAdmit(t *testing.T) { stateSetup: func(ts testState) { ts.grCacheMock.EXPECT().Get(adminClusterGR.Name).Return(&adminClusterGR, nil) ts.rtCacheMock.EXPECT().Get(adminRT.Name).Return(&adminRT, nil).AnyTimes() + setSarResponse(false, nil, testUser, adminClusterGR.Name, ts.sarMock) + }, + }, + allowed: false, + }, + { + name: "escalation in GR Cluster Roles, bind allowed", + args: args{ + newGRB: func() *v3.GlobalRoleBinding { + return &v3.GlobalRoleBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-grb", + }, + UserName: testUser, + GlobalRoleName: adminClusterGR.Name, + } + }, + stateSetup: func(ts testState) { + ts.grCacheMock.EXPECT().Get(adminClusterGR.Name).Return(&adminClusterGR, nil) + ts.rtCacheMock.EXPECT().Get(adminRT.Name).Return(&adminRT, nil).AnyTimes() + setSarResponse(true, nil, testUser, adminClusterGR.Name, ts.sarMock) + }, + }, + allowed: true, + }, + { + name: "escalation in GR Cluster Roles, bind error", + args: args{ + newGRB: func() *v3.GlobalRoleBinding { + return &v3.GlobalRoleBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-grb", + }, + UserName: testUser, + GlobalRoleName: adminClusterGR.Name, + } + }, + stateSetup: func(ts testState) { + ts.grCacheMock.EXPECT().Get(adminClusterGR.Name).Return(&adminClusterGR, nil) + ts.rtCacheMock.EXPECT().Get(adminRT.Name).Return(&adminRT, nil).AnyTimes() + setSarResponse(false, fmt.Errorf("server not available"), testUser, adminClusterGR.Name, ts.sarMock) }, }, allowed: false, @@ -296,6 +407,7 @@ func TestAdmit(t *testing.T) { }, notFoundName) ts.grCacheMock.EXPECT().Get(notFoundRoleGR.Name).Return(¬FoundRoleGR, nil) ts.rtCacheMock.EXPECT().Get(notFoundName).Return(nil, notFoundError) + setSarResponse(false, nil, testUser, notFoundRoleGR.Name, ts.sarMock) }, }, allowed: false, @@ -314,6 +426,7 @@ func TestAdmit(t *testing.T) { }, stateSetup: func(ts testState) { ts.grCacheMock.EXPECT().Get(errName).Return(nil, errServer) + setSarResponse(false, nil, testUser, errName, ts.sarMock) }, }, wantError: true, @@ -546,8 +659,8 @@ func TestAdmit(t *testing.T) { test.args.stateSetup(state) } grResolver := auth.NewGlobalRoleResolver(auth.NewRoleTemplateResolver(state.rtCacheMock, nil), state.grCacheMock) - grbResolver := resolvers.NewGRBClusterRuleResolver(state.grbCacheMock, grResolver, nil) - admitters := globalrolebinding.NewValidator(state.resolver, grbResolver).Admitters() + grbResolver := resolvers.NewGRBClusterRuleResolver(state.grbCacheMock, grResolver) + admitters := globalrolebinding.NewValidator(state.resolver, grbResolver, state.sarMock).Admitters() require.Len(t, admitters, 1) req := createGRBRequest(t, test) @@ -567,8 +680,8 @@ func Test_UnexpectedErrors(t *testing.T) { t.Parallel() state := newDefaultState(t) grResolver := auth.NewGlobalRoleResolver(auth.NewRoleTemplateResolver(state.rtCacheMock, nil), state.grCacheMock) - grbResolver := resolvers.NewGRBClusterRuleResolver(state.grbCacheMock, grResolver, nil) - validator := globalrolebinding.NewValidator(state.resolver, grbResolver) + grbResolver := resolvers.NewGRBClusterRuleResolver(state.grbCacheMock, grResolver) + validator := globalrolebinding.NewValidator(state.resolver, grbResolver, state.sarMock) admitters := validator.Admitters() require.Len(t, admitters, 1, "wanted only one admitter") admitter := admitters[0] @@ -589,3 +702,20 @@ func Test_UnexpectedErrors(t *testing.T) { _, err = admitter.Admit(req) require.Error(t, err, "Admit should fail on bad request object") } + +func setSarResponse(allowed bool, testErr error, targetUser string, targetGrName string, sarMock *k8fake.FakeSubjectAccessReviews) { + sarMock.Fake.AddReactor("create", "subjectaccessreviews", func(action k8testing.Action) (handled bool, ret runtime.Object, err error) { + createAction := action.(k8testing.CreateActionImpl) + review := createAction.GetObject().(*authorizationv1.SubjectAccessReview) + spec := review.Spec + + isForGRGVR := spec.ResourceAttributes.Group == "management.cattle.io" && spec.ResourceAttributes.Version == "v3" && + spec.ResourceAttributes.Resource == "globalroles" + if spec.User == targetUser && spec.ResourceAttributes.Verb == "bind" && + spec.ResourceAttributes.Namespace == "" && spec.ResourceAttributes.Name == targetGrName && isForGRGVR { + review.Status.Allowed = allowed + return true, review, testErr + } + return false, nil, nil + }) +} diff --git a/pkg/resources/management.cattle.io/v3/roletemplate/validator.go b/pkg/resources/management.cattle.io/v3/roletemplate/validator.go index 391c654b5..1c8d6aba1 100644 --- a/pkg/resources/management.cattle.io/v3/roletemplate/validator.go +++ b/pkg/resources/management.cattle.io/v3/roletemplate/validator.go @@ -28,6 +28,7 @@ const ( emptyContext = "" rtRefIndex = "management.cattle.io/rt-by-reference" rtGlobalRefIndex = "management.cattle.io/rt-by-ref-grb" + escalateVerb = "escalate" ) var gvr = schema.GroupVersionResource{ @@ -136,7 +137,7 @@ func (a *admitter) Admit(request *admission.Request) (*admissionv1.AdmissionResp return admission.ResponseBadRequest(err.Error()), nil } - allowed, err := auth.EscalationAuthorized(request, gvr, a.sar, "") + allowed, err := auth.RequestUserHasVerb(request, gvr, a.sar, escalateVerb, "", "") if err != nil { logrus.Warnf("Failed to check for the 'escalate' verb on RoleTemplates: %v", err) } else if allowed { diff --git a/pkg/server/handlers.go b/pkg/server/handlers.go index bb44c9a56..889d55fe2 100644 --- a/pkg/server/handlers.go +++ b/pkg/server/handlers.go @@ -34,10 +34,10 @@ func Validation(clients *clients.Clients) ([]admission.ValidatingAdmissionHandle if clients.MultiClusterManagement { crtbResolver := resolvers.NewCRTBRuleResolver(clients.Management.ClusterRoleTemplateBinding().Cache(), clients.RoleTemplateResolver) prtbResolver := resolvers.NewPRTBRuleResolver(clients.Management.ProjectRoleTemplateBinding().Cache(), clients.RoleTemplateResolver) - grbResolver := resolvers.NewGRBClusterRuleResolver(clients.Management.GlobalRoleBinding().Cache(), clients.GlobalRoleResolver, clients.K8s.AuthorizationV1().SubjectAccessReviews()) + 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) - globalRoleBindings := globalrolebinding.NewValidator(clients.DefaultResolver, grbResolver) + globalRoles := globalrole.NewValidator(clients.DefaultResolver, grbResolver, clients.K8s.AuthorizationV1().SubjectAccessReviews()) + globalRoleBindings := globalrolebinding.NewValidator(clients.DefaultResolver, grbResolver, clients.K8s.AuthorizationV1().SubjectAccessReviews()) 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())