gpg-interface: set trust level of missing key to "undefined"

Commit Message

Jeff King April 19, 2023, 1:29 a.m. UTC

On Tue, Apr 18, 2023 at 09:24:20AM -0700, Junio C Hamano wrote:

> Jeff King <[e-mail]> writes:
> 
> > which restores the original behavior, or if the original was papering
> > over another bug (e.g., should this be "undefined"?). Certainly the
> > empty string matches other placeholders like %GS for this case (since we
> > obviously don't know anything about the signer).
> 
> Heh, I shouldn't have wasted my cycles in "git log" but in my
> newsreader ;-)
> 
> Looking at the original before the gpg_trust_level_to_str() function
> was introduced, the switch statement looks like it is missing the
> usual "default: BUG()" for unhandled enum.  My version made it mimic
> what ssh side seems to do, but I tend to prefer your empty string
> that differentiates between "we never saw any trust level" and "the
> system says this key should never be trusted".

Actually it gets weirder even, as I think we're violating the C standard
a bit here. ;)

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.

-- >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 <[e-mail]>
Reported-by: Rolf Eike Beer <[e-mail]>
---
 gpg-interface.c          |  2 +-
 t/t7510-signed-commit.sh | 21 ++++++++++++++-------
 2 files changed, 15 insertions(+), 8 deletions(-)

Comments

Junio C Hamano April 19, 2023, 3:30 p.m. UTC | #1

Jeff King <[e-mail]> 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 <[e-mail]>
> Reported-by: Rolf Eike Beer <[e-mail]>
> ---
>  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 <[e-mail]>
>  	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 <[e-mail]>
>  
>  
>  	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 <[e-mail]>
>  	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 <[e-mail]>
>  	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 <[e-mail]>
>  	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
>  '

Jeff King April 22, 2023, 10:47 a.m. UTC | #2

On Wed, Apr 19, 2023 at 08:30:35AM -0700, Junio C Hamano wrote:

> Jeff King <[e-mail]> 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.

One thing that I wondered about for using UNDEFINED is that we do this:

  static enum signature_trust_level configured_min_trust_level = TRUST_UNDEFINED;

which is then later compared with:

  status |= sigc->result != 'G';
  status |= sigc->trust_level < configured_min_trust_level;

So before my patch the uninitialized state is (supposedly) less than the
min level, and after they are the same. For the reasons I gave in the
commit message, I think that less-than comparison was already broken.
And likewise, for the reasons I gave, it hopefully never matters since
the result would never be 'G' in that case.

So I think it's fine, but I definitely had to stare at it for a while.
This all comes from 54887b4689 (gpg-interface: add minTrustLevel as a
configuration option, 2019-12-27), which does discuss some of the
implications, but I think my patch is in line with the logic there.

-Peff

Junio C Hamano April 24, 2023, 4:22 p.m. UTC | #3

Jeff King <[e-mail]> writes:

> So before my patch the uninitialized state is (supposedly) less than the
> min level, and after they are the same. For the reasons I gave in the
> commit message, I think that less-than comparison was already broken.
> And likewise, for the reasons I gave, it hopefully never matters since
> the result would never be 'G' in that case.

Yes * 2.

> So I think it's fine, but I definitely had to stare at it for a while.
> This all comes from 54887b4689 (gpg-interface: add minTrustLevel as a
> configuration option, 2019-12-27), which does discuss some of the
> implications, but I think my patch is in line with the logic there.

Yes.

Thanks.

Patch

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 <[e-mail]>
 	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 <[e-mail]>
 
 
 	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 <[e-mail]>
 	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 <[e-mail]>
 	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 <[e-mail]>
 	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
 '