Re: [PATCH] gpg-interface: fix for gpgsm v2.3

[Fabian Stelzer <fs@gigacodes.de>]

On 09.02.2022 11:20, Todd Zullinger wrote:
>Fabian Stelzer wrote:
>> On 07.02.2022 11:38, Todd Zullinger wrote:
>>> How did it fail for you?  It passes all the tests when I've
>>> run it against Fedora and RHEL-based hosts.  If it's flaky
>>> on other systems, that would put a damper on doing it this
>>> way.  Though it _should_ work.
>>
>> Sorry for the delays, I'm a bit busy with other things at the moment.
>
>No apologies needed.  This is something I worked on back in
>November and had yet to send to the list, so I'm the last
>person to rush another. :)
>
>> I did get an interactive popup asking if I would like to
>> trust the key when I ran the t4202 test. This never
>> happened with the old variant.
>
>Interesting.  I do have a patch in my gnupg-2.3 series to
>reload the gpg agent after changing the trustlist, as the
>changes were not picked up prior to that.  In my case, I was
>running the tests in an environment where gpg could not
>prompt me.  (It also seems like we should try harder to have
>the test suite reject such prompts).
>

Yes, gpg-agent in general can be problematic for the tests. I'm not familiar 
enough with gpg but I don't know if we can get by without it?

>--- 8< ---
>Subject: [PATCH] t/lib-gpg: reload gpg components after updating trustlist
>
>With gpgsm from gnupg-2.3, the changes to the trustlist.txt do not
>appear to be picked up without refreshing the gpg-agent.  Use the 'all'
>keyword to reload all of the gpg components.  The scdaemon is started as
>a child of gpg-agent, for example.
>
>We used to have a --kill at this spot, but I removed it in 2e285e7803
>(t/lib-gpg: drop redundant killing of gpg-agent, 2019-02-07).  It seems
>like it might be necessary (again) for 2.3.
>
>Signed-off-by: Todd Zullinger <tmz@pobox.com>
>---
>
>Notes:
>    An alternative to doing this dance with the trustlist.txt and having to
>    kill and/or reload the gpg-agent to pick up the change right after the
>    import in each test might be to make this part of the steps used when
>    adding/updating/removing certificates in t/lib-gpg.
>
>    If not as a one-time affair when a cert is added/update/removed, then
>    perhaps as a step taken by/in t/lib-gpg.sh only once.  It could populate
>    a gpghome to be copied into the trash dir for each test which used
>    gpg/gpgsm.  I haven't measured the effect of the extra reload precisely,
>    but I'm sure it's not free.
>
>    (For what it's worth, it didn't add any noticeable amount of time to the
>    full builds/test runs I made while working on this, so it's a seemingly
>    small cost, at least.)
>
>    Also, hello Henning,
>
>    Way back, in February 2019¹, when I submitted 2e285e7803 to remove the
>    "redundant" killing of the gpg-agent, you said:
>
>    > Killing the agent once should be enough, i remember manually killing
>    > it many times as i was looking for a way to generate certs and trust
>    > (configure gpgsm for the test). That is probably why i copied it over
>    > in the first place.
>
>    As I wrote this patch to partially restore the gpg-agent killing (now
>    just a reload), I thought this might have been the sort of issue that
>    you hit while testing.
>
>    It could be unrelated, but it sounds quite similar to what I found with
>    gnupg-2.3 when trying to get it to pick up the trustlist.txt changes.  I
>    thought you might at least enjoy seeing it come back around.  :)
>
>    ¹ <20190208093324.7b17f270@md1za8fc.ad001.siemens.net>
>
> t/lib-gpg.sh | 1 +
> 1 file changed, 1 insertion(+)
>
>diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh
>index 6bc083ca77..38e2c0f4fb 100644
>--- a/t/lib-gpg.sh
>+++ b/t/lib-gpg.sh
>@@ -75,6 +75,7 @@ test_lazy_prereq GPGSM '
> 	gpgsm --homedir "${GNUPGHOME}" -K --with-colons |
> 	awk -F ":" "/^fpr:/ {printf \"%s S relax\\n\", \$10}" \
> 		>"${GNUPGHOME}/trustlist.txt" &&
>+	(gpgconf --reload all || : ) &&
>
> 	echo hello | gpgsm --homedir "${GNUPGHOME}" >/dev/null \
> 	       -u committer@example.com -o /dev/null --sign -
>
>--- 8< ---

This patch fixes it for me.

>
>I have another patch which changes the earlier gpgconf call
>which kills gpg-agent to kill all gpg daemons, as there are
>some others which could potentially interfere with the
>tests:
>
>--- 8< ---
>Subject: [PATCH] t/lib-gpg: kill all gpg components, not just gpg-agent
>
>The gpg-agent is one of several processes that newer releases of GnuPG
>start automatically.  Issue a kill to each of them to ensure they do not
>affect separate tests.  (Yes, the separate GNUPGHOME should do that
>already. If we find that is case, we could drop the --kill entirely.)
>
>In terms of compatibility, the 'all' keyword was added to the --kill &
>--reload options in GnuPG 2.1.18.  Debian and RHEL are often used as
>indicators of how a change might affect older systems we often try to
>support.
>
>    - Debian Strech (old old stable), which has limited security support
>      until June 2022, has GnuPG 2.1.18 (or 2.2.x in backports).
>
>    - CentOS/RHEL 7, which is supported until June 2024, has GnuPG
>      2.0.22, which lacks the --kill option, so the change won't have
>      any impact.
>
>Signed-off-by: Todd Zullinger <tmz@pobox.com>
>---
> t/lib-gpg.sh | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh
>index d675698a2d..2bb309a8c1 100644
>--- a/t/lib-gpg.sh
>+++ b/t/lib-gpg.sh
>@@ -40,7 +40,7 @@ test_lazy_prereq GPG '
> 		#		> lib-gpg/ownertrust
> 		mkdir "$GNUPGHOME" &&
> 		chmod 0700 "$GNUPGHOME" &&
>-		(gpgconf --kill gpg-agent || : ) &&
>+		(gpgconf --kill all || : ) &&
> 		gpg --homedir "${GNUPGHOME}" --import \
> 			"$TEST_DIRECTORY"/lib-gpg/keyring.gpg &&
> 		gpg --homedir "${GNUPGHOME}" --import-ownertrust \
>--- 8< ---
>
>I have the series in the gpg-misc-fixes branch:
>
>    https://github.com/tmzullinger/git/commits/gpg-misc-fixes
>
>(That series does differ in that it has `string_list_split()`
>instead of the simpler `strbuf_addch(&gpg_status, '\n');` to
>fix the gpgsm parsing.)
>
>Iff you think it would be useful or helpful, I can post that
>series.

I have prepared the patch with the simple strstr() matching I can post in a 
bit. I would add your two gpg test lib patches to it if thats ok?

Thanks

>
>Thanks,
>
>-- 
>Todd