Jeff King [off-list ref] writes:
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).
OK, this time a real log message.
-- >8 --
Subject: [PATCH] gpg-interface: propagate exit status from gpg back to the callers
When gpg-interface API unified support for signature verification
codepaths for signed tags and signed commits in mid 2015 at around
v2.6.0-rc0~114, we accidentally loosened the GPG signature
verification.
Before that change, signed commits were verified by looking for
"G"ood signature from GPG, while ignoring the exit status of "gpg
--verify" process, while signed tags were verified by simply passing
the exit status of "gpg --verify" through. The unified code we
currently have ignores the exit status of "gpg --verify" and returns
successful verification when the signature matches an unexpired key
regardless of the trust placed on the key (i.e. in addition to "G"ood
ones, we accept "U"ntrusted ones).
Make these commands signal failure with their exit status when
underlying "gpg --verify" (or the custom command specified by
"gpg.program" configuration variable) does so. This essentially
changes their behaviour in a backward incompatible way to reject
signatures that have been made with untrusted keys even if they
correctly verify, as that is how "gpg --verify" behaves.
Note that the code still overrides a zero exit status obtained from
"gpg" (or gpg.program) if the output does not say the signature is
good or computes correctly but made with untrusted keys, to catch
a poorly written wrapper around "gpg" the user may give us.
We could exclude "U"ntrusted support from this fallback code, but
that would be making two backward incompatible changes in a single
commit, so let's avoid that for now. A follow-up change could do so
if desired.
Helped-by: Vojtech Myslivec [off-list ref]
Helped-by: brian m. carlson [off-list ref]
Helped-by: Jeff King [off-list ref]
Signed-off-by: Junio C Hamano <redacted>
---
gpg-interface.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/gpg-interface.c b/gpg-interface.c
index 0647bd6348..b5e64c55e2 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -81,12 +81,13 @@ int check_signature(const char *payload, size_t plen, const char *signature,
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)
--
2.18.0-547-g1d89318c48
On Thu, Aug 09, 2018 at 11:40:27AM -0700, Junio C Hamano wrote:
Jeff King [off-list ref] writes:
quoted
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).
OK, this time a real log message.
-- >8 --
Subject: [PATCH] gpg-interface: propagate exit status from gpg back to the callers
[...]
Thanks, the explanation and the patch look good to me.
I'm on the fence over whether a follow-up patch to take away the "U" is
worth it. In practice the code should never trigger either way, since
gpg would already have exited non-zero in such a case.
-Peff
On Thu, Aug 09, 2018 at 11:40:27AM -0700, Junio C Hamano wrote:
-- >8 --
Subject: [PATCH] gpg-interface: propagate exit status from gpg back to the callers
When gpg-interface API unified support for signature verification
codepaths for signed tags and signed commits in mid 2015 at around
v2.6.0-rc0~114, we accidentally loosened the GPG signature
verification.
Before that change, signed commits were verified by looking for
"G"ood signature from GPG, while ignoring the exit status of "gpg
--verify" process, while signed tags were verified by simply passing
the exit status of "gpg --verify" through. The unified code we
currently have ignores the exit status of "gpg --verify" and returns
successful verification when the signature matches an unexpired key
regardless of the trust placed on the key (i.e. in addition to "G"ood
ones, we accept "U"ntrusted ones).
Make these commands signal failure with their exit status when
underlying "gpg --verify" (or the custom command specified by
"gpg.program" configuration variable) does so. This essentially
changes their behaviour in a backward incompatible way to reject
signatures that have been made with untrusted keys even if they
correctly verify, as that is how "gpg --verify" behaves.
Note that the code still overrides a zero exit status obtained from
"gpg" (or gpg.program) if the output does not say the signature is
good or computes correctly but made with untrusted keys, to catch
a poorly written wrapper around "gpg" the user may give us.
We could exclude "U"ntrusted support from this fallback code, but
that would be making two backward incompatible changes in a single
commit, so let's avoid that for now. A follow-up change could do so
if desired.
This looks great to me. Thanks.
--
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204
On 9.8.2018 20:40, Junio C Hamano wrote:
Jeff King [off-list ref] writes:
quoted
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).
OK, this time a real log message.
Now it is possible to achieve git verify-tag/verify-commit exits
unsuccessfully when signatures are not trusted. I would like to
introduce some tests for this behavior to prevent this problem in the
future.
Thank you all for discussion and for solving the issue.
Vojtech and Karel