diff mbox series

[v1,1/2] t: increase test coverage of signature verification output

Message ID 20200304114804.19108-2-hji@dyntopia.com (mailing list archive)
State New, archived
Headers show
Series [v1,1/2] t: increase test coverage of signature verification output | expand

Commit Message

Hans Jerry Illikainen March 4, 2020, 11:48 a.m. UTC
There weren't any tests for unsuccessful signature verification of
signed merge tags shown in 'git log'.  There also weren't any tests for
the GPG output from 'git fmt-merge-msg'.  This was noticed while
investigating a buggy refactor that slipped through the test suite; see
commit 72b006f4bfd30b7c5037c163efaf279ab65bea9c.

This commit adds signature verification tests to the 'log' and
'fmt-merge-msg' builtins.

Thanks to Linus Torvalds for reporting and finding the (now reverted)
commit that introduced the regression.

Note that the "log --show-signature for merged tag with GPG failure"
test case is really hacky.  It relies on an implementation detail of
verify_signed_buffer() -- namely, it assumes that the signature is
written to a temporary file whose path is under TMPDIR.

The rationale for that test case is to check whether the code path that
yields the "No signature" message is reachable on failure.  The
functionality in log-tree.c that may show this message does some
pre-parsing of a possible signature that prevents the GPG interface from
being invoked if a signature is actually missing.  And I haven't been
able to construct a signature that both 1. satisfies that
pre-processing, and 2. causes GPG to fail without any sort of output on
stderr along the lines of "this is a bogus/corrupt/... signature" (the
"No signature" message should only be shown if GPG produce no output).

Signed-off-by: Hans Jerry Illikainen <hji@dyntopia.com>
---
 t/t4202-log.sh           | 106 +++++++++++++++++++++++++++++++++++++++
 t/t6200-fmt-merge-msg.sh |  23 +++++++++
 2 files changed, 129 insertions(+)

Comments

Johannes Schindelin March 14, 2020, 11:25 p.m. UTC | #1
Hi Hans,

I was wondering why your patches made the CI/PR builds fail on macOS and
Windows. This was a pretty hard thing to figure out, see below:

On Wed, 4 Mar 2020, Hans Jerry Illikainen wrote:

> diff --git a/t/t6200-fmt-merge-msg.sh b/t/t6200-fmt-merge-msg.sh
> index 8a72b4c43a..1922c1c42e 100755
> --- a/t/t6200-fmt-merge-msg.sh
> +++ b/t/t6200-fmt-merge-msg.sh
> @@ -6,6 +6,7 @@
>  test_description='fmt-merge-msg test'
>
>  . ./test-lib.sh
> +. "$TEST_DIRECTORY/lib-gpg.sh"
>
>  test_expect_success setup '
>  	echo one >one &&
> @@ -73,6 +74,10 @@ test_expect_success setup '
>  	apos="'\''"
>  '
>
> +test_expect_success GPG '

For developers who are very familiar with Git's test suite, it is really
hard to spot what is wrong with this line, and I was fooled for quite a
few days, too.

The thing is that this `GPG` looks like an innocent prereq and it is
correct: this test case really depends on GPG being present and working.

But it is not a prereq.

This is used as the _title_ of the test case. And on the macOS/Windows
agents, the GPG prereq is not met.

The reason is that this `test_expect_success` call only receives two
arguments, so it does not interpret the first one as a prereq. But I think
that this `GPG` was actually intended as a prereq, so the test case's
title is missing.

Could you kindly change this patch so that it adds a title, e.g. `set up
signed tag`?

That should let the CI build pass again.

Thank you,
Dscho

