Prevent non hyper-v admin users to execute machine commands #27650

lstocchi · 2025-12-01T18:25:34Z

Following the work done in #26277 (which allowed init and remove operations for the "Hyper-V Administrators" group), this PR extends that logic to all actions targeting Hyper-v machines.

it resolves #27614

Checklist

Ensure you have completed the following checklist for your pull request to be reviewed:

Certify you wrote the patch or otherwise have the right to pass it on as an open-source patch by signing all
commits. (git commit -s). (If needed, use git commit -s --amend). The author email must match
the sign-off email address. See CONTRIBUTING.md
for more information.
Referenced issues using Fixes: #00000 in commit message (if applicable)
Tests have been added/updated (or no tests are needed)
Documentation has been updated (or no documentation changes are needed)
All commits pass make validatepr (format/lint checks)
Release note entered in the section below (or None if no user-facing changes)

Does this PR introduce a user-facing change?

Permission validation for Hyper-V has been extended to cover all actions. Users in the "Hyper-V Administrators" group can now execute all actions without needing mandatory full local Administrator privileges. Users without the right privileges will now see an error message when attempting to execute these commands.

mheon · 2025-12-01T18:32:54Z

@l0rd PTAL

mheon · 2025-12-01T18:33:17Z

pkg/machine/provider/platform_test.go

 		assert.Equal(t, []define.VMType{define.AppleHvVirt, define.LibKrun}, SupportedProviders())
 	case "windows":
-		assert.Equal(t, []define.VMType{define.WSLVirt, define.HyperVVirt}, SupportedProviders())
+		assert.Equal(t, []define.VMType{define.HyperVVirt, define.WSLVirt}, SupportedProviders())


Why the ordering swap?

Bc SupportedProviders() returns []define.VMType{define.HyperVVirt, define.WSLVirt} and assert.Equal performs a strict comparison so the items order matters. It failed the way it was written.

And CI isn't catching it because pkg\machine\provider is skipped. @lstocchi would you mind removing that package from the skipped ones?

Can't you use assert.ElementsMatch or similar? I dislike having tests be this fragile.

@mheon @l0rd done both changes

mheon · 2025-12-01T18:35:03Z

pkg/machine/provider/platform_windows.go

+// Variables to hold permission check functions for testing purposes.
+var (
+	hasAdminRightsFunc       = windows.HasAdminRights
+	hasHyperVAdminRightsFunc = hyperv.HasHyperVAdminRights


These don't seem to add much as written. It seems like a single functioning combining both would be more useful, and line 86 could just be windows.HasAdminRights()

@lstocchi you could add a new hyperv.CanList() function similar to the hyperv.canCreate() where you hide the logic.

done. I named it HasHyperVPermissions as it's more generic and not just related to list, let me know

baude · 2025-12-01T21:55:05Z

pkg/machine/provider/platform_windows.go

 		new(wsl.WSLStubber),
-		new(hyperv.HyperVStubber),
 	}
