[PATCH REBASED] verify: search keyid in hashed signature subpackets

[PATCH REBASED] verify: search keyid in hashed signature subpackets

[Daniel Axtens]

Currently GRUB2 verify logic searches PGP keyid only in unhashed subpackets of
PGP signature packet. As a result, signatures generated with GoLang openpgp
package (https://godoc.org/golang.org/x/crypto/openpgp) could not be verified,
because this package puts keyid in hashed subpackets and GRUB code never
initializes the keyid variable, therefore is not able to find "verification
key" with id 0x0.

(Add further description per thread at https://lists.gnu.org/archive/html/grub-devel/2016-11/msg00073.html)

Signed-off-by: Ignat Korchagin <ignat@cloudflare.com>
[ modified by Charles Duffy <charles@dyfis.net>, needs his Signed-off-by before merging ]
[ modified by dja: rebase, split out 'readbuf' to both readbuf and subpacket_buf for clarity
  signature_test still passes but I have not run any other tests ]
[ Under DCO rules I cannot provied a signed-off-by until Charles certifies his work can be redistributed ]
---
 grub-core/commands/pgp.c | 117 ++++++++++++++++++++++++++-------------
 1 file changed, 77 insertions(+), 40 deletions(-)

diff --git a/grub-core/commands/pgp.c b/grub-core/commands/pgp.c
index bbf6871fe71f..ad91f462bb91 100644
--- a/grub-core/commands/pgp.c
+++ b/grub-core/commands/pgp.c
@@ -448,6 +448,38 @@ struct grub_pubkey_context
   void *hash_context;
 };
 
+static grub_uint64_t
+grub_subpacket_keyid_search (const grub_uint8_t * sub, grub_ssize_t sub_len)
+{
+  const grub_uint8_t *ptr;
+  grub_uint32_t l;
+  grub_uint64_t keyid = 0;
+
+  for (ptr = sub; ptr < sub + sub_len; ptr += l)
+    {
+      if (*ptr < 192)
+       l = *ptr++;
+      else if (*ptr < 255)
+       {
+         if (ptr + 1 >= sub + sub_len)
+           break;
+         l = (((ptr[0] & ~192) << GRUB_CHAR_BIT) | ptr[1]) + 192;
+         ptr += 2;
+       }
+      else
+       {
+         if (ptr + 5 >= sub + sub_len)
+           break;
+         l = grub_be_to_cpu32 (grub_get_unaligned32 (ptr + 1));
+         ptr += 5;
+       }
+      if (*ptr == 0x10 && l >= 8)
+       keyid = grub_get_unaligned64 (ptr + 1);
+    }
+
+  return keyid;
+}
+
 static grub_err_t
 grub_verify_signature_init (struct grub_pubkey_context *ctxt, grub_file_t sig)
 {
@@ -520,7 +552,7 @@ grub_verify_signature_real (struct grub_pubkey_context *ctxt,
   gcry_mpi_t mpis[10];
   grub_uint8_t pk = ctxt->v4.pkeyalgo;
   grub_size_t i;
-  grub_uint8_t *readbuf = NULL;
+  grub_uint8_t *readbuf = NULL, *subpacket_buf = NULL;
   unsigned char *hval;
   grub_ssize_t rem = grub_be_to_cpu16 (ctxt->v4.hashed_sub);
   grub_uint32_t headlen = grub_cpu_to_be32 (rem + 6);
@@ -538,17 +570,29 @@ grub_verify_signature_real (struct grub_pubkey_context *ctxt,
 
   ctxt->hash->write (ctxt->hash_context, &ctxt->v, sizeof (ctxt->v));
   ctxt->hash->write (ctxt->hash_context, &ctxt->v4, sizeof (ctxt->v4));
-  while (rem)
+
+  subpacket_buf = grub_malloc (rem);
+  if (!subpacket_buf)
+    goto fail;
+
+  r = 0;
+  while (r < rem)
     {
-      r = grub_file_read (ctxt->sig, readbuf,
-			  rem < READBUF_SIZE ? rem : READBUF_SIZE);
-      if (r < 0)
-	goto fail;
-      if (r == 0)
+      grub_ssize_t rr = grub_file_read (ctxt->sig, subpacket_buf + r, rem - r);
+      if (rr < 0)
+        goto fail;
+      if (rr == 0)
 	break;
-      ctxt->hash->write (ctxt->hash_context, readbuf, r);
-      rem -= r;
+      r += rr;
     }
+  if (r != rem)
+    goto fail;
+  ctxt->hash->write (ctxt->hash_context, subpacket_buf, rem);
+
+  keyid = grub_subpacket_keyid_search (subpacket_buf, rem);
+  grub_free (subpacket_buf);
+  subpacket_buf = NULL;
+
   ctxt->hash->write (ctxt->hash_context, &ctxt->v, sizeof (ctxt->v));
   s = 0xff;
   ctxt->hash->write (ctxt->hash_context, &s, sizeof (s));
@@ -556,37 +600,27 @@ grub_verify_signature_real (struct grub_pubkey_context *ctxt,
   r = grub_file_read (ctxt->sig, &unhashed_sub, sizeof (unhashed_sub));
   if (r != sizeof (unhashed_sub))
     goto fail;
-  {
-    grub_uint8_t *ptr;
-    grub_uint32_t l;
-    rem = grub_be_to_cpu16 (unhashed_sub);
-    if (rem > READBUF_SIZE)
-      goto fail;
-    r = grub_file_read (ctxt->sig, readbuf, rem);
-    if (r != rem)
-      goto fail;
-    for (ptr = readbuf; ptr < readbuf + rem; ptr += l)
-      {
-	if (*ptr < 192)
-	  l = *ptr++;
-	else if (*ptr < 255)
-	  {
-	    if (ptr + 1 >= readbuf + rem)
-	      break;
-	    l = (((ptr[0] & ~192) << GRUB_CHAR_BIT) | ptr[1]) + 192;
-	    ptr += 2;
-	  }
-	else
-	  {
-	    if (ptr + 5 >= readbuf + rem)
-	      break;
-	    l = grub_be_to_cpu32 (grub_get_unaligned32 (ptr + 1));
-	    ptr += 5;
-	  }
-	if (*ptr == 0x10 && l >= 8)
-	  keyid = grub_get_unaligned64 (ptr + 1);
-      }
-  }
+
+  rem = grub_be_to_cpu16 (unhashed_sub);
+  subpacket_buf = grub_malloc (rem);
+  if (!subpacket_buf)
+    goto fail;
+
+  r = 0;
+  while (r < rem)
+    {
+     grub_ssize_t rr = grub_file_read (ctxt->sig, subpacket_buf + r, rem - r);
+     if (rr < 0)
+       goto fail;
+     if (rr == 0)
+       break;
+     r += rr;
+    }
+  if (r != rem)
+    goto fail;
+
+  if (keyid == 0)
+    keyid = grub_subpacket_keyid_search (subpacket_buf, rem);
 
   ctxt->hash->final (ctxt->hash_context);
 
@@ -656,10 +690,13 @@ grub_verify_signature_real (struct grub_pubkey_context *ctxt,
     goto fail;
 
   grub_free (readbuf);
+  grub_free (subpacket_buf);
 
   return GRUB_ERR_NONE;
 
  fail:
+  if (subpacket_buf)
+    grub_free (subpacket_buf);
   grub_free (readbuf);
   if (!grub_errno)
     return grub_error (GRUB_ERR_BAD_SIGNATURE, N_("bad signature"));
-- 
2.20.1

Re: [PATCH REBASED] verify: search keyid in hashed signature subpackets

[Charles Duffy]

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

Amended the test repo to apply this patch; it applies and works-as-intended
on both 2.04 and current master.

As for the DCO assertions, my portion of the contribution was implemented
strictly on personal time/equipment, so I'm able to to make the relevant
assertions in my individual capacity; amended below thusly.

On Thu, May 28, 2020 at 10:06 PM Daniel Axtens <dja@axtens.net> wrote:

> Currently GRUB2 verify logic searches PGP keyid only in unhashed
> subpackets of
> PGP signature packet. As a result, signatures generated with GoLang openpgp
> package (https://godoc.org/golang.org/x/crypto/openpgp) could not be
> verified,
> because this package puts keyid in hashed subpackets and GRUB code never
> initializes the keyid variable, therefore is not able to find "verification
> key" with id 0x0.
>
> (Add further description per thread at
> https://lists.gnu.org/archive/html/grub-devel/2016-11/msg00073.html)
>
> Signed-off-by: Ignat Korchagin <ignat@cloudflare.com>
>
Signed-off-by: Charles Duffy <charles@dyfis.net>
[ modified by Charles Duffy: rebase from pre-2.02 release to 2.02 final ]

> [ modified by dja: rebase, split out 'readbuf' to both readbuf and
> subpacket_buf for clarity
>   signature_test still passes but I have not run any other tests ]
> [ Under DCO rules I cannot provied a signed-off-by until Charles certifies
> his work can be redistributed ]
> ---
>  grub-core/commands/pgp.c | 117 ++++++++++++++++++++++++++-------------
>  1 file changed, 77 insertions(+), 40 deletions(-)
>
> diff --git a/grub-core/commands/pgp.c b/grub-core/commands/pgp.c
> index bbf6871fe71f..ad91f462bb91 100644
> --- a/grub-core/commands/pgp.c
> +++ b/grub-core/commands/pgp.c
> @@ -448,6 +448,38 @@ struct grub_pubkey_context
>    void *hash_context;
>  };
>
> +static grub_uint64_t
> +grub_subpacket_keyid_search (const grub_uint8_t * sub, grub_ssize_t
> sub_len)
> +{
> +  const grub_uint8_t *ptr;
> +  grub_uint32_t l;
> +  grub_uint64_t keyid = 0;
> +
> +  for (ptr = sub; ptr < sub + sub_len; ptr += l)
> +    {
> +      if (*ptr < 192)
> +       l = *ptr++;
> +      else if (*ptr < 255)
> +       {
> +         if (ptr + 1 >= sub + sub_len)
> +           break;
> +         l = (((ptr[0] & ~192) << GRUB_CHAR_BIT) | ptr[1]) + 192;
> +         ptr += 2;
> +       }
> +      else
> +       {
> +         if (ptr + 5 >= sub + sub_len)
> +           break;
> +         l = grub_be_to_cpu32 (grub_get_unaligned32 (ptr + 1));
> +         ptr += 5;
> +       }
> +      if (*ptr == 0x10 && l >= 8)
> +       keyid = grub_get_unaligned64 (ptr + 1);
> +    }
> +
> +  return keyid;
> +}
> +
>  static grub_err_t
>  grub_verify_signature_init (struct grub_pubkey_context *ctxt, grub_file_t
> sig)
>  {
> @@ -520,7 +552,7 @@ grub_verify_signature_real (struct grub_pubkey_context
> *ctxt,
>    gcry_mpi_t mpis[10];
>    grub_uint8_t pk = ctxt->v4.pkeyalgo;
>    grub_size_t i;
> -  grub_uint8_t *readbuf = NULL;
> +  grub_uint8_t *readbuf = NULL, *subpacket_buf = NULL;
>    unsigned char *hval;
>    grub_ssize_t rem = grub_be_to_cpu16 (ctxt->v4.hashed_sub);
>    grub_uint32_t headlen = grub_cpu_to_be32 (rem + 6);
> @@ -538,17 +570,29 @@ grub_verify_signature_real (struct
> grub_pubkey_context *ctxt,
>
>    ctxt->hash->write (ctxt->hash_context, &ctxt->v, sizeof (ctxt->v));
>    ctxt->hash->write (ctxt->hash_context, &ctxt->v4, sizeof (ctxt->v4));
> -  while (rem)
> +
> +  subpacket_buf = grub_malloc (rem);
> +  if (!subpacket_buf)
> +    goto fail;
> +
> +  r = 0;
> +  while (r < rem)
>      {
> -      r = grub_file_read (ctxt->sig, readbuf,
> -                         rem < READBUF_SIZE ? rem : READBUF_SIZE);
> -      if (r < 0)
> -       goto fail;
> -      if (r == 0)
> +      grub_ssize_t rr = grub_file_read (ctxt->sig, subpacket_buf + r, rem
> - r);
> +      if (rr < 0)
> +        goto fail;
> +      if (rr == 0)
>         break;
> -      ctxt->hash->write (ctxt->hash_context, readbuf, r);
> -      rem -= r;
> +      r += rr;
>      }
> +  if (r != rem)
> +    goto fail;
> +  ctxt->hash->write (ctxt->hash_context, subpacket_buf, rem);
> +
> +  keyid = grub_subpacket_keyid_search (subpacket_buf, rem);
> +  grub_free (subpacket_buf);
> +  subpacket_buf = NULL;
> +
>    ctxt->hash->write (ctxt->hash_context, &ctxt->v, sizeof (ctxt->v));
>    s = 0xff;
>    ctxt->hash->write (ctxt->hash_context, &s, sizeof (s));
> @@ -556,37 +600,27 @@ grub_verify_signature_real (struct
> grub_pubkey_context *ctxt,
>    r = grub_file_read (ctxt->sig, &unhashed_sub, sizeof (unhashed_sub));
>    if (r != sizeof (unhashed_sub))
>      goto fail;
> -  {
> -    grub_uint8_t *ptr;
> -    grub_uint32_t l;
> -    rem = grub_be_to_cpu16 (unhashed_sub);
> -    if (rem > READBUF_SIZE)
> -      goto fail;
> -    r = grub_file_read (ctxt->sig, readbuf, rem);
> -    if (r != rem)
> -      goto fail;
> -    for (ptr = readbuf; ptr < readbuf + rem; ptr += l)
> -      {
> -       if (*ptr < 192)
> -         l = *ptr++;
> -       else if (*ptr < 255)
> -         {
> -           if (ptr + 1 >= readbuf + rem)
> -             break;
> -           l = (((ptr[0] & ~192) << GRUB_CHAR_BIT) | ptr[1]) + 192;
> -           ptr += 2;
> -         }
> -       else
> -         {
> -           if (ptr + 5 >= readbuf + rem)
> -             break;
> -           l = grub_be_to_cpu32 (grub_get_unaligned32 (ptr + 1));
> -           ptr += 5;
> -         }
> -       if (*ptr == 0x10 && l >= 8)
> -         keyid = grub_get_unaligned64 (ptr + 1);
> -      }
> -  }
> +
> +  rem = grub_be_to_cpu16 (unhashed_sub);
> +  subpacket_buf = grub_malloc (rem);
> +  if (!subpacket_buf)
> +    goto fail;
> +
> +  r = 0;
> +  while (r < rem)
> +    {
> +     grub_ssize_t rr = grub_file_read (ctxt->sig, subpacket_buf + r, rem
> - r);
> +     if (rr < 0)
> +       goto fail;
> +     if (rr == 0)
> +       break;
> +     r += rr;
> +    }
> +  if (r != rem)
> +    goto fail;
> +
> +  if (keyid == 0)
> +    keyid = grub_subpacket_keyid_search (subpacket_buf, rem);
>
>    ctxt->hash->final (ctxt->hash_context);
>
> @@ -656,10 +690,13 @@ grub_verify_signature_real (struct
> grub_pubkey_context *ctxt,
>      goto fail;
>
>    grub_free (readbuf);
> +  grub_free (subpacket_buf);
>
>    return GRUB_ERR_NONE;
>
>   fail:
> +  if (subpacket_buf)
> +    grub_free (subpacket_buf);
>    grub_free (readbuf);
>    if (!grub_errno)
>      return grub_error (GRUB_ERR_BAD_SIGNATURE, N_("bad signature"));
> --
> 2.20.1
>
>

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

Re: [PATCH REBASED] verify: search keyid in hashed signature subpackets

[Daniel Axtens]

Charles Duffy <charles@dyfis.net> writes:

> Amended the test repo to apply this patch; it applies and works-as-intended
> on both 2.04 and current master.
>
> As for the DCO assertions, my portion of the contribution was implemented
> strictly on personal time/equipment, so I'm able to to make the relevant
> assertions in my individual capacity; amended below thusly.

Awesome, me too.

>> (Add further description per thread at
>> https://lists.gnu.org/archive/html/grub-devel/2016-11/msg00073.html)

I will leave doing further revisions to you - looking through the thread
from 2016 it looks like the commit message needs more details and maybe
some variable names and constants need to be cleaned up etc. Now that we
have all the relevant Signed-off-bys, that should all be just a matter of
programming. My understanding is that you should maintain all three
S-O-Bs in the commit message for future spins, but I've never been clear
on what order they should be in if you make further revisions.

>>
>> Signed-off-by: Ignat Korchagin <ignat@cloudflare.com>
>>
> Signed-off-by: Charles Duffy <charles@dyfis.net>
> [ modified by Charles Duffy: rebase from pre-2.02 release to 2.02 final ]
>
>> [ modified by dja: rebase, split out 'readbuf' to both readbuf and
>> subpacket_buf for clarity
>>   signature_test still passes but I have not run any other tests ]
Signed-off-by: Daniel Axtens <dja@axtens.net>


>> ---
>>  grub-core/commands/pgp.c | 117 ++++++++++++++++++++++++++-------------
>>  1 file changed, 77 insertions(+), 40 deletions(-)
>>
>> diff --git a/grub-core/commands/pgp.c b/grub-core/commands/pgp.c
>> index bbf6871fe71f..ad91f462bb91 100644
>> --- a/grub-core/commands/pgp.c
>> +++ b/grub-core/commands/pgp.c
>> @@ -448,6 +448,38 @@ struct grub_pubkey_context
>>    void *hash_context;
>>  };
>>
>> +static grub_uint64_t
>> +grub_subpacket_keyid_search (const grub_uint8_t * sub, grub_ssize_t
>> sub_len)
>> +{
>> +  const grub_uint8_t *ptr;
>> +  grub_uint32_t l;
>> +  grub_uint64_t keyid = 0;
>> +
>> +  for (ptr = sub; ptr < sub + sub_len; ptr += l)
>> +    {
>> +      if (*ptr < 192)
>> +       l = *ptr++;
>> +      else if (*ptr < 255)
>> +       {
>> +         if (ptr + 1 >= sub + sub_len)
>> +           break;
>> +         l = (((ptr[0] & ~192) << GRUB_CHAR_BIT) | ptr[1]) + 192;
>> +         ptr += 2;
>> +       }
>> +      else
>> +       {
>> +         if (ptr + 5 >= sub + sub_len)
>> +           break;
>> +         l = grub_be_to_cpu32 (grub_get_unaligned32 (ptr + 1));
>> +         ptr += 5;
>> +       }
>> +      if (*ptr == 0x10 && l >= 8)
>> +       keyid = grub_get_unaligned64 (ptr + 1);
>> +    }
>> +
>> +  return keyid;
>> +}
>> +
>>  static grub_err_t
>>  grub_verify_signature_init (struct grub_pubkey_context *ctxt, grub_file_t
>> sig)
>>  {
>> @@ -520,7 +552,7 @@ grub_verify_signature_real (struct grub_pubkey_context
>> *ctxt,
>>    gcry_mpi_t mpis[10];
>>    grub_uint8_t pk = ctxt->v4.pkeyalgo;
>>    grub_size_t i;
>> -  grub_uint8_t *readbuf = NULL;
>> +  grub_uint8_t *readbuf = NULL, *subpacket_buf = NULL;
>>    unsigned char *hval;
>>    grub_ssize_t rem = grub_be_to_cpu16 (ctxt->v4.hashed_sub);
>>    grub_uint32_t headlen = grub_cpu_to_be32 (rem + 6);
>> @@ -538,17 +570,29 @@ grub_verify_signature_real (struct
>> grub_pubkey_context *ctxt,
>>
>>    ctxt->hash->write (ctxt->hash_context, &ctxt->v, sizeof (ctxt->v));
>>    ctxt->hash->write (ctxt->hash_context, &ctxt->v4, sizeof (ctxt->v4));
>> -  while (rem)
>> +
>> +  subpacket_buf = grub_malloc (rem);
>> +  if (!subpacket_buf)
>> +    goto fail;
>> +
>> +  r = 0;
>> +  while (r < rem)
>>      {
>> -      r = grub_file_read (ctxt->sig, readbuf,
>> -                         rem < READBUF_SIZE ? rem : READBUF_SIZE);
>> -      if (r < 0)
>> -       goto fail;
>> -      if (r == 0)
>> +      grub_ssize_t rr = grub_file_read (ctxt->sig, subpacket_buf + r, rem
>> - r);
>> +      if (rr < 0)
>> +        goto fail;
>> +      if (rr == 0)
>>         break;
>> -      ctxt->hash->write (ctxt->hash_context, readbuf, r);
>> -      rem -= r;
>> +      r += rr;
>>      }
>> +  if (r != rem)
>> +    goto fail;
>> +  ctxt->hash->write (ctxt->hash_context, subpacket_buf, rem);
>> +
>> +  keyid = grub_subpacket_keyid_search (subpacket_buf, rem);
>> +  grub_free (subpacket_buf);
>> +  subpacket_buf = NULL;
>> +
>>    ctxt->hash->write (ctxt->hash_context, &ctxt->v, sizeof (ctxt->v));
>>    s = 0xff;
>>    ctxt->hash->write (ctxt->hash_context, &s, sizeof (s));
>> @@ -556,37 +600,27 @@ grub_verify_signature_real (struct
>> grub_pubkey_context *ctxt,
>>    r = grub_file_read (ctxt->sig, &unhashed_sub, sizeof (unhashed_sub));
>>    if (r != sizeof (unhashed_sub))
>>      goto fail;
>> -  {
>> -    grub_uint8_t *ptr;
>> -    grub_uint32_t l;
>> -    rem = grub_be_to_cpu16 (unhashed_sub);
>> -    if (rem > READBUF_SIZE)
>> -      goto fail;
>> -    r = grub_file_read (ctxt->sig, readbuf, rem);
>> -    if (r != rem)
>> -      goto fail;
>> -    for (ptr = readbuf; ptr < readbuf + rem; ptr += l)
>> -      {
>> -       if (*ptr < 192)
>> -         l = *ptr++;
>> -       else if (*ptr < 255)
>> -         {
>> -           if (ptr + 1 >= readbuf + rem)
>> -             break;
>> -           l = (((ptr[0] & ~192) << GRUB_CHAR_BIT) | ptr[1]) + 192;
>> -           ptr += 2;
>> -         }
>> -       else
>> -         {
>> -           if (ptr + 5 >= readbuf + rem)
>> -             break;
>> -           l = grub_be_to_cpu32 (grub_get_unaligned32 (ptr + 1));
>> -           ptr += 5;
>> -         }
>> -       if (*ptr == 0x10 && l >= 8)
>> -         keyid = grub_get_unaligned64 (ptr + 1);
>> -      }
>> -  }
>> +
>> +  rem = grub_be_to_cpu16 (unhashed_sub);
>> +  subpacket_buf = grub_malloc (rem);
>> +  if (!subpacket_buf)
>> +    goto fail;
>> +
>> +  r = 0;
>> +  while (r < rem)
>> +    {
>> +     grub_ssize_t rr = grub_file_read (ctxt->sig, subpacket_buf + r, rem
>> - r);
>> +     if (rr < 0)
>> +       goto fail;
>> +     if (rr == 0)
>> +       break;
>> +     r += rr;
>> +    }
>> +  if (r != rem)
>> +    goto fail;
>> +
>> +  if (keyid == 0)
>> +    keyid = grub_subpacket_keyid_search (subpacket_buf, rem);
>>
>>    ctxt->hash->final (ctxt->hash_context);
>>
>> @@ -656,10 +690,13 @@ grub_verify_signature_real (struct
>> grub_pubkey_context *ctxt,
>>      goto fail;
>>
>>    grub_free (readbuf);
>> +  grub_free (subpacket_buf);
>>
>>    return GRUB_ERR_NONE;
>>
>>   fail:
>> +  if (subpacket_buf)
>> +    grub_free (subpacket_buf);
>>    grub_free (readbuf);
>>    if (!grub_errno)
>>      return grub_error (GRUB_ERR_BAD_SIGNATURE, N_("bad signature"));
>> --
>> 2.20.1
>>
>>

Re: [PATCH REBASED] verify: search keyid in hashed signature subpackets

[Daniel Kiper]

On Fri, May 29, 2020 at 02:10:46PM +1000, Daniel Axtens wrote:
> Charles Duffy <charles@dyfis.net> writes:
>
> > Amended the test repo to apply this patch; it applies and works-as-intended
> > on both 2.04 and current master.
> >
> > As for the DCO assertions, my portion of the contribution was implemented
> > strictly on personal time/equipment, so I'm able to to make the relevant
> > assertions in my individual capacity; amended below thusly.
>
> Awesome, me too.

Oh, nice to see that work revived...

> >> (Add further description per thread at
> >> https://lists.gnu.org/archive/html/grub-devel/2016-11/msg00073.html)
>
> I will leave doing further revisions to you - looking through the thread
> from 2016 it looks like the commit message needs more details and maybe
> some variable names and constants need to be cleaned up etc. Now that we
> have all the relevant Signed-off-bys, that should all be just a matter of
> programming. My understanding is that you should maintain all three

You mean that I have to wait for next version of it...

> S-O-Bs in the commit message for future spins, but I've never been clear
> on what order they should be in if you make further revisions.

Well, it seems to me that it depends on the project and maintainers
preference. I prefer the oldest SOB at the top. So, in this case:

  Signed-off-by: Ignat Korchagin <ignat@cloudflare.com>
  Signed-off-by: Charles Duffy <charles@dyfis.net>
  Signed-off-by: Daniel Axtens <dja@axtens.net>

Daniel

Re: [PATCH REBASED] verify: search keyid in hashed signature subpackets

[Daniel Axtens]

Daniel Kiper <dkiper@net-space.pl> writes:

> On Fri, May 29, 2020 at 02:10:46PM +1000, Daniel Axtens wrote:
>> Charles Duffy <charles@dyfis.net> writes:
>>
>> > Amended the test repo to apply this patch; it applies and works-as-intended
>> > on both 2.04 and current master.
>> >
>> > As for the DCO assertions, my portion of the contribution was implemented
>> > strictly on personal time/equipment, so I'm able to to make the relevant
>> > assertions in my individual capacity; amended below thusly.
>>
>> Awesome, me too.
>
> Oh, nice to see that work revived...
>
>> >> (Add further description per thread at
>> >> https://lists.gnu.org/archive/html/grub-devel/2016-11/msg00073.html)
>>
>> I will leave doing further revisions to you - looking through the thread
>> from 2016 it looks like the commit message needs more details and maybe
>> some variable names and constants need to be cleaned up etc. Now that we
>> have all the relevant Signed-off-bys, that should all be just a matter of
>> programming. My understanding is that you should maintain all three
>
> You mean that I have to wait for next version of it...

I looked back at the 2016 thread and you had some comments there about
the clarity of the code and the details in the commit message. I imagine
those comments still stand. I was just trying to be clear to Charles
that I wasn't going to take on the task of addressing those comments,
and that he should address those and respin the patch.

>
>> S-O-Bs in the commit message for future spins, but I've never been clear
>> on what order they should be in if you make further revisions.
>
> Well, it seems to me that it depends on the project and maintainers
> preference. I prefer the oldest SOB at the top. So, in this case:
>
>   Signed-off-by: Ignat Korchagin <ignat@cloudflare.com>
>   Signed-off-by: Charles Duffy <charles@dyfis.net>
>   Signed-off-by: Daniel Axtens <dja@axtens.net>

Noted.

Regards,
Daniel