git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
* [PATCH] t/gpg: simplify test for unknown key
@ 2022-01-07  9:14 Fabian Stelzer
  2022-01-07 21:43 ` Junio C Hamano
                   ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: Fabian Stelzer @ 2022-01-07  9:14 UTC (permalink / raw)
  To: git
  Cc: Fabian Stelzer, Han-Wen Nienhuys, brian m. carlson,
	Randall S. Becker, Bagas Sanjaya, Hans Jerry Illikainen,
	Felipe Contreras, Eric Sunshine, Gwyneth Morgan, Jonathan Tan,
	Josh Steadmon, Ævar Arnfjörð Bjarmason

To test for a key that is completely unknown to the keyring we need one
to sign the commit with. This was done by generating a new key and not
add it into the keyring. To avoid the key generation overhead and
problems where GPG did hang in CI during it, switch GNUPGHOME to an
empty directory instead, therefore making all used keys unknown for this
single `verify-commit` call.

Reported-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Fabian Stelzer <fs@gigacodes.de>
---
This was reported by Ævar in <211222.86ilvhpbl0.gmgdl@evledraar.gmail.com>.
Just using an empty keyring / gpg homedir should achieve the same effect and 
keeps the stress of generating a gpg key out of the CI.


 t/t7510-signed-commit.sh | 22 ++--------------------
 1 file changed, 2 insertions(+), 20 deletions(-)

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: unknown@git.com
-	Expire-Date: 0
-	%no-ask-passphrase
-	%no-protection
-	EOF
-	gpg --batch --gen-key keydetails &&
-	echo 13 >file && git commit -a -S"unknown@git.com" -m thirteenth &&
-	git tag thirteenth-signed &&
-	DELETE_FINGERPRINT=$(gpg -K --with-colons --fingerprint --batch unknown@git.com | grep "^fpr" | head -n 1 | awk -F ":" "{print \$10;}") &&
-	gpg --batch --yes --delete-secret-keys $DELETE_FINGERPRINT &&
-	gpg --batch --yes --delete-keys unknown@git.com
+	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
-- 
2.33.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH] t/gpg: simplify test for unknown key
  2022-01-07  9:14 [PATCH] t/gpg: simplify test for unknown key Fabian Stelzer
@ 2022-01-07 21:43 ` Junio C Hamano
  2022-01-11 16:56 ` Ævar Arnfjörð Bjarmason
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 10+ messages in thread
From: Junio C Hamano @ 2022-01-07 21:43 UTC (permalink / raw)
  To: Fabian Stelzer
  Cc: git, Han-Wen Nienhuys, brian m. carlson, Randall S. Becker,
	Bagas Sanjaya, Hans Jerry Illikainen, Felipe Contreras,
	Eric Sunshine, Gwyneth Morgan, Jonathan Tan, Josh Steadmon,
	Ævar Arnfjörð Bjarmason

Fabian Stelzer <fs@gigacodes.de> writes:

> To test for a key that is completely unknown to the keyring we need one
> to sign the commit with. This was done by generating a new key and not
> add it into the keyring. To avoid the key generation overhead and
> problems where GPG did hang in CI during it, switch GNUPGHOME to an
> empty directory instead, therefore making all used keys unknown for this
> single `verify-commit` call.
>
> Reported-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> Signed-off-by: Fabian Stelzer <fs@gigacodes.de>
> ---
> This was reported by Ævar in <211222.86ilvhpbl0.gmgdl@evledraar.gmail.com>.
> Just using an empty keyring / gpg homedir should achieve the same effect and 
> keeps the stress of generating a gpg key out of the CI.

Clever.  Losing lines of code and gaining more stability in CI is a
great thing.

>
>
>  t/t7510-signed-commit.sh | 22 ++--------------------
>  1 file changed, 2 insertions(+), 20 deletions(-)
>
> 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: unknown@git.com
> -	Expire-Date: 0
> -	%no-ask-passphrase
> -	%no-protection
> -	EOF
> -	gpg --batch --gen-key keydetails &&
> -	echo 13 >file && git commit -a -S"unknown@git.com" -m thirteenth &&
> -	git tag thirteenth-signed &&
> -	DELETE_FINGERPRINT=$(gpg -K --with-colons --fingerprint --batch unknown@git.com | grep "^fpr" | head -n 1 | awk -F ":" "{print \$10;}") &&
> -	gpg --batch --yes --delete-secret-keys $DELETE_FINGERPRINT &&
> -	gpg --batch --yes --delete-keys unknown@git.com
> +	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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] t/gpg: simplify test for unknown key
  2022-01-07  9:14 [PATCH] t/gpg: simplify test for unknown key Fabian Stelzer
  2022-01-07 21:43 ` Junio C Hamano
