* [PATCH 0/2] fast-import: add mode to re-sign invalid commit signatures
@ 2026-02-23 19:41 Justin Tobler
2026-02-23 19:41 ` [PATCH 1/2] commit: remove unused forward declaration Justin Tobler
` (4 more replies)
0 siblings, 5 replies; 60+ messages in thread
From: Justin Tobler @ 2026-02-23 19:41 UTC (permalink / raw)
To: git; +Cc: sandals, christian.couder, Justin Tobler
Greetings,
With c20f112e51 (fast-import: add 'strip-if-invalid' mode to
--signed-commits=<mode>, 2025-11-17), it became possible to remove
invalid signatures from commits via git-fast-import(1) while maintaining
valid commits. Building upon this functionality, a user may want to
re-sign these invalid commit signatures. This series introduces the
`re-sign-if-invalid` mode to do so accordingly.
The newly added mode in this series currently ignores
`extensions.compatObjectFormat` when generating the new signatures. From
my understanding, to generate the compatability structure would also
require us to reconstruct the compatability object for the object being
signed. I think this would be possible to do, but would require getting
the mapped OIDs for the commit parents and tree. I'm not competely sure
of a good way to go about this yet though. I'm also not completely
certain if this is something that should be adressed as part of this
series, or could be done later down the road. So for now I've opted to
delay its implementation. I'm open going down the other route if that is
preferred though.
The first commit is a simple cleanup for something I noticed while
reading though commit signing code. The second commit actually
introduces the new `--signed-commits` mode.
Thanks,
-Justin
Justin Tobler (2):
commit: remove unused forward declaration
fast-import: add mode to re-sign invalid commit signatures
Documentation/git-fast-import.adoc | 3 +
builtin/fast-export.c | 6 ++
builtin/fast-import.c | 43 +++++++--
commit.h | 2 -
gpg-interface.c | 2 +
gpg-interface.h | 1 +
t/t9305-fast-import-signatures.sh | 142 +++++++++++++++--------------
7 files changed, 125 insertions(+), 74 deletions(-)
base-commit: 7c02d39fc2ed2702223c7674f73150d9a7e61ba4
--
2.53.0
^ permalink raw reply [flat|nested] 60+ messages in thread* [PATCH 1/2] commit: remove unused forward declaration 2026-02-23 19:41 [PATCH 0/2] fast-import: add mode to re-sign invalid commit signatures Justin Tobler @ 2026-02-23 19:41 ` Justin Tobler 2026-02-24 9:35 ` Patrick Steinhardt 2026-02-23 19:41 ` [PATCH 2/2] fast-import: add mode to re-sign invalid commit signatures Justin Tobler ` (3 subsequent siblings) 4 siblings, 1 reply; 60+ messages in thread From: Justin Tobler @ 2026-02-23 19:41 UTC (permalink / raw) To: git; +Cc: sandals, christian.couder, Justin Tobler In 6206089cbd (commit: write commits for both hashes, 2023-10-01), `sign_with_header()` was removed, but its forward declaration in "commit.h" was left. Remove the unused declaration. Signed-off-by: Justin Tobler <jltobler@gmail.com> --- commit.h | 2 -- 1 file changed, 2 deletions(-) diff --git a/commit.h b/commit.h index 1635de418b..f0c38cb444 100644 --- a/commit.h +++ b/commit.h @@ -390,8 +390,6 @@ LAST_ARG_MUST_BE_NULL int run_commit_hook(int editor_is_used, const char *index_file, int *invoked_hook, const char *name, ...); -/* Sign a commit or tag buffer, storing the result in a header. */ -int sign_with_header(struct strbuf *buf, const char *keyid); /* Parse the signature out of a header. */ int parse_buffer_signed_by_header(const char *buffer, unsigned long size, -- 2.53.0 ^ permalink raw reply related [flat|nested] 60+ messages in thread
* Re: [PATCH 1/2] commit: remove unused forward declaration 2026-02-23 19:41 ` [PATCH 1/2] commit: remove unused forward declaration Justin Tobler @ 2026-02-24 9:35 ` Patrick Steinhardt 0 siblings, 0 replies; 60+ messages in thread From: Patrick Steinhardt @ 2026-02-24 9:35 UTC (permalink / raw) To: Justin Tobler; +Cc: git, sandals, christian.couder On Mon, Feb 23, 2026 at 01:41:45PM -0600, Justin Tobler wrote: > In 6206089cbd (commit: write commits for both hashes, 2023-10-01), > `sign_with_header()` was removed, but its forward declaration in > "commit.h" was left. Remove the unused declaration. Indeed, the definition of that function doesn't exist anymore. Patrick ^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH 2/2] fast-import: add mode to re-sign invalid commit signatures 2026-02-23 19:41 [PATCH 0/2] fast-import: add mode to re-sign invalid commit signatures Justin Tobler 2026-02-23 19:41 ` [PATCH 1/2] commit: remove unused forward declaration Justin Tobler @ 2026-02-23 19:41 ` Justin Tobler 2026-02-24 9:33 ` Patrick Steinhardt 2026-02-24 13:40 ` [PATCH 0/2] " Christian Couder ` (2 subsequent siblings) 4 siblings, 1 reply; 60+ messages in thread From: Justin Tobler @ 2026-02-23 19:41 UTC (permalink / raw) To: git; +Cc: sandals, christian.couder, Justin Tobler 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. Signed-off-by: Justin Tobler <jltobler@gmail.com> --- Documentation/git-fast-import.adoc | 3 + builtin/fast-export.c | 6 ++ builtin/fast-import.c | 43 +++++++-- gpg-interface.c | 2 + gpg-interface.h | 1 + t/t9305-fast-import-signatures.sh | 142 +++++++++++++++-------------- 6 files changed, 125 insertions(+), 72 deletions(-) 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. Options for Frontends ~~~~~~~~~~~~~~~~~~~~~ diff --git a/builtin/fast-export.c b/builtin/fast-export.c index 0c5d2386d8..76fad1dec5 100644 --- a/builtin/fast-export.c +++ b/builtin/fast-export.c @@ -825,6 +825,9 @@ static void handle_commit(struct commit *commit, struct rev_info *rev, case SIGN_STRIP_IF_INVALID: die(_("'strip-if-invalid' is not a valid mode for " "git fast-export with --signed-commits=<mode>")); + case SIGN_RESIGN_IF_INVALID: + die(_("'re-sign-if-invalid' is not a valid mode for " + "git fast-export with --signed-commits=<mode>")); default: BUG("invalid signed_commit_mode value %d", signed_commit_mode); } @@ -970,6 +973,9 @@ static void handle_tag(const char *name, struct tag *tag) case SIGN_STRIP_IF_INVALID: die(_("'strip-if-invalid' is not a valid mode for " "git fast-export with --signed-tags=<mode>")); + case SIGN_RESIGN_IF_INVALID: + die(_("'re-sign-if-invalid' is not a valid mode for " + "git fast-export with --signed-tags=<mode>")); default: BUG("invalid signed_commit_mode value %d", signed_commit_mode); } 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 }; @@ -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); + 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 + * 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. + */ + strbuf_addstr(&payload, signature_check.payload); + sign_buffer(&payload, &signature, key); + add_header_signature(new_data, &signature, the_hash_algo); + + strbuf_release(&signature); + strbuf_release(&payload); + free(key); + } + finalize_commit_buffer(new_data, NULL, NULL, msg); } else { strbuf_swap(new_data, &tmp_buf); @@ -2927,6 +2952,7 @@ static void parse_new_commit(const char *arg) /* fallthru */ case SIGN_VERBATIM: case SIGN_STRIP_IF_INVALID: + case SIGN_RESIGN_IF_INVALID: import_one_signature(&sig_sha1, &sig_sha256, v); break; @@ -3011,9 +3037,11 @@ static void parse_new_commit(const char *arg) "encoding %s\n", encoding); - if (signed_commit_mode == SIGN_STRIP_IF_INVALID && + if ((signed_commit_mode == SIGN_STRIP_IF_INVALID || + signed_commit_mode == SIGN_RESIGN_IF_INVALID) && (sig_sha1.hash_algo || sig_sha256.hash_algo)) - handle_strip_if_invalid(&new_data, &sig_sha1, &sig_sha256, &msg); + handle_invalid_signature(&new_data, &sig_sha1, &sig_sha256, &msg, + signed_commit_mode); else finalize_commit_buffer(&new_data, &sig_sha1, &sig_sha256, &msg); @@ -3060,6 +3088,9 @@ static void handle_tag_signature(struct strbuf *msg, const char *name) case SIGN_STRIP_IF_INVALID: die(_("'strip-if-invalid' is not a valid mode for " "git fast-import with --signed-tags=<mode>")); + case SIGN_RESIGN_IF_INVALID: + die(_("'re-sign-if-invalid' is not a valid mode for " + "git fast-import with --signed-tags=<mode>")); default: BUG("invalid signed_tag_mode value %d from tag '%s'", signed_tag_mode, name); 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; diff --git a/gpg-interface.h b/gpg-interface.h index 789d1ffac4..2ab2a21e1a 100644 --- a/gpg-interface.h +++ b/gpg-interface.h @@ -112,6 +112,7 @@ enum sign_mode { SIGN_WARN_STRIP, SIGN_STRIP, SIGN_STRIP_IF_INVALID, + SIGN_RESIGN_IF_INVALID, }; /* diff --git a/t/t9305-fast-import-signatures.sh b/t/t9305-fast-import-signatures.sh index 022dae02e4..b52fb75976 100755 --- a/t/t9305-fast-import-signatures.sh +++ b/t/t9305-fast-import-signatures.sh @@ -103,71 +103,81 @@ test_expect_success GPG 'strip both OpenPGP signatures with --signed-commits=war test_line_count = 2 out ' -test_expect_success GPG 'import commit with no signature with --signed-commits=strip-if-invalid' ' - git fast-export main >output && - git -C new fast-import --quiet --signed-commits=strip-if-invalid <output >log 2>&1 && - test_must_be_empty log -' - -test_expect_success GPG 'keep valid OpenPGP signature with --signed-commits=strip-if-invalid' ' - rm -rf new && - git init new && - - git fast-export --signed-commits=verbatim openpgp-signing >output && - git -C new fast-import --quiet --signed-commits=strip-if-invalid <output >log 2>&1 && - IMPORTED=$(git -C new rev-parse --verify refs/heads/openpgp-signing) && - test $OPENPGP_SIGNING = $IMPORTED && - git -C new cat-file commit "$IMPORTED" >actual && - test_grep -E "^gpgsig(-sha256)? " actual && - test_must_be_empty log -' - -test_expect_success GPG 'strip signature invalidated by message change with --signed-commits=strip-if-invalid' ' - rm -rf new && - git init new && - - git fast-export --signed-commits=verbatim openpgp-signing >output && - - # Change the commit message, which invalidates the signature. - # The commit message length should not change though, otherwise the - # corresponding `data <length>` command would have to be changed too. - sed "s/OpenPGP signed commit/OpenPGP forged commit/" output >modified && - - git -C new fast-import --quiet --signed-commits=strip-if-invalid <modified >log 2>&1 && - - IMPORTED=$(git -C new rev-parse --verify refs/heads/openpgp-signing) && - test $OPENPGP_SIGNING != $IMPORTED && - git -C new cat-file commit "$IMPORTED" >actual && - test_grep ! -E "^gpgsig" actual && - test_grep "stripping invalid signature" log -' - -test_expect_success GPGSM 'keep valid X.509 signature with --signed-commits=strip-if-invalid' ' - rm -rf new && - git init new && - - git fast-export --signed-commits=verbatim x509-signing >output && - git -C new fast-import --quiet --signed-commits=strip-if-invalid <output >log 2>&1 && - IMPORTED=$(git -C new rev-parse --verify refs/heads/x509-signing) && - test $X509_SIGNING = $IMPORTED && - git -C new cat-file commit "$IMPORTED" >actual && - test_grep -E "^gpgsig(-sha256)? " actual && - test_must_be_empty log -' - -test_expect_success GPGSSH 'keep valid SSH signature with --signed-commits=strip-if-invalid' ' - rm -rf new && - git init new && - - test_config -C new gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && - - git fast-export --signed-commits=verbatim ssh-signing >output && - git -C new fast-import --quiet --signed-commits=strip-if-invalid <output >log 2>&1 && - IMPORTED=$(git -C new rev-parse --verify refs/heads/ssh-signing) && - test $SSH_SIGNING = $IMPORTED && - git -C new cat-file commit "$IMPORTED" >actual && - test_grep -E "^gpgsig(-sha256)? " actual && - test_must_be_empty log -' +for mode in strip-if-invalid re-sign-if-invalid +do + test_expect_success GPG "import commit with no signature with --signed-commits=$mode" ' + git fast-export main >output && + git -C new fast-import --quiet --signed-commits=$mode <output >log 2>&1 && + test_must_be_empty log + ' + + test_expect_success GPG "keep valid OpenPGP signature with --signed-commits=$mode" ' + rm -rf new && + git init new && + + git fast-export --signed-commits=verbatim openpgp-signing >output && + git -C new fast-import --quiet --signed-commits=$mode <output >log 2>&1 && + IMPORTED=$(git -C new rev-parse --verify refs/heads/openpgp-signing) && + test $OPENPGP_SIGNING = $IMPORTED && + git -C new cat-file commit "$IMPORTED" >actual && + test_grep -E "^gpgsig(-sha256)? " actual && + test_must_be_empty log + ' + + test_expect_success GPG "strip signature invalidated by message change with --signed-commits=$mode" ' + rm -rf new && + git init new && + + git fast-export --signed-commits=verbatim openpgp-signing >output && + + # Change the commit message, which invalidates the signature. + # The commit message length should not change though, otherwise the + # corresponding `data <length>` command would have to be changed too. + sed "s/OpenPGP signed commit/OpenPGP forged commit/" output >modified && + + git -C new fast-import --quiet --signed-commits=$mode <modified >log 2>&1 && + + IMPORTED=$(git -C new rev-parse --verify refs/heads/openpgp-signing) && + test $OPENPGP_SIGNING != $IMPORTED && + git -C new cat-file commit "$IMPORTED" >actual && + test_grep "stripping invalid signature" log && + + if test "$mode" = strip-if-invalid + then + test_grep ! -E "^gpgsig" actual + else + test_grep -E "^gpgsig(-sha256)? " actual && + git -C new verify-commit "$IMPORTED" + fi + ' + + test_expect_success GPGSM "keep valid X.509 signature with --signed-commits=$mode" ' + rm -rf new && + git init new && + + git fast-export --signed-commits=verbatim x509-signing >output && + git -C new fast-import --quiet --signed-commits=$mode <output >log 2>&1 && + IMPORTED=$(git -C new rev-parse --verify refs/heads/x509-signing) && + test $X509_SIGNING = $IMPORTED && + git -C new cat-file commit "$IMPORTED" >actual && + test_grep -E "^gpgsig(-sha256)? " actual && + test_must_be_empty log + ' + + test_expect_success GPGSSH "keep valid SSH signature with --signed-commits=$mode" ' + rm -rf new && + git init new && + + test_config -C new gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && + + git fast-export --signed-commits=verbatim ssh-signing >output && + git -C new fast-import --quiet --signed-commits=$mode <output >log 2>&1 && + IMPORTED=$(git -C new rev-parse --verify refs/heads/ssh-signing) && + test $SSH_SIGNING = $IMPORTED && + git -C new cat-file commit "$IMPORTED" >actual && + test_grep -E "^gpgsig(-sha256)? " actual && + test_must_be_empty log + ' +done test_done -- 2.53.0 ^ permalink raw reply related [flat|nested] 60+ messages in thread
* Re: [PATCH 2/2] fast-import: add mode to re-sign invalid commit signatures 2026-02-23 19:41 ` [PATCH 2/2] fast-import: add mode to re-sign invalid commit signatures Justin Tobler @ 2026-02-24 9:33 ` Patrick Steinhardt 2026-02-24 18:33 ` Justin Tobler 0 siblings, 1 reply; 60+ messages in thread From: Patrick Steinhardt @ 2026-02-24 9:33 UTC (permalink / raw) To: Justin Tobler; +Cc: git, sandals, christian.couder 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. > 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. > 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? > + 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/? > + * 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. > 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 ^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH 2/2] fast-import: add mode to re-sign invalid commit signatures 2026-02-24 9:33 ` Patrick Steinhardt @ 2026-02-24 18:33 ` Justin Tobler 0 siblings, 0 replies; 60+ messages in thread From: Justin Tobler @ 2026-02-24 18:33 UTC (permalink / raw) To: Patrick Steinhardt; +Cc: git, sandals, christian.couder 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 ^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH 0/2] fast-import: add mode to re-sign invalid commit signatures 2026-02-23 19:41 [PATCH 0/2] fast-import: add mode to re-sign invalid commit signatures Justin Tobler 2026-02-23 19:41 ` [PATCH 1/2] commit: remove unused forward declaration Justin Tobler 2026-02-23 19:41 ` [PATCH 2/2] fast-import: add mode to re-sign invalid commit signatures Justin Tobler @ 2026-02-24 13:40 ` Christian Couder 2026-02-24 22:41 ` brian m. carlson 2026-03-06 20:53 ` [PATCH v2 0/3] " Justin Tobler 4 siblings, 0 replies; 60+ messages in thread From: Christian Couder @ 2026-02-24 13:40 UTC (permalink / raw) To: Justin Tobler; +Cc: git, sandals Hi, On Mon, Feb 23, 2026 at 8:42 PM Justin Tobler <jltobler@gmail.com> wrote: > > Greetings, > > With c20f112e51 (fast-import: add 'strip-if-invalid' mode to > --signed-commits=<mode>, 2025-11-17), it became possible to remove > invalid signatures from commits via git-fast-import(1) while maintaining > valid commits. Building upon this functionality, a user may want to s/valid commits/valid commit signatures/ > re-sign these invalid commit signatures. This series introduces the > `re-sign-if-invalid` mode to do so accordingly. > > The newly added mode in this series currently ignores > `extensions.compatObjectFormat` when generating the new signatures. From > my understanding, to generate the compatability structure would also Here and below: s/compatability/compatibility/ > require us to reconstruct the compatability object for the object being > signed. I think this would be possible to do, but would require getting > the mapped OIDs for the commit parents and tree. I'm not competely sure s/competely/completely/ > of a good way to go about this yet though. I'm also not completely > certain if this is something that should be adressed as part of this s/adressed/addressed/ > series, or could be done later down the road. So for now I've opted to > delay its implementation. I'm open going down the other route if that is > preferred though. That's a reasonable approach to me. Thanks for taking over this. ^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH 0/2] fast-import: add mode to re-sign invalid commit signatures 2026-02-23 19:41 [PATCH 0/2] fast-import: add mode to re-sign invalid commit signatures Justin Tobler ` (2 preceding siblings ...) 2026-02-24 13:40 ` [PATCH 0/2] " Christian Couder @ 2026-02-24 22:41 ` brian m. carlson 2026-02-24 22:45 ` Junio C Hamano 2026-03-02 22:49 ` Justin Tobler 2026-03-06 20:53 ` [PATCH v2 0/3] " Justin Tobler 4 siblings, 2 replies; 60+ messages in thread From: brian m. carlson @ 2026-02-24 22:41 UTC (permalink / raw) To: Justin Tobler; +Cc: git, christian.couder [-- Attachment #1: Type: text/plain, Size: 2760 bytes --] On 2026-02-23 at 19:41:44, Justin Tobler wrote: > Greetings, > > With c20f112e51 (fast-import: add 'strip-if-invalid' mode to > --signed-commits=<mode>, 2025-11-17), it became possible to remove > invalid signatures from commits via git-fast-import(1) while maintaining > valid commits. Building upon this functionality, a user may want to > re-sign these invalid commit signatures. This series introduces the > `re-sign-if-invalid` mode to do so accordingly. > > The newly added mode in this series currently ignores > `extensions.compatObjectFormat` when generating the new signatures. From > my understanding, to generate the compatability structure would also > require us to reconstruct the compatability object for the object being > signed. I think this would be possible to do, but would require getting > the mapped OIDs for the commit parents and tree. I'm not competely sure > of a good way to go about this yet though. I'm also not completely > certain if this is something that should be adressed as part of this > series, or could be done later down the road. So for now I've opted to > delay its implementation. I'm open going down the other route if that is > preferred though. There's an API for converting object IDs to another algorithm: `repo_oid_to_algop`. If you want to convert a non-blob object, there's `convert_object_file`, which will serialize the object in the other format (blobs are invariant in the hash algorithm transformation, so converting them is not necessary). Those are present right now in the codebase and using them would be a good idea. If you want to test your code in interoperability mode, you can rebase onto the `sha256-interop` branch of https://github.com/bk2204/git.git and run with `GIT_TEST_DEFAULT_HASH=sha256:sha1`. If you're _not_ going to implement that in interoperability mode, then I'd rather you just die in that case so that the test fails and then I or someone else will fix it. `extensions.compatObjectFormat` is presently experimental and the data formats will change, so nobody should be relying on it working as it stands right now. There _will_ be more compatibility breakage coming in future series, for instance. I _would_ recommend regardless that you add a test like in t7004's "signed tag with embedded PGP message" if you apply this to tags as well as commits. That requires a special case in our interoperability code (since it normally converts things that look like signatures, but when we're _generating_ a tag, we don't want to do that since there are no signatures yet) and making sure we do the same thing in fast-import will avoid corruption in our conversions. -- brian m. carlson (they/them) Toronto, Ontario, CA [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 262 bytes --] ^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH 0/2] fast-import: add mode to re-sign invalid commit signatures 2026-02-24 22:41 ` brian m. carlson @ 2026-02-24 22:45 ` Junio C Hamano 2026-03-02 22:49 ` Justin Tobler 1 sibling, 0 replies; 60+ messages in thread From: Junio C Hamano @ 2026-02-24 22:45 UTC (permalink / raw) To: brian m. carlson; +Cc: Justin Tobler, git, christian.couder "brian m. carlson" <sandals@crustytoothpaste.net> writes: > If you're _not_ going to implement that in interoperability mode, then > I'd rather you just die in that case so that the test fails and then I > or someone else will fix it. `extensions.compatObjectFormat` is > presently experimental and the data formats will change, so nobody > should be relying on it working as it stands right now. There _will_ be > more compatibility breakage coming in future series, for instance. It sounds like a very prudent thing to do to die as unsupported. Thanks! > I _would_ recommend regardless that you add a test like in t7004's > "signed tag with embedded PGP message" if you apply this to tags as well > as commits. That requires a special case in our interoperability code > (since it normally converts things that look like signatures, but when > we're _generating_ a tag, we don't want to do that since there are no > signatures yet) and making sure we do the same thing in fast-import will > avoid corruption in our conversions. ^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH 0/2] fast-import: add mode to re-sign invalid commit signatures 2026-02-24 22:41 ` brian m. carlson 2026-02-24 22:45 ` Junio C Hamano @ 2026-03-02 22:49 ` Justin Tobler 1 sibling, 0 replies; 60+ messages in thread From: Justin Tobler @ 2026-03-02 22:49 UTC (permalink / raw) To: brian m. carlson, git, christian.couder On 26/02/24 10:41PM, brian m. carlson wrote: > If you're _not_ going to implement that in interoperability mode, then > I'd rather you just die in that case so that the test fails and then I > or someone else will fix it. `extensions.compatObjectFormat` is > presently experimental and the data formats will change, so nobody > should be relying on it working as it stands right now. There _will_ be > more compatibility breakage coming in future series, for instance. That sounds very sensible. In the next version I'll update to instead die() as unsupported if we attempt to re-sign commit signatures in interoperability mode. > I _would_ recommend regardless that you add a test like in t7004's > "signed tag with embedded PGP message" if you apply this to tags as well > as commits. That requires a special case in our interoperability code > (since it normally converts things that look like signatures, but when > we're _generating_ a tag, we don't want to do that since there are no > signatures yet) and making sure we do the same thing in fast-import will > avoid corruption in our conversions. Thanks, I'll look into this. This patch series currently only applies this new mode to commits, but I plan to tackle tag signatures in a separate followup series. Thanks, -Justin ^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH v2 0/3] fast-import: add mode to re-sign invalid commit signatures 2026-02-23 19:41 [PATCH 0/2] fast-import: add mode to re-sign invalid commit signatures Justin Tobler ` (3 preceding siblings ...) 2026-02-24 22:41 ` brian m. carlson @ 2026-03-06 20:53 ` Justin Tobler 2026-03-06 20:53 ` [PATCH v2 1/3] commit: remove unused forward declaration Justin Tobler ` (3 more replies) 4 siblings, 4 replies; 60+ messages in thread From: Justin Tobler @ 2026-03-06 20:53 UTC (permalink / raw) To: git; +Cc: sandals, christian.couder, ps, gister, Justin Tobler Greetings, With c20f112e51 (fast-import: add 'strip-if-invalid' mode to --signed-commits=<mode>, 2025-11-17), it became possible to remove invalid signatures from commits via git-fast-import(1) while maintaining valid commit signatures. Building upon this functionality, a user may want to re-sign these invalid commit signatures. This series introduces the `re-sign-if-invalid` mode to do so accordingly. The newly added mode in this series currently ignores `extensions.compatObjectFormat` when generating the new signatures. From my understanding, to generate the compatibility structure would also require us to reconstruct the compatibility object for the object being signed. I think this would be possible to do, but would require getting the mapped OIDs for the commit parents and tree. I'm not completely sure of a good way to go about this yet though. I'm also not completely certain if this is something that should be addressed as part of this series, or could be done later down the road. So for now I've opted to delay its implementation. I'm open going down the other route if that is preferred though. The first commit is a simple cleanup for something I noticed while reading though commit signing code. The second commit actually introduces the new `--signed-commits` mode. Changes since V1: - Improved commit messages and comments to better explain why interoperability mode is not currently supported. - Clarified documentation for re-sign-if-invalid mode. - Renamed `handle_invalid_signature()` to `handle_signature_if_invalid()`. - Added warning messages specific to commit resigning. - Fixed some small typos. - Added support for explicitly specifying the signing key ID via `--signed-commits=re-sign-if-invalid[=<keyid>]` similar to how it can specified in git-commit(1). - We now die() as unsupported when attempting to re-sign an invalid commit signature in interoperability mode. - We now die() when failing to re-sign a commit. Thanks, -Justin Justin Tobler (3): commit: remove unused forward declaration gpg-interface: introduce sign_buffer_with_key() fast-import: add mode to re-sign invalid commit signatures Documentation/git-fast-import.adoc | 4 + builtin/fast-export.c | 8 +- builtin/fast-import.c | 79 ++++++++++++---- commit.c | 16 +--- commit.h | 2 - gpg-interface.c | 36 ++++++-- gpg-interface.h | 14 ++- t/t9305-fast-import-signatures.sh | 140 ++++++++++++++++++----------- 8 files changed, 205 insertions(+), 94 deletions(-) Range-diff against v1: 1: 0d00b72ee0 = 1: 0d00b72ee0 commit: remove unused forward declaration -: ---------- > 2: 499025532c gpg-interface: introduce sign_buffer_with_key() 2: 16e4022616 ! 3: bea1a42eb9 fast-import: add mode to re-sign invalid commit signatures @@ Commit message 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. + to just stripping the signature. To facilitate this, introduce a + "re-sign-if-invalid" mode for the `--signed-commits` option. Optionally, + a key ID may be explicitly provided in the form + `re-sign-if-invalid[=<keyid>]` to specify which signing key should be + used when re-signing invalid commit signatures. - 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. + Note that to properly support interoperability mode when re-signing + commit signatures, the commit buffer must be created in both the + repository and compatability object formats to generate the appropriate + signatures accordingly. As currently implemented, the commit buffer for + the compatability object format is not reconstructed and thus re-signing + commits in interoperability mode is not yet supported. Support may be + added in the future. Signed-off-by: Justin Tobler <jltobler@gmail.com> @@ Documentation/git-fast-import.adoc: 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. ++* `re-sign-if-invalid[=<keyid>]`, similar to `strip-if-invalid`, verifies ++ commit signatures and replaces invalid signatures with newly created ones. ++ Valid signatures are left unchanged. If `<keyid>` is provided, that key is ++ used for re-signing; otherwise the configured default signing key is used. Options for Frontends ~~~~~~~~~~~~~~~~~~~~~ ## builtin/fast-export.c ## +@@ builtin/fast-export.c: static int parse_opt_sign_mode(const struct option *opt, + if (unset) + return 0; + +- if (parse_sign_mode(arg, val)) ++ if (parse_sign_mode(arg, val, NULL)) + return error(_("unknown %s mode: %s"), opt->long_name, arg); + + return 0; @@ builtin/fast-export.c: static void handle_commit(struct commit *commit, struct rev_info *rev, case SIGN_STRIP_IF_INVALID: die(_("'strip-if-invalid' is not a valid mode for " @@ builtin/fast-export.c: static void handle_tag(const char *name, struct tag *tag) } ## builtin/fast-import.c ## +@@ builtin/fast-import.c: static const char *global_prefix; + + static enum sign_mode signed_tag_mode = SIGN_VERBATIM; + static enum sign_mode signed_commit_mode = SIGN_VERBATIM; ++static const char *signed_commit_keyid; + + /* Memory pools */ + static struct mem_pool fi_mem_pool = { @@ builtin/fast-import.c: static void finalize_commit_buffer(struct strbuf *new_data, strbuf_addbuf(new_data, msg); } @@ builtin/fast-import.c: static void finalize_commit_buffer(struct strbuf *new_dat - 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) ++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 }; @@ builtin/fast-import.c: static void handle_strip_if_invalid(struct strbuf *new_data, - warning(_("stripping invalid signature for commit\n" - " allegedly by %s"), signer); + const char *subject; + int subject_len = find_commit_subject(msg->buf, &subject); -+ if (mode == SIGN_RESIGN_IF_INVALID) { +- if (subject_len > 100) +- warning(_("stripping invalid signature for commit '%.100s...'\n" +- " allegedly by %s"), subject, signer); +- else if (subject_len > 0) +- warning(_("stripping invalid signature for commit '%.*s'\n" +- " allegedly by %s"), subject_len, subject, signer); +- else +- warning(_("stripping invalid signature for commit\n" +- " allegedly by %s"), signer); ++ if (mode == SIGN_STRIP_IF_INVALID) { ++ if (subject_len > 100) ++ warning(_("stripping invalid signature for commit '%.100s...'\n" ++ " allegedly by %s"), subject, signer); ++ else if (subject_len > 0) ++ warning(_("stripping invalid signature for commit '%.*s'\n" ++ " allegedly by %s"), subject_len, subject, signer); ++ else ++ warning(_("stripping invalid signature for commit\n" ++ " allegedly by %s"), signer); ++ } else if (mode == SIGN_RESIGN_IF_INVALID) { + struct strbuf signature = STRBUF_INIT; + struct strbuf payload = STRBUF_INIT; -+ char *key = get_signing_key(); ++ ++ if (subject_len > 100) ++ warning(_("re-signing invalid signature for commit '%.100s...'\n" ++ " allegedly by %s"), subject, signer); ++ else if (subject_len > 0) ++ warning(_("re-signing invalid signature for commit '%.*s'\n" ++ " allegedly by %s"), subject_len, subject, signer); ++ else ++ warning(_("re-signing invalid signature for commit\n" ++ " allegedly by %s"), signer); + + /* -+ * Commits are resigned using the repository object -+ * 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. ++ * NEEDSWORK: To properly support interoperability mode ++ * when re-signing commit signatures, the commit buffer ++ * must be provided in both the repository and ++ * compatability object formats. As currently ++ * implemented, only the repository object format is ++ * considered meaning compatability signatures cannot be ++ * generated. Thus, attempting to re-sign commit ++ * signatures in interoperability mode is currently ++ * unsupported. + */ ++ if (the_repository->compat_hash_algo) ++ die(_("re-signing signatures in interoperability mode is unsupported")); ++ + strbuf_addstr(&payload, signature_check.payload); -+ sign_buffer(&payload, &signature, key); ++ if (sign_buffer_with_key(&payload, &signature, signed_commit_keyid)) ++ die(_("failed to sign commit object")); + add_header_signature(new_data, &signature, the_hash_algo); + + strbuf_release(&signature); + strbuf_release(&payload); -+ free(key); + } -+ + finalize_commit_buffer(new_data, NULL, NULL, msg); } else { - strbuf_swap(new_data, &tmp_buf); @@ builtin/fast-import.c: static void parse_new_commit(const char *arg) /* fallthru */ case SIGN_VERBATIM: @@ builtin/fast-import.c: static void parse_new_commit(const char *arg) + signed_commit_mode == SIGN_RESIGN_IF_INVALID) && (sig_sha1.hash_algo || sig_sha256.hash_algo)) - handle_strip_if_invalid(&new_data, &sig_sha1, &sig_sha256, &msg); -+ handle_invalid_signature(&new_data, &sig_sha1, &sig_sha256, &msg, -+ signed_commit_mode); ++ handle_signature_if_invalid(&new_data, &sig_sha1, &sig_sha256, ++ &msg, signed_commit_mode); else finalize_commit_buffer(&new_data, &sig_sha1, &sig_sha256, &msg); @@ builtin/fast-import.c: static void handle_tag_signature(struct strbuf *msg, cons default: BUG("invalid signed_tag_mode value %d from tag '%s'", signed_tag_mode, name); +@@ builtin/fast-import.c: static int parse_one_option(const char *option) + } else if (skip_prefix(option, "export-pack-edges=", &option)) { + option_export_pack_edges(option); + } else if (skip_prefix(option, "signed-commits=", &option)) { +- if (parse_sign_mode(option, &signed_commit_mode)) ++ if (parse_sign_mode(option, &signed_commit_mode, &signed_commit_keyid)) + usagef(_("unknown --signed-commits mode '%s'"), option); + } else if (skip_prefix(option, "signed-tags=", &option)) { +- if (parse_sign_mode(option, &signed_tag_mode)) ++ if (parse_sign_mode(option, &signed_tag_mode, NULL)) + usagef(_("unknown --signed-tags mode '%s'"), option); + } else if (!strcmp(option, "quiet")) { + show_stats = 0; ## gpg-interface.c ## -@@ gpg-interface.c: int parse_sign_mode(const char *arg, enum sign_mode *mode) +@@ gpg-interface.c: static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature, + return ret; + } + +-int parse_sign_mode(const char *arg, enum sign_mode *mode) ++int parse_sign_mode(const char *arg, enum sign_mode *mode, const char **keyid) + { +- if (!strcmp(arg, "abort")) ++ if (!strcmp(arg, "abort")) { + *mode = SIGN_ABORT; +- else if (!strcmp(arg, "verbatim") || !strcmp(arg, "ignore")) ++ } else if (!strcmp(arg, "verbatim") || !strcmp(arg, "ignore")) { + *mode = SIGN_VERBATIM; +- else if (!strcmp(arg, "warn-verbatim") || !strcmp(arg, "warn")) ++ } else if (!strcmp(arg, "warn-verbatim") || !strcmp(arg, "warn")) { + *mode = SIGN_WARN_VERBATIM; +- else if (!strcmp(arg, "warn-strip")) ++ } else if (!strcmp(arg, "warn-strip")) { + *mode = SIGN_WARN_STRIP; +- else if (!strcmp(arg, "strip")) ++ } else if (!strcmp(arg, "strip")) { *mode = SIGN_STRIP; - else if (!strcmp(arg, "strip-if-invalid")) +- else if (!strcmp(arg, "strip-if-invalid")) ++ } else if (!strcmp(arg, "strip-if-invalid")) { *mode = SIGN_STRIP_IF_INVALID; -+ else if (!strcmp(arg, "re-sign-if-invalid")) +- else ++ } else if (!strcmp(arg, "re-sign-if-invalid")) { + *mode = SIGN_RESIGN_IF_INVALID; - else ++ } else if (skip_prefix(arg, "re-sign-if-invalid=", &arg)) { ++ *mode = SIGN_RESIGN_IF_INVALID; ++ if (keyid) ++ *keyid = arg; ++ } else { return -1; ++ } return 0; + } ## gpg-interface.h ## @@ gpg-interface.h: enum sign_mode { @@ gpg-interface.h: enum sign_mode { }; /* + * Return 0 if `arg` can be parsed into an `enum sign_mode`. Return -1 +- * otherwise. ++ * otherwise. If the parsed mode is SIGN_RESIGN_IF_INVALID and GPG key provided ++ * in the arguments in the form `re-sign-if-invalid=<keyid>`, the key-ID is ++ * parsed into `char **keyid`. + */ +-int parse_sign_mode(const char *arg, enum sign_mode *mode); ++int parse_sign_mode(const char *arg, enum sign_mode *mode, const char **keyid); + + #endif ## t/t9305-fast-import-signatures.sh ## @@ t/t9305-fast-import-signatures.sh: test_expect_success GPG 'strip both OpenPGP signatures with --signed-commits=war @@ t/t9305-fast-import-signatures.sh: test_expect_success GPG 'strip both OpenPGP s -' - -test_expect_success GPG 'strip signature invalidated by message change with --signed-commits=strip-if-invalid' ' -- rm -rf new && -- git init new && -- -- git fast-export --signed-commits=verbatim openpgp-signing >output && -- -- # Change the commit message, which invalidates the signature. -- # The commit message length should not change though, otherwise the -- # corresponding `data <length>` command would have to be changed too. -- sed "s/OpenPGP signed commit/OpenPGP forged commit/" output >modified && -- -- git -C new fast-import --quiet --signed-commits=strip-if-invalid <modified >log 2>&1 && -- -- IMPORTED=$(git -C new rev-parse --verify refs/heads/openpgp-signing) && -- test $OPENPGP_SIGNING != $IMPORTED && -- git -C new cat-file commit "$IMPORTED" >actual && -- test_grep ! -E "^gpgsig" actual && -- test_grep "stripping invalid signature" log --' -- --test_expect_success GPGSM 'keep valid X.509 signature with --signed-commits=strip-if-invalid' ' -- rm -rf new && -- git init new && -- -- git fast-export --signed-commits=verbatim x509-signing >output && -- git -C new fast-import --quiet --signed-commits=strip-if-invalid <output >log 2>&1 && -- IMPORTED=$(git -C new rev-parse --verify refs/heads/x509-signing) && -- test $X509_SIGNING = $IMPORTED && -- git -C new cat-file commit "$IMPORTED" >actual && -- test_grep -E "^gpgsig(-sha256)? " actual && -- test_must_be_empty log --' -- --test_expect_success GPGSSH 'keep valid SSH signature with --signed-commits=strip-if-invalid' ' -- rm -rf new && -- git init new && -- -- test_config -C new gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && -- -- git fast-export --signed-commits=verbatim ssh-signing >output && -- git -C new fast-import --quiet --signed-commits=strip-if-invalid <output >log 2>&1 && -- IMPORTED=$(git -C new rev-parse --verify refs/heads/ssh-signing) && -- test $SSH_SIGNING = $IMPORTED && -- git -C new cat-file commit "$IMPORTED" >actual && -- test_grep -E "^gpgsig(-sha256)? " actual && -- test_must_be_empty log --' +for mode in strip-if-invalid re-sign-if-invalid +do + test_expect_success GPG "import commit with no signature with --signed-commits=$mode" ' @@ t/t9305-fast-import-signatures.sh: test_expect_success GPG 'strip both OpenPGP s + test_must_be_empty log + ' + -+ test_expect_success GPG "strip signature invalidated by message change with --signed-commits=$mode" ' ++ test_expect_success GPG "handle signature invalidated by message change with --signed-commits=$mode" ' + rm -rf new && + git init new && + @@ t/t9305-fast-import-signatures.sh: test_expect_success GPG 'strip both OpenPGP s + IMPORTED=$(git -C new rev-parse --verify refs/heads/openpgp-signing) && + test $OPENPGP_SIGNING != $IMPORTED && + git -C new cat-file commit "$IMPORTED" >actual && -+ test_grep "stripping invalid signature" log && + + if test "$mode" = strip-if-invalid + then ++ test_grep "stripping invalid signature" log && + test_grep ! -E "^gpgsig" actual + else ++ test_grep "re-signing invalid signature" log && + test_grep -E "^gpgsig(-sha256)? " actual && + git -C new verify-commit "$IMPORTED" + fi @@ t/t9305-fast-import-signatures.sh: test_expect_success GPG 'strip both OpenPGP s + test_must_be_empty log + ' +done ++ ++test_expect_success GPGSSH "re-sign invalid commit with explicit keyid" ' + rm -rf new && + git init new && + +@@ t/t9305-fast-import-signatures.sh: test_expect_success GPG 'strip signature invalidated by message change with --si + # corresponding `data <length>` command would have to be changed too. + sed "s/OpenPGP signed commit/OpenPGP forged commit/" output >modified && + +- git -C new fast-import --quiet --signed-commits=strip-if-invalid <modified >log 2>&1 && ++ # Configure the target repository with an invalid default signing key. ++ test_config -C new user.signingkey "not-a-real-key-id" && ++ test_config -C new gpg.format ssh && ++ test_config -C new gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && ++ test_must_fail git -C new fast-import --quiet \ ++ --signed-commits=re-sign-if-invalid <modified >/dev/null 2>&1 && ++ ++ # Import using explicitly provided signing key. ++ git -C new fast-import --quiet \ ++ --signed-commits=re-sign-if-invalid="${GPGSSH_KEY_PRIMARY}" <modified && + + IMPORTED=$(git -C new rev-parse --verify refs/heads/openpgp-signing) && + test $OPENPGP_SIGNING != $IMPORTED && + git -C new cat-file commit "$IMPORTED" >actual && +- test_grep ! -E "^gpgsig" actual && +- test_grep "stripping invalid signature" log +-' +- +-test_expect_success GPGSM 'keep valid X.509 signature with --signed-commits=strip-if-invalid' ' +- rm -rf new && +- git init new && +- +- git fast-export --signed-commits=verbatim x509-signing >output && +- git -C new fast-import --quiet --signed-commits=strip-if-invalid <output >log 2>&1 && +- IMPORTED=$(git -C new rev-parse --verify refs/heads/x509-signing) && +- test $X509_SIGNING = $IMPORTED && +- git -C new cat-file commit "$IMPORTED" >actual && + test_grep -E "^gpgsig(-sha256)? " actual && +- test_must_be_empty log +-' +- +-test_expect_success GPGSSH 'keep valid SSH signature with --signed-commits=strip-if-invalid' ' +- rm -rf new && +- git init new && +- +- test_config -C new gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && +- +- git fast-export --signed-commits=verbatim ssh-signing >output && +- git -C new fast-import --quiet --signed-commits=strip-if-invalid <output >log 2>&1 && +- IMPORTED=$(git -C new rev-parse --verify refs/heads/ssh-signing) && +- test $SSH_SIGNING = $IMPORTED && +- git -C new cat-file commit "$IMPORTED" >actual && +- test_grep -E "^gpgsig(-sha256)? " actual && +- test_must_be_empty log ++ git -C new verify-commit "$IMPORTED" + ' test_done base-commit: 7c02d39fc2ed2702223c7674f73150d9a7e61ba4 -- 2.53.0.381.g628a66ccf6 ^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH v2 1/3] commit: remove unused forward declaration 2026-03-06 20:53 ` [PATCH v2 0/3] " Justin Tobler @ 2026-03-06 20:53 ` Justin Tobler 2026-03-06 20:53 ` [PATCH v2 2/3] gpg-interface: introduce sign_buffer_with_key() Justin Tobler ` (2 subsequent siblings) 3 siblings, 0 replies; 60+ messages in thread From: Justin Tobler @ 2026-03-06 20:53 UTC (permalink / raw) To: git; +Cc: sandals, christian.couder, ps, gister, Justin Tobler In 6206089cbd (commit: write commits for both hashes, 2023-10-01), `sign_with_header()` was removed, but its forward declaration in "commit.h" was left. Remove the unused declaration. Signed-off-by: Justin Tobler <jltobler@gmail.com> --- commit.h | 2 -- 1 file changed, 2 deletions(-) diff --git a/commit.h b/commit.h index 1635de418b..f0c38cb444 100644 --- a/commit.h +++ b/commit.h @@ -390,8 +390,6 @@ LAST_ARG_MUST_BE_NULL int run_commit_hook(int editor_is_used, const char *index_file, int *invoked_hook, const char *name, ...); -/* Sign a commit or tag buffer, storing the result in a header. */ -int sign_with_header(struct strbuf *buf, const char *keyid); /* Parse the signature out of a header. */ int parse_buffer_signed_by_header(const char *buffer, unsigned long size, -- 2.53.0.381.g628a66ccf6 ^ permalink raw reply related [flat|nested] 60+ messages in thread
* [PATCH v2 2/3] gpg-interface: introduce sign_buffer_with_key() 2026-03-06 20:53 ` [PATCH v2 0/3] " Justin Tobler 2026-03-06 20:53 ` [PATCH v2 1/3] commit: remove unused forward declaration Justin Tobler @ 2026-03-06 20:53 ` Justin Tobler 2026-03-10 9:01 ` Christian Couder 2026-03-06 20:53 ` [PATCH v2 3/3] fast-import: add mode to re-sign invalid commit signatures Justin Tobler 2026-03-10 20:11 ` [PATCH v3 0/3] " Justin Tobler 3 siblings, 1 reply; 60+ messages in thread From: Justin Tobler @ 2026-03-06 20:53 UTC (permalink / raw) To: git; +Cc: sandals, christian.couder, ps, gister, Justin Tobler The `sign_commit_to_strbuf()` helper in "commit.c" provides fallback logic to get the default configured signing key when a key is not provided and handles generating the commit signature accordingly. This signing operation is not really specific to commits as any arbitrary buffer can be signed. Also, in a subsequent commit, this same logic is reused by git-fast-import(1) when resigning invalid commit signatures. Introduce `sign_buffer_with_key()` to centralize signing key resolution in gpg-interface to allow callers to reuse the same behavior without duplicating logic. Signed-off-by: Justin Tobler <jltobler@gmail.com> --- commit.c | 16 ++-------------- gpg-interface.c | 13 +++++++++++++ gpg-interface.h | 7 +++++++ 3 files changed, 22 insertions(+), 14 deletions(-) diff --git a/commit.c b/commit.c index d16ae73345..1677b1ef25 100644 --- a/commit.c +++ b/commit.c @@ -1148,18 +1148,6 @@ int add_header_signature(struct strbuf *buf, struct strbuf *sig, const struct gi return 0; } -static int sign_commit_to_strbuf(struct strbuf *sig, struct strbuf *buf, const char *keyid) -{ - char *keyid_to_free = NULL; - int ret = 0; - if (!keyid || !*keyid) - keyid = keyid_to_free = get_signing_key(); - if (sign_buffer(buf, sig, keyid)) - ret = -1; - free(keyid_to_free); - return ret; -} - int parse_signed_commit(const struct commit *commit, struct strbuf *payload, struct strbuf *signature, const struct git_hash_algo *algop) @@ -1737,7 +1725,7 @@ int commit_tree_extended(const char *msg, size_t msg_len, oidcpy(&parent_buf[i++], &p->item->object.oid); write_commit_tree(&buffer, msg, msg_len, tree, parent_buf, nparents, author, committer, extra); - if (sign_commit && sign_commit_to_strbuf(&sig, &buffer, sign_commit)) { + if (sign_commit && sign_buffer_with_key(&buffer, &sig, sign_commit)) { result = -1; goto out; } @@ -1769,7 +1757,7 @@ int commit_tree_extended(const char *msg, size_t msg_len, free_commit_extra_headers(compat_extra); free(mapped_parents); - if (sign_commit && sign_commit_to_strbuf(&compat_sig, &compat_buffer, sign_commit)) { + if (sign_commit && sign_buffer_with_key(&compat_buffer, &compat_sig, sign_commit)) { result = -1; goto out; } diff --git a/gpg-interface.c b/gpg-interface.c index 87fb6605fb..a72fa35061 100644 --- a/gpg-interface.c +++ b/gpg-interface.c @@ -980,6 +980,19 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *sig return use_format->sign_buffer(buffer, signature, signing_key); } +int sign_buffer_with_key(struct strbuf *buffer, struct strbuf *signature, + const char *signing_key) +{ + char *keyid_to_free = NULL; + int ret = 0; + if (!signing_key || !*signing_key) + signing_key = keyid_to_free = get_signing_key(); + if (sign_buffer(buffer, signature, signing_key)) + ret = -1; + free(keyid_to_free); + return ret; +} + /* * Strip CR from the line endings, in case we are on Windows. * NEEDSWORK: make it trim only CRs before LFs and rename diff --git a/gpg-interface.h b/gpg-interface.h index 789d1ffac4..a32741aeda 100644 --- a/gpg-interface.h +++ b/gpg-interface.h @@ -83,6 +83,13 @@ size_t parse_signed_buffer(const char *buf, size_t size); int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *signing_key); +/* + * Similar to `sign_buffer()`, but uses the default configured signing key as + * returned by `get_signing_key()` when the provided "signing_key" is NULL or + * empty. Returns 0 on success, non-zero on failure. + */ +int sign_buffer_with_key(struct strbuf *buffer, struct strbuf *signature, + const char *signing_key); /* * Returns corresponding string in lowercase for a given member of -- 2.53.0.381.g628a66ccf6 ^ permalink raw reply related [flat|nested] 60+ messages in thread
* Re: [PATCH v2 2/3] gpg-interface: introduce sign_buffer_with_key() 2026-03-06 20:53 ` [PATCH v2 2/3] gpg-interface: introduce sign_buffer_with_key() Justin Tobler @ 2026-03-10 9:01 ` Christian Couder 2026-03-10 18:04 ` Justin Tobler 0 siblings, 1 reply; 60+ messages in thread From: Christian Couder @ 2026-03-10 9:01 UTC (permalink / raw) To: Justin Tobler; +Cc: git, sandals, ps, gister On Fri, Mar 6, 2026 at 9:54 PM Justin Tobler <jltobler@gmail.com> wrote: > > The `sign_commit_to_strbuf()` helper in "commit.c" provides fallback > logic to get the default configured signing key when a key is not > provided and handles generating the commit signature accordingly. This > signing operation is not really specific to commits as any arbitrary > buffer can be signed. Also, in a subsequent commit, this same logic is > reused by git-fast-import(1) when resigning invalid commit signatures. Nit: s/resigning/re-signing/ > Introduce `sign_buffer_with_key()` to centralize signing key resolution > in gpg-interface to allow callers to reuse the same behavior without > duplicating logic. Nit: I think it would be a bit clearer if the change was described as: - moving the `sign_commit_to_strbuf()` helper from "commit.c" to "gpg-interface.c", - renaming it to `sign_buffer_with_key()`, and - exporting it (so it can later be used by both "commit.c" and "builtin/fast-import.c"). Or did I miss something? ^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH v2 2/3] gpg-interface: introduce sign_buffer_with_key() 2026-03-10 9:01 ` Christian Couder @ 2026-03-10 18:04 ` Justin Tobler 0 siblings, 0 replies; 60+ messages in thread From: Justin Tobler @ 2026-03-10 18:04 UTC (permalink / raw) To: Christian Couder; +Cc: git, sandals, ps On 26/03/10 10:01AM, Christian Couder wrote: > On Fri, Mar 6, 2026 at 9:54 PM Justin Tobler <jltobler@gmail.com> wrote: > > > > The `sign_commit_to_strbuf()` helper in "commit.c" provides fallback > > logic to get the default configured signing key when a key is not > > provided and handles generating the commit signature accordingly. This > > signing operation is not really specific to commits as any arbitrary > > buffer can be signed. Also, in a subsequent commit, this same logic is > > reused by git-fast-import(1) when resigning invalid commit signatures. > > Nit: s/resigning/re-signing/ Will fix. > > Introduce `sign_buffer_with_key()` to centralize signing key resolution > > in gpg-interface to allow callers to reuse the same behavior without > > duplicating logic. > > Nit: I think it would be a bit clearer if the change was described as: > > - moving the `sign_commit_to_strbuf()` helper from "commit.c" to > "gpg-interface.c", > - renaming it to `sign_buffer_with_key()`, and > - exporting it (so it can later be used by both "commit.c" and > "builtin/fast-import.c"). > > Or did I miss something? That's correct. I'll update the commit message in the next version to try to be a bit more clear here. Thanks. -Justin ^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH v2 3/3] fast-import: add mode to re-sign invalid commit signatures 2026-03-06 20:53 ` [PATCH v2 0/3] " Justin Tobler 2026-03-06 20:53 ` [PATCH v2 1/3] commit: remove unused forward declaration Justin Tobler 2026-03-06 20:53 ` [PATCH v2 2/3] gpg-interface: introduce sign_buffer_with_key() Justin Tobler @ 2026-03-06 20:53 ` Justin Tobler 2026-03-10 9:27 ` Christian Couder 2026-03-10 20:11 ` [PATCH v3 0/3] " Justin Tobler 3 siblings, 1 reply; 60+ messages in thread From: Justin Tobler @ 2026-03-06 20:53 UTC (permalink / raw) To: git; +Cc: sandals, christian.couder, ps, gister, Justin Tobler 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 facilitate this, introduce a "re-sign-if-invalid" mode for the `--signed-commits` option. Optionally, a key ID may be explicitly provided in the form `re-sign-if-invalid[=<keyid>]` to specify which signing key should be used when re-signing invalid commit signatures. Note that to properly support interoperability mode when re-signing commit signatures, the commit buffer must be created in both the repository and compatability object formats to generate the appropriate signatures accordingly. As currently implemented, the commit buffer for the compatability object format is not reconstructed and thus re-signing commits in interoperability mode is not yet supported. Support may be added in the future. Signed-off-by: Justin Tobler <jltobler@gmail.com> --- Documentation/git-fast-import.adoc | 4 + builtin/fast-export.c | 8 +- builtin/fast-import.c | 79 ++++++++++++---- gpg-interface.c | 23 +++-- gpg-interface.h | 7 +- t/t9305-fast-import-signatures.sh | 140 ++++++++++++++++++----------- 6 files changed, 183 insertions(+), 78 deletions(-) diff --git a/Documentation/git-fast-import.adoc b/Documentation/git-fast-import.adoc index 479c4081da..08f7d5d89a 100644 --- a/Documentation/git-fast-import.adoc +++ b/Documentation/git-fast-import.adoc @@ -86,6 +86,10 @@ 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[=<keyid>]`, similar to `strip-if-invalid`, verifies + commit signatures and replaces invalid signatures with newly created ones. + Valid signatures are left unchanged. If `<keyid>` is provided, that key is + used for re-signing; otherwise the configured default signing key is used. Options for Frontends ~~~~~~~~~~~~~~~~~~~~~ diff --git a/builtin/fast-export.c b/builtin/fast-export.c index 0c5d2386d8..0ab8465ae3 100644 --- a/builtin/fast-export.c +++ b/builtin/fast-export.c @@ -64,7 +64,7 @@ static int parse_opt_sign_mode(const struct option *opt, if (unset) return 0; - if (parse_sign_mode(arg, val)) + if (parse_sign_mode(arg, val, NULL)) return error(_("unknown %s mode: %s"), opt->long_name, arg); return 0; @@ -825,6 +825,9 @@ static void handle_commit(struct commit *commit, struct rev_info *rev, case SIGN_STRIP_IF_INVALID: die(_("'strip-if-invalid' is not a valid mode for " "git fast-export with --signed-commits=<mode>")); + case SIGN_RESIGN_IF_INVALID: + die(_("'re-sign-if-invalid' is not a valid mode for " + "git fast-export with --signed-commits=<mode>")); default: BUG("invalid signed_commit_mode value %d", signed_commit_mode); } @@ -970,6 +973,9 @@ static void handle_tag(const char *name, struct tag *tag) case SIGN_STRIP_IF_INVALID: die(_("'strip-if-invalid' is not a valid mode for " "git fast-export with --signed-tags=<mode>")); + case SIGN_RESIGN_IF_INVALID: + die(_("'re-sign-if-invalid' is not a valid mode for " + "git fast-export with --signed-tags=<mode>")); default: BUG("invalid signed_commit_mode value %d", signed_commit_mode); } diff --git a/builtin/fast-import.c b/builtin/fast-import.c index b8a7757cfd..f6bd8556f5 100644 --- a/builtin/fast-import.c +++ b/builtin/fast-import.c @@ -190,6 +190,7 @@ static const char *global_prefix; static enum sign_mode signed_tag_mode = SIGN_VERBATIM; static enum sign_mode signed_commit_mode = SIGN_VERBATIM; +static const char *signed_commit_keyid; /* Memory pools */ static struct mem_pool fi_mem_pool = { @@ -2836,10 +2837,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_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 }; @@ -2856,15 +2858,52 @@ static void handle_strip_if_invalid(struct strbuf *new_data, const char *subject; int subject_len = find_commit_subject(msg->buf, &subject); - if (subject_len > 100) - warning(_("stripping invalid signature for commit '%.100s...'\n" - " allegedly by %s"), subject, signer); - else if (subject_len > 0) - warning(_("stripping invalid signature for commit '%.*s'\n" - " allegedly by %s"), subject_len, subject, signer); - else - warning(_("stripping invalid signature for commit\n" - " allegedly by %s"), signer); + if (mode == SIGN_STRIP_IF_INVALID) { + if (subject_len > 100) + warning(_("stripping invalid signature for commit '%.100s...'\n" + " allegedly by %s"), subject, signer); + else if (subject_len > 0) + warning(_("stripping invalid signature for commit '%.*s'\n" + " allegedly by %s"), subject_len, subject, signer); + else + warning(_("stripping invalid signature for commit\n" + " allegedly by %s"), signer); + } else if (mode == SIGN_RESIGN_IF_INVALID) { + struct strbuf signature = STRBUF_INIT; + struct strbuf payload = STRBUF_INIT; + + if (subject_len > 100) + warning(_("re-signing invalid signature for commit '%.100s...'\n" + " allegedly by %s"), subject, signer); + else if (subject_len > 0) + warning(_("re-signing invalid signature for commit '%.*s'\n" + " allegedly by %s"), subject_len, subject, signer); + else + warning(_("re-signing invalid signature for commit\n" + " allegedly by %s"), signer); + + /* + * NEEDSWORK: To properly support interoperability mode + * when re-signing commit signatures, the commit buffer + * must be provided in both the repository and + * compatability object formats. As currently + * implemented, only the repository object format is + * considered meaning compatability signatures cannot be + * generated. Thus, attempting to re-sign commit + * signatures in interoperability mode is currently + * unsupported. + */ + if (the_repository->compat_hash_algo) + die(_("re-signing signatures in interoperability mode is unsupported")); + + strbuf_addstr(&payload, signature_check.payload); + if (sign_buffer_with_key(&payload, &signature, signed_commit_keyid)) + die(_("failed to sign commit object")); + add_header_signature(new_data, &signature, the_hash_algo); + + strbuf_release(&signature); + strbuf_release(&payload); + } finalize_commit_buffer(new_data, NULL, NULL, msg); } else { @@ -2927,6 +2966,7 @@ static void parse_new_commit(const char *arg) /* fallthru */ case SIGN_VERBATIM: case SIGN_STRIP_IF_INVALID: + case SIGN_RESIGN_IF_INVALID: import_one_signature(&sig_sha1, &sig_sha256, v); break; @@ -3011,9 +3051,11 @@ static void parse_new_commit(const char *arg) "encoding %s\n", encoding); - if (signed_commit_mode == SIGN_STRIP_IF_INVALID && + if ((signed_commit_mode == SIGN_STRIP_IF_INVALID || + signed_commit_mode == SIGN_RESIGN_IF_INVALID) && (sig_sha1.hash_algo || sig_sha256.hash_algo)) - handle_strip_if_invalid(&new_data, &sig_sha1, &sig_sha256, &msg); + handle_signature_if_invalid(&new_data, &sig_sha1, &sig_sha256, + &msg, signed_commit_mode); else finalize_commit_buffer(&new_data, &sig_sha1, &sig_sha256, &msg); @@ -3060,6 +3102,9 @@ static void handle_tag_signature(struct strbuf *msg, const char *name) case SIGN_STRIP_IF_INVALID: die(_("'strip-if-invalid' is not a valid mode for " "git fast-import with --signed-tags=<mode>")); + case SIGN_RESIGN_IF_INVALID: + die(_("'re-sign-if-invalid' is not a valid mode for " + "git fast-import with --signed-tags=<mode>")); default: BUG("invalid signed_tag_mode value %d from tag '%s'", signed_tag_mode, name); @@ -3649,10 +3694,10 @@ static int parse_one_option(const char *option) } else if (skip_prefix(option, "export-pack-edges=", &option)) { option_export_pack_edges(option); } else if (skip_prefix(option, "signed-commits=", &option)) { - if (parse_sign_mode(option, &signed_commit_mode)) + if (parse_sign_mode(option, &signed_commit_mode, &signed_commit_keyid)) usagef(_("unknown --signed-commits mode '%s'"), option); } else if (skip_prefix(option, "signed-tags=", &option)) { - if (parse_sign_mode(option, &signed_tag_mode)) + if (parse_sign_mode(option, &signed_tag_mode, NULL)) usagef(_("unknown --signed-tags mode '%s'"), option); } else if (!strcmp(option, "quiet")) { show_stats = 0; diff --git a/gpg-interface.c b/gpg-interface.c index a72fa35061..e028984546 100644 --- a/gpg-interface.c +++ b/gpg-interface.c @@ -1155,21 +1155,28 @@ static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature, return ret; } -int parse_sign_mode(const char *arg, enum sign_mode *mode) +int parse_sign_mode(const char *arg, enum sign_mode *mode, const char **keyid) { - if (!strcmp(arg, "abort")) + if (!strcmp(arg, "abort")) { *mode = SIGN_ABORT; - else if (!strcmp(arg, "verbatim") || !strcmp(arg, "ignore")) + } else if (!strcmp(arg, "verbatim") || !strcmp(arg, "ignore")) { *mode = SIGN_VERBATIM; - else if (!strcmp(arg, "warn-verbatim") || !strcmp(arg, "warn")) + } else if (!strcmp(arg, "warn-verbatim") || !strcmp(arg, "warn")) { *mode = SIGN_WARN_VERBATIM; - else if (!strcmp(arg, "warn-strip")) + } else if (!strcmp(arg, "warn-strip")) { *mode = SIGN_WARN_STRIP; - else if (!strcmp(arg, "strip")) + } else if (!strcmp(arg, "strip")) { *mode = SIGN_STRIP; - else if (!strcmp(arg, "strip-if-invalid")) + } else if (!strcmp(arg, "strip-if-invalid")) { *mode = SIGN_STRIP_IF_INVALID; - else + } else if (!strcmp(arg, "re-sign-if-invalid")) { + *mode = SIGN_RESIGN_IF_INVALID; + } else if (skip_prefix(arg, "re-sign-if-invalid=", &arg)) { + *mode = SIGN_RESIGN_IF_INVALID; + if (keyid) + *keyid = arg; + } else { return -1; + } return 0; } diff --git a/gpg-interface.h b/gpg-interface.h index a32741aeda..8f1fad43e9 100644 --- a/gpg-interface.h +++ b/gpg-interface.h @@ -119,12 +119,15 @@ enum sign_mode { SIGN_WARN_STRIP, SIGN_STRIP, SIGN_STRIP_IF_INVALID, + SIGN_RESIGN_IF_INVALID, }; /* * Return 0 if `arg` can be parsed into an `enum sign_mode`. Return -1 - * otherwise. + * otherwise. If the parsed mode is SIGN_RESIGN_IF_INVALID and GPG key provided + * in the arguments in the form `re-sign-if-invalid=<keyid>`, the key-ID is + * parsed into `char **keyid`. */ -int parse_sign_mode(const char *arg, enum sign_mode *mode); +int parse_sign_mode(const char *arg, enum sign_mode *mode, const char **keyid); #endif diff --git a/t/t9305-fast-import-signatures.sh b/t/t9305-fast-import-signatures.sh index 022dae02e4..2a3f04b42d 100755 --- a/t/t9305-fast-import-signatures.sh +++ b/t/t9305-fast-import-signatures.sh @@ -103,26 +103,85 @@ test_expect_success GPG 'strip both OpenPGP signatures with --signed-commits=war test_line_count = 2 out ' -test_expect_success GPG 'import commit with no signature with --signed-commits=strip-if-invalid' ' - git fast-export main >output && - git -C new fast-import --quiet --signed-commits=strip-if-invalid <output >log 2>&1 && - test_must_be_empty log -' - -test_expect_success GPG 'keep valid OpenPGP signature with --signed-commits=strip-if-invalid' ' - rm -rf new && - git init new && - - git fast-export --signed-commits=verbatim openpgp-signing >output && - git -C new fast-import --quiet --signed-commits=strip-if-invalid <output >log 2>&1 && - IMPORTED=$(git -C new rev-parse --verify refs/heads/openpgp-signing) && - test $OPENPGP_SIGNING = $IMPORTED && - git -C new cat-file commit "$IMPORTED" >actual && - test_grep -E "^gpgsig(-sha256)? " actual && - test_must_be_empty log -' - -test_expect_success GPG 'strip signature invalidated by message change with --signed-commits=strip-if-invalid' ' +for mode in strip-if-invalid re-sign-if-invalid +do + test_expect_success GPG "import commit with no signature with --signed-commits=$mode" ' + git fast-export main >output && + git -C new fast-import --quiet --signed-commits=$mode <output >log 2>&1 && + test_must_be_empty log + ' + + test_expect_success GPG "keep valid OpenPGP signature with --signed-commits=$mode" ' + rm -rf new && + git init new && + + git fast-export --signed-commits=verbatim openpgp-signing >output && + git -C new fast-import --quiet --signed-commits=$mode <output >log 2>&1 && + IMPORTED=$(git -C new rev-parse --verify refs/heads/openpgp-signing) && + test $OPENPGP_SIGNING = $IMPORTED && + git -C new cat-file commit "$IMPORTED" >actual && + test_grep -E "^gpgsig(-sha256)? " actual && + test_must_be_empty log + ' + + test_expect_success GPG "handle signature invalidated by message change with --signed-commits=$mode" ' + rm -rf new && + git init new && + + git fast-export --signed-commits=verbatim openpgp-signing >output && + + # Change the commit message, which invalidates the signature. + # The commit message length should not change though, otherwise the + # corresponding `data <length>` command would have to be changed too. + sed "s/OpenPGP signed commit/OpenPGP forged commit/" output >modified && + + git -C new fast-import --quiet --signed-commits=$mode <modified >log 2>&1 && + + IMPORTED=$(git -C new rev-parse --verify refs/heads/openpgp-signing) && + test $OPENPGP_SIGNING != $IMPORTED && + git -C new cat-file commit "$IMPORTED" >actual && + + if test "$mode" = strip-if-invalid + then + test_grep "stripping invalid signature" log && + test_grep ! -E "^gpgsig" actual + else + test_grep "re-signing invalid signature" log && + test_grep -E "^gpgsig(-sha256)? " actual && + git -C new verify-commit "$IMPORTED" + fi + ' + + test_expect_success GPGSM "keep valid X.509 signature with --signed-commits=$mode" ' + rm -rf new && + git init new && + + git fast-export --signed-commits=verbatim x509-signing >output && + git -C new fast-import --quiet --signed-commits=$mode <output >log 2>&1 && + IMPORTED=$(git -C new rev-parse --verify refs/heads/x509-signing) && + test $X509_SIGNING = $IMPORTED && + git -C new cat-file commit "$IMPORTED" >actual && + test_grep -E "^gpgsig(-sha256)? " actual && + test_must_be_empty log + ' + + test_expect_success GPGSSH "keep valid SSH signature with --signed-commits=$mode" ' + rm -rf new && + git init new && + + test_config -C new gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && + + git fast-export --signed-commits=verbatim ssh-signing >output && + git -C new fast-import --quiet --signed-commits=$mode <output >log 2>&1 && + IMPORTED=$(git -C new rev-parse --verify refs/heads/ssh-signing) && + test $SSH_SIGNING = $IMPORTED && + git -C new cat-file commit "$IMPORTED" >actual && + test_grep -E "^gpgsig(-sha256)? " actual && + test_must_be_empty log + ' +done + +test_expect_success GPGSSH "re-sign invalid commit with explicit keyid" ' rm -rf new && git init new && @@ -133,41 +192,22 @@ test_expect_success GPG 'strip signature invalidated by message change with --si # corresponding `data <length>` command would have to be changed too. sed "s/OpenPGP signed commit/OpenPGP forged commit/" output >modified && - git -C new fast-import --quiet --signed-commits=strip-if-invalid <modified >log 2>&1 && + # Configure the target repository with an invalid default signing key. + test_config -C new user.signingkey "not-a-real-key-id" && + test_config -C new gpg.format ssh && + test_config -C new gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && + test_must_fail git -C new fast-import --quiet \ + --signed-commits=re-sign-if-invalid <modified >/dev/null 2>&1 && + + # Import using explicitly provided signing key. + git -C new fast-import --quiet \ + --signed-commits=re-sign-if-invalid="${GPGSSH_KEY_PRIMARY}" <modified && IMPORTED=$(git -C new rev-parse --verify refs/heads/openpgp-signing) && test $OPENPGP_SIGNING != $IMPORTED && git -C new cat-file commit "$IMPORTED" >actual && - test_grep ! -E "^gpgsig" actual && - test_grep "stripping invalid signature" log -' - -test_expect_success GPGSM 'keep valid X.509 signature with --signed-commits=strip-if-invalid' ' - rm -rf new && - git init new && - - git fast-export --signed-commits=verbatim x509-signing >output && - git -C new fast-import --quiet --signed-commits=strip-if-invalid <output >log 2>&1 && - IMPORTED=$(git -C new rev-parse --verify refs/heads/x509-signing) && - test $X509_SIGNING = $IMPORTED && - git -C new cat-file commit "$IMPORTED" >actual && test_grep -E "^gpgsig(-sha256)? " actual && - test_must_be_empty log -' - -test_expect_success GPGSSH 'keep valid SSH signature with --signed-commits=strip-if-invalid' ' - rm -rf new && - git init new && - - test_config -C new gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && - - git fast-export --signed-commits=verbatim ssh-signing >output && - git -C new fast-import --quiet --signed-commits=strip-if-invalid <output >log 2>&1 && - IMPORTED=$(git -C new rev-parse --verify refs/heads/ssh-signing) && - test $SSH_SIGNING = $IMPORTED && - git -C new cat-file commit "$IMPORTED" >actual && - test_grep -E "^gpgsig(-sha256)? " actual && - test_must_be_empty log + git -C new verify-commit "$IMPORTED" ' test_done -- 2.53.0.381.g628a66ccf6 ^ permalink raw reply related [flat|nested] 60+ messages in thread
* Re: [PATCH v2 3/3] fast-import: add mode to re-sign invalid commit signatures 2026-03-06 20:53 ` [PATCH v2 3/3] fast-import: add mode to re-sign invalid commit signatures Justin Tobler @ 2026-03-10 9:27 ` Christian Couder 2026-03-10 18:09 ` Justin Tobler 0 siblings, 1 reply; 60+ messages in thread From: Christian Couder @ 2026-03-10 9:27 UTC (permalink / raw) To: Justin Tobler; +Cc: git, sandals, ps, gister On Fri, Mar 6, 2026 at 9:54 PM Justin Tobler <jltobler@gmail.com> wrote: > @@ -825,6 +825,9 @@ static void handle_commit(struct commit *commit, struct rev_info *rev, > case SIGN_STRIP_IF_INVALID: > die(_("'strip-if-invalid' is not a valid mode for " > "git fast-export with --signed-commits=<mode>")); > + case SIGN_RESIGN_IF_INVALID: Everywhere in this patch, I think "RE_SIGN" might be more consistent than "RESIGN" for this name. > + die(_("'re-sign-if-invalid' is not a valid mode for " > + "git fast-export with --signed-commits=<mode>")); [...] > @@ -2856,15 +2858,52 @@ static void handle_strip_if_invalid(struct strbuf *new_data, > const char *subject; > int subject_len = find_commit_subject(msg->buf, &subject); > > - if (subject_len > 100) > - warning(_("stripping invalid signature for commit '%.100s...'\n" > - " allegedly by %s"), subject, signer); > - else if (subject_len > 0) > - warning(_("stripping invalid signature for commit '%.*s'\n" > - " allegedly by %s"), subject_len, subject, signer); > - else > - warning(_("stripping invalid signature for commit\n" > - " allegedly by %s"), signer); > + if (mode == SIGN_STRIP_IF_INVALID) { > + if (subject_len > 100) > + warning(_("stripping invalid signature for commit '%.100s...'\n" > + " allegedly by %s"), subject, signer); > + else if (subject_len > 0) > + warning(_("stripping invalid signature for commit '%.*s'\n" > + " allegedly by %s"), subject_len, subject, signer); > + else > + warning(_("stripping invalid signature for commit\n" > + " allegedly by %s"), signer); > + } else if (mode == SIGN_RESIGN_IF_INVALID) { > + struct strbuf signature = STRBUF_INIT; > + struct strbuf payload = STRBUF_INIT; > + > + if (subject_len > 100) > + warning(_("re-signing invalid signature for commit '%.100s...'\n" > + " allegedly by %s"), subject, signer); > + else if (subject_len > 0) > + warning(_("re-signing invalid signature for commit '%.*s'\n" > + " allegedly by %s"), subject_len, subject, signer); > + else > + warning(_("re-signing invalid signature for commit\n" > + " allegedly by %s"), signer); Maybe a helper function could be used to avoid duplicating the warning logic. > + /* > + * NEEDSWORK: To properly support interoperability mode > + * when re-signing commit signatures, the commit buffer > + * must be provided in both the repository and > + * compatability object formats. As currently s/compatability/compatibility/ > + * implemented, only the repository object format is > + * considered meaning compatability signatures cannot be s/compatability/compatibility/ > + * generated. Thus, attempting to re-sign commit > + * signatures in interoperability mode is currently > + * unsupported. > + */ > + if (the_repository->compat_hash_algo) > + die(_("re-signing signatures in interoperability mode is unsupported")); > + > + strbuf_addstr(&payload, signature_check.payload); > + if (sign_buffer_with_key(&payload, &signature, signed_commit_keyid)) > + die(_("failed to sign commit object")); > + add_header_signature(new_data, &signature, the_hash_algo); > + > + strbuf_release(&signature); > + strbuf_release(&payload); > + } Except for these small issues and the few nits in the previous patch, this looks good to me. Thanks for working on it. ^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH v2 3/3] fast-import: add mode to re-sign invalid commit signatures 2026-03-10 9:27 ` Christian Couder @ 2026-03-10 18:09 ` Justin Tobler 0 siblings, 0 replies; 60+ messages in thread From: Justin Tobler @ 2026-03-10 18:09 UTC (permalink / raw) To: Christian Couder; +Cc: git, sandals, ps On 26/03/10 10:27AM, Christian Couder wrote: > On Fri, Mar 6, 2026 at 9:54 PM Justin Tobler <jltobler@gmail.com> wrote: > > > @@ -825,6 +825,9 @@ static void handle_commit(struct commit *commit, struct rev_info *rev, > > case SIGN_STRIP_IF_INVALID: > > die(_("'strip-if-invalid' is not a valid mode for " > > "git fast-export with --signed-commits=<mode>")); > > + case SIGN_RESIGN_IF_INVALID: > > Everywhere in this patch, I think "RE_SIGN" might be more consistent > than "RESIGN" for this name. That's fair, will change. > > + die(_("'re-sign-if-invalid' is not a valid mode for " > > + "git fast-export with --signed-commits=<mode>")); > > [...] > > > @@ -2856,15 +2858,52 @@ static void handle_strip_if_invalid(struct strbuf *new_data, > > const char *subject; > > int subject_len = find_commit_subject(msg->buf, &subject); > > > > - if (subject_len > 100) > > - warning(_("stripping invalid signature for commit '%.100s...'\n" > > - " allegedly by %s"), subject, signer); > > - else if (subject_len > 0) > > - warning(_("stripping invalid signature for commit '%.*s'\n" > > - " allegedly by %s"), subject_len, subject, signer); > > - else > > - warning(_("stripping invalid signature for commit\n" > > - " allegedly by %s"), signer); > > + if (mode == SIGN_STRIP_IF_INVALID) { > > + if (subject_len > 100) > > + warning(_("stripping invalid signature for commit '%.100s...'\n" > > + " allegedly by %s"), subject, signer); > > + else if (subject_len > 0) > > + warning(_("stripping invalid signature for commit '%.*s'\n" > > + " allegedly by %s"), subject_len, subject, signer); > > + else > > + warning(_("stripping invalid signature for commit\n" > > + " allegedly by %s"), signer); > > + } else if (mode == SIGN_RESIGN_IF_INVALID) { > > + struct strbuf signature = STRBUF_INIT; > > + struct strbuf payload = STRBUF_INIT; > > + > > + if (subject_len > 100) > > + warning(_("re-signing invalid signature for commit '%.100s...'\n" > > + " allegedly by %s"), subject, signer); > > + else if (subject_len > 0) > > + warning(_("re-signing invalid signature for commit '%.*s'\n" > > + " allegedly by %s"), subject_len, subject, signer); > > + else > > + warning(_("re-signing invalid signature for commit\n" > > + " allegedly by %s"), signer); > > Maybe a helper function could be used to avoid duplicating the warning logic. Ya, I could extract this out to a helper that prints the appropriate warning. Due to being translated, I'm not quite sure if there would be a good way to make the message strings more generic though. Will update in the next version. -Justin ^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH v3 0/3] fast-import: add mode to re-sign invalid commit signatures 2026-03-06 20:53 ` [PATCH v2 0/3] " Justin Tobler ` (2 preceding siblings ...) 2026-03-06 20:53 ` [PATCH v2 3/3] fast-import: add mode to re-sign invalid commit signatures Justin Tobler @ 2026-03-10 20:11 ` Justin Tobler 2026-03-10 20:11 ` [PATCH v3 1/3] commit: remove unused forward declaration Justin Tobler ` (4 more replies) 3 siblings, 5 replies; 60+ messages in thread From: Justin Tobler @ 2026-03-10 20:11 UTC (permalink / raw) To: git; +Cc: sandals, christian.couder, ps, gitster, Justin Tobler Greetings, With c20f112e51 (fast-import: add 'strip-if-invalid' mode to --signed-commits=<mode>, 2025-11-17), it became possible to remove invalid signatures from commits via git-fast-import(1) while maintaining valid commit signatures. Building upon this functionality, a user may want to re-sign these invalid commit signatures. This series introduces the `re-sign-if-invalid` mode to do so accordingly. The newly added mode in this series currently ignores `extensions.compatObjectFormat` when generating the new signatures. From my understanding, to generate the compatibility structure would also require us to reconstruct the compatibility object for the object being signed. I think this would be possible to do, but would require getting the mapped OIDs for the commit parents and tree. I'm not completely sure of a good way to go about this yet though. I'm also not completely certain if this is something that should be addressed as part of this series, or could be done later down the road. So for now I've opted to delay its implementation. I'm open going down the other route if that is preferred though. The first commit is a simple cleanup for something I noticed while reading though commit signing code. The second commit actually introduces the new `--signed-commits` mode. Changes since V2: - Adapted commit message in second patch to improve clarity. - Fixed typos. - Renamed SIGN_RESIGN_IF_INVALID to SIGN_RE_SIGN_IF_INVALID. - Created separate helper function to handle printing invalid signature warnings. Changes since V1: - Improved commit messages and comments to better explain why interoperability mode is not currently supported. - Clarified documentation for re-sign-if-invalid mode. - Renamed `handle_invalid_signature()` to `handle_signature_if_invalid()`. - Added warning messages specific to commit resigning. - Fixed some small typos. - Added support for explicitly specifying the signing key ID via `--signed-commits=re-sign-if-invalid[=<keyid>]` similar to how it can specified in git-commit(1). - We now die() as unsupported when attempting to re-sign an invalid commit signature in interoperability mode. - We now die() when failing to re-sign a commit. Thanks, -Justin Justin Tobler (3): commit: remove unused forward declaration gpg-interface: introduce sign_buffer_with_key() fast-import: add mode to re-sign invalid commit signatures Documentation/git-fast-import.adoc | 4 + builtin/fast-export.c | 8 +- builtin/fast-import.c | 101 ++++++++++++++++----- commit.c | 16 +--- commit.h | 2 - gpg-interface.c | 36 ++++++-- gpg-interface.h | 14 ++- t/t9305-fast-import-signatures.sh | 140 ++++++++++++++++++----------- 8 files changed, 222 insertions(+), 99 deletions(-) Range-diff against v2: 1: 0d00b72ee0 = 1: 0d00b72ee0 commit: remove unused forward declaration 2: 499025532c ! 2: 0b0a06347d gpg-interface: introduce sign_buffer_with_key() @@ Commit message provided and handles generating the commit signature accordingly. This signing operation is not really specific to commits as any arbitrary buffer can be signed. Also, in a subsequent commit, this same logic is - reused by git-fast-import(1) when resigning invalid commit signatures. - Introduce `sign_buffer_with_key()` to centralize signing key resolution - in gpg-interface to allow callers to reuse the same behavior without - duplicating logic. + reused by git-fast-import(1) when re-signing invalid commit signatures. + + Move the `sign_commit_to_strbuf()` helper from "commit.c" to + "gpg-interface.c" and rename it to `sign_buffer_with_key()`. Also export + this function so it can be used by "commit.c" and + "builtin/fast-import.c" in the subsequent commit. Signed-off-by: Justin Tobler <jltobler@gmail.com> 3: bea1a42eb9 ! 3: 57a27ccc61 fast-import: add mode to re-sign invalid commit signatures @@ builtin/fast-export.c: static void handle_commit(struct commit *commit, struct r case SIGN_STRIP_IF_INVALID: die(_("'strip-if-invalid' is not a valid mode for " "git fast-export with --signed-commits=<mode>")); -+ case SIGN_RESIGN_IF_INVALID: ++ case SIGN_RE_SIGN_IF_INVALID: + die(_("'re-sign-if-invalid' is not a valid mode for " + "git fast-export with --signed-commits=<mode>")); default: @@ builtin/fast-export.c: static void handle_tag(const char *name, struct tag *tag) case SIGN_STRIP_IF_INVALID: die(_("'strip-if-invalid' is not a valid mode for " "git fast-export with --signed-tags=<mode>")); -+ case SIGN_RESIGN_IF_INVALID: ++ case SIGN_RE_SIGN_IF_INVALID: + die(_("'re-sign-if-invalid' is not a valid mode for " + "git fast-export with --signed-tags=<mode>")); default: @@ builtin/fast-import.c: static void finalize_commit_buffer(struct strbuf *new_dat - struct signature_data *sig_sha1, - struct signature_data *sig_sha256, - struct strbuf *msg) ++static void warn_invalid_signature(struct signature_check *check, ++ const char *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) { +- const char *signer = signature_check.signer ? +- signature_check.signer : _("unknown"); +- const char *subject; +- int subject_len = find_commit_subject(msg->buf, &subject); ++ const char *signer = check->signer ? check->signer : _("unknown"); ++ const char *subject; ++ int subject_len = find_commit_subject(msg, &subject); + ++ switch (mode) { ++ case SIGN_STRIP_IF_INVALID: + if (subject_len > 100) + warning(_("stripping invalid signature for commit '%.100s...'\n" + " allegedly by %s"), subject, signer); +@@ builtin/fast-import.c: 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_RE_SIGN_IF_INVALID: ++ if (subject_len > 100) ++ warning(_("re-signing invalid signature for commit '%.100s...'\n" ++ " allegedly by %s"), subject, signer); ++ else if (subject_len > 0) ++ warning(_("re-signing invalid signature for commit '%.*s'\n" ++ " allegedly by %s"), subject_len, subject, signer); ++ else ++ warning(_("re-signing invalid signature for commit\n" ++ " allegedly by %s"), signer); ++ break; ++ default: ++ BUG("unsupported signing mode"); ++ } ++} ++ +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 }; -@@ builtin/fast-import.c: static void handle_strip_if_invalid(struct strbuf *new_data, - const char *subject; - int subject_len = find_commit_subject(msg->buf, &subject); - -- if (subject_len > 100) -- warning(_("stripping invalid signature for commit '%.100s...'\n" -- " allegedly by %s"), subject, signer); -- else if (subject_len > 0) -- warning(_("stripping invalid signature for commit '%.*s'\n" -- " allegedly by %s"), subject_len, subject, signer); -- else -- warning(_("stripping invalid signature for commit\n" -- " allegedly by %s"), signer); -+ if (mode == SIGN_STRIP_IF_INVALID) { -+ if (subject_len > 100) -+ warning(_("stripping invalid signature for commit '%.100s...'\n" -+ " allegedly by %s"), subject, signer); -+ else if (subject_len > 0) -+ warning(_("stripping invalid signature for commit '%.*s'\n" -+ " allegedly by %s"), subject_len, subject, signer); -+ else -+ warning(_("stripping invalid signature for commit\n" -+ " allegedly by %s"), signer); -+ } else if (mode == SIGN_RESIGN_IF_INVALID) { ++{ ++ 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_RE_SIGN_IF_INVALID) { + struct strbuf signature = STRBUF_INIT; + struct strbuf payload = STRBUF_INIT; + -+ if (subject_len > 100) -+ warning(_("re-signing invalid signature for commit '%.100s...'\n" -+ " allegedly by %s"), subject, signer); -+ else if (subject_len > 0) -+ warning(_("re-signing invalid signature for commit '%.*s'\n" -+ " allegedly by %s"), subject_len, subject, signer); -+ else -+ warning(_("re-signing invalid signature for commit\n" -+ " allegedly by %s"), signer); -+ + /* + * NEEDSWORK: To properly support interoperability mode + * when re-signing commit signatures, the commit buffer + * must be provided in both the repository and -+ * compatability object formats. As currently ++ * compatibility object formats. As currently + * implemented, only the repository object format is -+ * considered meaning compatability signatures cannot be ++ * considered meaning compatibility signatures cannot be + * generated. Thus, attempting to re-sign commit + * signatures in interoperability mode is currently + * unsupported. @@ builtin/fast-import.c: static void parse_new_commit(const char *arg) /* fallthru */ case SIGN_VERBATIM: case SIGN_STRIP_IF_INVALID: -+ case SIGN_RESIGN_IF_INVALID: ++ case SIGN_RE_SIGN_IF_INVALID: import_one_signature(&sig_sha1, &sig_sha256, v); break; @@ builtin/fast-import.c: static void parse_new_commit(const char *arg) - if (signed_commit_mode == SIGN_STRIP_IF_INVALID && + if ((signed_commit_mode == SIGN_STRIP_IF_INVALID || -+ signed_commit_mode == SIGN_RESIGN_IF_INVALID) && ++ signed_commit_mode == SIGN_RE_SIGN_IF_INVALID) && (sig_sha1.hash_algo || sig_sha256.hash_algo)) - handle_strip_if_invalid(&new_data, &sig_sha1, &sig_sha256, &msg); + handle_signature_if_invalid(&new_data, &sig_sha1, &sig_sha256, @@ builtin/fast-import.c: static void handle_tag_signature(struct strbuf *msg, cons case SIGN_STRIP_IF_INVALID: die(_("'strip-if-invalid' is not a valid mode for " "git fast-import with --signed-tags=<mode>")); -+ case SIGN_RESIGN_IF_INVALID: ++ case SIGN_RE_SIGN_IF_INVALID: + die(_("'re-sign-if-invalid' is not a valid mode for " + "git fast-import with --signed-tags=<mode>")); default: @@ gpg-interface.c: static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *mode = SIGN_STRIP_IF_INVALID; - else + } else if (!strcmp(arg, "re-sign-if-invalid")) { -+ *mode = SIGN_RESIGN_IF_INVALID; ++ *mode = SIGN_RE_SIGN_IF_INVALID; + } else if (skip_prefix(arg, "re-sign-if-invalid=", &arg)) { -+ *mode = SIGN_RESIGN_IF_INVALID; ++ *mode = SIGN_RE_SIGN_IF_INVALID; + if (keyid) + *keyid = arg; + } else { @@ gpg-interface.h: enum sign_mode { SIGN_WARN_STRIP, SIGN_STRIP, SIGN_STRIP_IF_INVALID, -+ SIGN_RESIGN_IF_INVALID, ++ SIGN_RE_SIGN_IF_INVALID, }; /* * Return 0 if `arg` can be parsed into an `enum sign_mode`. Return -1 - * otherwise. -+ * otherwise. If the parsed mode is SIGN_RESIGN_IF_INVALID and GPG key provided ++ * otherwise. If the parsed mode is SIGN_RE_SIGN_IF_INVALID and GPG key provided + * in the arguments in the form `re-sign-if-invalid=<keyid>`, the key-ID is + * parsed into `char **keyid`. */ base-commit: 7c02d39fc2ed2702223c7674f73150d9a7e61ba4 -- 2.53.0.381.g628a66ccf6 ^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH v3 1/3] commit: remove unused forward declaration 2026-03-10 20:11 ` [PATCH v3 0/3] " Justin Tobler @ 2026-03-10 20:11 ` Justin Tobler 2026-03-10 22:29 ` Junio C Hamano 2026-03-10 20:11 ` [PATCH v3 2/3] gpg-interface: introduce sign_buffer_with_key() Justin Tobler ` (3 subsequent siblings) 4 siblings, 1 reply; 60+ messages in thread From: Justin Tobler @ 2026-03-10 20:11 UTC (permalink / raw) To: git; +Cc: sandals, christian.couder, ps, gitster, Justin Tobler In 6206089cbd (commit: write commits for both hashes, 2023-10-01), `sign_with_header()` was removed, but its forward declaration in "commit.h" was left. Remove the unused declaration. Signed-off-by: Justin Tobler <jltobler@gmail.com> --- commit.h | 2 -- 1 file changed, 2 deletions(-) diff --git a/commit.h b/commit.h index 1635de418b..f0c38cb444 100644 --- a/commit.h +++ b/commit.h @@ -390,8 +390,6 @@ LAST_ARG_MUST_BE_NULL int run_commit_hook(int editor_is_used, const char *index_file, int *invoked_hook, const char *name, ...); -/* Sign a commit or tag buffer, storing the result in a header. */ -int sign_with_header(struct strbuf *buf, const char *keyid); /* Parse the signature out of a header. */ int parse_buffer_signed_by_header(const char *buffer, unsigned long size, -- 2.53.0.381.g628a66ccf6 ^ permalink raw reply related [flat|nested] 60+ messages in thread
* Re: [PATCH v3 1/3] commit: remove unused forward declaration 2026-03-10 20:11 ` [PATCH v3 1/3] commit: remove unused forward declaration Justin Tobler @ 2026-03-10 22:29 ` Junio C Hamano 0 siblings, 0 replies; 60+ messages in thread From: Junio C Hamano @ 2026-03-10 22:29 UTC (permalink / raw) To: Justin Tobler; +Cc: git, sandals, christian.couder, ps Justin Tobler <jltobler@gmail.com> writes: > In 6206089cbd (commit: write commits for both hashes, 2023-10-01), > `sign_with_header()` was removed, but its forward declaration in > "commit.h" was left. Remove the unused declaration. > > Signed-off-by: Justin Tobler <jltobler@gmail.com> > --- > commit.h | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/commit.h b/commit.h > index 1635de418b..f0c38cb444 100644 > --- a/commit.h > +++ b/commit.h > @@ -390,8 +390,6 @@ LAST_ARG_MUST_BE_NULL > int run_commit_hook(int editor_is_used, const char *index_file, > int *invoked_hook, const char *name, ...); > > -/* Sign a commit or tag buffer, storing the result in a header. */ > -int sign_with_header(struct strbuf *buf, const char *keyid); > /* Parse the signature out of a header. */ > int parse_buffer_signed_by_header(const char *buffer, > unsigned long size, Nice and obvious clean-up. ^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH v3 2/3] gpg-interface: introduce sign_buffer_with_key() 2026-03-10 20:11 ` [PATCH v3 0/3] " Justin Tobler 2026-03-10 20:11 ` [PATCH v3 1/3] commit: remove unused forward declaration Justin Tobler @ 2026-03-10 20:11 ` Justin Tobler 2026-03-10 22:33 ` Junio C Hamano 2026-03-10 20:11 ` [PATCH v3 3/3] fast-import: add mode to re-sign invalid commit signatures Justin Tobler ` (2 subsequent siblings) 4 siblings, 1 reply; 60+ messages in thread From: Justin Tobler @ 2026-03-10 20:11 UTC (permalink / raw) To: git; +Cc: sandals, christian.couder, ps, gitster, Justin Tobler The `sign_commit_to_strbuf()` helper in "commit.c" provides fallback logic to get the default configured signing key when a key is not provided and handles generating the commit signature accordingly. This signing operation is not really specific to commits as any arbitrary buffer can be signed. Also, in a subsequent commit, this same logic is reused by git-fast-import(1) when re-signing invalid commit signatures. Move the `sign_commit_to_strbuf()` helper from "commit.c" to "gpg-interface.c" and rename it to `sign_buffer_with_key()`. Also export this function so it can be used by "commit.c" and "builtin/fast-import.c" in the subsequent commit. Signed-off-by: Justin Tobler <jltobler@gmail.com> --- commit.c | 16 ++-------------- gpg-interface.c | 13 +++++++++++++ gpg-interface.h | 7 +++++++ 3 files changed, 22 insertions(+), 14 deletions(-) diff --git a/commit.c b/commit.c index d16ae73345..1677b1ef25 100644 --- a/commit.c +++ b/commit.c @@ -1148,18 +1148,6 @@ int add_header_signature(struct strbuf *buf, struct strbuf *sig, const struct gi return 0; } -static int sign_commit_to_strbuf(struct strbuf *sig, struct strbuf *buf, const char *keyid) -{ - char *keyid_to_free = NULL; - int ret = 0; - if (!keyid || !*keyid) - keyid = keyid_to_free = get_signing_key(); - if (sign_buffer(buf, sig, keyid)) - ret = -1; - free(keyid_to_free); - return ret; -} - int parse_signed_commit(const struct commit *commit, struct strbuf *payload, struct strbuf *signature, const struct git_hash_algo *algop) @@ -1737,7 +1725,7 @@ int commit_tree_extended(const char *msg, size_t msg_len, oidcpy(&parent_buf[i++], &p->item->object.oid); write_commit_tree(&buffer, msg, msg_len, tree, parent_buf, nparents, author, committer, extra); - if (sign_commit && sign_commit_to_strbuf(&sig, &buffer, sign_commit)) { + if (sign_commit && sign_buffer_with_key(&buffer, &sig, sign_commit)) { result = -1; goto out; } @@ -1769,7 +1757,7 @@ int commit_tree_extended(const char *msg, size_t msg_len, free_commit_extra_headers(compat_extra); free(mapped_parents); - if (sign_commit && sign_commit_to_strbuf(&compat_sig, &compat_buffer, sign_commit)) { + if (sign_commit && sign_buffer_with_key(&compat_buffer, &compat_sig, sign_commit)) { result = -1; goto out; } diff --git a/gpg-interface.c b/gpg-interface.c index 87fb6605fb..a72fa35061 100644 --- a/gpg-interface.c +++ b/gpg-interface.c @@ -980,6 +980,19 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *sig return use_format->sign_buffer(buffer, signature, signing_key); } +int sign_buffer_with_key(struct strbuf *buffer, struct strbuf *signature, + const char *signing_key) +{ + char *keyid_to_free = NULL; + int ret = 0; + if (!signing_key || !*signing_key) + signing_key = keyid_to_free = get_signing_key(); + if (sign_buffer(buffer, signature, signing_key)) + ret = -1; + free(keyid_to_free); + return ret; +} + /* * Strip CR from the line endings, in case we are on Windows. * NEEDSWORK: make it trim only CRs before LFs and rename diff --git a/gpg-interface.h b/gpg-interface.h index 789d1ffac4..a32741aeda 100644 --- a/gpg-interface.h +++ b/gpg-interface.h @@ -83,6 +83,13 @@ size_t parse_signed_buffer(const char *buf, size_t size); int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *signing_key); +/* + * Similar to `sign_buffer()`, but uses the default configured signing key as + * returned by `get_signing_key()` when the provided "signing_key" is NULL or + * empty. Returns 0 on success, non-zero on failure. + */ +int sign_buffer_with_key(struct strbuf *buffer, struct strbuf *signature, + const char *signing_key); /* * Returns corresponding string in lowercase for a given member of -- 2.53.0.381.g628a66ccf6 ^ permalink raw reply related [flat|nested] 60+ messages in thread
* Re: [PATCH v3 2/3] gpg-interface: introduce sign_buffer_with_key() 2026-03-10 20:11 ` [PATCH v3 2/3] gpg-interface: introduce sign_buffer_with_key() Justin Tobler @ 2026-03-10 22:33 ` Junio C Hamano 0 siblings, 0 replies; 60+ messages in thread From: Junio C Hamano @ 2026-03-10 22:33 UTC (permalink / raw) To: Justin Tobler; +Cc: git, sandals, christian.couder, ps Justin Tobler <jltobler@gmail.com> writes: > The `sign_commit_to_strbuf()` helper in "commit.c" provides fallback > logic to get the default configured signing key when a key is not > provided and handles generating the commit signature accordingly. This > signing operation is not really specific to commits as any arbitrary > buffer can be signed. Also, in a subsequent commit, this same logic is > reused by git-fast-import(1) when re-signing invalid commit signatures. > > Move the `sign_commit_to_strbuf()` helper from "commit.c" to > "gpg-interface.c" and rename it to `sign_buffer_with_key()`. Also export > this function so it can be used by "commit.c" and > "builtin/fast-import.c" in the subsequent commit. > > Signed-off-by: Justin Tobler <jltobler@gmail.com> > --- > commit.c | 16 ++-------------- > gpg-interface.c | 13 +++++++++++++ > gpg-interface.h | 7 +++++++ > 3 files changed, 22 insertions(+), 14 deletions(-) Sennsible restructuring that makes the machinery easier to reuse. Updated function is named more appropriately for public consumption. Overall a very welcome preparation step. > diff --git a/commit.c b/commit.c > index d16ae73345..1677b1ef25 100644 > --- a/commit.c > +++ b/commit.c > @@ -1148,18 +1148,6 @@ int add_header_signature(struct strbuf *buf, struct strbuf *sig, const struct gi > return 0; > } > > -static int sign_commit_to_strbuf(struct strbuf *sig, struct strbuf *buf, const char *keyid) > -{ > - char *keyid_to_free = NULL; > - int ret = 0; > - if (!keyid || !*keyid) > - keyid = keyid_to_free = get_signing_key(); > - if (sign_buffer(buf, sig, keyid)) > - ret = -1; > - free(keyid_to_free); > - return ret; > -} > - > int parse_signed_commit(const struct commit *commit, > struct strbuf *payload, struct strbuf *signature, > const struct git_hash_algo *algop) > @@ -1737,7 +1725,7 @@ int commit_tree_extended(const char *msg, size_t msg_len, > oidcpy(&parent_buf[i++], &p->item->object.oid); > > write_commit_tree(&buffer, msg, msg_len, tree, parent_buf, nparents, author, committer, extra); > - if (sign_commit && sign_commit_to_strbuf(&sig, &buffer, sign_commit)) { > + if (sign_commit && sign_buffer_with_key(&buffer, &sig, sign_commit)) { > result = -1; > goto out; > } > @@ -1769,7 +1757,7 @@ int commit_tree_extended(const char *msg, size_t msg_len, > free_commit_extra_headers(compat_extra); > free(mapped_parents); > > - if (sign_commit && sign_commit_to_strbuf(&compat_sig, &compat_buffer, sign_commit)) { > + if (sign_commit && sign_buffer_with_key(&compat_buffer, &compat_sig, sign_commit)) { > result = -1; > goto out; > } > diff --git a/gpg-interface.c b/gpg-interface.c > index 87fb6605fb..a72fa35061 100644 > --- a/gpg-interface.c > +++ b/gpg-interface.c > @@ -980,6 +980,19 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *sig > return use_format->sign_buffer(buffer, signature, signing_key); > } > > +int sign_buffer_with_key(struct strbuf *buffer, struct strbuf *signature, > + const char *signing_key) > +{ > + char *keyid_to_free = NULL; > + int ret = 0; > + if (!signing_key || !*signing_key) > + signing_key = keyid_to_free = get_signing_key(); > + if (sign_buffer(buffer, signature, signing_key)) > + ret = -1; > + free(keyid_to_free); > + return ret; > +} > + > /* > * Strip CR from the line endings, in case we are on Windows. > * NEEDSWORK: make it trim only CRs before LFs and rename > diff --git a/gpg-interface.h b/gpg-interface.h > index 789d1ffac4..a32741aeda 100644 > --- a/gpg-interface.h > +++ b/gpg-interface.h > @@ -83,6 +83,13 @@ size_t parse_signed_buffer(const char *buf, size_t size); > int sign_buffer(struct strbuf *buffer, struct strbuf *signature, > const char *signing_key); > > +/* > + * Similar to `sign_buffer()`, but uses the default configured signing key as > + * returned by `get_signing_key()` when the provided "signing_key" is NULL or > + * empty. Returns 0 on success, non-zero on failure. > + */ > +int sign_buffer_with_key(struct strbuf *buffer, struct strbuf *signature, > + const char *signing_key); > > /* > * Returns corresponding string in lowercase for a given member of ^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH v3 3/3] fast-import: add mode to re-sign invalid commit signatures 2026-03-10 20:11 ` [PATCH v3 0/3] " Justin Tobler 2026-03-10 20:11 ` [PATCH v3 1/3] commit: remove unused forward declaration Justin Tobler 2026-03-10 20:11 ` [PATCH v3 2/3] gpg-interface: introduce sign_buffer_with_key() Justin Tobler @ 2026-03-10 20:11 ` Justin Tobler 2026-03-10 20:49 ` [PATCH v3 0/3] " Junio C Hamano 2026-03-11 17:31 ` [PATCH v4 " Justin Tobler 4 siblings, 0 replies; 60+ messages in thread From: Justin Tobler @ 2026-03-10 20:11 UTC (permalink / raw) To: git; +Cc: sandals, christian.couder, ps, gitster, Justin Tobler 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 facilitate this, introduce a "re-sign-if-invalid" mode for the `--signed-commits` option. Optionally, a key ID may be explicitly provided in the form `re-sign-if-invalid[=<keyid>]` to specify which signing key should be used when re-signing invalid commit signatures. Note that to properly support interoperability mode when re-signing commit signatures, the commit buffer must be created in both the repository and compatability object formats to generate the appropriate signatures accordingly. As currently implemented, the commit buffer for the compatability object format is not reconstructed and thus re-signing commits in interoperability mode is not yet supported. Support may be added in the future. Signed-off-by: Justin Tobler <jltobler@gmail.com> --- Documentation/git-fast-import.adoc | 4 + builtin/fast-export.c | 8 +- builtin/fast-import.c | 101 ++++++++++++++++----- gpg-interface.c | 23 +++-- gpg-interface.h | 7 +- t/t9305-fast-import-signatures.sh | 140 ++++++++++++++++++----------- 6 files changed, 200 insertions(+), 83 deletions(-) diff --git a/Documentation/git-fast-import.adoc b/Documentation/git-fast-import.adoc index 479c4081da..08f7d5d89a 100644 --- a/Documentation/git-fast-import.adoc +++ b/Documentation/git-fast-import.adoc @@ -86,6 +86,10 @@ 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[=<keyid>]`, similar to `strip-if-invalid`, verifies + commit signatures and replaces invalid signatures with newly created ones. + Valid signatures are left unchanged. If `<keyid>` is provided, that key is + used for re-signing; otherwise the configured default signing key is used. Options for Frontends ~~~~~~~~~~~~~~~~~~~~~ diff --git a/builtin/fast-export.c b/builtin/fast-export.c index 0c5d2386d8..2067613267 100644 --- a/builtin/fast-export.c +++ b/builtin/fast-export.c @@ -64,7 +64,7 @@ static int parse_opt_sign_mode(const struct option *opt, if (unset) return 0; - if (parse_sign_mode(arg, val)) + if (parse_sign_mode(arg, val, NULL)) return error(_("unknown %s mode: %s"), opt->long_name, arg); return 0; @@ -825,6 +825,9 @@ static void handle_commit(struct commit *commit, struct rev_info *rev, case SIGN_STRIP_IF_INVALID: die(_("'strip-if-invalid' is not a valid mode for " "git fast-export with --signed-commits=<mode>")); + case SIGN_RE_SIGN_IF_INVALID: + die(_("'re-sign-if-invalid' is not a valid mode for " + "git fast-export with --signed-commits=<mode>")); default: BUG("invalid signed_commit_mode value %d", signed_commit_mode); } @@ -970,6 +973,9 @@ static void handle_tag(const char *name, struct tag *tag) case SIGN_STRIP_IF_INVALID: die(_("'strip-if-invalid' is not a valid mode for " "git fast-export with --signed-tags=<mode>")); + case SIGN_RE_SIGN_IF_INVALID: + die(_("'re-sign-if-invalid' is not a valid mode for " + "git fast-export with --signed-tags=<mode>")); default: BUG("invalid signed_commit_mode value %d", signed_commit_mode); } diff --git a/builtin/fast-import.c b/builtin/fast-import.c index b8a7757cfd..472d9ab712 100644 --- a/builtin/fast-import.c +++ b/builtin/fast-import.c @@ -190,6 +190,7 @@ static const char *global_prefix; static enum sign_mode signed_tag_mode = SIGN_VERBATIM; static enum sign_mode signed_commit_mode = SIGN_VERBATIM; +static const char *signed_commit_keyid; /* Memory pools */ static struct mem_pool fi_mem_pool = { @@ -2836,26 +2837,15 @@ 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 warn_invalid_signature(struct signature_check *check, + const char *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) { - const char *signer = signature_check.signer ? - signature_check.signer : _("unknown"); - const char *subject; - int subject_len = find_commit_subject(msg->buf, &subject); + const char *signer = check->signer ? check->signer : _("unknown"); + const char *subject; + int subject_len = find_commit_subject(msg, &subject); + switch (mode) { + case SIGN_STRIP_IF_INVALID: if (subject_len > 100) warning(_("stripping invalid signature for commit '%.100s...'\n" " allegedly by %s"), subject, signer); @@ -2865,6 +2855,67 @@ 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_RE_SIGN_IF_INVALID: + if (subject_len > 100) + warning(_("re-signing invalid signature for commit '%.100s...'\n" + " allegedly by %s"), subject, signer); + else if (subject_len > 0) + warning(_("re-signing invalid signature for commit '%.*s'\n" + " allegedly by %s"), subject_len, subject, signer); + else + warning(_("re-signing invalid signature for commit\n" + " allegedly by %s"), signer); + break; + default: + BUG("unsupported signing mode"); + } +} + +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_RE_SIGN_IF_INVALID) { + struct strbuf signature = STRBUF_INIT; + struct strbuf payload = STRBUF_INIT; + + /* + * NEEDSWORK: To properly support interoperability mode + * when re-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 re-sign commit + * signatures in interoperability mode is currently + * unsupported. + */ + if (the_repository->compat_hash_algo) + die(_("re-signing signatures in interoperability mode is unsupported")); + + strbuf_addstr(&payload, signature_check.payload); + if (sign_buffer_with_key(&payload, &signature, signed_commit_keyid)) + die(_("failed to sign commit object")); + add_header_signature(new_data, &signature, the_hash_algo); + + strbuf_release(&signature); + strbuf_release(&payload); + } finalize_commit_buffer(new_data, NULL, NULL, msg); } else { @@ -2927,6 +2978,7 @@ static void parse_new_commit(const char *arg) /* fallthru */ case SIGN_VERBATIM: case SIGN_STRIP_IF_INVALID: + case SIGN_RE_SIGN_IF_INVALID: import_one_signature(&sig_sha1, &sig_sha256, v); break; @@ -3011,9 +3063,11 @@ static void parse_new_commit(const char *arg) "encoding %s\n", encoding); - if (signed_commit_mode == SIGN_STRIP_IF_INVALID && + if ((signed_commit_mode == SIGN_STRIP_IF_INVALID || + signed_commit_mode == SIGN_RE_SIGN_IF_INVALID) && (sig_sha1.hash_algo || sig_sha256.hash_algo)) - handle_strip_if_invalid(&new_data, &sig_sha1, &sig_sha256, &msg); + handle_signature_if_invalid(&new_data, &sig_sha1, &sig_sha256, + &msg, signed_commit_mode); else finalize_commit_buffer(&new_data, &sig_sha1, &sig_sha256, &msg); @@ -3060,6 +3114,9 @@ static void handle_tag_signature(struct strbuf *msg, const char *name) case SIGN_STRIP_IF_INVALID: die(_("'strip-if-invalid' is not a valid mode for " "git fast-import with --signed-tags=<mode>")); + case SIGN_RE_SIGN_IF_INVALID: + die(_("'re-sign-if-invalid' is not a valid mode for " + "git fast-import with --signed-tags=<mode>")); default: BUG("invalid signed_tag_mode value %d from tag '%s'", signed_tag_mode, name); @@ -3649,10 +3706,10 @@ static int parse_one_option(const char *option) } else if (skip_prefix(option, "export-pack-edges=", &option)) { option_export_pack_edges(option); } else if (skip_prefix(option, "signed-commits=", &option)) { - if (parse_sign_mode(option, &signed_commit_mode)) + if (parse_sign_mode(option, &signed_commit_mode, &signed_commit_keyid)) usagef(_("unknown --signed-commits mode '%s'"), option); } else if (skip_prefix(option, "signed-tags=", &option)) { - if (parse_sign_mode(option, &signed_tag_mode)) + if (parse_sign_mode(option, &signed_tag_mode, NULL)) usagef(_("unknown --signed-tags mode '%s'"), option); } else if (!strcmp(option, "quiet")) { show_stats = 0; diff --git a/gpg-interface.c b/gpg-interface.c index a72fa35061..2570b641f2 100644 --- a/gpg-interface.c +++ b/gpg-interface.c @@ -1155,21 +1155,28 @@ static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature, return ret; } -int parse_sign_mode(const char *arg, enum sign_mode *mode) +int parse_sign_mode(const char *arg, enum sign_mode *mode, const char **keyid) { - if (!strcmp(arg, "abort")) + if (!strcmp(arg, "abort")) { *mode = SIGN_ABORT; - else if (!strcmp(arg, "verbatim") || !strcmp(arg, "ignore")) + } else if (!strcmp(arg, "verbatim") || !strcmp(arg, "ignore")) { *mode = SIGN_VERBATIM; - else if (!strcmp(arg, "warn-verbatim") || !strcmp(arg, "warn")) + } else if (!strcmp(arg, "warn-verbatim") || !strcmp(arg, "warn")) { *mode = SIGN_WARN_VERBATIM; - else if (!strcmp(arg, "warn-strip")) + } else if (!strcmp(arg, "warn-strip")) { *mode = SIGN_WARN_STRIP; - else if (!strcmp(arg, "strip")) + } else if (!strcmp(arg, "strip")) { *mode = SIGN_STRIP; - else if (!strcmp(arg, "strip-if-invalid")) + } else if (!strcmp(arg, "strip-if-invalid")) { *mode = SIGN_STRIP_IF_INVALID; - else + } else if (!strcmp(arg, "re-sign-if-invalid")) { + *mode = SIGN_RE_SIGN_IF_INVALID; + } else if (skip_prefix(arg, "re-sign-if-invalid=", &arg)) { + *mode = SIGN_RE_SIGN_IF_INVALID; + if (keyid) + *keyid = arg; + } else { return -1; + } return 0; } diff --git a/gpg-interface.h b/gpg-interface.h index a32741aeda..e9f451f366 100644 --- a/gpg-interface.h +++ b/gpg-interface.h @@ -119,12 +119,15 @@ enum sign_mode { SIGN_WARN_STRIP, SIGN_STRIP, SIGN_STRIP_IF_INVALID, + SIGN_RE_SIGN_IF_INVALID, }; /* * Return 0 if `arg` can be parsed into an `enum sign_mode`. Return -1 - * otherwise. + * otherwise. If the parsed mode is SIGN_RE_SIGN_IF_INVALID and GPG key provided + * in the arguments in the form `re-sign-if-invalid=<keyid>`, the key-ID is + * parsed into `char **keyid`. */ -int parse_sign_mode(const char *arg, enum sign_mode *mode); +int parse_sign_mode(const char *arg, enum sign_mode *mode, const char **keyid); #endif diff --git a/t/t9305-fast-import-signatures.sh b/t/t9305-fast-import-signatures.sh index 022dae02e4..2a3f04b42d 100755 --- a/t/t9305-fast-import-signatures.sh +++ b/t/t9305-fast-import-signatures.sh @@ -103,26 +103,85 @@ test_expect_success GPG 'strip both OpenPGP signatures with --signed-commits=war test_line_count = 2 out ' -test_expect_success GPG 'import commit with no signature with --signed-commits=strip-if-invalid' ' - git fast-export main >output && - git -C new fast-import --quiet --signed-commits=strip-if-invalid <output >log 2>&1 && - test_must_be_empty log -' - -test_expect_success GPG 'keep valid OpenPGP signature with --signed-commits=strip-if-invalid' ' - rm -rf new && - git init new && - - git fast-export --signed-commits=verbatim openpgp-signing >output && - git -C new fast-import --quiet --signed-commits=strip-if-invalid <output >log 2>&1 && - IMPORTED=$(git -C new rev-parse --verify refs/heads/openpgp-signing) && - test $OPENPGP_SIGNING = $IMPORTED && - git -C new cat-file commit "$IMPORTED" >actual && - test_grep -E "^gpgsig(-sha256)? " actual && - test_must_be_empty log -' - -test_expect_success GPG 'strip signature invalidated by message change with --signed-commits=strip-if-invalid' ' +for mode in strip-if-invalid re-sign-if-invalid +do + test_expect_success GPG "import commit with no signature with --signed-commits=$mode" ' + git fast-export main >output && + git -C new fast-import --quiet --signed-commits=$mode <output >log 2>&1 && + test_must_be_empty log + ' + + test_expect_success GPG "keep valid OpenPGP signature with --signed-commits=$mode" ' + rm -rf new && + git init new && + + git fast-export --signed-commits=verbatim openpgp-signing >output && + git -C new fast-import --quiet --signed-commits=$mode <output >log 2>&1 && + IMPORTED=$(git -C new rev-parse --verify refs/heads/openpgp-signing) && + test $OPENPGP_SIGNING = $IMPORTED && + git -C new cat-file commit "$IMPORTED" >actual && + test_grep -E "^gpgsig(-sha256)? " actual && + test_must_be_empty log + ' + + test_expect_success GPG "handle signature invalidated by message change with --signed-commits=$mode" ' + rm -rf new && + git init new && + + git fast-export --signed-commits=verbatim openpgp-signing >output && + + # Change the commit message, which invalidates the signature. + # The commit message length should not change though, otherwise the + # corresponding `data <length>` command would have to be changed too. + sed "s/OpenPGP signed commit/OpenPGP forged commit/" output >modified && + + git -C new fast-import --quiet --signed-commits=$mode <modified >log 2>&1 && + + IMPORTED=$(git -C new rev-parse --verify refs/heads/openpgp-signing) && + test $OPENPGP_SIGNING != $IMPORTED && + git -C new cat-file commit "$IMPORTED" >actual && + + if test "$mode" = strip-if-invalid + then + test_grep "stripping invalid signature" log && + test_grep ! -E "^gpgsig" actual + else + test_grep "re-signing invalid signature" log && + test_grep -E "^gpgsig(-sha256)? " actual && + git -C new verify-commit "$IMPORTED" + fi + ' + + test_expect_success GPGSM "keep valid X.509 signature with --signed-commits=$mode" ' + rm -rf new && + git init new && + + git fast-export --signed-commits=verbatim x509-signing >output && + git -C new fast-import --quiet --signed-commits=$mode <output >log 2>&1 && + IMPORTED=$(git -C new rev-parse --verify refs/heads/x509-signing) && + test $X509_SIGNING = $IMPORTED && + git -C new cat-file commit "$IMPORTED" >actual && + test_grep -E "^gpgsig(-sha256)? " actual && + test_must_be_empty log + ' + + test_expect_success GPGSSH "keep valid SSH signature with --signed-commits=$mode" ' + rm -rf new && + git init new && + + test_config -C new gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && + + git fast-export --signed-commits=verbatim ssh-signing >output && + git -C new fast-import --quiet --signed-commits=$mode <output >log 2>&1 && + IMPORTED=$(git -C new rev-parse --verify refs/heads/ssh-signing) && + test $SSH_SIGNING = $IMPORTED && + git -C new cat-file commit "$IMPORTED" >actual && + test_grep -E "^gpgsig(-sha256)? " actual && + test_must_be_empty log + ' +done + +test_expect_success GPGSSH "re-sign invalid commit with explicit keyid" ' rm -rf new && git init new && @@ -133,41 +192,22 @@ test_expect_success GPG 'strip signature invalidated by message change with --si # corresponding `data <length>` command would have to be changed too. sed "s/OpenPGP signed commit/OpenPGP forged commit/" output >modified && - git -C new fast-import --quiet --signed-commits=strip-if-invalid <modified >log 2>&1 && + # Configure the target repository with an invalid default signing key. + test_config -C new user.signingkey "not-a-real-key-id" && + test_config -C new gpg.format ssh && + test_config -C new gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && + test_must_fail git -C new fast-import --quiet \ + --signed-commits=re-sign-if-invalid <modified >/dev/null 2>&1 && + + # Import using explicitly provided signing key. + git -C new fast-import --quiet \ + --signed-commits=re-sign-if-invalid="${GPGSSH_KEY_PRIMARY}" <modified && IMPORTED=$(git -C new rev-parse --verify refs/heads/openpgp-signing) && test $OPENPGP_SIGNING != $IMPORTED && git -C new cat-file commit "$IMPORTED" >actual && - test_grep ! -E "^gpgsig" actual && - test_grep "stripping invalid signature" log -' - -test_expect_success GPGSM 'keep valid X.509 signature with --signed-commits=strip-if-invalid' ' - rm -rf new && - git init new && - - git fast-export --signed-commits=verbatim x509-signing >output && - git -C new fast-import --quiet --signed-commits=strip-if-invalid <output >log 2>&1 && - IMPORTED=$(git -C new rev-parse --verify refs/heads/x509-signing) && - test $X509_SIGNING = $IMPORTED && - git -C new cat-file commit "$IMPORTED" >actual && test_grep -E "^gpgsig(-sha256)? " actual && - test_must_be_empty log -' - -test_expect_success GPGSSH 'keep valid SSH signature with --signed-commits=strip-if-invalid' ' - rm -rf new && - git init new && - - test_config -C new gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && - - git fast-export --signed-commits=verbatim ssh-signing >output && - git -C new fast-import --quiet --signed-commits=strip-if-invalid <output >log 2>&1 && - IMPORTED=$(git -C new rev-parse --verify refs/heads/ssh-signing) && - test $SSH_SIGNING = $IMPORTED && - git -C new cat-file commit "$IMPORTED" >actual && - test_grep -E "^gpgsig(-sha256)? " actual && - test_must_be_empty log + git -C new verify-commit "$IMPORTED" ' test_done -- 2.53.0.381.g628a66ccf6 ^ permalink raw reply related [flat|nested] 60+ messages in thread
* Re: [PATCH v3 0/3] fast-import: add mode to re-sign invalid commit signatures 2026-03-10 20:11 ` [PATCH v3 0/3] " Justin Tobler ` (2 preceding siblings ...) 2026-03-10 20:11 ` [PATCH v3 3/3] fast-import: add mode to re-sign invalid commit signatures Justin Tobler @ 2026-03-10 20:49 ` Junio C Hamano 2026-03-10 21:06 ` Justin Tobler 2026-03-11 17:31 ` [PATCH v4 " Justin Tobler 4 siblings, 1 reply; 60+ messages in thread From: Junio C Hamano @ 2026-03-10 20:49 UTC (permalink / raw) To: Justin Tobler; +Cc: git, sandals, christian.couder, ps Justin Tobler <jltobler@gmail.com> writes: > With c20f112e51 (fast-import: add 'strip-if-invalid' mode to > --signed-commits=<mode>, 2025-11-17), it became possible to remove > invalid signatures from commits via git-fast-import(1) while maintaining > valid commit signatures. Building upon this functionality, a user may > want to re-sign these invalid commit signatures. This series introduces > the `re-sign-if-invalid` mode to do so accordingly. I know that this "re-sign" used to be "resign", and the update is indeed a replacement, but I wonder if we can just say "sign"? When we see a signature on an object we are rewriting, we either "strip" it, or we "sign" it (afresh). It is not like we are retaining the old signature, and signing on top of it. We are discarding the old one so there is no difference from signing the object that never had a signature, no? ^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH v3 0/3] fast-import: add mode to re-sign invalid commit signatures 2026-03-10 20:49 ` [PATCH v3 0/3] " Junio C Hamano @ 2026-03-10 21:06 ` Justin Tobler 2026-03-10 21:20 ` Junio C Hamano 0 siblings, 1 reply; 60+ messages in thread From: Justin Tobler @ 2026-03-10 21:06 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, sandals, christian.couder, ps On 26/03/10 01:49PM, Junio C Hamano wrote: > Justin Tobler <jltobler@gmail.com> writes: > > > With c20f112e51 (fast-import: add 'strip-if-invalid' mode to > > --signed-commits=<mode>, 2025-11-17), it became possible to remove > > invalid signatures from commits via git-fast-import(1) while maintaining > > valid commit signatures. Building upon this functionality, a user may > > want to re-sign these invalid commit signatures. This series introduces > > the `re-sign-if-invalid` mode to do so accordingly. > > I know that this "re-sign" used to be "resign", and the update is > indeed a replacement, but I wonder if we can just say "sign"? > > When we see a signature on an object we are rewriting, we either > "strip" it, or we "sign" it (afresh). It is not like we are > retaining the old signature, and signing on top of it. We are > discarding the old one so there is no difference from signing the > object that never had a signature, no? From my perspective, "re-sign" implies that the signature was previously signed, but we are now going to sign it again. Indeed, the resulting commit signing is functionally the same as if the object never had a previous signature though. Also, "if-invalid" already implies that the object is signed, but its signature is invalid. So it could be argued that "re-sign" is already redundant. Ultimately, I don't feel super strongly, but I can send another version that changes this option to "sign-if-invalid". It's probably a bit simpler this way too. I guess the enum value would need to be changed to "SIGN_SIGN_IF_INVALID"? Or maybe just "SIGN_IF_INVALID"? Thanks, -Justin ^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH v3 0/3] fast-import: add mode to re-sign invalid commit signatures 2026-03-10 21:06 ` Justin Tobler @ 2026-03-10 21:20 ` Junio C Hamano 2026-03-10 22:13 ` Justin Tobler 0 siblings, 1 reply; 60+ messages in thread From: Junio C Hamano @ 2026-03-10 21:20 UTC (permalink / raw) To: Justin Tobler; +Cc: git, sandals, christian.couder, ps Justin Tobler <jltobler@gmail.com> writes: > From my perspective, "re-sign" implies that the signature was previously > signed, but we are now going to sign it again. Indeed, the resulting > commit signing is functionally the same as if the object never had a > previous signature though. Also, "if-invalid" already implies that the > object is signed, but its signature is invalid. So it could be argued > that "re-sign" is already redundant. Yup. if-invalid part indeed was why I thought "re-" was redundant. Also, if a project is redoing its history with such a bulk operation, I wonder if it _still_ makes sense to tie this re-signing to the --signed-{tags,commits} option. Adding signature to commits that were not signed is not covered well with the "--signed-commits=<mode>" option. A project may have required that all commits and tags to be signed, in which case "--signed-*=sign-if-invalid" would create a new history with everything freshly signed, but if the original history has signed and unsigned commits, and if they want to sign all the objects while rewriting their history, they may find it more handy if we let them do --signed-commits=strip-if-invalid --sign-commits i.e., drop the invalid ones and make sure all commits are signed. ^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH v3 0/3] fast-import: add mode to re-sign invalid commit signatures 2026-03-10 21:20 ` Junio C Hamano @ 2026-03-10 22:13 ` Justin Tobler 2026-03-10 22:39 ` Junio C Hamano 0 siblings, 1 reply; 60+ messages in thread From: Justin Tobler @ 2026-03-10 22:13 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, sandals, christian.couder, ps On 26/03/10 02:20PM, Junio C Hamano wrote: > Justin Tobler <jltobler@gmail.com> writes: > > > From my perspective, "re-sign" implies that the signature was previously > > signed, but we are now going to sign it again. Indeed, the resulting > > commit signing is functionally the same as if the object never had a > > previous signature though. Also, "if-invalid" already implies that the > > object is signed, but its signature is invalid. So it could be argued > > that "re-sign" is already redundant. > > Yup. if-invalid part indeed was why I thought "re-" was redundant. > > Also, if a project is redoing its history with such a bulk > operation, I wonder if it _still_ makes sense to tie this re-signing > to the --signed-{tags,commits} option. Adding signature to commits > that were not signed is not covered well with the > "--signed-commits=<mode>" option. Ya, the --signed-{tags,commits} option is really only intended to specify how already signed objects should be handled. Adding a mode to sign unsigned objects likely wouldn't fit well. I do think this "re-signing" mode still makes sense though since it is limited to the subset of objects that were previously signed and the signature invalid. > A project may have required that all commits and tags to be signed, > in which case "--signed-*=sign-if-invalid" would create a new > history with everything freshly signed, but if the original history > has signed and unsigned commits, and if they want to sign all the > objects while rewriting their history, they may find it more handy > if we let them do --signed-commits=strip-if-invalid --sign-commits > i.e., drop the invalid ones and make sure all commits are signed. This certainly seems like a reasonable use case, but if we want to support leaving previously unsigned objects unsigned too, `--signed-commits=strip-if-invalid --signed-commits` wouldn't be granular enough. My thinking is that users may want such targeted object re-signing when bulk rewriting history via tools such as git-filter-repo. I do think that it could make sense to still add a separate `--signed-commits` option in the future though that targets the remaining unsigned objects. Thanks, -Justin ^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH v3 0/3] fast-import: add mode to re-sign invalid commit signatures 2026-03-10 22:13 ` Justin Tobler @ 2026-03-10 22:39 ` Junio C Hamano 2026-03-10 23:03 ` Justin Tobler 0 siblings, 1 reply; 60+ messages in thread From: Junio C Hamano @ 2026-03-10 22:39 UTC (permalink / raw) To: Justin Tobler; +Cc: git, sandals, christian.couder, ps Justin Tobler <jltobler@gmail.com> writes: > This certainly seems like a reasonable use case, but if we want to > support leaving previously unsigned objects unsigned too, > `--signed-commits=strip-if-invalid --signed-commits` wouldn't be > granular enough. Yes, --signed-commits=(re-)sign-if-invalid is a perfect match for that use case. I am just saying that if you add the machinery needed to re-sign, you would be able to reuse it to sign objects that weren't signed in the first place, so that is wherea yet another feature "--sign-commits=all" may fit. ^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH v3 0/3] fast-import: add mode to re-sign invalid commit signatures 2026-03-10 22:39 ` Junio C Hamano @ 2026-03-10 23:03 ` Justin Tobler 0 siblings, 0 replies; 60+ messages in thread From: Justin Tobler @ 2026-03-10 23:03 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, sandals, christian.couder, ps On 26/03/10 03:39PM, Junio C Hamano wrote: > Justin Tobler <jltobler@gmail.com> writes: > > > This certainly seems like a reasonable use case, but if we want to > > support leaving previously unsigned objects unsigned too, > > `--signed-commits=strip-if-invalid --signed-commits` wouldn't be > > granular enough. > > Yes, --signed-commits=(re-)sign-if-invalid is a perfect match for > that use case. I am just saying that if you add the machinery > needed to re-sign, you would be able to reuse it to sign objects > that weren't signed in the first place, so that is wherea yet > another feature "--sign-commits=all" may fit. Ah ok. Ya, adding a signed-commits mode to cover signing all rewritten objects could make sense. I am planning to also add an --abort-if-invalid mode in a followup series. I'll may explore adding this other mode too. Thanks, -Justin ^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH v4 0/3] fast-import: add mode to re-sign invalid commit signatures 2026-03-10 20:11 ` [PATCH v3 0/3] " Justin Tobler ` (3 preceding siblings ...) 2026-03-10 20:49 ` [PATCH v3 0/3] " Junio C Hamano @ 2026-03-11 17:31 ` Justin Tobler 2026-03-11 17:31 ` [PATCH v4 1/3] commit: remove unused forward declaration Justin Tobler ` (3 more replies) 4 siblings, 4 replies; 60+ messages in thread From: Justin Tobler @ 2026-03-11 17:31 UTC (permalink / raw) To: git; +Cc: sandals, christian.couder, ps, gitster, Justin Tobler Greetings, With c20f112e51 (fast-import: add 'strip-if-invalid' mode to --signed-commits=<mode>, 2025-11-17), it became possible to remove invalid signatures from commits via git-fast-import(1) while maintaining valid commit signatures. Building upon this functionality, a user may want to re-sign these invalid commit signatures. This series introduces the `sign-if-invalid` mode to do so accordingly. The newly added mode in this series currently ignores `extensions.compatObjectFormat` when generating the new signatures. From my understanding, to generate the compatibility structure would also require us to reconstruct the compatibility object for the object being signed. I think this would be possible to do, but would require getting the mapped OIDs for the commit parents and tree. I'm not completely sure of a good way to go about this yet though. I'm also not completely certain if this is something that should be addressed as part of this series, or could be done later down the road. So for now I've opted to delay its implementation. I'm open going down the other route if that is preferred though. The first commit is a simple cleanup for something I noticed while reading though commit signing code. The second commit actually introduces the new `--signed-commits` mode. Changes since V3: - Rename the `re-sign-if-invalid` mode to `sign-if-invalid`. The "if-invalid" already implies the signatures was previously signed making "re-sign" redundant. Changes since V2: - Adapted commit message in second patch to improve clarity. - Fixed typos. - Renamed SIGN_RESIGN_IF_INVALID to SIGN_RE_SIGN_IF_INVALID. - Created separate helper function to handle printing invalid signature warnings. Changes since V1: - Improved commit messages and comments to better explain why interoperability mode is not currently supported. - Clarified documentation for re-sign-if-invalid mode. - Renamed `handle_invalid_signature()` to `handle_signature_if_invalid()`. - Added warning messages specific to commit resigning. - Fixed some small typos. - Added support for explicitly specifying the signing key ID via `--signed-commits=re-sign-if-invalid[=<keyid>]` similar to how it can specified in git-commit(1). - We now die() as unsupported when attempting to re-sign an invalid commit signature in interoperability mode. - We now die() when failing to re-sign a commit. Thanks, -Justin Justin Tobler (3): commit: remove unused forward declaration gpg-interface: introduce sign_buffer_with_key() fast-import: add mode to sign commits with invalid signatures Documentation/git-fast-import.adoc | 4 + builtin/fast-export.c | 8 +- builtin/fast-import.c | 100 ++++++++++++++++----- commit.c | 16 +--- commit.h | 2 - gpg-interface.c | 36 ++++++-- gpg-interface.h | 14 ++- t/t9305-fast-import-signatures.sh | 140 ++++++++++++++++++----------- 8 files changed, 221 insertions(+), 99 deletions(-) Range-diff against v3: 1: 0d00b72ee0 = 1: 0d00b72ee0 commit: remove unused forward declaration 2: 0b0a06347d ! 2: 87f590f1f8 gpg-interface: introduce sign_buffer_with_key() @@ Commit message provided and handles generating the commit signature accordingly. This signing operation is not really specific to commits as any arbitrary buffer can be signed. Also, in a subsequent commit, this same logic is - reused by git-fast-import(1) when re-signing invalid commit signatures. + reused by git-fast-import(1) when signing commits with invalid + signatures. Move the `sign_commit_to_strbuf()` helper from "commit.c" to "gpg-interface.c" and rename it to `sign_buffer_with_key()`. Also export 3: 57a27ccc61 ! 3: 8b01ad1570 fast-import: add mode to re-sign invalid commit signatures @@ Metadata Author: Justin Tobler <jltobler@gmail.com> ## Commit message ## - fast-import: add mode to re-sign invalid commit signatures + fast-import: add mode to sign commits with invalid signatures 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 facilitate this, introduce a - "re-sign-if-invalid" mode for the `--signed-commits` option. Optionally, - a key ID may be explicitly provided in the form - `re-sign-if-invalid[=<keyid>]` to specify which signing key should be - used when re-signing invalid commit signatures. + encountered, a user may want the option to sign the commit again as + opposed to just stripping the signature. To facilitate this, introduce a + "sign-if-invalid" mode for the `--signed-commits` option. Optionally, a + key ID may be explicitly provided in the form + `sign-if-invalid[=<keyid>]` to specify which signing key should be used + when signing invalid commit signatures. - Note that to properly support interoperability mode when re-signing - commit signatures, the commit buffer must be created in both the - repository and compatability object formats to generate the appropriate - signatures accordingly. As currently implemented, the commit buffer for - the compatability object format is not reconstructed and thus re-signing + Note that to properly support interoperability mode when signing commit + signatures, the commit buffer must be created in both the repository and + compatability object formats to generate the appropriate signatures + accordingly. As currently implemented, the commit buffer for the + compatability object format is not reconstructed and thus signing commits in interoperability mode is not yet supported. Support may be added in the future. @@ Documentation/git-fast-import.adoc: 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[=<keyid>]`, similar to `strip-if-invalid`, verifies ++* `sign-if-invalid[=<keyid>]`, similar to `strip-if-invalid`, verifies + commit signatures and replaces invalid signatures with newly created ones. + Valid signatures are left unchanged. If `<keyid>` is provided, that key is -+ used for re-signing; otherwise the configured default signing key is used. ++ used for signing; otherwise the configured default signing key is used. Options for Frontends ~~~~~~~~~~~~~~~~~~~~~ @@ builtin/fast-export.c: static void handle_commit(struct commit *commit, struct r case SIGN_STRIP_IF_INVALID: die(_("'strip-if-invalid' is not a valid mode for " "git fast-export with --signed-commits=<mode>")); -+ case SIGN_RE_SIGN_IF_INVALID: -+ die(_("'re-sign-if-invalid' is not a valid mode for " ++ case SIGN_SIGN_IF_INVALID: ++ die(_("'sign-if-invalid' is not a valid mode for " + "git fast-export with --signed-commits=<mode>")); default: BUG("invalid signed_commit_mode value %d", signed_commit_mode); @@ builtin/fast-export.c: static void handle_tag(const char *name, struct tag *tag) case SIGN_STRIP_IF_INVALID: die(_("'strip-if-invalid' is not a valid mode for " "git fast-export with --signed-tags=<mode>")); -+ case SIGN_RE_SIGN_IF_INVALID: -+ die(_("'re-sign-if-invalid' is not a valid mode for " ++ case SIGN_SIGN_IF_INVALID: ++ die(_("'sign-if-invalid' is not a valid mode for " + "git fast-export with --signed-tags=<mode>")); default: BUG("invalid signed_commit_mode value %d", signed_commit_mode); @@ builtin/fast-import.c: static void handle_strip_if_invalid(struct strbuf *new_da warning(_("stripping invalid signature for commit\n" " allegedly by %s"), signer); + break; -+ case SIGN_RE_SIGN_IF_INVALID: ++ case SIGN_SIGN_IF_INVALID: + if (subject_len > 100) -+ warning(_("re-signing invalid signature for commit '%.100s...'\n" ++ warning(_("signing commit with invalid signature for '%.100s...'\n" + " allegedly by %s"), subject, signer); + else if (subject_len > 0) -+ warning(_("re-signing invalid signature for commit '%.*s'\n" ++ warning(_("signing commit with invalid signature for '%.*s'\n" + " allegedly by %s"), subject_len, subject, signer); + else -+ warning(_("re-signing invalid signature for commit\n" ++ warning(_("signing commit with invalid signature\n" + " allegedly by %s"), signer); + break; + default: @@ builtin/fast-import.c: static void handle_strip_if_invalid(struct strbuf *new_da + if (ret) { + warn_invalid_signature(&signature_check, msg->buf, mode); + -+ if (mode == SIGN_RE_SIGN_IF_INVALID) { ++ if (mode == SIGN_SIGN_IF_INVALID) { + struct strbuf signature = STRBUF_INIT; + struct strbuf payload = STRBUF_INIT; + + /* + * NEEDSWORK: To properly support interoperability mode -+ * when re-signing commit signatures, the commit buffer ++ * 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 re-sign commit -+ * signatures in interoperability mode is currently -+ * unsupported. ++ * generated. Thus, attempting to sign commit signatures ++ * in interoperability mode is currently unsupported. + */ + if (the_repository->compat_hash_algo) -+ die(_("re-signing signatures in interoperability mode is unsupported")); ++ die(_("signing signatures in interoperability mode is unsupported")); + + strbuf_addstr(&payload, signature_check.payload); + if (sign_buffer_with_key(&payload, &signature, signed_commit_keyid)) @@ builtin/fast-import.c: static void parse_new_commit(const char *arg) /* fallthru */ case SIGN_VERBATIM: case SIGN_STRIP_IF_INVALID: -+ case SIGN_RE_SIGN_IF_INVALID: ++ case SIGN_SIGN_IF_INVALID: import_one_signature(&sig_sha1, &sig_sha256, v); break; @@ builtin/fast-import.c: static void parse_new_commit(const char *arg) - if (signed_commit_mode == SIGN_STRIP_IF_INVALID && + if ((signed_commit_mode == SIGN_STRIP_IF_INVALID || -+ signed_commit_mode == SIGN_RE_SIGN_IF_INVALID) && ++ signed_commit_mode == SIGN_SIGN_IF_INVALID) && (sig_sha1.hash_algo || sig_sha256.hash_algo)) - handle_strip_if_invalid(&new_data, &sig_sha1, &sig_sha256, &msg); + handle_signature_if_invalid(&new_data, &sig_sha1, &sig_sha256, @@ builtin/fast-import.c: static void handle_tag_signature(struct strbuf *msg, cons case SIGN_STRIP_IF_INVALID: die(_("'strip-if-invalid' is not a valid mode for " "git fast-import with --signed-tags=<mode>")); -+ case SIGN_RE_SIGN_IF_INVALID: -+ die(_("'re-sign-if-invalid' is not a valid mode for " ++ case SIGN_SIGN_IF_INVALID: ++ die(_("'sign-if-invalid' is not a valid mode for " + "git fast-import with --signed-tags=<mode>")); default: BUG("invalid signed_tag_mode value %d from tag '%s'", @@ gpg-interface.c: static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf + } else if (!strcmp(arg, "strip-if-invalid")) { *mode = SIGN_STRIP_IF_INVALID; - else -+ } else if (!strcmp(arg, "re-sign-if-invalid")) { -+ *mode = SIGN_RE_SIGN_IF_INVALID; -+ } else if (skip_prefix(arg, "re-sign-if-invalid=", &arg)) { -+ *mode = SIGN_RE_SIGN_IF_INVALID; ++ } else if (!strcmp(arg, "sign-if-invalid")) { ++ *mode = SIGN_SIGN_IF_INVALID; ++ } else if (skip_prefix(arg, "sign-if-invalid=", &arg)) { ++ *mode = SIGN_SIGN_IF_INVALID; + if (keyid) + *keyid = arg; + } else { @@ gpg-interface.h: enum sign_mode { SIGN_WARN_STRIP, SIGN_STRIP, SIGN_STRIP_IF_INVALID, -+ SIGN_RE_SIGN_IF_INVALID, ++ SIGN_SIGN_IF_INVALID, }; /* * Return 0 if `arg` can be parsed into an `enum sign_mode`. Return -1 - * otherwise. -+ * otherwise. If the parsed mode is SIGN_RE_SIGN_IF_INVALID and GPG key provided -+ * in the arguments in the form `re-sign-if-invalid=<keyid>`, the key-ID is -+ * parsed into `char **keyid`. ++ * otherwise. If the parsed mode is SIGN_SIGN_IF_INVALID and GPG key provided in ++ * the arguments in the form `sign-if-invalid=<keyid>`, the key-ID is parsed ++ * into `char **keyid`. */ -int parse_sign_mode(const char *arg, enum sign_mode *mode); +int parse_sign_mode(const char *arg, enum sign_mode *mode, const char **keyid); @@ t/t9305-fast-import-signatures.sh: test_expect_success GPG 'strip both OpenPGP s -' - -test_expect_success GPG 'strip signature invalidated by message change with --signed-commits=strip-if-invalid' ' -+for mode in strip-if-invalid re-sign-if-invalid ++for mode in strip-if-invalid sign-if-invalid +do + test_expect_success GPG "import commit with no signature with --signed-commits=$mode" ' + git fast-export main >output && @@ t/t9305-fast-import-signatures.sh: test_expect_success GPG 'strip both OpenPGP s + test_grep "stripping invalid signature" log && + test_grep ! -E "^gpgsig" actual + else -+ test_grep "re-signing invalid signature" log && ++ test_grep "signing commit with invalid signature" log && + test_grep -E "^gpgsig(-sha256)? " actual && + git -C new verify-commit "$IMPORTED" + fi @@ t/t9305-fast-import-signatures.sh: test_expect_success GPG 'strip both OpenPGP s + ' +done + -+test_expect_success GPGSSH "re-sign invalid commit with explicit keyid" ' ++test_expect_success GPGSSH "sign invalid commit with explicit keyid" ' rm -rf new && git init new && @@ t/t9305-fast-import-signatures.sh: test_expect_success GPG 'strip signature inva + test_config -C new gpg.format ssh && + test_config -C new gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && + test_must_fail git -C new fast-import --quiet \ -+ --signed-commits=re-sign-if-invalid <modified >/dev/null 2>&1 && ++ --signed-commits=sign-if-invalid <modified >/dev/null 2>&1 && + + # Import using explicitly provided signing key. + git -C new fast-import --quiet \ -+ --signed-commits=re-sign-if-invalid="${GPGSSH_KEY_PRIMARY}" <modified && ++ --signed-commits=sign-if-invalid="${GPGSSH_KEY_PRIMARY}" <modified && IMPORTED=$(git -C new rev-parse --verify refs/heads/openpgp-signing) && test $OPENPGP_SIGNING != $IMPORTED && base-commit: 7c02d39fc2ed2702223c7674f73150d9a7e61ba4 -- 2.53.0.381.g628a66ccf6 ^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH v4 1/3] commit: remove unused forward declaration 2026-03-11 17:31 ` [PATCH v4 " Justin Tobler @ 2026-03-11 17:31 ` Justin Tobler 2026-03-11 17:31 ` [PATCH v4 2/3] gpg-interface: introduce sign_buffer_with_key() Justin Tobler ` (2 subsequent siblings) 3 siblings, 0 replies; 60+ messages in thread From: Justin Tobler @ 2026-03-11 17:31 UTC (permalink / raw) To: git; +Cc: sandals, christian.couder, ps, gitster, Justin Tobler In 6206089cbd (commit: write commits for both hashes, 2023-10-01), `sign_with_header()` was removed, but its forward declaration in "commit.h" was left. Remove the unused declaration. Signed-off-by: Justin Tobler <jltobler@gmail.com> --- commit.h | 2 -- 1 file changed, 2 deletions(-) diff --git a/commit.h b/commit.h index 1635de418b..f0c38cb444 100644 --- a/commit.h +++ b/commit.h @@ -390,8 +390,6 @@ LAST_ARG_MUST_BE_NULL int run_commit_hook(int editor_is_used, const char *index_file, int *invoked_hook, const char *name, ...); -/* Sign a commit or tag buffer, storing the result in a header. */ -int sign_with_header(struct strbuf *buf, const char *keyid); /* Parse the signature out of a header. */ int parse_buffer_signed_by_header(const char *buffer, unsigned long size, -- 2.53.0.381.g628a66ccf6 ^ permalink raw reply related [flat|nested] 60+ messages in thread
* [PATCH v4 2/3] gpg-interface: introduce sign_buffer_with_key() 2026-03-11 17:31 ` [PATCH v4 " Justin Tobler 2026-03-11 17:31 ` [PATCH v4 1/3] commit: remove unused forward declaration Justin Tobler @ 2026-03-11 17:31 ` Justin Tobler 2026-03-12 10:22 ` Patrick Steinhardt 2026-03-11 17:31 ` [PATCH v4 3/3] fast-import: add mode to sign commits with invalid signatures Justin Tobler 2026-03-12 19:22 ` [PATCH v5 0/3] fast-import: add mode to re-sign invalid commit signatures Justin Tobler 3 siblings, 1 reply; 60+ messages in thread From: Justin Tobler @ 2026-03-11 17:31 UTC (permalink / raw) To: git; +Cc: sandals, christian.couder, ps, gitster, Justin Tobler The `sign_commit_to_strbuf()` helper in "commit.c" provides fallback logic to get the default configured signing key when a key is not provided and handles generating the commit signature accordingly. This signing operation is not really specific to commits as any arbitrary buffer can be signed. Also, in a subsequent commit, this same logic is reused by git-fast-import(1) when signing commits with invalid signatures. Move the `sign_commit_to_strbuf()` helper from "commit.c" to "gpg-interface.c" and rename it to `sign_buffer_with_key()`. Also export this function so it can be used by "commit.c" and "builtin/fast-import.c" in the subsequent commit. Signed-off-by: Justin Tobler <jltobler@gmail.com> --- commit.c | 16 ++-------------- gpg-interface.c | 13 +++++++++++++ gpg-interface.h | 7 +++++++ 3 files changed, 22 insertions(+), 14 deletions(-) diff --git a/commit.c b/commit.c index d16ae73345..1677b1ef25 100644 --- a/commit.c +++ b/commit.c @@ -1148,18 +1148,6 @@ int add_header_signature(struct strbuf *buf, struct strbuf *sig, const struct gi return 0; } -static int sign_commit_to_strbuf(struct strbuf *sig, struct strbuf *buf, const char *keyid) -{ - char *keyid_to_free = NULL; - int ret = 0; - if (!keyid || !*keyid) - keyid = keyid_to_free = get_signing_key(); - if (sign_buffer(buf, sig, keyid)) - ret = -1; - free(keyid_to_free); - return ret; -} - int parse_signed_commit(const struct commit *commit, struct strbuf *payload, struct strbuf *signature, const struct git_hash_algo *algop) @@ -1737,7 +1725,7 @@ int commit_tree_extended(const char *msg, size_t msg_len, oidcpy(&parent_buf[i++], &p->item->object.oid); write_commit_tree(&buffer, msg, msg_len, tree, parent_buf, nparents, author, committer, extra); - if (sign_commit && sign_commit_to_strbuf(&sig, &buffer, sign_commit)) { + if (sign_commit && sign_buffer_with_key(&buffer, &sig, sign_commit)) { result = -1; goto out; } @@ -1769,7 +1757,7 @@ int commit_tree_extended(const char *msg, size_t msg_len, free_commit_extra_headers(compat_extra); free(mapped_parents); - if (sign_commit && sign_commit_to_strbuf(&compat_sig, &compat_buffer, sign_commit)) { + if (sign_commit && sign_buffer_with_key(&compat_buffer, &compat_sig, sign_commit)) { result = -1; goto out; } diff --git a/gpg-interface.c b/gpg-interface.c index 87fb6605fb..a72fa35061 100644 --- a/gpg-interface.c +++ b/gpg-interface.c @@ -980,6 +980,19 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *sig return use_format->sign_buffer(buffer, signature, signing_key); } +int sign_buffer_with_key(struct strbuf *buffer, struct strbuf *signature, + const char *signing_key) +{ + char *keyid_to_free = NULL; + int ret = 0; + if (!signing_key || !*signing_key) + signing_key = keyid_to_free = get_signing_key(); + if (sign_buffer(buffer, signature, signing_key)) + ret = -1; + free(keyid_to_free); + return ret; +} + /* * Strip CR from the line endings, in case we are on Windows. * NEEDSWORK: make it trim only CRs before LFs and rename diff --git a/gpg-interface.h b/gpg-interface.h index 789d1ffac4..a32741aeda 100644 --- a/gpg-interface.h +++ b/gpg-interface.h @@ -83,6 +83,13 @@ size_t parse_signed_buffer(const char *buf, size_t size); int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *signing_key); +/* + * Similar to `sign_buffer()`, but uses the default configured signing key as + * returned by `get_signing_key()` when the provided "signing_key" is NULL or + * empty. Returns 0 on success, non-zero on failure. + */ +int sign_buffer_with_key(struct strbuf *buffer, struct strbuf *signature, + const char *signing_key); /* * Returns corresponding string in lowercase for a given member of -- 2.53.0.381.g628a66ccf6 ^ permalink raw reply related [flat|nested] 60+ messages in thread
* Re: [PATCH v4 2/3] gpg-interface: introduce sign_buffer_with_key() 2026-03-11 17:31 ` [PATCH v4 2/3] gpg-interface: introduce sign_buffer_with_key() Justin Tobler @ 2026-03-12 10:22 ` Patrick Steinhardt 2026-03-12 13:58 ` Justin Tobler 0 siblings, 1 reply; 60+ messages in thread From: Patrick Steinhardt @ 2026-03-12 10:22 UTC (permalink / raw) To: Justin Tobler; +Cc: git, sandals, christian.couder, gitster On Wed, Mar 11, 2026 at 12:31:46PM -0500, Justin Tobler wrote: > diff --git a/gpg-interface.h b/gpg-interface.h > index 789d1ffac4..a32741aeda 100644 > --- a/gpg-interface.h > +++ b/gpg-interface.h > @@ -83,6 +83,13 @@ size_t parse_signed_buffer(const char *buf, size_t size); > int sign_buffer(struct strbuf *buffer, struct strbuf *signature, > const char *signing_key); > > +/* > + * Similar to `sign_buffer()`, but uses the default configured signing key as > + * returned by `get_signing_key()` when the provided "signing_key" is NULL or > + * empty. Returns 0 on success, non-zero on failure. > + */ > +int sign_buffer_with_key(struct strbuf *buffer, struct strbuf *signature, > + const char *signing_key); I think this interface is a bit confusing, as you wouldn't really be able to tell what the difference between `sign_buffer()` and `sign_buffer_with_key()` is without having a deeper look. Naively, I would expect the latter function to be the one that actually mandates that the user provides a key, but it's the other way round. Would it be preferable to instead extend `sign_buffer()` to take a flags parameter and then introduce `SIGN_BUFFER_USE_DEFAULT_KEY` to make it fall back to the configured signing key? If so, we could drop `sign_commit_to_strbuf()` completely. Patrick ^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH v4 2/3] gpg-interface: introduce sign_buffer_with_key() 2026-03-12 10:22 ` Patrick Steinhardt @ 2026-03-12 13:58 ` Justin Tobler 0 siblings, 0 replies; 60+ messages in thread From: Justin Tobler @ 2026-03-12 13:58 UTC (permalink / raw) To: Patrick Steinhardt; +Cc: git, sandals, christian.couder, gitster On 26/03/12 11:22AM, Patrick Steinhardt wrote: > On Wed, Mar 11, 2026 at 12:31:46PM -0500, Justin Tobler wrote: > > diff --git a/gpg-interface.h b/gpg-interface.h > > index 789d1ffac4..a32741aeda 100644 > > --- a/gpg-interface.h > > +++ b/gpg-interface.h > > @@ -83,6 +83,13 @@ size_t parse_signed_buffer(const char *buf, size_t size); > > int sign_buffer(struct strbuf *buffer, struct strbuf *signature, > > const char *signing_key); > > > > +/* > > + * Similar to `sign_buffer()`, but uses the default configured signing key as > > + * returned by `get_signing_key()` when the provided "signing_key" is NULL or > > + * empty. Returns 0 on success, non-zero on failure. > > + */ > > +int sign_buffer_with_key(struct strbuf *buffer, struct strbuf *signature, > > + const char *signing_key); > > I think this interface is a bit confusing, as you wouldn't really be > able to tell what the difference between `sign_buffer()` and > `sign_buffer_with_key()` is without having a deeper look. Naively, I > would expect the latter function to be the one that actually mandates > that the user provides a key, but it's the other way round. > > Would it be preferable to instead extend `sign_buffer()` to take a flags > parameter and then introduce `SIGN_BUFFER_USE_DEFAULT_KEY` to make it > fall back to the configured signing key? If so, we could drop > `sign_commit_to_strbuf()` completely. That's fair, and this suggestion sounds completely sensible to me. There are only a handful to `sign_buffer()` callers, so it should create too much churn either. Will send another version adapted accordingly. -Justin ^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH v4 3/3] fast-import: add mode to sign commits with invalid signatures 2026-03-11 17:31 ` [PATCH v4 " Justin Tobler 2026-03-11 17:31 ` [PATCH v4 1/3] commit: remove unused forward declaration Justin Tobler 2026-03-11 17:31 ` [PATCH v4 2/3] gpg-interface: introduce sign_buffer_with_key() Justin Tobler @ 2026-03-11 17:31 ` Justin Tobler 2026-03-12 10:23 ` Patrick Steinhardt 2026-03-12 19:22 ` [PATCH v5 0/3] fast-import: add mode to re-sign invalid commit signatures Justin Tobler 3 siblings, 1 reply; 60+ messages in thread From: Justin Tobler @ 2026-03-11 17:31 UTC (permalink / raw) To: git; +Cc: sandals, christian.couder, ps, gitster, Justin Tobler 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 sign the commit again as opposed to just stripping the signature. To facilitate this, introduce a "sign-if-invalid" mode for the `--signed-commits` option. Optionally, a key ID may be explicitly provided in the form `sign-if-invalid[=<keyid>]` to specify which signing key should be used when signing invalid commit signatures. Note that to properly support interoperability mode when signing commit signatures, the commit buffer must be created in both the repository and compatability object formats to generate the appropriate signatures accordingly. As currently implemented, the commit buffer for the compatability object format is not reconstructed and thus signing commits in interoperability mode is not yet supported. Support may be added in the future. Signed-off-by: Justin Tobler <jltobler@gmail.com> --- Documentation/git-fast-import.adoc | 4 + builtin/fast-export.c | 8 +- builtin/fast-import.c | 100 ++++++++++++++++----- gpg-interface.c | 23 +++-- gpg-interface.h | 7 +- t/t9305-fast-import-signatures.sh | 140 ++++++++++++++++++----------- 6 files changed, 199 insertions(+), 83 deletions(-) diff --git a/Documentation/git-fast-import.adoc b/Documentation/git-fast-import.adoc index 479c4081da..b3f42d4637 100644 --- a/Documentation/git-fast-import.adoc +++ b/Documentation/git-fast-import.adoc @@ -86,6 +86,10 @@ 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. +* `sign-if-invalid[=<keyid>]`, similar to `strip-if-invalid`, verifies + commit signatures and replaces invalid signatures with newly created ones. + Valid signatures are left unchanged. If `<keyid>` is provided, that key is + used for signing; otherwise the configured default signing key is used. Options for Frontends ~~~~~~~~~~~~~~~~~~~~~ diff --git a/builtin/fast-export.c b/builtin/fast-export.c index 0c5d2386d8..13621b0d6a 100644 --- a/builtin/fast-export.c +++ b/builtin/fast-export.c @@ -64,7 +64,7 @@ static int parse_opt_sign_mode(const struct option *opt, if (unset) return 0; - if (parse_sign_mode(arg, val)) + if (parse_sign_mode(arg, val, NULL)) return error(_("unknown %s mode: %s"), opt->long_name, arg); return 0; @@ -825,6 +825,9 @@ static void handle_commit(struct commit *commit, struct rev_info *rev, case SIGN_STRIP_IF_INVALID: die(_("'strip-if-invalid' is not a valid mode for " "git fast-export with --signed-commits=<mode>")); + case SIGN_SIGN_IF_INVALID: + die(_("'sign-if-invalid' is not a valid mode for " + "git fast-export with --signed-commits=<mode>")); default: BUG("invalid signed_commit_mode value %d", signed_commit_mode); } @@ -970,6 +973,9 @@ static void handle_tag(const char *name, struct tag *tag) case SIGN_STRIP_IF_INVALID: die(_("'strip-if-invalid' is not a valid mode for " "git fast-export with --signed-tags=<mode>")); + case SIGN_SIGN_IF_INVALID: + die(_("'sign-if-invalid' is not a valid mode for " + "git fast-export with --signed-tags=<mode>")); default: BUG("invalid signed_commit_mode value %d", signed_commit_mode); } 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 @@ -190,6 +190,7 @@ static const char *global_prefix; static enum sign_mode signed_tag_mode = SIGN_VERBATIM; static enum sign_mode signed_commit_mode = SIGN_VERBATIM; +static const char *signed_commit_keyid; /* Memory pools */ static struct mem_pool fi_mem_pool = { @@ -2836,26 +2837,15 @@ 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 warn_invalid_signature(struct signature_check *check, + const char *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) { - const char *signer = signature_check.signer ? - signature_check.signer : _("unknown"); - const char *subject; - int subject_len = find_commit_subject(msg->buf, &subject); + const char *signer = check->signer ? check->signer : _("unknown"); + const char *subject; + int subject_len = find_commit_subject(msg, &subject); + switch (mode) { + case SIGN_STRIP_IF_INVALID: if (subject_len > 100) warning(_("stripping invalid signature for commit '%.100s...'\n" " allegedly by %s"), subject, signer); @@ -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"); + } +} + +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")); + + strbuf_addstr(&payload, signature_check.payload); + if (sign_buffer_with_key(&payload, &signature, signed_commit_keyid)) + die(_("failed to sign commit object")); + add_header_signature(new_data, &signature, the_hash_algo); + + strbuf_release(&signature); + strbuf_release(&payload); + } finalize_commit_buffer(new_data, NULL, NULL, msg); } else { @@ -2927,6 +2977,7 @@ static void parse_new_commit(const char *arg) /* fallthru */ case SIGN_VERBATIM: case SIGN_STRIP_IF_INVALID: + case SIGN_SIGN_IF_INVALID: import_one_signature(&sig_sha1, &sig_sha256, v); break; @@ -3011,9 +3062,11 @@ static void parse_new_commit(const char *arg) "encoding %s\n", encoding); - if (signed_commit_mode == SIGN_STRIP_IF_INVALID && + if ((signed_commit_mode == SIGN_STRIP_IF_INVALID || + signed_commit_mode == SIGN_SIGN_IF_INVALID) && (sig_sha1.hash_algo || sig_sha256.hash_algo)) - handle_strip_if_invalid(&new_data, &sig_sha1, &sig_sha256, &msg); + handle_signature_if_invalid(&new_data, &sig_sha1, &sig_sha256, + &msg, signed_commit_mode); else finalize_commit_buffer(&new_data, &sig_sha1, &sig_sha256, &msg); @@ -3060,6 +3113,9 @@ static void handle_tag_signature(struct strbuf *msg, const char *name) case SIGN_STRIP_IF_INVALID: die(_("'strip-if-invalid' is not a valid mode for " "git fast-import with --signed-tags=<mode>")); + case SIGN_SIGN_IF_INVALID: + die(_("'sign-if-invalid' is not a valid mode for " + "git fast-import with --signed-tags=<mode>")); default: BUG("invalid signed_tag_mode value %d from tag '%s'", signed_tag_mode, name); @@ -3649,10 +3705,10 @@ static int parse_one_option(const char *option) } else if (skip_prefix(option, "export-pack-edges=", &option)) { option_export_pack_edges(option); } else if (skip_prefix(option, "signed-commits=", &option)) { - if (parse_sign_mode(option, &signed_commit_mode)) + if (parse_sign_mode(option, &signed_commit_mode, &signed_commit_keyid)) usagef(_("unknown --signed-commits mode '%s'"), option); } else if (skip_prefix(option, "signed-tags=", &option)) { - if (parse_sign_mode(option, &signed_tag_mode)) + if (parse_sign_mode(option, &signed_tag_mode, NULL)) usagef(_("unknown --signed-tags mode '%s'"), option); } else if (!strcmp(option, "quiet")) { show_stats = 0; diff --git a/gpg-interface.c b/gpg-interface.c index a72fa35061..2dd428ee2c 100644 --- a/gpg-interface.c +++ b/gpg-interface.c @@ -1155,21 +1155,28 @@ static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature, return ret; } -int parse_sign_mode(const char *arg, enum sign_mode *mode) +int parse_sign_mode(const char *arg, enum sign_mode *mode, const char **keyid) { - if (!strcmp(arg, "abort")) + if (!strcmp(arg, "abort")) { *mode = SIGN_ABORT; - else if (!strcmp(arg, "verbatim") || !strcmp(arg, "ignore")) + } else if (!strcmp(arg, "verbatim") || !strcmp(arg, "ignore")) { *mode = SIGN_VERBATIM; - else if (!strcmp(arg, "warn-verbatim") || !strcmp(arg, "warn")) + } else if (!strcmp(arg, "warn-verbatim") || !strcmp(arg, "warn")) { *mode = SIGN_WARN_VERBATIM; - else if (!strcmp(arg, "warn-strip")) + } else if (!strcmp(arg, "warn-strip")) { *mode = SIGN_WARN_STRIP; - else if (!strcmp(arg, "strip")) + } else if (!strcmp(arg, "strip")) { *mode = SIGN_STRIP; - else if (!strcmp(arg, "strip-if-invalid")) + } else if (!strcmp(arg, "strip-if-invalid")) { *mode = SIGN_STRIP_IF_INVALID; - else + } else if (!strcmp(arg, "sign-if-invalid")) { + *mode = SIGN_SIGN_IF_INVALID; + } else if (skip_prefix(arg, "sign-if-invalid=", &arg)) { + *mode = SIGN_SIGN_IF_INVALID; + if (keyid) + *keyid = arg; + } else { return -1; + } return 0; } diff --git a/gpg-interface.h b/gpg-interface.h index a32741aeda..25f6209fcf 100644 --- a/gpg-interface.h +++ b/gpg-interface.h @@ -119,12 +119,15 @@ enum sign_mode { SIGN_WARN_STRIP, SIGN_STRIP, SIGN_STRIP_IF_INVALID, + SIGN_SIGN_IF_INVALID, }; /* * Return 0 if `arg` can be parsed into an `enum sign_mode`. Return -1 - * otherwise. + * otherwise. If the parsed mode is SIGN_SIGN_IF_INVALID and GPG key provided in + * the arguments in the form `sign-if-invalid=<keyid>`, the key-ID is parsed + * into `char **keyid`. */ -int parse_sign_mode(const char *arg, enum sign_mode *mode); +int parse_sign_mode(const char *arg, enum sign_mode *mode, const char **keyid); #endif diff --git a/t/t9305-fast-import-signatures.sh b/t/t9305-fast-import-signatures.sh index 022dae02e4..38b3e3b537 100755 --- a/t/t9305-fast-import-signatures.sh +++ b/t/t9305-fast-import-signatures.sh @@ -103,26 +103,85 @@ test_expect_success GPG 'strip both OpenPGP signatures with --signed-commits=war test_line_count = 2 out ' -test_expect_success GPG 'import commit with no signature with --signed-commits=strip-if-invalid' ' - git fast-export main >output && - git -C new fast-import --quiet --signed-commits=strip-if-invalid <output >log 2>&1 && - test_must_be_empty log -' - -test_expect_success GPG 'keep valid OpenPGP signature with --signed-commits=strip-if-invalid' ' - rm -rf new && - git init new && - - git fast-export --signed-commits=verbatim openpgp-signing >output && - git -C new fast-import --quiet --signed-commits=strip-if-invalid <output >log 2>&1 && - IMPORTED=$(git -C new rev-parse --verify refs/heads/openpgp-signing) && - test $OPENPGP_SIGNING = $IMPORTED && - git -C new cat-file commit "$IMPORTED" >actual && - test_grep -E "^gpgsig(-sha256)? " actual && - test_must_be_empty log -' - -test_expect_success GPG 'strip signature invalidated by message change with --signed-commits=strip-if-invalid' ' +for mode in strip-if-invalid sign-if-invalid +do + test_expect_success GPG "import commit with no signature with --signed-commits=$mode" ' + git fast-export main >output && + git -C new fast-import --quiet --signed-commits=$mode <output >log 2>&1 && + test_must_be_empty log + ' + + test_expect_success GPG "keep valid OpenPGP signature with --signed-commits=$mode" ' + rm -rf new && + git init new && + + git fast-export --signed-commits=verbatim openpgp-signing >output && + git -C new fast-import --quiet --signed-commits=$mode <output >log 2>&1 && + IMPORTED=$(git -C new rev-parse --verify refs/heads/openpgp-signing) && + test $OPENPGP_SIGNING = $IMPORTED && + git -C new cat-file commit "$IMPORTED" >actual && + test_grep -E "^gpgsig(-sha256)? " actual && + test_must_be_empty log + ' + + test_expect_success GPG "handle signature invalidated by message change with --signed-commits=$mode" ' + rm -rf new && + git init new && + + git fast-export --signed-commits=verbatim openpgp-signing >output && + + # Change the commit message, which invalidates the signature. + # The commit message length should not change though, otherwise the + # corresponding `data <length>` command would have to be changed too. + sed "s/OpenPGP signed commit/OpenPGP forged commit/" output >modified && + + git -C new fast-import --quiet --signed-commits=$mode <modified >log 2>&1 && + + IMPORTED=$(git -C new rev-parse --verify refs/heads/openpgp-signing) && + test $OPENPGP_SIGNING != $IMPORTED && + git -C new cat-file commit "$IMPORTED" >actual && + + if test "$mode" = strip-if-invalid + then + test_grep "stripping invalid signature" log && + test_grep ! -E "^gpgsig" actual + else + test_grep "signing commit with invalid signature" log && + test_grep -E "^gpgsig(-sha256)? " actual && + git -C new verify-commit "$IMPORTED" + fi + ' + + test_expect_success GPGSM "keep valid X.509 signature with --signed-commits=$mode" ' + rm -rf new && + git init new && + + git fast-export --signed-commits=verbatim x509-signing >output && + git -C new fast-import --quiet --signed-commits=$mode <output >log 2>&1 && + IMPORTED=$(git -C new rev-parse --verify refs/heads/x509-signing) && + test $X509_SIGNING = $IMPORTED && + git -C new cat-file commit "$IMPORTED" >actual && + test_grep -E "^gpgsig(-sha256)? " actual && + test_must_be_empty log + ' + + test_expect_success GPGSSH "keep valid SSH signature with --signed-commits=$mode" ' + rm -rf new && + git init new && + + test_config -C new gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && + + git fast-export --signed-commits=verbatim ssh-signing >output && + git -C new fast-import --quiet --signed-commits=$mode <output >log 2>&1 && + IMPORTED=$(git -C new rev-parse --verify refs/heads/ssh-signing) && + test $SSH_SIGNING = $IMPORTED && + git -C new cat-file commit "$IMPORTED" >actual && + test_grep -E "^gpgsig(-sha256)? " actual && + test_must_be_empty log + ' +done + +test_expect_success GPGSSH "sign invalid commit with explicit keyid" ' rm -rf new && git init new && @@ -133,41 +192,22 @@ test_expect_success GPG 'strip signature invalidated by message change with --si # corresponding `data <length>` command would have to be changed too. sed "s/OpenPGP signed commit/OpenPGP forged commit/" output >modified && - git -C new fast-import --quiet --signed-commits=strip-if-invalid <modified >log 2>&1 && + # Configure the target repository with an invalid default signing key. + test_config -C new user.signingkey "not-a-real-key-id" && + test_config -C new gpg.format ssh && + test_config -C new gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && + test_must_fail git -C new fast-import --quiet \ + --signed-commits=sign-if-invalid <modified >/dev/null 2>&1 && + + # Import using explicitly provided signing key. + git -C new fast-import --quiet \ + --signed-commits=sign-if-invalid="${GPGSSH_KEY_PRIMARY}" <modified && IMPORTED=$(git -C new rev-parse --verify refs/heads/openpgp-signing) && test $OPENPGP_SIGNING != $IMPORTED && git -C new cat-file commit "$IMPORTED" >actual && - test_grep ! -E "^gpgsig" actual && - test_grep "stripping invalid signature" log -' - -test_expect_success GPGSM 'keep valid X.509 signature with --signed-commits=strip-if-invalid' ' - rm -rf new && - git init new && - - git fast-export --signed-commits=verbatim x509-signing >output && - git -C new fast-import --quiet --signed-commits=strip-if-invalid <output >log 2>&1 && - IMPORTED=$(git -C new rev-parse --verify refs/heads/x509-signing) && - test $X509_SIGNING = $IMPORTED && - git -C new cat-file commit "$IMPORTED" >actual && test_grep -E "^gpgsig(-sha256)? " actual && - test_must_be_empty log -' - -test_expect_success GPGSSH 'keep valid SSH signature with --signed-commits=strip-if-invalid' ' - rm -rf new && - git init new && - - test_config -C new gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && - - git fast-export --signed-commits=verbatim ssh-signing >output && - git -C new fast-import --quiet --signed-commits=strip-if-invalid <output >log 2>&1 && - IMPORTED=$(git -C new rev-parse --verify refs/heads/ssh-signing) && - test $SSH_SIGNING = $IMPORTED && - git -C new cat-file commit "$IMPORTED" >actual && - test_grep -E "^gpgsig(-sha256)? " actual && - test_must_be_empty log + git -C new verify-commit "$IMPORTED" ' test_done -- 2.53.0.381.g628a66ccf6 ^ permalink raw reply related [flat|nested] 60+ messages in thread
* Re: [PATCH v4 3/3] fast-import: add mode to sign commits with invalid signatures 2026-03-11 17:31 ` [PATCH v4 3/3] fast-import: add mode to sign commits with invalid signatures Justin Tobler @ 2026-03-12 10:23 ` Patrick Steinhardt 2026-03-12 14:08 ` Justin Tobler 0 siblings, 1 reply; 60+ messages in thread From: Patrick Steinhardt @ 2026-03-12 10:23 UTC (permalink / raw) To: Justin Tobler; +Cc: git, sandals, christian.couder, gitster 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 ^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH v4 3/3] fast-import: add mode to sign commits with invalid signatures 2026-03-12 10:23 ` Patrick Steinhardt @ 2026-03-12 14:08 ` Justin Tobler 2026-03-12 14:22 ` Patrick Steinhardt 0 siblings, 1 reply; 60+ messages in thread From: Justin Tobler @ 2026-03-12 14:08 UTC (permalink / raw) To: Patrick Steinhardt; +Cc: git, sandals, christian.couder, gitster On 26/03/12 11:23AM, Patrick Steinhardt wrote: > 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. I'm not really against removing these warning as I also agree it creates a bunch of noise. If we get rid of them for "sign-if-invalid" though, shouldn't we also get rid of them for "strip-if-invalid"? If the user asks to strip commits, I figure they would expect such cases as well. If we think removing the warning altogether is sensible, I can add another prepatory commit that simply removes the warning for the "strip-if-invalid" case. > > +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"? Ah yes! Will fix in the next version. Thanks for reading closely :) -Justin ^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH v4 3/3] fast-import: add mode to sign commits with invalid signatures 2026-03-12 14:08 ` Justin Tobler @ 2026-03-12 14:22 ` Patrick Steinhardt 2026-03-12 17:21 ` Justin Tobler 0 siblings, 1 reply; 60+ messages in thread From: Patrick Steinhardt @ 2026-03-12 14:22 UTC (permalink / raw) To: Justin Tobler; +Cc: git, sandals, christian.couder, gitster On Thu, Mar 12, 2026 at 09:08:46AM -0500, Justin Tobler wrote: > On 26/03/12 11:23AM, Patrick Steinhardt wrote: > > 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. > > I'm not really against removing these warning as I also agree it creates > a bunch of noise. If we get rid of them for "sign-if-invalid" though, > shouldn't we also get rid of them for "strip-if-invalid"? If the user > asks to strip commits, I figure they would expect such cases as well. If > we think removing the warning altogether is sensible, I can add another > prepatory commit that simply removes the warning for the > "strip-if-invalid" case. Yeah, it kind of falls into the same space, agreed. As said, I won't insist on changing this. Maybe the right way to approach this is to keep it as-is for now and create a follow-up patch where you propose to strip it from both sites? Patrick ^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH v4 3/3] fast-import: add mode to sign commits with invalid signatures 2026-03-12 14:22 ` Patrick Steinhardt @ 2026-03-12 17:21 ` Justin Tobler 0 siblings, 0 replies; 60+ messages in thread From: Justin Tobler @ 2026-03-12 17:21 UTC (permalink / raw) To: Patrick Steinhardt; +Cc: git, sandals, christian.couder, gitster On 26/03/12 03:22PM, Patrick Steinhardt wrote: > On Thu, Mar 12, 2026 at 09:08:46AM -0500, Justin Tobler wrote: > > On 26/03/12 11:23AM, Patrick Steinhardt wrote: > > > 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. > > > > I'm not really against removing these warning as I also agree it creates > > a bunch of noise. If we get rid of them for "sign-if-invalid" though, > > shouldn't we also get rid of them for "strip-if-invalid"? If the user > > asks to strip commits, I figure they would expect such cases as well. If > > we think removing the warning altogether is sensible, I can add another > > prepatory commit that simply removes the warning for the > > "strip-if-invalid" case. > > Yeah, it kind of falls into the same space, agreed. As said, I won't > insist on changing this. Maybe the right way to approach this is to keep > it as-is for now and create a follow-up patch where you propose to strip > it from both sites? That's fair. I'll leave it as-is for now and submit a separate follow up patch after this gets merged that proposes removing the warnings altogether. We can see what folks think about it there. Thanks, -Justin ^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH v5 0/3] fast-import: add mode to re-sign invalid commit signatures 2026-03-11 17:31 ` [PATCH v4 " Justin Tobler ` (2 preceding siblings ...) 2026-03-11 17:31 ` [PATCH v4 3/3] fast-import: add mode to sign commits with invalid signatures Justin Tobler @ 2026-03-12 19:22 ` Justin Tobler 2026-03-12 19:22 ` [PATCH v5 1/3] commit: remove unused forward declaration Justin Tobler ` (4 more replies) 3 siblings, 5 replies; 60+ messages in thread From: Justin Tobler @ 2026-03-12 19:22 UTC (permalink / raw) To: git; +Cc: sandals, christian.couder, ps, gitster, Justin Tobler Greetings, With c20f112e51 (fast-import: add 'strip-if-invalid' mode to --signed-commits=<mode>, 2025-11-17), it became possible to remove invalid signatures from commits via git-fast-import(1) while maintaining valid commit signatures. Building upon this functionality, a user may want to re-sign these invalid commit signatures. This series introduces the `sign-if-invalid` mode to do so accordingly. The newly added mode in this series currently ignores `extensions.compatObjectFormat` when generating the new signatures. From my understanding, to generate the compatibility structure would also require us to reconstruct the compatibility object for the object being signed. I think this would be possible to do, but would require getting the mapped OIDs for the commit parents and tree. I'm not completely sure of a good way to go about this yet though. I'm also not completely certain if this is something that should be addressed as part of this series, or could be done later down the road. So for now I've opted to delay its implementation. I'm open going down the other route if that is preferred though. The first commit is a simple cleanup for something I noticed while reading though commit signing code. The second commit actually introduces the new `--signed-commits` mode. Changes since V4: - Instead of introducing a separate `sign_buffer_with_key()` helper, extend `sign_buffer()` to support a SIGN_BUFFER_USE_DEFAULT_KEY flag. - Fixed message in die(). Changes since V3: - Rename the `re-sign-if-invalid` mode to `sign-if-invalid`. The "if-invalid" already implies the signatures was previously signed making "re-sign" redundant. Changes since V2: - Adapted commit message in second patch to improve clarity. - Fixed typos. - Renamed SIGN_RESIGN_IF_INVALID to SIGN_RE_SIGN_IF_INVALID. - Created separate helper function to handle printing invalid signature warnings. Changes since V1: - Improved commit messages and comments to better explain why interoperability mode is not currently supported. - Clarified documentation for re-sign-if-invalid mode. - Renamed `handle_invalid_signature()` to `handle_signature_if_invalid()`. - Added warning messages specific to commit resigning. - Fixed some small typos. - Added support for explicitly specifying the signing key ID via `--signed-commits=re-sign-if-invalid[=<keyid>]` similar to how it can specified in git-commit(1). - We now die() as unsupported when attempting to re-sign an invalid commit signature in interoperability mode. - We now die() when failing to re-sign a commit. Thanks, -Justin Justin Tobler (3): commit: remove unused forward declaration gpg-interface: allow sign_buffer() to use default signing key fast-import: add mode to sign commits with invalid signatures Documentation/git-fast-import.adoc | 4 + builtin/fast-export.c | 8 +- builtin/fast-import.c | 101 ++++++++++++++++----- builtin/tag.c | 4 +- commit.c | 19 ++-- commit.h | 2 - gpg-interface.c | 36 +++++--- gpg-interface.h | 19 +++- send-pack.c | 2 +- t/t9305-fast-import-signatures.sh | 140 ++++++++++++++++++----------- 10 files changed, 229 insertions(+), 106 deletions(-) Range-diff against v4: 1: 0d00b72ee0 = 1: 0d00b72ee0 commit: remove unused forward declaration 2: 87f590f1f8 ! 2: 7a0deed77b gpg-interface: introduce sign_buffer_with_key() @@ Metadata Author: Justin Tobler <jltobler@gmail.com> ## Commit message ## - gpg-interface: introduce sign_buffer_with_key() + gpg-interface: allow sign_buffer() to use default signing key The `sign_commit_to_strbuf()` helper in "commit.c" provides fallback logic to get the default configured signing key when a key is not @@ Commit message reused by git-fast-import(1) when signing commits with invalid signatures. - Move the `sign_commit_to_strbuf()` helper from "commit.c" to - "gpg-interface.c" and rename it to `sign_buffer_with_key()`. Also export - this function so it can be used by "commit.c" and - "builtin/fast-import.c" in the subsequent commit. + Remove the `sign_commit_to_strbuf()` helper from "commit.c" and extend + `sign_buffer()` in "gpg-interface.c" to support using the default key as + a fallback when the `SIGN_BUFFER_USE_DEFAULT_KEY` flag is provided. Call + sites are updated accordingly. Signed-off-by: Justin Tobler <jltobler@gmail.com> + ## builtin/tag.c ## +@@ builtin/tag.c: static int do_sign(struct strbuf *buffer, struct object_id **compat_oid, + char *keyid = get_signing_key(); + int ret = -1; + +- if (sign_buffer(buffer, &sig, keyid)) ++ if (sign_buffer(buffer, &sig, keyid, 0)) + goto out; + + if (compat) { +@@ builtin/tag.c: static int do_sign(struct strbuf *buffer, struct object_id **compat_oid, + if (convert_object_file(the_repository ,&compat_buf, algo, compat, + buffer->buf, buffer->len, OBJ_TAG, 1)) + goto out; +- if (sign_buffer(&compat_buf, &compat_sig, keyid)) ++ if (sign_buffer(&compat_buf, &compat_sig, keyid, 0)) + goto out; + add_header_signature(&compat_buf, &sig, algo); + strbuf_addbuf(&compat_buf, &compat_sig); + ## commit.c ## @@ commit.c: int add_header_signature(struct strbuf *buf, struct strbuf *sig, const struct gi return 0; @@ commit.c: int commit_tree_extended(const char *msg, size_t msg_len, write_commit_tree(&buffer, msg, msg_len, tree, parent_buf, nparents, author, committer, extra); - if (sign_commit && sign_commit_to_strbuf(&sig, &buffer, sign_commit)) { -+ if (sign_commit && sign_buffer_with_key(&buffer, &sig, sign_commit)) { ++ if (sign_commit && sign_buffer(&buffer, &sig, sign_commit, ++ SIGN_BUFFER_USE_DEFAULT_KEY)) { result = -1; goto out; } @@ commit.c: int commit_tree_extended(const char *msg, size_t msg_len, free(mapped_parents); - if (sign_commit && sign_commit_to_strbuf(&compat_sig, &compat_buffer, sign_commit)) { -+ if (sign_commit && sign_buffer_with_key(&compat_buffer, &compat_sig, sign_commit)) { ++ if (sign_commit && sign_buffer(&compat_buffer, &compat_sig, ++ sign_commit, ++ SIGN_BUFFER_USE_DEFAULT_KEY)) { result = -1; goto out; } ## gpg-interface.c ## -@@ gpg-interface.c: int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *sig - return use_format->sign_buffer(buffer, signature, signing_key); +@@ gpg-interface.c: const char *gpg_trust_level_to_str(enum signature_trust_level level) + return sigcheck_gpg_trust_level[level].display_key; } -+int sign_buffer_with_key(struct strbuf *buffer, struct strbuf *signature, -+ const char *signing_key) -+{ +-int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *signing_key) ++int sign_buffer(struct strbuf *buffer, struct strbuf *signature, ++ const char *signing_key, enum sign_buffer_flags flags) + { + char *keyid_to_free = NULL; + int ret = 0; -+ if (!signing_key || !*signing_key) ++ + gpg_interface_lazy_init(); + +- return use_format->sign_buffer(buffer, signature, signing_key); ++ if (flags & SIGN_BUFFER_USE_DEFAULT_KEY && (!signing_key || !*signing_key)) + signing_key = keyid_to_free = get_signing_key(); -+ if (sign_buffer(buffer, signature, signing_key)) -+ ret = -1; ++ ++ ret = use_format->sign_buffer(buffer, signature, signing_key); + free(keyid_to_free); + return ret; -+} -+ + } + /* - * Strip CR from the line endings, in case we are on Windows. - * NEEDSWORK: make it trim only CRs before LFs and rename ## gpg-interface.h ## +@@ gpg-interface.h: int parse_signature(const char *buf, size_t size, struct strbuf *payload, struct + */ + size_t parse_signed_buffer(const char *buf, size_t size); + ++/* Flags for sign_buffer(). */ ++enum sign_buffer_flags { ++ /* ++ * Use the default configured signing key as returned by `get_signing_key()` ++ * when the provided "signing_key" is NULL or empty. ++ */ ++ SIGN_BUFFER_USE_DEFAULT_KEY = (1 << 0), ++}; ++ + /* + * Create a detached signature for the contents of "buffer" and append + * it after "signature"; "buffer" and "signature" can be the same @@ gpg-interface.h: size_t parse_signed_buffer(const char *buf, size_t size); + * at the end. Returns 0 on success, non-zero on failure. + */ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, - const char *signing_key); - -+/* -+ * Similar to `sign_buffer()`, but uses the default configured signing key as -+ * returned by `get_signing_key()` when the provided "signing_key" is NULL or -+ * empty. Returns 0 on success, non-zero on failure. -+ */ -+int sign_buffer_with_key(struct strbuf *buffer, struct strbuf *signature, -+ const char *signing_key); +- const char *signing_key); +- ++ const char *signing_key, enum sign_buffer_flags flags); /* * Returns corresponding string in lowercase for a given member of + + ## send-pack.c ## +@@ send-pack.c: static int generate_push_cert(struct strbuf *req_buf, + if (!update_seen) + goto free_return; + +- if (sign_buffer(&cert, &cert, signing_key)) ++ if (sign_buffer(&cert, &cert, signing_key, 0)) + die(_("failed to sign the push certificate")); + + packet_buf_write(req_buf, "push-cert%c%s", 0, cap_string); 3: 8b01ad1570 ! 3: e659971e84 fast-import: add mode to sign commits with invalid signatures @@ builtin/fast-import.c: static void handle_strip_if_invalid(struct strbuf *new_da + * in interoperability mode is currently unsupported. + */ + if (the_repository->compat_hash_algo) -+ die(_("signing signatures in interoperability mode is unsupported")); ++ die(_("signing commits in interoperability mode is unsupported")); + + strbuf_addstr(&payload, signature_check.payload); -+ if (sign_buffer_with_key(&payload, &signature, signed_commit_keyid)) ++ if (sign_buffer(&payload, &signature, signed_commit_keyid, ++ SIGN_BUFFER_USE_DEFAULT_KEY)) + die(_("failed to sign commit object")); + add_header_signature(new_data, &signature, the_hash_algo); + base-commit: 7c02d39fc2ed2702223c7674f73150d9a7e61ba4 -- 2.53.0.381.g628a66ccf6 ^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH v5 1/3] commit: remove unused forward declaration 2026-03-12 19:22 ` [PATCH v5 0/3] fast-import: add mode to re-sign invalid commit signatures Justin Tobler @ 2026-03-12 19:22 ` Justin Tobler 2026-03-12 19:22 ` [PATCH v5 2/3] gpg-interface: allow sign_buffer() to use default signing key Justin Tobler ` (3 subsequent siblings) 4 siblings, 0 replies; 60+ messages in thread From: Justin Tobler @ 2026-03-12 19:22 UTC (permalink / raw) To: git; +Cc: sandals, christian.couder, ps, gitster, Justin Tobler In 6206089cbd (commit: write commits for both hashes, 2023-10-01), `sign_with_header()` was removed, but its forward declaration in "commit.h" was left. Remove the unused declaration. Signed-off-by: Justin Tobler <jltobler@gmail.com> --- commit.h | 2 -- 1 file changed, 2 deletions(-) diff --git a/commit.h b/commit.h index 1635de418b..f0c38cb444 100644 --- a/commit.h +++ b/commit.h @@ -390,8 +390,6 @@ LAST_ARG_MUST_BE_NULL int run_commit_hook(int editor_is_used, const char *index_file, int *invoked_hook, const char *name, ...); -/* Sign a commit or tag buffer, storing the result in a header. */ -int sign_with_header(struct strbuf *buf, const char *keyid); /* Parse the signature out of a header. */ int parse_buffer_signed_by_header(const char *buffer, unsigned long size, -- 2.53.0.381.g628a66ccf6 ^ permalink raw reply related [flat|nested] 60+ messages in thread
* [PATCH v5 2/3] gpg-interface: allow sign_buffer() to use default signing key 2026-03-12 19:22 ` [PATCH v5 0/3] fast-import: add mode to re-sign invalid commit signatures Justin Tobler 2026-03-12 19:22 ` [PATCH v5 1/3] commit: remove unused forward declaration Justin Tobler @ 2026-03-12 19:22 ` Justin Tobler 2026-03-12 20:20 ` Junio C Hamano 2026-03-12 19:22 ` [PATCH v5 3/3] fast-import: add mode to sign commits with invalid signatures Justin Tobler ` (2 subsequent siblings) 4 siblings, 1 reply; 60+ messages in thread From: Justin Tobler @ 2026-03-12 19:22 UTC (permalink / raw) To: git; +Cc: sandals, christian.couder, ps, gitster, Justin Tobler The `sign_commit_to_strbuf()` helper in "commit.c" provides fallback logic to get the default configured signing key when a key is not provided and handles generating the commit signature accordingly. This signing operation is not really specific to commits as any arbitrary buffer can be signed. Also, in a subsequent commit, this same logic is reused by git-fast-import(1) when signing commits with invalid signatures. Remove the `sign_commit_to_strbuf()` helper from "commit.c" and extend `sign_buffer()` in "gpg-interface.c" to support using the default key as a fallback when the `SIGN_BUFFER_USE_DEFAULT_KEY` flag is provided. Call sites are updated accordingly. Signed-off-by: Justin Tobler <jltobler@gmail.com> --- builtin/tag.c | 4 ++-- commit.c | 19 +++++-------------- gpg-interface.c | 13 +++++++++++-- gpg-interface.h | 12 ++++++++++-- send-pack.c | 2 +- 5 files changed, 29 insertions(+), 21 deletions(-) diff --git a/builtin/tag.c b/builtin/tag.c index aeb04c487f..540d783c67 100644 --- a/builtin/tag.c +++ b/builtin/tag.c @@ -167,7 +167,7 @@ static int do_sign(struct strbuf *buffer, struct object_id **compat_oid, char *keyid = get_signing_key(); int ret = -1; - if (sign_buffer(buffer, &sig, keyid)) + if (sign_buffer(buffer, &sig, keyid, 0)) goto out; if (compat) { @@ -176,7 +176,7 @@ static int do_sign(struct strbuf *buffer, struct object_id **compat_oid, if (convert_object_file(the_repository ,&compat_buf, algo, compat, buffer->buf, buffer->len, OBJ_TAG, 1)) goto out; - if (sign_buffer(&compat_buf, &compat_sig, keyid)) + if (sign_buffer(&compat_buf, &compat_sig, keyid, 0)) goto out; add_header_signature(&compat_buf, &sig, algo); strbuf_addbuf(&compat_buf, &compat_sig); diff --git a/commit.c b/commit.c index d16ae73345..1b9b2d4499 100644 --- a/commit.c +++ b/commit.c @@ -1148,18 +1148,6 @@ int add_header_signature(struct strbuf *buf, struct strbuf *sig, const struct gi return 0; } -static int sign_commit_to_strbuf(struct strbuf *sig, struct strbuf *buf, const char *keyid) -{ - char *keyid_to_free = NULL; - int ret = 0; - if (!keyid || !*keyid) - keyid = keyid_to_free = get_signing_key(); - if (sign_buffer(buf, sig, keyid)) - ret = -1; - free(keyid_to_free); - return ret; -} - int parse_signed_commit(const struct commit *commit, struct strbuf *payload, struct strbuf *signature, const struct git_hash_algo *algop) @@ -1737,7 +1725,8 @@ int commit_tree_extended(const char *msg, size_t msg_len, oidcpy(&parent_buf[i++], &p->item->object.oid); write_commit_tree(&buffer, msg, msg_len, tree, parent_buf, nparents, author, committer, extra); - if (sign_commit && sign_commit_to_strbuf(&sig, &buffer, sign_commit)) { + if (sign_commit && sign_buffer(&buffer, &sig, sign_commit, + SIGN_BUFFER_USE_DEFAULT_KEY)) { result = -1; goto out; } @@ -1769,7 +1758,9 @@ int commit_tree_extended(const char *msg, size_t msg_len, free_commit_extra_headers(compat_extra); free(mapped_parents); - if (sign_commit && sign_commit_to_strbuf(&compat_sig, &compat_buffer, sign_commit)) { + if (sign_commit && sign_buffer(&compat_buffer, &compat_sig, + sign_commit, + SIGN_BUFFER_USE_DEFAULT_KEY)) { result = -1; goto out; } diff --git a/gpg-interface.c b/gpg-interface.c index 87fb6605fb..ce935908cc 100644 --- a/gpg-interface.c +++ b/gpg-interface.c @@ -973,11 +973,20 @@ const char *gpg_trust_level_to_str(enum signature_trust_level level) return sigcheck_gpg_trust_level[level].display_key; } -int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *signing_key) +int sign_buffer(struct strbuf *buffer, struct strbuf *signature, + const char *signing_key, enum sign_buffer_flags flags) { + char *keyid_to_free = NULL; + int ret = 0; + gpg_interface_lazy_init(); - return use_format->sign_buffer(buffer, signature, signing_key); + if (flags & SIGN_BUFFER_USE_DEFAULT_KEY && (!signing_key || !*signing_key)) + signing_key = keyid_to_free = get_signing_key(); + + ret = use_format->sign_buffer(buffer, signature, signing_key); + free(keyid_to_free); + return ret; } /* diff --git a/gpg-interface.h b/gpg-interface.h index 789d1ffac4..37f3ac42db 100644 --- a/gpg-interface.h +++ b/gpg-interface.h @@ -74,6 +74,15 @@ int parse_signature(const char *buf, size_t size, struct strbuf *payload, struct */ size_t parse_signed_buffer(const char *buf, size_t size); +/* Flags for sign_buffer(). */ +enum sign_buffer_flags { + /* + * Use the default configured signing key as returned by `get_signing_key()` + * when the provided "signing_key" is NULL or empty. + */ + SIGN_BUFFER_USE_DEFAULT_KEY = (1 << 0), +}; + /* * Create a detached signature for the contents of "buffer" and append * it after "signature"; "buffer" and "signature" can be the same @@ -81,8 +90,7 @@ size_t parse_signed_buffer(const char *buf, size_t size); * at the end. Returns 0 on success, non-zero on failure. */ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, - const char *signing_key); - + const char *signing_key, enum sign_buffer_flags flags); /* * Returns corresponding string in lowercase for a given member of diff --git a/send-pack.c b/send-pack.c index 67d6987b1c..07ecfae4de 100644 --- a/send-pack.c +++ b/send-pack.c @@ -391,7 +391,7 @@ static int generate_push_cert(struct strbuf *req_buf, if (!update_seen) goto free_return; - if (sign_buffer(&cert, &cert, signing_key)) + if (sign_buffer(&cert, &cert, signing_key, 0)) die(_("failed to sign the push certificate")); packet_buf_write(req_buf, "push-cert%c%s", 0, cap_string); -- 2.53.0.381.g628a66ccf6 ^ permalink raw reply related [flat|nested] 60+ messages in thread
* Re: [PATCH v5 2/3] gpg-interface: allow sign_buffer() to use default signing key 2026-03-12 19:22 ` [PATCH v5 2/3] gpg-interface: allow sign_buffer() to use default signing key Justin Tobler @ 2026-03-12 20:20 ` Junio C Hamano 2026-03-12 20:24 ` Justin Tobler 0 siblings, 1 reply; 60+ messages in thread From: Junio C Hamano @ 2026-03-12 20:20 UTC (permalink / raw) To: Justin Tobler; +Cc: git, sandals, christian.couder, ps Justin Tobler <jltobler@gmail.com> writes: > - return use_format->sign_buffer(buffer, signature, signing_key); > + if (flags & SIGN_BUFFER_USE_DEFAULT_KEY && (!signing_key || !*signing_key)) > + signing_key = keyid_to_free = get_signing_key(); Micronit. I would have preferred to see an extra pair of parentheses here, i.e., if ((flags & SIGN_BUFFER_USE_DEFAULT_KEY) && (!signing_key || !*signing_key)) It would make it more obvious what two conditions are required to enter the body, even to those who well know the operator precedence rules between & and &&. ^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH v5 2/3] gpg-interface: allow sign_buffer() to use default signing key 2026-03-12 20:20 ` Junio C Hamano @ 2026-03-12 20:24 ` Justin Tobler 0 siblings, 0 replies; 60+ messages in thread From: Justin Tobler @ 2026-03-12 20:24 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, sandals, christian.couder, ps On 26/03/12 01:20PM, Junio C Hamano wrote: > Justin Tobler <jltobler@gmail.com> writes: > > > - return use_format->sign_buffer(buffer, signature, signing_key); > > + if (flags & SIGN_BUFFER_USE_DEFAULT_KEY && (!signing_key || !*signing_key)) > > + signing_key = keyid_to_free = get_signing_key(); > > Micronit. > > I would have preferred to see an extra pair of parentheses here, i.e., > > if ((flags & SIGN_BUFFER_USE_DEFAULT_KEY) && > (!signing_key || !*signing_key)) > > It would make it more obvious what two conditions are required to > enter the body, even to those who well know the operator precedence > rules between & and &&. That's completely fair. I don't mind fixing and sending another version. Thanks, -Justin ^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH v5 3/3] fast-import: add mode to sign commits with invalid signatures 2026-03-12 19:22 ` [PATCH v5 0/3] fast-import: add mode to re-sign invalid commit signatures Justin Tobler 2026-03-12 19:22 ` [PATCH v5 1/3] commit: remove unused forward declaration Justin Tobler 2026-03-12 19:22 ` [PATCH v5 2/3] gpg-interface: allow sign_buffer() to use default signing key Justin Tobler @ 2026-03-12 19:22 ` Justin Tobler 2026-03-12 20:20 ` Junio C Hamano 2026-03-12 23:58 ` Jeff King 2026-03-12 20:20 ` [PATCH v5 0/3] fast-import: add mode to re-sign invalid commit signatures Junio C Hamano 2026-03-13 1:39 ` [PATCH v6 " Justin Tobler 4 siblings, 2 replies; 60+ messages in thread From: Justin Tobler @ 2026-03-12 19:22 UTC (permalink / raw) To: git; +Cc: sandals, christian.couder, ps, gitster, Justin Tobler 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 sign the commit again as opposed to just stripping the signature. To facilitate this, introduce a "sign-if-invalid" mode for the `--signed-commits` option. Optionally, a key ID may be explicitly provided in the form `sign-if-invalid[=<keyid>]` to specify which signing key should be used when signing invalid commit signatures. Note that to properly support interoperability mode when signing commit signatures, the commit buffer must be created in both the repository and compatability object formats to generate the appropriate signatures accordingly. As currently implemented, the commit buffer for the compatability object format is not reconstructed and thus signing commits in interoperability mode is not yet supported. Support may be added in the future. Signed-off-by: Justin Tobler <jltobler@gmail.com> --- Documentation/git-fast-import.adoc | 4 + builtin/fast-export.c | 8 +- builtin/fast-import.c | 101 ++++++++++++++++----- gpg-interface.c | 23 +++-- gpg-interface.h | 7 +- t/t9305-fast-import-signatures.sh | 140 ++++++++++++++++++----------- 6 files changed, 200 insertions(+), 83 deletions(-) diff --git a/Documentation/git-fast-import.adoc b/Documentation/git-fast-import.adoc index 479c4081da..b3f42d4637 100644 --- a/Documentation/git-fast-import.adoc +++ b/Documentation/git-fast-import.adoc @@ -86,6 +86,10 @@ 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. +* `sign-if-invalid[=<keyid>]`, similar to `strip-if-invalid`, verifies + commit signatures and replaces invalid signatures with newly created ones. + Valid signatures are left unchanged. If `<keyid>` is provided, that key is + used for signing; otherwise the configured default signing key is used. Options for Frontends ~~~~~~~~~~~~~~~~~~~~~ diff --git a/builtin/fast-export.c b/builtin/fast-export.c index 0c5d2386d8..13621b0d6a 100644 --- a/builtin/fast-export.c +++ b/builtin/fast-export.c @@ -64,7 +64,7 @@ static int parse_opt_sign_mode(const struct option *opt, if (unset) return 0; - if (parse_sign_mode(arg, val)) + if (parse_sign_mode(arg, val, NULL)) return error(_("unknown %s mode: %s"), opt->long_name, arg); return 0; @@ -825,6 +825,9 @@ static void handle_commit(struct commit *commit, struct rev_info *rev, case SIGN_STRIP_IF_INVALID: die(_("'strip-if-invalid' is not a valid mode for " "git fast-export with --signed-commits=<mode>")); + case SIGN_SIGN_IF_INVALID: + die(_("'sign-if-invalid' is not a valid mode for " + "git fast-export with --signed-commits=<mode>")); default: BUG("invalid signed_commit_mode value %d", signed_commit_mode); } @@ -970,6 +973,9 @@ static void handle_tag(const char *name, struct tag *tag) case SIGN_STRIP_IF_INVALID: die(_("'strip-if-invalid' is not a valid mode for " "git fast-export with --signed-tags=<mode>")); + case SIGN_SIGN_IF_INVALID: + die(_("'sign-if-invalid' is not a valid mode for " + "git fast-export with --signed-tags=<mode>")); default: BUG("invalid signed_commit_mode value %d", signed_commit_mode); } diff --git a/builtin/fast-import.c b/builtin/fast-import.c index b8a7757cfd..50de88e2ea 100644 --- a/builtin/fast-import.c +++ b/builtin/fast-import.c @@ -190,6 +190,7 @@ static const char *global_prefix; static enum sign_mode signed_tag_mode = SIGN_VERBATIM; static enum sign_mode signed_commit_mode = SIGN_VERBATIM; +static const char *signed_commit_keyid; /* Memory pools */ static struct mem_pool fi_mem_pool = { @@ -2836,26 +2837,15 @@ 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 warn_invalid_signature(struct signature_check *check, + const char *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) { - const char *signer = signature_check.signer ? - signature_check.signer : _("unknown"); - const char *subject; - int subject_len = find_commit_subject(msg->buf, &subject); + const char *signer = check->signer ? check->signer : _("unknown"); + const char *subject; + int subject_len = find_commit_subject(msg, &subject); + switch (mode) { + case SIGN_STRIP_IF_INVALID: if (subject_len > 100) warning(_("stripping invalid signature for commit '%.100s...'\n" " allegedly by %s"), subject, signer); @@ -2865,6 +2855,67 @@ 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"); + } +} + +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 commits in interoperability mode is unsupported")); + + strbuf_addstr(&payload, signature_check.payload); + if (sign_buffer(&payload, &signature, signed_commit_keyid, + SIGN_BUFFER_USE_DEFAULT_KEY)) + die(_("failed to sign commit object")); + add_header_signature(new_data, &signature, the_hash_algo); + + strbuf_release(&signature); + strbuf_release(&payload); + } finalize_commit_buffer(new_data, NULL, NULL, msg); } else { @@ -2927,6 +2978,7 @@ static void parse_new_commit(const char *arg) /* fallthru */ case SIGN_VERBATIM: case SIGN_STRIP_IF_INVALID: + case SIGN_SIGN_IF_INVALID: import_one_signature(&sig_sha1, &sig_sha256, v); break; @@ -3011,9 +3063,11 @@ static void parse_new_commit(const char *arg) "encoding %s\n", encoding); - if (signed_commit_mode == SIGN_STRIP_IF_INVALID && + if ((signed_commit_mode == SIGN_STRIP_IF_INVALID || + signed_commit_mode == SIGN_SIGN_IF_INVALID) && (sig_sha1.hash_algo || sig_sha256.hash_algo)) - handle_strip_if_invalid(&new_data, &sig_sha1, &sig_sha256, &msg); + handle_signature_if_invalid(&new_data, &sig_sha1, &sig_sha256, + &msg, signed_commit_mode); else finalize_commit_buffer(&new_data, &sig_sha1, &sig_sha256, &msg); @@ -3060,6 +3114,9 @@ static void handle_tag_signature(struct strbuf *msg, const char *name) case SIGN_STRIP_IF_INVALID: die(_("'strip-if-invalid' is not a valid mode for " "git fast-import with --signed-tags=<mode>")); + case SIGN_SIGN_IF_INVALID: + die(_("'sign-if-invalid' is not a valid mode for " + "git fast-import with --signed-tags=<mode>")); default: BUG("invalid signed_tag_mode value %d from tag '%s'", signed_tag_mode, name); @@ -3649,10 +3706,10 @@ static int parse_one_option(const char *option) } else if (skip_prefix(option, "export-pack-edges=", &option)) { option_export_pack_edges(option); } else if (skip_prefix(option, "signed-commits=", &option)) { - if (parse_sign_mode(option, &signed_commit_mode)) + if (parse_sign_mode(option, &signed_commit_mode, &signed_commit_keyid)) usagef(_("unknown --signed-commits mode '%s'"), option); } else if (skip_prefix(option, "signed-tags=", &option)) { - if (parse_sign_mode(option, &signed_tag_mode)) + if (parse_sign_mode(option, &signed_tag_mode, NULL)) usagef(_("unknown --signed-tags mode '%s'"), option); } else if (!strcmp(option, "quiet")) { show_stats = 0; diff --git a/gpg-interface.c b/gpg-interface.c index ce935908cc..c26bd32120 100644 --- a/gpg-interface.c +++ b/gpg-interface.c @@ -1151,21 +1151,28 @@ static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature, return ret; } -int parse_sign_mode(const char *arg, enum sign_mode *mode) +int parse_sign_mode(const char *arg, enum sign_mode *mode, const char **keyid) { - if (!strcmp(arg, "abort")) + if (!strcmp(arg, "abort")) { *mode = SIGN_ABORT; - else if (!strcmp(arg, "verbatim") || !strcmp(arg, "ignore")) + } else if (!strcmp(arg, "verbatim") || !strcmp(arg, "ignore")) { *mode = SIGN_VERBATIM; - else if (!strcmp(arg, "warn-verbatim") || !strcmp(arg, "warn")) + } else if (!strcmp(arg, "warn-verbatim") || !strcmp(arg, "warn")) { *mode = SIGN_WARN_VERBATIM; - else if (!strcmp(arg, "warn-strip")) + } else if (!strcmp(arg, "warn-strip")) { *mode = SIGN_WARN_STRIP; - else if (!strcmp(arg, "strip")) + } else if (!strcmp(arg, "strip")) { *mode = SIGN_STRIP; - else if (!strcmp(arg, "strip-if-invalid")) + } else if (!strcmp(arg, "strip-if-invalid")) { *mode = SIGN_STRIP_IF_INVALID; - else + } else if (!strcmp(arg, "sign-if-invalid")) { + *mode = SIGN_SIGN_IF_INVALID; + } else if (skip_prefix(arg, "sign-if-invalid=", &arg)) { + *mode = SIGN_SIGN_IF_INVALID; + if (keyid) + *keyid = arg; + } else { return -1; + } return 0; } diff --git a/gpg-interface.h b/gpg-interface.h index 37f3ac42db..a365586ce1 100644 --- a/gpg-interface.h +++ b/gpg-interface.h @@ -120,12 +120,15 @@ enum sign_mode { SIGN_WARN_STRIP, SIGN_STRIP, SIGN_STRIP_IF_INVALID, + SIGN_SIGN_IF_INVALID, }; /* * Return 0 if `arg` can be parsed into an `enum sign_mode`. Return -1 - * otherwise. + * otherwise. If the parsed mode is SIGN_SIGN_IF_INVALID and GPG key provided in + * the arguments in the form `sign-if-invalid=<keyid>`, the key-ID is parsed + * into `char **keyid`. */ -int parse_sign_mode(const char *arg, enum sign_mode *mode); +int parse_sign_mode(const char *arg, enum sign_mode *mode, const char **keyid); #endif diff --git a/t/t9305-fast-import-signatures.sh b/t/t9305-fast-import-signatures.sh index 022dae02e4..38b3e3b537 100755 --- a/t/t9305-fast-import-signatures.sh +++ b/t/t9305-fast-import-signatures.sh @@ -103,26 +103,85 @@ test_expect_success GPG 'strip both OpenPGP signatures with --signed-commits=war test_line_count = 2 out ' -test_expect_success GPG 'import commit with no signature with --signed-commits=strip-if-invalid' ' - git fast-export main >output && - git -C new fast-import --quiet --signed-commits=strip-if-invalid <output >log 2>&1 && - test_must_be_empty log -' - -test_expect_success GPG 'keep valid OpenPGP signature with --signed-commits=strip-if-invalid' ' - rm -rf new && - git init new && - - git fast-export --signed-commits=verbatim openpgp-signing >output && - git -C new fast-import --quiet --signed-commits=strip-if-invalid <output >log 2>&1 && - IMPORTED=$(git -C new rev-parse --verify refs/heads/openpgp-signing) && - test $OPENPGP_SIGNING = $IMPORTED && - git -C new cat-file commit "$IMPORTED" >actual && - test_grep -E "^gpgsig(-sha256)? " actual && - test_must_be_empty log -' - -test_expect_success GPG 'strip signature invalidated by message change with --signed-commits=strip-if-invalid' ' +for mode in strip-if-invalid sign-if-invalid +do + test_expect_success GPG "import commit with no signature with --signed-commits=$mode" ' + git fast-export main >output && + git -C new fast-import --quiet --signed-commits=$mode <output >log 2>&1 && + test_must_be_empty log + ' + + test_expect_success GPG "keep valid OpenPGP signature with --signed-commits=$mode" ' + rm -rf new && + git init new && + + git fast-export --signed-commits=verbatim openpgp-signing >output && + git -C new fast-import --quiet --signed-commits=$mode <output >log 2>&1 && + IMPORTED=$(git -C new rev-parse --verify refs/heads/openpgp-signing) && + test $OPENPGP_SIGNING = $IMPORTED && + git -C new cat-file commit "$IMPORTED" >actual && + test_grep -E "^gpgsig(-sha256)? " actual && + test_must_be_empty log + ' + + test_expect_success GPG "handle signature invalidated by message change with --signed-commits=$mode" ' + rm -rf new && + git init new && + + git fast-export --signed-commits=verbatim openpgp-signing >output && + + # Change the commit message, which invalidates the signature. + # The commit message length should not change though, otherwise the + # corresponding `data <length>` command would have to be changed too. + sed "s/OpenPGP signed commit/OpenPGP forged commit/" output >modified && + + git -C new fast-import --quiet --signed-commits=$mode <modified >log 2>&1 && + + IMPORTED=$(git -C new rev-parse --verify refs/heads/openpgp-signing) && + test $OPENPGP_SIGNING != $IMPORTED && + git -C new cat-file commit "$IMPORTED" >actual && + + if test "$mode" = strip-if-invalid + then + test_grep "stripping invalid signature" log && + test_grep ! -E "^gpgsig" actual + else + test_grep "signing commit with invalid signature" log && + test_grep -E "^gpgsig(-sha256)? " actual && + git -C new verify-commit "$IMPORTED" + fi + ' + + test_expect_success GPGSM "keep valid X.509 signature with --signed-commits=$mode" ' + rm -rf new && + git init new && + + git fast-export --signed-commits=verbatim x509-signing >output && + git -C new fast-import --quiet --signed-commits=$mode <output >log 2>&1 && + IMPORTED=$(git -C new rev-parse --verify refs/heads/x509-signing) && + test $X509_SIGNING = $IMPORTED && + git -C new cat-file commit "$IMPORTED" >actual && + test_grep -E "^gpgsig(-sha256)? " actual && + test_must_be_empty log + ' + + test_expect_success GPGSSH "keep valid SSH signature with --signed-commits=$mode" ' + rm -rf new && + git init new && + + test_config -C new gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && + + git fast-export --signed-commits=verbatim ssh-signing >output && + git -C new fast-import --quiet --signed-commits=$mode <output >log 2>&1 && + IMPORTED=$(git -C new rev-parse --verify refs/heads/ssh-signing) && + test $SSH_SIGNING = $IMPORTED && + git -C new cat-file commit "$IMPORTED" >actual && + test_grep -E "^gpgsig(-sha256)? " actual && + test_must_be_empty log + ' +done + +test_expect_success GPGSSH "sign invalid commit with explicit keyid" ' rm -rf new && git init new && @@ -133,41 +192,22 @@ test_expect_success GPG 'strip signature invalidated by message change with --si # corresponding `data <length>` command would have to be changed too. sed "s/OpenPGP signed commit/OpenPGP forged commit/" output >modified && - git -C new fast-import --quiet --signed-commits=strip-if-invalid <modified >log 2>&1 && + # Configure the target repository with an invalid default signing key. + test_config -C new user.signingkey "not-a-real-key-id" && + test_config -C new gpg.format ssh && + test_config -C new gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && + test_must_fail git -C new fast-import --quiet \ + --signed-commits=sign-if-invalid <modified >/dev/null 2>&1 && + + # Import using explicitly provided signing key. + git -C new fast-import --quiet \ + --signed-commits=sign-if-invalid="${GPGSSH_KEY_PRIMARY}" <modified && IMPORTED=$(git -C new rev-parse --verify refs/heads/openpgp-signing) && test $OPENPGP_SIGNING != $IMPORTED && git -C new cat-file commit "$IMPORTED" >actual && - test_grep ! -E "^gpgsig" actual && - test_grep "stripping invalid signature" log -' - -test_expect_success GPGSM 'keep valid X.509 signature with --signed-commits=strip-if-invalid' ' - rm -rf new && - git init new && - - git fast-export --signed-commits=verbatim x509-signing >output && - git -C new fast-import --quiet --signed-commits=strip-if-invalid <output >log 2>&1 && - IMPORTED=$(git -C new rev-parse --verify refs/heads/x509-signing) && - test $X509_SIGNING = $IMPORTED && - git -C new cat-file commit "$IMPORTED" >actual && test_grep -E "^gpgsig(-sha256)? " actual && - test_must_be_empty log -' - -test_expect_success GPGSSH 'keep valid SSH signature with --signed-commits=strip-if-invalid' ' - rm -rf new && - git init new && - - test_config -C new gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && - - git fast-export --signed-commits=verbatim ssh-signing >output && - git -C new fast-import --quiet --signed-commits=strip-if-invalid <output >log 2>&1 && - IMPORTED=$(git -C new rev-parse --verify refs/heads/ssh-signing) && - test $SSH_SIGNING = $IMPORTED && - git -C new cat-file commit "$IMPORTED" >actual && - test_grep -E "^gpgsig(-sha256)? " actual && - test_must_be_empty log + git -C new verify-commit "$IMPORTED" ' test_done -- 2.53.0.381.g628a66ccf6 ^ permalink raw reply related [flat|nested] 60+ messages in thread
* Re: [PATCH v5 3/3] fast-import: add mode to sign commits with invalid signatures 2026-03-12 19:22 ` [PATCH v5 3/3] fast-import: add mode to sign commits with invalid signatures Justin Tobler @ 2026-03-12 20:20 ` Junio C Hamano 2026-03-12 20:29 ` Justin Tobler 2026-03-12 23:58 ` Jeff King 1 sibling, 1 reply; 60+ messages in thread From: Junio C Hamano @ 2026-03-12 20:20 UTC (permalink / raw) To: Justin Tobler; +Cc: git, sandals, christian.couder, ps Justin Tobler <jltobler@gmail.com> writes: > + 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); A very minor point, but my reading hiccuped around these messages, sounding as if we are adding an invalid signature to the commit. Perhaps "replacing an invalid signature for commit" or "re-signing commit that has an invalid signature" or along that lines would reduce the chance of confusion? ^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH v5 3/3] fast-import: add mode to sign commits with invalid signatures 2026-03-12 20:20 ` Junio C Hamano @ 2026-03-12 20:29 ` Justin Tobler 0 siblings, 0 replies; 60+ messages in thread From: Justin Tobler @ 2026-03-12 20:29 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, sandals, christian.couder, ps On 26/03/12 01:20PM, Junio C Hamano wrote: > Justin Tobler <jltobler@gmail.com> writes: > > > + 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); > > A very minor point, but my reading hiccuped around these messages, > sounding as if we are adding an invalid signature to the commit. > > Perhaps "replacing an invalid signature for commit" or "re-signing > commit that has an invalid signature" or along that lines would > reduce the chance of confusion? Ya, maybe "replacing invalid signature for commit ..." would be better. I know Patrick is suggesting we consider getting rid of these warning messages altogether in a followup series. For now though, I'll update it in the next version. Thanks, -Justin ^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH v5 3/3] fast-import: add mode to sign commits with invalid signatures 2026-03-12 19:22 ` [PATCH v5 3/3] fast-import: add mode to sign commits with invalid signatures Justin Tobler 2026-03-12 20:20 ` Junio C Hamano @ 2026-03-12 23:58 ` Jeff King 2026-03-13 0:17 ` Justin Tobler 1 sibling, 1 reply; 60+ messages in thread From: Jeff King @ 2026-03-12 23:58 UTC (permalink / raw) To: Justin Tobler; +Cc: git, sandals, christian.couder, ps, gitster On Thu, Mar 12, 2026 at 02:22:28PM -0500, Justin Tobler wrote: > +test_expect_success GPGSSH "sign invalid commit with explicit keyid" ' > rm -rf new && > git init new && This test is failing on Windows in GitHub's CI. You can't see it from the context, but the next line of this test is: git fast-export --signed-commits=verbatim openpgp-signing >output && But the openpgp-signing branch will only have been created if the GPG prereq is set. Should this be referencing ssh-signing instead? Or should it be using GPG,GPGSSH as prereqs? -Peff ^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH v5 3/3] fast-import: add mode to sign commits with invalid signatures 2026-03-12 23:58 ` Jeff King @ 2026-03-13 0:17 ` Justin Tobler 0 siblings, 0 replies; 60+ messages in thread From: Justin Tobler @ 2026-03-13 0:17 UTC (permalink / raw) To: Jeff King; +Cc: git, sandals, christian.couder, ps, gitster On 26/03/12 07:58PM, Jeff King wrote: > On Thu, Mar 12, 2026 at 02:22:28PM -0500, Justin Tobler wrote: > > > +test_expect_success GPGSSH "sign invalid commit with explicit keyid" ' > > rm -rf new && > > git init new && > > This test is failing on Windows in GitHub's CI. > > You can't see it from the context, but the next line of this test is: > > git fast-export --signed-commits=verbatim openpgp-signing >output && > > But the openpgp-signing branch will only have been created if the GPG > prereq is set. Should this be referencing ssh-signing instead? Or should > it be using GPG,GPGSSH as prereqs? Ahh, it look like this should be referencing ssh-signing. I'll include this fix in my next version. Thanks! :) -Justin ^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH v5 0/3] fast-import: add mode to re-sign invalid commit signatures 2026-03-12 19:22 ` [PATCH v5 0/3] fast-import: add mode to re-sign invalid commit signatures Justin Tobler ` (2 preceding siblings ...) 2026-03-12 19:22 ` [PATCH v5 3/3] fast-import: add mode to sign commits with invalid signatures Justin Tobler @ 2026-03-12 20:20 ` Junio C Hamano 2026-03-12 20:30 ` Justin Tobler 2026-03-13 1:39 ` [PATCH v6 " Justin Tobler 4 siblings, 1 reply; 60+ messages in thread From: Junio C Hamano @ 2026-03-12 20:20 UTC (permalink / raw) To: Justin Tobler; +Cc: git, sandals, christian.couder, ps Justin Tobler <jltobler@gmail.com> writes: > Changes since V4: > - Instead of introducing a separate `sign_buffer_with_key()` helper, > extend `sign_buffer()` to support a SIGN_BUFFER_USE_DEFAULT_KEY flag. > - Fixed message in die(). I left small comments on two patches, but everything looks quite well done to me in this iteration. ^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH v5 0/3] fast-import: add mode to re-sign invalid commit signatures 2026-03-12 20:20 ` [PATCH v5 0/3] fast-import: add mode to re-sign invalid commit signatures Junio C Hamano @ 2026-03-12 20:30 ` Justin Tobler 0 siblings, 0 replies; 60+ messages in thread From: Justin Tobler @ 2026-03-12 20:30 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, sandals, christian.couder, ps On 26/03/12 01:20PM, Junio C Hamano wrote: > Justin Tobler <jltobler@gmail.com> writes: > > > Changes since V4: > > - Instead of introducing a separate `sign_buffer_with_key()` helper, > > extend `sign_buffer()` to support a SIGN_BUFFER_USE_DEFAULT_KEY flag. > > - Fixed message in die(). > > I left small comments on two patches, but everything looks quite > well done to me in this iteration. Thanks, I've amended locally and will sent another version a little bit latter today. :) -Justin ^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH v6 0/3] fast-import: add mode to re-sign invalid commit signatures 2026-03-12 19:22 ` [PATCH v5 0/3] fast-import: add mode to re-sign invalid commit signatures Justin Tobler ` (3 preceding siblings ...) 2026-03-12 20:20 ` [PATCH v5 0/3] fast-import: add mode to re-sign invalid commit signatures Junio C Hamano @ 2026-03-13 1:39 ` Justin Tobler 2026-03-13 1:39 ` [PATCH v6 1/3] commit: remove unused forward declaration Justin Tobler ` (3 more replies) 4 siblings, 4 replies; 60+ messages in thread From: Justin Tobler @ 2026-03-13 1:39 UTC (permalink / raw) To: git; +Cc: sandals, christian.couder, ps, gitster, peff, Justin Tobler Greetings, With c20f112e51 (fast-import: add 'strip-if-invalid' mode to --signed-commits=<mode>, 2025-11-17), it became possible to remove invalid signatures from commits via git-fast-import(1) while maintaining valid commit signatures. Building upon this functionality, a user may want to re-sign these invalid commit signatures. This series introduces the `sign-if-invalid` mode to do so accordingly. The newly added mode in this series currently ignores `extensions.compatObjectFormat` when generating the new signatures. From my understanding, to generate the compatibility structure would also require us to reconstruct the compatibility object for the object being signed. I think this would be possible to do, but would require getting the mapped OIDs for the commit parents and tree. I'm not completely sure of a good way to go about this yet though. I'm also not completely certain if this is something that should be addressed as part of this series, or could be done later down the road. So for now I've opted to delay its implementation. I'm open going down the other route if that is preferred though. The first commit is a simple cleanup for something I noticed while reading though commit signing code. The second commit actually introduces the new `--signed-commits` mode. Changes since V5: - Fixed a test that was incorrectly referencing the openpgp-signing branch when it should be using the ssh-signing branch. - Changed warning message wording. - Added some parentheses in a conditional statement to clarify operation order. Changes since V4: - Instead of introducing a separate `sign_buffer_with_key()` helper, extend `sign_buffer()` to support a SIGN_BUFFER_USE_DEFAULT_KEY flag. - Fixed message in die(). Changes since V3: - Rename the `re-sign-if-invalid` mode to `sign-if-invalid`. The "if-invalid" already implies the signatures was previously signed making "re-sign" redundant. Changes since V2: - Adapted commit message in second patch to improve clarity. - Fixed typos. - Renamed SIGN_RESIGN_IF_INVALID to SIGN_RE_SIGN_IF_INVALID. - Created separate helper function to handle printing invalid signature warnings. Changes since V1: - Improved commit messages and comments to better explain why interoperability mode is not currently supported. - Clarified documentation for re-sign-if-invalid mode. - Renamed `handle_invalid_signature()` to `handle_signature_if_invalid()`. - Added warning messages specific to commit resigning. - Fixed some small typos. - Added support for explicitly specifying the signing key ID via `--signed-commits=re-sign-if-invalid[=<keyid>]` similar to how it can specified in git-commit(1). - We now die() as unsupported when attempting to re-sign an invalid commit signature in interoperability mode. - We now die() when failing to re-sign a commit. Thanks, -Justin Justin Tobler (3): commit: remove unused forward declaration gpg-interface: allow sign_buffer() to use default signing key fast-import: add mode to sign commits with invalid signatures Documentation/git-fast-import.adoc | 4 + builtin/fast-export.c | 8 +- builtin/fast-import.c | 101 +++++++++++++++----- builtin/tag.c | 4 +- commit.c | 19 +--- commit.h | 2 - gpg-interface.c | 36 ++++++-- gpg-interface.h | 19 +++- send-pack.c | 2 +- t/t9305-fast-import-signatures.sh | 144 ++++++++++++++++++----------- 10 files changed, 231 insertions(+), 108 deletions(-) Range-diff against v5: 1: 0d00b72ee0 = 1: 0d00b72ee0 commit: remove unused forward declaration 2: 7a0deed77b ! 2: 5224c1766f gpg-interface: allow sign_buffer() to use default signing key @@ gpg-interface.c: const char *gpg_trust_level_to_str(enum signature_trust_level l gpg_interface_lazy_init(); - return use_format->sign_buffer(buffer, signature, signing_key); -+ if (flags & SIGN_BUFFER_USE_DEFAULT_KEY && (!signing_key || !*signing_key)) ++ if ((flags & SIGN_BUFFER_USE_DEFAULT_KEY) && (!signing_key || !*signing_key)) + signing_key = keyid_to_free = get_signing_key(); + + ret = use_format->sign_buffer(buffer, signature, signing_key); 3: e659971e84 ! 3: d9ad73e05b fast-import: add mode to sign commits with invalid signatures @@ builtin/fast-import.c: static void handle_strip_if_invalid(struct strbuf *new_da + break; + case SIGN_SIGN_IF_INVALID: + if (subject_len > 100) -+ warning(_("signing commit with invalid signature for '%.100s...'\n" ++ warning(_("replacing invalid signature for commit '%.100s...'\n" + " allegedly by %s"), subject, signer); + else if (subject_len > 0) -+ warning(_("signing commit with invalid signature for '%.*s'\n" ++ warning(_("replacing invalid signature for commit '%.*s'\n" + " allegedly by %s"), subject_len, subject, signer); + else -+ warning(_("signing commit with invalid signature\n" ++ warning(_("replacing invalid signature for commit\n" + " allegedly by %s"), signer); + break; + default: @@ t/t9305-fast-import-signatures.sh: test_expect_success GPG 'strip both OpenPGP s + test_grep "stripping invalid signature" log && + test_grep ! -E "^gpgsig" actual + else -+ test_grep "signing commit with invalid signature" log && ++ test_grep "replacing invalid signature" log && + test_grep -E "^gpgsig(-sha256)? " actual && + git -C new verify-commit "$IMPORTED" + fi @@ t/t9305-fast-import-signatures.sh: test_expect_success GPG 'strip both OpenPGP s rm -rf new && git init new && -@@ t/t9305-fast-import-signatures.sh: test_expect_success GPG 'strip signature invalidated by message change with --si - # corresponding `data <length>` command would have to be changed too. - sed "s/OpenPGP signed commit/OpenPGP forged commit/" output >modified && +- git fast-export --signed-commits=verbatim openpgp-signing >output && ++ git fast-export --signed-commits=verbatim ssh-signing >output && + # Change the commit message, which invalidates the signature. + # The commit message length should not change though, otherwise the + # corresponding `data <length>` command would have to be changed too. +- sed "s/OpenPGP signed commit/OpenPGP forged commit/" output >modified && +- - git -C new fast-import --quiet --signed-commits=strip-if-invalid <modified >log 2>&1 && -+ # Configure the target repository with an invalid default signing key. -+ test_config -C new user.signingkey "not-a-real-key-id" && -+ test_config -C new gpg.format ssh && -+ test_config -C new gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && -+ test_must_fail git -C new fast-import --quiet \ -+ --signed-commits=sign-if-invalid <modified >/dev/null 2>&1 && -+ -+ # Import using explicitly provided signing key. -+ git -C new fast-import --quiet \ -+ --signed-commits=sign-if-invalid="${GPGSSH_KEY_PRIMARY}" <modified && - - IMPORTED=$(git -C new rev-parse --verify refs/heads/openpgp-signing) && - test $OPENPGP_SIGNING != $IMPORTED && - git -C new cat-file commit "$IMPORTED" >actual && +- +- IMPORTED=$(git -C new rev-parse --verify refs/heads/openpgp-signing) && +- test $OPENPGP_SIGNING != $IMPORTED && +- git -C new cat-file commit "$IMPORTED" >actual && - test_grep ! -E "^gpgsig" actual && - test_grep "stripping invalid signature" log -' @@ t/t9305-fast-import-signatures.sh: test_expect_success GPG 'strip signature inva - IMPORTED=$(git -C new rev-parse --verify refs/heads/x509-signing) && - test $X509_SIGNING = $IMPORTED && - git -C new cat-file commit "$IMPORTED" >actual && - test_grep -E "^gpgsig(-sha256)? " actual && +- test_grep -E "^gpgsig(-sha256)? " actual && - test_must_be_empty log -' - -test_expect_success GPGSSH 'keep valid SSH signature with --signed-commits=strip-if-invalid' ' - rm -rf new && - git init new && -- -- test_config -C new gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && -- ++ sed "s/SSH signed commit/SSH forged commit/" output >modified && + ++ # Configure the target repository with an invalid default signing key. ++ test_config -C new user.signingkey "not-a-real-key-id" && ++ test_config -C new gpg.format ssh && + test_config -C new gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && ++ test_must_fail git -C new fast-import --quiet \ ++ --signed-commits=sign-if-invalid <modified >/dev/null 2>&1 && ++ ++ # Import using explicitly provided signing key. ++ git -C new fast-import --quiet \ ++ --signed-commits=sign-if-invalid="${GPGSSH_KEY_PRIMARY}" <modified && + - git fast-export --signed-commits=verbatim ssh-signing >output && - git -C new fast-import --quiet --signed-commits=strip-if-invalid <output >log 2>&1 && -- IMPORTED=$(git -C new rev-parse --verify refs/heads/ssh-signing) && + IMPORTED=$(git -C new rev-parse --verify refs/heads/ssh-signing) && - test $SSH_SIGNING = $IMPORTED && -- git -C new cat-file commit "$IMPORTED" >actual && -- test_grep -E "^gpgsig(-sha256)? " actual && ++ test $SSH_SIGNING != $IMPORTED && + git -C new cat-file commit "$IMPORTED" >actual && + test_grep -E "^gpgsig(-sha256)? " actual && - test_must_be_empty log + git -C new verify-commit "$IMPORTED" ' base-commit: 7c02d39fc2ed2702223c7674f73150d9a7e61ba4 -- 2.53.0.381.g628a66ccf6 ^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH v6 1/3] commit: remove unused forward declaration 2026-03-13 1:39 ` [PATCH v6 " Justin Tobler @ 2026-03-13 1:39 ` Justin Tobler 2026-03-13 1:39 ` [PATCH v6 2/3] gpg-interface: allow sign_buffer() to use default signing key Justin Tobler ` (2 subsequent siblings) 3 siblings, 0 replies; 60+ messages in thread From: Justin Tobler @ 2026-03-13 1:39 UTC (permalink / raw) To: git; +Cc: sandals, christian.couder, ps, gitster, peff, Justin Tobler In 6206089cbd (commit: write commits for both hashes, 2023-10-01), `sign_with_header()` was removed, but its forward declaration in "commit.h" was left. Remove the unused declaration. Signed-off-by: Justin Tobler <jltobler@gmail.com> --- commit.h | 2 -- 1 file changed, 2 deletions(-) diff --git a/commit.h b/commit.h index 1635de418b..f0c38cb444 100644 --- a/commit.h +++ b/commit.h @@ -390,8 +390,6 @@ LAST_ARG_MUST_BE_NULL int run_commit_hook(int editor_is_used, const char *index_file, int *invoked_hook, const char *name, ...); -/* Sign a commit or tag buffer, storing the result in a header. */ -int sign_with_header(struct strbuf *buf, const char *keyid); /* Parse the signature out of a header. */ int parse_buffer_signed_by_header(const char *buffer, unsigned long size, -- 2.53.0.381.g628a66ccf6 ^ permalink raw reply related [flat|nested] 60+ messages in thread
* [PATCH v6 2/3] gpg-interface: allow sign_buffer() to use default signing key 2026-03-13 1:39 ` [PATCH v6 " Justin Tobler 2026-03-13 1:39 ` [PATCH v6 1/3] commit: remove unused forward declaration Justin Tobler @ 2026-03-13 1:39 ` Justin Tobler 2026-03-13 6:31 ` Patrick Steinhardt 2026-03-13 1:39 ` [PATCH v6 3/3] fast-import: add mode to sign commits with invalid signatures Justin Tobler 2026-03-13 4:29 ` [PATCH v6 0/3] fast-import: add mode to re-sign invalid commit signatures Junio C Hamano 3 siblings, 1 reply; 60+ messages in thread From: Justin Tobler @ 2026-03-13 1:39 UTC (permalink / raw) To: git; +Cc: sandals, christian.couder, ps, gitster, peff, Justin Tobler The `sign_commit_to_strbuf()` helper in "commit.c" provides fallback logic to get the default configured signing key when a key is not provided and handles generating the commit signature accordingly. This signing operation is not really specific to commits as any arbitrary buffer can be signed. Also, in a subsequent commit, this same logic is reused by git-fast-import(1) when signing commits with invalid signatures. Remove the `sign_commit_to_strbuf()` helper from "commit.c" and extend `sign_buffer()` in "gpg-interface.c" to support using the default key as a fallback when the `SIGN_BUFFER_USE_DEFAULT_KEY` flag is provided. Call sites are updated accordingly. Signed-off-by: Justin Tobler <jltobler@gmail.com> --- builtin/tag.c | 4 ++-- commit.c | 19 +++++-------------- gpg-interface.c | 13 +++++++++++-- gpg-interface.h | 12 ++++++++++-- send-pack.c | 2 +- 5 files changed, 29 insertions(+), 21 deletions(-) diff --git a/builtin/tag.c b/builtin/tag.c index aeb04c487f..540d783c67 100644 --- a/builtin/tag.c +++ b/builtin/tag.c @@ -167,7 +167,7 @@ static int do_sign(struct strbuf *buffer, struct object_id **compat_oid, char *keyid = get_signing_key(); int ret = -1; - if (sign_buffer(buffer, &sig, keyid)) + if (sign_buffer(buffer, &sig, keyid, 0)) goto out; if (compat) { @@ -176,7 +176,7 @@ static int do_sign(struct strbuf *buffer, struct object_id **compat_oid, if (convert_object_file(the_repository ,&compat_buf, algo, compat, buffer->buf, buffer->len, OBJ_TAG, 1)) goto out; - if (sign_buffer(&compat_buf, &compat_sig, keyid)) + if (sign_buffer(&compat_buf, &compat_sig, keyid, 0)) goto out; add_header_signature(&compat_buf, &sig, algo); strbuf_addbuf(&compat_buf, &compat_sig); diff --git a/commit.c b/commit.c index d16ae73345..1b9b2d4499 100644 --- a/commit.c +++ b/commit.c @@ -1148,18 +1148,6 @@ int add_header_signature(struct strbuf *buf, struct strbuf *sig, const struct gi return 0; } -static int sign_commit_to_strbuf(struct strbuf *sig, struct strbuf *buf, const char *keyid) -{ - char *keyid_to_free = NULL; - int ret = 0; - if (!keyid || !*keyid) - keyid = keyid_to_free = get_signing_key(); - if (sign_buffer(buf, sig, keyid)) - ret = -1; - free(keyid_to_free); - return ret; -} - int parse_signed_commit(const struct commit *commit, struct strbuf *payload, struct strbuf *signature, const struct git_hash_algo *algop) @@ -1737,7 +1725,8 @@ int commit_tree_extended(const char *msg, size_t msg_len, oidcpy(&parent_buf[i++], &p->item->object.oid); write_commit_tree(&buffer, msg, msg_len, tree, parent_buf, nparents, author, committer, extra); - if (sign_commit && sign_commit_to_strbuf(&sig, &buffer, sign_commit)) { + if (sign_commit && sign_buffer(&buffer, &sig, sign_commit, + SIGN_BUFFER_USE_DEFAULT_KEY)) { result = -1; goto out; } @@ -1769,7 +1758,9 @@ int commit_tree_extended(const char *msg, size_t msg_len, free_commit_extra_headers(compat_extra); free(mapped_parents); - if (sign_commit && sign_commit_to_strbuf(&compat_sig, &compat_buffer, sign_commit)) { + if (sign_commit && sign_buffer(&compat_buffer, &compat_sig, + sign_commit, + SIGN_BUFFER_USE_DEFAULT_KEY)) { result = -1; goto out; } diff --git a/gpg-interface.c b/gpg-interface.c index 87fb6605fb..dca192d5c4 100644 --- a/gpg-interface.c +++ b/gpg-interface.c @@ -973,11 +973,20 @@ const char *gpg_trust_level_to_str(enum signature_trust_level level) return sigcheck_gpg_trust_level[level].display_key; } -int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *signing_key) +int sign_buffer(struct strbuf *buffer, struct strbuf *signature, + const char *signing_key, enum sign_buffer_flags flags) { + char *keyid_to_free = NULL; + int ret = 0; + gpg_interface_lazy_init(); - return use_format->sign_buffer(buffer, signature, signing_key); + if ((flags & SIGN_BUFFER_USE_DEFAULT_KEY) && (!signing_key || !*signing_key)) + signing_key = keyid_to_free = get_signing_key(); + + ret = use_format->sign_buffer(buffer, signature, signing_key); + free(keyid_to_free); + return ret; } /* diff --git a/gpg-interface.h b/gpg-interface.h index 789d1ffac4..37f3ac42db 100644 --- a/gpg-interface.h +++ b/gpg-interface.h @@ -74,6 +74,15 @@ int parse_signature(const char *buf, size_t size, struct strbuf *payload, struct */ size_t parse_signed_buffer(const char *buf, size_t size); +/* Flags for sign_buffer(). */ +enum sign_buffer_flags { + /* + * Use the default configured signing key as returned by `get_signing_key()` + * when the provided "signing_key" is NULL or empty. + */ + SIGN_BUFFER_USE_DEFAULT_KEY = (1 << 0), +}; + /* * Create a detached signature for the contents of "buffer" and append * it after "signature"; "buffer" and "signature" can be the same @@ -81,8 +90,7 @@ size_t parse_signed_buffer(const char *buf, size_t size); * at the end. Returns 0 on success, non-zero on failure. */ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, - const char *signing_key); - + const char *signing_key, enum sign_buffer_flags flags); /* * Returns corresponding string in lowercase for a given member of diff --git a/send-pack.c b/send-pack.c index 67d6987b1c..07ecfae4de 100644 --- a/send-pack.c +++ b/send-pack.c @@ -391,7 +391,7 @@ static int generate_push_cert(struct strbuf *req_buf, if (!update_seen) goto free_return; - if (sign_buffer(&cert, &cert, signing_key)) + if (sign_buffer(&cert, &cert, signing_key, 0)) die(_("failed to sign the push certificate")); packet_buf_write(req_buf, "push-cert%c%s", 0, cap_string); -- 2.53.0.381.g628a66ccf6 ^ permalink raw reply related [flat|nested] 60+ messages in thread
* Re: [PATCH v6 2/3] gpg-interface: allow sign_buffer() to use default signing key 2026-03-13 1:39 ` [PATCH v6 2/3] gpg-interface: allow sign_buffer() to use default signing key Justin Tobler @ 2026-03-13 6:31 ` Patrick Steinhardt 0 siblings, 0 replies; 60+ messages in thread From: Patrick Steinhardt @ 2026-03-13 6:31 UTC (permalink / raw) To: Justin Tobler; +Cc: git, sandals, christian.couder, gitster, peff On Thu, Mar 12, 2026 at 08:39:37PM -0500, Justin Tobler wrote: > The `sign_commit_to_strbuf()` helper in "commit.c" provides fallback > logic to get the default configured signing key when a key is not > provided and handles generating the commit signature accordingly. This > signing operation is not really specific to commits as any arbitrary > buffer can be signed. Also, in a subsequent commit, this same logic is > reused by git-fast-import(1) when signing commits with invalid > signatures. > > Remove the `sign_commit_to_strbuf()` helper from "commit.c" and extend > `sign_buffer()` in "gpg-interface.c" to support using the default key as > a fallback when the `SIGN_BUFFER_USE_DEFAULT_KEY` flag is provided. Call > sites are updated accordingly. Thanks, this looks much nicer to me now. Patrick ^ permalink raw reply [flat|nested] 60+ messages in thread
* [PATCH v6 3/3] fast-import: add mode to sign commits with invalid signatures 2026-03-13 1:39 ` [PATCH v6 " Justin Tobler 2026-03-13 1:39 ` [PATCH v6 1/3] commit: remove unused forward declaration Justin Tobler 2026-03-13 1:39 ` [PATCH v6 2/3] gpg-interface: allow sign_buffer() to use default signing key Justin Tobler @ 2026-03-13 1:39 ` Justin Tobler 2026-03-13 6:31 ` Patrick Steinhardt 2026-03-13 4:29 ` [PATCH v6 0/3] fast-import: add mode to re-sign invalid commit signatures Junio C Hamano 3 siblings, 1 reply; 60+ messages in thread From: Justin Tobler @ 2026-03-13 1:39 UTC (permalink / raw) To: git; +Cc: sandals, christian.couder, ps, gitster, peff, Justin Tobler 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 sign the commit again as opposed to just stripping the signature. To facilitate this, introduce a "sign-if-invalid" mode for the `--signed-commits` option. Optionally, a key ID may be explicitly provided in the form `sign-if-invalid[=<keyid>]` to specify which signing key should be used when signing invalid commit signatures. Note that to properly support interoperability mode when signing commit signatures, the commit buffer must be created in both the repository and compatability object formats to generate the appropriate signatures accordingly. As currently implemented, the commit buffer for the compatability object format is not reconstructed and thus signing commits in interoperability mode is not yet supported. Support may be added in the future. Signed-off-by: Justin Tobler <jltobler@gmail.com> --- Documentation/git-fast-import.adoc | 4 + builtin/fast-export.c | 8 +- builtin/fast-import.c | 101 +++++++++++++++----- gpg-interface.c | 23 +++-- gpg-interface.h | 7 +- t/t9305-fast-import-signatures.sh | 144 ++++++++++++++++++----------- 6 files changed, 202 insertions(+), 85 deletions(-) diff --git a/Documentation/git-fast-import.adoc b/Documentation/git-fast-import.adoc index 479c4081da..b3f42d4637 100644 --- a/Documentation/git-fast-import.adoc +++ b/Documentation/git-fast-import.adoc @@ -86,6 +86,10 @@ 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. +* `sign-if-invalid[=<keyid>]`, similar to `strip-if-invalid`, verifies + commit signatures and replaces invalid signatures with newly created ones. + Valid signatures are left unchanged. If `<keyid>` is provided, that key is + used for signing; otherwise the configured default signing key is used. Options for Frontends ~~~~~~~~~~~~~~~~~~~~~ diff --git a/builtin/fast-export.c b/builtin/fast-export.c index 0c5d2386d8..13621b0d6a 100644 --- a/builtin/fast-export.c +++ b/builtin/fast-export.c @@ -64,7 +64,7 @@ static int parse_opt_sign_mode(const struct option *opt, if (unset) return 0; - if (parse_sign_mode(arg, val)) + if (parse_sign_mode(arg, val, NULL)) return error(_("unknown %s mode: %s"), opt->long_name, arg); return 0; @@ -825,6 +825,9 @@ static void handle_commit(struct commit *commit, struct rev_info *rev, case SIGN_STRIP_IF_INVALID: die(_("'strip-if-invalid' is not a valid mode for " "git fast-export with --signed-commits=<mode>")); + case SIGN_SIGN_IF_INVALID: + die(_("'sign-if-invalid' is not a valid mode for " + "git fast-export with --signed-commits=<mode>")); default: BUG("invalid signed_commit_mode value %d", signed_commit_mode); } @@ -970,6 +973,9 @@ static void handle_tag(const char *name, struct tag *tag) case SIGN_STRIP_IF_INVALID: die(_("'strip-if-invalid' is not a valid mode for " "git fast-export with --signed-tags=<mode>")); + case SIGN_SIGN_IF_INVALID: + die(_("'sign-if-invalid' is not a valid mode for " + "git fast-export with --signed-tags=<mode>")); default: BUG("invalid signed_commit_mode value %d", signed_commit_mode); } diff --git a/builtin/fast-import.c b/builtin/fast-import.c index b8a7757cfd..935e688e33 100644 --- a/builtin/fast-import.c +++ b/builtin/fast-import.c @@ -190,6 +190,7 @@ static const char *global_prefix; static enum sign_mode signed_tag_mode = SIGN_VERBATIM; static enum sign_mode signed_commit_mode = SIGN_VERBATIM; +static const char *signed_commit_keyid; /* Memory pools */ static struct mem_pool fi_mem_pool = { @@ -2836,26 +2837,15 @@ 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 warn_invalid_signature(struct signature_check *check, + const char *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) { - const char *signer = signature_check.signer ? - signature_check.signer : _("unknown"); - const char *subject; - int subject_len = find_commit_subject(msg->buf, &subject); + const char *signer = check->signer ? check->signer : _("unknown"); + const char *subject; + int subject_len = find_commit_subject(msg, &subject); + switch (mode) { + case SIGN_STRIP_IF_INVALID: if (subject_len > 100) warning(_("stripping invalid signature for commit '%.100s...'\n" " allegedly by %s"), subject, signer); @@ -2865,6 +2855,67 @@ 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(_("replacing invalid signature for commit '%.100s...'\n" + " allegedly by %s"), subject, signer); + else if (subject_len > 0) + warning(_("replacing invalid signature for commit '%.*s'\n" + " allegedly by %s"), subject_len, subject, signer); + else + warning(_("replacing invalid signature for commit\n" + " allegedly by %s"), signer); + break; + default: + BUG("unsupported signing mode"); + } +} + +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 commits in interoperability mode is unsupported")); + + strbuf_addstr(&payload, signature_check.payload); + if (sign_buffer(&payload, &signature, signed_commit_keyid, + SIGN_BUFFER_USE_DEFAULT_KEY)) + die(_("failed to sign commit object")); + add_header_signature(new_data, &signature, the_hash_algo); + + strbuf_release(&signature); + strbuf_release(&payload); + } finalize_commit_buffer(new_data, NULL, NULL, msg); } else { @@ -2927,6 +2978,7 @@ static void parse_new_commit(const char *arg) /* fallthru */ case SIGN_VERBATIM: case SIGN_STRIP_IF_INVALID: + case SIGN_SIGN_IF_INVALID: import_one_signature(&sig_sha1, &sig_sha256, v); break; @@ -3011,9 +3063,11 @@ static void parse_new_commit(const char *arg) "encoding %s\n", encoding); - if (signed_commit_mode == SIGN_STRIP_IF_INVALID && + if ((signed_commit_mode == SIGN_STRIP_IF_INVALID || + signed_commit_mode == SIGN_SIGN_IF_INVALID) && (sig_sha1.hash_algo || sig_sha256.hash_algo)) - handle_strip_if_invalid(&new_data, &sig_sha1, &sig_sha256, &msg); + handle_signature_if_invalid(&new_data, &sig_sha1, &sig_sha256, + &msg, signed_commit_mode); else finalize_commit_buffer(&new_data, &sig_sha1, &sig_sha256, &msg); @@ -3060,6 +3114,9 @@ static void handle_tag_signature(struct strbuf *msg, const char *name) case SIGN_STRIP_IF_INVALID: die(_("'strip-if-invalid' is not a valid mode for " "git fast-import with --signed-tags=<mode>")); + case SIGN_SIGN_IF_INVALID: + die(_("'sign-if-invalid' is not a valid mode for " + "git fast-import with --signed-tags=<mode>")); default: BUG("invalid signed_tag_mode value %d from tag '%s'", signed_tag_mode, name); @@ -3649,10 +3706,10 @@ static int parse_one_option(const char *option) } else if (skip_prefix(option, "export-pack-edges=", &option)) { option_export_pack_edges(option); } else if (skip_prefix(option, "signed-commits=", &option)) { - if (parse_sign_mode(option, &signed_commit_mode)) + if (parse_sign_mode(option, &signed_commit_mode, &signed_commit_keyid)) usagef(_("unknown --signed-commits mode '%s'"), option); } else if (skip_prefix(option, "signed-tags=", &option)) { - if (parse_sign_mode(option, &signed_tag_mode)) + if (parse_sign_mode(option, &signed_tag_mode, NULL)) usagef(_("unknown --signed-tags mode '%s'"), option); } else if (!strcmp(option, "quiet")) { show_stats = 0; diff --git a/gpg-interface.c b/gpg-interface.c index dca192d5c4..32f2880976 100644 --- a/gpg-interface.c +++ b/gpg-interface.c @@ -1151,21 +1151,28 @@ static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature, return ret; } -int parse_sign_mode(const char *arg, enum sign_mode *mode) +int parse_sign_mode(const char *arg, enum sign_mode *mode, const char **keyid) { - if (!strcmp(arg, "abort")) + if (!strcmp(arg, "abort")) { *mode = SIGN_ABORT; - else if (!strcmp(arg, "verbatim") || !strcmp(arg, "ignore")) + } else if (!strcmp(arg, "verbatim") || !strcmp(arg, "ignore")) { *mode = SIGN_VERBATIM; - else if (!strcmp(arg, "warn-verbatim") || !strcmp(arg, "warn")) + } else if (!strcmp(arg, "warn-verbatim") || !strcmp(arg, "warn")) { *mode = SIGN_WARN_VERBATIM; - else if (!strcmp(arg, "warn-strip")) + } else if (!strcmp(arg, "warn-strip")) { *mode = SIGN_WARN_STRIP; - else if (!strcmp(arg, "strip")) + } else if (!strcmp(arg, "strip")) { *mode = SIGN_STRIP; - else if (!strcmp(arg, "strip-if-invalid")) + } else if (!strcmp(arg, "strip-if-invalid")) { *mode = SIGN_STRIP_IF_INVALID; - else + } else if (!strcmp(arg, "sign-if-invalid")) { + *mode = SIGN_SIGN_IF_INVALID; + } else if (skip_prefix(arg, "sign-if-invalid=", &arg)) { + *mode = SIGN_SIGN_IF_INVALID; + if (keyid) + *keyid = arg; + } else { return -1; + } return 0; } diff --git a/gpg-interface.h b/gpg-interface.h index 37f3ac42db..a365586ce1 100644 --- a/gpg-interface.h +++ b/gpg-interface.h @@ -120,12 +120,15 @@ enum sign_mode { SIGN_WARN_STRIP, SIGN_STRIP, SIGN_STRIP_IF_INVALID, + SIGN_SIGN_IF_INVALID, }; /* * Return 0 if `arg` can be parsed into an `enum sign_mode`. Return -1 - * otherwise. + * otherwise. If the parsed mode is SIGN_SIGN_IF_INVALID and GPG key provided in + * the arguments in the form `sign-if-invalid=<keyid>`, the key-ID is parsed + * into `char **keyid`. */ -int parse_sign_mode(const char *arg, enum sign_mode *mode); +int parse_sign_mode(const char *arg, enum sign_mode *mode, const char **keyid); #endif diff --git a/t/t9305-fast-import-signatures.sh b/t/t9305-fast-import-signatures.sh index 022dae02e4..ac4228127a 100755 --- a/t/t9305-fast-import-signatures.sh +++ b/t/t9305-fast-import-signatures.sh @@ -103,71 +103,111 @@ test_expect_success GPG 'strip both OpenPGP signatures with --signed-commits=war test_line_count = 2 out ' -test_expect_success GPG 'import commit with no signature with --signed-commits=strip-if-invalid' ' - git fast-export main >output && - git -C new fast-import --quiet --signed-commits=strip-if-invalid <output >log 2>&1 && - test_must_be_empty log -' - -test_expect_success GPG 'keep valid OpenPGP signature with --signed-commits=strip-if-invalid' ' - rm -rf new && - git init new && - - git fast-export --signed-commits=verbatim openpgp-signing >output && - git -C new fast-import --quiet --signed-commits=strip-if-invalid <output >log 2>&1 && - IMPORTED=$(git -C new rev-parse --verify refs/heads/openpgp-signing) && - test $OPENPGP_SIGNING = $IMPORTED && - git -C new cat-file commit "$IMPORTED" >actual && - test_grep -E "^gpgsig(-sha256)? " actual && - test_must_be_empty log -' - -test_expect_success GPG 'strip signature invalidated by message change with --signed-commits=strip-if-invalid' ' +for mode in strip-if-invalid sign-if-invalid +do + test_expect_success GPG "import commit with no signature with --signed-commits=$mode" ' + git fast-export main >output && + git -C new fast-import --quiet --signed-commits=$mode <output >log 2>&1 && + test_must_be_empty log + ' + + test_expect_success GPG "keep valid OpenPGP signature with --signed-commits=$mode" ' + rm -rf new && + git init new && + + git fast-export --signed-commits=verbatim openpgp-signing >output && + git -C new fast-import --quiet --signed-commits=$mode <output >log 2>&1 && + IMPORTED=$(git -C new rev-parse --verify refs/heads/openpgp-signing) && + test $OPENPGP_SIGNING = $IMPORTED && + git -C new cat-file commit "$IMPORTED" >actual && + test_grep -E "^gpgsig(-sha256)? " actual && + test_must_be_empty log + ' + + test_expect_success GPG "handle signature invalidated by message change with --signed-commits=$mode" ' + rm -rf new && + git init new && + + git fast-export --signed-commits=verbatim openpgp-signing >output && + + # Change the commit message, which invalidates the signature. + # The commit message length should not change though, otherwise the + # corresponding `data <length>` command would have to be changed too. + sed "s/OpenPGP signed commit/OpenPGP forged commit/" output >modified && + + git -C new fast-import --quiet --signed-commits=$mode <modified >log 2>&1 && + + IMPORTED=$(git -C new rev-parse --verify refs/heads/openpgp-signing) && + test $OPENPGP_SIGNING != $IMPORTED && + git -C new cat-file commit "$IMPORTED" >actual && + + if test "$mode" = strip-if-invalid + then + test_grep "stripping invalid signature" log && + test_grep ! -E "^gpgsig" actual + else + test_grep "replacing invalid signature" log && + test_grep -E "^gpgsig(-sha256)? " actual && + git -C new verify-commit "$IMPORTED" + fi + ' + + test_expect_success GPGSM "keep valid X.509 signature with --signed-commits=$mode" ' + rm -rf new && + git init new && + + git fast-export --signed-commits=verbatim x509-signing >output && + git -C new fast-import --quiet --signed-commits=$mode <output >log 2>&1 && + IMPORTED=$(git -C new rev-parse --verify refs/heads/x509-signing) && + test $X509_SIGNING = $IMPORTED && + git -C new cat-file commit "$IMPORTED" >actual && + test_grep -E "^gpgsig(-sha256)? " actual && + test_must_be_empty log + ' + + test_expect_success GPGSSH "keep valid SSH signature with --signed-commits=$mode" ' + rm -rf new && + git init new && + + test_config -C new gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && + + git fast-export --signed-commits=verbatim ssh-signing >output && + git -C new fast-import --quiet --signed-commits=$mode <output >log 2>&1 && + IMPORTED=$(git -C new rev-parse --verify refs/heads/ssh-signing) && + test $SSH_SIGNING = $IMPORTED && + git -C new cat-file commit "$IMPORTED" >actual && + test_grep -E "^gpgsig(-sha256)? " actual && + test_must_be_empty log + ' +done + +test_expect_success GPGSSH "sign invalid commit with explicit keyid" ' rm -rf new && git init new && - git fast-export --signed-commits=verbatim openpgp-signing >output && + git fast-export --signed-commits=verbatim ssh-signing >output && # Change the commit message, which invalidates the signature. # The commit message length should not change though, otherwise the # corresponding `data <length>` command would have to be changed too. - sed "s/OpenPGP signed commit/OpenPGP forged commit/" output >modified && - - git -C new fast-import --quiet --signed-commits=strip-if-invalid <modified >log 2>&1 && - - IMPORTED=$(git -C new rev-parse --verify refs/heads/openpgp-signing) && - test $OPENPGP_SIGNING != $IMPORTED && - git -C new cat-file commit "$IMPORTED" >actual && - test_grep ! -E "^gpgsig" actual && - test_grep "stripping invalid signature" log -' - -test_expect_success GPGSM 'keep valid X.509 signature with --signed-commits=strip-if-invalid' ' - rm -rf new && - git init new && - - git fast-export --signed-commits=verbatim x509-signing >output && - git -C new fast-import --quiet --signed-commits=strip-if-invalid <output >log 2>&1 && - IMPORTED=$(git -C new rev-parse --verify refs/heads/x509-signing) && - test $X509_SIGNING = $IMPORTED && - git -C new cat-file commit "$IMPORTED" >actual && - test_grep -E "^gpgsig(-sha256)? " actual && - test_must_be_empty log -' - -test_expect_success GPGSSH 'keep valid SSH signature with --signed-commits=strip-if-invalid' ' - rm -rf new && - git init new && + sed "s/SSH signed commit/SSH forged commit/" output >modified && + # Configure the target repository with an invalid default signing key. + test_config -C new user.signingkey "not-a-real-key-id" && + test_config -C new gpg.format ssh && test_config -C new gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" && + test_must_fail git -C new fast-import --quiet \ + --signed-commits=sign-if-invalid <modified >/dev/null 2>&1 && + + # Import using explicitly provided signing key. + git -C new fast-import --quiet \ + --signed-commits=sign-if-invalid="${GPGSSH_KEY_PRIMARY}" <modified && - git fast-export --signed-commits=verbatim ssh-signing >output && - git -C new fast-import --quiet --signed-commits=strip-if-invalid <output >log 2>&1 && IMPORTED=$(git -C new rev-parse --verify refs/heads/ssh-signing) && - test $SSH_SIGNING = $IMPORTED && + test $SSH_SIGNING != $IMPORTED && git -C new cat-file commit "$IMPORTED" >actual && test_grep -E "^gpgsig(-sha256)? " actual && - test_must_be_empty log + git -C new verify-commit "$IMPORTED" ' test_done -- 2.53.0.381.g628a66ccf6 ^ permalink raw reply related [flat|nested] 60+ messages in thread
* Re: [PATCH v6 3/3] fast-import: add mode to sign commits with invalid signatures 2026-03-13 1:39 ` [PATCH v6 3/3] fast-import: add mode to sign commits with invalid signatures Justin Tobler @ 2026-03-13 6:31 ` Patrick Steinhardt 0 siblings, 0 replies; 60+ messages in thread From: Patrick Steinhardt @ 2026-03-13 6:31 UTC (permalink / raw) To: Justin Tobler; +Cc: git, sandals, christian.couder, gitster, peff On Thu, Mar 12, 2026 at 08:39:38PM -0500, Justin Tobler wrote: > diff --git a/builtin/fast-import.c b/builtin/fast-import.c > index b8a7757cfd..935e688e33 100644 > --- a/builtin/fast-import.c > +++ b/builtin/fast-import.c > @@ -2865,6 +2855,67 @@ 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(_("replacing invalid signature for commit '%.100s...'\n" > + " allegedly by %s"), subject, signer); > + else if (subject_len > 0) > + warning(_("replacing invalid signature for commit '%.*s'\n" > + " allegedly by %s"), subject_len, subject, signer); > + else > + warning(_("replacing invalid signature for commit\n" > + " allegedly by %s"), signer); > + break; > + default: > + BUG("unsupported signing mode"); > + } > +} The wording of those warnings also reads better than before now. Patrick ^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH v6 0/3] fast-import: add mode to re-sign invalid commit signatures 2026-03-13 1:39 ` [PATCH v6 " Justin Tobler ` (2 preceding siblings ...) 2026-03-13 1:39 ` [PATCH v6 3/3] fast-import: add mode to sign commits with invalid signatures Justin Tobler @ 2026-03-13 4:29 ` Junio C Hamano 2026-03-13 6:31 ` Patrick Steinhardt 3 siblings, 1 reply; 60+ messages in thread From: Junio C Hamano @ 2026-03-13 4:29 UTC (permalink / raw) To: Justin Tobler; +Cc: git, sandals, christian.couder, ps, peff Justin Tobler <jltobler@gmail.com> writes: > Changes since V5: > - Fixed a test that was incorrectly referencing the openpgp-signing > branch when it should be using the ssh-signing branch. > - Changed warning message wording. > - Added some parentheses in a conditional statement to clarify operation > order. All three of the above look familiar ;-) Looking good. Will replace. Thanks. ^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH v6 0/3] fast-import: add mode to re-sign invalid commit signatures 2026-03-13 4:29 ` [PATCH v6 0/3] fast-import: add mode to re-sign invalid commit signatures Junio C Hamano @ 2026-03-13 6:31 ` Patrick Steinhardt 0 siblings, 0 replies; 60+ messages in thread From: Patrick Steinhardt @ 2026-03-13 6:31 UTC (permalink / raw) To: Junio C Hamano; +Cc: Justin Tobler, git, sandals, christian.couder, peff On Thu, Mar 12, 2026 at 09:29:37PM -0700, Junio C Hamano wrote: > Justin Tobler <jltobler@gmail.com> writes: > > > Changes since V5: > > - Fixed a test that was incorrectly referencing the openpgp-signing > > branch when it should be using the ssh-signing branch. > > - Changed warning message wording. > > - Added some parentheses in a conditional statement to clarify operation > > order. > > All three of the above look familiar ;-) Looking good. > > Will replace. Thanks. I'm happy with this version. Thanks! Patrick ^ permalink raw reply [flat|nested] 60+ messages in thread
end of thread, other threads:[~2026-03-13 6:32 UTC | newest] Thread overview: 60+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-02-23 19:41 [PATCH 0/2] fast-import: add mode to re-sign invalid commit signatures Justin Tobler 2026-02-23 19:41 ` [PATCH 1/2] commit: remove unused forward declaration Justin Tobler 2026-02-24 9:35 ` Patrick Steinhardt 2026-02-23 19:41 ` [PATCH 2/2] fast-import: add mode to re-sign invalid commit signatures Justin Tobler 2026-02-24 9:33 ` Patrick Steinhardt 2026-02-24 18:33 ` Justin Tobler 2026-02-24 13:40 ` [PATCH 0/2] " Christian Couder 2026-02-24 22:41 ` brian m. carlson 2026-02-24 22:45 ` Junio C Hamano 2026-03-02 22:49 ` Justin Tobler 2026-03-06 20:53 ` [PATCH v2 0/3] " Justin Tobler 2026-03-06 20:53 ` [PATCH v2 1/3] commit: remove unused forward declaration Justin Tobler 2026-03-06 20:53 ` [PATCH v2 2/3] gpg-interface: introduce sign_buffer_with_key() Justin Tobler 2026-03-10 9:01 ` Christian Couder 2026-03-10 18:04 ` Justin Tobler 2026-03-06 20:53 ` [PATCH v2 3/3] fast-import: add mode to re-sign invalid commit signatures Justin Tobler 2026-03-10 9:27 ` Christian Couder 2026-03-10 18:09 ` Justin Tobler 2026-03-10 20:11 ` [PATCH v3 0/3] " Justin Tobler 2026-03-10 20:11 ` [PATCH v3 1/3] commit: remove unused forward declaration Justin Tobler 2026-03-10 22:29 ` Junio C Hamano 2026-03-10 20:11 ` [PATCH v3 2/3] gpg-interface: introduce sign_buffer_with_key() Justin Tobler 2026-03-10 22:33 ` Junio C Hamano 2026-03-10 20:11 ` [PATCH v3 3/3] fast-import: add mode to re-sign invalid commit signatures Justin Tobler 2026-03-10 20:49 ` [PATCH v3 0/3] " Junio C Hamano 2026-03-10 21:06 ` Justin Tobler 2026-03-10 21:20 ` Junio C Hamano 2026-03-10 22:13 ` Justin Tobler 2026-03-10 22:39 ` Junio C Hamano 2026-03-10 23:03 ` Justin Tobler 2026-03-11 17:31 ` [PATCH v4 " Justin Tobler 2026-03-11 17:31 ` [PATCH v4 1/3] commit: remove unused forward declaration Justin Tobler 2026-03-11 17:31 ` [PATCH v4 2/3] gpg-interface: introduce sign_buffer_with_key() Justin Tobler 2026-03-12 10:22 ` Patrick Steinhardt 2026-03-12 13:58 ` Justin Tobler 2026-03-11 17:31 ` [PATCH v4 3/3] fast-import: add mode to sign commits with invalid signatures Justin Tobler 2026-03-12 10:23 ` Patrick Steinhardt 2026-03-12 14:08 ` Justin Tobler 2026-03-12 14:22 ` Patrick Steinhardt 2026-03-12 17:21 ` Justin Tobler 2026-03-12 19:22 ` [PATCH v5 0/3] fast-import: add mode to re-sign invalid commit signatures Justin Tobler 2026-03-12 19:22 ` [PATCH v5 1/3] commit: remove unused forward declaration Justin Tobler 2026-03-12 19:22 ` [PATCH v5 2/3] gpg-interface: allow sign_buffer() to use default signing key Justin Tobler 2026-03-12 20:20 ` Junio C Hamano 2026-03-12 20:24 ` Justin Tobler 2026-03-12 19:22 ` [PATCH v5 3/3] fast-import: add mode to sign commits with invalid signatures Justin Tobler 2026-03-12 20:20 ` Junio C Hamano 2026-03-12 20:29 ` Justin Tobler 2026-03-12 23:58 ` Jeff King 2026-03-13 0:17 ` Justin Tobler 2026-03-12 20:20 ` [PATCH v5 0/3] fast-import: add mode to re-sign invalid commit signatures Junio C Hamano 2026-03-12 20:30 ` Justin Tobler 2026-03-13 1:39 ` [PATCH v6 " Justin Tobler 2026-03-13 1:39 ` [PATCH v6 1/3] commit: remove unused forward declaration Justin Tobler 2026-03-13 1:39 ` [PATCH v6 2/3] gpg-interface: allow sign_buffer() to use default signing key Justin Tobler 2026-03-13 6:31 ` Patrick Steinhardt 2026-03-13 1:39 ` [PATCH v6 3/3] fast-import: add mode to sign commits with invalid signatures Justin Tobler 2026-03-13 6:31 ` Patrick Steinhardt 2026-03-13 4:29 ` [PATCH v6 0/3] fast-import: add mode to re-sign invalid commit signatures Junio C Hamano 2026-03-13 6:31 ` Patrick Steinhardt
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox