[PATCH] tag: fix sign_buffer() call to create a signed tag

[PATCH] tag: fix sign_buffer() call to create a signed tag

[Junio C Hamano]

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(-)

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);
-- 
2.43.0-561-g235986be82

Re: [PATCH] tag: fix sign_buffer() call to create a signed tag

[Jeff King]

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

Re: [PATCH] tag: fix sign_buffer() call to create a signed tag

[Junio C Hamano]

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.

Re: [PATCH] tag: fix sign_buffer() call to create a signed tag

[Junio C Hamano]

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;
 	}
 
-- 
2.43.0-561-g235986be82

Re: [PATCH] tag: fix sign_buffer() call to create a signed tag

[Jeff King]

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

Re: [PATCH] tag: fix sign_buffer() call to create a signed tag

[Jeff King]

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