git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Fabian Stelzer <fs@gigacodes.de>
Date
Feb 7, 2022, 10:52 UTC
Message-ID
<20220207105240.dk443kcozynlonpp@fs>
In-Reply-To
<Yfw0kapgSSWO3Pyx@pobox.com>
On 03.02.2022 15:01, Todd Zullinger wrote:
Show 99 quoted lines
>Hi Fabian,
>
>Fabian Stelzer wrote:
>> gpgsm v2.3 changed some details about its output:
>>  - instead of displaying `fingerprint:` for keys it will print `sha1
>>    fpr:` and `sha2 fpr:`
>>  - some wording of errors has changed
>>  - signing will omit an extra debug output line before the [GNUPG]: tag
>
>Thanks for sending this.  I noticed these as well, as Fedora
>started shipping gnupg-2.3 a few months back.  I have been
>trying (and failing) to make time to submit (when I know I
>won't be too distracted to actually converse about them).
>The commits I made for the tests in Fedora are all here:
>
>    https://src.fedoraproject.org/rpms/git/c/a7d2f7e53
>
>I don't intend that as something anyone here should feel the
>need to chase down.  But since they provide some additional
>context on the changes I made in the same area, it might
>help if anyone's curious.
>
>> diff --git a/gpg-interface.c b/gpg-interface.c
>> index b52eb0e2e0..299e7f588a 100644
>> --- a/gpg-interface.c
>> +++ b/gpg-interface.c
>> @@ -939,7 +939,7 @@ static int sign_buffer_gpg(struct strbuf *buffer, struct strbuf *signature,
>>  			   signature, 1024, &gpg_status, 0);
>>  	sigchain_pop(SIGPIPE);
>>
>> -	ret |= !strstr(gpg_status.buf, "\n[GNUPG:] SIG_CREATED ");
>> +	ret |= !strstr(gpg_status.buf, "[GNUPG:] SIG_CREATED ");
>>  	strbuf_release(&gpg_status);
>>  	if (ret)
>>  		return error(_("gpg failed to sign the data"));
>
>As Junio noted, this loosens the GPG parsing a good bit.  I
>worried it could lead to security issues as well.  The
>simple fix I made in Fedora was to add a newline to the
>gpg_status string buffer before adding the command output to
>it:
>
>    diff --git a/gpg-interface.c b/gpg-interface.c
>    index 3e7255a2a9..d179dfb3ab 100644
>    --- a/gpg-interface.c
>    +++ b/gpg-interface.c
>    @@ -859,6 +859,12 @@ static int sign_buffer_gpg(struct strbuf *buffer, struct strbuf *signature,
>
>	bottom = signature->len;
>
>    +	/*
>    +	 * Ensure gpg_status begins with a newline or we'll fail to match if
>    +	 * the SIG_CREATED line is at the start of the gpg output.
>    +	 */
>    +	strbuf_addch(&gpg_status, '\n');
>    +
>	/*
>	* When the username signingkey is bad, program could be terminated
>	* because gpg exits without reading and then write gets SIGPIPE.
>
>https://src.fedoraproject.org/rpms/git/blob/a7d2f7e53/f/0005-gpg-interface-match-SIG_CREATED-if-it-s-the-first-li.patch
>
>But that seemed like a bit of a hack.  What I had queued up
>to submit for discussion (as I'm not sure that it isn't
>entirely horrible) used the string-list API to parse the gpg
>output:
>
>    diff --git a/gpg-interface.c b/gpg-interface.c
>    index b52eb0e2e0..e63ccdcb11 100644
>    --- a/gpg-interface.c
>    +++ b/gpg-interface.c
>    @@ -921,6 +921,7 @@ static int sign_buffer_gpg(struct strbuf *buffer, struct strbuf *signature,
>     	int ret;
>     	size_t bottom;
>     	struct strbuf gpg_status = STRBUF_INIT;
>    +	struct string_list lines = { .cmp = starts_with };
>
>     	strvec_pushl(&gpg.args,
>     		     use_format->program,
>    @@ -939,8 +940,11 @@ static int sign_buffer_gpg(struct strbuf *buffer, struct strbuf *signature,
>     			   signature, 1024, &gpg_status, 0);
>     	sigchain_pop(SIGPIPE);
>
>    -	ret |= !strstr(gpg_status.buf, "\n[GNUPG:] SIG_CREATED ");
>    +	string_list_split_in_place(&lines, gpg_status.buf, '\n', -1);
>    +	ret |= !unsorted_string_list_has_string(&lines, "[GNUPG:] SIG_CREATED ");
>     	strbuf_release(&gpg_status);
>    +	string_list_clear(&lines, 0);
>    +
>     	if (ret)
>     		return error(_("gpg failed to sign the data"));
>
>That's the commit I was most in doubt about though, as my C
>"skills" are close to non-existant.  I'd rather have
>something ugly and clear (like the `strbuf_addch(...)`
>above) than clever and wrong in gpg-interface.c.
>
>(To be clear, I mean "clever and wrong" in regard to my use
>of the string list API, not anyone else's code.)

string_list_split seems a bit like overkill. My first thought was actually sth like:

cp = strstr(gpg_status.buf, "[GNUPG]: SIG_CREATED");
if (cp == gpg_status.buf || --cp == '\n')
	// found

But that would fail in case the string came up before the actual success message at the beginning of a line. So Junios variant of using the for() loop is more robust and would normally find the correct string on its first iteration anyway.

Show 18 quoted lines
>
>> diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh
>> index 3e7ee1386a..6c2dd4b14b 100644
>> --- a/t/lib-gpg.sh
>> +++ b/t/lib-gpg.sh
>> @@ -73,8 +73,8 @@ test_lazy_prereq GPGSM '
>>  		--import "$TEST_DIRECTORY"/lib-gpg/gpgsm_cert.p12 &&
>>
>>  	gpgsm --homedir "${GNUPGHOME}" -K |
>> -	grep fingerprint: |
>> -	cut -d" " -f4 |
>> +	grep -E "(fingerprint|sha1 fpr):" |
>> +	cut -d":" -f2- | tr -d " " |
>>  	tr -d "\\n" >"${GNUPGHOME}/trustlist.txt" &&
>
>I think this whole thing can (and should) be simplified by
>using gpg's --with-colons output which is intended to be
>machine parsable.

I looked for sth like this but gpgs --help does not list it so i didn't dig deeper. I've checked the blame and it seems like this was introduced >19 years ago. So i guess we can probably use this ^^

Show 23 quoted lines
>
>If we'd been using that previously, we wouldn't need to make
>any further changes here.
>
>I think we're making our lives difficult by screen scraping
>here wher we don't need to do so.
>
>The change I made for the Fedora package to fix this does
>it like this:
>
>    --- a/t/lib-gpg.sh
>    +++ b/t/lib-gpg.sh
>    @@ -72,12 +72,10 @@ test_lazy_prereq GPGSM '
>                    --passphrase-fd 0 --pinentry-mode loopback \
>                    --import "$TEST_DIRECTORY"/lib-gpg/gpgsm_cert.p12 &&
>
>    -	gpgsm --homedir "${GNUPGHOME}" -K |
>    -	grep fingerprint: |
>    -	cut -d" " -f4 |
>    -	tr -d "\\n" >"${GNUPGHOME}/trustlist.txt" &&
>    +	gpgsm --homedir "${GNUPGHOME}" -K --with-colons |
>    +	awk -F ":" "/^fpr:/ {printf \"%s S relax\\n\", \$10}" \
>    +		>"${GNUPGHOME}/trustlist.txt" &&
This does not quite work for me. It will add the fingerprint without the 
colons into the trustlist which is not valid :/
It would need sth like:
+       gpgsm --with-colons --homedir "${GNUPGHOME}" -K |
+       awk -F ":" "/^(fpr|fingerprint):/ {gsub(/.{2}/, \"&:\", \$10); 
printf \"%s S relax\\n\", substr(\$10, 1, length(\$10)-1)}" \
+        >"${GNUPGHOME}/trustlist.txt" &&

which looks needlessly complicated. There is probably some better way to do this with/without awk.

Show 39 quoted lines
>
>    -	echo " S relax" >>"${GNUPGHOME}/trustlist.txt" &&
>            echo hello | gpgsm --homedir "${GNUPGHOME}" >/dev/null \
>                   -u committer@example.com -o /dev/null --sign -
>     '
>
>With a commit message:
>
>    https://src.fedoraproject.org/rpms/git/blob/a7d2f7e53/f/0001-t-lib-gpg-use-with-colons-when-parsing-gpgsm-output.patch
>
>I was hoping to submit that small series in the next day or
>two, while I've got a few days away from $work.  If doing it
>that way is appealing, I can submit them.  But only if that
>looks like a reasonable improvement to you and others.
>
>>  	echo " S relax" >>"${GNUPGHOME}/trustlist.txt" &&
>> diff --git a/t/t4202-log.sh b/t/t4202-log.sh
>> index 5049559861..08556493ce 100755
>> --- a/t/t4202-log.sh
>> +++ b/t/t4202-log.sh
>> @@ -1931,7 +1931,10 @@ test_expect_success GPGSM 'log --graph --show-signature for merged tag x509 miss
>>  	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
>> +	(
>> +		grep "^| | gpgsm: certificate not found" actual ||
>> +		grep "^| | gpgsm: failed to find the certificate: Not found" actual
>> +	)
>>  '
>>
>>  test_expect_success GPGSM 'log --graph --show-signature for merged tag x509 bad signature' '
>
>Can we make this simpler by adjusting the grep pattern?
>It's certainly a slight trade-off in ease of reading, but it
>saves a subshell and an extra command:
>
>    -	grep "^| | gpgsm: certificate not found" actual
>    +	grep -Ei "^| | gpgsm:( failed to find the)? certificate:? not found" actual
Thanks, will do.
Show 20 quoted lines
>
>I did that in a separate patch:
>
>    https://src.fedoraproject.org/rpms/git/blob/a7d2f7e53/f/0004-t4202-match-gpgsm-output-from-GnuPG-2.3.patch
>
>IMO, this is a bug in gnupg-2.3.  I submitted a patch to
>resolve it back in November, but have not gotten any
>response as yet. :(
>
>    https://lists.gnupg.org/pipermail/gnupg-devel/2021-November/034991.html
>
>Not that it will preclude us from having to fix this for the
>test suite, but it's worth noting why the change is needed
>(and when it will no longer be relevant -- if/when we don't
>care to support the few early gnupg-2.3.x releases).
>
>Thanks,
>
>-- 
>Todd
Previous: Junio C HamanoNext: Todd Zullinger
Message 7 of 24 in “gpg-interface: fix for gpgsm v2.3”
  1. gpg-interface: fix for gpgsm v2.3Fabian Stelzer, Feb 3, 2022
  2. Junio C HamanoFeb 3, 2022
  3. Todd ZullingerFeb 3, 2022
  4. Junio C HamanoFeb 3, 2022
  5. Todd ZullingerFeb 3, 2022
  6. Junio C HamanoFeb 3, 2022
  7. Fabian StelzerFeb 7, 2022
  8. Todd ZullingerFeb 7, 2022
  9. Fabian StelzerFeb 9, 2022
  10. Todd ZullingerFeb 9, 2022
  11. Fabian StelzerFeb 21, 2022
  12. Todd ZullingerFeb 23, 2022
  13. 2/3 t/lib-gpg: reload gpg components after updating trustlistFabian Stelzer, Feb 24, 2022
  14. 3/3 t/lib-gpg: kill all gpg components, not just gpg-agentFabian Stelzer, Feb 24, 2022
  15. 1/3 gpg-interface/gpgsm: fix for v2.3Fabian Stelzer, Feb 24, 2022
  16. Todd ZullingerFeb 28, 2022
  17. 1/3 gpg-interface/gpgsm: fix for v2.3Fabian Stelzer, Mar 2, 2022
  18. Junio C HamanoMar 2, 2022
  19. Fabian StelzerMar 3, 2022
  20. 1/3 gpg-interface/gpgsm: fix for v2.3Fabian Stelzer, Mar 4, 2022
  21. 3/3 t/lib-gpg: kill all gpg components, not just gpg-agentFabian Stelzer, Mar 4, 2022
  22. 2/3 t/lib-gpg: reload gpg components after updating trustlistFabian Stelzer, Mar 4, 2022
  23. 2/3 t/lib-gpg: reload gpg components after updating trustlistFabian Stelzer, Mar 2, 2022
  24. 3/3 t/lib-gpg: kill all gpg components, not just gpg-agentFabian Stelzer, Mar 2, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.