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

7 messages, 3 authors, 2016-06-15 · open the first message on its own page

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

From: Junio C Hamano <hidden>
Date: 2016-06-15 23:01:44

Jeff King [off-list ref] writes:
On Mon, Jun 23, 2014 at 10:52:38AM -0700, Junio C Hamano wrote:
quoted
quoted
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

From: Michael J Gruber <hidden>
Date: 2016-06-15 23:01:45

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 2/4] gpg-interface: provide access to the payload

From: Michael J Gruber <hidden>
Date: 2016-06-15 23:01:45

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 <redacted>
---
 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

From: Michael J Gruber <hidden>
Date: 2016-06-15 23:01:45

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 <redacted>
---
 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

From: Michael J Gruber <hidden>
Date: 2016-06-15 23:01:45

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 <redacted>
---
 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

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

From: Michael J Gruber <hidden>
Date: 2016-06-15 23:01:45

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 <redacted>
---
 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

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

From: Jeff King <hidden>
Date: 2016-06-15 23:01:46

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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help