Enforce updates on login via dom0 launcher app #396

emkll · 2020-01-06T21:21:22Z

closes #341, closes freedomofpress/securedrop-updater#34

The goal is to enforce critical VM (incl dom0) updates on client login. For now, the interface is launched via the .desktop shortcut, but we also have this run on startup (similar to how updates are handled in #355)

Test Plan

make clone on this branch
make prep-dom0 succeeds without error
clicking on Desktop icon spawns the launcher
Logs for the updater appear ~/.securedrop-launcher/logs/launcher.log
The following assumes your workstation is already up-to-date since

Scenario 1: All VMs up to date:

The launcher simply runs, checks for updates, no updates are required, the client can be launched.
sd-svs flags are in place (/home/user/.securedrop.client/{sdw-last-updated,sdw-update-flag} are correct
dom0 flags (~/.securedrop-launcher/{sdw-last-updated,sdw-update-flag} are correct

Scenario 2: Reboot required

Open fedora-30, downgrade a package and shutdown the template (e.g: sudo dnf downgrade zlib && sudo poweroff)
The launcher indicates update is required
Updates are applied
The launcher recommends a user reboot
sd-svs flags are in place (/home/user/.securedrop.client/{sdw-last-updated,sdw-update-flag} are correct
dom0 flags (~/.securedrop-launcher/{sdw-last-updated,sdw-update-flag} are correct
after reboot, Scenario 1 completes without error

Scenario 3: Updates required but no reboot required

Open sd-svs-buster-template and downgrade securedrop-client (sudo apt install securedrop-client=<version> and shutdown the template
The launcher indicates update is required
Updates are applied
The launcher recommends you start the client
sd-svs flags are in place (/home/user/.securedrop.client/{sdw-last-updated,sdw-update-flag} are correct
dom0 flags (~/.securedrop-launcher/{sdw-last-updated,sdw-update-flag} are correct
The client starts on the latest version.

Follow-ups

~~Improve the UI theme/styling, (e.g., add SecureDrop logo)~~ Tweak updater icon, label #413
~~Parallelize updates (2x concurrency)~~ Perform update checks concurrently #403
~~Start the Launcher on boot, similar to Updates sd-svs-disp-template on XFCE login #355~~ 415
~~Use dom0 date flag to skip checks if check was done less than 8h ago~~ Skip VM updates if last check was performed <X hours ago #402
~~remove daily securedrop-update-cron once 4. is completed (per [WIP] Add security update notification script #389 (comment))~~ Remove update cron job #412
~~Only reboot AppVMs that need rebooting (AppVMs whose TemplateVM was updated as part of this run, see Enforce updates on login via dom0 launcher app #396 (comment))~~ After applying updates, only reboot VMs that need updating securedrop-updater#38

redshiftzero

this is awesome work, I've provisioned and tried this out, works great, a few questions/thoughts inline

launcher/sdw_updater_gui/UpdaterApp.py

dom0/securedrop-updater-cli

launcher/sdw_updater_gui/sdw_updater.ui

redshiftzero · 2020-01-07T17:29:29Z

note to other people trying this that right now the strings listed in #341 (comment) aren't used - this PR thus far is just a demo of the basic functionality, we can update the text shown to the user (and probably hide the window that has the full details of what is happening) once we agree on the basic functionality/technical approach

dom0/securedrop-updater-cli

redshiftzero

(posting some notes made from synchronous PR review)

dom0/securedrop-updater-cli

launcher/sdw_updater_gui/UpdaterApp.py

launcher/sdw_updater_gui/UpdaterAppUi.py

eloquence · 2020-01-10T01:18:34Z

I may be catching this branch at a bad time but just noting that the progress bar is currently frozen at 1% for me during the "checking for updates" stage (it immediately switches to 100% once the checks are complete).

eloquence · 2020-01-10T01:19:37Z

(Regarding the icon, I noticed that it's possible to adjust the Qubes icon settings to a higher size, which also means more text for the label. It's set to 64 pixels in the above screenshot. If it's possible to change that preference during provisioning, that may be a nice follow-up tweak.)

eloquence · 2020-01-10T01:48:53Z

I'm noticing that every time I run the launcher, the VMs get the green "restart required" indicator in the Qube manager widget (see top right in the screenshot below), even if no updates were available.

To verify this behavior, I did restart the sd-svs VM, which removed the indicator, then ran the launcher (via shortcut) to check for updates (none were available). Then I checked the Qube manager widget, and the green "restart required" indicator was back next to sd-svs.

redshiftzero

looking really good. @kushaldas or others if you have comments or suggestions on the overall approach and code structure here please chime in as I think this will be ready for official review soon

launcher/sdw_updater_gui/UpdaterApp.py

redshiftzero · 2020-01-13T21:47:07Z

launcher/sdw_updater_gui/Updater.py

+        sdlog.error(str(e))
+        updates_required = True
+    finally:
+        try:


nit/optional: candidate for factoring out into separate function (code in the finally block is used in 2 places), would make testing a bit easier for these functions, but also fine to leave since it's only used in two places (if we end up adding a third we can factor out at that time)

launcher/sdw_updater_gui/UpdaterApp.py

launcher/tests/test_updater.py

launcher/sdw_updater_gui/Updater.py

Will now shut down all VMs, and then boot all VMs to pick up updates.

This reduces the amount of callbacks and threads required, and simply update the progressBar from {Update,Upgrade}Threads by emitting a signal to update_progress_bar slot

This is required because Qubes will autostart VMs, and this guards against failure of `qvm-start` in those scenarios.

- improve generator usage - improve file handling - remove variables that are no longer use - correct comment

…ndow and prevent resizing

Use <hn> tags in lieu of standalone label tags, with rich text markup

- Upgrade dict returned by the app is now properly populated with results - svs flag is now in /home/user/.securedrop.client/

- Add Cancel button - Add Horizontal Spacer to button row to alight buttons to the right - Fix failed update scenario - Fix vertical margins/alignment

- sd-svs -> sd-app - sd-export -> sd-devices - sd-svs-disp -> sd-viewer

If a user were to close/exit the launcher and re-run it, they would be able to circumvent the required reboot. To ensure reboots were applied, we will now track the timestamp for the dom0 update status flag, and compare with the current system boot time. If the boot time is before the reboot flag, the launcher will ask the user to reboot, else it will apply whatever status is required.

redshiftzero

I've confirmed all of my comments have been addressed, thanks for your patience on this @emkll

I performed a full functional test of this in Qubes after reprovisioning on latest master (after the VM name changes):

make prep-dom0 succeeds without error
clicking on Desktop icon spawns the launcher
Logs for the updater appear in ~/.securedrop-launcher/logs/launcher.log

Scenario 1: All VMs up to date:

The launcher simply runs, checks for updates, no updates are required, the client can be launched.
sd-svs flags are in place (/home/user/.securedrop.client/{sdw-last-updated,sdw-update-flag} are correct
dom0 flags (~/.securedrop-launcher/{sdw-last-updated,sdw-update-flag} are correct

Scenario 2: Reboot required

The launcher indicates update is required
Updates are applied
The launcher recommends a user reboot
sd-svs flags are in place (/home/user/.securedrop.client/{sdw-last-updated,sdw-update-flag} are correct
dom0 flags (~/.securedrop-launcher/{sdw-last-updated,sdw-update-flag} are correct
after reboot, Scenario 1 completes without error

Scenario 3: Updates required but no reboot required

The launcher indicates update is required
Updates are applied
The launcher recommends you start the client
sd-svs flags are in place (/home/user/.securedrop.client/{sdw-last-updated,sdw-update-flag} are correct
dom0 flags (~/.securedrop-launcher/{sdw-last-updated,sdw-update-flag} are correct
The client starts on the latest version.

one other followup we can file is adding the reboot tasks to the progress bar (such that it's not stalled at 100% during the reboot cycle)

I'm going to hold off on formally approving until a second person can review (and ideally test) since given the criticality of this feature it would be good to have another set of eyes. If no one is available given our other wip, I will merge regardless in a couple of days.

redshiftzero

@conorsch mentioned in chat he's reviewing this so since we might get a second set of eyes before EOD I will approve so that my review is not blocking. Confirming once more for the comment skimmer that all my comments are addressed.

conorsch

Changes are solid. Performed a visual review of the diff, and have no requests on top of what's already been discussed (and implemented). In terms of functional review, have completed scenarios 1 & 3, with no major snags. The follow-ups identified in the OP are indeed worth tracking in separate tickets (e.g. #411).

Mighty fine work here, @emkll.

conorsch · 2020-01-22T01:22:19Z

In order to unblock merge, I'm dismissing @kushaldas's prior review. Have confirmed that the changes requested were indeed addressed. Great attention to detail in that review—thanks, @kushaldas!

All changes have been addressed! \m/

emkll added the needs discussion label Jan 6, 2020

emkll force-pushed the 341-enforce-updates-client-login branch from 2e3d863 to ab0ce6f Compare January 6, 2020 22:19

emkll added this to Discussion Points in SecureDrop Team Board Jan 7, 2020

redshiftzero self-requested a review January 7, 2020 15:39

redshiftzero reviewed Jan 7, 2020

View reviewed changes

emkll force-pushed the 341-enforce-updates-client-login branch from ce3b8a2 to 5e8bba3 Compare January 7, 2020 18:29

eloquence moved this from Discussion Points to In Development in SecureDrop Team Board Jan 7, 2020

emkll force-pushed the 341-enforce-updates-client-login branch from 5e8bba3 to 3b42bd4 Compare January 8, 2020 16:16

redshiftzero reviewed Jan 9, 2020

View reviewed changes

dom0/securedrop-updater-cli Outdated Show resolved Hide resolved

redshiftzero suggested changes Jan 9, 2020

View reviewed changes

eloquence mentioned this pull request Jan 9, 2020

Skip VM updates if last check was performed <X hours ago #402

Closed

emkll force-pushed the 341-enforce-updates-client-login branch from a34c8ce to 5cf777c Compare January 9, 2020 23:26

eloquence mentioned this pull request Jan 10, 2020

Perform update checks concurrently #403

Closed

ninavizz mentioned this pull request Jan 10, 2020

Workstation LogIn Updater freedomofpress/securedrop-ux#98

Closed

emkll force-pushed the 341-enforce-updates-client-login branch 4 times, most recently from c484c5d to 7a2561d Compare January 13, 2020 21:16

redshiftzero reviewed Jan 13, 2020

View reviewed changes

launcher/sdw_updater_gui/Updater.py Outdated Show resolved Hide resolved

This was referenced Jan 13, 2020

Display system alert recommending shutdown after 5 days of activity #236

Closed

[WIP] Add security update notification script #389

Closed

emkll force-pushed the 341-enforce-updates-client-login branch 4 times, most recently from 6674cf4 to 9e8d1ff Compare January 14, 2020 14:42

emkll and others added 13 commits January 20, 2020 14:42

Add test coverage

2356a92

Fix reboot logic

34aecda

Will now shut down all VMs, and then boot all VMs to pick up updates.

Use generator to iterate through update checks/upgrades

fe1ab6b

This reduces the amount of callbacks and threads required, and simply update the progressBar from {Update,Upgrade}Threads by emitting a signal to update_progress_bar slot

Only use tempfiles for testing for dom0, sd-svs paths are not required.

4bab247

Use --skip-if-running when starting VMs

b938816

This is required because Qubes will autostart VMs, and this guards against failure of `qvm-start` in those scenarios.

Re-use shutdown function instead of calling qvm-shutdown

f2abeab

Address review comments:

e0a21c6

- improve generator usage - improve file handling - remove variables that are no longer use - correct comment

UI fixes: Remove redundant text box, hide buttons in-place, resize wi…

e3b3a3a

…ndow and prevent resizing

Update text

ce048c7

Use <hn> tags in lieu of standalone label tags, with rich text markup

Address review comments

3d4ce23

- Upgrade dict returned by the app is now properly populated with results - svs flag is now in /home/user/.securedrop.client/

Bug/UI fixes:

8ede5f4

- Add Cancel button - Add Horizontal Spacer to button row to alight buttons to the right - Fix failed update scenario - Fix vertical margins/alignment

Edits mainly for brevity, formatting, punctuation tweaks

2728e59

Rename VMs based on latest changes

91efa24

- sd-svs -> sd-app - sd-export -> sd-devices - sd-svs-disp -> sd-viewer

emkll force-pushed the 341-enforce-updates-client-login branch from 607ed78 to 91efa24 Compare January 20, 2020 19:51

redshiftzero requested a review from kushaldas January 22, 2020 00:29

redshiftzero reviewed Jan 22, 2020

View reviewed changes

redshiftzero approved these changes Jan 22, 2020

View reviewed changes

eloquence mentioned this pull request Jan 22, 2020

Updater progress bar should only reach 100% once all related tasks have been completed #411

Closed

conorsch self-requested a review January 22, 2020 01:17

conorsch approved these changes Jan 22, 2020

View reviewed changes

conorsch merged commit 08a064e into master Jan 22, 2020

SecureDrop Team Board automation moved this from Under Review to Done Jan 22, 2020

eloquence mentioned this pull request Jan 22, 2020

Remove update cron job #412

Closed

This was referenced Jan 22, 2020

Parallelize updates / update checks #414

Closed

Start updater at boot, remove existing svs-disp backgound updater #415

Closed

This was referenced Jan 23, 2020

Enforce dom0 salt state as part of preflight updates #427

Closed

Remove legacy updater #430

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enforce updates on login via dom0 launcher app #396

Enforce updates on login via dom0 launcher app #396

emkll commented Jan 6, 2020 •

edited

redshiftzero left a comment

redshiftzero commented Jan 7, 2020

redshiftzero left a comment

eloquence commented Jan 10, 2020

eloquence commented Jan 10, 2020

eloquence commented Jan 10, 2020 •

edited

redshiftzero left a comment

redshiftzero Jan 13, 2020

redshiftzero left a comment

redshiftzero left a comment

conorsch left a comment

conorsch commented Jan 22, 2020

Enforce updates on login via dom0 launcher app #396

Enforce updates on login via dom0 launcher app #396

Conversation

emkll commented Jan 6, 2020 • edited

Test Plan

Scenario 1: All VMs up to date:

Scenario 2: Reboot required

Scenario 3: Updates required but no reboot required

Follow-ups

redshiftzero left a comment

Choose a reason for hiding this comment

redshiftzero commented Jan 7, 2020

redshiftzero left a comment

Choose a reason for hiding this comment

eloquence commented Jan 10, 2020

eloquence commented Jan 10, 2020

eloquence commented Jan 10, 2020 • edited

redshiftzero left a comment

Choose a reason for hiding this comment

redshiftzero Jan 13, 2020

Choose a reason for hiding this comment

redshiftzero left a comment

Choose a reason for hiding this comment

Scenario 1: All VMs up to date:

Scenario 2: Reboot required

Scenario 3: Updates required but no reboot required

redshiftzero left a comment

Choose a reason for hiding this comment

conorsch left a comment

Choose a reason for hiding this comment

conorsch commented Jan 22, 2020

emkll commented Jan 6, 2020 •

edited

eloquence commented Jan 10, 2020 •

edited