Re: [PATCH 1/1] verify-tag/verify-commit should exit unsuccessfully when signature is not trusted

Jeff King [off-list ref] writes:

There was a patch at the start of this thread, but it specifically
checks for "sigc->result == U".  That's probably OK, since I think it
restores the behavior in earlier versions of Git. But I wonder if we
should simply be storing the fact that gpg exited non-zero and relaying
that. That would fix this problem and truly make the rule "if gpg
reported an error, we propagate that".

Yeah, I like that.  Something like this, perhaps?  Points to note:

 * status gets the return value from verify_signed_buffer(), which
   essentially is what wait_or_whine() gives us for the "gpg
   --verify" process.

 * Even if status says "failed", we still need to parse the output
   to set sigc->result.  We used to use sigc->result as the sole
   source of our return value, but now we turn 'status' into 'bad'
   (i.e. non-zero) after parsing and finding it is not mechanically
   good (which is the same criteria as we have always used before).
   An already bad status is left as bad.

 * And we return 'status'.

If we choose to blindly trust the exit status of "gpg --verify" and
not interpret the result ourselves, we can lose the "smudge status
to be bad if not G/U" bit, which I offhand do not think makes much
difference either way.  I just left it there because showing what
can be removed and saying it can be dropped is easier than showing
the result of removal and saying it can be added--simply because I
need to describe "it" if I go the latter route.

 gpg-interface.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/gpg-interface.c b/gpg-interface.c
index 09ddfbc267..2e0885c511 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c

@@ -67,26 +67,27 @@ static void parse_gpg_output(struct signature_check *sigc)
 int check_signature(const char *payload, size_t plen, const char *signature,
 	size_t slen, struct signature_check *sigc)
 {
 	struct strbuf gpg_output = STRBUF_INIT;
 	struct strbuf gpg_status = STRBUF_INIT;
 	int status;
 
 	sigc->result = 'N';
 
 	status = verify_signed_buffer(payload, plen, signature, slen,
 				      &gpg_output, &gpg_status);
 	if (status && !gpg_output.len)
 		goto out;
 	sigc->payload = xmemdupz(payload, plen);
 	sigc->gpg_output = strbuf_detach(&gpg_output, NULL);
 	sigc->gpg_status = strbuf_detach(&gpg_status, NULL);
 	parse_gpg_output(sigc);
+	status |= sigc->result != 'G' && sigc->result != 'U';
 
  out:
 	strbuf_release(&gpg_status);
 	strbuf_release(&gpg_output);
 
-	return sigc->result != 'G' && sigc->result != 'U';
+	return !!status;
 }
 
 void print_signature_buffer(const struct signature_check *sigc, unsigned flags)

On Thu, Aug 09, 2018 at 08:30:25AM -0700, Junio C Hamano wrote:

Jeff King [off-list ref] writes:

quoted

There was a patch at the start of this thread, but it specifically
checks for "sigc->result == U".  That's probably OK, since I think it
restores the behavior in earlier versions of Git. But I wonder if we
should simply be storing the fact that gpg exited non-zero and relaying
that. That would fix this problem and truly make the rule "if gpg
reported an error, we propagate that".

Yeah, I like that.  Something like this, perhaps?  Points to note:

 * status gets the return value from verify_signed_buffer(), which
   essentially is what wait_or_whine() gives us for the "gpg
   --verify" process.

 * Even if status says "failed", we still need to parse the output
   to set sigc->result.  We used to use sigc->result as the sole
   source of our return value, but now we turn 'status' into 'bad'
   (i.e. non-zero) after parsing and finding it is not mechanically
   good (which is the same criteria as we have always used before).
   An already bad status is left as bad.

 * And we return 'status'.

Yeah, this is exactly what I had in mind. And the size of the code
change is much smaller than I feared. The case that I thought might be
complicated is still reading the output after we've seen the non-zero
status, but the existing "if (status && !gpg_output.len)" covers that.

If we choose to blindly trust the exit status of "gpg --verify" and
not interpret the result ourselves, we can lose the "smudge status
to be bad if not G/U" bit, which I offhand do not think makes much
difference either way.  I just left it there because showing what
can be removed and saying it can be dropped is easier than showing
the result of removal and saying it can be added--simply because I
need to describe "it" if I go the latter route.

I guess leaving it serves as a sort of cross-check if gpg would return a
zero exit code but indicate in the status result that the signature was
not good. Sort of a belt-and-suspenders, I guess (which might not be
that implausible if we think about somebody wrapping gpg with a sloppy
bit of shell code that loses the exit code -- it's their fault, but it
might be nice for us to err on the conservative side).

Probably it should go back to just "result != G" then, though (thus
bringing the whole conversation full circle :) ).

I could live with or without it, though.

-Peff