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

Protected attributes #27330

Closed

Conversation

olegshaldybin
Copy link
Contributor

@olegshaldybin olegshaldybin commented Jun 14, 2016

This change adds support for protected attributes to RBAC API group. Protected attributes allow users to associate certain labels and annotations with RBAC roles: only users in these roles can add or modify objects with protected attributes.

This enables fine-grained authorization based on the object metadata. It should be possible to build complex policies for services, images and other objects based on labels and annotations, and use protected attributes to prevent unprivileged users from using that metadata.

Please see proposal and documentation for more details.

Analytics


This change is Reviewable

@k8s-bot
Copy link

k8s-bot commented Jun 14, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

2 similar comments
@k8s-bot
Copy link

k8s-bot commented Jun 14, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

@k8s-bot
Copy link

k8s-bot commented Jun 14, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

@k8s-github-robot
Copy link

Adding label:do-not-merge because PR changes docs prohibited to auto merge
See http://kubernetes.io/editdocs/ for information about editing docs

@k8s-github-robot k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/new-api size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. labels Jun 14, 2016
@olegshaldybin
Copy link
Contributor Author

@ericchiang @erictune @liggitt @smarterclayton PTAL so we can discuss this approach and its implications, or at-mention whoever you think might be interested. There are some interesting use cases that protected attributes can enable. Looking forward to discuss some of those. Please note that I am only starting to learn K8S internals and might be missing some changes. I did some manual testing in addition to unit tests though, and planning to add integration tests too.

This should be considered an experimental alpha API, just like RBAC itself.

Needs #26680 for unit tests to go green (as they expect proper filtering on a clientset).
Needs #27193 in order for deletes to respect protected attributes.

@smarterclayton smarterclayton assigned deads2k and unassigned lavalamp Jun 14, 2016
@smarterclayton
Copy link
Contributor

xref #2211 #17549 #7893 for backstory

On Mon, Jun 13, 2016 at 9:19 PM, Oleg Shaldybin notifications@github.com
wrote:

@ericchiang https://github.com/ericchiang @erictune
https://github.com/erictune @liggitt https://github.com/liggitt
@smarterclayton https://github.com/smarterclayton PTAL so we can
discuss this approach and its implications, or at-mention whoever you think
might be interested. There are some interesting use cases that protected
attributes can enable. Looking forward to discuss some of those. Please
note that I am only starting to learn K8S internals and might be missing
some changes. I did some manual testing in addition to unit tests though,
and planning to add integration tests too.

This should be considered an experimental alpha API, just like RBAC
itself.

Needs #26680 #26680 for
unit tests to go green (as they expect proper filtering on a clientset).
Needs #27193 #27193 in
order for deletes to respect protected attributes.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#27330 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ABG_p_uVIWYabUNHKZ7eq_yRs8Fwewkdks5qLgGFgaJpZM4I04DA
.

@smarterclayton
Copy link
Contributor

#15390 is probably the controlling proposal at this point. We have general agreement that we want to do field level access control sometime soon, but labels and annotations (and taints and tolerations and selectors) may be special cases of that.

@smarterclayton
Copy link
Contributor

It would be good to update the documentation you've added to be more in the proposal style - see some of the more recent items in the proposals directory for some sections to flesh out.

@smarterclayton smarterclayton self-assigned this Jun 14, 2016
@olegshaldybin
Copy link
Contributor Author

@smarterclayton Sounds good, I'll create a more detailed proposal with some context on use cases and alternatives considered. Thanks for cross-referencing!

@k8s-github-robot k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/design Categorizes issue or PR as related to design. kind/new-api labels Jun 14, 2016
@olegshaldybin
Copy link
Contributor Author

Added proposal.

@smarterclayton
Copy link
Contributor

@kubernetes/sig-auth we should queue this up for the next meeting to discuss

@smarterclayton smarterclayton added the sig/auth Categorizes an issue or PR as relevant to SIG Auth. label Jun 14, 2016
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 14, 2016
@deads2k
Copy link
Contributor

deads2k commented Jun 14, 2016

@kubernetes/sig-auth we should queue this up for the next meeting to discuss

Yes, I think we need to discuss a few points that the proposal here hinted at, but didn't go into enough depth describing. Having true, field level authorization is hard, but I also thinks its where the most value is. It would require having the external representation of the object in admission, but once you have that, labels and annotations become special cases.

I think there should also be a broader discussion of what a field level policy rule should look like. This is focused on a simple "allow mutation or disallow mutation", but bounded ranges and enumerated literals are also very high value: "he can scale it, but not below 5 or above 10" and "he can set this field, but only to A or B". If API server config moves into etcd, we'll probably want the ability to extend these into something like IP addresses in or out of CIDR.

We also need to decide whether these will be deny rules, allow rules, or a mix. The mix is most convenient, but we punted on that from RBAC for a reason. We could re-evaluate for field level policy rule mutation.

Those are the first few that concern me, there may be others. I think that I'll defer reviewing this implementation until I feel like we've settled where we want to go.

@olegshaldybin Will you be able to attend sig-auth tomorrow?

@k8s-bot
Copy link

k8s-bot commented Jun 15, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

@deads2k
Copy link
Contributor

deads2k commented Jun 15, 2016

@olegshaldybin sig-auth cancelled this week.

@k8s-bot
Copy link

k8s-bot commented Jun 23, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

@erictune
Copy link
Member

We discussed this in the SIG meeting. @deads2k had some comments which I will let him summarize.

I suggested that we needed:

  • an idea how this might apply to specific fields, so that we aren't forcing people to use labels and annotations for what should be first class values.
  • more than 2 motivating use cases
  • examples of how you would create ProtectedAttributes to solve a specific problem, like not letting user Alice create a Service with external load balancers.

@liggitt
Copy link
Member

liggitt commented Jun 29, 2016

there was also a desire to build field-level authorization in a way that didn't require use of RBAC, and could work with other authorizers (possibly similar to the approach PodSecurityPolicy is taking, expressing field-level policy restrictions separately from the users/groups it applies to)

@erictune
Copy link
Member

erictune commented Jun 29, 2016

Other concerns:

  • protected labels cannot automatically be propagated from a controller to its pod or vice versa (which is a useful thing to do).
  • if labels are used to control behavior, there will be a temptation to automatically add them, but really, labels are intended for users to organize things. System added ones will clutter the view of labels.

These issues don't apply to annotations.

@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 29, 2016
@deads2k
Copy link
Contributor

deads2k commented Jun 30, 2016

I think that I would start by trying to use jsonpath to select the set of nodes that are being used. I haven't looked into it deeply, but assuming it's like xpath, you can easily end up with multiple nodes selected. I would probably start without allowing backrefs.

The json path will have to be built against a given group/version/kind since the serialization format is important. Evaluation via reflection is straightforward and I'd consider it acceptable for a initial implementation, but it is possible to implement a Navigable interface on objects that has a Navigate(nextOperation op) interface{}. You can generate these methods from go types with json tags to allow navigation without reflection against the API objects you have.

I think we could start with the simplest kinds of field restrictions: "in" and "not-in". Those are pretty easy for an initial implementation, but they will force you to consider evaluation order and trumping rules which will be essential for an extensible solution that can grow to include additional kinds of predicates.

@k8s-github-robot
Copy link

@olegshaldybin PR needs rebase

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 12, 2016
@k8s-bot
Copy link

k8s-bot commented Aug 22, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

@k8s-github-robot
Copy link

This PR hasn't been active in 90 days. Closing this PR. Please reopen if you would like to work towards merging this change, if/when the PR is ready for the next round of review.

cc @deads2k @olegshaldybin @smarterclayton

You can add 'keep-open' label to prevent this from happening again, or add a comment to keep it open another 90 days

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/design Categorizes issue or PR as related to design. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. sig/auth Categorizes an issue or PR as relevant to SIG Auth. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants