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

Inconsistent handling of namespace-scoped users between view/deploy #3680

Closed
dlaloue-vmware opened this issue Oct 27, 2021 · 5 comments · Fixed by #3991
Closed

Inconsistent handling of namespace-scoped users between view/deploy #3680

dlaloue-vmware opened this issue Oct 27, 2021 · 5 comments · Fixed by #3991
Assignees
Labels
component/apis-server Issue related to kubeapps api-server kind/bug An issue that reports a defect in an existing feature
Projects

Comments

@dlaloue-vmware
Copy link
Collaborator

Create a namespace-scoped user to namespace X by granting the standard cluster role 'edit'.

The user can login to Kubeapps, confirm that only namespace X is available in the context, view the catalog (from global repositories, e.g. bitnami), select for example nginx from bitnami to install, configure everything and click deploy.

On clicking deploy, the action is not allowed because of not having access to the global repositories.
Access was not required by the kubeapps apis when looking at the catalog or app repository page.
why is it required for deploying?

Screen Shot 2021-10-27 at 12 08 34 PM

@project-bot project-bot bot added this to Inbox in Kubeapps Oct 27, 2021
@absoludity
Copy link
Contributor

Access was not required by the kubeapps apis when looking at the catalog

We don't fetch the AppRepository resource when displaying the catalog, it's just displaying the cached data.

or app repository page.

That one surprises me - I'd need to check.

why is it required for deploying?

It shouldn't be required when deploying from a public repo - this is a bug. Previously (before the kubeapps-apis), the kubeops used the users creds for most things, but used the service account for fetching app repositories in the kubeapps namespace. I've avoided doing that so far with the helm plugin, mainly because we should not require fetching the public AppRepository CR to deploy the chart - we should have the metadata already. We need to find out why we are doing so (could be a mistake, related to pulling the entire index.yaml when deploying)

@absoludity absoludity added component/apis-server Issue related to kubeapps api-server kind/bug An issue that reports a defect in an existing feature priority/medium labels Oct 28, 2021
@ppbaena ppbaena moved this from Inbox to Next iteration discussion in Kubeapps Nov 8, 2021
@ppbaena ppbaena moved this from Next iteration discussion to Committed in Kubeapps Nov 8, 2021
@antgamdia antgamdia moved this from Committed to In progress in Kubeapps Nov 10, 2021
@antgamdia
Copy link
Contributor

Sorry for the long read, TL;DR: I think is necessary to create the rolebinding in a user's namespace to clusterrole kubeapps:kubeapps:apprepositories-read. Otherwise it won't work as expected at all.


I'm trying to repro this issue, although I thought #3700 would have addressed it, I think hasn't.

Create a namespace-scoped user to namespace X by granting the standard cluster role 'edit'.

What do you mean by this? Aren't you granting any permissions to apprepostories at all? If so, I can repro it by creating a SA to get the token, for the sake of simplicity, which has rolebinding "edit" on the "default" namespace, that is:

kubectl create serviceaccount kubeapps-edit -n kubeapps
kubectl create rolebinding kubeapps-edit -n default --clusterrole=edit --serviceaccount kubeapps:kubeapps-editrolebinding.rbac.authorization.k8s.io/kubeapps-edit created

Assuming no permissions whatsoever on the apprepositories CR, the very first line of this function will fail:

https://github.com/kubeapps/kubeapps/blob/597de8d06b663172a462e9cd3cccb0973da27036/cmd/kubeapps-apis/plugins/helm/packages/v1alpha1/server.go#L1058-L1063

However, I'd bet an "edit user" should, at least, have read permissions on the apprepos. Any reason not to grant them?

I mean, when no apprepos perms, the UI is not really happy about it:

You can't see apps from your private repo nor filter them
image

You can't perform any interactions whatsoever with the App Repositories tab:
image

or app repository page.

That one surprises me - I'd need to check.

As I just said, without any apprepo perms you cannot interact with the AppRepos view, so I'm also confused. Could you list the rolebindings you used in both the kubeapps and the user's namespace @dlaloue-vmware ?

To "fix" this issue I need to grant the user with apprepositories-read in the user's namespace.

kubectl create rolebinding kubeapps-repositories-read -n default --clusterrole kubeapps:kubeapps:apprepositories-read --serviceaccount kubeapps:kubeapps-edit

If granting it in the kubeapps namespace (below), you will able to deploy globals apps, but you won't be even able to list the apps from a user's repo (and the app repositories tab will also fail)

kubectl create rolebinding kubeapps-repositories-read -n kubeapps --clusterrole kubeapps:kubeapps:apprepositories-read --serviceaccount kubeapps:kubeapps-edit

To sum up, I've put together all this info in a matrix:

edit apprepositories-read in DEFAULT ns apprepositories-read in KUBEAPPS ns x Deploy from DEFAULT Deploy from KUBEAPPS View apps from DEFAULT View apps from KUBEAPPS View apprepos tab
1 0 0 x no no no yes no
1 0 1 x no yes no yes no
1 1 0 x yes yes yes yes yes
1 1 1 x yes yes yes yes yes

According to this matrix, the appropriate solution would be: edit role in the user's ns + apprepositories-read in the user's ns

That said, does it make sense ? As Michael stated:

It shouldn't be required when deploying from a public repo - this is a bug.

But, I don't fully agree here (probably it's just lack of knowledge/background: what if the repo is not a public one? I mean, even if "global" it might require credentials as well as image pull secrets? I guess that we should be retrieving these data from the AppRepository CR, so we should have (read) access at least, shouldn't we?

