fix: simplify checker for access to namespaces

[erikgb]

Closes #3702

What changed?

This PR greatly simplifies the check for access to namespace. If/when this is merged:

• A user with permission to list namespaces will see all namespaces.
• A user with permission to get configmaps in the namespace will include the namespace in the list the user is allowed to see. We could probably use any namespace resource for this check, but configmaps was selected mainly because all kube namespaces contains the kube-root-ca.crt configmap.

All the access to kube APIs impersonates the logged-in user in weave-gitops, so this should be a safe change. We might need to add additional RBAC checks in UI to avoid enabling features the logged-in user doesn't have access to, which will result in an error if used.

For now, I have left the old checker as "dead" code, including tests. Please let me know if I should delete it (we probably should). I have now replaced the existing code.

Why was this change made?

The current logic appears complex, hard to maintain, and seems somehow disconnected from the fact that weave-gitops impersonate users. The current check also requires that a user has read access to secrets - which cannot be required for all use of weave-gitops, ref. #3702. If some features require RBAC to secrets, we should instead guard those features with an additional SelfSubjectAccessReview/SelfSubjectRulesReview check.

How was this change implemented?

How did you validate the change?

I have made tests unit testing the new checker. It's challenging to make my local workstation environment run weve-gitops, so I would appreciate some help in verifying that this change doesn't totally break the UI with errors.

Release notes

Documentation Changes

[mproffitt]

For now, I have left the old checker as "dead" code

In general I would prefer not to leave dead code lying around and as long as this code has no further use, I would opt for removing it. We can always recover it from history if we decide some part of it is required in the future.

[erikgb]

For now, I have left the old checker as "dead" code

In general I would prefer not to leave dead code lying around and as long as this code has no further use, I would opt for removing it. We can always recover it from history if we decide some part of it is required in the future.

I agree @mproffitt. Done.

[eloo-abi]

@erikgb thanks for opening this PR.
but i'm curious why you try to check for configmaps to see if a user has access to a namespace?

as far as i can see configmaps are not used anyway in the gitops-ui..
so to have least privileges for the gitops-ui user we also do not plan to grant access to configsmaps..
that would mean we are in the same issue again :D

maybe it would make to check for a resource which is actually used by the gitops ui..
so i would suggest to check maybe for helmrelease?

[erikgb]

@erikgb thanks for opening this PR. but i'm curious why you try to check for configmaps to see if a user has access to a namespace?

as far as i can see configmaps are not used anyway in the gitops-ui.. so to have least privileges for the gitops-ui user we also do not plan to grant access to configsmaps.. that would mean we are in the same issue again :D

To me, this check is about checking access to the namespace. I wish Kubernetes had something to provide in this area, but it seems like they left multi-tenancy to the downstream distributions. I selected configmaps because the kube-root-ca.crt configmap is a Kubernetes thing. Would it be better if I changed the check to require access to this specific configmap? My idea was to build this custom namespace access check around the Kubernetes default user-facing roles.

maybe it would make to check for a resource which is actually used by the gitops ui.. so i would suggest to check maybe for helmrelease?

I was hoping to avoid the discussion about which Flux resource is more important here. 😉 We don't use helmreleases at all in our clusters and rely only on kustomizations. As noted in the PR description, ww-gitops should impersonate the user in all calls to the api-server, so the UI should either error or check more concrete RBAC before enabling a feature to the user (better).

[eloo-abi]

Would it be better if I changed the check to require access to this specific configmap? My idea was to build this custom namespace access check around the Kubernetes default user-facing roles.

i guess no.. because if the user has no access to configmaps at all (which is not necessary for this app at all) the user will not see anything..

I was hoping to avoid the discussion about which Flux resource is more important here. 😉 We don't use helmreleases at all in our clusters and rely only on kustomizations. As noted in the PR description, ww-gitops should impersonate the user in all calls to the api-server, so the UI should either error or check more concrete RBAC before enabling a feature to the user (better).

also a good point..
so maybe it would be the best to check if the user has access to ANY namespaced resource from flux?

[erikgb]

Would it be better if I changed the check to require access to this specific configmap? My idea was to build this custom namespace access check around the Kubernetes default user-facing roles.

i guess no.. because if the user has no access to configmaps at all (which is not necessary for this app at all) the user will not see anything..

But isn't access to configmaps required to work with Flux? Are you putting all config directly into the Flux resources?

I was hoping to avoid the discussion about which Flux resource is more important here. 😉 We don't use helmreleases at all in our clusters and rely only on kustomizations. As noted in the PR description, ww-gitops should impersonate the user in all calls to the api-server, so the UI should either error or check more concrete RBAC before enabling a feature to the user (better).

also a good point.. so maybe it would be the best to check if the user has access to ANY namespaced resource from flux?

That is a much heavier check, and would be equivalent to what the current checker is trying to do - except that the current code uses a non-authorative API (SelfSubjectRulesReview).

[eloo-abi]

But isn't access to configmaps required to work with Flux? Are you putting all config directly into the Flux resources?

in the gitops-ui im not putting any config anywhere :D
sometimes it just read only..

and the actions which can be done in the UI is suspending, resuming and syncing flux resources.. which is done as a patch (annotate) on the flux resources directly.

So as far as i know there is no configmap read when i use the gitops ui..

so IMHO the most relevant check for the UI is that a user can access the resources which are using the UI..
so a slightly adjust version of the old behaviour would match maybe the best.. which could be like this:

	{
		APIGroups: []string{"kustomize.toolkit.fluxcd.io"},
		Resources: []string{"kustomizations"},
		Verbs:     []string{"get", "list"},
	},
	{
		APIGroups: []string{"helm.toolkit.fluxcd.io"},
		Resources: []string{"helmreleases"},
		Verbs:     []string{"get", "list"},
	},
	{
		APIGroups: []string{"source.toolkit.fluxcd.io"},
		Resources: []string{"buckets", "helmcharts", "helmrepositories", "gitrepositories", "ocirepositories"},
		Verbs:     []string{"get", "list"},
	}

and personally it would be enough to have access to at least one of this resources..
because maybe i want only grant users access to see helmreleases and nothing else..

i'm also not sure why a specific check is done anyway.. at least the k8s dashboard not doing this and relies just on the result from the API server and then shows a warning that someone has no access to this resource..

so if all requests are impersonated properly i guess a dedicated check would not be necessary?

[erikgb]

so if all requests are impersonated properly i guess a dedicated check would not be necessary?

The problem is that Kubernetes has NO WAY to list namespaces a user has access to. It's all or nothing.

because maybe i want only grant users access to see helmreleases and nothing else..

IMO this should not be supported by ww-gitops. What if another user would like to do the same, but for kustomizations? 🤔 I would say that some well-defined RBAC defines access to a namespace in ww-gitops. I suggest configmaps - since it is a standard Kubernetes API and is usually accessible to all the Kubernetes default user-facing roles. @casibbald @mproffitt any opinions here? What should define that a user has access to a namespace?

[casibbald]

It could be that we check for any RBAC for a namespace, and if one exists, then we set up rules for it; otherwise, the behaviour remains how it is now. Could such a workflow be satisfactory?
I'm concerned about having it locked down at the start cause new users might be confused when they have no UI access to resources.

[erikgb]

It could be that we check for any RBAC for a namespace

@casibbald Could you please define exactly what this is? User RBAC to get a namespace by name? This works in my cluster, but kubectl emits a warning when doing it. I asked in #sig-auth Slack about this. Please have a look at the thread.

[mproffitt]

I would be wary of trying to implement too heavy a check here and I'd also be wary of trying to parse RBAC to set up rules for the UI which kind of feels more like over-engineering.

I do think that just the check for the configmap might be a little sparse but it is a perfectly valid method of testing that a namespace can be accessed, if a little confusing in this context.

A better check might be to use the top level flux APIs (kustomize, helm, source) and if any of those return true, we read that namespace but that is relatively expensive in terms of API calls so maybe restricting this down to just Kustomize and HelmRelease is valid.

The problem with that "better check" is it fails when a release is deployed across namespace, how do we then show the contents of the deployment when kustomization and helmrelease might be blocked in that namespace?

The answer to that is "we probably don't need to care" - I don't think I've ever come across that scenario when a user is allowed to create flux resources only in a subset of namespaces they are otherwise allowed to deploy in to and it's a safe assumption to make that we can rely on it.

I know in the current UI this leads to a deluge of errors if a user doesn't have access to resources and it is worth looking at how the UI behaves in this context to present a better user experience but I would almost certainly cover this as a separate issue.

I absolutely agree with the comment that we should not try and support what happens if a user is only granted access to just helmreleases or just kustomizations. The UI would be pretty useless in that regard anyway as you'd only be able to see the state of that single resource type and not anything that belongs to it.

TL;DR Whilst I'm happy with the configmap check as being valid, checking for either helmrelease or kustomize may be more relevant without over-engineering.