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

Minimal rbac to view and reconcile resources in UI #3702

Open
adberger opened this issue May 15, 2023 · 17 comments · May be fixed by #4666
Open

Minimal rbac to view and reconcile resources in UI #3702

adberger opened this issue May 15, 2023 · 17 comments · May be fixed by #4666
Labels
area/ui Issues that require front-end work priority_medium Items we want to complete in the next 60 days type/enhancement New feature or request type/spike

Comments

@adberger
Copy link

Hi there

We are currently trying to limit the flux resources which a specific user/group can see & use (e.g. sync).
We tried to apply the following role, but the user doesn't see any resources matching the names specified.

apiVersion: rbac.authorization.k8s.io/v1
kind: Role
metadata:
  name: example
  namespace: tenant-1
- apiGroups:
  - kustomize.toolkit.fluxcd.io
  resourceNames:
  - example-kustomization
  resources:
  - kustomizations
  verbs:
  - get
  - list
  - watch
  - patch
- apiGroups:
  - source.toolkit.fluxcd.io
  resourceNames:
  - example-gitrepository
  resources:
  - gitrepositories
  verbs:
  - get
  - list
  - watch
  - patch

The following requests over kubectl work:

kubectl get kustomizations.kustomize.toolkit.fluxcd.io -n tenant-1 example-kustomization
kubectl get gitrepositories.source.toolkit.fluxcd.io -n tenant-1 example-gitrepository

Additionally setting

  # Read access for all other Kubernetes objects
  - apiGroups: ["*"]
    resources: ["*"]
    verbs: [ "get", "list", "watch" ]

according to the docs, the user can see "too much".

@jpellizzari
Copy link
Contributor

@adberger Thanks for the report. This is a known issue, and one that cropped up for me again in a different form this week.

The root cause of this is how we determine whether a user can "see" a namespace:

func (sc simpleChecker) FilterAccessibleNamespaces(ctx context.Context, auth typedauth.AuthorizationV1Interface, namespaces []corev1.Namespace) ([]corev1.Namespace, error) {

Kubernetes doesn't have a concept of "yes you can see this namespace", because it only knows about verbs against kinds in a namespace, so we layered on the access logic to convert it to a bool.

We aren't handling the resourceName case at the moment.

@adberger
Copy link
Author

@jpellizzari I see, that's unfortunate.

We now do it like this for the moment:

---
apiVersion: rbac.authorization.k8s.io/v1
kind: Role
metadata:
  name: example
  namespace: flux-system
rules:
- apiGroups: [""]
  resources: ["pods", "secrets", "events"]
  verbs: ["get", "list"]
- apiGroups: ["apps"]
  resources: ["deployments", "replicasets"]
  verbs: ["get", "list"]
- apiGroups: ["kustomize.toolkit.fluxcd.io"]
  resources: ["kustomizations"]
  verbs: ["get", "list"]
- apiGroups: ["kustomize.toolkit.fluxcd.io"]
  resources: ["kustomizations"]
  resourceNames:
    - exampleResource
  verbs: ["patch"]
- apiGroups: ["helm.toolkit.fluxcd.io"]
  resources: ["helmreleases"]
  verbs: ["get", "list"]
- apiGroups: ["source.toolkit.fluxcd.io"]
  resources: ["buckets", "helmcharts", "gitrepositories", "helmrepositories", "ocirepositories"]
  verbs: ["get", "list"]
- apiGroups: ["source.toolkit.fluxcd.io"]
  resources: ["gitrepositories"]
  resourceNames:
    - exampleResource
  verbs: ["patch"]
- apiGroups: [""]
  resources: ["events"]
  verbs: ["get", "list", "watch"]
---
apiVersion: rbac.authorization.k8s.io/v1
kind: RoleBinding
metadata:
  name: example
  namespace: flux-system
subjects:
- kind: Group
  name: groupXY
  apiGroup: rbac.authorization.k8s.io
roleRef:
  kind: Role
  name: example
  apiGroup: rbac.authorization.k8s.io

The users can see everything, but only sync their resources.

@yiannistri
Copy link
Contributor

I'm afraid we need more time to test this change.

@squaremo
Copy link
Contributor

I think the problem in the original report is that Weave GitOps wants to be able to see groups and resources in addition to "sources.toolkit.fluxcd.io" and "kustomization.toolkit.fluxcd.io" (the requirements are expressed as rules in https://github.com/weaveworks/weave-gitops/blob/main/core/nsaccess/nsaccess.go). Having resourceNames in a rule doesn't cause problems -- it's present in the example of RBAC that does work.

@adberger
Copy link
Author

I think the problem in the original report is that Weave GitOps wants to be able to see groups and resources in addition to "sources.toolkit.fluxcd.io" and "kustomization.toolkit.fluxcd.io" (the requirements are expressed as rules in https://github.com/weaveworks/weave-gitops/blob/main/core/nsaccess/nsaccess.go). Having resourceNames in a rule doesn't cause problems -- it's present in the example of RBAC that does work.

Sure, my provided example works, but with this configured, every user see every flux resource of a namespace in the UI (which is not intended) but can only sync their resources (which is intended).

I would like to specify an rbac like this:

---
apiVersion: rbac.authorization.k8s.io/v1
kind: Role
metadata:
  name: example
  namespace: flux-system
rules:
- apiGroups: [""]
  resources: ["pods", "secrets", "events"]
  verbs: ["get", "list"]
- apiGroups: ["apps"]
  resources: ["deployments", "replicasets"]
  verbs: ["get", "list"]
- apiGroups: ["kustomize.toolkit.fluxcd.io"]
  resources: ["kustomizations"]
  resourceNames:
    - exampleResource
  verbs: ["get", "list", "patch]
- apiGroups: ["helm.toolkit.fluxcd.io"]
  resources: ["helmreleases"]
  verbs: ["get", "list"]
- apiGroups: ["source.toolkit.fluxcd.io"]
  resources: ["buckets", "helmcharts", "gitrepositories", "helmrepositories", "ocirepositories"]
  resourceNames:
    - exampleResource
  verbs: ["get", "list", "patch"]
- apiGroups: [""]
  resources: ["events"]
  verbs: ["get", "list", "watch"]
---
apiVersion: rbac.authorization.k8s.io/v1
kind: RoleBinding
metadata:
  name: example
  namespace: flux-system
subjects:
- kind: Group
  name: groupXY
  apiGroup: rbac.authorization.k8s.io
roleRef:
  kind: Role
  name: example
  apiGroup: rbac.authorization.k8s.io

@squaremo squaremo added the type/enhancement New feature or request label Jun 5, 2023
@squaremo
Copy link
Contributor

squaremo commented Jun 5, 2023

every user see every flux resource of a namespace in the UI (which is not intended) but can only sync their resources (which is intended)

Right, the access you need to give people so they can see namespaces in the UI, is (much) more than just ["list", "get"] on some subset of Flux resources in those namespaces.

I can see why having more access makes the display richer (you can display the status of workloads, say), but you ought to be able to see your stuff in the UI if you can access it via RBAC. "Ought" as in: Weave GitOps does not do this, and it would be better if it did.

@yiannistri yiannistri added the priority_medium Items we want to complete in the next 60 days label Jun 5, 2023
@lasomethingsomething lasomethingsomething added the area/ui Issues that require front-end work label Aug 10, 2023
@adberger
Copy link
Author

Any progress on this concern?

@lasomethingsomething
Copy link
Contributor

@bigkevmcd and @foot Can you please take a look at this?

@bigkevmcd
Copy link
Contributor

@LappleApple I guess we could take the original attempt, and improve it by looking for the presence of the named resources (resourceNames) in the checked ns, this would restrict to only namespaces where the named resources are present.

Not sure when we would get a chance to do this tho'

@eloo-abi
Copy link

Hi there,
just to bring this topic up again.

I have check today the RBAC a bit because we want not to grant permissions for "secrets" in general and thus i have check where this is really needed.

I have found a reference here: https://github.com/weaveworks/weave-gitops/blob/main/core/nsaccess/nsaccess.go#L21
This seems to be for the "is a namespace accessible"
But i have not found any real usage of secrets so far..

So i have tested what happens with a "fake" permission here like this:

...
  - apiGroups:
    - ""
    resources:
    - secrets
    resourceNames:
    - some-random-secret-which-must-not-be-available-secret
    verbs:
    - get
    - list

so i grant access to only a single not existing secret (which is like no access) but it looks like this is "fair enough" for the "nsaccess" checker..

So i would suggest to remove the "secrets" from the nsaccess check and also from the minimal rbac if there is really no need for it.

@casibbald
Copy link
Collaborator

@erikgb @mproffitt
Opinions on this?

@erikgb
Copy link
Contributor

erikgb commented Jan 30, 2025

I agree it would be ideal to not grant cluster-wide access to secrets - if possible. Do we have any idea why this RBAC was given in the first place? If it's only to check if a user has access to a namespace, there are better solutions now with SubjectAccessReview: https://kubernetes.io/docs/reference/access-authn-authz/authorization/#checking-api-access.

@erikgb
Copy link
Contributor

erikgb commented Jan 30, 2025

I had a closer look at this now, and I don't understand why cluster-wide read access to secrets is required. The code referred to by @eloo-abi is running with the impersonate client as far as I can reason. It is impossible to see from the Git log why this permission was added in the first place. It's been part of the Helm chart since the beginning.

I would suggest a "feature gate" Helm value to disable the cluster-wide read access to secrets. Then our users can use this toggle to see if something breaks without it. We can eventually change the default after a couple of releases. I/we would also be interested in removing this permission, so I would enable this new toggle once available in a release. Does this sound like a good start @eloo-abi?

I can prepare a PR to make it even clearer what I mean.

@erikgb
Copy link
Contributor

erikgb commented Jan 30, 2025

I just opened #4659. PTAL!

@eloo-abi
Copy link

@erikgb yes this sounds good to disable that feature..

but i guess your PR will not solve the main problem as the nsaccess will still check for the permissions to secrets..
so if a user has no GET/LIST secrets permissions the UI will still be empty because the UI thinks the user has no access to the namespace.

so its more important to remove the secrets resource from the nsaccess code

@erikgb
Copy link
Contributor

erikgb commented Jan 31, 2025

but i guess your PR will not solve the main problem as the nsaccess will still check for the permissions to secrets.. so if a user has no GET/LIST secrets permissions the UI will still be empty because the UI thinks the user has no access to the namespace.

so its more important to remove the secrets resource from the nsaccess code

Yes, we have to do more in this area to fully fix the problem. I think the whole nsaccess code should be reviewed and improved. Since ww-gitops impersonates the user, I think this check could be a lot more lightweight - since the check is not authorative. Checking RBAC to read a named namespace should be sufficient IMO, but we need to think/work more on this before performing any changes.

@erikgb
Copy link
Contributor

erikgb commented Feb 1, 2025

I just opened #4666 to address this issue. Please take a look an tell me what you think!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ui Issues that require front-end work priority_medium Items we want to complete in the next 60 days type/enhancement New feature or request type/spike
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants