* [PATCH] tag: fix sign_buffer() call to create a signed tag
@ 2024-02-07 18:46 Junio C Hamano
2024-02-08 0:47 ` Jeff King
0 siblings, 1 reply; 6+ messages in thread
From: Junio C Hamano @ 2024-02-07 18:46 UTC (permalink / raw)
To: git; +Cc: Sergey Kosukhin
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] tag: fix sign_buffer() call to create a signed tag
2024-02-07 18:46 [PATCH] tag: fix sign_buffer() call to create a signed tag Junio C Hamano
@ 2024-02-08 0:47 ` Jeff King
2024-02-08 3:08 ` Junio C Hamano
0 siblings, 1 reply; 6+ messages in thread
From: Jeff King @ 2024-02-08 0:47 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Sergey Kosukhin
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] tag: fix sign_buffer() call to create a signed tag
2024-02-08 0:47 ` Jeff King
@ 2024-02-08 3:08 ` Junio C Hamano
2024-02-08 5:29 ` Junio C Hamano
2024-02-08 20:26 ` Jeff King
0 siblings, 2 replies; 6+ messages in thread
From: Junio C Hamano @ 2024-02-08 3:08 UTC (permalink / raw)
To: Jeff King; +Cc: git, Sergey Kosukhin
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] tag: fix sign_buffer() call to create a signed tag
2024-02-08 3:08 ` Junio C Hamano
@ 2024-02-08 5:29 ` Junio C Hamano
2024-02-08 21:27 ` Jeff King
2024-02-08 20:26 ` Jeff King
1 sibling, 1 reply; 6+ messages in thread
From: Junio C Hamano @ 2024-02-08 5:29 UTC (permalink / raw)
To: Jeff King; +Cc: git, Sergey Kosukhin
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] tag: fix sign_buffer() call to create a signed tag
2024-02-08 3:08 ` Junio C Hamano
2024-02-08 5:29 ` Junio C Hamano
@ 2024-02-08 20:26 ` Jeff King
1 sibling, 0 replies; 6+ messages in thread
From: Jeff King @ 2024-02-08 20:26 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Sergey Kosukhin
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] tag: fix sign_buffer() call to create a signed tag
2024-02-08 5:29 ` Junio C Hamano
@ 2024-02-08 21:27 ` Jeff King
0 siblings, 0 replies; 6+ messages in thread
From: Jeff King @ 2024-02-08 21:27 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Sergey Kosukhin
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-02-08 21:27 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-02-07 18:46 [PATCH] tag: fix sign_buffer() call to create a signed tag Junio C Hamano
2024-02-08 0:47 ` Jeff King
2024-02-08 3:08 ` Junio C Hamano
2024-02-08 5:29 ` Junio C Hamano
2024-02-08 21:27 ` Jeff King
2024-02-08 20:26 ` Jeff King
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.