From 5541189c6c681352f0b3313c70f7b3800ba76dfb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Charles-Edouard=20Br=C3=A9t=C3=A9ch=C3=A9?= Date: Wed, 16 Mar 2022 17:15:46 +0100 Subject: [PATCH] refactor: UserInfo validation (#3399) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Charles-Edouard Brétéché --- api/kyverno/v1/common_types.go | 16 -------- api/kyverno/v1/user_info_test.go | 48 +++++++++++++++++++++++ api/kyverno/v1/user_info_types.go | 62 ++++++++++++++++++++++++++++++ pkg/policy/validate.go | 63 ++++--------------------------- pkg/policy/validate_test.go | 60 ----------------------------- 5 files changed, 117 insertions(+), 132 deletions(-) create mode 100644 api/kyverno/v1/user_info_test.go create mode 100644 api/kyverno/v1/user_info_types.go diff --git a/api/kyverno/v1/common_types.go b/api/kyverno/v1/common_types.go index 0e01cae0a8..38949a0d57 100755 --- a/api/kyverno/v1/common_types.go +++ b/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. diff --git a/api/kyverno/v1/user_info_test.go b/api/kyverno/v1/user_info_test.go new file mode 100644 index 0000000000..68854ed246 --- /dev/null +++ b/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]") +} diff --git a/api/kyverno/v1/user_info_types.go b/api/kyverno/v1/user_info_types.go new file mode 100644 index 0000000000..e63392e120 --- /dev/null +++ b/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 +} diff --git a/pkg/policy/validate.go b/pkg/policy/validate.go index bd28759a3c..767eeca00f 100644 --- a/pkg/policy/validate.go +++ b/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 { - return fmt.Sprintf("resources.%s", path), err + if errs := rule.MatchResources.UserInfo.Validate(path.Child("match")); len(errs) != 0 { + 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 diff --git a/pkg/policy/validate_test.go b/pkg/policy/validate_test.go index 447f0142d4..d12c9aa1cc 100644 --- a/pkg/policy/validate_test.go +++ b/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(`