Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add additional authorization check for create-on-update #65150

Merged
merged 1 commit into from Jul 3, 2018

Conversation

@jennybuckley
Copy link
Contributor

jennybuckley commented Jun 15, 2018

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:

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.
}
if authorized, _, _ := scope.Authorizer.Authorize(createAttributes); authorized != authorizer.DecisionAllow {
// Instead of surfacing the error, just treat it as if the resource doesn't allow
// create on update, this way the Update loop will continue retrying.

This comment has been minimized.

@liggitt

liggitt Jun 15, 2018

Member

this response seems odd... what assumptions does this make about how storage Update handles not found errors? how does this surface to the user?

This comment has been minimized.

@lavalamp

lavalamp Jun 15, 2018

Member

I agree with this comment. Why not just fail and return an error stating the authz problem? The only benefit of retrying is if there is actually a race and we expect another component to create before our next iteration. But that case should be super super rare?

This comment has been minimized.

@jennybuckley

jennybuckley Jun 15, 2018

Author Contributor

The transformers on the objectInfo get evaluated before the update function checks the AllowCreateOnUpdate, so doing that would cause StatusForbidden to be returned to clients unauthorized to create making an update request to a nonexistent resource which doesn't allow create-on-update.

This comment has been minimized.

@jennybuckley

jennybuckley Jun 15, 2018

Author Contributor

We could have this unauthorized case return some special error that the Update function then checks it would return here https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store.go#L527 if create-on-update is disabled

This comment has been minimized.

@liggitt

liggitt Jun 15, 2018

Member

The transformers on the objectInfo get evaluated before the update function checks the AllowCreateOnUpdate

What if we hooked the create authz check into the createValidation passed down instead? That gets called after the storage has decided it wants to persist the input, past the point where it would have returned a NotFound error if it didn't AllowCreateOnUpdate.

if hasUID, err := hasUID(oldObj); err != nil {
return nil, err
} else if !hasUID && scope.Authorizer != nil {
createAttributes := authorizer.AttributesRecord{

This comment has been minimized.

@liggitt

liggitt Jun 15, 2018

Member

we should only need to run this check once per request... hoist out into a do.Once?

This comment has been minimized.

@lavalamp

lavalamp Jun 15, 2018

Member

It shouldn't be harmful to call multiple times, other than it takes time. But, I think if we want to cache decisions, we should do that in whatever is implementing the Authorize method, rather than ad-hoc by only calling once per request. That will make it easier to provide guarantees on how long things are cached and what the propagation delay therefore might be for a rule change.

This comment has been minimized.

@liggitt

liggitt Jun 15, 2018

Member

It shouldn't be harmful to call multiple times, other than it takes time. But, I think if we want to cache decisions, we should do that in whatever is implementing the Authorize method, rather than ad-hoc by only calling once per request.

Thinking of authorization as a logical layer, once you're through the layer for a particular API operation, the same request doesn't go back through again. The only reason we're checking here is that you haven't yet made it through the layer for the create operation. Especially in cases like patch where the server retries application of the object as part of persisting a single request, I don't think we want to recall this many times.

APIGroup: scope.Resource.Group,
APIVersion: scope.Resource.Version,
Resource: scope.Resource.Resource,
Subresource: scope.Subresource,

This comment has been minimized.

@liggitt

liggitt Jun 15, 2018

Member

need to think through whether we need to handle subresources differently here... we don't create-on-update on subresources today, do we? should an update of a missing subresource escalate to a create and return a forbidden error, or return a not found error?

This comment has been minimized.

@lavalamp

lavalamp Jun 15, 2018

Member

I think we should start off taking the position that no subresource allows create-on-update. We can always relax later when there is a concrete need (I don't know of one).

This comment has been minimized.

@liggitt

liggitt Jun 15, 2018

Member

I think we should start off taking the position that no subresource allows create-on-update. We can always relax later when there is a concrete need (I don't know of one).

I'd tend to agree.

@lavalamp
Copy link
Member

lavalamp left a comment

Looks very close, nice tests

@@ -81,6 +82,9 @@ type APIGroupVersion struct {

// OpenAPIConfig lets the individual handlers build a subset of the OpenAPI schema before they are installed.
OpenAPIConfig *openapicommon.Config

// Authorizer determines whether a user is allowed to make a certain request.
Authorizer authorizer.Authorizer

This comment has been minimized.

@lavalamp

lavalamp Jun 15, 2018

Member

Perhaps place right above Admit? And add to the description--folks will expect authz has already been called, so under what circumstances will this be called again?

@@ -54,6 +55,7 @@ type RequestScope struct {
Defaulter runtime.ObjectDefaulter
Typer runtime.ObjectTyper
UnsafeConvertor runtime.ObjectConvertor
Authorizer authorizer.Authorizer

This comment has been minimized.

@lavalamp

lavalamp Jun 15, 2018

Member

Surprised to see this in both group info and here. Maybe it will make sense after I read more.

This comment has been minimized.

@lavalamp

lavalamp Jun 15, 2018

Member

I guess it is copied here from the group info struct for each request? It seems a little strange to me to copy this into the APIGroupVersion struct at all--it should be constant for the whole apiserver, not vary by endpoint, right?

This comment has been minimized.

@jennybuckley

jennybuckley Jun 15, 2018

Author Contributor

Yeah, It's the same for all the endpoints, but I thought this was the only way to expose it to the installer and then to the individual handlers

if hasUID, err := hasUID(oldObj); err != nil {
return nil, err
} else if !hasUID && scope.Authorizer != nil {
createAttributes := authorizer.AttributesRecord{

This comment has been minimized.

@lavalamp

lavalamp Jun 15, 2018

Member

It shouldn't be harmful to call multiple times, other than it takes time. But, I think if we want to cache decisions, we should do that in whatever is implementing the Authorize method, rather than ad-hoc by only calling once per request. That will make it easier to provide guarantees on how long things are cached and what the propagation delay therefore might be for a rule change.

APIGroup: scope.Resource.Group,
APIVersion: scope.Resource.Version,
Resource: scope.Resource.Resource,
Subresource: scope.Subresource,

This comment has been minimized.

@lavalamp

lavalamp Jun 15, 2018

Member

I think we should start off taking the position that no subresource allows create-on-update. We can always relax later when there is a concrete need (I don't know of one).

}
if authorized, _, _ := scope.Authorizer.Authorize(createAttributes); authorized != authorizer.DecisionAllow {
// Instead of surfacing the error, just treat it as if the resource doesn't allow
// create on update, this way the Update loop will continue retrying.

This comment has been minimized.

@lavalamp

lavalamp Jun 15, 2018

Member

I agree with this comment. Why not just fail and return an error stating the authz problem? The only benefit of retrying is if there is actually a race and we expect another component to create before our next iteration. But that case should be super super rare?

@@ -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

This comment has been minimized.

@lavalamp

lavalamp Jun 15, 2018

Member

We are changing the permissions necessary for create-on-update; we need a big release note for this. I think it is a good change, it means patch/put never implies create--formerly sometimes it did in a hard to reason about way. But administrators will need to know. Can you add to the feature branch change log file I started so we don't forget?

This comment has been minimized.

@jennybuckley

jennybuckley Jun 15, 2018

Author Contributor

This PR is to master, since it is just affecting create on update, which is broken in master. Should I remake this to the feature branch?

transformers = append(transformers, func(ctx context.Context, newObj, oldObj runtime.Object) (runtime.Object, error) {
if hasUID, err := hasUID(oldObj); err != nil {
return nil, err
} else if !hasUID && scope.Authorizer != nil {

This comment has been minimized.

@liggitt

liggitt Jun 15, 2018

Member

skipping this check if no authorizer is provided seems like an opportunity for misconstruction to leave a security hole. can we require scope.Authorizer to be set? as far as I'm aware, every handler (except probably unit tests :-/) has at least an "AlwaysAllow" authorizer implementation set

This comment has been minimized.

@jennybuckley

jennybuckley Jun 15, 2018

Author Contributor

When the apiserver is coming up, it is allowed for the authorizer to be unset, it just emits a warning saying authorization is disabled. I think if we allow that at startup then we should allow for this, maybe we could emit a warning when the handler is registered too, if the authorizer is unset?

This comment has been minimized.

@liggitt

liggitt Jun 15, 2018

Member

When the apiserver is coming up, it is allowed for the authorizer to be unset, it just emits a warning saying authorization is disabled. I think if we allow that at startup then we should allow for this

Hmm, it still seems safer for the top level server creator to plug in an AlwaysAllow impl in that case. Skipping on nil here means we don't know if we were wired incorrectly or if the server creator really intended to run without authorization.

@lavalamp

This comment has been minimized.

Copy link
Member

lavalamp commented Jun 21, 2018

/assign @cheftako

@jennybuckley

This comment has been minimized.

Copy link
Contributor Author

jennybuckley commented Jun 27, 2018

This is failing because the expected behavior in the test is wrong now, because I changed the error code. Fixing now.

@jennybuckley jennybuckley force-pushed the jennybuckley:create-on-update-authorizer branch from b289c26 to 488bba3 Jun 27, 2018

@jennybuckley

This comment has been minimized.

Copy link
Contributor Author

jennybuckley commented Jun 29, 2018

@liggitt
This is now updated to address all your comments.

if authorizerErr != nil {
return errors.NewInternalError(authorizerErr)
}
return errors.NewUnauthorized(authorizerReason)

This comment has been minimized.

@liggitt

liggitt Jul 3, 2018

Member

should be errors.NewForbidden (thank the http spec for the insane difference)

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Jul 3, 2018

update the release note to call out the specific resources this changes behavior for (I think just limitrange and endpoints objects). e.g.:

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.

// Create the namespace used later in the test
{superUser, "POST", "", "namespaces", "", "", limitRangeNamespace, http.StatusCreated},

{"limitrange-updater", "PUT", "", "limitranges", "limitrange-namespace", "a", aLimitRange, http.StatusUnauthorized},

This comment has been minimized.

@liggitt

liggitt Jul 3, 2018

Member

should be http.StatusForbidden

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Jul 3, 2018

nits on http status type and release note, lgtm otherwise

@jennybuckley jennybuckley force-pushed the jennybuckley:create-on-update-authorizer branch from 488bba3 to cc5c17e Jul 3, 2018

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Jul 3, 2018

/lgtm

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jul 3, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jennybuckley, liggitt

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Jul 3, 2018

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 here.

@k8s-github-robot k8s-github-robot merged commit 0e6d3f2 into kubernetes:master Jul 3, 2018

17 checks passed

Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation jennybuckley authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.