threads / patch / 60867

patchtag: fix sign_buffer() call to create a signed tag

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

## tl;dr

6 messages between Feb 7, 2024 and Feb 8, 2024. Diffs are folded; open one to read it.

replies: 5people: 2as markdown or json

Junio C Hamano· Feb 7, 2024, 18:46 UTC · lore

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(-)
Show changes to 2 files +2 −2

builtin/tag.c, gpg-interface.h

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
Jeff King· Feb 8, 2024, 00:47 UTC · re: Junio C Hamano · lore

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

On Wed, Feb 07, 2024 at 10:46:54AM -0800, Junio C Hamano wrote:
Show 20 quoted lines
> 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
Junio C Hamano· Feb 8, 2024, 03:08 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 13 quoted lines
>>  * 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· Feb 8, 2024, 05:29 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 5 quoted lines
> 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(-)
Show changes to gpg-interface.c +1 −1
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
Jeff King· Feb 8, 2024, 21:27 UTC · re: Junio C Hamano · lore

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

On Wed, Feb 07, 2024 at 09:29:00PM -0800, Junio C Hamano wrote:
Show 23 quoted lines
> 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
Jeff King· Feb 8, 2024, 20:26 UTC · re: Junio C Hamano · lore

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

On Wed, Feb 07, 2024 at 07:08:37PM -0800, Junio C Hamano wrote:
Show 12 quoted lines
> > 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).

Show 5 quoted lines
> 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

← back to recent threads