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

design-proposal: limit Live Migration initial permissions to cluster-admin #377

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tiraboschi
Copy link
Member

What this PR does / why we need it:
In kubernetes we have two distinct pre-defined administrative roles:

  • cluster-admin that is allowed to to perform any action on any resource
  • admin that is allowed to read/write/amend almost all the resources in a namespace

We can assume that that granting to a developer/devops person the admin role to a namespace that he "owns" is a common practice.

In order to enqueue o cancel a live migration request, the user is supposed to create a VirtualMachineInstanceMigration object that is a namespaced resource that lives in
the same namespace of the VM object.
Till now, a namespace admin is allowed to create/delete VirtualMachineInstanceMigration objects in his namespaces.

The issue that this proposal is trying to address is that live migration is also a critical feature for cluster wide tasks such as upgrades, node drains and so on.
So today an unprivileged user can already easily delay a cluster upgrade or a node drain simply continuously enqueuing migrations or even block it by continuously deleting VirtualMachineInstanceMigration objects as soon as they appear on the cluster.

This proposal is about allowing, by default, only cluster-admin users to create/delete VirtualMachineInstanceMigration objects as a hardening measure (principle of least privilege).
Please notice that this is not an API change.

A cluster-admin will still be allowed to grant the migration role to selected users or groups.
Other actions like hotplugging that are only implicitly relying on a live migration are not going to be affected by this change.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:
Please notice that this is not an API change but simply hardening (principle of least privilege) on the default KubeVirt roles.
A cluster-admin will still be allowed to grant the migration role to selected users or groups or even restore the previous behaviour.

Checklist

This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.

Release note:

limit Live Migration initial permissions to cluster-admin

@kubevirt-bot kubevirt-bot added the dco-signoff: yes Indicates the PR's author has DCO signed all their commits. label Jan 16, 2025
@kubevirt-bot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign rmohr for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Copy link
Member

@xpivarc xpivarc left a comment

Choose a reason for hiding this comment

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

Looks good overall

design-proposals/hardening-vmim.md Show resolved Hide resolved

## Update/Rollback Compatibility
After an upgrade, KubeVirt operator will amend `kubevirt.io:admin`, `kubevirt.io:edit` cluster roles removing the rules that allow namespace admin to manage VMIMs so an upgraded cluster will behave exactly a fresh deployed one.
A cluster admin could still restore the previous behavior simply relabeling the new `kubevirt.io:migrate` clusterrole. The KubeVirt operator will ignore custom labels there honoring the cluster-admin configuration.
Copy link
Member

Choose a reason for hiding this comment

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

Note worthy would be that for smooth upgrades the admin is required to pre-deploy the cluster role and bind it.

Copy link
Member

Choose a reason for hiding this comment

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

Should our operator actually do this if we see that it is an upgrade?

Copy link
Member Author

Choose a reason for hiding this comment

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

Personally I think that aiming to have clusters deployed with an old version and then upgraded behaving by default exactly as fresh clusters is more than desiderable in terms of maintainability and harmonization.
Otherwise a cluster admin managing two different clusters deployed starting with different versions could be surprised by a different behaviour that is hard to track and justify.
A really conscious cluster admin could still pre-deploy a temporary clusterrole to grant migration rights to all the namespace admins (labelling it with rbac.authorization.k8s.io/aggregate-to-admin=true ) and remove it only when he is absolutely sure that all the users that needs to be able to trigger live migrations got bound to the new kubevirt.io:migrate cluster role.
I added it to the proposal.

…admin

In kubernetes we have two distinct pre-defined
administrative roles:
- cluster-admin that is allowed to to perform any action on any resource
- admin that is allowed to read/write/amend almost all the resources in a namespace
We can assume that that granting to a developer/devops person
the admin role to a namespace that he "owns" is a common practice.

In order to enque o cancel a live migration request,
the user is supposed to create a VirtualMachineInstanceMigration object
that is a namespaced resource that lives in
the same namespace of the VM object.
Till now, a namespace admin is allowed to create/delete
VirtualMachineInstanceMigration objects in his namespaces.

But a live migration is also a critical feature for
cluster wide tasks such as upgrades, node drains and so on.
So today an unprivileged user can already easily delay a
cluster upgrade or a node drain simply continuously enqueuing
migrations or even block it by continuously deleting
VirtualMachineInstanceMigration objects as soon as they appear.

This proposal is about allowing, by default, only cluster-admin users
to create/delete VirtualMachineInstanceMigration objects
as a hardening measure (principle of least privilege).

A cluster-admin will still be allowed to grant the migration role
to selected users or groups.

Other actions like hotplugging that are only implicitly relying
on a live migration are not going to be affected by this change.

Signed-off-by: Simone Tiraboschi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dco-signoff: yes Indicates the PR's author has DCO signed all their commits. size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants