Explicit cluster roles for controller/webhook, not cluster-admin #872

vaikas · 2019-03-08T16:02:18Z

Fixes #749

Proposed Changes

Create a service account for running webhook (eventing-webhook)
create 2 new cluster roles: knative-eventing-webhook, knative-eventing-controller
create 2 clusterrole bindings

Release Note

 * webhook now runs as: eventing-webhook service account
 * controller now runs as eventing-controller service account
 * controller and webhook run with less priviledges (was using cluster-admin)

Harwayne · 2019-03-08T18:17:39Z

config/200-webhook-clusterrole.yaml

+    resources: ["channels", "clusterchannelprovisioners", "subscriptions"]
+    verbs: ["get", "list", "create", "watch"]
+  - apiGroups: ["eventing.knative.dev"]
+    resources: ["channels/status", "clusterchannelprovisioners/status", "subscriptions/status"]


My preference would be to put these into the previous list, so that after each foo, we then have foo/status. I think that will make it easier to see when one has been forgotten.

config/200-controller-clusterrole.yaml

config/200-webhook-clusterrole.yaml

grantr

Thanks so much for doing this @vaikas-google! It's embarrassing that we've had cluster-admin for so long.

How did you come up with the list of permissions for the controller SA?

grantr · 2019-03-08T18:15:32Z

config/200-controller-clusterrole.yaml

+    verbs: ["get", "list", "create", "update", "delete", "patch", "watch"]
+  - apiGroups: ["apiextensions.k8s.io"]
+    resources: ["customresourcedefinitions"]
+    verbs: ["get", "list", "create", "update", "delete", "patch", "watch"]


Does the controller really need write permissions on CRDs?

Do these permissions apply to CRDs or all the custom resources whose type is a CRD? If the former, why do we need any permissions on CRDs?

left over cruft, removed.

grantr · 2019-03-08T18:17:33Z

config/200-webhook-clusterrole.yaml

+    verbs: ["get", "list", "watch"]
+  # For manipulating certs into secrets
+  - apiGroups: [""]
+    resources: ["secrets"]


I wonder if this is a candidate for a (namespaced) Role so the webhook doesn't have access to all secrets everywhere. Seems like maybe configmaps and deployments permissions could also be namespaced.

ok to make this into a separate issue and tackle that in a followup?

+1 to a followup issue.

config/200-webhook-clusterrole.yaml

evankanderson

A few more comments, but thanks for doing this.

If you want the further refinements to go into another PR, feel free to open an issue; this is already a definite improvement.

/hold
/lgtm

evankanderson · 2019-03-26T21:43:37Z

config/200-controller-clusterrole.yaml

+  name: knative-eventing-controller
+rules:
+  - apiGroups: [""]
+    resources: ["namespaces", "secrets", "configmaps", "services", "events", "serviceaccounts"]


Do we need list/delete/watch on events? (Or create/update/patch for namespaces, secrets or serviceaccounts?)

May not be worth pulling out separately.

evankanderson · 2019-03-26T21:44:57Z

config/200-controller-clusterrole.yaml

+    resources: ["routes", "services"]
+    verbs: ["get", "list", "watch"]
+  - apiGroups: ["serving.knative.dev"]
+    resources: ["routes/status", "services/status"]


We merge the "foo" and "foo/status" on line 34. Do we want to do the same here?

sure thing, done.

evankanderson · 2019-03-26T21:46:04Z

config/200-controller-clusterrole.yaml

+  - apiGroups: ["eventing.knative.dev"]
+    resources: ["brokers", "brokers/status", "channels", "channels/status", "clusterchannelprovisioners", "clusterchannelprovisioners/status", "subscriptions", "subscriptions/status", "triggers", "triggers/status",]
+    verbs: ["get", "list", "create", "update", "delete", "patch", "watch"]
+  - apiGroups: ["apps"]


It's strange to have this apiGroup down here but virtualservices on line 30.

Maybe put in a comment # Resources we create as part of our side effects

my bad :) moved...

evankanderson · 2019-03-26T21:46:20Z

config/200-controller-clusterrole.yaml

+    verbs: ["get", "list", "create", "update", "delete", "patch", "watch"]
+  - apiGroups: ["rbac.authorization.k8s.io"]
+    resources: ["rolebindings"]
+    verbs: ["get", "list", "create", "update", "delete", "patch", "watch"]


Why do we need these permissions on rolebindings?

these are needed for triggers.

Ah -- this is actually needed for the namespace Broker auto-provisioner. https://github.com/knative/eventing/blob/master/pkg/reconciler/v1alpha1/namespace/namespace.go#L264

I wonder if we want to extract the Broker auto-provisioner at some point in the future.

evankanderson · 2019-03-26T21:59:44Z

config/200-webhook-clusterrole.yaml

+    verbs: ["get", "list", "watch"]
+  # For manipulating certs into secrets
+  - apiGroups: [""]
+    resources: ["secrets"]


+1 to a followup issue.

evankanderson

/lgtm
/approve

evankanderson · 2019-03-26T23:24:14Z

config/200-controller-clusterrole.yaml

+    verbs: ["get", "list", "create", "update", "delete", "patch", "watch"]
+  - apiGroups: ["rbac.authorization.k8s.io"]
+    resources: ["rolebindings"]
+    verbs: ["get", "list", "create", "update", "delete", "patch", "watch"]


Ah -- this is actually needed for the namespace Broker auto-provisioner. https://github.com/knative/eventing/blob/master/pkg/reconciler/v1alpha1/namespace/namespace.go#L264

I wonder if we want to extract the Broker auto-provisioner at some point in the future.

