{"thread":{"id":"60860","subject":"Wrong exit code on failed SSH signing","startedAt":"2024-02-06T16:24:13Z","lastAt":"2024-02-06T21:25:42Z","messageCount":2,"participants":["Sergey Kosukhin","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"488076","messageId":"CAGMF1KiFNnr1nFBg2+mRqiurXpxOOXAcoWc0GciRKDoWzpJSkA@mail.gmail.com","threadId":"60860","inReplyTo":null,"subject":"Wrong exit code on failed SSH signing","fromName":"Sergey Kosukhin","fromEmail":"skosukhin@gmail.com","sentAt":"2024-02-06T16:24:01Z","receivedAt":"2024-02-06T16:24:13Z","isPatch":false,"sender":{"key":"skosukhin@gmail.com","avatar":null},"body":"Hello!\n\nThere seems to be a bug in the sign_buffer_ssh function in\ngpg-interface.c: a possible exit code of ssh-keygen is 255, which is\nreturned as-is by sign_buffer_ssh. The problem is that, for example,\nthe function build_tag_object in builtin/tag.c considers only negative\nvalues as a failure. Since 255 >= 0, the error message \"unable to sign\nthe tag\" is not emitted and git exits normally with zero exit code. It\nmight be enough to return -1 in sign_buffer_ssh if ret is not zero.\n\nI am sorry if this has already been reported or taken care of. Thank you.\n\nBest regards,\nSergey\n"},{"id":"488094","messageId":"xmqqy1bxffov.fsf@gitster.g","threadId":"60860","inReplyTo":"CAGMF1KiFNnr1nFBg2+mRqiurXpxOOXAcoWc0GciRKDoWzpJSkA@mail.gmail.com","subject":"Re: Wrong exit code on failed SSH signing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-06T21:25:36Z","receivedAt":"2024-02-06T21:25:42Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sergey Kosukhin <skosukhin@gmail.com> writes:\n\n> There seems to be a bug in the sign_buffer_ssh function in\n> gpg-interface.c: a possible exit code of ssh-keygen is 255, which is\n> returned as-is by sign_buffer_ssh. The problem is that, for example,\n> the function build_tag_object in builtin/tag.c considers only negative\n> values as a failure. Since 255 >= 0, the error message \"unable to sign\n> the tag\" is not emitted and git exits normally with zero exit code. It\n> might be enough to return -1 in sign_buffer_ssh if ret is not zero.\n\nThanks for noticing and an excellent initial diagnosis.\n\nThere are three callers of sign_buffer():\n\n - send-pack.c:generate_push_cert() lets the user sign the push\n   certificate, and non-zero return from sign_buffer() is a sign\n   that we failed to sign.\n\n - commit.c:sign_with_header() lets the user sign a commit object,\n   and non-zero return from sign_buffer() is taken as an error.\n\n - builtin/tag.c:do_sign() calls sign_buffer() and propagates the\n   return value to its caller, which assumes that positive return\n   values are not errors.\n\nIt seems to me that what needs fixing is the last caller.  Perhaps\ninside \"git tag\" implementation, there is a local convention that\nerrors are signaled with negative values, and that is fine, but then\nbuiltin/tag.c:do_sign() should be doing the same translation as\nbuiltin/tag.c:verify_tag() does, I would say.\n\nThe latter calls gpg_verify_tag() and upon non-zero return,\ntranslates that to a return of -1 to its caller, like so:\n\n        static int verify_tag(const char *name, const char *ref UNUSED,\n                              const struct object_id *oid, void *cb_data)\n        {\n                int flags;\n                struct ref_format *format = cb_data;\n                flags = GPG_VERIFY_VERBOSE;\n\n                if (format->format)\n                        flags = GPG_VERIFY_OMIT_STATUS;\n\n                if (gpg_verify_tag(oid, name, flags))\n                        return -1;\n\n                ...\n\nSo perhaps something like this with a proper log message would be a\nbetter fix?\n\n builtin/tag.c   | 2 +-\n gpg-interface.h | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git c/builtin/tag.c w/builtin/tag.c\nindex f036cf32f5..37473ac21f 100644\n--- c/builtin/tag.c\n+++ w/builtin/tag.c\n@@ -153,7 +153,7 @@ static int verify_tag(const char *name, const char *ref UNUSED,\n \n static int do_sign(struct strbuf *buffer)\n {\n-\treturn sign_buffer(buffer, buffer, get_signing_key());\n+\treturn sign_buffer(buffer, buffer, get_signing_key()) ? -1 : 0;\n }\n \n static const char tag_template[] =\ndiff --git c/gpg-interface.h w/gpg-interface.h\nindex 143cdc1c02..7cd98161f7 100644\n--- c/gpg-interface.h\n+++ w/gpg-interface.h\n@@ -66,7 +66,7 @@ size_t parse_signed_buffer(const char *buf, size_t size);\n  * Create a detached signature for the contents of \"buffer\" and append\n  * it after \"signature\"; \"buffer\" and \"signature\" can be the same\n  * strbuf instance, which would cause the detached signature appended\n- * at the end.\n+ * at the end.  Returns 0 on success, non-zero on failure.\n  */\n int sign_buffer(struct strbuf *buffer, struct strbuf *signature,\n \t\tconst char *signing_key);\n"}]}