mbox series

[0/4] Expose gpgsig in pretty-print

Message ID 20181213212256.48122-1-john.a.passaro@gmail.com (mailing list archive)
Headers show
Series Expose gpgsig in pretty-print | expand

Message

John Passaro Dec. 13, 2018, 9:22 p.m. UTC
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

Comments

Michał Górny Dec. 14, 2018, 4:11 a.m. UTC | #1
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').
John Passaro Dec. 14, 2018, 4:07 p.m. UTC | #2
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
Michał Górny Dec. 14, 2018, 4:48 p.m. UTC | #3
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.
John Passaro Dec. 14, 2018, 11:10 p.m. UTC | #4
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
John Passaro Dec. 14, 2018, 11:13 p.m. UTC | #5
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
Junio C Hamano Dec. 15, 2018, 12:26 a.m. UTC | #6
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.
Jeff King Dec. 17, 2018, 8:24 p.m. UTC | #7
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
John Passaro Dec. 19, 2018, 5:59 a.m. UTC | #8
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!
Michał Górny Dec. 21, 2018, 1:52 p.m. UTC | #9
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!