threads / discuss / 33416

Remote helpers and signed tags

Subject: Remote helpers and signed tags

## tl;dr

14 messages between Apr 7, 2013 and Apr 17, 2013.

replies: 13people: 4as markdown or json

John Keeping· Apr 7, 2013, 10:34 UTC · lore

It appears to be impossible to push signed tags using a remote helper that supports only fast-export. This is reported against gitifyhg[1] but I think it is actually a Git issue.

[1] https://github.com/buchuki/gitifyhg/issues/59

I can reproduce the error using a trivial remote helper (run this in a clone of git.git):

-- >8 -- cat >git-remote-export <<EOF && #!/bin/sh

alias=$1 url=${2-$1}

while read -r line
do
	case "$line" in
	capabilities)
		echo 'export'
		echo 'refspec *:*'
		echo
		;;
	list)
		echo
		;;
	export)
		while read -r line
		do
			echo "$line" >&3
			test "$line" = done && break
		done 3>"$url"
		echo
		;;
	'')
		exit
		;;
	*)
		echo >&2 "unsupported command: $line"
		exit 1
		;;
	esac
done
EOF
chmod +x git-remote-export &&
PATH="$(pwd):$PATH" git push "export::$(pwd)/v1.8.2.export" v1.8.2
-- 8< --
This produces:
    fatal: Encountered signed tag 572a535454612a046e7dd7404dcca94d6243c788;
        use --signed-tag=<mode> to handle it.
    fatal: Error while running fast-export

which is not particularly helpful for a user who doesn't know how the remote helper is implemented, particularly because adding --signed-tag=<mode> to the command won't work.

I think there are two problems here:
    1) The error message is misleading: "--signed-tag" isn't an option
       to git-push and as a user I don't know why Git thought I wanted
       to run fast-export.
    2) There is no way (that I have found) to change the signed-tag
       behaviour of git-fast-export when it is being invoked for a
       remote helper.

How do remote helpers using the "push" method handle this? In that case it seems to be completely up to the helper program to decide what to do.

I wonder if the way forward is to do some combination of:
    a) Add a --signed-tags option to git-push, which is either passed to
       fast-export or given as a new "option signed-tags" to the
       remote-helper when using the push interface (and ignored for the
       connect interface).
    b) Add a configuration variable to specify how to handle signed tags
       when pushing to a remote that uses a remote helper that cannot
       handle them; something like "remote.<name>.signedTags".
    c) Improve the "Error while running fast-export" message to
       something more like "Error pushing with fast-export (using helper
       git-remote-foo)".
Jonathan Nieder· Apr 7, 2013, 21:46 UTC · re: John Keeping · lore

Re: Remote helpers and signed tags

Hi John,
John Keeping wrote:
> It appears to be impossible to push signed tags using a remote helper
> that supports only fast-export.
[...]
Show 6 quoted lines
>     fatal: Encountered signed tag 572a535454612a046e7dd7404dcca94d6243c788;
>         use --signed-tag=<mode> to handle it.
>     fatal: Error while running fast-export
>
> which is not particularly helpful for a user who doesn't know how the
> remote helper is implemented
Yeah, this is idiotic.

The remote helper infrastructure is certainly being unhelpful here. I wonder if transport-helper should just pass --signed-tag=strip and be done with it (leaving open the possibility of a capability to switch to --signed-tag=verbatim when someone wants to teach the testgit helper to support that). What do you think?

Thanks, Jonathan

Sverre Rabbelier· Apr 8, 2013, 00:02 UTC · re: Jonathan Nieder · lore

Re: Remote helpers and signed tags

On Sun, Apr 7, 2013 at 2:46 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 5 quoted lines
> The remote helper infrastructure is certainly being unhelpful here.  I
> wonder if transport-helper should just pass --signed-tag=strip and be
> done with it (leaving open the possibility of a capability to switch
> to --signed-tag=verbatim when someone wants to teach the testgit
> helper to support that).  What do you think?

I think that's (at least for now) the right thing to do. Passing anything but signed-tag=strip should be triggered by a capability from the helper, since most helpers won't know how to deal with signed tags.

-- Cheers,

Sverre Rabbelier
John Keeping· Apr 14, 2013, 10:57 UTC · re: Sverre Rabbelier · lore

[PATCH 0/3] Handle signed tags with 'export' remote helpers

