[PATCH] git-tag: don't use gpg's stdin, stdout when signing tags

[PATCH] git-tag: don't use gpg's stdin, stdout when signing tags

[Gerrit Pape]

When using gpg with some console based gpg-agent, acquiring the
passphrase through the agent fails if stdin and stdout of gpg are
redirected.  With this commit, git-tag uses temporary files instead
of standard input/output when signing a tag to support such gpg-agent
usage.

The problem was reported by Loïc Minier through
 http://bugs.debian.org/507642

Signed-off-by: Gerrit Pape <pape@smarden.org>
---
 builtin-tag.c |   51 ++++++++++++++++++++++++++++++++++-----------------
 1 files changed, 34 insertions(+), 17 deletions(-)

diff --git a/builtin-tag.c b/builtin-tag.c
index 01e7374..e350352 100644
--- a/builtin-tag.c
+++ b/builtin-tag.c
@@ -159,10 +159,15 @@ static int verify_tag(const char *name, const char *ref,
 static int do_sign(struct strbuf *buffer)
 {
 	struct child_process gpg;
-	const char *args[4];
+	const char *args[7];
 	char *bracket;
 	int len;
 	int i, j;
+	int fd;
+	char *unsignpath, *signpath;
+
+	unsignpath = git_pathdup("TAG_UNSIGNEDMSG");
+	signpath = git_pathdup("TAG_SIGNEDMSG");
 
 	if (!*signingkey) {
 		if (strlcpy(signingkey, git_committer_info(IDENT_ERROR_ON_NO_NAME),
@@ -179,27 +184,39 @@ static int do_sign(struct strbuf *buffer)
 
 	memset(&gpg, 0, sizeof(gpg));
 	gpg.argv = args;
-	gpg.in = -1;
-	gpg.out = -1;
+	gpg.in = 0;
+	gpg.out = 1;
 	args[0] = "gpg";
 	args[1] = "-bsau";
 	args[2] = signingkey;
-	args[3] = NULL;
-
-	if (start_command(&gpg))
-		return error("could not run gpg.");
-
-	if (write_in_full(gpg.in, buffer->buf, buffer->len) != buffer->len) {
-		close(gpg.in);
-		close(gpg.out);
-		finish_command(&gpg);
-		return error("gpg did not accept the tag data");
+	args[3] = "-o";
+	args[4] = signpath;
+	args[5] = unsignpath;
+	args[6] = NULL;
+
+	fd = open(unsignpath, O_CREAT | O_TRUNC | O_WRONLY, 0600);
+	if (fd < 0)
+		die("could not create file '%s': %s",
+					unsignpath, strerror(errno));
+	write_or_die(fd, buffer->buf, buffer->len);
+	close(fd);
+
+	if (run_command(&gpg)) {
+		unlink(unsignpath);
+		unlink(signpath);
+		return error("gpg failed.");
 	}
-	close(gpg.in);
-	len = strbuf_read(buffer, gpg.out, 1024);
-	close(gpg.out);
+	unlink(unsignpath);
+
+	fd = open(signpath, O_RDONLY);
+	if (fd < 0)
+		die ("could not open file '%s': %s",
+					signpath, strerror(errno));
+	len = strbuf_read(buffer, fd, 1024);
+	close(fd);
+	unlink(signpath);
 
-	if (finish_command(&gpg) || !len || len < 0)
+	if (!len || len < 0)
 		return error("gpg failed to sign the tag");
 
 	/* Strip CR from the line endings, in case we are on Windows. */
-- 
1.6.1.3

Re: [PATCH] git-tag: don't use gpg's stdin, stdout when signing tags

[Todd Zullinger]

[-- Attachment #1: Type: text/plain, Size: 1654 bytes --]

Gerrit Pape wrote:
> When using gpg with some console based gpg-agent, acquiring the
> passphrase through the agent fails if stdin and stdout of gpg are
> redirected.  With this commit, git-tag uses temporary files instead
> of standard input/output when signing a tag to support such
> gpg-agent usage.
>
> The problem was reported by Loïc Minier through
> http://bugs.debian.org/507642

I sign tags using gpg-agent with the curse pinentry often and it works
here.  Perhaps Loïc has not set GPG_TTY as the gpg-agent documentation
suggests?  If I unset GPG_TTY, I get the sort of failure indicated in
the bug report.  With it set tag signing works as expected.

Quoting the gpg-agent docs:

  You should always add the following lines to your `.bashrc' or
  whatever initialization file is used for all shell invocations:

     GPG_TTY=`tty`
     export GPG_TTY

  It is important that this environment variable always reflects the
  output of the `tty' command.  For W32 systems this option is not
  required.

Now, I'm not sure if that's a reason not to include this patch.  :)

I just wanted to mention that it can and does work if you have GPG_TTY
set.  This is often needed for other tools as well, e.g. with mutt, so
users of the curses pinentry are best off setting it rather than
hoping individual apps work around it.

-- 
Todd        OpenPGP -> KeyID: 0xBEAF0CE3 | URL: www.pobox.com/~tmz/pgp
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Some people are like Slinkies... not really good for anything, but you
still can't help but smile when you see one tumble down the stairs.


[-- Attachment #2: Type: application/pgp-signature, Size: 542 bytes --]

Re: [PATCH] git-tag: don't use gpg's stdin, stdout when signing tags

[Gerrit Pape]

On Fri, Feb 20, 2009 at 08:46:34AM -0500, Todd Zullinger wrote:
> Gerrit Pape wrote:
> > When using gpg with some console based gpg-agent, acquiring the
> > passphrase through the agent fails if stdin and stdout of gpg are
> > redirected.  With this commit, git-tag uses temporary files instead
> > of standard input/output when signing a tag to support such
> > gpg-agent usage.
> >
> > The problem was reported by Loïc Minier through
> > http://bugs.debian.org/507642
> 
> I sign tags using gpg-agent with the curse pinentry often and it works
> here.  Perhaps Loïc has not set GPG_TTY as the gpg-agent documentation
> suggests?  If I unset GPG_TTY, I get the sort of failure indicated in
> the bug report.  With it set tag signing works as expected.

Thanks a lot Todd and Johannes for teaching me.  From my POV this patch
can be dropped.

Regards, Gerrit.

Re: [PATCH] git-tag: don't use gpg's stdin, stdout when signing tags

[Johannes Sixt]

Gerrit Pape schrieb:
>  	memset(&gpg, 0, sizeof(gpg));
>  	gpg.argv = args;
> -	gpg.in = -1;
> -	gpg.out = -1;
> +	gpg.in = 0;
> +	gpg.out = 1;

I assume you mean with this that gpg should read from fd 0 and write to 
fd 1, IOW, it should use the standard channels. If I am right, then the 
memset above has initialized gpg as needed already. Then gpg.argv is the 
only thing you are setting up in struct child_process gpg; but in this 
case you can use a convenience function...

>  	args[0] = "gpg";
>  	args[1] = "-bsau";
>  	args[2] = signingkey;
> -	args[3] = NULL;
...
> +	args[3] = "-o";
> +	args[4] = signpath;
> +	args[5] = unsignpath;
> +	args[6] = NULL;
...
> +	if (run_command(&gpg)) {

... here (note: no struct child_process needed):

	if (run_command_v_opt(args, 0)) {

(Just in case this patch is required...)

-- Hannes