On Thu, Apr 24, 2025 at 11:41 PM Elijah Newren <newren@xxxxxxxxx> wrote:
>
> On Thu, Apr 24, 2025 at 1:39 PM Christian Couder
> <christian.couder@xxxxxxxxx> 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.
Yeah, the second half of the sentence was about switching tracks to
discuss other things we might do in the future.
> 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." ?
Yeah, that was the idea. Thanks for spelling it in a better way than I did.
In v2, I have actually tried an approach based on verifying
signatures, and I'd be happy to know your opinion about the different
approaches.
Thanks!
[...]
> > +Signatures are not yet checked in the current implementation though.
>
> Thanks for calling this out.
[...]
> > @@ -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.
Yeah, if we want to continue in the direction of this patch, let's
discuss it 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.
Yeah, I think it's useful to check that we can get the same commit back.
> Should there also be a
> test for when something is tweaked, such as the encoding, and whether
> the signature is still found?
[...]
> > +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...
Thanks for your review and all your comments!
As I am not sure which approach is best and I am trying a different
approach in v2, it might not be worth discussing some of the details
of the approach I took in this v1 anymore. So I will skip answering
some of your specific comments unless we decide to go back to that
approach later.
