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

don't block creation on lack of delete powers #19404

Merged
merged 2 commits into from
May 4, 2018

Conversation

deads2k
Copy link
Contributor

@deads2k deads2k commented Apr 18, 2018

Create and delete aren't the same thing, but the alternatives seem worse. This stops checking for deletion powers on create.

WIP so we don't accidentally merge this without an upstream pull.

@bparees @openshift/sig-master

@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. sig/master labels Apr 18, 2018
@openshift-ci-robot openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 18, 2018
@soltysh
Copy link
Contributor

soltysh commented Apr 18, 2018

Create and delete aren't the same thing, but the alternatives seem worse. This stops checking for deletion powers on create.

Where this is coming from that we need to open the discussion for changing it?

@liggitt
Copy link
Contributor

liggitt commented Apr 18, 2018

Where this is coming from that we need to open the discussion for changing it?

template service broker sets up ownerref relationships on created objects at creation time. we don't really want to give it delete permissions just to pass this check. it doesn't seem unreasonable to let the creator of an object declare an owning object. protecting against an update by someone without delete permission adding an ownerref and triggering deletion is more valuable.

@enj
Copy link
Contributor

enj commented Apr 19, 2018

it doesn't seem unreasonable to let the creator of an object declare an owning object

That sounds OK. I am wondering if there are any weird read semantics we need to consider. For example, can you probe for the existence of resource A (that you cannot see) by setting ownerrefs on resource B (which you own) and seeing if your resource B gets immediately GC'd? I am not sure what that gets you, but it does remind of blind SQL injection attacks.

@openshift/sig-security

@liggitt
Copy link
Contributor

liggitt commented Apr 19, 2018

You need to know the uid...

@tnozicka
Copy link
Contributor

+1; I have hit the same issue as service catalog with openshift-acme

@enj
Copy link
Contributor

enj commented Apr 23, 2018

You need to know the uid...

Ah I forgot about that. Do not really see any issue with this then.

@deads2k deads2k force-pushed the server-24-admission branch from 1c15a35 to fd8a8c5 Compare May 3, 2018 16:09
@deads2k deads2k changed the title WIP don't block creation on lack of delete powers don't block creation on lack of delete powers May 3, 2018
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 3, 2018
@deads2k
Copy link
Contributor Author

deads2k commented May 3, 2018

Opened upstream. Ready for review. This is not a clean pick.

@deads2k
Copy link
Contributor Author

deads2k commented May 3, 2018

/retest

@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k

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

@liggitt
Copy link
Contributor

liggitt commented May 3, 2018

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 3, 2018
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-ci-robot
Copy link

openshift-ci-robot commented May 4, 2018

@deads2k: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
ci/openshift-jenkins/unit 410094c link /test unit

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@openshift-merge-robot openshift-merge-robot merged commit 28bd83f into openshift:master May 4, 2018
@deads2k deads2k deleted the server-24-admission branch July 3, 2018 17:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. sig/master sig/security size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants