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 menu items for sd-devices and sd-whonix #1112

Merged
merged 1 commit into from
Jul 3, 2024
Merged

Set menu items for sd-devices and sd-whonix #1112

merged 1 commit into from
Jul 3, 2024

Conversation

legoktm
Copy link
Member

@legoktm legoktm commented Jun 27, 2024

Status

Ready for review

Description of Changes

These are the only two VMs that will be visible on production systems and have specific tools we want users to be able to directly start.

  • sd-devices: Files (Nautilus) and Disks
  • sd-whonix: Anon Connection Wizard and Tor Control Panel

Ideally we would do all of this in salt, but since we need to run stuff via dom0 after we do things in the VMs, it would require adjusting the order of some salt states, which is a bit too intrusive for a fix during the RC/QA period.

A TODO indicates that this is not an ideal situation, and freedomofpress/securedrop-client#2078 tracks one potential fix

Fixes #520.
Fixes #1109.

Testing

  • Provision an SDW dev instance (or staging would be fine too)
  • menu items for sd-devices are exactly set(Files, Disks)
  • menu items for sd-whonix are exactly set(Anon Connection Wizard, Tor Control Panel)

Deployment

n/a, only supporting new installs for 1.0.0

Checklist

  • All tests (make test) pass in dom0
    • n.b. I didn't add a test for this as I felt asserting the same features we set are present wasn't useful right now.

@legoktm
Copy link
Member Author

legoktm commented Jun 27, 2024

For some reason the menu is only showing me "Files" for sd-devices but when I open up the Qubes Manager, I see both Disks/Files listed under "Applications show in App Menu"

@legoktm
Copy link
Member Author

legoktm commented Jun 27, 2024

I noticed that there was no entry for Disks in ~/.local/share/qubes-appmenus/apps, so calling qvm-sync-appmenus --regenerate-only sd-devices fixes it. But I don't understand why that wasn't necessary for the other VMs...

@legoktm legoktm marked this pull request as draft June 28, 2024 14:58
@legoktm legoktm marked this pull request as draft June 28, 2024 14:58
@legoktm
Copy link
Member Author

legoktm commented Jun 28, 2024

From what I can tell, the problem is that the dom0 salt state that runs qvm-sync-appmenus happens before the securedrop-export package is installed, so gnome-disk-utility isn't installed yet.

@rocodes
Copy link
Contributor

rocodes commented Jun 28, 2024

I was initially surprised to learn that the customizations belong on sd-devices and not sd-devices-dvm, but it makes sense (and maybe that explains the difference in behaviour between that and sd-whonix, which is not disposable?).

Re sd-app: Since we're hiding it from showing up on the menu, I don't have strong feelings about the sync-appmenus step. (I do think though that it is friendly to leave a terminal application at minimum in all of our VMs, even the ones that are internal, but not a huge deal). The only caveat to that would be if syncing appmenus also had other effects (eg updating other/linked files that are used elsewhere in the system, such as global settings menus etc). I took a quick peek through qubes-desktop-linux-common (where the appmenus stuff lives) and don't see any obvious gotchas or side effects but haven't looked closely

@legoktm legoktm marked this pull request as ready for review June 28, 2024 17:29
@legoktm
Copy link
Member Author

legoktm commented Jun 28, 2024

From what I can tell, the problem is that the dom0 salt state that runs qvm-sync-appmenus happens before the securedrop-export package is installed, so gnome-disk-utility isn't installed yet.

I discussed this with @rocodes who explained that we'd need to add another salt run in provision-all so we can run stuff in dom0 after we've provisioned VMs, which is doable but more involved. I suggested we just do the qvm-sync-appmenu calls directly in that script as a hack to avoid needing to adjust a lot of salt stuff for now. Ro was also looking into and thinking that we can do this in the deb package postinst, which is tracked as freedomofpress/securedrop-client#2078. I think in the long run we want it in either salt or deb packages (or RPM I suppose) instead of manually like this, but this is acceptable as a narrow fix during the QA period.

Re sd-app: Since we're hiding it from showing up on the menu, I don't have strong feelings about the sync-appmenus step. (I do think though that it is friendly to leave a terminal application at minimum in all of our VMs, even the ones that are internal, but not a huge deal). The only caveat to that would be if syncing appmenus also had other effects (eg updating other/linked files that are used elsewhere in the system, such as global settings menus etc).

I restored the sync for the small template just to avoid us needing to think/reason about whether removing it will have other effects. I don't think there's any real downside to keeping it.

@rocodes
Copy link
Contributor

rocodes commented Jun 28, 2024

Thanks for summarizing - yeah so what I was saying that calling highstate on dom0 in the provisoin-all script means that all the dom0 actions happen together, but some of those actions (create vms) must happen before deb packages are installed in VMs, whereas some (appmenu sync) must happen after actions are installed in vms.

