From: Elijah Newren Date: Fri, 24 Oct 2025 02:03:48 GMT Subject: Re: [PATCH v3 5/5] fast-import: add '--signed-tags=' option Message-ID: In-Reply-To: <20251013084857.1646783-6-christian.couder@gmail.com> On Mon, Oct 13, 2025 at 4:49 AM Christian Couder wrote: > > Recently, eaaddf5791 (fast-import: add '--signed-commits=' > option, 2025-09-17) added support for controlling how signed commits > are handled by `git fast-import`, but there is no option yet to > decide about signed tags. > > To remediate that, let's add a '--signed-tags=' option to > `git fast-import` too. > > With this, both `git fast-export` and `git fast-import` have both > a '--signed-tags=' and a '--signed-commits=' supporting > the same s. > > Signed-off-by: Christian Couder > --- > Documentation/git-fast-import.adoc | 5 ++ > builtin/fast-import.c | 43 ++++++++++++++++ > t/meson.build | 1 + > t/t9306-fast-import-signed-tags.sh | 80 ++++++++++++++++++++++++++++++ > 4 files changed, 129 insertions(+) > create mode 100755 t/t9306-fast-import-signed-tags.sh > > diff --git a/Documentation/git-fast-import.adoc b/Documentation/git-fast-import.adoc > index 85ed7a7270..b74179a6c8 100644 > --- a/Documentation/git-fast-import.adoc > +++ b/Documentation/git-fast-import.adoc > @@ -66,6 +66,11 @@ 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'). Sorry for not catching this earlier with the --signed-commits series (was otherwise occupied), but this worries me. If we ship with this as the default, people will come to depend upon it, and I think it's a bad long term default. Long term, we'd want to check if the signatures are valid and keep if so and do something else if not (e.g. re-sign or abort or strip). Maybe verbatim is better than abort out of the options you've implemented so far, but I think setting the default now to verbatim means people start depending on it and we cannot change it later. Could we change to 'abort', for both this and --signed-commits, before the 2.52 release, and then re-discuss once you have the other options implemented? > + > --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 > diff --git a/builtin/fast-import.c b/builtin/fast-import.c > index 2010e78475..60d6faa465 100644 > --- a/builtin/fast-import.c > +++ b/builtin/fast-import.c > @@ -188,6 +188,7 @@ static int global_argc; > static const char **global_argv; > static const char *global_prefix; > > +static enum sign_mode signed_tag_mode = SIGN_VERBATIM; > static enum sign_mode signed_commit_mode = SIGN_VERBATIM; Here's where you define the defaults, in case there is no --signed-{tag,commit} flags. I think we should go with abort to allow us to later change to a better default once one is implemented. > /* Memory pools */ > @@ -2961,6 +2962,43 @@ static void parse_new_commit(const char *arg) > b->last_commit = object_count_by_type[OBJ_COMMIT]; > } > > +static void handle_tag_signature(struct strbuf *msg, const char *name) > +{ > + size_t sig_offset = parse_signed_buffer(msg->buf, msg->len); > + > + /* If there is no signature, there is nothing to do. */ > + if (sig_offset >= msg->len) > + return; > + > + 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 */ > + case SIGN_VERBATIM: > + /* Nothing to do, the signature will be put into the imported tag. */ > + break; > + > + /* Second, modes that remove the signature */ > + case SIGN_WARN_STRIP: > + warning(_("stripping a tag signature for tag '%s'"), name); > + /* fallthru */ > + case SIGN_STRIP: > + /* Truncate the buffer to remove the signature */ > + strbuf_setlen(msg, sig_offset); > + break; > + > + /* Third, BUG */ > + default: > + BUG("invalid signed_tag_mode value %d from tag '%s'", > + signed_tag_mode, name); > + } Makes sense, you either keep the signature as is, or strip it, with potentially sending a warning (or error) beforehand. > +} > + > static void parse_new_tag(const char *arg) > { > static struct strbuf msg = STRBUF_INIT; > @@ -3024,6 +3062,8 @@ static void parse_new_tag(const char *arg) > /* tag payload/message */ > parse_data(&msg, 0, NULL); > > + handle_tag_signature(&msg, t->name); > + > /* build the tag object */ > strbuf_reset(&new_data); > > @@ -3544,6 +3584,9 @@ static int parse_one_option(const char *option) > } else if (skip_prefix(option, "signed-commits=", &option)) { > if (parse_sign_mode(option, &signed_commit_mode)) > usagef(_("unknown --signed-commits mode '%s'"), option); > + } else if (skip_prefix(option, "signed-tags=", &option)) { > + if (parse_sign_mode(option, &signed_tag_mode)) > + usagef(_("unknown --signed-tags mode '%s'"), option); Re-using the parse_sign_mode() function previously introduced for --signed-commits... > } else if (!strcmp(option, "quiet")) { > show_stats = 0; > quiet = 1; > diff --git a/t/meson.build b/t/meson.build > index 11376b9e25..cb8c2b4b30 100644 > --- a/t/meson.build > +++ b/t/meson.build > @@ -1036,6 +1036,7 @@ integration_tests = [ > 't9303-fast-import-compression.sh', > 't9304-fast-import-marks.sh', > 't9305-fast-import-signatures.sh', > + 't9306-fast-import-signed-tags.sh', > 't9350-fast-export.sh', > 't9351-fast-export-anonymize.sh', > 't9400-git-cvsserver-server.sh', > diff --git a/t/t9306-fast-import-signed-tags.sh b/t/t9306-fast-import-signed-tags.sh > new file mode 100755 > index 0000000000..363619e7d1 > --- /dev/null > +++ b/t/t9306-fast-import-signed-tags.sh > @@ -0,0 +1,80 @@ > +#!/bin/sh > + > +test_description='git fast-import --signed-tags=' > + > +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main > + > +. ./test-lib.sh > +. "$TEST_DIRECTORY/lib-gpg.sh" > + > +test_expect_success 'set up unsigned initial commit and import repo' ' > + test_commit first && > + git init new > +' > + > +test_expect_success 'import no signed tag with --signed-tags=abort' ' > + git fast-export --signed-tags=verbatim >output && > + git -C new fast-import --quiet --signed-tags=abort +' > + > +test_expect_success GPG 'set up OpenPGP signed tag' ' > + git tag -s -m "OpenPGP signed tag" openpgp-signed first && > + OPENPGP_SIGNED=$(git rev-parse --verify refs/tags/openpgp-signed) && > + git fast-export --signed-tags=verbatim openpgp-signed >output > +' > + > +test_expect_success GPG 'import OpenPGP signed tag with --signed-tags=abort' ' > + test_must_fail git -C new fast-import --quiet --signed-tags=abort +' > + > +test_expect_success GPG 'import OpenPGP signed tag with --signed-tags=verbatim' ' > + git -C new fast-import --quiet --signed-tags=verbatim log 2>&1 && > + IMPORTED=$(git -C new rev-parse --verify refs/tags/openpgp-signed) && > + test $OPENPGP_SIGNED = $IMPORTED && > + test_must_be_empty log > +' > + > +test_expect_success GPGSM 'setup X.509 signed tag' ' > + test_config gpg.format x509 && > + test_config user.signingkey $GIT_COMMITTER_EMAIL && > + > + git tag -s -m "X.509 signed tag" x509-signed first && > + X509_SIGNED=$(git rev-parse --verify refs/tags/x509-signed) && > + git fast-export --signed-tags=verbatim x509-signed >output > +' > + > +test_expect_success GPGSM 'import X.509 signed tag with --signed-tags=warn-strip' ' > + git -C new fast-import --quiet --signed-tags=warn-strip log 2>&1 && > + test_grep "stripping a tag signature for tag '\''x509-signed'\''" log && > + IMPORTED=$(git -C new rev-parse --verify refs/tags/x509-signed) && > + test $X509_SIGNED != $IMPORTED && > + git -C new cat-file -p x509-signed >out && > + test_grep ! "SIGNED MESSAGE" out > +' > + > +test_expect_success GPGSSH 'setup SSH signed tag' ' > + test_config gpg.format ssh && > + test_config user.signingkey "${GPGSSH_KEY_PRIMARY}" && > + > + git tag -s -m "SSH signed tag" ssh-signed first && > + SSH_SIGNED=$(git rev-parse --verify refs/tags/ssh-signed) && > + git fast-export --signed-tags=verbatim ssh-signed >output > +' > + > +test_expect_success GPGSSH 'import SSH signed tag with --signed-tags=warn-verbatim' ' > + git -C new fast-import --quiet --signed-tags=warn-verbatim log 2>&1 && > + test_grep "importing a tag signature verbatim for tag '\''ssh-signed'\''" log && > + IMPORTED=$(git -C new rev-parse --verify refs/tags/ssh-signed) && > + test $SSH_SIGNED = $IMPORTED > +' > + > +test_expect_success GPGSSH 'import SSH signed tag with --signed-tags=strip' ' > + git -C new fast-import --quiet --signed-tags=strip log 2>&1 && > + test_must_be_empty log && > + IMPORTED=$(git -C new rev-parse --verify refs/tags/ssh-signed) && > + test $SSH_SIGNED != $IMPORTED && > + git -C new cat-file -p ssh-signed >out && > + test_grep ! "SSH SIGNATURE" out > +' > + > +test_done > -- > 2.51.0.438.g6987fc0bae This all looks good to me, other than the default as noted above.