Skip to content

add option to relax socket_vmnet validation #2662

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

Closed

Conversation

avoidik
Copy link

@avoidik avoidik commented Sep 27, 2024

hello,

with this merge request I'd like to introduce an option to be able to relax socket_vmnet verification logic

regards

@avoidik avoidik force-pushed the feature/relax-socket-vmnet-verification branch from d58db8d to c71a83e Compare September 27, 2024 09:53
Signed-off-by: Viacheslav Vasilyev <[email protected]>
Signed-off-by: Viacheslav Vasilyev <[email protected]>
Signed-off-by: Viacheslav Vasilyev <[email protected]>
Copy link
Member

@jandubois jandubois left a comment

Choose a reason for hiding this comment

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

I don't think this is needed; you can just delete the sudoers entry in the network.yaml file and enable password-less sudo on the machine. Please file an issue (or PR) if that isn't working.

Allowing relaxed verification is already opening the possibility of privilege escalation without a password prompt, so requiring the user to enable it makes the risk explicit.

Thanks for filing this PR though, it made me look at the code and realize that we should not even create the sudoers file unless all binaries are secure (#2663).

@avoidik
Copy link
Author

avoidik commented Sep 28, 2024

@jandubois it won't work if a user is not a member of the admin group, although is listed in the sudoers file and is an owner of the directory, here is a relevant error message (with the sudoers line removed in the networks.yaml file):

$ limactl start --name docker-cri-aarch64 --arch aarch64 --cpus 2 --memory 4 --disk 20 --vm-type vz --tty=false template://vmnet
INFO[0000] Terminal is not available, proceeding without opening an editor
WARN[0000] `vmType: vz` is experimental
FATA[0000] networks.yaml field `paths.socketVMNet` error: dir "/opt/homebrew/Cellar/socket_vmnet/1.1.4" owner XXXis not an admin
$ ls -ld /opt/homebrew/Cellar/socket_vmnet/1.1.4
drwxr-xr-x@ 11 user.name  admin  352 Sep 26 21:42 /opt/homebrew/Cellar/socket_vmnet/1.1.4
$ echo $USER
user.name
$ id -u
XXX

this check is in this line:

adminGroup, err := user.LookupGroup("admin")

since I'm a sudoer user already, I'm able to change the homebrew's socket_vmnet directory owner to an admin, so that the check passes, however it then complains that the entire directories chain up to / should be owned by a root (or any user which is a member of the admin group)

$ sudo chown -R root:admin /opt/homebrew/Cellar/socket_vmnet/1.1.4
$ limactl start --name docker-cri-aarch64 --arch aarch64 --cpus 2 --memory 4 --disk 20 --vm-type vz --tty=false template://vmnet
INFO[0000] Terminal is not available, proceeding without opening an editor
WARN[0000] `vmType: vz` is experimental
FATA[0000] networks.yaml field `paths.socketVMNet` error: dir "/opt/homebrew/Cellar/socket_vmnet" owner XXXis not an admin
$ sudo chown -R root:admin /opt/homebrew/Cellar/socket_vmnet/
$ limactl start --name docker-cri-aarch64 --arch aarch64 --cpus 2 --memory 4 --disk 20 --vm-type vz --tty=false template://vmnet
INFO[0000] Terminal is not available, proceeding without opening an editor
WARN[0000] `vmType: vz` is experimental
FATA[0000] networks.yaml field `paths.socketVMNet` error: dir "/opt/homebrew/Cellar" owner XXXis not an admin

@jandubois
Copy link
Member

however it then complains that the entire directories chain up to / should be owned by a root (or any user which is a member of the admin group)

Yes, this is necessary to keep this feature secure. I've shown in #1437 (comment) how you can enable password-less sudo and remove the configuration setting for the sudoers file. Then limactl start will no longer verify the security of the daemon executable because you allow execution of any binary by root anyways.

@jandubois jandubois closed this Sep 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants