From 841c37841af934103387e7cc71fef941fdf71cff Mon Sep 17 00:00:00 2001 From: mickael e Date: Wed, 29 May 2019 10:18:11 -0400 Subject: [PATCH] Address review comments * rename sd-export-dvm to rename-export-usb-dvm * remove executable bit on certain files * improved error/exception handling --- .flake8 | 1 + Makefile | 4 +-- dom0/sd-export-files.sls | 4 +-- dom0/sd-export.sls | 6 ++-- scripts/list-vms | 2 +- sd-export/send-to-usb | 66 ++++++++++++++++++++++++---------------- tests/test_sd_export.py | 2 +- tests/test_vms_exist.py | 2 +- 8 files changed, 51 insertions(+), 36 deletions(-) diff --git a/.flake8 b/.flake8 index 05bd31a48..1de5103ed 100644 --- a/.flake8 +++ b/.flake8 @@ -1,2 +1,3 @@ [flake8] ignore: W605 +max-line-length = 99 diff --git a/Makefile b/Makefile index 02fe2185c..d8af40113 100644 --- a/Makefile +++ b/Makefile @@ -58,7 +58,7 @@ sd-export: prep-salt ## Provisions SD Export VM sudo qubesctl top.enable sd-export sudo qubesctl top.enable sd-export-files sudo qubesctl --show-output --targets sd-export-template state.highstate - sudo qubesctl --show-output --targets sd-export-dvm state.highstate + sudo qubesctl --show-output --targets sd-export-export-dvm state.highstate clean-salt: assert-dom0 ## Purges SD Salt configuration from dom0 @echo "Purging Salt config..." @@ -88,7 +88,7 @@ remove-sd-export: assert-dom0 ## Destroys SD EXPORT VMs @qvm-kill sd-export-usb || true @qvm-usb detach sd-export-usb || true @./scripts/destroy-vm sd-export-usb - @./scripts/destroy-vm sd-export-dvm + @./scripts/destroy-vm sd-export-usb-dvm clean: assert-dom0 destroy-all clean-salt ## Destroys all SD VMs sudo dnf -y -q remove securedrop-workstation-dom0-config || true diff --git a/dom0/sd-export-files.sls b/dom0/sd-export-files.sls index 99a0b90b9..8bcf8664c 100644 --- a/dom0/sd-export-files.sls +++ b/dom0/sd-export-files.sls @@ -26,7 +26,7 @@ sd-export-desktop-file: - source: salt://sd/sd-export/send-to-usb.desktop - user: root - group: root - - mode: 755 + - mode: 644 - makedirs: True cmd.run: - name: sudo update-desktop-database /usr/share/applications @@ -39,7 +39,7 @@ sd-export-file-format: - source: salt://sd/sd-export/application-x-sd-export.xml - user: root - group: root - - mode: 755 + - mode: 644 - makedirs: True cmd.run: - name: sudo update-mime-database /usr/share/mime diff --git a/dom0/sd-export.sls b/dom0/sd-export.sls index c4fca8cb5..a947aa383 100644 --- a/dom0/sd-export.sls +++ b/dom0/sd-export.sls @@ -20,9 +20,9 @@ sd-export-template: - require: - sls: sd-workstation-template -sd-export-dvm: +sd-export-usb-dvm: qvm.vm: - - name: sd-export-dvm + - name: sd-export-usb-dvm - present: - template: sd-export-template - label: yellow @@ -56,5 +56,5 @@ create-named-sd-export-dispvm-and-permanently-attach: cmd.run: - name: > qvm-remove --force sd-export-usb || true; - qvm-create --class DispVM --template sd-export-dvm --label red sd-export-usb; + qvm-create --class DispVM --template sd-export-usb-dvm --label red sd-export-usb; qvm-usb attach --persistent sd-export-usb {{ d.usb.device }} || true; diff --git a/scripts/list-vms b/scripts/list-vms index ef58a20c0..f6fa398f0 100755 --- a/scripts/list-vms +++ b/scripts/list-vms @@ -17,7 +17,7 @@ declare -a sd_workstation_vm_names=( sd-svs-disp sd-svs-disp-template sd-export-template - sd-export-dvm + sd-export-usb-dvm sd-export-usb ) diff --git a/sd-export/send-to-usb b/sd-export/send-to-usb index 289d2b405..e7ce76891 100755 --- a/sd-export/send-to-usb +++ b/sd-export/send-to-usb @@ -1,4 +1,4 @@ -#! /usr/bin/python3 +#!/usr/bin/env python3 import datetime import json @@ -16,7 +16,7 @@ FILE = sys.argv[1] folder_name = "" encryption_method = "" encryption_key = "" -target_folder = "sd-export-{}".format(datetime.datetime.now().isoformat()) +target_folder = "sd-export-{}".format(datetime.datetime.now().strftime("%Y%m%d-%H%M%S")) tmpdir = tempfile.mkdtemp() @@ -28,7 +28,8 @@ if os.path.exists(FILE): except Exception as e: # exit with 0 return code otherwise the os will attempt to open # the file with another application - print("Error opening export bundle") + print("Error opening export bundle:") + print(e.output) sys.exit(0) try: @@ -38,16 +39,16 @@ if os.path.exists(FILE): encryption_method = data["encryption-method"] encryption_key = data["encryption-key"] except Exception as e: - print("Error parsing metadata") + print("Error parsing metadata:") + print(e.output) sys.exit(0) # we only support luks for now if encryption_method != "luks": - print("Unsupported export encryption") + print("Unsupported export encryption:") sys.exit(0) # the luks device is not already unlocked - if not os.path.exists(os.path.join("/dev/mapper/", ENCRYPTED_DEVICE)): p = subprocess.Popen( ["sudo", "cryptsetup", "luksOpen", DEVICE, ENCRYPTED_DEVICE], @@ -58,27 +59,40 @@ if os.path.exists(FILE): stdout_data = p.communicate(input=str.encode(encryption_key, "utf-8"))[0] rc = p.returncode if rc != 0: - print("Bad passphrase or luks error") + print("Bad passphrase or luks error:") sys.exit(0) - # mount target not created - if not os.path.exists(MOUNTPOINT): - out = subprocess.call(["sudo", "mkdir", MOUNTPOINT]) - out = subprocess.call(["sudo", "chown", "-R", "user:user", MOUNTPOINT]) - out = subprocess.call( - ["sudo", "mount", os.path.join("/dev/mapper/", ENCRYPTED_DEVICE), MOUNTPOINT] - ) + # for the entire rest of the script, large try/catch block as we do not + # want to return other than 0, otherwise another application will attempt + # to handle the script + try: + # mount target not created + if not os.path.exists(MOUNTPOINT): + subprocess.check_call(["sudo", "mkdir", MOUNTPOINT]) + subprocess.check_call(["sudo", "chown", "-R", "user:user", MOUNTPOINT]) + subprocess.check_call( + [ + "sudo", + "mount", + os.path.join("/dev/mapper/", ENCRYPTED_DEVICE), + MOUNTPOINT + ] + ) - # move files to drive (overrites files with same filename) and unmount drive - target_folder_path = os.path.join(MOUNTPOINT, target_folder) - subprocess.call(["mkdir", target_folder_path]) - export_data = os.path.join(tmpdir, folder_name, "export_data/") - shutil.move(export_data, target_folder_path) + # move files to drive (overrites files with same filename) and unmount drive + target_folder_path = os.path.join(MOUNTPOINT, target_folder) + subprocess.check_call(["mkdir", target_folder_path]) + export_data = os.path.join(tmpdir, folder_name, "export_data/") + shutil.move(export_data, target_folder_path) - # sync the filesystem, unmount drive and lock the luks volume - # we use call here to ensure they are blocking and avoid races - subprocess.call(["sync"]) - subprocess.call(["sudo", "umount", MOUNTPOINT]) - subprocess.call(["sudo", "cryptsetup", "luksClose", ENCRYPTED_DEVICE]) - # race condition when using shutils - subprocess.call(["rm", "-rf", tmpdir]) + # sync the filesystem, unmount drive and lock the luks volume + # we use call here to ensure they are blocking and avoid races + subprocess.check_call(["sync"]) + subprocess.check_call(["sudo", "umount", MOUNTPOINT]) + subprocess.check_call(["sudo", "cryptsetup", "luksClose", ENCRYPTED_DEVICE]) + # race condition when using shutils + subprocess.check_call(["rm", "-rf", tmpdir]) + except (subprocess.CalledProcessError, os.OSError) as e: + print("An error occurred while mounting disk or copying files to disk:") + print(e.output) + sys.exit(0) diff --git a/tests/test_sd_export.py b/tests/test_sd_export.py index cedc78b24..2053f698e 100644 --- a/tests/test_sd_export.py +++ b/tests/test_sd_export.py @@ -6,7 +6,7 @@ class SD_Export_Tests(SD_VM_Local_Test): def setUp(self): - self.vm_name = "sd-export-dvm" + self.vm_name = "sd-export-usb-dvm" super(SD_Export_Tests, self).setUp() def test_files_are_properly_copied(self): diff --git a/tests/test_vms_exist.py b/tests/test_vms_exist.py index 23ac7cac9..e008d9adb 100644 --- a/tests/test_vms_exist.py +++ b/tests/test_vms_exist.py @@ -125,7 +125,7 @@ def sd_export_template(self): self._check_kernel(vm) def sd_export_dvm(self): - vm = self.app.domains["sd-export-dvm"] + vm = self.app.domains["sd-export-usb-dvm"] nvm = vm.netvm self.assertTrue(nvm is None) self.assertTrue('sd-workstation' in vm.tags)