Merge pull request #65150 from jennybuckley/create-on-update-authorizer

Automatic merge from submit-queue (batch tested with PRs 65677, 65711, 65150, 65726). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

 Add additional authorization check for create-on-update

**What this PR does / why we need it**:
Currently it is possible for a user who is only authorized to update objects to send a PUT request for an object that doesn't currently exist, and if that resource allows create on update, it will all them to create the object. This PR fixes that bug and adds a test case which fails on master, but succeeds when the additional authorization check is done.

/sig api-machinery
/kind bug
/cc @liggitt @lavalamp 

**Release note**:
```release-note
LimitRange and Endpoints resources can be created via an update API call if the object does not already exist. When this occurs, an authorization check is now made to ensure the user making the API call is authorized to create the object. In previous releases, only an update authorization check was performed.
```

staging/src/k8s.io/apiserver/pkg/endpoints/BUILD

@@ -74,6 +74,7 @@ go_library(
         "//staging/src/k8s.io/apimachinery/pkg/util/errors:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/util/sets:go_default_library",
         "//staging/src/k8s.io/apiserver/pkg/admission:go_default_library",
+        "//staging/src/k8s.io/apiserver/pkg/authorization/authorizer:go_default_library",
         "//staging/src/k8s.io/apiserver/pkg/endpoints/discovery:go_default_library",
         "//staging/src/k8s.io/apiserver/pkg/endpoints/handlers:go_default_library",
         "//staging/src/k8s.io/apiserver/pkg/endpoints/handlers/negotiation:go_default_library",

staging/src/k8s.io/apiserver/pkg/endpoints/groupversion.go

@@ -28,6 +28,7 @@ import (
 	utilerrors "k8s.io/apimachinery/pkg/util/errors"
 	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/apiserver/pkg/admission"
+	"k8s.io/apiserver/pkg/authorization/authorizer"
 	"k8s.io/apiserver/pkg/endpoints/discovery"
 	"k8s.io/apiserver/pkg/registry/rest"
 	openapicommon "k8s.io/kube-openapi/pkg/common"
@@ -71,6 +72,11 @@ type APIGroupVersion struct {
 	Linker          runtime.SelfLinker
 	UnsafeConvertor runtime.ObjectConvertor
 
+	// Authorizer determines whether a user is allowed to make a certain request. The Handler does a preliminary
+	// authorization check using the request URI but it may be necessary to make additional checks, such as in
+	// the create-on-update case
+	Authorizer authorizer.Authorizer
+
 	Admit admission.Interface
 
 	MinRequestTimeout time.Duration

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/BUILD

@@ -71,6 +71,7 @@ go_library(
         "//staging/src/k8s.io/apimachinery/pkg/watch:go_default_library",
         "//staging/src/k8s.io/apiserver/pkg/admission:go_default_library",
         "//staging/src/k8s.io/apiserver/pkg/audit:go_default_library",
+        "//staging/src/k8s.io/apiserver/pkg/authorization/authorizer:go_default_library",
         "//staging/src/k8s.io/apiserver/pkg/endpoints/handlers/negotiation:go_default_library",
         "//staging/src/k8s.io/apiserver/pkg/endpoints/handlers/responsewriters:go_default_library",
         "//staging/src/k8s.io/apiserver/pkg/endpoints/metrics:go_default_library",

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/rest.go

@@ -35,6 +35,7 @@ import (
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
 	"k8s.io/apiserver/pkg/admission"
+	"k8s.io/apiserver/pkg/authorization/authorizer"
 	"k8s.io/apiserver/pkg/endpoints/handlers/responsewriters"
 	"k8s.io/apiserver/pkg/endpoints/metrics"
 	"k8s.io/apiserver/pkg/endpoints/request"
@@ -54,6 +55,7 @@ type RequestScope struct {
 	Defaulter       runtime.ObjectDefaulter
 	Typer           runtime.ObjectTyper
 	UnsafeConvertor runtime.ObjectConvertor
+	Authorizer      authorizer.Authorizer
 
 	TableConvertor rest.TableConvertor
 	OpenAPISchema  openapiproto.Schema

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/update.go

@@ -20,6 +20,7 @@ import (
 	"context"
 	"fmt"
 	"net/http"
+	"sync"
 	"time"
 
 	"k8s.io/apimachinery/pkg/api/errors"
@@ -27,6 +28,7 @@ import (
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apiserver/pkg/admission"
 	"k8s.io/apiserver/pkg/audit"
+	"k8s.io/apiserver/pkg/authorization/authorizer"
 	"k8s.io/apiserver/pkg/endpoints/handlers/negotiation"
 	"k8s.io/apiserver/pkg/endpoints/request"
 	"k8s.io/apiserver/pkg/registry/rest"
@@ -102,6 +104,19 @@ func UpdateResource(r rest.Updater, scope RequestScope, admit admission.Interfac
 			})
 		}
 
+		createAuthorizerAttributes := authorizer.AttributesRecord{
+			User:            userInfo,
+			ResourceRequest: true,
+			Path:            req.URL.Path,
+			Verb:            "create",
+			APIGroup:        scope.Resource.Group,
+			APIVersion:      scope.Resource.Version,
+			Resource:        scope.Resource.Resource,
+			Subresource:     scope.Subresource,
+			Namespace:       namespace,
+			Name:            name,
+		}
+
 		trace.Step("About to store object in database")
 		wasCreated := false
 		result, err := finishRequest(timeout, func() (runtime.Object, error) {
@@ -109,7 +124,7 @@ func UpdateResource(r rest.Updater, scope RequestScope, admit admission.Interfac
 				ctx,
 				name,
 				rest.DefaultUpdatedObjectInfo(obj, transformers...),
-				rest.AdmissionToValidateObjectFunc(admit, staticAdmissionAttributes),
+				withAuthorization(rest.AdmissionToValidateObjectFunc(admit, staticAdmissionAttributes), scope.Authorizer, createAuthorizerAttributes),
 				rest.AdmissionToValidateObjectUpdateFunc(admit, staticAdmissionAttributes),
 				false,
 			)
@@ -141,3 +156,35 @@ func UpdateResource(r rest.Updater, scope RequestScope, admit admission.Interfac
 		transformResponseObject(ctx, scope, req, w, status, result)
 	}
 }
+
+func withAuthorization(validate rest.ValidateObjectFunc, a authorizer.Authorizer, attributes authorizer.Attributes) rest.ValidateObjectFunc {
+	var once sync.Once
+	var authorizerDecision authorizer.Decision
+	var authorizerReason string
+	var authorizerErr error
+	return func(obj runtime.Object) error {
+		if a == nil {
+			return errors.NewInternalError(fmt.Errorf("no authorizer provided, unable to authorize a create on update"))
+		}
+		once.Do(func() {
+			authorizerDecision, authorizerReason, authorizerErr = a.Authorize(attributes)
+		})
+		// an authorizer like RBAC could encounter evaluation errors and still allow the request, so authorizer decision is checked before error here.
+		if authorizerDecision == authorizer.DecisionAllow {
+			// Continue to validating admission
+			return validate(obj)
+		}
+		if authorizerErr != nil {
+			return errors.NewInternalError(authorizerErr)
+		}
+
+		// The user is not authorized to perform this action, so we need to build the error response
+		gr := schema.GroupResource{
+			Group:    attributes.GetAPIGroup(),
+			Resource: attributes.GetResource(),
+		}
+		name := attributes.GetName()
+		err := fmt.Errorf("%v", authorizerReason)
+		return errors.NewForbidden(gr, name, err)
+	}
+}

staging/src/k8s.io/apiserver/pkg/endpoints/installer.go

@@ -483,6 +483,7 @@ func (a *APIInstaller) registerResourceHandlers(path string, storage rest.Storag
 		Defaulter:       a.group.Defaulter,
 		Typer:           a.group.Typer,
 		UnsafeConvertor: a.group.UnsafeConvertor,
+		Authorizer:      a.group.Authorizer,
 
 		// TODO: Check for the interface on storage
 		TableConvertor: tableProvider,

staging/src/k8s.io/apiserver/pkg/server/config.go

@@ -464,6 +464,7 @@ func (c completedConfig) New(name string, delegationTarget DelegationTarget) (*G
 		admissionControl:       c.AdmissionControl,
 		Serializer:             c.Serializer,
 		AuditBackend:           c.AuditBackend,
+		Authorizer:             c.Authorization.Authorizer,
 		delegationTarget:       delegationTarget,
 		HandlerChainWaitGroup:  c.HandlerChainWaitGroup,
 

staging/src/k8s.io/apiserver/pkg/server/genericapiserver.go

@@ -36,6 +36,7 @@ import (
 	utilwaitgroup "k8s.io/apimachinery/pkg/util/waitgroup"
 	"k8s.io/apiserver/pkg/admission"
 	"k8s.io/apiserver/pkg/audit"
+	"k8s.io/apiserver/pkg/authorization/authorizer"
 	genericapi "k8s.io/apiserver/pkg/endpoints"
 	"k8s.io/apiserver/pkg/endpoints/discovery"
 	"k8s.io/apiserver/pkg/registry/rest"
@@ -138,6 +139,11 @@ type GenericAPIServer struct {
 	// auditing. The backend is started after the server starts listening.
 	AuditBackend audit.Backend
 
+	// Authorizer determines whether a user is allowed to make a certain request. The Handler does a preliminary
+	// authorization check using the request URI but it may be necessary to make additional checks, such as in
+	// the create-on-update case
+	Authorizer authorizer.Authorizer
+
 	// enableAPIResponseCompression indicates whether API Responses should support compression
 	// if the client requests it via Accept-Encoding
 	enableAPIResponseCompression bool
@@ -422,6 +428,7 @@ func (s *GenericAPIServer) newAPIGroupVersion(apiGroupInfo *APIGroupInfo, groupV
 		MinRequestTimeout:            s.minRequestTimeout,
 		EnableAPIResponseCompression: s.enableAPIResponseCompression,
 		OpenAPIConfig:                s.openAPIConfig,
+		Authorizer:                   s.Authorizer,
 	}
 }
 

test/integration/auth/rbac_test.go

@@ -219,6 +219,15 @@ var (
     }
   }
 }
+`
+	aLimitRange = `
+{
+  "apiVersion": "v1",
+  "kind": "LimitRange",
+  "metadata": {
+    "name": "a"%s
+  }
+}
 `
 	podNamespace = `
 {
@@ -246,6 +255,15 @@ var (
 	"name": "forbidden-namespace"%s
   }
 }
+`
+	limitRangeNamespace = `
+{
+  "apiVersion": "` + testapi.Groups[api.GroupName].GroupVersion().String() + `",
+  "kind": "Namespace",
+  "metadata": {
+	"name": "limitrange-namespace"%s
+  }
+}
 `
 )
 
@@ -409,6 +427,40 @@ func TestRBAC(t *testing.T) {
 				{superUser, "DELETE", "rbac.authorization.k8s.io", "rolebindings", "job-namespace", "pi", "", http.StatusOK},
 			},
 		},
+		{
+			bootstrapRoles: bootstrapRoles{
+				clusterRoles: []rbacapi.ClusterRole{
+					{
+						ObjectMeta: metav1.ObjectMeta{Name: "allow-all"},
+						Rules:      []rbacapi.PolicyRule{ruleAllowAll},
+					},
+					{
+						ObjectMeta: metav1.ObjectMeta{Name: "update-limitranges"},
+						Rules: []rbacapi.PolicyRule{
+							rbacapi.NewRule("update").Groups("").Resources("limitranges").RuleOrDie(),
+						},
+					},
+				},
+				clusterRoleBindings: []rbacapi.ClusterRoleBinding{
+					{
+						ObjectMeta: metav1.ObjectMeta{Name: "update-limitranges"},
+						Subjects: []rbacapi.Subject{
+							{Kind: "User", Name: "limitrange-updater"},
+						},
+						RoleRef: rbacapi.RoleRef{Kind: "ClusterRole", Name: "update-limitranges"},
+					},
+				},
+			},
+			requests: []request{
+				// Create the namespace used later in the test
+				{superUser, "POST", "", "namespaces", "", "", limitRangeNamespace, http.StatusCreated},
+
+				{"limitrange-updater", "PUT", "", "limitranges", "limitrange-namespace", "a", aLimitRange, http.StatusForbidden},
+				{superUser, "PUT", "", "limitranges", "limitrange-namespace", "a", aLimitRange, http.StatusCreated},
+				{superUser, "PUT", "", "limitranges", "limitrange-namespace", "a", aLimitRange, http.StatusOK},
+				{"limitrange-updater", "PUT", "", "limitranges", "limitrange-namespace", "a", aLimitRange, http.StatusOK},
+			},
+		},
 	}
 
 	for i, tc := range tests {
@@ -424,6 +476,7 @@ func TestRBAC(t *testing.T) {
 			"job-writer-namespace":             {Name: "job-writer-namespace"},
 			"nonescalating-rolebinding-writer": {Name: "nonescalating-rolebinding-writer"},
 			"pod-reader":                       {Name: "pod-reader"},
+			"limitrange-updater":               {Name: "limitrange-updater"},
 			"user-with-no-permissions":         {Name: "user-with-no-permissions"},
 		}))
 		_, s, closeFn := framework.RunAMaster(masterConfig)