[RFC PATCH 0/4] Teach git fetch to verify signed tags automatically

[RFC PATCH 0/4] Teach git fetch to verify signed tags automatically

[Deskin Miller]

It struck me a while back when I fetched a new tagged release from git.git that
if I wanted to verify the tag's signature, I'd have to issue another command to
do so.  Shouldn't git be able to do that for me automatically, when it fetches
signed tags?  Now it does.  Also, 'git remote update' gets this for free.

Individual commit messages explain things reasonably well, I hope; here are a
few points for discussion:

-Is refactoring builtin-verify-tag.c the right thing to do?
-Now that the SIGPIPE ignoring is occurring at a lower level, should it be
 removed from cmd_verify_tag?
-Output format: good, bad, ugly?
-What to do if a tag is found to have a bad signature?

Deskin Miller (4):
  Refactor builtin-verify-tag.c
  verify-tag.c: ignore SIGPIPE around gpg invocation
  verify-tag.c: suppress gpg output if asked
  Make git fetch verify signed tags

 Makefile             |    2 +
 builtin-fetch.c      |   25 +++++++++++----
 builtin-verify-tag.c |   61 ++----------------------------------
 t/t7004-tag.sh       |   37 ++++++++++++++++++++++
 verify-tag.c         |   84 ++++++++++++++++++++++++++++++++++++++++++++++++++
 verify-tag.h         |   10 ++++++
 6 files changed, 155 insertions(+), 64 deletions(-)
 create mode 100644 verify-tag.c
 create mode 100644 verify-tag.h

[RFC PATCH 1/4] Refactor builtin-verify-tag.c

[Deskin Miller]

builtin-verify-tag.c didn't expose any of its functionality to be used
internally.  Refactor some of it into new verify-tag.c and expose
verify_tag_sha1 able to be called from elsewhere in git.

Signed-off-by: Deskin Miller <deskinm@umich.edu>
---
 Makefile             |    2 +
 builtin-verify-tag.c |   61 ++-------------------------------------
 verify-tag.c         |   77 ++++++++++++++++++++++++++++++++++++++++++++++++++
 verify-tag.h         |   10 ++++++
 4 files changed, 93 insertions(+), 57 deletions(-)
 create mode 100644 verify-tag.c
 create mode 100644 verify-tag.h

diff --git a/Makefile b/Makefile
index 35adafa..b372aa4 100644
--- a/Makefile
+++ b/Makefile
@@ -392,6 +392,7 @@ LIB_H += tree-walk.h
 LIB_H += unpack-trees.h
 LIB_H += userdiff.h
 LIB_H += utf8.h
+LIB_H += verify-tag.h
 LIB_H += wt-status.h
 
 LIB_OBJS += abspath.o
@@ -490,6 +491,7 @@ LIB_OBJS += unpack-trees.o
 LIB_OBJS += userdiff.o
 LIB_OBJS += usage.o
 LIB_OBJS += utf8.o
+LIB_OBJS += verify-tag.o
 LIB_OBJS += walker.o
 LIB_OBJS += wrapper.o
 LIB_OBJS += write_or_die.o
diff --git a/builtin-verify-tag.c b/builtin-verify-tag.c
index 729a159..dd350e8 100644
--- a/builtin-verify-tag.c
+++ b/builtin-verify-tag.c
@@ -7,65 +7,16 @@
  */
 #include "cache.h"
 #include "builtin.h"
-#include "tag.h"
-#include "run-command.h"
+#include "verify-tag.h"
 #include <signal.h>
 
 static const char builtin_verify_tag_usage[] =
 		"git verify-tag [-v|--verbose] <tag>...";
 
