Re: [PATCHv3] gpg-interface: check gpg signature creation status
Jeff King venit, vidit, dixit 16.06.2016 11:25: > On Wed, Jun 15, 2016 at 09:17:54AM +0200, Michael J Gruber wrote: > >> As for the flexibility: >> We do code specifically for gpg, which happens to work for gpg2 also. >> The patch doesn't add any gpg ui requirements that we don't require >> elsewhere already. >> More flexibility requires a completely pluggable approach - gpgsm >> already fails to meet the gpg command line ui. > > Does it? I haven't run it in production, but I did some tests with gpgsm > a few months ago and found it to be a drop-in replacement, at least with > respect to git. Though I don't think that matters one way or the other > for the current discussion; it _does_ do --status-fd. You are right! I solely trusted in the documentation rather than simply trying it out. gpgsm even supports the undocumented short options. Only the resulting header is different (plus the fact that I have to turn of crl checks for whatever reason). >> In any case, "status-fd" is *the* way to interface with gpg reliably >> just like plumbing commands are *the* way to interface with git reliably. > > Fair enough. I've generally found its exit code pretty reliable. It's > unclear to me if the problem you saw was because gpg was exiting 0 but > producing no signature, or if your debugging was masking its exit code. > > Either way, I do not mind using --status-fd; it seems like it should be > more robust in general. It's the tip commit in the series I'm about to > post. > >> As for the read locking: >> I'm sorry I have no idea about that area at all. I thought that I'm >> doing the same that we do for verify, but apparently not. My >> strbuf_read-fu is not that strong either (read: copy&paste). I trust >> your assessment completely, though. > > Yeah, you're right that the deadlock thing is also a possibility on the > verification side. I'm not sure whether it's possible to trigger in > practice or not. See the analysis in the series. With great admiration I've looked at your series which solves this problem at the root. Way above my head (the solution, not the root, fortunately). >> As for the original problem: >> That had a different cause, as we know now (rebase dropping signatures >> without hint). I still think we should check gpg status codes properly. > > Yep, I agree. > > -Peff > -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv3] gpg-interface: check gpg signature creation status
On Wed, Jun 15, 2016 at 09:17:54AM +0200, Michael J Gruber wrote: > As for the flexibility: > We do code specifically for gpg, which happens to work for gpg2 also. > The patch doesn't add any gpg ui requirements that we don't require > elsewhere already. > More flexibility requires a completely pluggable approach - gpgsm > already fails to meet the gpg command line ui. Does it? I haven't run it in production, but I did some tests with gpgsm a few months ago and found it to be a drop-in replacement, at least with respect to git. Though I don't think that matters one way or the other for the current discussion; it _does_ do --status-fd. > In any case, "status-fd" is *the* way to interface with gpg reliably > just like plumbing commands are *the* way to interface with git reliably. Fair enough. I've generally found its exit code pretty reliable. It's unclear to me if the problem you saw was because gpg was exiting 0 but producing no signature, or if your debugging was masking its exit code. Either way, I do not mind using --status-fd; it seems like it should be more robust in general. It's the tip commit in the series I'm about to post. > As for the read locking: > I'm sorry I have no idea about that area at all. I thought that I'm > doing the same that we do for verify, but apparently not. My > strbuf_read-fu is not that strong either (read: copy&paste). I trust > your assessment completely, though. Yeah, you're right that the deadlock thing is also a possibility on the verification side. I'm not sure whether it's possible to trigger in practice or not. See the analysis in the series. > As for the original problem: > That had a different cause, as we know now (rebase dropping signatures > without hint). I still think we should check gpg status codes properly. Yep, I agree. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv3] gpg-interface: check gpg signature creation status
Jeff King venit, vidit, dixit 15.06.2016 02:56: > On Tue, Jun 14, 2016 at 04:47:35PM -0700, Junio C Hamano wrote: > >> Jeff King writes: >> >>> I'm still undecided on whether it is a better approach than making >>> sure the stdout we got looks sane. In particular I'd worry that it >>> would make things harder for somebody trying to plug in something >>> gpg-like (e.g., if you wanted to do something exotic like call a >>> program which fetched the signature from a remote device or >>> something). But it's probably not _that_ hard for such a script >>> to emulate --status-fd. >> >> I share the same thinking, but at the same time, it already is a >> requirement to give --status-fd output that is close enough on the >> signature verification side, isn't it? > > Yeah, though I could see somebody wanting to sit amidst the signing side > but not verification (e.g., if your keys are elsewhere from the machine > running git). Of course such a case could probably ferry --status-fd > from the other side anyway. > > I admit I don't know of such a case in practice, though, and > implementing a rudimentary --status-fd to say "SIG OK" or whatever on > the signing side is not _that_ big a deal. So if we think this approach > is a more robust solution in the normal case, let's not hold it up over > what-ifs. > > -Peff As for the flexibility: We do code specifically for gpg, which happens to work for gpg2 also. The patch doesn't add any gpg ui requirements that we don't require elsewhere already. More flexibility requires a completely pluggable approach - gpgsm already fails to meet the gpg command line ui. In any case, "status-fd" is *the* way to interface with gpg reliably just like plumbing commands are *the* way to interface with git reliably. As for the read locking: I'm sorry I have no idea about that area at all. I thought that I'm doing the same that we do for verify, but apparently not. My strbuf_read-fu is not that strong either (read: copy&paste). I trust your assessment completely, though. As for the original problem: That had a different cause, as we know now (rebase dropping signatures without hint). I still think we should check gpg status codes properly. Michael -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv3] gpg-interface: check gpg signature creation status
On Tue, Jun 14, 2016 at 06:26:33PM -0400, Jeff King wrote: > > > > bottom = signature->len; > > > > - len = strbuf_read(signature, gpg.out, 1024); > > > > + strbuf_read(signature, gpg.out, 1024); > > > > + strbuf_read(&err, gpg.err, 0); > > > > > > H, isn't this asking for a deadlock? When GPG spews more than > > > what would fit in a pipe buffer to its standard error (hence gets > > > blocked), its standard output may not complete, and the we would get > > > stuck by attempting to read from gpg.out, failing to reach the other > > > strbuf_read() that would unblock GPG by reading from gpg.err? > > > > Yeah, it definitely is a deadlock. I think we'd need a select loop to > > read into multiple strbufs at once (we can't use "struct async" because > > that might happen in another process). > > Something like this on top of Michael's patch (only lightly tested). I wondered if this is something we might run into in other places, but just haven't in practice. After grepping existing calls to strbuf_read(), I think the only other case is the one in verify_signed_buffer(), which reads all of stderr before trying to read stdout. I suspect that _could_ deadlock but doesn't tend to in practice. Interestingly, it also writes in full to gpg's stdin before reading anything. If gpg buffers all of its input before writing, that's fine, but in theory it does not have to. I have no idea if it's possible for it to be a problem in practice. That can be solved in the same select loop, though obviously it's not just strbuf_read_parallel() at that point, but rather a kind of a feed_and_capture_command(). -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv3] gpg-interface: check gpg signature creation status
On Tue, Jun 14, 2016 at 04:47:35PM -0700, Junio C Hamano wrote: > Jeff King writes: > > > I'm still undecided on whether it is a better approach than making > > sure the stdout we got looks sane. In particular I'd worry that it > > would make things harder for somebody trying to plug in something > > gpg-like (e.g., if you wanted to do something exotic like call a > > program which fetched the signature from a remote device or > > something). But it's probably not _that_ hard for such a script > > to emulate --status-fd. > > I share the same thinking, but at the same time, it already is a > requirement to give --status-fd output that is close enough on the > signature verification side, isn't it? Yeah, though I could see somebody wanting to sit amidst the signing side but not verification (e.g., if your keys are elsewhere from the machine running git). Of course such a case could probably ferry --status-fd from the other side anyway. I admit I don't know of such a case in practice, though, and implementing a rudimentary --status-fd to say "SIG OK" or whatever on the signing side is not _that_ big a deal. So if we think this approach is a more robust solution in the normal case, let's not hold it up over what-ifs. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv3] gpg-interface: check gpg signature creation status
Jeff King writes: > I'm still undecided on whether it is a better approach than making > sure the stdout we got looks sane. In particular I'd worry that it > would make things harder for somebody trying to plug in something > gpg-like (e.g., if you wanted to do something exotic like call a > program which fetched the signature from a remote device or > something). But it's probably not _that_ hard for such a script > to emulate --status-fd. I share the same thinking, but at the same time, it already is a requirement to give --status-fd output that is close enough on the signature verification side, isn't it? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv3] gpg-interface: check gpg signature creation status
On Tue, Jun 14, 2016 at 05:50:19PM -0400, Jeff King wrote: > On Tue, Jun 14, 2016 at 11:13:54AM -0700, Junio C Hamano wrote: > > > Michael J Gruber writes: > > > > > bottom = signature->len; > > > - len = strbuf_read(signature, gpg.out, 1024); > > > + strbuf_read(signature, gpg.out, 1024); > > > + strbuf_read(&err, gpg.err, 0); > > > > H, isn't this asking for a deadlock? When GPG spews more than > > what would fit in a pipe buffer to its standard error (hence gets > > blocked), its standard output may not complete, and the we would get > > stuck by attempting to read from gpg.out, failing to reach the other > > strbuf_read() that would unblock GPG by reading from gpg.err? > > Yeah, it definitely is a deadlock. I think we'd need a select loop to > read into multiple strbufs at once (we can't use "struct async" because > that might happen in another process). Something like this on top of Michael's patch (only lightly tested). I'm still undecided on whether it is a better approach than making sure the stdout we got looks sane. In particular I'd worry that it would make things harder for somebody trying to plug in something gpg-like (e.g., if you wanted to do something exotic like call a program which fetched the signature from a remote device or something). But it's probably not _that_ hard for such a script to emulate --status-fd. --- diff --git a/gpg-interface.c b/gpg-interface.c index 850dc81..576e462 100644 --- a/gpg-interface.c +++ b/gpg-interface.c @@ -153,6 +153,7 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *sig const char *args[5]; size_t i, j, bottom; struct strbuf err = STRBUF_INIT; + struct strbuf_reader readers[2]; gpg.argv = args; gpg.in = -1; @@ -183,8 +184,15 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *sig close(gpg.in); bottom = signature->len; - strbuf_read(signature, gpg.out, 1024); - strbuf_read(&err, gpg.err, 0); + + readers[0].buf = signature; + readers[0].fd = gpg.out; + readers[0].hint = 1024; + readers[1].buf = &err; + readers[1].fd = gpg.err; + readers[1].hint = 1024; + strbuf_read_parallel(readers, 2); + close(gpg.out); close(gpg.err); diff --git a/strbuf.c b/strbuf.c index 1ba600b..f674b23 100644 --- a/strbuf.c +++ b/strbuf.c @@ -395,6 +395,58 @@ ssize_t strbuf_read_once(struct strbuf *sb, int fd, size_t hint) return cnt; } +void strbuf_read_parallel(struct strbuf_reader *readers, int nr) +{ + int i; + struct pollfd *pfd; + + ALLOC_ARRAY(pfd, nr); + for (i = 0; i < nr; i++) + readers[i].error = 0; + + while (1) { + int pollsize = 0; + int ret; + + for (i = 0; i < nr; i++) { + if (readers[i].fd < 0) + continue; + pfd[pollsize].fd = readers[i].fd; + pfd[pollsize].events = POLLIN; + readers[i].pfd = &pfd[pollsize]; + pollsize++; + } + + if (!pollsize) + break; + + ret = poll(pfd, pollsize, -1); + if (ret < 0) { + if (errno == EINTR) + continue; + /* should never happen? */ + die_errno("poll failed"); + } + + for (i = 0; i < nr; i++) { + if (readers[i].fd < 0) + continue; + if (readers[i].pfd->revents & + (POLLIN|POLLHUP|POLLERR|POLLNVAL)) { + ret = strbuf_read_once(readers[i].buf, + readers[i].fd, + readers[i].hint); + if (ret < 0) + readers[i].error = errno; + if (ret <= 0) + readers[i].fd = -1; + } + } + } + + free(pfd); +} + ssize_t strbuf_write(struct strbuf *sb, FILE *f) { return sb->len ? fwrite(sb->buf, 1, sb->len, f) : 0; diff --git a/strbuf.h b/strbuf.h index 7987405..b93822e 100644 --- a/strbuf.h +++ b/strbuf.h @@ -374,6 +374,25 @@ extern ssize_t strbuf_read(struct strbuf *, int fd, size_t hint); */ extern ssize_t strbuf_read_once(struct strbuf *, int fd, size_t hint); +/** + * Read from several descriptors in parallel, each into its own strbuf. + * This can be used, for example, to capture stdout and stderr from a + * sub-process without worrying about deadlocks. + */ +struct strbuf_reader { + /* Initialized by caller */ + struct strbuf *buf; + int fd; + size_t hint;
Re: [PATCHv3] gpg-interface: check gpg signature creation status
On Tue, Jun 14, 2016 at 11:13:54AM -0700, Junio C Hamano wrote: > Michael J Gruber writes: > > > bottom = signature->len; > > - len = strbuf_read(signature, gpg.out, 1024); > > + strbuf_read(signature, gpg.out, 1024); > > + strbuf_read(&err, gpg.err, 0); > > H, isn't this asking for a deadlock? When GPG spews more than > what would fit in a pipe buffer to its standard error (hence gets > blocked), its standard output may not complete, and the we would get > stuck by attempting to read from gpg.out, failing to reach the other > strbuf_read() that would unblock GPG by reading from gpg.err? Yeah, it definitely is a deadlock. I think we'd need a select loop to read into multiple strbufs at once (we can't use "struct async" because that might happen in another process). -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv3] gpg-interface: check gpg signature creation status
Michael J Gruber writes: > bottom = signature->len; > - len = strbuf_read(signature, gpg.out, 1024); > + strbuf_read(signature, gpg.out, 1024); > + strbuf_read(&err, gpg.err, 0); H, isn't this asking for a deadlock? When GPG spews more than what would fit in a pipe buffer to its standard error (hence gets blocked), its standard output may not complete, and the we would get stuck by attempting to read from gpg.out, failing to reach the other strbuf_read() that would unblock GPG by reading from gpg.err? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv3] gpg-interface: check gpg signature creation status
When we create a signature, it may happen that gpg returns with "success" but not with an actual detached signature on stdout. Check for the correct signature creation status to catch these cases better. Really, --status-fd parsing is the only way to check gpg status reliably. We do the same for verify already. Signed-off-by: Michael J Gruber --- That must be the real real thing now... gpg-interface.c | 22 +++--- t/t7004-tag.sh | 10 +- 2 files changed, 24 insertions(+), 8 deletions(-) diff --git a/gpg-interface.c b/gpg-interface.c index c4b1e8c..850dc81 100644 --- a/gpg-interface.c +++ b/gpg-interface.c @@ -150,17 +150,19 @@ const char *get_signing_key(void) int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *signing_key) { struct child_process gpg = CHILD_PROCESS_INIT; - const char *args[4]; - ssize_t len; + const char *args[5]; size_t i, j, bottom; + struct strbuf err = STRBUF_INIT; gpg.argv = args; gpg.in = -1; gpg.out = -1; + gpg.err = -1; args[0] = gpg_program; - args[1] = "-bsau"; - args[2] = signing_key; - args[3] = NULL; + args[1] = "--status-fd=2"; + args[2] = "-bsau"; + args[3] = signing_key; + args[4] = NULL; if (start_command(&gpg)) return error(_("could not run gpg.")); @@ -174,19 +176,25 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *sig if (write_in_full(gpg.in, buffer->buf, buffer->len) != buffer->len) { close(gpg.in); close(gpg.out); + close(gpg.err); finish_command(&gpg); return error(_("gpg did not accept the data")); } close(gpg.in); bottom = signature->len; - len = strbuf_read(signature, gpg.out, 1024); + strbuf_read(signature, gpg.out, 1024); + strbuf_read(&err, gpg.err, 0); close(gpg.out); + close(gpg.err); sigchain_pop(SIGPIPE); - if (finish_command(&gpg) || !len || len < 0) + if (finish_command(&gpg) || !strstr(err.buf, "\n[GNUPG:] SIG_CREATED ")) { + strbuf_release(&err); return error(_("gpg failed to sign the data")); + } + strbuf_release(&err); /* Strip CR from the line endings, in case we are on Windows. */ for (i = j = bottom; i < signature->len; i++) diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh index f9b7d79..467e968 100755 --- a/t/t7004-tag.sh +++ b/t/t7004-tag.sh @@ -1202,10 +1202,18 @@ 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' \ + 'git tag -s fails if gpg is misconfigured (bad key)' \ 'test_must_fail git tag -s -m tail tag-gpg-failure' git config --unset user.signingkey +# try to produce invalid signature +git config gpg.program echo +test_expect_success GPG \ + 'git tag -s fails if gpg is misconfigured (bad signature format)' \ + 'test_must_fail git tag -s -m tail tag-gpg-failure' +git config --unset gpg.program + + # try to verify without gpg: rm -rf gpghome -- 2.9.0.382.g87fd384 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html