threads / patch / 17919

patchgit-tag: don't use gpg's stdin, stdout when signing tags

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

## tl;dr

4 messages between Feb 20, 2009 and Feb 23, 2009. Diffs are folded; open one to read it.

replies: 3people: 3as markdown or json

Gerrit Pape· Feb 20, 2009, 11:38 UTC · lore

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(-)
Show changes to builtin-tag.c +34 −17
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
Todd Zullinger· Feb 20, 2009, 13:46 UTC · re: Gerrit Pape · lore

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

Gerrit Pape wrote:
Show 8 quoted lines
> 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.
Gerrit Pape· Feb 23, 2009, 15:23 UTC · re: Todd Zullinger · lore

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

On Fri, Feb 20, 2009 at 08:46:34AM -0500, Todd Zullinger wrote:
Show 14 quoted lines
> 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.
Johannes Sixt· Feb 20, 2009, 17:56 UTC · re: Gerrit Pape · lore

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

Gerrit Pape schrieb:
Show 6 quoted lines
>  	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

← back to recent threads