Re: [PATCH v6 01/12] luks2: Add slot_key member to struct grub_luks2_keyslot/segment/digest

[Daniel Kiper]

On Thu, Dec 03, 2020 at 10:46:12AM -0600, Glenn Washburn wrote:
> On Thu, 3 Dec 2020 13:35:28 +0100
> Daniel Kiper <dkiper@net-space.pl> wrote:
>
> > On Thu, Dec 03, 2020 at 01:23:17AM -0600, Glenn Washburn wrote:
> > > On Wed, 2 Dec 2020 18:01:47 +0100
> > > Daniel Kiper <dkiper@net-space.pl> wrote:
> > >
> > > > On Fri, Nov 27, 2020 at 03:03:33AM -0600, Glenn Washburn wrote:
> > > > > This allows code using these structs to know the named key
> > > > > associated with these json data structures. In the future we can
> > > > > use these to provide better error messages to the user.
> > > > >
> > > > > Get rid of idx variable in luks2_get_keyslot which was
> > > > > overloaded to be used
> > > >
> > > > I prefer if you add "()" to the function names, i.e.
> > > > luks2_get_keyslot(), in the comments and commit messages. This
> > > > way it is easier to understand what you mean.
> > >
> > > Ok.
> > >
> > > > > for both keyslot and segment slot keys.
> > > > >
> > > > > Signed-off-by: Glenn Washburn <development@efficientek.com>
> > > > > ---
> > > > >  grub-core/disk/luks2.c | 13 ++++++++-----
> > > > >  1 file changed, 8 insertions(+), 5 deletions(-)
> > > > >
> > > > > diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
> > > > > index d96764a02..ab2c31dcd 100644
> > > > > --- a/grub-core/disk/luks2.c
> > > > > +++ b/grub-core/disk/luks2.c
> > > > > @@ -65,6 +65,7 @@ typedef struct grub_luks2_header
> > > > > grub_luks2_header_t;
> > > > >
> > > > >  struct grub_luks2_keyslot
> > > > >  {
> > > > > +  grub_uint64_t slot_key;
> > > >
> > > > Could you be more consistent and use keyslot instead of slot_key
> > > > here?
> > >
> > > I intentionally chose not to use keyslot because I thought it was
> > > confusing.  slot_key is not a keyslot in the sense of a slot for a
> > > cryptographic key or key material as in the usage keyslot in the
> > > struct named "grub_luks2_keyslot".  Its the key value of a "slot"
> > > in a json associative array which is modeling a sparse array.
> > > Perhaps just "key" might be more to your liking?
> >
> > IMO "key" is more confusing in this context and it would require at
> > least a comment to clarify what it means. Is not json_slot_key better
> > then? Probably with some comment what is it too...
>
> Yeah, that's a better name in terms of its descriptive power, but feels
> kinda long.  I'm fine with using that though, and will use that for the
> next patch series, unless I hear otherwise.

Yeah, unfortunately world is not perfect... :-(

> > > > >    grub_int64_t key_size;
> > > > >    grub_int64_t priority;
> > > > >    struct
> > > > > @@ -103,6 +104,7 @@ typedef struct grub_luks2_keyslot
> > > > > grub_luks2_keyslot_t;
> > > > >
> > > > >  struct grub_luks2_segment
> > > > >  {
> > > > > +  grub_uint64_t slot_key;
> > > >
> > > > Ditto. The code below uses keyslot instead...
> > > >
> > > > >    grub_uint64_t offset;
> > > > >    const char *size;
> > > > >    const char *encryption;
> > > > > @@ -112,6 +114,7 @@ typedef struct grub_luks2_segment
> > > > > grub_luks2_segment_t;
> > > > >
> > > > >  struct grub_luks2_digest
> > > > >  {
> > > > > +  grub_uint64_t slot_key;
> > > > >    /* Both keyslots and segments are interpreted as bitfields
> > > > > here */ grub_uint64_t keyslots;
> > > > >    grub_uint64_t      segments;
> > > > > @@ -259,12 +262,11 @@ luks2_get_keyslot (grub_luks2_keyslot_t
> > > > > *k, grub_luks2_digest_t *d, grub_luks2_s {
> > > > >    grub_json_t keyslots, keyslot, digests, digest, segments,
> > > > > segment; grub_size_t i, size;
> > > > > -  grub_uint64_t idx;
> > > > >
> > > > >    /* Get nth keyslot */
> > > > >    if (grub_json_getvalue (&keyslots, root, "keyslots") ||
> > > > >        grub_json_getchild (&keyslot, &keyslots, keyslot_idx) ||
> > > > > -      grub_json_getuint64 (&idx, &keyslot, NULL) ||
> > > > > +      grub_json_getuint64 (&k->slot_key, &keyslot, NULL) ||
> >
> > Should not keyslot be renamed to json_slot_key too?
>
> If I understand you correctly, you're asking about the variable keyslot
> of type grub_json_t.  Here keyslot is a json object representing a
> keyslot (ie collection of parameters used to decrypt an encrypted key).
> It is not what I call a "slot key", which is the value of a key in a
> json associative array.  So keyslot is a json object, where as a slot
> key is always a json string (afaik).  I believe that it is currently
> appropriately named.

OK, so, leave it as it is right now.

Daniel