Add sd-export VMs and basic export flow

[emkll]

Description

Towards #84 :
This adds:

• sd-export-template, a TemplateVM to manage every package related to (all?) exports
• sd-export-usb-dvm DispVM template: a Disposable VM
• sd-export-usb named disposable VM (that will be used once per session).

USB devices connected to a given USB port will automatically be attached to sd-export-usb, and sd-export-usb has a mime handler that will automatically copy .gpg files to the attached USB storage.

There are a few limitations in this current approach:

1. Dedicated USB port for exporting. It seems like that would less error prone than using the Qubes menus, if there's some sort of labeling on the workstation itself.

2. Transfer device provisioning is not automatic, it may be difficult to do without risking the data.

3. Export staging area (Nautilus window in sd-export-usb) would provide more user flexibility, but also more complexity.

4. Error handling/feedback may be challenging with the qvm-open-in-vm approach

Test Plan

• Plug in the usb stick and update config.json per the docs
• Create an encrypted transfer device (ext4-luks encrypted volume)
• make all and make test or make sd-export and make test complete successfully and all tests pass
• Create a tarball (important, extension must be .sd-export) in sd-svs containing the file structure described in the docs.
• In sd-svs, run qvm-open-in-vm sd-export-usb <file>.sdexport completes without error
• The file was successfully copied to the disk
• Ensure sd-export-usb is indeed disposable (for example, home directory contents don't persist reboots)

Note that the first iteration (85f3d67) was using non-encrypted devices, with symmetrically-encrypted files. Due to the default behavior or most tools extracting and storing the plaintext version next to the encrypted version on the filesystem, using a disk that was not full disk encrypted is likely not a very good idea, should the USB be misplaced.

Demo

[conorsch]

A few comments based on initial read-through of the diff. Have not yet begun functional steps listed in the test plan, still to come.

[conorsch]

Mode should be 644 for desktop file

[conorsch]

Mode should be 644 for XML file

[conorsch]

Let's prefer sd-export-usb to sd-export in all cases, since we also plan to support export methods other than USB. So these lines would become:

• sd-export-usb-template
• sd-export-usb-dvm
• sd-export-usb

And the relevant config lines referring to these VMs should be updated, as well. Discussion in the motivating ticket (#84) indicates that we may want to consolidate all export functionality into a single stateless VM. Given the disparate config needs (such as net/no-net) between just USB export and e.g. Onionshare, let's plan to name explicitly from the start, and consolidate if and only if we identify a sound method of doing so.

[emkll]

I've renamed sd-export-dvm to sd-export-usb-dvm. I think this makes sense, we want a DispVM template AppVM to not have network access.

In an attempt to minimize the total amount of templates, I think it might make sense to use the same template for USB exports and other exports(networked, OnionShare exports), as to reduce the time to upgrade templates (which is already quite long, as each template must be updated independently). Since we will likely be creating a AppVM/DispVM template for network-specific exports, I think it might make sense for them to share templates.

I don't feel strongly about sharing the template, happy to break up the template further if you think it's warranted.

[conorsch]

Remove whitespace in shebang line. While it's technically permitted for whitespace to be present, it's inconsistent with the other scripts in the project:

$ grep -rIPoh '^#!\s?+.*$' | sort | uniq -c
     10 #!/bin/bash
     10 #!/bin/sh
      1 #!/usr/bin/env python3
      1 #! /usr/bin/python3

Recommend using #!/usr/bin/env python3, same as in the sd-proxy script. The whitespace in the shebang line also breaks autodetection of the file as a Python script, meaning the flake8 linting didn't catch it. 🙃 Expect a few linting errors once the shebang is updated.

[conorsch]

Nit: subprocess.call returns the exit code from the run command, not output from the command, as implied by "out". As with the subsequent subprocess.calls, fine to omit the out = altogether, since we're not using it.

Might be worth using check_call instead and catching CalledProcessError to exit 0, otherwise we can't know that the commands are succeeding. In general, error handling in all the mimetype handler scripts is tricky, as we don't want to fail over over to an alternative solution.

[conorsch]

It's a shame to force removal of the VM on every provisioning run. A quick glance at the qvm docs seems to confirm we don't have a Salt-managed method for this, but perhaps we could wrangle it with the Python API. Fine as-is, for now, we'll want to switch wholesale to the Python API for VM creation if necessary (#159).

[conorsch]

Here's a patch to improve the error handling somewhat on this rather forceful one-liner, by breaking it up:

Author: Conor Schaefer <[email protected]>
Date:   Mon Jun 3 17:23:42 2019 -0700

    Breaks up sd-export-dvm salt config
    
    Error handling is difficult in bash one-liners, so let's leverage
    Saltstack for qvm configs as far as possible. The `qvm-check` command
    returns false if the VM does not exist, so we'll create it only
    conditionally.

diff --git a/dom0/sd-export.sls b/dom0/sd-export.sls
index 58cc269..32c879c 100644
--- a/dom0/sd-export.sls
+++ b/dom0/sd-export.sls
@@ -47,16 +47,31 @@ sd-export-template-sync-appmenus:
     - onchanges:
       - qvm: sd-export-template
 
-{% import_json "sd/config.json" as d %}
-
 # Here we must create as the salt stack does not appear to allow us to create
 # VMs with the class DispVM and attach the usb device specified in the config
 # permanently to this VM
-create-named-sd-export-dispvm-and-permanently-attach:
+sd-export-create-named-dispvm:
+  cmd.run:
+    - name: >
+        qvm-check sd-export-usb ||
+        qvm-create --class DispVM --template sd-export-usb-dvm --label red sd-export-usb
+    - require:
+      - qvm: sd-export-usb-dvm
+
+{% import_json "sd/config.json" as d %}
+
+sd-export-named-dispvm-permanently-attach-usb:
   cmd.run:
     - name: >
-        qvm-kill sd-export-usb || true;
-        qvm-remove --force sd-export-usb || true;
-        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;
-        qvm-tags sd-export-usb add sd-workstation
+        qvm-usb attach --persistent sd-export-usb {{ d.usb.device }} || true
+    - require:
+      - cmd: sd-export-create-named-dispvm
+
+sd-export-named-dispvm-add-tags:
+  qvm.vm:
+    - name: sd-export-usb
+    - tags:
+      - add:
+        - sd-workstation
+    - require:
+      - cmd: sd-export-create-named-dispvm

Since I encountered problems during initial review, I'm not appending this commit, to make sure we've working from the same branch as we debug.

[emkll]

Thanks @conorsch for the first pass, I've addressed most of your comments (see inline)

[conorsch]

Had some problems using the export flow via manual testing. Commented in-line where the problem seems to lie—happy to discuss further if anything's unclear, @emkll.

See also a patch provided to clean up the new template logic, given the special case of the DispVM class.

In order to test, I reformatted a USB stick on the CLI, using fdisk, cryptsetup, and mkfs.ext4. I'm able to view files in it fine, but I did not use the Gnome Disk Utility for creating the USB, as requested in the test plan. I suspect the problems reported about fs ownership would exist either way, but in case not, wanted to be clear about divergence from the test plan.

[conorsch]

Add check for self.assertTrue(vm.klass == "DispVM") to the sd-export-usb config, since it's the only VM we expect with that class name.

[conorsch]

The mkdir command on L83 failed for me reliably:

user@sd-svs:~$ qvm-open-in-vm sd-export-usb submission.sd-export
mkdir: cannot create directory '/media/usb/sd-export-2019-0603-182117': Permission denied
Exception ignored in: <_io.TextIOWrapper name='<stdout>' mode='w' encoding='UTF-8'>
OSError: [Errno 9] Bad file descriptor

It appears that the fresh LUKS ext4 USB device I'm using has the root dir owned by root, so all subsequent operations failed. As written, the chown operation preceding didn't fix, because it ran chown on the host dir, not the filesystem inside the USB device.

[conorsch]

Even after editing the send-to-usb script in-place, I'm still unable to open a submission:

user@sd-svs:~$ qvm-open-in-vm sd-export-usb submission.sd-export
mount: /dev/mapper/encrypted_volume is already mounted or /media/usb busy
           /dev/mapper/encrypted_volume is already mounted on /media/usb
Exception ignored in: <_io.TextIOWrapper name='<stdout>' mode='w' encoding='UTF-8'>
OSError: [Errno 9] Bad file descriptor

So we either need more conditional logic on when to run those mount commands, or we should adjust the shell-out calls to operate essentially as set +e would in bash, meaning execution errors don't halt the script. A custom function with the try/except logic enclosed within would accommodate, for instance. Let's aim for 1) more informative error output and 2) smarter conditional logic at minimum, then evaluate if we need a bigger reorg.

