| Message ID | xmqq4jek9ko1.fsf@gitster.g (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | abfbff61efc4d91dc964eb2360760fa640ad0f0f |
| Headers | show |
| Series | tag: fix sign_buffer() call to create a signed tag | expand |
On Wed, Feb 07, 2024 at 10:46:54AM -0800, Junio C Hamano wrote: > The command "git tag -s" internally calls sign_buffer() to make a > cryptographic signature using the chosen backend like GPG and SSH. > The internal helper functions used by "git tag" implementation seem > to use a "negative return values are errors, zero or positive return > values are not" convention, and there are places (e.g., verify_tag() > that calls gpg_verify_tag()) that these internal helper functions > translate return values that signal errors to conform to this > convention, but do_sign() that calls sign_buffer() forgets to do so. > > Fix it, so that a failed call to sign_buffer() that can return the > exit status from pipe_command() will not be overlooked. > > Reported-by: Sergey Kosukhin <skosukhin@gmail.com> > Signed-off-by: Junio C Hamano <gitster@pobox.com> > --- > > * We alternatively could fix individual sign_buffer() backend that > signals an error with a positive value (sign_buffer_ssh() in this > case) to return a negative value, but this would hopefully be > more future-proof. FWIW, I would have gone the other way, and fixed sign_buffer_ssh(). Your solution here is future-proofing the tag code against other sign_buffer_*() functions behaving like ssh. But it is also leaving other sign_buffer() callers to introduce the same bug. Your documentation change at least makes that less likely. But given how much of our code uses the "negative is error" convention, I wouldn't be surprised to see it happen anyway. -Peff
Jeff King <peff@peff.net> writes: >> * We alternatively could fix individual sign_buffer() backend that >> signals an error with a positive value (sign_buffer_ssh() in this >> case) to return a negative value, but this would hopefully be >> more future-proof. > > FWIW, I would have gone the other way, and fixed sign_buffer_ssh(). Your > solution here is future-proofing the tag code against other > sign_buffer_*() functions behaving like ssh. But it is also leaving > other sign_buffer() callers to introduce the same bug. > > Your documentation change at least makes that less likely. But given how > much of our code uses the "negative is error" convention, I wouldn't be > surprised to see it happen anyway. Yeah, but other callers are prepared to honor the current return value convention used by gpg-interface, so "fixing" sign_buffer_ssh() would not give us any future-proofing. We could do belt and suspenders by tightening the other callers to only expect negative for errors (but then what should they do when they receive non-zero positive? Should they BUG() out???) while teaching sign_buffer_ssh() that our convention is to return negative for an error, of course, but I am not sure if it that is worth it. Thanks.
Junio C Hamano <gitster@pobox.com> writes: > We could do belt and suspenders by tightening the other callers to > only expect negative for errors (but then what should they do when > they receive non-zero positive? Should they BUG() out???) while > teaching sign_buffer_ssh() that our convention is to return negative > for an error, of course, but I am not sure if it that is worth it. Actually, we could loosen the caller(s) while tightening the callee(s), which is the more usual approach we would take in a situation like this. Here is what I am tempted to pile on top of the patch. ----- >8 --------- >8 --------- >8 --------- >8 --------- >8 ----- Subject: [PATCH] ssh signing: signal an error with a negative return value The other backend for the sign_buffer() function followed our usual "an error is signalled with a negative return" convention, but the SSH signer did not. Even though we already fixed the caller that assumed only a negative return value is an error, tighten the callee to signal an error with a negative return as well. This way, the callees will be strict on what they produce, while the callers will be lenient in what they accept. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- gpg-interface.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gpg-interface.c b/gpg-interface.c index 48f43c5a21..e19a69c400 100644 --- a/gpg-interface.c +++ b/gpg-interface.c @@ -1088,7 +1088,7 @@ static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature, if (strstr(signer_stderr.buf, "usage:")) error(_("ssh-keygen -Y sign is needed for ssh signing (available in openssh version 8.2p1+)")); - error("%s", signer_stderr.buf); + ret = error("%s", signer_stderr.buf); goto out; }
On Wed, Feb 07, 2024 at 07:08:37PM -0800, Junio C Hamano wrote: > > FWIW, I would have gone the other way, and fixed sign_buffer_ssh(). Your > > solution here is future-proofing the tag code against other > > sign_buffer_*() functions behaving like ssh. But it is also leaving > > other sign_buffer() callers to introduce the same bug. > > > > Your documentation change at least makes that less likely. But given how > > much of our code uses the "negative is error" convention, I wouldn't be > > surprised to see it happen anyway. > > Yeah, but other callers are prepared to honor the current return > value convention used by gpg-interface, so "fixing" sign_buffer_ssh() > would not give us any future-proofing. It future-proofs against a hypothetical new sign_buffer() caller (just like your patch future-proofs against a hypothetical new signing backend). > We could do belt and suspenders by tightening the other callers to > only expect negative for errors (but then what should they do when > they receive non-zero positive? Should they BUG() out???) while > teaching sign_buffer_ssh() that our convention is to return negative > for an error, of course, but I am not sure if it that is worth it. I'm not sure that's worth it, since we'd only notice if the error triggered (so writing a test). -Peff
On Wed, Feb 07, 2024 at 09:29:00PM -0800, Junio C Hamano wrote: > Junio C Hamano <gitster@pobox.com> writes: > > > We could do belt and suspenders by tightening the other callers to > > only expect negative for errors (but then what should they do when > > they receive non-zero positive? Should they BUG() out???) while > > teaching sign_buffer_ssh() that our convention is to return negative > > for an error, of course, but I am not sure if it that is worth it. > > Actually, we could loosen the caller(s) while tightening the > callee(s), which is the more usual approach we would take in a > situation like this. Here is what I am tempted to pile on top of > the patch. > > ----- >8 --------- >8 --------- >8 --------- >8 --------- >8 ----- > Subject: [PATCH] ssh signing: signal an error with a negative return value > > The other backend for the sign_buffer() function followed our usual > "an error is signalled with a negative return" convention, but the > SSH signer did not. Even though we already fixed the caller that > assumed only a negative return value is an error, tighten the callee > to signal an error with a negative return as well. This way, the > callees will be strict on what they produce, while the callers will > be lenient in what they accept. Yeah, I think that would possibly lead to fewer surprises and is worth doing. -Peff
diff --git a/builtin/tag.c b/builtin/tag.c index 3918eacbb5..b28ead06ea 100644 --- a/builtin/tag.c +++ b/builtin/tag.c @@ -176,7 +176,7 @@ static int verify_tag(const char *name, const char *ref UNUSED, static int do_sign(struct strbuf *buffer) { - return sign_buffer(buffer, buffer, get_signing_key()); + return sign_buffer(buffer, buffer, get_signing_key()) ? -1 : 0; } static const char tag_template[] = diff --git a/gpg-interface.h b/gpg-interface.h index 143cdc1c02..7cd98161f7 100644 --- a/gpg-interface.h +++ b/gpg-interface.h @@ -66,7 +66,7 @@ size_t parse_signed_buffer(const char *buf, size_t size); * Create a detached signature for the contents of "buffer" and append * it after "signature"; "buffer" and "signature" can be the same * strbuf instance, which would cause the detached signature appended - * at the end. + * at the end. Returns 0 on success, non-zero on failure. */ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *signing_key);
The command "git tag -s" internally calls sign_buffer() to make a cryptographic signature using the chosen backend like GPG and SSH. The internal helper functions used by "git tag" implementation seem to use a "negative return values are errors, zero or positive return values are not" convention, and there are places (e.g., verify_tag() that calls gpg_verify_tag()) that these internal helper functions translate return values that signal errors to conform to this convention, but do_sign() that calls sign_buffer() forgets to do so. Fix it, so that a failed call to sign_buffer() that can return the exit status from pipe_command() will not be overlooked. Reported-by: Sergey Kosukhin <skosukhin@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> --- * We alternatively could fix individual sign_buffer() backend that signals an error with a positive value (sign_buffer_ssh() in this case) to return a negative value, but this would hopefully be more future-proof. builtin/tag.c | 2 +- gpg-interface.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)