> +	git tag -s -m signed-tag-msg signed-good-tag left
> +'
> +
>  test_expect_success 'message for merging local branch' '
>  	echo "Merge branch ${apos}left${apos}" >expected &&
>
> @@ -83,6 +88,24 @@ test_expect_success 'message for merging local branch' '
>  	test_cmp expected actual
>  '
>
> +test_expect_success GPG 'message for merging local tag signed by good key' '
> +	git checkout master &&
> +	git fetch . signed-good-tag &&
> +	git fmt-merge-msg <.git/FETCH_HEAD >actual 2>&1 &&
> +	grep "^Merge tag ${apos}signed-good-tag${apos}" actual &&
> +	grep "^# gpg: Signature made" actual &&
> +	grep "^# gpg: Good signature from" actual
> +'
> +
> +test_expect_success GPG 'message for merging local tag signed by unknown key' '
> +	git checkout master &&
> +	git fetch . signed-good-tag &&
> +	GNUPGHOME=. git fmt-merge-msg <.git/FETCH_HEAD >actual 2>&1 &&
> +	grep "^Merge tag ${apos}signed-good-tag${apos}" actual &&
> +	grep "^# gpg: Signature made" actual &&
> +	grep "^# gpg: Can${apos}t check signature: \(public key not found\|No public key\)" actual
> +'
> +
>  test_expect_success 'message for merging external branch' '
>  	echo "Merge branch ${apos}left${apos} of $(pwd)" >expected &&
>
> --
> 2.25.1.709.g558d21736a
>
>
>
Junio C Hamano March 15, 2020, 4:40 p.m. UTC | #2
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Hi Hans,
>
> I was wondering why your patches made the CI/PR builds fail on macOS and
> Windows. This was a pretty hard thing to figure out, see below:
>
> On Wed, 4 Mar 2020, Hans Jerry Illikainen wrote:
>
>> diff --git a/t/t6200-fmt-merge-msg.sh b/t/t6200-fmt-merge-msg.sh
>> index 8a72b4c43a..1922c1c42e 100755
>> --- a/t/t6200-fmt-merge-msg.sh
>> +++ b/t/t6200-fmt-merge-msg.sh
>> @@ -6,6 +6,7 @@
>>  test_description='fmt-merge-msg test'
>>
>>  . ./test-lib.sh
>> +. "$TEST_DIRECTORY/lib-gpg.sh"
>>
>>  test_expect_success setup '
>>  	echo one >one &&
>> @@ -73,6 +74,10 @@ test_expect_success setup '
>>  	apos="'\''"
>>  '
>>
>> +test_expect_success GPG '

Ahh, thanks for a pair of sharp eyes.
diff mbox series

Patch

diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 0f766ba65f..1783c6f7d9 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -1607,6 +1607,67 @@  test_expect_success GPG 'log --graph --show-signature for merged tag' '
 	grep "^| | gpg: Good signature" actual
 '
 
+test_expect_success GPG 'log --graph --show-signature for merged tag with missing key' '
+	test_when_finished "git reset --hard && git checkout master" &&
+	git checkout -b plain-nokey master &&
+	echo aaa >bar &&
+	git add bar &&
+	git commit -m bar_commit &&
+	git checkout -b tagged-nokey master &&
+	echo bbb >baz &&
+	git add baz &&
+	git commit -m baz_commit &&
+	git tag -s -m signed_tag_msg signed_tag_nokey &&
+	git checkout plain-nokey &&
+	git merge --no-ff -m msg signed_tag_nokey &&
+	GNUPGHOME=. git log --graph --show-signature -n1 plain-nokey >actual &&
+	grep "^|\\\  merged tag" actual &&
+	grep "^| | gpg: Signature made" actual &&
+	grep "^| | gpg: Can'"'"'t check signature: \(public key not found\|No public key\)" actual
+'
+
+test_expect_success GPG 'log --graph --show-signature for merged tag with bad signature' '
+	test_when_finished "git reset --hard && git checkout master" &&
+	git checkout -b plain-bad master &&
+	echo aaa >bar &&
+	git add bar &&
+	git commit -m bar_commit &&
+	git checkout -b tagged-bad master &&
+	echo bbb >baz &&
+	git add baz &&
+	git commit -m baz_commit &&
+	git tag -s -m signed_tag_msg signed_tag_bad &&
+	git cat-file tag signed_tag_bad >raw &&
+	sed -e "s/signed_tag_msg/forged/" raw >forged &&
+	git hash-object -w -t tag forged >forged.tag &&
+	git checkout plain-bad &&
+	git merge --no-ff -m msg "$(cat forged.tag)" &&
+	git log --graph --show-signature -n1 plain-bad >actual &&
+	grep "^|\\\  merged tag" actual &&
+	grep "^| | gpg: Signature made" actual &&
+	grep "^| | gpg: BAD signature from" actual
+'
+
+test_expect_success GPG 'log --show-signature for merged tag with GPG failure' '
+	test_when_finished "git reset --hard && git checkout master" &&
+	git checkout -b plain-fail master &&
+	echo aaa >bar &&
+	git add bar &&
+	git commit -m bar_commit &&
+	git checkout -b tagged-fail master &&
+	echo bbb >baz &&
+	git add baz &&
+	git commit -m baz_commit &&
+	git tag -s -m signed_tag_msg signed_tag_fail &&
+	git checkout plain-fail &&
+	git merge --no-ff -m msg signed_tag_fail &&
+	TMPDIR="$(pwd)/bogus" git log --show-signature -n1 plain-fail >actual &&
+	grep "^merged tag" actual &&
+	grep "^No signature" actual &&
+	! grep "^gpg: Signature made" actual
+'
+
+
 test_expect_success GPG 'log --graph --show-signature for merged tag in shallow clone' '
 	test_when_finished "git reset --hard && git checkout master" &&
 	git checkout -b plain-shallow master &&
