Extend the gc admission plugin to check ownerReference.blockOwnerDeletion #43876

caesarxuchao · 2017-03-30T21:43:52Z

#Extend the gc admission plugin to prevent user who doesn't have delete permission of the owner from changing blockOwnerDeletion field of existing ownerReferences, or adding ownerReference with blockOwnerDeletion=true.

The plugin need a RESTMapper to translate ownerRef.Kind to Resource. It should be using a dynamic one. However, as discussed in #42615, such a RESTMapper will be built after watchable discovery API is implemented, so in this PR the plugin is using the api.Registry.RESTMapper(), which is also used by the garbage collector currently.

Extending the gc admission plugin so that a user who doesn't have delete permission of the *owner* cannot modify blockOwnerDeletion field of existing ownerReferences, or add new ownerReference with blockOwnerDeletion=true

cc @lavalamp

k8s-github-robot · 2017-03-30T21:44:03Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: caesarxuchao
We suggest the following additional approvers: @deads2k, @derekwaynecarr

Needs approval from an approver in each of these OWNERS Files:

~~cmd/kube-apiserver/OWNERS~~ [caesarxuchao]
federation/cmd/federation-apiserver/OWNERS
pkg/kubeapiserver/OWNERS
plugin/pkg/admission/OWNERS

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-reviewable · 2017-03-30T21:44:59Z

This change is

caesarxuchao · 2017-03-31T19:44:14Z

@k8s-bot verify test this

deads2k · 2017-03-31T20:35:10Z

@sttts since I'm out next week

caesarxuchao · 2017-04-04T18:23:58Z

@sttts could you take a look? Thanks.

0xmichalis · 2017-04-04T19:00:03Z

plugin/pkg/admission/gc/gc_admission.go

