After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 796146 - Support VeraCrypt
Support VeraCrypt
Status: RESOLVED OBSOLETE
Product: gnome-disk-utility
Classification: Core
Component: Disks UI
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gnome-disk-utility-maint
gnome-disk-utility-maint
Depends on:
Blocks:
 
 
Reported: 2018-05-15 18:11 UTC by segfault
Modified: 2018-05-24 10:45 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Add support for unlocking TrueCrypt/VeraCrypt (21.36 KB, patch)
2018-05-15 18:13 UTC, segfault
needs-work Details | Review
Screenshot of unlock dialog with TrueCrypt/VeraCrypt options (84.97 KB, image/png)
2018-05-15 18:14 UTC, segfault
  Details
Add TrueCrypt/VeraCrypt support (22.06 KB, patch)
2018-05-17 13:21 UTC, segfault
needs-work Details | Review
Add TrueCrypt/VeraCrypt support (24.15 KB, patch)
2018-05-18 15:55 UTC, segfault
none Details | Review

Description segfault 2018-05-15 18:11:48 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
Comment 1 segfault 2018-05-15 18:13:16 UTC
Created attachment 372080 [details] [review]
Add support for unlocking TrueCrypt/VeraCrypt
Comment 2 segfault 2018-05-15 18:14:08 UTC
Created attachment 372081 [details]
Screenshot of unlock dialog with TrueCrypt/VeraCrypt options
Comment 3 segfault 2018-05-15 18:14:36 UTC
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.
Comment 4 Kai Lüke 2018-05-16 19:30:18 UTC
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"
Comment 5 segfault 2018-05-17 11:08:08 UTC
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?
Comment 6 Kai Lüke 2018-05-17 12:44:10 UTC
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 ;)
Comment 7 segfault 2018-05-17 13:21:25 UTC
Created attachment 372149 [details] [review]
Add TrueCrypt/VeraCrypt support
Comment 8 segfault 2018-05-17 13:23:17 UTC
(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).
Comment 9 Kai Lüke 2018-05-17 17:14:29 UTC
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?
Comment 10 segfault 2018-05-18 15:54:22 UTC
(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.
Comment 11 segfault 2018-05-18 15:55:05 UTC
Created attachment 372188 [details] [review]
Add TrueCrypt/VeraCrypt support
Comment 12 segfault 2018-05-18 15:59:48 UTC
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.
Comment 13 Kai Lüke 2018-05-21 07:22:00 UTC
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.
Comment 14 GNOME Infrastructure Team 2018-05-24 10:45:11 UTC
-- 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.