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

From: Jeff King <peff@peff.net>
Date: Aug 9, 2018, 17:12 UTC
Message-ID: <20180809171219.GF1439@sigill.intra.peff.net>
In-Reply-To: <xmqqtvo3d0by.fsf@gitster-ct.c.googlers.com>

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

Show 23 quoted lines

> Jeff King <peff@peff.net> 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'.

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.

Show 7 quoted lines

> 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