[PATCH 0/3] fast-export, fast-import: implement signed-commits

[PATCH 0/3] fast-export, fast-import: implement signed-commits

[Luke Shumaker]

From: Luke Shumaker <lukeshu@datawire.io>

fast-export has an existing --signed-tags= flag that controls how to
handle tag signatures.  However, there is no equivalent for commit
signatures; it just silently strips the signature out of the commit
(analogously to --signed-tags=strip).

So implement a --signed-commits= flag in fast-export, and implement
the receiving side of it in fast-import.

Luke Shumaker (3):
  git-fast-import.txt: add missing LF in the BNF
  fast-export: rename --signed-tags='warn' to 'warn-verbatim'
  fast-export, fast-import: implement signed-commits

 Documentation/git-fast-export.txt | 18 +++++--
 Documentation/git-fast-import.txt |  9 +++-
 builtin/fast-export.c             | 88 +++++++++++++++++++++++++------
 builtin/fast-import.c             | 15 ++++++
 t/t9350-fast-export.sh            | 82 ++++++++++++++++++++++++++++
 5 files changed, 191 insertions(+), 21 deletions(-)

-- 
2.31.1

Happy hacking,
~ Luke Shumaker

[PATCH 1/3] git-fast-import.txt: add missing LF in the BNF

[Luke Shumaker]

From: Luke Shumaker <lukeshu@datawire.io>

Signed-off-by: Luke Shumaker <lukeshu@datawire.io>
---
 Documentation/git-fast-import.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
index 39cfa05b28..458af0a2d6 100644
--- a/Documentation/git-fast-import.txt
+++ b/Documentation/git-fast-import.txt
@@ -437,7 +437,7 @@ change to the project.
 	original-oid?
 	('author' (SP <name>)? SP LT <email> GT SP <when> LF)?
 	'committer' (SP <name>)? SP LT <email> GT SP <when> LF
-	('encoding' SP <encoding>)?
+	('encoding' SP <encoding> LF)?
 	data
 	('from' SP <commit-ish> LF)?
 	('merge' SP <commit-ish> LF)*
-- 
2.31.1

[PATCH 2/3] fast-export: rename --signed-tags='warn' to 'warn-verbatim'

[Luke Shumaker]

From: Luke Shumaker <lukeshu@datawire.io>

But still keep --signed-tags=warn as an undocumented alias.  This name
is clearer as it has symmetry with warn-strip:

                   action              ->                   action
       +----------------------------+  ->       +----------------------------+
  msg? |      verbatim |      strip |  ->  msg? |      verbatim |      strip |
       |          warn | warn-strip |  ->       | warn-verbatim | warn-strip |
       +----------------------------+  ->       +----------------------------+

Signed-off-by: Luke Shumaker <lukeshu@datawire.io>
---
 Documentation/git-fast-export.txt |  6 +++---
 builtin/fast-export.c             |  2 +-
 t/t9350-fast-export.sh            | 12 ++++++++++++
 3 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-fast-export.txt b/Documentation/git-fast-export.txt
index 1978dbdc6a..d4a2bfe037 100644
--- a/Documentation/git-fast-export.txt
+++ b/Documentation/git-fast-export.txt
@@ -27,7 +27,7 @@ OPTIONS
 	Insert 'progress' statements every <n> objects, to be shown by
 	'git fast-import' during import.
 
---signed-tags=(verbatim|warn|warn-strip|strip|abort)::
+--signed-tags=(verbatim|warn-verbatim|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.
@@ -36,8 +36,8 @@ When asking to 'abort' (which is the default), this program will die
 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.
+exported and with 'warn-verbatim', 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 85a76e0ef8..d121dd2ee6 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -55,7 +55,7 @@ static int parse_opt_signed_tag_mode(const struct option *opt,
 		signed_tag_mode = SIGNED_TAG_ABORT;
 	else if (!strcmp(arg, "verbatim") || !strcmp(arg, "ignore"))
 		signed_tag_mode = VERBATIM;
-	else if (!strcmp(arg, "warn"))
+	else if (!strcmp(arg, "warn-verbatim") || !strcmp(arg, "warn"))
 		signed_tag_mode = WARN;
 	else if (!strcmp(arg, "warn-strip"))
 		signed_tag_mode = WARN_STRIP;
diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh
index 409b48e244..db0e58b1e8 100755
--- a/t/t9350-fast-export.sh
+++ b/t/t9350-fast-export.sh
@@ -253,6 +253,18 @@ test_expect_success 'signed-tags=verbatim' '
 
 '
 
+test_expect_success 'signed-tags=warn' '
+	git fast-export --signed-tags=warn sign-your-name >output 2>err &&
+	grep PGP output &&
+	test -s err
+'
+
+test_expect_success 'signed-tags=warn-verbatim' '
+	git fast-export --signed-tags=warn sign-your-name >output 2>err &&
+	grep PGP output &&
+	test -s err
+'
+
 test_expect_success 'signed-tags=strip' '
 
 	git fast-export --signed-tags=strip sign-your-name > output &&
-- 
2.31.1

[PATCH 3/3] fast-export, fast-import: implement signed-commits

[Luke Shumaker]

From: Luke Shumaker <lukeshu@datawire.io>

fast-export has an existing --signed-tags= flag that controls how to
handle tag signatures.  However, there is no equivalent for commit
signatures; it just silently strips the signature out of the commit
(analogously to --signed-tags=strip).

While signatures are generally problematic for fast-export/fast-import
(because hashes are likely to change), if they're going to support tag
signatures, there's no reason to not also support commit signatures.

So, implement signed-commits.

On the fast-export side, try to be as much like signed-tags as possible,
in both implementation and in user-interface; with the exception that
the default should be `--signed-commits=strip` (compared to the default
`--signed-tags=abort`), in order to continue defaulting to the
historical behavior.  Only bother implementing "gpgsig", not
"gpgsig-sha256"; the existing signed-tag support doesn't implement
"gpgsig-sha256" either.

On the fast-import side, I'm not entirely sure that I got the ordering
correct between "gpgsig" and "encoding" when generating the commit
object.

Signed-off-by: Luke Shumaker <lukeshu@datawire.io>
---
 Documentation/git-fast-export.txt | 12 +++++
 Documentation/git-fast-import.txt |  7 +++
 builtin/fast-export.c             | 86 +++++++++++++++++++++++++------
 builtin/fast-import.c             | 15 ++++++
 t/t9350-fast-export.sh            | 70 +++++++++++++++++++++++++
 5 files changed, 174 insertions(+), 16 deletions(-)

diff --git a/Documentation/git-fast-export.txt b/Documentation/git-fast-export.txt
index d4a2bfe037..6fdb678b54 100644
--- a/Documentation/git-fast-export.txt
+++ b/Documentation/git-fast-export.txt
@@ -39,6 +39,18 @@ warning will be displayed, with 'verbatim', they will be silently
 exported and with 'warn-verbatim', they will be exported, but you will
 see a warning.
 
+--signed-commits=(verbatim|warn-verbatim|warn-strip|strip|abort)::
+	Specify how to handle signed commits.  Since any transformation
+	after the export can change the commit (which can also happen
+	when excluding revisions) the signatures will not match.
++
+When asking to 'abort', this program will die when encountering a
+signed commit.  With 'strip' (which is the default), the commits 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-verbatim', 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.
 	Since revisions and files to export can be limited by path,
diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
index 458af0a2d6..3d0c5dbf7d 100644
--- a/Documentation/git-fast-import.txt
+++ b/Documentation/git-fast-import.txt
@@ -437,6 +437,7 @@ change to the project.
 	original-oid?
 	('author' (SP <name>)? SP LT <email> GT SP <when> LF)?
 	'committer' (SP <name>)? SP LT <email> GT SP <when> LF
+	('gpgsig' LF data)?
 	('encoding' SP <encoding> LF)?
 	data
 	('from' SP <commit-ish> LF)?
@@ -505,6 +506,12 @@ that was selected by the --date-format=<fmt> command-line option.
 See ``Date Formats'' above for the set of supported formats, and
 their syntax.
 
+`gpgsig`
+^^^^^^^^
+
+The optional `gpgsig` command is used to include a PGP/GPG signature
+that signs the commit data.
+
 `encoding`
 ^^^^^^^^^^
 The optional `encoding` command indicates the encoding of the commit
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index d121dd2ee6..d48adbc9b9 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -30,8 +30,11 @@ static const char *fast_export_usage[] = {
 	NULL
 };
 
+enum sign_mode { SIGN_ABORT, SIGN_VERBATIM, SIGN_STRIP, SIGN_VERBATIM_WARN, SIGN_STRIP_WARN };
+
 static int progress;
-static enum { SIGNED_TAG_ABORT, VERBATIM, WARN, WARN_STRIP, STRIP } signed_tag_mode = SIGNED_TAG_ABORT;
+static enum sign_mode signed_tag_mode = SIGN_ABORT;
+static enum sign_mode signed_commit_mode = SIGN_STRIP;
 static enum { TAG_FILTERING_ABORT, DROP, REWRITE } tag_of_filtered_mode = TAG_FILTERING_ABORT;
 static enum { REENCODE_ABORT, REENCODE_YES, REENCODE_NO } reencode_mode = REENCODE_ABORT;
 static int fake_missing_tagger;
@@ -48,21 +51,24 @@ static int anonymize;
 static struct hashmap anonymized_seeds;
 static struct revision_sources revision_sources;
 
-static int parse_opt_signed_tag_mode(const struct option *opt,
+static int parse_opt_sign_mode(const struct option *opt,
 				     const char *arg, int unset)
 {
-	if (unset || !strcmp(arg, "abort"))
-		signed_tag_mode = SIGNED_TAG_ABORT;
+	enum sign_mode *valptr = opt->value;
+	if (unset)
+		return 0;
+	else if (!strcmp(arg, "abort"))
+		*valptr = SIGN_ABORT;
 	else if (!strcmp(arg, "verbatim") || !strcmp(arg, "ignore"))
-		signed_tag_mode = VERBATIM;
+		*valptr = SIGN_VERBATIM;
 	else if (!strcmp(arg, "warn-verbatim") || !strcmp(arg, "warn"))
-		signed_tag_mode = WARN;
+		*valptr = SIGN_VERBATIM_WARN;
 	else if (!strcmp(arg, "warn-strip"))
-		signed_tag_mode = WARN_STRIP;
+		*valptr = SIGN_STRIP_WARN;
 	else if (!strcmp(arg, "strip"))
-		signed_tag_mode = STRIP;
+		*valptr = SIGN_STRIP;
 	else
-		return error("Unknown signed-tags mode: %s", arg);
+		return error("Unknown %s mode: %s", opt->long_name, arg);
 	return 0;
 }
 
@@ -499,6 +505,28 @@ static void show_filemodify(struct diff_queue_struct *q,
 	}
 }
 
+static const char *find_signature(const char *begin, const char *end)
+{
+	const char *needle = "\ngpgsig ";
+	char *bod, *eod, *eol;
+
+	bod = memmem(begin, end ? end - begin : strlen(begin),
+		     needle, strlen(needle));
+	if (!bod)
+		return NULL;
+	bod += strlen(needle);
+	eod = strchrnul(bod, '\n');
+	while (eod[0] == '\n' && eod[1] == ' ') {
+		eod = strchrnul(eod+1, '\n');
+	}
+	*eod = '\0';
+
+	while ((eol = strstr(bod, "\n ")))
+		memmove(eol+1, eol+2, strlen(eol+1));
+
+	return bod;
+}
+
 static const char *find_encoding(const char *begin, const char *end)
 {
 	const char *needle = "\nencoding ";
@@ -621,7 +649,7 @@ static void handle_commit(struct commit *commit, struct rev_info *rev,
 	int saved_output_format = rev->diffopt.output_format;
 	const char *commit_buffer;
 	const char *author, *author_end, *committer, *committer_end;
-	const char *encoding, *message;
+	const char *encoding, *signature, *message;
 	char *reencoded = NULL;
 	struct commit_list *p;
 	const char *refname;
@@ -644,6 +672,7 @@ static void handle_commit(struct commit *commit, struct rev_info *rev,
 	committer++;
 	committer_end = strchrnul(committer, '\n');
 	message = strstr(committer_end, "\n\n");
+	signature = find_signature(committer_end, message);
 	encoding = find_encoding(committer_end, message);
 	if (message)
 		message += 2;
@@ -703,6 +732,28 @@ static void handle_commit(struct commit *commit, struct rev_info *rev,
 	printf("%.*s\n%.*s\n",
 	       (int)(author_end - author), author,
 	       (int)(committer_end - committer), committer);
+	if (signature)
+		switch(signed_commit_mode) {
+		case SIGN_ABORT:
+			die("encountered signed commit %s; use "
+			    "--signed-commits=<mode> to handle it",
+			    oid_to_hex(&commit->object.oid));
+		case SIGN_VERBATIM_WARN:
+			warning("exporting signed commit %s",
+				oid_to_hex(&commit->object.oid));
+			/* fallthru */
+		case SIGN_VERBATIM:
+			printf("gpgsig\ndata %u\n%s",
+			       (unsigned)strlen(signature),
+			       signature);
+			break;
+		case SIGN_STRIP_WARN:
+			warning("stripping signature from commit %s",
+			       oid_to_hex(&commit->object.oid));
+			/* fallthru */
+		case SIGN_STRIP:
+			break;
+		}
 	if (!reencoded && encoding)
 		printf("encoding %s\n", encoding);
 	printf("data %u\n%s",
@@ -830,21 +881,21 @@ static void handle_tag(const char *name, struct tag *tag)
 					       "\n-----BEGIN PGP SIGNATURE-----\n");
 		if (signature)
 			switch(signed_tag_mode) {
-			case SIGNED_TAG_ABORT:
+			case SIGN_ABORT:
 				die("encountered signed tag %s; use "
 				    "--signed-tags=<mode> to handle it",
 				    oid_to_hex(&tag->object.oid));
-			case WARN:
+			case SIGN_VERBATIM_WARN:
 				warning("exporting signed tag %s",
 					oid_to_hex(&tag->object.oid));
 				/* fallthru */
-			case VERBATIM:
+			case SIGN_VERBATIM:
 				break;
-			case WARN_STRIP:
+			case SIGN_STRIP_WARN:
 				warning("stripping signature from tag %s",
 					oid_to_hex(&tag->object.oid));
 				/* fallthru */
-			case STRIP:
+			case SIGN_STRIP:
 				message_size = signature + 1 - message;
 				break;
 			}
@@ -1197,7 +1248,10 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)
 			    N_("show progress after <n> objects")),
 		OPT_CALLBACK(0, "signed-tags", &signed_tag_mode, N_("mode"),
 			     N_("select handling of signed tags"),
-			     parse_opt_signed_tag_mode),
+			     parse_opt_sign_mode),
+		OPT_CALLBACK(0, "signed-commits", &signed_commit_mode, N_("mode"),
+			     N_("select handling of signed commits"),
+			     parse_opt_sign_mode),
 		OPT_CALLBACK(0, "tag-of-filtered-object", &tag_of_filtered_mode, N_("mode"),
 			     N_("select handling of tags that tag filtered objects"),
 			     parse_opt_tag_of_filtered_mode),
diff --git a/builtin/fast-import.c b/builtin/fast-import.c
index 3afa81cf9a..74d08e09fd 100644
--- a/builtin/fast-import.c
+++ b/builtin/fast-import.c
@@ -2669,7 +2669,9 @@ static struct hash_list *parse_merge(unsigned int *count)
 
 static void parse_new_commit(const char *arg)
 {
+	static struct strbuf sig = STRBUF_INIT;
 	static struct strbuf msg = STRBUF_INIT;
+	struct string_list siglines = STRING_LIST_INIT_NODUP;
 	struct branch *b;
 	char *author = NULL;
 	char *committer = NULL;
@@ -2696,6 +2698,12 @@ static void parse_new_commit(const char *arg)
 	}
 	if (!committer)
 		die("Expected committer but didn't get one");
+	if (!strcmp(command_buf.buf, "gpgsig")) {
+		read_next_command();
+		parse_data(&sig, 0, NULL);
+		read_next_command();
+	} else
+		strbuf_setlen(&sig, 0);
 	if (skip_prefix(command_buf.buf, "encoding ", &v)) {
 		encoding = xstrdup(v);
 		read_next_command();
@@ -2769,8 +2777,15 @@ static void parse_new_commit(const char *arg)
 		strbuf_addf(&new_data,
 			"encoding %s\n",
 			encoding);
+	if (sig.len) {
+		strbuf_addstr(&new_data, "gpgsig ");
+		string_list_split_in_place(&siglines, sig.buf, '\n', -1);
+		strbuf_add_separated_string_list(&new_data, "\n ", &siglines);
+		strbuf_addch(&new_data, '\n');
+	}
 	strbuf_addch(&new_data, '\n');
 	strbuf_addbuf(&new_data, &msg);
+	string_list_clear(&siglines, 1);
 	free(author);
 	free(committer);
 	free(encoding);
diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh
index db0e58b1e8..49a2827be2 100755
--- a/t/t9350-fast-export.sh
+++ b/t/t9350-fast-export.sh
@@ -8,6 +8,7 @@ GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
 . ./test-lib.sh
+. "$TEST_DIRECTORY/lib-gpg.sh"
 
 test_expect_success 'setup' '
 
@@ -278,9 +279,78 @@ test_expect_success 'signed-tags=warn-strip' '
 	test -s err
 '
 
+test_expect_success GPG 'set up signed commit' '
+
+	# Generate a commit with both "gpgsig" and "encoding" set, so
+	# that we can test that fast-import gets the ordering correct
+	# between the two.
+	test_config i18n.commitEncoding ISO-8859-1 &&
+	git checkout -f -b commit-signing main &&
+	echo Sign your name > file-sign &&
+	git add file-sign &&
+	git commit -S -m "signed commit" &&
+	COMMIT_SIGNING=$(git rev-parse --verify commit-signing)
+
+'
+
+test_expect_success GPG 'signed-commits=abort' '
+
+	test_must_fail git fast-export --signed-commits=abort commit-signing
+
+'
+
+test_expect_success GPG 'signed-commits=verbatim' '
+
+	git fast-export --signed-commits=verbatim --reencode=no commit-signing >output &&
+	grep ^gpgsig output &&
+	grep "encoding ISO-8859-1" output &&
+	(cd new &&
+	 git fast-import &&
+	 test $COMMIT_SIGNING = $(git rev-parse --verify refs/heads/commit-signing)) <output
+
+'
+
+test_expect_success GPG 'signed-commits=warn-verbatim' '
+
+	git fast-export --signed-commits=warn-verbatim --reencode=no commit-signing >output 2>err &&
+	grep ^gpgsig output &&
+	grep "encoding ISO-8859-1" output &&
+	test -s err &&
+	(cd new &&
+	 git fast-import &&
+	 test $COMMIT_SIGNING = $(git rev-parse --verify refs/heads/commit-signing)) <output
+
+'
+
+test_expect_success GPG 'signed-commits=strip' '
+
+	git fast-export --signed-commits=strip --reencode=no commit-signing >output &&
+	! grep ^gpgsig output &&
+	grep "^encoding ISO-8859-1" output &&
+	sed "s/commit-signing/commit-strip-signing/" output |
+		(cd new &&
+		 git fast-import &&
+		 test $COMMIT_SIGNING != $(git rev-parse --verify refs/heads/commit-strip-signing))
+
+'
+
+test_expect_success GPG 'signed-commits=warn-strip' '
+
+	git fast-export --signed-commits=warn-strip --reencode=no commit-signing >output 2>err &&
+	! grep ^gpgsig output &&
+	grep "^encoding ISO-8859-1" output &&
+	test -s err &&
+	sed "s/commit-signing/commit-strip-signing/" output |
+		(cd new &&
+		 git fast-import &&
+		 test $COMMIT_SIGNING != $(git rev-parse --verify refs/heads/commit-strip-signing))
+
+'
+
 test_expect_success 'setup submodule' '
 
 	git checkout -f main &&
+	{ git update-ref -d refs/heads/commit-signing || true; } &&
 	mkdir sub &&
 	(
 		cd sub &&
-- 
2.31.1

Re: [PATCH 2/3] fast-export: rename --signed-tags='warn' to 'warn-verbatim'

[Taylor Blau]

On Mon, Apr 19, 2021 at 04:54:40PM -0600, Luke Shumaker wrote:
> From: Luke Shumaker <lukeshu@datawire.io>
>
> But still keep --signed-tags=warn as an undocumented alias.  This name
> is clearer as it has symmetry with warn-strip:
>
>                    action              ->                   action
>        +----------------------------+  ->       +----------------------------+
>   msg? |      verbatim |      strip |  ->  msg? |      verbatim |      strip |
>        |          warn | warn-strip |  ->       | warn-verbatim | warn-strip |
>        +----------------------------+  ->       +----------------------------+

This table is rather confusing to me. What's unclear to me is what
"msg?" and "action" are referring to. After reading your patch, I think
it may be clearer to say:

    The --signed-tags option takes one of five arguments specifying how
    to handle singed tags during export. Among these arguments, strip is
    to warn-strip as verbatim is to warn. (The unmentioned argument is
    'abort', which stops the fast-export process entirely). That is,
    signatures are either stripped or copied verbatim while exporting,
    with or without a warning.

    Make clear that the "warn" option instructs fast-export to copy
    signatures verbatim by matching the pattern (and calling the option
    "warn-verbatim").

    To maintain backwards compatibility, "warn" is still recognized as
    an undocumented alias.

> +test_expect_success 'signed-tags=warn' '
> +	git fast-export --signed-tags=warn sign-your-name >output 2>err &&
> +	grep PGP output &&
> +	test -s err
> +'
> +
> +test_expect_success 'signed-tags=warn-verbatim' '
> +	git fast-export --signed-tags=warn sign-your-name >output 2>err &&

s/warn/warn-verbatim ?

Thanks,
Taylor

Re: [PATCH 3/3] fast-export, fast-import: implement signed-commits

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 2752 bytes --]

On 2021-04-19 at 22:54:41, Luke Shumaker wrote:
> From: Luke Shumaker <lukeshu@datawire.io>
> 
> fast-export has an existing --signed-tags= flag that controls how to
> handle tag signatures.  However, there is no equivalent for commit
> signatures; it just silently strips the signature out of the commit
> (analogously to --signed-tags=strip).
> 
> While signatures are generally problematic for fast-export/fast-import
> (because hashes are likely to change), if they're going to support tag
> signatures, there's no reason to not also support commit signatures.
> 
> So, implement signed-commits.
> 
> On the fast-export side, try to be as much like signed-tags as possible,
> in both implementation and in user-interface; with the exception that
> the default should be `--signed-commits=strip` (compared to the default
> `--signed-tags=abort`), in order to continue defaulting to the
> historical behavior.  Only bother implementing "gpgsig", not
> "gpgsig-sha256"; the existing signed-tag support doesn't implement
> "gpgsig-sha256" either.

I would appreciate it if we did in fact implement it.  I would like to
use this functionality to round-trip objects between SHA-1 and SHA-256,
and it would be nice if both worked.

The situation with tags is different: the signature using the current
algorithm is always trailing, and the signature for the other algorithm
is in the header.  That wasn't how we intended it to be, but that's how
it ended up being.

As a result, tag output can support SHA-256 data, but with your
proposal, SHA-256 commits wouldn't work at all.  Considering SHA-1 is
wildly insecure and therefore signing SHA-1 commits adds very little
security, whereas SHA-256 is presently considered strong, I'd argue that
only supporting SHA-1 isn't the right move here.

Provided we do that and the test suite passes under both algorithms, I'm
strongly in favor of this feature.  In fact, I had been thinking about
implementing this feature myself just the other day, so I'm delighted
you decided to do it.

> diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
> index 458af0a2d6..3d0c5dbf7d 100644
> --- a/Documentation/git-fast-import.txt
> +++ b/Documentation/git-fast-import.txt
> @@ -437,6 +437,7 @@ change to the project.
>  	original-oid?
>  	('author' (SP <name>)? SP LT <email> GT SP <when> LF)?
>  	'committer' (SP <name>)? SP LT <email> GT SP <when> LF
> +	('gpgsig' LF data)?

Could we emit this as "gpgsig sha1 data" and "gpgsig sha256 data"?  That
would allow us to consider the future case where the hash algorithm
changes again without requiring a change of format.
-- 
brian m. carlson (he/him or they/them)
Houston, Texas, US

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [PATCH 3/3] fast-export, fast-import: implement signed-commits

[Taylor Blau]

On Mon, Apr 19, 2021 at 04:54:41PM -0600, Luke Shumaker wrote:
> From: Luke Shumaker <lukeshu@datawire.io>
>
> fast-export has an existing --signed-tags= flag that controls how to
> handle tag signatures.  However, there is no equivalent for commit
> signatures; it just silently strips the signature out of the commit
> (analogously to --signed-tags=strip).
>
> While signatures are generally problematic for fast-export/fast-import
> (because hashes are likely to change), if they're going to support tag
> signatures, there's no reason to not also support commit signatures.
>
> So, implement signed-commits.
>
> On the fast-export side, try to be as much like signed-tags as possible,
> in both implementation and in user-interface; with the exception that
> the default should be `--signed-commits=strip` (compared to the default
> `--signed-tags=abort`), in order to continue defaulting to the
> historical behavior.  Only bother implementing "gpgsig", not
> "gpgsig-sha256"; the existing signed-tag support doesn't implement
> "gpgsig-sha256" either.
>
> On the fast-import side, I'm not entirely sure that I got the ordering
> correct between "gpgsig" and "encoding" when generating the commit
> object.
>
> Signed-off-by: Luke Shumaker <lukeshu@datawire.io>
> ---
>  Documentation/git-fast-export.txt | 12 +++++
>  Documentation/git-fast-import.txt |  7 +++
>  builtin/fast-export.c             | 86 +++++++++++++++++++++++++------
>  builtin/fast-import.c             | 15 ++++++
>  t/t9350-fast-export.sh            | 70 +++++++++++++++++++++++++
>  5 files changed, 174 insertions(+), 16 deletions(-)
>
> diff --git a/Documentation/git-fast-export.txt b/Documentation/git-fast-export.txt
> index d4a2bfe037..6fdb678b54 100644
> --- a/Documentation/git-fast-export.txt
> +++ b/Documentation/git-fast-export.txt
> @@ -39,6 +39,18 @@ warning will be displayed, with 'verbatim', they will be silently
>  exported and with 'warn-verbatim', they will be exported, but you will
>  see a warning.
>
> +--signed-commits=(verbatim|warn-verbatim|warn-strip|strip|abort)::
> +	Specify how to handle signed commits.  Since any transformation
> +	after the export can change the commit (which can also happen
> +	when excluding revisions) the signatures will not match.
> ++
> +When asking to 'abort', this program will die when encountering a
> +signed commit.  With 'strip' (which is the default), the commits 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-verbatim', they will be exported,
> +but you will see a warning.
> +

OK, this all seems normal to me. But it may be worth shortening it to
say "behaves exactly as --signed-tags, but for commits", or something.

>  --tag-of-filtered-object=(abort|drop|rewrite)::
>  	Specify how to handle tags whose tagged object is filtered out.
>  	Since revisions and files to export can be limited by path,
> diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
> index 458af0a2d6..3d0c5dbf7d 100644
> --- a/Documentation/git-fast-import.txt
> +++ b/Documentation/git-fast-import.txt
> @@ -437,6 +437,7 @@ change to the project.
>  	original-oid?
>  	('author' (SP <name>)? SP LT <email> GT SP <when> LF)?
>  	'committer' (SP <name>)? SP LT <email> GT SP <when> LF
> +	('gpgsig' LF data)?

Is this missing a LF after data?

> +static const char *find_signature(const char *begin, const char *end)
> +{
> +	const char *needle = "\ngpgsig ";
> +	char *bod, *eod, *eol;
> +
> +	bod = memmem(begin, end ? end - begin : strlen(begin),
> +		     needle, strlen(needle));
> +	if (!bod)
> +		return NULL;
> +	bod += strlen(needle);
> +	eod = strchrnul(bod, '\n');
> +	while (eod[0] == '\n' && eod[1] == ' ') {
> +		eod = strchrnul(eod+1, '\n');
> +	}
> +	*eod = '\0';
> +
> +	while ((eol = strstr(bod, "\n ")))
> +		memmove(eol+1, eol+2, strlen(eol+1));

Hmm. I'm not quite sure I follow these last two lines. Perhaps a comment
would help? The rest of this patch looks reasonable to me.

Thanks,
Taylor

Re: [PATCH 2/3] fast-export: rename --signed-tags='warn' to 'warn-verbatim'

[Luke Shumaker]

On Mon, 19 Apr 2021 18:27:35 -0600,
Taylor Blau wrote:
> 
> On Mon, Apr 19, 2021 at 04:54:40PM -0600, Luke Shumaker wrote:
> > From: Luke Shumaker <lukeshu@datawire.io>
> >
> > But still keep --signed-tags=warn as an undocumented alias.  This name
> > is clearer as it has symmetry with warn-strip:
> >
> >                    action              ->                   action
> >        +----------------------------+  ->       +----------------------------+
> >   msg? |      verbatim |      strip |  ->  msg? |      verbatim |      strip |
> >        |          warn | warn-strip |  ->       | warn-verbatim | warn-strip |
> >        +----------------------------+  ->       +----------------------------+
> 
> This table is rather confusing to me. What's unclear to me is what
> "msg?" and "action" are referring to. After reading your patch, I think
> it may be clearer to say:
> 
>     The --signed-tags option takes one of five arguments specifying how
>     to handle singed tags during export. Among these arguments, strip is
>     to warn-strip as verbatim is to warn. (The unmentioned argument is
>     'abort', which stops the fast-export process entirely). That is,
>     signatures are either stripped or copied verbatim while exporting,
>     with or without a warning.
> 
>     Make clear that the "warn" option instructs fast-export to copy
>     signatures verbatim by matching the pattern (and calling the option
>     "warn-verbatim").
> 
>     To maintain backwards compatibility, "warn" is still recognized as
>     an undocumented alias.

Thank you, I'll take much of this wording.

> > +test_expect_success 'signed-tags=warn' '
> > +	git fast-export --signed-tags=warn sign-your-name >output 2>err &&
> > +	grep PGP output &&
> > +	test -s err
> > +'
> > +
> > +test_expect_success 'signed-tags=warn-verbatim' '
> > +	git fast-export --signed-tags=warn sign-your-name >output 2>err &&
> 
> s/warn/warn-verbatim ?

Indeed, oops!

I'll also add a comment clarifying that the signed-tags=warn test is
testing for backward compatibility.

-- 
Happy hacking,
~ Luke Shumaker

Re: [PATCH 3/3] fast-export, fast-import: implement signed-commits

[Luke Shumaker]

On Mon, 19 Apr 2021 16:54:41 -0600,
Luke Shumaker wrote:
> On the fast-import side, I'm not entirely sure that I got the ordering
> correct between "gpgsig" and "encoding" when generating the commit
> object.

Whoops, I forgot to take that out of the commit message; I did check
the ordering before submitting the patch.

-- 
Happy hacking,
~ Luke Shumaker

Re: [PATCH 3/3] fast-export, fast-import: implement signed-commits

[Luke Shumaker]

On Mon, 19 Apr 2021 19:45:46 -0600,
Taylor Blau wrote:
> > diff --git a/Documentation/git-fast-export.txt b/Documentation/git-fast-export.txt
> > index d4a2bfe037..6fdb678b54 100644
> > --- a/Documentation/git-fast-export.txt
> > +++ b/Documentation/git-fast-export.txt
> > @@ -39,6 +39,18 @@ warning will be displayed, with 'verbatim', they will be silently
> >  exported and with 'warn-verbatim', they will be exported, but you will
> >  see a warning.
> >
> > +--signed-commits=(verbatim|warn-verbatim|warn-strip|strip|abort)::
> > +	Specify how to handle signed commits.  Since any transformation
> > +	after the export can change the commit (which can also happen
> > +	when excluding revisions) the signatures will not match.
> > ++
> > +When asking to 'abort', this program will die when encountering a
> > +signed commit.  With 'strip' (which is the default), the commits 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-verbatim', they will be exported,
> > +but you will see a warning.
> > +
> 
> OK, this all seems normal to me. But it may be worth shortening it to
> say "behaves exactly as --signed-tags, but for commits", or something.

Good suggestion, it would also make it more obvious that the default
is different, since I'd have to call it out explictly.

> >  --tag-of-filtered-object=(abort|drop|rewrite)::
> >  	Specify how to handle tags whose tagged object is filtered out.
> >  	Since revisions and files to export can be limited by path,
> > diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
> > index 458af0a2d6..3d0c5dbf7d 100644
> > --- a/Documentation/git-fast-import.txt
> > +++ b/Documentation/git-fast-import.txt
> > @@ -437,6 +437,7 @@ change to the project.
> >  	original-oid?
> >  	('author' (SP <name>)? SP LT <email> GT SP <when> LF)?
> >  	'committer' (SP <name>)? SP LT <email> GT SP <when> LF
> > +	('gpgsig' LF data)?
> 
> Is this missing a LF after data?

No, the definition of `data` has a byte-count prefix, so it doesn't
need an `LF` to act as a terminator (and it also already includes an
optional trailing `LF?` just in case you want to include one).

In fact, my implementation in fast-export does not include the LF,
which is why the test greps for "encoding ISO-8859-1" instead of
"^encoding ISO-8859-1".

I'll add a comment saying that it's intentional.

> > +static const char *find_signature(const char *begin, const char *end)
> > +{
> > +	const char *needle = "\ngpgsig ";
> > +	char *bod, *eod, *eol;
> > +
> > +	bod = memmem(begin, end ? end - begin : strlen(begin),
> > +		     needle, strlen(needle));
> > +	if (!bod)
> > +		return NULL;
> > +	bod += strlen(needle);
> > +	eod = strchrnul(bod, '\n');
> > +	while (eod[0] == '\n' && eod[1] == ' ') {
> > +		eod = strchrnul(eod+1, '\n');
> > +	}
> > +	*eod = '\0';
> > +
> > +	while ((eol = strstr(bod, "\n ")))
> > +		memmove(eol+1, eol+2, strlen(eol+1));
> 
> Hmm. I'm not quite sure I follow these last two lines. Perhaps a comment
> would help? The rest of this patch looks reasonable to me.

In the commit object, multi-line header values are stored by prefixing
continuation lines begin with a space.  So within the commit object,
it looks like

    "gpgsig -----BEGIN PGP SIGNATURE-----\n"
    " Version: GnuPG v1.4.5 (GNU/Linux)\n"
    " \n"
    " base64_pem_here\n"
    " -----END PGP SIGNATURE-----\n"

However, we want the raw value; we want to return

    "-----BEGIN PGP SIGNATURE-----\n"
    "Version: GnuPG v1.4.5 (GNU/Linux)\n"
    "\n"
    "base64_pem_here\n"
    "-----END PGP SIGNATURE-----\n"

without all the extra spaces.  That's what those two lines are doing,
stripping out the extra spaces.

I'll add some comments.

-- 
Happy hacking,
~ Luke Shumaker

Re: [PATCH 3/3] fast-export, fast-import: implement signed-commits

[Luke Shumaker]

On Mon, 19 Apr 2021 19:41:55 -0600,
brian m. carlson wrote:
> 
> [1  <text/plain; utf-8 (quoted-printable)>]
> On 2021-04-19 at 22:54:41, Luke Shumaker wrote:
> > From: Luke Shumaker <lukeshu@datawire.io>
> > 
> > fast-export has an existing --signed-tags= flag that controls how to
> > handle tag signatures.  However, there is no equivalent for commit
> > signatures; it just silently strips the signature out of the commit
> > (analogously to --signed-tags=strip).
> > 
> > While signatures are generally problematic for fast-export/fast-import
> > (because hashes are likely to change), if they're going to support tag
> > signatures, there's no reason to not also support commit signatures.
> > 
> > So, implement signed-commits.
> > 
> > On the fast-export side, try to be as much like signed-tags as possible,
> > in both implementation and in user-interface; with the exception that
> > the default should be `--signed-commits=strip` (compared to the default
> > `--signed-tags=abort`), in order to continue defaulting to the
> > historical behavior.  Only bother implementing "gpgsig", not
> > "gpgsig-sha256"; the existing signed-tag support doesn't implement
> > "gpgsig-sha256" either.
> 
> I would appreciate it if we did in fact implement it.  I would like to
> use this functionality to round-trip objects between SHA-1 and SHA-256,
> and it would be nice if both worked.
> 
> The situation with tags is different: the signature using the current
> algorithm is always trailing, and the signature for the other algorithm
> is in the header.  That wasn't how we intended it to be, but that's how
> it ended up being.
> 
> As a result, tag output can support SHA-256 data,

I don't believe that's true?  With SHA-1-signed tags, the signature
gets included in the fast-import stream as part of the tag message
(the `data` line in the BNF).  Since SHA-256-signed tags have their
signature as a header (rather than just appending it to the message),
we'd have to add a 'gpgsig' sub-command to the 'tag' top-level-command
(like I've done to the 'commit' top-level-command).

>                                                   but with your
> proposal, SHA-256 commits wouldn't work at all.  Considering SHA-1 is
> wildly insecure and therefore signing SHA-1 commits adds very little
> security, whereas SHA-256 is presently considered strong, I'd argue that
> only supporting SHA-1 isn't the right move here.

The main reason I didn't implement SHA-256 support (well, besides that
the repo I'm working on turned out to not have any SHA-256-signed
commits in it) is that I had questions about SHA-256 that I didn't
know/couldn't find the answers to.

However, looking again, I see a few of the answers in
t7510-signed-commit.sh, so I'll have a go at it.  If I get stuck, I'll
go ahead and implement the below "gpgsig sha1" suggestion, and leave
the sha256 implementation to someone else.

> Provided we do that and the test suite passes under both algorithms, I'm
> strongly in favor of this feature.  In fact, I had been thinking about
> implementing this feature myself just the other day, so I'm delighted
> you decided to do it.

That's one of the big reasons I didn't implement both--I wasn't sure
how to test sha256 (within the test harness, `git commit -S` gives a
sha1 signature).

I see that t7510-signed-commit.sh 'verify-commit verifies multiply
signed commits' tests sha256 by hard-coding a raw commit object in the
test itself, and feeding that to `git hash-object`.  I'd prefer to
figure out how to get `git commit` itself to generate a sha256
signature rather than a sha1 signature, so that I can _know_ that I'm
getting the ordering of headers the same as `git commit`.  But I don't
think that needs to be a blocker; if the test doesn't do the same
ordering as `git commit`, I guess that can just be a bugfix later?

> > diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
> > index 458af0a2d6..3d0c5dbf7d 100644
> > --- a/Documentation/git-fast-import.txt
> > +++ b/Documentation/git-fast-import.txt
> > @@ -437,6 +437,7 @@ change to the project.
> >  	original-oid?
> >  	('author' (SP <name>)? SP LT <email> GT SP <when> LF)?
> >  	'committer' (SP <name>)? SP LT <email> GT SP <when> LF
> > +	('gpgsig' LF data)?
> 
> Could we emit this as "gpgsig sha1 data" and "gpgsig sha256 data"?  That
> would allow us to consider the future case where the hash algorithm
> changes again without requiring a change of format.

I like that idea.  I'll implement it.

FWIW, I thought about instead adding a fast-import command to insert
arbitrary headers in to the commit object, rather than having to add a
new command for every new header we want to be able to round-trip.
But it's like, if we're exposing that much of the low-levels of a
commit object, why are we keeping up the facade fast-import stream at
all, instead of streaming raw Git objects around?

-- 
Happy hacking,
~ Luke Shumaker

Re: [PATCH 3/3] fast-export, fast-import: implement signed-commits

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 4722 bytes --]

On 2021-04-20 at 17:15:25, Luke Shumaker wrote:
> On Mon, 19 Apr 2021 19:41:55 -0600,
> brian m. carlson wrote:
> > I would appreciate it if we did in fact implement it.  I would like to
> > use this functionality to round-trip objects between SHA-1 and SHA-256,
> > and it would be nice if both worked.
> > 
> > The situation with tags is different: the signature using the current
> > algorithm is always trailing, and the signature for the other algorithm
> > is in the header.  That wasn't how we intended it to be, but that's how
> > it ended up being.
> > 
> > As a result, tag output can support SHA-256 data,
> 
> I don't believe that's true?  With SHA-1-signed tags, the signature
> gets included in the fast-import stream as part of the tag message
> (the `data` line in the BNF).  Since SHA-256-signed tags have their
> signature as a header (rather than just appending it to the message),
> we'd have to add a 'gpgsig' sub-command to the 'tag' top-level-command
> (like I've done to the 'commit' top-level-command).

If you're using a repository that's SHA-1, then the tag signature that's
part of the message is a signature over the SHA-1 contents of the
object, and the gpgsig-sha256 header is a signature over the SHA-256
contents of the object.  If you're using a repository that's SHA-256,
it's reversed: the signature at the end of the message covers the
SHA-256 contents of the object and the gpgsig header covers the SHA-1
contents.

It isn't currently possible to create objects with both signatures in
place, but that will be possible in the future.

> >                                                   but with your
> > proposal, SHA-256 commits wouldn't work at all.  Considering SHA-1 is
> > wildly insecure and therefore signing SHA-1 commits adds very little
> > security, whereas SHA-256 is presently considered strong, I'd argue that
> > only supporting SHA-1 isn't the right move here.
> 
> The main reason I didn't implement SHA-256 support (well, besides that
> the repo I'm working on turned out to not have any SHA-256-signed
> commits in it) is that I had questions about SHA-256 that I didn't
> know/couldn't find the answers to.

Currently, repositories using SHA-256 currently don't interoperate with
SHA-1 repositories.  However, if you want to create a test repo, you can
do so with "git init --object-format=sha256" in an empty directory.

If you want to run the testsuite in SHA-256 mode, set
GIT_TEST_DEFAULT_HASH=sha256, and all the repositories created will use
SHA-256.

That should be sufficient to get this series such that it will work with
simple SHA-256 repos.  If you have more questions about this work or how
to get things working, I'm happy to answer them.

> However, looking again, I see a few of the answers in
> t7510-signed-commit.sh, so I'll have a go at it.  If I get stuck, I'll
> go ahead and implement the below "gpgsig sha1" suggestion, and leave
> the sha256 implementation to someone else.

Not implementing this means the CI will fail when the testsuite is run
in SHA-256 mode, so your patch probably won't be accepted.

> > Provided we do that and the test suite passes under both algorithms, I'm
> > strongly in favor of this feature.  In fact, I had been thinking about
> > implementing this feature myself just the other day, so I'm delighted
> > you decided to do it.
> 
> That's one of the big reasons I didn't implement both--I wasn't sure
> how to test sha256 (within the test harness, `git commit -S` gives a
> sha1 signature).
> 
> I see that t7510-signed-commit.sh 'verify-commit verifies multiply
> signed commits' tests sha256 by hard-coding a raw commit object in the
> test itself, and feeding that to `git hash-object`.  I'd prefer to
> figure out how to get `git commit` itself to generate a sha256
> signature rather than a sha1 signature, so that I can _know_ that I'm
> getting the ordering of headers the same as `git commit`.  But I don't
> think that needs to be a blocker; if the test doesn't do the same
> ordering as `git commit`, I guess that can just be a bugfix later?

Yes, dual-signed objects have to manually created right now; there's no
tooling to create them because that code hasn't landed yet.  It's in my
tree and very broken.  But you can create SHA-256 repositories as I
mentioned above and test those, and the testsuite does run in that mode,
so it should be easy enough to check at least single-signed commits for
now, even if you don't implement dual-signed ones.  I think it's fine if
that comes later, and I can pick that up as part of a future series.
-- 
brian m. carlson (he/him or they/them)
Houston, Texas, US

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [PATCH 0/3] fast-export, fast-import: implement signed-commits

[Elijah Newren]

On Mon, Apr 19, 2021 at 3:54 PM Luke Shumaker <lukeshu@lukeshu.com> wrote:
>
> From: Luke Shumaker <lukeshu@datawire.io>
>
> fast-export has an existing --signed-tags= flag that controls how to
> handle tag signatures.  However, there is no equivalent for commit
> signatures; it just silently strips the signature out of the commit
> (analogously to --signed-tags=strip).
>
> So implement a --signed-commits= flag in fast-export, and implement
> the receiving side of it in fast-import.

I understand adding an option to fast-export, but shouldn't there also
be one for fast-import?  In particular, I can see users wanting any of
the following:

* I want these signatures exported and imported because I know I won't
tweak them and they'll still be valid.
* I want these signatures even though they'll be invalid.  Whatever,
I'll just deal with it.
* I want the signatures exported and imported *when they will remain
valid*.  Always exporting them makes sense, because fast-export
doesn't know about tweaks I'll be making to its output before feeding
it to fast-import.  But fast-import should have options to
strip-if-invalid/warn-if-invalid/error-if-invalid/import-without-warning
for these tags (though they don't have to use these exact names).

I know fast-import doesn't do anything of the sort for signed tags,
but fast-import also doesn't support signed tags as per this comment
in the documentation:

"""
Signing annotated tags during import from within fast-import is not
supported.  Trying to include your own PGP/GPG signature is not
recommended, as the frontend does not (easily) have access to the
complete set of bytes which normally goes into such a signature.
If signing is required, create lightweight tags from within fast-import with
`reset`, then create the annotated versions of those tags offline
with the standard 'git tag' process.
"""

it just happens to "work" since the signature is part of the
annotation and fast-import doesn't attempt to read or validate the
annotation in any way, treating it as free-from text.  I'd say users
relying on this are on somewhat shaky ground.

But here you're adding explicit fast-import directives to the language
for signatures of commits, so you clearly do need to care.  And I
suspect fast-import's default should be error-if-invalid rather than
import-without-warning.

> Luke Shumaker (3):
>   git-fast-import.txt: add missing LF in the BNF
>   fast-export: rename --signed-tags='warn' to 'warn-verbatim'
>   fast-export, fast-import: implement signed-commits
>
>  Documentation/git-fast-export.txt | 18 +++++--
>  Documentation/git-fast-import.txt |  9 +++-
>  builtin/fast-export.c             | 88 +++++++++++++++++++++++++------
>  builtin/fast-import.c             | 15 ++++++
>  t/t9350-fast-export.sh            | 82 ++++++++++++++++++++++++++++
>  5 files changed, 191 insertions(+), 21 deletions(-)
>
> --
> 2.31.1
>
> Happy hacking,
> ~ Luke Shumaker

Re: [PATCH 0/3] fast-export, fast-import: implement signed-commits

[Luke Shumaker]

On Wed, 21 Apr 2021 12:12:40 -0600,
Elijah Newren wrote:
> 
> On Mon, Apr 19, 2021 at 3:54 PM Luke Shumaker <lukeshu@lukeshu.com> wrote:
> >
> > From: Luke Shumaker <lukeshu@datawire.io>
> >
> > fast-export has an existing --signed-tags= flag that controls how to
> > handle tag signatures.  However, there is no equivalent for commit
> > signatures; it just silently strips the signature out of the commit
> > (analogously to --signed-tags=strip).
> >
> > So implement a --signed-commits= flag in fast-export, and implement
> > the receiving side of it in fast-import.
> 
> I understand adding an option to fast-export, but shouldn't there also
> be one for fast-import?  In particular, I can see users wanting any of
> the following:
> 
> * I want these signatures exported and imported because I know I won't
> tweak them and they'll still be valid.
> * I want these signatures even though they'll be invalid.  Whatever,
> I'll just deal with it.
> * I want the signatures exported and imported *when they will remain
> valid*.  Always exporting them makes sense, because fast-export
> doesn't know about tweaks I'll be making to its output before feeding
> it to fast-import.  But fast-import should have options to
> strip-if-invalid/warn-if-invalid/error-if-invalid/import-without-warning
> for these tags (though they don't have to use these exact names).
> 
> I know fast-import doesn't do anything of the sort for signed tags,
> but fast-import also doesn't support signed tags as per this comment
> in the documentation:
> 
> """
> Signing annotated tags during import from within fast-import is not
> supported.  Trying to include your own PGP/GPG signature is not
> recommended, as the frontend does not (easily) have access to the
> complete set of bytes which normally goes into such a signature.
> If signing is required, create lightweight tags from within fast-import with
> `reset`, then create the annotated versions of those tags offline
> with the standard 'git tag' process.
> """
> 
> it just happens to "work" since the signature is part of the
> annotation and fast-import doesn't attempt to read or validate the
> annotation in any way, treating it as free-from text.  I'd say users
> relying on this are on somewhat shaky ground.
> 
> But here you're adding explicit fast-import directives to the language
> for signatures of commits, so you clearly do need to care.  And I
> suspect fast-import's default should be error-if-invalid rather than
> import-without-warning.

I agree that this would be a good and useful flag to add to
fast-import.  However, I don't think that it's a necessary thing to
add for this work, and I think that it's beyond the scope of what I am
willing to implement.

I see where you're coming from when you say that tags and commits are
different in this regard, but I don't agree.  The signed tags
situation does look like shakey ground, but IMO once fast-export
gained the --signed-tags option, that was saying "this is the correct
and stable way of encoding a tag signature in a fast-import stream".
The fact that it is wonky in that it is shoved in to the message is an
accident of implementation history, and to me doesn't say anything
about the nature of the thing.

I think that such a flag in fast-import is just as worthy of existing
for tags as it is worthy of existing for commits.

Actually, I'm not sure I think such flags belong in fast-import
itself, I think it may make good sense to have such a filter
implemented as an external tool.  Fast-export and fast-import seem to
avoid using the many parts of standard git library functions,
apparently (to me) to avoid allocations because "fast".  Given that
trouble gone to just to avoid allocations, calling out to GPG seems
prohibitively slow by comparison.  But I agree such functionality
would be good and useful, regardless of whether I think it belongs in
fast-import itself.

-- 
Happy hacking,
~ Luke Shumaker

Re: [PATCH 3/3] fast-export, fast-import: implement signed-commits

[Luke Shumaker]

On Tue, 20 Apr 2021 17:07:09 -0600,
brian m. carlson wrote:
> On 2021-04-20 at 17:15:25, Luke Shumaker wrote:
> > I don't believe that's true?  With SHA-1-signed tags, the signature
> > gets included in the fast-import stream as part of the tag message
> > (the `data` line in the BNF).  Since SHA-256-signed tags have their
> > signature as a header (rather than just appending it to the message),
> > we'd have to add a 'gpgsig' sub-command to the 'tag' top-level-command
> > (like I've done to the 'commit' top-level-command).
> 
> If you're using a repository that's SHA-1, then the tag signature that's
> part of the message is a signature over the SHA-1 contents of the
> object, and the gpgsig-sha256 header is a signature over the SHA-256
> contents of the object.  If you're using a repository that's SHA-256,
> it's reversed: the signature at the end of the message covers the
> SHA-256 contents of the object and the gpgsig header covers the SHA-1
> contents.

Good to know!  It seems I've been mislead by
Documentation/technical/hash-function-transition.txt

> Not implementing this means the CI will fail when the testsuite is run
> in SHA-256 mode, so your patch probably won't be accepted.

Gotcha.  I guess I will be implementing it then.  I'll let you know if
I have any further questions, the information you've given already has
been very helpful!

-- 
Happy hacking,
~ Luke Shumaker