Merge pull request #328 from andreas-kupries/ak-gh40584-rules-validation

Fix roletemplate.rules validation, added standard k8s checks

docs.md

@@ -115,9 +115,13 @@ Note: all checks are bypassed if the GlobalRole is being deleted, or if only the
 #### Invalid Fields - Create and Update
 
 On create or update, the following checks take place:
-- The webhook checks that each rule has at least one verb.
+- The webhook validates each rule using the standard Kubernetes RBAC checks (see next section).
 - Each new RoleTemplate referred to in `inheritedClusterRoles` must have a context of `cluster` and not be `locked`. This validation is skipped for RoleTemplates in `inheritedClusterRoles` for the prior version of this object.
 
+#### Rules Without Verbs, Resources, API groups
+
+Rules without verbs, resources, or apigroups are not permitted. The `rules` included in a GlobalRole are of the same type as the rules used by standard Kubernetes RBAC types (such as `Roles` from `rbac.authorization.k8s.io/v1`). Because of this, they inherit the same restrictions as these types, including this one.
+
 #### Escalation Prevention
 
  Escalation checks are bypassed if a user has the `escalate` verb on the GlobalRole that they are attempting to update or create. This can also be given through a wildcard permission (i.e. the `*` verb also gives `escalate`).
@@ -253,9 +257,9 @@ Note: all checks are bypassed if the RoleTemplate is being deleted
 
 Circular references to a `RoleTemplate` (a inherits b, b inherits a) are not allowed. More specifically, if "roleTemplate1" is included in the `roleTemplateNames` of "roleTemplate2", then "roleTemplate2" must not be included in the `roleTemplateNames` of "roleTemplate1". This check prevents the creation of roles whose end-state cannot be resolved.
 
-#### Rules Without Verbs 
+#### Rules Without Verbs, Resources, API groups
 
-Rules without verbs are not permitted. The `rules` included in a RoleTemplate are of the same type as the rules used by standard Kubernetes RBAC types (such as `Roles` from `rbac.authorization.k8s.io/v1`). Because of this, they inherit the same restrictions as these types, including this one.
+Rules without verbs, resources, or apigroups are not permitted. The `rules` included in a RoleTemplate are of the same type as the rules used by standard Kubernetes RBAC types (such as `Roles` from `rbac.authorization.k8s.io/v1`). Because of this, they inherit the same restrictions as these types, including this one.
 
 #### Escalation Prevention
 

pkg/resources/common/validation.go

@@ -1,14 +1,17 @@
 package common
 
 import (
-	"fmt"
+	"errors"
 	"net/http"
 
 	"github.com/rancher/webhook/pkg/admission"
 	"github.com/rancher/webhook/pkg/auth"
 	admissionv1 "k8s.io/api/admission/v1"
 	rbacv1 "k8s.io/api/rbac/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/util/validation/field"
+	"k8s.io/kubernetes/pkg/apis/rbac"
+	rbacValidation "k8s.io/kubernetes/pkg/apis/rbac/validation"
 )
 
 func CheckCreatorID(request *admission.Request, oldObj, newObj metav1.Object) *metav1.Status {
@@ -44,13 +47,14 @@ func CheckCreatorID(request *admission.Request, oldObj, newObj metav1.Object) *m
 	return nil
 }
 