[conorsch]

The sys.exit(0) is solid, we want to preserve that—but it appears the print() statements aren't percolating to the qvm-open-in-vm command. Perhaps if we used sys.stderr.write(), we'd be able to view the debugging output here during development. Ideally we'd use proper logging (#19), but we can follow up on that when ready.

[conorsch]

Here's a patch to improve the error handling somewhat on this rather forceful one-liner, by breaking it up:

Author: Conor Schaefer <[email protected]>
Date:   Mon Jun 3 17:23:42 2019 -0700

    Breaks up sd-export-dvm salt config
    
    Error handling is difficult in bash one-liners, so let's leverage
    Saltstack for qvm configs as far as possible. The `qvm-check` command
    returns false if the VM does not exist, so we'll create it only
    conditionally.

diff --git a/dom0/sd-export.sls b/dom0/sd-export.sls
index 58cc269..32c879c 100644
--- a/dom0/sd-export.sls
+++ b/dom0/sd-export.sls
@@ -47,16 +47,31 @@ sd-export-template-sync-appmenus:
     - onchanges:
       - qvm: sd-export-template
 
-{% import_json "sd/config.json" as d %}
-
 # Here we must create as the salt stack does not appear to allow us to create
 # VMs with the class DispVM and attach the usb device specified in the config
 # permanently to this VM
