Skip to content
This repository was archived by the owner on Apr 17, 2025. It is now read-only.

Add permissions on hnc CRDs to default clusterRoles: cluster-reader, view, edit, admin #318

Merged
merged 1 commit into from
Jan 29, 2024
Merged

Add permissions on hnc CRDs to default clusterRoles: cluster-reader, view, edit, admin #318

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions config/rbac/aggregate_to_admin.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
# Automatically aggregated to clusterRoles: admin
apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRole
metadata:
name: admin
labels:
rbac.authorization.k8s.io/aggregate-to-admin: "true"
rules:
- apiGroups:
- hnc.x-k8s.io
resources:
- hierarchicalresourcequotas
- subnamespaceanchors
- hierarchyconfigurations
verbs:
- '*'
15 changes: 15 additions & 0 deletions config/rbac/aggregate_to_edit.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
# Automatically aggregated to clusterRoles: edit -> admin
apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRole
metadata:
name: edit
labels:
rbac.authorization.k8s.io/aggregate-to-edit: "true"
rules:
- apiGroups:
- hnc.x-k8s.io
resources:
Copy link

Choose a reason for hiding this comment

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

current admin-role has * so hncconfigurations too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why hncconfigurations too?
isnt its clusterWide object?
you mean hierarchyconfigurations?

Copy link

@jouve jouve Sep 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

indeed :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I dont have practice with hierarchyconfigurations so I dont know the consequences, a bit afraid to add something I dont know

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok from what I see this CR allows to add annotations/labels to current namespace and his children.
This roleBinding is aggregated to edit/admin permission, user that has edit/admin shouldn't be able to modify the namespace including annotations/labels.

If hierarchyconfigurations would influence only the children namespaces then yes I would add it, but because it effects the parent namespace as well, I consider it as a security breach because edit/admin on the namespace shouldnt have permissions to modify the parent namespace.

Sorry I wont add it to the list, if there will be a way to configure hierarchyconfigurations only to effect the children (maybe from hncConfiguration or something) then yes we can add it as default edit permission.

But thank you for the feedback!

Copy link
Contributor

Choose a reason for hiding this comment

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

It might make sense to distinguish between hnc-edit, which gives permission to the namespaced objects (SubnamespaceAnchor, HierarchyConfiguration, HierarchicalResourceQuota) and hnc-admin, which would add permission to administer HNC itself (HNCConfiguration). That is, hnc-edit would let users modify any part of the namespace hierarchy, while hnc-admin would let users change the way HNC works.

Being able to edit HierarchyConfiguration looks like it lets you change the parent of a namespace, but in reality HNC itself imposes limitations on this:

However, being the admin of a namespace does not give you free reign over that namespace’s config. For example, if you are the admin of a child namespace but not its parent, you may not change the parent of your namespace, as this would remove privileges from the admin of the parent (otherwise known as a superadmin, which in HNC's context means an admin of any ancestor namespace). Similarly, even if your namespace is a root, you may not set its parent to any namespace of which you are not currently an admin, since the namespace could then inherit sensitive information such as Secrets.

In general, to change the parent of your namespace N from A to B, you must have the following privileges:

  • You must be the admin of the highest namespace that will no longer be an ancestor of namespace N after this change, in order to confirm that you are happy to lose your privileges in namespace N.
  • You must be the admin of namespace B, in order to acknowledge that sensitive objects from B may be copied into namespace N.

This is implemented here.

Copy link

@jouve jouve Sep 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

for clarification, admin is supposed to be used with a RoleBinding ref, so a user would not be able to use HNCConfiguration which is ClusterScoped anyway:

  • view: view resources in namespace
  • edit: view+edit resources (except rbac Role/Rolebinding) in namespace
  • admin: view+edit resource (incl. Role/RoleBinding) in namespace
  • cluster-admin: view+edit anything (resources * verbs: *) ref , with clusterrolebinding

as a matter of preference, I prefer explicit declarations instead of *

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds great, but my point was that without HNC, when giving someone admin on namespace A, he cannot change the labels nor annotations of that namespace.
But with the help of HNC, the admin user can configure hiearchyConfiguration to add annotations including on self namespace A.

So its kind of a workaround to be able to manage the namesoace labels+annotations without actually having the permissions.

But now as I think, why not to actually give the ability for the user to label or annotate his namespace even if he cant modify the object? (as long as he is admin)
I bet its not allowed as default only because labels+annotations are part of the object, and its impossible to give permissions only on a specific fields in the object.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, thanks @jouve , yes, if these are all supposed to be used by RoleBindings and not ClusterRoleBindings, then there's no point to add HNCConfiguration.

Sounds great, but my point was that without HNC, when giving someone admin on namespace A, he cannot change the labels nor annotations of that namespace.

There's no general concept of "admin" on a namespace without HNC AFAIK - either you can edit the namespace object, in which case you can modify the labels/annotations, or you can't. As you say, there's no way to grant permissions to only specific fields.

With HNC, if you are allowed to modify the HierarchyConfiguration in namespace A, then you can also edit the specific labels and annotations managed by HNC. It's not a general-purpose way to modify any label/annotation. But yes, granting someone hnc-admin at the cluster level would allow them to modify any managed label/annotation on any namespace; that's by design.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But yes, granting someone hnc-admin at the cluster level would allow them to modify any managed label/annotation on any namespace; that's by design.

Thats the point, if someone is granted at not cluster level (only namespaced) he can still modify the annotations of his namespace.

Simple user has only roleBinding and can create hierarchyConfiguration, HNC will create the annotation for him even if he doesn't has permissions.

Anyways I think if someone has admin on the namespace it makes sense that he could add labels to the namespace.

Give me 30 min I will update the PR

- hierarchicalresourcequotas
- subnamespaceanchors
verbs:
- '*'
16 changes: 16 additions & 0 deletions config/rbac/aggregate_to_view.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
# Automatically aggregated to clusterRoles: cluster-reader, view -> edit -> admin
apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRole
metadata:
name: view
labels:
rbac.authorization.k8s.io/aggregate-to-view: "true"
rules:
- apiGroups:
- hnc.x-k8s.io
resources:
- '*'
verbs:
- get
- list
- watch
18 changes: 0 additions & 18 deletions config/rbac/hnc_admin.yaml
View file
Open in desktop

This file was deleted.

4 changes: 3 additions & 1 deletion config/rbac/kustomization.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
resources:
- role.yaml
- role_binding.yaml
- hnc_admin.yaml
- aggregate_to_view.yaml
- aggregate_to_edit.yaml
- aggregate_to_admin.yaml
- leader_election_role.yaml
- leader_election_role_binding.yaml