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> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Jeff King committed Apr 18, 2023 at 21:29 UTC 7891e465856e539c4a102dadec6dca9ac51c38df
2 files changed +15 -8
gpg-interface.c
+1 -1
@@ -633,7 +633,7 @@ int check_signature(struct signature_check *sigc,
633 int status;
634
635 sigc->result = 'N';
636 - sigc->trust_level = -1;
636 + sigc->trust_level = TRUST_UNDEFINED;
637
638 fmt = get_format_by_sig(signature);
639 if (!fmt)
t/t7510-signed-commit.sh
+14 -7
@@ -221,84 +221,91 @@ test_expect_success GPG 'amending already signed commit' '
221 test_expect_success GPG 'show good signature with custom format' '
222 cat >expect <<-\EOF &&
223 G
224 + ultimate
225 13B6F51ECDDE430D
226 C O Mitter <committer@example.com>
227 73D758744BE721698EC54E8713B6F51ECDDE430D
228 73D758744BE721698EC54E8713B6F51ECDDE430D
229 EOF
229 - git log -1 --format="%G?%n%GK%n%GS%n%GF%n%GP" sixth-signed >actual &&
230 + git log -1 --format="%G?%n%GT%n%GK%n%GS%n%GF%n%GP" sixth-signed >actual &&
231 test_cmp expect actual
232 '
233
234 test_expect_success GPG 'show bad signature with custom format' '
235 cat >expect <<-\EOF &&
236 B
237 + undefined
238 13B6F51ECDDE430D
239 C O Mitter <committer@example.com>
240
241
242 EOF
241 - git log -1 --format="%G?%n%GK%n%GS%n%GF%n%GP" $(cat forged1.commit) >actual &&
243 + git log -1 --format="%G?%n%GT%n%GK%n%GS%n%GF%n%GP" $(cat forged1.commit) >actual &&
244 test_cmp expect actual
245 '
246
247 test_expect_success GPG 'show untrusted signature with custom format' '
248 cat >expect <<-\EOF &&
249 U
250 + undefined
251 65A0EEA02E30CAD7
252 Eris Discordia <discord@example.net>
253 F8364A59E07FFE9F4D63005A65A0EEA02E30CAD7
254 D4BE22311AD3131E5EDA29A461092E85B7227189
255 EOF
253 - git log -1 --format="%G?%n%GK%n%GS%n%GF%n%GP" eighth-signed-alt >actual &&
256 + git log -1 --format="%G?%n%GT%n%GK%n%GS%n%GF%n%GP" eighth-signed-alt >actual &&
257 test_cmp expect actual
258 '
259
260 test_expect_success GPG 'show untrusted signature with undefined trust level' '
261 cat >expect <<-\EOF &&
262 + U
263 undefined
264 65A0EEA02E30CAD7
265 Eris Discordia <discord@example.net>
266 F8364A59E07FFE9F4D63005A65A0EEA02E30CAD7
267 D4BE22311AD3131E5EDA29A461092E85B7227189
268 EOF
265 - git log -1 --format="%GT%n%GK%n%GS%n%GF%n%GP" eighth-signed-alt >actual &&
269 + git log -1 --format="%G?%n%GT%n%GK%n%GS%n%GF%n%GP" eighth-signed-alt >actual &&
270 test_cmp expect actual
271 '
272
273 test_expect_success GPG 'show untrusted signature with ultimate trust level' '
274 cat >expect <<-\EOF &&
275 + G
276 ultimate
277 13B6F51ECDDE430D
278 C O Mitter <committer@example.com>
279 73D758744BE721698EC54E8713B6F51ECDDE430D
280 73D758744BE721698EC54E8713B6F51ECDDE430D
281 EOF
277 - git log -1 --format="%GT%n%GK%n%GS%n%GF%n%GP" sixth-signed >actual &&
282 + git log -1 --format="%G?%n%GT%n%GK%n%GS%n%GF%n%GP" sixth-signed >actual &&
283 test_cmp expect actual
284 '
285
286 test_expect_success GPG 'show unknown signature with custom format' '
287 cat >expect <<-\EOF &&
288 E
289 + undefined
290 65A0EEA02E30CAD7
291
292
293
294 EOF
289 - GNUPGHOME="$GNUPGHOME_NOT_USED" git log -1 --format="%G?%n%GK%n%GS%n%GF%n%GP" eighth-signed-alt >actual &&
295 + GNUPGHOME="$GNUPGHOME_NOT_USED" git log -1 --format="%G?%n%GT%n%GK%n%GS%n%GF%n%GP" eighth-signed-alt >actual &&
296 test_cmp expect actual
297 '
298
299 test_expect_success GPG 'show lack of signature with custom format' '
300 cat >expect <<-\EOF &&
301 N
302 + undefined
303
304
305
306
307 EOF
301 - git log -1 --format="%G?%n%GK%n%GS%n%GF%n%GP" seventh-unsigned >actual &&
308 + git log -1 --format="%G?%n%GT%n%GK%n%GS%n%GF%n%GP" seventh-unsigned >actual &&
309 test_cmp expect actual
310 '
311