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

[John Passaro <john.a.passaro@gmail.com>]

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