[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[PATCH v4 0/7] Refactor/improve cryptomount data passing to crypto modul
|
From: |
Glenn Washburn |
|
Subject: |
[PATCH v4 0/7] Refactor/improve cryptomount data passing to crypto modules |
|
Date: |
Sat, 4 Dec 2021 01:15:43 -0600 |
Updates since v3:
* Many updates based on feedback from Daniel and Patrick
* Make removal of global "have_it" happen before rearchitecting cryptomount
arg passing
* Add changes that improve cryptomount error messaging
---
This patch series refactors the way cryptomount passes data to the crypto
modules. Currently, the method has been by global variable and function call
argument, neither of which are ideal. This method passes data via a
grub_cryptomount_args struct, which can be added to over time as opposed to
continually adding arguments to the cryptodisk recover_key (as is being
proposed in the keyfile and detached header patches).
Patch #4 removes the found_uuid flag from the cargs struct, which is not
needed because the same information can be obtained from the return value
of grub_device_iterate.
To make thing simpler and easier to understand, the "have_it" global variable
is gotten rid of first in patch #2. Taking advantage of this change, patch #3
improves some long standing issues in cryptomount error messaging.
Then, the infrastructure for passing argument data to cryptodisk backends is
implemented in patch #4 along with adding a new -p parameter to cryptomount
partly as an example to show how a password would be passed to the crypto
module backends. The backends do nothing with this data in this patch, but
print a message saying that sending a password is unimplemented.
Patch #5 takes advantage of this new data passing mechanism to refactor the
essentially duplicated code in each crypto backend module for inputting the
password and puts that functionality in the cryptodisk code. Conceptually,
the crypto backends should not be getting user input anyway.
Patch #6 gets rid of some long time globals in cryptodisk, moving them
into the passed struct.
My intention is for this patch series to lay the foundation for an improved
patch series providing detached header and keyfile support (I already have
the series updated and ready to send once this is accepted). I also believe
tha this will somewhat simplify the patch series by James Bottomley in
passing secrets to the crypto backends.
Glenn
Glenn Washburn (7):
luks2: Add debug message to align with luks and geli modules
cryptodisk: Refactor to discard have_it global
cryptodisk: Improve error messaging in cryptomount invocations
cryptodisk: Add infrastructure to pass data from cryptomount to
cryptodisk modules
cryptodisk: Refactor password input out of crypto dev modules into
cryptodisk
cryptodisk: Move global variables into grub_cryptomount_args struct
cryptodisk: Improve handling of partition name in cryptomount password
prompt
docs/grub.texi | 9 ++-
grub-core/disk/cryptodisk.c | 147 +++++++++++++++++++++++++-----------
grub-core/disk/geli.c | 35 +++------
grub-core/disk/luks.c | 37 +++------
grub-core/disk/luks2.c | 38 ++++------
include/grub/cryptodisk.h | 19 ++++-
include/grub/err.h | 1 +
7 files changed, 165 insertions(+), 121 deletions(-)
Range-diff against v3:
-: --------- > 1: c71461896 luks2: Add debug message to align with luks and
geli modules
-: --------- > 2: 37c2adcf5 cryptodisk: Refactor to discard have_it global
-: --------- > 3: 675aaf68c cryptodisk: Improve error messaging in
cryptomount invocations
1: ef344591f ! 4: 6def57f22 cryptodisk: Add infrastructure to pass data from
cryptomount to cryptodisk modules
@@ Metadata
## Commit message ##
cryptodisk: Add infrastructure to pass data from cryptomount to
cryptodisk modules
+ Previously, the cryptomount arguments were passed by global variable
and
+ function call argument, neither of which are ideal. This change passes
data
+ via a grub_cryptomount_args struct, which can be added to over time as
+ opposed to continually adding arguments to the cryptodisk scan and
+ recover_key.
+
As an example, passing a password as a cryptomount argument is
implemented.
However, the backends are not implemented, so testing this will return
a not
implemented error.
+ Also, add comments to cryptomount argument parsing to make it more
obvious
+ which argument states are being handled.
+
## grub-core/disk/cryptodisk.c ##
@@ grub-core/disk/cryptodisk.c: static const struct grub_arg_option
options[] =
/* TRANSLATORS: It's still restricted to cryptodisks only. */
@@ grub-core/disk/cryptodisk.c: grub_cryptodisk_scan_device (const char
*name,
+ err = grub_cryptodisk_scan_device_real (name, source, cargs);
grub_disk_close (source);
-
+
@@ grub-core/disk/cryptodisk.c: static grub_err_t
grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
{
@@ grub-core/disk/cryptodisk.c: static grub_err_t
if (argc < 1 && !state[1].set && !state[2].set)
return grub_error (GRUB_ERR_BAD_ARGUMENT, "device name required");
+@@ grub-core/disk/cryptodisk.c: grub_cmd_cryptomount
(grub_extcmd_context_t ctxt, int argc, char **args)
+ if (grub_cryptodisk_list == NULL)
+ return grub_error (GRUB_ERR_BAD_MODULE, "no cryptodisk modules
loaded");
+- if (state[0].set)
+ if (state[3].set) /* password */
+ {
+ cargs.key_data = (grub_uint8_t *) state[3].arg;
+ cargs.key_len = grub_strlen (state[3].arg);
+ }
+
- have_it = 0;
-- if (state[0].set)
+ if (state[0].set) /* uuid */
{
+ int found_uuid;
grub_cryptodisk_t dev;
-
@@ grub-core/disk/cryptodisk.c: grub_cmd_cryptomount
(grub_extcmd_context_t ctxt, int argc, char **args)
check_boot = state[2].set;
search_uuid = args[0];
-- grub_device_iterate (&grub_cryptodisk_scan_device, NULL);
-+ grub_device_iterate (&grub_cryptodisk_scan_device, &cargs);
+- found_uuid = grub_device_iterate (&grub_cryptodisk_scan_device,
NULL);
++ found_uuid = grub_device_iterate (&grub_cryptodisk_scan_device,
&cargs);
search_uuid = NULL;
- if (!have_it)
- return grub_error (GRUB_ERR_BAD_ARGUMENT, "no such cryptodisk found");
- return GRUB_ERR_NONE;
+ if (found_uuid)
+@@ grub-core/disk/cryptodisk.c: grub_cmd_cryptomount
(grub_extcmd_context_t ctxt, int argc, char **args)
+ }
+ return grub_errno;
}
- else if (state[1].set || (argc == 0 && state[2].set))
+ else if (state[1].set || (argc == 0 && state[2].set)) /* -a|-b */
@@ grub-core/disk/geli.c: recover_key (grub_disk_t source,
grub_cryptodisk_t dev)
grub_err_t err;
+ /* Keyfiles are not implemented yet */
-+ if (cargs->key_data || cargs->key_len)
++ if (cargs->key_data != NULL || cargs->key_len)
+ return GRUB_ERR_NOT_IMPLEMENTED_YET;
+
if (dev->cipher->cipher->blocksize > GRUB_CRYPTO_MAX_CIPHER_BLOCKSIZE)
@@ grub-core/disk/luks.c: luks_recover_key (grub_disk_t source,
char *tmp;
+ /* Keyfiles are not implemented yet */
-+ if (cargs->key_data || cargs->key_len)
++ if (cargs->key_data != NULL || cargs->key_len)
+ return GRUB_ERR_NOT_IMPLEMENTED_YET;
+
err = grub_disk_read (source, 0, 0, sizeof (header), &header);
@@ grub-core/disk/luks2.c: luks2_recover_key (grub_disk_t source,
grub_err_t ret;
+ /* Keyfiles are not implemented yet */
-+ if (cargs->key_data || cargs->key_len)
++ if (cargs->key_data != NULL || cargs->key_len)
+ return GRUB_ERR_NOT_IMPLEMENTED_YET;
+
ret = luks2_read_header (source, &header);
2: cfeb864ce ! 5: ef6394f1e cryptodisk: Refactor password input out of crypto
dev modules into cryptodisk
@@ Commit message
not getting user input. This has the added benefit of simplifying the
code
such that three essentially duplicate pieces of code are merged into
one.
+ Add documentation of passphrase option for cryptomount as it is now
usable.
+
+ ## docs/grub.texi ##
+@@ docs/grub.texi: Alias for @code{hashsum --hash crc32 arg @dots{}}. See
command @command{hashsum}
+ @node cryptomount
+ @subsection cryptomount
+
+-@deffn Command cryptomount device|@option{-u} uuid|@option{-a}|@option{-b}
+-Setup access to encrypted device. If necessary, passphrase
+-is requested interactively. Option @var{device} configures specific grub
device
++@deffn Command cryptomount [@option{-p} password] device|@option{-u}
uuid|@option{-a}|@option{-b}
++Setup access to encrypted device. If @option{-p} is not given, a
passphrase
++is requested interactively. Otherwise, the given @var{password} will be
used and
++no passphrase will be requested interactively.
++Option @var{device} configures specific grub device
+ (@pxref{Naming convention}); option @option{-u} @var{uuid} configures
device
+ with specified @var{uuid}; option @option{-a} configures all detected
encrypted
+ devices; option @option{-b} configures all geli containers that have boot
flag set.
+
++
+ GRUB suports devices encrypted using LUKS, LUKS2 and geli. Note that
necessary
+ modules (@var{luks}, @var{luks2} and @var{geli}) have to be loaded
manually
+ before this command can be used. For LUKS2 only the PBKDF2 key derivation
+
## grub-core/disk/cryptodisk.c ##
@@ grub-core/disk/cryptodisk.c: grub_cryptodisk_scan_device_real (const
char *name,
grub_disk_t source,
@@ grub-core/disk/cryptodisk.c: grub_cryptodisk_scan_device_real (const
char *name,
- return err;
- }
+
-+ if (cargs->key_len == 0)
++ if (!cargs->key_len)
+ {
+ /* Get the passphrase from the user, if no key data. */
+ askpass = 1;
-+ if (source->partition)
++ if (source->partition != NULL)
+ part = grub_partition_get_name (source->partition);
+ grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "), source->name,
-+ source->partition ? "," : "", part ? : "",
++ source->partition != NULL ? "," : "",
++ part != NULL ? part : "",
+ dev->uuid);
+ grub_free (part);
+
+ cargs->key_data = grub_malloc (GRUB_CRYPTODISK_MAX_PASSPHRASE);
-+ if (!cargs->key_data)
++ if (cargs->key_data == NULL)
+ return grub_errno;
+
+ if (!grub_password_get ((char *) cargs->key_data,
GRUB_CRYPTODISK_MAX_PASSPHRASE))
+ {
-+ ret = grub_error (GRUB_ERR_BAD_ARGUMENT, "Passphrase not supplied");
++ ret = grub_error (GRUB_ERR_BAD_ARGUMENT, "passphrase not supplied");
+ goto error;
+ }
+ cargs->key_len = grub_strlen ((char *) cargs->key_data);
+ }
+
+ ret = cr->recover_key (source, dev, cargs);
-+ if (ret)
++ if (ret != GRUB_ERR_NONE)
+ goto error;
grub_cryptodisk_insert (dev, name, source);
- have_it = 1;
-
- return GRUB_ERR_NONE;
+ goto cleanup;
}
-- return GRUB_ERR_NONE;
+- return grub_error (GRUB_ERR_BAD_MODULE, "no cryptodisk module can
handle this device");
++ ret = grub_error (GRUB_ERR_BAD_MODULE, "no cryptodisk module can handle
this device");
+ goto cleanup;
+
-+error:
++ error:
+ cryptodisk_close (dev);
-+cleanup:
++
++ cleanup:
+ if (askpass)
+ {
+ cargs->key_len = 0;
@@ grub-core/disk/geli.c: recover_key (grub_disk_t source,
grub_cryptodisk_t dev, g
grub_err_t err;
- /* Keyfiles are not implemented yet */
-- if (cargs->key_data || cargs->key_len)
+- if (cargs->key_data != NULL || cargs->key_len)
- return GRUB_ERR_NOT_IMPLEMENTED_YET;
+ if (cargs->key_data == NULL || cargs->key_len == 0)
-+ return grub_error (GRUB_ERR_BAD_ARGUMENT, "No key data");
++ return grub_error (GRUB_ERR_BAD_ARGUMENT, "no key data");
if (dev->cipher->cipher->blocksize > GRUB_CRYPTO_MAX_CIPHER_BLOCKSIZE)
return grub_error (GRUB_ERR_BUG, "cipher block is too long");
@@ grub-core/disk/luks.c: luks_recover_key (grub_disk_t source,
- char *tmp;
- /* Keyfiles are not implemented yet */
-- if (cargs->key_data || cargs->key_len)
+- if (cargs->key_data != NULL || cargs->key_len)
- return GRUB_ERR_NOT_IMPLEMENTED_YET;
-+ if (cargs->key_data == NULL || cargs->key_len == 0)
-+ return grub_error (GRUB_ERR_BAD_ARGUMENT, "No key data");
++ if (cargs->key_data == NULL || cargs->key_len == 0)
++ return grub_error (GRUB_ERR_BAD_ARGUMENT, "no key data");
err = grub_disk_read (source, 0, 0, sizeof (header), &header);
if (err)
@@ grub-core/disk/luks2.c: luks2_recover_key (grub_disk_t source,
grub_err_t ret;
- /* Keyfiles are not implemented yet */
-- if (cargs->key_data || cargs->key_len)
+- if (cargs->key_data != NULL || cargs->key_len)
- return GRUB_ERR_NOT_IMPLEMENTED_YET;
+ if (cargs->key_data == NULL || cargs->key_len == 0)
-+ return grub_error (GRUB_ERR_BAD_ARGUMENT, "No key data");
++ return grub_error (GRUB_ERR_BAD_ARGUMENT, "no key data");
ret = luks2_read_header (source, &header);
if (ret)
3: bfe1a2708 ! 6: 242ee2798 cryptodisk: Move global variables into
grub_cryptomount_args struct
@@ Metadata
## Commit message ##
cryptodisk: Move global variables into grub_cryptomount_args struct
+ Note that cargs.search_uuid does not need to be initialized in various
parts
+ of the cryptomount argument parsing, just once when cargs is declared
with a
+ struct initializer. The previous code used a global variable which
would
+ retain the value across cryptomount invocations.
+
## grub-core/disk/cryptodisk.c ##
@@ grub-core/disk/cryptodisk.c: grub_util_cryptodisk_get_uuid (grub_disk_t
disk)
#endif
--static int check_boot, have_it;
+-static int check_boot;
-static char *search_uuid;
-
static void
@@ grub-core/disk/cryptodisk.c: grub_util_cryptodisk_get_uuid (grub_disk_t
disk)
{
@@ grub-core/disk/cryptodisk.c: grub_cryptodisk_scan_device_real (const
char *name,
+ if (dev)
+ {
+- if (grub_strcasecmp (search_uuid, dev->uuid) == 0)
++ if (grub_strcasecmp (cargs->search_uuid, dev->uuid) == 0)
+ return GRUB_ERR_NONE;
+ else
+ return GRUB_ERR_EXISTS;
+@@ grub-core/disk/cryptodisk.c: grub_cryptodisk_scan_device_real (const
char *name,
+
FOR_CRYPTODISK_DEVS (cr)
{
- dev = cr->scan (source, search_uuid, check_boot);
@@ grub-core/disk/cryptodisk.c: grub_cryptodisk_scan_device_real (const
char *name,
if (grub_errno)
return grub_errno;
if (!dev)
-@@ grub-core/disk/cryptodisk.c: grub_cryptodisk_scan_device_real (const
char *name,
-
- grub_cryptodisk_insert (dev, name, source);
-
-- have_it = 1;
-+ cargs->found_uuid = 1;
+@@ grub-core/disk/cryptodisk.c: grub_cryptodisk_cheat_mount (const char
*sourcedev, const char *cheat)
+ grub_cryptodisk_t dev;
+ grub_cryptodisk_dev_t cr;
+ grub_disk_t source;
++ struct grub_cryptomount_args cargs = {0};
- goto cleanup;
- }
+ /* Try to open disk. */
+ source = grub_disk_open (sourcedev);
@@ grub-core/disk/cryptodisk.c: grub_cryptodisk_cheat_mount (const char
*sourcedev, const char *cheat)
FOR_CRYPTODISK_DEVS (cr)
{
- dev = cr->scan (source, search_uuid, check_boot);
-+ dev = cr->scan (source, NULL);
++ dev = cr->scan (source, &cargs);
if (grub_errno)
return grub_errno;
if (!dev)
@@ grub-core/disk/cryptodisk.c: grub_cryptodisk_scan_device (const char
*name,
-
- if (err)
+ grub_error_push();
+ else
grub_print_error ();
-- return have_it && search_uuid ? 1 : 0;
-+ return (cargs->found_uuid && cargs->search_uuid) ? 1 : 0;
+- return (err == GRUB_ERR_NONE && search_uuid != NULL);
++ return (err == GRUB_ERR_NONE && cargs->search_uuid != NULL);
}
static grub_err_t
-@@ grub-core/disk/cryptodisk.c: grub_cmd_cryptomount
(grub_extcmd_context_t ctxt, int argc, char **args)
- cargs.key_len = grub_strlen (state[3].arg);
- }
-
-- have_it = 0;
- if (state[0].set) /* uuid */
- {
- grub_cryptodisk_t dev;
@@ grub-core/disk/cryptodisk.c: grub_cmd_cryptomount
(grub_extcmd_context_t ctxt, int argc, char **args)
return GRUB_ERR_NONE;
}
@@ grub-core/disk/cryptodisk.c: grub_cmd_cryptomount (grub_extcmd_context_t
ctxt, i
- search_uuid = args[0];
+ cargs.check_boot = state[2].set;
+ cargs.search_uuid = args[0];
- grub_device_iterate (&grub_cryptodisk_scan_device, &cargs);
+ found_uuid = grub_device_iterate (&grub_cryptodisk_scan_device,
&cargs);
- search_uuid = NULL;
-- if (!have_it)
-+ if (!cargs.found_uuid)
- return grub_error (GRUB_ERR_BAD_ARGUMENT, "no such cryptodisk found");
- return GRUB_ERR_NONE;
+ if (found_uuid)
+ return GRUB_ERR_NONE;
+@@ grub-core/disk/cryptodisk.c: grub_cmd_cryptomount
(grub_extcmd_context_t ctxt, int argc, char **args)
}
else if (state[1].set || (argc == 0 && state[2].set)) /* -a|-b */
{
@@ grub-core/disk/geli.c: configure_ciphers (grub_disk_t disk, const char
*check_uu
}
- if (check_uuid && grub_strcasecmp (check_uuid, uuid) != 0)
-+ if (cargs->search_uuid && grub_strcasecmp (cargs->search_uuid, uuid) !=
0)
++ if (cargs->search_uuid != NULL && grub_strcasecmp (cargs->search_uuid,
uuid) != 0)
{
- grub_dprintf ("geli", "%s != %s\n", uuid, check_uuid);
+ grub_dprintf ("geli", "%s != %s\n", uuid, cargs->search_uuid);
@@ grub-core/disk/luks.c: configure_ciphers (grub_disk_t disk, const char
*check_uu
*optr = 0;
- if (check_uuid && grub_strcasecmp (check_uuid, uuid) != 0)
-+ if (cargs->search_uuid && grub_strcasecmp (cargs->search_uuid, uuid) !=
0)
++ if (cargs->search_uuid != NULL && grub_strcasecmp (cargs->search_uuid,
uuid) != 0)
{
- grub_dprintf ("luks", "%s != %s\n", uuid, check_uuid);
+ grub_dprintf ("luks", "%s != %s\n", uuid, cargs->search_uuid);
return NULL;
}
-@@ grub-core/disk/luks.c: luks_recover_key (grub_disk_t source,
- grub_err_t err;
- grub_size_t max_stripes = 1;
-
-- if (cargs->key_data == NULL || cargs->key_len == 0)
-+ if (cargs->key_data == NULL || cargs->key_len == 0)
- return grub_error (GRUB_ERR_BAD_ARGUMENT, "No key data");
-
- err = grub_disk_read (source, 0, 0, sizeof (header), &header);
## grub-core/disk/luks2.c ##
@@ grub-core/disk/luks2.c: luks2_read_header (grub_disk_t disk,
grub_luks2_header_t *outhdr)
@@ grub-core/disk/luks2.c: luks2_scan (grub_disk_t disk, const char
*check_uuid, in
uuid[j] = '\0';
- if (check_uuid && grub_strcasecmp (check_uuid, uuid) != 0)
-+ if (cargs->search_uuid && grub_strcasecmp (cargs->search_uuid, uuid) !=
0)
- return NULL;
++ if (cargs->search_uuid != NULL && grub_strcasecmp (cargs->search_uuid,
uuid) != 0)
+ {
+- grub_dprintf ("luks2", "%s != %s\n", uuid, check_uuid);
++ grub_dprintf ("luks2", "%s != %s\n", uuid, cargs->search_uuid);
+ return NULL;
+ }
- cryptodisk = grub_zalloc (sizeof (*cryptodisk));
## include/grub/cryptodisk.h ##
@@ include/grub/cryptodisk.h: typedef gcry_err_code_t
@@ include/grub/cryptodisk.h: typedef gcry_err_code_t
{
+ /* scan: Flag to indicate that only bootable volumes should be
decrypted */
+ grub_uint32_t check_boot : 1;
-+ grub_uint32_t found_uuid : 1;
+ /* scan: Only volumes matching this UUID should be decrpyted */
+ char *search_uuid;
+ /* recover_key: Key data used to decrypt voume */
4: 157e08487 < -: --------- cryptodisk: Remove unneeded found_uuid from
cryptomount args
-: --------- > 7: 40a6f2d1b cryptodisk: Improve handling of partition name in
cryptomount password prompt
--
2.27.0