From 93947663d9106010b085a9ab439eabe476ceaaec Mon Sep 17 00:00:00 2001 From: Eric Chiang Date: Mon, 1 Aug 2016 17:47:47 -0700 Subject: [PATCH] RBAC: don't allow rules to mix non-resource URLs and resources --- pkg/apis/rbac/types.go | 3 + pkg/apis/rbac/v1alpha1/types.go | 9 +- pkg/apis/rbac/validation/validation.go | 38 ++- pkg/apis/rbac/validation/validation_test.go | 264 ++++++++++++++++---- 4 files changed, 262 insertions(+), 52 deletions(-) diff --git a/pkg/apis/rbac/types.go b/pkg/apis/rbac/types.go index 44a38971b45..9e971b4f8d2 100644 --- a/pkg/apis/rbac/types.go +++ b/pkg/apis/rbac/types.go @@ -50,14 +50,17 @@ type PolicyRule struct { AttributeRestrictions runtime.Object // APIGroups is the name of the APIGroup that contains the resources. // If multiple API groups are specified, any action requested against one of the enumerated resources in any API group will be allowed. + APIGroups []string // Resources is a list of resources this rule applies to. ResourceAll represents all resources. Resources []string // ResourceNames is an optional white list of names that the rule applies to. An empty set means that everything is allowed. ResourceNames []string + // NonResourceURLs is a set of partial urls that a user should have access to. *s are allowed, but only as the full, final step in the path // If an action is not a resource API request, then the URL is split on '/' and is checked against the NonResourceURLs to look for a match. // Since non-resource URLs are not namespaced, this field is only applicable for ClusterRoles referenced from a ClusterRoleBinding. + // Rules can either apply to API resources (such as "pods" or "secrets") or non-resource URL paths (such as "/api"), but not both. NonResourceURLs []string } diff --git a/pkg/apis/rbac/v1alpha1/types.go b/pkg/apis/rbac/v1alpha1/types.go index a260facea9b..e8a815ee67c 100644 --- a/pkg/apis/rbac/v1alpha1/types.go +++ b/pkg/apis/rbac/v1alpha1/types.go @@ -35,16 +35,19 @@ type PolicyRule struct { // AttributeRestrictions will vary depending on what the Authorizer/AuthorizationAttributeBuilder pair supports. // If the Authorizer does not recognize how to handle the AttributeRestrictions, the Authorizer should report an error. AttributeRestrictions runtime.RawExtension `json:"attributeRestrictions,omitempty" protobuf:"bytes,2,opt,name=attributeRestrictions"` + // APIGroups is the name of the APIGroup that contains the resources. If multiple API groups are specified, any action requested against one of // the enumerated resources in any API group will be allowed. - APIGroups []string `json:"apiGroups" protobuf:"bytes,3,rep,name=apiGroups"` + APIGroups []string `json:"apiGroups,omitempty" protobuf:"bytes,3,rep,name=apiGroups"` // Resources is a list of resources this rule applies to. ResourceAll represents all resources. - Resources []string `json:"resources" protobuf:"bytes,4,rep,name=resources"` + Resources []string `json:"resources,omitempty" protobuf:"bytes,4,rep,name=resources"` // ResourceNames is an optional white list of names that the rule applies to. An empty set means that everything is allowed. ResourceNames []string `json:"resourceNames,omitempty" protobuf:"bytes,5,rep,name=resourceNames"` - // NonResourceURLsSlice is a set of partial urls that a user should have access to. *s are allowed, but only as the full, final step in the path + + // NonResourceURLs is a set of partial urls that a user should have access to. *s are allowed, but only as the full, final step in the path // This name is intentionally different than the internal type so that the DefaultConvert works nicely and because the ordering may be different. // Since non-resource URLs are not namespaced, this field is only applicable for ClusterRoles referenced from a ClusterRoleBinding. + // Rules can either apply to API resources (such as "pods" or "secrets") or non-resource URL paths (such as "/api"), but not both. NonResourceURLs []string `json:"nonResourceURLs,omitempty" protobuf:"bytes,6,rep,name=nonResourceURLs"` } diff --git a/pkg/apis/rbac/validation/validation.go b/pkg/apis/rbac/validation/validation.go index 574a7b6dedb..f4df4dbe4e2 100644 --- a/pkg/apis/rbac/validation/validation.go +++ b/pkg/apis/rbac/validation/validation.go @@ -46,7 +46,43 @@ func ValidateClusterRoleUpdate(policy *rbac.ClusterRole, oldRole *rbac.ClusterRo } func validateRole(role *rbac.Role, isNamespaced bool) field.ErrorList { - return validation.ValidateObjectMeta(&role.ObjectMeta, isNamespaced, minimalNameRequirements, field.NewPath("metadata")) + allErrs := field.ErrorList{} + allErrs = append(allErrs, validation.ValidateObjectMeta(&role.ObjectMeta, isNamespaced, minimalNameRequirements, field.NewPath("metadata"))...) + + for i, rule := range role.Rules { + if err := validatePolicyRule(rule, isNamespaced, field.NewPath("rules").Index(i)); err != nil { + allErrs = append(allErrs, err...) + } + } + if len(allErrs) != 0 { + return allErrs + } + return nil +} + +func validatePolicyRule(rule rbac.PolicyRule, isNamespaced bool, fldPath *field.Path) field.ErrorList { + allErrs := field.ErrorList{} + if len(rule.Verbs) == 0 { + allErrs = append(allErrs, field.Required(fldPath.Child("verbs"), "verbs must contain at least one value")) + } + + if len(rule.NonResourceURLs) > 0 { + if isNamespaced { + allErrs = append(allErrs, field.Invalid(fldPath.Child("nonResourceURLs"), rule.NonResourceURLs, "namespaced rules cannot apply to non-resource URLs")) + } + if len(rule.APIGroups) > 0 || len(rule.Resources) > 0 || len(rule.ResourceNames) > 0 { + allErrs = append(allErrs, field.Invalid(fldPath.Child("nonResourceURLs"), rule.NonResourceURLs, "rules cannot apply to both regular resources and non-resource URLs")) + } + return allErrs + } + + if len(rule.APIGroups) == 0 { + allErrs = append(allErrs, field.Required(fldPath.Child("apiGroups"), "resource rules must supply at least one api group")) + } + if len(rule.Resources) == 0 { + allErrs = append(allErrs, field.Required(fldPath.Child("resources"), "resource rules must supply at least one resource")) + } + return allErrs } func validateRoleUpdate(role *rbac.Role, oldRole *rbac.Role, isNamespaced bool) field.ErrorList { diff --git a/pkg/apis/rbac/validation/validation_test.go b/pkg/apis/rbac/validation/validation_test.go index 1325bef99e3..7e4f1699a1f 100644 --- a/pkg/apis/rbac/validation/validation_test.go +++ b/pkg/apis/rbac/validation/validation_test.go @@ -172,54 +172,6 @@ func TestValidateRoleBindingUpdate(t *testing.T) { } } -func TestValidateRole(t *testing.T) { - errs := validateRole( - &rbac.Role{ - ObjectMeta: api.ObjectMeta{Namespace: api.NamespaceDefault, Name: "master"}, - }, - true, - ) - if len(errs) != 0 { - t.Errorf("expected success: %v", errs) - } - - errorCases := map[string]struct { - A rbac.Role - T field.ErrorType - F string - }{ - "zero-length namespace": { - A: rbac.Role{ - ObjectMeta: api.ObjectMeta{Name: "default"}, - }, - T: field.ErrorTypeRequired, - F: "metadata.namespace", - }, - "zero-length name": { - A: rbac.Role{ - ObjectMeta: api.ObjectMeta{Namespace: api.NamespaceDefault}, - }, - T: field.ErrorTypeRequired, - F: "metadata.name", - }, - } - for k, v := range errorCases { - errs := validateRole(&v.A, true) - if len(errs) == 0 { - t.Errorf("expected failure %s for %v", k, v.A) - continue - } - for i := range errs { - if errs[i].Type != v.T { - t.Errorf("%s: expected errors to have type %s: %v", k, v.T, errs[i]) - } - if errs[i].Field != v.F { - t.Errorf("%s: expected errors to have field %s: %v", k, v.F, errs[i]) - } - } - } -} - func TestNonResourceURLCovers(t *testing.T) { tests := []struct { owner string @@ -244,3 +196,219 @@ func TestNonResourceURLCovers(t *testing.T) { } } } + +type validateRoleTest struct { + role rbac.Role + isNamespaced bool + wantErr bool + errType field.ErrorType + field string +} + +func (v validateRoleTest) test(t *testing.T) { + errs := validateRole(&v.role, v.isNamespaced) + if len(errs) == 0 { + if v.wantErr { + t.Fatal("expected validation error") + } + return + } + if !v.wantErr { + t.Errorf("didn't expect error, got %v", errs) + return + } + for i := range errs { + if errs[i].Type != v.errType { + t.Errorf("expected errors to have type %s: %v", v.errType, errs[i]) + } + if errs[i].Field != v.field { + t.Errorf("expected errors to have field %s: %v", v.field, errs[i]) + } + } +} + +func TestValidateRoleZeroLengthNamespace(t *testing.T) { + validateRoleTest{ + role: rbac.Role{ + ObjectMeta: api.ObjectMeta{Name: "default"}, + }, + isNamespaced: true, + wantErr: true, + errType: field.ErrorTypeRequired, + field: "metadata.namespace", + }.test(t) +} + +func TestValidateRoleZeroLengthName(t *testing.T) { + validateRoleTest{ + role: rbac.Role{ + ObjectMeta: api.ObjectMeta{Namespace: "default"}, + }, + isNamespaced: true, + wantErr: true, + errType: field.ErrorTypeRequired, + field: "metadata.name", + }.test(t) +} + +func TestValidateRoleValidRole(t *testing.T) { + validateRoleTest{ + role: rbac.Role{ + ObjectMeta: api.ObjectMeta{ + Namespace: "default", + Name: "default", + }, + }, + isNamespaced: true, + wantErr: false, + }.test(t) +} + +func TestValidateRoleValidRoleNoNamespace(t *testing.T) { + validateRoleTest{ + role: rbac.Role{ + ObjectMeta: api.ObjectMeta{ + Name: "default", + }, + }, + isNamespaced: false, + wantErr: false, + }.test(t) +} + +func TestValidateRoleNonResourceURL(t *testing.T) { + validateRoleTest{ + role: rbac.Role{ + ObjectMeta: api.ObjectMeta{ + Name: "default", + }, + Rules: []rbac.PolicyRule{ + { + Verbs: []string{"get"}, + NonResourceURLs: []string{"/*"}, + }, + }, + }, + isNamespaced: false, + wantErr: false, + }.test(t) +} + +func TestValidateRoleNamespacedNonResourceURL(t *testing.T) { + validateRoleTest{ + role: rbac.Role{ + ObjectMeta: api.ObjectMeta{ + Namespace: "default", + Name: "default", + }, + Rules: []rbac.PolicyRule{ + { + // non-resource URLs are invalid for namespaced rules + Verbs: []string{"get"}, + NonResourceURLs: []string{"/*"}, + }, + }, + }, + isNamespaced: true, + wantErr: true, + errType: field.ErrorTypeInvalid, + field: "rules[0].nonResourceURLs", + }.test(t) +} + +func TestValidateRoleNonResourceURLNoVerbs(t *testing.T) { + validateRoleTest{ + role: rbac.Role{ + ObjectMeta: api.ObjectMeta{ + Name: "default", + }, + Rules: []rbac.PolicyRule{ + { + Verbs: []string{}, + NonResourceURLs: []string{"/*"}, + }, + }, + }, + isNamespaced: false, + wantErr: true, + errType: field.ErrorTypeRequired, + field: "rules[0].verbs", + }.test(t) +} + +func TestValidateRoleMixedNonResourceAndResource(t *testing.T) { + validateRoleTest{ + role: rbac.Role{ + ObjectMeta: api.ObjectMeta{ + Name: "default", + }, + Rules: []rbac.PolicyRule{ + { + Verbs: []string{"get"}, + NonResourceURLs: []string{"/*"}, + APIGroups: []string{"v1"}, + Resources: []string{"pods"}, + }, + }, + }, + wantErr: true, + errType: field.ErrorTypeInvalid, + field: "rules[0].nonResourceURLs", + }.test(t) +} + +func TestValidateRoleValidResource(t *testing.T) { + validateRoleTest{ + role: rbac.Role{ + ObjectMeta: api.ObjectMeta{ + Name: "default", + }, + Rules: []rbac.PolicyRule{ + { + Verbs: []string{"get"}, + APIGroups: []string{"v1"}, + Resources: []string{"pods"}, + }, + }, + }, + wantErr: false, + }.test(t) +} + +func TestValidateRoleNoAPIGroup(t *testing.T) { + validateRoleTest{ + role: rbac.Role{ + ObjectMeta: api.ObjectMeta{ + Name: "default", + }, + Rules: []rbac.PolicyRule{ + { + Verbs: []string{"get"}, + Resources: []string{"pods"}, + }, + }, + }, + wantErr: true, + errType: field.ErrorTypeRequired, + field: "rules[0].apiGroups", + }.test(t) +} + +func TestValidateRoleNoResources(t *testing.T) { + validateRoleTest{ + role: rbac.Role{ + ObjectMeta: api.ObjectMeta{ + Name: "default", + }, + Rules: []rbac.PolicyRule{ + { + Verbs: []string{"get"}, + APIGroups: []string{"v1"}, + }, + }, + }, + wantErr: true, + errType: field.ErrorTypeRequired, + field: "rules[0].resources", + }.test(t) +}