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.

Sign up for GitHub

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

Jump to bottom

feat: supporting OwnerReferencesPermissionEnforcement admission controller #776

Closed
wants to merge 1 commit into from
Closed

feat: supporting OwnerReferencesPermissionEnforcement admission controller #776

wants to merge 1 commit into from

Conversation

prometherion
Copy link
Member

@prometherion prometherion commented Jun 21, 2023 •
edited

Closes #773.

Let's say I got this Tenant definition:

apiVersion: capsule.clastix.io/v1beta2
kind: Tenant
metadata:
  name: test
spec:
  owners:
  - kind: User
    name: alice

As a result, the new controller will reconcile the following resources:

apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRole
metadata:
  name: capsule:test:orpe
  ownerReferences:
  - apiVersion: capsule.clastix.io/v1beta2
    blockOwnerDeletion: true
    controller: true
    kind: Tenant
    name: test
    uid: 75722d36-f033-4727-8cd6-99b75b4e59dd
rules:
- apiGroups:
  - capsule.clastix.io
  resourceNames:
  - test
  resources:
  - tenants/finalizers
  verbs:
  - update
---
apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRoleBinding
metadata:
  name: capsule:test:orpe
  ownerReferences:
  - apiVersion: capsule.clastix.io/v1beta2
    blockOwnerDeletion: true
    controller: true
    kind: Tenant
    name: test
    uid: 75722d36-f033-4727-8cd6-99b75b4e59dd
roleRef:
  apiGroup: rbac.authorization.k8s.io
  kind: ClusterRole
  name: capsule:test:orpe
subjects:
- apiGroup: rbac.authorization.k8s.io
  kind: User
  name: alice

If the Tenant will be updated with different users, the resulting ClusterRoleBinding will reflect the owners.

As a result, alice will be allowed to patch Tenant finalizers.

$: kubectl --as alice --as-group capsule.clastix.io auth can-i update tenants/test --subresource finalizers
Warning: resource 'tenants' is not namespace scoped in group 'capsule.clastix.io'

yes

@netlify
Copy link

netlify bot commented Jun 21, 2023 •
edited

✅ Deploy Preview for capsule-documentation canceled.

Name Link
🔨 Latest commit 7d0b7d4
🔍 Latest deploy log https://app.netlify.com/sites/capsule-documentation/deploys/64aed3f641e7660008cd85bd

@prometherion prometherion force-pushed the issues/773 branch 2 times, most recently from fb5c61b to fab7de6 Compare June 21, 2023 15:48
erikgb
erikgb suggested changes Jun 21, 2023
View reviewed changes
Copy link

@erikgb erikgb left a comment •
edited

Choose a reason for hiding this comment

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

It looks like the proposed change will fix the issue, but I wonder if the additional cluster-level RBAC granted to standard users could represent some risks.

I would also like to see some tests added - to ensure this fix does not just "disappear".

Also suggesting merging this RBAC controller for tenants with the standard RBAC granted to tenant users in a follow-up PR. That could make the permissions granted to namespaces more explicit. I haven't dug deeply into the code, but I expect a webhook enforcing update/delete permissions on namespaces. This could also be expressed more clearly in a composite cluster role granted to tenant owners. And ensure cleanup of resources in a cluster after uninstalling Capsule. This is now implemented as a hackish post-delete Helm Job.

controllers/rbac/owner_references_permission_enforcement.go Outdated
}

func (o OwnerReferencesPermissionEnforcement) resourceName(tnt *capsulev1beta2.Tenant) string {
return fmt.Sprintf("%s-orpe", tnt.Name)
Copy link

Choose a reason for hiding this comment

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

Suggested change
return fmt.Sprintf("%s-orpe", tnt.Name)
return fmt.Sprintf("capsule-tenant:%s-orpe", tnt.Name)

I suggest adding more context to the resource names to ensure they don't collide with other resources in the cluster. This will also make it easier for a cluster-admin to see where the CR and CRB are originating from.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the suggestion, however, I would suggest the pattern capsule:%s:orpe.

controllers/rbac/owner_references_permission_enforcement.go Outdated
crb.RoleRef = rbacv1.RoleRef{
APIGroup: rbacv1.SchemeGroupVersion.Group,
Kind: "ClusterRole",
Name: tnt.Name,
Copy link

Choose a reason for hiding this comment

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

Suggested change
Name: tnt.Name,
Name: o.resourceName(tnt),

@prometherion prometherion marked this pull request as ready for review July 12, 2023 16:29
@prometherion
Copy link
Member Author

prometherion commented Jul 12, 2023 •
edited

@erikgb I marked the PR ready for review and added a new CLI flag named --enabled-owner-references-permission-enforcement which will start the additional controller which grants the required RBAC when the OwnerReferencesPermissionEnforcement admission controller is enabled in the cluster.

For backward compatibility reasons, the default value of the flag is set to false and can be toggled at the operator's wish.

May I ask you to give it a try, please?

@prometherion
Copy link
Member Author

prometherion commented Jul 12, 2023 •
edited

I wonder if the additional cluster-level RBAC granted to standard users could represent some risks

The right is granted only to the tenant owned by the tenant owner. I would prefer having a concrete example of a potential exploit case rather than wonderings.

And ensure cleanup of resources in a cluster after uninstalling Capsule. This is now implemented as a hackish post-delete Helm Job.

This is not required since the cluster scoped resources, such as ClusterRole and ClusterRoleBinding, have controller reference by the Tenant resource which deletion is propagated automatically thanks to it.

@prometherion prometherion requested a review from erikgb July 12, 2023 16:38
@prometherion prometherion mentioned this pull request Jul 27, 2023
Closed
@erikgb
Copy link

erikgb commented Jul 29, 2023

@prometherion I could try to test this PR in one of our Openshfit clusters or one of our other clusters with the Kubernetes OwnerReferencesPermissionEnforcement admission controller enabled. Are images published from pull requests? If not, could you kindly push an image I can use for the test?

@oliverbaehler
Copy link
Collaborator

Stale

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@erikgb erikgb Awaiting requested review from erikgb

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support for clusters with OwnerReferencesPermissionEnforcement admission plugin enabled
3 participants
@prometherion @erikgb @oliverbaehler