[0/1] Limit search for primary key fingerprint

Message

Hans Jerry Illikainen Nov. 16, 2019, 6:06 p.m. UTC

As part of implementing signature verification for git clone, I decided
to refactor/unify the code for commit and merge verification to make it
reusable during clones.

This lead me to discover that git requires merge signatures to be
trusted (as opposed to TRUST_UNKNOWN or TRUST_NEVER).  This is unlike
the behavior of verify-tag and verify-commit.

So, I figured that I'd make the minimum trust level configurable to make
the behavior of merge/commit/tag consistent.  And while doing so, I
noticed that parse_gpg_output() in gpg-interface.c assumes that the
VALIDSIG status line has a field with a fingerprint for the primary key;
but that is only the case for OpenPGP signatures [1].

The consequence of that assumption is that the subsequent status line is
interpreted as the primary fingerprint for X509 signatures.  I'm not
sure if the order is hardcoded in GnuPG, but in my testing the TRUST_
status line always came after VALIDSIG -- and that breaks the config
option to set a minimum trust level (not part of this patch):

,----
| $ git log -n1 --format="primary key: %GP" signed-x509
| gpgsm: Signature made 2019-11-16 14:13:09 using certificate ID 0xFA23FD65
| gpgsm: Good signature from "/CN=C O Mitter/O=Example/SN=C O/GN=Mitter"
| gpgsm:                 aka "[e-mail]"
| primary key: TRUST_FULLY 0 shell
`----

[1]: https://git.gnupg.org/cgi-bin/gitweb.cgi?p=gnupg.git;a=blob;f=doc/DETAILS


Hans Jerry Illikainen (1):
  gpg-interface: limit search for primary key fingerprint

 gpg-interface.c | 20 +++++++++++++++-----
 t/t4202-log.sh  |  6 ++++++
 2 files changed, 21 insertions(+), 5 deletions(-)

--
2.24.0.156.g69483321b9.dirty

Comments

Jonathan Nieder Nov. 16, 2019, 7:49 p.m. UTC | #1

Hi,

Hans Jerry Illikainen wrote:

> As part of implementing signature verification for git clone, I decided
> to refactor/unify the code for commit and merge verification to make it
> reusable during clones.

Thanks for writing this.

Most of the text in this cover letter would be useful to have in the
commit message.  From the commit message alone, I could see that you
were fixing a bug, but I could not see the motivation or workflow it
is part of.  If I were to later discover an issue triggered by this
commit, I wouldn't have enough information to weigh tradeoffs about
the right way to address such an issue.

Thanks and hope that helps,
Jonathan

> This lead me to discover that git requires merge signatures to be
> trusted (as opposed to TRUST_UNKNOWN or TRUST_NEVER).  This is unlike
> the behavior of verify-tag and verify-commit.
>
> So, I figured that I'd make the minimum trust level configurable to make
> the behavior of merge/commit/tag consistent.  And while doing so, I
> noticed that parse_gpg_output() in gpg-interface.c assumes that the
> VALIDSIG status line has a field with a fingerprint for the primary key;
> but that is only the case for OpenPGP signatures [1].
>
> The consequence of that assumption is that the subsequent status line is
> interpreted as the primary fingerprint for X509 signatures.  I'm not
> sure if the order is hardcoded in GnuPG, but in my testing the TRUST_
> status line always came after VALIDSIG -- and that breaks the config
> option to set a minimum trust level (not part of this patch):
>
> ,----
> | $ git log -n1 --format="primary key: %GP" signed-x509
> | gpgsm: Signature made 2019-11-16 14:13:09 using certificate ID 0xFA23FD65
> | gpgsm: Good signature from "/CN=C O Mitter/O=Example/SN=C O/GN=Mitter"
> | gpgsm:                 aka "[e-mail]"
> | primary key: TRUST_FULLY 0 shell
> `----
>
> [1]: https://git.gnupg.org/cgi-bin/gitweb.cgi?p=gnupg.git;a=blob;f=doc/DETAILS

Junio C Hamano Nov. 18, 2019, 4:45 a.m. UTC | #2

Jonathan Nieder <[e-mail]> writes:

> Hans Jerry Illikainen wrote:
>
>> As part of implementing signature verification for git clone, I decided
>> to refactor/unify the code for commit and merge verification to make it
>> reusable during clones.
>
> Thanks for writing this.
>
> Most of the text in this cover letter would be useful to have in the
> commit message.  From the commit message alone, I could see that you
> were fixing a bug, but I could not see the motivation or workflow it
> is part of.  If I were to later discover an issue triggered by this
> commit, I wouldn't have enough information to weigh tradeoffs about
> the right way to address such an issue.

After reading the proposed log message of [PATCH v1 1/1], I have to
disagree.  It does not matter if we will later see new code in the
clone codepath that would use the gpg-interface API.  Whether it
happens or not, this change to look for the key fingerprint only on
the same line is something we should consider independently.

On the other hand, why the author of this change thought that it may
be necessary thing to do is an excellent material to tell the story
behind the patch in the cover letter.

Thanks.