fluxcd / flux2

Open and extensible continuous delivery solution for Kubernetes. Powered by GitOps Toolkit.
https://fluxcd.io
Apache License 2.0
6.43k stars 595 forks source link

BUG:Unnecessary RBAC permissions #4819

Closed Yseona closed 4 months ago

Yseona commented 4 months ago

Describe the bug

The bug is that the Deployment kustomize-controller, image-automation-controller, helm-controller and Daemonsets source-controller, notification-controller, image-reflector-controller in the charts have too much RBAC permissions than they need. The service account of these deployments and daemonsets are bound to the same clusterrole crd-controller(install.yaml), so that they have the following unnecessary permissions:

After reading the source code of the images (including fluxcd/helm-controller, fluxcd/image-automation-controller, etc), I didn't find any Kubernetes API usages using these permissions in the source code of . Besides, some of these unused permissions may have potential risks. For example, if malicious users gain control of a Kubernetes node running a source-controller pod, they can use the create deployments permission to create privileged containers with malicious container images.

Steps to reproduce

Use install.yaml with default values.

Expected behavior

Therefore, for security reasons, I suggest checking these permissions to determine if they are truly unnecessary. If they are, the issue should be fixed by removing the unnecessary permission or other feasible methods.

Screenshots and recordings

No response

OS / Distro

N/A

Flux version

N/A

Flux check

N/A

Git provider

No response

Container Registry provider

No response

Additional context

No response

Code of Conduct

kingdonb commented 4 months ago

kustomize-controller: delete verb of the daemonsets resource (ClusterRole)

The kustomize controller can create daemonsets, or any Kubernetes resource if they are in the manifests applied. If spec.prune is enabled, then it must also be able to delete them. This is inclusive of cluster-wide non-namespaced resources like CustomResourceDefinition.

The helm controller similarly can create and delete unbounded types of resources. It is responsible for managing the whole lifecycle of every kind of resource, anything that you can put into a Helm chart. Those are the applier kinds.

I will happily review the permissions with you, but I have questions about how you came to the conclusions. I'm using the rolesum kubectl krew plugin to see the permissions of each service account in the flux-system namespace. The notification-controller for example doesn't appear to have delete of Deployment:

% k rolesum -n flux-system notification-controller
ServiceAccount: flux-system/notification-controller
Secrets:

Policies:

• [CRB] */crd-controller-flux-system ⟶  [CR] */crd-controller-flux-system
  Resource                          Name  Exclude  Verbs  G L W C U P D DC
  *.helm.toolkit.fluxcd.io          [*]     [-]     [-]   ✔ ✔ ✔ ✔ ✔ ✔ ✔ ✔
  *.image.toolkit.fluxcd.io         [*]     [-]     [-]   ✔ ✔ ✔ ✔ ✔ ✔ ✔ ✔
  *.kustomize.toolkit.fluxcd.io     [*]     [-]     [-]   ✔ ✔ ✔ ✔ ✔ ✔ ✔ ✔
  *.notification.toolkit.fluxcd.io  [*]     [-]     [-]   ✔ ✔ ✔ ✔ ✔ ✔ ✔ ✔
  *.source.toolkit.fluxcd.io        [*]     [-]     [-]   ✔ ✔ ✔ ✔ ✔ ✔ ✔ ✔
  configmaps                        [*]     [-]     [-]   ✔ ✔ ✔ ✔ ✔ ✔ ✔ ✖
  configmaps/status                 [*]     [-]     [-]   ✔ ✖ ✖ ✖ ✔ ✔ ✖ ✖
  events                            [*]     [-]     [-]   ✖ ✖ ✖ ✔ ✖ ✔ ✖ ✖
  leases.coordination.k8s.io        [*]     [-]     [-]   ✔ ✔ ✔ ✔ ✔ ✔ ✔ ✖
  namespaces                        [*]     [-]     [-]   ✔ ✔ ✔ ✖ ✖ ✖ ✖ ✖
  secrets                           [*]     [-]     [-]   ✔ ✔ ✔ ✖ ✖ ✖ ✖ ✖
  serviceaccounts                   [*]     [-]     [-]   ✔ ✔ ✔ ✖ ✖ ✖ ✖ ✖

The CRD-controller role is perhaps too broad - I don't know why it was decided to give all of the controllers access to all the CRD in the fluxcd.io namespace. But permissions are in most cases quite a bit more limited than you suggest. (Can you explain how you came to the conclusion that notification-controller can delete deployments?)

Here's the "god-mode" clusterrolebinding to worry about:

• [CRB] */cluster-reconciler-flux-system ⟶  [CR] */cluster-admin
  Resource  Name  Exclude  Verbs  G L W C U P D DC
  *.*       [*]     [-]     [-]   ✔ ✔ ✔ ✔ ✔ ✔ ✔ ✔

This one above is assigned to the kustomize-controller and helm-controller, it is necessary and intentional since they are both flux appliers and they might need to apply and garbage collect any arbitrary type of resource including DaemonSets and Namespaces, CRDs, etc which are non-namespaced. They are definitely OP with privileges, because they might need to manage any resource on the cluster. The others are more limited. They (image-automation, source, notification, image-reflector) should all be limited to only what crd-controller can do, which is still quite a lot but does not include some of the things you mentioned that would be concerning.

If you can exec pods in the flux-system namespace and bind arbitrary service accounts, the game is up. If you can exec in a pod with one of these god-mode service accounts (an applier like kustomize-controller or helm-controller) then the game is also up.

I don't think we consider either of those as attack modes, because of the posture that flux-system namespace, widely, is the high-security area that should be protected from tenant users; when it is properly isolated the cluster users who are not super-admins cannot abuse the RBAC rules in that namespace (and cluster-admins who already had this power are all trusted not to abuse it)

I am not on the security team, and we have undergone multiple security audits which you can review the results of, I think they must have considered this question before. I don't want to go into too much detail as security disclosures should be done privately per the process outlined here: https://fluxcd.io/security/#report-a-vulnerability

If you think you have a way to exploit Flux as it is currently deployed, please do follow up, but perhaps someone from the security team can weigh in here with a bit more authority. I don't want to quash your report as I frankly don't have the experience or the authority to do all that, but I don't think you're describing anything that we would consider a vulnerability.

stefanprodan commented 4 months ago

The clusterrole crd-controller does not have such permissions, see https://github.com/fluxcd/flux2/blob/main/manifests/rbac/controller.yaml

Yseona commented 4 months ago

Sorry, I made a big mistake when I was writing this Issue, in fact, by analyzing the source code of these mirrors, I found that there are no API calls in the source code that require the use of list secrets permissions. So I was wondering if list secrets permissions are not necessary for these deployments and daemonsets?