Wrong exit code on failed SSH signing

Wrong exit code on failed SSH signing

[Sergey Kosukhin]

Hello!

There seems to be a bug in the sign_buffer_ssh function in
gpg-interface.c: a possible exit code of ssh-keygen is 255, which is
returned as-is by sign_buffer_ssh. The problem is that, for example,
the function build_tag_object in builtin/tag.c considers only negative
values as a failure. Since 255 >= 0, the error message "unable to sign
the tag" is not emitted and git exits normally with zero exit code. It
might be enough to return -1 in sign_buffer_ssh if ret is not zero.

I am sorry if this has already been reported or taken care of. Thank you.

Best regards,
Sergey

Re: Wrong exit code on failed SSH signing

[Junio C Hamano]

Sergey Kosukhin <skosukhin@gmail.com> writes:

> There seems to be a bug in the sign_buffer_ssh function in
> gpg-interface.c: a possible exit code of ssh-keygen is 255, which is
> returned as-is by sign_buffer_ssh. The problem is that, for example,
> the function build_tag_object in builtin/tag.c considers only negative
> values as a failure. Since 255 >= 0, the error message "unable to sign
> the tag" is not emitted and git exits normally with zero exit code. It
> might be enough to return -1 in sign_buffer_ssh if ret is not zero.

Thanks for noticing and an excellent initial diagnosis.

There are three callers of sign_buffer():

 - send-pack.c:generate_push_cert() lets the user sign the push
   certificate, and non-zero return from sign_buffer() is a sign
   that we failed to sign.

 - commit.c:sign_with_header() lets the user sign a commit object,
   and non-zero return from sign_buffer() is taken as an error.

 - builtin/tag.c:do_sign() calls sign_buffer() and propagates the
   return value to its caller, which assumes that positive return
   values are not errors.

It seems to me that what needs fixing is the last caller.  Perhaps
inside "git tag" implementation, there is a local convention that
errors are signaled with negative values, and that is fine, but then
builtin/tag.c:do_sign() should be doing the same translation as
builtin/tag.c:verify_tag() does, I would say.

The latter calls gpg_verify_tag() and upon non-zero return,
translates that to a return of -1 to its caller, like so:

        static int verify_tag(const char *name, const char *ref UNUSED,
                              const struct object_id *oid, void *cb_data)
        {
                int flags;
                struct ref_format *format = cb_data;
                flags = GPG_VERIFY_VERBOSE;

                if (format->format)
                        flags = GPG_VERIFY_OMIT_STATUS;

                if (gpg_verify_tag(oid, name, flags))
                        return -1;

                ...

So perhaps something like this with a proper log message would be a
better fix?

 builtin/tag.c   | 2 +-
 gpg-interface.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git c/builtin/tag.c w/builtin/tag.c
index f036cf32f5..37473ac21f 100644
--- c/builtin/tag.c
+++ w/builtin/tag.c
@@ -153,7 +153,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 c/gpg-interface.h w/gpg-interface.h
index 143cdc1c02..7cd98161f7 100644
--- c/gpg-interface.h
+++ w/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);