From 161918a4af7c4f422dac477c955630f754f2e997 Mon Sep 17 00:00:00 2001 From: Conor Schaefer Date: Fri, 19 Oct 2018 01:40:03 -0400 Subject: [PATCH 1/9] Configures automatic updates to VMs via dom0 Using the "placeholder" top file strategy identified by @marmarek, in order to trigger automatic VM boots when Salt tasks target the VMs. Otherwise, Salt will report "SKIPPED" on the powered off VMs. Rather than manually boot the VMs each time we want to provision them, then power them off again, let's let Salt handle that. The 'securedrop-update' script can be run interactively by Admins, and is also configured to run once daily via cron, to ensure that updates are applied on a rather regular schedule. --- dom0/securedrop-update | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) create mode 100755 dom0/securedrop-update diff --git a/dom0/securedrop-update b/dom0/securedrop-update new file mode 100755 index 00000000..55cf40cf --- /dev/null +++ b/dom0/securedrop-update @@ -0,0 +1,39 @@ +#!/bin/bash +# Utility for dom- to ensure all updates are regularly installed +set -e +set -u + +# Number of VMs to update in parallel. Default is 4, +# which can be memory-intensive. +SECUREDROP_MAX_CONCURRENCY=2 + + +# Ensure elevated privileges +if [[ "$EUID" -ne 0 ]]; then + echo "Script must be run as root! Exiting..." + exit 1 +fi + +# Display GUI feedback about update process +function securedrop-update-feedback() { + # Unpack msg as arg1 + local msg="$1" + shift + + # Running `notify-send` as root doesn't work, must be normal user. + # Setting 30s expire time (in ms) since it's a long-running cmd. + su user -c "notify-send \ + --icon /usr/share/securedrop/icons/sd-logo.png \ + --expire-time 30000 \ + '$msg'" +} + +securedrop-update-feedback "SecureDrop: Updating dom0 configuration..." +sudo qubes-dom0-update -y + +securedrop-update-feedback "SecureDrop: Updating application..." +qubesctl --templates \ + --max-concurrency "$SECUREDROP_MAX_CONCURRENCY" \ + pkg.upgrade refresh=true + +securedrop-update-feedback "SecureDrop: All updates complete!" From 99660b56329f9b4fc658ff1cb3b800cf59b8cf48 Mon Sep 17 00:00:00 2001 From: Conor Schaefer Date: Fri, 19 Oct 2018 02:02:02 -0400 Subject: [PATCH 2/9] Writes configs for dom0 via salt We intend to package these dom0-specific config items into an RPM, but for now we'll continue to use Salt to copy the files around via the Makefile. Note that the `sd-dom0-files.sls` filename implies the list is comprehensive, but in fact there are dom0-specific configs scattered through the other SLS files, mostly VM specifications and RPC policy grants. --- Makefile | 8 +++++-- dom0/sd-dom0-files.sls | 48 ++++++++++++++++++++++++++++++++++++++++++ dom0/sd-dom0-files.top | 6 ++++++ dom0/sd-vm-updates.top | 9 ++++++++ 4 files changed, 69 insertions(+), 2 deletions(-) create mode 100644 dom0/sd-dom0-files.sls create mode 100644 dom0/sd-dom0-files.top create mode 100644 dom0/sd-vm-updates.top diff --git a/Makefile b/Makefile index af77dec0..24f27989 100644 --- a/Makefile +++ b/Makefile @@ -8,14 +8,13 @@ endif ## Builds and provisions all VMs required for testing workstation all: assert-dom0 validate clean update-fedora-templates \ - update-whonix-templates prep-whonix sd-workstation-template \ + update-whonix-templates prep-whonix prep-dom0 sd-workstation-template \ sd-whonix sd-svs sd-gpg \ sd-journalist sd-svs-disp clone: assert-dom0 ## Pulls the latest repo from work VM to dom0 @./scripts/clone-to-dom0 - sd-workstation-template: prep-salt ## Provisions base template for SDW AppVMs sudo qubesctl top.enable sd-workstation-template sudo qubesctl top.enable sd-workstation-template-files @@ -128,6 +127,11 @@ prep-whonix: ## enables apparmor on whonix-ws-14 and whonix-gw-14 qvm-prefs -s whonix-gw-14 kernelopts "nopat apparmor=1 security=apparmor" qvm-prefs -s whonix-ws-14 kernelopts "nopat apparmor=1 security=apparmor" +prep-dom0: prep-salt # Copies dom0 config files for VM updates + sudo qubesctl top.enable sd-vm-updates + sudo qubesctl top.enable sd-dom0-files + sudo qubesctl --targets dom0 state.highstate + list-vms: ## Prints all Qubes VMs managed by Workstation salt config @./scripts/list-vms diff --git a/dom0/sd-dom0-files.sls b/dom0/sd-dom0-files.sls new file mode 100644 index 00000000..8f71580e --- /dev/null +++ b/dom0/sd-dom0-files.sls @@ -0,0 +1,48 @@ +# -*- coding: utf-8 -*- +# vim: set syntax=yaml ts=2 sw=2 sts=2 et : + +## +# Installs dom0 config scripts specific to tracking updates +# over time. These scripts should be ported to an RPM package. +## + + +# Copy script to system location so admins can run ad-hoc +dom0-update-securedrop-script: + file.managed: + - name: /usr/bin/securedrop-update + - source: salt://securedrop-update + - user: root + - group: root + - mode: 755 + +# Symlink update script into cron, for single point of update +dom0-update-securedrop-script-cron: + file.symlink: + - name: /etc/cron.daily/securedrop-update-cron + - target: /usr/bin/securedrop-update + +# Create directory for storing SecureDrop-specific icons +dom0-securedrop-icons-directory: + file.directory: + - name: /usr/share/securedrop/icons + - user: root + - group: root + - mode: 755 + - makedirs: True + +# Copy SecureDrop icon for use in GUI feedback. It's also present in +# the Salt directory, but the permissions on that dir don't permit +# normal user reads. +dom0-securedrop-icon: + file.managed: + - name: /usr/share/securedrop/icons/sd-logo.png + - source: salt://sd/sd-journalist/logo-small.png + - user: root + - group: root + - mode: 644 + # Dependency on parent dir should be explicitly declared, + # but the require syntax below was throwing an error that the + # referenced task was "not available". + # require: + # - dom0-securedrop-icons-directory diff --git a/dom0/sd-dom0-files.top b/dom0/sd-dom0-files.top new file mode 100644 index 00000000..3509270d --- /dev/null +++ b/dom0/sd-dom0-files.top @@ -0,0 +1,6 @@ +# -*- coding: utf-8 -*- +# vim: set syntax=yaml ts=2 sw=2 sts=2 et : + +base: + dom0: + - sd-dom0-files diff --git a/dom0/sd-vm-updates.top b/dom0/sd-vm-updates.top new file mode 100644 index 00000000..f3b391d5 --- /dev/null +++ b/dom0/sd-vm-updates.top @@ -0,0 +1,9 @@ +# -*- coding: utf-8 -*- +# vim: set syntax=yaml ts=2 sw=2 sts=2 et : + +# "Placeholder" config to trigger TemplateVM boots, +# so upgrades can be applied automatically via cron. +base: + qubes:type:template: + - match: pillar + - topd From 9a5535fadf4a4c52ad37b948686da572ddb4f397 Mon Sep 17 00:00:00 2001 From: Conor Schaefer Date: Fri, 19 Oct 2018 13:48:08 -0400 Subject: [PATCH 3/9] Adds clarifying comments to the update script Factored in some advice received during pre-review. For now we're taking an interative approach to automating the updates. Currently we want, in order: 1. All dom0 RPMs up to date 2. All TemplateVMs up to date with packages (either RPMs or debs) What's not yet implemented is a strategy to automatically enforce the VM state regularly. That'll likely be a `qubesctl state.highstate` command, but punting for now to simplify testing of this already significant change. --- dom0/securedrop-update | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/dom0/securedrop-update b/dom0/securedrop-update index 55cf40cf..f5ecc59b 100755 --- a/dom0/securedrop-update +++ b/dom0/securedrop-update @@ -28,6 +28,9 @@ function securedrop-update-feedback() { '$msg'" } +# `qubesctl pkg.upgrade` will automatically update dom0 packages, as well, +# but we *first* want the freshest RPMs from dom0, *then* we'll want to +# update the VMs themselves. securedrop-update-feedback "SecureDrop: Updating dom0 configuration..." sudo qubes-dom0-update -y @@ -36,4 +39,8 @@ qubesctl --templates \ --max-concurrency "$SECUREDROP_MAX_CONCURRENCY" \ pkg.upgrade refresh=true +# Here would be a good place for state.highstate, to re-apply the VM configs. +# Let's first make sure the package upgrade logic is stable, we can circle +# back to enforce the Salt configs regularly. + securedrop-update-feedback "SecureDrop: All updates complete!" From 532a0ae35fa96badbe3a0894567e2fe6c4ae15b2 Mon Sep 17 00:00:00 2001 From: Conor Schaefer Date: Wed, 24 Oct 2018 14:55:38 -0400 Subject: [PATCH 4/9] Expands securedrop-update script Tackling requested changes during review: * supports custom dom0 usernames * omits --templates on pkg upgrade to include dom0 * uses state.highstate to enforce VM config * notify about reboot request (so updates are applied) We'll want to clean up the reboot recommendation once we have more UX feedback. For now, it's enough to notify that updates aren't actually in effect (due to AppVMs not having been restarted). --- dom0/securedrop-update | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/dom0/securedrop-update b/dom0/securedrop-update index f5ecc59b..00abb7a6 100755 --- a/dom0/securedrop-update +++ b/dom0/securedrop-update @@ -21,26 +21,34 @@ function securedrop-update-feedback() { shift # Running `notify-send` as root doesn't work, must be normal user. - # Setting 30s expire time (in ms) since it's a long-running cmd. - su user -c "notify-send \ + # Setting 60s expire time (in ms) since it's a long-running cmd. + local qubes_user + qubes_user="$(id -nu 1000)" + su "$qubes_user" -c "notify-send \ + --app-name 'SecureDrop Workstation' \ --icon /usr/share/securedrop/icons/sd-logo.png \ - --expire-time 30000 \ - '$msg'" + --expire-time 60000 \ + 'SecureDrop: $msg'" } # `qubesctl pkg.upgrade` will automatically update dom0 packages, as well, # but we *first* want the freshest RPMs from dom0, *then* we'll want to # update the VMs themselves. -securedrop-update-feedback "SecureDrop: Updating dom0 configuration..." -sudo qubes-dom0-update -y -securedrop-update-feedback "SecureDrop: Updating application..." -qubesctl --templates \ +securedrop-update-feedback "Updating application..." +qubesctl \ --max-concurrency "$SECUREDROP_MAX_CONCURRENCY" \ pkg.upgrade refresh=true +securedrop-update-feedback "Updating VM configuration..." +qubesctl \ + --max-concurrency "$SECUREDROP_MAX_CONCURRENCY" \ + state.highstate + # Here would be a good place for state.highstate, to re-apply the VM configs. # Let's first make sure the package upgrade logic is stable, we can circle # back to enforce the Salt configs regularly. -securedrop-update-feedback "SecureDrop: All updates complete!" +securedrop-update-feedback \ + "Updates installed. Please reboot the workstation \ +to ensure the latest security fixes are applied." From 5e9075c695ed90407f0748830842dd54f296c3bb Mon Sep 17 00:00:00 2001 From: mickael e Date: Thu, 25 Oct 2018 11:06:53 -0400 Subject: [PATCH 5/9] Tweak securedrop-update script - remove comments that were already addressed - restore dom0 package updates - perform update package action only in templates --- dom0/securedrop-update | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/dom0/securedrop-update b/dom0/securedrop-update index 00abb7a6..8a3eaaae 100755 --- a/dom0/securedrop-update +++ b/dom0/securedrop-update @@ -34,9 +34,11 @@ function securedrop-update-feedback() { # `qubesctl pkg.upgrade` will automatically update dom0 packages, as well, # but we *first* want the freshest RPMs from dom0, *then* we'll want to # update the VMs themselves. +securedrop-update-feedback "SecureDrop: Updating dom0 configuration..." +sudo qubes-dom0-update -y securedrop-update-feedback "Updating application..." -qubesctl \ +qubesctl --templates \ --max-concurrency "$SECUREDROP_MAX_CONCURRENCY" \ pkg.upgrade refresh=true @@ -45,10 +47,6 @@ qubesctl \ --max-concurrency "$SECUREDROP_MAX_CONCURRENCY" \ state.highstate -# Here would be a good place for state.highstate, to re-apply the VM configs. -# Let's first make sure the package upgrade logic is stable, we can circle -# back to enforce the Salt configs regularly. - securedrop-update-feedback \ "Updates installed. Please reboot the workstation \ to ensure the latest security fixes are applied." From b4106c938f9b231056f075d1b60335a7d95ec942 Mon Sep 17 00:00:00 2001 From: mickael e Date: Thu, 25 Oct 2018 12:12:54 -0400 Subject: [PATCH 6/9] Fix flake8 It seems like the flake8 container/rules have changed, mostly indentation. Ignore W605 is for invalid escape sequence '\s' in test_gpg.py:16 --- .flake8 | 2 ++ sd-journalist/sd-process-display | 2 +- sd-svs/decrypt-sd-submission | 4 ++-- tests/integration/test_integration | 30 +++++++++++++++--------------- tests/test_svs.py | 20 ++++++++++---------- 5 files changed, 30 insertions(+), 28 deletions(-) create mode 100644 .flake8 diff --git a/.flake8 b/.flake8 new file mode 100644 index 00000000..05bd31a4 --- /dev/null +++ b/.flake8 @@ -0,0 +1,2 @@ +[flake8] +ignore: W605 diff --git a/sd-journalist/sd-process-display b/sd-journalist/sd-process-display index 84696164..738c5440 100644 --- a/sd-journalist/sd-process-display +++ b/sd-journalist/sd-process-display @@ -1,7 +1,7 @@ import sys import PyQt4.QtCore as QtCore from PyQt4.QtGui import QDialog, QDialogButtonBox, QApplication, \ - QLabel, QVBoxLayout, QImage, QPixmap + QLabel, QVBoxLayout, QImage, QPixmap from PyQt4.QtCore import Qt import os import threading diff --git a/sd-svs/decrypt-sd-submission b/sd-svs/decrypt-sd-submission index d627aaad..ddf55634 100755 --- a/sd-svs/decrypt-sd-submission +++ b/sd-svs/decrypt-sd-submission @@ -37,7 +37,7 @@ try: # given a malicious tarball tar.extractall(tmpdir) -except Exception as e: +except Exception: send_progress("DECRYPTION_BUNDLE_OPEN_FAILURE") # although we're exiting with failure, we return a # 0 exit code so xdg does not try to re-open this file with @@ -139,7 +139,7 @@ try: print(os.path.join(tmpdir, "extracted")) distutils.dir_util.copy_tree(os.path.join(tmpdir, "extracted"), "/home/user/Sources/", update=1) -except Exception as e: +except Exception: send_progress("DECRYPTED_BUNDLE_COPY_FAILURE") shutil.rmtree(tmpdir) sys.exit(0) diff --git a/tests/integration/test_integration b/tests/integration/test_integration index dfa758ff..2d595517 100755 --- a/tests/integration/test_integration +++ b/tests/integration/test_integration @@ -142,13 +142,13 @@ def run_q(n): if run_q("full-success"): receiver.expected_states = [ - "DECRYPTION_PROCESS_START", - "SUBMISSION_BUNDLE_UNBUNDLED", - "SUBMISSION_FILES_EXTRACTED", - "SUBMISSION_FILE_DECRYPTION_SUCCEEDED", - "SUBMISSION_FILE_DECRYPTION_SUCCEEDED", - "DECRYPTED_BUNDLE_ON_SVS", - "DECRYPTED_FILES_AVAILABLE", + "DECRYPTION_PROCESS_START", + "SUBMISSION_BUNDLE_UNBUNDLED", + "SUBMISSION_FILES_EXTRACTED", + "SUBMISSION_FILE_DECRYPTION_SUCCEEDED", + "SUBMISSION_FILE_DECRYPTION_SUCCEEDED", + "DECRYPTED_BUNDLE_ON_SVS", + "DECRYPTED_FILES_AVAILABLE", ] receiver.cmd = ["qvm-open-in-vm", "sd-svs", "./all.sd-xfer"] receiver.test_name = "full successful open" @@ -156,8 +156,8 @@ if run_q("full-success"): if run_q("empty-download"): receiver.expected_states = [ - "DECRYPTION_PROCESS_START", - "DECRYPTION_BUNDLE_OPEN_FAILURE", + "DECRYPTION_PROCESS_START", + "DECRYPTION_BUNDLE_OPEN_FAILURE", ] receiver.cmd = ["qvm-open-in-vm", "sd-svs", "./empty.sd-xfer"] receiver.test_name = "empty sd-xfer" @@ -165,12 +165,12 @@ if run_q("empty-download"): if run_q("bad-gpg-key"): receiver.expected_states = [ - "DECRYPTION_PROCESS_START", - "SUBMISSION_BUNDLE_UNBUNDLED", - "SUBMISSION_FILES_EXTRACTED", - "SUBMISSION_FILE_DECRYPTION_FAILED", - "SUBMISSION_FILE_DECRYPTION_FAILED", - "SUBMISSION_FILE_NO_FILES_FOUND", + "DECRYPTION_PROCESS_START", + "SUBMISSION_BUNDLE_UNBUNDLED", + "SUBMISSION_FILES_EXTRACTED", + "SUBMISSION_FILE_DECRYPTION_FAILED", + "SUBMISSION_FILE_DECRYPTION_FAILED", + "SUBMISSION_FILE_NO_FILES_FOUND", ] receiver.cmd = ["qvm-open-in-vm", "sd-svs", "./bad-key.sd-xfer"] receiver.test_name = "bad encryption key" diff --git a/tests/test_svs.py b/tests/test_svs.py index 30d8e38f..3b933a51 100644 --- a/tests/test_svs.py +++ b/tests/test_svs.py @@ -10,28 +10,28 @@ def setUp(self): def test_decrypt_sd_submission(self): self.assertFilesMatch( - "/usr/bin/decrypt-sd-submission", - "sd-svs/decrypt-sd-submission") + "/usr/bin/decrypt-sd-submission", + "sd-svs/decrypt-sd-submission") def test_decrypt_sd_submission_desktop(self): self.assertFilesMatch( - "/usr/share/applications/decrypt-sd-submission.desktop", - "sd-svs/decrypt-sd-submission.desktop") + "/usr/share/applications/decrypt-sd-submission.desktop", + "sd-svs/decrypt-sd-submission.desktop") def test_decrypt_sd_user_profile(self): self.assertFilesMatch( - "/etc/profile.d/sd-svs-qubes-gpg-domain.sh", - "sd-svs/dot-profile") + "/etc/profile.d/sd-svs-qubes-gpg-domain.sh", + "sd-svs/dot-profile") def test_open_in_dvm_desktop(self): self.assertFilesMatch( - "/usr/share/applications/open-in-dvm.desktop", - "sd-svs/open-in-dvm.desktop") + "/usr/share/applications/open-in-dvm.desktop", + "sd-svs/open-in-dvm.desktop") def test_mimeapps(self): self.assertFilesMatch( - "/usr/share/applications/mimeapps.list", - "sd-svs/mimeapps.list") + "/usr/share/applications/mimeapps.list", + "sd-svs/mimeapps.list") def load_tests(loader, tests, pattern): From ded9423fabe6a8399c6a973c1ae41d042a196ad6 Mon Sep 17 00:00:00 2001 From: Conor Schaefer Date: Tue, 30 Oct 2018 10:25:56 -0700 Subject: [PATCH 7/9] Cleans up notifications in dom0 update logic Pointed out by @joshuathayer during review; the "SecureDrop:" prefix was redundant, since it's added by the display function. --- dom0/securedrop-update | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dom0/securedrop-update b/dom0/securedrop-update index 8a3eaaae..0cdf75c6 100755 --- a/dom0/securedrop-update +++ b/dom0/securedrop-update @@ -34,7 +34,7 @@ function securedrop-update-feedback() { # `qubesctl pkg.upgrade` will automatically update dom0 packages, as well, # but we *first* want the freshest RPMs from dom0, *then* we'll want to # update the VMs themselves. -securedrop-update-feedback "SecureDrop: Updating dom0 configuration..." +securedrop-update-feedback "Updating dom0 configuration..." sudo qubes-dom0-update -y securedrop-update-feedback "Updating application..." From 2ee0d932ec7c45fdb99cad5a2db3c58aef10e309 Mon Sep 17 00:00:00 2001 From: Conor Schaefer Date: Tue, 30 Oct 2018 19:29:26 -0400 Subject: [PATCH 8/9] Tests for apt packages up to date During review, @emkll caught that not all apt packages were updated as expected. These tests are a bit aggressive, and will fail if the AppVMs haven't been rebooted recently. That's a bit annoying, but I'd rather accept that friction than have a regression in the automatic upgrade logic. --- tests/test_vms_platform.py | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/tests/test_vms_platform.py b/tests/test_vms_platform.py index 06a94827..7c808630 100644 --- a/tests/test_vms_platform.py +++ b/tests/test_vms_platform.py @@ -38,6 +38,27 @@ def _validate_vm_platform(self, vm): platform = self._get_platform_info(vm) self.assertIn(platform, SUPPORTED_PLATFORMS) + def _ensure_packages_up_to_date(self, vm): + """ + Asserts that all available apt packages are installed; no pending + updates. + """ + cmd = "apt list --upgradable" + stdout, stderr = vm.run(cmd) + results = stdout.rstrip() + # `apt list` will always print "Listing..." to stdout, + # so expect only that string. + self.assertEqual(results, "Listing...") + + def test_all_sd_vms_uptodate(self): + """ + Asserts that all VMs have all available apt packages at the latest + versions, with no updates pending. + """ + for vm_name in WANTED_VMS: + vm = self.app.domains[vm_name] + self._ensure_packages_up_to_date(vm) + def test_sd_journalist_template(self): """ Asserts that the 'sd-journalist' VM is using a supported base OS. From 4c75e27893dc885d20857b7019e080ef63d467cb Mon Sep 17 00:00:00 2001 From: Conor Schaefer Date: Tue, 30 Oct 2018 20:59:07 -0400 Subject: [PATCH 9/9] Adds dist_upgrade to pkg.upgrade command Without `dist_upgrade=true`, the pkg.upgrade wasn't forcing all packages to their latest versions. This approach works well on Debian-based VMs, as all the SecureDrop Workstation components currently are, but there's a significant drawback: it silently fails on Fedora-based VMs, stating that the "--dist_upgrade" option is not valid for dnf. You must pass `--show-output` in order to observe the dnf failures; without it, the tasks are reported as "OK". Tried to use the "pkg.uptodate" Salt module rather than "pkg.uptodate", but the Qubes VMs reported that module wasn't available. The "dist_upgrade" option isn't explicitly documented [0], but presumably gets inherited via Salt magic from the aptpkg.upgrade module [1]. Adding `--skip-dom0` since we already upgraded dom0 packages via a previous step (qubes-dom0-update). [0] https://docs.saltstack.com/en/2017.7/ref/states/all/salt.states.pkg.html#salt.states.pkg.uptodate [1] https://docs.saltstack.com/en/2017.7/ref/modules/all/salt.modules.aptpkg.html#salt.modules.aptpkg.upgrade --- dom0/securedrop-update | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dom0/securedrop-update b/dom0/securedrop-update index 0cdf75c6..7a6c3812 100755 --- a/dom0/securedrop-update +++ b/dom0/securedrop-update @@ -38,9 +38,9 @@ securedrop-update-feedback "Updating dom0 configuration..." sudo qubes-dom0-update -y securedrop-update-feedback "Updating application..." -qubesctl --templates \ +qubesctl --skip-dom0 --templates \ --max-concurrency "$SECUREDROP_MAX_CONCURRENCY" \ - pkg.upgrade refresh=true + pkg.upgrade refresh=true dist_upgrade=true securedrop-update-feedback "Updating VM configuration..." qubesctl \