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

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

[John Passaro]

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

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

[John Passaro]

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(-)

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

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

[John Passaro]

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(-)

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

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

[John Passaro]

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(-)

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

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

[John Passaro]

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(-)

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

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

[Michał Górny]

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

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

-- 
Best regards,
Michał Górny

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 963 bytes --]

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

[John Passaro]

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!

--
JP

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

[Michał Górny]

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

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.

-- 
Best regards,
Michał Górny

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 963 bytes --]

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

[John Passaro]

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

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

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

[John Passaro]

On Fri, Dec 14, 2018 at 6:10 PM John Passaro <john.a.passaro@gmail.com> wrote:
>
> 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.
>
> 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

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

[Junio C Hamano]

Michał Górny <mgorny@gentoo.org> writes:

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

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

[Jeff King]

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.

-Peff

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

[John Passaro]

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:
>
> 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!

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

[Michał Górny]

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

On Wed, 2018-12-19 at 00:59 -0500, John Passaro wrote:
> 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.

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

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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 963 bytes --]