+		}
+		allowed, reason, err := a.authorizer.Authorize(attribute)
+		if !allowed {
+			return admission.NewForbidden(attributes, fmt.Errorf("cannot set blockOwnerDeletion in an ownerReference refers to a resource you can't delete: %v, %v", reason, err))


sttts · 2017-04-05T07:00:41Z

plugin/pkg/admission/gc/gc_admission.go

+
+// Returns new blocking ownerReferences, and references whose blockOwnerDeletion field is changed.
+// Changes between nil and false are ignored.
+func changingBlockOwnerDeletion(newObj, oldObj runtime.Object) []metav1.OwnerReference {


Misleading name, sounds like a predicate. better: changedDeletionBlockingOwnerRefs.

sttts · 2017-04-05T07:07:55Z

plugin/pkg/admission/gc/gc_admission.go

+
+	var changedRefs []metav1.OwnerReference
+	indexedOldRefs := indexByUID(oldMeta.GetOwnerReferences())
+	for _, newRef := range newRefs {


if an OwnerRef goes away in newObj, this will go unnoticed. Might be intended, but makes this unsymmetric. The func names all sound symmetric (changingBlockOwnerDeletion).

It's intentional. If a user has permission on the dependent object, she should have the right to remove the ownerRef so that the object doesn't share life cycle with the owner object.

Currently this admission plugin also disallows a user setting ref.blockOwnerDeletion from to true to false if she doesn't have delete permission of the owner object. Probably we should just allow that since the user can change the deletion behavior of the owner in the same way by removing the ref.

These two cases make the function asymmetric. I'll change the function name.

Updated the code to match what I wrote in the comment above.

sttts · 2017-04-05T07:09:35Z

plugin/pkg/admission/gc/gc_admission.go

+	return authorizer.AttributesRecord{
+		User: attributes.GetUserInfo(),
+		Verb: "delete",
+		// ownerReference can only refer to an object in the same namespace, so attributes.GetnNamespace() equals to the owner's namespace


GetnNamespace

sttts · 2017-04-05T07:14:31Z

plugin/pkg/admission/gc/gc_admission.go

+	for _, ref := range changedRefs {
+		attribute, err := a.toDeleteAttribute(ref, attributes)
+		if err != nil {
+			// An error occurs if a RESTMapping cannot be found for


Is this a security issue? If the RESTMapper is not up-to-date, we will allow ObjectReferences which would otherwise be forbidden. Until we have watchable discovery this might be fine as this only applies to TPRs. Can you add a comment that we switch to safe default "Forbidden" when we have watchable discovery ?

Good point. Further, if we are making the default "Forbidden" in the future, we should make it "Forbidden" now. Making validation rules tighter is backwards incompatible. Updated.

caesarxuchao · 2017-04-05T21:12:27Z

Comments addresed. PTAL. Thanks.

deads2k · 2017-04-11T19:05:03Z

plugin/pkg/admission/gc/gc_admission.go

+		return nil
+	}
+	newRefs := newMeta.GetOwnerReferences()
+	blockingNewRefs := blockingOwnerRefs(newRefs)


if this list is empty, return.

deads2k · 2017-04-11T19:06:33Z

plugin/pkg/admission/gc/gc_admission.go

+	}
+
+	var ret []metav1.OwnerReference
+	indexedOldRefs := indexByUID(oldMeta.GetOwnerReferences())


I would actually bet on it being just as fast to iterate the n-squared loop, but I don't really care.

deads2k · 2017-04-11T19:08:32Z

plugin/pkg/admission/gc/gc_admission.go

+	var ret []metav1.OwnerReference
+	indexedOldRefs := indexByUID(oldMeta.GetOwnerReferences())
+	for _, ref := range blockingNewRefs {
+		if oldRef, ok := indexedOldRefs[ref.UID]; ok {


optional nit: reduce nesting depth like

oldRef, ok := indexOldRefs[ref.UID] if !ok{ ret = append(... continue } wasNotBlocking := ... if wasNotBlocking{ ret = append(... continue }

deads2k · 2017-04-11T19:10:48Z

plugin/pkg/admission/gc/gc_admission.go

 func (a *gcPermissionsEnforcement) SetAuthorizer(authorizer authorizer.Authorizer) {
 	a.authorizer = authorizer
 }

+func (a *gcPermissionsEnforcement) SetRESTMapper(restMapper meta.RESTMapper) {
+	a.restMapper = restMapper


check that you have this in Validate

deads2k · 2017-04-11T19:11:43Z

plugin/pkg/admission/gc/gc_admission.go

@@ -99,10 +120,93 @@ func isChangingOwnerReference(newObj, oldObj runtime.Object) bool {
 	return false
 }

+// translates ref to a DeleteAttribute deleting the object referred by the ref.
+func (a *gcPermissionsEnforcement) toDeleteAttribute(ref metav1.OwnerReference, attributes admission.Attributes) (authorizer.AttributesRecord, error) {


ownerRefToDeleteAttribute?

deads2k · 2017-04-11T19:12:51Z

plugin/pkg/admission/gc/gc_admission.go

+	if err != nil {
+		return authorizer.AttributesRecord{}, err
+	}
+	mapping, err := a.restMapper.RESTMapping(schema.GroupKind{Group: groupVersion.Group, Kind: ref.Kind}, groupVersion.Version)


ResourcesFor and check for any match?

ResourcesFor takes a Resource, not a Kind, as input: https://github.com/kubernetes/apimachinery/blob/master/pkg/api/meta/interfaces.go#L136

RESTMapping().Resource seems the only way to translate Kind to Resource.

RESTMapping().Resource seems the only way to translate Kind to Resource.

RESTMappings then. We should handle the multi-match case.

The behavior is undefined if there are multiple matches. The admission controller cannot determine to which resource the user is trying to set the ownerReference.
In the garbage collector, it only handles the first match.
I think it's a bug in the design phase that the ownerReference records the Kind instead of Resource. I don't think we should fix it by trying all restmappings, rather, we should add a Resource field to the ownerReference. WDYT?

I think it's a bug in the design phase that the ownerReference records the Kind instead of Resource. I don't think we should fix it by trying all restmappings, rather, we should add a Resource field to the ownerReference. WDYT?

I have long thought that, but its controversial. In other places, @liggitt wants to keep kind assumes a canonical mapping from one to the other. I can't tell what @lavalamp wants given comments in the TPR thread, but I previously thought he liked the one to one mapping.

Given that there is more than one mapping possible, we can start out strict (must have delete access to all) and make it less strict later if we find it to be a problem.

I would not start down the path of changing the API without broad agreement.

Regardless of whether Kind <-> Resource is a one-on-one mapping, recording Resource in ownerReference seems correct, because Resource indicates a unique way to retrieve the owner object.

I think the problem of adding ownerReference.Resource now is that we have to keep it alongside the Kind field, due to the deprecation policy. That can cause confusion.

Given that there is more than one mapping possible, we can start out strict (must have delete access to all) and make it less strict later if we find it to be a problem.

Ok.

deads2k · 2017-04-11T19:15:35Z

plugin/pkg/admission/gc/gc_admission_test.go

+			username:        "non-rc-deleter",
+			resource:        api.SchemeGroupVersion.WithResource("pods"),
+			newObj:          podWithOwnerRefs(blockRC1, notBlockRC2),
+			expectedAllowed: false,


We have multiple deny branches now. Please check the error message to make sure it's the failure you're expecting.

deads2k · 2017-04-11T19:16:38Z

Looks structurally sound. A few comments I'd like to see fixed up.

caesarxuchao · 2017-04-12T22:30:30Z

@deads2k i addressed all the comments except the one regarding calling ResourcesFor(). I replied to that comment inline. PTAL. Thanks.

deads2k · 2017-04-13T18:46:59Z

After the multiple mappings handled, this lgtm.

caesarxuchao · 2017-04-13T18:53:10Z

Multiple-mapping case handled. PTAL. Thanks.

deads2k · 2017-04-13T18:53:18Z

lgtm, don't forget to squash.

…rmission of the owner from setting blockOwnerDeletion

caesarxuchao · 2017-04-13T18:56:03Z

Thanks for the reviews. Squashed. Applying the labels per comment.

k8s-github-robot · 2017-04-14T06:18:05Z

Automatic merge from submit-queue (batch tested with PRs 44440, 44038, 44302, 44316, 43876)

caesarxuchao assigned deads2k Mar 30, 2017

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 30, 2017

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Mar 30, 2017

caesarxuchao removed the release-note-label-needed label Mar 30, 2017

k8s-github-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Mar 30, 2017

deads2k assigned sttts Mar 31, 2017

0xmichalis reviewed Apr 4, 2017

View reviewed changes

sttts reviewed Apr 5, 2017

View reviewed changes

deads2k reviewed Apr 11, 2017

View reviewed changes

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 12, 2017

caesarxuchao force-pushed the blockOwnerDeletion-admission branch from 5f2be3c to 587926c Compare April 12, 2017 22:29

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 12, 2017

k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 12, 2017

caesarxuchao force-pushed the blockOwnerDeletion-admission branch from 587926c to a956f7c Compare April 12, 2017 22:55

add gc admission plugin that prevents user who doesn't have delete pe…

9d7a8df

…rmission of the owner from setting blockOwnerDeletion

caesarxuchao force-pushed the blockOwnerDeletion-admission branch from fce1926 to 9d7a8df Compare April 13, 2017 18:55

caesarxuchao added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 13, 2017

k8s-github-robot merged commit 3b9eb1a into kubernetes:master Apr 14, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend the gc admission plugin to check ownerReference.blockOwnerDeletion #43876

Extend the gc admission plugin to check ownerReference.blockOwnerDeletion #43876

caesarxuchao commented Mar 30, 2017 •

edited

Loading

k8s-github-robot commented Mar 30, 2017

k8s-reviewable commented Mar 30, 2017

caesarxuchao commented Mar 31, 2017

deads2k commented Mar 31, 2017

caesarxuchao commented Apr 4, 2017

0xmichalis Apr 4, 2017

sttts Apr 5, 2017

sttts Apr 5, 2017

caesarxuchao Apr 5, 2017

caesarxuchao Apr 5, 2017

sttts Apr 5, 2017

sttts Apr 5, 2017

caesarxuchao Apr 5, 2017

caesarxuchao commented Apr 5, 2017

deads2k Apr 11, 2017

deads2k Apr 11, 2017

deads2k Apr 11, 2017

deads2k Apr 11, 2017

deads2k Apr 11, 2017

deads2k Apr 11, 2017

caesarxuchao Apr 12, 2017

deads2k Apr 13, 2017

caesarxuchao Apr 13, 2017

deads2k Apr 13, 2017

caesarxuchao Apr 13, 2017

deads2k Apr 11, 2017

deads2k commented Apr 11, 2017

caesarxuchao commented Apr 12, 2017

deads2k commented Apr 13, 2017

caesarxuchao commented Apr 13, 2017

deads2k commented Apr 13, 2017

caesarxuchao commented Apr 13, 2017 •

edited

Loading

k8s-github-robot commented Apr 14, 2017

Extend the gc admission plugin to check ownerReference.blockOwnerDeletion #43876

Extend the gc admission plugin to check ownerReference.blockOwnerDeletion #43876

Conversation

caesarxuchao commented Mar 30, 2017 • edited Loading

k8s-github-robot commented Mar 30, 2017

k8s-reviewable commented Mar 30, 2017

caesarxuchao commented Mar 31, 2017

deads2k commented Mar 31, 2017

caesarxuchao commented Apr 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caesarxuchao commented Apr 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Apr 11, 2017

caesarxuchao commented Apr 12, 2017

deads2k commented Apr 13, 2017

caesarxuchao commented Apr 13, 2017

deads2k commented Apr 13, 2017

caesarxuchao commented Apr 13, 2017 • edited Loading

k8s-github-robot commented Apr 14, 2017

caesarxuchao commented Mar 30, 2017 •

edited

Loading

caesarxuchao commented Apr 13, 2017 •

edited

Loading