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

Allow Tenant Users to list namespaces and nodes #187

Closed
carpenterm opened this issue Feb 15, 2022 · 12 comments
Closed

Allow Tenant Users to list namespaces and nodes #187

carpenterm opened this issue Feb 15, 2022 · 12 comments
Labels
documentation Improvements or additions to documentation enhancement New feature or request

Comments

@carpenterm
Copy link
Contributor

It seems like Capsule Proxy only allows Tenant Owners to list cluster-scoped resources like nodes, namespaces, storage classes, ingress classes, etc. This is great, but the Tenant Owner isn’t necessarily the one using the cluster on a day to day basis. In our case they aren’t usually building and deploying applications or debugging application issues - this falls to regular users. Listing nodes and namespaces is an essential part of any user’s role, so we need them to be able to do this and preferably have read only (list/get) access on all of the other cluster-scoped objects too.

Are there existing plans to support this or known issues enabling this functionality, or is it already possible some other way which I’ve missed?

Could we add a users[].proxySettings field in the Tenant spec that controls access to these operations similar to the way it works for owners?

@bsctl
Copy link
Member

bsctl commented Feb 16, 2022

@carpenterm thanks for using Capsule. It looks we have already addressed a similar case #149 closed by #151. Sadly to say, it's not documented in the user guide. Could you please check if the above matches your needs?

@carpenterm
Copy link
Contributor Author

@bsctl Thanks for the quick response. If there is an existing feature that provides this capability we are happy to try it - any chance you could provide some brief pointers on how to get it working so we don’t have to reverse engineer it?

@bsctl
Copy link
Member

bsctl commented Feb 16, 2022

Sure, it would be nice to get help from core maintainers @prometherion and @MaxFedotov.

@MaxFedotov
Copy link
Collaborator

MaxFedotov commented Feb 16, 2022

Hi @carpenterm.

Currently, namespaces can be viewed by everyone, who has a corresponding rolebinding created in it.
With other cluster-level resources (like nodes), things are a bit more complicated, because there is no way to filter our which nodes should be shown for a user, who have a rolebinding created only in a single namespace inside a tenant (and obviously which actions he should be able to do with node - get, edit, etc..?)

One option can be to add a flag for capsule-proxy, which will allow for users who have a rolebinding created in one of the tenant namespaces to be able to perform only GET operation for cluster-scoped objets.

Another one is to move this configuration to the tenant level, as you proposed.

@carpenterm
Copy link
Contributor Author

Thanks @MaxFedotov. It looks like this was added in the latest release and we aren't using that yet so we'll test v0.2.0. Listing namespaces definitely makes this significantly less problematic but we do support listing nodes in our current (non-Capsule) service so we will need to solve the node listing issue before we can move to production with Capsule.

It would be nice to have a way of defining permissions that is consistent across any of the cluster-scoped objects Capsule allows owners to list, since regular users do need to see these in their day jobs. The biggest one is nodes, this is heavily used by our developers today. Since you are able to restrict storage classes, ingress classes, etc by tenant then the users vs just the owners will need to know what these restricted types are, else restricting them is problematic in the first place since it then becomes a communication/documentation requirement on the owners to inform the users of the types in some offline manner.

It would be fine to have a flag that enables this, although I think get and list would be desirable. I would imagine longer term it makes sense to have a users[].proxySettings field for those edge cases where folks need to have different permissions for different kinds - I guess the use cases would be similar to those that drove you to add owners[].proxySettings in the first place. This way you wouldn't be baking broad decisions about policy into a single flag.

@prometherion prometherion added documentation Improvements or additions to documentation enhancement New feature or request labels Feb 16, 2022
@prometherion
Copy link
Member

Hey @carpenterm, thanks for the interest in Capsule, we really appreciated it!

It seems we're not documenting the Namespace listing feature offered to Tenant users: we should track this down and fix it in a different issue I'm going to open on the Capsule repository, would be great having help from you, @carpenterm!

The feature is absolutely interesting and valuable, we have to find a way to allow this. Regarding Namespace it was easy, since using the RoleBinding reflector code crafted by @MaxFedotov, we can easily intercept those requests.

It would be fine to have a flag that enables this, although I think get and list would be desirable. I would imagine longer term it makes sense to have a users[].proxySettings field for those edge cases where folks need to have different permissions for different kinds

Although getting the point, I'm not sure we want to drill down Capsule on the user management field: we're totally relying on the Kubernetes one and we don't want to create a yet-another-users-management-tool for Kubernetes.

A possible, and dirty workaround, would be creating fake Role and related RoleBinding for each user, something as follows.

apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRole
metadata:
  name: capsule-tenant-nodes-reader
rules:
- apiGroups:
  - "capsule.clastix.io"
  resources:
  - "nodes"
  verbs:
  - get
  - list
  - watch

This fake ClusterRole could be bound to the Tenant users and shouldn't provide any privilege escalation according to the standard RBAC due to the wrong apiGroups pointing to the Capsule API group.

We don't have any resource named nodes but that would be required since we can use the RoleBindingReflector and, from that, understand if the user is able to list the Tenant nodes.

This could be even applied to all the Tenant namespaces using the additionalRoleBindings specification key.

The big drawback is that the RoleBindingReflector is highly coupled for Namespaces, it should be declined for several other resources.

I'm wondering at the same time if we shouldn't instead address the feature from the /spec/owners standpoint, adding (maybe) an additional setting like allowNamespaceCreation, since I think that in some cases the Tenant users wouldn't be eligible to create new Namespaces, since they're users.

Any opinion on these approaches, @bsctl @MaxFedotov?

@bsctl
Copy link
Member

bsctl commented Feb 17, 2022

Although getting the point, I'm not sure we want to drill down Capsule on the user management field: we're totally relying on the Kubernetes one and we don't want to create a yet-another-users-management-tool for Kubernetes.

+1 on this.

I'm wondering at the same time if we shouldn't instead address the feature from the /spec/owners standpoint, adding (maybe) an additional setting like allowNamespaceCreation, since I think that in some cases the Tenant users wouldn't be eligible to create new Namespaces, since they're users.

Allowing roles and permission definition on the owners spec, it sounds to me as we are rewriting the Kubernetes RBAC. Although the feature is absolutely interesting and valuable, we have to find another way to allow this.

Imho, the fake ClusterRole could be a temporary viable workaround, while we should have seriously to think a redesign of the whole Capsule proxy logic.

@carpenterm
Copy link
Contributor Author

Thanks @prometherion and @bsctl for your comments.

A possible, and dirty workaround, would be creating fake Role and related RoleBinding for each user...

Imho, the fake ClusterRole could be a temporary viable workaround...

I understand wanting to leverage existing functionality to get something out sooner, however, my main issue with the proposal above is that you are saying it is temporary/hacky/dirty. Could we look for something less hacky and hopefully longer term to help those building on top of Capsule?

Another issue is that Tenant Owners will see and be able to edit these roles given they have admin in their namespaces - I view these policies as Platform Owner controls since Platform Owners are the ones who manage the cluster-level resources - not Tenant Owners. Therefore, Tenant Owners shouldn't be able to affect these cluster-level policies. I appreciate this may just be the way we are using Capsule and other approaches may vary.

Allowing roles and permission definition on the owners spec, it sounds to me as we are rewriting the Kubernetes RBAC. Although the feature is absolutely interesting and valuable, we have to find another way to allow this.

If I understand correctly, your main concern with including this in the Tenant spec is including the definition of the users rather than letting access be determined by the Roles and RoleBindings inside the namespaces. Another approach which would address this concern, would be to have a spec.proxySettings field which applies for all users in the Tenant and doesn't contain the users. This would allow the spec.owners[].proxySettings be used to optionally grant additional privileges to owners?

apiVersion: capsule.clastix.io/v1beta1
kind: Tenant
metadata:
  name: oil
spec:
  owners:
  - kind: Group
    name: oil-owners
    proxySettings:
    - kind: Nodes
      operations:
      - List
      - Update
      - Delete
  proxySettings:
  - kind: Nodes
    operations:
    - List
  nodeSelector:
    kubernetes.io/hostname: capsule-gold-qwerty

we should have seriously to think a redesign of the whole Capsule proxy logic

Is this referencing something that is already planned?

@bsctl
Copy link
Member

bsctl commented Feb 21, 2022

Hi @carpenterm let’s to discuss this on slack. Please reach us on socials so we can send an invite for our slack channel.

@bsctl bsctl closed this as completed Feb 21, 2022
@bsctl bsctl reopened this Feb 21, 2022
@prometherion
Copy link
Member

Am I wrong or this feature has been addressed on v0.3.0 release using the ProxySettings CRD?

@oliverbaehler
Copy link
Collaborator

@prometherion yes I think you are right :3! @carpenterm the issue describing the new proxySettings can be found here: #196. Could you check if this feature fulfills your feature needs?

The feature is also document here: https://capsule.clastix.io/docs/general/proxy#tenant-owner-authorization under ProxySetting Use Case

@prometherion
Copy link
Member

Closing due to inactivity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants