Re: [PATCH] Unbreak interactive GPG prompt upon signing
Hi Michael, On Tue, 6 Sep 2016, Michael J Gruber wrote: > Johannes Schindelin venit, vidit, dixit 06.09.2016 10:01: > > With the recent update in efee955 (gpg-interface: check gpg signature > > creation status, 2016-06-17), we ask GPG to send all status updates to > > stderr, and then catch the stderr in an strbuf. > > > > But GPG might fail, and send error messages to stderr. And we simply > > do not show them to the user. > > > > Even worse: this swallows any interactive prompt for a passphrase. And > > detaches stderr from the tty so that the passphrase cannot be read. > > > > So while the first problem could be fixed (by printing the captured > > stderr upon error), the second problem cannot be easily fixed, and > > presents a major regression. > > My Git has that commit and does ask me for the passphrase on the tty. > Also, I do get error messages: > > git tag -u pebcak -s testt -m m > error: gpg failed to sign the data > error: unable to sign the tag That is not GPG's error message. It just leaves users puzzled, is what it does. > which we could (maybe should) amend by gpg's stderr. Right. But then we still do not solve the problem. The problem being that some platforms cannot use getpass(prompt): it simply does not exist. On Windows, we do not even have a /dev/tty (technically, GPG, being an MSYS2 program, knows about /dev/tty, but we spawn it from a non-MSYS2 program, so there is a disconnect). > > So let's just revert commit efee9553a4f97b2ecd8f49be19606dd4cf7d9c28. > > That "just" reintroduces the problem that the orignal patch solves. Right. Which is: when some user misconfigures gpg, causing Git to run something different that simply succeeds, there is no signature. This is a minor issue, as it requires a user to configure gpg, and do a bad job at it. Not being able to input the passphrase on Windows is a major issue, as the user has done nothing wrong. > The passphrase/tty issue must be Windows specific - or the non-issue > Linux-specific, if you prefer. Sure. Let's talk about semantics. Oh wait, maybe we should work on resolving the issue instead. > > This fixes https://github.com/git-for-windows/git/issues/871 To reiterate: this is the problem I need to see solved. Ciao, Dscho
Re: [PATCH] Unbreak interactive GPG prompt upon signing
Johannes Schindelin venit, vidit, dixit 06.09.2016 10:01: > With the recent update in efee955 (gpg-interface: check gpg signature > creation status, 2016-06-17), we ask GPG to send all status updates to > stderr, and then catch the stderr in an strbuf. > > But GPG might fail, and send error messages to stderr. And we simply > do not show them to the user. > > Even worse: this swallows any interactive prompt for a passphrase. And > detaches stderr from the tty so that the passphrase cannot be read. > > So while the first problem could be fixed (by printing the captured > stderr upon error), the second problem cannot be easily fixed, and > presents a major regression. My Git has that commit and does ask me for the passphrase on the tty. Also, I do get error messages: git tag -u pebcak -s testt -m m error: gpg failed to sign the data error: unable to sign the tag which we could (maybe should) amend by gpg's stderr. > So let's just revert commit efee9553a4f97b2ecd8f49be19606dd4cf7d9c28. That "just" reintroduces the problem that the orignal patch solves. The passphrase/tty issue must be Windows specific - or the non-issue Linux-specific, if you prefer. > This fixes https://github.com/git-for-windows/git/issues/871 > > Cc: Michael J Gruber> Signed-off-by: Johannes Schindelin > --- > Published-As: https://github.com/dscho/git/releases/tag/fix-gpg-v1 > Fetch-It-Via: git fetch https://github.com/dscho/git fix-gpg-v1 > > gpg-interface.c | 8 ++-- > t/t7004-tag.sh | 9 + > 2 files changed, 3 insertions(+), 14 deletions(-) > > diff --git a/gpg-interface.c b/gpg-interface.c > index 8672eda..3f3a3f7 100644 > --- a/gpg-interface.c > +++ b/gpg-interface.c > @@ -153,11 +153,9 @@ int sign_buffer(struct strbuf *buffer, struct strbuf > *signature, const char *sig > struct child_process gpg = CHILD_PROCESS_INIT; > int ret; > size_t i, j, bottom; > - struct strbuf gpg_status = STRBUF_INIT; > > argv_array_pushl(, >gpg_program, > - "--status-fd=2", >"-bsau", signing_key, >NULL); > > @@ -169,12 +167,10 @@ int sign_buffer(struct strbuf *buffer, struct strbuf > *signature, const char *sig >*/ > sigchain_push(SIGPIPE, SIG_IGN); > ret = pipe_command(, buffer->buf, buffer->len, > -signature, 1024, _status, 0); > +signature, 1024, NULL, 0); > sigchain_pop(SIGPIPE); > > - ret |= !strstr(gpg_status.buf, "\n[GNUPG:] SIG_CREATED "); > - strbuf_release(_status); > - if (ret) > + if (ret || signature->len == bottom) > return error(_("gpg failed to sign the data")); > > /* Strip CR from the line endings, in case we are on Windows. */ > diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh > index 8b0f71a..f9b7d79 100755 > --- a/t/t7004-tag.sh > +++ b/t/t7004-tag.sh > @@ -1202,17 +1202,10 @@ test_expect_success GPG,RFC1991 \ > # try to sign with bad user.signingkey > git config user.signingkey BobTheMouse > test_expect_success GPG \ > - 'git tag -s fails if gpg is misconfigured (bad key)' \ > + 'git tag -s fails if gpg is misconfigured' \ > 'test_must_fail git tag -s -m tail tag-gpg-failure' > git config --unset user.signingkey > > -# try to produce invalid signature > -test_expect_success GPG \ > - 'git tag -s fails if gpg is misconfigured (bad signature format)' \ > - 'test_config gpg.program echo && > - test_must_fail git tag -s -m tail tag-gpg-failure' > - > - > # try to verify without gpg: > > rm -rf gpghome >
[PATCH] Unbreak interactive GPG prompt upon signing
With the recent update in efee955 (gpg-interface: check gpg signature creation status, 2016-06-17), we ask GPG to send all status updates to stderr, and then catch the stderr in an strbuf. But GPG might fail, and send error messages to stderr. And we simply do not show them to the user. Even worse: this swallows any interactive prompt for a passphrase. And detaches stderr from the tty so that the passphrase cannot be read. So while the first problem could be fixed (by printing the captured stderr upon error), the second problem cannot be easily fixed, and presents a major regression. So let's just revert commit efee9553a4f97b2ecd8f49be19606dd4cf7d9c28. This fixes https://github.com/git-for-windows/git/issues/871 Cc: Michael J GruberSigned-off-by: Johannes Schindelin --- Published-As: https://github.com/dscho/git/releases/tag/fix-gpg-v1 Fetch-It-Via: git fetch https://github.com/dscho/git fix-gpg-v1 gpg-interface.c | 8 ++-- t/t7004-tag.sh | 9 + 2 files changed, 3 insertions(+), 14 deletions(-) diff --git a/gpg-interface.c b/gpg-interface.c index 8672eda..3f3a3f7 100644 --- a/gpg-interface.c +++ b/gpg-interface.c @@ -153,11 +153,9 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *sig struct child_process gpg = CHILD_PROCESS_INIT; int ret; size_t i, j, bottom; - struct strbuf gpg_status = STRBUF_INIT; argv_array_pushl(, gpg_program, -"--status-fd=2", "-bsau", signing_key, NULL); @@ -169,12 +167,10 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *sig */ sigchain_push(SIGPIPE, SIG_IGN); ret = pipe_command(, buffer->buf, buffer->len, - signature, 1024, _status, 0); + signature, 1024, NULL, 0); sigchain_pop(SIGPIPE); - ret |= !strstr(gpg_status.buf, "\n[GNUPG:] SIG_CREATED "); - strbuf_release(_status); - if (ret) + if (ret || signature->len == bottom) return error(_("gpg failed to sign the data")); /* Strip CR from the line endings, in case we are on Windows. */ diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh index 8b0f71a..f9b7d79 100755 --- a/t/t7004-tag.sh +++ b/t/t7004-tag.sh @@ -1202,17 +1202,10 @@ test_expect_success GPG,RFC1991 \ # try to sign with bad user.signingkey git config user.signingkey BobTheMouse test_expect_success GPG \ - 'git tag -s fails if gpg is misconfigured (bad key)' \ + 'git tag -s fails if gpg is misconfigured' \ 'test_must_fail git tag -s -m tail tag-gpg-failure' git config --unset user.signingkey -# try to produce invalid signature -test_expect_success GPG \ - 'git tag -s fails if gpg is misconfigured (bad signature format)' \ - 'test_config gpg.program echo && -test_must_fail git tag -s -m tail tag-gpg-failure' - - # try to verify without gpg: rm -rf gpghome -- 2.10.0.windows.1.6.gc4f481a base-commit: 6ebdac1bab966b720d776aa43ca188fe378b1f4b