[v8,9/9] ssh signing: test that gpg fails for unknown keys

Commit Message

Fabian Stelzer Sept. 10, 2021, 8:07 p.m. UTC

From: Fabian Stelzer <[e-mail]>

Test that verify-commit/tag will fail when a gpg key is completely
unknown. To do this we have to generate a key, use it for a signature
and delete it from our keyring aferwards completely.

Signed-off-by: Fabian Stelzer <[e-mail]>
---
 t/t7510-signed-commit.sh | 29 ++++++++++++++++++++++++++++-
 1 file changed, 28 insertions(+), 1 deletion(-)

Comments

Ævar Arnfjörð Bjarmason Dec. 22, 2021, 3:18 a.m. UTC | #1

On Fri, Sep 10 2021, Fabian Stelzer via GitGitGadget wrote:

> From: Fabian Stelzer <[e-mail]>
>
> Test that verify-commit/tag will fail when a gpg key is completely
> unknown. To do this we have to generate a key, use it for a signature
> and delete it from our keyring aferwards completely.
>
> Signed-off-by: Fabian Stelzer <[e-mail]>
> ---
>  t/t7510-signed-commit.sh | 29 ++++++++++++++++++++++++++++-
>  1 file changed, 28 insertions(+), 1 deletion(-)
>
> diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
> index 8df5a74f1db..d65a0171f29 100755
> --- a/t/t7510-signed-commit.sh
> +++ b/t/t7510-signed-commit.sh
> @@ -71,7 +71,25 @@ test_expect_success GPG 'create signed commits' '
>  	git tag eleventh-signed $(cat oid) &&
>  	echo 12 | git commit-tree --gpg-sign=B7227189 HEAD^{tree} >oid &&
>  	test_line_count = 1 oid &&
> -	git tag twelfth-signed-alt $(cat oid)
> +	git tag twelfth-signed-alt $(cat oid) &&
> +
> +	cat >keydetails <<-\EOF &&
> +	Key-Type: RSA
> +	Key-Length: 2048
> +	Subkey-Type: RSA
> +	Subkey-Length: 2048
> +	Name-Real: Unknown User
> +	Name-Email: [e-mail]
> +	Expire-Date: 0
> +	%no-ask-passphrase
> +	%no-protection
> +	EOF
> +	gpg --batch --gen-key keydetails &&
> +	echo 13 >file && git commit -a -S"[e-mail]" -m thirteenth &&
> +	git tag thirteenth-signed &&
> +	DELETE_FINGERPRINT=$(gpg -K --with-colons --fingerprint --batch [e-mail] | grep "^fpr" | head -n 1 | awk -F ":" "{print \$10;}") &&
> +	gpg --batch --yes --delete-secret-keys $DELETE_FINGERPRINT &&
> +	gpg --batch --yes --delete-keys [e-mail]
>  '
>  
>  test_expect_success GPG 'verify and show signatures' '
> @@ -110,6 +128,13 @@ test_expect_success GPG 'verify and show signatures' '
>  	)
>  '
>  
> +test_expect_success GPG 'verify-commit exits failure on unknown signature' '
> +	test_must_fail git verify-commit thirteenth-signed 2>actual &&
> +	! grep "Good signature from" actual &&
> +	! grep "BAD signature from" actual &&
> +	grep -q -F -e "No public key" -e "public key not found" actual
> +'
> +
>  test_expect_success GPG 'verify-commit exits success on untrusted signature' '
>  	git verify-commit eighth-signed-alt 2>actual &&
>  	grep "Good signature from" actual &&
> @@ -338,6 +363,8 @@ test_expect_success GPG 'show double signature with custom format' '
>  '
>  
>  
> +# NEEDSWORK: This test relies on the test_tick commit/author dates from the first
> +# 'create signed commits' test even though it creates its own
>  test_expect_success GPG 'verify-commit verifies multiply signed commits' '
>  	git init multiply-signed &&
>  	cd multiply-signed &&

