Re: [PATCH] fast-(import|export): improve on the signature algorithm name

>
> On Thu, Apr 24, 2025 at 1:39 PM Christian Couder
> <christian.couder@gmail.com> wrote:
> >
> > A recent commit, d9cb0e6ff8 (fast-export, fast-import: add support for
> > signed-commits, 2025-03-10), added support for signed commits.
> >
> > However, when processing signatures `git fast-export` outputs "gpgsig
> > sha1" not just when it encounters an OpenPGP SHA-1 signature, but also
> > when it encounters an SSH or X.509 signature. This is not very
> > informative to say the least, and this might prevent tools that process
> > the output from easily and properly handling signatures.
> >
> > Let's improve on that by reusing the existing code from
> > "gpg-interface.{c,h}" to detect the signature algorithm, and then put
> > the signature algorithm name (like "openpgp", "x509" or "ssh") instead
> > of "sha1" in the output. If we can't detect the signature algorithm we
> > will use "unknown". It might be a signature added by an external tool
> > and we should likely keep it.
> >
> > Similarly on the `git fast-import` side, let's use the existing code
> > from "gpg-interface.{c,h}" to check if a signature algorithm name is
> > valid. In case of an "unknown" signature algorithm name, we will warn
> > but still keep it. Future work might implement several options to let
> > users deal with it in different ways, and might implement checking
> > known signatures too.
>
> The last sentence is somewhat ambiguous about whether it is only about
> the "unknown" case or whether the second half of the sentence was
> switching tracks to discuss something else about the known cases.

> Do
> you perhaps mean something like "Future work might implement several
> options to let users deal with an "unknown" signature algorithm, and
> when we have a valid signature algorithm, we may be able to not only
> verify the signature algorithm name but start verifying the signature
> itself to ensure it is valid as well."  ?

> > @@ -2830,12 +2831,15 @@ static void parse_new_commit(const char *arg)
> >                         "encoding %s\n",
> >                         encoding);
> >         if (sig_alg) {
> > -               if (!strcmp(sig_alg, "sha1"))
> > -                       strbuf_addstr(&new_data, "gpgsig ");
> > -               else if (!strcmp(sig_alg, "sha256"))
> > +               if (!strcmp(sig_alg, "sha256"))
> >                         strbuf_addstr(&new_data, "gpgsig-sha256 ");
> > -               else
> > -                       die("Expected gpgsig algorithm sha1 or sha256, got %s", sig_alg);
> > +               else if (valid_signature_name(sig_alg))
> > +                       strbuf_addstr(&new_data, "gpgsig ");
> > +               else if (!strcmp(sig_alg, "unknown")) {
> > +                       warning("Unknown gpgsig algorithm name!");
> > +                       strbuf_addstr(&new_data, "gpgsig ");
> > +               } else
> > +                       die("Invalid gpgsig algorithm name, got '%s'", sig_alg);
> >                 string_list_split_in_place(&siglines, sig.buf, "\n", -1);
> >                 strbuf_add_separated_string_list(&new_data, "\n ", &siglines);
> >                 strbuf_addch(&new_data, '\n');
>
> I'm not very familiar with gpg and other signatures, and was stuck
> trying to parse this logic when a review from Junio came in, and I
> decided to read it since he often "thinks out loud" to see if that'd
> explain it better.  Sadly, didn't help... ;-)  But I'll watch for any
> follow-up response you add over there.

> > @@ -381,6 +381,62 @@ test_expect_success GPG 'signed-commits=warn-strip' '
> >
> >  '
> >
> > +test_expect_success GPGSM 'setup x509 signed commit' '
> > +
> > +       git checkout -b x509-signing main &&
> > +       test_config gpg.format x509 &&
> > +       test_config user.signingkey $GIT_COMMITTER_EMAIL &&
> > +       echo "x509 content" >file_for_x509 &&
> > +       git add file_for_x509 &&
> > +       git commit -S -m "X.509 signed commit" &&
> > +       X509_COMMIT=$(git rev-parse --verify HEAD) &&
> > +       git checkout main
> > +
> > +'
> > +
> > +test_expect_success GPGSM 'x509 signature identified' '
> > +
> > +       git fast-export --signed-commits=verbatim --reencode=no x509-signing >output 2>err &&
>
> Is --reencode=no important here or does this work with --reencode=yes
> as well?  (I understand the default being --reencode=abort and fact
> that you are reusing an example that used a specialized encoding means
> you need to specify something, was just curious if this particular
> value was important)
>
> > +       grep "^gpgsig x509" output &&
> > +       test ! -s err &&
> > +       (
> > +               cd new &&
> > +               git fast-import &&
> > +               STRIPPED=$(git rev-parse --verify refs/heads/x509-signing) &&
> > +               test $X509_COMMIT = $STRIPPED
>
> Ah, --reencode=no is critical for the test, but only because you are
> trying to ensure you get the same commit back.

> > +test_expect_success GPGSSH 'ssh signature identified' '
> > +
> > +       git fast-export --signed-commits=verbatim --reencode=no ssh-signing >output 2>err &&
>
> Out of curiosity, any particular reason to export the entire history
> instead of just the new commit you just added (though you'd likely
> want --reference-excluded-parents if you did that, which really ought
> to be the default)?  Anyway...