[PATCH 0/3] verify-commit: verify commit signatures

[PATCH 0/3] verify-commit: verify commit signatures

[Michael J Gruber]

Hi there,

Some of you may remember me from my more active times...

Anyways, a recent blog post about signed commits in git triggered me to
look at our tools for that again. It seems that we only have the
log/pretty family on the user facing side, but everything we need under
the hood.

So here's a suggestion to implement verify-commit in a way which is
completely analogous to verify-tag. In fact, it could be coded more
elegantly, but I kept it this way so that we could merge the two more
easily in case we wish to do so.

I will follow up with tests if the design principle is something we agree
upon.

Michael J Gruber (3):
  pretty: free the gpg status buf
  gpg-interface: provide access to the payload
  verify-commit: scriptable commit signature verification

 Documentation/git-verify-commit.txt | 28 +++++++++++
 Makefile                            |  1 +
 builtin.h                           |  1 +
 builtin/merge.c                     |  1 +
 builtin/verify-commit.c             | 98 +++++++++++++++++++++++++++++++++++++
 command-list.txt                    |  1 +
 commit.c                            |  1 +
 git.c                               |  1 +
 gpg-interface.h                     |  1 +
 pretty.c                            |  2 +
 10 files changed, 135 insertions(+)
 create mode 100644 Documentation/git-verify-commit.txt
 create mode 100644 builtin/verify-commit.c

-- 
2.0.0.533.gae2e602

[PATCH 1/3] pretty: free the gpg status buf

[Michael J Gruber]

4a868fd (pretty: parse the gpg status lines rather than the output, 2013-02-14)
made the gpg status lines available to callers and made sure they freed
the used space, but missed one spot.

Free the status line buffer also in the remaining spot.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 pretty.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/pretty.c b/pretty.c
index 4f51287..f1e8a70 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1538,6 +1538,7 @@ void format_commit_message(const struct commit *commit,
 	free(context.commit_encoding);
 	logmsg_free(context.message, commit);
 	free(context.signature_check.gpg_output);
+	free(context.signature_check.gpg_status);
 	free(context.signature_check.signer);
 }
 
-- 
2.0.0.533.gae2e602

[PATCH 2/3] gpg-interface: provide access to the payload

[Michael J Gruber]

In contrast to tag signatures, commit signatures are put into the
header, that is between the other header parts and commit messages.

Provide access to the commit content sans the signature, which is the
payload that is actually signed. Commit signature verification does the
parsing anyways, and callers may wish to act on or display the commit
object sans the signature.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 builtin/merge.c | 1 +
 commit.c        | 1 +
 gpg-interface.h | 1 +
 pretty.c        | 1 +
 4 files changed, 4 insertions(+)

diff --git a/builtin/merge.c b/builtin/merge.c
index 428ca24..6a9812a 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1282,6 +1282,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 				printf(_("Commit %s has a good GPG signature by %s\n"),
 				       hex, signature_check.signer);
 
+			free(signature_check.payload);
 			free(signature_check.gpg_output);
 			free(signature_check.gpg_status);
 			free(signature_check.signer);
diff --git a/commit.c b/commit.c
index f479331..e9686b2 100644
--- a/commit.c
+++ b/commit.c
@@ -1219,6 +1219,7 @@ void check_commit_signature(const struct commit* commit, struct signature_check
 				      &gpg_output, &gpg_status);
 	if (status && !gpg_output.len)
 		goto out;
+	sigc->payload = strbuf_detach(&payload, NULL);
 	sigc->gpg_output = strbuf_detach(&gpg_output, NULL);
 	sigc->gpg_status = strbuf_detach(&gpg_status, NULL);
 	parse_gpg_output(sigc);
