Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Return export device state details as part of export attempts #264

Closed
eloquence opened this issue Jun 5, 2019 · 8 comments · Fixed by freedomofpress/securedrop-export#5

Comments

@eloquence
Copy link
Member

eloquence commented Jun 5, 2019

To integrate the export workflow prototyped in #259 in the client, it would be useful to be able to distinguish the following states of the export device, with recommended MoSCoW prioritization:

  • (Must) No device is plugged into the designated export port
  • (Could) A device is plugged in, but it does not appear to be a storage device
  • (Should) A storage device is plugged in, but it does not appear to be encrypted (i.e. we could write to it, but we won't because we enforce encryption)
  • (Should) The storage device does not have enough space for the export to be completed successfully
  • (Must) A storage device is plugged in, but the passphrase is incorrect
  • (Must) Other error mounting or accessing the storage device

This would allow us to immediately attempt to kick off an export when the user requests it, and show the user the appropriate message if the conditions for a successful export are not met.

@eloquence
Copy link
Member Author

We've not made a final decision on how we want to handle passphrases for export. If the client ends up sometimes prompting the user to enter a passphrase (or provide other pre-export information), that may argue for having a separate "pre-flight" check that just tests if the USB device is plugged in and of the correct type, before displaying the passphrase prompt.

@emkll
Copy link
Contributor

emkll commented Jun 20, 2019

(Must) No device is plugged into the designated export port

Assuming the persistent attach in dom0 works as expected, running lsusb in sd-export-usb and comparing to a known default state would be sufficient to indentify if a usb device is connected.

(Could) A device is plugged in, but it does not appear to be a storage device

Per the above, and running lsblk in a similar fashion in sd-export-usb would be able to tell us if a block device is attached.

(Should) A storage device is plugged in, but it does not appear to be encrypted (i.e. we could write to it, but we won't because we enforce encryption)

This might be a bit tricky to implement reliably as logical volumes might have different names accross vms/installs partitions. This will be complicated further if we decide to support multiple encryption algorithms. It might be easier to merge this requirement with the "A storage device is plugged in, but the passphrase is incorrect" error below.

(Should) The storage device does not have enough space for the export to be completed successfully

This can be handled in the send-to-usb script by looking at the size of the file and the remaining space on disk via df or in python using os.statvfs

(Must) A storage device is plugged in, but the passphrase is incorrect

This is already handled in the send-to-usb script and happens when cryptsetup command returns non-zero. These exceptions are already caught, we need to formalize the string to be parsed by the client.

(Must) Other error mounting or accessing the storage device

The mounting and copying operations are distinct and happen after the decryption operation in send-to-usb, these exceptions are already caught, we need to formalize the string to be parsed by the client.

@eloquence
Copy link
Member Author

(Should) A storage device is plugged in, but it does not appear to be encrypted (i.e. we could write to it, but we won't because we enforce encryption)

It might be easier to merge this requirement with the "A storage device is plugged in, but the passphrase is incorrect" error below.

To clarify why this would be helpful, I think we can reasonably assume that users will at some point attempt to just use a random USB drive for export. At that point, the ideal UX would be to tell the user that this won't work (and why) before we even prompt for the passphrase.

But for beta, we could have an error after entering the passphrase, to the effect of "passphrase is incorrect and/or the drive isn't encrypted".

@ninavizz
Copy link
Member

It feels like bait/switch or tone-deaf, to request users create/use s00per secure passwords... and then wait to tell them things like "ohey, that's the wrong kind of drive" until after they've entered in a password.

On a whole aside from giving user feedback about a thing, after they've gone ahead to do other things, being a very standard UX anti-pattern. Personally, that's a hard-stop usability "No" not because it would block a user from proceeding, but because it will frustrate non-Linux/dev (so, most... if not all) users. That needs to matter.

@ninavizz
Copy link
Member

Current flow-map from wireframes, that speaks to these things:

image

@eloquence eloquence added this to Current Sprint - 6/26-7/10 in SecureDrop Team Board Jul 9, 2019
@eloquence eloquence moved this from Current Sprint - 6/26-7/10 to Nominated for next sprint in SecureDrop Team Board Jul 9, 2019
@eloquence eloquence moved this from Nominated for next sprint to Current Sprint - 7/10-7/24 in SecureDrop Team Board Jul 10, 2019
@emkll
Copy link
Contributor

emkll commented Jul 11, 2019

As part of the 10/7-24/7 Sprint, I propose we implement the return codes for the must as follows. If we need any of the should/could items, we can address them once we begin client integration:

(Must) No device is plugged into the designated export port

I propose we create a new "device-type" in metadata.json called usb-test that returns the strings:

  • USB_CONNECTED if a USB device is connected
  • USB_NOT_CONNECTED if no USB device is connected

We can get this information by running lsusb in the send-to-usb script sd-export-usb. With no usb devices connected, that command returns an empty string.

(Must) A storage device is plugged in, but the passphrase is incorrect

I propose we return the following string if the passphrase is wrong:

  • USB_BAD_PASSPHRASE

We get to this point in the code when unlocking the LUKS volume.

msg = "Bad passphrase or luks error."
.

 (Must) Other error mounting or accessing the storage device

I propose we return the following string if the there's an error mounting or accessing storage device (mount or luks failure):

  • USB_MOUNT_ERROR

We get to this point in the code when mounting the volume after unlocking the device here:

subprocess.check_call(["sudo", "cryptsetup", "luksClose", ENCRYPTED_DEVICE])

The following items I propose we don't immediately implement as they require significant changes in send-to-usb script and would be easier to implement once packaging is done and quick iteration is possible.

(Could) A device is plugged in, but it does not appear to be a storage device

The usb-test device described above could check to see if it's a block device or other (if there's anything new under lsblk).

  • USB_NO_STORAGE

(Should) A storage device is plugged in, but it does not appear to be encrypted (i.e. we could write to it, but we won't because we enforce encryption)

We should implement Veracrypt first (#265) , as return code/error messages might change. We can return something like:

  • USB_NOT_ENCRYPTED

(Should) The storage device does not have enough space for the export to be completed successfully

This would require to manually check available space and comparing with what's available. It would require a new method in send-to-usb, and we could return something like:

  • USB_INSUFFICIENT_SPACE

@eloquence
Copy link
Member Author

Hi @emkll,

At a high level this sounds good to me! The part I'm wondering a bit about is this:

I propose we create a new "device-type" in metadata.json called usb-test that returns the strings ..

Should we treat this as a send-to-usb action? It seems a bit odd to go through the "archive with metadata" process for an operation that's fundamentally different. Perhaps it would make sense for the process to start with running a probe-usb command? But that's more of an architectural question independent of the string identifiers, and perhaps closer to the pre-flight issue #281.

As far as the string identifiers are concerned, these seem sensible to me; I'll file an issue for the USB_INSUFFICIENT_SPACE detection and add it to the epic as a stretch goal.

Bumping to @creviera and @redshiftzero for additional comment :)

@redshiftzero
Copy link
Contributor

these error codes looks good, one nit:

  • USB_NOT_ENCRYPTED -> USB_NO_SUPPORTED_ENCRYPTION ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants