verify-commit: verify commit signatures

Quoted 9 lines

builtin/merge.c +1 -0

[...]
+			free(signature_check.payload);
 			free(signature_check.gpg_output);
 			free(signature_check.gpg_status);
 			free(signature_check.signer);
[...]

pretty.c void format_commit_message(const struct commit *commit, · line 1543

Quoted 5 lines

pretty.c +1 -0

[...]
+	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?

Quoted 1 line

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?

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 <[email protected]>

---

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

Documentation/git-verify-commit.txt new file +0 -0

new file mode 100644
index 0000000..dcd7803

Documentation/git-verify-commit.txt new file +26 -0

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

Makefile +0 -0

index 07ea105..b92418d 100644
@@ -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

builtin.h +0 -0

index c47c110..5d91f31 100644
@@ -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);

builtin/verify-commit.c new file +21 -0

new file mode 100644
index 0000000..69b7c6d
@@ -0,0 +1,98 @@
+/*
+ * Builtin "git commit-commit"
+ *
+ * Copyright (c) 2014 Michael J Gruber <[email protected]>
+ *
+ * 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)

builtin/verify-commit.c new file +47 -0

+{
+	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)

builtin/verify-commit.c new file +30 -0

+{
+	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;
+}

command-list.txt +0 -0

index cf36c3d..a3ff0c9 100644
@@ -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

git.c +1 -0

index 7780572..bbc3293 100644
@@ -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 },

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 <[email protected]>

Quoted 10 lines

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.

Quoted 1 line

Signed-off-by Michael J Gruber <[email protected]>

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

Quoted 2 lines

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.

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 <[email protected]> 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.

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.

Quoted 3 lines · Jeff King

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?

Quoted 3 lines · Jeff King

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?

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

Quoted 1 line

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?

Quoted 5 lines

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

Quoted 6 lines

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

Quoted 6 lines

Quoted 6 lines

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

Quoted 8 lines

Quoted 6 lines

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.

Jeff King venit, vidit, dixit 16.06.2014 22:39:

Quoted 20 lines · Junio C Hamano

Quoted 6 lines

Quoted 6 lines

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

Quoted 8 lines

Quoted 6 lines

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.

Quoted 6 lines

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

Michael J Gruber venit, vidit, dixit 27.06.2014 14:31:

Quoted 34 lines

Jeff King venit, vidit, dixit 16.06.2014 22:39:

Quoted 20 lines · Junio C Hamano

Quoted 6 lines

Quoted 6 lines

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

Quoted 8 lines

Quoted 6 lines

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.

Quoted 6 lines

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).

Quoted 6 lines

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

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

Fix it.

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.

Michael J Gruber venit, vidit, dixit 27.06.2014 14:31:

Quoted 34 lines

Jeff King venit, vidit, dixit 16.06.2014 22:39:

Quoted 20 lines · Junio C Hamano

Quoted 6 lines

Quoted 6 lines

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

Quoted 8 lines

Quoted 6 lines

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.

Quoted 6 lines

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).

Quoted 6 lines

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

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

The commit could carry it's own commit signature, couldn't it?

Quoted 3 lines · Michael J Gruber

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.

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.

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.

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 <[email protected]>

---
 pretty.c | 1 +
 1 file changed, 1 insertion(+)

pretty.c +0 -0

index 4f51287..f1e8a70 100644

builtin/verify-commit.c line 40

Quoted 8 lines

builtin/verify-commit.c new file +7 -0

+
+	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?

builtin/verify-commit.c line 60

Quoted 8 lines

builtin/verify-commit.c new file +8 -0

+	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

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.

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.

Quoted 3 lines

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).

Quoted 2 lines

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

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 <[email protected]>

---
 t/t7510-signed-commit.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

t/t7510-signed-commit.sh +0 -0

index 5ddac1a..a5ba48e 100755

Quoted 17 lines

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 <[email protected]>

---
 t/t7510-signed-commit.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

t/t7510-signed-commit.sh +0 -0

index 5ddac1a..a5ba48e 100755

t/t7510-signed-commit.sh test_expect_success GPG 'show signatures' ' · line 53

Quoted 6 lines

t/t7510-signed-commit.sh +1 -1

@@ -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.

Jeff King venit, vidit, dixit 13.06.2014 13:46:

Quoted 46 lines · Michael J Gruber

Quoted 17 lines

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 <[email protected]>

---
 t/t7510-signed-commit.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

t/t7510-signed-commit.sh +0 -0

index 5ddac1a..a5ba48e 100755

t/t7510-signed-commit.sh test_expect_success GPG 'show signatures' ' · line 53

Quoted 6 lines

t/t7510-signed-commit.sh +1 -1

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

Quoted 5 lines

(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 J Gruber venit, vidit, dixit 13.06.2014 14:04:

Quoted 53 lines

Jeff King venit, vidit, dixit 13.06.2014 13:46:

Quoted 46 lines · Michael J Gruber

Quoted 17 lines

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 <[email protected]>

---
 t/t7510-signed-commit.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

t/t7510-signed-commit.sh +0 -0

index 5ddac1a..a5ba48e 100755

t/t7510-signed-commit.sh test_expect_success GPG 'show signatures' ' · line 53

Quoted 6 lines

t/t7510-signed-commit.sh +1 -1

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

Quoted 5 lines

(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.

Quoted 6 lines

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

Quoted 8 lines

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

.... 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)

Am 6/13/2014 14:33, schrieb Michael J Gruber:

Quoted 14 lines

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

Quoted 2 lines

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.

(quoted diff) +0 -0

 			! grep "BAD signature from" actual || exit 1
 			echo $commit OK

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

-- Hannes

Johannes Sixt venit, vidit, dixit 13.06.2014 14:54:

Quoted 15 lines

Am 6/13/2014 14:33, schrieb Michael J Gruber:

Quoted 14 lines

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

Quoted 8 lines

Quoted 2 lines

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:

Review test_expect_success GPG 'show signatures' ' · line 62

Quoted 9 lines

Ah, here it is:

(quoted diff) +1 -1

@@ -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.

Quoted 6 lines

(quoted diff) +0 -0

 			! 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."

Am 6/13/2014 15:06, schrieb Michael J Gruber:

Quoted 50 lines

Johannes Sixt venit, vidit, dixit 13.06.2014 14:54:

Quoted 15 lines

Am 6/13/2014 14:33, schrieb Michael J Gruber:

Quoted 14 lines

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

Quoted 8 lines

Quoted 2 lines

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:

Review test_expect_success GPG 'show signatures' ' · line 62

Quoted 9 lines

Ah, here it is:

(quoted diff) +1 -1

@@ -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.

Quoted 6 lines

(quoted diff) +0 -0

 			! 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

Quoted 8 lines

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

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).

t/t7510-signed-commit.sh test_expect_success GPG 'show signatures' ' · line 53

Quoted 6 lines

t/t7510-signed-commit.sh +1 -1

 	(
 		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.

t/t7510-signed-commit.sh test_expect_success GPG 'show signatures' ' · line 89

Quoted 7 lines

test_expect_success GPG 'detect fudged signature' '

t/t7510-signed-commit.sh +1 -0

 	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.

Jeff King venit, vidit, dixit 13.06.2014 13:51:

Quoted 11 lines · Michael J Gruber

t/t7510-signed-commit.sh test_expect_success GPG 'show signatures' ' · line 89

Quoted 7 lines

test_expect_success GPG 'detect fudged signature' '

t/t7510-signed-commit.sh +1 -0

 	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.

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.

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

Quoted 8 lines · 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 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.

Quoted 6 lines

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.

Quoted 7 lines · Junio C Hamano

Quoted 6 lines

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.

Quoted 6 lines

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.

t/t7510-signed-commit.sh test_expect_success GPG 'show signatures' ' · line 102

Quoted 3 lines · Michael J Gruber

t/t7510-signed-commit.sh +1 -0

 	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 ... &&

t/t7510-signed-commit.sh test_expect_success GPG 'detect fudged signature

Quoted 5 lines

git show --pretty=short --show-signature $(cat forged1.commit)
actual1 &&

t/t7510-signed-commit.sh +0 -0

 	grep "BAD signature from" actual1 &&
 	! grep "Good signature from" actual1

@@ -94,6 +110,7 @@ test_expect_success GPG 'detect fudged signature

with NUL' '

t/t7510-signed-commit.sh test_expect_success GPG 'detect fudged signature with NUL' ' · line 113

Quoted 4 lines

t/t7510-signed-commit.sh +1 -0

 	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 ... &&

t/t7510-signed-commit.sh test_expect_success GPG 'detect fudged signature

Quoted 6 lines

(quoted diff) +0 -0

 	git show --pretty=short --show-signature $(cat forged2.commit)

actual2 &&

t/t7510-signed-commit.sh +0 -0

 	grep "BAD signature from" actual2 &&
 	! grep "Good signature from" actual2

@@ -102,6 +119,7 @@ test_expect_success GPG 'detect fudged signature

with NUL' '

t/t7510-signed-commit.sh test_expect_success GPG 'detect fudged signature with NUL' ' · line 125

Quoted 7 lines

t/t7510-signed-commit.sh +1 -0

 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.

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?

Quoted 1 line

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

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

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, ...