-create-named-sd-export-dispvm-and-permanently-attach:
+sd-export-create-named-dispvm:
+  cmd.run:
+    - name: >
+        qvm-check sd-export-usb ||
+        qvm-create --class DispVM --template sd-export-usb-dvm --label red sd-export-usb
+    - require:
+      - qvm: sd-export-usb-dvm
+
+{% import_json "sd/config.json" as d %}
+
+sd-export-named-dispvm-permanently-attach-usb:
   cmd.run:
     - name: >
-        qvm-kill sd-export-usb || true;
-        qvm-remove --force sd-export-usb || true;
-        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;
-        qvm-tags sd-export-usb add sd-workstation
+        qvm-usb attach --persistent sd-export-usb {{ d.usb.device }} || true
+    - require:
+      - cmd: sd-export-create-named-dispvm
+
+sd-export-named-dispvm-add-tags:
+  qvm.vm:
+    - name: sd-export-usb
+    - tags:
+      - add:
+        - sd-workstation
+    - require:
+      - cmd: sd-export-create-named-dispvm

Since I encountered problems during initial review, I'm not appending this commit, to make sure we've working from the same branch as we debug.

[conorsch]

The recent changes to the script resolved the ownership problems I'd reported. Able to use the removable LUKS-encrypted device reliably. Testing is of course slow. Let's look for opportunities to flesh out functional tests to verify the inter-VM behavior.

After confirming the functionality in the script was working as advertised, I tried to make the script more resilient to unexpected non-zero exits. See the new try/except logic. Details in commit messages, but in short, it's mostly slicing into defs, with no changes to core logic.

Also tacked on a slight change to the provisioning logic, mentioned in a patch above, and a corresponding test tracking the DispVM status for sd-export-usb.

Quite happy with these changes going in immediately. Since I appended changes on top of yours, leaving the PR unmerged so you can sign-off on them, @emkll.

[conorsch]

Since I appended changes on top of yours, leaving the PR unmerged so you can sign-off on them

Feel free to squash/amend as you see fit, as well!

[emkll]

Previous iterations of this step would be run after qvm-usb detach on the sd-export-usb domain this is because when running make sd-export using more than once, with different values for usb.device will result in an error while starting the sd-export-usb vm (see screenshot below)

[conorsch]

Nice catch on the clean behavior, I can confirm that without your commit, I need to run make clean twice for it to purge the export VMs. Running one last time with your changes to confirm a complete run on first try.

[emkll]

Thanks for the changes @conorsch , this significantly improves the error handling. Your changes LGTM.

I've also added efc1ede to properly handle the removal of the sd-export VMs, as well as dbe00eb to handle the case where the usb device ID is changed in config.json, to avoid two usb devices from being attached to sd-export-usb

[conorsch]

Ran through another round of testing, looks great. The cleanup tweaks in particular are great to have—nice catch. I'm going to merge without adding any additional tests. This test passes in most configurations:

Patch of sd-export-usb qvm-device test

commit 7c4cbf48a8fb16aaca962ffda0e119a1dd7b1914
Author: Conor Schaefer <[email protected]>
Date:   Wed Jun 5 16:52:48 2019 -0700

    Adds tests for sd-export-usb devices
    
    The config tests assert that `qvm-usb` shows proper device attachment
    for the transfer device USB port (hardcoded in config.json) to the
    `sd-export-usb` VM. The test is smart enough to ignore cases where
    the USB stick isn't plugged in (it won't be found in the qvm-usb output,
    so essentially the test is skipped), but it will fail if a developer
    plugs in a USB stick after provisioning without one.

diff --git a/tests/test_sd_export.py b/tests/test_sd_export.py
index c6dc4e4..2ee2b69 100644
--- a/tests/test_sd_export.py
+++ b/tests/test_sd_export.py
@@ -1,5 +1,8 @@
+import json
+import subprocess
 import unittest
 
+
 from base import SD_VM_Local_Test
 
 
@@ -7,6 +10,10 @@ class SD_Export_Tests(SD_VM_Local_Test):
 
     def setUp(self):
         self.vm_name = "sd-export-usb-dvm"
+        with open("config.json") as c:
+            config = json.load(c)
+            usb_device = config["usb"]["device"]
+        self.usb_device = usb_device
         super(SD_Export_Tests, self).setUp()
 
     def test_files_are_properly_copied(self):
@@ -20,6 +27,28 @@ class SD_Export_Tests(SD_VM_Local_Test):
     def test_sd_export_package_installed(self):
         self.assertTrue(self._package_is_installed("cryptsetup"))
 
+    def get_qvm_usb_devices(self):
+        raw_output = subprocess.check_output("qvm-usb").decode("utf-8").rstrip()
+        raw_lines = raw_output.split("\n")
+        usb_devices = []
+
+        for raw_line in raw_output.split("\n"):
+            # Each device contains 2 or 3 info items:
+            # <device_id>, <device_name>, [attached_vm]
+            # 3rd item is attached VM, not always present.
+            device_info = raw_line.split()
+            usb_devices.append(device_info)
+
+        return usb_devices
+
+    def test_sd_export_usb_attached(self):
+        usb_devices = self.get_qvm_usb_devices()
+        for usb_device in usb_devices:
+            device_id = usb_device[0]
+            if device_id == self.usb_device:
+                device_vm = usb_device[2]
+                assert device_vm == "sd-export-usb"
+
 
 def load_tests(loader, tests, pattern):
     suite = unittest.TestLoader().loadTestsFromTestCase(SD_Export_Tests)

It's important to be aware that the USB drive must be attached during initial provisioning. If the device was not connected when the salt tasks ran, then the device will never be attached to the sd-export-usb, even after plugging it in and confirming the device ids match. That install-time limitation has significant implications for the admin/journalist story during the pilot.

Thanks for your patience during iterative review, @emkll. The implementation as written is an extremely solid start—looking forward to building on it!