[fluxv2] check RBAC settings of the currently logged in user when listing packages/repositories

[gfichtenholt]

See related discussion in #4298 (comment)
Flux plugin currently has an issue. For the purpose of this issue, consider 2 "personas" involved:

1. an out-of-band background process running in the context of kubeapps-internal-kubeappsapis service account that watches all helm repositories added/updated/delete in the cluster and sync's them to redis cache

2. a user who logs into the UX dashboard or (gRPC API server) and is wishing to
   list available packages or maybe install a package. This user may have very different
   RBAC settings from those of kubeapps-internal-kubeappsapis service account, which may or may not
   allow them to list a given repository (e.g. by not having read access to the namespace in which the repository exists)

Flux plugin currently ignores (2). That is, any user that can login to the dashboard will
be allowed to read any repository from redis cache thus bypassing the RBAC settings for this user.

This needs to be fixed. As a start, I may want to do something along the lines of what
direct helm plugin does in hasAccessToNamespace() https://github.com/kubeapps/kubeapps/blob/5d4ea934808f8258d44d7edee7f71d5a6c1494e0/cmd/kubeapps-apis/plugins/helm/packages/v1alpha1/server.go#L439. To be clear this is not a 100% accurate solution because the RBAC access may be more fine-grained than the namespace-level but its a start

[gfichtenholt]

