From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1n4tCp-0007x5-41 for mharc-grub-devel@gnu.org; Tue, 04 Jan 2022 18:27:03 -0500 Received: from eggs.gnu.org ([209.51.188.92]:45020) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1n4skB-0004pS-D3 for grub-devel@gnu.org; Tue, 04 Jan 2022 17:57:27 -0500 Received: from [2607:f8b0:4864:20::b32] (port=47073 helo=mail-yb1-xb32.google.com) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1n4sk9-0008FR-El for grub-devel@gnu.org; Tue, 04 Jan 2022 17:57:27 -0500 Received: by mail-yb1-xb32.google.com with SMTP id w13so78638564ybs.13 for ; Tue, 04 Jan 2022 14:57:24 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=TNBq+prrzTLQCYUSoYEctC8JXiyMnalmHs6AYhkURsQ=; b=OEM6BApf8GNhuYvK8zEpdb2YJKHB3K5U9Nh2y8iNKXnwscdQfhSZy+G2NkfV+nV0ZK ZNhF2eDgmG7LzpnCZIgatjrPzZE3sKoQNBXrXZPK8hhbePlXEjxTce6BMmIiaxzjLvhO +Ly74XJ9KuMrbSY0GQypUwvyOiG2C5FwDdlVZqdqvlBTdBv4YlJhFL+1UQYnWuUP1mpo /6bn30u8f5g5FkRpfAzHIdoUYXdgPUtHowUMpOiCc0QFpF4lpZjAw9q3gg+2SK1FaCUJ mYU1m4quvdJ57uuKoUoCcNUO2D5M3UISjyUeK3UZOv0F4FjAVazlUHaq18RV6jJefUdI 7X5g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=TNBq+prrzTLQCYUSoYEctC8JXiyMnalmHs6AYhkURsQ=; b=xtOl0IAsa4jiS3p01KRcrW5QzJTKDevQTXecm6OOPwRbfOUHQXwdJXbz6HCKNmBw/z H5Ddwsza1YWToA0bRlEFK5kBdJ6uN0G1j3u00nn+UVPtPj4Dtvvp8KmNP6emSXkZlmgB SDf89pkmMThfE8Gvt/YY0G6LOQdw6Gzm7PdWLbNBbWh0YhMVe2EwVNYqaXvZGUA/QvFf W1QJhpbZEygttEC1ZU9TNuTwJgBYe8VMqWWjGKfvdtfcD7Bu7bM0s/PvTGtYc1XcQxuo CP3qKqNy020QWt6LWKuFqChda791pMCPuGc34bYOs9/BUMhajLVJUVYZoqQGf1GBJezj wQMw== X-Gm-Message-State: AOAM532QRdhd0kaRaOsuQZ75yrAy9XbuXZZTAt+r0t0l2tBl2ZeiAFgq roq6kiDwo8UU46FhBWyxBkrSBWmSn7LqG9LAbIePIgn7/WZVKw== X-Google-Smtp-Source: ABdhPJzJQ1+xEZtM9gnREZpesazvw7XgoIdHBjpth7MUj0qL/pRbYVMAh38+f2dqxCVYxKZvLfLvM1xsi4hhOlHOQko= X-Received: by 2002:a25:640a:: with SMTP id y10mr50342506ybb.189.1641337043981; Tue, 04 Jan 2022 14:57:23 -0800 (PST) MIME-Version: 1.0 References: <20220104154222.142bd77e@crass-HP-ZBook-15-G2> <20220104160656.18fbd917@crass-HP-ZBook-15-G2> In-Reply-To: <20220104160656.18fbd917@crass-HP-ZBook-15-G2> From: Dmitry Date: Wed, 5 Jan 2022 01:57:12 +0300 Message-ID: Subject: Re: [PATCH v8 3/7] cryptodisk: enable the backends to implement detached headers To: Glenn Washburn Cc: Daniel Kiper , grub-devel , "Denis 'GNUtoo' Carikli" , Patrick Steinhardt , John Lane Content-Type: multipart/alternative; boundary="000000000000769a5905d4c98ff8" X-Host-Lookup-Failed: Reverse DNS lookup failed for 2607:f8b0:4864:20::b32 (failed) Received-SPF: pass client-ip=2607:f8b0:4864:20::b32; envelope-from=reagentoo@gmail.com; helo=mail-yb1-xb32.google.com X-Spam_score_int: 6 X-Spam_score: 0.6 X-Spam_bar: / X-Spam_report: (0.6 / 5.0 requ) DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, FREEMAIL_FROM=0.001, HTML_MESSAGE=0.001, 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-Mailman-Approved-At: Tue, 04 Jan 2022 18:27:01 -0500 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:57:27 -0000 --000000000000769a5905d4c98ff8 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable =D1=81=D1=80, 5 =D1=8F=D0=BD=D0=B2. 2022 =D0=B3. =D0=B2 01:07, Glenn Washbu= rn : > On Tue, 4 Jan 2022 15:42:22 -0600 > Glenn Washburn 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/0921badcf817071639058bc4a534c8e6bd= 05f212/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 > --000000000000769a5905d4c98ff8 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


=
=D1=81=D1=80, 5 =D1=8F=D0=BD=D0=B2. 2= 022 =D0=B3. =D0=B2 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 fr= om 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?
<= br>
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 sta= te 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 structu= re.
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 t= his structure. This would
reflect the essence of this structure, = but further implementation the code will
not be pretty in this ca= se.

I still suggest expanding the number of parameters for the recov= er_key function

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

>
> >=C2=A0 };
> >=C2=A0 typedef struct grub_cryptomount_args *grub_cryptomount_args= _t;
> >=C2=A0
> > 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
> >=C2=A0 =C2=A0 =C2=A0 GRUB_FILE_TYPE_FONT,
> >=C2=A0 =C2=A0 =C2=A0 /* File holding encryption key for encrypted = ZFS.=C2=A0 */
> >=C2=A0 =C2=A0 =C2=A0 GRUB_FILE_TYPE_ZFS_ENCRYPTION_KEY,
> > +=C2=A0 =C2=A0 /* File holding the encryption metadata header */<= br> > > +=C2=A0 =C2=A0 GRUB_FILE_TYPE_CRYPTODISK_DETACHED_HEADER,
> >=C2=A0 =C2=A0 =C2=A0 /* File we open n grub-fstest.=C2=A0 */
> >=C2=A0 =C2=A0 =C2=A0 GRUB_FILE_TYPE_FSTEST,
> >=C2=A0 =C2=A0 =C2=A0 /* File we open n grub-mount.=C2=A0 */
>
> Dmitry
--000000000000769a5905d4c98ff8--