@@ -1648,6 +1709,51 @@  test_expect_success GPGSM 'log --graph --show-signature for merged tag x509' '
 	grep "^| | gpgsm: Good signature" actual
 '
 
+test_expect_success GPGSM 'log --graph --show-signature for merged tag x509 missing key' '
+	test_when_finished "git reset --hard && git checkout master" &&
+	test_config gpg.format x509 &&
+	test_config user.signingkey $GIT_COMMITTER_EMAIL &&
+	git checkout -b plain-x509-nokey master &&
+	echo aaa >bar &&
+	git add bar &&
+	git commit -m bar_commit &&
+	git checkout -b tagged-x509-nokey master &&
+	echo bbb >baz &&
+	git add baz &&
+	git commit -m baz_commit &&
+	git tag -s -m signed_tag_msg signed_tag_x509_nokey &&
+	git checkout plain-x509-nokey &&
+	git merge --no-ff -m msg signed_tag_x509_nokey &&
+	GNUPGHOME=. git log --graph --show-signature -n1 plain-x509-nokey >actual &&
+	grep "^|\\\  merged tag" actual &&
+	grep "^| | gpgsm: certificate not found" actual
+'
+
+test_expect_success GPGSM 'log --graph --show-signature for merged tag x509 bad signature' '
+	test_when_finished "git reset --hard && git checkout master" &&
+	test_config gpg.format x509 &&
+	test_config user.signingkey $GIT_COMMITTER_EMAIL &&
+	git checkout -b plain-x509-bad master &&
+	echo aaa >bar &&
+	git add bar &&
+	git commit -m bar_commit &&
+	git checkout -b tagged-x509-bad master &&
+	echo bbb >baz &&
+	git add baz &&
+	git commit -m baz_commit &&
+	git tag -s -m signed_tag_msg signed_tag_x509_bad &&
+	git cat-file tag signed_tag_x509_bad >raw &&
+	sed -e "s/signed_tag_msg/forged/" raw >forged &&
+	git hash-object -w -t tag forged >forged.tag &&
+	git checkout plain-x509-bad &&
+	git merge --no-ff -m msg "$(cat forged.tag)" &&
+	git log --graph --show-signature -n1 plain-x509-bad >actual &&
+	grep "^|\\\  merged tag" actual &&
+	grep "^| | gpgsm: Signature made" actual &&
+	grep "^| | gpgsm: invalid signature" actual
+'
+
+
 test_expect_success GPG '--no-show-signature overrides --show-signature' '
 	git log -1 --show-signature --no-show-signature signed >actual &&
 	! grep "^gpg:" actual
diff --git a/t/t6200-fmt-merge-msg.sh b/t/t6200-fmt-merge-msg.sh
index 8a72b4c43a..1922c1c42e 100755
--- a/t/t6200-fmt-merge-msg.sh
+++ b/t/t6200-fmt-merge-msg.sh
@@ -6,6 +6,7 @@ 
 test_description='fmt-merge-msg test'
 
 . ./test-lib.sh
+. "$TEST_DIRECTORY/lib-gpg.sh"
 
 test_expect_success setup '
 	echo one >one &&
@@ -73,6 +74,10 @@  test_expect_success setup '
 	apos="'\''"
 '
 
+test_expect_success GPG '
+	git tag -s -m signed-tag-msg signed-good-tag left
+'
+
 test_expect_success 'message for merging local branch' '
 	echo "Merge branch ${apos}left${apos}" >expected &&
 
@@ -83,6 +88,24 @@  test_expect_success 'message for merging local branch' '
 	test_cmp expected actual
 '
 
+test_expect_success GPG 'message for merging local tag signed by good key' '
+	git checkout master &&
+	git fetch . signed-good-tag &&
+	git fmt-merge-msg <.git/FETCH_HEAD >actual 2>&1 &&
+	grep "^Merge tag ${apos}signed-good-tag${apos}" actual &&
+	grep "^# gpg: Signature made" actual &&
+	grep "^# gpg: Good signature from" actual
+'
+
+test_expect_success GPG 'message for merging local tag signed by unknown key' '
+	git checkout master &&
+	git fetch . signed-good-tag &&
+	GNUPGHOME=. git fmt-merge-msg <.git/FETCH_HEAD >actual 2>&1 &&
+	grep "^Merge tag ${apos}signed-good-tag${apos}" actual &&
+	grep "^# gpg: Signature made" actual &&
+	grep "^# gpg: Can${apos}t check signature: \(public key not found\|No public key\)" actual
+'
+
 test_expect_success 'message for merging external branch' '
 	echo "Merge branch ${apos}left${apos} of $(pwd)" >expected &&