@ 2022-01-11 16:56 ` Ævar Arnfjörð Bjarmason
  2022-01-11 17:26   ` Fabian Stelzer
  2022-01-12 12:07 ` [PATCH v2] " Fabian Stelzer
  2022-01-14  3:52 ` [PATCH] " Josh Steadmon
  3 siblings, 1 reply; 10+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-01-11 16:56 UTC (permalink / raw)
  To: Fabian Stelzer
  Cc: git, Han-Wen Nienhuys, brian m. carlson, Randall S. Becker,
	Bagas Sanjaya, Hans Jerry Illikainen, Felipe Contreras,
	Eric Sunshine, Gwyneth Morgan, Jonathan Tan, Josh Steadmon


On Fri, Jan 07 2022, Fabian Stelzer wrote:

> To test for a key that is completely unknown to the keyring we need one
> to sign the commit with. This was done by generating a new key and not
> add it into the keyring. To avoid the key generation overhead and
> problems where GPG did hang in CI during it, switch GNUPGHOME to an
> empty directory instead, therefore making all used keys unknown for this
> single `verify-commit` call.
>
> Reported-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> Signed-off-by: Fabian Stelzer <fs@gigacodes.de>
> ---
> This was reported by Ævar in <211222.86ilvhpbl0.gmgdl@evledraar.gmail.com>.
> Just using an empty keyring / gpg homedir should achieve the same effect and 
> keeps the stress of generating a gpg key out of the CI.

Thanks, it would be great to have this in and before v2.35.0. I've run
into several boxes (on the GCC farm) that hang without this patch.

>  t/t7510-signed-commit.sh | 22 ++--------------------
>  1 file changed, 2 insertions(+), 20 deletions(-)
>
> 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: unknown@git.com
> -	Expire-Date: 0
> -	%no-ask-passphrase
> -	%no-protection
> -	EOF
> -	gpg --batch --gen-key keydetails &&
> -	echo 13 >file && git commit -a -S"unknown@git.com" -m thirteenth &&
> -	git tag thirteenth-signed &&
> -	DELETE_FINGERPRINT=$(gpg -K --with-colons --fingerprint --batch unknown@git.com | grep "^fpr" | head -n 1 | awk -F ":" "{print \$10;}") &&
> -	gpg --batch --yes --delete-secret-keys $DELETE_FINGERPRINT &&
> -	gpg --batch --yes --delete-keys unknown@git.com
> +	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 &&