diff --git a/gpg-interface.h b/gpg-interface.h
index a85cb5b..d727c25 100644
--- a/gpg-interface.h
+++ b/gpg-interface.h
@@ -2,6 +2,7 @@
 #define GPG_INTERFACE_H
 
 struct signature_check {
+	char *payload;
 	char *gpg_output;
 	char *gpg_status;
 	char result; /* 0 (not checked),
diff --git a/pretty.c b/pretty.c
index f1e8a70..24fb877 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1537,6 +1537,7 @@ void format_commit_message(const struct commit *commit,
 
 	free(context.commit_encoding);
 	logmsg_free(context.message, commit);
+	free(context.signature_check.payload);
 	free(context.signature_check.gpg_output);
 	free(context.signature_check.gpg_status);
 	free(context.signature_check.signer);
-- 
2.0.0.533.gae2e602

Re: [PATCH 2/3] gpg-interface: provide access to the payload

[Jeff King]

On Fri, Jun 06, 2014 at 04:15:27PM +0200, Michael J Gruber wrote:

> diff --git a/builtin/merge.c b/builtin/merge.c
> [...]
> +			free(signature_check.payload);
>  			free(signature_check.gpg_output);
>  			free(signature_check.gpg_status);
>  			free(signature_check.signer);
> [...]
> diff --git a/pretty.c b/pretty.c
> [...]
> +	free(context.signature_check.payload);
>  	free(context.signature_check.gpg_output);
>  	free(context.signature_check.gpg_status);
>  	free(context.signature_check.signer);

Perhaps this is a sign that we need a "signature_check_clear()" helper?

-Peff

Re: [PATCH 2/3] gpg-interface: provide access to the payload

[Michael J Gruber]

Jeff King venit, vidit, dixit 13.06.2014 09:55:
> On Fri, Jun 06, 2014 at 04:15:27PM +0200, Michael J Gruber wrote:
> 
>> diff --git a/builtin/merge.c b/builtin/merge.c
>> [...]
>> +			free(signature_check.payload);
>>  			free(signature_check.gpg_output);
>>  			free(signature_check.gpg_status);
>>  			free(signature_check.signer);
>> [...]
>> diff --git a/pretty.c b/pretty.c
>> [...]
>> +	free(context.signature_check.payload);
>>  	free(context.signature_check.gpg_output);
>>  	free(context.signature_check.gpg_status);
>>  	free(context.signature_check.signer);
> 
> Perhaps this is a sign that we need a "signature_check_clear()" helper?

... or simply switch to language which has (or can overload) free for an
object :)

Do we have prior art for such helpers so that the new one would be
analogous?

Michael

Re: [PATCH 2/3] gpg-interface: provide access to the payload

[Jeff King]

On Fri, Jun 13, 2014 at 11:44:28AM +0200, Michael J Gruber wrote:

> > Perhaps this is a sign that we need a "signature_check_clear()" helper?
> 
> ... or simply switch to language which has (or can overload) free for an
> object :)

I hear somebody has reimplemented git in pure javascript. ;P

> Do we have prior art for such helpers so that the new one would be
> analogous?

I was thinking of credential_clear, string_list_clear, etc. Literally
just:

  void signature_check_clear(struct signature_check *s)
  {
	free(s->gpg_output);
	free(s->gpg_status);
	free(s->signer);
	free(s->key);
  }

Your first commit fixed a leak on gpg_status.  Did it also need to handle
the "key" field there?

For some structs, we'd also do:

	memset(s, 0, sizeof(*s));

to get us back to a usable, initialized state so the struct can be
reused. However, check_commit_signature doesn't care if the struct is
initialized or not (i.e., there is no initialized state). Doing so does
help detect use-after-free conditions, though.

-Peff

[PATCH 3/3] verify-commit: scriptable commit signature verification

[Michael J Gruber]

Commit signatures can be verified using "git show -s --show-signature"
or the "%G?" pretty format and parsing the output, which is well suited
for user inspection, but not for scripting.

Provide a command "verify-commit" which is analogous to "verify-tag": It
returns 0 for good signatures and non-zero otherwise, has the gpg output
on stderr and (optionally) the commit object on stdout, sans the
signature, just like "verify-tag" does.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 Documentation/git-verify-commit.txt | 28 +++++++++++
 Makefile                            |  1 +
 builtin.h                           |  1 +
 builtin/verify-commit.c             | 98 +++++++++++++++++++++++++++++++++++++
 command-list.txt                    |  1 +
 git.c                               |  1 +
 6 files changed, 130 insertions(+)
 create mode 100644 Documentation/git-verify-commit.txt
 create mode 100644 builtin/verify-commit.c

diff --git a/Documentation/git-verify-commit.txt b/Documentation/git-verify-commit.txt
new file mode 100644
index 0000000..dcd7803
--- /dev/null
+++ b/Documentation/git-verify-commit.txt
@@ -0,0 +1,28 @@
+git-verify-commit(1)
+=================
+
+NAME
+----
+git-verify-commit - Check the GPG signature of commits
+
+SYNOPSIS
+--------
+[verse]
+'git verify-commit' <commit>...
+
+DESCRIPTION
+-----------
+Validates the gpg signature created by 'git commit -S'.
+
+OPTIONS
+-------
+-v::
+--verbose::
+	Print the contents of the commit object before validating it.
+
+<commit>...::
+	SHA-1 identifiers of Git commit objects.
+
+GIT
+---
+Part of the linkgit:git[1] suite
diff --git a/Makefile b/Makefile
index 07ea105..b92418d 100644
--- a/Makefile
+++ b/Makefile
@@ -999,6 +999,7 @@ BUILTIN_OBJS += builtin/update-ref.o
 BUILTIN_OBJS += builtin/update-server-info.o
 BUILTIN_OBJS += builtin/upload-archive.o
 BUILTIN_OBJS += builtin/var.o
+BUILTIN_OBJS += builtin/verify-commit.o
 BUILTIN_OBJS += builtin/verify-pack.o
 BUILTIN_OBJS += builtin/verify-tag.o
 BUILTIN_OBJS += builtin/write-tree.o
diff --git a/builtin.h b/builtin.h
index c47c110..5d91f31 100644
--- a/builtin.h
+++ b/builtin.h
@@ -128,6 +128,7 @@ extern int cmd_update_server_info(int argc, const char **argv, const char *prefi
 extern int cmd_upload_archive(int argc, const char **argv, const char *prefix);
 extern int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix);
 extern int cmd_var(int argc, const char **argv, const char *prefix);
+extern int cmd_verify_commit(int argc, const char **argv, const char *prefix);
 extern int cmd_verify_tag(int argc, const char **argv, const char *prefix);
 extern int cmd_version(int argc, const char **argv, const char *prefix);
 extern int cmd_whatchanged(int argc, const char **argv, const char *prefix);
diff --git a/builtin/verify-commit.c b/builtin/verify-commit.c
new file mode 100644
index 0000000..69b7c6d
--- /dev/null
+++ b/builtin/verify-commit.c
@@ -0,0 +1,98 @@
+/*
+ * Builtin "git commit-commit"
+ *
+ * Copyright (c) 2014 Michael J Gruber <git@drmicha.warpmail.net>
+ *
+ * Based on git-verify-tag
+ */
+#include "cache.h"
+#include "builtin.h"
+#include "commit.h"
+#include "run-command.h"
+#include <signal.h>
+#include "parse-options.h"
+#include "gpg-interface.h"
+
+static const char * const verify_commit_usage[] = {
+		N_("git verify-commit [-v|--verbose] <commit>..."),
+		NULL
+};
+
+static int run_gpg_verify(const unsigned char *sha1, const char *buf, unsigned long size, int verbose)
+{
+	struct signature_check signature_check;
+
+	memset(&signature_check, 0, sizeof(signature_check));
+
+	check_commit_signature(lookup_commit(sha1), &signature_check);
+
+	if (verbose && signature_check.payload)
+		fputs(signature_check.payload, stdout);
+
+	if (signature_check.gpg_output)
+		fputs(signature_check.gpg_output, stderr);
+
+	free(signature_check.gpg_output);
+	free(signature_check.gpg_status);
+	free(signature_check.signer);
+	free(signature_check.key);
+	return signature_check.result != 'G';
+}
+
+static int verify_commit(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))
+		return error("commit '%s' not found.", name);
+
+	type = sha1_object_info(sha1, NULL);
+	if (type != OBJ_COMMIT)
+		return error("%s: cannot verify a non-commit 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(sha1, buf, size, verbose);
+
+	free(buf);
+	return ret;
+}
+
+static int git_verify_commit_config(const char *var, const char *value, void *cb)
+{
+	int status = git_gpg_config(var, value, cb);
+	if (status)
+		return status;
+	return git_default_config(var, value, cb);
+}
+
+int cmd_verify_commit(int argc, const char **argv, const char *prefix)
+{
+	int i = 1, verbose = 0, had_error = 0;
+	const struct option verify_commit_options[] = {
+		OPT__VERBOSE(&verbose, N_("print commit contents")),
+		OPT_END()
+	};
+
+	git_config(git_verify_commit_config, NULL);
+
+	argc = parse_options(argc, argv, prefix, verify_commit_options,
+			     verify_commit_usage, PARSE_OPT_KEEP_ARGV0);
+	if (argc <= i)
+		usage_with_options(verify_commit_usage, verify_commit_options);
+
+	/* 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_commit(argv[i++], verbose))
+			had_error = 1;
+	return had_error;
+}
diff --git a/command-list.txt b/command-list.txt
index cf36c3d..a3ff0c9 100644
--- a/command-list.txt
+++ b/command-list.txt
@@ -132,6 +132,7 @@ git-update-server-info                  synchingrepositories
 git-upload-archive                      synchelpers
 git-upload-pack                         synchelpers
 git-var                                 plumbinginterrogators
+git-verify-commit                       ancillaryinterrogators
 git-verify-pack                         plumbinginterrogators
 git-verify-tag                          ancillaryinterrogators
 gitweb                                  ancillaryinterrogators
diff --git a/git.c b/git.c
index 7780572..bbc3293 100644
--- a/git.c
+++ b/git.c
@@ -441,6 +441,7 @@ static struct cmd_struct commands[] = {
 	{ "upload-archive", cmd_upload_archive },
 	{ "upload-archive--writer", cmd_upload_archive_writer },
 	{ "var", cmd_var, RUN_SETUP_GENTLY },
+	{ "verify-commit", cmd_verify_commit, RUN_SETUP },
 	{ "verify-pack", cmd_verify_pack },
 	{ "verify-tag", cmd_verify_tag, RUN_SETUP },
 	{ "version", cmd_version },
-- 
2.0.0.533.gae2e602

Re: [PATCH 3/3] verify-commit: scriptable commit signature verification

[Michael J Gruber]

On 6. Juni 2014 16:15:28 MESZ, Michael J Gruber <git@drmicha.warpmail.net> wrote:
>Commit signatures can be verified using "git show -s --show-signature"
>or the "%G?" pretty format and parsing the output, which is well suited
>for user inspection, but not for scripting.
>
>Provide a command "verify-commit" which is analogous to "verify-tag":
>It
>returns 0 for good signatures and non-zero otherwise, has the gpg
>output
>on stderr and (optionally) the commit object on stdout, sans the
>signature, just like "verify-tag" does.
>
>Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
>---
> Documentation/git-verify-commit.txt | 28 +++++++++++
> Makefile                            |  1 +
> builtin.h                           |  1 +
>builtin/verify-commit.c             | 98
>+++++++++++++++++++++++++++++++++++++
> command-list.txt                    |  1 +
> git.c                               |  1 +
> 6 files changed, 130 insertions(+)
> create mode 100644 Documentation/git-verify-commit.txt
> create mode 100644 builtin/verify-commit.c
>
>diff --git a/Documentation/git-verify-commit.txt
>b/Documentation/git-verify-commit.txt
>new file mode 100644
>index 0000000..dcd7803
>--- /dev/null
>+++ b/Documentation/git-verify-commit.txt
>@@ -0,0 +1,28 @@
>+git-verify-commit(1)
>+=================

That line will need 3 more "=" thanks to asciidoc stubbornness.

v2 plus the tests coming tomorrow when my dev box is online again.

>+
>+NAME
>+----
>+git-verify-commit - Check the GPG signature of commits
>+
>+SYNOPSIS
>+--------
>+[verse]
>+'git verify-commit' <commit>...
>+
>+DESCRIPTION
>+-----------
>+Validates the gpg signature created by 'git commit -S'.
>+
>+OPTIONS
>+-------
>+-v::
>+--verbose::
>+	Print the contents of the commit object before validating it.
>+
>+<commit>...::
>+	SHA-1 identifiers of Git commit objects.
>+
>+GIT
>+---
>+Part of the linkgit:git[1] suite
>diff --git a/Makefile b/Makefile
>index 07ea105..b92418d 100644
>--- a/Makefile
>+++ b/Makefile
>@@ -999,6 +999,7 @@ BUILTIN_OBJS += builtin/update-ref.o
> BUILTIN_OBJS += builtin/update-server-info.o
> BUILTIN_OBJS += builtin/upload-archive.o
> BUILTIN_OBJS += builtin/var.o
>+BUILTIN_OBJS += builtin/verify-commit.o
> BUILTIN_OBJS += builtin/verify-pack.o
> BUILTIN_OBJS += builtin/verify-tag.o
> BUILTIN_OBJS += builtin/write-tree.o
>diff --git a/builtin.h b/builtin.h
>index c47c110..5d91f31 100644
>--- a/builtin.h
>+++ b/builtin.h
>@@ -128,6 +128,7 @@ extern int cmd_update_server_info(int argc, const
>char **argv, const char *prefi
>extern int cmd_upload_archive(int argc, const char **argv, const char
>*prefix);
>extern int cmd_upload_archive_writer(int argc, const char **argv, const
>char *prefix);
> extern int cmd_var(int argc, const char **argv, const char *prefix);
>+extern int cmd_verify_commit(int argc, const char **argv, const char
>*prefix);
>extern int cmd_verify_tag(int argc, const char **argv, const char
>*prefix);
>extern int cmd_version(int argc, const char **argv, const char
>*prefix);
>extern int cmd_whatchanged(int argc, const char **argv, const char
>*prefix);
>diff --git a/builtin/verify-commit.c b/builtin/verify-commit.c
>new file mode 100644
>index 0000000..69b7c6d
>--- /dev/null
>+++ b/builtin/verify-commit.c
>@@ -0,0 +1,98 @@
>+/*
>+ * Builtin "git commit-commit"
>+ *
>+ * Copyright (c) 2014 Michael J Gruber <git@drmicha.warpmail.net>
>+ *
>+ * Based on git-verify-tag
>+ */
>+#include "cache.h"
>+#include "builtin.h"
>+#include "commit.h"
>+#include "run-command.h"
>+#include <signal.h>
>+#include "parse-options.h"
>+#include "gpg-interface.h"
>+
>+static const char * const verify_commit_usage[] = {
>+		N_("git verify-commit [-v|--verbose] <commit>..."),
>+		NULL
>+};
>+
>+static int run_gpg_verify(const unsigned char *sha1, const char *buf,
>unsigned long size, int verbose)
>+{
>+	struct signature_check signature_check;
>+
>+	memset(&signature_check, 0, sizeof(signature_check));
>+
>+	check_commit_signature(lookup_commit(sha1), &signature_check);
>+
>+	if (verbose && signature_check.payload)
>+		fputs(signature_check.payload, stdout);
>+
>+	if (signature_check.gpg_output)
>+		fputs(signature_check.gpg_output, stderr);
>+
>+	free(signature_check.gpg_output);
>+	free(signature_check.gpg_status);
>+	free(signature_check.signer);
>+	free(signature_check.key);
>+	return signature_check.result != 'G';
>+}
>+
>+static int verify_commit(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))
>+		return error("commit '%s' not found.", name);
>+
>+	type = sha1_object_info(sha1, NULL);
>+	if (type != OBJ_COMMIT)
>+		return error("%s: cannot verify a non-commit 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(sha1, buf, size, verbose);
>+
>+	free(buf);
>+	return ret;
>+}
>+
>+static int git_verify_commit_config(const char *var, const char
>*value, void *cb)
>+{
>+	int status = git_gpg_config(var, value, cb);
>+	if (status)
>+		return status;
>+	return git_default_config(var, value, cb);
>+}
>+
>+int cmd_verify_commit(int argc, const char **argv, const char *prefix)
>+{
>+	int i = 1, verbose = 0, had_error = 0;
>+	const struct option verify_commit_options[] = {
>+		OPT__VERBOSE(&verbose, N_("print commit contents")),
>+		OPT_END()
>+	};
>+
>+	git_config(git_verify_commit_config, NULL);
>+
>+	argc = parse_options(argc, argv, prefix, verify_commit_options,
>+			     verify_commit_usage, PARSE_OPT_KEEP_ARGV0);
>+	if (argc <= i)
>+		usage_with_options(verify_commit_usage, verify_commit_options);
>+
>+	/* 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_commit(argv[i++], verbose))
>+			had_error = 1;
>+	return had_error;
>+}
>diff --git a/command-list.txt b/command-list.txt
>index cf36c3d..a3ff0c9 100644
>--- a/command-list.txt
>+++ b/command-list.txt
>@@ -132,6 +132,7 @@ git-update-server-info                 
>synchingrepositories
> git-upload-archive                      synchelpers
> git-upload-pack                         synchelpers
> git-var                                 plumbinginterrogators
>+git-verify-commit                       ancillaryinterrogators
> git-verify-pack                         plumbinginterrogators
> git-verify-tag                          ancillaryinterrogators
> gitweb                                  ancillaryinterrogators
>diff --git a/git.c b/git.c
>index 7780572..bbc3293 100644
>--- a/git.c
>+++ b/git.c
>@@ -441,6 +441,7 @@ static struct cmd_struct commands[] = {
> 	{ "upload-archive", cmd_upload_archive },
> 	{ "upload-archive--writer", cmd_upload_archive_writer },
> 	{ "var", cmd_var, RUN_SETUP_GENTLY },
>+	{ "verify-commit", cmd_verify_commit, RUN_SETUP },
> 	{ "verify-pack", cmd_verify_pack },
> 	{ "verify-tag", cmd_verify_tag, RUN_SETUP },
> 	{ "version", cmd_version },

Re: [PATCH 3/3] verify-commit: scriptable commit signature verification

[Jeff King]

On Fri, Jun 06, 2014 at 04:15:28PM +0200, Michael J Gruber wrote:

> Commit signatures can be verified using "git show -s --show-signature"
> or the "%G?" pretty format and parsing the output, which is well suited
> for user inspection, but not for scripting.
> 
> Provide a command "verify-commit" which is analogous to "verify-tag": It
> returns 0 for good signatures and non-zero otherwise, has the gpg output
> on stderr and (optionally) the commit object on stdout, sans the
> signature, just like "verify-tag" does.
> 
> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>

I think the general direction of this series is reasonable.

Did you give any thought to just having a "git verify" command, instead
of separate tag/verify commands?

Another thought, that may be orthogonal to your series: what does it
mean to verify a commit? We check for _some_ signature from a key that
is in your keyring. But we do not check whether the signature matches
the committer field (or for tags, the tagger field). You have to parse
the gpg output, run "git cat-file", and then correlate the two. Should
there be an option to have git check that one of the signed uids from
gpg matches the commit's committer?

-Peff

Re: [PATCH 3/3] verify-commit: scriptable commit signature verification

[Michael J Gruber]

Jeff King venit, vidit, dixit 13.06.2014 10:02:
> On Fri, Jun 06, 2014 at 04:15:28PM +0200, Michael J Gruber wrote:
> 
>> Commit signatures can be verified using "git show -s --show-signature"
>> or the "%G?" pretty format and parsing the output, which is well suited
>> for user inspection, but not for scripting.
>>
>> Provide a command "verify-commit" which is analogous to "verify-tag": It
>> returns 0 for good signatures and non-zero otherwise, has the gpg output
>> on stderr and (optionally) the commit object on stdout, sans the
>> signature, just like "verify-tag" does.
>>
>> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
> 
> I think the general direction of this series is reasonable.
> 
> Did you give any thought to just having a "git verify" command, instead
> of separate tag/verify commands?

Yes. (mathematician's answer)

You know not only the outcome but also why I refrained from doing so:
compatibility. We would need to deprecate verify-tag.

But there is also a more subtle reason: If you want to verify a signed
commit, you want to be sure that it actually is a commit. "verify" could
easily branch code paths based on the object type, but I'm not sure that
is desirable, at least not by default.

> Another thought, that may be orthogonal to your series: what does it
> mean to verify a commit? We check for _some_ signature from a key that
> is in your keyring. But we do not check whether the signature matches
> the committer field (or for tags, the tagger field). You have to parse
> the gpg output, run "git cat-file", and then correlate the two. Should
> there be an option to have git check that one of the signed uids from
> gpg matches the commit's committer?
> 
> -Peff
> 

That is a general issue with verifying signatures: it can be automated
only if you employ a strict trust model and a very limited keyring.
"valid signature" means only as much as the signatures that your gpg
accepts can be really trusted.

Comparing uid's really buys you nothing in the sense that everyone can
have a key with uid "Jeff King <peff@peff.net> signed by some other
keys. On the other hand, it's perfectly OK to use different uids for git
commits and signatures. The e-mail address I use for the git list and
commits, for example, is clearly a "plus address", which helps me
organize things; my personal key has the primary address as uid.

I really think all this is up to local policies for individual use cases.

Michael

Re: [PATCH 3/3] verify-commit: scriptable commit signature verification

[Jeff King]

On Fri, Jun 13, 2014 at 11:55:22AM +0200, Michael J Gruber wrote:

> > Did you give any thought to just having a "git verify" command, instead
> > of separate tag/verify commands?
> 
> Yes. (mathematician's answer)

Cute.

> You know not only the outcome but also why I refrained from doing so:
> compatibility. We would need to deprecate verify-tag.

Yes, we'd certainly leave verify-tag in place for compatibility. I don't
think that makes "git verify" a bad idea necessarily, if it is a better
interface. But...

> But there is also a more subtle reason: If you want to verify a signed
> commit, you want to be sure that it actually is a commit. "verify" could
> easily branch code paths based on the object type, but I'm not sure that
> is desirable, at least not by default.

Yes, I wasn't sure about that part. I think it really depends on what
people want to use it for. I was thinking more of a porcelain, anyway,
to just check whatever signatures are available. But I don't really sign
my commits right now anyway, so I'm somewhat guessing.

Even if we decide to do something like that, I suppose having
verify-commit isn't the end of the world. It could just remain the
plumbing interface, as verify-tag would. So I don't think my half-formed
thoughts are any reason to block your series.

> That is a general issue with verifying signatures: it can be automated
> only if you employ a strict trust model and a very limited keyring.
> "valid signature" means only as much as the signatures that your gpg
> accepts can be really trusted.
> 
> Comparing uid's really buys you nothing in the sense that everyone can
> have a key with uid "Jeff King <peff@peff.net> signed by some other
> keys.

Sort of. The crypto proves that the commit was signed by a particular
key, and then there are two mappings:

  1. What is the identity associated with that key?

  2. Is that identity somebody who "should" have signed the commit?

We assume that GPG takes care of the first one with the web of trust.
Even if you have the key and can check the signature, it will still
complain about an untrusted uid (and we reflect that with 'U' in the gpg
status). There is no point in looking at step 2 if step 1 did not check
out.

For the second one, I think it really depends on the project workflow,
and you may even want multiple policies within a project (e.g., perhaps
only some uids can merge to master). But one obvious check we can make
is "does the identity in the commit data match one of the uids?". True,
you don't _need_ that; you can always just use "%GS", and throw away the
committer and author headers. But we show those names in lots of output.
You could, for example, "verify" that each commit's author id matches
its gpg signature, and then use the regular tools to do further work
(e.g., running "git blame"), and be confident that the author you see
matches the signature.

This should definitely be optional. Even something as simple as "author
id matches the gpg key" would not always work (for example, in git.git
we pick patches from the list, which means only Junio can sign the
objects, but he is not the author). But just because it is optional does
not mean it would not be a useful tool for some workflows.

> On the other hand, it's perfectly OK to use different uids for git
> commits and signatures. The e-mail address I use for the git list and
> commits, for example, is clearly a "plus address", which helps me
> organize things; my personal key has the primary address as uid.
> 
> I really think all this is up to local policies for individual use cases.

Very much agreed. My suggestion was more about providing tools for
people to build those policies.

I realize this isn't really your itch to scratch. It's just that when I
see a description like "verify a commit", I wonder what exactly "verify"
means.

-Peff

Re: [PATCH 3/3] verify-commit: scriptable commit signature verification

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I realize this isn't really your itch to scratch. It's just that when I
> see a description like "verify a commit", I wonder what exactly "verify"
> means.

I think that is an important point.  If a tool only verifies the
signature of the commit when conceivably other aspect of it could
also be verified but we cannot decide how or we decide we should not
dictate one-way-fits-all, using a generic name "verify-commit" or
"verify" without marking that it is currently only on the signature
clearly somewhere might close the door to the future.

    git verify <object>::
        Verify whatever we currently deem is appropriate for the
        given type of object.

    git verify --gpg-signature::
	Verify the GPG signature for a signed tag, a signed commit,
        or a merge with signed tags.

    git verify --commit-author <committish>::
	Verify the GPG signer matches the "author " header of the
	commit.

and more, perhaps?

Re: [PATCH 3/3] verify-commit: scriptable commit signature verification

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 13.06.2014 19:06:
> Jeff King <peff@peff.net> writes:
> 
>> I realize this isn't really your itch to scratch. It's just that when I
>> see a description like "verify a commit", I wonder what exactly "verify"
>> means.
> 
> I think that is an important point.  If a tool only verifies the
> signature of the commit when conceivably other aspect of it could
> also be verified but we cannot decide how or we decide we should not
> dictate one-way-fits-all, using a generic name "verify-commit" or
> "verify" without marking that it is currently only on the signature
> clearly somewhere might close the door to the future.
> 
>     git verify <object>::
>         Verify whatever we currently deem is appropriate for the
>         given type of object.
> 
>     git verify --gpg-signature::
> 	Verify the GPG signature for a signed tag, a signed commit,
>         or a merge with signed tags.
> 
>     git verify --commit-author <committish>::
> 	Verify the GPG signer matches the "author " header of the
> 	commit.
> 
> and more, perhaps?
> 

So what does that mean? And what does it mean for verify-tag, which does
nothing but checking the return code from gpg, just like the proposed
verify-commit?

As pointed out, strict verification is a matter of policy, very much
like accepting certain ref updates etc. is. Do we want a signature
verification hook?

We currently don't have a scriptable commit signature verification in
the same way we have one for tag signatures. That's the gap that I
wanted to fill in in response to a blog post about commit signatures in
git. But it's not my itch, I don't use signatures.

Michael

Re: [PATCH 3/3] verify-commit: scriptable commit signature verification

[Jeff King]

On Fri, Jun 13, 2014 at 10:06:10AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > I realize this isn't really your itch to scratch. It's just that when I
> > see a description like "verify a commit", I wonder what exactly "verify"
> > means.
> 
> I think that is an important point.  If a tool only verifies the
> signature of the commit when conceivably other aspect of it could
> also be verified but we cannot decide how or we decide we should not
> dictate one-way-fits-all, using a generic name "verify-commit" or
> "verify" without marking that it is currently only on the signature
> clearly somewhere might close the door to the future.
> 
>     git verify <object>::
>         Verify whatever we currently deem is appropriate for the
>         given type of object.
> 
>     git verify --gpg-signature::
> 	Verify the GPG signature for a signed tag, a signed commit,
>         or a merge with signed tags.
> 
>     git verify --commit-author <committish>::
> 	Verify the GPG signer matches the "author " header of the
> 	commit.
> 
> and more, perhaps?

That is certainly the direction I was thinking of when I suggested "git
verify".

However, I do not think it is too bad a thing to add a verify-commit
that matches verify-tag, as long as they do the exact same thing
(namely, check the gpg signature). We may find it is later obsoleted by
"git verify --gpg-signature", but given that verify-tag is already there
and will remain for compatibility, I don't think we are increasing the
cognitive load too much.

Your middle example above did make me think of one other thing, though.
As you noted, we actually have _three_ signature types:

  1. signed tags

  2. signed commits

  3. merges with embedded mergetag headers

We already have a tool for (1). Michael is adding a tool for (2). How
would one check (3) in a similar way?

-Peff

Re: [PATCH 3/3] verify-commit: scriptable commit signature verification

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Jun 13, 2014 at 10:06:10AM -0700, Junio C Hamano wrote:
> ...
>> and more, perhaps?
>
> That is certainly the direction I was thinking of when I suggested "git
> verify".
>
> However, I do not think it is too bad a thing to add a verify-commit
> that matches verify-tag, as long as they do the exact same thing
> (namely, check the gpg signature). We may find it is later obsoleted by
> "git verify --gpg-signature", but given that verify-tag is already there
> and will remain for compatibility, I don't think we are increasing the
> cognitive load too much.

Yup, I think we are exactly on the same page.

Thanks for sanity-checking.

> Your middle example above did make me think of one other thing, though.
> As you noted, we actually have _three_ signature types:
>
>   1. signed tags
>
>   2. signed commits
>
>   3. merges with embedded mergetag headers
>
> We already have a tool for (1). Michael is adding a tool for (2). How
> would one check (3) in a similar way?

Hmph, somehow I misread the patch that it was for both 2 & 3 X-<.

Re: [PATCH 3/3] verify-commit: scriptable commit signature verification

[Jeff King]

On Mon, Jun 16, 2014 at 01:34:20PM -0700, Junio C Hamano wrote:

> > Your middle example above did make me think of one other thing, though.
> > As you noted, we actually have _three_ signature types:
> >
> >   1. signed tags
> >
> >   2. signed commits
> >
> >   3. merges with embedded mergetag headers
> >
> > We already have a tool for (1). Michael is adding a tool for (2). How
> > would one check (3) in a similar way?
> 
> Hmph, somehow I misread the patch that it was for both 2 & 3 X-<.

I was just assuming it handles only (2) without checking further, so I
may be wrong. But I do not think it makes sense to conflate (2) and (3).
A merge commit may have both, and they are separate signatures.

For that matter, is there a way to expose (3) currently, besides via
--show-signature? It does not trigger "%GG" and friends (nor should it).
It may make sense to add extra format specifiers for mergetag
signatures. Though I do not use them myself, so I am not clear on what
the use case is besides a manual, human verification of a particular
merge.

-Peff

Re: [PATCH 3/3] verify-commit: scriptable commit signature verification

[Michael J Gruber]

Jeff King venit, vidit, dixit 16.06.2014 22:39:
> On Mon, Jun 16, 2014 at 01:34:20PM -0700, Junio C Hamano wrote:
> 
>>> Your middle example above did make me think of one other thing, though.
>>> As you noted, we actually have _three_ signature types:
>>>
>>>   1. signed tags
>>>
>>>   2. signed commits
>>>
>>>   3. merges with embedded mergetag headers
>>>
>>> We already have a tool for (1). Michael is adding a tool for (2). How
>>> would one check (3) in a similar way?
>>
>> Hmph, somehow I misread the patch that it was for both 2 & 3 X-<.
> 
> I was just assuming it handles only (2) without checking further, so I
> may be wrong. But I do not think it makes sense to conflate (2) and (3).
> A merge commit may have both, and they are separate signatures.
> 
> For that matter, is there a way to expose (3) currently, besides via
> --show-signature? It does not trigger "%GG" and friends (nor should it).
> It may make sense to add extra format specifiers for mergetag
> signatures. Though I do not use them myself, so I am not clear on what
> the use case is besides a manual, human verification of a particular
> merge.

I'm afraid I'm on a weekly git schedule at best, sorry. Just trying to
catch up on this:

Admittedly, I simply don't know about "3.". I know only 1. and 2. (and
don't remember why they are implemented differently).

Are they documented/decribed somewhere?

Meanwhile, I'm rebasing on top of the %G related patches by Junio and
Jeff and hope to send out a v4 later today.

Michael

Re: [PATCH 3/3] verify-commit: scriptable commit signature verification

[Michael J Gruber]

Michael J Gruber venit, vidit, dixit 27.06.2014 14:31:
> Jeff King venit, vidit, dixit 16.06.2014 22:39:
>> On Mon, Jun 16, 2014 at 01:34:20PM -0700, Junio C Hamano wrote:
>>
>>>> Your middle example above did make me think of one other thing, though.
>>>> As you noted, we actually have _three_ signature types:
>>>>
>>>>   1. signed tags
>>>>
>>>>   2. signed commits
>>>>
>>>>   3. merges with embedded mergetag headers
>>>>
>>>> We already have a tool for (1). Michael is adding a tool for (2). How
>>>> would one check (3) in a similar way?
>>>
>>> Hmph, somehow I misread the patch that it was for both 2 & 3 X-<.
>>
>> I was just assuming it handles only (2) without checking further, so I
>> may be wrong. But I do not think it makes sense to conflate (2) and (3).
>> A merge commit may have both, and they are separate signatures.
>>
>> For that matter, is there a way to expose (3) currently, besides via
>> --show-signature? It does not trigger "%GG" and friends (nor should it).
>> It may make sense to add extra format specifiers for mergetag
>> signatures. Though I do not use them myself, so I am not clear on what
>> the use case is besides a manual, human verification of a particular
>> merge.
> 
> I'm afraid I'm on a weekly git schedule at best, sorry. Just trying to
> catch up on this:
> 
> Admittedly, I simply don't know about "3.". I know only 1. and 2. (and
> don't remember why they are implemented differently).
> 
> Are they documented/decribed somewhere?
> 
> Meanwhile, I'm rebasing on top of the %G related patches by Junio and
> Jeff and hope to send out a v4 later today.
> 
> Michael

OK, found the two commits which "git log -Smergetag" outputs, but no tests.

A merge commit with embedded signed tag it is, then.

The commit could carry it's own commit signature, couldn't it?
That would suggest that we use "git verify-tag" to verify the embedded
signed tag of a merge commit and "git verify-commit" to verify the
commit signature.

OTOH I would like these basic commands to be as strict as possible,
including type-checks. Does that mean having "git verify-mergetag" which
verifies that it is being used on a merge commit with embedded mergetag?

(BTW: Is there anything keeping a non-merge commit from having an
embedded (merge) tag?)

Michael

Re: [PATCH 3/3] verify-commit: scriptable commit signature verification

[Michael J Gruber]

Michael J Gruber venit, vidit, dixit 27.06.2014 14:49:
> Michael J Gruber venit, vidit, dixit 27.06.2014 14:31:
>> Jeff King venit, vidit, dixit 16.06.2014 22:39:
>>> On Mon, Jun 16, 2014 at 01:34:20PM -0700, Junio C Hamano wrote:
>>>
>>>>> Your middle example above did make me think of one other thing, though.
>>>>> As you noted, we actually have _three_ signature types:
>>>>>
>>>>>   1. signed tags
>>>>>
>>>>>   2. signed commits
>>>>>
>>>>>   3. merges with embedded mergetag headers
>>>>>
>>>>> We already have a tool for (1). Michael is adding a tool for (2). How
>>>>> would one check (3) in a similar way?
>>>>
>>>> Hmph, somehow I misread the patch that it was for both 2 & 3 X-<.
>>>
>>> I was just assuming it handles only (2) without checking further, so I
>>> may be wrong. But I do not think it makes sense to conflate (2) and (3).
>>> A merge commit may have both, and they are separate signatures.
>>>
>>> For that matter, is there a way to expose (3) currently, besides via
>>> --show-signature? It does not trigger "%GG" and friends (nor should it).
>>> It may make sense to add extra format specifiers for mergetag
>>> signatures. Though I do not use them myself, so I am not clear on what
>>> the use case is besides a manual, human verification of a particular
>>> merge.
>>
>> I'm afraid I'm on a weekly git schedule at best, sorry. Just trying to
>> catch up on this:
>>
>> Admittedly, I simply don't know about "3.". I know only 1. and 2. (and
>> don't remember why they are implemented differently).
>>
>> Are they documented/decribed somewhere?
>>
>> Meanwhile, I'm rebasing on top of the %G related patches by Junio and
>> Jeff and hope to send out a v4 later today.
>>
>> Michael
> 
> OK, found the two commits which "git log -Smergetag" outputs, but no tests.
> 
> A merge commit with embedded signed tag it is, then.
> 
> The commit could carry it's own commit signature, couldn't it?
> That would suggest that we use "git verify-tag" to verify the embedded
> signed tag of a merge commit and "git verify-commit" to verify the
> commit signature.
> 
> OTOH I would like these basic commands to be as strict as possible,
> including type-checks. Does that mean having "git verify-mergetag" which
> verifies that it is being used on a merge commit with embedded mergetag?
> 
> (BTW: Is there anything keeping a non-merge commit from having an
> embedded (merge) tag?)
> 
> Michael

Another observation:

git merge -S branch
#fix conflict
git commit # "Merge --continue"

forgets that the merge was supposed to be signed. One needs to "commit
-S" to sign the merge.

Another one:
The color for merge tags in "log --color" is terrible (colored
background)...

Michael

[PATCH] log: correctly identify mergetag signature verification status

[Michael J Gruber]

A wrong '}' made our code record the results of mergetag signature
verification incorrectly.

Fix it.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
This is the "color fix"...

 log-tree.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/log-tree.c b/log-tree.c
index 10e6844..fdea358 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -453,9 +453,9 @@ static void show_one_mergetag(struct rev_info *opt,
 					 &verify_message, NULL)) {
 			if (verify_message.len <= gpg_message_offset)
 				strbuf_addstr(&verify_message, "No signature\n");
-			else
-				status = 0;
 		}
+		else
+			status = 0;
 
 	show_sig_lines(opt, status, verify_message.buf);
 	strbuf_release(&verify_message);
-- 
2.0.1.563.g162087b.dirty

Re: [PATCH] log: correctly identify mergetag signature verification status

[Jeff King]

On Fri, Jun 27, 2014 at 03:18:36PM +0200, Michael J Gruber wrote:

> A wrong '}' made our code record the results of mergetag signature
> verification incorrectly.
> 
> Fix it.

I think this is the right thing to do, but we went from:

  if (...)
	if (...) {
		if (...)
			...
		else
			...
	}

to:

  if (...)
	if (...) {
		if (...)
			...
	}
	else
		...

I think part of the point of the original {} was to make it more clear
which else went with which if. Perhaps we should use more here.

I also find the logic a bit hard to follow in that the outer conditions are:

  if (needed_for_goodsig)
	if (sig_is_not_good)
		...

Perhaps it would be easier to read (and would have made the logic error
you are fixing more obvious) as:

  if (extra->len > payload_size) {
	if (!verify_signed_buffer(...))
		status = 0; /* good; all other code paths leave it as -1 */
	else if (verify_message.len <= gpg_message_offset)
		strbuf_addstr(&verify_message, "No signature\n");
  }

