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

Jump to bottom

Bug 1752045: UPSTREAM: <carry>:Add a RBAC checker for external IP ranger #23783

Merged
merged 1 commit into from Oct 2, 2019
Merged

Bug 1752045: UPSTREAM: <carry>:Add a RBAC checker for external IP ranger #23783

merged 1 commit into from Oct 2, 2019

Conversation

abhat
Copy link
Contributor

@abhat abhat commented Sep 13, 2019

This PR adds a RBAC checker for adding an External IP range for a service.

@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. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 13, 2019
@abhat
Copy link
Contributor Author

abhat commented Sep 13, 2019

/assign @danwinship

@abhat abhat changed the title Add a RBAC checker for external IP ranger UPSTREAM: <carry>:Add a RBAC checker for external IP ranger Sep 13, 2019
@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 16, 2019
@abhat abhat force-pushed the rbac_external_ipranger branch 3 times, most recently from be4f251 to b523b01 Compare September 17, 2019 02:26
@abhat abhat marked this pull request as ready for review September 17, 2019 15:35
@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 Sep 17, 2019
@soltysh
Copy link
Member

soltysh commented Sep 18, 2019

/cc @deads2k @sttts
since this is your area and on top of this is adding another carry patch

@squeed
Copy link
Contributor

squeed commented Sep 18, 2019

/retitle Bug 1752045: UPSTREAM: :Add a RBAC checker for external IP ranger

@openshift-ci-robot openshift-ci-robot changed the title UPSTREAM: <carry>:Add a RBAC checker for external IP ranger Bug 1752045: UPSTREAM: <carry>:Add a RBAC checker for external IP ranger Sep 18, 2019
@openshift-ci-robot
Copy link

@abhat: This pull request references Bugzilla bug 1752045, which is invalid:

  • expected the bug to target the "4.2.0" release, but it targets "4.3.0" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

Bug 1752045: UPSTREAM: :Add a RBAC checker for external IP ranger

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.

@openshift-ci-robot openshift-ci-robot added the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Sep 18, 2019
@squeed
Copy link
Contributor

squeed commented Sep 18, 2019

FWIW, this used to live in pkg/network, but since it's part of the kube-apiserver, vendor/... is the authoritative source.

@abhat
Copy link
Contributor Author

abhat commented Sep 18, 2019

Update: fixed a typo in the testname in one of the admission tests in the recent force-push.

@deads2k
Copy link
Contributor

deads2k commented Sep 20, 2019 •
edited

/hold

I'd like to see an enhancement for this that outlines the problem, links prior art, and suggests different options for the RBAC permissions.

EDIT: It should not take more than hour. Think of this as your alternative to writing it three times for dev, doc, and test. You'll be able to link everyone to a single spot.

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 20, 2019
@deads2k
Copy link
Contributor

deads2k commented Sep 20, 2019

1 Is this really a namespace scoped check?
2. Does subdivision of this power by namespace make sense?
3. Should regular users with this power in one namespace be able delegate that power to other users?
4. Is this an escalating permission for the sake of authorization scope grants?

@danwinship
Copy link
Contributor

We don't really expect anyone to actually be granted permission to do this. The RBAC check is just an escape hatch for cluster admins to be able to bypass the checks if absolutely needed. The design was based on what the restricted endpoints admission controller does, but that's a few years old and maybe that's no longer considered the right thing?

That said:

1 Is this really a namespace scoped check?
2. Does subdivision of this power by namespace make sense?
3. Should regular users with this power in one namespace be able delegate that power to other users?

No to all three: being able to bypass external IP restrictions in any namespace gives you the power to attack people in every namespace.

  1. Is this an escalating permission for the sake of authorization scope grants?

I don't know what you mean by that

@abhat abhat force-pushed the rbac_external_ipranger branch 2 times, most recently from 89c07e1 to ce4d829 Compare September 26, 2019 16:39
@abhat
Copy link
Contributor Author

abhat commented Sep 26, 2019

/retest

1 similar comment
@abhat
Copy link
Contributor Author

abhat commented Sep 30, 2019

/retest

@abhat
UPSTREAM: <carry>:Add a RBAC checker for external IP ranger
63cbd7a 
In case the supplied external IPs in the service spec are valid,
allow the RBAC check to override the specified ranges in the
external IP range checker.
@abhat
Copy link
Contributor Author

abhat commented Oct 1, 2019

/bugzilla refresh

@openshift-ci-robot openshift-ci-robot added bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. and removed bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels Oct 1, 2019
@openshift-ci-robot
Copy link

@abhat: This pull request references Bugzilla bug 1752045, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

/bugzilla refresh

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.

@deads2k
Copy link
Contributor

deads2k commented Oct 1, 2019

/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 Oct 1, 2019
@danwinship
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 1, 2019
@deads2k
Copy link
Contributor

deads2k commented Oct 2, 2019

/approve

@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhat, danwinship, 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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 2, 2019
@openshift-merge-robot openshift-merge-robot merged commit 57f1e30 into openshift:master Oct 2, 2019
@openshift-ci-robot
Copy link

@abhat: All pull requests linked via external trackers have merged. Bugzilla bug 1752045 has been moved to the MODIFIED state.

In response to this:

Bug 1752045: UPSTREAM: :Add a RBAC checker for external IP ranger

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.

@abhat abhat deleted the rbac_external_ipranger branch October 11, 2019 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danwinship danwinship danwinship left review comments

@deads2k deads2k deads2k left review comments

@bparees bparees Awaiting requested review from bparees

@soltysh soltysh Awaiting requested review from soltysh

@sttts sttts Awaiting requested review from sttts

Assignees

@danwinship danwinship

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@abhat @soltysh @squeed @openshift-ci-robot @deads2k @danwinship @openshift-merge-robot