{"thread":{"id":"9797","subject":"[PATCH 2/2] git-tag -s must fail if gpg is broken and cannot sign tags","startedAt":"2007-09-06T04:21:15Z","lastAt":"2007-09-08T05:41:09Z","messageCount":5,"participants":["Shawn O. Pearce","Junio C Hamano","Carlos Rica"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"52683","messageId":"20070906042115.GA343@spearce.org","threadId":"9797","inReplyTo":null,"subject":"[PATCH 2/2] git-tag -s must fail if gpg is broken and cannot sign tags","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-09-06T04:21:15Z","receivedAt":"2007-09-06T04:21:15Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"If the user has misconfigured `user.signingkey` in their .git/config\nor just doesn't have any secret keys on their keyring and they ask\nfor a signed tag with `git tag -s` we better make sure the resulting\ntag was actually signed by gpg.\n\nPrior versions of builtin git-tag allowed this failure to slip\nby without error as they were not checking the return value of\nthe finish_command() so they did not notice when gpg exited with\nan error exit status. They also did not fail if gpg produced an\nempty output or if read_in_full received an error from the read\nsystem call while trying to read the pipe back from gpg.\n\nFinally we did not actually honor any return value from the do_sign\nfunction as it returns ssize_t but was being stored into an unsigned\nlong. This caused the compiler to optimize out the die condition,\nallowing git-tag to continue along and create the tag object.\n\nWith these issues fixed `git-tag -s` will now fail to create the\ntag and will report a non-zero exit status to its caller, thereby\nallowing automated helper scripts to detect (and recover from)\nfailure if gpg is not working properly.\n\nSigned-off-by: Shawn O. Pearce \n---\n\n I think this and my prior contrib/workdir patch should both go into\n maint. This one in particular; it hurt us today when an automated\n tool that runs `git tag -s` didn't notice the GnuPG problems.\n\n builtin-tag.c | 8 +++++---\n t/t7004-tag.sh | 7 +++++++\n 2 files changed, 12 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin-tag.c b/builtin-tag.c\nindex 348919c..aadf850 100644\n--- a/builtin-tag.c\n+++ b/builtin-tag.c\n@@ -217,7 +217,8 @@ static ssize_t do_sign(char *buffer, size_t size, size_t max)\n \tgpg.close_in = 0;\n \tlen = read_in_full(gpg.out, buffer + size, max - size);\n \n-\tfinish_command(&gpg);\n+\tif (finish_command(&gpg) || !len || len < 0)\n+\t\treturn error(\"gpg failed to sign the tag\");\n \n \tif (len == max - size)\n \t\treturn error(\"could not read the entire signature from gpg.\");\n@@ -310,9 +311,10 @@ static void create_tag(const unsigned char *object, const char *tag,\n \tsize += header_len;\n \n \tif (sign) {\n-\t\tsize = do_sign(buffer, size, max_size);\n-\t\tif (size < 0)\n+\t\tssize_t r = do_sign(buffer, size, max_size);\n+\t\tif (r < 0)\n \t\t\tdie(\"unable to sign the tag\");\n+\t\tsize = r;\n \t}\n \n \tif (write_sha1_file(buffer, size, tag_type, result) < 0)\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex 606d4f2..0d07bc3 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -990,6 +990,13 @@ test_expect_success \\\n \tgit diff expect actual\n '\n \n+# try to sign with bad user.signingkey\n+git config user.signingkey BobTheMouse\n+test_expect_failure \\\n+\t'git-tag -s fails if gpg is misconfigured' \\\n+\t'git tag -s -m tail tag-gpg-failure'\n+git config --unset user.signingkey\n+\n # try to verify without gpg:\n \n rm -rf gpghome\n-- \n1.5.3.1.840.g0fedbc\n"},{"id":"52684","messageId":"20070906042653.GQ18160@spearce.org","threadId":"9797","inReplyTo":"20070906042115.GA343@spearce.org","subject":"Re: [PATCH 2/2] git-tag -s must fail if gpg is broken and cannot sign tags","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-09-06T04:26:53Z","receivedAt":"2007-09-06T04:26:53Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"\"Shawn O. Pearce\" wrote:\n> If the user has misconfigured `user.signingkey` in their .git/config\n> or just doesn't have any secret keys on their keyring and they ask\n> for a signed tag with `git tag -s` we better make sure the resulting\n> tag was actually signed by gpg.\n...\n> I think this and my prior contrib/workdir patch should both go into\n> maint. This one in particular; it hurt us today when an automated\n> tool that runs `git tag -s` didn't notice the GnuPG problems.\n\nI'm sorry, despite the subject of this email this is actually a\n*one* patch series. The 2/2 is because I applied and tested this\non top of the contrib/workdir patch I was talking about above and\nmy email sending script thought it was a two patch series.\n\n-- \nShawn.\n"},{"id":"52702","messageId":"7vd4wwe2n5.fsf@gitster.siamese.dyndns.org","threadId":"9797","inReplyTo":"20070906042653.GQ18160@spearce.org","subject":"Re: [PATCH 2/2] git-tag -s must fail if gpg is broken and cannot sign tags","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-09-06T06:20:14Z","receivedAt":"2007-09-06T06:20:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Shawn O. Pearce\" writes:\n\n> \"Shawn O. Pearce\" wrote:\n>> If the user has misconfigured `user.signingkey` in their .git/config\n>> or just doesn't have any secret keys on their keyring and they ask\n>> for a signed tag with `git tag -s` we better make sure the resulting\n>> tag was actually signed by gpg.\n> ...\n>> I think this and my prior contrib/workdir patch should both go into\n>> maint. This one in particular; it hurt us today when an automated\n>> tool that runs `git tag -s` didn't notice the GnuPG problems.\n>\n> I'm sorry, despite the subject of this email this is actually a\n> *one* patch series. The 2/2 is because I applied and tested this\n> on top of the contrib/workdir patch I was talking about above and\n> my email sending script thought it was a two patch series.\n\nThis seems to fail the test depending on the order processes\nhappen to be scheduled. I haven't looked at it closely yet.\n"},{"id":"52848","messageId":"20070907045833.GZ18160@spearce.org","threadId":"9797","inReplyTo":"7vd4wwe2n5.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 2/2] git-tag -s must fail if gpg is broken and cannot sign tags","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-09-07T04:58:33Z","receivedAt":"2007-09-07T04:58:33Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Junio C Hamano wrote:\n> \"Shawn O. Pearce\" writes:\n> \n> > \"Shawn O. Pearce\" wrote:\n> >> If the user has misconfigured `user.signingkey` in their .git/config\n> >> or just doesn't have any secret keys on their keyring and they ask\n> >> for a signed tag with `git tag -s` we better make sure the resulting\n> >> tag was actually signed by gpg.\n> \n> This seems to fail the test depending on the order processes\n> happen to be scheduled. I haven't looked at it closely yet.\n\nThat's not good. I noticed stepping through the code last night\nthat if gpg is misconfigured (e.g. set a bad user.signingkey in\n.git/config) it will terminate and send SIGPIPE to git-tag, which\nmakes it terminate.\n\nAll my change did was implement proper error handling. So if you\nare seeing failures now then we probably have a problem with the\ncode without my patch too...\n\n-- \nShawn.\n"},{"id":"52973","messageId":"1b46aba20709072241v2a6b071tdeb0e9fc35ea217d@mail.gmail.com","threadId":"9797","inReplyTo":"20070907045833.GZ18160@spearce.org","subject":"Re: [PATCH 2/2] git-tag -s must fail if gpg is broken and cannot sign tags","fromName":"Carlos Rica","fromEmail":"jasampler@gmail.com","sentAt":"2007-09-08T05:41:09Z","receivedAt":"2007-09-08T05:41:09Z","isPatch":true,"sender":{"key":"jasampler@gmail.com","avatar":null},"body":"2007/9/7, Shawn O. Pearce :\n> Junio C Hamano wrote:\n> > \"Shawn O. Pearce\" writes:\n> >\n> > > \"Shawn O. Pearce\" wrote:\n> > >> If the user has misconfigured `user.signingkey` in their .git/config\n> > >> or just doesn't have any secret keys on their keyring and they ask\n> > >> for a signed tag with `git tag -s` we better make sure the resulting\n> > >> tag was actually signed by gpg.\n> >\n> > This seems to fail the test depending on the order processes\n> > happen to be scheduled. I haven't looked at it closely yet.\n>\n> That's not good. I noticed stepping through the code last night\n> that if gpg is misconfigured (e.g. set a bad user.signingkey in\n> .git/config) it will terminate and send SIGPIPE to git-tag, which\n> makes it terminate.\n\nI haven't tested it enough, but now I know that the program is terminated\nin write_or_die(gpg.in, buffer, size), and it is passing the test or not\ndepending on the system, because I added some code before the test\nand then it worked for me and if I remove that test, it is failing again.\nThese messages are printed:\n gpg: skipped \"BobTheMouse\": secret key not available\n gpg: signing failed: secret key not available\nJust after start_command and before write_in_full.\n\nPossibly the reason is that code in write_in_full() that makes exit(0)\nwithout a warning when EPIPE is returned, or possibly is write()\nin xwrite(), that dies directly when EPIPE is received like it was for\nbuiltin-verify-tag.c. Catching the signal EPIPE doesn't worked for me,\nso I will do some checks more to trace the code more exactly\nin my system.\n\n> All my change did was implement proper error handling. So if you\n> are seeing failures now then we probably have a problem with the\n> code without my patch too...\n\nThe test seems to fail also without your patch, as you say.\n"}]}