refactor: UserInfo validation (#3399)

Signed-off-by: Charles-Edouard Brétéché <charled.breteche@gmail.com>

api/kyverno/v1/common_types.go

@@ -4,7 +4,6 @@ import (
 	"encoding/json"
 	"reflect"
 
-	rbacv1 "k8s.io/api/rbac/v1"
 	"k8s.io/apiextensions-apiserver/pkg/apis/apiextensions"
 	apiextv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -457,21 +456,6 @@ type ResourceFilter struct {
 	ResourceDescription `json:"resources,omitempty" yaml:"resources,omitempty"`
 }
 
-// UserInfo contains information about the user performing the operation.
-type UserInfo struct {
-	// Roles is the list of namespaced role names for the user.
-	// +optional
-	Roles []string `json:"roles,omitempty" yaml:"roles,omitempty"`
-
-	// ClusterRoles is the list of cluster-wide role names for the user.
-	// +optional
-	ClusterRoles []string `json:"clusterRoles,omitempty" yaml:"clusterRoles,omitempty"`
-
-	// Subjects is the list of subject names like users, user groups, and service accounts.
-	// +optional
-	Subjects []rbacv1.Subject `json:"subjects,omitempty" yaml:"subjects,omitempty"`
-}
-
 // ResourceDescription contains criteria used to match resources.
 type ResourceDescription struct {
 	// Kinds is a list of resource kinds.

api/kyverno/v1/user_info_test.go

@@ -0,0 +1,48 @@
+package v1
+
+import (
+	"testing"
+
+	"gotest.tools/assert"
+	rbacv1 "k8s.io/api/rbac/v1"
+	"k8s.io/apimachinery/pkg/util/validation/field"
+)
+
+func Test_Validate_ServiceAccount(t *testing.T) {
+	subject := UserInfo{
+		Subjects: []rbacv1.Subject{{
+			Kind: "ServiceAccount",
+			Name: "sa-1",
+		}, {
+			Kind: "ServiceAccount",
+			Name: "sa-2",
+		}},
+	}
+	path := field.NewPath("dummy")
+	errs := subject.Validate(path)
+	assert.Equal(t, len(errs), 2)
+	assert.Equal(t, errs[0].Field, "dummy.subjects[0].namespace")
+	assert.Equal(t, errs[1].Field, "dummy.subjects[1].namespace")
+}
+
+func Test_Validate_EmptyUserInfo(t *testing.T) {
+	subject := UserInfo{
+		Subjects: nil,
+	}
+	path := field.NewPath("dummy")
+	errs := subject.Validate(path)
+	assert.Equal(t, len(errs), 0)
+}
+
+func Test_Validate_Roles(t *testing.T) {
+	subject := UserInfo{
+		Roles: []string{
+			"namespace1:name1",
+			"name2",
+		},
+	}
+	path := field.NewPath("dummy")
+	errs := subject.Validate(path)
+	assert.Equal(t, len(errs), 1)
+	assert.Equal(t, errs[0].Field, "dummy.roles[1]")
+}

api/kyverno/v1/user_info_types.go

@@ -0,0 +1,62 @@
+package v1
+
+import (
+	"fmt"
+	"strings"
+
+	rbacv1 "k8s.io/api/rbac/v1"
+	"k8s.io/apimachinery/pkg/util/validation/field"
+)
+
+// UserInfo contains information about the user performing the operation.
+type UserInfo struct {
+	// Roles is the list of namespaced role names for the user.
+	// +optional
+	Roles []string `json:"roles,omitempty" yaml:"roles,omitempty"`
+
+	// ClusterRoles is the list of cluster-wide role names for the user.
+	// +optional
+	ClusterRoles []string `json:"clusterRoles,omitempty" yaml:"clusterRoles,omitempty"`
+
+	// Subjects is the list of subject names like users, user groups, and service accounts.
+	// +optional
+	Subjects []rbacv1.Subject `json:"subjects,omitempty" yaml:"subjects,omitempty"`
+}
+
+// ValidateSubjects implements programmatic validation of Subjects
+func (u *UserInfo) ValidateSubjects(path *field.Path) field.ErrorList {
+	var errs field.ErrorList
+	for index, subject := range u.Subjects {
+		entry := path.Index(index)
+		if subject.Kind == "" {
+			errs = append(errs, field.Required(entry.Child("kind"), ""))
+		}
+		if subject.Name == "" {
+			errs = append(errs, field.Required(entry.Child("name"), ""))
+		}
+		if subject.Kind == rbacv1.ServiceAccountKind && subject.Namespace == "" {
+			errs = append(errs, field.Required(entry.Child("namespace"), fmt.Sprintf("namespace is required when Kind is %s", rbacv1.ServiceAccountKind)))
+		}
+	}
+	return errs
+}
+
+// ValidateRoles implements programmatic validation of Roles
+func (u *UserInfo) ValidateRoles(path *field.Path) field.ErrorList {
+	var errs field.ErrorList
+	for i, r := range u.Roles {
+		role := strings.Split(r, ":")
+		if len(role) != 2 {
+			errs = append(errs, field.Invalid(path.Index(i), r, "Role is expected to be in namespace:name format"))
+		}
+	}
+	return errs
+}
+
+// Validate implements programmatic validation
+func (u *UserInfo) Validate(path *field.Path) field.ErrorList {
+	var errs field.ErrorList
+	errs = append(errs, u.ValidateSubjects(path.Child("subjects"))...)
+	errs = append(errs, u.ValidateRoles(path.Child("roles"))...)
+	return errs
+}

pkg/policy/validate.go

@@ -24,7 +24,6 @@ import (
 	"github.com/minio/pkg/wildcard"
 	"github.com/pkg/errors"
 	v1beta1 "k8s.io/api/admission/v1beta1"
-	rbacv1 "k8s.io/api/rbac/v1"
 	"k8s.io/apiextensions-apiserver/pkg/apis/apiextensions"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/validation/field"
@@ -151,7 +150,7 @@ func Validate(policy *kyverno.ClusterPolicy, client *dclient.Client, mock bool,
 		}
 
 		// validate resource description
-		if path, err := validateResources(rule); err != nil {
+		if path, err := validateResources(rulePath, rule); err != nil {
 			return nil, fmt.Errorf("path: spec.rules[%d].%s: %v", i, path, err)
 		}
 
@@ -905,10 +904,13 @@ func ruleOnlyDealsWithResourceMetaData(rule kyverno.Rule) bool {
 	return true
 }
 
-func validateResources(rule kyverno.Rule) (string, error) {
+func validateResources(path *field.Path, rule kyverno.Rule) (string, error) {
 	// validate userInfo in match and exclude
-	if path, err := validateUserInfo(rule); err != nil {
+	if errs := rule.MatchResources.UserInfo.Validate(path.Child("match")); len(errs) != 0 {
-		return fmt.Sprintf("resources.%s", path), err
+		return "match", errs.ToAggregate()
+	}
+	if errs := rule.ExcludeResources.UserInfo.Validate(path.Child("exclude")); len(errs) != 0 {
+		return "exclude", errs.ToAggregate()
 	}
 
 	if (len(rule.MatchResources.Any) > 0 || len(rule.MatchResources.All) > 0) && !reflect.DeepEqual(rule.MatchResources.ResourceDescription, kyverno.ResourceDescription{}) {
@@ -1294,57 +1296,6 @@ func validateMatchedResourceDescription(rd kyverno.ResourceDescription) (string,
 	return "", nil
 }
 
-func validateUserInfo(rule kyverno.Rule) (string, error) {
-	if err := validateRoles(rule.MatchResources.Roles); err != nil {
-		return "match.roles", err
-	}
-
-	if err := validateSubjects(rule.MatchResources.Subjects); err != nil {
-		return "match.subjects", err
-	}
-
-	if err := validateRoles(rule.ExcludeResources.Roles); err != nil {
-		return "exclude.roles", err
-	}
-
-	if err := validateSubjects(rule.ExcludeResources.Subjects); err != nil {
-		return "exclude.subjects", err
-	}
-
-	return "", nil
-}
-
-// a role must in format namespace:name
-func validateRoles(roles []string) error {
-	if len(roles) == 0 {
-		return nil
-	}
-
-	for _, r := range roles {
-		role := strings.Split(r, ":")
-		if len(role) != 2 {
-			return fmt.Errorf("invalid role %s, expect namespace:name", r)
-		}
-	}
-	return nil
-}
-
-// a namespace should be set in kind ServiceAccount of a subject
-func validateSubjects(subjects []rbacv1.Subject) error {
-	if len(subjects) == 0 {
-		return nil
-	}
-
-	for _, subject := range subjects {
-		if subject.Kind == "ServiceAccount" {
-			if subject.Namespace == "" {
-				return fmt.Errorf("service account %s in subject expects a namespace", subject.Name)
-			}
-		}
-	}
-	return nil
-}
-
 func validateExcludeResourceDescription(rd kyverno.ResourceDescription) (string, error) {
 	if reflect.DeepEqual(rd, kyverno.ResourceDescription{}) {
 		// exclude is not mandatory

pkg/policy/validate_test.go

@@ -745,66 +745,6 @@ func Test_Validate_ErrorFormat(t *testing.T) {
 	assert.Assert(t, err != nil)
 }
 
-func Test_Validate_EmptyUserInfo(t *testing.T) {
-	rawRule := []byte(`
-	{
-		"name": "test",
-		"match": {
-		   "subjects": null
-		}
-	 }`)
-
-	var rule kyverno.Rule
-	err := json.Unmarshal(rawRule, &rule)
-	assert.NilError(t, err)
-
-	_, errNew := validateUserInfo(rule)
-	assert.NilError(t, errNew)
-}
-
-func Test_Validate_Roles(t *testing.T) {
-	rawRule := []byte(`{
-		"name": "test",
-		"match": {
-		   "roles": [
-			  "namespace1:name1",
-			  "name2"
-		   ]
-		}
-	 }`)
-
-	var rule kyverno.Rule
-	err := json.Unmarshal(rawRule, &rule)
-	assert.NilError(t, err)
-
-	path, err := validateUserInfo(rule)
-	assert.Assert(t, err != nil)
-	assert.Assert(t, path == "match.roles")
-}
-
-func Test_Validate_ServiceAccount(t *testing.T) {
-	rawRule := []byte(`
-	{
-		"name": "test",
-		"exclude": {
-		   "subjects": [
-			  {
-				 "kind": "ServiceAccount",
-				 "name": "testname"
-			  }
-		   ]
-		}
-	 }`)
-
-	var rule kyverno.Rule
-	err := json.Unmarshal(rawRule, &rule)
-	assert.NilError(t, err)
-
-	path, err := validateUserInfo(rule)
-	assert.Assert(t, err != nil)
-	assert.Assert(t, path == "exclude.subjects")
-}
-
 func Test_BackGroundUserInfo_match_roles(t *testing.T) {
 	var err error
 	rawPolicy := []byte(`