The t7510-signed-commit.sh script hangs on startup with this change, and
with -vx we show:
    
    [...]
    ++ git tag twelfth-signed-alt 17f06d503ee50df92746c17f6cced6feb5940cf5
    ++ cat
    ++ gpg --batch --gen-key keydetails
    gpg: skipping control `%no-protection' ()

This is on a CentOS 7.9 box on the GCC Farm:
    
    [avar@gcc135 t]$ uname -a ; gpg --version
    Linux gcc135.osuosl.org 4.18.0-80.7.2.el7.ppc64le #1 SMP Thu Sep 12 15:45:05 UTC 2019 ppc64le ppc64le ppc64le GNU/Linux
    gpg (GnuPG) 2.0.22
    libgcrypt 1.5.3
    Copyright (C) 2013 Free Software Foundation, Inc.
    License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
    This is free software: you are free to change and redistribute it.
    There is NO WARRANTY, to the extent permitted by law.
    
    Home: ~/.gnupg
    Supported algorithms:
    Pubkey: RSA, ?, ?, ELG, DSA
    Cipher: IDEA, 3DES, CAST5, BLOWFISH, AES, AES192, AES256, TWOFISH,
            CAMELLIA128, CAMELLIA192, CAMELLIA256
    Hash: MD5, SHA1, RIPEMD160, SHA256, SHA384, SHA512, SHA224
    Compression: Uncompressed, ZIP, ZLIB, BZIP2

Fabian Stelzer Dec. 22, 2021, 10:13 a.m. UTC | #2

On 22.12.2021 04:18, Ævar Arnfjörð Bjarmason wrote:
>
>On Fri, Sep 10 2021, Fabian Stelzer via GitGitGadget wrote:
>
>> From: Fabian Stelzer <[e-mail]>
>>
>> Test that verify-commit/tag will fail when a gpg key is completely
>> unknown. To do this we have to generate a key, use it for a signature
>> and delete it from our keyring aferwards completely.
>>
>> Signed-off-by: Fabian Stelzer <[e-mail]>
>> +
>> +	cat >keydetails <<-\EOF &&
>> +	Key-Type: RSA
>> +	Key-Length: 2048
>> +	Subkey-Type: RSA
>> +	Subkey-Length: 2048
>> +	Name-Real: Unknown User
>> +	Name-Email: [e-mail]
>> +	Expire-Date: 0
>> +	%no-ask-passphrase
>> +	%no-protection
>> +	EOF
>> +	gpg --batch --gen-key keydetails &&
>>
>The t7510-signed-commit.sh script hangs on startup with this change, and
>with -vx we show:
>
>    [...]
>    ++ git tag twelfth-signed-alt 17f06d503ee50df92746c17f6cced6feb5940cf5
>    ++ cat
>    ++ gpg --batch --gen-key keydetails
>    gpg: skipping control `%no-protection' ()
>
>This is on a CentOS 7.9 box on the GCC Farm:
>
>    [avar@gcc135 t]$ uname -a ; gpg --version
>    Linux gcc135.osuosl.org 4.18.0-80.7.2.el7.ppc64le #1 SMP Thu Sep 12 15:45:05 UTC 2019 ppc64le ppc64le ppc64le GNU/Linux
>    gpg (GnuPG) 2.0.22
>    libgcrypt 1.5.3
>    Copyright (C) 2013 Free Software Foundation, Inc.
>    License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
>    This is free software: you are free to change and redistribute it.
>    There is NO WARRANTY, to the extent permitted by law.
>
>    Home: ~/.gnupg
>    Supported algorithms:
>    Pubkey: RSA, ?, ?, ELG, DSA
>    Cipher: IDEA, 3DES, CAST5, BLOWFISH, AES, AES192, AES256, TWOFISH,
>            CAMELLIA128, CAMELLIA192, CAMELLIA256
>    Hash: MD5, SHA1, RIPEMD160, SHA256, SHA384, SHA512, SHA224
>    Compression: Uncompressed, ZIP, ZLIB, BZIP2

Hm. I have an identical centos 7.9 installation (same versions/features) and 
the key is generated without issues. Does the VM maybe have not enough 
entropy for generating a gpg key?
Otherwise we could of course pre-generate the key and commit it. I'm usually 
not a fan of this since over time it can become unclear how it was generated 
or if the committed version still matches what would be generated today.
But of course I don't want to slow down CI with rsa key generation stuff :/
If missing entropy is the problem, then maybe CI could benefit from 
something like haveged in general (other tests might want more entropy too).

brian m. carlson Dec. 22, 2021, 3:58 p.m. UTC | #3