That is, for each conditional to check one more thing needed for a good
signature, and then know that all other code paths leave status as -1.

-Peff

Re: [PATCH] log: correctly identify mergetag signature verification status

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Perhaps it would be easier to read (and would have made the logic error
> you are fixing more obvious) as:
>
>   if (extra->len > payload_size) {
> 	if (!verify_signed_buffer(...))
> 		status = 0; /* good; all other code paths leave it as -1 */
> 	else if (verify_message.len <= gpg_message_offset)
> 		strbuf_addstr(&verify_message, "No signature\n");
>   }
>
> That is, for each conditional to check one more thing needed for a good
> signature, and then know that all other code paths leave status as -1.

Thanks.  Let's do it this way, then.

 log-tree.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/log-tree.c b/log-tree.c
index 1982631..b4bbfe1 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -446,16 +446,17 @@ static void show_one_mergetag(struct rev_info *opt,
 
 	payload_size = parse_signature(extra->value, extra->len);
 	status = -1;
-	if (extra->len > payload_size)
-		if (verify_signed_buffer(extra->value, payload_size,
-					 extra->value + payload_size,
-					 extra->len - payload_size,
-					 &verify_message, NULL)) {
-			if (verify_message.len <= gpg_message_offset)
-				strbuf_addstr(&verify_message, "No signature\n");
-			else
-				status = 0;
-		}
+	if (extra->len > payload_size) {
+		/* could have a good signature */
+		if (!verify_signed_buffer(extra->value, payload_size,
+					  extra->value + payload_size,
+					  extra->len - payload_size,
+					  &verify_message, NULL))
+			status = 0; /* good */
+		else if (verify_message.len <= gpg_message_offset)
+			strbuf_addstr(&verify_message, "No signature\n");
+		/* otherwise we couldn't verify, which is shown as bad */
+	}
 
 	show_sig_lines(opt, status, verify_message.buf);
 	strbuf_release(&verify_message);

Re: [PATCH 3/3] verify-commit: scriptable commit signature verification

[Michael J Gruber]

Michael J Gruber venit, vidit, dixit 27.06.2014 14:49:
> Michael J Gruber venit, vidit, dixit 27.06.2014 14:31:
>> Jeff King venit, vidit, dixit 16.06.2014 22:39:
>>> On Mon, Jun 16, 2014 at 01:34:20PM -0700, Junio C Hamano wrote:
>>>
>>>>> Your middle example above did make me think of one other thing, though.
>>>>> As you noted, we actually have _three_ signature types:
>>>>>
>>>>>   1. signed tags
>>>>>
>>>>>   2. signed commits
>>>>>
>>>>>   3. merges with embedded mergetag headers
>>>>>
>>>>> We already have a tool for (1). Michael is adding a tool for (2). How
>>>>> would one check (3) in a similar way?
>>>>
>>>> Hmph, somehow I misread the patch that it was for both 2 & 3 X-<.
>>>
>>> I was just assuming it handles only (2) without checking further, so I
>>> may be wrong. But I do not think it makes sense to conflate (2) and (3).
>>> A merge commit may have both, and they are separate signatures.
>>>
>>> For that matter, is there a way to expose (3) currently, besides via
>>> --show-signature? It does not trigger "%GG" and friends (nor should it).
>>> It may make sense to add extra format specifiers for mergetag
>>> signatures. Though I do not use them myself, so I am not clear on what
>>> the use case is besides a manual, human verification of a particular
>>> merge.
>>
>> I'm afraid I'm on a weekly git schedule at best, sorry. Just trying to
>> catch up on this:
>>
>> Admittedly, I simply don't know about "3.". I know only 1. and 2. (and
>> don't remember why they are implemented differently).
>>
>> Are they documented/decribed somewhere?
>>
>> Meanwhile, I'm rebasing on top of the %G related patches by Junio and
>> Jeff and hope to send out a v4 later today.
>>
>> Michael
> 
> OK, found the two commits which "git log -Smergetag" outputs, but no tests.
> 
> A merge commit with embedded signed tag it is, then.
> 
> The commit could carry it's own commit signature, couldn't it?
> That would suggest that we use "git verify-tag" to verify the embedded
> signed tag of a merge commit and "git verify-commit" to verify the
> commit signature.
> 
> OTOH I would like these basic commands to be as strict as possible,
> including type-checks. Does that mean having "git verify-mergetag" which
> verifies that it is being used on a merge commit with embedded mergetag?

... or an extension <ref>^{mergetag} to our machinery, defaulting to the
tag object containing the mergetag for the 2nd parent, with an optional
version <ref>^{mergetag}<n>?

OTOH, verifying a mergetag involves both looking at the signed tag and
the parent list of the commit. We probably should require signed (merge)
tags on all but the first parent. Oh Can of Worms, oh Can of Worms ("Oh
Can'o'Worms" to the tune of "Oh Canada").

Michael

Re: [PATCH 3/3] verify-commit: scriptable commit signature verification

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

> ... or an extension <ref>^{mergetag} to our machinery, defaulting to the
> tag object containing the mergetag for the 2nd parent, with an optional
> version <ref>^{mergetag}<n>?

One thing you should not forget is that with mergetag, the original
tag object is not even necessary to exist in the repository, and
often the original tag will not be propagated to the general public.

You _could_ extract the necessary information from the commit that
merges a signed tag to recreate the tag, but for what purpose?  Your
"$commit^{mergetag}<n>" needs to recreate the named tag object but
it is unclear to me how you envision it to be used.

Re: [PATCH 3/3] verify-commit: scriptable commit signature verification

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

> A merge commit with embedded signed tag it is, then.
>
> The commit could carry it's own commit signature, couldn't it?

Yes, an integrator can choose to sign a merge he creates, merging
the work by a contributor who gave him a pull-request for a tag
signed by the contributor.  The resulting commit will embed the
contributor's signature to let historians verify the second parent,
as well as the integrator's signature to allow verification of the
merge result.  The integrator does not need to keep the signed tag
used as an implementation detail of transferring the signature of
the contributor, and in general such a signed tag used only to
request pulls is not available to the general public and historians
after such a merge is created.

As these signatures are part of a commit object, "git verify-commit"
would be the logical place to validate them, if we were to do so.

Re: [PATCH 3/3] verify-commit: scriptable commit signature verification

[Jeff King]

On Fri, Jun 27, 2014 at 11:36:47AM -0700, Junio C Hamano wrote:

> Michael J Gruber <git@drmicha.warpmail.net> writes:
> 
> > A merge commit with embedded signed tag it is, then.
> >
> > The commit could carry it's own commit signature, couldn't it?
> 
> Yes, an integrator can choose to sign a merge he creates, merging
> the work by a contributor who gave him a pull-request for a tag
> signed by the contributor.  The resulting commit will embed the
> contributor's signature to let historians verify the second parent,
> as well as the integrator's signature to allow verification of the
> merge result.  The integrator does not need to keep the signed tag
> used as an implementation detail of transferring the signature of
> the contributor, and in general such a signed tag used only to
> request pulls is not available to the general public and historians
> after such a merge is created.
> 
> As these signatures are part of a commit object, "git verify-commit"
> would be the logical place to validate them, if we were to do so.

We already disagreed on this earlier in the discussion, but I have given
it some more thought, and somehow using "verify-commit" for mergetags
still feels wrong to me. Let me see if I can put it into words.

First off, I agree that "verify-tag" is probably not the right place.
There _is_ no tag object to verify anymore (the only reason it is a tag
at all is that the signature came out of what once was a tag).

Let us imagine that we have a "verify-commit" that verifies commit
signatures made by "commit -S". If I run "verify-commit foo", that
implies to me two things:

  1. I am verifying the signature over the contents of "foo".

     But the mergetag is _not_ a statement about "foo"; the person who
     signed it never saw "foo". It is a statement about foo^2 at best,
     or possibly about the whole history of foo^1..foo^2.

  2. I am verifying _only_ the contents of foo. That is, I expect people
     to use "commit -S" to cryptographically claim authorship of a
     commit. Whereas with tags, I think we have typically taken them to
     mean "I have signed the tip of a segment of history, and I am
     taking responsibility for the final state" (e.g., signing release
     tags).

     I realize that this claim is somewhat tenuous. It's not something
     inherent in the crypto, but rather in the social convention of what
     it means to sign a commit. One could easily say "signing a commit
     is about signing the whole tree state". But I would ask, then: what
     is the point of signing a commit versus signing a tag?  I think
     that people who wanted commit signatures wanted them to give a
     stronger guarantee of authorship of individual commits.

     Git has largely stayed agnostic about what such signatures mean.
     But if we accept that some projects _are_ going to make that
     distinction, I think conflating verification of the two within the
     same command leads to a potential for confusion.

So for that reason, I think I'd be in favor of simply treating mergetag
signatures as a separate, third entity. Give them their own
%G-specifiers, and give them a separate plumbing command. Let projects
work out how they want to use them, but do not create any particular
affiliation between mergetags and commit signatures (nor between tag
signatures and mergetag signatures).

-Peff

Re: [PATCH 3/3] verify-commit: scriptable commit signature verification

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> First off, I agree that "verify-tag" is probably not the right place.
> There _is_ no tag object to verify anymore (the only reason it is a tag
> at all is that the signature came out of what once was a tag).

Yes, if we imagine that the header were called "mergesig", it may be
more apparent what it is.  Similar to the commit signature itself
(which gives GPG protection over the commit itself), a "mergesig"
covers one of the parent commit object name recorded in the commit.

You could look at it as one aspect of the child commit object being
verified, and that is why I would suggest it is the responsibility
of verify-commit that is run on the child commit (I saw suggestions
to verify other aspects such as "author ident matches the GPG signer
of the signature on the commit itself", etc.)

> Let us imagine that we have a "verify-commit" that verifies commit
> signatures made by "commit -S". If I run "verify-commit foo", that
> implies to me two things:
>
>   1. I am verifying the signature over the contents of "foo".
>
>      But the mergetag is _not_ a statement about "foo"; the person who
>      signed it never saw "foo". It is a statement about foo^2 at best,
>      or possibly about the whole history of foo^1..foo^2.
>
>   2. I am verifying _only_ the contents of foo. That is, I expect people
>      to use "commit -S" to cryptographically claim authorship of a
>      commit. Whereas with tags, I think we have typically taken them to
>      mean "I have signed the tip of a segment of history, and I am
>      taking responsibility for the final state" (e.g., signing release
>      tags).

I think you are making it too hard ;-).  Fundamentally, a signature
on a commit object itself _can_ be used to attest _some_ property
about the whole history behind it by the signer, but it does not
prevent a project from adopting a lot looser convention.  If a
project's stance is "commit signature in this project is about the
contents of the tree" (i.e. the signer does not have much confidence
in his memory on how he got there), the result from verify-commit on
the signature of the commit itself should be interpreted in the
context of such a project as such---the signer is confident that the
tree matches what he signed and the signature wouldn't mean any more
than that.

>      I realize that this claim is somewhat tenuous. It's not something
>      inherent in the crypto, but rather in the social convention of what
>      it means to sign a commit.

Yup, I think we are on the same page.

>      But I would ask, then: what
>      is the point of signing a commit versus signing a tag?  I think
>      that people who wanted commit signatures wanted them to give a
>      stronger guarantee of authorship of individual commits.

There are practical concerns.

 - tags are easier to manage because you can make a commit, give it
   out for trials by guinea pigs and then after it proves good, you
   can add a signed tag after the fact without rewriting the commit
   itself.  Incidentally, this is the primary reason behind the
   design of mergetags that were introduced after the k.org break-in
   fiasco.

   Theoretically you could have argued that lieutenants can sign the
   commits they want Linus to pull and we wouldn't have had to add
   anything to the system to support the new "not only we can use
   third-party publishing sites that we do not know how much we can
   trust, such as GitHub, we do not even have to trust k.org blindly"
   workflow.  But they really wanted to be able to commit first,
   let time pass and then sign to request pulling.

 - tags are a lot more cumbersome if you want to sign each and every
   commit (which some of us view as pointless, given the chain of
   SHA-1 hashes our history storage is based on), because you end up
   having to name, keep and transfer O(n) refs to represent them for
   the history of project of size n.  Embedding signature in each
   commit would be the only workable way if somebody wants to sign
   each and every commit.

>      Git has largely stayed agnostic about what such signatures mean.
>      But if we accept that some projects _are_ going to make that
>      distinction, I think conflating verification of the two within the
>      same command leads to a potential for confusion.

I agree that the users and scripts must be able to inspect the
validity of "gpgsig" and "mergetag"(s) separately.  When you inspect
a merge commit, if gpgsig verifies but mergetag does not, we know
that the integrator created and signed the merge but we do not know
if the lieutenant whose name appears on the mergetag really asked
the side branch to be pulled, or if the merge was made in response
to a request by an imposter.  For that matter, when verifying an
octopus merge, we must be able to see the validity of "mergetag" for
each parent separately.  We can do that by having two separate
commands (one to verify "gpgsig", the other to verify "mergetag"),
or we can do that with a separate command line options to a single
command ("verify-commit --gpgsig" and "verify-commit --mergsig <n>")
or a single "verify-commit" may report multi-values to its output
for scripts to parse.  I do not have a strong opinion.

[PATCHv2 0/6] verify-commit: verify commit signatures

[Michael J Gruber]

Patch1: unchanged
Patch2: unchanged
Patch3: as fixed in Junio's tree already (===)
Patch4: minor test fix for t7510
Patch5: tests for the verify-commit
Patch6: make all struct signature_check users employ the same clear helper

Michael J Gruber (6):
  pretty: free the gpg status buf
  gpg-interface: provide access to the payload
  verify-commit: scriptable commit signature verification
  t7510: exit for loop with test result
  t7510: test verify-commit
  gpg-interface: provide clear helper for struct signature_check

 Documentation/git-verify-commit.txt | 28 +++++++++++
 Makefile                            |  1 +
 builtin.h                           |  1 +
 builtin/merge.c                     |  5 +-
 builtin/verify-commit.c             | 95 +++++++++++++++++++++++++++++++++++++
 command-list.txt                    |  1 +
 commit.c                            |  1 +
 git.c                               |  1 +
 gpg-interface.c                     | 14 ++++++
 gpg-interface.h                     |  2 +
 pretty.c                            |  3 +-
 t/t7510-signed-commit.sh            | 24 ++++++++--
 12 files changed, 167 insertions(+), 9 deletions(-)
 create mode 100644 Documentation/git-verify-commit.txt
 create mode 100644 builtin/verify-commit.c

-- 
2.0.0.426.g37dbf84

[PATCHv2 1/6] pretty: free the gpg status buf

[Michael J Gruber]

4a868fd (pretty: parse the gpg status lines rather than the output, 2013-02-14)
made the gpg status lines available to callers and made sure they freed
the used space, but missed one spot.

Free the status line buffer also in the remaining spot.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 pretty.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/pretty.c b/pretty.c
index 4f51287..f1e8a70 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1538,6 +1538,7 @@ void format_commit_message(const struct commit *commit,
 	free(context.commit_encoding);
 	logmsg_free(context.message, commit);
 	free(context.signature_check.gpg_output);
+	free(context.signature_check.gpg_status);
 	free(context.signature_check.signer);
 }
 
-- 
2.0.0.426.g37dbf84

Re: [PATCHv2 1/6] pretty: free the gpg status buf

[Jeff King]

On Fri, Jun 13, 2014 at 12:42:43PM +0200, Michael J Gruber wrote:

> 4a868fd (pretty: parse the gpg status lines rather than the output, 2013-02-14)
> made the gpg status lines available to callers and made sure they freed
> the used space, but missed one spot.
> 
> Free the status line buffer also in the remaining spot.
> 
> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
> ---
>  pretty.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/pretty.c b/pretty.c
> index 4f51287..f1e8a70 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -1538,6 +1538,7 @@ void format_commit_message(const struct commit *commit,
>  	free(context.commit_encoding);
>  	logmsg_free(context.message, commit);
>  	free(context.signature_check.gpg_output);
> +	free(context.signature_check.gpg_status);
>  	free(context.signature_check.signer);

What about .key?

I would have expected your patch 6 to come first, which would fix this,
and then save you from making similar mistakes in patch 3. :)

-Peff

[PATCHv2 2/6] gpg-interface: provide access to the payload

[Michael J Gruber]

In contrast to tag signatures, commit signatures are put into the
header, that is between the other header parts and commit messages.

Provide access to the commit content sans the signature, which is the
payload that is actually signed. Commit signature verification does the
parsing anyways, and callers may wish to act on or display the commit
object sans the signature.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 builtin/merge.c | 1 +
 commit.c        | 1 +
 gpg-interface.h | 1 +
 pretty.c        | 1 +
 4 files changed, 4 insertions(+)

diff --git a/builtin/merge.c b/builtin/merge.c
index 428ca24..6a9812a 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1282,6 +1282,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 				printf(_("Commit %s has a good GPG signature by %s\n"),
 				       hex, signature_check.signer);
 
+			free(signature_check.payload);
 			free(signature_check.gpg_output);
 			free(signature_check.gpg_status);
 			free(signature_check.signer);
diff --git a/commit.c b/commit.c
index 881be3b..d0ad7f0 100644
--- a/commit.c
+++ b/commit.c
@@ -1219,6 +1219,7 @@ void check_commit_signature(const struct commit* commit, struct signature_check
 				      &gpg_output, &gpg_status);
 	if (status && !gpg_output.len)
 		goto out;
+	sigc->payload = strbuf_detach(&payload, NULL);
 	sigc->gpg_output = strbuf_detach(&gpg_output, NULL);
 	sigc->gpg_status = strbuf_detach(&gpg_status, NULL);
 	parse_gpg_output(sigc);
