threads / patch / 50032

patch, 4 partsExpose gpgsig in pretty-print

Subject: [PATCH 0/4] Expose gpgsig in pretty-print

## tl;dr

14 messages between Dec 13, 2018 and Dec 21, 2018. Diffs are folded; open one to read it.

replies: 13people: 4as markdown or json

John Passaro· Dec 13, 2018, 21:22 UTC · lore

Currently, users who do not have GPG installed have no way to discern signed from unsigned commits without examining raw commit data. I propose two new pretty-print placeholders to expose this information:

%GR: full ("R"aw) contents of gpgsig header %G+: Y/N if the commit has nonempty gpgsig header or not

The second is of course much more likely to be used, but having exposed the one, exposing the other too adds almost no complexity.

I'm open to suggestion on the names of these placeholders.

This commit is based on master but e5a329a279 ("run-command: report exec failure" 2018-12-11) is required for the tests to pass.

One note is that this change touches areas of the pretty-format documentation that are radically revamped in aw/pretty-trailers: see 42617752d4 ("doc: group pretty-format.txt placeholders descriptions" 2018-12-08). I have another version of this branch based on that branch as well, so you can use that in case conflicts with aw/pretty-trailers arise.

See:
- https://github.com/jpassaro/git/tree/jp/pretty-expose-gpgsig
- https://github.com/jpassaro/git/tree/jp/pretty-expose-gpgsig--based-on-aw-pretty-trailers
John Passaro (4):
  pretty: expose raw commit signature
  t/t7510-signed-commit.sh: test new placeholders
  doc, tests: pretty behavior when gpg missing
  docs/pretty-formats: add explanation + copy edits
 Documentation/pretty-formats.txt |  21 ++++--
 pretty.c                         |  36 ++++++++-
 t/t7510-signed-commit.sh         | 125 +++++++++++++++++++++++++++++--
 3 files changed, 167 insertions(+), 15 deletions(-)

base-commit: 5d826e972970a784bd7a7bdf587512510097b8c7 prerequisite-patch-id: aedfe228fd293714d9cd0392ac22ff1cba7365db

-- 
2.19.1
John Passaro· Dec 13, 2018, 21:22 UTC · re: John Passaro · lore

[PATCH 1/4] pretty: expose raw commit signature

Add new pretty-format placeholders %GR and %G+ to support inspecting gpgsig commit header in pretty format, even if GPG is not available.

Signed-off-by: John Passaro <john.a.passaro@gmail.com>
---
 Documentation/pretty-formats.txt |  2 ++
 pretty.c                         | 36 ++++++++++++++++++++++++++++++--
 2 files changed, 36 insertions(+), 2 deletions(-)
Show changes to 2 files +36 −2

Documentation/pretty-formats.txt, pretty.c

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 417b638cd8..582454a4f7 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -142,6 +142,8 @@ The placeholders are:
 ifndef::git-rev-list[]
 - '%N': commit notes
 endif::git-rev-list[]
+- '%GR': contents of the commits signature (blank when unsigned)
+- '%G+': "Y" if the commit is signed, "N" otherwise
 - '%GG': raw verification message from GPG for a signed commit
 - '%G?': show "G" for a good (valid) signature,
   "B" for a bad signature,
diff --git a/pretty.c b/pretty.c
index b83a3ecd23..d142b457b5 100644
--- a/pretty.c
+++ b/pretty.c
@@ -768,6 +768,9 @@ struct format_commit_context {
 	unsigned commit_header_parsed:1;
 	unsigned commit_message_parsed:1;
 	struct signature_check signature_check;
+	unsigned signature_checked:2;
+	struct strbuf signature;
+	struct strbuf signature_payload;
 	enum flush_type flush_type;
 	enum trunc_type truncate;
 	const char *message;
@@ -1228,8 +1231,30 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 	}
 
 	if (placeholder[0] == 'G') {
-		if (!c->signature_check.result)
-			check_commit_signature(c->commit, &(c->signature_check));
+		if (!c->signature_checked) {
+			parse_signed_commit(c->commit, &(c->signature_payload), &(c->signature));
+			c->signature_checked = 1;
+		}
+		switch (placeholder[1]) {
+		case 'R':
+			strbuf_addbuf(sb, &(c->signature));
+			break;
+		case '+':
+			strbuf_addch(sb, c->signature.len ? 'Y' : 'N');
+			break;
+		default:
+			goto do_signature_check;
+		}
+		return 2;
+
+do_signature_check:
+		if (c->signature_checked < 2) {
+			if (c->signature.len)
+				check_signature(c->signature_payload.buf, c->signature_payload.len,
+						c->signature.buf, c->signature.len,
+						&(c->signature_check));
+			c->signature_checked = 2;
+		}
 		switch (placeholder[1]) {
 		case 'G':
 			if (c->signature_check.gpg_output)
@@ -1246,6 +1271,9 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 			case 'Y':
 			case 'R':
 				strbuf_addch(sb, c->signature_check.result);
+				break;
+			case 0: // i.e. no signature so we never ran the check
+				strbuf_addch(sb, 'N');
 			}
 			break;
 		case 'S':
@@ -1527,6 +1555,8 @@ void format_commit_message(const struct commit *commit,
 	context.commit = commit;
 	context.pretty_ctx = pretty_ctx;
 	context.wrap_start = sb->len;
+	strbuf_init(&context.signature, 0);
+	strbuf_init(&context.signature_payload, 0);
 	/*
 	 * convert a commit message to UTF-8 first
 	 * as far as 'format_commit_item' assumes it in UTF-8
@@ -1556,6 +1586,8 @@ void format_commit_message(const struct commit *commit,
 			strbuf_attach(sb, out, outsz, outsz + 1);
 	}
 
+	strbuf_release(&context.signature);
+	strbuf_release(&context.signature_payload);
 	free(context.commit_encoding);
 	unuse_commit_buffer(commit, context.message);
 }
-- 
2.19.1
John Passaro· Dec 13, 2018, 21:22 UTC · re: John Passaro · lore

[PATCH 2/4] t/t7510-signed-commit.sh: test new placeholders

Test that %GR output ("Raw" contents of "gpgsig" header) looks like ASCII-armored GPG signature.

Test %G+ (Y/N for presence/absence of "gpgsig" header) by adding it to existing format tests for signed commits.

Signed-off-by: John Passaro <john.a.passaro@gmail.com>
---
 t/t7510-signed-commit.sh | 30 +++++++++++++++++++++++++-----
 1 file changed, 25 insertions(+), 5 deletions(-)
Show changes to t/t7510-signed-commit.sh +25 −5
diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
index 86d3f93fa2..aff6b1eb3a 100755
--- a/t/t7510-signed-commit.sh
+++ b/t/t7510-signed-commit.sh
@@ -177,8 +177,9 @@ test_expect_success GPG 'show good signature with custom format' '
 	C O Mitter <committer@example.com>
 	73D758744BE721698EC54E8713B6F51ECDDE430D
 	73D758744BE721698EC54E8713B6F51ECDDE430D
+	Y
 	EOF
-	git log -1 --format="%G?%n%GK%n%GS%n%GF%n%GP" sixth-signed >actual &&
+	git log -1 --format="%G?%n%GK%n%GS%n%GF%n%GP%n%G+" sixth-signed >actual &&
 	test_cmp expect actual
 '
 
@@ -189,8 +190,9 @@ test_expect_success GPG 'show bad signature with custom format' '
 	C O Mitter <committer@example.com>
 
 
+	Y
 	EOF
-	git log -1 --format="%G?%n%GK%n%GS%n%GF%n%GP" $(cat forged1.commit) >actual &&
+	git log -1 --format="%G?%n%GK%n%GS%n%GF%n%GP%n%G+" $(cat forged1.commit) >actual &&
 	test_cmp expect actual
 '
 
@@ -201,8 +203,9 @@ test_expect_success GPG 'show untrusted signature with custom format' '
 	Eris Discordia <discord@example.net>
 	F8364A59E07FFE9F4D63005A65A0EEA02E30CAD7
 	D4BE22311AD3131E5EDA29A461092E85B7227189
+	Y
 	EOF
-	git log -1 --format="%G?%n%GK%n%GS%n%GF%n%GP" eighth-signed-alt >actual &&
+	git log -1 --format="%G?%n%GK%n%GS%n%GF%n%GP%n%G+" eighth-signed-alt >actual &&
 	test_cmp expect actual
 '
 
@@ -213,8 +216,9 @@ test_expect_success GPG 'show unknown signature with custom format' '
 
 
 
+	Y
 	EOF
-	GNUPGHOME="$GNUPGHOME_NOT_USED" git log -1 --format="%G?%n%GK%n%GS%n%GF%n%GP" eighth-signed-alt >actual &&
+	GNUPGHOME="$GNUPGHOME_NOT_USED" git log -1 --format="%G?%n%GK%n%GS%n%GF%n%GP%n%G+" eighth-signed-alt >actual &&
 	test_cmp expect actual
 '
 
@@ -225,11 +229,27 @@ test_expect_success GPG 'show lack of signature with custom format' '
 
 
 
+	N
 	EOF
-	git log -1 --format="%G?%n%GK%n%GS%n%GF%n%GP" seventh-unsigned >actual &&
+	git log -1 --format="%G?%n%GK%n%GS%n%GF%n%GP%n%G+" seventh-unsigned >actual &&
 	test_cmp expect actual
 '
 
+test_expect_success GPG 'show lack of raw signature with custom format' '
+	git log -1 --format=format:"%GR" seventh-unsigned > actual &&
+	test_must_be_empty actual
+'
+
+test_expect_success GPG 'show raw signature with custom format' '
+	git log -1 --format=format:"%GR" sixth-signed >output &&
+	cat output &&
+	head -n1 output | grep -q "^---*BEGIN PGP SIGNATURE---*$" &&
+	sed 1d output | grep -q "^$" &&
+	sed "1,/^$/d" output | grep -q "^[a-zA-Z0-9+/][a-zA-Z0-9+/=]*$" &&
+	tail -n2 output | head -n1 | grep -q "^=[a-zA-Z0-9+/][a-zA-Z0-9+/=]*$" &&
+	tail -n1 output | grep -q "^---*END PGP SIGNATURE---*$"
+'
+
 test_expect_success GPG 'log.showsignature behaves like --show-signature' '
 	test_config log.showsignature true &&
 	git show initial >actual &&
-- 
2.19.1
John Passaro· Dec 13, 2018, 21:22 UTC · re: John Passaro · lore

[PATCH 3/4] doc, tests: pretty behavior when gpg missing

Test that when GPG cannot be run, new placeholders %GR and %G+ are unaffected, %G? always returns 'N', and other GPG-related placeholders return blank.

As of e5a329a279 ("run-command: report exec failure" 2018-12-11), if GPG cannot be run and placeholders requiring GPG are given, git complains to stderr that GPG cannot be found. That commit included low-level tests for this behavior. Now, test it also at the level of everyday user commands.

Signed-off-by: John Passaro <john.a.passaro@gmail.com>
---
 Documentation/pretty-formats.txt |  6 +-
 t/t7510-signed-commit.sh         | 95 ++++++++++++++++++++++++++++++++
 2 files changed, 99 insertions(+), 2 deletions(-)
Show changes to 2 files +99 −2

Documentation/pretty-formats.txt, t/t7510-signed-commit.sh

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 582454a4f7..4a83796250 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -144,7 +144,9 @@ ifndef::git-rev-list[]
 endif::git-rev-list[]
 - '%GR': contents of the commits signature (blank when unsigned)
 - '%G+': "Y" if the commit is signed, "N" otherwise
-- '%GG': raw verification message from GPG for a signed commit
+- '%GG': raw verification message from GPG for a signed commit.
+  This and all the other %G* placeholders, other than %GR, %G+, and
+  %G?, return blank if GPG cannot be run.
 - '%G?': show "G" for a good (valid) signature,
   "B" for a bad signature,
   "U" for a good signature with unknown validity,
@@ -152,7 +154,7 @@ endif::git-rev-list[]
   "Y" for a good signature made by an expired key,
   "R" for a good signature made by a revoked key,
   "E" if the signature cannot be checked (e.g. missing key)
-  and "N" for no signature
+  and "N" for no signature (e.g. unsigned, or GPG cannot be run)
 - '%GS': show the name of the signer for a signed commit
 - '%GK': show the key used to sign a signed commit
 - '%GF': show the fingerprint of the key used to sign a signed commit
diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
index aff6b1eb3a..d65425eddc 100755
--- a/t/t7510-signed-commit.sh
+++ b/t/t7510-signed-commit.sh
@@ -170,6 +170,48 @@ test_expect_success GPG 'amending already signed commit' '
 	! grep "BAD signature from" actual
 '
 
+test_expect_success GPG 'show custom format fields for signed commit if gpg is missing' '
+	cat >expect <<-\EOF &&
+	N
+
+
+
+
+	Y
+	EOF
+	test_config gpg.program this-is-not-a-program &&
+	git log -n1 --format="%G?%n%GK%n%GS%n%GF%n%GP%n%G+" sixth-signed >actual 2>/dev/null &&
+	test_cmp expect actual
+'
+
+test_expect_success GPG 'show custom format fields for unsigned commit if gpg is missing' '
+	cat >expect <<-\EOF &&
+	N
+
+
+
+
+	N
+	EOF
+	test_config gpg.program this-is-not-a-program &&
+	git log -n1 --format="%G?%n%GK%n%GS%n%GF%n%GP%n%G+" seventh-unsigned >actual 2>/dev/null &&
+	test_cmp expect actual
+'
+
+test_expect_success GPG 'show error for custom format fields on signed commit if gpg is missing' '
+	test_config gpg.program this-is-not-a-program &&
+	git log -n1 --format="%G?%n%GK%n%GS%n%GF%n%GP%n%G+" sixth-signed >/dev/null 2>errors &&
+	test $(wc -l <errors) = 1 &&
+	test_i18ngrep "^error: " errors &&
+	grep this-is-not-a-program errors
+'
+
+test_expect_success GPG 'do not run gpg at all for unsigned commit' '
+	test_config gpg.program this-is-not-a-program &&
+	git log -n1 --format="%G?%n%GK%n%GS%n%GF%n%GP%n%G+" seventh-unsigned >/dev/null 2>errors &&
+	test_must_be_empty errors
+'
+
 test_expect_success GPG 'show good signature with custom format' '
 	cat >expect <<-\EOF &&
 	G
@@ -240,6 +282,14 @@ test_expect_success GPG 'show lack of raw signature with custom format' '
 	test_must_be_empty actual
 '
 
+test_expect_success GPG 'show lack of raw signature with custom format without running GPG' '
+	echo N > expected &&
+	test_config gpg.program this-is-not-a-program &&
+	git log -1 --format="%G+%GR" seventh-unsigned >actual 2>errors &&
+	test_cmp expected actual &&
+	test_must_be_empty errors
+'
+
 test_expect_success GPG 'show raw signature with custom format' '
 	git log -1 --format=format:"%GR" sixth-signed >output &&
 	cat output &&
@@ -250,6 +300,51 @@ test_expect_success GPG 'show raw signature with custom format' '
 	tail -n1 output | grep -q "^---*END PGP SIGNATURE---*$"
 '
 
+test_expect_success GPG 'show raw signature with custom format without running GPG' '
+	test_config gpg.program this-is-not-a-program &&
+	git log -1 --format=format:"%GR" sixth-signed >rawsig 2>errors &&
+	cat rawsig &&
+	head -n1 rawsig | grep -q "^---*BEGIN PGP SIGNATURE---*$" &&
+	sed 1d rawsig | grep -q "^$" &&
+	sed "1,/^$/d" rawsig | grep -q "^[a-zA-Z0-9+/][a-zA-Z0-9+/=]*$" &&
+	tail -n2 rawsig | head -n1 | grep -q "^=[a-zA-Z0-9+/][a-zA-Z0-9+/=]*$" &&
+	tail -n1 rawsig | grep -q "^---*END PGP SIGNATURE---*$" &&
+	test_must_be_empty errors
+'
+
+test_expect_success GPG 'show presence of gpgsig with custom format when gpg is missing without errors' '
+	echo Y > expected &&
+	git log -1 --format=%G+ sixth-signed >output 2>errors &&
+	test_cmp expected output &&
+	test_must_be_empty errors
+'
+
+test_expect_success GPG 'show presence of invalid gpgsig header' '
+	printf gpgsig >gpgsig-header &&
+	tee prank-signature <<-\EOF | sed "s/^/ /" >>gpgsig-header &&
+	this is not a signature but an awful...
+					   888
+					   888
+					   888
+	88888b.  888d888  8888b.  88888b.  888  888
+	888 "88b 888P"       "88b 888 "88b 888 .88P
+	888  888 888     .d888888 888  888 888888K
+	888 d88P 888     888  888 888  888 888 "88b
+	88888P"  888     "Y888888 888  888 888  888
+	888
+	888
+	888
+	EOF
+	git cat-file commit seventh-unsigned >bare-commit-data &&
+	sed "/^committer/r gpgsig-header" bare-commit-data >commit-data &&
+	git hash-object -w -t commit commit-data >commit &&
+	echo Y >expected &&
+	cat prank-signature >>expected &&
+	git log -n1 --format=format:%G+%n%GR $(cat commit) >actual 2>errors &&
+	test_cmp expected actual &&
+	test_must_be_empty errors
+'
+
 test_expect_success GPG 'log.showsignature behaves like --show-signature' '
 	test_config log.showsignature true &&
 	git show initial >actual &&
-- 
2.19.1
John Passaro· Dec 13, 2018, 21:22 UTC · re: John Passaro · lore

[PATCH 4/4] docs/pretty-formats: add explanation + copy edits

Clarify description of %G? = "U" to say it can mean good signature but untrusted key.

Make wording consistent between %G* placeholders and other placeholders by removing the verb "show".

Signed-off-by: John Passaro <john.a.passaro@gmail.com>
---
 Documentation/pretty-formats.txt | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)
Show changes to Documentation/pretty-formats.txt +7 −6
diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 4a83796250..32c2f75060 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -147,18 +147,19 @@ endif::git-rev-list[]
 - '%GG': raw verification message from GPG for a signed commit.
   This and all the other %G* placeholders, other than %GR, %G+, and
   %G?, return blank if GPG cannot be run.
-- '%G?': show "G" for a good (valid) signature,
+- '%G?': "G" for a good (valid) signature,
   "B" for a bad signature,
-  "U" for a good signature with unknown validity,
+  "U" for a good signature with unknown validity (e.g. key is known but
+  not trusted),
   "X" for a good signature that has expired,
   "Y" for a good signature made by an expired key,
   "R" for a good signature made by a revoked key,
   "E" if the signature cannot be checked (e.g. missing key)
   and "N" for no signature (e.g. unsigned, or GPG cannot be run)
-- '%GS': show the name of the signer for a signed commit
-- '%GK': show the key used to sign a signed commit
-- '%GF': show the fingerprint of the key used to sign a signed commit
-- '%GP': show the fingerprint of the primary key whose subkey was used
+- '%GS': name of the signer for a signed commit
+- '%GK': key used to sign a signed commit
+- '%GF': fingerprint of the key used to sign a signed commit
+- '%GP': fingerprint of the primary key whose subkey was used
   to sign a signed commit
 - '%gD': reflog selector, e.g., `refs/stash@{1}` or
   `refs/stash@{2 minutes ago`}; the format follows the rules described
-- 
2.19.1
Michał Górny· Dec 14, 2018, 04:11 UTC · re: John Passaro · lore

Re: [PATCH 0/4] Expose gpgsig in pretty-print

On Thu, 2018-12-13 at 16:22 -0500, John Passaro wrote:
Show 40 quoted lines
> Currently, users who do not have GPG installed have no way to discern
> signed from unsigned commits without examining raw commit data. I
> propose two new pretty-print placeholders to expose this information:
> 
> %GR: full ("R"aw) contents of gpgsig header
> %G+: Y/N if the commit has nonempty gpgsig header or not
> 
> The second is of course much more likely to be used, but having exposed
> the one, exposing the other too adds almost no complexity.
> 
> I'm open to suggestion on the names of these placeholders.
> 
> This commit is based on master but e5a329a279 ("run-command: report exec
> failure" 2018-12-11) is required for the tests to pass.
> 
> One note is that this change touches areas of the pretty-format
> documentation that are radically revamped in aw/pretty-trailers: see
> 42617752d4 ("doc: group pretty-format.txt placeholders descriptions"
> 2018-12-08). I have another version of this branch based on that branch
> as well, so you can use that in case conflicts with aw/pretty-trailers
> arise.
> 
> See:
> - https://github.com/jpassaro/git/tree/jp/pretty-expose-gpgsig
> - https://github.com/jpassaro/git/tree/jp/pretty-expose-gpgsig--based-on-aw-pretty-trailers
> 
> John Passaro (4):
>   pretty: expose raw commit signature
>   t/t7510-signed-commit.sh: test new placeholders
>   doc, tests: pretty behavior when gpg missing
>   docs/pretty-formats: add explanation + copy edits
> 
>  Documentation/pretty-formats.txt |  21 ++++--
>  pretty.c                         |  36 ++++++++-
>  t/t7510-signed-commit.sh         | 125 +++++++++++++++++++++++++++++--
>  3 files changed, 167 insertions(+), 15 deletions(-)
> 
> 
> base-commit: 5d826e972970a784bd7a7bdf587512510097b8c7
> prerequisite-patch-id: aedfe228fd293714d9cd0392ac22ff1cba7365db

Just a suggestion: since the raw signature is not very useful without the commit data to check it against, and the commit data is non-trivial to construct (requires mangling raw data anyway), maybe you could either add another placeholder to get the data for signature verification, or (alternatively or simultaneously) add a placeholder that prints both data and signature in the OpenPGP message format (i.e. something you can pass straight to 'gpg --verify').

-- 
Best regards,
Michał Górny
John Passaro· Dec 14, 2018, 16:07 UTC · re: Michał Górny · lore

Re: [PATCH 0/4] Expose gpgsig in pretty-print

On Thu, Dec 13, 2018 at 11:12 PM Michał Górny <mgorny@gentoo.org> wrote:
Show 51 quoted lines
>
> On Thu, 2018-12-13 at 16:22 -0500, John Passaro wrote:
> > Currently, users who do not have GPG installed have no way to discern
> > signed from unsigned commits without examining raw commit data. I
> > propose two new pretty-print placeholders to expose this information:
> >
> > %GR: full ("R"aw) contents of gpgsig header
> > %G+: Y/N if the commit has nonempty gpgsig header or not
> >
> > The second is of course much more likely to be used, but having exposed
> > the one, exposing the other too adds almost no complexity.
> >
> > I'm open to suggestion on the names of these placeholders.
> >
> > This commit is based on master but e5a329a279 ("run-command: report exec
> > failure" 2018-12-11) is required for the tests to pass.
> >
> > One note is that this change touches areas of the pretty-format
> > documentation that are radically revamped in aw/pretty-trailers: see
> > 42617752d4 ("doc: group pretty-format.txt placeholders descriptions"
> > 2018-12-08). I have another version of this branch based on that branch
> > as well, so you can use that in case conflicts with aw/pretty-trailers
> > arise.
> >
> > See:
> > - https://github.com/jpassaro/git/tree/jp/pretty-expose-gpgsig
> > - https://github.com/jpassaro/git/tree/jp/pretty-expose-gpgsig--based-on-aw-pretty-trailers
> >
> > John Passaro (4):
> >   pretty: expose raw commit signature
> >   t/t7510-signed-commit.sh: test new placeholders
> >   doc, tests: pretty behavior when gpg missing
> >   docs/pretty-formats: add explanation + copy edits
> >
> >  Documentation/pretty-formats.txt |  21 ++++--
> >  pretty.c                         |  36 ++++++++-
> >  t/t7510-signed-commit.sh         | 125 +++++++++++++++++++++++++++++--
> >  3 files changed, 167 insertions(+), 15 deletions(-)
> >
> >
> > base-commit: 5d826e972970a784bd7a7bdf587512510097b8c7
> > prerequisite-patch-id: aedfe228fd293714d9cd0392ac22ff1cba7365db
>
> Just a suggestion: since the raw signature is not very useful without
> the commit data to check it against, and the commit data is non-trivial
> to construct (requires mangling raw data anyway), maybe you could either
> add another placeholder to get the data for signature verification, or
> (alternatively or simultaneously) add a placeholder that prints both
> data and signature in the OpenPGP message format (i.e. something you can
> pass straight to 'gpg --verify').
>
That's a great idea!
Then I might rename the other new placeholders too:

%Gs: signed commit signature (blank when unsigned) %Gp: signed commit payload (i.e. in practice minus the gpgsig header; also blank when unsigned as well) %Gq: query/question whether is signed commit ("Y"/"N")

Thus establishing %G<lowercase> as the gpg-related placeholders that do not actually require gpg.

And add a test that %Gp%n%Gs or the like passes gpg --verify.
I'll put in a v2 later today or tomorrow. Thank you for the feedback!

-- JP

Michał Górny· Dec 14, 2018, 16:48 UTC · re: John Passaro · lore

Re: [PATCH 0/4] Expose gpgsig in pretty-print

On Fri, 2018-12-14 at 11:07 -0500, John Passaro wrote:
Show 69 quoted lines
> On Thu, Dec 13, 2018 at 11:12 PM Michał Górny <mgorny@gentoo.org> wrote:
> > 
> > On Thu, 2018-12-13 at 16:22 -0500, John Passaro wrote:
> > > Currently, users who do not have GPG installed have no way to discern
> > > signed from unsigned commits without examining raw commit data. I
> > > propose two new pretty-print placeholders to expose this information:
> > > 
> > > %GR: full ("R"aw) contents of gpgsig header
> > > %G+: Y/N if the commit has nonempty gpgsig header or not
> > > 
> > > The second is of course much more likely to be used, but having exposed
> > > the one, exposing the other too adds almost no complexity.
> > > 
> > > I'm open to suggestion on the names of these placeholders.
> > > 
> > > This commit is based on master but e5a329a279 ("run-command: report exec
> > > failure" 2018-12-11) is required for the tests to pass.
> > > 
> > > One note is that this change touches areas of the pretty-format
> > > documentation that are radically revamped in aw/pretty-trailers: see
> > > 42617752d4 ("doc: group pretty-format.txt placeholders descriptions"
> > > 2018-12-08). I have another version of this branch based on that branch
> > > as well, so you can use that in case conflicts with aw/pretty-trailers
> > > arise.
> > > 
> > > See:
> > > - https://github.com/jpassaro/git/tree/jp/pretty-expose-gpgsig
> > > - https://github.com/jpassaro/git/tree/jp/pretty-expose-gpgsig--based-on-aw-pretty-trailers
> > > 
> > > John Passaro (4):
> > >   pretty: expose raw commit signature
> > >   t/t7510-signed-commit.sh: test new placeholders
> > >   doc, tests: pretty behavior when gpg missing
> > >   docs/pretty-formats: add explanation + copy edits
> > > 
> > >  Documentation/pretty-formats.txt |  21 ++++--
> > >  pretty.c                         |  36 ++++++++-
> > >  t/t7510-signed-commit.sh         | 125 +++++++++++++++++++++++++++++--
> > >  3 files changed, 167 insertions(+), 15 deletions(-)
> > > 
> > > 
> > > base-commit: 5d826e972970a784bd7a7bdf587512510097b8c7
> > > prerequisite-patch-id: aedfe228fd293714d9cd0392ac22ff1cba7365db
> > 
> > Just a suggestion: since the raw signature is not very useful without
> > the commit data to check it against, and the commit data is non-trivial
> > to construct (requires mangling raw data anyway), maybe you could either
> > add another placeholder to get the data for signature verification, or
> > (alternatively or simultaneously) add a placeholder that prints both
> > data and signature in the OpenPGP message format (i.e. something you can
> > pass straight to 'gpg --verify').
> > 
> 
> That's a great idea!
> 
> Then I might rename the other new placeholders too:
> 
> %Gs: signed commit signature (blank when unsigned)
> %Gp: signed commit payload (i.e. in practice minus the gpgsig header;
> also blank when unsigned as well)
> %Gq: query/question whether is signed commit ("Y"/"N")
> 
> Thus establishing %G<lowercase> as the gpg-related placeholders that
> do not actually require gpg.
> 
> And add a test that %Gp%n%Gs or the like passes gpg --verify.
> 
> I'll put in a v2 later today or tomorrow. Thank you for the feedback!
> 

Technically speaking, '%Gp%n%Gs' sounds a bit odd, given that the payload needs to be preceded by the PGP message header but instead of footer it has the signature's header. Also note that some lines in the payload may need to be escaped.

-- 
Best regards,
Michał Górny
John Passaro· Dec 14, 2018, 23:10 UTC · re: Michał Górny · lore

Re: [PATCH 0/4] Expose gpgsig in pretty-print

On Fri, Dec 14, 2018 at 11:49 AM Michał Górny <mgorny@gentoo.org> wrote:
Show 76 quoted lines
>
> On Fri, 2018-12-14 at 11:07 -0500, John Passaro wrote:
> > On Thu, Dec 13, 2018 at 11:12 PM Michał Górny <mgorny@gentoo.org> wrote:
> > >
> > > On Thu, 2018-12-13 at 16:22 -0500, John Passaro wrote:
> > > > Currently, users who do not have GPG installed have no way to discern
> > > > signed from unsigned commits without examining raw commit data. I
> > > > propose two new pretty-print placeholders to expose this information:
> > > >
> > > > %GR: full ("R"aw) contents of gpgsig header
> > > > %G+: Y/N if the commit has nonempty gpgsig header or not
> > > >
> > > > The second is of course much more likely to be used, but having exposed
> > > > the one, exposing the other too adds almost no complexity.
> > > >
> > > > I'm open to suggestion on the names of these placeholders.
> > > >
> > > > This commit is based on master but e5a329a279 ("run-command: report exec
> > > > failure" 2018-12-11) is required for the tests to pass.
> > > >
> > > > One note is that this change touches areas of the pretty-format
> > > > documentation that are radically revamped in aw/pretty-trailers: see
> > > > 42617752d4 ("doc: group pretty-format.txt placeholders descriptions"
> > > > 2018-12-08). I have another version of this branch based on that branch
> > > > as well, so you can use that in case conflicts with aw/pretty-trailers
> > > > arise.
> > > >
> > > > See:
> > > > - https://github.com/jpassaro/git/tree/jp/pretty-expose-gpgsig
> > > > - https://github.com/jpassaro/git/tree/jp/pretty-expose-gpgsig--based-on-aw-pretty-trailers
> > > >
> > > > John Passaro (4):
> > > >   pretty: expose raw commit signature
> > > >   t/t7510-signed-commit.sh: test new placeholders
> > > >   doc, tests: pretty behavior when gpg missing
> > > >   docs/pretty-formats: add explanation + copy edits
> > > >
> > > >  Documentation/pretty-formats.txt |  21 ++++--
> > > >  pretty.c                         |  36 ++++++++-
> > > >  t/t7510-signed-commit.sh         | 125 +++++++++++++++++++++++++++++--
> > > >  3 files changed, 167 insertions(+), 15 deletions(-)
> > > >
> > > >
> > > > base-commit: 5d826e972970a784bd7a7bdf587512510097b8c7
> > > > prerequisite-patch-id: aedfe228fd293714d9cd0392ac22ff1cba7365db
> > >
> > > Just a suggestion: since the raw signature is not very useful without
> > > the commit data to check it against, and the commit data is non-trivial
> > > to construct (requires mangling raw data anyway), maybe you could either
> > > add another placeholder to get the data for signature verification, or
> > > (alternatively or simultaneously) add a placeholder that prints both
> > > data and signature in the OpenPGP message format (i.e. something you can
> > > pass straight to 'gpg --verify').
> > >
> >
> > That's a great idea!
> >
> > Then I might rename the other new placeholders too:
> >
> > %Gs: signed commit signature (blank when unsigned)
> > %Gp: signed commit payload (i.e. in practice minus the gpgsig header;
> > also blank when unsigned as well)
> > %Gq: query/question whether is signed commit ("Y"/"N")
> >
> > Thus establishing %G<lowercase> as the gpg-related placeholders that
> > do not actually require gpg.
> >
> > And add a test that %Gp%n%Gs or the like passes gpg --verify.
> >
> > I'll put in a v2 later today or tomorrow. Thank you for the feedback!
> >
>
> Technically speaking, '%Gp%n%Gs' sounds a bit odd, given that
> the payload needs to be preceded by the PGP message header but instead
> of footer it has the signature's header.  Also note that some lines in
> the payload may need to be escaped.

It's indeed failing with "-----BEGIN PGP SIGNED MESSAGE-----%n%Gp%n%Gs"

This appears to be a misunderstanding on my part of how cleartext GPG messages work, as this message seems to fail verification even when I construct it manually:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

tree 3820419b50e9827493beedf6a1423e7d9c7edf3b parent 356f37356edf1a45c8494870e1c051e2fbe529fa author A U Thor <author@example.com> 1112912413 -0700 committer C O Mitter <committer@example.com> 1112912533 -0700

sixth -----BEGIN PGP SIGNATURE-----

iHQEABECADQWIQRz11h0S+chaY7FTocTtvUezd5DDQUCXBQzKRYcY29tbWl0dGVy QGV4YW1wbGUuY29tAAoJEBO29R7N3kMN2QYAnA2A/VpD4qMI9rJlIYnyyO32rTXz AJ0drWlJsASMcf6AEX6nSQPxWq81Fg== =fr2F -----END PGP SIGNATURE-----

I got this by running tho following inside the trash directory for t7510,
which as far as I can tell is roughly equivalent to
The gpg --verify fails.
{
  echo "-----BEGIN PGP SIGNED MESSAGE-----"
  echo Hash: SHA256
  echo
  git cat-file commit sixth-signed | perl -ne '/^(?:gpgsig)? / || print'
  git cat-file commit sixth-signed | perl -ne 's/^(?:gpgsig)? // && print'
} | tee commit-as-signed-message | gpg --verify

All seems to work fine when I treat %Gs as a detached signature. How should the combined message be constructed properly? (Goes to usefulness of printing signature payload, and indeed of raw crypto data in general.)

> --
> Best regards,
> Michał Górny
John Passaro· Dec 14, 2018, 23:13 UTC · re: John Passaro · lore

Re: [PATCH 0/4] Expose gpgsig in pretty-print

On Fri, Dec 14, 2018 at 6:10 PM John Passaro <john.a.passaro@gmail.com> wrote:
Show 113 quoted lines
>
> On Fri, Dec 14, 2018 at 11:49 AM Michał Górny <mgorny@gentoo.org> wrote:
> >
> > On Fri, 2018-12-14 at 11:07 -0500, John Passaro wrote:
> > > On Thu, Dec 13, 2018 at 11:12 PM Michał Górny <mgorny@gentoo.org> wrote:
> > > >
> > > > On Thu, 2018-12-13 at 16:22 -0500, John Passaro wrote:
> > > > > Currently, users who do not have GPG installed have no way to discern
> > > > > signed from unsigned commits without examining raw commit data. I
> > > > > propose two new pretty-print placeholders to expose this information:
> > > > >
> > > > > %GR: full ("R"aw) contents of gpgsig header
> > > > > %G+: Y/N if the commit has nonempty gpgsig header or not
> > > > >
> > > > > The second is of course much more likely to be used, but having exposed
> > > > > the one, exposing the other too adds almost no complexity.
> > > > >
> > > > > I'm open to suggestion on the names of these placeholders.
> > > > >
> > > > > This commit is based on master but e5a329a279 ("run-command: report exec
> > > > > failure" 2018-12-11) is required for the tests to pass.
> > > > >
> > > > > One note is that this change touches areas of the pretty-format
> > > > > documentation that are radically revamped in aw/pretty-trailers: see
> > > > > 42617752d4 ("doc: group pretty-format.txt placeholders descriptions"
> > > > > 2018-12-08). I have another version of this branch based on that branch
> > > > > as well, so you can use that in case conflicts with aw/pretty-trailers
> > > > > arise.
> > > > >
> > > > > See:
> > > > > - https://github.com/jpassaro/git/tree/jp/pretty-expose-gpgsig
> > > > > - https://github.com/jpassaro/git/tree/jp/pretty-expose-gpgsig--based-on-aw-pretty-trailers
> > > > >
> > > > > John Passaro (4):
> > > > >   pretty: expose raw commit signature
> > > > >   t/t7510-signed-commit.sh: test new placeholders
> > > > >   doc, tests: pretty behavior when gpg missing
> > > > >   docs/pretty-formats: add explanation + copy edits
> > > > >
> > > > >  Documentation/pretty-formats.txt |  21 ++++--
> > > > >  pretty.c                         |  36 ++++++++-
> > > > >  t/t7510-signed-commit.sh         | 125 +++++++++++++++++++++++++++++--
> > > > >  3 files changed, 167 insertions(+), 15 deletions(-)
> > > > >
> > > > >
> > > > > base-commit: 5d826e972970a784bd7a7bdf587512510097b8c7
> > > > > prerequisite-patch-id: aedfe228fd293714d9cd0392ac22ff1cba7365db
> > > >
> > > > Just a suggestion: since the raw signature is not very useful without
> > > > the commit data to check it against, and the commit data is non-trivial
> > > > to construct (requires mangling raw data anyway), maybe you could either
> > > > add another placeholder to get the data for signature verification, or
> > > > (alternatively or simultaneously) add a placeholder that prints both
> > > > data and signature in the OpenPGP message format (i.e. something you can
> > > > pass straight to 'gpg --verify').
> > > >
> > >
> > > That's a great idea!
> > >
> > > Then I might rename the other new placeholders too:
> > >
> > > %Gs: signed commit signature (blank when unsigned)
> > > %Gp: signed commit payload (i.e. in practice minus the gpgsig header;
> > > also blank when unsigned as well)
> > > %Gq: query/question whether is signed commit ("Y"/"N")
> > >
> > > Thus establishing %G<lowercase> as the gpg-related placeholders that
> > > do not actually require gpg.
> > >
> > > And add a test that %Gp%n%Gs or the like passes gpg --verify.
> > >
> > > I'll put in a v2 later today or tomorrow. Thank you for the feedback!
> > >
> >
> > Technically speaking, '%Gp%n%Gs' sounds a bit odd, given that
> > the payload needs to be preceded by the PGP message header but instead
> > of footer it has the signature's header.  Also note that some lines in
> > the payload may need to be escaped.
>
> It's indeed failing with
> "-----BEGIN PGP SIGNED MESSAGE-----%n%Gp%n%Gs"
>
> This appears to be a misunderstanding on my part of how cleartext GPG
> messages work, as this message seems to fail verification even when I
> construct it manually:
>
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA256
>
> tree 3820419b50e9827493beedf6a1423e7d9c7edf3b
> parent 356f37356edf1a45c8494870e1c051e2fbe529fa
> author A U Thor <author@example.com> 1112912413 -0700
> committer C O Mitter <committer@example.com> 1112912533 -0700
>
> sixth
> -----BEGIN PGP SIGNATURE-----
>
> iHQEABECADQWIQRz11h0S+chaY7FTocTtvUezd5DDQUCXBQzKRYcY29tbWl0dGVy
> QGV4YW1wbGUuY29tAAoJEBO29R7N3kMN2QYAnA2A/VpD4qMI9rJlIYnyyO32rTXz
> AJ0drWlJsASMcf6AEX6nSQPxWq81Fg==
> =fr2F
> -----END PGP SIGNATURE-----
>
> I got this by running tho following inside the trash directory for t7510,
> which as far as I can tell is roughly equivalent to
> The gpg --verify fails.
> {
>   echo "-----BEGIN PGP SIGNED MESSAGE-----"
>   echo Hash: SHA256
>   echo
>   git cat-file commit sixth-signed | perl -ne '/^(?:gpgsig)? / || print'
>   git cat-file commit sixth-signed | perl -ne 's/^(?:gpgsig)? // && print'
> } | tee commit-as-signed-message | gpg --verify

I should add that when I took out Hash: SHA256 (don't ask), gpg went from saying "signature digest conflict" to saying "BAD signature". Not quite as bad but still confusing.

Show 10 quoted lines
>
> All seems to work fine when I treat %Gs as a detached signature.
> How should the combined message be constructed properly? (Goes
> to usefulness of printing signature payload, and indeed of raw crypto
> data in general.)
>
>
> > --
> > Best regards,
> > Michał Górny
Jeff King· Dec 17, 2018, 20:24 UTC · re: John Passaro · lore

Re: [PATCH 0/4] Expose gpgsig in pretty-print

On Fri, Dec 14, 2018 at 11:07:03AM -0500, John Passaro wrote:
Show 5 quoted lines
> Then I might rename the other new placeholders too:
> 
> %Gs: signed commit signature (blank when unsigned)
> %Gp: signed commit payload (i.e. in practice minus the gpgsig header;
> also blank when unsigned as well)

One complication: the pretty-printing code sees the commit data in the i18n.logOutputEncoding charset (utf8 by default). But the signature will be over the raw commit data. That's also utf8 by default, but there may be an encoding header indicating that it's something else. In that case, you couldn't actually verify the signature from the "%Gs%Gp" pair.

I don't think that's insurmountable in the code. You'll have to jump through a few hoops to make sure you have the _original_ payload, but we obviously do have that data. However, it does feel a little weird to include content from a different encoding in the middle of the log output stream which claims to be i18n.logOutputEncoding.

-Peff
John Passaro· Dec 19, 2018, 05:59 UTC · re: Jeff King · lore

Re: [PATCH 0/4] Expose gpgsig in pretty-print

On Fri, Dec 14, 2018 at 6:10 PM John Passaro wrote:
> All seems to work fine when I treat %Gs as a detached signature.

In light of this, my best guess as to why the cleartext PGP message didn't verify properly is that the commit data normally doesn't end with \n, but as far as I can tell there's no way to express that in the cleartext format. I don't see a way around this. However, as long as the following works, I think we have proof-of-concept that this enhancement allows you to play with signature data however you please without leaving it to git under the hood:

gpg --verify <(git show -s --format=format:%Gs commit) <(git show -s --format=format:%Gp commit)

On Mon, Dec 17, 2018 at 3:24 PM Jeff King <peff@peff.net> wrote:
Show 21 quoted lines
>
> On Fri, Dec 14, 2018 at 11:07:03AM -0500, John Passaro wrote:
>
> > Then I might rename the other new placeholders too:
> >
> > %Gs: signed commit signature (blank when unsigned)
> > %Gp: signed commit payload (i.e. in practice minus the gpgsig header;
> > also blank when unsigned as well)
>
> One complication: the pretty-printing code sees the commit data in the
> i18n.logOutputEncoding charset (utf8 by default). But the signature will
> be over the raw commit data. That's also utf8 by default, but there may
> be an encoding header indicating that it's something else. In that case,
> you couldn't actually verify the signature from the "%Gs%Gp" pair.
>
> I don't think that's insurmountable in the code. You'll have to jump
> through a few hoops to make sure you have the _original_ payload, but we
> obviously do have that data. However, it does feel a little weird to
> include content from a different encoding in the middle of the log
> output stream which claims to be i18n.logOutputEncoding.
>

Thanks for the feedback! This is an interesting conflict. If the user requests %Gp, the payload for the signature, they almost certainly do want it in the original encoding; if i18n.logOutputEncoding is something incompatible, whether explicitly or by default, that seems like an error. Not much we can do to reconcile the two requests (commit encoding vs output encoding) so seems reasonable to treat it as fatal.

Updated patch coming as soon as I work out Peff's aforementioned "few hoops" to get properly encoded data -- and also how to test success and failure!

Michał Górny· Dec 21, 2018, 13:52 UTC · re: John Passaro · lore

Re: [PATCH 0/4] Expose gpgsig in pretty-print

On Wed, 2018-12-19 at 00:59 -0500, John Passaro wrote:
Show 7 quoted lines
> On Fri, Dec 14, 2018 at 6:10 PM John Passaro wrote:
> > All seems to work fine when I treat %Gs as a detached signature.
> 
> In light of this, my best guess as to why the cleartext PGP message
> didn't verify properly is that the commit data normally doesn't end
> with \n, but as far as I can tell there's no way to express that in
> the cleartext format. I don't see a way around this.

You are most likely right. I've just skimmed through RFC 4880 and indeed it seems to rely on the newline encoding being quite normalized in the message.

Show 7 quoted lines
> However, as long
> as the following works, I think we have proof-of-concept that this
> enhancement allows you to play with signature data however you please
> without leaving it to git under the hood:
> 
> gpg --verify <(git show -s --format=format:%Gs commit) <(git show -s
> --format=format:%Gp commit)
That's a nice trick.  Thanks for the effort you're putting into this!
Show 35 quoted lines
> 
> On Mon, Dec 17, 2018 at 3:24 PM Jeff King <peff@peff.net> wrote:
> > 
> > On Fri, Dec 14, 2018 at 11:07:03AM -0500, John Passaro wrote:
> > 
> > > Then I might rename the other new placeholders too:
> > > 
> > > %Gs: signed commit signature (blank when unsigned)
> > > %Gp: signed commit payload (i.e. in practice minus the gpgsig header;
> > > also blank when unsigned as well)
> > 
> > One complication: the pretty-printing code sees the commit data in the
> > i18n.logOutputEncoding charset (utf8 by default). But the signature will
> > be over the raw commit data. That's also utf8 by default, but there may
> > be an encoding header indicating that it's something else. In that case,
> > you couldn't actually verify the signature from the "%Gs%Gp" pair.
> > 
> > I don't think that's insurmountable in the code. You'll have to jump
> > through a few hoops to make sure you have the _original_ payload, but we
> > obviously do have that data. However, it does feel a little weird to
> > include content from a different encoding in the middle of the log
> > output stream which claims to be i18n.logOutputEncoding.
> > 
> 
> Thanks for the feedback! This is an interesting conflict. If the user
> requests %Gp, the payload for the signature, they almost certainly do
> want it in the original encoding; if i18n.logOutputEncoding is
> something incompatible, whether explicitly or by default, that seems
> like an error. Not much we can do to reconcile the two requests
> (commit encoding vs output encoding) so seems reasonable to treat it
> as fatal.
> 
> Updated patch coming as soon as I work out Peff's aforementioned "few
> hoops" to get properly encoded data -- and also how to test success
> and failure!
-- 
Best regards,
Michał Górny
Junio C Hamano· Dec 15, 2018, 00:26 UTC · re: Michał Górny · lore

Re: [PATCH 0/4] Expose gpgsig in pretty-print

Michał Górny <mgorny@gentoo.org> writes:
Show 7 quoted lines
> Just a suggestion: since the raw signature is not very useful without
> the commit data to check it against, and the commit data is non-trivial
> to construct (requires mangling raw data anyway), maybe you could either
> add another placeholder to get the data for signature verification, or
> (alternatively or simultaneously) add a placeholder that prints both
> data and signature in the OpenPGP message format (i.e. something you can
> pass straight to 'gpg --verify').

Yeah, the last would be the most usable; anything short of that, I have to suspect that going from "cat-file commit", rather than using this new %Gsomething placeholder, would be more practical.

← back to recent threads