Before I noticed this thread (I looked at
https://lore.kernel.org/git/20211230111038.jtoqytdhkilv2732@fs/ first,
and the In-Reply-To chain wasn't connected) I was about to submit
exactly this patch for you but with:

	-       test_must_fail git verify-commit thirteenth-signed 2>actual &&
	+       test_must_fail env GNUPGHOME="$GNUPGHOME_NOT_USED" git verify-commit initial 2>actual &&

Both of those are probably a good thing to do here. I.e.:

 1. Didn't we have portability issues with "ENV_VAR=VALUE shell_function ..." ?
 2. You're pointing to a nonexisting ./empty_home, but shouldn't we use
    $GNUPGHOME_NOT_USED? The existing "show unknown signature with custom format"
    test in the same file does that.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] t/gpg: simplify test for unknown key
  2022-01-11 16:56 ` Ævar Arnfjörð Bjarmason
@ 2022-01-11 17:26   ` Fabian Stelzer
  2022-01-11 19:40     ` Taylor Blau
  0 siblings, 1 reply; 10+ messages in thread
From: Fabian Stelzer @ 2022-01-11 17:26 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Han-Wen Nienhuys, brian m. carlson, Randall S. Becker,
	Bagas Sanjaya, Hans Jerry Illikainen, Felipe Contreras,
	Eric Sunshine, Gwyneth Morgan, Jonathan Tan, Josh Steadmon

On 11.01.2022 17:56, Ævar Arnfjörð Bjarmason wrote:
>
>On Fri, Jan 07 2022, Fabian Stelzer wrote:
>
>> To test for a key that is completely unknown to the keyring we need one
>> to sign the commit with. This was done by generating a new key and not
>> add it into the keyring. To avoid the key generation overhead and
>> problems where GPG did hang in CI during it, switch GNUPGHOME to an
>> empty directory instead, therefore making all used keys unknown for this
>> single `verify-commit` call.
>>
>> Reported-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> Signed-off-by: Fabian Stelzer <fs@gigacodes.de>
>> ---
>> This was reported by Ævar in <211222.86ilvhpbl0.gmgdl@evledraar.gmail.com>.
>> Just using an empty keyring / gpg homedir should achieve the same effect and
>> keeps the stress of generating a gpg key out of the CI.
>
>Thanks, it would be great to have this in and before v2.35.0. I've run
>into several boxes (on the GCC farm) that hang without this patch.
>
>>  t/t7510-signed-commit.sh | 22 ++--------------------
>>  1 file changed, 2 insertions(+), 20 deletions(-)
>>
>> 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: unknown@git.com
>> -	Expire-Date: 0
>> -	%no-ask-passphrase
>> -	%no-protection
>> -	EOF
>> -	gpg --batch --gen-key keydetails &&
>> -	echo 13 >file && git commit -a -S"unknown@git.com" -m thirteenth &&
>> -	git tag thirteenth-signed &&
>> -	DELETE_FINGERPRINT=$(gpg -K --with-colons --fingerprint --batch unknown@git.com | grep "^fpr" | head -n 1 | awk -F ":" "{print \$10;}") &&
>> -	gpg --batch --yes --delete-secret-keys $DELETE_FINGERPRINT &&
>> -	gpg --batch --yes --delete-keys unknown@git.com
>> +	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 &&
>
>Before I noticed this thread (I looked at
>https://lore.kernel.org/git/20211230111038.jtoqytdhkilv2732@fs/ first,
>and the In-Reply-To chain wasn't connected)

Yeah, sorry about that. I forgot to add the in-reply-to :/

>I was about to submit
>exactly this patch for you but with:
>
>	-       test_must_fail git verify-commit thirteenth-signed 2>actual &&
>	+       test_must_fail env GNUPGHOME="$GNUPGHOME_NOT_USED" git verify-commit initial 2>actual &&
>
>Both of those are probably a good thing to do here. I.e.:
>
> 1. Didn't we have portability issues with "ENV_VAR=VALUE shell_function ..." ?

I'm not good with portability stuff and trust your judgment on this.

> 2. You're pointing to a nonexisting ./empty_home, but shouldn't we use
>    $GNUPGHOME_NOT_USED? The existing "show unknown signature with custom format"
>    test in the same file does that.

I was not aware of $GNUPGHOME_NOT_USED but it is used in a similar fashion.  
However it is set to the old value of $GNUPGHOME before we change it in 
lib-gpg.sh which seems wrong to me. Wouldn't it then just pick up the gpg 
homedir of whatever the test environment has?
Using the variable is good, but i would set it to a known empty directory 
or?

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] t/gpg: simplify test for unknown key
  2022-01-11 17:26   ` Fabian Stelzer
@ 2022-01-11 19:40     ` Taylor Blau
  2022-01-12 12:10       ` Fabian Stelzer
  0 siblings, 1 reply; 10+ messages in thread
From: Taylor Blau @ 2022-01-11 19:40 UTC (permalink / raw)
  To: Fabian Stelzer
  Cc: Ævar Arnfjörð Bjarmason, git, Han-Wen Nienhuys,
	brian m. carlson, Randall S. Becker, Bagas Sanjaya,
	Hans Jerry Illikainen, Felipe Contreras, Eric Sunshine,
	Gwyneth Morgan, Jonathan Tan, Josh Steadmon

On Tue, Jan 11, 2022 at 06:26:21PM +0100, Fabian Stelzer wrote:
> > I was about to submit
> > exactly this patch for you but with:
> >
> > 	-       test_must_fail git verify-commit thirteenth-signed 2>actual &&
> > 	+       test_must_fail env GNUPGHOME="$GNUPGHOME_NOT_USED" git verify-commit initial 2>actual &&
> >
> > Both of those are probably a good thing to do here. I.e.:
> >
> > 1. Didn't we have portability issues with "ENV_VAR=VALUE shell_function ..." ?
>
> I'm not good with portability stuff and trust your judgment on this.

See [1] and the ensuing discussion for a good summary. Re-reading
that thread and comparing it with what we see with `git grep
test_must_fail env -- t` confirms that that

    test_must_fail env GNUPGHOME="$GNUPGHOME_NOT_USED" git verify-commit ...

