{"thread":{"id":"60867","subject":"[PATCH] tag: fix sign_buffer() call to create a signed tag","startedAt":"2024-02-07T18:46:59Z","lastAt":"2024-02-08T21:27:12Z","messageCount":6,"participants":["Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"488168","messageId":"xmqq4jek9ko1.fsf@gitster.g","threadId":"60867","inReplyTo":null,"subject":"[PATCH] tag: fix sign_buffer() call to create a signed tag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-07T18:46:54Z","receivedAt":"2024-02-07T18:46:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The command \"git tag -s\" internally calls sign_buffer() to make a\ncryptographic signature using the chosen backend like GPG and SSH.\nThe internal helper functions used by \"git tag\" implementation seem\nto use a \"negative return values are errors, zero or positive return\nvalues are not\" convention, and there are places (e.g., verify_tag()\nthat calls gpg_verify_tag()) that these internal helper functions\ntranslate return values that signal errors to conform to this\nconvention, but do_sign() that calls sign_buffer() forgets to do so.\n\nFix it, so that a failed call to sign_buffer() that can return the\nexit status from pipe_command() will not be overlooked.\n\nReported-by: Sergey Kosukhin \nSigned-off-by: Junio C Hamano \n---\n\n * We alternatively could fix individual sign_buffer() backend that\n signals an error with a positive value (sign_buffer_ssh() in this\n case) to return a negative value, but this would hopefully be\n more future-proof.\n\n builtin/tag.c | 2 +-\n gpg-interface.h | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 3918eacbb5..b28ead06ea 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -176,7 +176,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 a/gpg-interface.h b/gpg-interface.h\nindex 143cdc1c02..7cd98161f7 100644\n--- a/gpg-interface.h\n+++ b/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-- \n2.43.0-561-g235986be82\n\n"},{"id":"488199","messageId":"20240208004757.GA1059751@coredump.intra.peff.net","threadId":"60867","inReplyTo":"xmqq4jek9ko1.fsf@gitster.g","subject":"Re: [PATCH] tag: fix sign_buffer() call to create a signed tag","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-02-08T00:47:57Z","receivedAt":"2024-02-08T00:47:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 07, 2024 at 10:46:54AM -0800, Junio C Hamano wrote:\n\n> The command \"git tag -s\" internally calls sign_buffer() to make a\n> cryptographic signature using the chosen backend like GPG and SSH.\n> The internal helper functions used by \"git tag\" implementation seem\n> to use a \"negative return values are errors, zero or positive return\n> values are not\" convention, and there are places (e.g., verify_tag()\n> that calls gpg_verify_tag()) that these internal helper functions\n> translate return values that signal errors to conform to this\n> convention, but do_sign() that calls sign_buffer() forgets to do so.\n> \n> Fix it, so that a failed call to sign_buffer() that can return the\n> exit status from pipe_command() will not be overlooked.\n> \n> Reported-by: Sergey Kosukhin \n> Signed-off-by: Junio C Hamano \n> ---\n> \n> * We alternatively could fix individual sign_buffer() backend that\n> signals an error with a positive value (sign_buffer_ssh() in this\n> case) to return a negative value, but this would hopefully be\n> more future-proof.\n\nFWIW, I would have gone the other way, and fixed sign_buffer_ssh(). Your\nsolution here is future-proofing the tag code against other\nsign_buffer_*() functions behaving like ssh. But it is also leaving\nother sign_buffer() callers to introduce the same bug.\n\nYour documentation change at least makes that less likely. But given how\nmuch of our code uses the \"negative is error\" convention, I wouldn't be\nsurprised to see it happen anyway.\n\n-Peff\n"},{"id":"488204","messageId":"xmqq5xyzr6tm.fsf@gitster.g","threadId":"60867","inReplyTo":"20240208004757.GA1059751@coredump.intra.peff.net","subject":"Re: [PATCH] tag: fix sign_buffer() call to create a signed tag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-08T03:08:37Z","receivedAt":"2024-02-08T03:08:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> * We alternatively could fix individual sign_buffer() backend that\n>> signals an error with a positive value (sign_buffer_ssh() in this\n>> case) to return a negative value, but this would hopefully be\n>> more future-proof.\n>\n> FWIW, I would have gone the other way, and fixed sign_buffer_ssh(). Your\n> solution here is future-proofing the tag code against other\n> sign_buffer_*() functions behaving like ssh. But it is also leaving\n> other sign_buffer() callers to introduce the same bug.\n>\n> Your documentation change at least makes that less likely. But given how\n> much of our code uses the \"negative is error\" convention, I wouldn't be\n> surprised to see it happen anyway.\n\nYeah, but other callers are prepared to honor the current return\nvalue convention used by gpg-interface, so \"fixing\" sign_buffer_ssh()\nwould not give us any future-proofing.\n\nWe could do belt and suspenders by tightening the other callers to\nonly expect negative for errors (but then what should they do when\nthey receive non-zero positive? Should they BUG() out???) while\nteaching sign_buffer_ssh() that our convention is to return negative\nfor an error, of course, but I am not sure if it that is worth it.\n\nThanks.\n\n"},{"id":"488208","messageId":"xmqqv86zplr7.fsf@gitster.g","threadId":"60867","inReplyTo":"xmqq5xyzr6tm.fsf@gitster.g","subject":"Re: [PATCH] tag: fix sign_buffer() call to create a signed tag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-08T05:29:00Z","receivedAt":"2024-02-08T05:29:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> We could do belt and suspenders by tightening the other callers to\n> only expect negative for errors (but then what should they do when\n> they receive non-zero positive? Should they BUG() out???) while\n> teaching sign_buffer_ssh() that our convention is to return negative\n> for an error, of course, but I am not sure if it that is worth it.\n\nActually, we could loosen the caller(s) while tightening the\ncallee(s), which is the more usual approach we would take in a\nsituation like this. Here is what I am tempted to pile on top of\nthe patch.\n\n----- >8 --------- >8 --------- >8 --------- >8 --------- >8 -----\nSubject: [PATCH] ssh signing: signal an error with a negative return value\n\nThe other backend for the sign_buffer() function followed our usual\n\"an error is signalled with a negative return\" convention, but the\nSSH signer did not. Even though we already fixed the caller that\nassumed only a negative return value is an error, tighten the callee\nto signal an error with a negative return as well. This way, the\ncallees will be strict on what they produce, while the callers will\nbe lenient in what they accept.\n\nSigned-off-by: Junio C Hamano \n---\n gpg-interface.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex 48f43c5a21..e19a69c400 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -1088,7 +1088,7 @@ static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature,\n \t\tif (strstr(signer_stderr.buf, \"usage:\"))\n \t\t\terror(_(\"ssh-keygen -Y sign is needed for ssh signing (available in openssh version 8.2p1+)\"));\n \n-\t\terror(\"%s\", signer_stderr.buf);\n+\t\tret = error(\"%s\", signer_stderr.buf);\n \t\tgoto out;\n \t}\n \n-- \n2.43.0-561-g235986be82\n\n"},{"id":"488253","messageId":"20240208202634.GA1090198@coredump.intra.peff.net","threadId":"60867","inReplyTo":"xmqq5xyzr6tm.fsf@gitster.g","subject":"Re: [PATCH] tag: fix sign_buffer() call to create a signed tag","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-02-08T20:26:34Z","receivedAt":"2024-02-08T20:26:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 07, 2024 at 07:08:37PM -0800, Junio C Hamano wrote:\n\n> > FWIW, I would have gone the other way, and fixed sign_buffer_ssh(). Your\n> > solution here is future-proofing the tag code against other\n> > sign_buffer_*() functions behaving like ssh. But it is also leaving\n> > other sign_buffer() callers to introduce the same bug.\n> >\n> > Your documentation change at least makes that less likely. But given how\n> > much of our code uses the \"negative is error\" convention, I wouldn't be\n> > surprised to see it happen anyway.\n> \n> Yeah, but other callers are prepared to honor the current return\n> value convention used by gpg-interface, so \"fixing\" sign_buffer_ssh()\n> would not give us any future-proofing.\n\nIt future-proofs against a hypothetical new sign_buffer() caller (just\nlike your patch future-proofs against a hypothetical new signing\nbackend).\n\n> We could do belt and suspenders by tightening the other callers to\n> only expect negative for errors (but then what should they do when\n> they receive non-zero positive? Should they BUG() out???) while\n> teaching sign_buffer_ssh() that our convention is to return negative\n> for an error, of course, but I am not sure if it that is worth it.\n\nI'm not sure that's worth it, since we'd only notice if the error\ntriggered (so writing a test).\n\n-Peff\n"},{"id":"488256","messageId":"20240208212710.GA1095331@coredump.intra.peff.net","threadId":"60867","inReplyTo":"xmqqv86zplr7.fsf@gitster.g","subject":"Re: [PATCH] tag: fix sign_buffer() call to create a signed tag","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-02-08T21:27:10Z","receivedAt":"2024-02-08T21:27:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 07, 2024 at 09:29:00PM -0800, Junio C Hamano wrote:\n\n> Junio C Hamano writes:\n> \n> > We could do belt and suspenders by tightening the other callers to\n> > only expect negative for errors (but then what should they do when\n> > they receive non-zero positive? Should they BUG() out???) while\n> > teaching sign_buffer_ssh() that our convention is to return negative\n> > for an error, of course, but I am not sure if it that is worth it.\n> \n> Actually, we could loosen the caller(s) while tightening the\n> callee(s), which is the more usual approach we would take in a\n> situation like this. Here is what I am tempted to pile on top of\n> the patch.\n> \n> ----- >8 --------- >8 --------- >8 --------- >8 --------- >8 -----\n> Subject: [PATCH] ssh signing: signal an error with a negative return value\n> \n> The other backend for the sign_buffer() function followed our usual\n> \"an error is signalled with a negative return\" convention, but the\n> SSH signer did not. Even though we already fixed the caller that\n> assumed only a negative return value is an error, tighten the callee\n> to signal an error with a negative return as well. This way, the\n> callees will be strict on what they produce, while the callers will\n> be lenient in what they accept.\n\nYeah, I think that would possibly lead to fewer surprises and is worth\ndoing.\n\n-Peff\n"}]}