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

tenant owner can't impersonate a namespace admin #424

Closed
brightzheng100 opened this issue Sep 18, 2021 · 15 comments
Closed

tenant owner can't impersonate a namespace admin #424

brightzheng100 opened this issue Sep 18, 2021 · 15 comments
Labels
blocked-needs-validation Issue need triage and validation enhancement New feature or request needs-discussion No outline on the feature, discussion is welcome

Comments

@brightzheng100
Copy link
Contributor

Bug description

The tenant owner Alice can't impersonate the namespace admin Joe assigned by her.
Only Cluster Admin can do this as of now.

How to reproduce

By following the docs, the tenant owner Alice assign Joe as the namespace admin:

export KUBECONFIG=alice-oil.kubeconfig

kubectl apply -f - << EOF
apiVersion: rbac.authorization.k8s.io/v1
kind: RoleBinding
metadata:
  labels:
  name: oil-development:admin
  namespace: oil-development
roleRef:
  apiGroup: rbac.authorization.k8s.io
  kind: ClusterRole
  name: admin
subjects:
- apiGroup: rbac.authorization.k8s.io
  kind: User
  name: joe
EOF

But she can't impersonate Joe:

kubectl --as joe --as-group capsule.clastix.io auth can-i create pod -n oil-development
Error from server (Forbidden): users "joe" is forbidden: User "alice" cannot impersonate resource "users" in API group "" at the cluster scope

And only Cluster admin can.

unset KUBECONFIG
kubectl --as joe --as-group capsule.clastix.io auth can-i create pod -n oil-development
yes
kubectl --as joe --as-group capsule.clastix.io auth can-i create pod -n oil-production
no

Expected behavior

The tenant owner should be able to act like a Cluster Admin within the assigned tenant.

Additional context

  • Capsule version: v0.1.0
  • Kubernetes version: v1.21.2
@brightzheng100 brightzheng100 added blocked-needs-validation Issue need triage and validation bug Something isn't working labels Sep 18, 2021
@bsctl
Copy link
Member

bsctl commented Sep 19, 2021

@brightzheng100 Thanks for reporting this issue.

Currently, Capsule operator creates a RoleBinding between the tenant owner identity, e.g. alice user and the regular user-facing ClusterRole admin

$ kubectl get rolebindings -n oil-development
NAME                    ROLE                                    AGE
namespace-deleter       ClusterRole/capsule-namespace-deleter   8d
namespace:admin         ClusterRole/admin                       8d

The User-facing ClusterRole admin does not provide users and groups impersonating but the serviceaccounts

$ kubectl describe ClusterRole/admin
Name:         admin
Labels:       kubernetes.io/bootstrapping=rbac-defaults
Annotations:  rbac.authorization.kubernetes.io/autoupdate: true
PolicyRule:
  Resources                                       Non-Resource URLs  Resource Names  Verbs
  ---------                                       -----------------  --------------  -----
  clusterpolicies.kyverno.io                      []                 []              [*]
  clusterreportchangerequests.kyverno.io          []                 []              [*]
  policies.kyverno.io                             []                 []              [*]
  reportchangerequests.kyverno.io                 []                 []              [*]
  clusterpolicyreports.wgpolicyk8s.io/v1alpha1    []                 []              [*]
  policyreports.wgpolicyk8s.io/v1alpha1           []                 []              [*]
  rolebindings.rbac.authorization.k8s.io          []                 []              [create delete deletecollection get list patch update watch]
  roles.rbac.authorization.k8s.io                 []                 []              [create delete deletecollection get list patch update watch]
  configmaps                                      []                 []              [create delete deletecollection patch update get list watch]
  endpoints                                       []                 []              [create delete deletecollection patch update get list watch]
  persistentvolumeclaims                          []                 []              [create delete deletecollection patch update get list watch]
  pods                                            []                 []              [create delete deletecollection patch update get list watch]
  replicationcontrollers/scale                    []                 []              [create delete deletecollection patch update get list watch]
  replicationcontrollers                          []                 []              [create delete deletecollection patch update get list watch]
  services                                        []                 []              [create delete deletecollection patch update get list watch]
  daemonsets.apps                                 []                 []              [create delete deletecollection patch update get list watch]
  deployments.apps/scale                          []                 []              [create delete deletecollection patch update get list watch]
  deployments.apps                                []                 []              [create delete deletecollection patch update get list watch]
  replicasets.apps/scale                          []                 []              [create delete deletecollection patch update get list watch]
  replicasets.apps                                []                 []              [create delete deletecollection patch update get list watch]
  statefulsets.apps/scale                         []                 []              [create delete deletecollection patch update get list watch]
  statefulsets.apps                               []                 []              [create delete deletecollection patch update get list watch]
  horizontalpodautoscalers.autoscaling            []                 []              [create delete deletecollection patch update get list watch]
  cronjobs.batch                                  []                 []              [create delete deletecollection patch update get list watch]
  jobs.batch                                      []                 []              [create delete deletecollection patch update get list watch]
  daemonsets.extensions                           []                 []              [create delete deletecollection patch update get list watch]
  deployments.extensions/scale                    []                 []              [create delete deletecollection patch update get list watch]
  deployments.extensions                          []                 []              [create delete deletecollection patch update get list watch]
  ingresses.extensions                            []                 []              [create delete deletecollection patch update get list watch]
  networkpolicies.extensions                      []                 []              [create delete deletecollection patch update get list watch]
  replicasets.extensions/scale                    []                 []              [create delete deletecollection patch update get list watch]
  replicasets.extensions                          []                 []              [create delete deletecollection patch update get list watch]
  replicationcontrollers.extensions/scale         []                 []              [create delete deletecollection patch update get list watch]
  ingresses.networking.k8s.io                     []                 []              [create delete deletecollection patch update get list watch]
  networkpolicies.networking.k8s.io               []                 []              [create delete deletecollection patch update get list watch]
  poddisruptionbudgets.policy                     []                 []              [create delete deletecollection patch update get list watch]
  deployments.apps/rollback                       []                 []              [create delete deletecollection patch update]
  deployments.extensions/rollback                 []                 []              [create delete deletecollection patch update]
  localsubjectaccessreviews.authorization.k8s.io  []                 []              [create]
  pods/attach                                     []                 []              [get list watch create delete deletecollection patch update]
  pods/exec                                       []                 []              [get list watch create delete deletecollection patch update]
  pods/portforward                                []                 []              [get list watch create delete deletecollection patch update]
  pods/proxy                                      []                 []              [get list watch create delete deletecollection patch update]
  secrets                                         []                 []              [get list watch create delete deletecollection patch update]
  services/proxy                                  []                 []              [get list watch create delete deletecollection patch update]
  bindings                                        []                 []              [get list watch]
  events                                          []                 []              [get list watch]
  limitranges                                     []                 []              [get list watch]
  namespaces/status                               []                 []              [get list watch]
  namespaces                                      []                 []              [get list watch]
  persistentvolumeclaims/status                   []                 []              [get list watch]
  pods/log                                        []                 []              [get list watch]
  pods/status                                     []                 []              [get list watch]
  replicationcontrollers/status                   []                 []              [get list watch]
  resourcequotas/status                           []                 []              [get list watch]
  resourcequotas                                  []                 []              [get list watch]
  services/status                                 []                 []              [get list watch]
  controllerrevisions.apps                        []                 []              [get list watch]
  daemonsets.apps/status                          []                 []              [get list watch]
  deployments.apps/status                         []                 []              [get list watch]
  replicasets.apps/status                         []                 []              [get list watch]
  statefulsets.apps/status                        []                 []              [get list watch]
  horizontalpodautoscalers.autoscaling/status     []                 []              [get list watch]
  cronjobs.batch/status                           []                 []              [get list watch]
  jobs.batch/status                               []                 []              [get list watch]
  daemonsets.extensions/status                    []                 []              [get list watch]
  deployments.extensions/status                   []                 []              [get list watch]
  ingresses.extensions/status                     []                 []              [get list watch]
  replicasets.extensions/status                   []                 []              [get list watch]
  nodes.metrics.k8s.io                            []                 []              [get list watch]
  pods.metrics.k8s.io                             []                 []              [get list watch]
  ingresses.networking.k8s.io/status              []                 []              [get list watch]
  poddisruptionbudgets.policy/status              []                 []              [get list watch]
  serviceaccounts                                 []                 []              [impersonate create delete deletecollection patch update get list watch]

