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

UPSTREAM: <carry>: Remove write permissions on daemonsets from Kubernetes bootstrap policy #18971

Conversation

enj
Copy link
Contributor

@enj enj commented Mar 13, 2018

Due to how daemonsets interact with the project node selector, we need to limit write access to them to the cluster admin.

Bug 1536304
Bug 1501514

Signed-off-by: Monis Khan [email protected]

/kind bug
/assign @liggitt @deads2k @simo5 @smarterclayton
@openshift/sig-security

/cherrypick release-3.9

@openshift-ci-robot openshift-ci-robot added sig/security kind/bug Categorizes issue or PR as related to a bug. labels Mar 13, 2018
@openshift-merge-robot openshift-merge-robot added the vendor-update Touching vendor dir or related files label Mar 13, 2018
@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 13, 2018
enj added 2 commits March 13, 2018 16:01
…etes bootstrap policy

Due to how daemonsets interact with the project node selector,
we need to limit write access to them to the cluster admin.

Bug 1536304
Bug 1501514

Signed-off-by: Monis Khan <[email protected]>
@enj enj force-pushed the enj/i/disable_daemonset_carry/1536304,1501514 branch from 6b82223 to a42347b Compare March 13, 2018 20:04
@simo5
Copy link
Contributor

simo5 commented Mar 13, 2018

/lgtm
/hold
@openshift/sig-security PTAL and add more comments to cancel hold

@openshift-ci-robot openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Mar 13, 2018
@tnozicka
Copy link
Contributor

I think it would be consistent to remove read permission as well otherwise it just more confusing. There is no reason regular user should even see a daemonset nor create it.

/cherrypick release-3.9

@openshift-cherrypick-robot

@tnozicka: once the present PR merges, I will cherry-pick it on top of release-3.9 in a new PR and assign it to you.

In response to this:

I think it would be consistent to remove read permission as well otherwise it just more confusing. There is no reason regular user should even see a daemonset nor create it.

/cherrypick release-3.9

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.

@liggitt
Copy link
Contributor

liggitt commented Mar 13, 2018

I think it would be consistent to remove read permission as well otherwise it just more confusing. There is no reason regular user should even see a daemonset nor create it.

no. oc get all will attempt to view daemonsets (they are included in the all category), and we don't want to cause errors for that command

@tnozicka
Copy link
Contributor

@liggitt so if someone disables DS for their regular users on their cluster like Online; oc get all will break? Shouldn't we better fix the discovery?

Why should making security decision about permissions be restricted by a way how some command is implemented?

@liggitt
Copy link
Contributor

liggitt commented Mar 13, 2018

@liggitt so if someone disables DS for their regular users on their cluster like Online; oc get all will break? Shouldn't we better fix the discovery?

Letting a user see daemonsets running in their namespace is not problematic.

@tnozicka
Copy link
Contributor

Letting a user see daemonsets running in their namespace is not problematic.

Yes, it isn't problematic. It is also not ideal. But I can live with that.

@tnozicka
Copy link
Contributor

removing just write lgtm

do we need that in 3.7? (do we use the same permission model from upstream there?)

/retest

@mfojtik
Copy link
Contributor

mfojtik commented Mar 14, 2018

/approve

The read permission is acceptable to me, as a user I want to see what runs in my namespace even if I can't create such resources.

@simo5 can we cancel the hold here?

@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: enj, mfojtik, simo5

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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 14, 2018
@simo5
Copy link
Contributor

simo5 commented Mar 14, 2018

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 14, 2018
@openshift-merge-robot
Copy link
Contributor

/test all [submit-queue is verifying that this PR is safe to merge]

@enj
Copy link
Contributor Author

enj commented Mar 14, 2018

We pulled these in #17976 so this only needs to go back to 3.9.

@openshift-ci-robot
Copy link

openshift-ci-robot commented Mar 14, 2018

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

Test name Commit Details Rerun command
ci/openshift-jenkins/gcp a42347b link /test gcp

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
Copy link
Contributor

Automatic merge from submit-queue.

@openshift-cherrypick-robot

@tnozicka: new pull request created: #18977

In response to this:

I think it would be consistent to remove read permission as well otherwise it just more confusing. There is no reason regular user should even see a daemonset nor create it.

/cherrypick release-3.9

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.

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. kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. sig/security size/L Denotes a PR that changes 100-499 lines, ignoring generated files. vendor-update Touching vendor dir or related files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants