Execute add/remove registry entries in elevated mode with hyperv #24813
Conversation
openshift-ci
bot
added
the
do-not-merge/release-note-label-needed
pkg/machine/hyperv/vsock/vsock.go
Outdated
| "strings" | ||
|
|
||
| "github.com/Microsoft/go-winio" | ||
| "github.com/containers/podman/v5/pkg/machine/sockets" | ||
| "github.com/containers/podman/v5/pkg/machine/wsl/wutil" |
There was a problem hiding this comment.
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)
| 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) { |
There was a problem hiding this comment.
should this and associated funcs go into libhvee?
There was a problem hiding this comment.
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.
pkg/machine/hyperv/vsock/vsock.go
Outdated
| return err | ||
| } | ||
|
|
||
| d, _ := os.Getwd() |
There was a problem hiding this comment.
why are we eating the error? i know it is done elsewhere too.
There was a problem hiding this comment.
Yes, i copied the existing code. Updated 👍
|
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!! |
|
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. |
openshift-ci
bot
added
release-note
and removed
do-not-merge/release-note-label-needed
lstocchi
force-pushed
the
hypervElevatedExec
branch
from
bc58a14 to
369163b
Compare
@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? |
lstocchi
force-pushed
the
hypervElevatedExec
branch
from
369163b to
6709178
Compare
|
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 |
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
force-pushed
the
hypervElevatedExec
branch
from
6709178 to
ed47c52
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: lstocchi 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 |
Done, i followed the logic that was in place for the admin check |
|
Ephemeral COPR build failed. @containers/packit-build please check. |
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