So impersonate users is not allowed to tenant owners. In the current design, the tenant owners act as admin of the namespaces belonging to the tenant and they cannot impersonate outside the namespaces they own.

On the other side, giving impersonate users and groups permission can lead to a security issue since the tenant owner can use this capability to access resources not allowed to.

For example, if we provide as cluster admin

kubectl apply -f - << EOF
apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRole
metadata:
  name: impersonator
rules:
- apiGroups: [""]
  resources: ["users"]
  verbs: ["impersonate"]
EOF

kubectl apply -f - << EOF
apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRoleBinding
metadata:
  name: impersonator
roleRef:
  apiGroup: rbac.authorization.k8s.io
  kind: ClusterRole
  name: impersonator
subjects:
- apiGroup: rbac.authorization.k8s.io
  kind: Group
  name: capsule.clastix.io
EOF

the tenant owner alice can impersonate the user joe

alice@cmp:~$ kubectl --as joe get sa -n oil-development
NAME      SECRETS   AGE
default   1         8d

At same time, she can impersonate any other user

alice@cmp:~$ kubectl --as bob get sa -n water-development
NAME      SECRETS   AGE
default   1         8d

Any idea how to solve this in a safe way would be really appreciated.

@bsctl bsctl added enhancement New feature or request needs-discussion No outline on the feature, discussion is welcome and removed bug Something isn't working labels Sep 19, 2021
@brightzheng100
Copy link
Contributor Author

I don't know the rationale behind the ClusterRole/admin design in terms of permissions, but that makes some sense too.

As we're assuming the Tenant Owner works like the Cluster Admin in the assigned namespaces, so this security issue shouldn't be a big concern just like we shouldn't challenge why Cluster Admin can impersonate anyone within the assigned cluster.

So this may raise a potential requirement, where a custom cluster role, instead of the in-built admin may be used while creating the Tenant for the assigned Tenant Owners.

In this case, the design might be slightly changed to something like:

type OwnerSpec struct {
	Name string `json:"name"`
	Kind Kind   `json:"kind"`
        // +kubebuilder:default=admin
	Role string `json:"role,omitempty"`
}

Not sure whether this makes any sense, thanks!

@bsctl
Copy link
Member

bsctl commented Sep 20, 2021

@brightzheng100 thanks for your suggestion, definitively something we can consider for next releases.

@prometherion
Copy link
Member

Before considering this feature request for the next release, I'd like to chime in on this topic.

The goal of Capsule is to provide the ability to provision in a fashioned self-service way Namespace without the help of external parties, such as opening tickets, sending emails, and so on. The users will be granted admin capabilities on the created Namespace resources using the ClusterRole resource named admin with capabilities @bsctl already shared in the discussion.

The imperonsate feature is absolutely a nice feature that I used several times to understand what was not going on with my applications, and it's convenient. At the same time, it's really important to understand that _with great power comes great responsibility: impersonating another user in a multi-tenant scenario, in most of the scenarios, is a privilege escalation since it allows to bypass any permission set by RBAC boundaries.

Additionally, we have always to remember that Kubernetes API hasn't been designed with a User first approach, a fact that implies most of the limitations that users are complaining about, like getting the list of owner Namespaces: the rationale is that these resources are cluster scoped and the ACL is based on the resource objects rather than the user, as most of the web applications are doing. The same applies to the user and groups resources, although these are just headers and not resources even tho backed by RBAC, that's another story.

With that said, I just wanted to explain why we (and Kubernetes with its admin ClusterRole) are not providing the impersonate verb, now it's time to understand how to provide this enhancement and I'd say we got two options: working on Capsule code base or, rather, working on capsule-proxy.

Working on Capsule code base

The operator could start watching for any RoleBinding with a kind subject equals to User (or even Group?) on the Tenant namespaces and create a ClusterRole as follows.

apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRole
metadata:
  name: impersonator-${tenant_name}
rules:
- apiGroups:
  - ""
  resources:
  - groups
  verbs:
  - impersonate
- apiGroups:
  - ""
  resourceNames:
  - joe
  resources:
  - users
  verbs:
  - impersonate

with a following ClusterRoleBinding as the following one.

apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRoleBinding
metadata:
  name: impersonator-${tenant_name}-${tenant_owner}
roleRef:
  apiGroup: rbac.authorization.k8s.io
  kind: ClusterRole
  name: impersonator-${tenant_name}
subjects:
- apiGroup: rbac.authorization.k8s.io
  kind: User
  name: ${tenant_owner}

All the resources must be attached to the Tenant using the ControllerReference to take advantage of the reconciliation and the GC, too.

I tested this and it seems working since we're targeting the subjects using the resourceNames filed, limiting the scope of the permission just to a specific set of users.

Pros

  • It's just a change on the code-base, so could be shipped with a patch release
  • Reusing the Kubernetes capabilities, no reinventing the wheel, it's just automation

Cons

  • Sprawl of ClusterRoleBinding when dealing with multiple Tenant resources owning multiple Namespaces
  • Higher memory usage on Capsule side due to watching
  • Expected higher latency requested by reconciliation for said resources (more stuff to do)

Working with capsule-proxy

Our proxy is providing a smart approach to overcome Kubernetes limitations and could be playing a role also here, as we're doing with the listing of Namespaces, Ingress and PodPriority and Storage Class resources.

We would just pipe the request in a middleware matching the control of the impersonate headers and check if the targeted user got a RoleBinding matching the criteria: if that's the case, proxy to the upstream API Server updating the request with the right fields, otherwise do nothing and let Kubernetes deal with the failure since the user is not entitled to impersonate a user.

Pros

  • No need to touch the Capsule code base
  • Overcome Kubernetes limitations as we're doing with other resources in a separate project

Cons

  • Addressing this issue/feature would require capsule-proxy, although it's highly recommended.

Said so, we have to pick a choice, and would be great to have as much feedback as possible from the community in order to start a discussion and find the optimal solution.

@MaxFedotov please, join the discussion, your opinion is highly appreciated.

@brightzheng100
Copy link
Contributor Author

I personally think that having the flexibility of setting up a ClusterRole for a Tenant, which defaults to predefined admin, is already good enough as it can cater for more similar requirements around "can I assign xxx permissions to some specific Tenants?".
As you know, such a change is easy and 100% backwards compatible.

The capsule-proxy is really a good design to complement what K8s upstream is lacking in terms of having part of the permissions, like a verb with some specified namespaces**, or maybe some CRDs within some namespaces if possible.

@MaxFedotov
Copy link
Collaborator

MaxFedotov commented Sep 23, 2021

@prometherion
The problem I was thinking about is the possibility to make a security issue with impersonating.
Let's imagine that user Joe had permissions in namespaces in two tenants:

  • oil-foo (owner of tenant oil is Alice)
  • gas-bar (owner of tenant gas is Bob).

If Alice will have permissions to impersonate Joe, will she be able to access gas-bar namespace?

