Re: [PATCH v3 5/5] fast-import: add '--signed-tags=<mode>' option

From: Elijah Newren <newren@gmail.com>
Date: Oct 24, 2025, 02:03 UTC
Message-ID: <CABPp-BGQ=3Tuik-PCerkaK4R0b1roSVLXLKs2-+E11vDrH6WaQ@mail.gmail.com>
In-Reply-To: <20251013084857.1646783-6-christian.couder@gmail.com>

On Mon, Oct 13, 2025 at 4:49 AM Christian Couder <christian.couder@gmail.com> wrote:

Show 34 quoted lines

>
> Recently, eaaddf5791 (fast-import: add '--signed-commits=<mode>'
> 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=<mode>' option to
> `git fast-import` too.
>
> With this, both `git fast-export` and `git fast-import` have both
> a '--signed-tags=<mode>' and a '--signed-commits=<mode>' supporting
> the same <mode>s.
>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
>  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?

Show 14 quoted lines

> +
>  --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.

Show 40 quoted lines

>  /* 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=<mode> 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.

Show 21 quoted lines

> +}
> +
>  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...

Show 103 quoted lines

>         } 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=<mode>'
> +
> +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 <output
> +'
> +
> +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 <output
> +'
> +
> +test_expect_success GPG 'import OpenPGP signed tag with --signed-tags=verbatim' '
> +       git -C new fast-import --quiet --signed-tags=verbatim <output >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 <output >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 <output >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 <output >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.