Detect different kinds of storage devices attached to sd-export-usb
#16
Conversation
|
Moved back to "In development" for now. The plan of record discussed during standup today was to keep this branch alive for testing purposes, but to attempt a more complete resolution that will work for both print and export. |
|
Even so, I can confirm that I was able to perform a successful export with this workaround in place! 🎉 I then unlocked the USB in |
When you unlocked the USB in Files in sd-export-usb, did you tell Qubes to remember your passphrase forever? The workaround for #12 is to unplug and plug your transfer device back in. Not the best, but it has worked consistently for me. As far as this workaround goes, I don't see why we should wait on merging a fix for a known issue, unless I'm underestimating the effort it takes to build and deploy. |
securedrop_export/export.py
Outdated
| try: | ||
| p = subprocess.check_output(["lsusb", "-s", "{}:".format(self.pci_bus_id)]) | ||
| subprocess.check_output(["lsblk", "-p", "-o", "KNAME", DEVICE], stderr=subprocess.PIPE) |
There was a problem hiding this comment.
thanks @creviera this does indeed work reliably for disks!
The reason we used lsusb here is because we wanted to check_usb_connected function to be able to detect both printers and block devices.
The command here will return 32: lsblk: /dev/sda1/ is not a block device. I see 2 options:
a) search for a more reliable solution that works for printers and drives
b) we can separate the preflight checks for USB connected:
i. For disks, using the method described here
ii. For printers, using either lsusb or another method
what do you think?
There was a problem hiding this comment.
Oops, somehow I missed this comment! I seeeeeee. My vote goes to whatever stabilizes export the quickest, so I think that's b, and then we can research a.
sounds like this is the path the group is going down and @rmol has already worked on some useful code for getting #!/usr/bin/env python3
import logging
import operator
import os
import re
EXPORT_CLASSES = {"07": "Printer", "08": "Storage"}
EXPORT_INTERFACE_CLASSES = EXPORT_CLASSES.keys()
VENDOR_ID_RE = re.compile("^(?P<id>[0-9A-Fa-f]{4}) (?P<name>.*)$")
PRODUCT_ID_RE = re.compile("^\t(?P<id>[0-9A-Fa-f]{4}) (?P<name>.*)$")
def load_product_info():
info = {"vendors": {}, "products": {}}
vendor_id = vendor_name = None
product_id = product_name = None
with open("/var/lib/usbutils/usb.ids") as f:
for line in f:
if line[0] == "#" or not line:
continue
p = PRODUCT_ID_RE.match(line)
if p:
product_id = p.group(1)
product_name = p.group(2)
info["products"][vendor_id][product_id] = product_name
else:
v = VENDOR_ID_RE.match(line)
if v:
vendor_id = v.group(1)
vendor_name = v.group(2)
info["vendors"][vendor_id] = vendor_name
info["products"][vendor_id] = {}
elif vendor_id:
# don't want all the class IDs and such below product info
break
return info
def get_device_info(product_info, directory):
interface_class_file = os.path.join(directory, "bInterfaceClass")
try:
if not os.path.isfile(interface_class_file):
return None
interface_class = open(interface_class_file).read().strip()
if interface_class in EXPORT_INTERFACE_CLASSES:
os.chdir(directory)
parent = os.path.dirname(os.getcwd())
bus_number = open(os.path.join(parent, "busnum")).read().strip()
dev_number = open(os.path.join(parent, "devnum")).read().strip()
vendor_id = open(os.path.join(parent, "idVendor")).read().strip()
product_id = open(os.path.join(parent, "idProduct")).read().strip()
info = {
"bus": bus_number,
"device": dev_number,
"interface_class": EXPORT_CLASSES[interface_class],
"product": product_info["products"].get(vendor_id, {}).get(product_id),
"vendor": product_info["vendors"].get(vendor_id),
}
return info
except Exception as e:
logging.error("Could not read device info from %s: %s", directory, e)
def get_export_devices(product_info):
export_devices = []
for directory, subdirs, files in os.walk("/sys/bus/usb/devices"):
for s in subdirs:
s = os.path.join(directory, s)
info = get_device_info(product_info, s)
if info:
export_devices.append(info)
return export_devices
if __name__ == "__main__":
product_info = load_product_info()
export_devices = get_export_devices(product_info)
export_devices.sort(key=operator.itemgetter("product"))
export_devices.sort(key=operator.itemgetter("vendor"))
export_devices.sort(key=operator.itemgetter("device"))
export_devices.sort(key=operator.itemgetter("bus"))
export_devices.sort(key=operator.itemgetter("interface_class"))
for d in export_devices:
print(
"Bus: {bus:>2s} Device: {device:>2s} Type: {interface_class} "
"Vendor: {vendor} Product: {product}".format(**d)
)I'm going to close this pr from my fork since I want to open it up for other people on the team to commit to the working branch and contribute directly. |
|
Reopening because:
After more discussion with @emkll we realized that the current code does not need to be used for print because we use lpinfo and return |
I've not yet gotten that to work, only when using Testing with a LUKS device, I don't see any device files or (This is on Ubuntu, but the output on Qubes is essentially the same). |
|
Ah, yes, I remember you showing me that last time and our idea was to check for |
This is because I think you have the whole device encrypted and then partitions inside of it. In other cases, we first create a partition and then encrypt it. |
Just checking for This PR is a good way to handle the issue in my mind. |
@eloquence, looks like you're using the default If you do: do you get an output of 0? |
When creating the export device, did you use MBR/DOS or EPT as the partitioning scheme? So far, I think we've all used the former (MBR/DOS) ? It would be interesting to see if this is what causes the inconsistency here. I think the approach by @creviera of iterating over the partitions in sda makes sense.
@eloquence (and potentially others): I would strongly advise to test these scripts/tooling under Debian/Qubes. Testing on multiple platforms (e.g. Ubuntu) introduces needless confusion to an already very complex flow. Some reasons come to mind:
|
|
[tested in Qubes/Debian VM]
That command results in "Device sda doesn't exist or access denied.", exit code 4. However, the command
results in exit code 0 (no message).
I don't recall; it was done through the GNOME Disk utility. I'll try to create a fresh one and write down the steps. |
|
OK, the only way I've found to create a USB drive in
This results in a device like I suspect I created the USB drive that behaved similarly using the path above, but I don't know for sure. |
perfect, that's what I meant :] I still need to resolve conflict but this should be ready for testing. |
|
rebased and updated tests. ready for review. |
|
Note: this implements part of our solution for #18 (we still need to remove persistent attachment from the Export VM and add the multiple-devices error, all of which will be done separately) |
eloquence
changed the title
sd-export-usb using lsblk to improve reliability
Test planThis resolves the inconsistent bus ID ordering issue reported here: freedomofpress/securedrop-workstation#307 (comment) causing the Both Erik and I have been able to see this issue frequently, so if you have problems repro'ing based on the information in that issue linked above, please let one of us know. Also please test and see Export now works in Erik's case where |
There was a problem hiding this comment.
thanks @creviera
What partition scheme are you using? I am using a MBR formatted, ext-4 luks-encrypted partition (which is in sda1)
The initial preflight check works 🎉 . However, the preflight check for luks encryption fails. This is because it uses the device id which contains the partition number, and the root device is not a luks volume, in the context of an MBR-formatted drive:
user@sd-export-usb:~$ sudo cryptsetup isLuks /dev/sda
user@sd-export-usb:~$ echo $?
130
user@sd-export-usb:~$ sudo cryptsetup isLuks /dev/sda1
user@sd-export-usb:~$ echo $?
0
user@sd-export-usb:~$ lsblk
NAME MAJ:MIN RM SIZE RO TYPE MOUNTPOINT
sda 8:0 1 14.5G 0 disk
└─sda1 8:1 1 14.5G 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 2G 0 disk /rw
xvdc 202:32 0 12G 0 disk
├─xvdc1 202:33 0 1G 0 part [SWAP]
└─xvdc3 202:35 0 11G 0 part
user@sd-export-usb:~$
In the interest of reducing confusion and the amount of possible code paths to check for encrypted device, as part of this PR, let's agree on and provide very high level documentation on what partitioning scheme, and which operating system we use to format and partition the drives. Otherwise, we will have inconsistent results, and may be difficult/time consuming to support all cases.
sgtm |
sssoleileraaa
changed the title
sd-export-usb using lsblk to improve reliabilitysd-export-usb
|
I addressed the requested change to update the docs to describe what kind of export devices we support and how to create them. I updated both the luks-encryption check and code to mount the device in order to support as many ways of creating luks-encrypted devices as I could think of using Test this PR with:
|
There was a problem hiding this comment.
Thanks @creviera this works perfectly in my local testing, this is a very significant improvement 🎉
Testing was done as follows:
- build deb package on this branch, and install in sd-export-template
- Export works as expected with MBR + luks/ext4 parition
- Export works as expected with GPT + luks/ext4 partition
- Export works with FDE as instructed in the README changes introduced in this PR
- Visual review of the diff
The two comments inline are so minor, I'll leave it at your option if you want to add a commit addressing these, or simply merging as-is.
I've discovered an (unrelated to this PR) bug in the client, the ticket is tracked in freedomofpress/securedrop-client#607
Description
Resolves #18 (see #18 (comment)).
/cc @emkll