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

From: Patrick Steinhardt <ps@pks.im>
Date: Feb 24, 2026, 09:33 UTC
Message-ID: <aZ1wblYGQssyNYsk@pks.im>
In-Reply-To: <20260223194146.3476768-3-jltobler@gmail.com>

On Mon, Feb 23, 2026 at 01:41:46PM -0600, Justin Tobler wrote:

Show 10 quoted lines

> 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.

Show 11 quoted lines

> 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.

Show 20 quoted lines

> 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.

> @@ -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?

Show 7 quoted lines

> +		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/?

Show 8 quoted lines

> +			 * 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.

I think meanwhile though it's okay that we don't handle compatibility hashes yet.

Show 13 quoted lines

> 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>]"?

Thanks!

Patrick