Re: [PATCH] gpg-interface: set trust level of missing key to "undefined"

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

> Here's the patch that I came up with, though it does not distinguish
> between "we did not see any trust level" and "gpg told us the trust
> level was undefined". I think that's OK. That level is still below
> TRUST_NEVER. But if we really want to distinguish we can introduce a new
> value for the enum.

Good.

In my zeroth draft, I added to the enum a new TRUST_FAILED = -1 to
be used for the initialization assignment and get stringified in the
gpg_trust_level_to_str() function, which gave us the distinction and
made sure the enum is signed.  But in the end, I decided it was not
worth risking upsetting the end-user scripts that assumed the
current set of levels with a new "level" that is not known to them.

Initializing to undefined like this patch is with much less damage
to the codebase, and existing end-user scripts are probably prepared
to react to "undefined" already and treat it as even less trustworthy
than the "never" ones.

Will queue.  Thanks.

> -- >8 --
> Subject: gpg-interface: set trust level of missing key to "undefined"
>
> In check_signature(), we initialize the trust_level field to "-1", with
> the idea that if gpg does not return a trust level at all (if there is
> no signature, or if the signature is made by an unknown key), we'll
> use that value. But this has two problems:
>
>   1. Since the field is an enum, it's up to the compiler to decide what
>      underlying storage to use, and it only has to fit the values we've
>      declared. So we may not be able to store "-1" at all. And indeed,
>      on my system (linux with gcc), the resulting enum is an unsigned
>      32-bit value, and -1 becomes 4294967295.
>
>      The difference may seem academic (and you even get "-1" if you pass
>      it to printf("%d")), but it means that code like this:
>
>        status |= sigc->trust_level < configured_min_trust_level;
>
>      does not necessarily behave as expected. This turns out not to be a
>      bug in practice, though, because we keep the "-1" only when gpg did
>      not report a signature from a known key, in which case the line
>      above:
>
>        status |= sigc->result != 'G';
>
>      would always set status to non-zero anyway. So only a 'G' signature
>      with no parsed trust level would cause a problem, which doesn't
>      seem likely to trigger (outside of unexpected gpg behavior).
>
>   2. When using the "%GT" format placeholder, we pass the value to
>      gpg_trust_level_to_str(), which complains that the value is out of
>      range with a BUG(). This behavior was introduced by 803978da49
>      (gpg-interface: add function for converting trust level to string,
>      2022-07-11). Before that, we just did a switch() on the enum, and
>      anything that wasn't matched would end up as the empty string.
>
>      Curiously, solving this by naively doing:
>
>        if (level < 0)
>                return "";
>
>      in that function isn't sufficient. Because of (1) above, the
>      compiler can (and does in my case) actually remove that conditional
>      as dead code!
>
> We can solve both by representing this state as an enum value. We could
> do this by adding a new "unknown" value. But this really seems to match
> the existing "undefined" level well. GPG describes this as "Not enough
> information for calculation".
>
> We have tests in t7510 that trigger this case (verifying a signature
> from a key that we don't have, and then checking various %G
> placeholders), but they didn't notice the BUG() because we didn't look
> at %GT for that case! Let's make sure we check all %G placeholders for
> each case in the formatting tests.
>
> The interesting ones here are "show unknown signature with custom
> format" and "show lack of signature with custom format", both of which
> would BUG() before, and now turn %GT into "undefined". Prior to
> 803978da49 they would have turned it into the empty string, but I think
> saying "undefined" consistently is a reasonable outcome, and probably
> makes life easier for anyone parsing the output (and any such parser had
> to be ready to see "undefined" already).
>
> The other modified tests produce the same output before and after this
> patch, but now we're consistently checking both %G? and %GT in all of
> them.
>
> Signed-off-by: Jeff King <peff@peff.net>
> Reported-by: Rolf Eike Beer <eb@emlix.com>
> ---
>  gpg-interface.c          |  2 +-
>  t/t7510-signed-commit.sh | 21 ++++++++++++++-------
>  2 files changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/gpg-interface.c b/gpg-interface.c
> index aceeb08336..f3ac5acdd9 100644
> --- a/gpg-interface.c
> +++ b/gpg-interface.c
> @@ -650,7 +650,7 @@ int check_signature(struct signature_check *sigc,
>  	gpg_interface_lazy_init();
>  
>  	sigc->result = 'N';
> -	sigc->trust_level = -1;
> +	sigc->trust_level = TRUST_UNDEFINED;
>  
>  	fmt = get_format_by_sig(signature);
>  	if (!fmt)
> diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
> index 48f86cb367..ccbc416402 100755
> --- a/t/t7510-signed-commit.sh
> +++ b/t/t7510-signed-commit.sh
> @@ -221,84 +221,91 @@ test_expect_success GPG 'amending already signed commit' '
>  test_expect_success GPG 'show good signature with custom format' '
>  	cat >expect <<-\EOF &&
>  	G
> +	ultimate
>  	13B6F51ECDDE430D
>  	C O Mitter <committer@example.com>
>  	73D758744BE721698EC54E8713B6F51ECDDE430D
>  	73D758744BE721698EC54E8713B6F51ECDDE430D
>  	EOF
> -	git log -1 --format="%G?%n%GK%n%GS%n%GF%n%GP" sixth-signed >actual &&
> +	git log -1 --format="%G?%n%GT%n%GK%n%GS%n%GF%n%GP" sixth-signed >actual &&
>  	test_cmp expect actual
>  '
>  
>  test_expect_success GPG 'show bad signature with custom format' '
>  	cat >expect <<-\EOF &&
>  	B
> +	undefined
>  	13B6F51ECDDE430D
>  	C O Mitter <committer@example.com>
>  
>  
>  	EOF
> -	git log -1 --format="%G?%n%GK%n%GS%n%GF%n%GP" $(cat forged1.commit) >actual &&
> +	git log -1 --format="%G?%n%GT%n%GK%n%GS%n%GF%n%GP" $(cat forged1.commit) >actual &&
>  	test_cmp expect actual
>  '
>  
>  test_expect_success GPG 'show untrusted signature with custom format' '
>  	cat >expect <<-\EOF &&
>  	U
> +	undefined
>  	65A0EEA02E30CAD7
>  	Eris Discordia <discord@example.net>
>  	F8364A59E07FFE9F4D63005A65A0EEA02E30CAD7
>  	D4BE22311AD3131E5EDA29A461092E85B7227189
>  	EOF
> -	git log -1 --format="%G?%n%GK%n%GS%n%GF%n%GP" eighth-signed-alt >actual &&
> +	git log -1 --format="%G?%n%GT%n%GK%n%GS%n%GF%n%GP" eighth-signed-alt >actual &&
>  	test_cmp expect actual
>  '
>  
>  test_expect_success GPG 'show untrusted signature with undefined trust level' '
>  	cat >expect <<-\EOF &&
> +	U
>  	undefined
>  	65A0EEA02E30CAD7
>  	Eris Discordia <discord@example.net>
>  	F8364A59E07FFE9F4D63005A65A0EEA02E30CAD7
>  	D4BE22311AD3131E5EDA29A461092E85B7227189
>  	EOF
> -	git log -1 --format="%GT%n%GK%n%GS%n%GF%n%GP" eighth-signed-alt >actual &&
> +	git log -1 --format="%G?%n%GT%n%GK%n%GS%n%GF%n%GP" eighth-signed-alt >actual &&
>  	test_cmp expect actual
>  '
>  
>  test_expect_success GPG 'show untrusted signature with ultimate trust level' '
>  	cat >expect <<-\EOF &&
> +	G
>  	ultimate
>  	13B6F51ECDDE430D
>  	C O Mitter <committer@example.com>
>  	73D758744BE721698EC54E8713B6F51ECDDE430D
>  	73D758744BE721698EC54E8713B6F51ECDDE430D
>  	EOF
> -	git log -1 --format="%GT%n%GK%n%GS%n%GF%n%GP" sixth-signed >actual &&
> +	git log -1 --format="%G?%n%GT%n%GK%n%GS%n%GF%n%GP" sixth-signed >actual &&
>  	test_cmp expect actual
>  '
>  
>  test_expect_success GPG 'show unknown signature with custom format' '
>  	cat >expect <<-\EOF &&
>  	E
> +	undefined
>  	65A0EEA02E30CAD7
>  
>  
>  
>  	EOF
> -	GNUPGHOME="$GNUPGHOME_NOT_USED" git log -1 --format="%G?%n%GK%n%GS%n%GF%n%GP" eighth-signed-alt >actual &&
> +	GNUPGHOME="$GNUPGHOME_NOT_USED" git log -1 --format="%G?%n%GT%n%GK%n%GS%n%GF%n%GP" eighth-signed-alt >actual &&
>  	test_cmp expect actual
>  '
>  
>  test_expect_success GPG 'show lack of signature with custom format' '
>  	cat >expect <<-\EOF &&
>  	N
> +	undefined
>  
>  
>  
>  
>  	EOF
> -	git log -1 --format="%G?%n%GK%n%GS%n%GF%n%GP" seventh-unsigned >actual &&
> +	git log -1 --format="%G?%n%GT%n%GK%n%GS%n%GF%n%GP" seventh-unsigned >actual &&
>  	test_cmp expect actual
>  '