make rbac authorizer use rule comparison, not covers

pkg/apis/rbac/helpers.go

@@ -17,9 +17,80 @@ limitations under the License.
 package rbac
 
 import (
+	"strings"
+
 	"k8s.io/kubernetes/pkg/api/unversioned"
 )
 
 func RoleRefGroupKind(roleRef RoleRef) unversioned.GroupKind {
 	return unversioned.GroupKind{Group: roleRef.APIGroup, Kind: roleRef.Kind}
 }
+
+func VerbMatches(rule PolicyRule, requestedVerb string) bool {
+	for _, ruleVerb := range rule.Verbs {
+		if ruleVerb == VerbAll {
+			return true
+		}
+		if ruleVerb == requestedVerb {
+			return true
+		}
+	}
+
+	return false
+}
+
+func APIGroupMatches(rule PolicyRule, requestedGroup string) bool {
+	for _, ruleGroup := range rule.APIGroups {
+		if ruleGroup == APIGroupAll {
+			return true
+		}
+		if ruleGroup == requestedGroup {
+			return true
+		}
+	}
+
+	return false
+}
+
+func ResourceMatches(rule PolicyRule, requestedResource string) bool {
+	for _, ruleResource := range rule.Resources {
+		if ruleResource == ResourceAll {
+			return true
+		}
+		if ruleResource == requestedResource {
+			return true
+		}
+	}
+
+	return false
+}
+
+func ResourceNameMatches(rule PolicyRule, requestedName string) bool {
+	if len(rule.ResourceNames) == 0 {
+		return true
+	}
+
+	for _, ruleName := range rule.ResourceNames {
+		if ruleName == requestedName {
+			return true
+		}
+	}
+
+	return false
+}
+
+func NonResourceURLMatches(rule PolicyRule, requestedURL string) bool {
+	for _, ruleURL := range rule.NonResourceURLs {
+		if ruleURL == NonResourceAll {
+			return true
+		}
+		if ruleURL == requestedURL {
+			return true
+		}
+		if strings.HasSuffix(ruleURL, "*") && strings.HasPrefix(requestedURL, strings.TrimRight(ruleURL, "*")) {
+			return true
+		}
+	}
+
+	return false
+}

pkg/apis/rbac/validation/policy_comparator.go

@@ -131,7 +131,6 @@ func nonResourceURLCovers(ownerPath, subPath string) bool {
 // ruleCovers determines whether the ownerRule (which may have multiple verbs, resources, and resourceNames) covers
 // the subrule (which may only contain at most one verb, resource, and resourceName)
 func ruleCovers(ownerRule, subRule rbac.PolicyRule) bool {
-
 	verbMatches := has(ownerRule.Verbs, rbac.VerbAll) || hasAll(ownerRule.Verbs, subRule.Verbs)
 	groupMatches := has(ownerRule.APIGroups, rbac.APIGroupAll) || hasAll(ownerRule.APIGroups, subRule.APIGroups)
 	resourceMatches := has(ownerRule.Resources, rbac.ResourceAll) || hasAll(ownerRule.Resources, subRule.Resources)

pkg/apis/rbac/validation/rulevalidation.go

@@ -34,37 +34,29 @@ type AuthorizationRuleResolver interface {
 	// of the role binding, the empty string if a cluster role binding.
 	GetRoleReferenceRules(ctx api.Context, roleRef rbac.RoleRef, namespace string) ([]rbac.PolicyRule, error)
 
-	// GetEffectivePolicyRules returns the list of rules that apply to a given user in a given namespace and error.  If an error is returned, the slice of
+	// RulesFor returns the list of rules that apply to a given user in a given namespace and error.  If an error is returned, the slice of
 	// PolicyRules may not be complete, but it contains all retrievable rules.  This is done because policy rules are purely additive and policy determinations
 	// can be made on the basis of those rules that are found.
-	GetEffectivePolicyRules(ctx api.Context) ([]rbac.PolicyRule, error)
+	RulesFor(user user.Info, namespace string) ([]rbac.PolicyRule, error)
 }
 
 // ConfirmNoEscalation determines if the roles for a given user in a given namespace encompass the provided role.
 func ConfirmNoEscalation(ctx api.Context, ruleResolver AuthorizationRuleResolver, rules []rbac.PolicyRule) error {
 	ruleResolutionErrors := []error{}
 
-	ownerLocalRules, err := ruleResolver.GetEffectivePolicyRules(ctx)
+	user, ok := api.UserFrom(ctx)
+	if !ok {
+		return fmt.Errorf("no user on context")
+	}
+	namespace, _ := api.NamespaceFrom(ctx)
+
+	ownerRules, err := ruleResolver.RulesFor(user, namespace)
 	if err != nil {
 		// As per AuthorizationRuleResolver contract, this may return a non fatal error with an incomplete list of policies. Log the error and continue.
-		user, _ := api.UserFrom(ctx)
 		glog.V(1).Infof("non-fatal error getting local rules for %v: %v", user, err)
 		ruleResolutionErrors = append(ruleResolutionErrors, err)
 	}
 
-	masterContext := api.WithNamespace(ctx, "")
-	ownerGlobalRules, err := ruleResolver.GetEffectivePolicyRules(masterContext)
-	if err != nil {
-		// Same case as above. Log error, don't fail.
-		user, _ := api.UserFrom(ctx)
-		glog.V(1).Infof("non-fatal error getting global rules for %v: %v", user, err)
-		ruleResolutionErrors = append(ruleResolutionErrors, err)
-	}
-
-	ownerRules := make([]rbac.PolicyRule, 0, len(ownerGlobalRules)+len(ownerLocalRules))
-	ownerRules = append(ownerRules, ownerLocalRules...)
-	ownerRules = append(ownerRules, ownerGlobalRules...)
-
 	ownerRightsCover, missingRights := Covers(ownerRules, rules)
 	if !ownerRightsCover {
 		user, _ := api.UserFrom(ctx)
@@ -100,7 +92,53 @@ type ClusterRoleBindingLister interface {
 	ListClusterRoleBindings(ctx api.Context, options *api.ListOptions) (*rbac.ClusterRoleBindingList, error)
 }
 
-// GetRoleReferenceRules attempts resolve the RoleBinding or ClusterRoleBinding.
+func (r *DefaultRuleResolver) RulesFor(user user.Info, namespace string) ([]rbac.PolicyRule, error) {
+	policyRules := []rbac.PolicyRule{}
+	errorlist := []error{}
+
+	ctx := api.NewContext()
+	if clusterRoleBindings, err := r.clusterRoleBindingLister.ListClusterRoleBindings(ctx, &api.ListOptions{}); err != nil {
+		errorlist = append(errorlist, err)
+
+	} else {
+		for _, clusterRoleBinding := range clusterRoleBindings.Items {
+			if !appliesTo(user, clusterRoleBinding.Subjects, "") {
+				continue
+			}
+			rules, err := r.GetRoleReferenceRules(ctx, clusterRoleBinding.RoleRef, "")
+			if err != nil {
+				errorlist = append(errorlist, err)
+				continue
+			}
+			policyRules = append(policyRules, rules...)
+		}
+	}
+
+	if len(namespace) > 0 {
+		ctx := api.WithNamespace(api.NewContext(), namespace)
+
+		if roleBindings, err := r.roleBindingLister.ListRoleBindings(ctx, &api.ListOptions{}); err != nil {
+			errorlist = append(errorlist, err)
+
+		} else {
+			for _, roleBinding := range roleBindings.Items {
+				if !appliesTo(user, roleBinding.Subjects, namespace) {
+					continue
+				}
+				rules, err := r.GetRoleReferenceRules(ctx, roleBinding.RoleRef, namespace)
+				if err != nil {
+					errorlist = append(errorlist, err)
+					continue
+				}
+				policyRules = append(policyRules, rules...)
+			}
+		}
+	}
+
+	return policyRules, utilerrors.NewAggregate(errorlist)
+}
+
+// GetRoleReferenceRules attempts to resolve the RoleBinding or ClusterRoleBinding.
 func (r *DefaultRuleResolver) GetRoleReferenceRules(ctx api.Context, roleRef rbac.RoleRef, bindingNamespace string) ([]rbac.PolicyRule, error) {
 	switch kind := rbac.RoleRefGroupKind(roleRef); kind {
 	case rbac.Kind("Role"):
@@ -121,83 +159,36 @@ func (r *DefaultRuleResolver) GetRoleReferenceRules(ctx api.Context, roleRef rba
 		return nil, fmt.Errorf("unsupported role reference kind: %q", kind)
 	}
 }
-
-func (r *DefaultRuleResolver) GetEffectivePolicyRules(ctx api.Context) ([]rbac.PolicyRule, error) {
-	policyRules := []rbac.PolicyRule{}
-	errorlist := []error{}
-
-	if namespace := api.NamespaceValue(ctx); len(namespace) == 0 {
-		clusterRoleBindings, err := r.clusterRoleBindingLister.ListClusterRoleBindings(ctx, &api.ListOptions{})
-		if err != nil {
-			return nil, err
-		}
-
-		for _, clusterRoleBinding := range clusterRoleBindings.Items {
-			if ok, err := appliesTo(ctx, clusterRoleBinding.Subjects); err != nil {
-				errorlist = append(errorlist, err)
-			} else if !ok {
-				continue
-			}
-			rules, err := r.GetRoleReferenceRules(ctx, clusterRoleBinding.RoleRef, namespace)
-			if err != nil {
-				errorlist = append(errorlist, err)
-				continue
-			}
-			policyRules = append(policyRules, rules...)
-		}
-	} else {
-		roleBindings, err := r.roleBindingLister.ListRoleBindings(ctx, &api.ListOptions{})
-		if err != nil {
-			return nil, err
-		}
-
-		for _, roleBinding := range roleBindings.Items {
-			if ok, err := appliesTo(ctx, roleBinding.Subjects); err != nil {
-				errorlist = append(errorlist, err)
-			} else if !ok {
-				continue
-			}
-			rules, err := r.GetRoleReferenceRules(ctx, roleBinding.RoleRef, namespace)
-			if err != nil {
-				errorlist = append(errorlist, err)
-				continue
-			}
-			policyRules = append(policyRules, rules...)
+func appliesTo(user user.Info, bindingSubjects []rbac.Subject, namespace string) bool {
+	for _, bindingSubject := range bindingSubjects {
+		if appliesToUser(user, bindingSubject, namespace) {
+			return true
 		}
 	}
-
-	if len(errorlist) != 0 {
-		return policyRules, utilerrors.NewAggregate(errorlist)
-	}
-	return policyRules, nil
+	return false
 }
 
-func appliesTo(ctx api.Context, subjects []rbac.Subject) (bool, error) {
-	user, ok := api.UserFrom(ctx)
-	if !ok {
-		return false, fmt.Errorf("no user data associated with context")
-	}
-	for _, subject := range subjects {
-		if ok, err := appliesToUser(user, subject); err != nil || ok {
-			return ok, err
-		}
-	}
-	return false, nil
-}
-
-func appliesToUser(user user.Info, subject rbac.Subject) (bool, error) {
+func appliesToUser(user user.Info, subject rbac.Subject, namespace string) bool {
 	switch subject.Kind {
 	case rbac.UserKind:
-		return subject.Name == rbac.UserAll || user.GetName() == subject.Name, nil
+		return subject.Name == rbac.UserAll || user.GetName() == subject.Name
+
 	case rbac.GroupKind:
-		return has(user.GetGroups(), subject.Name), nil
+		return has(user.GetGroups(), subject.Name)
+
 	case rbac.ServiceAccountKind:
-		if subject.Namespace == "" {
-			return false, fmt.Errorf("subject of kind service account without specified namespace")
+		// default the namespace to namespace we're working in if its available.  This allows rolebindings that reference
+		// SAs in th local namespace to avoid having to qualify them.
+		saNamespace := namespace
+		if len(subject.Namespace) > 0 {
+			saNamespace = subject.Namespace
 		}
-		return serviceaccount.MakeUsername(subject.Namespace, subject.Name) == user.GetName(), nil
+		if len(saNamespace) == 0 {
+			return false
+		}
+		return serviceaccount.MakeUsername(saNamespace, subject.Name) == user.GetName()
 	default:
-		return false, fmt.Errorf("unknown subject kind: %s", subject.Kind)
+		return false
 	}
 }
 

pkg/apis/rbac/validation/rulevalidation_test.go

@@ -114,52 +114,38 @@ func TestDefaultRuleResolver(t *testing.T) {
 		staticRoles
 
 		// For a given context, what are the rules that apply?
-		ctx            api.Context
+		user           user.Info
+		namespace      string
 		effectiveRules []rbac.PolicyRule
 	}{
 		{
-			staticRoles: staticRoles1,
-			ctx: api.WithNamespace(
-				api.WithUser(api.NewContext(), &user.DefaultInfo{Name: "foobar"}), "namespace1",
-			),
+			staticRoles:    staticRoles1,
+			user:           &user.DefaultInfo{Name: "foobar"},
+			namespace:      "namespace1",
 			effectiveRules: []rbac.PolicyRule{ruleReadPods, ruleReadServices},
 		},
 		{
-			staticRoles: staticRoles1,
-			ctx: api.WithNamespace(
-				// Same as above but diffrerent namespace. Should return no rules.
-				api.WithUser(api.NewContext(), &user.DefaultInfo{Name: "foobar"}), "namespace2",
-			),
+			staticRoles:    staticRoles1,
+			user:           &user.DefaultInfo{Name: "foobar"},
+			namespace:      "namespace2",
 			effectiveRules: []rbac.PolicyRule{},
 		},
-		{
-			staticRoles: staticRoles1,
-			// GetEffectivePolicyRules only returns the policies for the namespace, not the master namespace.
-			ctx: api.WithNamespace(
-				api.WithUser(api.NewContext(), &user.DefaultInfo{
-					Name: "foobar", Groups: []string{"admin"},
-				}), "namespace1",
-			),
-			effectiveRules: []rbac.PolicyRule{ruleReadPods, ruleReadServices},
-		},
 		{
 			staticRoles: staticRoles1,
 			// Same as above but without a namespace. Only cluster rules should apply.
-			ctx: api.WithUser(api.NewContext(), &user.DefaultInfo{
-				Name: "foobar", Groups: []string{"admin"},
-			}),
+			user:           &user.DefaultInfo{Name: "foobar", Groups: []string{"admin"}},
 			effectiveRules: []rbac.PolicyRule{ruleAdmin},
 		},
 		{
 			staticRoles:    staticRoles1,
-			ctx:            api.WithUser(api.NewContext(), &user.DefaultInfo{}),
+			user:           &user.DefaultInfo{},
 			effectiveRules: []rbac.PolicyRule{},
 		},
 	}
 
 	for i, tc := range tests {
 		ruleResolver := newMockRuleResolver(&tc.staticRoles)
-		rules, err := ruleResolver.GetEffectivePolicyRules(tc.ctx)
+		rules, err := ruleResolver.RulesFor(tc.user, tc.namespace)
 		if err != nil {
 			t.Errorf("case %d: GetEffectivePolicyRules(context)=%v", i, err)
 			continue
@@ -179,7 +165,8 @@ func TestDefaultRuleResolver(t *testing.T) {
 func TestAppliesTo(t *testing.T) {
 	tests := []struct {
 		subjects  []rbac.Subject
-		ctx       api.Context
+		user      user.Info
+		namespace string
 		appliesTo bool
 		testCase  string
 	}{
@@ -187,7 +174,7 @@ func TestAppliesTo(t *testing.T) {
 			subjects: []rbac.Subject{
 				{Kind: rbac.UserKind, Name: "foobar"},
 			},
-			ctx:       api.WithUser(api.NewContext(), &user.DefaultInfo{Name: "foobar"}),
+			user:      &user.DefaultInfo{Name: "foobar"},
 			appliesTo: true,
 			testCase:  "single subject that matches username",
 		},
@@ -196,7 +183,7 @@ func TestAppliesTo(t *testing.T) {
 				{Kind: rbac.UserKind, Name: "barfoo"},
 				{Kind: rbac.UserKind, Name: "foobar"},
 			},
-			ctx:       api.WithUser(api.NewContext(), &user.DefaultInfo{Name: "foobar"}),
+			user:      &user.DefaultInfo{Name: "foobar"},
 			appliesTo: true,
 			testCase:  "multiple subjects, one that matches username",
 		},
@@ -205,7 +192,7 @@ func TestAppliesTo(t *testing.T) {
 				{Kind: rbac.UserKind, Name: "barfoo"},
 				{Kind: rbac.UserKind, Name: "foobar"},
 			},
-			ctx:       api.WithUser(api.NewContext(), &user.DefaultInfo{Name: "zimzam"}),
+			user:      &user.DefaultInfo{Name: "zimzam"},
 			appliesTo: false,
 			testCase:  "multiple subjects, none that match username",
 		},
@@ -214,7 +201,7 @@ func TestAppliesTo(t *testing.T) {
 				{Kind: rbac.UserKind, Name: "barfoo"},
 				{Kind: rbac.GroupKind, Name: "foobar"},
 			},
-			ctx:       api.WithUser(api.NewContext(), &user.DefaultInfo{Name: "zimzam", Groups: []string{"foobar"}}),
+			user:      &user.DefaultInfo{Name: "zimzam", Groups: []string{"foobar"}},
 			appliesTo: true,
 			testCase:  "multiple subjects, one that match group",
 		},
@@ -223,10 +210,8 @@ func TestAppliesTo(t *testing.T) {
 				{Kind: rbac.UserKind, Name: "barfoo"},
 				{Kind: rbac.GroupKind, Name: "foobar"},
 			},
-			ctx: api.WithNamespace(
-				api.WithUser(api.NewContext(), &user.DefaultInfo{Name: "zimzam", Groups: []string{"foobar"}}),
-				"namespace1",
-			),
+			user:      &user.DefaultInfo{Name: "zimzam", Groups: []string{"foobar"}},
+			namespace: "namespace1",
 			appliesTo: true,
 			testCase:  "multiple subjects, one that match group, should ignore namespace",
 		},
@@ -236,10 +221,8 @@ func TestAppliesTo(t *testing.T) {
 				{Kind: rbac.GroupKind, Name: "foobar"},
 				{Kind: rbac.ServiceAccountKind, Namespace: "kube-system", Name: "default"},
 			},
-			ctx: api.WithNamespace(
-				api.WithUser(api.NewContext(), &user.DefaultInfo{Name: "system:serviceaccount:kube-system:default"}),
-				"default",
-			),
+			user:      &user.DefaultInfo{Name: "system:serviceaccount:kube-system:default"},
+			namespace: "default",
 			appliesTo: true,
 			testCase:  "multiple subjects with a service account that matches",
 		},
@@ -247,21 +230,15 @@ func TestAppliesTo(t *testing.T) {
 			subjects: []rbac.Subject{
 				{Kind: rbac.UserKind, Name: "*"},
 			},
-			ctx: api.WithNamespace(
-				api.WithUser(api.NewContext(), &user.DefaultInfo{Name: "foobar"}),
-				"default",
-			),
+			user:      &user.DefaultInfo{Name: "foobar"},
+			namespace: "default",
 			appliesTo: true,
 			testCase:  "multiple subjects with a service account that matches",
 		},
 	}
 
 	for _, tc := range tests {
-		got, err := appliesTo(tc.ctx, tc.subjects)
-		if err != nil {
-			t.Errorf("case %q %v", tc.testCase, err)
-			continue
-		}
+		got := appliesTo(tc.user, tc.subjects, tc.namespace)
 		if got != tc.appliesTo {
 			t.Errorf("case %q want appliesTo=%t, got appliesTo=%t", tc.testCase, tc.appliesTo, got)
 		}

plugin/pkg/auth/authorizer/rbac/rbac.go

@@ -18,56 +18,41 @@ limitations under the License.
 package rbac
 
 import (
-	"k8s.io/kubernetes/pkg/api"
 	"k8s.io/kubernetes/pkg/apis/rbac"
 	"k8s.io/kubernetes/pkg/apis/rbac/validation"
 	"k8s.io/kubernetes/pkg/auth/authorizer"
+	"k8s.io/kubernetes/pkg/auth/user"
 	"k8s.io/kubernetes/pkg/registry/clusterrole"
 	"k8s.io/kubernetes/pkg/registry/clusterrolebinding"
 	"k8s.io/kubernetes/pkg/registry/role"
 	"k8s.io/kubernetes/pkg/registry/rolebinding"
 )
 
+type RequestToRuleMapper interface {
+	// RulesFor returns all known PolicyRules and any errors that happened while locating those rules.
+	// Any rule returned is still valid, since rules are deny by default.  If you can pass with the rules
+	// supplied, you do not have to fail the request.  If you cannot, you should indicate the error along
+	// with your denial.
+	RulesFor(subject user.Info, namespace string) ([]rbac.PolicyRule, error)
+}
+
 type RBACAuthorizer struct {
 	superUser string
 
-	authorizationRuleResolver validation.AuthorizationRuleResolver
+	authorizationRuleResolver RequestToRuleMapper
 }
 
-func (r *RBACAuthorizer) Authorize(attr authorizer.Attributes) (bool, string, error) {
-	if r.superUser != "" && attr.GetUser() != nil && attr.GetUser().GetName() == r.superUser {
+func (r *RBACAuthorizer) Authorize(requestAttributes authorizer.Attributes) (bool, string, error) {
+	if r.superUser != "" && requestAttributes.GetUser() != nil && requestAttributes.GetUser().GetName() == r.superUser {
 		return true, "", nil
 	}
 
-	ctx := api.WithNamespace(api.WithUser(api.NewContext(), attr.GetUser()), attr.GetNamespace())
-
-	// Frame the authorization request as a privilege escalation check.
-	var requestedRule rbac.PolicyRule
-	if attr.IsResourceRequest() {
-		resource := attr.GetResource()
-		if len(attr.GetSubresource()) > 0 {
-			resource = attr.GetResource() + "/" + attr.GetSubresource()
-		}
-		requestedRule = rbac.PolicyRule{
-			Verbs:         []string{attr.GetVerb()},
-			APIGroups:     []string{attr.GetAPIGroup()}, // TODO(ericchiang): add api version here too?
-			Resources:     []string{resource},
-			ResourceNames: []string{attr.GetName()},
-		}
-	} else {
-		requestedRule = rbac.PolicyRule{
-			Verbs:           []string{attr.GetVerb()},
-			NonResourceURLs: []string{attr.GetPath()},
-		}
+	rules, ruleResolutionError := r.authorizationRuleResolver.RulesFor(requestAttributes.GetUser(), requestAttributes.GetNamespace())
+	if RulesAllow(requestAttributes, rules...) {
+		return true, "", nil
 	}
 
-	// TODO(nhlfr): Try to find more lightweight way to check attributes than escalation checks.
-	err := validation.ConfirmNoEscalation(ctx, r.authorizationRuleResolver, []rbac.PolicyRule{requestedRule})
-	if err != nil {
-		return false, err.Error(), nil
-	}
-
-	return true, "", nil
+	return false, "", ruleResolutionError
 }
 
 func New(roleRegistry role.Registry, roleBindingRegistry rolebinding.Registry, clusterRoleRegistry clusterrole.Registry, clusterRoleBindingRegistry clusterrolebinding.Registry, superUser string) *RBACAuthorizer {
@@ -82,3 +67,30 @@ func New(roleRegistry role.Registry, roleBindingRegistry rolebinding.Registry, c
 	}
 	return authorizer
 }
+
+func RulesAllow(requestAttributes authorizer.Attributes, rules ...rbac.PolicyRule) bool {
+	for _, rule := range rules {
+		if RuleAllows(requestAttributes, rule) {
+			return true
+		}
+	}
+
+	return false
+}
+
+func RuleAllows(requestAttributes authorizer.Attributes, rule rbac.PolicyRule) bool {
+	if requestAttributes.IsResourceRequest() {
+		resource := requestAttributes.GetResource()
+		if len(requestAttributes.GetSubresource()) > 0 {
+			resource = requestAttributes.GetResource() + "/" + requestAttributes.GetSubresource()
+		}
+
+		return rbac.VerbMatches(rule, requestAttributes.GetVerb()) &&
+			rbac.APIGroupMatches(rule, requestAttributes.GetAPIGroup()) &&
+			rbac.ResourceMatches(rule, resource) &&
+			rbac.ResourceNameMatches(rule, requestAttributes.GetName())
+	}
+
+	return rbac.VerbMatches(rule, requestAttributes.GetVerb()) &&
+		rbac.NonResourceURLMatches(rule, requestAttributes.GetPath())
+}