From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1n4rxY-0005b3-NW for mharc-grub-devel@gnu.org; Tue, 04 Jan 2022 17:07:12 -0500 Received: from eggs.gnu.org ([209.51.188.92]:36386) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1n4rxX-0005au-37 for grub-devel@gnu.org; Tue, 04 Jan 2022 17:07:11 -0500 Received: from [2607:f8b0:4864:20::72c] (port=35497 helo=mail-qk1-x72c.google.com) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1n4rxR-00008l-VE for grub-devel@gnu.org; Tue, 04 Jan 2022 17:07:10 -0500 Received: by mail-qk1-x72c.google.com with SMTP id 131so36356154qkk.2 for ; Tue, 04 Jan 2022 14:07:05 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=efficientek-com.20210112.gappssmtp.com; s=20210112; h=date:from:to:cc:subject:message-id:in-reply-to:references:reply-to :mime-version:content-transfer-encoding; bh=5j7lq6TQtlPqEupXZ+7TtjgAbe5OdNg97Tmh+o18diM=; b=dB+dkynZjlG5h+GqJf2dCoke0lmUardga50Mi57esb/jLJdO+Aka6rRAuJz91GQVVR jLGvqqkdUkg7KYkNHtGj+aypf3hAHNmSwIVQYysK1271i4coFf42H5YOXNqIOOh+0/mQ yiWuxRI1lAPJtXKK6Cps0/tD42fP9g/Nvuie9LYMjxeN8zoZJbA13hThApeMB8f9LOTs uJUasQ6gWLQI80Qate4oC87eeZuwUyz9MM1pJ+QaGg7ejV6PJkictUMucCnpUOQ23nGr aMkxYxew47LrJkSnGf0mRT9BciBF74Q4qoveEphOMBYR8ysQLlmAwaYWRnT07QcCGVaG 5m5w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:in-reply-to :references:reply-to:mime-version:content-transfer-encoding; bh=5j7lq6TQtlPqEupXZ+7TtjgAbe5OdNg97Tmh+o18diM=; b=yNNeOKn3Uy2F8RxW++gYd35V9uLMIxbn6XPLKcjmAAGkC+7yGafqyV1BAKFxq0Bk+q 5PSd0sslkrPGXpNxgCMM67QFNwep2RBDnv7t+oak7BGaLZLWq4nIBJdWyG2k1aNl8f4Z vPTW0YevpctgvkZsH3adPV0TxF+LjhKIabQYSWV0lO+SlWUrmf30TtBd1aVZV+K3orZm T9dL8utsbMVuGF+pQUuOKUfUcp16Kos81+BBsZ6LVIaWt+y75kPY8sKINXL4XTPTtAbc dcKz/eA0Mh9wAMfTUiQDIYBAcHvhTq2MK89u3mQxnu0CFy9YFD78aXbexIUoKHwvvnxj 1vmA== X-Gm-Message-State: AOAM532L1Ql47fOaLHaUQfApNyeWyoz4bfdRcLoVGwZq+JHakcCdlC/I btqGGsJt42xgNRjCo6m+55uMTQ== X-Google-Smtp-Source: ABdhPJzB9B60B1rf446vuWXTrpVqaLaDUNdiTQ+bfitVpvJ/A+YzB83xTJ3X91E3aVWgMMkOTA6qMg== X-Received: by 2002:a05:620a:2493:: with SMTP id i19mr36601376qkn.40.1641334024870; Tue, 04 Jan 2022 14:07:04 -0800 (PST) Received: from crass-HP-ZBook-15-G2 ([37.218.244.251]) by smtp.gmail.com with ESMTPSA id u188sm30319412qkh.30.2022.01.04.14.07.03 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 04 Jan 2022 14:07:04 -0800 (PST) Date: Tue, 4 Jan 2022 16:06:56 -0600 From: Glenn Washburn To: Daniel Kiper , grub-devel@gnu.org Cc: Dmitry , Denis 'GNUtoo' Carikli , Patrick Steinhardt , John Lane Subject: Re: [PATCH v8 3/7] cryptodisk: enable the backends to implement detached headers Message-ID: <20220104160656.18fbd917@crass-HP-ZBook-15-G2> In-Reply-To: <20220104154222.142bd77e@crass-HP-ZBook-15-G2> References: <20220104154222.142bd77e@crass-HP-ZBook-15-G2> Reply-To: development@efficientek.com X-Mailer: Claws Mail 3.18.0 (GTK+ 2.24.33; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-Host-Lookup-Failed: Reverse DNS lookup failed for 2607:f8b0:4864:20::72c (failed) Received-SPF: pass client-ip=2607:f8b0:4864:20::72c; envelope-from=development@efficientek.com; helo=mail-qk1-x72c.google.com X-Spam_score_int: 8 X-Spam_score: 0.8 X-Spam_bar: / X-Spam_report: (0.8 / 5.0 requ) DKIM_SIGNED=0.1, DKIM_VALID=-0.1, RCVD_IN_DNSWL_NONE=-0.0001, RDNS_NONE=0.793, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=no autolearn_force=no X-Spam_action: no action X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 04 Jan 2022 22:07:11 -0000 On Tue, 4 Jan 2022 15:42:22 -0600 Glenn Washburn 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 wrote: > > > Signed-off-by: John Lane > > GNUtoo@cyberdimension.org: rebase, patch split, small fixes, commit message > > Signed-off-by: Denis 'GNUtoo' Carikli > > development@efficientek.com: rebase, rework for cryptomount parameter passing > > Signed-off-by: Glenn Washburn > > --- > > 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] "), > > + N_("[-p password] [-H file] "), > > 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 > > #include > > #include > > +#include > > #include > > #include > > #include > > @@ -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 > > +#include > > #include > > #include > > #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