Re: [PATCH v8 3/7] cryptodisk: enable the backends to implement detached headers

[Glenn Washburn]

On Tue, 4 Jan 2022 15:42:22 -0600
Glenn Washburn <development@efficientek.com> wrote:

> This comes from Dmitry author of the previously submitted LUKS2 master
> key support. Since he's not on the list, I'm taking some of his
> response and re-posting it here (modified to be faithful to his
> original message) and will add him to future discussions on this.
> 
> Glenn
> 
> On Tue, 4 Jan 2022 21:25:14 +0300
> Dmitry <reagentoo@gmail.com> wrote:
> 
> > Signed-off-by: John Lane <john@lane.uk.net>
> > GNUtoo@cyberdimension.org: rebase, patch split, small fixes, commit message
> > Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
> > development@efficientek.com: rebase, rework for cryptomount parameter 
> > passing
> > Signed-off-by: Glenn Washburn <development@efficientek.com>
> > ---
> >  grub-core/disk/cryptodisk.c | 15 ++++++++++++++-
> >  grub-core/disk/geli.c       | 10 ++++++++++
> >  grub-core/disk/luks.c       |  8 ++++++++
> >  grub-core/disk/luks2.c      |  8 ++++++++
> >  include/grub/cryptodisk.h   |  2 ++
> >  include/grub/file.h         |  2 ++
> >  6 files changed, 44 insertions(+), 1 deletion(-)
> > 
> > diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> > index 497097394..e90f680f0 100644
> > --- a/grub-core/disk/cryptodisk.c
> > +++ b/grub-core/disk/cryptodisk.c
> > @@ -42,6 +42,7 @@ static const struct grub_arg_option options[] =
> >      {"all", 'a', 0, N_("Mount all."), 0, 0},
> >      {"boot", 'b', 0, N_("Mount all volumes with `boot' flag set."), 0, 0},
> >      {"password", 'p', 0, N_("Password to open volumes."), 0, 
> > ARG_TYPE_STRING},
> > +    {"header", 'H', 0, N_("Read header from file"), 0, ARG_TYPE_STRING},
> >      {0, 0, 0, 0, 0, 0}
> >    };
> >  
> > @@ -1173,6 +1174,18 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, 
> > int argc, char **args)
> >        cargs.key_len = grub_strlen (state[3].arg);
> >      }
> >  
> > +  if (state[4].set) /* Detached header */
> > +    {
> > +      if (state[0].set)
> > +   return grub_error (GRUB_ERR_BAD_ARGUMENT,
> > +                      N_("Cannot use UUID lookup with detached header"));
> > +
> > +      cargs.hdr_file = grub_file_open (state[4].arg,
> > +                       GRUB_FILE_TYPE_CRYPTODISK_DETACHED_HEADER);
> > +      if (!cargs.hdr_file)
> > +   return grub_errno;
> > +    }
> > +
> >    if (state[0].set) /* uuid */
> >      {
> >        int found_uuid;
> > @@ -1385,7 +1398,7 @@ GRUB_MOD_INIT (cryptodisk)
> >  {
> >    grub_disk_dev_register (&grub_cryptodisk_dev);
> >    cmd = grub_register_extcmd ("cryptomount", grub_cmd_cryptomount, 0,
> > -                         N_("[-p password] <SOURCE|-u UUID|-a|-b>"),
> > +                         N_("[-p password] [-H file] <SOURCE|-u 
> > UUID|-a|-b>"),
> >                           N_("Mount a crypto device."), options);
> >    grub_procfs_register ("luks_script", &luks_script);
> >  }
> > diff --git a/grub-core/disk/geli.c b/grub-core/disk/geli.c
> > index 5b3a11881..0b8046746 100644
> > --- a/grub-core/disk/geli.c
> > +++ b/grub-core/disk/geli.c
> > @@ -52,6 +52,7 @@
> >  #include <grub/dl.h>
> >  #include <grub/err.h>
> >  #include <grub/disk.h>
> > +#include <grub/file.h>
> >  #include <grub/crypto.h>
> >  #include <grub/partition.h>
> >  #include <grub/i18n.h>
> > @@ -121,6 +122,7 @@ enum
> >  
> >  /* FIXME: support version 0.  */
> >  /* FIXME: support big-endian pre-version-4 volumes.  */
> > +/* FIXME: support for detached headers.  */
> >  /* FIXME: support for keyfiles.  */
> >  /* FIXME: support for HMAC.  */
> >  const char *algorithms[] = {
> > @@ -252,6 +254,10 @@ geli_scan (grub_disk_t disk, grub_cryptomount_args_t 
> > cargs)
> >    grub_disk_addr_t sector;
> >    grub_err_t err;
> >  
> > +  /* Detached headers are not implemented yet */
> > +  if (cargs->hdr_file)
> > +    return NULL;
> > +
> >    if (2 * GRUB_MD_SHA256->mdlen + 1 > GRUB_CRYPTODISK_MAX_UUID_LENGTH)
> >      return NULL;
> >  
> > @@ -412,6 +418,10 @@ geli_recover_key (grub_disk_t source, 
> > grub_cryptodisk_t dev, grub_cryptomount_ar
> >    if (cargs->key_data == NULL || cargs->key_len == 0)
> >      return grub_error (GRUB_ERR_BAD_ARGUMENT, "no key data");
> >  
> > +  /* Detached headers are not implemented yet */
> > +  if (cargs->hdr_file)
> > +     return GRUB_ERR_NOT_IMPLEMENTED_YET;
> > +
> >    if (dev->cipher->cipher->blocksize > GRUB_CRYPTO_MAX_CIPHER_BLOCKSIZE)
> >      return grub_error (GRUB_ERR_BUG, "cipher block is too long");
> >  
> > diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
> > index d57257b3e..032a9db3c 100644
> > --- a/grub-core/disk/luks.c
> > +++ b/grub-core/disk/luks.c
> > @@ -75,6 +75,10 @@ luks_scan (grub_disk_t disk, grub_cryptomount_args_t 
> > cargs)
> >    char hashspec[sizeof (header.hashSpec) + 1];
> >    grub_err_t err;
> >  
> > +  /* Detached headers are not implemented yet */
> > +  if (cargs->hdr_file)
> > +    return NULL;
> > +
> >    if (cargs->check_boot)
> >      return NULL;
> >  
> > @@ -164,6 +168,10 @@ luks_recover_key (grub_disk_t source,
> >    if (cargs->key_data == NULL || cargs->key_len == 0)
> >      return grub_error (GRUB_ERR_BAD_ARGUMENT, "no key data");
> >  
> > +  /* Detached headers are not implemented yet */
> > +  if (cargs->hdr_file)
> > +     return GRUB_ERR_NOT_IMPLEMENTED_YET;
> > +
> >    err = grub_disk_read (source, 0, 0, sizeof (header), &header);
> >    if (err)
> >      return err;
> > diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
> > index ccfacb63a..567368f11 100644
> > --- a/grub-core/disk/luks2.c
> > +++ b/grub-core/disk/luks2.c
> > @@ -353,6 +353,10 @@ luks2_scan (grub_disk_t disk, grub_cryptomount_args_t 
> > cargs)
> >    char uuid[sizeof (header.uuid) + 1];
> >    grub_size_t i, j;
> >  
> > +  /* Detached headers are not implemented yet */
> > +  if (cargs->hdr_file)
> > +    return NULL;
> > +
> >    if (cargs->check_boot)
> >      return NULL;
> >  
> > @@ -560,6 +564,10 @@ luks2_recover_key (grub_disk_t source,
> >    if (cargs->key_data == NULL || cargs->key_len == 0)
> >      return grub_error (GRUB_ERR_BAD_ARGUMENT, "no key data");
> >  
> > +  /* Detached headers are not implemented yet */
> > +  if (cargs->hdr_file)
> > +     return GRUB_ERR_NOT_IMPLEMENTED_YET;
> > +
> >    ret = luks2_read_header (source, &header);
> >    if (ret)
> >      return ret;
> > diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
> > index c6524c9ea..9fe451de9 100644
> > --- a/include/grub/cryptodisk.h
> > +++ b/include/grub/cryptodisk.h
> > @@ -20,6 +20,7 @@
> >  #define GRUB_CRYPTODISK_HEADER     1
> >  
> >  #include <grub/disk.h>
> > +#include <grub/file.h>
> >  #include <grub/crypto.h>
> >  #include <grub/list.h>
> >  #ifdef GRUB_UTIL
> > @@ -77,6 +78,7 @@ struct grub_cryptomount_args
> >    grub_uint8_t *key_data;
> >    /* recover_key: Length of key_data */
> >    grub_size_t key_len;
> > +  grub_file_t hdr_file;
> 
> You pass the external header in the file using grub_file_t.
> Wouldn't it be better to use grub_disk_t for this? This will greatly
> shorten the code.
> You don't have to change the code of the luks2_read_header() and
> luks2_scan() functions.
> Plus it will give more flexibility to the user. Optionally, the user
> can use a separate disk for the header, or open the file with "loopback
> hdr_loop (...)/path/to/hdr"

I'm generally very pro-flexibility, but I'm not sure I like this from a
user perspective. For the common case, which is detached headers in a
file, this will cause the user to do more work (create a loopback
device of the file). What's a reasonable scenario where a user would
want the detached header on a device as opposed to a file system? Am I
correct in thinking that you use such functionality?

I do like that it simplifies the code. An alternative route to get the
same simplification while still being given a file path as an argument
to cryptomount would be to internally create a loopback device and use
that instead of passing a file around. A possible undesirabe
side-effect, this might require being dependent on the loopback module.

Glenn

> 
> >  };
> >  typedef struct grub_cryptomount_args *grub_cryptomount_args_t;
> >  
> > diff --git a/include/grub/file.h b/include/grub/file.h
> > index 31567483c..3a3c49a04 100644
> > --- a/include/grub/file.h
> > +++ b/include/grub/file.h
> > @@ -90,6 +90,8 @@ enum grub_file_type
> >      GRUB_FILE_TYPE_FONT,
> >      /* File holding encryption key for encrypted ZFS.  */
> >      GRUB_FILE_TYPE_ZFS_ENCRYPTION_KEY,
> > +    /* File holding the encryption metadata header */
> > +    GRUB_FILE_TYPE_CRYPTODISK_DETACHED_HEADER,
> >      /* File we open n grub-fstest.  */
> >      GRUB_FILE_TYPE_FSTEST,
> >      /* File we open n grub-mount.  */
> 
> Dmitry