On 2021-12-22 at 10:13:26, Fabian Stelzer wrote:
> Hm. I have an identical centos 7.9 installation (same versions/features) and
> the key is generated without issues. Does the VM maybe have not enough
> entropy for generating a gpg key?
> Otherwise we could of course pre-generate the key and commit it. I'm usually
> not a fan of this since over time it can become unclear how it was generated
> or if the committed version still matches what would be generated today.
> But of course I don't want to slow down CI with rsa key generation stuff :/
> If missing entropy is the problem, then maybe CI could benefit from
> something like haveged in general (other tests might want more entropy too).

GnuPG is notorious for using /dev/random for generating keys, so yes,
this is likely to block in a variety of situations.  We don't see this
on newer systems because they've replaced the blocking /dev/random with
a non-blocking one except for when the CSPRNG hasn't been seeded at
least once.

The problem isn't lack of entropy, but the fact that there's no reason
to use /dev/random since /dev/urandom is suitable for all cryptographic
needs once initialized.  On modern versions of Linux, one just uses
getrandom(2), which deals with the uninitialized case and otherwise
doesn't block.  However, CentOS 7 is old.

Ævar Arnfjörð Bjarmason Dec. 26, 2021, 10:53 p.m. UTC | #4

On Wed, Dec 22 2021, Fabian Stelzer wrote:

> On 22.12.2021 04:18, Ævar Arnfjörð Bjarmason wrote:
>>
>>On Fri, Sep 10 2021, Fabian Stelzer via GitGitGadget wrote:
>>
>>> From: Fabian Stelzer <[e-mail]>
>>>
>>> Test that verify-commit/tag will fail when a gpg key is completely
>>> unknown. To do this we have to generate a key, use it for a signature
>>> and delete it from our keyring aferwards completely.
>>>
>>> Signed-off-by: Fabian Stelzer <[e-mail]>
>>> +
>>> +	cat >keydetails <<-\EOF &&
>>> +	Key-Type: RSA
>>> +	Key-Length: 2048
>>> +	Subkey-Type: RSA
>>> +	Subkey-Length: 2048
>>> +	Name-Real: Unknown User
>>> +	Name-Email: [e-mail]
>>> +	Expire-Date: 0
>>> +	%no-ask-passphrase
>>> +	%no-protection
>>> +	EOF
>>> +	gpg --batch --gen-key keydetails &&
>>>
>>The t7510-signed-commit.sh script hangs on startup with this change, and
>>with -vx we show:
>>
>>    [...]
>>    ++ git tag twelfth-signed-alt 17f06d503ee50df92746c17f6cced6feb5940cf5
>>    ++ cat
>>    ++ gpg --batch --gen-key keydetails
>>    gpg: skipping control `%no-protection' ()
>>
>>This is on a CentOS 7.9 box on the GCC Farm:
>>
>>    [avar@gcc135 t]$ uname -a ; gpg --version
>>    Linux gcc135.osuosl.org 4.18.0-80.7.2.el7.ppc64le #1 SMP Thu Sep 12 15:45:05 UTC 2019 ppc64le ppc64le ppc64le GNU/Linux
>>    gpg (GnuPG) 2.0.22
>>    libgcrypt 1.5.3
>>    Copyright (C) 2013 Free Software Foundation, Inc.
>>    License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
>>    This is free software: you are free to change and redistribute it.
>>    There is NO WARRANTY, to the extent permitted by law.
>>
>>    Home: ~/.gnupg
>>    Supported algorithms:
>>    Pubkey: RSA, ?, ?, ELG, DSA
>>    Cipher: IDEA, 3DES, CAST5, BLOWFISH, AES, AES192, AES256, TWOFISH,
>>            CAMELLIA128, CAMELLIA192, CAMELLIA256
>>    Hash: MD5, SHA1, RIPEMD160, SHA256, SHA384, SHA512, SHA224
>>    Compression: Uncompressed, ZIP, ZLIB, BZIP2
>
> Hm. I have an identical centos 7.9 installation (same
> versions/features) and the key is generated without issues. Does the
> VM maybe have not enough entropy for generating a gpg key?
> Otherwise we could of course pre-generate the key and commit it. I'm
> usually not a fan of this since over time it can become unclear how it
> was generated or if the committed version still matches what would be
> generated today.
> But of course I don't want to slow down CI with rsa key generation stuff :/
> If missing entropy is the problem, then maybe CI could benefit from
> something like haveged in general (other tests might want more entropy
> too).

Late reply. It's not a VM, but yes. I've confirmed that it's due to
/dev/random hanging.

I don't understand why we need to generate a key at all.

It looks like your 1bfb57f642d (ssh signing: test that gpg fails for
unknown keys, 2021-09-10) is just trying to test the case where we sign
with a key, and then don't have that key anymore.

The below POC patch seems to work just as well, and will succeed with:

    ./t7510-signed-commit.sh --run=1,3

Of course a lot of other tests now fail, because they relied on the
[e-mail] key.

But that seems easily solved by just moving this test to its own file,
or deleting/re-importing the key for just that test or whatever. If we
truly need yet another key why are we making it on the fly instead of
adding it to t/lib-gpg/keyring.gpg like the others?

diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
index 9882b69ae29..eec2a045cbc 100755
--- a/t/t7510-signed-commit.sh
+++ b/t/t7510-signed-commit.sh
@@ -73,23 +73,11 @@ test_expect_success GPG 'create signed commits' '
 	test_line_count = 1 oid &&
 	git tag twelfth-signed-alt $(cat oid) &&
 
-	cat >keydetails <<-\EOF &&
-	Key-Type: RSA
-	Key-Length: 2048
-	Subkey-Type: RSA
-	Subkey-Length: 2048
-	Name-Real: Unknown User
-	Name-Email: [e-mail]
-	Expire-Date: 0
-	%no-ask-passphrase
-	%no-protection
-	EOF
-	gpg --batch --gen-key keydetails &&
-	echo 13 >file && git commit -a -S"[e-mail]" -m thirteenth &&
+	echo 13 >file && git commit -a -S"[e-mail]" -m thirteenth &&
 	git tag thirteenth-signed &&
-	DELETE_FINGERPRINT=$(gpg -K --with-colons --fingerprint --batch [e-mail] | grep "^fpr" | head -n 1 | awk -F ":" "{print \$10;}") &&
+	DELETE_FINGERPRINT=$(gpg -K --with-colons --fingerprint --batch [e-mail] | grep "^fpr" | head -n 1 | awk -F ":" "{print \$10;}") &&
 	gpg --batch --yes --delete-secret-keys $DELETE_FINGERPRINT &&
-	gpg --batch --yes --delete-keys [e-mail]
+	gpg --batch --yes --delete-keys [e-mail]
 '
 
 test_expect_success GPG 'verify and show signatures' '

Fabian Stelzer Dec. 30, 2021, 11:10 a.m. UTC | #5

On 26.12.2021 23:53, Ævar Arnfjörð Bjarmason wrote:
>>
>> Hm. I have an identical centos 7.9 installation (same
>> versions/features) and the key is generated without issues. Does the
>> VM maybe have not enough entropy for generating a gpg key?
>> Otherwise we could of course pre-generate the key and commit it. I'm
>> usually not a fan of this since over time it can become unclear how it
>> was generated or if the committed version still matches what would be
>> generated today.
>> But of course I don't want to slow down CI with rsa key generation stuff :/
>> If missing entropy is the problem, then maybe CI could benefit from
>> something like haveged in general (other tests might want more entropy
>> too).
>
>Late reply. It's not a VM, but yes. I've confirmed that it's due to
>/dev/random hanging.
>
>I don't understand why we need to generate a key at all.

You are right, we don't need to. I initially toyed with the GPG commands to 
disable/export/reimport a key but without success (I'm not terribly familiar 
with GPG though). 

>
>It looks like your 1bfb57f642d (ssh signing: test that gpg fails for
>unknown keys, 2021-09-10) is just trying to test the case where we sign
>with a key, and then don't have that key anymore.
>

It tests verifying a commit for which the key is not in our keyring at all.  
All the other tests only use present keys (with varying trust levels) or 
completely unsigned commits for the failure check. 

I think we could do the following though and simply point git to an empty 
keyring to be able to verify this:

diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
index 9882b69ae2..2d38580847 100755
--- a/t/t7510-signed-commit.sh
+++ b/t/t7510-signed-commit.sh
@@ -71,25 +71,7 @@ test_expect_success GPG 'create signed commits' '
  	git tag eleventh-signed $(cat oid) &&
  	echo 12 | git commit-tree --gpg-sign=B7227189 HEAD^{tree} >oid &&
  	test_line_count = 1 oid &&
-	git tag twelfth-signed-alt $(cat oid) &&
-
-	cat >keydetails <<-\EOF &&
-	Key-Type: RSA
-	Key-Length: 2048
-	Subkey-Type: RSA
-	Subkey-Length: 2048
-	Name-Real: Unknown User
-	Name-Email: [e-mail]
-	Expire-Date: 0
-	%no-ask-passphrase
-	%no-protection
-	EOF
-	gpg --batch --gen-key keydetails &&
-	echo 13 >file && git commit -a -S"[e-mail]" -m thirteenth &&
-	git tag thirteenth-signed &&
-	DELETE_FINGERPRINT=$(gpg -K --with-colons --fingerprint --batch [e-mail] | grep "^fpr" | head -n 1 | awk -F ":" "{print \$10;}") &&
-	gpg --batch --yes --delete-secret-keys $DELETE_FINGERPRINT &&
-	gpg --batch --yes --delete-keys [e-mail]
+	git tag twelfth-signed-alt $(cat oid)
  '
  
  test_expect_success GPG 'verify and show signatures' '
@@ -129,7 +111,7 @@ test_expect_success GPG 'verify and show signatures' '
  '
  
  test_expect_success GPG 'verify-commit exits failure on unknown signature' '
-	test_must_fail git verify-commit thirteenth-signed 2>actual &&
+	GNUPGHOME=./empty_home test_must_fail git verify-commit initial 2>actual &&
  	! grep "Good signature from" actual &&
  	! grep "BAD signature from" actual &&
  	grep -q -F -e "No public key" -e "public key not found" actual

Patch

diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
index 8df5a74f1db..d65a0171f29 100755
--- a/t/t7510-signed-commit.sh
+++ b/t/t7510-signed-commit.sh
@@ -71,7 +71,25 @@  test_expect_success GPG 'create signed commits' '
 	git tag eleventh-signed $(cat oid) &&
 	echo 12 | git commit-tree --gpg-sign=B7227189 HEAD^{tree} >oid &&
 	test_line_count = 1 oid &&
-	git tag twelfth-signed-alt $(cat oid)
+	git tag twelfth-signed-alt $(cat oid) &&
+
+	cat >keydetails <<-\EOF &&
+	Key-Type: RSA
+	Key-Length: 2048
+	Subkey-Type: RSA
+	Subkey-Length: 2048
+	Name-Real: Unknown User
+	Name-Email: [e-mail]
+	Expire-Date: 0
+	%no-ask-passphrase
+	%no-protection
+	EOF
+	gpg --batch --gen-key keydetails &&
+	echo 13 >file && git commit -a -S"[e-mail]" -m thirteenth &&
+	git tag thirteenth-signed &&
+	DELETE_FINGERPRINT=$(gpg -K --with-colons --fingerprint --batch [e-mail] | grep "^fpr" | head -n 1 | awk -F ":" "{print \$10;}") &&
+	gpg --batch --yes --delete-secret-keys $DELETE_FINGERPRINT &&
+	gpg --batch --yes --delete-keys [e-mail]
 '
 
 test_expect_success GPG 'verify and show signatures' '
@@ -110,6 +128,13 @@  test_expect_success GPG 'verify and show signatures' '
 	)
 '
 
+test_expect_success GPG 'verify-commit exits failure on unknown signature' '
+	test_must_fail git verify-commit thirteenth-signed 2>actual &&
+	! grep "Good signature from" actual &&
+	! grep "BAD signature from" actual &&
+	grep -q -F -e "No public key" -e "public key not found" actual
+'
+
 test_expect_success GPG 'verify-commit exits success on untrusted signature' '
 	git verify-commit eighth-signed-alt 2>actual &&
 	grep "Good signature from" actual &&
@@ -338,6 +363,8 @@  test_expect_success GPG 'show double signature with custom format' '
 '
 
 
+# NEEDSWORK: This test relies on the test_tick commit/author dates from the first
+# 'create signed commits' test even though it creates its own
 test_expect_success GPG 'verify-commit verifies multiply signed commits' '
 	git init multiply-signed &&
 	cd multiply-signed &&