On Sun, Apr 07, 2013 at 05:02:48PM -0700, Sverre Rabbelier wrote:
Show 11 quoted lines
> On Sun, Apr 7, 2013 at 2:46 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> > The remote helper infrastructure is certainly being unhelpful here.  I
> > wonder if transport-helper should just pass --signed-tag=strip and be
> > done with it (leaving open the possibility of a capability to switch
> > to --signed-tag=verbatim when someone wants to teach the testgit
> > helper to support that).  What do you think?
> 
> I think that's (at least for now) the right thing to do. Passing
> anything but signed-tag=strip should be triggered by a capability from
> the helper, since most helpers won't know how to deal with signed
> tags.
I don't like the idea of silently stripping tags, so how about this?

Patch 1 adds a new 'warn-strip' mode to 'fast-export --signed-tags=...' which strips tags but issues a warning when doing so. Then we make transport-helper use that before finally adding a new capability to allow a remote helper to change '--signed-tags=warn-strip' into '--signed-tags=verbatim'.

John Keeping (3):
  fast-export: add --signed-tags=warn-strip mode
  transport-helper: pass --signed-tags=warn-strip to fast-export
  transport-helper: add 'signed-tags' capability
 Documentation/git-fast-export.txt   | 10 ++++++----
 Documentation/gitremote-helpers.txt |  4 ++++
 builtin/fast-export.c               |  8 +++++++-
 git-remote-testgit                  |  1 +
 t/t5801-remote-helpers.sh           | 20 ++++++++++++++++++++
 t/t9350-fast-export.sh              |  6 ++++++
 transport-helper.c                  |  7 ++++++-
 7 files changed, 50 insertions(+), 6 deletions(-)
-- 
1.8.2.694.ga76e9c3.dirty
John Keeping· Apr 14, 2013, 10:57 UTC · re: John Keeping · lore

[PATCH 1/3] fast-export: add --signed-tags=warn-strip mode

This issues a warning while stripping signatures from signed tags, which allows us to use it as default behaviour for remote helpers which cannot specify how to handle signed tags.

Signed-off-by: John Keeping <john@keeping.me.uk>
---
 Documentation/git-fast-export.txt | 10 ++++++----
 builtin/fast-export.c             |  8 +++++++-
 t/t9350-fast-export.sh            |  6 ++++++
 3 files changed, 19 insertions(+), 5 deletions(-)
diff --git a/Documentation/git-fast-export.txt b/Documentation/git-fast-export.txt
index feab7a3..03fc8c3 100644
--- a/Documentation/git-fast-export.txt
+++ b/Documentation/git-fast-export.txt
@@ -27,15 +27,17 @@ OPTIONS
 	Insert 'progress' statements every <n> objects, to be shown by
 	'git fast-import' during import.
 
---signed-tags=(verbatim|warn|strip|abort)::
+--signed-tags=(verbatim|warn|warn-strip|strip|abort)::
 	Specify how to handle signed tags.  Since any transformation
 	after the export can change the tag names (which can also happen
 	when excluding revisions) the signatures will not match.
 +
 When asking to 'abort' (which is the default), this program will die
-when encountering a signed tag.  With 'strip', the tags will be made
-unsigned, with 'verbatim', they will be silently exported
-and with 'warn', they will be exported, but you will see a warning.
+when encountering a signed tag.  With 'strip', the tags will silently
+be made unsigned, with 'warn-strip' they will be made unsigned but a
+warning will be displayed, with 'verbatim', they will be silently
+exported and with 'warn', they will be exported, but you will see a
+warning.
 
 --tag-of-filtered-object=(abort|drop|rewrite)::
 	Specify how to handle tags whose tagged object is filtered out.
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 725c0a7..d60d675 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -24,7 +24,7 @@ static const char *fast_export_usage[] = {
 };
 
 static int progress;
-static enum { ABORT, VERBATIM, WARN, STRIP } signed_tag_mode = ABORT;
+static enum { ABORT, VERBATIM, WARN, WARN_STRIP, STRIP } signed_tag_mode = ABORT;
 static enum { ERROR, DROP, REWRITE } tag_of_filtered_mode = ERROR;
 static int fake_missing_tagger;
 static int use_done_feature;
@@ -40,6 +40,8 @@ static int parse_opt_signed_tag_mode(const struct option *opt,
 		signed_tag_mode = VERBATIM;
 	else if (!strcmp(arg, "warn"))
 		signed_tag_mode = WARN;
+	else if (!strcmp(arg, "warn-strip"))
+		signed_tag_mode = WARN_STRIP;
 	else if (!strcmp(arg, "strip"))
 		signed_tag_mode = STRIP;
 	else
@@ -428,6 +430,10 @@ static void handle_tag(const char *name, struct tag *tag)
 				/* fallthru */
 			case VERBATIM:
 				break;
+			case WARN_STRIP:
+				warning ("Stripping signature from tag %s",
+					 sha1_to_hex(tag->object.sha1));
+				/* fallthru */
 			case STRIP:
 				message_size = signature + 1 - message;
 				break;
diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh
index 9320b4f..2471bc6 100755
--- a/t/t9350-fast-export.sh
+++ b/t/t9350-fast-export.sh
@@ -146,6 +146,12 @@ test_expect_success 'signed-tags=strip' '
 
 '
 
+test_expect_success 'signed-tags=warn-strip' '
+	git fast-export --signed-tags=warn-strip sign-your-name >output 2>err &&
+	! grep PGP output &&
+	test -s err
+'
+
 test_expect_success 'setup submodule' '
 
 	git checkout -f master &&

-- 
1.8.2.694.ga76e9c3.dirty
Sverre Rabbelier· Apr 16, 2013, 04:09 UTC · re: John Keeping · lore

Re: [PATCH 1/3] fast-export: add --signed-tags=warn-strip mode

On Sun, Apr 14, 2013 at 3:57 AM, John Keeping <john@keeping.me.uk> wrote:
> This issues a warning while stripping signatures from signed tags, which
> allows us to use it as default behaviour for remote helpers which cannot
> specify how to handle signed tags.

Perhaps it makes sense to instead count the number of signed tags and emit "Stripped signature from %d tags"? For example, for git.git it would be on the order of a hundred warning lines.

-- Cheers,

Sverre Rabbelier
Junio C Hamano· Apr 16, 2013, 04:47 UTC · re: Sverre Rabbelier · lore

Re: [PATCH 1/3] fast-export: add --signed-tags=warn-strip mode

Sverre Rabbelier <srabbelier@gmail.com> writes:
Show 8 quoted lines
> On Sun, Apr 14, 2013 at 3:57 AM, John Keeping <john@keeping.me.uk> wrote:
>> This issues a warning while stripping signatures from signed tags, which
>> allows us to use it as default behaviour for remote helpers which cannot
>> specify how to handle signed tags.
>
> Perhaps it makes sense to instead count the number of signed tags and
> emit "Stripped signature from %d tags"? For example, for git.git it
> would be on the order of a hundred warning lines.

When you see 78 in the output and you know you have 92 tags in the repository, is that sufficient to let you go on, or do we also need an easy way to tell which ones are those 78 that were stripped and the remaining 14 were not stripped?

There is no reason to worry about "some signed tags are stripped but not others", so it feels that the number alone should be sufficient, I guess. If those remaining 14 weren't stripped, that is (at least at the moment) by definition because they are unsigned, annotated tags.

Sverre Rabbelier· Apr 16, 2013, 04:50 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/3] fast-export: add --signed-tags=warn-strip mode

On Mon, Apr 15, 2013 at 9:47 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 10 quoted lines
> When you see 78 in the output and you know you have 92 tags in the
> repository, is that sufficient to let you go on, or do we also need
> an easy way to tell which ones are those 78 that were stripped and
> the remaining 14 were not stripped?
>
> There is no reason to worry about "some signed tags are stripped but
> not others", so it feels that the number alone should be sufficient,
> I guess.  If those remaining 14 weren't stripped, that is (at least
> at the moment) by definition because they are unsigned, annotated
> tags.

Or because they were not exported? Perhaps "78 tags stripped, 92 exported in total".

-- Cheers,

Sverre Rabbelier
John Keeping· Apr 16, 2013, 08:42 UTC · re: Sverre Rabbelier · lore

Re: [PATCH 1/3] fast-export: add --signed-tags=warn-strip mode

On Mon, Apr 15, 2013 at 09:50:42PM -0700, Sverre Rabbelier wrote:
Show 14 quoted lines
> On Mon, Apr 15, 2013 at 9:47 PM, Junio C Hamano <gitster@pobox.com> wrote:
> > When you see 78 in the output and you know you have 92 tags in the
> > repository, is that sufficient to let you go on, or do we also need
> > an easy way to tell which ones are those 78 that were stripped and
> > the remaining 14 were not stripped?
> >
> > There is no reason to worry about "some signed tags are stripped but
> > not others", so it feels that the number alone should be sufficient,
> > I guess.  If those remaining 14 weren't stripped, that is (at least
> > at the moment) by definition because they are unsigned, annotated
> > tags.
> 
> Or because they were not exported? Perhaps "78 tags stripped, 92
> exported in total".

I think I prefer Jonathan's suggestion to this one if we need to change it.

The reason I didn't do this initially was that I assumed that from a remote helper we would, in general, not be pushing any tags which already exist, so the number of tags to push will be small.

Printing one message per tag also matches the current behaviour for --signed-tags=warn. I don't want to make the behaviour for "warn" and "warn-strip" different, so should "warn" also print a summary message instead of a message for each tag?

Junio C Hamano· Apr 17, 2013, 04:48 UTC · re: John Keeping · lore

Re: [PATCH 1/3] fast-export: add --signed-tags=warn-strip mode

John Keeping <john@keeping.me.uk> writes:
> Printing one message per tag also matches the current behaviour for
> --signed-tags=warn.  I don't want to make the behaviour for "warn" and
> "warn-strip" different,...

That is a valid point. Nobody has complained that the current warning is too noisy, so perhaps the patch is good as-is?

What do others think?
Sverre Rabbelier· Apr 17, 2013, 05:02 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/3] fast-export: add --signed-tags=warn-strip mode

On Tue, Apr 16, 2013 at 9:48 PM, Junio C Hamano <gitster@pobox.com> wrote:
> That is a valid point. Nobody has complained that the current
> warning is too noisy, so perhaps the patch is good as-is?
Ah, hadn't realized that. Probably fine then.

-- Cheers,

Sverre Rabbelier
Jonathan Nieder· Apr 16, 2013, 05:32 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/3] fast-export: add --signed-tags=warn-strip mode

Junio C Hamano wrote:
> Sverre Rabbelier <srabbelier@gmail.com> writes:
Show 8 quoted lines
>> Perhaps it makes sense to instead count the number of signed tags and
>> emit "Stripped signature from %d tags"? For example, for git.git it
>> would be on the order of a hundred warning lines.
>
> When you see 78 in the output and you know you have 92 tags in the
> repository, is that sufficient to let you go on, or do we also need
> an easy way to tell which ones are those 78 that were stripped and
> the remaining 14 were not stripped?
I suspect the actually relevant information is
	warning: stripping signature before pushing signed tags
The count and the list of signed tags are not too important.
John Keeping· Apr 14, 2013, 10:57 UTC · re: John Keeping · lore

[PATCH 2/3] transport-helper: pass --signed-tags=warn-strip to fast-export

Currently, attempting to push a signed tag to a remote helper which uses fast-export results in the remote helper failing because the default fast-export action for signed tags is "abort". This is not helpful for users because there is no way to pass additional arguments to fast-export here, either from the remote helper or from the command line.

In general, the signature will be invalidated by whatever transformation a remote helper performs on a tag to push it to a repository in a different format so the correct behaviour is to strip the tag. Doing this silently may surprise people, so use "warn-strip" to issue a warning when a signed tag is encountered.

Signed-off-by: John Keeping <john@keeping.me.uk>
---
 t/t5801-remote-helpers.sh | 10 ++++++++++
 transport-helper.c        |  3 ++-
 2 files changed, 12 insertions(+), 1 deletion(-)
diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh
index f387027..9b287db 100755
--- a/t/t5801-remote-helpers.sh
+++ b/t/t5801-remote-helpers.sh
@@ -6,6 +6,7 @@
 test_description='Test remote-helper import and export commands'
 
 . ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-gpg.sh
 
 if ! type "${BASH-bash}" >/dev/null 2>&1; then
 	skip_all='skipping remote-testgit tests, bash not available'
@@ -166,4 +167,13 @@ test_expect_success 'push ref with existing object' '
 	compare_refs local dup server dup
 '
 
+test_expect_success GPG 'push signed tag' '
+	(cd local &&
+	git checkout master &&
+	git tag -s -m signed-tag signed-tag &&
+	git push origin signed-tag
+	) &&
+	compare_refs local signed-tag^{} server signed-tag^{}
+'
+
 test_done
diff --git a/transport-helper.c b/transport-helper.c
index dcd8d97..3ce8259 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -410,9 +410,10 @@ static int get_exporter(struct transport *transport,
 	/* we need to duplicate helper->in because we want to use it after
 	 * fastexport is done with it. */
 	fastexport->out = dup(helper->in);
-	fastexport->argv = xcalloc(5 + revlist_args->nr, sizeof(*fastexport->argv));
+	fastexport->argv = xcalloc(6 + revlist_args->nr, sizeof(*fastexport->argv));
 	fastexport->argv[argc++] = "fast-export";
 	fastexport->argv[argc++] = "--use-done-feature";
+	fastexport->argv[argc++] = "--signed-tags=warn-strip";
 	if (data->export_marks)
 		fastexport->argv[argc++] = data->export_marks;
 	if (data->import_marks)

-- 
1.8.2.694.ga76e9c3.dirty
John Keeping· Apr 14, 2013, 10:57 UTC · re: John Keeping · lore

[PATCH 3/3] transport-helper: add 'signed-tags' capability

This allows a remote helper using the 'export' protocol to specify that it supports signed tags, changing the handing from 'warn-strip' to 'verbatim'.

Signed-off-by: John Keeping <john@keeping.me.uk>
---
 Documentation/gitremote-helpers.txt |  4 ++++
 git-remote-testgit                  |  1 +
 t/t5801-remote-helpers.sh           | 12 +++++++++++-
 transport-helper.c                  |  6 +++++-
 4 files changed, 21 insertions(+), 2 deletions(-)
diff --git a/Documentation/gitremote-helpers.txt b/Documentation/gitremote-helpers.txt
index f506031..da74641 100644
--- a/Documentation/gitremote-helpers.txt
+++ b/Documentation/gitremote-helpers.txt
@@ -202,6 +202,10 @@ capability then it should advertise `refspec *:*`.
 	marks specified in <file> before processing any input. For details,
 	read up on '--import-marks=<file>' in linkgit:git-fast-export[1].
 
+'signed-tags'::
+	This modifies the 'export' capability, instructing Git to pass
+	'--signed-tags=verbatim' to linkgit:git-fast-export[1].  In the
+	absence of this capability, Git will use '--signed-tags=warn-strip'.
 
 
 
diff --git a/git-remote-testgit b/git-remote-testgit
index b395c8d..e7ed3a3 100755
--- a/git-remote-testgit
+++ b/git-remote-testgit
@@ -38,6 +38,7 @@ do
 			echo "*import-marks $gitmarks"
 			echo "*export-marks $gitmarks"
 		fi
+		test -n "$GIT_REMOTE_TESTGIT_SIGNED_TAGS" && echo "signed-tags"
 		echo
 		;;
 	list)
diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh
index 9b287db..69212cd 100755
--- a/t/t5801-remote-helpers.sh
+++ b/t/t5801-remote-helpers.sh
@@ -173,7 +173,17 @@ test_expect_success GPG 'push signed tag' '
 	git tag -s -m signed-tag signed-tag &&
 	git push origin signed-tag
 	) &&
-	compare_refs local signed-tag^{} server signed-tag^{}
+	compare_refs local signed-tag^{} server signed-tag^{} &&
+	test_must_fail compare_refs local signed-tag server signed-tag
+'
+
+test_expect_success GPG 'push signed tag with signed-tags capability' '
+	(cd local &&
+	git checkout master &&
+	git tag -s -m signed-tag signed-tag-2 &&
+	GIT_REMOTE_TESTGIT_SIGNED_TAGS=1 git push origin signed-tag-2
+	) &&
+	compare_refs local signed-tag-2 server signed-tag-2
 '
 
 test_done
diff --git a/transport-helper.c b/transport-helper.c
index 3ce8259..5f8d075 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -25,6 +25,7 @@ struct helper_data {
 		option : 1,
 		push : 1,
 		connect : 1,
+		signed_tags : 1,
 		no_disconnect_req : 1;
 	char *export_marks;
 	char *import_marks;
@@ -191,6 +192,8 @@ static struct child_process *get_helper(struct transport *transport)
 			refspecs[refspec_nr++] = xstrdup(capname + strlen("refspec "));
 		} else if (!strcmp(capname, "connect")) {
 			data->connect = 1;
+		} else if (!strcmp(capname, "signed-tags")) {
+			data->signed_tags = 1;
 		} else if (!prefixcmp(capname, "export-marks ")) {
 			struct strbuf arg = STRBUF_INIT;
 			strbuf_addstr(&arg, "--export-marks=");
@@ -413,7 +416,8 @@ static int get_exporter(struct transport *transport,
 	fastexport->argv = xcalloc(6 + revlist_args->nr, sizeof(*fastexport->argv));
 	fastexport->argv[argc++] = "fast-export";
 	fastexport->argv[argc++] = "--use-done-feature";
-	fastexport->argv[argc++] = "--signed-tags=warn-strip";
+	fastexport->argv[argc++] = data->signed_tags ?
+		"--signed-tags=verbatim" : "--signed-tags=warn-strip";
 	if (data->export_marks)
 		fastexport->argv[argc++] = data->export_marks;
 	if (data->import_marks)

-- 
1.8.2.694.ga76e9c3.dirty

← back to recent threads