add clusterrole aggregation doc #1219

deads2k · 2017-10-17T16:17:55Z

In order to support easy RBAC integration for CustomResources and Extension
APIServers, we need to have a way for API extenders to add permissions to the
"normal" roles for admin, edit, and view.

A doc form of kubernetes/kubernetes#54005

@kubernetes/sig-auth-feature-requests

ericchiang · 2017-10-17T17:34:32Z

sig-auth/cluster-role-aggregation.md

+# Cluster Role Aggregation
+In order to support easy RBAC integration for CustomResources and Extension
+APIServers, we need to have a way for API extenders to add permissions to the
+"normal" roles for admin, edit, and view.


This isn't explicitly stated, but it seems to be implied that we see admin, edit, and view being switched to aggregated rules so admins can register their permissions on the default ClusterRoles?

This isn't explicitly stated, but it seems to be implied that we see admin, edit, and view being switched to aggregated rules so admins can register their permissions on the default ClusterRoles?

Yes. The open pull demonstrates a simple way to transition them.

ericchiang · 2017-10-17T17:39:26Z

sig-auth/cluster-role-aggregation.md

+subject to overwriting at any point.
+
+`aggregationRule` needs to be protected from escalation.  The simplest way to
+do this is to restrict it to users with verb=`*`, apiGroups=`*`, resources=`*`.  We


How will this work with the bind verb the GKE and other webhook authorizers use?

Those govern writes of rolebindings, not roles. There's an outstanding enhancement issue for cross authorizer escalation checks for role mutation: kubernetes/kubernetes#43409

ncdc · 2017-10-18T13:16:55Z

I like this! cc @mattmoyer @jbeda @timothysc

liggitt · 2017-10-17T19:59:30Z

sig-auth/cluster-role-aggregation.md

+type AggregationRule struct {
+	// Selector holds a list of selectors which will be used to find ClusterRoles and create the rules.
+	// If any of the selectors match, then the ClusterRole's permissions will be added
+	Selectors []metav1.LabelSelector


clusterRoleSelectors? all other selector fields in the API are either selector (and select pods) or are <type>Selector (nodeSelector, namespaceSelector, podSelector)

clusterRoleSelectors? all other selector fields in the API are either selector (and select pods) or are Selector (nodeSelector, namespaceSelector, podSelector)

No preference.

liggitt · 2017-10-17T20:04:19Z

sig-auth/cluster-role-aggregation.md

+metadata:
+  name: etcd-operator-admin
+  label:
+    rbac.authorization.k8s.io/aggregate-to-admin: true


rbac.authorization.k8s.io/aggregate-to-clusterrole-name: admin to match the example above?

rbac.authorization.k8s.io/aggregate-to-clusterrole-name: admin to match the example above?

Turns out that doing it this way (name in the key), allows the role to try to aggregate itself to multiple cluster roles. The other way does not.

true. update the other one, then.

liggitt · 2017-10-17T20:06:01Z

sig-auth/cluster-role-aggregation.md

+	// AggregationRule is an optional field that describes how to build the Rules for this ClusterRole.
+	// If AggregationRule is set, then the Rules are controller managed and direct changes to Rules will be
+	// stomped by the controller.
+	AggregationRule *AggregationRule


what would reconciliation for this field look like? that will be important in transitioning the existing admin/edit/view roles to aggregated roles

what would we do in the following cases:

persisted role has null, desired role has aggregation rule (meaning setting an aggregation rule could tighten permissions in rules, which we avoid doing automatically)

persisted role has aggregation rule, desired role has null (meaning any rules we added to would get stomped)

persisted role has aggregation rule, desired role has aggregation rule with different selectors

what would reconciliation for this field look like? that will be important in transitioning the existing admin/edit/view roles to aggregated roles

I replaced as a unit: https://github.com/kubernetes/kubernetes/pull/54005/files#diff-769f1c7c52943946bdc1ec3d7c9ec5d2R205 . It could tighten. I could write a one-time migration for those three using the reconcile post-start hook if you're particularly concerned. I think its unlikely that many people will have problems going forward.

what would reconciliation for this field look like? that will be important in transitioning the existing admin/edit/view roles to aggregated roles
I replaced as a unit: https://github.com/kubernetes/kubernetes/pull/54005/files#diff-769f1c7c52943946bdc1ec3d7c9ec5d2R205 . It could tighten. I could write a one-time migration for those three using the reconcile post-start hook if you're particularly concerned. I think its unlikely that many people will have problems going forward.

I have updated this so that selectors which are not present in the actual get added. On a tightening reconcile, the extra selectors are removed.

erictune · 2017-10-26T22:42:56Z

Thanks for this, David, seems super useful.

liggitt · 2017-11-06T21:20:25Z

LGTM, just need to update the label/selector in the examples to match

ericchiang · 2017-11-06T22:15:21Z

lgtm

/cc @ecordell @xiang90 --- FYI this lets us tie CRD permissions to default cluster roles

k8s-ci-robot · 2017-11-06T22:15:26Z

@ericchiang: GitHub didn't allow me to request PR reviews from the following users: CRD, to, default, lets, us, cluster, this, tie, permissions, roles, FYI.

Note that only kubernetes members can review this PR, and authors cannot review their own PRs.

In response to this:

lgtm

/cc @ecordell @xiang90 --- FYI this lets us tie CRD permissions to default cluster roles

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 · 2017-11-08T14:05:45Z

/lgtm

k8s-github-robot · 2017-11-08T14:08:57Z

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

k8s-github-robot · 2017-11-08T16:13:59Z

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

k8s-github-robot · 2017-11-08T18:00:06Z

Automatic merge from submit-queue.

Automatic merge from submit-queue (batch tested with PRs 54005, 55127, 53850, 55486, 53440). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. aggregate cluster roles xref kubernetes/community#1219 kubernetes/enhancements#502 This is a pull with API types, a controller, and a demonstration of how to move admin, edit, and view. Once we agree on the shape, I'll I added ```yaml aggregationRule: clusterRoleSelectors: - matchLabels: rbac.authorization.k8s.io/aggregate-to-admin: true ``` to the `ClusterRole`. A controller then goes and gathers all the matching ClusterRoles and sets the `rules` to the union of matching cluster roles. @kubernetes/sig-auth-pr-reviews ```release-note RBAC ClusterRoles can now select other roles to aggregate ```

Automatic merge from submit-queue (batch tested with PRs 54005, 55127, 53850, 55486, 53440). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. aggregate cluster roles xref kubernetes/community#1219 kubernetes/enhancements#502 This is a pull with API types, a controller, and a demonstration of how to move admin, edit, and view. Once we agree on the shape, I'll I added ```yaml aggregationRule: clusterRoleSelectors: - matchLabels: rbac.authorization.k8s.io/aggregate-to-admin: true ``` to the `ClusterRole`. A controller then goes and gathers all the matching ClusterRoles and sets the `rules` to the union of matching cluster roles. @kubernetes/sig-auth-pr-reviews ```release-note RBAC ClusterRoles can now select other roles to aggregate ``` Kubernetes-commit: f575c55589db84ef4d392823120f0238fd19ad93

@deads2k

Automatic merge from submit-queue. sig-auth: move role aggregation proposal to proposal directory #1219 added a proposal to the wrong directory. /sig auth /assign @deads2k

Automatic merge from submit-queue (batch tested with PRs 54005, 55127, 53850, 55486, 53440). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. aggregate cluster roles xref kubernetes/community#1219 kubernetes/enhancements#502 This is a pull with API types, a controller, and a demonstration of how to move admin, edit, and view. Once we agree on the shape, I'll I added ```yaml aggregationRule: clusterRoleSelectors: - matchLabels: rbac.authorization.k8s.io/aggregate-to-admin: true ``` to the `ClusterRole`. A controller then goes and gathers all the matching ClusterRoles and sets the `rules` to the union of matching cluster roles. @kubernetes/sig-auth-pr-reviews ```release-note RBAC ClusterRoles can now select other roles to aggregate ``` Kubernetes-commit: f575c55589db84ef4d392823120f0238fd19ad93

deads2k mentioned this pull request Oct 17, 2017

aggregate cluster roles kubernetes/kubernetes#54005

Merged

deads2k force-pushed the role-aggregation branch from e6632a3 to 9edd64e Compare October 17, 2017 16:24

ericchiang reviewed Oct 17, 2017

View reviewed changes

k8s-github-robot assigned liggitt and ericchiang Oct 17, 2017

liggitt reviewed Oct 18, 2017

View reviewed changes

deads2k mentioned this pull request Oct 27, 2017

ClusterRole Aggregation kubernetes/enhancements#502

Closed

k8s-ci-robot requested review from ecordell and xiang90 November 6, 2017 22:15

ericchiang removed request for ecordell and xiang90 November 6, 2017 22:15

add clusterrole aggregation doc

1180709

deads2k force-pushed the role-aggregation branch from 9edd64e to 1180709 Compare November 7, 2017 15:56

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 8, 2017

k8s-github-robot merged commit a4dc44f into kubernetes:master Nov 8, 2017

ericchiang mentioned this pull request Nov 15, 2017

sig-auth: move role aggregation proposal to proposal directory #1402

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add clusterrole aggregation doc #1219

add clusterrole aggregation doc #1219

deads2k commented Oct 17, 2017

ericchiang Oct 17, 2017

deads2k Oct 18, 2017

ericchiang Oct 17, 2017

liggitt Oct 17, 2017

ncdc commented Oct 18, 2017

liggitt Oct 17, 2017

deads2k Oct 18, 2017

liggitt Oct 17, 2017

deads2k Oct 18, 2017

liggitt Oct 18, 2017

liggitt Oct 17, 2017

deads2k Oct 18, 2017

deads2k Oct 20, 2017

erictune commented Oct 26, 2017

liggitt commented Nov 6, 2017

ericchiang commented Nov 6, 2017

k8s-ci-robot commented Nov 6, 2017

liggitt commented Nov 8, 2017

k8s-github-robot commented Nov 8, 2017

k8s-github-robot commented Nov 8, 2017

k8s-github-robot commented Nov 8, 2017

add clusterrole aggregation doc #1219

add clusterrole aggregation doc #1219

Conversation

deads2k commented Oct 17, 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

ncdc commented Oct 18, 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

erictune commented Oct 26, 2017

liggitt commented Nov 6, 2017

ericchiang commented Nov 6, 2017

k8s-ci-robot commented Nov 6, 2017

liggitt commented Nov 8, 2017

k8s-github-robot commented Nov 8, 2017

k8s-github-robot commented Nov 8, 2017

k8s-github-robot commented Nov 8, 2017