@prometherion
Copy link
Member

prometherion commented Sep 23, 2021

If Alice will have permissions to impersonate Joe, will she be able to access gas-bar namespace?

Hadn't tested yet, it seems doable, tho, and this confirms my concerns regarding the privilege esclation.

I think we have to investigate addressing this on capsule-proxy if we don't have any other option.

@bsctl
Copy link
Member

bsctl commented Sep 23, 2021

I think we have to investigate addressing this on capsule-proxy if we don't have any other option.

In order to impersonate, we have to give wider permission and using the capsule-proxy for this job makes sense to me as we done for kubectl get ns.

NB
In the current implementation, the tenant admin acts as namespace admin for all the namespaces of the tenant and that's coherent with the model we designed, so I turned this issue into a feature request.

@prometherion
Copy link
Member

With that said, we have to track this down on capsule-proxy repository, I'll link it once created.

@prometherion
Copy link
Member

The capsule-proxy is really a good design to complement what K8s upstream is lacking in terms of having part of the permissions, like a verb with some specified namespaces**, or maybe some CRDs within some namespaces if possible.

Would be better to move the discussion to the proper repository, although I can briefly answer your questions here.

We wanted to keep the same look and feel, and developer experience too, of native kubernetes, without adding any additional extra thing like a new path or verb.

Allowing tenants to install CRDs, rather, is another issue we cannot overcome with the soft multi-tenancy model implemented by Capsule since it's a hard limitation put in place by Kubernetes and at the current state just a separated etcd store would be able to provide this.

@brightzheng100
Copy link
Contributor Author

Allowing tenants to install CRDs, rather, is another issue we cannot overcome with the soft multi-tenancy model implemented by Capsule since it's a hard limitation put in place by Kubernetes and at the current state just a separated etcd store would be able to provide this.

I guess that's what Virtual Cluster within multi-tenancy WG is trying to solve -- a dedicated control plane for each tenant -- which definitely is a much heavier solution.
Not sure whether there is a lighter weight solution in the community. But yeah, we may discuss this in the capsule-proxy where might put this into consideration, if possible

@bsctl
Copy link
Member

bsctl commented Sep 25, 2021

@brightzheng100 it's quite accepted into the community two types of multi-tenancy for Kubernetes:

  • soft multi-tenancy where the control plane is shared between all tenants
  • harder multi-tenancy where each tenant has its own control-plane

The first one is addressed very well by Capsule. For the second one, may you can be interested into a new project we're working on. It's called Kamaji. The project is still in its infancy and comments and contributions are really welcomed. Would you like to help us?

@brightzheng100
Copy link
Contributor Author

Yeah and this is a good blog there: Three Tenancy Models For Kubernetes

Not sure how Karmaji differentiates itself from:

@prometherion
Copy link
Member

I don't know each of one the projects you mentioned, so my tl;dr; could be highly wrong.

  • Virtual Clusters is a compatible Cluster API provider to install pod-based Kubernetes clusters (API server, scheduler, controller-manager, and etcd).
  • Karmada looks like a federation of clusters, but not entirely sure.
  • Open Cluster Management looks like an entire toolset to manage many Kubernetes instances using an open format

If I made some mistakes on these, please, let me know!

Said so, Kamaji takes a different approach, using a shared and multi-tenant etcd cluster, in which each tenant is used by a set of pod-based Kubernetes Control Plane.

If you got more concerns and inquiries I'd say to move the discussion to the Kamaji repository to keep sticking the discussion regarding the feature request.

@prometherion
Copy link
Member

We encountered this requirement while working on #528 and we're addressing it in Capsule Proxy with projectcapsule/capsule-proxy#216.

Going to close this, let's move an eventual discussion there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked-needs-validation Issue need triage and validation enhancement New feature or request needs-discussion No outline on the feature, discussion is welcome
Projects
None yet
Development

No branches or pull requests

4 participants