Merge pull request #13259 from deads2k/auth-05-interface

Merged by openshift-bot

Author: OpenShift Bot

pkg/authorization/api/synthetic.go

@@ -7,10 +7,10 @@ const (
 	CustomBuildResource          = "builds/custom"
 	JenkinsPipelineBuildResource = "builds/jenkinspipeline"
 
-	NodeMetricsResource = "nodes/metrics"
-	NodeStatsResource   = "nodes/stats"
-	NodeSpecResource    = "nodes/spec"
-	NodeLogResource     = "nodes/log"
+	NodeMetricsSubresource = "metrics"
+	NodeStatsSubresource   = "stats"
+	NodeSpecSubresource    = "spec"
+	NodeLogSubresource     = "log"
 
 	RestrictedEndpointsResource = "endpoints/restricted"
 )

pkg/authorization/authorizer/adapter/attributes.go

@@ -1,8 +1,6 @@
 package adapter
 
 import (
-	"strings"
-
 	kapi "k8s.io/kubernetes/pkg/api"
 	kauthorizer "k8s.io/kubernetes/pkg/auth/authorizer"
 	"k8s.io/kubernetes/pkg/auth/user"
@@ -38,16 +36,11 @@ func OriginAuthorizerAttributes(kattrs kauthorizer.Attributes) (kapi.Context, oa
 		APIGroup:     kattrs.GetAPIGroup(),
 		APIVersion:   kattrs.GetAPIVersion(),
 		Resource:     kattrs.GetResource(),
+		Subresource:  kattrs.GetSubresource(),
 		ResourceName: kattrs.GetName(),
 
 		NonResourceURL: kattrs.IsResourceRequest() == false,
 		URL:            kattrs.GetPath(),
-
-		// TODO: add to kube authorizer attributes
-		// RequestAttributes interface{}
-	}
-	if len(kattrs.GetSubresource()) > 0 {
-		oattrs.Resource = kattrs.GetResource() + "/" + kattrs.GetSubresource()
 	}
 
 	return ctx, oattrs
@@ -86,19 +79,11 @@ func (a AdapterAttributes) GetName() string {
 }
 
 func (a AdapterAttributes) GetSubresource() string {
-	tokens := strings.SplitN(a.authorizationAttributes.GetResource(), "/", 2)
-	if len(tokens) != 2 {
-		return ""
-	}
-	return tokens[1]
+	return a.authorizationAttributes.GetSubresource()
 }
 
 func (a AdapterAttributes) GetResource() string {
-	tokens := strings.SplitN(a.authorizationAttributes.GetResource(), "/", 2)
-	if len(tokens) < 1 {
-		return ""
-	}
-	return tokens[0]
+	return a.authorizationAttributes.GetResource()
 }
 
 // GetUserName satisfies the kubernetes authorizer.Attributes interface

pkg/authorization/authorizer/adapter/attributes_test.go

@@ -22,6 +22,7 @@ func TestRoundTrip(t *testing.T) {
 		APIVersion:     "av",
 		APIGroup:       "ag",
 		Resource:       "r",
+		Subresource:    "sub",
 		ResourceName:   "rn",
 		NonResourceURL: true,
 		URL:            "/123",
@@ -73,6 +74,9 @@ func TestRoundTrip(t *testing.T) {
 	if oattrs.GetResource() != oattrs2.GetResource() {
 		t.Errorf("Expected %v, got %v", oattrs.GetResource(), oattrs2.GetResource())
 	}
+	if oattrs.GetSubresource() != oattrs2.GetSubresource() {
+		t.Errorf("Expected %v, got %v", oattrs.GetSubresource(), oattrs2.GetSubresource())
+	}
 
 	// Ensure origin-specific info is preserved
 	if oattrs.GetAPIVersion() != oattrs2.GetAPIVersion() {
@@ -95,7 +99,7 @@ func TestRoundTrip(t *testing.T) {
 func TestAttributeIntersection(t *testing.T) {
 	// These are the things we expect to be shared
 	// Everything in this list should be used by OriginAuthorizerAttributes
-	expectedIntersection := sets.NewString("GetVerb", "GetResource", "GetAPIGroup", "GetAPIVersion")
+	expectedIntersection := sets.NewString("GetVerb", "GetResource", "GetSubresource", "GetAPIGroup", "GetAPIVersion")
 
 	// These are the things we expect to only be in the Kubernetes interface
 	// Everything in this list should be used by OriginAuthorizerAttributes or derivative (like IsReadOnly)

pkg/authorization/authorizer/attributes.go

@@ -14,29 +14,42 @@ type DefaultAuthorizationAttributes struct {
 	APIVersion     string
 	APIGroup       string
 	Resource       string
+	Subresource    string
 	ResourceName   string
 	NonResourceURL bool
 	URL            string
 }
 
 // ToDefaultAuthorizationAttributes coerces Action to DefaultAuthorizationAttributes.  Namespace is not included
 // because the authorizer takes that information on the context
-func ToDefaultAuthorizationAttributes(in authorizationapi.Action) DefaultAuthorizationAttributes {
+func ToDefaultAuthorizationAttributes(in authorizationapi.Action) Action {
+	tokens := strings.SplitN(in.Resource, "/", 2)
+	resource := ""
+	subresource := ""
+	switch {
+	case len(tokens) == 2:
+		subresource = tokens[1]
+		fallthrough
+	case len(tokens) == 1:
+		resource = tokens[0]
+	}
+
 	return DefaultAuthorizationAttributes{
 		Verb:           in.Verb,
 		APIGroup:       in.Group,
 		APIVersion:     in.Version,
-		Resource:       in.Resource,
+		Resource:       resource,
+		Subresource:    subresource,
 		ResourceName:   in.ResourceName,
 		URL:            in.Path,
 		NonResourceURL: in.IsNonResourceURL,
 	}
 }
 
-func (a DefaultAuthorizationAttributes) RuleMatches(rule authorizationapi.PolicyRule) (bool, error) {
+func RuleMatches(a Action, rule authorizationapi.PolicyRule) (bool, error) {
 	if a.IsNonResourceURL() {
-		if a.nonResourceMatches(rule) {
-			if a.verbMatches(rule.Verbs) {
+		if nonResourceMatches(a, rule) {
+			if verbMatches(a, rule.Verbs) {
 				return true, nil
 			}
 		}
@@ -50,12 +63,12 @@ func (a DefaultAuthorizationAttributes) RuleMatches(rule authorizationapi.Policy
 		return false, nil
 	}
 
-	if a.verbMatches(rule.Verbs) {
-		if a.apiGroupMatches(rule.APIGroups) {
+	if verbMatches(a, rule.Verbs) {
+		if apiGroupMatches(a, rule.APIGroups) {
 
 			allowedResourceTypes := authorizationapi.NormalizeResources(rule.Resources)
-			if a.resourceMatches(allowedResourceTypes) {
-				if a.nameMatches(rule.ResourceNames) {
+			if resourceMatches(a, allowedResourceTypes) {
+				if nameMatches(a, rule.ResourceNames) {
 					return true, nil
 				}
 			}
@@ -65,7 +78,7 @@ func (a DefaultAuthorizationAttributes) RuleMatches(rule authorizationapi.Policy
 	return false, nil
 }
 
-func (a DefaultAuthorizationAttributes) apiGroupMatches(allowedGroups []string) bool {
+func apiGroupMatches(a Action, allowedGroups []string) bool {
 	// allowedGroups is expected to be small, so I don't feel bad about this.
 	for _, allowedGroup := range allowedGroups {
 		if allowedGroup == authorizationapi.APIGroupAll {
@@ -80,32 +93,36 @@ func (a DefaultAuthorizationAttributes) apiGroupMatches(allowedGroups []string)
 	return false
 }
 
-func (a DefaultAuthorizationAttributes) verbMatches(verbs sets.String) bool {
+func verbMatches(a Action, verbs sets.String) bool {
 	return verbs.Has(authorizationapi.VerbAll) || verbs.Has(strings.ToLower(a.GetVerb()))
 }
 
-func (a DefaultAuthorizationAttributes) resourceMatches(allowedResourceTypes sets.String) bool {
-	return allowedResourceTypes.Has(authorizationapi.ResourceAll) || allowedResourceTypes.Has(strings.ToLower(a.GetResource()))
+func resourceMatches(a Action, allowedResourceTypes sets.String) bool {
+	if allowedResourceTypes.Has(authorizationapi.ResourceAll) {
+		return true
+	}
+
+	if len(a.GetSubresource()) == 0 {
+		return allowedResourceTypes.Has(strings.ToLower(a.GetResource()))
+	}
+
+	return allowedResourceTypes.Has(strings.ToLower(a.GetResource() + "/" + a.GetSubresource()))
 }
 
 // nameMatches checks to see if the resourceName of the action is in a the specified whitelist.  An empty whitelist indicates that any name is allowed.
 // An empty string in the whitelist should only match the action's resourceName if the resourceName itself is empty string.  This behavior allows for the
 // combination of a whitelist for gets in the same rule as a list that won't have a resourceName.  I don't recommend writing such a rule, but we do
 // handle it like you'd expect: white list is respected for gets while not preventing the list you explicitly asked for.
-func (a DefaultAuthorizationAttributes) nameMatches(allowedResourceNames sets.String) bool {
+func nameMatches(a Action, allowedResourceNames sets.String) bool {
 	if len(allowedResourceNames) == 0 {
 		return true
 	}
 
 	return allowedResourceNames.Has(a.GetResourceName())
 }
 
-func (a DefaultAuthorizationAttributes) GetVerb() string {
-	return a.Verb
-}
-
 // nonResourceMatches take the remainer of a URL and attempts to match it against a series of explicitly allowed steps that can end in a wildcard
-func (a DefaultAuthorizationAttributes) nonResourceMatches(rule authorizationapi.PolicyRule) bool {
+func nonResourceMatches(a Action, rule authorizationapi.PolicyRule) bool {
 	for allowedNonResourcePath := range rule.NonResourceURLs {
 		// if the allowed resource path ends in a wildcard, check to see if the URL starts with it
 		if strings.HasSuffix(allowedNonResourcePath, "*") {
@@ -135,6 +152,10 @@ func splitPath(thePath string) []string {
 // DefaultAuthorizationAttributes satisfies the Action interface
 var _ Action = DefaultAuthorizationAttributes{}
 
+func (a DefaultAuthorizationAttributes) GetVerb() string {
+	return a.Verb
+}
+
 func (a DefaultAuthorizationAttributes) GetAPIVersion() string {
 	return a.APIVersion
 }
@@ -147,6 +168,10 @@ func (a DefaultAuthorizationAttributes) GetResource() string {
 	return a.Resource
 }
 
+func (a DefaultAuthorizationAttributes) GetSubresource() string {
+	return a.Subresource
+}
+
 func (a DefaultAuthorizationAttributes) GetResourceName() string {
 	return a.ResourceName
 }

pkg/authorization/authorizer/attributes_builder.go

@@ -30,16 +30,12 @@ func (a *openshiftAuthorizationAttributeBuilder) GetAttributes(req *http.Request
 		}, nil
 	}
 
-	resource := requestInfo.Resource
-	if len(requestInfo.Subresource) > 0 {
-		resource = requestInfo.Resource + "/" + requestInfo.Subresource
-	}
-
 	return DefaultAuthorizationAttributes{
 		Verb:           requestInfo.Verb,
 		APIGroup:       requestInfo.APIGroup,
 		APIVersion:     requestInfo.APIVersion,
-		Resource:       resource,
+		Resource:       requestInfo.Resource,
+		Subresource:    requestInfo.Subresource,
 		ResourceName:   requestInfo.Name,
 		NonResourceURL: false,
 		URL:            requestInfo.Path,

pkg/authorization/authorizer/attributes_test.go

@@ -1,6 +1,7 @@
 package authorizer
 
 import (
+	"strings"
 	"testing"
 
 	authorizationapi "github.com/openshift/origin/pkg/authorization/api"
@@ -29,7 +30,21 @@ func (a authorizationAttributesAdapter) GetAPIGroup() string {
 }
 
 func (a authorizationAttributesAdapter) GetResource() string {
-	return a.attrs.Resource
+	// to match the RequestInfoFactory assuming an in.resource of one/two/three, one==resource, two==subresource, three=nothing
+	tokens := strings.SplitN(a.attrs.Resource, "/", 2)
+	if len(tokens) >= 1 {
+		return tokens[0]
+	}
+	return ""
+}
+
+func (a authorizationAttributesAdapter) GetSubresource() string {
+	// to match the RequestInfoFactory assuming an in.resource of one/two/three, one==resource, two==subresource, three=nothing
+	tokens := strings.SplitN(a.attrs.Resource, "/", 2)
+	if len(tokens) >= 2 {
+		return tokens[1]
+	}
+	return ""
 }
 
 func (a authorizationAttributesAdapter) GetResourceName() string {

pkg/authorization/authorizer/authorizer.go

@@ -20,9 +20,7 @@ func NewAuthorizer(ruleResolver rulevalidation.AuthorizationRuleResolver, forbid
 	return &openshiftAuthorizer{ruleResolver, forbiddenMessageMaker}
 }
 
-func (a *openshiftAuthorizer) Authorize(ctx kapi.Context, passedAttributes Action) (bool, string, error) {
-	attributes := CoerceToDefaultAuthorizationAttributes(passedAttributes)
-
+func (a *openshiftAuthorizer) Authorize(ctx kapi.Context, attributes Action) (bool, string, error) {
 	user, ok := kapi.UserFrom(ctx)
 	if !ok {
 		return false, "", errors.New("no user available on context")
@@ -54,9 +52,7 @@ func (a *openshiftAuthorizer) GetAllowedSubjects(ctx kapi.Context, attributes Ac
 	return a.getAllowedSubjectsFromNamespaceBindings(namespace, attributes)
 }
 
-func (a *openshiftAuthorizer) getAllowedSubjectsFromNamespaceBindings(namespace string, passedAttributes Action) (sets.String, sets.String, error) {
-	attributes := CoerceToDefaultAuthorizationAttributes(passedAttributes)
-
+func (a *openshiftAuthorizer) getAllowedSubjectsFromNamespaceBindings(namespace string, attributes Action) (sets.String, sets.String, error) {
 	var errs []error
 
 	roleBindings, err := a.ruleResolver.GetRoleBindings(namespace)
@@ -77,7 +73,7 @@ func (a *openshiftAuthorizer) getAllowedSubjectsFromNamespaceBindings(namespace
 		}
 
 		for _, rule := range role.Rules() {
-			matches, err := attributes.RuleMatches(rule)
+			matches, err := RuleMatches(attributes, rule)
 			if err != nil {
 				errs = append(errs, err)
 				continue
@@ -96,14 +92,12 @@ func (a *openshiftAuthorizer) getAllowedSubjectsFromNamespaceBindings(namespace
 // authorizeWithNamespaceRules returns isAllowed, reason, and error.  If an error is returned, isAllowed and reason are still valid.  This seems strange
 // but errors are not always fatal to the authorization process.  It is entirely possible to get an error and be able to continue determine authorization
 // status in spite of it.  This is most common when a bound role is missing, but enough roles are still present and bound to authorize the request.
-func (a *openshiftAuthorizer) authorizeWithNamespaceRules(user user.Info, namespace string, passedAttributes Action) (bool, string, error) {
-	attributes := CoerceToDefaultAuthorizationAttributes(passedAttributes)
-
+func (a *openshiftAuthorizer) authorizeWithNamespaceRules(user user.Info, namespace string, attributes Action) (bool, string, error) {
 	allRules, ruleRetrievalError := a.ruleResolver.RulesFor(user, namespace)
 
 	var errs []error
 	for _, rule := range allRules {
-		matches, err := attributes.RuleMatches(rule)
+		matches, err := RuleMatches(attributes, rule)
 		if err != nil {
 			errs = append(errs, err)
 			continue
@@ -126,24 +120,6 @@ func (a *openshiftAuthorizer) authorizeWithNamespaceRules(user user.Info, namesp
 	return false, "", kerrors.NewAggregate(errs)
 }
 
-// TODO this may or may not be the behavior we want for managing rules.  As a for instance, a verb might be specified
-// that our attributes builder will never satisfy.  For now, I think gets us close.  Maybe a warning message of some kind?
-func CoerceToDefaultAuthorizationAttributes(passedAttributes Action) *DefaultAuthorizationAttributes {
-	attributes, ok := passedAttributes.(*DefaultAuthorizationAttributes)
-	if !ok {
-		attributes = &DefaultAuthorizationAttributes{
-			APIGroup:       passedAttributes.GetAPIGroup(),
-			Verb:           passedAttributes.GetVerb(),
-			Resource:       passedAttributes.GetResource(),
-			ResourceName:   passedAttributes.GetResourceName(),
-			NonResourceURL: passedAttributes.IsNonResourceURL(),
-			URL:            passedAttributes.GetURL(),
-		}
-	}
-
-	return attributes
-}
-
 func doesApplyToUser(ruleUsers, ruleGroups sets.String, user user.Info) bool {
 	if ruleUsers.Has(user.GetName()) {
 		return true

pkg/authorization/authorizer/bootstrap_policy_test.go

@@ -378,8 +378,9 @@ func TestAdminGetStatusInMallet(t *testing.T) {
 	test := &authorizeTest{
 		context: kapi.WithUser(kapi.WithNamespace(kapi.NewContext(), "mallet"), &user.DefaultInfo{Name: "Matthew"}),
 		attributes: &DefaultAuthorizationAttributes{
-			Verb:     "get",
-			Resource: "pods/status",
+			Verb:        "get",
+			Resource:    "pods",
+			Subresource: "status",
 		},
 		expectedAllowed: true,
 		expectedReason:  "allowed by rule in mallet",

pkg/authorization/authorizer/cache/authorizer.go

@@ -128,6 +128,7 @@ func cacheKey(ctx kapi.Context, a authorizer.Action) (string, error) {
 		"apiVersion":     a.GetAPIVersion(),
 		"apiGroup":       a.GetAPIGroup(),
 		"resource":       a.GetResource(),
+		"subresource":    a.GetSubresource(),
 		"resourceName":   a.GetResourceName(),
 		"nonResourceURL": a.IsNonResourceURL(),
 		"url":            a.GetURL(),

pkg/authorization/authorizer/cache/authorizer_test.go

@@ -29,7 +29,7 @@ func TestCacheKey(t *testing.T) {
 		"empty": {
 			Context:     kapi.NewContext(),
 			Attrs:       &authorizer.DefaultAuthorizationAttributes{},
-			ExpectedKey: `{"apiGroup":"","apiVersion":"","nonResourceURL":false,"resource":"","resourceName":"","url":"","verb":""}`,
+			ExpectedKey: `{"apiGroup":"","apiVersion":"","nonResourceURL":false,"resource":"","resourceName":"","subresource":"","url":"","verb":""}`,
 		},
 		"full": {
 			Context: kapi.WithUser(kapi.WithNamespace(kapi.NewContext(), "myns"), &user.DefaultInfo{Name: "me", Groups: []string{"group1", "group2"}}),
@@ -38,11 +38,12 @@ func TestCacheKey(t *testing.T) {
 				APIVersion:     "av",
 				APIGroup:       "ag",
 				Resource:       "r",
+				Subresource:    "sub",
 				ResourceName:   "rn",
 				NonResourceURL: true,
 				URL:            "/abc",
 			},
-			ExpectedKey: `{"apiGroup":"ag","apiVersion":"av","groups":["group1","group2"],"namespace":"myns","nonResourceURL":true,"resource":"r","resourceName":"rn","scopes":null,"url":"/abc","user":"me","verb":"v"}`,
+			ExpectedKey: `{"apiGroup":"ag","apiVersion":"av","groups":["group1","group2"],"namespace":"myns","nonResourceURL":true,"resource":"r","resourceName":"rn","scopes":null,"subresource":"sub","url":"/abc","user":"me","verb":"v"}`,
 		},
 	}