From: Patrick Steinhardt <ps@pks.im>
Date: Thu, 12 Mar 2026 10:23:01 GMT
Subject: Re: [PATCH v4 3/3] fast-import: add mode to sign commits with invalid signatures
Message-ID: <abKUBRRgRmbJ1hRA@pks.im>
In-Reply-To: <20260311173147.2336432-4-jltobler@gmail.com>

On Wed, Mar 11, 2026 at 12:31:47PM -0500, Justin Tobler wrote:
> diff --git a/builtin/fast-import.c b/builtin/fast-import.c
> index b8a7757cfd..d6281ff119 100644
> --- a/builtin/fast-import.c
> +++ b/builtin/fast-import.c
> @@ -2865,6 +2855,66 @@ static void handle_strip_if_invalid(struct strbuf *new_data,
>  		else
>  			warning(_("stripping invalid signature for commit\n"
>  				  "  allegedly by %s"), signer);
> +		break;
> +	case SIGN_SIGN_IF_INVALID:
> +		if (subject_len > 100)
> +			warning(_("signing commit with invalid signature for '%.100s...'\n"
> +				  "  allegedly by %s"), subject, signer);
> +		else if (subject_len > 0)
> +			warning(_("signing commit with invalid signature for '%.*s'\n"
> +				  "  allegedly by %s"), subject_len, subject, signer);
> +		else
> +			warning(_("signing commit with invalid signature\n"
> +				  "  allegedly by %s"), signer);
> +		break;
> +	default:
> +		BUG("unsupported signing mode");
> +	}
> +}

I'm still not convinced that it makes sense to warn about this case.
After all the user has asked us to re-sign such commits, so they
probably expect such cases. These warnings would thus result in a ton of
noise in a repository where most commits are signed, drowning out the
potentially-useful warnings.

Anyway, I won't insist on a change here.

> +static void handle_signature_if_invalid(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 };
> +	int ret;
> +
> +	/* Check signature in a temporary commit buffer */
> +	strbuf_addbuf(&tmp_buf, new_data);
> +	finalize_commit_buffer(&tmp_buf, sig_sha1, sig_sha256, msg);
> +	ret = verify_commit_buffer(tmp_buf.buf, tmp_buf.len, &signature_check);
> +
> +	if (ret) {
> +		warn_invalid_signature(&signature_check, msg->buf, mode);
> +
> +		if (mode == SIGN_SIGN_IF_INVALID) {
> +			struct strbuf signature = STRBUF_INIT;
> +			struct strbuf payload = STRBUF_INIT;
> +
> +			/*
> +			 * NEEDSWORK: To properly support interoperability mode
> +			 * when signing commit signatures, the commit buffer
> +			 * must be provided in both the repository and
> +			 * compatibility object formats. As currently
> +			 * implemented, only the repository object format is
> +			 * considered meaning compatibility signatures cannot be
> +			 * generated. Thus, attempting to sign commit signatures
> +			 * in interoperability mode is currently unsupported.
> +			 */
> +			if (the_repository->compat_hash_algo)
> +				die(_("signing signatures in interoperability mode is unsupported"));

"signing signatures"? You probably meant "signing commits"?

Patrick