Address review comments

* rename sd-export-dvm to rename-export-usb-dvm
* remove executable bit on certain files
* improved error/exception handling

.flake8

@@ -1,2 +1,3 @@
 [flake8]
 ignore: W605
+max-line-length = 99

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

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

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;

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
 )
 

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)

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):

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)