Bug 54828

This message

 http://lists.freedesktop.org/archives/devkit-devel/2012-September/001315.html

contains the recommendations on how to implement this.

Hey,

+1 for this change.

It seems that this hasn't been worked on for quite some time. I'd be willing to write the patch if there is still a chance to get it included and there is someone here to merge it? Just give me a heads-up.

Best regards,
Thomas

Created attachment 128116 [details] [review]
Use binary IO channel for input_string

The channel encoding defines to which encoding the input string should
be transformed before writing it to the channel. Since the in-memory
encoding is always UTF-8 and the default encoding is also UTF-8, this is
a NO-OP by default — except that it checks that all characters are valid
UTF-8 characters. To avoid this check the encoding must be set to NULL
(=binary). Note that the actual transformation (=NO-OP) remains the
same.

Created attachment 128117 [details] [review]
Change udisks_spawned_job_new:input_string param to GString

Created attachment 128118 [details] [review]
Add test for binary input string

Created attachment 128119 [details] [review]
Add launch functions taking GString inputs

Created attachment 128120 [details] [review]
Add keyfile_contents blob to unlock options

Created attachment 128121 [details] [review]
Add --key-file option to udisksctl unlock

Created attachment 128122 [details] [review]
The 'ay' type seems to be working fine

According to [1] the glib binding somehow doesn't handle 'ay' correctly
and we therefore need 'a(y)'. I don't exactly get what this means and
whether this is obsolete: 'ay' appears to be working fine so far.

1: http://stackoverflow.com/a/24908253/650222

Created attachment 128123 [details] [review]
Document the keyfile_contents option

Hey, I wrote a patch for this feature a good while back which I was told to upload here (see https://lists.freedesktop.org/archives/devkit-devel/2015-September/001724.html). I'm sorry, if these are too many individual patches. You can also check out the branch directly from my git:

   git remote add thomas https://github.com/coldfix/udisks
   git fetch thomas
   git checkout -b support-keyfile thomas/support-keyfile

Just give me a note about any issues with these patches, so I can work on fixes.

I know, that these patches do not yet include tests for all new functionality and I'm willing to fix this. Please just give me a hint on which distro to run the tests and which packages are required. It seems that new kernels do not include the scsi_debug module anymore (I think?) on which the integration tests are based. So there might be a rewrite in order anyway.


Thanks, Thomas.

Created attachment 128139 [details] [review]
Add doc-comments for _gstring launch functions

Hello Thomas,

thanks for working on this!

(In reply to Thomas Gläßle from comment #11)
> I know, that these patches do not yet include tests for all new
> functionality and I'm willing to fix this. Please just give me a hint on
> which distro to run the tests and which packages are required. It seems that
> new kernels do not include the scsi_debug module anymore (I think?) on which
> the integration tests are based.

That would be news to me -- it's still present in current master (http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/scsi/scsi_debug.c), and it's incredibly useful for debugging and testing.

I usually run them under Ubuntu, but I know David Zeuthen ran them in Fedora back then (I made some effort to not introduce any distro specific stuff). src/tests/integration-test skips tests for which the corresponding mkfs utils are not installed, but if you want the full set, the Debian package names are:

  cryptsetup-bin dosfstools gir1.2-glib-2.0 kpartx libatasmart-bin lvm2 mdadm policykit-1 python3-dbus python3-gi reiserfsprogs xfsprogs

which is hopefully not too hard to translate to other distros. But for sure you need scsi_debug.ko, otherwise not much will work. If/once you have that, maybe just run the test and see how far you get? Feel free to contact me here or on IRC (pitti @ freenode) if you have problems with this, I'm very interested in making/keeping this test work on Fedora too.

Hey, thanks a lot for the info!

> That would be news to me -- it's still present in current master (http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/scsi/scsi_debug.c), and it's incredibly useful for debugging and testing.

Oh, I must have overlooked it somehow and jumped to premature conclusions since it's not present in archlinux. However, I now managed to compile the module and add it to my system - now I can run all the tests. :)

In order to really test the new keyfile functionality, I should first setup a device with a truly binary key. However, the `encrpyt.passphrase` parameter of `Block.Format` handles only strings currently. Would you agree if I add handling of binary data to that method as well? If so, what should be the name of the parameter?

While we're at it: what do you think about directly changing the handling of the parameters `Block.Format.encrypt.passphrase` and `Encrypted.Unlock.passphrase` to accept both strings or binary data and act accordingly? (Not sure if this is easy to do in the second case)

Best,

Thomas

Created attachment 128228 [details] [review]
Move udisks_string_wipe_and_free to a better place

Created attachment 128229 [details] [review]
Allow binary passphrases for Block.Format

Created attachment 128230 [details] [review]
Fix minor style issue: whitespace

Created attachment 128231 [details] [review]
Add tests for keyfile support

Hey, I added a test for the keyfile functionality. I had to change the "encrypt.passphrase" parameter of Block.Format to optionally accept keyfiles for this. Let me know if you think I should add/change something else.

I think the password parameters on the Encrypted.Unlock and Encrypted.ChangePassphrase methods would best be changed to accept both strings and bytearrays, but I don't know if that is easy/possible to change, as I did not dig into your interface generation. What do you think about this?

Best, Thomas

Hey,

did you have time to review the changes yet? I'll be happy to fix remaining issues. Or are you planning to hold this back until the merge with storaged is through?

Best, Thomas

This has been fixed in udisks 2.6.4.