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

Set named dispvm sd-proxy as a servicevm #1146

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

Conversation

rocodes
Copy link
Contributor

@rocodes rocodes commented Jul 11, 2024

Status

Ready for review

Description of Changes

Fixes #1145

Changes proposed in this pull request:

Set named qubes feature servicevm for named dispvm sd-proxy during Salt run.

Note: servicevm is approximately taken to mean "provides network" (https://github.com/QubesOS/qubes-desktop-linux-menu/blob/1ee90aa09903dbd470f540b22e48d0a500feff6e/qubes_menu/vm_manager.py#L122-L125), so this will show that sd-proxy "provides a network" in the appmenu (the network-connected icon shows up beside the qube). However, unlike qubes that really do provide a network, we don't set the qvm-pref provides_network to True for sd-proxy, so it won't show up as a networking option when new qubes are created. (This is good; we wouldn't want that.)

So we may be acting like outliers here by setting the qvm-feature (for menu + restart behaviour) but not the qvm-pref (for what capabilities it advertises to other Qubes).

Testing

Deployment

Any special considerations for deployment? Consider both:

  1. Upgrading existing pilot instances
  2. New installs

Checklist

If you have made changes to the provisioning logic

  • All tests (make test) pass in dom0

If you have added or removed files

  • I have updated MANIFEST.in and rpm-build/SPECS/securedrop-workstation-dom0-config.spec

If documentation is required

  • I have opened a PR in the docs repo for these changes, or will do so later
  • I would appreciate help with the documentation

@rocodes rocodes requested a review from a team July 11, 2024 14:23
@legoktm
Copy link
Member

legoktm commented Jul 11, 2024

Note: servicevm is approximately taken to mean "provides network" (https://github.com/QubesOS/qubes-desktop-linux-menu/blob/1ee90aa09903dbd470f540b22e48d0a500feff6e/qubes_menu/vm_manager.py#L122-L125), so this will show that sd-proxy "provides a network" in the appmenu (the network-connected icon shows up beside the qube).

TIL, thanks for finding that.

So we may be acting like outliers here by setting the qvm-feature (for menu + restart behaviour) but not the qvm-pref (for what capabilities it advertises to other Qubes).

Ack, we should probably check in with the Qubes team that this won't have any problematic consequences then.

Copy link
Contributor

@deeplow deeplow left a comment

Choose a reason for hiding this comment

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

LGTM (after checking upstream). I played around with this behavior last week and its seems to do what we want it to do. The menu (and even the Qubes updater) recognize it as a proper serviceVM.

@deeplow
Copy link
Contributor

deeplow commented Jul 15, 2024

Interesting! All this time I was thinking that Qubes had no internal concept of what a sys-vm was. I thought it was basically any qube which has " ☑️ provides network to other qubes". Hence why by default sys-usb providing network even though that makes no sense at all (looked this up now and there QubesOS/qubes-issues#7669 (comment) to be some reasoning behind it).

Thanks to this, I was also (hopefully) able to resolve the annoying situation with sys-usb QubesOS/qubes-mgmt-salt-dom0-virtual-machines#63 where if one takes the provides network (which makes sense in most cases anyways), it would stop being in the "services" tab in the menu.

So my guess it that servicevm feature exists to deal with exactly these kinds of situations and that we are in the clear about the change.

@deeplow
Copy link
Contributor

deeplow commented Jul 22, 2024

Just a follow up: Marek alerted to the fact that the servicevm property will be reset if provides network is ever toggled on and then off again. We don't expect that to happen here, but we're relying on some assumptions about how this particular feature was implemented upstream.

I'll see if we can conceptually detach servicevm from provides_network so that we're not relying on this "hack". It still seems to follow the glossary definition of servicevm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mark sd-proxy as a service VM so the updater will restart it
3 participants