# [PATCH 2/2] git-tag -s must fail if gpg is broken and cannot sign tags 5 messages from 2007-09-06 to 2007-09-08. Participants: Shawn O. Pearce, Junio C Hamano, Carlos Rica. Thread: https://gitlist.dev/t/9797 ## Shawn O. Pearce, 2007-09-06 04:21 Subject: [PATCH 2/2] git-tag -s must fail if gpg is broken and cannot sign tags Message-ID: <20070906042115.GA343@spearce.org> URL: https://gitlist.dev/e/20070906042115.GA343%40spearce.org ``` If the user has misconfigured `user.signingkey` in their .git/config or just doesn't have any secret keys on their keyring and they ask for a signed tag with `git tag -s` we better make sure the resulting tag was actually signed by gpg. Prior versions of builtin git-tag allowed this failure to slip by without error as they were not checking the return value of the finish_command() so they did not notice when gpg exited with an error exit status. They also did not fail if gpg produced an empty output or if read_in_full received an error from the read system call while trying to read the pipe back from gpg. Finally we did not actually honor any return value from the do_sign function as it returns ssize_t but was being stored into an unsigned long. This caused the compiler to optimize out the die condition, allowing git-tag to continue along and create the tag object. With these issues fixed `git-tag -s` will now fail to create the tag and will report a non-zero exit status to its caller, thereby allowing automated helper scripts to detect (and recover from) failure if gpg is not working properly. Signed-off-by: Shawn O. Pearce --- I think this and my prior contrib/workdir patch should both go into maint. This one in particular; it hurt us today when an automated tool that runs `git tag -s` didn't notice the GnuPG problems. builtin-tag.c | 8 +++++--- t/t7004-tag.sh | 7 +++++++ 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/builtin-tag.c b/builtin-tag.c index 348919c..aadf850 100644 --- a/builtin-tag.c +++ b/builtin-tag.c @@ -217,7 +217,8 @@ static ssize_t do_sign(char *buffer, size_t size, size_t max) gpg.close_in = 0; len = read_in_full(gpg.out, buffer + size, max - size); - finish_command(&gpg); + if (finish_command(&gpg) || !len || len < 0) + return error("gpg failed to sign the tag"); if (len == max - size) return error("could not read the entire signature from gpg."); @@ -310,9 +311,10 @@ static void create_tag(const unsigned char *object, const char *tag, size += header_len; if (sign) { - size = do_sign(buffer, size, max_size); - if (size < 0) + ssize_t r = do_sign(buffer, size, max_size); + if (r < 0) die("unable to sign the tag"); + size = r; } if (write_sha1_file(buffer, size, tag_type, result) < 0) diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh index 606d4f2..0d07bc3 100755 --- a/t/t7004-tag.sh +++ b/t/t7004-tag.sh @@ -990,6 +990,13 @@ test_expect_success \ git diff expect actual ' +# try to sign with bad user.signingkey +git config user.signingkey BobTheMouse +test_expect_failure \ + 'git-tag -s fails if gpg is misconfigured' \ + 'git tag -s -m tail tag-gpg-failure' +git config --unset user.signingkey + # try to verify without gpg: rm -rf gpghome -- 1.5.3.1.840.g0fedbc ``` ## Shawn O. Pearce, 2007-09-06 04:26 Subject: Re: [PATCH 2/2] git-tag -s must fail if gpg is broken and cannot sign tags Message-ID: <20070906042653.GQ18160@spearce.org> URL: https://gitlist.dev/e/20070906042653.GQ18160%40spearce.org In-Reply-To: <20070906042115.GA343@spearce.org> ``` "Shawn O. Pearce" wrote: > If the user has misconfigured `user.signingkey` in their .git/config > or just doesn't have any secret keys on their keyring and they ask > for a signed tag with `git tag -s` we better make sure the resulting > tag was actually signed by gpg. ... > I think this and my prior contrib/workdir patch should both go into > maint. This one in particular; it hurt us today when an automated > tool that runs `git tag -s` didn't notice the GnuPG problems. I'm sorry, despite the subject of this email this is actually a *one* patch series. The 2/2 is because I applied and tested this on top of the contrib/workdir patch I was talking about above and my email sending script thought it was a two patch series. -- Shawn. ``` ## Junio C Hamano, 2007-09-06 06:20 Subject: Re: [PATCH 2/2] git-tag -s must fail if gpg is broken and cannot sign tags Message-ID: <7vd4wwe2n5.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vd4wwe2n5.fsf%40gitster.siamese.dyndns.org In-Reply-To: <20070906042653.GQ18160@spearce.org> ``` "Shawn O. Pearce" writes: > "Shawn O. Pearce" wrote: >> If the user has misconfigured `user.signingkey` in their .git/config >> or just doesn't have any secret keys on their keyring and they ask >> for a signed tag with `git tag -s` we better make sure the resulting >> tag was actually signed by gpg. > ... >> I think this and my prior contrib/workdir patch should both go into >> maint. This one in particular; it hurt us today when an automated >> tool that runs `git tag -s` didn't notice the GnuPG problems. > > I'm sorry, despite the subject of this email this is actually a > *one* patch series. The 2/2 is because I applied and tested this > on top of the contrib/workdir patch I was talking about above and > my email sending script thought it was a two patch series. This seems to fail the test depending on the order processes happen to be scheduled. I haven't looked at it closely yet. ``` ## Shawn O. Pearce, 2007-09-07 04:58 Subject: Re: [PATCH 2/2] git-tag -s must fail if gpg is broken and cannot sign tags Message-ID: <20070907045833.GZ18160@spearce.org> URL: https://gitlist.dev/e/20070907045833.GZ18160%40spearce.org In-Reply-To: <7vd4wwe2n5.fsf@gitster.siamese.dyndns.org> ``` Junio C Hamano wrote: > "Shawn O. Pearce" writes: > > > "Shawn O. Pearce" wrote: > >> If the user has misconfigured `user.signingkey` in their .git/config > >> or just doesn't have any secret keys on their keyring and they ask > >> for a signed tag with `git tag -s` we better make sure the resulting > >> tag was actually signed by gpg. > > This seems to fail the test depending on the order processes > happen to be scheduled. I haven't looked at it closely yet. That's not good. I noticed stepping through the code last night that if gpg is misconfigured (e.g. set a bad user.signingkey in .git/config) it will terminate and send SIGPIPE to git-tag, which makes it terminate. All my change did was implement proper error handling. So if you are seeing failures now then we probably have a problem with the code without my patch too... -- Shawn. ``` ## Carlos Rica, 2007-09-08 05:41 Subject: Re: [PATCH 2/2] git-tag -s must fail if gpg is broken and cannot sign tags Message-ID: <1b46aba20709072241v2a6b071tdeb0e9fc35ea217d@mail.gmail.com> URL: https://gitlist.dev/e/1b46aba20709072241v2a6b071tdeb0e9fc35ea217d%40mail.gmail.com In-Reply-To: <20070907045833.GZ18160@spearce.org> ``` 2007/9/7, Shawn O. Pearce : > Junio C Hamano wrote: > > "Shawn O. Pearce" writes: > > > > > "Shawn O. Pearce" wrote: > > >> If the user has misconfigured `user.signingkey` in their .git/config > > >> or just doesn't have any secret keys on their keyring and they ask > > >> for a signed tag with `git tag -s` we better make sure the resulting > > >> tag was actually signed by gpg. > > > > This seems to fail the test depending on the order processes > > happen to be scheduled. I haven't looked at it closely yet. > > That's not good. I noticed stepping through the code last night > that if gpg is misconfigured (e.g. set a bad user.signingkey in > .git/config) it will terminate and send SIGPIPE to git-tag, which > makes it terminate. I haven't tested it enough, but now I know that the program is terminated in write_or_die(gpg.in, buffer, size), and it is passing the test or not depending on the system, because I added some code before the test and then it worked for me and if I remove that test, it is failing again. These messages are printed: gpg: skipped "BobTheMouse": secret key not available gpg: signing failed: secret key not available Just after start_command and before write_in_full. Possibly the reason is that code in write_in_full() that makes exit(0) without a warning when EPIPE is returned, or possibly is write() in xwrite(), that dies directly when EPIPE is received like it was for builtin-verify-tag.c. Catching the signal EPIPE doesn't worked for me, so I will do some checks more to trace the code more exactly in my system. > All my change did was implement proper error handling. So if you > are seeing failures now then we probably have a problem with the > code without my patch too... The test seems to fail also without your patch, as you say. ```