The 3 options I saw are:

  • switch qubesctl --show-output state.highstate to qubesctl --show-output state.sls <list all dom0 salt states except the qvm-sync-appmenus>, then later, after the right packages have been installed in the right vms, and apply the dom0 highstate (which will just apply the appmenu sync and everything else will be a no-op) or just the qvm-appmenus state in a separate provision-all line. This is ..fine, but a bit brittle.
  • Do as is done here and take the appmenu sync out of Salt, calling it at the right time (fine workaround in the interests of time, assuming no issues around waiting for the appmenu sync to finish)
  • Do as is suggested in Sync appmenus in package postinst securedrop-client#2078, which I think is the most correct way, but needs an update to securedrop client packages + a quick bit of testing so we'll do it as a followup

qvm-start --skip-if-running sd-large-bookworm-template && qvm-sync-appmenus sd-large-bookworm-template
qvm-start --skip-if-running whonix-gateway-17 && qvm-sync-appmenus whonix-gateway-17
# These are the two ones we show in prod VMs, so sync explicitly
qvm-sync-appmenus --regenerate-only sd-devices
Copy link
Contributor

@rocodes rocodes Jun 28, 2024

Choose a reason for hiding this comment

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

  • On systems with 16-24GB of RAM, this could be a lot of running VMs at once (because some of $all_sdw_vms_target are running too); I wonder if we'll run into memory errors. Might be worth it to shut down the templates after, and/or if we have to do this in batches now that we have more dvm templates and named dispvms. Haven't tested, just a thought

  • do we need to check if sd-devices and sd-whonix are running before qvm-sync-appmenus --regenerate-only on them? I know there's no error but I haven't checked if that's a mistake with the script or if it's fine if they're powered off.

Copy link
Member Author

Choose a reason for hiding this comment

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

Might be worth it to shut down the templates after

I can add shutdown after each one just in case.

do we need to check if sd-devices and sd-whonix are running before qvm-sync-appmenus --regenerate-only on them? I know there's no error but I haven't checked if that's a mistake with the script or if it's fine if they're powered off.

Because we do --regenerate-only, we don't need to start the VM. My mental model is that this step is just copying the desktop entries from the template VMs list in dom0 (which we just synced) and applying it to the specific AppVM. I'm not sure this is exactly right, but I will add it to the agenda for Tuesday's Qubes sync.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can add shutdown after each one just in case.

I didn't add --wait to qvm-shutdown for speed purposes, if you disagree, I'm totally fine adding it in.

These are the only two VMs that will be visible on production systems
and have specific tools we want users to be able to directly start.

* sd-devices: Files (Nautilus) and Disks
* sd-whonix: Anon Connection Wizard and Tor Control Panel

Ideally we would do all of this in salt, but since we need to run stuff
via dom0 after we do things in the VMs, it would require adjusting the
order of some salt states, which is a bit too intrusive for a fix during
the RC/QA period.

A TODO indicates that this is not an ideal situation, and
<freedomofpress/securedrop-client#2078> tracks
one potential fix.

Fixes #520.
Fixes #1109.
@rocodes
Copy link
Contributor

rocodes commented Jul 1, 2024

I'm just looking closer at the vm features. It looks like you can inspect a whitelist of permitted vms via qvm-appmenus --get-whitelist $vm.

qvm-features $vmname menu-items "item1.desktop item2.desktop" works for me at the commandline - I can set new menu items for sd-devices and they are immediately updated in the app menu without any other commands.

2 notes:

  • qvm-appmenus --remove $vm does remove the whitelist (so there are no menu entries visible for that VM in dom0), but a) entries are still accessible via get-whitelist and in qvm features, and b) some entries are broken even after attempting to regenerate the list. (I think this is a bug, going to see if I can get a clearer sense before I file anything upstream, but just a note of caution)

  • to see a list of menu items from dom0 commandline for a given vm, I used the very not prod ready --get-whitelist, but this is...well...strongly discouraged would be an understatement ;)

$ qvm-appmenus --get-available sd-devices
usage: [snipped]
error: this requires --i-understand-format-is-unstable and a sacrifice of one cute kitten

🙃

@legoktm
Copy link
Member Author

legoktm commented Jul 2, 2024

Discussion in today's Qubes meeting was that the post-install hook in the VMs should be running automatically, and if not, it's a bug. I'm going to create an isolated STR and file that upstream.

@rocodes rocodes self-assigned this Jul 3, 2024
Copy link
Contributor

@rocodes rocodes left a comment

Choose a reason for hiding this comment

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

LGTM - provisioning completes successfully, sd-whonix has Connection Wizard and Control Panel, sd-devices has Disks and Files.

During provisioning I noticed two appmenu related errors in the console log:
Failed to get icon for open-in-dvm: No icon received for sd-small-bookworm-template (it's true. there's no icon for that) and

Warning: not creating/updating '/home/$USER/.local/share/qubes-appmenus/sd-large-bullseye-template/apps.templates/send-to-usb.desktop' because of missing 'Name' key.

I will look into this separately in the interests of time.

@rocodes rocodes added this pull request to the merge queue Jul 3, 2024
Merged via the queue into main with commit a36ef71 Jul 3, 2024
7 checks passed
@legoktm legoktm deleted the menu-items branch August 29, 2024 22:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

sd-devices menu should include file manager Run qvm-sync-appmenus on AppVMs
2 participants