why is it required for deploying?

That said, nothing prevents us from working around it and splitting the fetchChartWithRegistrySecrets fn or making it smarter. For instance: if no perms to read apprepos, assume it is a very public one and defaults all the secrets to nil.

However, I can't think of a reason for doing this hack; I mean, why not just require the users the proper RBAC?

What I would love is a script o even a kubeapps config page (like: instead/besides of "you don't have access to any ns", display something like, "need help with permissions? try this tool: <react thing dynamically generating kubectl commands to ctrl+c depending on a user selection on a matrix of checkboxes>), but we don't have the bandwidth for doing such a thing haha.

So my two cents on this issue: address it with just a documentation improvement saying, ey, you also need read perms on apprepositories to deploy/upgrade.

WDYT?

@dlaloue-vmware
Copy link
Collaborator Author

Hi Antonio,

the use-case here is limited to a restricted user who is only granted access to a namespace, and with no grant to create private repositories - i.e. the user is limited to use only the global repositories the admin has configured.

the user is granted the kubernetes "edit" role so the user can deploy applications/pods (e.g. using helm cli).

using Kubeapps, the user can login, browse the charts from the global repositories, view details of a chart but it fails when trying to deploy.
the issue this is raising is that the user does not need any "view" grant to actually browse the global repositories, but it needs a "view" grant to deploy a chart. this seems inconsistent.

note: if the user was to be allowed to use private repositories in the namespace, then yes, the admin would need to create the appropriate role bindings in that namespace, but that was not the use case for this issue.

@absoludity
Copy link
Contributor

Sorry for the long read, TL;DR: I think is necessary to create the rolebinding in a user's namespace to clusterrole kubeapps:kubeapps:apprepositories-read. Otherwise it won't work as expected at all.

+1 that the user should be able to read app repositories in the namespace if they have read (or edit, obviously) in that namespace, but -1 to doing so via a rolebinding. I think it's been said before that we should just aggregate labels so that our cluster role (apprepositories-read) is aggregated with the existing read role (similar for edit).

That said, does it make sense ? As Michael stated:

It shouldn't be required when deploying from a public repo - this is a bug.

But, I don't fully agree here (probably it's just lack of knowledge/background: what if the repo is not a public one?

Yep, the above is only true for a public one.

I mean, even if "global" it might require credentials as well as image pull secrets?

The first is possible (a global app repo with credentials to access the repo), but the second is not currently (a global app repo with image pull secrets - we don't allow this as it'd require copying secrets to the target namespace).

I guess that we should be retrieving these data from the AppRepository CR, so we should have (read) access at least, shouldn't we?

Yes, either way, the correct solution is, as you said, that a user with read in a namespace should be able to read apprepositories (without an extra binding, see above).

the issue this is raising is that the user does not need any "view" grant to actually browse the global repositories, but it needs a "view" grant to deploy a chart. this seems inconsistent.

Right, this is the cause of the confusion. The only reason the user can browse the packages from the global repos without having read access to the AppRepository is because we're caching the data and not requiring access to the AppRepository to browse that cached data.

Even if we aggregate our apprepositories read/edit cluster roles with the default read/edit ones (as I think we should), it solves only half the problem that Dimitri has highlighted: users who are granted access (read+edit) to a particular namespace won't (necessarily, and shouldn't really) have access to the kubeapps namespace. I think the complete solution here will involve updating Kubeapps to use a separate kubeapps-repos-global or similar, and require that users can read that namespace (a few options here, but let's see).

@antgamdia antgamdia moved this from In progress to Committed in Kubeapps Dec 2, 2021
@antgamdia antgamdia removed their assignment Dec 2, 2021
@castelblanque castelblanque moved this from Committed to In progress in Kubeapps Dec 16, 2021
castelblanque pushed a commit that referenced this issue Dec 23, 2021
Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>
@castelblanque
Copy link
Collaborator

Agree with the solution of keeping global repos on its own kubeapps-repos-global namespace and requiring read/write permissions there.
Same for the roles aggregation of :apprepositories-read/:apprepositories-write into view and edit.

Created a separate issue for the global repos relocation (#3989) and the roles aggregation work is done for this issue.
PRs landed in both.

Merry christmas to everybody!!! 🎄 🎅 🎁

@castelblanque castelblanque moved this from In progress to Waiting For Review in Kubeapps Dec 24, 2021
castelblanque pushed a commit that referenced this issue Jan 12, 2022
Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>
castelblanque pushed a commit that referenced this issue Jan 12, 2022
Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>
Kubeapps automation moved this from Waiting For Review to Done Jan 13, 2022
castelblanque added a commit that referenced this issue Jan 13, 2022
* Added global repos namespace to Kubeapps (#3989)

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

* Fixed Go tests (#3989)

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

* Fixed Go tests (#3989)

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

* Aggregate repos read and write ClusterRoles to view and edit (#3680)

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

* Fix for IT in CI (#3989)

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

* Fixed Helm values parameter name (#3989)

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

* Default global namespace set to Kubeapps by now (#3989)

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

* Fixed tests (#3989)

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

* Trying to make CI pass due to timeouts

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

* Added role binding to access apprepositories in global namespace (#3680)

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

* Corrected Yaml structure (#3680)

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/apis-server Issue related to kubeapps api-server kind/bug An issue that reports a defect in an existing feature
Projects
No open projects
Kubeapps
  
Done
Development

Successfully merging a pull request may close this issue.

4 participants