Execute add/remove registry entries in elevated mode with hyperv #24813

lstocchi · 2024-12-10T09:54:30Z

Adding/removing a key in the Windows registry requires elevated rights. For this reason, using podman with hyperv had the constraint to run it as admin otherwise it was not possible to init/rm a hyperv machine.

This patch adds a couple of functions to add/remove registry entries in bulk in privileged mode. When creating/deleting a machine the user is asked only once to elevate the rights to perform the action. So now podman can be started without being admin.

This will also be leveraged by podman desktop so users could switch from wsl to hyperv without restarting desktop in elevated mode.

Does this PR introduce a user-facing change?

Not sure what you mean by user-facing change but i would say yes.
Now the user who init a hyperv machine is asked to give elevated rights to execute the script to add registry entries to the Windows registry

Podman will request elevated privileges to modify Windows Registry entries when initializing/removing a machine using Hyper-V, allowing to run Podman as a normal user.

baude · 2024-12-10T13:36:37Z

pkg/machine/hyperv/vsock/vsock.go

 	"strings"

 	"github.com/Microsoft/go-winio"
 	"github.com/containers/podman/v5/pkg/machine/sockets"
+	"github.com/containers/podman/v5/pkg/machine/wsl/wutil"


do we do this in other places in WSL? I dont like mixing providers by calling into each other when possible. would you consider pkg/machine/windows instead (apple does this)

baude · 2024-12-10T13:36:55Z

pkg/machine/hyperv/vsock/vsock.go

-func NewHVSockRegistryEntry(machineName string, purpose HVSockPurpose) (*HVSockRegistryEntry, error) {
+// CreateHVSockRegistryEntry is a constructor to make an instance of a registry entry in Windows. After making the new
+// object, you must call the add() method or AddHVSockRegistryEntries(...) to *actually* add it to the Windows registry.
+func CreateHVSockRegistryEntry(machineName string, purpose HVSockPurpose) (*HVSockRegistryEntry, error) {


should this and associated funcs go into libhvee?

Up to you, i can move it if you want. I checked libhvee and it looks like it's dedicated to hyperv but this funcs allow to add/rm entries to/from the Windows registry which are not tight to hyper-v. We use them for hyper-v here but they could be useful in other cases.

baude · 2024-12-10T13:37:12Z

pkg/machine/hyperv/vsock/vsock.go

+		return err
+	}
+
+	d, _ := os.Getwd()


why are we eating the error? i know it is done elsewhere too.

Yes, i copied the existing code. Updated 👍

baude · 2024-12-10T13:38:20Z

you will need to document this change somewhere as well as add a release note about running elevated once. you also will need a test and to make sure this passes all current tests obviously.

lstocchi · 2024-12-10T13:40:38Z

you will need to document this change somewhere as well as add a release note about running elevated once. you also will need a test and to make sure this passes all current tests obviously.

Sure, wanted some early feedback to know if i was doing something wrong or if it was an acceptable solution 👍 Gonna update it with all the rest!!

baude · 2024-12-10T16:36:22Z

does this approach require the user to be in a specific group on windows then ?

lstocchi · 2024-12-10T19:49:22Z

does this approach require the user to be in a specific group on windows then ?

AFAIK you still need to be in the hyper-v admin group. Not sure if we can do some change to libhvee to reduce the privileges on that front as well.

lstocchi · 2024-12-11T15:02:40Z

you will need to document this change somewhere as well as add a release note about running elevated once. you also will need a test and to make sure this passes all current tests obviously.

@baude I only found one place where it was mentioned that podman needed to be run as admin when using hyper-v, so i updated that doc. Need to add something else?

The tests seem to be running only on linux or i'm wrong? What tests should i add? Unit tests to verify the scripts are generate as expected? Any other idea/request?

baude · 2024-12-12T13:42:54Z

re: groups, what i was thinking more about is whether we now have to check for that instread.

lstocchi · 2024-12-12T16:53:02Z

re: groups, what i was thinking more about is whether we now have to check for that instread.

you mean adding a check to verify if the user is in the hyper-v admin group?

baude · 2024-12-13T13:46:33Z

re: groups, what i was thinking more about is whether we now have to check for that instread.

you mean adding a check to verify if the user is in the hyper-v admin group?

correct

openshift-ci · 2024-12-16T19:08:47Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: lstocchi
Once this PR has been reviewed and has the lgtm label, please assign baude for approval. For more information see the 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:

OWNERS

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

lstocchi · 2024-12-16T19:11:09Z

re: groups, what i was thinking more about is whether we now have to check for that instread.

Done, i followed the logic that was in place for the admin check

packit-as-a-service · 2024-12-16T19:13:04Z

Ephemeral COPR build failed. @containers/packit-build please check.

l0rd · 2025-01-07T17:26:23Z

@lstocchi thank you for submitting this PR. Requiring an admin terminal for HyperV machine commands is painful, and your help is appreciated. Here are some thoughts after reviewing and testing this PR:

What used to work (running machine init from an admin terminal) doesn't work anymore for a user that belongs to the Administrators group.
This PR introduces a new check that verifies whether the user belongs to the HyperV Administrators group. This is interesting, but it also raises the question: Is that enough? Is that necessary? Prior to this PR, the requirement was to be able to open a terminal as an administrator. To keep it simple, I would like to do the same check, or we may end up with problems similar to one in the first bullet point.
Opening a new Powershell window to run a command to create the registry key introduces new complexity and is a worse UX. This is not a blocker per se but other approaches, like the one used by gsudo, may be simpler and should be investigated. Also I am wondering if this approach works when there is no GUI (i.e. when connecting from another host using SSH).
The obvious solution is to avoid any elevated command at all (i.e. not create the registry key or create it in HKCU) and we should have a good explanation about why we need to run elevated commands.
There is another way to solve the problem of the podman desktop scenario. Podman desktop can run elevated commands without restarting and we could do that for machine commands when the provider is HyperV.
I am wondering if we could use the const WinBuiltinHyperVAdminsSid defined in https://pkg.go.dev/golang.org/x/sys/windows#pkg-constants instead of defining a new const hypervAdminGroupSid

lstocchi · 2025-01-14T11:01:26Z

Thanks for the review @l0rd and sorry for the slow response. I've been busy on other things.

What used to work (running machine init from an admin terminal) doesn't work anymore for a user that belongs to the Administrators group.

This PR introduces a new check that verifies whether the user belongs to the HyperV Administrators group. This is interesting, but it also raises the question: Is that enough? Is that necessary? Prior to this PR, the requirement was to be able to open a terminal as an administrator. To keep it simple, I would like to do the same check, or we may end up with problems similar to one in the first bullet point.

I re-added the check to verify that the user is admin.
There are 2 cases that are supported
You're an admin: everything is allowed
Non-admin: if you are NOT member of the hyper-v admin group you are NOT allowed to create/remove a hyper-v machine so it displays an error message. Otherwise, we ask to elevate privileges only to add/rm key/value pairs in the registry and the rest is executed in unprivileged mode.

Opening a new Powershell window to run a command to create the registry key introduces new complexity and is a worse UX. This is not a blocker per se but other approaches, like the one used by gsudo, may be simpler and should be investigated.

The drawback from my pov is that if we use gsudo or another tool we are adding an extra dependency/complexity. All windows users (or i would say 99%) have the powershell installed. This is not true for gsudo or other tools. So, if we start depending on them we need to bundle it with podman and also make sure to keep it updated/verify it works with new releases.

I'll try to see if by using the .NET framework (Microsoft.Win32.Registry) we can remove the powershell scripts.

Also I am wondering if this approach works when there is no GUI (i.e. when connecting from another host using SSH).

What do you mean? Having a hyper-v windows guest machine, connecting to it from the host and executing the podman machine init in the guest?

The obvious solution is to avoid any elevated command at all (i.e. not create the registry key or create it in HKCU) and we should have a good explanation about why we need to run elevated commands.

The explanation is that adding/removing things on the HKLM registry is only allowed to admin users. Should i add this somewhere on the doc or you mean something else?

I am wondering if we could use the const WinBuiltinHyperVAdminsSid defined in https://pkg.go.dev/golang.org/x/sys/windows#pkg-constants instead of defining a new const hypervAdminGroupSid

Good idea, done 👍 Thanks!!

Adding/removing a key in the Windows registry requires elevated rights. For this reason, using podman with hyperv had the constraint to run it as admin otherwise it was not possible to init/rm a hyperv machine. This patch adds a couple of functions to add/remove registry entries in bulk in privileged mode. When creating/deleting a machine the user is asked only once to elevate the rights to perform the action. So now podman can be started without being admin. The only constraint is that you must be a member of the Hyper-V administratos group. This will also be leveraged by podman desktop so users could switch from wsl to hyperv without restarting desktop in elevated mode. Signed-off-by: lstocchi <[email protected]>

lstocchi · 2025-01-17T14:34:31Z

@l0rd feel free to add more info if i missed something

l0rd

Thank you @lstocchi for the updates. I have tested it and it works fine. Well done. I have added some comments, probably the most important one is to add a test for the new AddHVSocketRegistryEntries.

l0rd · 2025-01-20T12:26:41Z

pkg/machine/wsl/util_windows.go

@@ -132,71 +104,7 @@ func HasAdminRights() bool {
 func relaunchElevatedWait() error {


If possible please remove this function. Callers can use wutil.LaunchElevatedWait directly.

l0rd · 2025-01-20T12:45:56Z

pkg/machine/provider/platform_windows.go

@@ -34,8 +34,8 @@ func Get() (vmconfigs.VMProvider, error) {
 	case define.WSLVirt:
 		return new(wsl.WSLStubber), nil
 	case define.HyperVVirt:
-		if !wsl.HasAdminRights() {
-			return nil, fmt.Errorf("hyperv machines require admin authority")
+		if !hyperv.HasHyperVAdminRights() && !wsl.HasAdminRights() {


If possible please add a comment here with a link to the doc where it is mentioned that, to manage HyperV VMs, the user need to belong to the HyperV Admins group.

l0rd · 2025-01-20T13:16:55Z

pkg/machine/hyperv/stubber.go

-	// Add vsock port numbers to mounts
-	err = createShares(mc)
+	// Create vsock port numbers to mounts
+	sharesVsock, err := createShares(mc)


considered that's a list of vsockets you may want to rename the variable sharesVsocks or even mountSharesVsockets

l0rd · 2025-01-20T13:53:20Z

pkg/machine/hyperv/vsock/vsock.go

+
+// AddHVSockRegistryEntries allows to *actually* add multiple registry entries to the Windows registry
+//
+// When init a podman machine with hyperV, we have to add registry entries under


What about moving this comment in the stubber, where this function is called, and when the vsocket are created?

l0rd · 2025-01-20T13:56:13Z

pkg/machine/hyperv/vsock/vsock.go

+// (e.g  "00000ac9" represents port 2761 -> hexadecimal AC9 is decimal 2761)
+//
+// This func supports bulk insertion so the user is asked for elevated rights only once
+func AddHVSockRegistryEntries(entries []HVSockRegistryEntry) error {


To make it clear what's the difference between this func and (hv *HVSockRegistryEntry) Add() what about renaming it ElevateAndAddEntries? The same for the RemoveHVSockRegistryEntries.

l0rd · 2025-01-20T14:08:48Z

pkg/machine/hyperv/vsock/vsock.go

+// (e.g  "00000ac9" represents port 2761 -> hexadecimal AC9 is decimal 2761)
+//
+// This func supports bulk insertion so the user is asked for elevated rights only once
+func AddHVSockRegistryEntries(entries []HVSockRegistryEntry) error {


Are (hv *HVSockRegistryEntry) Add(), NewHVSockRegistryEntry() etc...still used? Because otherwise we should remove them.

l0rd · 2025-01-20T14:09:20Z

pkg/machine/hyperv/vsock/vsock.go

+// (e.g  "00000ac9" represents port 2761 -> hexadecimal AC9 is decimal 2761)
+//
+// This func supports bulk insertion so the user is asked for elevated rights only once
+func AddHVSockRegistryEntries(entries []HVSockRegistryEntry) error {


You may want to add a unit test for these new functions.

l0rd · 2025-01-20T14:16:22Z

pkg/machine/hyperv/hutil.go

+	"golang.org/x/sys/windows"
+)
+
+func HasHyperVAdminRights() bool {


If it's not too hard, it would be helpful to add a test for this (that's not a blocker).

openshift-ci bot added the do-not-merge/release-note-label-needed Enforce release-note requirement, even if just None label Dec 10, 2024

github-actions bot added the machine label Dec 10, 2024

baude reviewed Dec 10, 2024

View reviewed changes

openshift-ci bot added release-note and removed do-not-merge/release-note-label-needed Enforce release-note requirement, even if just None labels Dec 11, 2024

lstocchi force-pushed the hypervElevatedExec branch from bc58a14 to 369163b Compare December 11, 2024 14:54

lstocchi force-pushed the hypervElevatedExec branch from 369163b to 6709178 Compare December 11, 2024 15:53

lstocchi force-pushed the hypervElevatedExec branch from 6709178 to ed47c52 Compare December 16, 2024 19:08

lstocchi requested a review from baude December 16, 2024 19:11

lstocchi force-pushed the hypervElevatedExec branch from ed47c52 to 02cbb6e Compare January 14, 2025 10:42

lstocchi force-pushed the hypervElevatedExec branch from 02cbb6e to 574d243 Compare January 17, 2025 12:38

This was referenced Jan 17, 2025

Add user to hyperv admin group during podman installation #25037

Open

Preregister entries on the windows registry to be reused with active hyperv machine #25038

Open

l0rd reviewed Jan 20, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Execute add/remove registry entries in elevated mode with hyperv #24813

Execute add/remove registry entries in elevated mode with hyperv #24813

lstocchi commented Dec 10, 2024 •

edited

Loading

baude Dec 10, 2024

baude Dec 10, 2024

lstocchi Dec 11, 2024

baude Dec 10, 2024

lstocchi Dec 11, 2024

baude commented Dec 10, 2024

lstocchi commented Dec 10, 2024

baude commented Dec 10, 2024

lstocchi commented Dec 10, 2024

lstocchi commented Dec 11, 2024 •

edited

Loading

baude commented Dec 12, 2024

lstocchi commented Dec 12, 2024

baude commented Dec 13, 2024

openshift-ci bot commented Dec 16, 2024

lstocchi commented Dec 16, 2024

packit-as-a-service bot commented Dec 16, 2024

l0rd commented Jan 7, 2025 •

edited

Loading

lstocchi commented Jan 14, 2025

lstocchi commented Jan 17, 2025

l0rd left a comment

l0rd Jan 20, 2025

l0rd Jan 20, 2025

l0rd Jan 20, 2025

l0rd Jan 20, 2025

l0rd Jan 20, 2025

l0rd Jan 20, 2025

l0rd Jan 20, 2025

l0rd Jan 20, 2025

		@@ -132,71 +104,7 @@ func HasAdminRights() bool {
		func relaunchElevatedWait() error {

Execute add/remove registry entries in elevated mode with hyperv #24813

Are you sure you want to change the base?

Execute add/remove registry entries in elevated mode with hyperv #24813

Conversation

lstocchi commented Dec 10, 2024 • edited Loading

Does this PR introduce a user-facing change?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

baude commented Dec 10, 2024

lstocchi commented Dec 10, 2024

baude commented Dec 10, 2024

lstocchi commented Dec 10, 2024

lstocchi commented Dec 11, 2024 • edited Loading

baude commented Dec 12, 2024

lstocchi commented Dec 12, 2024

baude commented Dec 13, 2024

openshift-ci bot commented Dec 16, 2024

lstocchi commented Dec 16, 2024

packit-as-a-service bot commented Dec 16, 2024

l0rd commented Jan 7, 2025 • edited Loading

lstocchi commented Jan 14, 2025

lstocchi commented Jan 17, 2025

l0rd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lstocchi commented Dec 10, 2024 •

edited

Loading

lstocchi commented Dec 11, 2024 •

edited

Loading

l0rd commented Jan 7, 2025 •

edited

Loading