From 388517c14ce62e1c52b091af862bbaf28dbabb7a Mon Sep 17 00:00:00 2001 From: Christian Couder Date: Mon, 17 Nov 2025 05:34:48 +0100 Subject: [PATCH 1/3] fast-import: refactor finalize_commit_buffer() In a following commit we are going to finalize commit buffers with or without signatures in order to check the signatures and possibly drop them. To do so easily and without duplication, let's refactor the current code that finalizes commit buffers into a new finalize_commit_buffer() function. Signed-off-by: Christian Couder Signed-off-by: Junio C Hamano --- builtin/fast-import.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/builtin/fast-import.c b/builtin/fast-import.c index 54d3e592c6..493de57ef6 100644 --- a/builtin/fast-import.c +++ b/builtin/fast-import.c @@ -2815,6 +2815,18 @@ static void import_one_signature(struct signature_data *sig_sha1, die(_("parse_one_signature() returned unknown hash algo")); } +static void finalize_commit_buffer(struct strbuf *new_data, + struct signature_data *sig_sha1, + struct signature_data *sig_sha256, + struct strbuf *msg) +{ + add_gpgsig_to_commit(new_data, "gpgsig ", sig_sha1); + add_gpgsig_to_commit(new_data, "gpgsig-sha256 ", sig_sha256); + + strbuf_addch(new_data, '\n'); + strbuf_addbuf(new_data, msg); +} + static void parse_new_commit(const char *arg) { static struct strbuf msg = STRBUF_INIT; @@ -2950,11 +2962,8 @@ static void parse_new_commit(const char *arg) "encoding %s\n", encoding); - add_gpgsig_to_commit(&new_data, "gpgsig ", &sig_sha1); - add_gpgsig_to_commit(&new_data, "gpgsig-sha256 ", &sig_sha256); + finalize_commit_buffer(&new_data, &sig_sha1, &sig_sha256, &msg); - strbuf_addch(&new_data, '\n'); - strbuf_addbuf(&new_data, &msg); free(author); free(committer); free(encoding); From cb034c020aba54360e7c19faf82021399bf131e7 Mon Sep 17 00:00:00 2001 From: Christian Couder Date: Mon, 17 Nov 2025 05:34:49 +0100 Subject: [PATCH 2/3] commit: refactor verify_commit_buffer() In a following commit, we are going to check commit signatures, but we won't have a commit yet, only a commit buffer, and we are going to discard this commit buffer if the signature is invalid. So it would be wasteful to create a commit that we might discard, just to be able to check a commit signature. It would be simpler instead to be able to check commit signatures using only a commit buffer instead of a commit. To be able to do that, let's extract some code from the check_commit_signature() function into a new verify_commit_buffer() function, and then let's make check_commit_signature() call verify_commit_buffer(). Note that this doesn't fundamentally change how check_commit_signature() works. It used to call parse_signed_commit() which calls repo_get_commit_buffer(), parse_buffer_signed_by_header() and repo_unuse_commit_buffer(). Now these 3 functions are called directly by verify_commit_buffer(). Signed-off-by: Christian Couder Signed-off-by: Junio C Hamano --- commit.c | 17 +++++++++++++++-- commit.h | 7 +++++++ 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/commit.c b/commit.c index 16d91b2bfc..709c9eed58 100644 --- a/commit.c +++ b/commit.c @@ -1315,7 +1315,8 @@ free_return: free(buf); } -int check_commit_signature(const struct commit *commit, struct signature_check *sigc) +int verify_commit_buffer(const char *buffer, size_t size, + struct signature_check *sigc) { struct strbuf payload = STRBUF_INIT; struct strbuf signature = STRBUF_INIT; @@ -1323,7 +1324,8 @@ int check_commit_signature(const struct commit *commit, struct signature_check * sigc->result = 'N'; - if (parse_signed_commit(commit, &payload, &signature, the_hash_algo) <= 0) + if (parse_buffer_signed_by_header(buffer, size, &payload, + &signature, the_hash_algo) <= 0) goto out; sigc->payload_type = SIGNATURE_PAYLOAD_COMMIT; @@ -1337,6 +1339,17 @@ int check_commit_signature(const struct commit *commit, struct signature_check * return ret; } +int check_commit_signature(const struct commit *commit, struct signature_check *sigc) +{ + unsigned long size; + const char *buffer = repo_get_commit_buffer(the_repository, commit, &size); + int ret = verify_commit_buffer(buffer, size, sigc); + + repo_unuse_commit_buffer(the_repository, commit, buffer); + + return ret; +} + void verify_merge_signature(struct commit *commit, int verbosity, int check_trust) { diff --git a/commit.h b/commit.h index 1d6e0c7518..5406dd2663 100644 --- a/commit.h +++ b/commit.h @@ -333,6 +333,13 @@ int remove_signature(struct strbuf *buf); */ int check_commit_signature(const struct commit *commit, struct signature_check *sigc); +/* + * Same as check_commit_signature() but accepts a commit buffer and + * its size, instead of a `struct commit *`. + */ +int verify_commit_buffer(const char *buffer, size_t size, + struct signature_check *sigc); + /* record author-date for each commit object */ struct author_date_slab; void record_author_date(struct author_date_slab *author_date, From c20f112e5149d1bd0d4741c4b28a65f81318309a Mon Sep 17 00:00:00 2001 From: Christian Couder Date: Mon, 17 Nov 2025 05:34:50 +0100 Subject: [PATCH 3/3] fast-import: add 'strip-if-invalid' mode to --signed-commits= Tools like `git filter-repo`[1] use `git fast-export` and `git fast-import` to rewrite repository history. When rewriting history using one such tool though, commit signatures might become invalid because the commits they sign changed due to the changes in the repository history made by the tool between the fast-export and the fast-import steps. Note that as far as signature handling goes: * Since fast-export doesn't know what changes filter-repo may make to the stream, it can't know whether the signatures will still be valid. * Since filter-repo doesn't know what history canonicalizations fast-export performed (and it performs a few), it can't know whether the signatures will still be valid. * Therefore, fast-import is the only process in the pipeline that can know whether a specified signature remains valid. Having invalid signatures in a rewritten repository could be confusing, so users rewritting history might prefer to simply discard signatures that are invalid at the fast-import step. For example a common use case is to rewrite only "recent" history. While specifying commit ranges corresponding to "recent" commits could work, users worry about getting it wrong and want to just automatically rewrite everything, expecting older commit signatures to be untouched. To let them do that, let's add a new 'strip-if-invalid' mode to the `--signed-commits=` option of `git fast-import`. It would be interesting for the `--signed-tags=` option to have this mode too, but we leave that for a future improvement. It might also be possible for `git fast-export` to have such a mode in its `--signed-commits=` and `--signed-tags=` options, but the use cases for it are much less clear, so we also leave that for possible future improvements. For now let's just die() if 'strip-if-invalid' is passed to these options where it hasn't been implemented yet. [1]: https://github.com/newren/git-filter-repo Helped-by: Elijah Newren Signed-off-by: Christian Couder Signed-off-by: Junio C Hamano --- Documentation/git-fast-import.adoc | 27 ++++++++---- builtin/fast-export.c | 38 ++++++++++++---- builtin/fast-import.c | 59 ++++++++++++++++++++++--- gpg-interface.c | 2 + gpg-interface.h | 1 + t/t9305-fast-import-signatures.sh | 69 +++++++++++++++++++++++++++++- 6 files changed, 173 insertions(+), 23 deletions(-) diff --git a/Documentation/git-fast-import.adoc b/Documentation/git-fast-import.adoc index b74179a6c8..479c4081da 100644 --- a/Documentation/git-fast-import.adoc +++ b/Documentation/git-fast-import.adoc @@ -66,15 +66,26 @@ fast-import stream! This option is enabled automatically for remote-helpers that use the `import` capability, as they are already trusted to run their own code. ---signed-tags=(verbatim|warn-verbatim|warn-strip|strip|abort):: - Specify how to handle signed tags. Behaves in the same way - as the same option in linkgit:git-fast-export[1], except that - default is 'verbatim' (instead of 'abort'). +`--signed-tags=(verbatim|warn-verbatim|warn-strip|strip|abort)`:: + Specify how to handle signed tags. Behaves in the same way as + the `--signed-commits=` below, except that the + `strip-if-invalid` mode is not yet supported. Like for signed + commits, the default mode is `verbatim`. ---signed-commits=(verbatim|warn-verbatim|warn-strip|strip|abort):: - Specify how to handle signed commits. Behaves in the same way - as the same option in linkgit:git-fast-export[1], except that - default is 'verbatim' (instead of 'abort'). +`--signed-commits=`:: + Specify how to handle signed commits. The following s + are supported: ++ +* `verbatim`, which is the default, will silently import commit + signatures. +* `warn-verbatim` will import them, but will display a warning. +* `abort` will make this program die when encountering a signed + commit. +* `strip` will silently make the commits unsigned. +* `warn-strip` will make them unsigned, but will display a warning. +* `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. Options for Frontends ~~~~~~~~~~~~~~~~~~~~~ diff --git a/builtin/fast-export.c b/builtin/fast-export.c index 7adbc55f0d..a839a8f9ac 100644 --- a/builtin/fast-export.c +++ b/builtin/fast-export.c @@ -797,10 +797,8 @@ static void handle_commit(struct commit *commit, struct rev_info *rev, (int)(committer_end - committer), committer); if (signatures.nr) { switch (signed_commit_mode) { - case SIGN_ABORT: - die("encountered signed commit %s; use " - "--signed-commits= to handle it", - oid_to_hex(&commit->object.oid)); + + /* Exporting modes */ case SIGN_WARN_VERBATIM: warning("exporting %"PRIuMAX" signature(s) for commit %s", (uintmax_t)signatures.nr, oid_to_hex(&commit->object.oid)); @@ -811,12 +809,25 @@ static void handle_commit(struct commit *commit, struct rev_info *rev, print_signature(item->string, item->util); } break; + + /* Stripping modes */ case SIGN_WARN_STRIP: warning("stripping signature(s) from commit %s", oid_to_hex(&commit->object.oid)); /* fallthru */ case SIGN_STRIP: break; + + /* Aborting modes */ + case SIGN_ABORT: + die(_("encountered signed commit %s; use " + "--signed-commits= to handle it"), + oid_to_hex(&commit->object.oid)); + case SIGN_STRIP_IF_INVALID: + die(_("'strip-if-invalid' is not a valid mode for " + "git fast-export with --signed-commits=")); + default: + BUG("invalid signed_commit_mode value %d", signed_commit_mode); } string_list_clear(&signatures, 0); } @@ -934,16 +945,16 @@ static void handle_tag(const char *name, struct tag *tag) size_t sig_offset = parse_signed_buffer(message, message_size); if (sig_offset < message_size) switch (signed_tag_mode) { - case SIGN_ABORT: - die("encountered signed tag %s; use " - "--signed-tags= to handle it", - oid_to_hex(&tag->object.oid)); + + /* Exporting modes */ case SIGN_WARN_VERBATIM: warning("exporting signed tag %s", oid_to_hex(&tag->object.oid)); /* fallthru */ case SIGN_VERBATIM: break; + + /* Stripping modes */ case SIGN_WARN_STRIP: warning("stripping signature from tag %s", oid_to_hex(&tag->object.oid)); @@ -951,6 +962,17 @@ static void handle_tag(const char *name, struct tag *tag) case SIGN_STRIP: message_size = sig_offset; break; + + /* Aborting modes */ + case SIGN_ABORT: + die(_("encountered signed tag %s; use " + "--signed-tags= to handle it"), + oid_to_hex(&tag->object.oid)); + case SIGN_STRIP_IF_INVALID: + die(_("'strip-if-invalid' is not a valid mode for " + "git fast-export with --signed-tags=")); + default: + BUG("invalid signed_commit_mode value %d", signed_commit_mode); } } diff --git a/builtin/fast-import.c b/builtin/fast-import.c index 493de57ef6..e2c6894461 100644 --- a/builtin/fast-import.c +++ b/builtin/fast-import.c @@ -2772,7 +2772,7 @@ static void add_gpgsig_to_commit(struct strbuf *commit_data, { struct string_list siglines = STRING_LIST_INIT_NODUP; - if (!sig->hash_algo) + if (!sig || !sig->hash_algo) return; strbuf_addstr(commit_data, header); @@ -2827,6 +2827,45 @@ 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) +{ + 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); + + 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); + + finalize_commit_buffer(new_data, NULL, NULL, msg); + } else { + strbuf_swap(new_data, &tmp_buf); + } + + signature_check_clear(&signature_check); + strbuf_release(&tmp_buf); +} + static void parse_new_commit(const char *arg) { static struct strbuf msg = STRBUF_INIT; @@ -2878,6 +2917,7 @@ static void parse_new_commit(const char *arg) warning(_("importing a commit signature verbatim")); /* fallthru */ case SIGN_VERBATIM: + case SIGN_STRIP_IF_INVALID: import_one_signature(&sig_sha1, &sig_sha256, v); break; @@ -2962,7 +3002,11 @@ static void parse_new_commit(const char *arg) "encoding %s\n", encoding); - finalize_commit_buffer(&new_data, &sig_sha1, &sig_sha256, &msg); + if (signed_commit_mode == SIGN_STRIP_IF_INVALID && + (sig_sha1.hash_algo || sig_sha256.hash_algo)) + handle_strip_if_invalid(&new_data, &sig_sha1, &sig_sha256, &msg); + else + finalize_commit_buffer(&new_data, &sig_sha1, &sig_sha256, &msg); free(author); free(committer); @@ -2984,9 +3028,6 @@ static void handle_tag_signature(struct strbuf *msg, const char *name) switch (signed_tag_mode) { /* First, modes that don't change anything */ - case SIGN_ABORT: - die(_("encountered signed tag; use " - "--signed-tags= to handle it")); case SIGN_WARN_VERBATIM: warning(_("importing a tag signature verbatim for tag '%s'"), name); /* fallthru */ @@ -3003,7 +3044,13 @@ static void handle_tag_signature(struct strbuf *msg, const char *name) strbuf_setlen(msg, sig_offset); break; - /* Third, BUG */ + /* Third, aborting modes */ + case SIGN_ABORT: + die(_("encountered signed tag; use " + "--signed-tags= to handle it")); + case SIGN_STRIP_IF_INVALID: + die(_("'strip-if-invalid' is not a valid mode for " + "git fast-import with --signed-tags=")); 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 d1e88da8c1..fe653b2464 100644 --- a/gpg-interface.c +++ b/gpg-interface.c @@ -1146,6 +1146,8 @@ int parse_sign_mode(const char *arg, enum sign_mode *mode) *mode = SIGN_WARN_STRIP; else if (!strcmp(arg, "strip")) *mode = SIGN_STRIP; + else if (!strcmp(arg, "strip-if-invalid")) + *mode = SIGN_STRIP_IF_INVALID; else return -1; return 0; diff --git a/gpg-interface.h b/gpg-interface.h index 50487aa148..71dde8cb80 100644 --- a/gpg-interface.h +++ b/gpg-interface.h @@ -111,6 +111,7 @@ enum sign_mode { SIGN_VERBATIM, SIGN_WARN_STRIP, SIGN_STRIP, + SIGN_STRIP_IF_INVALID, }; /* diff --git a/t/t9305-fast-import-signatures.sh b/t/t9305-fast-import-signatures.sh index c2b4271658..022dae02e4 100755 --- a/t/t9305-fast-import-signatures.sh +++ b/t/t9305-fast-import-signatures.sh @@ -79,7 +79,7 @@ test_expect_success GPG 'setup a commit with dual OpenPGP signatures on its SHA- echo B >explicit-sha256/B && git -C explicit-sha256 add B && test_tick && - git -C explicit-sha256 commit -S -m "signed" B && + git -C explicit-sha256 commit -S -m "signed commit" B && SHA256_B=$(git -C explicit-sha256 rev-parse dual-signed) && # Create the corresponding SHA-1 commit @@ -103,4 +103,71 @@ 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 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 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 ` 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 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 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 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 +' + test_done