diff --git a/api/swagger-spec/rbac.authorization.k8s.io_v1alpha1.json b/api/swagger-spec/rbac.authorization.k8s.io_v1alpha1.json index 6c5f674c72b..fa45a90ad28 100644 --- a/api/swagger-spec/rbac.authorization.k8s.io_v1alpha1.json +++ b/api/swagger-spec/rbac.authorization.k8s.io_v1alpha1.json @@ -3075,9 +3075,7 @@ "id": "v1alpha1.PolicyRule", "description": "PolicyRule holds information that describes a policy rule, but does not contain information about who the rule applies to or which namespace the rule applies to.", "required": [ - "verbs", - "apiGroups", - "resources" + "verbs" ], "properties": { "verbs": { @@ -3117,7 +3115,7 @@ "items": { "type": "string" }, - "description": "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 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." + "description": "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." } } }, 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/generated.proto b/pkg/apis/rbac/v1alpha1/generated.proto index 062f815b81b..4973e076575 100644 --- a/pkg/apis/rbac/v1alpha1/generated.proto +++ b/pkg/apis/rbac/v1alpha1/generated.proto @@ -91,9 +91,10 @@ message PolicyRule { // ResourceNames is an optional white list of names that the rule applies to. An empty set means that everything is allowed. repeated string resourceNames = 5; - // 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. repeated string nonResourceURLs = 6; } diff --git a/pkg/apis/rbac/v1alpha1/types.generated.go b/pkg/apis/rbac/v1alpha1/types.generated.go index bf7b3db2bf3..45dfb271e97 100644 --- a/pkg/apis/rbac/v1alpha1/types.generated.go +++ b/pkg/apis/rbac/v1alpha1/types.generated.go @@ -91,13 +91,15 @@ func (x *PolicyRule) CodecEncodeSelf(e *codec1978.Encoder) { _, _, _ = yysep2, yyq2, yy2arr2 const yyr2 bool = false yyq2[1] = true + yyq2[2] = len(x.APIGroups) != 0 + yyq2[3] = len(x.Resources) != 0 yyq2[4] = len(x.ResourceNames) != 0 yyq2[5] = len(x.NonResourceURLs) != 0 var yynn2 int if yyr2 || yy2arr2 { r.EncodeArrayStart(6) } else { - yynn2 = 3 + yynn2 = 1 for _, b := range yyq2 { if b { yynn2++ @@ -168,55 +170,67 @@ func (x *PolicyRule) CodecEncodeSelf(e *codec1978.Encoder) { } if yyr2 || yy2arr2 { z.EncSendContainerState(codecSelfer_containerArrayElem1234) - if x.APIGroups == nil { - r.EncodeNil() - } else { - yym12 := z.EncBinary() - _ = yym12 - if false { + if yyq2[2] { + if x.APIGroups == nil { + r.EncodeNil() } else { - z.F.EncSliceStringV(x.APIGroups, false, e) + yym12 := z.EncBinary() + _ = yym12 + if false { + } else { + z.F.EncSliceStringV(x.APIGroups, false, e) + } } + } else { + r.EncodeNil() } } else { - z.EncSendContainerState(codecSelfer_containerMapKey1234) - r.EncodeString(codecSelferC_UTF81234, string("apiGroups")) - z.EncSendContainerState(codecSelfer_containerMapValue1234) - if x.APIGroups == nil { - r.EncodeNil() - } else { - yym13 := z.EncBinary() - _ = yym13 - if false { + if yyq2[2] { + z.EncSendContainerState(codecSelfer_containerMapKey1234) + r.EncodeString(codecSelferC_UTF81234, string("apiGroups")) + z.EncSendContainerState(codecSelfer_containerMapValue1234) + if x.APIGroups == nil { + r.EncodeNil() } else { - z.F.EncSliceStringV(x.APIGroups, false, e) + yym13 := z.EncBinary() + _ = yym13 + if false { + } else { + z.F.EncSliceStringV(x.APIGroups, false, e) + } } } } if yyr2 || yy2arr2 { z.EncSendContainerState(codecSelfer_containerArrayElem1234) - if x.Resources == nil { - r.EncodeNil() - } else { - yym15 := z.EncBinary() - _ = yym15 - if false { + if yyq2[3] { + if x.Resources == nil { + r.EncodeNil() } else { - z.F.EncSliceStringV(x.Resources, false, e) + yym15 := z.EncBinary() + _ = yym15 + if false { + } else { + z.F.EncSliceStringV(x.Resources, false, e) + } } + } else { + r.EncodeNil() } } else { - z.EncSendContainerState(codecSelfer_containerMapKey1234) - r.EncodeString(codecSelferC_UTF81234, string("resources")) - z.EncSendContainerState(codecSelfer_containerMapValue1234) - if x.Resources == nil { - r.EncodeNil() - } else { - yym16 := z.EncBinary() - _ = yym16 - if false { + if yyq2[3] { + z.EncSendContainerState(codecSelfer_containerMapKey1234) + r.EncodeString(codecSelferC_UTF81234, string("resources")) + z.EncSendContainerState(codecSelfer_containerMapValue1234) + if x.Resources == nil { + r.EncodeNil() } else { - z.F.EncSliceStringV(x.Resources, false, e) + yym16 := z.EncBinary() + _ = yym16 + if false { + } else { + z.F.EncSliceStringV(x.Resources, false, e) + } } } } 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/v1alpha1/types_swagger_doc_generated.go b/pkg/apis/rbac/v1alpha1/types_swagger_doc_generated.go index f1c6fdbdf61..6d1c4bc9d32 100644 --- a/pkg/apis/rbac/v1alpha1/types_swagger_doc_generated.go +++ b/pkg/apis/rbac/v1alpha1/types_swagger_doc_generated.go @@ -75,7 +75,7 @@ var map_PolicyRule = map[string]string{ "apiGroups": "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.", "resources": "Resources is a list of resources this rule applies to. ResourceAll represents all resources.", "resourceNames": "ResourceNames is an optional white list of names that the rule applies to. An empty set means that everything is allowed.", - "nonResourceURLs": "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 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.", + "nonResourceURLs": "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.", } func (PolicyRule) SwaggerDoc() map[string]string { 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) +}