Re: [PATCH 2/2] fast-import: add mode to re-sign invalid commit signatures

[Justin Tobler <jltobler@gmail.com>]

On 26/02/24 10:33AM, Patrick Steinhardt wrote:
> On Mon, Feb 23, 2026 at 01:41:46PM -0600, Justin Tobler wrote:
> > With git-fast-import(1), handling of signed commits is controlled via
> > the `--signed-commits=<mode>` option. When an invalid signature is
> > encountered, a user may want the option to re-sign the commit as opposed
> > to just stripping the signature. To faciliate this, introduce a
> > "re-sign-if-invalid" mode for the `--signed-commits` option.
> > 
> > Note that commits are re-signed using only the repository object format
> > hash algorithm. If a commit has an additional signature due to the
> > `compatObjectFormat` repository extension being set, the other signature
> > is stripped.
> 
> This part here might use some explanation why this part is not done so
> that a future reader that ends up here doesn't have to wonder whether
> this is done with intent, or whether this was done because it was hard
> to do.

Good point. I'll expand the explaination here in the next version.

> > diff --git a/Documentation/git-fast-import.adoc b/Documentation/git-fast-import.adoc
> > index 479c4081da..b902a6e2b0 100644
> > --- a/Documentation/git-fast-import.adoc
> > +++ b/Documentation/git-fast-import.adoc
> > @@ -86,6 +86,9 @@ already trusted to run their own code.
> >  * `strip-if-invalid` will check signatures and, if they are invalid,
> >    will strip them and display a warning. The validation is performed
> >    in the same way as linkgit:git-verify-commit[1] does it.
> > +* `re-sign-if-invalid` is the same as `strip-if-invalid`, but additionally the
> > +  commits with invalid signatures are signed again, so that old invalid
> > +  signatures are replaced with new valid ones.
> 
> Okay. It's a bit curious to say it's the "same as `strip-if-invalid`",
> but I get what you mean by this, and I think a user would, too.

Ya, maybe it would be better to say that it is "similar to
`strip-if-invalid`". I'll try to rework the documentation here a little
bit in the next version.

> > diff --git a/builtin/fast-import.c b/builtin/fast-import.c
> > index b8a7757cfd..e34a373d2f 100644
> > --- a/builtin/fast-import.c
> > +++ b/builtin/fast-import.c
> > @@ -2836,10 +2836,11 @@ static void finalize_commit_buffer(struct strbuf *new_data,
> >  	strbuf_addbuf(new_data, msg);
> >  }
> >  
> > -static void handle_strip_if_invalid(struct strbuf *new_data,
> > -				    struct signature_data *sig_sha1,
> > -				    struct signature_data *sig_sha256,
> > -				    struct strbuf *msg)
> > +static void handle_invalid_signature(struct strbuf *new_data,
> > +				     struct signature_data *sig_sha1,
> > +				     struct signature_data *sig_sha256,
> > +				     struct strbuf *msg,
> > +				     enum sign_mode mode)
> >  {
> >  	struct strbuf tmp_buf = STRBUF_INIT;
> >  	struct signature_check signature_check = { 0 };
> 
> Should we maybe call this `handle_signature_if_invalid()`? Otherwise it
> sounds as if we already know the signature was invalid.

That sounds better. Will adapt.

> 
> > @@ -2866,6 +2867,30 @@ static void handle_strip_if_invalid(struct strbuf *new_data,
> >  			warning(_("stripping invalid signature for commit\n"
> >  				  "  allegedly by %s"), signer);
> 
> I wonder: does it still make sense to warn about those stripped
> signatures in case we re-sign anyway?

Ya, good point. I was originally thinking it would still make sense to
keep these messages since we are still stripping the signatures, but it
might be misleading if are also re-signing them. We could keep these,
but add an additional message if re-signing. That might be a little
noisy though. Maybe we just adapt the warning message when re-signing.

> > +		if (mode == SIGN_RESIGN_IF_INVALID) {
> > +			struct strbuf signature = STRBUF_INIT;
> > +			struct strbuf payload = STRBUF_INIT;
> > +			char *key = get_signing_key();
> > +
> > +			/*
> > +			 * Commits are resigned using the repository object
> 
> Poor commits. Maybe s/resigned/re-signed/?

Poor commits indeed, will change. XD

> > +			 * format hash algorithm only. Consequently if
> > +			 * extensions.compatObjectFormat is set, the
> > +			 * compatability hash is not currently used to
> > +			 * additionally sign the commit. If the commit payload
> > +			 * were reconstructed in the compatability format, it
> > +			 * would be possible to generate the other signature
> > +			 * accordingly though.
> > +			 */
> 
> Same as in the commit message, we should document whether this is done
> intentionally, or whether it may require more work going forward. If the
> latter, it might make sense to add a NEEDSWORK comment.

Ya, I think it should be possible to support compatibility hashes in the
future. I'll explain this better in a NEEDSWORK comment.

> I think meanwhile though it's okay that we don't handle compatibility
> hashes yet.
> 
> > diff --git a/gpg-interface.c b/gpg-interface.c
> > index 87fb6605fb..e7eb42d9d6 100644
> > --- a/gpg-interface.c
> > +++ b/gpg-interface.c
> > @@ -1156,6 +1156,8 @@ int parse_sign_mode(const char *arg, enum sign_mode *mode)
> >  		*mode = SIGN_STRIP;
> >  	else if (!strcmp(arg, "strip-if-invalid"))
> >  		*mode = SIGN_STRIP_IF_INVALID;
> > +	else if (!strcmp(arg, "re-sign-if-invalid"))
> > +		*mode = SIGN_RESIGN_IF_INVALID;
> >  	else
> >  		return -1;
> >  	return 0;
> 
> One thing I wonder here is which signing key is actually in use, and how
> the user would specify it. In git-commit(1) you can for example pass
> "--gpg-sign=<key-id>" to specify the key. Do we want to allow the same
> here, where you can pass "--signed-commits=re-sign-if-invalid[=<gpg-key>]"?

This seems sensible. I'll explore this in the next version.

Thanks for the review. :)

-Justin