is the right thing to do here.

> > 2. You're pointing to a nonexisting ./empty_home, but shouldn't we use
> >    $GNUPGHOME_NOT_USED? The existing "show unknown signature with custom format"
> >    test in the same file does that.
>
> I was not aware of $GNUPGHOME_NOT_USED but it is used in a similar fashion.
> However it is set to the old value of $GNUPGHOME before we change it in
> lib-gpg.sh which seems wrong to me. Wouldn't it then just pick up the gpg
> homedir of whatever the test environment has?
> Using the variable is good, but i would set it to a known empty directory
> or?

Yeah, t7510 captures the value of $GNUPGHOME as $GNUPGHOME_NOT_USED
before sourcing t/lib-gpg.sh. So long as nobody else has tampered with
$GNUPGHOME, they should get `$TRASH_DIRECTORY/gnupg-home-not-used`.

But I'm less certain that there isn't somebody accidentally ignoring the
"not-used" portion of the test $GNUPGHOME ;).

And I don't think that reasoning through it all is that worthwhile, so
I'm fine with what a much more direct ./empty_home here.

Thanks,
Taylor

[1]: https://lore.kernel.org/git/xmqqbn3l3kmc.fsf@gitster.mtv.corp.google.com/

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v2] t/gpg: simplify test for unknown key
  2022-01-07  9:14 [PATCH] t/gpg: simplify test for unknown key Fabian Stelzer
  2022-01-07 21:43 ` Junio C Hamano
  2022-01-11 16:56 ` Ævar Arnfjörð Bjarmason
@ 2022-01-12 12:07 ` Fabian Stelzer
  2022-01-12 18:57   ` Taylor Blau
  2022-01-12 19:23   ` Junio C Hamano
  2022-01-14  3:52 ` [PATCH] " Josh Steadmon
  3 siblings, 2 replies; 10+ messages in thread
From: Fabian Stelzer @ 2022-01-12 12:07 UTC (permalink / raw)
  To: git
  Cc: Fabian Stelzer, Ævar Arnfjörð Bjarmason,
	Han-Wen Nienhuys, brian m. carlson, Randall S. Becker,
	Bagas Sanjaya, Hans Jerry Illikainen, Felipe Contreras,
	Eric Sunshine, Gwyneth Morgan, Jonathan Tan, Josh Steadmon

To test for a key that is completely unknown to the keyring we need one
to sign the commit with. This was done by generating a new key and not
add it into the keyring. To avoid the key generation overhead and
problems where GPG did hang in CI during it, switch GNUPGHOME to the
empty $GNUPGHOME_NOT_USED instead, therefore making all used keys unknown 
for this single `verify-commit` call.

Reported-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Fabian Stelzer <fs@gigacodes.de>
---
 t/t7510-signed-commit.sh | 22 ++--------------------
 1 file changed, 2 insertions(+), 20 deletions(-)

diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
index 9882b69ae2..8593b7e3cb 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: unknown@git.com
-	Expire-Date: 0
-	%no-ask-passphrase
-	%no-protection
-	EOF
-	gpg --batch --gen-key keydetails &&
-	echo 13 >file && git commit -a -S"unknown@git.com" -m thirteenth &&
-	git tag thirteenth-signed &&
-	DELETE_FINGERPRINT=$(gpg -K --with-colons --fingerprint --batch unknown@git.com | grep "^fpr" | head -n 1 | awk -F ":" "{print \$10;}") &&
-	gpg --batch --yes --delete-secret-keys $DELETE_FINGERPRINT &&
-	gpg --batch --yes --delete-keys unknown@git.com
+	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 &&
+	test_must_fail env GNUPGHOME="$GNUPGHOME_NOT_USED" 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
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH] t/gpg: simplify test for unknown key
  2022-01-11 19:40     ` Taylor Blau
@ 2022-01-12 12:10       ` Fabian Stelzer
  0 siblings, 0 replies; 10+ messages in thread
From: Fabian Stelzer @ 2022-01-12 12:10 UTC (permalink / raw)
  To: Taylor Blau
  Cc: Ævar Arnfjörð Bjarmason, git, Han-Wen Nienhuys,
	brian m. carlson, Randall S. Becker, Bagas Sanjaya,
	Hans Jerry Illikainen, Felipe Contreras, Eric Sunshine,
	Gwyneth Morgan, Jonathan Tan, Josh Steadmon

On 11.01.2022 14:40, Taylor Blau wrote:
>On Tue, Jan 11, 2022 at 06:26:21PM +0100, Fabian Stelzer wrote:
>> > I was about to submit
>> > exactly this patch for you but with:
>> >
>> > 	-       test_must_fail git verify-commit thirteenth-signed 2>actual &&
>> > 	+       test_must_fail env GNUPGHOME="$GNUPGHOME_NOT_USED" git verify-commit initial 2>actual &&
>> >
>> > Both of those are probably a good thing to do here. I.e.:
>> >
>> > 1. Didn't we have portability issues with "ENV_VAR=VALUE shell_function ..." ?
>>
>> I'm not good with portability stuff and trust your judgment on this.
>
>See [1] and the ensuing discussion for a good summary. Re-reading
>that thread and comparing it with what we see with `git grep
>test_must_fail env -- t` confirms that that
>
>    test_must_fail env GNUPGHOME="$GNUPGHOME_NOT_USED" git verify-commit ...
>
>is the right thing to do here.

Thanks. Interesting

>
>> > 2. You're pointing to a nonexisting ./empty_home, but shouldn't we use
>> >    $GNUPGHOME_NOT_USED? The existing "show unknown signature with custom format"
>> >    test in the same file does that.
>>
>> I was not aware of $GNUPGHOME_NOT_USED but it is used in a similar fashion.
>> However it is set to the old value of $GNUPGHOME before we change it in
>> lib-gpg.sh which seems wrong to me. Wouldn't it then just pick up the gpg
>> homedir of whatever the test environment has?
>> Using the variable is good, but i would set it to a known empty directory
>> or?
>
>Yeah, t7510 captures the value of $GNUPGHOME as $GNUPGHOME_NOT_USED
>before sourcing t/lib-gpg.sh. So long as nobody else has tampered with
>$GNUPGHOME, they should get `$TRASH_DIRECTORY/gnupg-home-not-used`.
>
>But I'm less certain that there isn't somebody accidentally ignoring the
>"not-used" portion of the test $GNUPGHOME ;).
>
>And I don't think that reasoning through it all is that worthwhile, so
>I'm fine with what a much more direct ./empty_home here.
>

Yeah. I checked the other uses of GNUPGHOME and the NOT_USED seems fine (and 
more consistent with existing tests). So let's use it.
Even if some other (non gpg related) test accidentally pollutes this gpghome 
it would surely not do so with the actual signing key used in the test 
suite.

Thanks, I'll send a new patch in a second

>Thanks,
>Taylor
>
>[1]: https://lore.kernel.org/git/xmqqbn3l3kmc.fsf@gitster.mtv.corp.google.com/

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] t/gpg: simplify test for unknown key
  2022-01-12 12:07 ` [PATCH v2] " Fabian Stelzer
@ 2022-01-12 18:57   ` Taylor Blau
  2022-01-12 19:23   ` Junio C Hamano
  1 sibling, 0 replies; 10+ messages in thread
From: Taylor Blau @ 2022-01-12 18:57 UTC (permalink / raw)
  To: Fabian Stelzer
  Cc: git, Ævar Arnfjörð Bjarmason, Han-Wen Nienhuys,
	brian m. carlson, Randall S. Becker, Bagas Sanjaya,
	Hans Jerry Illikainen, Felipe Contreras, Eric Sunshine,
	Gwyneth Morgan, Jonathan Tan, Josh Steadmon

On Wed, Jan 12, 2022 at 01:07:57PM +0100, Fabian Stelzer wrote:
> To test for a key that is completely unknown to the keyring we need one
> to sign the commit with. This was done by generating a new key and not
> add it into the keyring. To avoid the key generation overhead and
> problems where GPG did hang in CI during it, switch GNUPGHOME to the
> empty $GNUPGHOME_NOT_USED instead, therefore making all used keys unknown
> for this single `verify-commit` call.
>
> Reported-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> Signed-off-by: Fabian Stelzer <fs@gigacodes.de>

Reviewed-by: Taylor Blau <me@ttaylorr.com>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] t/gpg: simplify test for unknown key
  2022-01-12 12:07 ` [PATCH v2] " Fabian Stelzer
  2022-01-12 18:57   ` Taylor Blau
@ 2022-01-12 19:23   ` Junio C Hamano
  1 sibling, 0 replies; 10+ messages in thread