diff --git a/gpg-interface.h b/gpg-interface.h
index a85cb5b..d727c25 100644
--- a/gpg-interface.h
+++ b/gpg-interface.h
@@ -2,6 +2,7 @@
 #define GPG_INTERFACE_H
 
 struct signature_check {
+	char *payload;
 	char *gpg_output;
 	char *gpg_status;
 	char result; /* 0 (not checked),
diff --git a/pretty.c b/pretty.c
index f1e8a70..24fb877 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1537,6 +1537,7 @@ void format_commit_message(const struct commit *commit,
 
 	free(context.commit_encoding);
 	logmsg_free(context.message, commit);
+	free(context.signature_check.payload);
 	free(context.signature_check.gpg_output);
 	free(context.signature_check.gpg_status);
 	free(context.signature_check.signer);
-- 
2.0.0.426.g37dbf84

[PATCHv2 3/6] verify-commit: scriptable commit signature verification

[Michael J Gruber]

Commit signatures can be verified using "git show -s --show-signature"
or the "%G?" pretty format and parsing the output, which is well suited
for user inspection, but not for scripting.

Provide a command "verify-commit" which is analogous to "verify-tag": It
returns 0 for good signatures and non-zero otherwise, has the gpg output
on stderr and (optionally) the commit object on stdout, sans the
signature, just like "verify-tag" does.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 Documentation/git-verify-commit.txt | 28 +++++++++++
 Makefile                            |  1 +
 builtin.h                           |  1 +
 builtin/verify-commit.c             | 98 +++++++++++++++++++++++++++++++++++++
 command-list.txt                    |  1 +
 git.c                               |  1 +
 6 files changed, 130 insertions(+)
 create mode 100644 Documentation/git-verify-commit.txt
 create mode 100644 builtin/verify-commit.c

diff --git a/Documentation/git-verify-commit.txt b/Documentation/git-verify-commit.txt
new file mode 100644
index 0000000..9413e28
--- /dev/null
+++ b/Documentation/git-verify-commit.txt
@@ -0,0 +1,28 @@
+git-verify-commit(1)
+====================
+
+NAME
+----
+git-verify-commit - Check the GPG signature of commits
+
+SYNOPSIS
+--------
+[verse]
+'git verify-commit' <commit>...
+
+DESCRIPTION
+-----------
+Validates the gpg signature created by 'git commit -S'.
+
+OPTIONS
+-------
+-v::
+--verbose::
+	Print the contents of the commit object before validating it.
+
+<commit>...::
+	SHA-1 identifiers of Git commit objects.
+
+GIT
+---
+Part of the linkgit:git[1] suite
diff --git a/Makefile b/Makefile
index 07ea105..b92418d 100644
--- a/Makefile
+++ b/Makefile
@@ -999,6 +999,7 @@ BUILTIN_OBJS += builtin/update-ref.o
 BUILTIN_OBJS += builtin/update-server-info.o
 BUILTIN_OBJS += builtin/upload-archive.o
 BUILTIN_OBJS += builtin/var.o
+BUILTIN_OBJS += builtin/verify-commit.o
 BUILTIN_OBJS += builtin/verify-pack.o
 BUILTIN_OBJS += builtin/verify-tag.o
 BUILTIN_OBJS += builtin/write-tree.o
diff --git a/builtin.h b/builtin.h
index c47c110..5d91f31 100644
--- a/builtin.h
+++ b/builtin.h
@@ -128,6 +128,7 @@ extern int cmd_update_server_info(int argc, const char **argv, const char *prefi
 extern int cmd_upload_archive(int argc, const char **argv, const char *prefix);
 extern int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix);
 extern int cmd_var(int argc, const char **argv, const char *prefix);
+extern int cmd_verify_commit(int argc, const char **argv, const char *prefix);
 extern int cmd_verify_tag(int argc, const char **argv, const char *prefix);
 extern int cmd_version(int argc, const char **argv, const char *prefix);
 extern int cmd_whatchanged(int argc, const char **argv, const char *prefix);
diff --git a/builtin/verify-commit.c b/builtin/verify-commit.c
new file mode 100644
index 0000000..69b7c6d
--- /dev/null
+++ b/builtin/verify-commit.c
@@ -0,0 +1,98 @@
+/*
+ * Builtin "git commit-commit"
+ *
+ * Copyright (c) 2014 Michael J Gruber <git@drmicha.warpmail.net>
+ *
+ * Based on git-verify-tag
+ */
+#include "cache.h"
+#include "builtin.h"
+#include "commit.h"
+#include "run-command.h"
+#include <signal.h>
+#include "parse-options.h"
+#include "gpg-interface.h"
+
+static const char * const verify_commit_usage[] = {
+		N_("git verify-commit [-v|--verbose] <commit>..."),
+		NULL
+};
+
+static int run_gpg_verify(const unsigned char *sha1, const char *buf, unsigned long size, int verbose)
+{
+	struct signature_check signature_check;
+
+	memset(&signature_check, 0, sizeof(signature_check));
+
+	check_commit_signature(lookup_commit(sha1), &signature_check);
+
+	if (verbose && signature_check.payload)
+		fputs(signature_check.payload, stdout);
+
+	if (signature_check.gpg_output)
+		fputs(signature_check.gpg_output, stderr);
+
+	free(signature_check.gpg_output);
+	free(signature_check.gpg_status);
+	free(signature_check.signer);
+	free(signature_check.key);
+	return signature_check.result != 'G';
+}
+
+static int verify_commit(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))
+		return error("commit '%s' not found.", name);
+
+	type = sha1_object_info(sha1, NULL);
+	if (type != OBJ_COMMIT)
+		return error("%s: cannot verify a non-commit 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(sha1, buf, size, verbose);
+
+	free(buf);
+	return ret;
+}
+
+static int git_verify_commit_config(const char *var, const char *value, void *cb)
+{
+	int status = git_gpg_config(var, value, cb);
+	if (status)
+		return status;
+	return git_default_config(var, value, cb);
+}
+
+int cmd_verify_commit(int argc, const char **argv, const char *prefix)
+{
+	int i = 1, verbose = 0, had_error = 0;
+	const struct option verify_commit_options[] = {
+		OPT__VERBOSE(&verbose, N_("print commit contents")),
+		OPT_END()
+	};
+
+	git_config(git_verify_commit_config, NULL);
+
+	argc = parse_options(argc, argv, prefix, verify_commit_options,
+			     verify_commit_usage, PARSE_OPT_KEEP_ARGV0);
+	if (argc <= i)
+		usage_with_options(verify_commit_usage, verify_commit_options);
+
+	/* 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_commit(argv[i++], verbose))
+			had_error = 1;
+	return had_error;
+}
diff --git a/command-list.txt b/command-list.txt
index cf36c3d..a3ff0c9 100644
--- a/command-list.txt
+++ b/command-list.txt
@@ -132,6 +132,7 @@ git-update-server-info                  synchingrepositories
 git-upload-archive                      synchelpers
 git-upload-pack                         synchelpers
 git-var                                 plumbinginterrogators
+git-verify-commit                       ancillaryinterrogators
 git-verify-pack                         plumbinginterrogators
 git-verify-tag                          ancillaryinterrogators
 gitweb                                  ancillaryinterrogators
diff --git a/git.c b/git.c
index 7780572..bbc3293 100644
--- a/git.c
+++ b/git.c
@@ -441,6 +441,7 @@ static struct cmd_struct commands[] = {
 	{ "upload-archive", cmd_upload_archive },
 	{ "upload-archive--writer", cmd_upload_archive_writer },
 	{ "var", cmd_var, RUN_SETUP_GENTLY },
+	{ "verify-commit", cmd_verify_commit, RUN_SETUP },
 	{ "verify-pack", cmd_verify_pack },
 	{ "verify-tag", cmd_verify_tag, RUN_SETUP },
 	{ "version", cmd_version },
-- 
2.0.0.426.g37dbf84

Re: [PATCHv2 3/6] verify-commit: scriptable commit signature verification

[Jeff King]

On Fri, Jun 13, 2014 at 12:42:45PM +0200, Michael J Gruber wrote:

> +
> +	free(signature_check.gpg_output);
> +	free(signature_check.gpg_status);
> +	free(signature_check.signer);
> +	free(signature_check.key);
> +	return signature_check.result != 'G';
> +}

How about .payload here?

> +	type = sha1_object_info(sha1, NULL);
> +	if (type != OBJ_COMMIT)
> +		return error("%s: cannot verify a non-commit object of type %s.",
> +				name, typename(type));
> +
> +	buf = read_sha1_file(sha1, &type, &size);
> +	if (!buf)
> +		return error("%s: unable to read file.", name);

I think you can drop the sha1_object_info call and just check "type"
from read_sha1_file (they _should_ agree, but if they do not, I'd rather
pay attention to the one that came along with the buffer). And this is
the uncommon error path, so expanding the object into memory before we
die is not a big deal.

Should this peel to a commit if given a tag? I'd say probably. I know
you raised the issue elsewhere of keeping things simple, but I think if
you are calling verify-commit, you know you want a commit, and we should
treat the argument as a commit-ish. Anyway, if you go that route, then
lookup_commit_or_die is probably what you want.

Also, minor nit, but we typically do not end the error messages with a
full stop (we've been rather inconsistent in the past, but these days
seem to mostly settle on no punctuation).

-Peff

Re: [PATCHv2 3/6] verify-commit: scriptable commit signature verification

[Michael J Gruber]

Jeff King venit, vidit, dixit 13.06.2014 13:19:
> On Fri, Jun 13, 2014 at 12:42:45PM +0200, Michael J Gruber wrote:
> 
>> +
>> +	free(signature_check.gpg_output);
>> +	free(signature_check.gpg_status);
>> +	free(signature_check.signer);
>> +	free(signature_check.key);
>> +	return signature_check.result != 'G';
>> +}
> 
> How about .payload here?

I sneekily fix this in 6/6... I thought 3/6 is on next already, too late
for a real v2. Otherwise I would put 6/6 before everything else.

>> +	type = sha1_object_info(sha1, NULL);
>> +	if (type != OBJ_COMMIT)
>> +		return error("%s: cannot verify a non-commit object of type %s.",
>> +				name, typename(type));
>> +
>> +	buf = read_sha1_file(sha1, &type, &size);
>> +	if (!buf)
>> +		return error("%s: unable to read file.", name);
> 
> I think you can drop the sha1_object_info call and just check "type"
> from read_sha1_file (they _should_ agree, but if they do not, I'd rather
> pay attention to the one that came along with the buffer). And this is
> the uncommon error path, so expanding the object into memory before we
> die is not a big deal.
> 
> Should this peel to a commit if given a tag? I'd say probably. I know
> you raised the issue elsewhere of keeping things simple, but I think if
> you are calling verify-commit, you know you want a commit, and we should
> treat the argument as a commit-ish. Anyway, if you go that route, then
> lookup_commit_or_die is probably what you want.
> 
> Also, minor nit, but we typically do not end the error messages with a
> full stop (we've been rather inconsistent in the past, but these days
> seem to mostly settle on no punctuation).
> 
> -Peff

Both of these issues actually come for following verify-tag as closely
as possible. If 3 is not applied already, I should do away with
sha1_object_info.

About the peeling I'm not so sure, since there's a difference between a
signed tag pointing to a commit and a signed commit. Since
verify-{tag,commit} are bare metal plumbing, I would expect callers to
use <rev>^{commit} explicitly if they don't care how <rev> peels to a
commit.

Michael

Re: [PATCHv2 3/6] verify-commit: scriptable commit signature verification

[Jeff King]

On Fri, Jun 13, 2014 at 01:45:58PM +0200, Michael J Gruber wrote:

> I sneekily fix this in 6/6... I thought 3/6 is on next already, too late
> for a real v2. Otherwise I would put 6/6 before everything else.

Ah, yeah, I assumed we were still re-rolling (and it looks like you're
just on pu so far).

> About the peeling I'm not so sure, since there's a difference between a
> signed tag pointing to a commit and a signed commit.

There is, but "verify-commit" is always going to verify the commit, no?
Not peeling will always result in an error, and never do anything
useful.

I admit it's probably not going to come up too often, though. And I
don't have any argument beyond "it makes sense to me", so I won't push
for it further.

-Peff

Re: [PATCHv2 3/6] verify-commit: scriptable commit signature verification

[Michael J Gruber]

Jeff King venit, vidit, dixit 13.06.2014 13:50:
> On Fri, Jun 13, 2014 at 01:45:58PM +0200, Michael J Gruber wrote:
> 
>> I sneekily fix this in 6/6... I thought 3/6 is on next already, too late
>> for a real v2. Otherwise I would put 6/6 before everything else.
> 
> Ah, yeah, I assumed we were still re-rolling (and it looks like you're
> just on pu so far).

We are, I had misread a "What's cooking". So a reroll it is.

>> About the peeling I'm not so sure, since there's a difference between a
>> signed tag pointing to a commit and a signed commit.
> 
> There is, but "verify-commit" is always going to verify the commit, no?
> Not peeling will always result in an error, and never do anything
> useful.
> 
> I admit it's probably not going to come up too often, though. And I
> don't have any argument beyond "it makes sense to me", so I won't push
> for it further.
> 
> -Peff

I guess it boils down to the fact how plumberish it's supposed to be.
Since it's about verification, for some definition of "verify", I'd
rather apply as few automatisms as possible. If the caller wants to know
whether commit deadbeef carries a valid commit signature I'd rather
check that very object.

I also picture doing the "git-verify" thing in the future (with
"--commit" and "--tag" options which make the command insist on the
object type), and then we would not want to peel tags under the hood.

Michael

[PATCHv2 4/6] t7510: exit for loop with test result

[Michael J Gruber]

When t7510 was introduced, the author made sure that a for loop in
a subshell would return with the appropriate error code.

Make sure this is true also the for the first line in each loop, which
was missed.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 t/t7510-signed-commit.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
index 5ddac1a..a5ba48e 100755
--- a/t/t7510-signed-commit.sh
+++ b/t/t7510-signed-commit.sh
@@ -49,7 +49,7 @@ test_expect_success GPG 'show signatures' '
 	(
 		for commit in initial second merge fourth-signed fifth-signed sixth-signed master
 		do
-			git show --pretty=short --show-signature $commit >actual &&
+			git show --pretty=short --show-signature $commit >actual || exit 1
 			grep "Good signature from" actual || exit 1
 			! grep "BAD signature from" actual || exit 1
 			echo $commit OK
@@ -58,7 +58,7 @@ test_expect_success GPG 'show signatures' '
 	(
 		for commit in merge^2 fourth-unsigned sixth-unsigned seventh-unsigned
 		do
-			git show --pretty=short --show-signature $commit >actual &&
+			git show --pretty=short --show-signature $commit >actual || exit 1
 			grep "Good signature from" actual && exit 1
 			! grep "BAD signature from" actual || exit 1
 			echo $commit OK
-- 
2.0.0.426.g37dbf84

Re: [PATCHv2 4/6] t7510: exit for loop with test result

[Jeff King]

On Fri, Jun 13, 2014 at 12:42:46PM +0200, Michael J Gruber wrote:

> When t7510 was introduced, the author made sure that a for loop in
> a subshell would return with the appropriate error code.
> 
> Make sure this is true also the for the first line in each loop, which
> was missed.
> 
> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
> ---
>  t/t7510-signed-commit.sh | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
> index 5ddac1a..a5ba48e 100755
> --- a/t/t7510-signed-commit.sh
> +++ b/t/t7510-signed-commit.sh
> @@ -49,7 +49,7 @@ test_expect_success GPG 'show signatures' '
>  	(
>  		for commit in initial second merge fourth-signed fifth-signed sixth-signed master
>  		do
> -			git show --pretty=short --show-signature $commit >actual &&
> +			git show --pretty=short --show-signature $commit >actual || exit 1
>  			grep "Good signature from" actual || exit 1

Hrm. The original is:

  X &&
  Y || exit 1

Won't that still exit (i.e., it is already correct)? Doing:

  for X in true false; do
    for Y in true false; do
      ($X && $Y || exit 1)
      echo "$X/$Y: $?"
    done
  done

yields:

  true/true: 0
  true/false: 1
  false/true: 1
  false/false: 1

(and should still short-circuit Y, because we go from left-to-right).

I do not mind changing it to keep the style of each line consistent,
though. I would have written it as a series of "&&"-chains, with a
single exit at the end, but I think that is just a matter of preference.

-Peff

Re: [PATCHv2 4/6] t7510: exit for loop with test result

[Michael J Gruber]

Jeff King venit, vidit, dixit 13.06.2014 13:46:
> On Fri, Jun 13, 2014 at 12:42:46PM +0200, Michael J Gruber wrote:
> 
>> When t7510 was introduced, the author made sure that a for loop in
>> a subshell would return with the appropriate error code.
>>
>> Make sure this is true also the for the first line in each loop, which
>> was missed.
>>
>> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
>> ---
>>  t/t7510-signed-commit.sh | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
>> index 5ddac1a..a5ba48e 100755
>> --- a/t/t7510-signed-commit.sh
>> +++ b/t/t7510-signed-commit.sh
>> @@ -49,7 +49,7 @@ test_expect_success GPG 'show signatures' '
>>  	(
>>  		for commit in initial second merge fourth-signed fifth-signed sixth-signed master
>>  		do
>> -			git show --pretty=short --show-signature $commit >actual &&
>> +			git show --pretty=short --show-signature $commit >actual || exit 1
>>  			grep "Good signature from" actual || exit 1
> 
> Hrm. The original is:
> 
>   X &&
>   Y || exit 1
> 
> Won't that still exit (i.e., it is already correct)? Doing:
> 
>   for X in true false; do
>     for Y in true false; do
>       ($X && $Y || exit 1)
>       echo "$X/$Y: $?"
>     done
>   done
> 
> yields:
> 
>   true/true: 0
>   true/false: 1
>   false/true: 1
>   false/false: 1
> 
> (and should still short-circuit Y, because we go from left-to-right).
> 
> I do not mind changing it to keep the style of each line consistent,
> though. I would have written it as a series of "&&"-chains, with a
> single exit at the end, but I think that is just a matter of preference.

If I remember correctly, I put something failing on the first line of
the original version, and the test succeeded. I think the point is that
we have a for loop in a subshell, and we need to make sure that the
false of one iteration is not overwritten by the true of the next one -
"exit 1" makes sure to "break" the for loop and exit the subshell.
(The chain should do that as well, I'll recheck.)

Michael

Re: [PATCHv2 4/6] t7510: exit for loop with test result

[Michael J Gruber]

Michael J Gruber venit, vidit, dixit 13.06.2014 14:04:
> Jeff King venit, vidit, dixit 13.06.2014 13:46:
>> On Fri, Jun 13, 2014 at 12:42:46PM +0200, Michael J Gruber wrote:
>>
>>> When t7510 was introduced, the author made sure that a for loop in
>>> a subshell would return with the appropriate error code.
>>>
>>> Make sure this is true also the for the first line in each loop, which
>>> was missed.
>>>
>>> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
>>> ---
>>>  t/t7510-signed-commit.sh | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
>>> index 5ddac1a..a5ba48e 100755
>>> --- a/t/t7510-signed-commit.sh
>>> +++ b/t/t7510-signed-commit.sh
>>> @@ -49,7 +49,7 @@ test_expect_success GPG 'show signatures' '
>>>  	(
>>>  		for commit in initial second merge fourth-signed fifth-signed sixth-signed master
>>>  		do
>>> -			git show --pretty=short --show-signature $commit >actual &&
>>> +			git show --pretty=short --show-signature $commit >actual || exit 1
>>>  			grep "Good signature from" actual || exit 1
>>
>> Hrm. The original is:
>>
>>   X &&
>>   Y || exit 1
>>
>> Won't that still exit (i.e., it is already correct)? Doing:
>>
>>   for X in true false; do
>>     for Y in true false; do
>>       ($X && $Y || exit 1)
>>       echo "$X/$Y: $?"
>>     done
>>   done
>>
>> yields:
>>
>>   true/true: 0
>>   true/false: 1
>>   false/true: 1
>>   false/false: 1
>>
>> (and should still short-circuit Y, because we go from left-to-right).
>>
>> I do not mind changing it to keep the style of each line consistent,
>> though. I would have written it as a series of "&&"-chains, with a
>> single exit at the end, but I think that is just a matter of preference.
> 
> If I remember correctly, I put something failing on the first line of
> the original version, and the test succeeded. I think the point is that
> we have a for loop in a subshell, and we need to make sure that the
> false of one iteration is not overwritten by the true of the next one -
> "exit 1" makes sure to "break" the for loop and exit the subshell.
> (The chain should do that as well, I'll recheck.)

... the chain does not, which is the point :)

With X && Y || exit 1 inside the loop, the loop statement will return
false, but the loop will continue (if X returns false), which is exactly
the problem that the exit avoids.

Make your example iterate over false true instead in the inner loop and
you'll see ;)

Michael

Re: [PATCHv2 4/6] t7510: exit for loop with test result

[Michael J Gruber]

Michael J Gruber venit, vidit, dixit 13.06.2014 14:22:
> Michael J Gruber venit, vidit, dixit 13.06.2014 14:04:
>> Jeff King venit, vidit, dixit 13.06.2014 13:46:
>>> On Fri, Jun 13, 2014 at 12:42:46PM +0200, Michael J Gruber wrote:
>>>
>>>> When t7510 was introduced, the author made sure that a for loop in
>>>> a subshell would return with the appropriate error code.
>>>>
>>>> Make sure this is true also the for the first line in each loop, which
>>>> was missed.
>>>>
>>>> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
>>>> ---
>>>>  t/t7510-signed-commit.sh | 4 ++--
>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
>>>> index 5ddac1a..a5ba48e 100755
>>>> --- a/t/t7510-signed-commit.sh
>>>> +++ b/t/t7510-signed-commit.sh
>>>> @@ -49,7 +49,7 @@ test_expect_success GPG 'show signatures' '
>>>>  	(
>>>>  		for commit in initial second merge fourth-signed fifth-signed sixth-signed master
>>>>  		do
>>>> -			git show --pretty=short --show-signature $commit >actual &&
>>>> +			git show --pretty=short --show-signature $commit >actual || exit 1
>>>>  			grep "Good signature from" actual || exit 1
>>>
>>> Hrm. The original is:
>>>
>>>   X &&
>>>   Y || exit 1
>>>
>>> Won't that still exit (i.e., it is already correct)? Doing:
>>>
>>>   for X in true false; do
>>>     for Y in true false; do
>>>       ($X && $Y || exit 1)
>>>       echo "$X/$Y: $?"
>>>     done
>>>   done
>>>
>>> yields:
>>>
>>>   true/true: 0
>>>   true/false: 1
>>>   false/true: 1
>>>   false/false: 1
>>>
>>> (and should still short-circuit Y, because we go from left-to-right).
>>>
>>> I do not mind changing it to keep the style of each line consistent,
>>> though. I would have written it as a series of "&&"-chains, with a
>>> single exit at the end, but I think that is just a matter of preference.
>>
>> If I remember correctly, I put something failing on the first line of
>> the original version, and the test succeeded. I think the point is that
>> we have a for loop in a subshell, and we need to make sure that the
>> false of one iteration is not overwritten by the true of the next one -
>> "exit 1" makes sure to "break" the for loop and exit the subshell.
>> (The chain should do that as well, I'll recheck.)
> 
> ... the chain does not, which is the point :)
> 
> With X && Y || exit 1 inside the loop, the loop statement will return
> false, but the loop will continue (if X returns false), which is exactly
> the problem that the exit avoids.
> 
> Make your example iterate over false true instead in the inner loop and
> you'll see ;)
> 
> Michael

... with this loop, sorry:

for X in true false; do
     for Y in false true; do
       ($X && $Y || exit 1)
     done
     echo "$X/last inner $Y: $?"
done

gives

true/last inner true: 0
false/last inner true: 1

even though on both cases we have at least one failure of Y. (failure of
one subtest = failure of the test)

Looping in the other order:

for X in true false; do
     for Y in true false; do
       ($X && $Y || exit 1)
     done
     echo "$X/last inner $Y: $?"
done

gives

true/last inner false: 1
false/last inner false: 1

as it should be.

Re: [PATCHv2 4/6] t7510: exit for loop with test result

[Jeff King]

On Fri, Jun 13, 2014 at 02:33:02PM +0200, Michael J Gruber wrote:

> > With X && Y || exit 1 inside the loop, the loop statement will return
> > false, but the loop will continue (if X returns false), which is exactly
> > the problem that the exit avoids.
> > 
> > Make your example iterate over false true instead in the inner loop and
> > you'll see ;)
> > 
> > Michael
> 
> ... with this loop, sorry:
> 
> for X in true false; do
>      for Y in false true; do
>        ($X && $Y || exit 1)
>      done
>      echo "$X/last inner $Y: $?"
> done

I'm somewhat confused, as my loops were meant only to expand the truth
table, not to simulate a real loop in the code. That is why I have a
subshell in the loop around my exit, to make sure we keep looping. In
the real code, the subshell surrounds the whole loop (so that an "exit"
leaves the entire loop without leaving the whole script).

The actual code is more like:

  (
    for i in a b c; do
      echo $i: got to first step &&
      test $i != b && 
      echo $i: got to second step || exit 1
    done
  )
  echo overall status: $?

which should fail on the second loop iteration. And it does:

  a: got to first step
  a: got to second step
  b: got to first step
  overall status: 1

That is, we short-circuit to the "exit 1" as soon as "test $i != b"
fails. You can replace the use of "$?" above with more "&&"-chaining, of
course.

-Peff

Re: [PATCHv2 4/6] t7510: exit for loop with test result

[Johannes Sixt]

Am 6/13/2014 14:33, schrieb Michael J Gruber:
> .... with this loop, sorry:
> 
> for X in true false; do
>      for Y in false true; do
>        ($X && $Y || exit 1)
>      done
>      echo "$X/last inner $Y: $?"
> done
> 
> gives
> 
> true/last inner true: 0
> false/last inner true: 1
> 
> even though on both cases we have at least one failure of Y. (failure of
> one subtest = failure of the test)

Place the loop(s) inside the subshell, and you observe termination on the
first failure, and a failure exit code of the subshell.

The change proposed in this patch should not be necessary. Something else
must be wrong with your tests.

Ah, here it is:

@@ -58,7 +58,7 @@ test_expect_success GPG 'show signatures' '
 	(
 		for commit in merge^2 fourth-unsigned sixth-unsigned seventh-unsigned
 		do
-			git show --pretty=short --show-signature $commit >actual &&
+			git show --pretty=short --show-signature $commit >actual || exit 1
 			grep "Good signature from" actual && exit 1
 			! grep "BAD signature from" actual || exit 1
 			echo $commit OK

Notice the '&& exit 1'! It should be '|| exit 1', right?

-- Hannes

Re: [PATCHv2 4/6] t7510: exit for loop with test result

[Michael J Gruber]

Johannes Sixt venit, vidit, dixit 13.06.2014 14:54:
> Am 6/13/2014 14:33, schrieb Michael J Gruber:
>> .... with this loop, sorry:
>>
>> for X in true false; do
>>      for Y in false true; do
>>        ($X && $Y || exit 1)
>>      done
>>      echo "$X/last inner $Y: $?"
>> done
>>
>> gives
>>
>> true/last inner true: 0
>> false/last inner true: 1
>>
>> even though on both cases we have at least one failure of Y. (failure of
>> one subtest = failure of the test)
> 
> Place the loop(s) inside the subshell, and you observe termination on the
> first failure, and a failure exit code of the subshell.
> 
> The change proposed in this patch should not be necessary. Something else
> must be wrong with your tests.

I know I started this (or Jeff did, who knows ;) ), but we keep
confusing each other more and more:

> Ah, here it is:
> 
> @@ -58,7 +58,7 @@ test_expect_success GPG 'show signatures' '
>  	(
>  		for commit in merge^2 fourth-unsigned sixth-unsigned seventh-unsigned
>  		do
> -			git show --pretty=short --show-signature $commit >actual &&
> +			git show --pretty=short --show-signature $commit >actual || exit 1
>  			grep "Good signature from" actual && exit 1

This is as in the original, it tests invalid signatures, so "Good
signature" should not be in the response.

>  			! grep "BAD signature from" actual || exit 1
>  			echo $commit OK
> 
> Notice the '&& exit 1'! It should be '|| exit 1', right?
> 
> -- Hannes

In other words, the original tests already had

grep foo && exit 1
! grep bar || exit 1

to test that we have neither foo nor bar. The reason is (supposedly) to
have this portion of the test mostly analogous to the previous one,
where we want foo and do want bar.

So this is completely unrelated.

Otoh, it seems the original test could have had

a &&
b &&
c || exit 1

or

a || exit 1
b || exit 1
c || exit 1

rather than

a &&
b || exit 1
c || exit 1

which I thought was incorrect (but I can't recreate the proof right
now). I'd say both of the former versions are preferable to the last one
unless there is a difference that neither Jeff nor I see.

I need a break before looking at this again ;)

Michael

Re: [PATCHv2 4/6] t7510: exit for loop with test result

[Johannes Sixt]

Am 6/13/2014 15:06, schrieb Michael J Gruber:
> Johannes Sixt venit, vidit, dixit 13.06.2014 14:54:
>> Am 6/13/2014 14:33, schrieb Michael J Gruber:
>>> .... with this loop, sorry:
>>>
>>> for X in true false; do
>>>      for Y in false true; do
>>>        ($X && $Y || exit 1)
>>>      done
>>>      echo "$X/last inner $Y: $?"
>>> done
>>>
>>> gives
>>>
>>> true/last inner true: 0
>>> false/last inner true: 1
>>>
>>> even though on both cases we have at least one failure of Y. (failure of
>>> one subtest = failure of the test)
>>
>> Place the loop(s) inside the subshell, and you observe termination on the
>> first failure, and a failure exit code of the subshell.
>>
>> The change proposed in this patch should not be necessary. Something else
>> must be wrong with your tests.
> 
> I know I started this (or Jeff did, who knows ;) ), but we keep
> confusing each other more and more:
> 
>> Ah, here it is:
>>
>> @@ -58,7 +58,7 @@ test_expect_success GPG 'show signatures' '
>>  	(
>>  		for commit in merge^2 fourth-unsigned sixth-unsigned seventh-unsigned
>>  		do
>> -			git show --pretty=short --show-signature $commit >actual &&
>> +			git show --pretty=short --show-signature $commit >actual || exit 1
>>  			grep "Good signature from" actual && exit 1
> 
> This is as in the original, it tests invalid signatures, so "Good
> signature" should not be in the response.
> 
>>  			! grep "BAD signature from" actual || exit 1
>>  			echo $commit OK
>>
>> Notice the '&& exit 1'! It should be '|| exit 1', right?
>>
>> -- Hannes
> 
> In other words, the original tests already had
> 
> grep foo && exit 1
> ! grep bar || exit 1
> 
> to test that we have neither foo nor bar. The reason is (supposedly) to
> have this portion of the test mostly analogous to the previous one,
> where we want foo and do want bar.
> 
> So this is completely unrelated.

I don't think so. What is the outcome of

	false &&  # simulate a regression
	grep foo && exit 1
	! grep bar || exit 1

assuming that the '! grep bar' happens to be true? Answer: The regression
is not diagnosed because the &&-chain is broken.

*That* is what I think you described earlier in this thread as "I put
something failing on the first line of the original version, and the test
succeeded."

-- Hannes

Re: [PATCHv2 4/6] t7510: exit for loop with test result

[Jeff King]

On Fri, Jun 13, 2014 at 03:21:55PM +0200, Johannes Sixt wrote:

> I don't think so. What is the outcome of
> 
> 	false &&  # simulate a regression
> 	grep foo && exit 1
> 	! grep bar || exit 1
> 
> assuming that the '! grep bar' happens to be true? Answer: The regression
> is not diagnosed because the &&-chain is broken.
> 
> *That* is what I think you described earlier in this thread as "I put
> something failing on the first line of the original version, and the test
> succeeded."

Yeah, I think that is the bit that I was missing from my original
confusion.

  false &&
  anything || exit 1

_does_ work. But that is not what is written. ;)

Thanks for pointing it out.

-Peff

Re: [PATCHv2 4/6] t7510: exit for loop with test result

[Michael J Gruber]

Johannes Sixt venit, vidit, dixit 13.06.2014 15:21:
> Am 6/13/2014 15:06, schrieb Michael J Gruber:
>> Johannes Sixt venit, vidit, dixit 13.06.2014 14:54:
>>> Am 6/13/2014 14:33, schrieb Michael J Gruber:
>>>> .... with this loop, sorry:
>>>>
>>>> for X in true false; do
>>>>      for Y in false true; do
>>>>        ($X && $Y || exit 1)
>>>>      done
>>>>      echo "$X/last inner $Y: $?"
>>>> done
>>>>
>>>> gives
>>>>
>>>> true/last inner true: 0
>>>> false/last inner true: 1
>>>>
>>>> even though on both cases we have at least one failure of Y. (failure of
>>>> one subtest = failure of the test)
>>>
>>> Place the loop(s) inside the subshell, and you observe termination on the
>>> first failure, and a failure exit code of the subshell.
>>>
>>> The change proposed in this patch should not be necessary. Something else
>>> must be wrong with your tests.
>>
>> I know I started this (or Jeff did, who knows ;) ), but we keep
>> confusing each other more and more:
>>
>>> Ah, here it is:
>>>
>>> @@ -58,7 +58,7 @@ test_expect_success GPG 'show signatures' '
>>>  	(
>>>  		for commit in merge^2 fourth-unsigned sixth-unsigned seventh-unsigned
>>>  		do
>>> -			git show --pretty=short --show-signature $commit >actual &&
>>> +			git show --pretty=short --show-signature $commit >actual || exit 1
>>>  			grep "Good signature from" actual && exit 1
>>
>> This is as in the original, it tests invalid signatures, so "Good
>> signature" should not be in the response.
>>
>>>  			! grep "BAD signature from" actual || exit 1
>>>  			echo $commit OK
>>>
>>> Notice the '&& exit 1'! It should be '|| exit 1', right?
>>>
>>> -- Hannes
>>
>> In other words, the original tests already had
>>
>> grep foo && exit 1
>> ! grep bar || exit 1
>>
>> to test that we have neither foo nor bar. The reason is (supposedly) to
>> have this portion of the test mostly analogous to the previous one,
>> where we want foo and do want bar.
>>
>> So this is completely unrelated.
> 
> I don't think so. What is the outcome of
> 
> 	false &&  # simulate a regression
> 	grep foo && exit 1
> 	! grep bar || exit 1
> 
> assuming that the '! grep bar' happens to be true? Answer: The regression
> is not diagnosed because the &&-chain is broken.
> 
> *That* is what I think you described earlier in this thread as "I put
> something failing on the first line of the original version, and the test
> succeeded."
> 
> -- Hannes

If you say that something I have said makes sense I'm happy, because I
can't confirm that myself right now. I'll take a break and look into a
rewrite of the form

a &&
b &&
test_must_fail c &&
d || exit 1

hoping that will make things both readable (by avoiding !) and concise
(by avoiding repeated exits).

Michael

Re: [PATCHv2 4/6] t7510: exit for loop with test result

[Johannes Sixt]

Am 6/13/2014 15:31, schrieb Michael J Gruber:
> rewrite of the form
> 
> a &&
> b &&
> test_must_fail c &&
> d || exit 1
> 
> hoping that will make things both readable (by avoiding !) and concise
> (by avoiding repeated exits).

Thanks!

Please note that we use 'test_must_fail' only for git invocations, but we
do write '!' in front of system commands that we expect to fail, e.g.,
'! grep'.

-- Hannes

Re: [PATCHv2 4/6] t7510: exit for loop with test result

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>>  	(
>>  		for commit in initial second merge fourth-signed fifth-signed sixth-signed master
>>  		do
>> -			git show --pretty=short --show-signature $commit >actual &&
>> +			git show --pretty=short --show-signature $commit >actual || exit 1
>>  			grep "Good signature from" actual || exit 1
>
> Hrm. The original is:
>
>   X &&
>   Y || exit 1
>
> Won't that still exit (i.e., it is already correct)? Doing:
>
>   for X in true false; do
>     for Y in true false; do
>       ($X && $Y || exit 1)
>       echo "$X/$Y: $?"
>     done
>   done
>
> yields:
>
>   true/true: 0
>   true/false: 1
>   false/true: 1
>   false/false: 1
>
> (and should still short-circuit Y, because we go from left-to-right).
>
> I do not mind changing it to keep the style of each line consistent,
> though. I would have written it as a series of "&&"-chains, with a
> single exit at the end, but I think that is just a matter of preference.

Yeah, series of && chain with a single exit at the end is good, and
the subshell is there only to allow us to do that "exit at the end".

[PATCHv2 5/6] t7510: test verify-commit

[Michael J Gruber]

This mixes the "git verify-commit" tests in with the "git show
--show-signature" tests, to keep the tests more readable.

The tests already mix in the "call show" tests with the "verify" tests.
So in case of a test beakage, a '-v' run would be needed to reveal the
exact point of breakage anyway.

Additionally, test the actual output of "git verify-commit" and "git
show --show-signature" and compare to "git cat-file".

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 t/t7510-signed-commit.sh | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
index a5ba48e..89941a7 100755
--- a/t/t7510-signed-commit.sh
+++ b/t/t7510-signed-commit.sh
@@ -45,10 +45,11 @@ test_expect_success GPG 'create signed commits' '
 	git tag seventh-signed
 '
 
-test_expect_success GPG 'show signatures' '
+test_expect_success GPG 'verify and show signatures' '
 	(
 		for commit in initial second merge fourth-signed fifth-signed sixth-signed master
 		do
+			git verify-commit $commit || exit 1
 			git show --pretty=short --show-signature $commit >actual || exit 1
 			grep "Good signature from" actual || exit 1
 			! grep "BAD signature from" actual || exit 1
@@ -58,6 +59,7 @@ test_expect_success GPG 'show signatures' '
 	(
 		for commit in merge^2 fourth-unsigned sixth-unsigned seventh-unsigned
 		do
+			! git verify-commit $commit || exit 1
 			git show --pretty=short --show-signature $commit >actual || exit 1
 			grep "Good signature from" actual && exit 1
 			! grep "BAD signature from" actual || exit 1
@@ -66,11 +68,25 @@ test_expect_success GPG 'show signatures' '
 	)
 '
 
+test_expect_success GPG 'show signed commit with signature' '
+	git show -s initial >commit &&
+	git show -s --show-signature initial >show &&
+	git verify-commit -v initial >verify.1 2>verify.2 &&
+	git cat-file commit initial >cat &&
+	grep -v "gpg: " show >show.commit &&
+	grep "gpg: " show >show.gpg &&
+	grep -v "^ " cat | grep -v "^gpgsig " >cat.commit &&
+	test_cmp show.commit commit &&
+	test_cmp show.gpg verify.2 &&
+	test_cmp cat.commit verify.1
+'
+
 test_expect_success GPG 'detect fudged signature' '
 	git cat-file commit master >raw &&
 
 	sed -e "s/seventh/7th forged/" raw >forged1 &&
 	git hash-object -w -t commit forged1 >forged1.commit &&
+	! git verify-commit $(cat forged1.commit) &&
 	git show --pretty=short --show-signature $(cat forged1.commit) >actual1 &&
 	grep "BAD signature from" actual1 &&
 	! grep "Good signature from" actual1
@@ -81,6 +97,7 @@ test_expect_success GPG 'detect fudged signature with NUL' '
 	cat raw >forged2 &&
 	echo Qwik | tr "Q" "\000" >>forged2 &&
 	git hash-object -w -t commit forged2 >forged2.commit &&
+	! git verify-commit $(cat forged2.commit) &&
 	git show --pretty=short --show-signature $(cat forged2.commit) >actual2 &&
 	grep "BAD signature from" actual2 &&
 	! grep "Good signature from" actual2
@@ -89,6 +106,7 @@ test_expect_success GPG 'detect fudged signature with NUL' '
 test_expect_success GPG 'amending already signed commit' '
 	git checkout fourth-signed^0 &&
 	git commit --amend -S --no-edit &&
+	git verify-commit HEAD &&
 	git show -s --show-signature HEAD >actual &&
 	grep "Good signature from" actual &&
 	! grep "BAD signature from" actual
-- 
2.0.0.426.g37dbf84

Re: [PATCHv2 5/6] t7510: test verify-commit

[Jeff King]

On Fri, Jun 13, 2014 at 12:42:47PM +0200, Michael J Gruber wrote:

>  test_expect_success GPG 'detect fudged signature' '
>  	git cat-file commit master >raw &&
>  
>  	sed -e "s/seventh/7th forged/" raw >forged1 &&
>  	git hash-object -w -t commit forged1 >forged1.commit &&
> +	! git verify-commit $(cat forged1.commit) &&

Please use test_must_fail here (and further down), which will catch
things like signal death.

-Peff

Re: [PATCHv2 5/6] t7510: test verify-commit

[Michael J Gruber]

Jeff King venit, vidit, dixit 13.06.2014 13:51:
> On Fri, Jun 13, 2014 at 12:42:47PM +0200, Michael J Gruber wrote:
> 
>>  test_expect_success GPG 'detect fudged signature' '
>>  	git cat-file commit master >raw &&
>>  
>>  	sed -e "s/seventh/7th forged/" raw >forged1 &&
>>  	git hash-object -w -t commit forged1 >forged1.commit &&
>> +	! git verify-commit $(cat forged1.commit) &&
> 
> Please use test_must_fail here (and further down), which will catch
> things like signal death.

Again, that is an issue of keeping the style of the surrounding code
(which is relatively new) vs. doing it differently. I don't mind
changing t7510 to a different style, though.

Michael

Re: [PATCHv2 5/6] t7510: test verify-commit

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

> Jeff King venit, vidit, dixit 13.06.2014 13:51:
>> On Fri, Jun 13, 2014 at 12:42:47PM +0200, Michael J Gruber wrote:
>> 
>>>  test_expect_success GPG 'detect fudged signature' '
>>>  	git cat-file commit master >raw &&
>>>  
>>>  	sed -e "s/seventh/7th forged/" raw >forged1 &&
>>>  	git hash-object -w -t commit forged1 >forged1.commit &&
>>> +	! git verify-commit $(cat forged1.commit) &&
>> 
>> Please use test_must_fail here (and further down), which will catch
>> things like signal death.
>
> Again, that is an issue of keeping the style of the surrounding code
> (which is relatively new) vs. doing it differently. I don't mind
> changing t7510 to a different style, though.

We do not have any "! git anything" in that script, don't we?  We do
not expect "grep" supplied by the platform to lie, and that is why
we should never use test_must_fail on them, but we do want to notice
when git starts segfaulting, and that is what test_must_fail is for.

We tell novices and cluelesses who do not know better to try to
follow and match the surrounding style.  This is because it would at
least not make things worse than if we let them run loose on our
codebase.  It is not about "if we have a bad style, it is better to
match that existing bad style to spread the badness than making it
partly better".  But you have been here long enough and know what is
preferred and and what is to be avoided---more importantly, Peff has
been here long enough to know and has given such an advice, so...

In any case, I do not offhand see anything wrong style-wise in 7510,
so please do not change anything in it for the sake of styles.

[PATCHv2 6/6] gpg-interface: provide clear helper for struct signature_check

[Michael J Gruber]

The struct has been growing members whose malloced memory needs to be
freed. Do this with one helper function so that no malloced memory shall
be left unfreed.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 builtin/merge.c         |  6 +-----
 builtin/verify-commit.c |  5 +----
 gpg-interface.c         | 14 ++++++++++++++
 gpg-interface.h         |  1 +
 pretty.c                |  5 +----
 5 files changed, 18 insertions(+), 13 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index 6a9812a..e50323d 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1282,11 +1282,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 				printf(_("Commit %s has a good GPG signature by %s\n"),
 				       hex, signature_check.signer);
 
-			free(signature_check.payload);
-			free(signature_check.gpg_output);
-			free(signature_check.gpg_status);
-			free(signature_check.signer);
-			free(signature_check.key);
+			signature_check_clear(&signature_check);
 		}
 	}
 
diff --git a/builtin/verify-commit.c b/builtin/verify-commit.c
index 69b7c6d..d254fcf 100644
--- a/builtin/verify-commit.c
+++ b/builtin/verify-commit.c
@@ -32,10 +32,7 @@ static int run_gpg_verify(const unsigned char *sha1, const char *buf, unsigned l
 	if (signature_check.gpg_output)
 		fputs(signature_check.gpg_output, stderr);
 
-	free(signature_check.gpg_output);
-	free(signature_check.gpg_status);
-	free(signature_check.signer);
-	free(signature_check.key);
+	signature_check_clear(&signature_check);
 	return signature_check.result != 'G';
 }
 
diff --git a/gpg-interface.c b/gpg-interface.c
index 8b0e874..ff07012 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -7,6 +7,20 @@
 static char *configured_signing_key;
 static const char *gpg_program = "gpg";
 
+void signature_check_clear(struct signature_check *sigc)
+{
+	free(sigc->payload);
+	free(sigc->gpg_output);
+	free(sigc->gpg_status);
+	free(sigc->signer);
+	free(sigc->key);
+	sigc->payload = NULL;
+	sigc->gpg_output = NULL;
+	sigc->gpg_status = NULL;
+	sigc->signer = NULL;
+	sigc->key = NULL;
+}
+
 void set_signing_key(const char *key)
 {
 	free(configured_signing_key);
diff --git a/gpg-interface.h b/gpg-interface.h
index d727c25..37c23da 100644
--- a/gpg-interface.h
+++ b/gpg-interface.h
@@ -14,6 +14,7 @@ struct signature_check {
 	char *key;
 };
 
+extern void signature_check_clear(struct signature_check *sigc);
 extern int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *signing_key);
 extern int verify_signed_buffer(const char *payload, size_t payload_size, const char *signature, size_t signature_size, struct strbuf *gpg_output, struct strbuf *gpg_status);
 extern int git_gpg_config(const char *, const char *, void *);
diff --git a/pretty.c b/pretty.c
index 24fb877..ac901b8 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1537,10 +1537,7 @@ void format_commit_message(const struct commit *commit,
 
 	free(context.commit_encoding);
 	logmsg_free(context.message, commit);
-	free(context.signature_check.payload);
-	free(context.signature_check.gpg_output);
-	free(context.signature_check.gpg_status);
-	free(context.signature_check.signer);
+	signature_check_clear(&context.signature_check);
 }
 
 static void pp_header(struct pretty_print_context *pp,
-- 
2.0.0.426.g37dbf84

[PATCHv3 0/5] verify-commit: verify commit signatures

[Michael J Gruber]

This incorporates all remarks about the test coding guidelines and
rearranging the series.

Open questions:
- There was some debate about (optionally) verifying more than what
git-verify-{commit,tag} currently do, or going for a generic git-verify command.
The former would require both to be changed (in order to treat similar cases similarly),
the latter would need a deprecation for git-verify-tag.

- I haven't looked yet at what happened over the weekend.

Michael J Gruber (5):
  gpg-interface: provide clear helper for struct signature_check
  gpg-interface: provide access to the payload
  verify-commit: scriptable commit signature verification
  t7510: exit for loop with test result
  t7510: test verify-commit

 Documentation/git-verify-commit.txt | 28 +++++++++++
 Makefile                            |  1 +
 builtin.h                           |  1 +
 builtin/merge.c                     |  5 +-
 builtin/verify-commit.c             | 93 +++++++++++++++++++++++++++++++++++++
 command-list.txt                    |  1 +
 commit.c                            |  1 +
 git.c                               |  1 +
 gpg-interface.c                     | 14 ++++++
 gpg-interface.h                     |  2 +
 pretty.c                            |  3 +-
 t/t7510-signed-commit.sh            | 24 ++++++++--
 12 files changed, 165 insertions(+), 9 deletions(-)
 create mode 100644 Documentation/git-verify-commit.txt
 create mode 100644 builtin/verify-commit.c

-- 
2.0.0.426.g37dbf84

[PATCHv3 1/5] gpg-interface: provide clear helper for struct signature_check

[Michael J Gruber]

The struct has been growing members whose malloced memory needs to be
freed. Do this with one helper function so that no malloced memory shall
be left unfreed.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 builtin/merge.c |  5 +----
 gpg-interface.c | 12 ++++++++++++
 gpg-interface.h |  1 +
 pretty.c        |  3 +--
 4 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index 428ca24..e50323d 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1282,10 +1282,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 				printf(_("Commit %s has a good GPG signature by %s\n"),
 				       hex, signature_check.signer);
 
-			free(signature_check.gpg_output);
-			free(signature_check.gpg_status);
-			free(signature_check.signer);
-			free(signature_check.key);
+			signature_check_clear(&signature_check);
 		}
 	}
 
diff --git a/gpg-interface.c b/gpg-interface.c
index 8b0e874..e71b59d 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -7,6 +7,18 @@
 static char *configured_signing_key;
 static const char *gpg_program = "gpg";
 
+void signature_check_clear(struct signature_check *sigc)
+{
+	free(sigc->gpg_output);
+	free(sigc->gpg_status);
+	free(sigc->signer);
+	free(sigc->key);
+	sigc->gpg_output = NULL;
+	sigc->gpg_status = NULL;
+	sigc->signer = NULL;
+	sigc->key = NULL;
+}
+
 void set_signing_key(const char *key)
 {
 	free(configured_signing_key);
diff --git a/gpg-interface.h b/gpg-interface.h
index a85cb5b..9f0784a 100644
--- a/gpg-interface.h
+++ b/gpg-interface.h
@@ -13,6 +13,7 @@ struct signature_check {
 	char *key;
 };
 
+extern void signature_check_clear(struct signature_check *sigc);
 extern int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *signing_key);
 extern int verify_signed_buffer(const char *payload, size_t payload_size, const char *signature, size_t signature_size, struct strbuf *gpg_output, struct strbuf *gpg_status);
 extern int git_gpg_config(const char *, const char *, void *);
diff --git a/pretty.c b/pretty.c
index 4f51287..ac901b8 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1537,8 +1537,7 @@ void format_commit_message(const struct commit *commit,
 
 	free(context.commit_encoding);
 	logmsg_free(context.message, commit);
-	free(context.signature_check.gpg_output);
-	free(context.signature_check.signer);
+	signature_check_clear(&context.signature_check);
 }
 
 static void pp_header(struct pretty_print_context *pp,
-- 
2.0.0.426.g37dbf84

[PATCHv3 2/5] gpg-interface: provide access to the payload

[Michael J Gruber]

In contrast to tag signatures, commit signatures are put into the
header, that is between the other header parts and commit messages.

Provide access to the commit content sans the signature, which is the
payload that is actually signed. Commit signature verification does the
parsing anyways, and callers may wish to act on or display the commit
object sans the signature.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 commit.c        | 1 +
 gpg-interface.c | 2 ++
 gpg-interface.h | 1 +
 3 files changed, 4 insertions(+)

diff --git a/commit.c b/commit.c
index 881be3b..d0ad7f0 100644
--- a/commit.c
+++ b/commit.c
@@ -1219,6 +1219,7 @@ void check_commit_signature(const struct commit* commit, struct signature_check
 				      &gpg_output, &gpg_status);
 	if (status && !gpg_output.len)
 		goto out;
+	sigc->payload = strbuf_detach(&payload, NULL);
 	sigc->gpg_output = strbuf_detach(&gpg_output, NULL);
 	sigc->gpg_status = strbuf_detach(&gpg_status, NULL);
 	parse_gpg_output(sigc);
diff --git a/gpg-interface.c b/gpg-interface.c
index e71b59d..ff07012 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -9,10 +9,12 @@ static const char *gpg_program = "gpg";
 
 void signature_check_clear(struct signature_check *sigc)
 {
+	free(sigc->payload);
 	free(sigc->gpg_output);
 	free(sigc->gpg_status);
 	free(sigc->signer);
 	free(sigc->key);
+	sigc->payload = NULL;
 	sigc->gpg_output = NULL;
 	sigc->gpg_status = NULL;
 	sigc->signer = NULL;
diff --git a/gpg-interface.h b/gpg-interface.h
index 9f0784a..37c23da 100644
--- a/gpg-interface.h
+++ b/gpg-interface.h
@@ -2,6 +2,7 @@
 #define GPG_INTERFACE_H
 
 struct signature_check {
+	char *payload;
 	char *gpg_output;
 	char *gpg_status;
 	char result; /* 0 (not checked),
-- 
2.0.0.426.g37dbf84

[PATCHv3 3/5] verify-commit: scriptable commit signature verification

[Michael J Gruber]

Commit signatures can be verified using "git show -s --show-signature"
or the "%G?" pretty format and parsing the output, which is well suited
for user inspection, but not for scripting.

Provide a command "verify-commit" which is analogous to "verify-tag": It
returns 0 for good signatures and non-zero otherwise, has the gpg output
on stderr and (optionally) the commit object on stdout, sans the
signature, just like "verify-tag" does.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 Documentation/git-verify-commit.txt | 28 +++++++++++
 Makefile                            |  1 +
 builtin.h                           |  1 +
 builtin/verify-commit.c             | 93 +++++++++++++++++++++++++++++++++++++
 command-list.txt                    |  1 +
 git.c                               |  1 +
 6 files changed, 125 insertions(+)
 create mode 100644 Documentation/git-verify-commit.txt
 create mode 100644 builtin/verify-commit.c

diff --git a/Documentation/git-verify-commit.txt b/Documentation/git-verify-commit.txt
new file mode 100644
index 0000000..9413e28
--- /dev/null
+++ b/Documentation/git-verify-commit.txt
@@ -0,0 +1,28 @@
+git-verify-commit(1)
+====================
+
+NAME
+----
+git-verify-commit - Check the GPG signature of commits
+
+SYNOPSIS
+--------
+[verse]
+'git verify-commit' <commit>...
+
+DESCRIPTION
+-----------
+Validates the gpg signature created by 'git commit -S'.
+
+OPTIONS
+-------
+-v::
+--verbose::
+	Print the contents of the commit object before validating it.
+
+<commit>...::
+	SHA-1 identifiers of Git commit objects.
+
+GIT
+---
+Part of the linkgit:git[1] suite
diff --git a/Makefile b/Makefile
index 07ea105..b92418d 100644
--- a/Makefile
+++ b/Makefile
@@ -999,6 +999,7 @@ BUILTIN_OBJS += builtin/update-ref.o
 BUILTIN_OBJS += builtin/update-server-info.o
 BUILTIN_OBJS += builtin/upload-archive.o
 BUILTIN_OBJS += builtin/var.o
+BUILTIN_OBJS += builtin/verify-commit.o
 BUILTIN_OBJS += builtin/verify-pack.o
 BUILTIN_OBJS += builtin/verify-tag.o
 BUILTIN_OBJS += builtin/write-tree.o
diff --git a/builtin.h b/builtin.h
index c47c110..5d91f31 100644
--- a/builtin.h
+++ b/builtin.h
@@ -128,6 +128,7 @@ extern int cmd_update_server_info(int argc, const char **argv, const char *prefi
 extern int cmd_upload_archive(int argc, const char **argv, const char *prefix);
 extern int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix);
 extern int cmd_var(int argc, const char **argv, const char *prefix);
+extern int cmd_verify_commit(int argc, const char **argv, const char *prefix);
 extern int cmd_verify_tag(int argc, const char **argv, const char *prefix);
 extern int cmd_version(int argc, const char **argv, const char *prefix);
 extern int cmd_whatchanged(int argc, const char **argv, const char *prefix);
diff --git a/builtin/verify-commit.c b/builtin/verify-commit.c
new file mode 100644
index 0000000..b0f8504
--- /dev/null
+++ b/builtin/verify-commit.c
@@ -0,0 +1,93 @@
+/*
+ * Builtin "git commit-commit"
+ *
+ * Copyright (c) 2014 Michael J Gruber <git@drmicha.warpmail.net>
+ *
+ * Based on git-verify-tag
+ */
+#include "cache.h"
+#include "builtin.h"
+#include "commit.h"
+#include "run-command.h"
+#include <signal.h>
+#include "parse-options.h"
+#include "gpg-interface.h"
+
+static const char * const verify_commit_usage[] = {
+		N_("git verify-commit [-v|--verbose] <commit>..."),
+		NULL
+};
+
+static int run_gpg_verify(const unsigned char *sha1, const char *buf, unsigned long size, int verbose)
+{
+	struct signature_check signature_check;
+
+	memset(&signature_check, 0, sizeof(signature_check));
+
+	check_commit_signature(lookup_commit(sha1), &signature_check);
+
+	if (verbose && signature_check.payload)
+		fputs(signature_check.payload, stdout);
+
+	if (signature_check.gpg_output)
+		fputs(signature_check.gpg_output, stderr);
+
+	signature_check_clear(&signature_check);
+	return signature_check.result != 'G';
+}
+
+static int verify_commit(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))
+		return error("commit '%s' not found.", name);
+
+	buf = read_sha1_file(sha1, &type, &size);
+	if (!buf)
+		return error("%s: unable to read file.", name);
+	if (type != OBJ_COMMIT)
+		return error("%s: cannot verify a non-commit object of type %s.",
+				name, typename(type));
+
+	ret = run_gpg_verify(sha1, buf, size, verbose);
+
+	free(buf);
+	return ret;
+}
+
+static int git_verify_commit_config(const char *var, const char *value, void *cb)
+{
+	int status = git_gpg_config(var, value, cb);
+	if (status)
+		return status;
+	return git_default_config(var, value, cb);
+}
+
+int cmd_verify_commit(int argc, const char **argv, const char *prefix)
+{
+	int i = 1, verbose = 0, had_error = 0;
+	const struct option verify_commit_options[] = {
+		OPT__VERBOSE(&verbose, N_("print commit contents")),
+		OPT_END()
+	};
+
+	git_config(git_verify_commit_config, NULL);
+
+	argc = parse_options(argc, argv, prefix, verify_commit_options,
+			     verify_commit_usage, PARSE_OPT_KEEP_ARGV0);
+	if (argc <= i)
+		usage_with_options(verify_commit_usage, verify_commit_options);
+
+	/* 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_commit(argv[i++], verbose))
+			had_error = 1;
+	return had_error;
+}
diff --git a/command-list.txt b/command-list.txt
index cf36c3d..a3ff0c9 100644
--- a/command-list.txt
+++ b/command-list.txt
@@ -132,6 +132,7 @@ git-update-server-info                  synchingrepositories
 git-upload-archive                      synchelpers
 git-upload-pack                         synchelpers
 git-var                                 plumbinginterrogators
+git-verify-commit                       ancillaryinterrogators
 git-verify-pack                         plumbinginterrogators
 git-verify-tag                          ancillaryinterrogators
 gitweb                                  ancillaryinterrogators
diff --git a/git.c b/git.c
index 7780572..bbc3293 100644
--- a/git.c
+++ b/git.c
@@ -441,6 +441,7 @@ static struct cmd_struct commands[] = {
 	{ "upload-archive", cmd_upload_archive },
 	{ "upload-archive--writer", cmd_upload_archive_writer },
 	{ "var", cmd_var, RUN_SETUP_GENTLY },
+	{ "verify-commit", cmd_verify_commit, RUN_SETUP },
 	{ "verify-pack", cmd_verify_pack },
 	{ "verify-tag", cmd_verify_tag, RUN_SETUP },
 	{ "version", cmd_version },
-- 
2.0.0.426.g37dbf84

[PATCHv3 4/5] t7510: exit for loop with test result

[Michael J Gruber]

t7510 uses for loops in a subshell, which need to make sure that the test
returns with the appropriate error code from within the loop.

Restructure the loops as the usual && chains with a single point of
"exit 1" at the end of the loop to make this clearer.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 t/t7510-signed-commit.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
index 5ddac1a..96cfddf 100755
--- a/t/t7510-signed-commit.sh
+++ b/t/t7510-signed-commit.sh
@@ -50,7 +50,7 @@ test_expect_success GPG 'show signatures' '
 		for commit in initial second merge fourth-signed fifth-signed sixth-signed master
 		do
 			git show --pretty=short --show-signature $commit >actual &&
-			grep "Good signature from" actual || exit 1
+			grep "Good signature from" actual &&
 			! grep "BAD signature from" actual || exit 1
 			echo $commit OK
 		done
@@ -59,7 +59,7 @@ test_expect_success GPG 'show signatures' '
 		for commit in merge^2 fourth-unsigned sixth-unsigned seventh-unsigned
 		do
 			git show --pretty=short --show-signature $commit >actual &&
-			grep "Good signature from" actual && exit 1
+			! grep "Good signature from" actual &&
 			! grep "BAD signature from" actual || exit 1
 			echo $commit OK
 		done
-- 
2.0.0.426.g37dbf84

[PATCHv3 5/5] t7510: test verify-commit

[Michael J Gruber]

This mixes the "git verify-commit" tests in with the "git show
--show-signature" tests, to keep the tests more readable.

The tests already mix in the "call show" tests with the "verify" tests.
So in case of a test beakage, a '-v' run would be needed to reveal the
exact point of breakage anyway.

Additionally, test the actual output of "git verify-commit" and "git
show --show-signature" and compare to "git cat-file".

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 t/t7510-signed-commit.sh | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
index 96cfddf..dd4b948 100755
--- a/t/t7510-signed-commit.sh
+++ b/t/t7510-signed-commit.sh
@@ -45,10 +45,11 @@ test_expect_success GPG 'create signed commits' '
 	git tag seventh-signed
 '
 
-test_expect_success GPG 'show signatures' '
+test_expect_success GPG 'verify and show signatures' '
 	(
 		for commit in initial second merge fourth-signed fifth-signed sixth-signed master
 		do
+			git verify-commit $commit &&
 			git show --pretty=short --show-signature $commit >actual &&
 			grep "Good signature from" actual &&
 			! grep "BAD signature from" actual || exit 1
@@ -58,6 +59,7 @@ test_expect_success GPG 'show signatures' '
 	(
 		for commit in merge^2 fourth-unsigned sixth-unsigned seventh-unsigned
 		do
+			test_must_fail git verify-commit $commit &&
 			git show --pretty=short --show-signature $commit >actual &&
 			! grep "Good signature from" actual &&
 			! grep "BAD signature from" actual || exit 1
@@ -66,11 +68,25 @@ test_expect_success GPG 'show signatures' '
 	)
 '
 
+test_expect_success GPG 'show signed commit with signature' '
+	git show -s initial >commit &&
+	git show -s --show-signature initial >show &&
+	git verify-commit -v initial >verify.1 2>verify.2 &&
+	git cat-file commit initial >cat &&
+	grep -v "gpg: " show >show.commit &&
+	grep "gpg: " show >show.gpg &&
+	grep -v "^ " cat | grep -v "^gpgsig " >cat.commit &&
+	test_cmp show.commit commit &&
+	test_cmp show.gpg verify.2 &&
+	test_cmp cat.commit verify.1
+'
+
 test_expect_success GPG 'detect fudged signature' '
 	git cat-file commit master >raw &&
 
 	sed -e "s/seventh/7th forged/" raw >forged1 &&
 	git hash-object -w -t commit forged1 >forged1.commit &&
+	! git verify-commit $(cat forged1.commit) &&
 	git show --pretty=short --show-signature $(cat forged1.commit) >actual1 &&
 	grep "BAD signature from" actual1 &&
 	! grep "Good signature from" actual1
@@ -81,6 +97,7 @@ test_expect_success GPG 'detect fudged signature with NUL' '
 	cat raw >forged2 &&
 	echo Qwik | tr "Q" "\000" >>forged2 &&
 	git hash-object -w -t commit forged2 >forged2.commit &&
+	! git verify-commit $(cat forged2.commit) &&
 	git show --pretty=short --show-signature $(cat forged2.commit) >actual2 &&
 	grep "BAD signature from" actual2 &&
 	! grep "Good signature from" actual2
@@ -89,6 +106,7 @@ test_expect_success GPG 'detect fudged signature with NUL' '
 test_expect_success GPG 'amending already signed commit' '
 	git checkout fourth-signed^0 &&
 	git commit --amend -S --no-edit &&
+	git verify-commit HEAD &&
 	git show -s --show-signature HEAD >actual &&
 	grep "Good signature from" actual &&
 	! grep "BAD signature from" actual
-- 
2.0.0.426.g37dbf84

Re: [PATCHv3 5/5] t7510: test verify-commit

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

>  test_expect_success GPG 'detect fudged signature' '
>  	git cat-file commit master >raw &&
>  
>  	sed -e "s/seventh/7th forged/" raw >forged1 &&
>  	git hash-object -w -t commit forged1 >forged1.commit &&
> +	! git verify-commit $(cat forged1.commit) &&

This should be "test_must_fail git verify-commit ...", I think.
Otherwise you would end up declaring a segfaulting implementation a
good one.

> +	! git verify-commit $(cat forged2.commit) &&

Likewise.

Re: [PATCHv3 0/5] verify-commit: verify commit signatures

[Jeff King]

On Mon, Jun 23, 2014 at 09:05:46AM +0200, Michael J Gruber wrote:

> This incorporates all remarks about the test coding guidelines and
> rearranging the series.
> 
> Open questions:
> - There was some debate about (optionally) verifying more than what
> git-verify-{commit,tag} currently do, or going for a generic git-verify command.
> The former would require both to be changed (in order to treat similar cases similarly),
> the latter would need a deprecation for git-verify-tag.

I think that a potential "git verify" doesn't need to block this series,
per the logic I gave elsewhere.

The one thing that does give me pause is that we do not seem to have any
way of accessing mergetag signatures. We should perhaps stop and think
for a second about how we might expose those (and whether it would fit
into the "git-verify-{commit,tag}" paradigm). I am tempted to say that
"git verify-tag" on a commit should verify the mergetag (right now it
would simply be an error). But I haven't though that hard on it.

I don't think implementation of that needs to be a blocker for this
series, but I'd rather see at least a vague plan so that we do not paint
ourselves into a corner.

> Michael J Gruber (5):
>   gpg-interface: provide clear helper for struct signature_check
>   gpg-interface: provide access to the payload
>   verify-commit: scriptable commit signature verification
>   t7510: exit for loop with test result
>   t7510: test verify-commit

I didn't see anything objectionable here. I think you may want to rebase
on top of jk/pretty-G-format-fixes. That makes your patch 4 redundant,
and your patch 5 will probably need a few minor updates to match
cleanups in the surrounding code.

-Peff

Re: [PATCHv3 0/5] verify-commit: verify commit signatures

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Jun 23, 2014 at 09:05:46AM +0200, Michael J Gruber wrote:
>
>> This incorporates all remarks about the test coding guidelines and
>> rearranging the series.
>> 
>> Open questions:
>> - There was some debate about (optionally) verifying more than what
>> git-verify-{commit,tag} currently do, or going for a generic git-verify command.
>> The former would require both to be changed (in order to treat similar cases similarly),
>> the latter would need a deprecation for git-verify-tag.
>
> I think that a potential "git verify" doesn't need to block this series,
> per the logic I gave elsewhere.
>
> The one thing that does give me pause is that we do not seem to have any
> way of accessing mergetag signatures. We should perhaps stop and think
> for a second about how we might expose those (and whether it would fit
> into the "git-verify-{commit,tag}" paradigm). I am tempted to say that
> "git verify-tag" on a commit should verify the mergetag (right now it
> would simply be an error). But I haven't though that hard on it.

I agree that "verify-commit" that lives next to "verify-tag" is fine
and does not have to wait for a unified "verify" that may not even
be a good idea, but if we were to verify the mergetags in one of
these "verify-$OBJECTTYPE" commands, I would think "verify-commit"
should be the one to check them, for the simple reason that they
appear in "commit" objects, not in "tag" objects.

I would imagine that I would not mind too much if "verify-tag"
delegated the verification to "verify-commit" automatically when it
is given a commit object, but for a command fairly low-level to be
useful for scripting, such a DWIMmage may be too unexpected and make
them unnecessarily unreliable.  Using scripts that want strictness
(and who in the right mind that wants to verify things do not want
strictness?) would need to "cat-file -t" upfront to switch on the
object type.

Re: [PATCHv3 0/5] verify-commit: verify commit signatures

[Jeff King]

On Mon, Jun 23, 2014 at 10:52:38AM -0700, Junio C Hamano wrote:

> > The one thing that does give me pause is that we do not seem to have any
> > way of accessing mergetag signatures. We should perhaps stop and think
> > for a second about how we might expose those (and whether it would fit
> > into the "git-verify-{commit,tag}" paradigm). I am tempted to say that
> > "git verify-tag" on a commit should verify the mergetag (right now it
> > would simply be an error). But I haven't though that hard on it.
> 
> I agree that "verify-commit" that lives next to "verify-tag" is fine
> and does not have to wait for a unified "verify" that may not even
> be a good idea, but if we were to verify the mergetags in one of
> these "verify-$OBJECTTYPE" commands, I would think "verify-commit"
> should be the one to check them, for the simple reason that they
> appear in "commit" objects, not in "tag" objects.

My thinking was the opposite: it is a signature on a tag, but that
signature happens to be stuffed into a commit object. But I do not have
a strong feeling either way.

-Peff

Re: [PATCHv3 0/5] verify-commit: verify commit signatures

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Jun 23, 2014 at 10:52:38AM -0700, Junio C Hamano wrote:
>
>> > The one thing that does give me pause is that we do not seem to have any
>> > way of accessing mergetag signatures. We should perhaps stop and think
>> > for a second about how we might expose those (and whether it would fit
>> > into the "git-verify-{commit,tag}" paradigm). I am tempted to say that
>> > "git verify-tag" on a commit should verify the mergetag (right now it
>> > would simply be an error). But I haven't though that hard on it.
>> 
>> I agree that "verify-commit" that lives next to "verify-tag" is fine
>> and does not have to wait for a unified "verify" that may not even
>> be a good idea, but if we were to verify the mergetags in one of
>> these "verify-$OBJECTTYPE" commands, I would think "verify-commit"
>> should be the one to check them, for the simple reason that they
>> appear in "commit" objects, not in "tag" objects.
>
> My thinking was the opposite: it is a signature on a tag, but that
> signature happens to be stuffed into a commit object. But I do not have
> a strong feeling either way.

Well, the whole point of storing mergetag inside commit objects is
so that these transient "please pull, here is a tag to prove you
that it is from me" tags do not have to be kept in the history;
hence people who are following along only see commits and not tags.
The signature being sent via a signed tag from the requestor to the
integrator is merely an implementation detail after the mergetag is
recorded and when people would want to verify them.

So...

[PATCHv4 0/4] verify-commit: verify commit signatures

[Michael J Gruber]

This is v3 rebased on current next (the %G works by Jeff & Junio).

Open questions:

- Should one of git verify-{commit,tag} learn how to verify mergetags?
(Probably no, it differs from both other cases.)

- Should we do this now or go for generic "git verify" right away?
That depends on whether signed commits need to be verified by scripts now,
or whether mergetags are more important.

For a general command which allows different verification policies,
I'm still wondering whether we may need hooks which receive all
the relevant information in the environment. Otherwise we'll have a ton of
options such as --match-committer-uid, --verify--AllParentsHaveMergeTags,
--verify--All-ParentsAreSignedCommits, --peel-to-commit, --merge-commit-only, ...

I imagine that a generic "git verify" would provide "git verify-{commit,tag}"
aliases which call "git verify" with options that reproduce the current (suggested)
behavior.

Michael J Gruber (4):
  gpg-interface: provide clear helper for struct signature_check
  gpg-interface: provide access to the payload
  verify-commit: scriptable commit signature verification
  t7510: test verify-commit

 Documentation/git-verify-commit.txt | 28 +++++++++++
 Makefile                            |  1 +
 builtin.h                           |  1 +
 builtin/merge.c                     |  5 +-
 builtin/verify-commit.c             | 93 +++++++++++++++++++++++++++++++++++++
 command-list.txt                    |  1 +
 commit.c                            |  1 +
 git.c                               |  1 +
 gpg-interface.c                     | 14 ++++++
 gpg-interface.h                     |  2 +
 pretty.c                            |  3 +-
 t/t7510-signed-commit.sh            | 20 +++++++-
 12 files changed, 163 insertions(+), 7 deletions(-)
 create mode 100644 Documentation/git-verify-commit.txt
 create mode 100644 builtin/verify-commit.c

-- 
2.0.1.563.g162087b.dirty

[PATCHv4 1/4] gpg-interface: provide clear helper for struct signature_check

[Michael J Gruber]

The struct has been growing members whose malloced memory needs to be
freed. Do this with one helper function so that no malloced memory shall
be left unfreed.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 builtin/merge.c |  5 +----
 gpg-interface.c | 12 ++++++++++++
 gpg-interface.h |  1 +
 pretty.c        |  3 +--
 4 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index b49c310..86e9c61 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1282,10 +1282,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 				printf(_("Commit %s has a good GPG signature by %s\n"),
 				       hex, signature_check.signer);
 
-			free(signature_check.gpg_output);
-			free(signature_check.gpg_status);
-			free(signature_check.signer);
-			free(signature_check.key);
+			signature_check_clear(&signature_check);
 		}
 	}
 
diff --git a/gpg-interface.c b/gpg-interface.c
index 8b0e874..e71b59d 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -7,6 +7,18 @@
 static char *configured_signing_key;
 static const char *gpg_program = "gpg";
 
+void signature_check_clear(struct signature_check *sigc)
+{
+	free(sigc->gpg_output);
+	free(sigc->gpg_status);
+	free(sigc->signer);
+	free(sigc->key);
+	sigc->gpg_output = NULL;
+	sigc->gpg_status = NULL;
+	sigc->signer = NULL;
+	sigc->key = NULL;
+}
+
 void set_signing_key(const char *key)
 {
 	free(configured_signing_key);
diff --git a/gpg-interface.h b/gpg-interface.h
index a85cb5b..9f0784a 100644
--- a/gpg-interface.h
+++ b/gpg-interface.h
@@ -13,6 +13,7 @@ struct signature_check {
 	char *key;
 };
 
+extern void signature_check_clear(struct signature_check *sigc);
 extern int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *signing_key);
 extern int verify_signed_buffer(const char *payload, size_t payload_size, const char *signature, size_t signature_size, struct strbuf *gpg_output, struct strbuf *gpg_status);
 extern int git_gpg_config(const char *, const char *, void *);
diff --git a/pretty.c b/pretty.c
index ce3bbaa..353af81 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1520,8 +1520,7 @@ void format_commit_message(const struct commit *commit,
 
 	free(context.commit_encoding);
 	unuse_commit_buffer(commit, context.message);
-	free(context.signature_check.gpg_output);
-	free(context.signature_check.signer);
+	signature_check_clear(&context.signature_check);
 }
 
 static void pp_header(struct pretty_print_context *pp,
-- 
2.0.1.563.g162087b.dirty

[PATCHv4 2/4] gpg-interface: provide access to the payload

[Michael J Gruber]

In contrast to tag signatures, commit signatures are put into the
header, that is between the other header parts and commit messages.

Provide access to the commit content sans the signature, which is the
payload that is actually signed. Commit signature verification does the
parsing anyways, and callers may wish to act on or display the commit
object sans the signature.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 commit.c        | 1 +
 gpg-interface.c | 2 ++
 gpg-interface.h | 1 +
 3 files changed, 4 insertions(+)

diff --git a/commit.c b/commit.c
index fb7897c..acb74b5 100644
--- a/commit.c
+++ b/commit.c
@@ -1270,6 +1270,7 @@ void check_commit_signature(const struct commit* commit, struct signature_check
 				      &gpg_output, &gpg_status);
 	if (status && !gpg_output.len)
 		goto out;
+	sigc->payload = strbuf_detach(&payload, NULL);
 	sigc->gpg_output = strbuf_detach(&gpg_output, NULL);
 	sigc->gpg_status = strbuf_detach(&gpg_status, NULL);
 	parse_gpg_output(sigc);
diff --git a/gpg-interface.c b/gpg-interface.c
index e71b59d..ff07012 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -9,10 +9,12 @@ static const char *gpg_program = "gpg";
 
 void signature_check_clear(struct signature_check *sigc)
 {
+	free(sigc->payload);
 	free(sigc->gpg_output);
 	free(sigc->gpg_status);
 	free(sigc->signer);
 	free(sigc->key);
+	sigc->payload = NULL;
 	sigc->gpg_output = NULL;
 	sigc->gpg_status = NULL;
 	sigc->signer = NULL;
diff --git a/gpg-interface.h b/gpg-interface.h
index 9f0784a..37c23da 100644
--- a/gpg-interface.h
+++ b/gpg-interface.h
@@ -2,6 +2,7 @@
 #define GPG_INTERFACE_H
 
 struct signature_check {
+	char *payload;
 	char *gpg_output;
 	char *gpg_status;
 	char result; /* 0 (not checked),
-- 
2.0.1.563.g162087b.dirty

[PATCHv4 3/4] verify-commit: scriptable commit signature verification

[Michael J Gruber]

Commit signatures can be verified using "git show -s --show-signature"
or the "%G?" pretty format and parsing the output, which is well suited
for user inspection, but not for scripting.

Provide a command "verify-commit" which is analogous to "verify-tag": It
returns 0 for good signatures and non-zero otherwise, has the gpg output
on stderr and (optionally) the commit object on stdout, sans the
signature, just like "verify-tag" does.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 Documentation/git-verify-commit.txt | 28 +++++++++++
 Makefile                            |  1 +
 builtin.h                           |  1 +
 builtin/verify-commit.c             | 93 +++++++++++++++++++++++++++++++++++++
 command-list.txt                    |  1 +
 git.c                               |  1 +
 6 files changed, 125 insertions(+)
 create mode 100644 Documentation/git-verify-commit.txt
 create mode 100644 builtin/verify-commit.c

diff --git a/Documentation/git-verify-commit.txt b/Documentation/git-verify-commit.txt
new file mode 100644
index 0000000..9413e28
--- /dev/null
+++ b/Documentation/git-verify-commit.txt
@@ -0,0 +1,28 @@
+git-verify-commit(1)
+====================
+
+NAME
+----
+git-verify-commit - Check the GPG signature of commits
+
+SYNOPSIS
+--------
+[verse]
+'git verify-commit' <commit>...
+
+DESCRIPTION
+-----------
+Validates the gpg signature created by 'git commit -S'.
+
+OPTIONS
+-------
+-v::
+--verbose::
+	Print the contents of the commit object before validating it.
+
+<commit>...::
+	SHA-1 identifiers of Git commit objects.
+
+GIT
+---
+Part of the linkgit:git[1] suite
diff --git a/Makefile b/Makefile
index 07ea105..b92418d 100644
--- a/Makefile
+++ b/Makefile
@@ -999,6 +999,7 @@ BUILTIN_OBJS += builtin/update-ref.o
 BUILTIN_OBJS += builtin/update-server-info.o
 BUILTIN_OBJS += builtin/upload-archive.o
 BUILTIN_OBJS += builtin/var.o
+BUILTIN_OBJS += builtin/verify-commit.o
 BUILTIN_OBJS += builtin/verify-pack.o
 BUILTIN_OBJS += builtin/verify-tag.o
 BUILTIN_OBJS += builtin/write-tree.o
diff --git a/builtin.h b/builtin.h
index c47c110..5d91f31 100644
--- a/builtin.h
+++ b/builtin.h
@@ -128,6 +128,7 @@ extern int cmd_update_server_info(int argc, const char **argv, const char *prefi
 extern int cmd_upload_archive(int argc, const char **argv, const char *prefix);
 extern int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix);
 extern int cmd_var(int argc, const char **argv, const char *prefix);
+extern int cmd_verify_commit(int argc, const char **argv, const char *prefix);
 extern int cmd_verify_tag(int argc, const char **argv, const char *prefix);
 extern int cmd_version(int argc, const char **argv, const char *prefix);
 extern int cmd_whatchanged(int argc, const char **argv, const char *prefix);
diff --git a/builtin/verify-commit.c b/builtin/verify-commit.c
new file mode 100644
index 0000000..b0f8504
--- /dev/null
+++ b/builtin/verify-commit.c
@@ -0,0 +1,93 @@
+/*
+ * Builtin "git commit-commit"
+ *
+ * Copyright (c) 2014 Michael J Gruber <git@drmicha.warpmail.net>
+ *
+ * Based on git-verify-tag
+ */
+#include "cache.h"
+#include "builtin.h"
+#include "commit.h"
+#include "run-command.h"
+#include <signal.h>
+#include "parse-options.h"
+#include "gpg-interface.h"
+
+static const char * const verify_commit_usage[] = {
+		N_("git verify-commit [-v|--verbose] <commit>..."),
+		NULL
+};
+
+static int run_gpg_verify(const unsigned char *sha1, const char *buf, unsigned long size, int verbose)
+{
+	struct signature_check signature_check;
+
+	memset(&signature_check, 0, sizeof(signature_check));
+
+	check_commit_signature(lookup_commit(sha1), &signature_check);
+
+	if (verbose && signature_check.payload)
+		fputs(signature_check.payload, stdout);
+
+	if (signature_check.gpg_output)
+		fputs(signature_check.gpg_output, stderr);
+
+	signature_check_clear(&signature_check);
+	return signature_check.result != 'G';
+}
+
+static int verify_commit(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))
+		return error("commit '%s' not found.", name);
+
+	buf = read_sha1_file(sha1, &type, &size);
+	if (!buf)
+		return error("%s: unable to read file.", name);
+	if (type != OBJ_COMMIT)
+		return error("%s: cannot verify a non-commit object of type %s.",
+				name, typename(type));
+
+	ret = run_gpg_verify(sha1, buf, size, verbose);
+
+	free(buf);
+	return ret;
+}
+
+static int git_verify_commit_config(const char *var, const char *value, void *cb)
+{
+	int status = git_gpg_config(var, value, cb);
+	if (status)
+		return status;
+	return git_default_config(var, value, cb);
+}
+
+int cmd_verify_commit(int argc, const char **argv, const char *prefix)
+{
+	int i = 1, verbose = 0, had_error = 0;
+	const struct option verify_commit_options[] = {
+		OPT__VERBOSE(&verbose, N_("print commit contents")),
+		OPT_END()
+	};
+
+	git_config(git_verify_commit_config, NULL);
+
+	argc = parse_options(argc, argv, prefix, verify_commit_options,
+			     verify_commit_usage, PARSE_OPT_KEEP_ARGV0);
+	if (argc <= i)
+		usage_with_options(verify_commit_usage, verify_commit_options);
+
+	/* 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_commit(argv[i++], verbose))
+			had_error = 1;
+	return had_error;
+}
diff --git a/command-list.txt b/command-list.txt
index cf36c3d..a3ff0c9 100644
--- a/command-list.txt
+++ b/command-list.txt
@@ -132,6 +132,7 @@ git-update-server-info                  synchingrepositories
 git-upload-archive                      synchelpers
 git-upload-pack                         synchelpers
 git-var                                 plumbinginterrogators
+git-verify-commit                       ancillaryinterrogators
 git-verify-pack                         plumbinginterrogators
 git-verify-tag                          ancillaryinterrogators
 gitweb                                  ancillaryinterrogators
diff --git a/git.c b/git.c
index dd54f57..5b6c761 100644
--- a/git.c
+++ b/git.c
@@ -478,6 +478,7 @@ static struct cmd_struct commands[] = {
 	{ "upload-archive", cmd_upload_archive },
 	{ "upload-archive--writer", cmd_upload_archive_writer },
 	{ "var", cmd_var, RUN_SETUP_GENTLY },
+	{ "verify-commit", cmd_verify_commit, RUN_SETUP },
 	{ "verify-pack", cmd_verify_pack },
 	{ "verify-tag", cmd_verify_tag, RUN_SETUP },
 	{ "version", cmd_version },
-- 
2.0.1.563.g162087b.dirty

[PATCHv4 4/4] t7510: test verify-commit

[Michael J Gruber]

This mixes the "git verify-commit" tests in with the "git show
--show-signature" tests, to keep the tests more readable.

The tests already mix in the "call show" tests with the "verify" tests.
So in case of a test beakage, a '-v' run would be needed to reveal the
exact point of breakage anyway.

Additionally, test the actual output of "git verify-commit" and "git
show --show-signature" and compare to "git cat-file".

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 t/t7510-signed-commit.sh | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
index e97477a..474dab3 100755
--- a/t/t7510-signed-commit.sh
+++ b/t/t7510-signed-commit.sh
@@ -48,10 +48,11 @@ test_expect_success GPG 'create signed commits' '
 	git tag eighth-signed-alt
 '
 
-test_expect_success GPG 'show signatures' '
+test_expect_success GPG 'verify and show signatures' '
 	(
 		for commit in initial second merge fourth-signed fifth-signed sixth-signed seventh-signed
 		do
+			git verify-commit $commit &&
 			git show --pretty=short --show-signature $commit >actual &&
 			grep "Good signature from" actual &&
 			! grep "BAD signature from" actual &&
@@ -61,6 +62,7 @@ test_expect_success GPG 'show signatures' '
 	(
 		for commit in merge^2 fourth-unsigned sixth-unsigned seventh-unsigned
 		do
+			test_must_fail git verify-commit $commit &&
 			git show --pretty=short --show-signature $commit >actual &&
 			! grep "Good signature from" actual &&
 			! grep "BAD signature from" actual &&
@@ -79,11 +81,25 @@ test_expect_success GPG 'show signatures' '
 	)
 '
 
+test_expect_success GPG 'show signed commit with signature' '
+	git show -s initial >commit &&
+	git show -s --show-signature initial >show &&
+	git verify-commit -v initial >verify.1 2>verify.2 &&
+	git cat-file commit initial >cat &&
+	grep -v "gpg: " show >show.commit &&
+	grep "gpg: " show >show.gpg &&
+	grep -v "^ " cat | grep -v "^gpgsig " >cat.commit &&
+	test_cmp show.commit commit &&
+	test_cmp show.gpg verify.2 &&
+	test_cmp cat.commit verify.1
+'
+
 test_expect_success GPG 'detect fudged signature' '
 	git cat-file commit seventh-signed >raw &&
 
 	sed -e "s/seventh/7th forged/" raw >forged1 &&
 	git hash-object -w -t commit forged1 >forged1.commit &&
+	! git verify-commit $(cat forged1.commit) &&
 	git show --pretty=short --show-signature $(cat forged1.commit) >actual1 &&
 	grep "BAD signature from" actual1 &&
 	! grep "Good signature from" actual1
@@ -94,6 +110,7 @@ test_expect_success GPG 'detect fudged signature with NUL' '
 	cat raw >forged2 &&
 	echo Qwik | tr "Q" "\000" >>forged2 &&
 	git hash-object -w -t commit forged2 >forged2.commit &&
+	! git verify-commit $(cat forged2.commit) &&
 	git show --pretty=short --show-signature $(cat forged2.commit) >actual2 &&
 	grep "BAD signature from" actual2 &&
 	! grep "Good signature from" actual2
@@ -102,6 +119,7 @@ test_expect_success GPG 'detect fudged signature with NUL' '
 test_expect_success GPG 'amending already signed commit' '
 	git checkout fourth-signed^0 &&
 	git commit --amend -S --no-edit &&
+	git verify-commit HEAD &&
 	git show -s --show-signature HEAD >actual &&
 	grep "Good signature from" actual &&
 	! grep "BAD signature from" actual
-- 
2.0.1.563.g162087b.dirty

Re: [PATCHv4 4/4] t7510: test verify-commit

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

>  	sed -e "s/seventh/7th forged/" raw >forged1 &&
>  	git hash-object -w -t commit forged1 >forged1.commit &&
> +	! git verify-commit $(cat forged1.commit) &&

test_must_fail git verify-commit ... &&

>  	git show --pretty=short --show-signature $(cat forged1.commit) >actual1 &&
>  	grep "BAD signature from" actual1 &&
>  	! grep "Good signature from" actual1
> @@ -94,6 +110,7 @@ test_expect_success GPG 'detect fudged signature with NUL' '
>  	cat raw >forged2 &&
>  	echo Qwik | tr "Q" "\000" >>forged2 &&
>  	git hash-object -w -t commit forged2 >forged2.commit &&
> +	! git verify-commit $(cat forged2.commit) &&

test_must_fail git verify-commit ... &&

>  	git show --pretty=short --show-signature $(cat forged2.commit) >actual2 &&
>  	grep "BAD signature from" actual2 &&
>  	! grep "Good signature from" actual2
> @@ -102,6 +119,7 @@ test_expect_success GPG 'detect fudged signature with NUL' '
>  test_expect_success GPG 'amending already signed commit' '
>  	git checkout fourth-signed^0 &&
>  	git commit --amend -S --no-edit &&
> +	git verify-commit HEAD &&
>  	git show -s --show-signature HEAD >actual &&
>  	grep "Good signature from" actual &&
>  	! grep "BAD signature from" actual

Most of the tests, unlike "git show --show-signature" tests, do not
seem to check the output from the command.  Is it because its
primary interface to scripts is its exit status [*1*]?

[Footnote]

*1* "Yes" is totally an acceptable answer and a justification for
not checking the output in many of these tests.

Re: [PATCHv4 4/4] t7510: test verify-commit

[Michael J Gruber]

On 27. Juni 2014 21:32:30 MESZ, Junio C Hamano <gitster@pobox.com> wrote:
>Michael J Gruber <git@drmicha.warpmail.net> writes:
>
>>  	sed -e "s/seventh/7th forged/" raw >forged1 &&
>>  	git hash-object -w -t commit forged1 >forged1.commit &&
>> +	! git verify-commit $(cat forged1.commit) &&
>
>test_must_fail git verify-commit ... &&
>
>>  	git show --pretty=short --show-signature $(cat forged1.commit)
>>actual1 &&
>>  	grep "BAD signature from" actual1 &&
>>  	! grep "Good signature from" actual1
>> @@ -94,6 +110,7 @@ test_expect_success GPG 'detect fudged signature
>with NUL' '
>>  	cat raw >forged2 &&
>>  	echo Qwik | tr "Q" "\000" >>forged2 &&
>>  	git hash-object -w -t commit forged2 >forged2.commit &&
>> +	! git verify-commit $(cat forged2.commit) &&
>
>test_must_fail git verify-commit ... &&

Sorry for missing or mis-rebasing these. I meant to cover them all.

>
>>  	git show --pretty=short --show-signature $(cat forged2.commit)
>>actual2 &&
>>  	grep "BAD signature from" actual2 &&
>>  	! grep "Good signature from" actual2
>> @@ -102,6 +119,7 @@ test_expect_success GPG 'detect fudged signature
>with NUL' '
>>  test_expect_success GPG 'amending already signed commit' '
>>  	git checkout fourth-signed^0 &&
>>  	git commit --amend -S --no-edit &&
>> +	git verify-commit HEAD &&
>>  	git show -s --show-signature HEAD >actual &&
>>  	grep "Good signature from" actual &&
>>  	! grep "BAD signature from" actual
>
>Most of the tests, unlike "git show --show-signature" tests, do not
>seem to check the output from the command.  Is it because its
>primary interface to scripts is its exit status [*1*]?
>
>[Footnote]
>
>*1* "Yes" is totally an acceptable answer and a justification for
>not checking the output in many of these tests.

Yes, the idea was to check the exit status in all cases (the loopy subtests) and the textual output in a few exemplary ones. I didn't want to bloat the test unnecessarily.

Michael

Re: [PATCHv4 0/4] verify-commit: verify commit signatures

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

> This is v3 rebased on current next (the %G works by Jeff & Junio).
>
> Open questions:
>
> - Should one of git verify-{commit,tag} learn how to verify mergetags?
> (Probably no, it differs from both other cases.)

If we were to teach one of them, "verify-commit" as part of
"verifying what is recorded in the commit object", would be the
logical place to do so.

It is OK to implement only verification of signatures on commit
objects themselves, but we would need a plan for handling other
kinds of verifications later, so that we can give a stable output to
scripts.

If we decide to signal successful verification of the signature on
the commit itself one way in this implementation, that should
reliably be the way to do so even if we later add verification of
other aspects on the commit object (e.g. mergetags it carries).  If
running "verify-commit $commit" and checking the zero-ness of its
exit status is the way, that should not change if later versions of
Git learns to verify mergetags as well; even if the given $commit
carries a mergetag that does not verify, as long as the signature in
the commit itself is valid, the script should continue to receive
"success" from the command.

> - Should we do this now or go for generic "git verify" right away?

I do not think we are ready to do "git verify" yet.  We first need
to design how "verify-commit" should communicate failure/success
combinations of verification of a commit that has a signature on
itself and a mergetag on one of its parents, and of a commit that
has zero or one signature on itself and two or more mergetags on its
parents.  Do we fail unless all of them are found to be valid?  Do
we use bits in exit status?  Output to the standard output, one line
per signature verified?

Re: [PATCHv4 0/4] verify-commit: verify commit signatures

[Jeff King]

On Fri, Jun 27, 2014 at 12:07:35PM -0700, Junio C Hamano wrote:

> > - Should we do this now or go for generic "git verify" right away?
> 
> I do not think we are ready to do "git verify" yet.

If there is one thing that this discussion have convinced me of, it is
this. :)

-Peff

Re: [PATCHv4 0/4] verify-commit: verify commit signatures

[Jeff King]

On Fri, Jun 27, 2014 at 04:13:22PM +0200, Michael J Gruber wrote:

> This is v3 rebased on current next (the %G works by Jeff & Junio).

Aside from the minor test issues Junio pointed out, I think this version
looks OK.

> For a general command which allows different verification policies,
> I'm still wondering whether we may need hooks which receive all
> the relevant information in the environment. Otherwise we'll have a ton of
> options such as --match-committer-uid, --verify--AllParentsHaveMergeTags,
> --verify--All-ParentsAreSignedCommits, --peel-to-commit, --merge-commit-only, ...

I'd guess these would be project policies that could go in config. But I
do not think your series needs to be concerned with that yet.

-Peff