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,6 +1,7 @@
 package common
 
 import (
+	"errors"
 	"fmt"
 	"net/http"
 
@@ -9,6 +10,9 @@ import (
 	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,7 +48,9 @@ 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
+// CheckForVerbs checks that all the rules in the given list have a verb set.
+// This is currently used in the validation of globalroles and roletemplates.
+// BEWARE This function may not be required anymore because both places also use ValidateRules, see below.
 func CheckForVerbs(rules []rbacv1.PolicyRule) error {
 	for i := range rules {
 		rule := rules[i]
@@ -54,3 +60,17 @@ func CheckForVerbs(rules []rbacv1.PolicyRule) error {
 	}
 	return nil
 }
+
+// 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))
+		if len(fieldErrs) != 0 {
+			returnErr = errors.Join(returnErr, fieldErrs.ToAggregate())
+		}
+	}
+	return returnErr
+}

pkg/resources/common/validation_test.go

@@ -0,0 +1,89 @@
+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)
+
+	// Note: The partial error message is prefixed with the resource name during test execution
+	tests := []struct {
+		name string            // label for testcase
+		data rbacv1.PolicyRule // policy rule to be validated
+		err  string            // partial error returned for a resource,     empty string -> no error
+	}{
+		{
+			name: "ok",
+			data: rbacv1.PolicyRule{
+				Verbs:     []string{"*"},
+				APIGroups: []string{""},
+				Resources: []string{"*"},
+			},
+			err: "",
+		},
+		{
+			name: "no-verbs",
+			data: rbacv1.PolicyRule{
+				Verbs:     []string{},
+				APIGroups: []string{""},
+				Resources: []string{"*"},
+			},
+			err: "[0].verbs: Required value: verbs must contain at least one value",
+		},
+		{
+			name: "no-api-groups",
+			data: rbacv1.PolicyRule{
+				Verbs:     []string{"*"},
+				APIGroups: []string{},
+				Resources: []string{"*"},
+			},
+			err: "[0].apiGroups: Required value: resource rules must supply at least one api group",
+		},
+		{
+			name: "no-resources",
+			data: rbacv1.PolicyRule{
+				Verbs:     []string{"*"},
+				APIGroups: []string{""},
+				Resources: []string{},
+			},
+			err: "[0].resources: Required value: resource rules must supply at least one resource",
+		},
+	}
+
+	for _, testcase := range tests {
+		t.Run("global/"+testcase.name, func(t *testing.T) {
+			err := ValidateRules([]v1.PolicyRule{
+				testcase.data,
+			}, false, gField)
+			if testcase.err == "" {
+				require.NoError(t, err)
+				return
+			}
+			require.Error(t, err)
+			require.Equal(t, err.Error(), gResource+testcase.err)
+		})
+		t.Run("namespaced/"+testcase.name, func(t *testing.T) {
+			err := ValidateRules([]v1.PolicyRule{
+				testcase.data,
+			}, true, nsField)
+			if testcase.err == "" {
+				require.NoError(t, err)
+				return
+			}
+			require.Error(t, err)
+			require.Equal(t, err.Error(), nsResource+testcase.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"
 )
@@ -123,9 +120,9 @@ 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)
+	returnError := common.ValidateRules(globalRules, false, fldPath)
 	for _, rules := range newGR.NamespacedRules {
-		returnError = errors.Join(returnError, validateRules(rules, true, fldPath))
+		returnError = errors.Join(returnError, common.ValidateRules(rules, true, fldPath))
 	}
 	if returnError != nil {
 		return admission.ResponseBadRequest(returnError.Error()), nil
@@ -258,14 +255,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,6 +132,11 @@ 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 template rules as per kubernetes rbac rules
+	if err := common.ValidateRules(rules, true, fldPath.Child("rules")); err != nil {
+		return admission.ResponseBadRequest(err.Error()), nil
+	}
+
 	// verify inherited rules have verbs
 	if err := common.CheckForVerbs(rules); err != nil {
 		return admission.ResponseBadRequest(err.Error()), nil