-
Notifications
You must be signed in to change notification settings - Fork 142
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
Support TCRYPT #495
Support TCRYPT #495
Conversation
Hello guys, thanks for your work, but how can I merge your changes to my System so that udisks2 and Udiskie recognizes my Veracrypt file systems? Please noob friendly, I'm not a Linux Pro and maybe it can help others with that problem too.. ;) |
src/udiskslinuxblock.c
Outdated
chi_square /= e; | ||
|
||
return CHI_SQUARE_LOWER_LIMIT < chi_square && chi_square < CHI_SQUARE_UPPER_LIMIT; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have enough time to look at all the changes here, but I can say right away that this function belongs to libblockdev.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. Done in commit 3d4a065 and bb3c036 in libblockdev.
@germangoergs: I think you should wait until this is merged and released in udisks if you are not familiar with compiling programs from source. |
@segfault3 That's not the problem, I'm not familiar with github. :D |
@germangoergs I would still recommend you to wait until this is released, but if you want to test it, you can clone my libblockdev and Udisks forks (see the GitHub help on how to clone a repository). Then check out the |
@segfault3 thank you, I will test it later. :) |
Jenkins, ok to test. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the changes, I'll try to do full review hopefully this week. I've just enabled tests on this PR (because libblockdev changes needed for this are now merged) and these two issues are causing the tests to fail.
src/udiskslinuxblock.c
Outdated
@@ -1017,7 +1022,8 @@ udisks_linux_block_update (UDisksLinuxBlock *block, | |||
{ | |||
gchar *dm_uuid; | |||
dm_uuid = get_sysfs_attr (device->udev_device, "dm/uuid"); | |||
if (dm_uuid != NULL && g_str_has_prefix (dm_uuid, "CRYPT-LUKS")) | |||
if (dm_uuid != NULL && | |||
(g_str_has_prefix (dm_uuid, "CRYPT-LUKS1") || g_str_has_prefix (dm_uuid, "CRYPT-TCRYPT"))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please change this back to CRYPT-LUKS
only, so this works with both LUKS version 1 and 2. (Originally changed here)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, sorry, seems I was sloppy when I rebased on master. Fixed in e874152.
src/udiskslinuxencrypted.c
Outdated
name = g_strdup_printf ("luks-%s", udisks_block_get_id_uuid (block)); | ||
else | ||
/* TCRYPT devices don't have a UUID, so we use the device number instead */ | ||
name = g_strdup_printf ("tcrypt-%lu", udisks_block_get_device_number (block)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please the G_GUINT64_FORMAT
macro here, %lu
doesn't work on 32 bit machines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 77818e0.
That's great! Note that I will be mostly unavailable the next two days. |
How is the review going? Is there anything I can do to help / speed things up? I took a look at the failed test cases, but they seem to be unrelated to the changes of this pull request. |
Jenkins, test this, please. |
It's quite a huge PR so everybody's just avoiding the review, sorry. I will take a look at it this week (most probably on Friday). |
Missed that, sorry. Reviewing now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. I think it would be better to squash the commits into just a few ones. Or even just a single one if there's nothing to separate.
src/udiskslinuxencrypted.c
Outdated
while (g_variant_iter_next (&iter, "&s", &path)) | ||
{ | ||
if (i >= MAX_TCRYPT_KEYFILES) | ||
break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add this to the condition of the while
loop. There's no need to have an if-break
at the beginning of the loop like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, fixed in 17f5efd.
src/udiskslinuxencrypted.c
Outdated
/* fallback mechanism: keyfile_contents -> passphrase -> crypttab_passphrase -> error (no key) */ | ||
if (!udisks_variant_lookup_binary (options, "keyfile_contents", &effective_passphrase)) { | ||
/* fallback mechanism: keyfile_contents (for LUKS) -> passphrase -> crypttab_passphrase -> error (no key) */ | ||
if (!is_luks || !udisks_variant_lookup_binary (options, "keyfile_contents", &effective_passphrase)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This condition says "If it's not LUKS or failed to get value of the "keyfile_contents" option...". The comment above it sounds like this is not what was desired. Anyway, I think this code needs some refactorization if we want to introduce even more complexity to it due to TrueCrypt.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bit confusing, but it does what it's supposed to do: If there are keyfile_contents and it's LUKS, then this block (which tries the fallbacks) is skipped.
Do you want me to try to refactorize this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about this? a338075
src/udiskslinuxencrypted.c
Outdated
* actual unlock, in order to have this set before the device | ||
* update triggered by the unlock. */ | ||
if (is_luks) | ||
{} /* LUKS is detectable, so we don't have to set the hint */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please squash the comments and do if (!is_luks)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in ec71668.
src/udiskslinuxencrypted.c
Outdated
gboolean read_only = FALSE; | ||
gboolean hidden = FALSE; | ||
gboolean system = FALSE; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use is_system
or something to avoid hiding the system()
function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in e4d8eb2.
src/udiskslinuxencrypted.c
Outdated
} | ||
|
||
/* save old encryption type to be able to restore it */ | ||
old_hint_encryption_type = udisks_encrypted_dup_hint_encryption_type(encrypted); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing space after the function name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 932aff4.
src/udiskslinuxencrypted.c
Outdated
{ | ||
/* We have to free old_hint_encryption_type if and only if it was not | ||
* used in udisks_encrypted_set_hint_encryption_type() */ | ||
g_free(old_hint_encryption_type); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing space again (and also above).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also fixed in 932aff4.
data/org.freedesktop.UDisks2.xml
Outdated
|
||
This is set during successful unlocking of an encrypted device, whose encryption | ||
type can only be determined by decrypting the device (currently only used | ||
for TCRYPT devices). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why should this not be used for LUKS
too? There's no reason to enforce checking multiple properties to get an encryption type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason I added this property is that TCRYPT
volumes cannot be reliably detected as such. To make it less confusing to users, I set the IdType
property to crypto_unknown
for volumes which might be TCRYPT
encrypted. But after a volume was successfully unlocked as TCRYPT
, we know for sure that it's TCRYPT
, so we set the HintEncryptionType
property. And then, when the interface is updated, the IdType
is set according to HintEncryptionType
, i.e. to crypto_TCRYPT
(see https://github.com/storaged-project/udisks/pull/495/files#diff-fd982868b9106b4c67ff22c3d8bd7179R138).
Doing the same for LUKS
doesn't make sense, because the IdType
is always set to crypto_LUKS
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you don't need it for LUKS, but I think it can be really confusing to have a property HintEncryptionType
that says NULL
for LUKS devices (or actually all non-truecrypt devices). I think that having this information twice is better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, done in b913bf7.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the documentation here didn't get fixed up in the above commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, fixed in 16fe066.
Also please bump the required version of libblockdev to the version that has the new function. |
Done in d6ce8d4. |
Great! :)
Sure. I guess it makes sense to wait with that until the review is completed and I can also squash the fixups. |
configure.ac
Outdated
@@ -154,7 +154,7 @@ PKG_CHECK_MODULES(GMODULE, [gmodule-2.0]) | |||
AC_SUBST(GMODULE_CFLAGS) | |||
AC_SUBST(GMODULE_LIBS) | |||
|
|||
PKG_CHECK_MODULES(BLOCKDEV, [blockdev >= 2.14]) | |||
PKG_CHECK_MODULES(BLOCKDEV, [blockdev >= 2.17]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove this for now -- libblockdev 2.17 is not released yet and tests are failing because of this. (We use daily builds of libblockdev on our testing VMs so the tests won't fail, but the version is still 2.16). I plan to release 2.17 soon (hopefully this week) so you can change it back to 2.17 before merging this. Sorry for complications.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 9566462.
src/udiskslinuxblock.c
Outdated
@@ -1175,8 +1181,25 @@ udisks_linux_block_update (UDisksLinuxBlock *block, | |||
udisks_block_set_id (iface, NULL); | |||
} | |||
|
|||
udisks_block_set_id_usage (iface, g_udev_device_get_property (device->udev_device, "ID_FS_USAGE")); | |||
udisks_block_set_id_type (iface, g_udev_device_get_property (device->udev_device, "ID_FS_TYPE")); | |||
seems_encrypted = bd_crypto_device_seems_encrypted (device_file, &error); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand this correctly, this will run the check on every device. I really don't like this -- we should at least run the check only on devices that "look empty" ("ID_FS_USAGE" udev property is empty).
I generally don't like the idea of running this automatically on devices -- we generally try to not read "random" devices (we take most of our information from udev) and it is possible this will fail for some "special" devices. And it can also produce some "false positives" -- for example LUKS with detached header will be detected as encrypted (it is encrypted, but we can't open it, because we don't support opening LUKS with detached header). This can be really confusing for gvfs/Nautilus and other "users".
I think we should consider doing some "whitelisting" for devices we want to check -- e.g. if user wants to run this check on a device, he has to create a special udev rule adding a property that will tell udisks not check the devices (something similar to UDISKS_IGNORE
we currently use to ignore "system" devices). We unfortunately don't have a config file for udisks, so udev rules are only possibility for something like this, but it is quite easy to create a new one and it is even possible to create an udev rule "outside" of udisks -- e.g. if some distribution decides they want to switch this on for all devices, it is easy to create such rule without patching udisks.
Any ideas/thoughts about this? I'm open to ideas/discussion, it is possible I'm being too scared without a reason, or my solution is overly complicated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand this correctly, this will run the check on every device.
Correct.
I really don't like this -- we should at least run the check only on devices that "look empty" ("ID_FS_USAGE" udev property is empty).
I would be okay with that, but it is something we had already considered and decided against, so let me explain why: The ID_FS_USAGE
property is set based on the filesystem magic number, which is usually only 2 bytes. So if a device which contains random data has the wrong bytes at a certain offset, it will be incorrectly identified as filesystem-formatted. As a result, udisks will not be able to unlock a given TCRYPT
volume with a chance of n
/ 65536, where n
= number of filesystems with a 2 byte magic number that are supported by udev.
I generally don't like the idea of running this automatically on devices -- we generally try to not read "random" devices (we take most of our information from udev)
I understand that, and I had the same feeling when I first realized that this will be required to detect TCRYPT devices. But we only try to read the first 512 bytes of the device, which udev does anyway, in order to parse the superblock. So I don't see why it would cause problems if we do that in libbockdev / udisks.
and it is possible this will fail for some "special" devices.
But then bd_crypto_device_seems_encrypted
would simply return FALSE
, no?
And it can also produce some "false positives" -- for example LUKS with detached header will be detected as encrypted (it is encrypted, but we can't open it, because we don't support opening LUKS with detached header). This can be really confusing for gvfs/Nautilus and other "users".
Absolutely, all LUKS volumes with a detached header will pass this test, as well as dm-crypt plain and Loop-AES volumes. To make this less confusing, we introduced the crypto_unknown
type, and in our GNOME Disks patches we add a sentence to the unlock dialog explaining that "this volume might be a VeraCrypt volume, as it contains random data".
I think we should consider doing some "whitelisting" for devices we want to check -- e.g. if user wants to run this check on a device, he has to create a special udev rule adding a property that will tell udisks not check the devices (something similar to UDISKS_IGNORE we currently use to ignore "system" devices). We unfortunately don't have a config file for udisks, so udev rules are only possibility for something like this, but it is quite easy to create a new one and it is even possible to create an udev rule "outside" of udisks -- e.g. if some distribution decides they want to switch this on for all devices, it is easy to create such rule without patching udisks.
I think this feature will be pretty useless if it requires the user to create a udev rule first, because if they are able to do that, they are also able to use cryptsetup
on the command line to unlock their volumes.
I also don't see how this can work for encrypted file containers, which can be unlocked by associating them with a loop device.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would be okay with that, but it is something we had already considered and decided against, so let me explain why: The ID_FS_USAGE property is set based on the filesystem magic number, which is usually only 2 bytes. So if a device which contains random data has the wrong bytes at a certain offset, it will be incorrectly identified as filesystem-formatted. As a result, udisks will not be able to unlock a given TCRYPT volume with a chance of n / 65536, where n = number of filesystems with a 2 byte magic number that are supported by udev.
That's a good point. On the other hand if udev thinks something is a filesystem udisks won't be the only one that is really confused. It might be worth testing what happens in that case (maybe nothing).
I understand that, and I had the same feeling when I first realized that this will be required to detect TCRYPT devices. But we only try to read the first 512 bytes of the device, which udev does anyway, in order to parse the superblock. So I don't see why it would cause problems if we do that in libbockdev / udisks.
Udev is caching a lot of information. With this change udisks will do this check for every udev change event on every block device. We already have some issues about udisks starting too long or using too much CPU because of some bugged devices.
I think this feature will be pretty useless if it requires the user to create a udev rule first, because if they are able to do that, they are also able to use cryptsetup on the command line to unlock their volumes.
You'd need to do that only once and than use udisks for unlocking, but is probably too complicated. Too bad we don't have some better/easier way of configuration.
Maybe I'm just too paranoid or scared of breaking something. Any thoughts @vpodzime ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I'm just too paranoid or scared of breaking something. Any thoughts @vpodzime ?
What about simply adding some "flag file" like /etc/udisks2/tcrypt.conf
that would enable all this machinery? It could even be empty (for now) and just tell udisks TrueCrypt/VeraCrypt devices should be taken into account. It can then be packaged separately and people who are using TC/VC would simply install this extra package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opinions @segfault3, @vojtechtrefny?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would not provide any easy/nice way to make the support installable as a package. Packagers would have to decide what the default should be and then people would have to change it manually.
How would that be different when using the flag file? The way I understand it, packagers would still have to decide what the default should be (ship the flag file or not), and then people would have to change it manually (create the flag file or delete it).
But I don't have a strong opinion on this, if you think the flag file is better, then it's fine for me - it's definitely easier to implement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The flag file could be put into a separate package like udisks2-truecrypt. And at some point the file could contain some configuration for the TC/VC-related behaviour.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. Ok, I will implement the check for the flag file then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. Ok, I will implement the check for the flag file then.
Thanks! Later we can implement some real configuration like blacklist and/or whitelist of devices that should be scanned, but for now this is an easy way to prevent potential confusing issues for users that don't use TC/VC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay. I pushed a commit now (03781dd).
One last nitpick. Please squash the commits now, @segfault3. |
Also, do you know if there's any reasonable way to test this new functionality @segfault3 ? |
Done in f78d1e5. |
Do you mean testing manually or writing tests? For the former, you can just create a TrueCrypt or VeraCrypt file container, associate it with a loop device, and use |
Would be nice to have such tests. However, IIRC, tcplay was practically impossible to use for tests due to the way it was getting the passphrase. |
OK, let me do one more review tomorrow and I hope this will finally be merged. It has been a long journey... |
You are right, using tcplay programmatically is a pain. There is also zuluCrypt-cli, which is also painful to use, because it has so many options, but you can create all kinds of encrypted volumes non-interactively, for example:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me otherwise. @vojtechtrefny please take a final look and merge if you will.
data/org.freedesktop.UDisks2.xml
Outdated
|
||
This is set during successful unlocking of an encrypted device, whose encryption | ||
type can only be determined by decrypting the device (currently only used | ||
for TCRYPT devices). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the documentation here didn't get fixed up in the above commit.
Looks good to me. Please squash the last commit and I will merge this. |
We can add some tests later. Cryptsetup uses pre-made truecrypt images in its tests, so we could just take one and use it in our tests. |
Done. |
Thanks a lot for your patience and hard work, @segfault3! |
Thank you too! |
Should tcrypt support be reflected in SupportedEncryptionTypes property of org.freedesktop.UDisks2.Manager? |
Yes, please create an issue here at GH if it's not. |
I think the original idea behind the property was to list encryption "types" UDisks can create so it makes sense to not include TrueCrypt/VeraCrypt. So maybe the property just needs better documentation. |
This adds support for TCRYPT volumes, including:
It requires changes in libblockdev, see libblockdev pull request #320.