Merge pull request #302 from MbolotSuse/escalate-bind-gr

Adds Support for Escalate and Bind verb to GRs/GRBs

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`

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,

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 {

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)

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{

pkg/resolvers/grbClusterResolver.go

@@ -1,57 +1,34 @@
 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 (
 	grbSubjectIndex = "management.cattle.io/grb-by-subject"
 	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.