From: Junio C Hamano @ 2022-01-12 19:23 UTC (permalink / raw)
  To: Fabian Stelzer
  Cc: git, Ævar Arnfjörð Bjarmason, Han-Wen Nienhuys,
	brian m. carlson, Randall S. Becker, Bagas Sanjaya,
	Hans Jerry Illikainen, Felipe Contreras, Eric Sunshine,
	Gwyneth Morgan, Jonathan Tan, Josh Steadmon

Fabian Stelzer <fs@gigacodes.de> writes:

> To test for a key that is completely unknown to the keyring we need one
> to sign the commit with. This was done by generating a new key and not
> add it into the keyring. To avoid the key generation overhead and
> problems where GPG did hang in CI during it, switch GNUPGHOME to the
> empty $GNUPGHOME_NOT_USED instead, therefore making all used keys unknown 
> for this single `verify-commit` call.
>
> Reported-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> Signed-off-by: Fabian Stelzer <fs@gigacodes.de>
> ---

The original one is already in 'next' so I'll revert the merge and
queue this one instead.

We could turn this into incremental and apply it on the original
one, which may give us a chance to highlight this common mistake
of using a single-shot environment with shell functions, but let's
not bother.

Thanks.


>  t/t7510-signed-commit.sh | 22 ++--------------------
>  1 file changed, 2 insertions(+), 20 deletions(-)
>
> diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
> index 9882b69ae2..8593b7e3cb 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: unknown@git.com
> -	Expire-Date: 0
> -	%no-ask-passphrase
> -	%no-protection
> -	EOF
> -	gpg --batch --gen-key keydetails &&
> -	echo 13 >file && git commit -a -S"unknown@git.com" -m thirteenth &&
> -	git tag thirteenth-signed &&
> -	DELETE_FINGERPRINT=$(gpg -K --with-colons --fingerprint --batch unknown@git.com | grep "^fpr" | head -n 1 | awk -F ":" "{print \$10;}") &&
> -	gpg --batch --yes --delete-secret-keys $DELETE_FINGERPRINT &&
> -	gpg --batch --yes --delete-keys unknown@git.com
> +	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 &&
> +	test_must_fail env GNUPGHOME="$GNUPGHOME_NOT_USED" 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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] t/gpg: simplify test for unknown key
  2022-01-07  9:14 [PATCH] t/gpg: simplify test for unknown key Fabian Stelzer
                   ` (2 preceding siblings ...)
  2022-01-12 12:07 ` [PATCH v2] " Fabian Stelzer
@ 2022-01-14  3:52 ` Josh Steadmon
  3 siblings, 0 replies; 10+ messages in thread
From: Josh Steadmon @ 2022-01-14  3:52 UTC (permalink / raw)
  To: Fabian Stelzer
  Cc: git, Han-Wen Nienhuys, brian m. carlson, Randall S. Becker,
	Bagas Sanjaya, Hans Jerry Illikainen, Felipe Contreras,
	Eric Sunshine, Gwyneth Morgan, Jonathan Tan,
	Ævar Arnfjörð Bjarmason

On 2022.01.07 10:14, Fabian Stelzer wrote:
> To test for a key that is completely unknown to the keyring we need one
> to sign the commit with. This was done by generating a new key and not
> add it into the keyring. To avoid the key generation overhead and
> problems where GPG did hang in CI during it, switch GNUPGHOME to an
> empty directory instead, therefore making all used keys unknown for this
> single `verify-commit` call.
> 
> Reported-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> Signed-off-by: Fabian Stelzer <fs@gigacodes.de>
> ---
> This was reported by Ævar in <211222.86ilvhpbl0.gmgdl@evledraar.gmail.com>.
> Just using an empty keyring / gpg homedir should achieve the same effect and 
> keeps the stress of generating a gpg key out of the CI.

Looks good to me.

Reviewed-by: Josh Steadmon <steadmon@google.com>

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2022-01-14  3:52 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-07  9:14 [PATCH] t/gpg: simplify test for unknown key Fabian Stelzer
2022-01-07 21:43 ` Junio C Hamano
2022-01-11 16:56 ` Ævar Arnfjörð Bjarmason
2022-01-11 17:26   ` Fabian Stelzer
2022-01-11 19:40     ` Taylor Blau
2022-01-12 12:10       ` Fabian Stelzer
2022-01-12 12:07 ` [PATCH v2] " Fabian Stelzer
2022-01-12 18:57   ` Taylor Blau
2022-01-12 19:23   ` Junio C Hamano
2022-01-14  3:52 ` [PATCH] " Josh Steadmon

Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).