-#define PGP_SIGNATURE "-----BEGIN PGP SIGNATURE-----"
-
-static int run_gpg_verify(const char *buf, unsigned long size, int verbose)
-{
-	struct child_process gpg;
-	const char *args_gpg[] = {"gpg", "--verify", "FILE", "-", NULL};
-	char path[PATH_MAX], *eol;
-	size_t len;
-	int fd, ret;
-
-	fd = git_mkstemp(path, PATH_MAX, ".git_vtag_tmpXXXXXX");
-	if (fd < 0)
-		return error("could not create temporary file '%s': %s",
-						path, strerror(errno));
-	if (write_in_full(fd, buf, size) < 0)
-		return error("failed writing temporary file '%s': %s",
-						path, strerror(errno));
-	close(fd);
-
-	/* find the length without signature */
-	len = 0;
-	while (len < size && prefixcmp(buf + len, PGP_SIGNATURE)) {
-		eol = memchr(buf + len, '\n', size - len);
-		len += eol ? eol - (buf + len) + 1 : size - len;
-	}
-	if (verbose)
-		write_in_full(1, buf, len);
-
-	memset(&gpg, 0, sizeof(gpg));
-	gpg.argv = args_gpg;
-	gpg.in = -1;
-	args_gpg[2] = path;
-	if (start_command(&gpg)) {
-		unlink(path);
-		return error("could not run gpg.");
-	}
-
-	write_in_full(gpg.in, buf, len);
-	close(gpg.in);
-	ret = finish_command(&gpg);
-
-	unlink(path);
-
-	return ret;
-}
-
 static int verify_tag(const char *name, int verbose)
 {
 	enum object_type type;
 	unsigned char sha1[20];
-	char *buf;
-	unsigned long size;
 	int ret;
 
 	if (get_sha1(name, sha1))
@@ -76,13 +27,9 @@ static int verify_tag(const char *name, int verbose)
 		return error("%s: cannot verify a non-tag object of type %s.",
 				name, typename(type));
 
-	buf = read_sha1_file(sha1, &type, &size);
-	if (!buf)
-		return error("%s: unable to read file.", name);
-
-	ret = run_gpg_verify(buf, size, verbose);
-
-	free(buf);
+	ret = verify_tag_sha1(sha1, verbose);
+	if (ret)
+		error("Failed to verify %s.", name);
 	return ret;
 }
 
diff --git a/verify-tag.c b/verify-tag.c
new file mode 100644
index 0000000..c9be331
--- /dev/null
+++ b/verify-tag.c
@@ -0,0 +1,77 @@
+/*
+ * Internals for "git verify-tag"
+ *
+ * Copyright (c) 2008 Deskin Miller <deskinm@umich.edu>
+ *
+ */
+#include "cache.h"
+#include "object.h"
+#include "run-command.h"
+
+#define PGP_SIGNATURE "-----BEGIN PGP SIGNATURE-----"
+
+static int run_gpg_verify(const char *buf, unsigned long size, int verbose)
+{
+	struct child_process gpg;
+	const char *args_gpg[] = {"gpg", "--verify", "FILE", "-", NULL};
+	char path[PATH_MAX], *eol;
+	size_t len;
+	int fd, ret;
+
+	fd = git_mkstemp(path, PATH_MAX, ".git_vtag_tmpXXXXXX");
+	if (fd < 0)
+		return error("could not create temporary file '%s': %s",
+						path, strerror(errno));
+	if (write_in_full(fd, buf, size) < 0)
+		return error("failed writing temporary file '%s': %s",
+						path, strerror(errno));
+	close(fd);
+
+	/* find the length without signature */
+	len = 0;
+	while (len < size && prefixcmp(buf + len, PGP_SIGNATURE)) {
+		eol = memchr(buf + len, '\n', size - len);
+		len += eol ? eol - (buf + len) + 1 : size - len;
+	}
+	if (verbose)
+		write_in_full(1, buf, len);
+
+	memset(&gpg, 0, sizeof(gpg));
+	gpg.argv = args_gpg;
+	gpg.in = -1;
+	args_gpg[2] = path;
+	if (start_command(&gpg)) {
+		unlink(path);
+		return error("could not run gpg.");
+	}
+
+	write_in_full(gpg.in, buf, len);
+	close(gpg.in);
+	ret = finish_command(&gpg);
+
+	unlink(path);
+
+	return ret;
+}
+
+int verify_tag_sha1(const unsigned char *sha1, int verbose)
+{
+	enum object_type type;
+	char *buf;
+	unsigned long size;
+	int ret;
+
+	type = sha1_object_info(sha1, NULL);
+	if (type != OBJ_TAG)
+		return error("Cannot verify a non-tag object of type %s.",
+				typename(type));
+
+	buf = read_sha1_file(sha1, &type, &size);
+	if (!buf)
+		return error("Cnable to read file.");
+
+	ret = run_gpg_verify(buf, size, verbose);
+
+	free(buf);
+	return ret;
+}
diff --git a/verify-tag.h b/verify-tag.h
new file mode 100644
index 0000000..45bdca7
--- /dev/null
+++ b/verify-tag.h
@@ -0,0 +1,10 @@
+#ifndef VERIFY_TAG_H
+#define VERIFY_TAG_H
+/*
+ * Internals for "git verify-tag"
+ *
+ * Copyright (c) 2008 Deskin Miller <deskinm@umich.edu>
+ */
+extern int verify_tag_sha1(const unsigned char *sha1, int verbose);
+
+#endif /* VERIFY_TAG_H */
-- 
1.6.0.4.770.ga8394

[RFC PATCH 2/4] verify-tag.c: ignore SIGPIPE around gpg invocation

[Deskin Miller]

builtin-verify-tag.c already sets SIG_IGN for SIGPIPE before calling
verify_tag, but new callers of verify_tag_sha1 may not have modified the
signal handler, and shouldn't have to.  Save and restore the signal
handler for SIGPIPE around the invocation of gpg.

Signed-off-by: Deskin Miller <deskinm@umich.edu>
---
 verify-tag.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/verify-tag.c b/verify-tag.c
index c9be331..c3e35f3 100644
--- a/verify-tag.c
+++ b/verify-tag.c
@@ -7,6 +7,7 @@
 #include "cache.h"
 #include "object.h"
 #include "run-command.h"
+#include <signal.h>
 
 #define PGP_SIGNATURE "-----BEGIN PGP SIGNATURE-----"
 
@@ -17,6 +18,7 @@ static int run_gpg_verify(const char *buf, unsigned long size, int verbose)
 	char path[PATH_MAX], *eol;
 	size_t len;
 	int fd, ret;
+	sighandler_t save_handle;
 
 	fd = git_mkstemp(path, PATH_MAX, ".git_vtag_tmpXXXXXX");
 	if (fd < 0)
@@ -40,8 +42,10 @@ static int run_gpg_verify(const char *buf, unsigned long size, int verbose)
 	gpg.argv = args_gpg;
 	gpg.in = -1;
 	args_gpg[2] = path;
+	save_handle = signal(SIGPIPE, SIG_IGN);
 	if (start_command(&gpg)) {
 		unlink(path);
+		signal(SIGPIPE, save_handle);
 		return error("could not run gpg.");
 	}
 
@@ -50,6 +54,7 @@ static int run_gpg_verify(const char *buf, unsigned long size, int verbose)
 	ret = finish_command(&gpg);
 
 	unlink(path);
+	signal(SIGPIPE, save_handle);
 
 	return ret;
 }
-- 
1.6.0.4.770.ga8394

[RFC PATCH 3/4] verify-tag.c: suppress gpg output if asked

[Deskin Miller]

Previously, tag verification would output messages from gpg on standard
error.  Allow this to be controlled by a parameter to verify_tag_sha1.

Signed-off-by: Deskin Miller <deskinm@umich.edu>
---
 verify-tag.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/verify-tag.c b/verify-tag.c
index c3e35f3..b47acc9 100644
--- a/verify-tag.c
+++ b/verify-tag.c
@@ -35,13 +35,15 @@ static int run_gpg_verify(const char *buf, unsigned long size, int verbose)
 		eol = memchr(buf + len, '\n', size - len);
 		len += eol ? eol - (buf + len) + 1 : size - len;
 	}
-	if (verbose)
+	if (verbose == 1)
 		write_in_full(1, buf, len);
 
 	memset(&gpg, 0, sizeof(gpg));
 	gpg.argv = args_gpg;
 	gpg.in = -1;
 	args_gpg[2] = path;
+	if (verbose == -1)
+		gpg.no_stderr = 1;
 	save_handle = signal(SIGPIPE, SIG_IGN);
 	if (start_command(&gpg)) {
 		unlink(path);
-- 
1.6.0.4.770.ga8394

[RFC PATCH 4/4] Make git fetch verify signed tags

[Deskin Miller]

When git fetch downloads signed tag objects, make it verify them right
then.  This extends the output summary of fetch to include "(good
signature)" for valid tags and "(BAD SIGNATURE)" for invalid tags.  If
the user does not have the correct key in the gpg keyring, gpg returns
2, verify_tag_sha1 returns -2 and nothing additional is output about
the tag's validity.

Alternate fetch method 'git remote update' gets this check as well due
to the use of the fetch routines.

Signed-off-by: Deskin Miller <deskinm@umich.edu>
---
 builtin-fetch.c |   25 ++++++++++++++++++-------
 t/t7004-tag.sh  |   37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 55 insertions(+), 7 deletions(-)

diff --git a/builtin-fetch.c b/builtin-fetch.c
index f151cfa..f7a50b7 100644
--- a/builtin-fetch.c
+++ b/builtin-fetch.c
@@ -10,6 +10,7 @@
 #include "transport.h"
 #include "run-command.h"
 #include "parse-options.h"
+#include "verify-tag.h"
 
 static const char * const builtin_fetch_usage[] = {
 	"git fetch [options] [<repository> <refspec>...]",
@@ -233,11 +234,16 @@ static int update_local_ref(struct ref *ref,
 
 	if (!is_null_sha1(ref->old_sha1) &&
 	    !prefixcmp(ref->name, "refs/tags/")) {
-		int r;
+		int r, v;
 		r = s_update_ref("updating tag", ref, 0);
-		sprintf(display, "%c %-*s %-*s -> %s%s", r ? '!' : '-',
+		if (type == OBJ_TAG)
+			v = verify_tag_sha1(ref->new_sha1, -1);
+		sprintf(display, "%c %-*s %-*s -> %s%s", r ? '!' :
+			(type == OBJ_TAG ? (v == -1 ? '!' : '-') : '-'),
 			SUMMARY_WIDTH, "[tag update]", REFCOL_WIDTH, remote,
-			pretty_ref, r ? "  (unable to update local ref)" : "");
+			pretty_ref, r ? "  (unable to update local ref)" :
+			(type == OBJ_TAG ? (v == 0 ? " (good signature)" :
+			(v == -1 ? " (BAD SIGNATURE)" : "")) : ""));
 		return r;
 	}
 
@@ -246,7 +252,7 @@ static int update_local_ref(struct ref *ref,
 	if (!current || !updated) {
 		const char *msg;
 		const char *what;
-		int r;
+		int r, v;
 		if (!strncmp(ref->name, "refs/tags/", 10)) {
 			msg = "storing tag";
 			what = "[new tag]";
@@ -257,9 +263,14 @@ static int update_local_ref(struct ref *ref,
 		}
 
 		r = s_update_ref(msg, ref, 0);
-		sprintf(display, "%c %-*s %-*s -> %s%s", r ? '!' : '*',
-			SUMMARY_WIDTH, what, REFCOL_WIDTH, remote, pretty_ref,
-			r ? "  (unable to update local ref)" : "");
+		if (type == OBJ_TAG)
+			v = verify_tag_sha1(ref->new_sha1, -1);
+		sprintf(display, "%c %-*s %-*s -> %s%s", r ? '!' :
+			(type == OBJ_TAG ? (v == -1 ? '!' : '*') : '*'),
+			SUMMARY_WIDTH, what, REFCOL_WIDTH, remote,
+			pretty_ref, r ? "  (unable to update local ref)" :
+			(type == OBJ_TAG ? (v == 0 ? " (good signature)" :
+			(v == -1 ? " (BAD SIGNATURE)" : "")) : ""));
 		return r;
 	}
 
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index f377fea..00327cc 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1037,6 +1037,43 @@ test_expect_success \
 	'test_must_fail git tag -s -m tail tag-gpg-failure'
 git config --unset user.signingkey
 
+git tag -s -m 'good tag' good-tag HEAD
+bad=$(git cat-file tag good-tag | sed -e 's/good-tag/bad-tag/' | git mktag)
+git tag bad-tag $bad
+head=$(git rev-parse HEAD)
+nonkey=$(cat <<EOF | git mktag
+object $head
+type commit
+tag v1.6.0.4
+tagger Junio C Hamano <gitster@pobox.com> 1226208581 -0800
+
+GIT 1.6.0.4
+-----BEGIN PGP SIGNATURE-----
+Version: GnuPG v1.4.9 (GNU/Linux)
+
+iEYEABECAAYFAkkWdUUACgkQwMbZpPMRm5rSmwCfWu+K/hXyLUnEWoOMYy1eKuMK
+KcoAnjB2qir794ibWPy6cn11uUbk7AlC
+=eaFZ
+-----END PGP SIGNATURE-----
+EOF
+)
+git tag nonkey-tag $nonkey
+
+echo 'bad-tag (BAD SIGNATURE)' > expect
+echo 'good-tag (good signature)' >> expect
+echo 'nonkey-tag' >> expect
+
+test_expect_success \
+	'git fetch verifies tags' \
+	'mkdir clone &&
+	(cd clone &&
+	git init &&
+	git remote add origin file://"$(cd .. && pwd)" &&
+	git fetch origin 2>../actual) &&
+	sed -i -ne "/ \(bad\|good\|nonkey\)-tag/s/^.*->[^a-z]*//p" actual &&
+	test_cmp expect actual
+	'
+
 # try to verify without gpg:
 
 rm -rf gpghome
-- 
1.6.0.4.770.ga8394

Re: [RFC PATCH 4/4] Make git fetch verify signed tags

[Johannes Schindelin]

Hi,

On Sun, 23 Nov 2008, Deskin Miller wrote:

> When git fetch downloads signed tag objects, make it verify them right 
> then.  This extends the output summary of fetch to include "(good 
> signature)" for valid tags and "(BAD SIGNATURE)" for invalid tags.  If 
> the user does not have the correct key in the gpg keyring, gpg returns 
> 2, verify_tag_sha1 returns -2 and nothing additional is output about the 
> tag's validity.

This must be turned off by default, IMO.  You cannot expect each and every 
developer to have gpg _and_ all those public keys installed.

Ciao,
Dscho

Re: [RFC PATCH 4/4] Make git fetch verify signed tags

[Deskin Miller]

On Mon, Nov 24, 2008 at 11:44:40AM +0100, Johannes Schindelin wrote:
> Hi,
> 
> On Sun, 23 Nov 2008, Deskin Miller wrote:
> 
> > When git fetch downloads signed tag objects, make it verify them right 
> > then.  This extends the output summary of fetch to include "(good 
> > signature)" for valid tags and "(BAD SIGNATURE)" for invalid tags.  If 
> > the user does not have the correct key in the gpg keyring, gpg returns 
> > 2, verify_tag_sha1 returns -2 and nothing additional is output about the 
> > tag's validity.
> 
> This must be turned off by default, IMO.  You cannot expect each and every 
> developer to have gpg _and_ all those public keys installed.

Adding a configuration variable to control this makes sense, and is on
my TODO list for v2 (core.autoVerifyTags?).  However, I don't see a
compelling reason to make it off by default, as if gpg isn't found, or a
particular public key isn't in the keyring, the output is no different
from what fetch prints now.

Deskin Miller

Re: [RFC PATCH 1/4] Refactor builtin-verify-tag.c

[Johannes Schindelin]

Hi,

On Sun, 23 Nov 2008, Deskin Miller wrote:

> builtin-verify-tag.c didn't expose any of its functionality to be used
> internally.  Refactor some of it into new verify-tag.c and expose
> verify_tag_sha1 able to be called from elsewhere in git.
> 
> Signed-off-by: Deskin Miller <deskinm@umich.edu>
> ---
>  Makefile             |    2 +
>  builtin-verify-tag.c |   61 ++-------------------------------------
>  verify-tag.c         |   77 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  verify-tag.h         |   10 ++++++
>  4 files changed, 93 insertions(+), 57 deletions(-)
>  create mode 100644 verify-tag.c
>  create mode 100644 verify-tag.h

I'll comment on the output of "format-patch -n -C -C" instead, as that 
makes it much easier to see what you actually did:

>  Makefile                             |    2 +
>  builtin-verify-tag.c                 |   61 ++-------------------------------
>  builtin-verify-tag.c => verify-tag.c |   48 ++++-----------------------
>  verify-tag.h                         |   10 +++++
>  4 files changed, 23 insertions(+), 98 deletions(-)
>  copy builtin-verify-tag.c => verify-tag.c (56%)
>  create mode 100644 verify-tag.h
>
> [...] 
> diff --git a/builtin-verify-tag.c b/verify-tag.c
> similarity index 56%
> copy from builtin-verify-tag.c
> copy to verify-tag.c
> index 729a159..c9be331 100644
> --- a/builtin-verify-tag.c
> +++ b/verify-tag.c
> @@ -1,18 +1,12 @@
>  /*
> - * Builtin "git verify-tag"
> + * Internals for "git verify-tag"

Agree.

>   *
> - * Copyright (c) 2007 Carlos Rica <jasampler@gmail.com>
> + * Copyright (c) 2008 Deskin Miller <deskinm@umich.edu>

Disagree.

Even if Carlos seemed to stop his work on Git entirely, which I find 
disappointing, you are _not_ free to pretend his work is yours.  And given 
this diff:

>   *
> - * Based on git-verify-tag.sh
>   */
>  #include "cache.h"
> -#include "builtin.h"
> -#include "tag.h"
> +#include "object.h"
>  #include "run-command.h"
> -#include <signal.h>
> -
> -static const char builtin_verify_tag_usage[] =
> -		"git verify-tag [-v|--verbose] <tag>...";
>  
>  #define PGP_SIGNATURE "-----BEGIN PGP SIGNATURE-----"
>  
> @@ -60,52 +54,24 @@ static int run_gpg_verify(const char *buf, unsigned long size, int verbose)
>  	return ret;
>  }
>  
> -static int verify_tag(const char *name, int verbose)
> +int verify_tag_sha1(const unsigned char *sha1, int verbose)
>  {
>  	enum object_type type;
> -	unsigned char sha1[20];
>  	char *buf;
>  	unsigned long size;
>  	int ret;
>  
> -	if (get_sha1(name, sha1))
> -		return error("tag '%s' not found.", name);
> -
>  	type = sha1_object_info(sha1, NULL);
>  	if (type != OBJ_TAG)
> -		return error("%s: cannot verify a non-tag object of type %s.",
> -				name, typename(type));
> +		return error("Cannot verify a non-tag object of type %s.",
> +				typename(type));
>  
>  	buf = read_sha1_file(sha1, &type, &size);
>  	if (!buf)
> -		return error("%s: unable to read file.", name);
> +		return error("Cnable to read file.");
>  
>  	ret = run_gpg_verify(buf, size, verbose);
>  
>  	free(buf);
>  	return ret;
>  }
> -
> -int cmd_verify_tag(int argc, const char **argv, const char *prefix)
> -{
> -	int i = 1, verbose = 0, had_error = 0;
> -
> -	git_config(git_default_config, NULL);
> -
> -	if (argc > 1 &&
> -	    (!strcmp(argv[i], "-v") || !strcmp(argv[i], "--verbose"))) {
> -		verbose = 1;
> -		i++;
> -	}
> -
> -	if (argc <= i)
> -		usage(builtin_verify_tag_usage);
> -
> -	/* sometimes the program was terminated because this signal
> -	 * was received in the process of writing the gpg input: */
> -	signal(SIGPIPE, SIG_IGN);
> -	while (i < argc)
> -		if (verify_tag(argv[i++], verbose))
> -			had_error = 1;
> -	return had_error;
> -}

I think pretty much all you did was deleting (and thereby you do not gain 
any copyright).

Except for one change: why on earth did you think it a good idea to 
suppress telling the user the _name_ of the tag when an error occurs?

I, for one, would find it way less than helpful to read

	Cannot verify a non-tag object of type blob.

than to read

	refs/tags/dscho-key: cannot verify a non-tag object of type blob.

Besides, I do not see where you warn that "tag <name> not found."  Changes 
like this one need to be justified (by saying in the commit message where 
the warning is already issued, and not letting the reviewer/reader leave 
wondering).

Please, next time you submit a patch like this, do the -C -C yourself.  
Letting all the reviewers do it looks lousy on the overall time balance 
sheet, and it may also lead to a potential reviewer preferring to do 
something else instead.

Now, Junio already said that he is not (yet) convinced that this change 
should be in Git proper, rather than a hook, so it is up to you to decide 
if you deem it important enough to try harder to convince people.

I, for one, would think that it may be a good change: AFAIK only hard-core 
gits use hooks, everybody else avoids them.  So if we deem verifying 
signatures important enough, we might want to have better support for it 
than some example hooks.

So color me half-convinced.

Ciao,
Dscho

Re: [RFC PATCH 1/4] Refactor builtin-verify-tag.c

[Deskin Miller]

On Mon, Nov 24, 2008 at 12:04:59PM +0100, Johannes Schindelin wrote:
> Hi,
> 
> On Sun, 23 Nov 2008, Deskin Miller wrote:
> 
> > builtin-verify-tag.c didn't expose any of its functionality to be used
> > internally.  Refactor some of it into new verify-tag.c and expose
> > verify_tag_sha1 able to be called from elsewhere in git.
> > 
> > Signed-off-by: Deskin Miller <deskinm@umich.edu>
> > ---
> >  Makefile             |    2 +
> >  builtin-verify-tag.c |   61 ++-------------------------------------
> >  verify-tag.c         |   77 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >  verify-tag.h         |   10 ++++++
> >  4 files changed, 93 insertions(+), 57 deletions(-)
> >  create mode 100644 verify-tag.c
> >  create mode 100644 verify-tag.h
> 
> I'll comment on the output of "format-patch -n -C -C" instead, as that 
> makes it much easier to see what you actually did:

Didn't realise -C -C was the magic incantation; I'll remember it for the
future.

> >  Makefile                             |    2 +
> >  builtin-verify-tag.c                 |   61 ++-------------------------------
> >  builtin-verify-tag.c => verify-tag.c |   48 ++++-----------------------
> >  verify-tag.h                         |   10 +++++
> >  4 files changed, 23 insertions(+), 98 deletions(-)
> >  copy builtin-verify-tag.c => verify-tag.c (56%)
> >  create mode 100644 verify-tag.h
> >
> > [...] 
> > diff --git a/builtin-verify-tag.c b/verify-tag.c
> > similarity index 56%
> > copy from builtin-verify-tag.c
> > copy to verify-tag.c
> > index 729a159..c9be331 100644
> > --- a/builtin-verify-tag.c
> > +++ b/verify-tag.c
> > @@ -1,18 +1,12 @@
> >  /*
> > - * Builtin "git verify-tag"
> > + * Internals for "git verify-tag"
> 
> Agree.
> 
> >   *
> > - * Copyright (c) 2007 Carlos Rica <jasampler@gmail.com>
> > + * Copyright (c) 2008 Deskin Miller <deskinm@umich.edu>
> 
> Disagree.
> 
> Even if Carlos seemed to stop his work on Git entirely, which I find 
> disappointing, you are _not_ free to pretend his work is yours.  And given 
> this diff:
> [...] 
> I think pretty much all you did was deleting (and thereby you do not gain 
> any copyright).

I realised my mistake in altering the copyright information just after
sending out these patches.  I think I'd written the header first in
verify-tag.c before copying the code in; though I couldn't say what I
thought I'd be writing that would end up protected by copyright.  At any
rate, it was an honest mistake, and I apologise, Carlos, for my
unintended plagarism; I'll be sure to restore the proper copyright
notice for any subsequent versions.

> Except for one change: why on earth did you think it a good idea to 
> suppress telling the user the _name_ of the tag when an error occurs?
> 
> I, for one, would find it way less than helpful to read
> 
> 	Cannot verify a non-tag object of type blob.
> 
> than to read
> 
> 	refs/tags/dscho-key: cannot verify a non-tag object of type blob.
>
> Besides, I do not see where you warn that "tag <name> not found."  Changes 
> like this one need to be justified (by saying in the commit message where 
> the warning is already issued, and not letting the reviewer/reader leave 
> wondering).

The verify_tag_sha1 function is newly exposed to the rest of git, and
has a different signature from verify_tag, which could take a ref while
verify_tag_sha1 takes a sha1.  verify_tag still includes both the checks
you refer to before calling verify_tag_sha1, so the error output is
identical in all cases before and after applying this patch.

The OBJ_TAG check, however, is duplicated so that internal git calls to
verify_tag_sha1 can't pass in e.g. a blob sha1 which just happens to
contain the same contents as a signed tag.

Actually, I initially did not leave the OBJ_TAG check in verify_tag, but
relied on it checking the return value of verify_tag_sha1 to see if an
error occurred, and printing 'Failed to verify <name>' in that case, for
precisely the reason you point out, that the ref name is very useful in
this failure case.  However, I ultimately decided to duplicate the check
so that the error output would match up exactly.
 
> Please, next time you submit a patch like this, do the -C -C yourself.  
> Letting all the reviewers do it looks lousy on the overall time balance 
> sheet, and it may also lead to a potential reviewer preferring to do 
> something else instead.

Will do; thanks for reviewing in spite of my shortcomings.

> Now, Junio already said that he is not (yet) convinced that this change 
> should be in Git proper, rather than a hook, so it is up to you to decide 
> if you deem it important enough to try harder to convince people.
> 
> I, for one, would think that it may be a good change: AFAIK only hard-core 
> gits use hooks, everybody else avoids them.  So if we deem verifying 
> signatures important enough, we might want to have better support for it 
> than some example hooks.
> 
> So color me half-convinced.
> 
> Ciao,
> Dscho

Deskin Miller 

Re: [RFC PATCH 0/4] Teach git fetch to verify signed tags automatically

[Junio C Hamano]

Deskin Miller <deskinm@umich.edu> writes:

> It struck me a while back when I fetched a new tagged release from git.git that
> if I wanted to verify the tag's signature, I'd have to issue another command to
> do so.  Shouldn't git be able to do that for me automatically, when it fetches
> signed tags?  Now it does.  Also, 'git remote update' gets this for free.

I think this should be done inside your own hook.  Not interested at all
in a solution to touch builtin-fetch.c, unless if the patch is about
adding a new hook so that people with other needs can use it as well.

Re: [RFC PATCH 0/4] Teach git fetch to verify signed tags automatically

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Deskin Miller <deskinm@umich.edu> writes:
>
>> It struck me a while back when I fetched a new tagged release from git.git that
>> if I wanted to verify the tag's signature, I'd have to issue another command to
>> do so.  Shouldn't git be able to do that for me automatically, when it fetches
>> signed tags?  Now it does.  Also, 'git remote update' gets this for free.
>
> I think this should be done inside your own hook.  Not interested at all
> in a solution to touch builtin-fetch.c, unless if the patch is about
> adding a new hook so that people with other needs can use it as well.

... or a much stronger case can be made why this shouldn't be done in a
hook.

I realize "not interested at all" was a bit too strong, so I am trying to
rephrase it here.  The cycle that begins with an RFC that leads to
discussion and review is about clarifying the rationale and design
incrementally, so please do not get offended by my no, and sorry for using
unnecessarily strong wording.

What I meant was more like "The justification as given in the message does
not interest me in the patch at all as it stands.  I do not understand why
this has to be done as a patch to git-fetch itself, not in a hook script,
or why doing it inside git-fetch is a better approach than doing it in a
hook (if there already is a hook mechanism to do this)".

Re: [RFC PATCH 0/4] Teach git fetch to verify signed tags automatically

[Deskin Miller]

On Sun, Nov 23, 2008 at 09:30:56PM -0800, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Deskin Miller <deskinm@umich.edu> writes:
> >
> >> It struck me a while back when I fetched a new tagged release from git.git that
> >> if I wanted to verify the tag's signature, I'd have to issue another command to
> >> do so.  Shouldn't git be able to do that for me automatically, when it fetches
> >> signed tags?  Now it does.  Also, 'git remote update' gets this for free.
> >
> > I think this should be done inside your own hook.  Not interested at all
> > in a solution to touch builtin-fetch.c, unless if the patch is about
> > adding a new hook so that people with other needs can use it as well.
> 
> ... or a much stronger case can be made why this shouldn't be done in a
> hook.
> 
> I realize "not interested at all" was a bit too strong, so I am trying to
> rephrase it here.  The cycle that begins with an RFC that leads to
> discussion and review is about clarifying the rationale and design
> incrementally, so please do not get offended by my no, and sorry for using
> unnecessarily strong wording.
> 
> What I meant was more like "The justification as given in the message does
> not interest me in the patch at all as it stands.  I do not understand why
> this has to be done as a patch to git-fetch itself, not in a hook script,
> or why doing it inside git-fetch is a better approach than doing it in a
> hook (if there already is a hook mechanism to do this)".

Let's try this then:

-----
Despite core git's built-in support of cryptographic authentication and
integrity verification through the use of signed tags, git still fails
to provide a good first line of defense against malicious entities
hijacking repositories and disseminating arbitrary code, since git does
not try to verify signed tags at the time they are fetched.  If such a
compromise occurred, the prospect of even one individual who did not
verify the newly-fetched tag prior to use gives this a large potential
value to attackers, and represents a commensurate risk to the git-using
community.

This patch series mitigates this risk by trying to verify each signed
tag when it is first fetched.  Since, however, not everyone is concerned
with the security of signed tags, this feature tries to be conservative
insofar as signatures with public keys which are missing from the user's
keyring do not cause anything to be said about the tag's validity;
furthermore, a configuration variable exists to disable these checks
entirely, if desired.
-----
*the RFC patch series v1 does not include such a configuration variable.

I appreciate that such verification could be accomplished by the
as-yet-nonexistent post-fetch hook, and if that hook existed, I probably
would have done this in that hook.  With that said, I do feel like this
feature merits consideration for inclusion in the builtin fetch code.
First, I very much agree with what Dscho said in his review of patch 1,
that hooks represent a rather more advanced feature of git than most
users are willing to investigate.

So the question, then, is whether this feature is important enough to
include in core git.  I of course think that it is important enough.
Given that we have git-tag installed by default when one builds git,
there is a certain commitment to supporting the use of signed tags; and
I see verifying them when first fetched as a logical continuation of
this support.  As such, a hook seems an unsuitable approach to provide
support for this new use of signed tags.

I'm happy to ask what suggestions you have for someone intending to
implement hooks around fetch; I've not looked at the code in this light,
but someone's got to do it sooner or later.

Deskin Miller

Re: [RFC PATCH 0/4] Teach git fetch to verify signed tags automatically

[Johannes Schindelin]

Hi,

On Thu, 27 Nov 2008, Deskin Miller wrote:

> This patch series mitigates this risk by trying to verify each signed 
> tag when it is first fetched.  Since, however, not everyone is concerned 
> with the security of signed tags, this feature tries to be conservative 
> insofar as signatures with public keys which are missing from the user's 
> keyring do not cause anything to be said about the tag's validity;

Now, in the context of security, this is not conservative.  Conservative 
would be to fail as soon as a signature could not be verified, be it that 
there is no key to match against, or that the signature is corrupt.

Your notion to fail silently if the necessary keys were not found makes 
your patch series rather useless, no?

After all, the whole idea is to let Git check if every signature is 
correct, and when Git does not fail, rely on them being valid.

So I think that the _only_ thing that would make sense is to fail _unless_ 
all the signatures were verified to be correct.

_That_ is why I want this feature to be off by default.

Ciao,
Dscho

Re: [RFC PATCH 0/4] Teach git fetch to verify signed tags automatically

[Johannes Schindelin]

Hi,

On Sun, 23 Nov 2008, Deskin Miller wrote:

> -What to do if a tag is found to have a bad signature?

Or even worse: if the public key was not found?  In dubio pro reo, they 
say, but OTOH you asked to verify the signatures...

Ciao,
Dscho

Re: [RFC PATCH 0/4] Teach git fetch to verify signed tags automatically

[Deskin Miller]

On Mon, Nov 24, 2008 at 11:41:27AM +0100, Johannes Schindelin wrote:
> On Sun, 23 Nov 2008, Deskin Miller wrote:
> 
> > -What to do if a tag is found to have a bad signature?
> 
> Or even worse: if the public key was not found?  In dubio pro reo, they 
> say, but OTOH you asked to verify the signatures...

I don't see how not finding the public key is `worse' than a bad
signature.  Compared to what the user learns currently when they run git
fetch and receive new signed tags, the case of not having the required
public key leaves them in exactly the same state: the user does not know
whether the signature is valid or not.

The user didn't ask to verify, as I see it; rather, they asked git to
*try* to verify.  If that fails in a way they don't expect, they're free
to investigate further with git tag -v for situations like not having
the right public key.

Deskin Miller 

Re: [RFC PATCH 0/4] Teach git fetch to verify signed tags automatically

[Junio C Hamano]

Deskin Miller <deskinm@umich.edu> writes:

> The user didn't ask to verify, as I see it; rather, they asked git to
> *try* to verify.

If that is your argument, I really do not see any point in your patch.
They asked git to fetch, and did not say anything about trying anything
else.