So one complication that arose is this:
unlike direct helm, flux allows one to read/use repositories from any namespace, unless explictly configured RBAC says otherwise. So, when the user wants to see the Catalog, i.e. GetAvailablePackageSummaries() is called, the first thing I do is call k8s API server List(...) for all namespaces to get a cluster-wide list of all repos. If the currently logged in user does not have such cluster-wide access, an error is raised 'rpc error: code = PermissionDenied desc = Forbidden to list the HelmRepository '/*' due to '[helmrepositories.source.toolkit.fluxcd.io](http://helmrepositories.source.toolkit.fluxcd.io/) is forbidden: User "system:serviceaccount:test-b3ot:test-caller-ctx-loser" cannot list resource "helmrepositories" in API group "[source.toolkit.fluxcd.io](http://source.toolkit.fluxcd.io/)" at the cluster scope'. This is a problem. My API should not be raising an error, but rather should be returning an empty list.

Also, before someone asks 'why use List() at all if the entries are in redis cache?', let me point out that any given entry in redis is a 'best effort only' kind of deal. It may be swapped out at any time to make room for other entries. In other words, I cannot rely on redis as a "source of truth"

[absoludity]

This is a problem. My API should not be raising an error, but rather should be returning an empty list.

I'm not sure that returning an empty list is the solution, wouldn't that mean that only admins (or users who can read HelmRepositories at the cluster scope) would be able to view a catalog, in any namespace?

Also, before someone asks 'why use List() at all if the entries are in redis cache?', let me point out that any given entry in redis is a 'best effort only' kind of deal. It may be swapped out at any time to make room for other entries. In other words, I cannot rely on redis as a "source of truth"

The auth'd user is asking "what packages are available for me to install". I thought we relied on the cache (whether it's best effort only or not) to respond with the packages available to install? I'm not sure how you could check for HelmRepositories for a user who cannot read at the cluster-scope, other than iterating over the namespaces to which the user has access to gather them (scales linearly with the number of namespaces to which they have access, but that may be small normally for users without the cluster-scoped read).

[gfichtenholt]

Mostly true what you said above (see below)

I am thinking that List() should be executed in the context of kubeapps-internal-kubeappsapis service account and then I manually filter out those repos that the caller doesn't have access to. What do you think?

[gfichtenholt]

we don't have this problem with helm plugin, because its cache 'postgresql' is unbounded. If it had an upper limit on how many entries can be stored, we'd have the same issue with helm

[gfichtenholt]

I'm not sure that returning an empty list is the solution, wouldn't that mean that only admins (or users who can read HelmRepositories at the cluster scope) would be able to view a catalog, in any namespace?

No, that's not quite what I meant. Right now the API raises an error. Hopefully we all agree that is not what we want. All I was trying to say is that API should be always be returning a list of packages from repos they have access to. If there are no such repos, the response would be empty. I was not trying to say that only those with cluster-wide access would get a non-empty response. I understand that is not practical. What I was trying to say I need to come up with a practical solution that satisfies that goal.

[absoludity]

Mostly true what you said above (see below)

I am thinking that List() should be executed in the context of kubeapps-internal-kubeappsapis service account and then I manually filter out those repos that the caller doesn't have access to. What do you think?

Yep, makes sense. It might still be expensive to determine the repos the caller doesn't have access to, but I'm sure you'll find a solution :)

[absoludity]

we don't have this problem with helm plugin, because its cache 'postgresql' is unbounded.

Yep, because it's disk-based (not memory).

If it had an upper limit on how many entries can be stored, we'd have the same issue with helm

Yep.

[gfichtenholt]

Mostly true what you said above (see below)
I am thinking that List() should be executed in the context of kubeapps-internal-kubeappsapis service account and then I manually filter out those repos that the caller doesn't have access to. What do you think?

Yep, makes sense. It might still be expensive to determine the repos the caller doesn't have access to, but I'm sure you'll find a solution :)

so far this approach appears to work

[gfichtenholt]

Hi
Actually removed the previous comment as there is a more basic question to be answered. Gotta learn how to walk before running 😄 We have an API called GetInstalledPackageSummaries(). I believe it is normal to invoke this w.r.t. a certain namespace, but the API also allows not to specify the namespace, which would mean cluster-wide. But the question isn't about that (yet). Suppose a given user does not have read access (to read HelmReleases) in some namespace X. Should the API return:
a) an empty list OR
b) raise (or more like pass through) an error along the lines of rpc error: code = PermissionDenied desc = Forbidden to list the HelmRelease 'test-ns2-wg3l/*' due to 'helmreleases.helm.toolkit.fluxcd.io is forbidden: User "system:serviceaccount:default:test-release-rbac-loser" cannot list resource "helmreleases" in API group "helm.toolkit.fluxcd.io" in the namespace "test-ns2-wg3l"

FWIW, I think GetInstalledPackageSummaries() should return an empty list in this scenario but GetInstalledPackageDetail or GetInstalledPackageResourceRefs should retrurn an error if a ref is passed in in the namespace the caller isn't allowed access to. Just my opinion. I see that carvel plugin raises an error for GetInstalledPackageSummaries, so I could be convinced otherwise

[absoludity]

I usually reason about these type of questions by checking what the CLI does. If I check with helm:

helm ls --kube-as-user kubeapps-user  
Error: list: failed to list: secrets is forbidden: User "kubeapps-user" cannot list resource "secrets" in API group "" in the namespace "default"

or carvel (just kubectl):

kubectl get packageinstalls --as kubeapps-user
Error from server (Forbidden): packageinstalls.packaging.carvel.dev is forbidden: User "kubeapps-user" cannot list resource "packageinstalls" in API group "packaging.carvel.dev" in the namespace "default"

So those two CLIs choose to give a strong immediate error identifying the problem, rather than returning an empty list. Yes, we have a slight difference in that we also want to be able to aggregate the results of GetInstalledPackageSummaries, which means if one plugin raises an error like this, that error is raised for the core API, but I don't see a problem with that (yet?). What problem do you see?

Perhaps the original question? AIUI, the issue was that with Flux, the authenticated user may have access to (the namespace of ) the installed package (HelmRelease) but not that of the available package? I'd test this out with the flux CLI, but I assume it just shows the data from the HelmRelease (ie. it may not add data from the related HelmChart). Could we not do the same in that case? (ie. just not add the extra info from the HelmChart if it's not accessible. I'd have to update the frontend code to handle that, but should be ok)

[gfichtenholt]

One problem I see is an inconsistency in flux plugin API semantics. We've said above that GetAvailablePackageSummaries() will NOT raise an error if the caller has no read access for HelmRepositories in specified, but GetInstalledPackageSummaries would raise an error if the caller has no read access for HelmReleases in specified namespace

[gfichtenholt]

Could we not do the same in that case? (ie. just not add the extra info from the HelmChart if it's not accessible

yes, I will make it work that way

[absoludity]

One problem I see is an inconsistency in flux plugin API semantics. We've said above that GetAvailablePackageSummaries() will NOT raise an error if the caller has no read access for HelmRepositories in specified,

Did we? I couldn't find that (but may have missed it). I thought we said that when a user calls GetAvailablePackageSummaries for namespace-A, we should not try to get all repos at the cluster scope (and so possibly error if RBAC doesn't allow it), but rather should only request repos to which the user has access. I'd expect it to still raise an error if the authenticated user doesn't have some required access to the requested namespace-A in the first place (though what that means here isn't clear to me - for flux it might be that there's not any required resource in the namespace, though it's not much use seeing a catalog for a namespace if you can't see applications, so in other plugins we just check if the user can read secrets in some situation).

Could we not do the same in that case? (ie. just not add the extra info from the HelmChart if it's not accessible

yes, I will make it work that way

Cool - if that works, great. Seems to keep the behaviour similar to the CLI.

[gfichtenholt]

regarding GetAvailablePackageSummaries() vs GetInstalledPackageSummaries(). I now feel its apples vs oranges because former always works cluster-wide and latter is always namespace-specific. So its kind of moot to talk about an inconsistency between them, which makes me feel better
see https://github.com/kubeapps/kubeapps/pull/4401/files#r824448161