knative-prow-robot · 2019-03-26T23:24:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: evankanderson, vaikas-google

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:

~~OWNERS~~ [evankanderson,vaikas-google]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

evankanderson · 2019-03-26T23:59:30Z

As long as this is titled "WIP", it won't merge. (FYI)

Harwayne · 2019-03-27T20:59:07Z

config/201-clusterrolebinding.yaml

+apiVersion: rbac.authorization.k8s.io/v1
+kind: ClusterRoleBinding
+metadata:
+  name: eventing-webhook-admin


Rename to eventing-webook, or something similar as this is no longer granting admin.

Do a similar rename to eventing-controller-admin. This will mean that it won't update the existing RBAC on upgrade, if you consider that important.

Harwayne · 2019-03-27T21:00:00Z

config/200-webhook-clusterrole.yaml

+  # Our own resources and statuses we care about
+  - apiGroups: ["eventing.knative.dev"]
+    resources: ["brokers", "brokers/status", "channels", "channels/status", "clusterchannelprovisioners", "clusterchannelprovisioners/status", "subscriptions", "subscriptions/status", "triggers", "triggers/status",]
+    verbs: ["get", "list", "create", "watch"]


We use create in the webhook?

Harwayne · 2019-03-27T21:01:53Z

/lgtm
/hold

Holding in case you want to change in response to my comments. If not, feel free to cancel the hold yourself.

evankanderson · 2019-03-27T21:08:30Z

/lgtm

vaikas · 2019-03-27T21:13:08Z

I've created:
#982
to audit the roles. I want to get this in as it's clearly an improvement and then we can improve in a follow up.

vaikas added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 8, 2019

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Mar 8, 2019

knative-prow-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 Mar 8, 2019

knative-prow-robot requested review from grantr and Harwayne March 8, 2019 16:02

Harwayne reviewed Mar 8, 2019

View reviewed changes

grantr reviewed Mar 8, 2019

View reviewed changes

vaikas force-pushed the issue-749 branch from 4e9893f to f014373 Compare March 26, 2019 16:38

Ville Aikas added 3 commits March 26, 2019 20:18

WIP: explicit cluster roles for controller/webhook, not cluster-admin

b470ea5

address PR feedback

2bcf1c6

add broker / trigger roles

8e1359f

vaikas force-pushed the issue-749 branch from b9ecbee to 8e1359f Compare March 26, 2019 20:18

apps / rolebindings to controller-clusterrole

1777c95

knative-prow-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 Mar 26, 2019

need to create deployments

03e150d

evankanderson reviewed Mar 26, 2019

View reviewed changes

knative-prow-robot assigned evankanderson Mar 26, 2019

knative-prow-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Mar 26, 2019

pr feedback round 239

0f22d99

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Mar 26, 2019

evankanderson reviewed Mar 26, 2019

View reviewed changes

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 26, 2019

webhook uses extensions instead of apps

64f838f

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Mar 27, 2019

vaikas mentioned this pull request Mar 27, 2019

Audit RBAC rules for Controller / Webhook to minimal set. #982

Closed

vaikas removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Mar 27, 2019

knative-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 27, 2019

vaikas changed the title ~~WIP: explicit cluster roles for controller/webhook, not cluster-admin~~ Explicit cluster roles for controller/webhook, not cluster-admin Mar 27, 2019

knative-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 27, 2019

Harwayne reviewed Mar 27, 2019

View reviewed changes

knative-prow-robot assigned Harwayne Mar 27, 2019

knative-prow-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Mar 27, 2019

vaikas removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 27, 2019

knative-prow-robot merged commit 9d995bc into knative:master Mar 27, 2019

vaikas deleted the issue-749 branch March 27, 2019 21:15

pierDipi pushed a commit to pierDipi/eventing that referenced this pull request Sep 24, 2024

Make pkg/auth independent from eventpolicy informer (knative#872)

3789dfe

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Explicit cluster roles for controller/webhook, not cluster-admin #872

Explicit cluster roles for controller/webhook, not cluster-admin #872

vaikas commented Mar 8, 2019 •

edited

Loading

Harwayne Mar 8, 2019

grantr left a comment

grantr Mar 8, 2019

Harwayne Mar 8, 2019

vaikas Mar 26, 2019

grantr Mar 8, 2019

vaikas Mar 26, 2019

evankanderson Mar 26, 2019

evankanderson left a comment

evankanderson Mar 26, 2019

evankanderson Mar 26, 2019

vaikas Mar 26, 2019

evankanderson Mar 26, 2019

vaikas Mar 26, 2019

evankanderson Mar 26, 2019

vaikas Mar 26, 2019

evankanderson Mar 26, 2019

evankanderson Mar 26, 2019

evankanderson left a comment

evankanderson Mar 26, 2019

knative-prow-robot commented Mar 26, 2019

evankanderson commented Mar 26, 2019

Harwayne Mar 27, 2019

Harwayne Mar 27, 2019

Harwayne commented Mar 27, 2019

evankanderson commented Mar 27, 2019

vaikas commented Mar 27, 2019

Explicit cluster roles for controller/webhook, not cluster-admin #872

Explicit cluster roles for controller/webhook, not cluster-admin #872

Conversation

vaikas commented Mar 8, 2019 • edited Loading

Proposed Changes

Choose a reason for hiding this comment

grantr left a 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

evankanderson left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evankanderson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knative-prow-robot commented Mar 26, 2019

evankanderson commented Mar 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Harwayne commented Mar 27, 2019

evankanderson commented Mar 27, 2019

vaikas commented Mar 27, 2019

vaikas commented Mar 8, 2019 •

edited

Loading