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

Aggregate repos read/write ClusterRoles to OOB view/edit (#3680) #3991

Merged
merged 16 commits into from
Jan 13, 2022

Conversation

castelblanque
Copy link
Collaborator

@castelblanque castelblanque commented Dec 23, 2021

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

Description of the change

This solution helps to solve the scenario suggested by Dimitri:

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).

This change sits on top of the feature added in #3989.

Aggregate labels so that our cluster repo roles are aggregated to the existing read/view roles.
Basically:

  • kubeapps:{{ KUBEAPPS_NAMESPACE }}:apprepositories-read is aggregated to the OOB view role
  • kubeapps:{{ KUBEAPPS_NAMESPACE }}:apprepositories-write is aggregated to the OOB edit role

Another alternative to label aggregation would be to add to Helm RBAC template a rolebinding specific to the global repos namespace, and targeted to any logged in user.Something like:

---
apiVersion: {{ include "common.capabilities.rbac.apiVersion" . }}
kind: RoleBinding
metadata:
  name: "kubeapps:{{ .Release.Namespace }}:global-repos-read"
  namespace: "{{ .Release.Namespace }}-{{ .Values.apprepository.globalReposNamespaceSuffix }}"
roleRef:
  apiGroup: rbac.authorization.k8s.io
  kind: ClusterRole
  name: "kubeapps:{{ .Release.Namespace }}:apprepositories-read"
subjects:
  - kind: Group
    name: system:authenticated

The above binding would not require for the user to have the view role.

Personally I believe that the roles aggregation is a simpler, more transparent approach.

Benefits

Kubeapps will allow viewing global repos to users with view role. And the same for user with the edit role.

Possible drawbacks

N/A

Applicable issues

Rafa Castelblanque added 4 commits December 23, 2021 16:33
Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>
Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>
Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>
Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>
@castelblanque castelblanque changed the title Aggregate repos read and write ClusterRoles to view and edit (#3680) Aggregate repos read/write ClusterRoles to OOB view/edit (#3680) Dec 24, 2021
Rafa Castelblanque added 3 commits December 24, 2021 09:42
Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>
Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>
…aggregation

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

@antgamdia antgamdia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm haven't thoroughly review how aggregated roles work yet, but it seems (to me) we should create the aggregationRule first, shouldn't we?

I mean, from what I get from the k8s website, it seems we have to:

  1. Define an aggregationRule in the "target" role, so to speak.
metadata:
  name: <whatever>
aggregationRule:
  clusterRoleSelectors:
  - matchLabels:
      rbac.example.com/aggregate-to-<whatever>: "true"
  1. Use it in other roles we want:
metadata:
  name: <whatever-2>
  labels:
    rbac.example.com/aggregate-to-<whatever>: "true"

Let me know if I'm wrong, though

@absoludity
Copy link
Contributor

I'm haven't thoroughly review how aggregated roles work yet, but it seems (to me) we should create the aggregationRule first, shouldn't we?

We're targeting the out-of-the-box read and edit cluster roles:

$ k get clusterrole view -o yaml | head -n4
aggregationRule:
  clusterRoleSelectors:
  - matchLabels:
      rbac.authorization.k8s.io/aggregate-to-view: "true"

@absoludity
Copy link
Contributor

Another alternative to label aggregation would be to add to Helm RBAC template a rolebinding specific to the global repos namespace, and targeted to any logged in user.Something like:

---
apiVersion: {{ include "common.capabilities.rbac.apiVersion" . }}
kind: RoleBinding
metadata:
  name: "kubeapps:{{ .Release.Namespace }}:global-repos-read"
  namespace: "{{ .Release.Namespace }}-{{ .Values.apprepository.globalReposNamespaceSuffix }}"
roleRef:
  apiGroup: rbac.authorization.k8s.io
  kind: ClusterRole
  name: "kubeapps:{{ .Release.Namespace }}:apprepositories-read"
subjects:
  - kind: Group
    name: system:authenticated

The above binding would not require for the user to have the view role.

Personally I believe that the roles aggregation is a simpler, more transparent approach.

Interesting. I wonder if both are applicable? That is:

  • If a user has edit on the global namespace, I'd expect them to be able to edit the app repositories there, and so we'd want to aggregate to the edit role as you've suggested, but,
  • If a user only has edit access to their own namespace (ie. they don't have view on any other namespaces, including the kubeapps global repos one), I'd still expect them to be able to see the globally available repositories. Your above rolebinding for all authenticated k8s users would allow them to see apprepositories (and app repositories only) in that namespace?

Copy link
Contributor

@absoludity absoludity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, but see my other comment about your alternative idea too :)

@castelblanque
Copy link
Collaborator Author

Interesting. I wonder if both are applicable? That is:

* If a user has edit on the global namespace, I'd expect them to be able to edit the app repositories there, and so we'd want to aggregate to the edit role as you've suggested, but,

* If a user only has edit access to their own namespace (ie. they don't have view on any other namespaces, including the kubeapps global repos one), I'd still expect them to be able to see the globally available repositories. Your above rolebinding for all authenticated k8s users would allow them to see apprepositories (and app repositories only) in that namespace?

That's a good one, thank you!
That would be the idea, the rolebinding applying only to reading repos only in the global-repos ns. Not sure if the lack of "edit" role in that ns would interfere though. I'll reproduce that scenario.

Rafa Castelblanque added 5 commits January 10, 2022 16:45
Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>
Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>
Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>
Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>
…aggregation

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>
@castelblanque
Copy link
Collaborator Author

  • If a user only has edit access to their own namespace (ie. they don't have view on any other namespaces, including the kubeapps global repos one), I'd still expect them to be able to see the globally available repositories. Your above rolebinding for all authenticated k8s users would allow them to see apprepositories (and app repositories only) in that namespace?

After giving it a try, user doesn't have visibility of the repos even if the RoleBinding mentioned in the top is applied. Probably due to the user not having view/edit access to the global namespace.
So, going forward with the roles aggregation.

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>
@absoludity
Copy link
Contributor

  • If a user only has edit access to their own namespace (ie. they don't have view on any other namespaces, including the kubeapps global repos one), I'd still expect them to be able to see the globally available repositories. Your above rolebinding for all authenticated k8s users would allow them to see apprepositories (and app repositories only) in that namespace?

After giving it a try, user doesn't have visibility of the repos even if the RoleBinding mentioned in the top is applied. Probably due to the user not having view/edit access to the global namespace. So, going forward with the roles aggregation.

Hmm, that'd be a shame. Can you demo with kubectl? I just tried here and it seems to work:

# Create two namespaces, but only one with the `system:authenticated` group role-binding:
k create namespace test-auth
namespace/test-auth created

k create namespace test-auth-2
namespace/test-auth-2 created

k create rolebinding -n test-auth test-rolebinding --clusterrole=kubeapps:kubeapps:apprepositories-read --group=system:authenticated 
rolebinding.rbac.authorization.k8s.io/test-rolebinding created

# Verify that, for example, being in `system:authenticated` enables getting app repositories in `test-auth`, but not in `test-auth-2`
k -n test-auth --as system:anonymous get apprepositories 
Error from server (Forbidden): apprepositories.kubeapps.com is forbidden: User "system:anonymous" cannot list resource "apprepositories" in API group "kubeapps.com" in the namespace "test-auth"

k -n test-auth --as system:anonymous --as-group system:authenticated get apprepositories
No resources found in test-auth namespace.

k -n test-auth-2 --as system:anonymous --as-group system:authenticated get apprepositories
Error from server (Forbidden): apprepositories.kubeapps.com is forbidden: User "system:anonymous" cannot list resource "apprepositories" in API group "kubeapps.com" in the namespace "test-auth-2"

Or what's the specific issue you're seeing?

Base automatically changed from 3989-global-repos-namespace to master January 12, 2022 08:59
Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>
@castelblanque
Copy link
Collaborator Author

  • If a user only has edit access to their own namespace (ie. they don't have view on any other namespaces, including the kubeapps global repos one), I'd still expect them to be able to see the globally available repositories. Your above rolebinding for all authenticated k8s users would allow them to see apprepositories (and app repositories only) in that namespace?

Or what's the specific issue you're seeing?

I was checking directly from the Dashboard.
Scenario is: serviceaccount (myuser) has edit role on its own namespace (myns).

With having only the aggregation rules in place, user has access to apprepositories in its own namespace (granted by edit), but not in the global namespace:

User "system:serviceaccount:myns:myuser" cannot list resource "apprepositories" in API group "kubeapps.com" in the namespace "kubeapps-repox-global"

My issue was that at this point the repos config page breaks entirely:

  • Request /api/v1/clusters/default/namespaces/myns/apprepositories returns 200 and correct response.
  • But request /api/v1/clusters/default/namespaces/kubeapps-repox-global/apprepositories returns 403.

image

I would expect that if user does not have access to global repos, at least current ns repos would show. Maybe this is intended by design? It seems inline, though, with the user being able to browse the catalog from global repos.

So I think we actually need both the aggregation rules and the rolebinding I proposed for a proper functioning. What do you think?

To your question Your above rolebinding for all authenticated k8s users would allow them to see apprepositories (and app repositories only) in that namespace?, I assume that if the cluster role kubeapps:*:apprepositories-read is bound only to apprepositories, that is the outcome, yes.

Rafa Castelblanque added 2 commits January 12, 2022 13:16
Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>
Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>
@absoludity
Copy link
Contributor

So I think we actually need both the aggregation rules and the rolebinding I proposed for a proper functioning. What do you think?

Yes, definitely. That's what I meant above when I said "I wonder if both are applicable?" (ie. that we would need both the role and your idea for the system:authenticated ).

Excellent, this is a nice change Rafa, thanks! Land when ready :)

@castelblanque castelblanque merged commit d6f71d0 into master Jan 13, 2022
@castelblanque castelblanque deleted the 3680-repos-read-role-aggregation branch January 13, 2022 07:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inconsistent handling of namespace-scoped users between view/deploy
3 participants