Support VeraCrypt; additional export improvements #1669
Conversation
rocodes
force-pushed
the
support-veracrypt
branch
2 times, most recently
from
8d16173 to
dcc76c7
Compare
rocodes
force-pushed
the
support-veracrypt
branch
4 times, most recently
from
025200e to
397aa2f
Compare
…ypt). DRY up qrexec calling methods. Replace ExportStatus values with new values (see securedrop-export 80-support-veracrypt). Rename methods in export.py for clarity. Remove disk-test (combine disk-test and usb-test).
…sponsive to state of export workflow
…, print dialog, and Device.
… new functional test.
|
(I noticed a bug that I will push another commit for - the |
|
Per handoff today: @legoktm will finalize tests next week, and I will review the week following. |
legoktm
force-pushed
the
support-veracrypt
branch
2 times, most recently
from
5dc47a4 to
29cd971
Compare
cfm
force-pushed
the
support-veracrypt
branch
from
526fc32 to
9ea108c
Compare
|
Rebased from |
legoktm
force-pushed
the
support-veracrypt
branch
from
9ea108c to
719cfce
Compare
|
I rebased and merged in the changes from freedomofpress/securedrop-export#126 |
legoktm
force-pushed
the
support-veracrypt
branch
from
23b0051 to
a75b9f6
Compare
There was a problem hiding this comment.
[This review covers only the securedrop-export changes from freedomofpress/securedrop#126, originally reviewed in freedomofpress/securedrop-export#126 (review).]
This is looking good. I've left a few observations and nits inline, plus these questions:
This feels very state-mechanistic especially now that there is varying precision in what we can know about LUKS versus VeraCrypt volumes. We don't have to worry about that here, since send-to-usb is a one-shot action at a particular time, but I'll raise this again in securedrop-client.
Re: freedomofpress/securedrop-export#126 (comment):
in order to accommodate different filesystems that could be inside Veracrypt containers re:mounting permissions, this PR introduces a change from using
sudo mountto usingdisksctlto handle mounting and unmounting. However, I'd appreciate a review re: file system mounting permissions and if we're OK with the changes.
I'd like to understand better (before releasing if not as part of reviewing this feature) what you view as the changes in permissions and your concerns about them. For a LUKS volume, at least, the permissions appear unchanged:
user@sd-app:~/QubesIncoming/sd-dev$ ls -al /media/user/
total 12
drwxr-x---+ 3 root root 4096 Dec 13 15:48 .
drwxr-xr-x 3 root root 4096 Dec 13 11:54 ..
drwx------ 24 user user 4096 Dec 13 15:05 'QA export_transf'And losing sudo to un/mount devices seems at face value like a good thing, even if sudo is a passwordless operation in a Qubes VM anyway.
Test plan
-
With
device: "disk-test": No USBs connected:NO_DEVICE_DETECTED -
With
device: "disk-test": > 1 USBs connected:MULTI_DEVICE_DETECTED -
With
device: "disk-test": Unsupported device connected (could be: device has 2 or more partitions, device has unrecognized encryption scheme, device is a locked VeraCrypt drive, which we won't support for now due to difficulty with error handling, or malformed device):INVALID_DEVICE_DETECTED
No: UNKNOWN_DEVICE_DETECTED (see case [7] below).
-
With
device: "disk-test": 1 LUKS-formatted USB connected and it's locked:DEVICE_LOCKED -
With
device: "disk-test": 1 LUKS-formatted device (partition or whole device encrypted) connected and it's unlocked:DEVICE_WRITABLE(and device will be mounted in/media/user), orSUCCESS_EXPORT(depending on the command in metadata.json). -
With
device: "disk":
user@sd-app:~/QubesIncoming/sd-dev$ lsblk /dev/sda
NAME MAJ:MIN RM SIZE RO TYPE MOUNTPOINT
sda 8:0 1 14.3G 0 disk
└─sda1 8:1 1 14.3G 0 part
└─luks-e538abe0-cd9e-4eab-b1d5-fb652acc235b
254:0 0 14.3G 0 crypt /media/user/QA export_transf
user@sd-app:~/QubesIncoming/sd-dev$ send-to-usb sample_export.sd_export.cfm
Usage:
udisksctl unmount [OPTION…]
Unmount a filesystem.
Options:
-p, --object-path Object to unmount
-b, --block-device Block device to unmount
-f, --force Force/lazy unmount
--no-user-interaction Do not authenticate the user if needed
DEVICE_ERRORFix proposed in freedomofpress/securedrop-export#126 (comment).
- 1 Veracrypt-formatted device (parition or whole device encrypted) and it's unlocked: same as above
We shouldn't assume a single partition will be /sdX1:
user@sd-app:~/QubesIncoming/sd-dev$ lsblk
NAME MAJ:MIN RM SIZE RO TYPE MOUNTPOINT
sda 8:0 1 14.3G 0 disk
└─sda2 8:2 1 14.1G 0 part
xvda 202:0 0 10G 0 disk
├─xvda1 202:1 0 200M 0 part
├─xvda2 202:2 0 2M 0 part
└─xvda3 202:3 0 9.8G 0 part /
xvdb 202:16 0 10G 0 disk /rw
xvdc 202:32 0 10G 0 disk
├─xvdc1 202:33 0 1G 0 part [SWAP]
└─xvdc3 202:35 0 9G 0 part
user@sd-app:~/QubesIncoming/sd-dev$ send-to-usb sample_export.sd_export
Device /dev/sda1 does not exist or access denied.
UNKNOWN_DEVICE_DETECTED(Will this also be eased by rocodes/securedrop-export#1?)
- Other error statuses are fairly self explanatory (
ERROR_MOUNT,ERROR_CLEANUP, etc)
export/securedrop_export/disk/cli.py
Outdated
| # Entries in /dev/mapper on sd-devices | ||
| _DEVMAPPER_SYSTEM = ["control", "dmroot"] | ||
|
|
There was a problem hiding this comment.
This comment prompts me to ask whether securedrop-export as implemented is necessarily or only de facto coupled to the sd-devices VM. If the former, I'd like to document that coupling in the readme; if the latter, I think we should avoid embedding that coupling in code and comments.
export/securedrop_export/disk/cli.py
Outdated
| (excluding `system` and `dmroot`). | ||
| """ | ||
| try: | ||
| ls = subprocess.check_output(["ls", "/dev/mapper/"], stderr=subprocess.PIPE) |
There was a problem hiding this comment.
This is the first of many references to /dev/mapper. I suggest factoring it out into a constant.
export/securedrop_export/disk/cli.py
Outdated
| "lsblk", | ||
| f"/dev/mapper/{item}", | ||
| "--noheadings", | ||
| "-o", |
There was a problem hiding this comment.
I think it's okay that we're lsblking directly here, since we don't use it so much that it's worth introducing something like https://github.com/genalt/blkinfo. But why not use the output of lsblk --json?
Whatever the output format, I suggest long arguments for readability:
| "-o", | |
| "--output", |
There was a problem hiding this comment.
Ah! I see that you've started this in rocodes/securedrop-export#1, which I support wholeheartedly.
export/securedrop_export/disk/cli.py
Outdated
| f"{volume.device_name}", | ||
| f"{self._DEFAULT_VC_CONTAINER_NAME}", |
There was a problem hiding this comment.
Nit:
| f"{volume.device_name}", | |
| f"{self._DEFAULT_VC_CONTAINER_NAME}", | |
| volume.device_name, | |
| self._DEFAULT_VC_CONTAINER_NAME, |
export/securedrop_export/disk/cli.py
Outdated
| # from /dev/mapper using `cryptsetup close`.) | ||
| status = ( | ||
| subprocess.check_output( | ||
| ["sudo", "cryptsetup", "status", f"/dev/mapper/{item}"] |
There was a problem hiding this comment.
I was hoping --debug-json might give us structured output here, but it's still mostly chatty.
export/securedrop_export/disk/cli.py
Outdated
| try: | ||
| logger.info("Mount volume in /media/user using udisksctl") | ||
| output = subprocess.check_output( | ||
| ["udisksctl", "mount", "-b", f"/dev/mapper/{volume.mapped_name}"] |
There was a problem hiding this comment.
Nit: Suggest long arguments for readability:
| ["udisksctl", "mount", "-b", f"/dev/mapper/{volume.mapped_name}"] | |
| ["udisksctl", "mount", "--block-device", f"/dev/mapper/{volume.mapped_name}"] |
| else: | ||
| return self.scan_single_device(all_devices[0]) | ||
| status, _ = self._check_volumes(volumes) |
There was a problem hiding this comment.
On first reading, I thought there was something to DRY here. There isn't, but it may be worth making that explicit:
| status, _ = self._check_volumes(volumes) | |
| # NB. Here we've checked for exactly 1 device. _check_volumes() will also check for exactly 1 volume. | |
| status, _ = self._check_volumes(volumes) |
export/securedrop_export/archive.py
Outdated
| @@ -26,7 +26,6 @@ class Metadata(object): | |||
| """ | |||
|
|
|||
| METADATA_FILE = "metadata.json" | |||
| SUPPORTED_ENCRYPTION_METHODS = ["luks"] | |||
There was a problem hiding this comment.
For Git posterity, can you explain why in the commit message? (Or squash with c6f871c.)
export/securedrop_export/disk/cli.py
Outdated
| @@ -281,7 +457,7 @@ def mount_volume(self, volume: Volume) -> MountedVolume: | |||
| Given an unlocked LUKS volume, return MountedVolume object. | |||
|
|
|||
| If volume is already mounted, mountpoint is not changed. Otherwise, | |||
| volume is mounted at _DEFAULT_MOUNTPOINT. | |||
| volume is mounted inside /media/user/ by udisksctl. | |||
There was a problem hiding this comment.
Mine mounts at /media/user/${VOLUME_NAME}.
There was a problem hiding this comment.
I'm sorry to report that all test cases failed with securedrop-client in sd-app and securedrop-export in sd-devices, run and built (respectively) from 23b3374.
For an export operation with...
- No USBs connected:
NO_DEVICE_DETECTED
The Client shows "Export successful" despite:
securedrop_export.disk.cli:83(_get_removable_devices) INFO: 0 connected
Dec 14 15:22:46 localhost 2023-12-14 15:22:46,593 - securedrop_export.disk.service:52(export) ERROR: Could not export, no available volumes (NO_DEVICE_DETECTED)
Dec 14 15:22:46 localhost 2023-12-14 15:22:46,593 - securedrop_export.main:164(_exit_gracefully) INFO: Exit gracefully with status: NO_DEVICE_DETECTED
Dec 14 15:22:46 localhost 2023-12-14 15:22:46,594 - securedrop_export.main:188(_write_status) INFO: Write status NO_DEVICE_DETECTED
Dec 14 15:22:46 localhost qubes.OpenInVM+-sd-app: NO_DEVICE_DETECTED
- > 1 USBs connected:
MULTI_DEVICE_DETECTED
The Client reports ExportStatus.UNEXPECTED_RETURN_STATUS from:
Dec 14 15:24:28 localhost qubes.OpenInVM+-sd-app: MULTI_DEVICE_DETECTED
- Unsupported device connected (could be: device has 2 or more partitions, device has unrecognized encryption scheme, device is a locked VeraCrypt drive, which we won't support for now due to difficulty with error handling, or malformed device):
INVALID_DEVICE_DETECTED
The Client reports ExportStatus.UNEXPECTED_RETURN_STATUS from:
Dec 14 15:27:30 localhost qubes.OpenInVM+-sd-app: Device /dev/sda1 does not exist or access denied.
Dec 14 15:27:30 localhost sudo: pam_unix(sudo:session): session closed for user root
Dec 14 15:27:30 localhost 2023-12-14 15:27:30,251 - securedrop_export.disk.cli:197(is_luks_volume) INFO: Target device is not LUKS-encrypted
Dec 14 15:27:30 localhost 2023-12-14 15:27:30,252 - securedrop_export.disk.cli:107(get_all_volumes) DEBUG: Not a LUKS volume. Checking if unlocked VeraCrypt.
Dec 14 15:27:30 localhost 2023-12-14 15:27:30,254 - securedrop_export.disk.cli:389(_attempt_get_unlocked_veracrypt_volume) INFO: No unlocked Veracrypt drive found.
Dec 14 15:27:30 localhost 2023-12-14 15:27:30,254 - securedrop_export.disk.cli:114(get_all_volumes) INFO: Device is not an unlocked Veracrypt drive.
Dec 14 15:27:30 localhost 2023-12-14 15:27:30,254 - securedrop_export.disk.service:103(_check_volumes) DEBUG: Device status is unknown
Dec 14 15:27:30 localhost 2023-12-14 15:27:30,254 - securedrop_export.disk.service:69(export) INFO: Could not export, volume check was UNKNOWN_DEVICE_DETECTED
Dec 14 15:27:30 localhost 2023-12-14 15:27:30,254 - securedrop_export.main:164(_exit_gracefully) INFO: Exit gracefully with status: UNKNOWN_DEVICE_DETECTED
Dec 14 15:27:30 localhost 2023-12-14 15:27:30,254 - securedrop_export.main:188(_write_status) INFO: Write status UNKNOWN_DEVICE_DETECTED
Dec 14 15:27:30 localhost qubes.OpenInVM+-sd-app: UNKNOWN_DEVICE_DETECTED
- 1 LUKS-formatted USB connected and it's locked:
DEVICE_LOCKED
The Client reports ExportStatus.UNEXPECTED_RETURN_STATUS from:
Dec 14 15:30:32 localhost 2023-12-14 15:30:32,260 - securedrop_export.disk.cli:298(unlock_luks_volume) DEBUG: Successfully unlocked.
Dec 14 15:30:32 localhost 2023-12-14 15:30:32,260 - securedrop_export.disk.service:128(_unlock_device) DEBUG: Volume unlocked, attempt to mount
Dec 14 15:30:32 localhost 2023-12-14 15:30:32,260 - securedrop_export.disk.cli:444(_get_mountpoint) DEBUG: Checking mountpoint
Dec 14 15:30:32 localhost 2023-12-14 15:30:32,263 - securedrop_export.disk.cli:476(mount_volume) INFO: Mount volume in /media/user using udisksctl
Dec 14 15:30:32 localhost qubes.OpenInVM+-sd-app: Error looking up object for device /dev/mapper/luks-e538abe0-cd9e-4eab-b1d5-fb652acc235b
Dec 14 15:30:32 localhost 2023-12-14 15:30:32,276 - securedrop_export.disk.cli:491(mount_volume) ERROR: Command '['udisksctl', 'mount', '-b', '/dev/mapper/luks-e538abe0-cd9e-4eab-b1d5-fb652acc235b']' returned non-zero exit status 1.
Dec 14 15:30:32 localhost 2023-12-14 15:30:32,276 - securedrop_export.disk.service:132(_unlock_device) ERROR:
Dec 14 15:30:32 localhost 2023-12-14 15:30:32,276 - securedrop_export.main:164(_exit_gracefully) INFO: Exit gracefully with status: ERROR_UNLOCK_LUKS
Dec 14 15:30:32 localhost 2023-12-14 15:30:32,276 - securedrop_export.main:188(_write_status) INFO: Write status ERROR_UNLOCK_LUKS
Dec 14 15:30:32 localhost qubes.OpenInVM+-sd-app: ERROR_UNLOCK_LUKS
...but the drive is actually unlocked and mounted in Nautilus. So then:
- 1 LUKS-formatted device (partition or whole device encrypted) connected and it's unlocked:
DEVICE_WRITABLE(and device will be mounted in/media/user), orSUCCESS_EXPORT(depending on the command in metadata.json).
...this fails because device is already mounted.
- 1 Veracrypt-formatted device (parition or whole device encrypted) and it's unlocked: same as above
The Client reports ExportStatus.UNEXPECTED_RETURN_STATUS because:
Dec 14 15:50:13 localhost 2023-12-14 15:50:13,368 - securedrop_export.disk.cli:83(_get_removable_devices) INFO: 1 connected
Dec 14 15:50:13 localhost 2023-12-14 15:50:13,368 - securedrop_export.disk.cli:169(_check_partitions) DEBUG: Checking device partitions on /dev/sda
Dec 14 15:50:13 localhost 2023-12-14 15:50:13,371 - securedrop_export.disk.cli:146(_get_partitioned_device) DEBUG: Counted 1 partitions
Dec 14 15:50:13 localhost 2023-12-14 15:50:13,371 - securedrop_export.disk.cli:157(_get_partitioned_device) DEBUG: One partition found
Dec 14 15:50:13 localhost 2023-12-14 15:50:13,371 - securedrop_export.disk.cli:187(is_luks_volume) DEBUG: Checking if target device is luks encrypted
Dec 14 15:50:13 localhost sudo: user : PWD=/home/user ; USER=root ; COMMAND=/usr/sbin/cryptsetup isLuks /dev/sda1
Dec 14 15:50:13 localhost sudo: pam_unix(sudo:session): session opened for user root(uid=0) by (uid=1000)
Dec 14 15:50:13 localhost qubes.OpenInVM+-sd-app: Device /dev/sda1 does not exist or access denied.
Dec 14 15:50:13 localhost sudo: pam_unix(sudo:session): session closed for user root
Dec 14 15:50:13 localhost 2023-12-14 15:50:13,393 - securedrop_export.disk.cli:197(is_luks_volume) INFO: Target device is not LUKS-encrypted
Dec 14 15:50:13 localhost 2023-12-14 15:50:13,393 - securedrop_export.disk.cli:107(get_all_volumes) DEBUG: Not a LUKS volume. Checking if unlocked VeraCrypt.
Dec 14 15:50:13 localhost 2023-12-14 15:50:13,395 - securedrop_export.disk.cli:389(_attempt_get_unlocked_veracrypt_volume) INFO: No unlocked Veracrypt drive found.
Dec 14 15:50:13 localhost 2023-12-14 15:50:13,396 - securedrop_export.disk.cli:114(get_all_volumes) INFO: Device is not an unlocked Veracrypt drive.
Dec 14 15:50:13 localhost 2023-12-14 15:50:13,396 - securedrop_export.disk.service:103(_check_volumes) DEBUG: Device status is unknown
Dec 14 15:50:13 localhost 2023-12-14 15:50:13,396 - securedrop_export.main:164(_exit_gracefully) INFO: Exit gracefully with status: UNKNOWN_DEVICE_DETECTED
Dec 14 15:50:13 localhost 2023-12-14 15:50:13,396 - securedrop_export.main:188(_write_status) INFO: Write status UNKNOWN_DEVICE_DETECTED
Dec 14 15:50:13 localhost qubes.OpenInVM+-sd-app: UNKNOWN_DEVICE_DETECTED
- With VeraCrypt volume moved to
/dev/sda1:
The Client shows "Export successful" despite:
Dec 14 17:20:44 localhost sudo: user : PWD=/home/user ; USER=root ; COMMAND=/usr/sbin/cryptsetup isLuks /dev/sda1
Dec 14 17:20:44 localhost sudo: pam_unix(sudo:session): session opened for user root(uid=0) by (uid=1000)
Dec 14 17:20:44 localhost sudo: pam_unix(sudo:session): session closed for user root
Dec 14 17:20:44 localhost 2023-12-14 17:20:44,677 - securedrop_export.disk.cli:197(is_luks_volume) INFO: Target device is not LUKS-encrypted
Dec 14 17:20:44 localhost 2023-12-14 17:20:44,677 - securedrop_export.disk.cli:107(get_all_volumes) DEBUG: Not a LUKS volume. Checking if unlocked VeraCrypt.
Dec 14 17:20:44 localhost 2023-12-14 17:20:44,679 - securedrop_export.disk.cli:389(_attempt_get_unlocked_veracrypt_volume) INFO: No unlocked Veracrypt drive found.
Dec 14 17:20:44 localhost 2023-12-14 17:20:44,680 - securedrop_export.disk.cli:114(get_all_volumes) INFO: Device is not an unlocked Veracrypt drive.
Dec 14 17:20:44 localhost 2023-12-14 17:20:44,680 - securedrop_export.disk.service:103(_check_volumes) DEBUG: Device status is unknown
Dec 14 17:20:44 localhost 2023-12-14 17:20:44,680 - securedrop_export.disk.service:69(export) INFO: Could not export, volume check was UNKNOWN_DEVICE_DETECTED
Dec 14 17:20:44 localhost 2023-12-14 17:20:44,680 - securedrop_export.main:164(_exit_gracefully) INFO: Exit gracefully with status: UNKNOWN_DEVICE_DETECTED
Dec 14 17:20:44 localhost 2023-12-14 17:20:44,680 - securedrop_export.main:188(_write_status) INFO: Write status UNKNOWN_DEVICE_DETECTED
Dec 14 17:20:44 localhost qubes.OpenInVM+-sd-app: UNKNOWN_DEVICE_DETECTED
- With
/dev/sda1unlocked:
After clicking Continue at Ready to export, the Client freezes and sd-devices displays:
Unable to handle mimetype of the requested file (exit status: 256)!
If you dismiss that error, then the Client shows "Export successful".
Dec 14 17:23:24 localhost 2023-12-14 17:23:24,987 - securedrop_export.disk.cli:352(_attempt_get_unlocked_veracrypt_volume) INFO: Unlocked VeraCrypt volume detected
Dec 14 17:23:24 localhost 2023-12-14 17:23:24,989 - securedrop_export.disk.cli:377(_attempt_get_unlocked_veracrypt_volume) INFO: Drive is already mounted at /media/user/AE85-5D67
Dec 14 17:23:24 localhost 2023-12-14 17:23:24,989 - securedrop_export.disk.service:97(_check_volumes) DEBUG: Device is unlocked and mounted
Dec 14 17:23:24 localhost 2023-12-14 17:23:24,991 - securedrop_export.disk.cli:511(write_data_to_device) DEBUG: Copying file to sd-export-20231214-172324
Dec 14 17:23:25 localhost 2023-12-14 17:23:25,107 - securedrop_export.disk.cli:514(write_data_to_device) INFO: File copied successfully to sd-export-20231214-172324
Dec 14 17:23:25 localhost 2023-12-14 17:23:25,107 - securedrop_export.disk.cli:533(cleanup_drive_and_tmpdir) DEBUG: Syncing filesystems
Dec 14 17:23:25 localhost 2023-12-14 17:23:25,128 - securedrop_export.disk.cli:552(_unmount_volume) DEBUG: Unmounting drive from /media/user/AE85-5D67
Dec 14 17:23:25 localhost systemd[672]: media-user-AE85\x2d5D67.mount: Succeeded.
Dec 14 17:23:25 localhost systemd[633]: media-user-AE85\x2d5D67.mount: Succeeded.
Dec 14 17:23:25 localhost systemd[1]: media-user-AE85\x2d5D67.mount: Succeeded.
Dec 14 17:23:25 localhost udisksd[871]: Cleaning up mount point /media/user/AE85-5D67 (device 254:0 is not mounted)
Dec 14 17:23:25 localhost udisksd[871]: Unmounted /dev/dm-0 on behalf of uid 1000
Dec 14 17:23:25 localhost systemd-udevd[8164]: dm-0: Process '/usr/lib/qubes/udev-block-add-change' failed with exit code 1.
Dec 14 17:23:25 localhost qubes.OpenInVM+-sd-app: cat: write error: Bad file descriptor
Dec 14 17:23:25 localhost 2023-12-14 17:23:25,385 - securedrop_export.disk.cli:588(_close_veracrypt_volume) DEBUG: Locking luks volume <securedrop_export.disk.volume.Volume object at 0x768b906b1f70>
Dec 14 17:23:25 localhost sudo: user : PWD=/home/user ; USER=root ; COMMAND=/usr/sbin/cryptsetup close mapped
Dec 14 17:23:25 localhost sudo: pam_unix(sudo:session): session opened for user root(uid=0) by (uid=1000)
Dec 14 17:23:25 localhost sudo: pam_unix(sudo:session): session closed for user root
Dec 14 17:23:25 localhost 2023-12-14 17:23:25,435 - securedrop_export.disk.cli:602(_remove_temp_directory) DEBUG: Deleting temporary directory /tmp/tmphsupizr6
Dec 14 17:23:25 localhost 2023-12-14 17:23:25,438 - securedrop_export.main:164(_exit_gracefully) INFO: Exit gracefully with status: SUCCESS_EXPORT
Dec 14 17:23:25 localhost 2023-12-14 17:23:25,438 - securedrop_export.main:188(_write_status) INFO: Write status SUCCESS_EXPORT
Dec 14 17:23:25 localhost qubes.OpenInVM+-sd-app: SUCCESS_EXPORT
I'll see how far I can get in tracing these failures this afternoon.
There was a problem hiding this comment.
Coming back to this after the holidays, let's make sure I'm running the code I should be.
user@sd-large-bullseye-template:~/QubesIncoming/sd-dev$ mkdir securedrop_export_0.3.0
user@sd-large-bullseye-template:~/QubesIncoming/sd-dev$ dpkg-deb -x securedrop-export_0.3.0+bullseye_all.deb securedrop_export_0.3.0/
user@sd-large-bullseye-template:~/QubesIncoming/sd-dev$ cd securedrop_export_0.3.0/
user@sd-large-bullseye-template:~/QubesIncoming/sd-dev/securedrop_export_0.3.0$ ls
opt usr
user@sd-large-bullseye-template:~/QubesIncoming/sd-dev/securedrop_export_0.3.0$ cd opt/venvs/securedrop-export/lib/python3.9/site-packages/securedrop_export
user@sd-large-bullseye-template:~/QubesIncoming/sd-dev/securedrop_export_0.3.0/opt/venvs/securedrop-export/lib/python3.9/site-packages/securedrop_export$ grep "Device is locked LUKS drive" disk/service.py
logger.debug("Device is locked LUKS drive")
user@sd-large-bullseye-template:~/QubesIncoming/sd-dev/securedrop_export_0.3.0/opt/venvs/securedrop-export/lib/python3.9/site-packages/securedrop_export$ grep "Device is locked LUKS drive" /opt/venvs/securedrop-export/lib/python3.9/site-packages/securedrop_export/disk/service.py
logger.debug("Device is locked LUKS drive")user@sd-app:~/securedrop-client/client$ git log -1 --oneline --no-show-signature
23b33747 (HEAD -> support-veracrypt, origin/support-veracrypt) fix(_unmount_volume): "udisksctl unmount" takes "--block-device"Okay, now let's start fresh.
user@sd-app:~/securedrop-client/client$ sudo apt install python3-pip
user@sd-app:~/securedrop-client/client$ sudo pip3 install poetry
user@sd-app:~/securedrop-client/client$ poetry install
user@sd-app:~/securedrop-client/client$ rm -rf ~/.securedrop_client/data ~/.securedrop_client/svs.sqlite
user@sd-app:~/securedrop-client/client$ poetry run alembic upgrade head
user@sd-app:~/securedrop-client/client$ poetry run python3 -m securedrop_clientWith that, I get the same results as in #1669 (review). Let's investigate with:
--- a/client/securedrop_client/export.py
+++ b/client/securedrop_client/export.py
@@ -245,6 +245,7 @@ class Export(QObject):
status = self._build_archive_and_export(
metadata=self.USB_TEST_METADATA, filename=self.USB_TEST_FN
)
+ logger.debug(f"status = {status}")
logger.debug("completed preflight checks: success")
self.preflight_check_call_success.emit(status)Initiating an export with no USB drive connected gives me:
Jan 4 15:40:53 localhost 2024-01-04 15:40:53,273 - securedrop_export.main:164(_exit_gracefully) INFO: Exit gracefully with status: NO_DEVICE_DETECTED
Jan 4 15:40:53 localhost 2024-01-04 15:40:53,273 - securedrop_export.main:188(_write_status) INFO: Write status NO_DEVICE_DETECTED
Jan 4 15:40:53 localhost qubes.OpenInVM+-sd-app: NO_DEVICE_DETECTED
==> QubesIncomingLogs/sd-app/syslog.log <==
Jan 4 15:40:53 localhost 2024-01-04 15:40:53,545 - securedrop_client.export:248(run_preflight_checks) DEBUG: status = ExportStatus.NO_DEVICE_DETECTED
Jan 4 15:40:53 localhost 2024-01-04 15:40:53,546 - securedrop_client.export:250(run_preflight_checks) DEBUG: completed preflight checks: success
Let's trace the flow here:
NO_DEVICE_DETECTEDis checked insecuredrop_client.gui.conversation.export.file_dialog.FileDialog._update_dialog()atand ._update_dialog()is called inFileDialog._on_export_preflight_check_failed()at .- But
completed preflight checks: successcomes fromFileDialog._on_export_preflight_check_succeeded(). Why? - This slot is connected to
securedrop_client.export.Export.preflight_check_call_successin . - When
Export.run_preflight_checks()is called, it calls...Export.build_archive_and_export(), which calls...Export._run_qrexec_export(), which...- returns
stdoutfor a return code of0; - raises
ValueErrorfor unexpectedstdout; or - raises
ExportStatusfor aCalledProcessError.
- returns
NO_DEVICE_DETECTEDis validstdout, so it's returned to...
Export_build_archive_and_export(), which returns it to...Export.run_preflight_checks(), which takes it asstatusand emitsExport.preflight_check_call_success(status), in this case to...
FileDialog._on_export_preflight_check_succeeded(), where it slips through an overbroadelsein. Uncommenting theelseto a narrowelifprevents trying to unlock a nonexistent device, but it doesn't actually handle theNO_DEVICE_DETECTEDstate.- ...so
FileDialog.update_dialog(), which does handle all of these states, is never called.
In summary, there's a mismatch along the chain of (a) states and return codes returned by qrexec; (b) return codes checked by securedrop_client.export.Export, and (c) states checked by securedrop_client.gui.conversation.export.file_dialog.FileDialog. It now appears that any successful execution of the send-to-usb script itself for a preflight check, whatever its status, will be interpreted as readiness to export.
In #1669 (review) I noted that the export side of this protocol feels state-mechanistic; even more so here on the client side. But the shortcut I would suggest is collapsing Export's success and failure signals into one per type (e.g., {preflight_check_call,export_usb_call,printer_preflight,print_call}.finished(status)) and having FileDialog evaluate status centrally before dispatching to FileDialog.update_dialog().
I'd be happy to collaborate on this next week!
rocodes
force-pushed
the
support-veracrypt
branch
from
23b3374 to
ee4e67e
Compare
|
Thank you the time and effort reviewing this, and sorry for leaving things in a half-done state. Per discussion, I'm closing this in favour of #1777, which addresses review feedback (so far, export feedback; soon, client/gui feedback) and some other known issues, and is monorepo friendly, but is different enough that I think it calls for a new PR. |
Description
Should maybe be merged into separate release or feature branch instead due to need for coordinated release with sd-export, to be discussed.
Refs freedomofpress/securedrop-export#126
Fixes #1462
Fixes freedomofpress/securedrop-workstation#265
Test Plan
Environment: Qubes (dev/staging)
securedrop-builder.sd-large-bullseye-template, install it, and shut down the template and your sd-devices vm if running. (You will have to either adjust dom0 RPC policies to permit thequbes.Filecopybetween your builder vm and the template, or copy the deb into dom0 then from dom0 into the template.)sd-workstation. Check out this branch and run the client (with./run.sh+ a development server) to test the changes here in conjunction with changes in sd-devices.Checklist
If these changes modify code paths involving cryptography, the opening of files in VMs or network (via the RPC service) traffic, Qubes testing in the staging environment is required. For fine tuning of the graphical user interface, testing in any environment in Qubes is required. Please check as applicable:
If these changes add or remove files other than client code, the AppArmor profile may need to be updated. Please check as applicable:
If these changes modify the database schema, you should include a database migration. Please check as applicable:
mainand confirmed that the migration is self-contained and applies cleanlymainand would like the reviewer to do so