Allow project members to update Role and RoleBindings

[mvladev]

What this PR does / why we need it:

Allows project members to define their own Role and RoleBindings.

Privilege escalation is prevented by the RBAC API.

Which issue(s) this PR fixes:
N/A

Special notes for your reviewer:

Continuation of #1865

Release note:

Project members are now allowed to define their own `Roles` and `RoleBindings` to further customize the access to the resources in their Project namespace. 

Project viewers are now allowed to view `Roles` and `RoleBindings` in their Project namespace. 

[rfranzke]

From the changes themselves it /lgtm, but I guess @vlerenc had some comments, so let's wait for it. :)

[vlerenc]

I was only wondering how critical that is and why we want to allow that to users now while we have so many other topics open in the UAM field. Also, I was wondering how the Dashboard code will cope with it (cc @grolu @petersutter @holgerkoser) without either ignoring the fine-grained permissions or complicating the code (adding checks left and right to check for certain permissions). Most UIs have only roles and code against these roles. Allowing the users to create their own roles with their own sets of permissions is more flexible (and there is certainly also demand for that), but also more effort, as it seems (and therefore I am not sure we have the bandwidth for that, but maybe the Dashboard colleagues have found a way to make that work - just asking).

[mvladev]

This PR has nothing to do with the dashboard. The dashboard is an optional component to the project.

[grolu]

@vlerenc we have found a way to deal with that. We don't need to implement roles in the dashboard. We still need to add logic for every action / detail / page etc. that may be hidden for a certain role, however we don't need to differentiate between the roles as we can check for the permissions directly.
See also gardener/dashboard#620

[rfranzke]

Thanks @grolu. I guess then everything is settled and we can go ahead (I understood the major concerns were about the dashboard support, right)? Or is there something else?

[vlerenc]

