[PATCH v8 0/7] Cryptodisk detached headers and key files

[PATCH v8 0/7] Cryptodisk detached headers and key files

[Glenn Washburn]

This patch series is an updated version of the v7 sent by Denis Carikli with
modifications to reflect changes in argument passing to crypto backends. The
previous patch #6 titled "Add support for LUKS1 key files" has been removed
as its not needed anymore. Patches #6 and #7 are new, for updating the
cryptomount help string and adding support for detached headers in the LUKS2
crypto backend, respectively.

I modified the commit tags from v7 as seemed appropriate to me, but they may
not be desirable as-is.

Glenn

Denis 'GNUtoo' Carikli (2):
  cryptodisk: luks: unify grub_cryptodisk_dev function names
  cryptodisk: geli: unify grub_cryptodisk_dev function names

Glenn Washburn (3):
  cryptodisk: enable the backends to implement detached headers
  cryptodisk: Improve cryptomount short help string
  luks2: Add detached header support

John Lane (2):
  cryptodisk: add support for LUKS1 detached headers
  cryptodisk: enable the backends to implement key files

 grub-core/disk/cryptodisk.c | 100 +++++++++++++++++++++++++++++++++++-
 grub-core/disk/geli.c       |  18 +++++--
 grub-core/disk/luks.c       |  48 +++++++++++++----
 grub-core/disk/luks2.c      |  59 +++++++++++++++++----
 include/grub/cryptodisk.h   |   4 ++
 include/grub/file.h         |   4 ++
 6 files changed, 208 insertions(+), 25 deletions(-)

