RBAC: don't allow rules to mix non-resource URLs and resources

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