-// CheckForVerbs checks that all the rules in the given list have a verb set
-func CheckForVerbs(rules []rbacv1.PolicyRule) error {
-	for i := range rules {
-		rule := rules[i]
-		if len(rule.Verbs) == 0 {
-			return fmt.Errorf("policyRules must have at least one verb: %s", rule.String())
-		}
+// ValidateRules calls on standard kubernetes RBAC functionality for the validation of policy rules
+// to validate Rancher rules. This is currently used in the validation of globalroles and roletemplates.
+func ValidateRules(rules []rbacv1.PolicyRule, isNamespaced bool, fldPath *field.Path) error {
+	var returnErr error
+	for index, r := range rules {
+		fieldErrs := rbacValidation.ValidatePolicyRule(rbac.PolicyRule(r), isNamespaced,
+			fldPath.Index(index))
+		returnErr = errors.Join(returnErr, fieldErrs.ToAggregate())
 	}
-	return nil
+	return returnErr
 }

pkg/resources/common/validation_test.go

@@ -0,0 +1,85 @@
+package common
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+	rbacv1 "k8s.io/api/rbac/v1"
+	v1 "k8s.io/api/rbac/v1"
+	"k8s.io/apimachinery/pkg/util/validation/field"
+)
+
+func TestValidateRules(t *testing.T) {
+	t.Parallel()
+
+	gResource := "something-global"
+	nsResource := "something-namespaced"
+
+	gField := field.NewPath(gResource)
+	nsField := field.NewPath(nsResource)
+
+	tests := []struct {
+		name   string            // label for testcase
+		data   rbacv1.PolicyRule // policy rule to be validated
+		haserr bool              // error expected ?
+	}{
+		{
+			name: "ok",
+			data: rbacv1.PolicyRule{
+				Verbs:     []string{"*"},
+				APIGroups: []string{""},
+				Resources: []string{"*"},
+			},
+		},
+		{
+			name: "no-verbs",
+			data: rbacv1.PolicyRule{
+				Verbs:     []string{},
+				APIGroups: []string{""},
+				Resources: []string{"*"},
+			},
+			haserr: true,
+		},
+		{
+			name: "no-api-groups",
+			data: rbacv1.PolicyRule{
+				Verbs:     []string{"*"},
+				APIGroups: []string{},
+				Resources: []string{"*"},
+			},
+			haserr: true,
+		},
+		{
+			name: "no-resources",
+			data: rbacv1.PolicyRule{
+				Verbs:     []string{"*"},
+				APIGroups: []string{""},
+				Resources: []string{},
+			},
+			haserr: true,
+		},
+	}
+
+	for _, testcase := range tests {
+		t.Run("global/"+testcase.name, func(t *testing.T) {
+			err := ValidateRules([]v1.PolicyRule{
+				testcase.data,
+			}, false, gField)
+			if testcase.haserr {
+				require.Error(t, err)
+				return
+			}
+			require.NoError(t, err)
+		})
+		t.Run("namespaced/"+testcase.name, func(t *testing.T) {
+			err := ValidateRules([]v1.PolicyRule{
+				testcase.data,
+			}, true, nsField)
+			if testcase.haserr {
+				require.Error(t, err)
+				return
+			}
+			require.NoError(t, err)
+		})
+	}
+}

pkg/resources/management.cattle.io/v3/globalrole/GlobalRole.md

@@ -5,9 +5,13 @@ Note: all checks are bypassed if the GlobalRole is being deleted, or if only the
 ### Invalid Fields - Create and Update
 
 On create or update, the following checks take place:
-- The webhook checks that each rule has at least one verb.
+- The webhook validates each rule using the standard Kubernetes RBAC checks (see next section).
 - Each new RoleTemplate referred to in `inheritedClusterRoles` must have a context of `cluster` and not be `locked`. This validation is skipped for RoleTemplates in `inheritedClusterRoles` for the prior version of this object.
 
+### Rules Without Verbs, Resources, API groups
+
+Rules without verbs, resources, or apigroups are not permitted. The `rules` included in a GlobalRole are of the same type as the rules used by standard Kubernetes RBAC types (such as `Roles` from `rbac.authorization.k8s.io/v1`). Because of this, they inherit the same restrictions as these types, including this one.
+
 ### Escalation Prevention
 
  Escalation checks are bypassed if a user has the `escalate` verb on the GlobalRole that they are attempting to update or create. This can also be given through a wildcard permission (i.e. the `*` verb also gives `escalate`).

pkg/resources/management.cattle.io/v3/globalrole/validator.go

@@ -13,13 +13,10 @@ import (
 	"github.com/rancher/webhook/pkg/resources/common"
 	admissionv1 "k8s.io/api/admission/v1"
 	admissionregistrationv1 "k8s.io/api/admissionregistration/v1"
-	v1 "k8s.io/api/rbac/v1"
 	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/apis/rbac"
-	rbacValidation "k8s.io/kubernetes/pkg/apis/rbac/validation"
 	"k8s.io/kubernetes/pkg/registry/rbac/validation"
 	"k8s.io/utils/trace"
 )
@@ -113,7 +110,7 @@ func (a *admitter) Admit(request *admission.Request) (*admissionv1.AdmissionResp
 		return nil, fmt.Errorf("%s operation %v: %w", gvr.Resource, request.Operation, admission.ErrUnsupportedOperation)
 	}
 
-	err = a.validateFields(oldGR, newGR, fldPath)
+	err = a.validateInheritedClusterRoles(oldGR, newGR, fldPath.Child("inheritedClusterRoles"))
 	if err != nil {
 		if errors.As(err, admission.Ptr(new(field.Error))) {
 			return admission.ResponseBadRequest(err.Error()), nil
@@ -123,10 +120,14 @@ func (a *admitter) Admit(request *admission.Request) (*admissionv1.AdmissionResp
 
 	// Validate the global and namespaced rules of the new GR
 	globalRules := a.grbResolver.GlobalRoleResolver.GlobalRulesFromRole(newGR)
-	returnError := validateRules(globalRules, false, fldPath)
-	for _, rules := range newGR.NamespacedRules {
-		returnError = errors.Join(returnError, validateRules(rules, true, fldPath))
+	returnError := common.ValidateRules(globalRules, false, fldPath.Child("rules"))
+
+	nsrPath := fldPath.Child("namespacedRules")
+	for index, rules := range newGR.NamespacedRules {
+		returnError = errors.Join(returnError, common.ValidateRules(rules, true,
+			nsrPath.Child(index)))
 	}
+
 	if returnError != nil {
 		return admission.ResponseBadRequest(returnError.Error()), nil
 	}
@@ -176,14 +177,6 @@ func validateCreateFields(oldRole *v3.GlobalRole, fldPath *field.Path) *field.Er
 	return nil
 }
 
-// validateFields validates fields validates that the defined rules all have verbs and check the inheritedClusterRoles.
-func (a *admitter) validateFields(oldRole, newRole *v3.GlobalRole, fldPath *field.Path) error {
-	if err := common.CheckForVerbs(newRole.Rules); err != nil {
-		return field.Required(fldPath.Child("rules"), err.Error())
-	}
-	return a.validateInheritedClusterRoles(oldRole, newRole, fldPath.Child("inheritedClusterRoles"))
-}
-
 // validateInheritedClusterRoles validates that new RoleTemplates specified by InheritedClusterRoles have a context of
 // cluster and are not locked. Does NOT check for user privilege escalation. May return a field.Error indicating the
 // source of the error.
@@ -258,14 +251,3 @@ func (a *admitter) validateUpdateFields(oldRole, newRole *v3.GlobalRole, fldPath
 	}
 	return nil
 }
-
-func validateRules(rules []v1.PolicyRule, isNamespaced bool, fldPath *field.Path) error {
-	var returnErr error
-	for _, r := range rules {
-		fieldErrs := rbacValidation.ValidatePolicyRule(rbac.PolicyRule(r), isNamespaced, fldPath)
-		if len(fieldErrs) != 0 {
-			returnErr = errors.Join(returnErr, fieldErrs.ToAggregate())
-		}
-	}
-	return returnErr
-}

pkg/resources/management.cattle.io/v3/roletemplate/RoleTemplate.md

@@ -6,9 +6,9 @@ Note: all checks are bypassed if the RoleTemplate is being deleted
 
 Circular references to a `RoleTemplate` (a inherits b, b inherits a) are not allowed. More specifically, if "roleTemplate1" is included in the `roleTemplateNames` of "roleTemplate2", then "roleTemplate2" must not be included in the `roleTemplateNames` of "roleTemplate1". This check prevents the creation of roles whose end-state cannot be resolved.
 
-### Rules Without Verbs 
+### Rules Without Verbs, Resources, API groups
 
-Rules without verbs are not permitted. The `rules` included in a RoleTemplate are of the same type as the rules used by standard Kubernetes RBAC types (such as `Roles` from `rbac.authorization.k8s.io/v1`). Because of this, they inherit the same restrictions as these types, including this one.
+Rules without verbs, resources, or apigroups are not permitted. The `rules` included in a RoleTemplate are of the same type as the rules used by standard Kubernetes RBAC types (such as `Roles` from `rbac.authorization.k8s.io/v1`). Because of this, they inherit the same restrictions as these types, including this one.
 
 ### Escalation Prevention
 

pkg/resources/management.cattle.io/v3/roletemplate/validator.go

@@ -132,8 +132,8 @@ func (a *admitter) Admit(request *admission.Request) (*admissionv1.AdmissionResp
 		return nil, fmt.Errorf("failed to get all rules for '%s': %w", newRT.Name, err)
 	}
 
-	// verify inherited rules have verbs
-	if err := common.CheckForVerbs(rules); err != nil {
+	// Verify template rules as per kubernetes rbac rules
+	if err := common.ValidateRules(rules, true, fldPath.Child("rules")); err != nil {
 		return admission.ResponseBadRequest(err.Error()), nil
 	}
 

tests/integration/globalRole_test.go

@@ -52,3 +52,69 @@ func (m *IntegrationSuite) TestGlobalRole() {
 	}
 	validateEndpoints(m.T(), endPoints, m.clientFactory)
 }
+
+func (m *IntegrationSuite) TestGlobalRoleNoResources() {
+	newObj := func() *v3.GlobalRole { return &v3.GlobalRole{} }
+	validCreateObj := &v3.GlobalRole{
+		ObjectMeta: v1.ObjectMeta{
+			Name: "test-globalrole-no-resources",
+		},
+		Rules: []rbacv1.PolicyRule{
+			{
+				Verbs:     []string{"GET", "WATCH"},
+				APIGroups: []string{"v1"},
+				Resources: []string{"pods"},
+			},
+		},
+	}
+	invalidCreate := func() *v3.GlobalRole {
+		invalidCreate := validCreateObj.DeepCopy()
+		if len(invalidCreate.Rules) != 0 {
+			invalidCreate.Rules[0].Resources = nil
+		}
+		return invalidCreate
+	}
+	validDelete := func() *v3.GlobalRole {
+		return validCreateObj
+	}
+	endPoints := &endPointObjs[*v3.GlobalRole]{
+		invalidCreate:  invalidCreate,
+		newObj:         newObj,
+		validCreateObj: validCreateObj,
+		validDelete:    validDelete,
+	}
+	validateEndpoints(m.T(), endPoints, m.clientFactory)
+}
+
+func (m *IntegrationSuite) TestGlobalRoleNoAPIGroups() {
+	newObj := func() *v3.GlobalRole { return &v3.GlobalRole{} }
+	validCreateObj := &v3.GlobalRole{
+		ObjectMeta: v1.ObjectMeta{
+			Name: "test-globalrole-no-apigroups",
+		},
+		Rules: []rbacv1.PolicyRule{
+			{
+				Verbs:     []string{"GET", "WATCH"},
+				APIGroups: []string{"v1"},
+				Resources: []string{"pods"},
+			},
+		},
+	}
+	invalidCreate := func() *v3.GlobalRole {
+		invalidCreate := validCreateObj.DeepCopy()
+		if len(invalidCreate.Rules) != 0 {
+			invalidCreate.Rules[0].APIGroups = nil
+		}
+		return invalidCreate
+	}
+	validDelete := func() *v3.GlobalRole {
+		return validCreateObj
+	}
+	endPoints := &endPointObjs[*v3.GlobalRole]{
+		invalidCreate:  invalidCreate,
+		newObj:         newObj,
+		validCreateObj: validCreateObj,
+		validDelete:    validDelete,
+	}
+	validateEndpoints(m.T(), endPoints, m.clientFactory)
+}