GNOME Bugzilla – Bug 796146
Support VeraCrypt
Last modified: 2018-05-24 10:45:11 UTC
This is part of the ongoing effort to integrate TrueCrypt/VeraCrypt support into GNOME. The attached patch extends the unlock dialog to support parameters required to unlock TrueCrypt/VeraCrypt volumes. It requires udisks patches which were merged (see https://github.com/storaged-project/udisks/pull/495), but not released yet. See also the thread on the devkit-devel mailing list: https://lists.freedesktop.org/archives/devkit-devel/2017-November/001800.html
Created attachment 372080 [details] [review] Add support for unlocking TrueCrypt/VeraCrypt
Created attachment 372081 [details] Screenshot of unlock dialog with TrueCrypt/VeraCrypt options
Kai: I think in the attached patch I addressed all your comments from your first review. Regarding the expander, I followed your advice and removed it, see the attached screenshot.
Review of attachment 372080 [details] [review]: Hello, thanks a lot for the work, the tooltips already help. My first impression of using the dialog as a newcomer to VeraCrypt is that it still required me to read the docs. Therefore I added some suggestions for the tooltip texts. Another thing I want to propose is communicating what options are required. How about implementing a logic to enable the button only if sufficent information is entered, i.e. at least passphrase or keyfile. I think this is better than e.g. greeting with "Please enter passphrase, keyfiles, or both and select the exact options which apply.". Still a placeholder text "optional" for the passphrase would be good, or? Bug: The LUKS dialog is supposed to be unchanged, I think, but it currently shows the VeraCrypt UI elements in the LUKS dialog for me as well (but unusable). So either do always show them in the UI and hide them in LUKS or vice versa. Sorry for comming up with that so late, but concerning PIM, what do you think about a "Numeric" GtkSpinButton(+GtkAdjustment) which is enabled by a GtkSwitch? This allows to validate the input automatically and instead of throwing an error when confirming the dialog. Currently it is not visible that the field does not need to be filled out - GtkSwitch would disable it. Another idea for your current implementation is to add a placeholder text "optional". What do you both think about that? I also marked some minor issues. Would you take care of it? Please reach out if you have questions. Regards, Kai ::: src/disks/gduunlockdialog.c @@ +101,3 @@ + grid = GTK_GRID (gtk_builder_get_object (data->builder, "unlock-device-grid")); + + button = gtk_file_chooser_button_new ("Select a Keyfile", GTK_FILE_CHOOSER_ACTION_OPEN); _() for gettext is missing @@ +200,3 @@ + data->num_keyfiles += 1; + } + data->keyfiles = g_variant_new("as", keyfiles_builder); space before brackets, also next line @@ +210,3 @@ + if (*err || errno || tmp_pim <= 0 || tmp_pim > G_MAXUINT32) + { + error = g_error_new(G_IO_ERROR, G_IO_ERROR_INVALID_DATA, space before brackets, also next lines @@ +286,3 @@ + // Add TCRYPT options if the device is (possibly) a TCRYPT volume + if (g_strcmp0 (data->type, "crypto_TCRYPT") == 0 || g_strcmp0 (data->type, "crypto_unknown") == 0) + { Could you adjust the dialog title here because it currently says "Enter passphrase to unlock"? Maybe something like "Enter encryption options to unlock". ::: src/disks/ui/unlock-device-dialog.ui @@ +133,3 @@ + <object class="GtkCheckButton" id="unlock-device-tcrypt-hidden-check-button"> + <property name="label" translatable="yes">_Hidden</property> + <property name="tooltip_markup" translatable="yes">Whether the volume is a hidden volume.</property> "Instead of unlocking this volume, attempt to unlock a secondary volume hidden inside." @@ +145,3 @@ + <object class="GtkCheckButton" id="unlock-device-tcrypt-system-check-button"> + <property name="label" translatable="yes">Windows system</property> + <property name="tooltip_markup" translatable="yes">Whether the volume is an encrypted system partition or drive. This is only supported for Windows systems.</property> "Unlock an encrypted Windows system partition or drive." @@ +208,3 @@ + <child> + <object class="GtkFileChooserButton" id="unlock-device-tcrypt-keyfile-chooser-button"> + <property name="title">Select a Keyfile</property> translatable="yes"
Thanks for the review. I didn't do a pull request via bugzilla yet: Do you prefer that I attach additional patches on top of the existing one, or a single one which replaces the existing one?
A complete one for the feature please, no fixup patches. I'm ok with packing all into one commit. By the way, according to the current plan next month GNOME Disks will migrate to GNOME's GitLab instance and this here will turn read-only. This implies a workflow with merge requests which is more common nowadays ;)
Created attachment 372149 [details] [review] Add TrueCrypt/VeraCrypt support
(In reply to Kai Lüke from comment #4) > My first impression of using the dialog as a newcomer to VeraCrypt is that it > still required me to read the docs. > Therefore I added some suggestions for the tooltip texts. Thanks, I added them in the new patch. > Another thing I want to propose is communicating what options are required. > How about implementing a logic to enable the button only if sufficent > information is entered, i.e. at least passphrase or keyfile. I'm not sure about this. It seems weird to me that the fields *above* the keyfiles button would only get activated once you set the keyfiles, because it's against the intuitive workflow of going through options from top to bottom. And I put the keyfiles button at the bottom deliberately, because once a keyfile is selected, a new button appears below - I think this works best if the button is at the bottom. What do you think? > Still a placeholder text "optional" for the passphrase would be good, or? I would be ok with that, but I'm not sure of "optional" really fits here. In order to unlock the volume, you have to specify exactly the passphrase used to encrypt the volume, if any. The user can't choose to omit the passphrase (if they used one for the volume). I'm also not that concerned about users being confused about whether they need to specify an option (except for the PIM maybe, see below), because they had to create the volume via TrueCrypt/VeraCrypt, which exposed them to the same options. So they should know which options they used for their volume. > Bug: The LUKS dialog is supposed to be unchanged, I think, but it currently > shows the VeraCrypt UI elements in the LUKS dialog for me as well (but > unusable). > So either do always show them in the UI and hide them in LUKS or vice versa. Oops, you're right of course. This bug was introduced when I removed the expander. > Sorry for comming up with that so late, but concerning PIM, what do you think > about a "Numeric" GtkSpinButton(+GtkAdjustment) which is enabled by a > GtkSwitch? > This allows to validate the input automatically and instead of throwing an > error when confirming the dialog. Currently it is not visible that the field > does not need to be filled out - GtkSwitch would disable it. We considered this before, but I think it would give a false impression of the purpose of the PIM. To me, the GtkSpinButton implies that the user can freely choose a value (which they might want to increase or decrease via the buttons). But for the matter of UX, the PIM is nothing else than an additional password: It has to be set to the exact same value chosen when the volume was created. And it doesn't make sense to increase or decrease it. > Another idea for > your current implementation is to add a placeholder text "optional". I have the same concerns as for the password entry: That "optional" is conceived as "I don't have to put anything in here", while actually the user has to put in the PIM iff they created the volume with one. But in this case, the user might not actually know what a PIM is, because this option is not available in TrueCrypt, but only in VeraCrypt (and even there not very prominently IIRC). So I think it would be good to have a hint here. Maybe something like "if any"? Or do you prefer "optional"? Or maybe the tooltip is good enough? > I also marked some minor issues. Thanks, I fixed those (and some more missing spaces before brackets).
Review of attachment 372149 [details] [review]: Hi, thanks for the update! > the fields *above* the keyfiles button would only get activated once you set the keyfiles Slight misunderstanding here, I mean the dialog action button, not the fields above. Because currently the dialog is confirmable without entering anything and then just an error message is the result, but there is no need for the action button to be enabled if neither passphrase nor keyfiles are specified. Anyway, this is also a problem of the LUKS dialog and should be fixed for both. We can do it in a second patch as well if you don't want to handle this now in this patch, no problem :) > I'm not sure of "optional" really fits here. In order to unlock the volume, you have to specify exactly the passphrase Yes, you are right, "optional" is not the right word (the passphrase has the initial focus, so the text is only visible if e.g. a keyfile is selected first). Same for the PIM case. "if any" is a bit short, maybe "if specified"? But passphrase and PIM can also have different texts. Just tooltips are a bit too less visual information if one encounters the dialog for the first time. > And it doesn't make sense to increase or decrease it. Yes, unfortunately that is the sideffect of using this numeric-only input element. Actually we could filter alphabetic input (c.f. the label change input element for FAT which is also filtered to only have 8 symbols), but as you said, if it's more like a second password and only people with the exact information can open it, I see your points, but maybe state that it is a number in the tooltip. Also given the fact that VeraCrypt support in UDisks requires the presence of /etc/udisks2/tcrypt.conf which distributions might or might not ship… One more thing, did you also get these warnings when using it? I somehow managed to have them but now can't reproduce exactly which corner case it was… (gnome-disks:9291): GLib-GIO-CRITICAL **: 01:46:37.132: g_file_get_uri: assertion 'G_IS_FILE (file)' failed (gnome-disks:9291): GLib-CRITICAL **: 01:46:37.132: g_variant_new_string: assertion 'string != NULL' failed (gnome-disks:9291): GLib-CRITICAL **: 01:46:37.132: g_variant_ref_sink: assertion 'value != NULL' failed (gnome-disks:9291): GLib-GIO-CRITICAL **: 01:46:37.132: g_settings_schema_key_type_check: assertion 'value != NULL' failed (gnome-disks:9291): GLib-CRITICAL **: 01:46:37.132: g_variant_get_type_string: assertion 'value != NULL' failed (gnome-disks:9291): GLib-GIO-CRITICAL **: 01:46:37.132: g_settings_set_value: key 'image-dir-uri' in 'org.gnome.Disks' expects type 's', but a GVariant of type '(null)' was given (gnome-disks:9291): GLib-CRITICAL **: 01:46:37.132: g_variant_unref: assertion 'value != NULL' failed Maybe you have an idea for the conditions to get this? Otherwise I will have a look in the next days. I think we are almost through then to include it, thanks for working on it from libblockdev up to the UI, that's a long way with some stones like building the chain and waiting for APIs to be merged. Best greetings, Kai ::: src/disks/gduunlockdialog.c @@ +288,3 @@ + GtkWidget *button = GTK_WIDGET (gtk_builder_get_object (data->builder, "unlock-device-tcrypt-keyfile-chooser-button")); + + gtk_window_set_title (data->dialog, "Set options to unlock"); _() missing for gettext and also a GTK_WINDOW () cast. warning: passing argument 1 of ‘gtk_window_set_title’ from incompatible pointer type [-Wincompatible-pointer-types] gtk_window_set_title (data->dialog, "Set options to unlock"); note: expected ‘GtkWindow * {aka struct _GtkWindow *}’ but argument is of type ‘GtkWidget * {aka struct _GtkWidget *}’ ::: src/disks/ui/unlock-device-dialog.ui @@ +134,3 @@ + <object class="GtkCheckButton" id="unlock-device-tcrypt-hidden-check-button"> + <property name="label" translatable="yes">_Hidden</property> + <property name="tooltip_markup" translatable="yes">"Instead of unlocking this volume, attempt to unlock a secondary volume hidden inside.</property> copy typo: leading " should not be there @@ +146,3 @@ + <child> + <object class="GtkCheckButton" id="unlock-device-tcrypt-system-check-button"> + <property name="label" translatable="yes">Windows system</property> Last time I forgot to check mnemonic key activation via Alt, so here it would be e.g. _system @@ +168,3 @@ + <property name="no_show_all">True</property> + <property name="can_focus">False</property> + <property name="label" translatable="yes">PIM</property> Here maybe PI_M to activate with Alt-M @@ +183,3 @@ + <child> + <object class="GtkEntry" id="unlock-device-pim-entry"> + <property name="tooltip_markup" translatable="yes">If set, the VeraCrypt PIM (Personal Iterations Multiplier) value to use for this volume.</property> Maybe call it "numeric value" even though Multiplier implies this?
(In reply to Kai Lüke from comment #9) > Review of attachment 372149 [details] [review] [review]: > > Slight misunderstanding here, I mean the dialog action button, not the > fields above. Because currently the dialog is confirmable without entering > anything and then just an error message is the result, but there is no need > for the action button to be enabled if neither passphrase nor keyfiles are > specified. Ah ok, got it :) > Anyway, this is also a problem of the LUKS dialog and should be fixed for > both. We can do it in a second patch as well if you don't want to handle > this now in this patch, no problem :) I implemented it now. > > I'm not sure of "optional" really fits here. In order to unlock the volume, you have to specify exactly the passphrase > > Yes, you are right, "optional" is not the right word (the passphrase has the > initial focus, so the text is only visible if e.g. a keyfile is selected > first). Same for the PIM case. "if any" is a bit short, maybe "if > specified"? But passphrase and PIM can also have different texts. Just > tooltips are a bit too less visual information if one encounters the dialog > for the first time. Ok, I added "if specified" for now. > > And it doesn't make sense to increase or decrease it. > > Yes, unfortunately that is the sideffect of using this numeric-only input > element. Actually we could filter alphabetic input (c.f. the label change > input element for FAT which is also filtered to only have 8 symbols), but as > you said, if it's more like a second password and only people with the exact > information can open it, I see your points, but maybe state that it is a > number in the tooltip. I added "numeric" to the tooltip. > One more thing, did you also get these warnings when using it? I somehow > managed to have them but now can't reproduce exactly which corner case it > was… I found and fixed a bug where I got different warnings: (gnome-disks:1212): Gtk-CRITICAL **: 17:09:44.137: gtk_builder_get_object: assertion 'GTK_IS_BUILDER (builder)' failed (gnome-disks:1212): Gtk-CRITICAL **: 17:09:44.214: gtk_grid_attach_next_to: assertion 'GTK_IS_GRID (grid)' failed (gnome-disks:1212): Gtk-CRITICAL **: 17:09:44.214: gtk_entry_get_text_length: assertion 'GTK_IS_ENTRY (entry)' failed (gnome-disks:1212): Gtk-CRITICAL **: 17:09:44.214: gtk_widget_set_sensitive: assertion 'GTK_IS_WIDGET (widget)' failed It was caused by an incorrect function signature of on_tcrypt_keyfile_set(), which for some reason had a GParamSpec parameter which was not expected. But I haven't seen the warning you posted and have no idea what could have caused them. > I think we are almost through then to include it, Cool! :) > thanks for working on it > from libblockdev up to the UI, that's a long way with some stones like > building the chain and waiting for APIs to be merged. Yeap, it's been a long way :) And it's not done yet, the next step is the integration into GvFS. > ::: src/disks/gduunlockdialog.c > @@ +288,3 @@ > + GtkWidget *button = GTK_WIDGET (gtk_builder_get_object > (data->builder, "unlock-device-tcrypt-keyfile-chooser-button")); > + > + gtk_window_set_title (data->dialog, "Set options to unlock"); > > _() missing for gettext and also a GTK_WINDOW () cast. Fixed. > ::: src/disks/ui/unlock-device-dialog.ui > @@ +134,3 @@ > + <object class="GtkCheckButton" > id="unlock-device-tcrypt-hidden-check-button"> > + <property name="label" > translatable="yes">_Hidden</property> > + <property name="tooltip_markup" > translatable="yes">"Instead of unlocking this volume, attempt to unlock a > secondary volume hidden inside.</property> > > copy typo: leading " should not be there Fixed. > @@ +146,3 @@ > + <child> > + <object class="GtkCheckButton" > id="unlock-device-tcrypt-system-check-button"> > + <property name="label" translatable="yes">Windows > system</property> > > Last time I forgot to check mnemonic key activation via Alt, so here it > would be e.g. _system Done. > @@ +168,3 @@ > + <property name="no_show_all">True</property> > + <property name="can_focus">False</property> > + <property name="label" translatable="yes">PIM</property> > > Here maybe PI_M to activate with Alt-M Done. > > @@ +183,3 @@ > + <child> > + <object class="GtkEntry" id="unlock-device-pim-entry"> > + <property name="tooltip_markup" translatable="yes">If set, > the VeraCrypt PIM (Personal Iterations Multiplier) value to use for this > volume.</property> > > Maybe call it "numeric value" even though Multiplier implies this? Done.
Created attachment 372188 [details] [review] Add TrueCrypt/VeraCrypt support
Sorry, I saw too late the "Check each existing attachment made obsolete by your new attachment." option, and I can't find another option to mark an attachment as obsolete.
Thanks, will test it again and have a look at these warnings. Yes, that's hidden with two more clicks in "Details", I've set it now.
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gnome-disk-utility/issues/84.