Remote helpers and signed tags

Remote helpers and signed tags

[John Keeping]

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)".

Re: Remote helpers and signed tags

[Jonathan Nieder]

Hi John,

John Keeping wrote:

> It appears to be impossible to push signed tags using a remote helper
> that supports only fast-export.
[...]
>     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

Re: Remote helpers and signed tags

[Sverre Rabbelier]

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.

--
Cheers,

Sverre Rabbelier

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

[John Keeping]

On Sun, Apr 07, 2013 at 05:02:48PM -0700, Sverre Rabbelier wrote:
> 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

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

[John Keeping]

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

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

[John Keeping]

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

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

[John Keeping]

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

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

[Sverre Rabbelier]

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

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

[Junio C Hamano]

Sverre Rabbelier <srabbelier@gmail.com> writes:

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

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

[Sverre Rabbelier]

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

--
Cheers,

Sverre Rabbelier

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

[Jonathan Nieder]

Junio C Hamano wrote:
> Sverre Rabbelier <srabbelier@gmail.com> writes:

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

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

[John Keeping]

On Mon, Apr 15, 2013 at 09:50:42PM -0700, Sverre Rabbelier wrote:
> 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?

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

[Junio C Hamano]

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?
 

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

[Sverre Rabbelier]

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