From my side yes @rfranzke. I was just a tad worried whether we can support so fine-grained permissions in the dashboard @mvladev (yes, another topic, but related in sofar as it checks today what is allowed for UX reasons, so that it doesn't show everything and everything full of errors/after getting to these errors). Thanks @grolu for the heads-up.

[petersutter]

@mvladev can you add in the description why we need this PR? What is the actual use case?

Yes with the PR that @grolu mentioned we can now efficiently check for on permission level for the selected namespace in the dashboard but I would not say that we can support any fine-grained permission that the project admin comes up with.
E.g. with this PR users can come up with their own roles and bindings so that the users in the project only see a subset of the resources. Like user a can't list shoots but can get shoot x, y and z. We can't support such a thing in the dashboard without huge effort.

How should we handle this degree of freedom in the dashboard? That's why I was also asking what the use case is for this PR.

[mvladev]

can you add in the description why we need this PR? What is the actual use case?

Users want to define their own Role / Rolebinding. For example you can create a service account and grant it access only to a specific Shoot / Secret.

How should we handle this degree of freedom in the dashboard? That's why I was also asking what the use case is for this PR.

Why this should be handled by the dashboard?

[petersutter]

Because users could be arguing "it works with kubectl, why can't I see the shoot / secret in the dashboard?"
Maybe it helps when we document the limitations

[mvladev]

Whats the problem with SelfSubjectRulesReview?

 k  create -f - -o yaml  << EOF
apiVersion: authorization.k8s.io/v1
kind: SelfSubjectRulesReview
spec:
  namespace: garden-namespace-1
EOF

apiVersion: authorization.k8s.io/v1
kind: SelfSubjectRulesReview
metadata:
  creationTimestamp: null
spec: {}
status:
  incomplete: false
  nonResourceRules:
  - nonResourceURLs:
    - '*'
    verbs:
    - '*'
  - nonResourceURLs:
    - /api
    - /api/*
    - /apis
    - /apis/*
    - /healthz
    - /openapi
    - /openapi/*
    - /swagger-2.0.0.pb-v1
    - /swagger.json
    - /swaggerapi
    - /swaggerapi/*
    - /version
    - /version/
    verbs:
    - get
  - nonResourceURLs:
    - /healthz
    - /version
    - /version/
    verbs:
    - get
  resourceRules:
  - apiGroups:
    - '*'
    resources:
    - '*'
    verbs:
    - '*'
  - apiGroups:
    - core.gardener.cloud
    resources:
    - cloudprofiles
    verbs:
    - get
    - list
    - watch
  - apiGroups:
    - core.gardener.cloud
    resources:
    - cloudprofiles
    - seeds
    verbs:
    - get
    - list
    - watch
  - apiGroups:
    - authorization.k8s.io
    resources:
    - selfsubjectaccessreviews
    - selfsubjectrulesreviews
    verbs:
    - create

[petersutter]

We already use SelfSubjectRulesReview to hide/show pages and buttons in gardener/dashboard#620 on the frontend logic and it works to support e.g. the viewer role.

But if the user only has the permission to e.g. get shoot x, y, z and does not have the permission to list shoots we get a problem on the backend as we need the list permission to list the shoots(, secrets) etc. How should we show the shoot list if the user does not have the permission to list the shoots?

[vpnachev]

/lgtm

[vpnachev]

@petersutter it looks SelfSubjectRulesReview is what the dashboard needs - it is giving the name of resources exposed to the user.

$ k -n garden-dev create role single-shoot --verb=get --resource=shoots.core.gardener.cloud --resource-name=my-shoot
$ k -n garden-dev create rolebinding single-shoot --role single-shoot --user jd@example.com
$ k --as jd@example.com  create -f - -o yaml  << EOF
apiVersion: authorization.k8s.io/v1
kind: SelfSubjectRulesReview
spec:
  namespace: garden-dev
EOF

apiVersion: authorization.k8s.io/v1
kind: SelfSubjectRulesReview
metadata:
  creationTimestamp: null
spec: {}
status:
  incomplete: false
  nonResourceRules:
  - nonResourceURLs:
    - /healthz
    - /version
    - /version/
    verbs:
    - get
  - nonResourceURLs:
    - /api
    - /api/*
    - /apis
    - /apis/*
    - /healthz
    - /openapi
    - /openapi/*
    - /version
    - /version/
    verbs:
    - get
  resourceRules:
  - apiGroups:
    - authorization.k8s.io
    resources:
    - selfsubjectaccessreviews
    - selfsubjectrulesreviews
    verbs:
    - create
  - apiGroups:
    - core.gardener.cloud
    resources:
    - cloudprofiles
    - seeds
    verbs:
    - get
    - list
    - watch
  - apiGroups:
    - core.gardener.cloud
    resourceNames:
    - my-shoot
    resources:
    - shoots
    verbs:
    - get

[petersutter]

Hi @vpnachev, so you suggest we should implement a fallback logic in case list is not allowed and try to determine the resource names for which get is allowed and then for all of them do a get?
The number of requests could be in the hundrets. Having a cache would not help as we would have to do selfsubjectaccessreview before serving each item from the cache.

[mvladev]

The number of requests could be in the hundrets. Having a cache would not help as we would have to do selfsubjectreview before serving each item from the cache.

I would suspect that -

a) this would be mainly used for automation ( service accounts and so), so dashboard won't be affected.
b) nobody would add a hundreds of shoot clusters to a single role.
c) this would only be applicable if the user opens the Project page. Then a single selfsubjectreview would be sufficient. Caches should be used of course.

[petersutter]

a) that's why I said we should document the limitations or what privileges are required for the dashboard in order to function properly
b) I agree but I have already seen many things where I thought that nobody would do this
c) a single selfsubjectaccessreview is not sufficient as you can't pass a list of resource names

[mvladev]

a) that's why I said we should document the limitations or what privileges are required for the dashboard in order to function properly

This is something the dashboard should handle. I don't think it's related to the scope of the PR.

b) I agree but I have already seen many things where I thought that nobody would do this

a single selfsubjectreview is not sufficient as you can't pass a list of resource names

selfsubjectreview returns the subject's permissions on a namespace:

# allow to "get" two shoots
kubectl -n garden-dev create role single-shoot --verb=get --resource=shoots.core.gardener.cloud --resource-name=my-shoot --resource-name=my-shoot-two
# assign joe to that role
kubectl -n garden-dev create rolebinding single-shoot --role single-shoot --user jd@example.com

# see joe's permissions:
kubectl --as jd@example.com  create -f - -o yaml  << EOF
apiVersion: authorization.k8s.io/v1
kind: SelfSubjectRulesReview
spec:
  namespace: garden-dev
EOF


apiVersion: authorization.k8s.io/v1
kind: SelfSubjectRulesReview
metadata:
  creationTimestamp: null
spec: {}
status:
  incomplete: false
  resourceRules:
  - apiGroups:
    - authorization.k8s.io
    resources:
    - selfsubjectaccessreviews
    - selfsubjectrulesreviews
    verbs:
    - create
  - apiGroups:
    - core.gardener.cloud
    resourceNames:
    - my-shoot
    - my-shoot-two
    resources:
    - shoots
    verbs:
    - get

you can clearly see that joe is allowed to get only my-shoot and my-shoot-two

[mvladev]

If I then add list permissions, the SelfSubjectRulesReview becomes:

kubectl -n garden-dev create role all-shoots --verb=list --resource=shoots.core.gardener.cloud
kubectl -n garden-dev create rolebinding all-shoots --role all-shoots --user jd@example.com

kubectl --as jd@example.com  create -f - -o yaml  << EOF
apiVersion: authorization.k8s.io/v1
kind: SelfSubjectRulesReview
spec:
  namespace: garden-dev
EOF
apiVersion: authorization.k8s.io/v1
kind: SelfSubjectRulesReview
metadata:
  creationTimestamp: null
spec: {}
status:
  incomplete: false
  resourceRules:
  - apiGroups:
    - authorization.k8s.io
    resources:
    - selfsubjectaccessreviews
    - selfsubjectrulesreviews
    verbs:
    - create
  - apiGroups:
    - core.gardener.cloud
    resourceNames:
    - my-shoot
    - my-shoot-two
    resources:
    - shoots
    verbs:
    - get
  - apiGroups:
    - core.gardener.cloud
    resources:
    - shoots
    verbs:
    - list

[petersutter]

c) I ment SubjectAccessReview

You are talking about SelfSubjectRulesReview. The docs cleary state:

It should NOT Be used by external systems to drive authorization decisions as this raises confused deputy, cache lifetime/revocation, and correctness concerns. SubjectAccessReview, and LocalAccessReview are the correct way to defer authorization decisions to the API server.

Hence we can't use SelfSubjectRulesReview and would have to use SubjectAccessReview which does not accept multiple resource names!

[mvladev]

This only applies if the apiserver is configured with --authorization-mode=Webhook and SelfSubjectRulesReview has incomplete: true status:

apiVersion: authorization.k8s.io/v1
kind: SelfSubjectRulesReview
spec: {}
status:
  incomplete: false
.....

As this PR affects only RBAC rules, SelfSubjectRulesReview can be used, given that incomplete is false

[petersutter]

even if it's configured with --authorization-mode=Webhook and SelfSubjectRulesReview has incomplete: true the doc says:

if a rule appears in a list it's safe to assume the subject has that permission

But why do you say that in this case you can use SelfSubjectRulesReview for authorization checks where the doc clearly states not do to so?

[rfranzke]

SelfSubjectRulesReview should be used by UIs to show/hide actions, or to quickly let an end user reason about their permissions.

This is the sentence before

It should NOT Be used by external systems to drive authorization decisions as this raises confused deputy, cache lifetime/revocation, and correctness concerns. SubjectAccessReview, and LocalAccessReview are the correct way to defer authorization decisions to the API server.

Is the dashboard a UI or an external system?

[petersutter]

Again, we use SelfSubjectRulesReview to show/hide actions on the UI, which is fine.

On the backend (the component that provides the data to the UI) we do not want to use SelfSubjectRulesReview as replacement of SubjectAccessReview for authorization checks when we use the dashboard's (admin) client or to retrieve data / watch data or serve something from the cache. This has nothing to do with hiding / showing actions on the UI.

[grolu]

Is the dashboard a UI or an external system?

The dashboard is an external system which servers an UI. We can (and already do use) SelfSubjectRulesReview to hide parts of the UI that the user has no access to and would not work anyway (due to missing permissions). However, we must never use SelfSubjectRulesReview to determine if a user can get data from our cache which we fetched using an other user.

[holgerkoser]

Is the dashboard a UI or an external system?

The question is not UI or external system. The question is do we use SelfSubjectRulesReview to drive authorization decisions?

[mvladev]

Okay I'll suggest to move this discussion to a separate issue in the Dashboard as this problem already exist in it and it as has nothing to do with this PR.

[vlerenc]

This is something the dashboard should handle. I don't think it's related to the scope of the PR.

@mvladev You wrote that already. However, the concern is that if that feature gets merged and we have no plan for the dashboard, it shouldn't be merged in the first place, if not absolutely necessary. Yes, there are very few customers who request that, but most of those few can be helped with pre-defined roles. Full dynamic roles may simply be too much for us and doing such things like replacing a list call with a custom check for clusters based on the response of SelfSubjectRulesReview is, I hope you agree, a lot of work for the dashboard colleagues. This is why I asked in the beginning. We can't put that kind of work onto the dashboard team, safe or not. It's just impossible for them to implement all these corner cases. That said, we can define how much we support, maybe nothing of that sort in the UI if not backed by a Dashboard-known role. That was @petersutter point, I believe. To set the expectations right. And if they are right, it's still possible to merge this one, if the need is large enough.

So, in essence, the Dashboard question is open. If you continue the discussion on a Dashboard BLI, please don't merge this one beforehand/because of the other one. If everybody is fine with the implications, sure. But if there is uncertainty and not sufficient value, please be cautious with new features. We have to support them all and live with them.

[vpnachev]

The Dashboard is either way not supporting all feature of the API, e.g. there is almost nothing for Seeds, CloudProfiles, Plants and ControllerRegistration. Also, nobody should expect it. The dashboard should support some basic scenarios, like it is already doing.

This change will not affect the current scenario with the pre-defined roles(owner, admin, viewer) so only these very limited number of users will not be able to use properly the Dashboard.

So I would go out with a proper documentation that the Dashboard is not supporting this RBAC openness, nothing more.

How does the Kubernetes Dashboard solves this issue?

[rfranzke]

+1 for @vpnachev's proposal. For me the question is whether the Gardener Dashboard wants to support these custom roles or not. I understand that listing with an admin user and then filtering out based on the result of the SelfSubjectAccessReview is no good option. (Though, I don't understand the technical argument that you discussed above, but anyways). Like @vpnachev said - the Kubernetes Dashboard does not support these fine-granular things either, and the reason clearly is that the Kubernetes API simply doesn't support it (if you don't have LIST privileges but only GET for two resources then you cannot get the list of these two resources from the API server).

To go forward, my concrete proposal is: Merge this PR and document that custom roles (all other than the roles defined in the Gardener API (admin/viewer at the moment)) are not necessarily supported by the Gardener Dashboard (right now). This helps giving end-users the right expectations for the GUI but still the freedom for the API. If @gardener/dashboard-maintainers want to support these custom roles in the future (by whatever mechanisms/implementation) then the discussion how can be followed up in a separate ticket on hgardener/dashboard. Is everybody OK with it?

[petersutter]

I'm fine with @rfranzke 's proposal

[grolu]

I'm also fine with it.

[rfranzke]

@mvladev can you please open a PR on either the g/dashboard or g/documentation repo that explains that these custom roles are not supported by the Dashboard as of today?