From 02ce924afdf67c6efca45a7b20ba91aa0f599d18 Mon Sep 17 00:00:00 2001 From: Christian Couder Date: Tue, 28 Oct 2025 12:54:46 +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 --- 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 54d3e592c6e..493de57ef67 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); -- GitLab From 1593adc7b20ea564cda3a3b2f3a44eded8cda722 Mon Sep 17 00:00:00 2001 From: Christian Couder Date: Tue, 28 Oct 2025 12:59:24 +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 --- commit.c | 17 +++++++++++++++-- commit.h | 7 +++++++ 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/commit.c b/commit.c index 16d91b2bfcf..709c9eed58a 100644 --- a/commit.c +++ b/commit.c @@ -1315,7 +1315,8 @@ static void handle_signed_tag(const struct commit *parent, struct commit_extra_h 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 1d6e0c7518b..5406dd26632 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, -- GitLab From f264cd25e5d31b1c0fb7173129a692f26816b85c Mon Sep 17 00:00:00 2001 From: Christian Couder Date: Mon, 27 Oct 2025 16:33:24 +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. 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. 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. While at it, let's also mark for translation some error messages linked to the `--signed-commits=` and `--signed-tags=` in `git fast-export`. [1]: https://github.com/newren/git-filter-repo Signed-off-by: Christian Couder --- Documentation/git-fast-import.adoc | 28 ++++--- builtin/fast-export.c | 46 ++++++++--- builtin/fast-import.c | 59 +++++++++++++-- gpg-interface.c | 2 + gpg-interface.h | 1 + t/t9305-fast-import-signatures.sh | 118 ++++++++++++++++++++++++++++- 6 files changed, 226 insertions(+), 28 deletions(-) diff --git a/Documentation/git-fast-import.adoc b/Documentation/git-fast-import.adoc index b74179a6c89..c9e49497cd9 100644 --- a/Documentation/git-fast-import.adoc +++ b/Documentation/git-fast-import.adoc @@ -66,15 +66,25 @@ 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-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-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=`:: + 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. Options for Frontends ~~~~~~~~~~~~~~~~~~~~~ diff --git a/builtin/fast-export.c b/builtin/fast-export.c index 7adbc55f0dc..1ad195b639a 100644 --- a/builtin/fast-export.c +++ b/builtin/fast-export.c @@ -797,12 +797,10 @@ 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", + warning(_("exporting %"PRIuMAX" signature(s) for commit %s"), (uintmax_t)signatures.nr, oid_to_hex(&commit->object.oid)); /* fallthru */ case SIGN_VERBATIM: @@ -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", + 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,23 +945,34 @@ 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", + 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", + warning(_("stripping signature from tag %s"), oid_to_hex(&tag->object.oid)); /* fallthru */ 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 493de57ef67..e2c68944610 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 d1e88da8c1b..fe653b24643 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 50487aa1483..71dde8cb807 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 c2b42716586..db77ace4723 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,120 @@ 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 GPG 'keep valid dual OpenPGP signatures with --signed-commits=strip-if-invalid' ' + rm -rf new && + git init new && + + git -C explicit-sha256 fast-export --signed-commits=verbatim dual-signed >output && + git -C new fast-import --quiet --signed-commits=strip-if-invalid log 2>&1 && + + git -C new cat-file commit refs/heads/dual-signed >actual && + test_grep -E "^gpgsig " actual && + test_grep -E "^gpgsig-sha256 " actual && + test_must_be_empty log && + + IMPORTED=$(git -C new rev-parse refs/heads/dual-signed) && + if test "$GIT_DEFAULT_HASH" = "sha1" + then + test $SHA1_B = $IMPORTED + else + test $SHA256_B = $IMPORTED + fi +' + +test_expect_success GPG 'strip both invalid dual OpenPGP signatures with --signed-commits=strip-if-invalid' ' + rm -rf new && + git init new && + + git -C explicit-sha256 fast-export --signed-commits=verbatim dual-signed >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/signed commit/forged commit/" output >modified && + + git -C new fast-import --quiet --signed-commits=strip-if-invalid log 2>&1 && + + git -C new cat-file commit refs/heads/dual-signed >actual && + test_grep ! -E "^gpgsig " actual && + test_grep ! -E "^gpgsig-sha256 " actual && + + IMPORTED=$(git -C new rev-parse refs/heads/dual-signed) && + if test "$GIT_DEFAULT_HASH" = "sha1" + then + test $SHA1_B != $IMPORTED + else + test $SHA256_B != $IMPORTED + fi && + + 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 -- GitLab