Range-diff against v7:
1:  2ad229622 ! 1:  e301e06b2 cryptodisk: luks: unify grub_cryptodisk_dev function names
    @@ grub-core/disk/luks.c: gcry_err_code_t AF_merge (const gcry_md_spec_t * hash, gr
      			  grub_size_t blocknumbers);
      
      static grub_cryptodisk_t
    --configure_ciphers (grub_disk_t disk, const char *check_uuid,
    --		   int check_boot)
    -+luks_scan (grub_disk_t disk, const char *check_uuid, int check_boot)
    +-configure_ciphers (grub_disk_t disk, grub_cryptomount_args_t cargs)
    ++luks_scan (grub_disk_t disk, grub_cryptomount_args_t cargs)
      {
        grub_cryptodisk_t newdev;
        const char *iptr;
2:  f5fd41a71 ! 2:  e759d96cd cryptodisk: geli: unify grub_cryptodisk_dev function names
    @@ grub-core/disk/geli.c: grub_util_get_geli_uuid (const char *dev)
      #endif
      
      static grub_cryptodisk_t
    --configure_ciphers (grub_disk_t disk, const char *check_uuid,
    --		   int boot_only)
    -+geli_scan (grub_disk_t disk, const char *check_uuid, int boot_only)
    +-configure_ciphers (grub_disk_t disk, grub_cryptomount_args_t cargs)
    ++geli_scan (grub_disk_t disk, grub_cryptomount_args_t cargs)
      {
        grub_cryptodisk_t newdev;
        struct grub_geli_phdr header;
    -@@ grub-core/disk/geli.c: configure_ciphers (grub_disk_t disk, const char *check_uuid,
    +@@ grub-core/disk/geli.c: configure_ciphers (grub_disk_t disk, grub_cryptomount_args_t cargs)
      }
      
      static grub_err_t
    --recover_key (grub_disk_t source, grub_cryptodisk_t dev)
    -+geli_recover_key (grub_disk_t source, grub_cryptodisk_t dev)
    +-recover_key (grub_disk_t source, grub_cryptodisk_t dev, grub_cryptomount_args_t cargs)
    ++geli_recover_key (grub_disk_t source, grub_cryptodisk_t dev, grub_cryptomount_args_t cargs)
      {
        grub_size_t keysize;
        grub_uint8_t digest[GRUB_CRYPTO_MAX_MDLEN];
    -@@ grub-core/disk/geli.c: recover_key (grub_disk_t source, grub_cryptodisk_t dev)
    +@@ grub-core/disk/geli.c: recover_key (grub_disk_t source, grub_cryptodisk_t dev, grub_cryptomount_args_t
      }
      
      struct grub_cryptodisk_dev geli_crypto = {
3:  365839627 < -:  --------- cryptodisk: enable the backends to implement detached headers
-:  --------- > 3:  ee04480ba cryptodisk: enable the backends to implement detached headers
4:  1e1257bb6 ! 4:  69684640b cryptodisk: add support for LUKS1 detached headers
    @@ Commit message
         Signed-off-by: John Lane <john@lane.uk.net>
         GNUtoo@cyberdimension.org: rebase, small fixes, commit message
         Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
    -    Reviewed-by: Patrick Steinhardt <ps@pks.im>
    +    development@efficientek.com: rebase
     
      ## grub-core/disk/luks.c ##
     @@
    @@ grub-core/disk/luks.c
      #include <grub/crypto.h>
      #include <grub/partition.h>
      #include <grub/i18n.h>
    -@@ grub-core/disk/luks.c: luks_scan (grub_disk_t disk, const char *check_uuid, int check_boot,
    +@@ grub-core/disk/luks.c: luks_scan (grub_disk_t disk, grub_cryptomount_args_t cargs)
        char ciphername[sizeof (header.cipherName) + 1];
        char ciphermode[sizeof (header.cipherMode) + 1];
        char hashspec[sizeof (header.hashSpec) + 1];
     -  grub_err_t err;
     -
     -  /* Detached headers are not implemented yet */
    --  if (hdr)
    +-  if (cargs->hdr_file)
     -    return NULL;
     +  grub_err_t err = GRUB_ERR_NONE;
      
    -   if (check_boot)
    +   if (cargs->check_boot)
          return NULL;
      
        /* Read the LUKS header.  */
     -  err = grub_disk_read (disk, 0, 0, sizeof (header), &header);
    -+  if (hdr)
    ++  if (cargs->hdr_file)
     +    {
    -+      if (grub_file_seek (hdr, 0) == (grub_off_t) -1)
    ++      if (grub_file_seek (cargs->hdr_file, 0) == (grub_off_t) -1)
     +	return NULL;
     +
    -+      if (grub_file_read (hdr, &header, sizeof (header)) != sizeof (header))
    ++      if (grub_file_read (cargs->hdr_file, &header, sizeof (header)) != sizeof (header))
     +	return NULL;
     +    }
     +  else
    @@ grub-core/disk/luks.c: luks_scan (grub_disk_t disk, const char *check_uuid, int
        if (err)
          {
            if (err == GRUB_ERR_OUT_OF_RANGE)
    -@@ grub-core/disk/luks.c: luks_recover_key (grub_disk_t source, grub_cryptodisk_t dev, grub_file_t hdr)
    +@@ grub-core/disk/luks.c: luks_recover_key (grub_disk_t source,
        grub_uint8_t candidate_digest[sizeof (header.mkDigest)];
        unsigned i;
        grub_size_t length;
     -  grub_err_t err;
     +  grub_err_t err = GRUB_ERR_NONE;
        grub_size_t max_stripes = 1;
    -   char *tmp;
     +  grub_uint32_t sector;
      
    +   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 (hdr)
    --    return GRUB_ERR_NOT_IMPLEMENTED_YET;
    +   if (cargs->hdr_file)
    +-     return GRUB_ERR_NOT_IMPLEMENTED_YET;
     +    {
    -+      if (grub_file_seek (hdr, 0) == (grub_off_t) -1)
    ++      if (grub_file_seek (cargs->hdr_file, 0) == (grub_off_t) -1)
     +	return grub_errno;
     +
    -+      if (grub_file_read (hdr, &header, sizeof (header)) != sizeof (header))
    ++      if (grub_file_read (cargs->hdr_file, &header, sizeof (header)) != sizeof (header))
     +	return grub_errno;
     +    }
     +  else
    @@ grub-core/disk/luks.c: luks_recover_key (grub_disk_t source, grub_cryptodisk_t d
        if (err)
          return err;
      
    -@@ grub-core/disk/luks.c: luks_recover_key (grub_disk_t source, grub_cryptodisk_t dev, grub_file_t hdr)
    +@@ grub-core/disk/luks.c: luks_recover_key (grub_disk_t source,
      	  return grub_crypto_gcry_error (gcry_err);
      	}
      
    @@ grub-core/disk/luks.c: luks_recover_key (grub_disk_t source, grub_cryptodisk_t d
     -			    grub_be_to_cpu32 (header.keyblock
     -					      [i].keyMaterialOffset), 0,
     -			    length, split_key);
    -+      if (hdr)
    ++      if (cargs->hdr_file)
     +      {
    -+        if (grub_file_seek (hdr, sector * 512) == (grub_off_t) -1)
    ++        if (grub_file_seek (cargs->hdr_file, sector * 512) == (grub_off_t) -1)
     +          return grub_errno;
    -+        if (grub_file_read (hdr, split_key, length) != (grub_ssize_t)length)
    ++        if (grub_file_read (cargs->hdr_file, split_key, length) != (grub_ssize_t)length)
     +          return grub_errno;
     +      }
     +      else
5:  a8b8c3f45 < -:  --------- cryptodisk: enable the backends to implement key files
6:  91a3795cc < -:  --------- cryptodisk: Add support for LUKS1 key files
-:  --------- > 5:  ded97bfa3 cryptodisk: enable the backends to implement key files
-:  --------- > 6:  62f04499c cryptodisk: Improve cryptomount short help string
-:  --------- > 7:  117658d72 luks2: Add detached header support
-- 
2.27.0

[PATCH v8 1/7] cryptodisk: luks: unify grub_cryptodisk_dev function names

[Glenn Washburn]

From: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>

Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
Reviewed-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/disk/luks.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
index f0feb3844..d57257b3e 100644
--- a/grub-core/disk/luks.c
+++ b/grub-core/disk/luks.c
@@ -63,7 +63,7 @@ gcry_err_code_t AF_merge (const gcry_md_spec_t * hash, grub_uint8_t * src,
 			  grub_size_t blocknumbers);
 
 static grub_cryptodisk_t
-configure_ciphers (grub_disk_t disk, grub_cryptomount_args_t cargs)
+luks_scan (grub_disk_t disk, grub_cryptomount_args_t cargs)
 {
   grub_cryptodisk_t newdev;
   const char *iptr;
@@ -297,7 +297,7 @@ luks_recover_key (grub_disk_t source,
 }
 
 struct grub_cryptodisk_dev luks_crypto = {
-  .scan = configure_ciphers,
+  .scan = luks_scan,
   .recover_key = luks_recover_key
 };
 
-- 
2.27.0

[PATCH v8 2/7] cryptodisk: geli: unify grub_cryptodisk_dev function names

[Glenn Washburn]

From: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>

Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
Reviewed-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/disk/geli.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/grub-core/disk/geli.c b/grub-core/disk/geli.c
index 23789c43f..5b3a11881 100644
--- a/grub-core/disk/geli.c
+++ b/grub-core/disk/geli.c
@@ -240,7 +240,7 @@ grub_util_get_geli_uuid (const char *dev)
 #endif
 
 static grub_cryptodisk_t
-configure_ciphers (grub_disk_t disk, grub_cryptomount_args_t cargs)
+geli_scan (grub_disk_t disk, grub_cryptomount_args_t cargs)
 {
   grub_cryptodisk_t newdev;
   struct grub_geli_phdr header;
@@ -395,7 +395,7 @@ configure_ciphers (grub_disk_t disk, grub_cryptomount_args_t cargs)
 }
 
 static grub_err_t
-recover_key (grub_disk_t source, grub_cryptodisk_t dev, grub_cryptomount_args_t cargs)
+geli_recover_key (grub_disk_t source, grub_cryptodisk_t dev, grub_cryptomount_args_t cargs)
 {
   grub_size_t keysize;
   grub_uint8_t digest[GRUB_CRYPTO_MAX_MDLEN];
@@ -567,8 +567,8 @@ recover_key (grub_disk_t source, grub_cryptodisk_t dev, grub_cryptomount_args_t
 }
 
 struct grub_cryptodisk_dev geli_crypto = {
-  .scan = configure_ciphers,
-  .recover_key = recover_key
+  .scan = geli_scan,
+  .recover_key = geli_recover_key
 };
 
 GRUB_MOD_INIT (geli)
-- 
2.27.0

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

[Glenn Washburn]

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;
 };
 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.  */
-- 
2.27.0

[PATCH v8 4/7] cryptodisk: add support for LUKS1 detached headers

[Glenn Washburn]

From: John Lane <john@lane.uk.net>

cryptsetup supports having a detached header through the
--header command line argument for both LUKS1 and LUKS2.

This adds support for LUKS1 detached headers.

Signed-off-by: John Lane <john@lane.uk.net>
GNUtoo@cyberdimension.org: rebase, small fixes, commit message
Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
development@efficientek.com: rebase
Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/disk/luks.c | 48 ++++++++++++++++++++++++++++++-------------
 1 file changed, 34 insertions(+), 14 deletions(-)

diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
index 032a9db3c..5a9157608 100644
--- a/grub-core/disk/luks.c
+++ b/grub-core/disk/luks.c
@@ -23,6 +23,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>
@@ -73,17 +74,23 @@ luks_scan (grub_disk_t disk, grub_cryptomount_args_t cargs)
   char ciphername[sizeof (header.cipherName) + 1];
   char ciphermode[sizeof (header.cipherMode) + 1];
   char hashspec[sizeof (header.hashSpec) + 1];
-  grub_err_t err;
-
-  /* Detached headers are not implemented yet */
-  if (cargs->hdr_file)
-    return NULL;
+  grub_err_t err = GRUB_ERR_NONE;
 
   if (cargs->check_boot)
     return NULL;
 
   /* Read the LUKS header.  */
-  err = grub_disk_read (disk, 0, 0, sizeof (header), &header);
+  if (cargs->hdr_file)
+    {
+      if (grub_file_seek (cargs->hdr_file, 0) == (grub_off_t) -1)
+	return NULL;
+
+      if (grub_file_read (cargs->hdr_file, &header, sizeof (header)) != sizeof (header))
+	return NULL;
+    }
+  else
+    err = grub_disk_read (disk, 0, 0, sizeof (header), &header);
+
   if (err)
     {
       if (err == GRUB_ERR_OUT_OF_RANGE)
@@ -162,17 +169,24 @@ luks_recover_key (grub_disk_t source,
   grub_uint8_t candidate_digest[sizeof (header.mkDigest)];
   unsigned i;
   grub_size_t length;
-  grub_err_t err;
+  grub_err_t err = GRUB_ERR_NONE;
   grub_size_t max_stripes = 1;
+  grub_uint32_t sector;
 
   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 (grub_file_seek (cargs->hdr_file, 0) == (grub_off_t) -1)
+	return grub_errno;
+
+      if (grub_file_read (cargs->hdr_file, &header, sizeof (header)) != sizeof (header))
+	return grub_errno;
+    }
+  else
+    err = grub_disk_read (source, 0, 0, sizeof (header), &header);
 
-  err = grub_disk_read (source, 0, 0, sizeof (header), &header);
   if (err)
     return err;
 
@@ -227,13 +241,19 @@ luks_recover_key (grub_disk_t source,
 	  return grub_crypto_gcry_error (gcry_err);
 	}
 
+      sector = grub_be_to_cpu32 (header.keyblock[i].keyMaterialOffset);
       length = (keysize * grub_be_to_cpu32 (header.keyblock[i].stripes));
 
       /* Read and decrypt the key material from the disk.  */
-      err = grub_disk_read (source,
-			    grub_be_to_cpu32 (header.keyblock
-					      [i].keyMaterialOffset), 0,
-			    length, split_key);
+      if (cargs->hdr_file)
+      {
+        if (grub_file_seek (cargs->hdr_file, sector * 512) == (grub_off_t) -1)
+          return grub_errno;
+        if (grub_file_read (cargs->hdr_file, split_key, length) != (grub_ssize_t)length)
+          return grub_errno;
+      }
+      else
+        err = grub_disk_read (source, sector, 0, length, split_key);
       if (err)
 	{
 	  grub_free (split_key);
-- 
2.27.0

[PATCH v8 5/7] cryptodisk: enable the backends to implement key files

[Glenn Washburn]

From: John Lane <john@lane.uk.net>

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 and rework to use cryptomount arg passing
Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/disk/cryptodisk.c | 83 +++++++++++++++++++++++++++++++++++++
 include/grub/cryptodisk.h   |  2 +
 include/grub/file.h         |  2 +
 3 files changed, 87 insertions(+)

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index e90f680f0..ea8ed20e2 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -43,6 +43,9 @@ static const struct grub_arg_option options[] =
     {"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},
+    {"keyfile", 'k', 0, N_("Key file"), 0, ARG_TYPE_STRING},
+    {"keyfile-offset", 'O', 0, N_("Key file offset (bytes)"), 0, ARG_TYPE_INT},
+    {"keyfile-size", 'S', 0, N_("Key file data size (bytes)"), 0, ARG_TYPE_INT},
     {0, 0, 0, 0, 0, 0}
   };
 
@@ -1186,6 +1189,86 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
 	return grub_errno;
     }
 
+  if (state[5].set) /* keyfile */
+    {
+      const char *p = NULL;
+      grub_file_t keyfile;
+      int keyfile_offset;
+      grub_size_t requested_keyfile_size = 0;
+
+
+      if (state[6].set) /* keyfile-offset */
+	{
+	  keyfile_offset = grub_strtoul (state[6].arg, &p, 0);
+
+	  if (grub_errno != GRUB_ERR_NONE)
+	    return grub_errno;
+
+	  if (*p != '\0')
+	    return grub_error (GRUB_ERR_BAD_ARGUMENT,
+			       N_("unrecognized number"));
+	}
+      else
+	{
+	  keyfile_offset = 0;
+	}
+
+      if (state[7].set) /* keyfile-size */
+	{
+	  requested_keyfile_size = grub_strtoul (state[7].arg, &p, 0);
+
+	  if (*p != '\0')
+	    return grub_error (GRUB_ERR_BAD_ARGUMENT,
+			       N_("unrecognized number"));
+
+	  if (grub_errno != GRUB_ERR_NONE)
+	    return grub_errno;
+
+	  if (requested_keyfile_size > GRUB_CRYPTODISK_MAX_KEYFILE_SIZE)
+	    return grub_error (GRUB_ERR_OUT_OF_RANGE,
+			      N_("Key file size exceeds maximum (%d)\n"),
+			      GRUB_CRYPTODISK_MAX_KEYFILE_SIZE);
+
+	  if (requested_keyfile_size == 0)
+	    return grub_error (GRUB_ERR_OUT_OF_RANGE,
+			      N_("Key file size is 0\n"));
+	}
+
+      keyfile = grub_file_open (state[5].arg,
+				GRUB_FILE_TYPE_CRYPTODISK_ENCRYPTION_KEY);
+      if (!keyfile)
+	return grub_errno;
+
+      if (grub_file_seek (keyfile, keyfile_offset) == (grub_off_t)-1)
+	return grub_errno;
+
+      if (requested_keyfile_size)
+	{
+	  if (requested_keyfile_size > (keyfile->size - keyfile_offset))
+	    return grub_error (GRUB_ERR_FILE_READ_ERROR,
+			       N_("Keyfile is too small: "
+				  "requested %" PRIuGRUB_SIZE " bytes, "
+				  "but the file only has %" PRIuGRUB_UINT64_T
+				  " bytes.\n"),
+			       requested_keyfile_size,
+			       keyfile->size);
+
+	  cargs.key_len = requested_keyfile_size;
+	}
+      else
+	{
+	  cargs.key_len = keyfile->size - keyfile_offset;
+	}
+
+      cargs.key_data = grub_malloc (cargs.key_len);
+      if (!cargs.key_data)
+	return GRUB_ERR_OUT_OF_MEMORY;
+
+      if (grub_file_read (keyfile, cargs.key_data, cargs.key_len) != (grub_ssize_t) cargs.key_len)
+	return grub_error (GRUB_ERR_FILE_READ_ERROR,
+			   (N_("Error reading key file\n")));
+    }
+
   if (state[0].set) /* uuid */
     {
       int found_uuid;
diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
index 9fe451de9..d94df68b6 100644
--- a/include/grub/cryptodisk.h
+++ b/include/grub/cryptodisk.h
@@ -62,6 +62,8 @@ typedef enum
 #define GRUB_CRYPTODISK_MAX_KEYLEN 128
 #define GRUB_CRYPTODISK_MAX_PASSPHRASE 256
 
+#define GRUB_CRYPTODISK_MAX_KEYFILE_SIZE 8192
+
 struct grub_cryptodisk;
 
 typedef gcry_err_code_t
diff --git a/include/grub/file.h b/include/grub/file.h
index 3a3c49a04..2d5d16cd2 100644
--- a/include/grub/file.h
+++ b/include/grub/file.h
@@ -92,6 +92,8 @@ enum grub_file_type
     GRUB_FILE_TYPE_ZFS_ENCRYPTION_KEY,
     /* File holding the encryption metadata header */
     GRUB_FILE_TYPE_CRYPTODISK_DETACHED_HEADER,
+    /* File holding the encryption key */
+    GRUB_FILE_TYPE_CRYPTODISK_ENCRYPTION_KEY,
     /* File we open n grub-fstest.  */
     GRUB_FILE_TYPE_FSTEST,
     /* File we open n grub-mount.  */
-- 
2.27.0

[PATCH v8 6/7] cryptodisk: Improve cryptomount short help string

[Glenn Washburn]

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/disk/cryptodisk.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index ea8ed20e2..319c84a6c 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -1481,7 +1481,9 @@ GRUB_MOD_INIT (cryptodisk)
 {
   grub_disk_dev_register (&grub_cryptodisk_dev);
   cmd = grub_register_extcmd ("cryptomount", grub_cmd_cryptomount, 0,
-			      N_("[-p password] [-H file] <SOURCE|-u UUID|-a|-b>"),
+			      N_("[-p password] [-k keyfile] [-O keyoffset]"
+				 " [-S keysize] [-H file]"
+				 " <SOURCE|-u UUID|-a|-b>"),
 			      N_("Mount a crypto device."), options);
   grub_procfs_register ("luks_script", &luks_script);
 }
-- 
2.27.0

[PATCH v8 7/7] luks2: Add detached header support

[Glenn Washburn]

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/disk/luks2.c | 67 ++++++++++++++++++++++++++++++------------
 1 file changed, 49 insertions(+), 18 deletions(-)

diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
index 567368f11..e92c28d45 100644
--- a/grub-core/disk/luks2.c
+++ b/grub-core/disk/luks2.c
@@ -313,13 +313,22 @@ luks2_get_keyslot (grub_luks2_keyslot_t *k, grub_luks2_digest_t *d, grub_luks2_s
 
 /* Determine whether to use primary or secondary header */
 static grub_err_t
-luks2_read_header (grub_disk_t disk, grub_luks2_header_t *outhdr)
+luks2_read_header (grub_disk_t disk, grub_file_t hdr_file, grub_luks2_header_t *outhdr)
 {
   grub_luks2_header_t primary, secondary, *header = &primary;
-  grub_err_t ret;
+  grub_err_t ret = GRUB_ERR_NONE;
 
   /* Read the primary LUKS header. */
-  ret = grub_disk_read (disk, 0, 0, sizeof (primary), &primary);
+  if (hdr_file)
+    {
+      if (grub_file_seek (hdr_file, 0) == (grub_off_t) -1)
+	ret = grub_errno;
+
+      else if (grub_file_read (hdr_file, &primary, sizeof (primary)) != sizeof (primary))
+	ret = grub_errno;
+    }
+  else
+    ret = grub_disk_read (disk, 0, 0, sizeof (primary), &primary);
   if (ret)
     return ret;
 
@@ -329,7 +338,16 @@ luks2_read_header (grub_disk_t disk, grub_luks2_header_t *outhdr)
     return GRUB_ERR_BAD_SIGNATURE;
 
   /* Read the secondary header. */
-  ret = grub_disk_read (disk, 0, grub_be_to_cpu64 (primary.hdr_size), sizeof (secondary), &secondary);
+  if (hdr_file)
+    {
+      if (grub_file_seek (hdr_file, grub_be_to_cpu64 (primary.hdr_size)) == (grub_off_t) -1)
+	ret = grub_errno;
+
+      else if (grub_file_read (hdr_file, &secondary, sizeof (secondary)) != sizeof (secondary))
+	ret = grub_errno;
+    }
+  else
+    ret = grub_disk_read (disk, 0, grub_be_to_cpu64 (primary.hdr_size), sizeof (secondary), &secondary);
   if (ret)
     return ret;
 
@@ -353,14 +371,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;
 
-  if (luks2_read_header (disk, &header))
+  if (luks2_read_header (disk, cargs->hdr_file, &header))
     {
       grub_errno = GRUB_ERR_NONE;
       return NULL;
@@ -427,6 +441,7 @@ luks2_verify_key (grub_luks2_digest_t *d, grub_uint8_t *candidate_key,
 static grub_err_t
 luks2_decrypt_key (grub_uint8_t *out_key,
 		   grub_disk_t source, grub_cryptodisk_t crypt,
+		   grub_cryptomount_args_t cargs,
 		   grub_luks2_keyslot_t *k,
 		   const grub_uint8_t *passphrase, grub_size_t passphraselen)
 {
@@ -502,7 +517,17 @@ luks2_decrypt_key (grub_uint8_t *out_key,
     }
 
   grub_errno = GRUB_ERR_NONE;
-  ret = grub_disk_read (source, 0, k->area.offset, k->area.size, split_key);
+  if (cargs->hdr_file)
+    {
+      if (grub_file_seek (cargs->hdr_file, k->area.offset) == (grub_off_t) -1)
+	ret = grub_errno;
+
+      else if (grub_file_read (cargs->hdr_file, split_key, k->area.size) != k->area.size)
+	ret = grub_errno;
+    }
+  else
+    ret = grub_disk_read (source, 0, k->area.offset, k->area.size, split_key);
+
   if (ret)
     {
       grub_error (GRUB_ERR_IO, "Read error: %s\n", grub_errmsg);
@@ -564,11 +589,7 @@ 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);
+  ret = luks2_read_header (source, cargs->hdr_file, &header);
   if (ret)
     return ret;
 
@@ -577,8 +598,18 @@ luks2_recover_key (grub_disk_t source,
       return GRUB_ERR_OUT_OF_MEMORY;
 
   /* Read the JSON area. */
-  ret = grub_disk_read (source, 0, grub_be_to_cpu64 (header.hdr_offset) + sizeof (header),
-			grub_be_to_cpu64 (header.hdr_size) - sizeof (header), json_header);
+  if (cargs->hdr_file)
+    {
+      if (grub_file_seek (cargs->hdr_file, grub_be_to_cpu64 (header.hdr_offset) + sizeof (header)) == (grub_off_t) -1)
+	ret = grub_errno;
+
+      else if (grub_file_read (cargs->hdr_file, json_header, grub_be_to_cpu64 (header.hdr_size) - sizeof (header)) != (grub_be_to_cpu64 (header.hdr_size) - sizeof (header)))
+	ret = grub_errno;
+    }
+  else
+    ret = grub_disk_read (source, 0, grub_be_to_cpu64 (header.hdr_offset) + sizeof (header),
+			  grub_be_to_cpu64 (header.hdr_size) - sizeof (header), json_header);
+
   if (ret)
       goto err;
 
@@ -716,7 +747,7 @@ luks2_recover_key (grub_disk_t source,
 	  crypt->total_sectors = max_crypt_sectors - crypt->offset_sectors;
 	}
 
-      ret = luks2_decrypt_key (candidate_key, source, crypt, &keyslot,
+      ret = luks2_decrypt_key (candidate_key, source, crypt, cargs, &keyslot,
 			       cargs->key_data, cargs->key_len);
       if (ret)
 	{
-- 
2.27.0

[PATCH v8 0/7] Cryptodisk detached headers and key files

[Maxim Fomin]

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐

On Sunday, January 2nd, 2022 at 3:52 AM, Glenn Washburn <development@efficientek.com> wrote:

> This patch series is an updated version of the v7 sent by Denis Carikli with
>
> modifications to reflect changes in argument passing to crypto backends. The
>
> previous patch #6 titled "Add support for LUKS1 key files" has been removed
>
> as its not needed anymore. Patches #6 and #7 are new, for updating the
>
> cryptomount help string and adding support for detached headers in the LUKS2
>
> crypto backend, respectively.
>
> I modified the commit tags from v7 as seemed appropriate to me, but they may
>
> not be desirable as-is.
>
> Glenn
>

Thanks for updating these patches.
I remember several attempts to introduce detached headers/keyfile in recent years and didn't believe it was going to happen anytime soon. Hope these patches will be merged in the next grub release.

Best regards,
Maxim Fomin

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

[Glenn Washburn]

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"

>  };
>  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

Re: [PATCH v8 5/7] cryptodisk: enable the backends to implement key files

[Glenn Washburn]

Also from Dmitry.

On Tue, 4 Jan 2022 21:25:14 +0300
Dmitry <reagentoo@gmail.com> wrote:

> From: John Lane <john@lane.uk.net>
> 
> 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 and rework to use cryptomount arg passing
> Signed-off-by: Glenn Washburn <development@efficientek.com>
> ---
>  grub-core/disk/cryptodisk.c | 83 +++++++++++++++++++++++++++++++++++++
>  include/grub/cryptodisk.h   |  2 +
>  include/grub/file.h         |  2 +
>  3 files changed, 87 insertions(+)
> 
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index e90f680f0..ea8ed20e2 100644
> --- a/grub-core/disk/cryptodisk.c
> +++ b/grub-core/disk/cryptodisk.c
> @@ -43,6 +43,9 @@ static const struct grub_arg_option options[] =
>      {"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},
> +    {"keyfile", 'k', 0, N_("Key file"), 0, ARG_TYPE_STRING},

You have custom options --header, --keyfile. I suggest renaming in a
similar 
way as in cryptsetup(8) - --header, --key-file, (--master-key-file)

> +    {"keyfile-offset", 'O', 0, N_("Key file offset (bytes)"), 0, ARG_TYPE_INT},
> +    {"keyfile-size", 'S', 0, N_("Key file data size (bytes)"), 0, ARG_TYPE_INT},
>      {0, 0, 0, 0, 0, 0}
>    };
>  
> @@ -1186,6 +1189,86 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
>  	return grub_errno;
>      }
>  
> +  if (state[5].set) /* keyfile */
> +    {
> +      const char *p = NULL;
> +      grub_file_t keyfile;
> +      int keyfile_offset;
> +      grub_size_t requested_keyfile_size = 0;
> +
> +
> +      if (state[6].set) /* keyfile-offset */
> +	{
> +	  keyfile_offset = grub_strtoul (state[6].arg, &p, 0);
> +
> +	  if (grub_errno != GRUB_ERR_NONE)
> +	    return grub_errno;
> +
> +	  if (*p != '\0')
> +	    return grub_error (GRUB_ERR_BAD_ARGUMENT,
> +			       N_("unrecognized number"));
> +	}
> +      else
> +	{
> +	  keyfile_offset = 0;
> +	}
> +
> +      if (state[7].set) /* keyfile-size */
> +	{
> +	  requested_keyfile_size = grub_strtoul (state[7].arg, &p, 0);
> +
> +	  if (*p != '\0')
> +	    return grub_error (GRUB_ERR_BAD_ARGUMENT,
> +			       N_("unrecognized number"));
> +
> +	  if (grub_errno != GRUB_ERR_NONE)
> +	    return grub_errno;
> +
> +	  if (requested_keyfile_size > GRUB_CRYPTODISK_MAX_KEYFILE_SIZE)
> +	    return grub_error (GRUB_ERR_OUT_OF_RANGE,
> +			      N_("Key file size exceeds maximum (%d)\n"),
> +			      GRUB_CRYPTODISK_MAX_KEYFILE_SIZE);
> +
> +	  if (requested_keyfile_size == 0)
> +	    return grub_error (GRUB_ERR_OUT_OF_RANGE,
> +			      N_("Key file size is 0\n"));
> +	}
> +
> +      keyfile = grub_file_open (state[5].arg,
> +				GRUB_FILE_TYPE_CRYPTODISK_ENCRYPTION_KEY);
> +      if (!keyfile)
> +	return grub_errno;
> +
> +      if (grub_file_seek (keyfile, keyfile_offset) == (grub_off_t)-1)
> +	return grub_errno;
> +
> +      if (requested_keyfile_size)
> +	{
> +	  if (requested_keyfile_size > (keyfile->size - keyfile_offset))
> +	    return grub_error (GRUB_ERR_FILE_READ_ERROR,
> +			       N_("Keyfile is too small: "
> +				  "requested %" PRIuGRUB_SIZE " bytes, "
> +				  "but the file only has %" PRIuGRUB_UINT64_T
> +				  " bytes.\n"),
> +			       requested_keyfile_size,
> +			       keyfile->size);
> +
> +	  cargs.key_len = requested_keyfile_size;
> +	}
> +      else
> +	{
> +	  cargs.key_len = keyfile->size - keyfile_offset;
> +	}
> +
> +      cargs.key_data = grub_malloc (cargs.key_len);
> +      if (!cargs.key_data)
> +	return GRUB_ERR_OUT_OF_MEMORY;
> +
> +      if (grub_file_read (keyfile, cargs.key_data, cargs.key_len) != (grub_ssize_t) cargs.key_len)
> +	return grub_error (GRUB_ERR_FILE_READ_ERROR,
> +			   (N_("Error reading key file\n")));
> +    }
> +
>    if (state[0].set) /* uuid */
>      {
>        int found_uuid;
> diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
> index 9fe451de9..d94df68b6 100644
> --- a/include/grub/cryptodisk.h
> +++ b/include/grub/cryptodisk.h
> @@ -62,6 +62,8 @@ typedef enum
>  #define GRUB_CRYPTODISK_MAX_KEYLEN 128
>  #define GRUB_CRYPTODISK_MAX_PASSPHRASE 256
>  
> +#define GRUB_CRYPTODISK_MAX_KEYFILE_SIZE 8192
> +
>  struct grub_cryptodisk;
>  
>  typedef gcry_err_code_t
> diff --git a/include/grub/file.h b/include/grub/file.h
> index 3a3c49a04..2d5d16cd2 100644
> --- a/include/grub/file.h
> +++ b/include/grub/file.h
> @@ -92,6 +92,8 @@ enum grub_file_type
>      GRUB_FILE_TYPE_ZFS_ENCRYPTION_KEY,
>      /* File holding the encryption metadata header */
>      GRUB_FILE_TYPE_CRYPTODISK_DETACHED_HEADER,
> +    /* File holding the encryption key */
> +    GRUB_FILE_TYPE_CRYPTODISK_ENCRYPTION_KEY,
>      /* File we open n grub-fstest.  */
>      GRUB_FILE_TYPE_FSTEST,
>      /* File we open n grub-mount.  */

Dmitry

Re: [PATCH v8 5/7] cryptodisk: enable the backends to implement key files

[Glenn Washburn]

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

> Also from Dmitry.
> 
> On Tue, 4 Jan 2022 21:25:14 +0300
> Dmitry <reagentoo@gmail.com> wrote:
> 
> > From: John Lane <john@lane.uk.net>
> > 
> > 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 and rework to use cryptomount arg passing
> > Signed-off-by: Glenn Washburn <development@efficientek.com>
> > ---
> >  grub-core/disk/cryptodisk.c | 83 +++++++++++++++++++++++++++++++++++++
> >  include/grub/cryptodisk.h   |  2 +
> >  include/grub/file.h         |  2 +
> >  3 files changed, 87 insertions(+)
> > 
> > diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> > index e90f680f0..ea8ed20e2 100644
> > --- a/grub-core/disk/cryptodisk.c
> > +++ b/grub-core/disk/cryptodisk.c
> > @@ -43,6 +43,9 @@ static const struct grub_arg_option options[] =
> >      {"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},
> > +    {"keyfile", 'k', 0, N_("Key file"), 0, ARG_TYPE_STRING},
> 
> You have custom options --header, --keyfile. I suggest renaming in a
> similar 
> way as in cryptsetup(8) - --header, --key-file, (--master-key-file)

I think sounds reasonable. I'll change this in the next version unless
I hear a good reason not too.

Glenn

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

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

[Dmitry]

[-- Attachment #1: Type: text/plain, Size: 2677 bytes --]

ср, 5 янв. 2022 г. в 01:07, Glenn Washburn <development@efficientek.com>:

> On Tue, 4 Jan 2022 15:42:22 -0600
> Glenn Washburn <development@efficientek.com> wrote:
>
> 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?
>

Actually no, I only use a file for the external header, not a disk.
I have now looked at the patches again and will try to state my point of
view in
more detail:

I don't think the hdr_file field as it stands in the patch set is relevant.
I mean
the hdr_file field of type grub_file_t in the grub_cryptomount_args
structure.
Even the grub_disk_t type may not be relevant here. You could only pass
a header file name or a disk name (as char*) through this structure. This
would
reflect the essence of this structure, but further implementation the code
will
not be pretty in this case.

I still suggest expanding the number of parameters for the recover_key
function
and use grub_disk_t to pass the header from the user directly.

I'll leave it here for your reference:
https://gitlab.com/reagentoo/grub/-/blob/0921badcf817071639058bc4a534c8e6bd05f212/grub-core/disk/luks2.c#L545

header_source is equal source - if we are not using external header


>
> 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
>

[-- Attachment #2: Type: text/html, Size: 3662 bytes --]

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

[Dmitry]

[-- Attachment #1: Type: text/plain, Size: 1698 bytes --]

ср, 5 янв. 2022 г. в 01:57, Dmitry <reagentoo@gmail.com>:

>
>
> ср, 5 янв. 2022 г. в 01:07, Glenn Washburn <development@efficientek.com>:
>
>> On Tue, 4 Jan 2022 15:42:22 -0600
>> Glenn Washburn <development@efficientek.com> wrote:
>>
>> 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?
>>
>
> Actually no, I only use a file for the external header, not a disk.
> I have now looked at the patches again and will try to state my point of
> view in
> more detail:
>
> I don't think the hdr_file field as it stands in the patch set is
> relevant. I mean
> the hdr_file field of type grub_file_t in the grub_cryptomount_args
> structure.
> Even the grub_disk_t type may not be relevant here. You could only pass
> a header file name or a disk name (as char*) through this structure. This
> would
> reflect the essence of this structure, but further implementation the code
> will
> not be pretty in this case.
>
> I still suggest expanding the number of parameters for the recover_key
> function
> and use grub_disk_t to pass the header from the user directly.
>

Although in general I'm quite satisfied with the current patch set. It
suits my
requirements. Maybe disk may be really useless and I overdid it.. It will
only
remain to add the master key parameter in the future.

[-- Attachment #2: Type: text/html, Size: 2628 bytes --]

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

[Dmitry]

[-- Attachment #1: Type: text/plain, Size: 1901 bytes --]

ср, 5 янв. 2022 г. в 02:30, Dmitry <reagentoo@gmail.com>:

>
>
> ср, 5 янв. 2022 г. в 01:57, Dmitry <reagentoo@gmail.com>:
>
>>
>>
>> ср, 5 янв. 2022 г. в 01:07, Glenn Washburn <development@efficientek.com>:
>>
>>> On Tue, 4 Jan 2022 15:42:22 -0600
>>> Glenn Washburn <development@efficientek.com> wrote:
>>>
>>> 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?
>>>
>>
>> Actually no, I only use a file for the external header, not a disk.
>> I have now looked at the patches again and will try to state my point of
>> view in
>> more detail:
>>
>> I don't think the hdr_file field as it stands in the patch set is
>> relevant. I mean
>> the hdr_file field of type grub_file_t in the grub_cryptomount_args
>> structure.
>> Even the grub_disk_t type may not be relevant here. You could only pass
>> a header file name or a disk name (as char*) through this structure. This
>> would
>>
>
So, please ignore these statements. Looks like it's not valid.


> reflect the essence of this structure, but further implementation the code
>> will
>> not be pretty in this case.
>>
>> I still suggest expanding the number of parameters for the recover_key
>> function
>> and use grub_disk_t to pass the header from the user directly.
>>
>
> Although in general I'm quite satisfied with the current patch set. It
> suits my
> requirements. Maybe disk may be really useless and I overdid it.. It will
> only
> remain to add the master key parameter in the future.
>
>

[-- Attachment #2: Type: text/html, Size: 3603 bytes --]

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

[Glenn Washburn]

On Wed, 5 Jan 2022 02:50:36 +0300
Dmitry <reagentoo@gmail.com> wrote:

> ср, 5 янв. 2022 г. в 02:30, Dmitry <reagentoo@gmail.com>:
> 
> >
> >
> > ср, 5 янв. 2022 г. в 01:57, Dmitry <reagentoo@gmail.com>:
> >
> >>
> >>
> >> ср, 5 янв. 2022 г. в 01:07, Glenn Washburn <development@efficientek.com>:
> >>
> >>> On Tue, 4 Jan 2022 15:42:22 -0600
> >>> Glenn Washburn <development@efficientek.com> wrote:
> >>>
> >>> 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?
> >>>
> >>
> >> Actually no, I only use a file for the external header, not a disk.
> >> I have now looked at the patches again and will try to state my point of
> >> view in
> >> more detail:
> >>
> >> I don't think the hdr_file field as it stands in the patch set is
> >> relevant. I mean
> >> the hdr_file field of type grub_file_t in the grub_cryptomount_args
> >> structure.
> >> Even the grub_disk_t type may not be relevant here. You could only pass
> >> a header file name or a disk name (as char*) through this structure. This
> >> would
> >>
> >
> So, please ignore these statements. Looks like it's not valid.

I still like the idea of not having to conditionally choose to use the
disk vs. file api for reading the header. I think it would be nice for
the -H argument to be either a file or a device. It seems to me the most
logical place for this to be handled is in the cryptomount arg
handling. If a file is passed, setup a loopback device and pass that as
the header, otherwise if its a device, just open it and pass it along.
This would make cryptodisk module dependent on the loopback module,
which I don't particularly like and may not be acceptable to others.
Dnaiel do you have an opinion about this?

Also, I've looked over the code again and I don't think the benefit of
having detached headers be disk internally is that great at this point
either. If more cryptodisk backends get added that support detached
headers, then the benefit will increase.

I definitely don't want to require that a disk device be passed in as
the detached header argument to cryptomount. So I think the current
approach is acceptable and further down the road someone can propose a
patch to go in the direction you suggest. If you have an alternative
proposal that I'm not thinking of, I'm more than willing to hear it out
and modify these patches if it sounds good.

Glenn

> > reflect the essence of this structure, but further implementation the code
> >> will
> >> not be pretty in this case.
> >>
> >> I still suggest expanding the number of parameters for the recover_key
> >> function
> >> and use grub_disk_t to pass the header from the user directly.
> >>
> >
> > Although in general I'm quite satisfied with the current patch set. It
> > suits my
> > requirements. Maybe disk may be really useless and I overdid it.. It will
> > only
> > remain to add the master key parameter in the future.
> >
> >

Re: [PATCH v8 1/7] cryptodisk: luks: unify grub_cryptodisk_dev function names

[Daniel Kiper]

On Sat, Jan 01, 2022 at 09:52:54PM -0600, Glenn Washburn wrote:
> From: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
>
> Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
> Reviewed-by: Patrick Steinhardt <ps@pks.im>
> Signed-off-by: Glenn Washburn <development@efficientek.com>

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Daniel

Re: [PATCH v8 2/7] cryptodisk: geli: unify grub_cryptodisk_dev function names

[Daniel Kiper]

On Sat, Jan 01, 2022 at 09:52:55PM -0600, Glenn Washburn wrote:
> From: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
>
> Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
> Reviewed-by: Patrick Steinhardt <ps@pks.im>
> Signed-off-by: Glenn Washburn <development@efficientek.com>

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Daniel

Re: [PATCH v8 0/7] Cryptodisk detached headers and key files

[Daniel Kiper]

On Sat, Jan 01, 2022 at 09:52:53PM -0600, Glenn Washburn wrote:
> This patch series is an updated version of the v7 sent by Denis Carikli with
> modifications to reflect changes in argument passing to crypto backends. The
> previous patch #6 titled "Add support for LUKS1 key files" has been removed
> as its not needed anymore. Patches #6 and #7 are new, for updating the
> cryptomount help string and adding support for detached headers in the LUKS2
> crypto backend, respectively.
>
> I modified the commit tags from v7 as seemed appropriate to me, but they may
> not be desirable as-is.
>
> Glenn
>
> Denis 'GNUtoo' Carikli (2):
>   cryptodisk: luks: unify grub_cryptodisk_dev function names
>   cryptodisk: geli: unify grub_cryptodisk_dev function names
>
> Glenn Washburn (3):
>   cryptodisk: enable the backends to implement detached headers
>   cryptodisk: Improve cryptomount short help string
>   luks2: Add detached header support
>
> John Lane (2):
>   cryptodisk: add support for LUKS1 detached headers
>   cryptodisk: enable the backends to implement key files

I think most of the patches if not all require shorter or longer
explanation why they are needed, what they are doing, etc. One liner
subject is not enough for this patch series. Additionally, there are
missing additions to the documentation. Please correct that and I will
continue reviewing the series.

Daniel