+	if hasAdminRightsFunc() || hasHyperVAdminRightsFunc() {


I am struggling with the logic here if I am a user. If I don't have permissions, should it not exist? Earlier, when we returned an error, that certainly made sense to me.

They have two different purposes imo. Above (GetByVMType()) returns an error because user is explicitly executing a command and if they do not have rights to do so we stop them and return an error.
GetAll is more a discovery thing. It is used by the list command. If someone wants to list machines, it's annoying to show an error message, maybe they're just interested to WSL. So if you don't have permissions to work with Hyper-V it skips it. Hyper-v console works the same - if you don't have the rights you see nothing.

This is about podman machine ls, and the problem has changed in 6.0. If the user doesn't have privileges we still want to list WSL machines.

But we should indeed fail when the user specifies --provider hyperv. And if the default provider is hyperv too.

Update GetAll() and GetByVMType() to add a check to prevent non hyper-v admin users to interact with hyperv machines. Users can work with hyperv machines only with elevated rights or if members of the hyperv administrators group Signed-off-by: lstocchi <lstocchi@redhat.com>

fixes broken windows tests and enables them to be run on windows CI Signed-off-by: lstocchi <lstocchi@redhat.com>

Signed-off-by: lstocchi <lstocchi@redhat.com>

l0rd

I did some tests and all work as expected with the expection of some corner cases.

Inspecting or starting a WSL machine, if the default provider is hyperv, fails:

> $env:CONTAINERS_MACHINE_PROVIDER="hyperv"
> podman.exe machine inspect mywslmachine`
Error: Hyper-V machines require Hyper-V admin rights...
> podman.exe machine start mywslmachine`
Error: Hyper-V machines require Hyper-V admin rights...

And this is awkward because podman machine ls works and shows the WSL machines. But that may not be related to this PR.

Also commands like machine cp, machine ssh etc..., against an hyperv machine, should work too, even if the user doesn't have Hyper-V admin privileges.

That said these scenarios aren't blockers from my point of view.

baude · 2025-12-06T15:47:25Z

@lstocchi will you be addressing the problems @l0rd mentions here or @l0rd do we need to plan for this?

else LGTM

l0rd · 2025-12-08T12:27:44Z

From my point of view this PR is now mergeable as is. It's up to @lstocchi, if he has bandwidth, to fix those corner cases. Otherwise I will open an issue and we can prioritize them later. Let's wait for him to come back (today is national holiday but tomorrow he should be back).

lstocchi · 2025-12-09T08:35:19Z

I did some tests and all work as expected with the expection of some corner cases.

Inspecting or starting a WSL machine, if the default provider is hyperv, fails:
> $env:CONTAINERS_MACHINE_PROVIDER="hyperv"
> podman.exe machine inspect mywslmachine`
Error: Hyper-V machines require Hyper-V admin rights...
> podman.exe machine start mywslmachine`
Error: Hyper-V machines require Hyper-V admin rights...
And this is awkward because podman machine ls works and shows the WSL machines. But that may not be related to this PR.

I think this is a change that would affect all OSes with more than 1 provider, not just Windows. I'd rather have a separate PR to test it also on macOS.

Also commands like machine cp, machine ssh etc..., against an hyperv machine, should work too, even if the user doesn't have Hyper-V admin privileges.

That said these scenarios aren't blockers from my point of view.

I'm going to add the change here as it seems related to this PR

l0rd · 2025-12-09T10:17:19Z

After discussion we decided that it was ok to merge the PR as is. I have created a separate issue #27723

/approve /lgtm

openshift-ci · 2025-12-09T10:17:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: l0rd, lstocchi

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [l0rd]

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

l0rd · 2025-12-09T12:14:45Z

/lgtm

openshift-ci bot added the release-note label Dec 1, 2025

github-actions bot added the machine label Dec 1, 2025

mheon reviewed Dec 1, 2025

View reviewed changes

baude reviewed Dec 1, 2025

View reviewed changes

lstocchi added 3 commits December 2, 2025 16:19

fix failing windows platform tests

85fe4de

fixes broken windows tests and enables them to be run on windows CI Signed-off-by: lstocchi <lstocchi@redhat.com>

add windows platform tests

d150051

Signed-off-by: lstocchi <lstocchi@redhat.com>

lstocchi force-pushed the i27614 branch from 4681bd4 to d150051 Compare December 2, 2025 15:32

l0rd reviewed Dec 2, 2025

View reviewed changes

l0rd mentioned this pull request Dec 9, 2025

Inconsistencies in Hyper-V podman machine commands #27723

Open

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 9, 2025

openshift-ci bot assigned l0rd Dec 9, 2025

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 9, 2025

openshift-merge-bot bot merged commit f66f7c8 into containers:main Dec 9, 2025
86 checks passed

lstocchi deleted the i27614 branch December 9, 2025 12:25

Prevent non hyper-v admin users to execute machine commands #27650

Prevent non hyper-v admin users to execute machine commands #27650

Conversation

lstocchi commented Dec 1, 2025

Checklist

Does this PR introduce a user-facing change?

Uh oh!

mheon commented Dec 1, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

l0rd left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

baude commented Dec 6, 2025

Uh oh!

l0rd commented Dec 8, 2025

Uh oh!

lstocchi commented Dec 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

l0rd commented Dec 9, 2025

Uh oh!

openshift-ci bot commented Dec 9, 2025

Uh oh!

l0rd commented Dec 9, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

l0rd left a comment •

edited

Loading

lstocchi commented Dec 9, 2025 •

edited

Loading