Merge pull request #29930 from ericchiang/rbac-validation-dont-mix-non-resource-urls-and-resources

Automatic merge from submit-queue

rbac validation: rules can't combine non-resource URLs and regular resources

This PR updates the validation used for RBAC to prevent rules from mixing non-resource URLs and regular resources.

For example the following is no longer valid

```yml
kind: ClusterRole
apiVersion: rbac.authorization.k8s.io/v1alpha1
metadata:
  name: admins
rules:
  - apiGroups: ["*"]
    resources: ["*"]
    verbs: ["*"]
    nonResourceURLs: ["*"]
```

And must be rewritten as so.

```yml
kind: ClusterRole
apiVersion: rbac.authorization.k8s.io/v1alpha1
metadata:
  name: admins
rules:
  - apiGroups: ["*"]
    resources: ["*"]
    verbs: ["*"]
  - nonResourceURLs: ["*"]
    verbs: ["*"]
``` 

It also:
* Mandates non-zero length arrays for required resources.
* Mandates non-resource URLs only be used for ClusterRoles (not namespaced Roles).
* Updates the swagger validation so `verbs` are the only required field in a rule. Further validation is done by the server.

Also, do we need to bump the API version?

Discussed by @erictune and @liggitt  in #28304

Updates kubernetes/features#2

cc @kubernetes/sig-auth 

Edit:
* Need to update the RBAC docs if this change goes in.

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."
      }
     }
    },

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
 }
 

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;
 }
 

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)
+						}
 					}
 				}
 			}

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"`
 }
 

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 {

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 {

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)
+}