[PATCH 0/5] refactor gpg-interface and add gpg verification for clones

[PATCH 0/5] refactor gpg-interface and add gpg verification for clones

[Hans Jerry Illikainen]

This series starts off with refactor of print_signature_buffer() to make
all output conditional based on the 'flags' parameter.  The print
function is also extended to optionally show one-line summaries of
signature verifications (previously that functionality existed in
verify_merge_signature()).

The helper functions for signature verification of commits are then
refactored.  The new gpg_verify_commit() function is modelled after
gpg_verify_tag().  This allows us to remove verify_merge_signature() and
the file-local run_gpg_verify() (from the verify-commit builtin).  It
also allows us to change check_commit_signature() into a local function
in commit.c.

A new configuration option is also introduced, gpg.verifySignatures.
This allows users to enable signature verification for all operations
that support it.  Individual operations can then use
<operation>.verifySignatures for finer-grained control.

And finally, signature verification is added to the clone builtin.  It
obeys --(no-)verify-signatures, clone.verifySignatures and
gpg.verifySignatures (in decreasing order of significance).

A notable quirk with signature verification for clones is
--recurse-submodules.  As mentioned in the commit message, the current
workaround is to disable signature verification for submodules by
passing --no-verify-signatures in submodule--helper.c

I'm very much open to suggestions for a better approach of dealing with
recursive clones.  However, I don't think --verify-signatures from the
clone builtin should propagate to submodules, because that would break a
workflow where a user:

1. trust the hash function
2. has audited an unsigned repository at a given point
3. has added the repository at that point as a submodule
4. has signed an object in the super repository where the audited
   submodule is referenced

So, I think it'd make more sense to introduce a
submodule.verifySignatures config knob to be used by both
--recurse-submodules and when the 'submodule' command is used directly.

I hope this patch series isn't too confusing/all over the place. I
wasn't sure whether the preparatory patches would have made sense in
isolation, so I opted to send it all in one go.

Hans Jerry Illikainen (5):
  gpg-interface: conditionally show the result in
    print_signature_buffer()
  gpg-interface: support one-line summaries in print_signature_buffer()
  commit: refactor signature verification helpers
  merge: verify signatures if gpg.verifySignatures is true
  clone: support signature verification

 Documentation/config.txt           |   2 +
 Documentation/config/clone.txt     |   3 +
 Documentation/config/gpg.txt       |   6 +
 Documentation/config/merge.txt     |   4 +-
 Documentation/git-clone.txt        |   4 +
 builtin/clone.c                    |  46 ++++
 builtin/merge.c                    |  22 +-
 builtin/pull.c                     |  18 +-
 builtin/submodule--helper.c        |   6 +
 builtin/tag.c                      |   4 +-
 builtin/verify-commit.c            |  26 +-
 builtin/verify-tag.c               |   4 +-
 commit.c                           |  58 ++--
 commit.h                           |  31 +--
 gpg-interface.c                    |  43 ++-
 gpg-interface.h                    |  11 +-
 pretty.c                           |   3 +-
 t/t5619-clone-verify-signatures.sh | 411 +++++++++++++++++++++++++++++
 t/t7612-merge-verify-signatures.sh |  27 ++
 tag.c                              |  19 +-
 20 files changed, 633 insertions(+), 115 deletions(-)
 create mode 100644 Documentation/config/clone.txt
 create mode 100755 t/t5619-clone-verify-signatures.sh

--
2.25.0.rc1.302.gc71d20beed

[PATCH 1/5] gpg-interface: conditionally show the result in print_signature_buffer()

[Hans Jerry Illikainen]

The print_signature_buffer() function in gpg-interface.c is used to
print the result of a GPG verified payload.  It takes a 'flags'
parameter that determines what to print.

Previously, the 'flags' parameter processed 2 flags:

- GPG_VERIFY_RAW: to print the raw output from GPG instead of the
  human(ish)-readable output.  One of these outputs were always
  shown, irregardless of any other flags.
- GPG_VERIFY_VERBOSE: to print the payload that was verified

Interestingly, there was also a third flag defined in gpg-interface.h;
GPG_VERIFY_OMIT_STATUS.  That flag wasn't used by the print function
itself -- instead, callers would check for the presence of
GPG_VERIFY_OMIT_STATUS before invoking print_signature_buffer().

It seems reasonable that the GPG interface should handle all flags
related to how the result should be (or shouldn't be) shown.  This patch
implements that behavior by removing GPG_VERIFY_OMIT_STATUS and adding
GPG_VERIFY_FULL.  If neither GPG_VERIFY_FULL nor GPG_VERIFY_VERBOSE is
present, then nothing is printed.  This allows callers to invoke
print_signature_buffer() unconditionally.

Signed-off-by: Hans Jerry Illikainen <hji@dyntopia.com>
---
 builtin/tag.c           | 4 ++--
 builtin/verify-commit.c | 2 +-
 builtin/verify-tag.c    | 4 ++--
 gpg-interface.c         | 2 +-
 gpg-interface.h         | 6 +++---
 tag.c                   | 4 +---
 6 files changed, 10 insertions(+), 12 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index e0a4c25382..8489e220e8 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -112,10 +112,10 @@ static int verify_tag(const char *name, const char *ref,
 {
 	int flags;
 	const struct ref_format *format = cb_data;
-	flags = GPG_VERIFY_VERBOSE;
+	flags = GPG_VERIFY_FULL | GPG_VERIFY_VERBOSE;
 
 	if (format->format)
-		flags = GPG_VERIFY_OMIT_STATUS;
+		flags = 0;
 
 	if (gpg_verify_tag(oid, name, flags))
 		return -1;
diff --git a/builtin/verify-commit.c b/builtin/verify-commit.c
index 40c69a0bed..2a099ec6ba 100644
--- a/builtin/verify-commit.c
+++ b/builtin/verify-commit.c
@@ -63,7 +63,7 @@ static int git_verify_commit_config(const char *var, const char *value, void *cb
 int cmd_verify_commit(int argc, const char **argv, const char *prefix)
 {
 	int i = 1, verbose = 0, had_error = 0;
-	unsigned flags = 0;
+	unsigned flags = GPG_VERIFY_FULL;
 	const struct option verify_commit_options[] = {
 		OPT__VERBOSE(&verbose, N_("print commit contents")),
 		OPT_BIT(0, "raw", &flags, N_("print raw gpg status output"), GPG_VERIFY_RAW),
diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
index f45136a06b..bd5e99925b 100644
--- a/builtin/verify-tag.c
+++ b/builtin/verify-tag.c
@@ -30,7 +30,7 @@ static int git_verify_tag_config(const char *var, const char *value, void *cb)
 int cmd_verify_tag(int argc, const char **argv, const char *prefix)
 {
 	int i = 1, verbose = 0, had_error = 0;
-	unsigned flags = 0;
+	unsigned flags = GPG_VERIFY_FULL;
 	struct ref_format format = REF_FORMAT_INIT;
 	const struct option verify_tag_options[] = {
 		OPT__VERBOSE(&verbose, N_("print tag contents")),
@@ -53,7 +53,7 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
 		if (verify_ref_format(&format))
 			usage_with_options(verify_tag_usage,
 					   verify_tag_options);
-		flags |= GPG_VERIFY_OMIT_STATUS;
+		flags = 0;
 	}
 
 	while (i < argc) {
diff --git a/gpg-interface.c b/gpg-interface.c
index 2d538bcd6e..fc182d39be 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -341,7 +341,7 @@ void print_signature_buffer(const struct signature_check *sigc, unsigned flags)
 	if (flags & GPG_VERIFY_VERBOSE && sigc->payload)
 		fputs(sigc->payload, stdout);
 
-	if (output)
+	if (flags & GPG_VERIFY_FULL && output)
 		fputs(output, stderr);
 }
 
diff --git a/gpg-interface.h b/gpg-interface.h
index f4e9b4f371..4631a91330 100644
--- a/gpg-interface.h
+++ b/gpg-interface.h
@@ -3,9 +3,9 @@
 
 struct strbuf;
 
-#define GPG_VERIFY_VERBOSE		1
-#define GPG_VERIFY_RAW			2
-#define GPG_VERIFY_OMIT_STATUS	4
+#define GPG_VERIFY_VERBOSE (1 << 0)
+#define GPG_VERIFY_RAW (1 << 1)
+#define GPG_VERIFY_FULL (1 << 2)
 
 enum signature_trust_level {
 	TRUST_UNDEFINED,
diff --git a/tag.c b/tag.c
index 71b544467e..b8d6da81eb 100644
--- a/tag.c
+++ b/tag.c
@@ -28,9 +28,7 @@ static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags)
 
 	ret = check_signature(buf, payload_size, buf + payload_size,
 				size - payload_size, &sigc);
-
-	if (!(flags & GPG_VERIFY_OMIT_STATUS))
-		print_signature_buffer(&sigc, flags);
+	print_signature_buffer(&sigc, flags);
 
 	signature_check_clear(&sigc);
 	return ret;
-- 
2.25.0.rc1.302.gc71d20beed

[PATCH 2/5] gpg-interface: support one-line summaries in print_signature_buffer()

[Hans Jerry Illikainen]

Most consumers of the GPG interface use print_signature_buffer() to show
the result from signature verifications.  One notable exception was
verify_merge_signature().

Previously, the verify_merge_signature() function processed the
signature_check structure in order to print a one-line summary of the
result.

I think that the one-line summary format has potential use in other
parts of Git; and not only for merges.  I also think that it makes sense
to have /one/ summary format in order to avoid confusing users with
different output styles for different operations.

This patch moves the one-line summary from verify_merge_signature() to
print_signature_buffer().  It also introduces a GPG_VERIFY_SHORT flag
that determines whether the summary should be printed.

Signed-off-by: Hans Jerry Illikainen <hji@dyntopia.com>
---
 builtin/verify-commit.c |  3 ++-
 commit.c                | 23 +++++------------------
 gpg-interface.c         | 41 ++++++++++++++++++++++++++++++++++++++++-
 gpg-interface.h         |  4 +++-
 tag.c                   |  7 ++++---
 5 files changed, 54 insertions(+), 24 deletions(-)

diff --git a/builtin/verify-commit.c b/builtin/verify-commit.c
index 2a099ec6ba..acc01a7be9 100644
--- a/builtin/verify-commit.c
+++ b/builtin/verify-commit.c
@@ -28,7 +28,8 @@ static int run_gpg_verify(struct commit *commit, unsigned flags)
 	memset(&signature_check, 0, sizeof(signature_check));
 
 	ret = check_commit_signature(commit, &signature_check);
-	print_signature_buffer(&signature_check, flags);
+	print_signature_buffer(&commit->object.oid, &signature_check, ret,
+			       flags);
 
 	signature_check_clear(&signature_check);
 	return ret;
diff --git a/commit.c b/commit.c
index 3f91d3efc5..519599469b 100644
--- a/commit.c
+++ b/commit.c
@@ -1139,29 +1139,16 @@ int check_commit_signature(const struct commit *commit, struct signature_check *
 void verify_merge_signature(struct commit *commit, int verbosity,
 			    int check_trust)
 {
-	char hex[GIT_MAX_HEXSZ + 1];
 	struct signature_check signature_check;
 	int ret;
 	memset(&signature_check, 0, sizeof(signature_check));
 
 	ret = check_commit_signature(commit, &signature_check);
-
-	find_unique_abbrev_r(hex, &commit->object.oid, DEFAULT_ABBREV);
-	switch (signature_check.result) {
-	case 'G':
-		if (ret || (check_trust && signature_check.trust_level < TRUST_MARGINAL))
-			die(_("Commit %s has an untrusted GPG signature, "
-			      "allegedly by %s."), hex, signature_check.signer);
-		break;
-	case 'B':
-		die(_("Commit %s has a bad GPG signature "
-		      "allegedly by %s."), hex, signature_check.signer);
-	default: /* 'N' */
-		die(_("Commit %s does not have a GPG signature."), hex);
-	}
-	if (verbosity >= 0 && signature_check.result == 'G')
-		printf(_("Commit %s has a good GPG signature by %s\n"),
-		       hex, signature_check.signer);
+	ret |= check_trust && signature_check.trust_level < TRUST_MARGINAL;
+	print_signature_buffer(&commit->object.oid, &signature_check, ret,
+			       GPG_VERIFY_SHORT);
+	if (ret)
+		die(_("Signature verification failed"));
 
 	signature_check_clear(&signature_check);
 }
diff --git a/gpg-interface.c b/gpg-interface.c
index fc182d39be..838ece2b37 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -5,6 +5,8 @@
 #include "gpg-interface.h"
 #include "sigchain.h"
 #include "tempfile.h"
+#include "object.h"
+#include "object-store.h"
 
 static char *configured_signing_key;
 static enum signature_trust_level configured_min_trust_level = TRUST_UNDEFINED;
@@ -333,16 +335,53 @@ int check_signature(const char *payload, size_t plen, const char *signature,
 	return !!status;
 }
 
-void print_signature_buffer(const struct signature_check *sigc, unsigned flags)
+void print_signature_buffer(const struct object_id *oid,
+			    const struct signature_check *sigc, int status,
+			    unsigned flags)
 {
 	const char *output = flags & GPG_VERIFY_RAW ?
 		sigc->gpg_status : sigc->gpg_output;
+	char hex[GIT_MAX_HEXSZ + 1];
+	char *type;
 
 	if (flags & GPG_VERIFY_VERBOSE && sigc->payload)
 		fputs(sigc->payload, stdout);
 
 	if (flags & GPG_VERIFY_FULL && output)
 		fputs(output, stderr);
+
+	if (flags & GPG_VERIFY_SHORT && oid) {
+		type = xstrdup_or_null(
+			type_name(oid_object_info(the_repository, oid, NULL)));
+		if (!type)
+			type = xstrdup("object");
+		*type = toupper(*type);
+
+		find_unique_abbrev_r(hex, oid, DEFAULT_ABBREV);
+
+		switch (sigc->result) {
+		case 'G':
+			if (status)
+				error(_("%s %s has an untrusted GPG signature, "
+					"allegedly by %s."),
+				      type, hex, sigc->signer);
+			else
+				printf(_("%s %s has a good GPG signature by %s\n"),
+				       type, hex, sigc->signer);
+			break;
+		case 'N':
+			error(_("%s %s does not have a GPG signature."), type,
+			      hex);
+			break;
+		default:
+			error(_("%s %s has a bad GPG signature "
+				"allegedly by %s."),
+			      type, hex, sigc->signer);
+			break;
+		}
+
+		free(type);
+	}
 }
 
 size_t parse_signature(const char *buf, size_t size)
diff --git a/gpg-interface.h b/gpg-interface.h
index 4631a91330..2f349f8ed5 100644
--- a/gpg-interface.h
+++ b/gpg-interface.h
@@ -6,6 +6,7 @@ struct strbuf;
 #define GPG_VERIFY_VERBOSE (1 << 0)
 #define GPG_VERIFY_RAW (1 << 1)
 #define GPG_VERIFY_FULL (1 << 2)
+#define GPG_VERIFY_SHORT (1 << 3)
 
 enum signature_trust_level {
 	TRUST_UNDEFINED,
@@ -60,7 +61,8 @@ const char *get_signing_key(void);
 int check_signature(const char *payload, size_t plen,
 		    const char *signature, size_t slen,
 		    struct signature_check *sigc);
-void print_signature_buffer(const struct signature_check *sigc,
+void print_signature_buffer(const struct object_id *oid,
+			    const struct signature_check *sigc, int status,
 			    unsigned flags);
 
 #endif
diff --git a/tag.c b/tag.c
index b8d6da81eb..f6ad4171f9 100644
--- a/tag.c
+++ b/tag.c
@@ -10,7 +10,8 @@
 
 const char *tag_type = "tag";
 
-static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags)
+static int run_gpg_verify(const struct object_id *oid, const char *buf,
+			  unsigned long size, unsigned flags)
 {
 	struct signature_check sigc;
 	size_t payload_size;
@@ -28,7 +29,7 @@ static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags)
 
 	ret = check_signature(buf, payload_size, buf + payload_size,
 				size - payload_size, &sigc);
-	print_signature_buffer(&sigc, flags);
+	print_signature_buffer(oid, &sigc, ret, flags);
 
 	signature_check_clear(&sigc);
 	return ret;
@@ -57,7 +58,7 @@ int gpg_verify_tag(const struct object_id *oid, const char *name_to_report,
 				name_to_report :
 				find_unique_abbrev(oid, DEFAULT_ABBREV));
 
-	ret = run_gpg_verify(buf, size, flags);
+	ret = run_gpg_verify(oid, buf, size, flags);
 
 	free(buf);
 	return ret;
-- 
2.25.0.rc1.302.gc71d20beed

[PATCH 3/5] commit: refactor signature verification helpers

[Hans Jerry Illikainen]

Previously, signature verification for commits had a number of helper
functions with slightly different behaviors:

- builtin/verify-commit.c had a file-local helper, so it wasn't reusable
  outside of the verify-commit builtin.
- commit.c had verify_merge_signature() that die()d on errors, making it
  difficult to reuse in parts of Git that mustn't (immediately) die on
  failure.
- commit.c also had check_commit_signature().  It's flexible enough to
  be used anywhere, but it isn't as nice as gpg_verify_tag().  More
  specifically, it doesn't take care of printing the result; nor does it
  accept and parse arbitrary object IDs.

This commit changes check_commit_signature() to file-local scope.  It
also introduces a gpg_verify_commit() function modelled after
gpg_verify_tag().  It is written with the intent of removing the need to
implement local helpers for operations that verifies commit signatures.
This should hopefully make the code paths to the GPG interface easier to
follow.

Signed-off-by: Hans Jerry Illikainen <hji@dyntopia.com>
---
 builtin/merge.c         | 16 ++++++++------
 builtin/pull.c          | 18 +++++-----------
 builtin/verify-commit.c | 25 +---------------------
 commit.c                | 47 ++++++++++++++++++++++++++++++-----------
 commit.h                | 31 ++++++---------------------
 gpg-interface.h         |  1 +
 pretty.c                |  3 ++-
 7 files changed, 60 insertions(+), 81 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index d127d2225f..e472f17738 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -62,8 +62,8 @@ static int show_diffstat = 1, shortlog_len = -1, squash;
 static int option_commit = -1;
 static int option_edit = -1;
 static int allow_trivial = 1, have_message, verify_signatures;
-static int check_trust_level = 1;
 static int overwrite_ignore = 1;
+static unsigned gpg_flags = GPG_VERIFY_SHORT | GPG_VERIFY_COMPAT;
 static struct strbuf merge_msg = STRBUF_INIT;
 static struct strategy **use_strategies;
 static size_t use_strategies_nr, use_strategies_alloc;
@@ -633,7 +633,7 @@ static int git_merge_config(const char *k, const char *v, void *cb)
 		sign_commit = git_config_bool(k, v) ? "" : NULL;
 		return 0;
 	} else if (!strcmp(k, "gpg.mintrustlevel")) {
-		check_trust_level = 0;
+		gpg_flags ^= GPG_VERIFY_COMPAT;
 	}
 
 	status = fmt_merge_msg_config(k, v, cb);
@@ -1399,9 +1399,10 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		if (remoteheads->next)
 			die(_("Can merge only exactly one commit into empty head"));
 
-		if (verify_signatures)
-			verify_merge_signature(remoteheads->item, verbosity,
-					       check_trust_level);
+		if (verify_signatures &&
+		    gpg_verify_commit(&remoteheads->item->object.oid, NULL,
+				      NULL, gpg_flags))
+			die(_("Signature verification failed"));
 
 		remote_head_oid = &remoteheads->item->object.oid;
 		read_empty(remote_head_oid, 0);
@@ -1424,8 +1425,9 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 
 	if (verify_signatures) {
 		for (p = remoteheads; p; p = p->next) {
-			verify_merge_signature(p->item, verbosity,
-					       check_trust_level);
+			if (gpg_verify_commit(&p->item->object.oid, NULL, NULL,
+					      gpg_flags))
+				die(_("Signature verification failed"));
 		}
 	}
 
diff --git a/builtin/pull.c b/builtin/pull.c
index d4e3e77c8e..e41c4032ae 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -107,11 +107,11 @@ static char *opt_ff;
 static char *opt_verify_signatures;
 static int opt_autostash = -1;
 static int config_autostash;
-static int check_trust_level = 1;
 static struct argv_array opt_strategies = ARGV_ARRAY_INIT;
 static struct argv_array opt_strategy_opts = ARGV_ARRAY_INIT;
 static char *opt_gpg_sign;
 static int opt_allow_unrelated_histories;
+static unsigned gpg_flags = GPG_VERIFY_SHORT | GPG_VERIFY_COMPAT;
 
 /* Options passed to git-fetch */
 static char *opt_all;
@@ -366,7 +366,7 @@ static int git_pull_config(const char *var, const char *value, void *cb)
 			RECURSE_SUBMODULES_ON : RECURSE_SUBMODULES_OFF;
 		return 0;
 	} else if (!strcmp(var, "gpg.mintrustlevel")) {
-		check_trust_level = 0;
+		gpg_flags ^= GPG_VERIFY_COMPAT;
 	}
 
 	status = git_gpg_config(var, value, cb);
@@ -589,17 +589,9 @@ static int run_fetch(const char *repo, const char **refspecs)
 static int pull_into_void(const struct object_id *merge_head,
 		const struct object_id *curr_head)
 {
-	if (opt_verify_signatures) {
-		struct commit *commit;
-
-		commit = lookup_commit(the_repository, merge_head);
-		if (!commit)
-			die(_("unable to access commit %s"),
-			    oid_to_hex(merge_head));
-
-		verify_merge_signature(commit, opt_verbosity,
-				       check_trust_level);
-	}
+	if (opt_verify_signatures)
+		if (gpg_verify_commit(merge_head, NULL, NULL, gpg_flags))
+			return 1;
 
 	/*
 	 * Two-way merge: we treat the index as based on an empty tree,
diff --git a/builtin/verify-commit.c b/builtin/verify-commit.c
index acc01a7be9..3f392654dd 100644
--- a/builtin/verify-commit.c
+++ b/builtin/verify-commit.c
@@ -20,37 +20,14 @@ static const char * const verify_commit_usage[] = {
 		NULL
 };
 
-static int run_gpg_verify(struct commit *commit, unsigned flags)
-{
-	struct signature_check signature_check;
-	int ret;
-
-	memset(&signature_check, 0, sizeof(signature_check));
-
-	ret = check_commit_signature(commit, &signature_check);
-	print_signature_buffer(&commit->object.oid, &signature_check, ret,
-			       flags);
-
-	signature_check_clear(&signature_check);
-	return ret;
-}
-
 static int verify_commit(const char *name, unsigned flags)
 {
 	struct object_id oid;
-	struct object *obj;
 
 	if (get_oid(name, &oid))
 		return error("commit '%s' not found.", name);
 
-	obj = parse_object(the_repository, &oid);
-	if (!obj)
-		return error("%s: unable to read file.", name);
-	if (obj->type != OBJ_COMMIT)
-		return error("%s: cannot verify a non-commit object of type %s.",
-				name, type_name(obj->type));
-
-	return run_gpg_verify((struct commit *)obj, flags);
+	return gpg_verify_commit(&oid, NULL, NULL, flags);
 }
 
 static int git_verify_commit_config(const char *var, const char *value, void *cb)
diff --git a/commit.c b/commit.c
index 519599469b..f8339916a6 100644
--- a/commit.c
+++ b/commit.c
@@ -1116,7 +1116,8 @@ static void handle_signed_tag(struct commit *parent, struct commit_extra_header
 	free(buf);
 }
 
-int check_commit_signature(const struct commit *commit, struct signature_check *sigc)
+static int check_commit_signature(const struct commit *commit,
+				  struct signature_check *sigc)
 {
 	struct strbuf payload = STRBUF_INIT;
 	struct strbuf signature = STRBUF_INIT;
@@ -1136,21 +1137,43 @@ int check_commit_signature(const struct commit *commit, struct signature_check *
 	return ret;
 }
 
-void verify_merge_signature(struct commit *commit, int verbosity,
-			    int check_trust)
+int gpg_verify_commit(const struct object_id *oid, const char *name_to_report,
+		      struct signature_check *sigc, unsigned flags)
 {
-	struct signature_check signature_check;
+	struct object *obj;
+	struct signature_check tmp = { 0 };
 	int ret;
-	memset(&signature_check, 0, sizeof(signature_check));
 
-	ret = check_commit_signature(commit, &signature_check);
-	ret |= check_trust && signature_check.trust_level < TRUST_MARGINAL;
-	print_signature_buffer(&commit->object.oid, &signature_check, ret,
-			       GPG_VERIFY_SHORT);
-	if (ret)
-		die(_("Signature verification failed"));
+	if (!sigc)
+		sigc = &tmp;
 
-	signature_check_clear(&signature_check);
+	obj = parse_object(the_repository, oid);
+	if (!obj)
+		return error("%s: unable to read file.",
+			     name_to_report ?
+			     name_to_report :
+			     find_unique_abbrev(oid, DEFAULT_ABBREV));
+
+	if (obj->type != OBJ_COMMIT)
+		return error("%s: cannot verify a non-commit object of type %s.",
+			     name_to_report ?
+			     name_to_report :
+			     find_unique_abbrev(oid, DEFAULT_ABBREV),
+			     type_name(obj->type));
+
+	ret = check_commit_signature((struct commit *)obj, sigc);
+	/*
+	 * Merge operations has historically required a trust level of
+	 * 'marginal' or higher as a default.  Backward-compatibility is
+	 * maintained here -- however, new users of this function should
+	 * delegate trust level verification to the GPG interface.
+	 */
+	ret |= flags & GPG_VERIFY_COMPAT && sigc->trust_level < TRUST_MARGINAL;
+
+	print_signature_buffer(oid, sigc, ret, flags);
+	signature_check_clear(&tmp);
+
+	return ret;
 }
 
 void append_merge_tag_headers(struct commit_list *parents,
diff --git a/commit.h b/commit.h
index 008a0fa4a0..a31aaa7304 100644
--- a/commit.h
+++ b/commit.h
@@ -364,13 +364,14 @@ int parse_signed_commit(const struct commit *commit,
 int remove_signature(struct strbuf *buf);
 
 /*
- * Check the signature of the given commit. The result of the check is stored
- * in sig->check_result, 'G' for a good signature, 'U' for a good signature
- * from an untrusted signer, 'B' for a bad signature and 'N' for no signature
- * at all.  This may allocate memory for sig->gpg_output, sig->gpg_status,
- * sig->signer and sig->key.
+ * Check the signature of the given commit.  The result is stored in sigc if it
+ * is non-NULL.  However, note that a return code of 0 from this function
+ * should be used to determine if the signature verified successfully, because
+ * multiple members in the signature_check structure are needed to sufficiently
+ * determine the outcome.
  */
-int check_commit_signature(const struct commit *commit, struct signature_check *sigc);
+int gpg_verify_commit(const struct object_id *oid, const char *name_to_report,
+		      struct signature_check *sigc, unsigned flags);
 
 /* record author-date for each commit object */
 struct author_date_slab;
@@ -378,24 +379,6 @@ void record_author_date(struct author_date_slab *author_date,
 			struct commit *commit);
 
 int compare_commits_by_author_date(const void *a_, const void *b_, void *unused);
-
-/*
- * Verify a single commit with check_commit_signature() and die() if it is not
- * a good signature. This isn't really suitable for general use, but is a
- * helper to implement consistent logic for pull/merge --verify-signatures.
- *
- * The check_trust parameter is meant for backward-compatibility.  The GPG
- * interface verifies key trust with a default trust level that is below the
- * default trust level for merge operations.  Its value should be non-zero if
- * the user hasn't set a minimum trust level explicitly in their configuration.
- *
- * If the user has set a minimum trust level, then that value should be obeyed
- * and check_trust should be zero, even if the configured trust level is below
- * the default trust level for merges.
- */
-void verify_merge_signature(struct commit *commit, int verbose,
-			    int check_trust);
-
 int compare_commits_by_commit_date(const void *a_, const void *b_, void *unused);
 int compare_commits_by_gen_then_commit_date(const void *a_, const void *b_, void *unused);
 
diff --git a/gpg-interface.h b/gpg-interface.h
index 2f349f8ed5..3f619cce0e 100644
--- a/gpg-interface.h
+++ b/gpg-interface.h
@@ -7,6 +7,7 @@ struct strbuf;
 #define GPG_VERIFY_RAW (1 << 1)
 #define GPG_VERIFY_FULL (1 << 2)
 #define GPG_VERIFY_SHORT (1 << 3)
+#define GPG_VERIFY_COMPAT (1 << 4)
 
 enum signature_trust_level {
 	TRUST_UNDEFINED,
diff --git a/pretty.c b/pretty.c
index f5fbbc5ffb..c19620ffe6 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1302,7 +1302,8 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 
 	if (placeholder[0] == 'G') {
 		if (!c->signature_check.result)
-			check_commit_signature(c->commit, &(c->signature_check));
+			gpg_verify_commit(&c->commit->object.oid, NULL,
+					  &(c->signature_check), 0);
 		switch (placeholder[1]) {
 		case 'G':
 			if (c->signature_check.gpg_output)
-- 
2.25.0.rc1.302.gc71d20beed

[PATCH 4/5] merge: verify signatures if gpg.verifySignatures is true

[Hans Jerry Illikainen]

Merge operations has had support for a merge.verifySignatures config
knob for quite some time.  However, there is no global option to enable
signature verification for all operations that support it.  This makes
sense because only merges (and, by extent, pulls) has support for
configurable signature verifications.

However, with the upcoming introduction of signature verification for
clones, it seems reasonable to have a global option that enables
verification for all operations that support it.  Otherwise, users would
have to track down and enable every *.verifySignatures knob.

This patch adds support for a global gpg.verifySignatures in
git_merge_config().  The global variant is overridden by both
merge.verifySignatures and the --(no-)verify-signatures command-line
parameter.

Signed-off-by: Hans Jerry Illikainen <hji@dyntopia.com>
---
 Documentation/config/gpg.txt       |  6 ++++++
 Documentation/config/merge.txt     |  4 +++-
 builtin/merge.c                    |  8 +++++---
 t/t7612-merge-verify-signatures.sh | 27 +++++++++++++++++++++++++++
 4 files changed, 41 insertions(+), 4 deletions(-)

diff --git a/Documentation/config/gpg.txt b/Documentation/config/gpg.txt
index d94025cb36..7bf64cff24 100644
--- a/Documentation/config/gpg.txt
+++ b/Documentation/config/gpg.txt
@@ -33,3 +33,9 @@ gpg.minTrustLevel::
 * `marginal`
 * `fully`
 * `ultimate`
+
+gpg.verifySignatures::
+	Verify that commits are signed with a valid key for operations
+	that support signature verification.  This option act as a
+	global default and can be overridden in sections specific to
+	individual operations.
diff --git a/Documentation/config/merge.txt b/Documentation/config/merge.txt
index 6a313937f8..7ff72fafc2 100644
--- a/Documentation/config/merge.txt
+++ b/Documentation/config/merge.txt
@@ -28,7 +28,9 @@ merge.ff::
 
 merge.verifySignatures::
 	If true, this is equivalent to the --verify-signatures command
-	line option. See linkgit:git-merge[1] for details.
+	line option. See linkgit:git-merge[1] for details.  Also see
+	`gpg.verifySignatures` for a global option to enable signature
+	verification.
 
 include::fmt-merge-msg.txt[]
 
diff --git a/builtin/merge.c b/builtin/merge.c
index e472f17738..539dd1dbfc 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -61,7 +61,7 @@ static const char * const builtin_merge_usage[] = {
 static int show_diffstat = 1, shortlog_len = -1, squash;
 static int option_commit = -1;
 static int option_edit = -1;
-static int allow_trivial = 1, have_message, verify_signatures;
+static int allow_trivial = 1, have_message, verify_signatures = -1;
 static int overwrite_ignore = 1;
 static unsigned gpg_flags = GPG_VERIFY_SHORT | GPG_VERIFY_COMPAT;
 static struct strbuf merge_msg = STRBUF_INIT;
@@ -610,6 +610,8 @@ static int git_merge_config(const char *k, const char *v, void *cb)
 		show_diffstat = git_config_bool(k, v);
 	else if (!strcmp(k, "merge.verifysignatures"))
 		verify_signatures = git_config_bool(k, v);
+	else if (!strcmp(k, "gpg.verifysignatures") && verify_signatures < 0)
+		verify_signatures = git_config_bool(k, v);
 	else if (!strcmp(k, "pull.twohead"))
 		return git_config_string(&pull_twohead, k, v);
 	else if (!strcmp(k, "pull.octopus"))
@@ -1399,7 +1401,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		if (remoteheads->next)
 			die(_("Can merge only exactly one commit into empty head"));
 
-		if (verify_signatures &&
+		if (verify_signatures == 1 &&
 		    gpg_verify_commit(&remoteheads->item->object.oid, NULL,
 				      NULL, gpg_flags))
 			die(_("Signature verification failed"));
@@ -1423,7 +1425,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		usage_with_options(builtin_merge_usage,
 			builtin_merge_options);
 
-	if (verify_signatures) {
+	if (verify_signatures == 1) {
 		for (p = remoteheads; p; p = p->next) {
 			if (gpg_verify_commit(&p->item->object.oid, NULL, NULL,
 					      gpg_flags))
diff --git a/t/t7612-merge-verify-signatures.sh b/t/t7612-merge-verify-signatures.sh
index a426f3a89a..10ab2fa119 100755
--- a/t/t7612-merge-verify-signatures.sh
+++ b/t/t7612-merge-verify-signatures.sh
@@ -125,6 +125,33 @@ test_expect_success GPG 'merge commit with bad signature with merge.verifySignat
 	git merge --no-verify-signatures $(cat forged.commit)
 '
 
+test_expect_success GPG 'merge commit with bad signature with gpg.verifySignatures=true and --no-verify-signatures' '
+	test_when_finished "git reset --hard && git checkout initial" &&
+	test_config gpg.verifySignatures true &&
+	git merge --no-verify-signatures $(cat forged.commit)
+'
+
+test_expect_success GPG 'merge commit with bad signature with gpg.verifySignatures=true and merge.verifySignatures=false' '
+	test_when_finished "git reset --hard && git checkout initial" &&
+	test_config gpg.verifySignatures true &&
+	test_config merge.verifySignatures false &&
+	git merge $(cat forged.commit)
+'
+
+test_expect_success GPG 'merge commit with bad signature with clone.verifySignatures=false and gpg.verifySignatures=true' '
+	test_when_finished "git reset --hard && git checkout initial" &&
+	test_config merge.verifySignatures false &&
+	test_config gpg.verifySignatures true &&
+	git merge $(cat forged.commit)
+'
+
+test_expect_success GPG 'merge commit with bad signature with gpg.verifySignatures=true' '
+	test_when_finished "git reset --hard && git checkout initial" &&
+	test_config gpg.verifySignatures true &&
+	test_must_fail git merge $(cat forged.commit) 2>mergeerror &&
+	test_i18ngrep "has a bad GPG signature allegedly by" mergeerror
+'
+
 test_expect_success GPG 'merge unsigned commit into unborn branch' '
 	test_when_finished "git checkout initial" &&
 	git checkout --orphan unborn &&
-- 
2.25.0.rc1.302.gc71d20beed

[PATCH 5/5] clone: support signature verification

[Hans Jerry Illikainen]

The merge operation (and as a consequence, pull) has had support for
signature verification for quite some time.  However, there were no
support for verifying GPG signatures for the initial clone.

Without signature verification for clone operations, users are forced to
checkout a repository before using verify-commit or verify-tag on the
tree.  This is potentially problematic for a few reasons:

- It's possible to forget to verify the tree when there's nothing that
  enforce signature verification before a cloned repository is used.

- Software on the users system might process the tree before it has been
  successfully verified -- for example, file managers that automagically
  creates thumbnails for images, videos and documents.

Now, this could be worked around with a --no-checkout clone followed by
signature verification and checkout.  There are also various scripts
floating around that more-or-less re-implements 'git clone' with
signature verification by using a combination of init + remote add +
fetch + verify-commit + merge [1].  But none of these options are
particularly user-friendly.

This patch implements signature verification for the clone builtin to
accommodate use-cases where the tree should be verified before use.

There is one major quirk in this patch; namely, recursive clones of
submodules.  It is worked around by passing --no-verify-signatures in
clone_submodule().  The rationale for this approach is that:

- The object ID for submodules are stored in the super repository.
  Thus, if the super is trusted and signed (and the hash function is
  secure), then it seems reasonable to also trust the submodules
  referenced in the super repository.

- Propagating signature verification to recursive clones of submodules
  would lessen the use of signature verification for clones, because
  then users would need the keys for each submodule author in their
  keyring.  Also, not all submodule refs may be signed.

[1] https://gist.github.com/tribut/50c0f7d0b8341fa6d1784c317d5275f0

Signed-off-by: Hans Jerry Illikainen <hji@dyntopia.com>
---
 Documentation/config.txt           |   2 +
 Documentation/config/clone.txt     |   3 +
 Documentation/git-clone.txt        |   4 +
 builtin/clone.c                    |  46 ++++
 builtin/submodule--helper.c        |   6 +
 t/t5619-clone-verify-signatures.sh | 411 +++++++++++++++++++++++++++++
 tag.c                              |  10 +-
 7 files changed, 478 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/config/clone.txt
 create mode 100755 t/t5619-clone-verify-signatures.sh

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 83e7bba872..fda69e660e 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -331,6 +331,8 @@ include::config/checkout.txt[]
 
 include::config/clean.txt[]
 
+include::config/clone.txt[]
+
 include::config/color.txt[]
 
 include::config/column.txt[]
diff --git a/Documentation/config/clone.txt b/Documentation/config/clone.txt
new file mode 100644
index 0000000000..9fd2ee3395
--- /dev/null
+++ b/Documentation/config/clone.txt
@@ -0,0 +1,3 @@
+clone.verifySignatures::
+	If true, this is equivalent to the --verify-signatures command
+	line option. See linkgit:git-clone[1] for details.
diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index bf24f1813a..47a9d1e182 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -285,6 +285,10 @@ or `--mirror` is given)
 	The number of submodules fetched at the same time.
 	Defaults to the `submodule.fetchJobs` option.
 
+--verify-signatures::
+--no-verify-signatures::
+	Verify that the newly created HEAD is signed with a valid key.
+
 <repository>::
 	The (possibly remote) repository to clone from.  See the
 	<<URLS,GIT URLS>> section below for more information on specifying
diff --git a/builtin/clone.c b/builtin/clone.c
index 0fc89ae2b9..037953bc4b 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -32,6 +32,8 @@
 #include "connected.h"
 #include "packfile.h"
 #include "list-objects-filter-options.h"
+#include "object.h"
+#include "tag.h"
 
 /*
  * Overall FIXMEs:
@@ -54,6 +56,7 @@ static int deepen;
 static char *option_template, *option_depth, *option_since;
 static char *option_origin = NULL;
 static char *option_branch = NULL;
+static int option_verify_signatures = -1;
 static struct string_list option_not = STRING_LIST_INIT_NODUP;
 static const char *real_git_dir;
 static char *option_upload_pack = "git-upload-pack";
@@ -120,6 +123,8 @@ static struct option builtin_clone_options[] = {
 		   N_("use <name> instead of 'origin' to track upstream")),
 	OPT_STRING('b', "branch", &option_branch, N_("branch"),
 		   N_("checkout <branch> instead of the remote's HEAD")),
+	OPT_BOOL(0, "verify-signatures", &option_verify_signatures,
+		 N_("verify the GPG signature of the newly created HEAD")),
 	OPT_STRING('u', "upload-pack", &option_upload_pack, N_("path"),
 		   N_("path to git-upload-pack on the remote")),
 	OPT_STRING(0, "depth", &option_depth, N_("depth"),
@@ -929,6 +934,40 @@ static int path_exists(const char *path)
 	return !stat(path, &sb);
 }
 
+static int verify_signature(const struct ref *our, const struct ref *remote)
+{
+	const struct object_id *oid = our ? &our->old_oid : &remote->old_oid;
+	enum object_type type = oid_object_info(the_repository, oid, NULL);
+	int flags = option_verbosity ? GPG_VERIFY_FULL : GPG_VERIFY_SHORT;
+
+	if (type == OBJ_COMMIT)
+		return gpg_verify_commit(oid, NULL, NULL, flags);
+	if (type == OBJ_TAG)
+		return gpg_verify_tag(oid, NULL, flags);
+	return error(_("%s: unknown object type: %s"),
+		     find_unique_abbrev(oid, DEFAULT_ABBREV), type_name(type));
+}
+
+static int git_clone_config(const char *var, const char *value, void *cb)
+{
+	int status;
+
+	if (!strcmp(var, "clone.verifysignatures")) {
+		option_verify_signatures = git_config_bool(var, value);
+		return 0;
+	}
+
+	if (!strcmp(var, "gpg.verifysignatures") &&
+	    option_verify_signatures < 0)
+		option_verify_signatures = git_config_bool(var, value);
+
+	status = git_gpg_config(var, value, cb);
+	if (status)
+		return status;
+
+	return git_default_config(var, value, cb);
+}
+
 int cmd_clone(int argc, const char **argv, const char *prefix)
 {
 	int is_bundle = 0, is_local;
@@ -952,6 +991,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	struct argv_array ref_prefixes = ARGV_ARRAY_INIT;
 
 	packet_trace_identity("clone");
+
+	git_config(git_clone_config, NULL);
+
 	argc = parse_options(argc, argv, prefix, builtin_clone_options,
 			     builtin_clone_usage, 0);
 
@@ -1267,6 +1309,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	else if (refs && complete_refs_before_fetch)
 		transport_fetch_refs(transport, mapped_refs);
 
+	if (option_verify_signatures > 0)
+		if (verify_signature(our_head_points_at, remote_head))
+			die(_("Signature verification failed"));
+
 	update_remote_refs(refs, mapped_refs, remote_head_points_at,
 			   branch_top.buf, reflog_msg.buf, transport,
 			   !is_local, filter_options.choice);
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index c72931ecd7..9e69c767c9 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1248,6 +1248,12 @@ static int clone_submodule(const char *path, const char *gitdir, const char *url
 	if (gitdir && *gitdir)
 		argv_array_pushl(&cp.args, "--separate-git-dir", gitdir, NULL);
 
+	/*
+	 * The --no-verify-signatures parameter has to be passed in order to
+	 * make verification of super-repositories work on recursive clones.
+	 */
+	argv_array_push(&cp.args, "--no-verify-signatures");
+
 	argv_array_push(&cp.args, "--");
 	argv_array_push(&cp.args, url);
 	argv_array_push(&cp.args, path);
diff --git a/t/t5619-clone-verify-signatures.sh b/t/t5619-clone-verify-signatures.sh
new file mode 100755
index 0000000000..7a93d57c2a
--- /dev/null
+++ b/t/t5619-clone-verify-signatures.sh
@@ -0,0 +1,411 @@
+#!/bin/sh
+
+test_description='Test cloning repos with signature verification'
+
+. ./test-lib.sh
+. "$TEST_DIRECTORY/lib-gpg.sh"
+
+test_expect_success GPG 'create repositories with signed commits and tags' '
+	echo 0 >a && git add a &&
+	test_tick && git commit -m "initial-unsigned" &&
+	git tag -a -m "unsigned v0" v0-unsigned &&
+
+	git clone . signed &&
+	(
+		cd signed &&
+		echo 1 >b && git add b &&
+		test_tick && git commit -S -m "signed" &&
+		git branch signed-branch &&
+		git tag -s -m "signed v1" v1-signed
+	) &&
+
+	git clone . unsigned &&
+	(
+		cd unsigned &&
+		echo 2 >c && git add c &&
+		test_tick && git commit -m "unsigned" &&
+		git tag v2-unsigned-shallow &&
+		git tag -a -m "unsigned and annotated" v2-unsigned-annotated
+	) &&
+
+	git clone signed unsigned-tip &&
+	(
+		cd unsigned-tip &&
+		echo 3 >d && git add d &&
+		test_tick && git commit -m "unsigned tip" &&
+		git tag -a -m "unsigned v3 tip" v3-unsigned-tip &&
+		git branch signed-branch origin/signed-branch
+	) &&
+
+	git clone signed unsigned-branch &&
+	(
+		cd unsigned-branch &&
+		git checkout -b unsigned-branch &&
+		git commit --amend --no-edit &&
+		git checkout master
+	) &&
+
+	git clone . signed-tag-unsigned-commit &&
+	(
+		cd signed-tag-unsigned-commit &&
+		git tag -s -m "signed/unsigned v4" v4-signed-tag-unsigned-commit
+	) &&
+
+	git clone . bad &&
+	(
+		cd bad &&
+		echo 4 >d && git add d &&
+		test_tick && git commit -S -m "bad" &&
+		git cat-file commit HEAD >raw &&
+		sed -e "s/^bad/forged bad/" raw >forged &&
+		git hash-object -w -t commit forged >forged.commit &&
+		git checkout $(cat forged.commit)
+	) &&
+
+	git clone . untrusted &&
+	(
+		cd untrusted &&
+		echo 5 >e && git add e &&
+		test_tick && git commit -SB7227189 -m "untrusted"
+	) &&
+
+	git clone unsigned unsigned-detached &&
+	(
+		cd unsigned-detached &&
+		echo 6 >f && git add f &&
+		test_tick && git commit -S -m "signed" &&
+		git checkout HEAD^
+	) &&
+
+	git clone signed signed-detached &&
+	(
+		cd signed-detached &&
+		echo 7 >g && git add g &&
+		test_tick && git commit -S -m "signed" &&
+		git checkout HEAD^
+	) &&
+
+	git clone signed signed-with-unsigned-submodule &&
+	(
+		cd signed-with-unsigned-submodule &&
+		git submodule add "file://$PWD/../unsigned" &&
+		git commit -S -m "add submodule"
+	) &&
+
+	git clone signed signed-with-signed-submodule &&
+	(
+		cd signed-with-signed-submodule &&
+		git submodule add "file://$PWD/../signed" &&
+		git commit -S -m "add submodule"
+	) &&
+
+	git clone unsigned unsigned-with-unsigned-submodule &&
+	(
+		cd unsigned-with-unsigned-submodule &&
+		git submodule add "file://$PWD/../unsigned" &&
+		git commit -m "add submodule"
+	) &&
+
+	git clone unsigned unsigned-with-signed-submodule &&
+	(
+		cd unsigned-with-signed-submodule &&
+		git submodule add "file://$PWD/../signed" &&
+		git commit -m "add submodule"
+	)
+'
+
+test_expect_success GPG 'clone signed with --verify-signatures' '
+	test_when_finished "rm -rf dst" &&
+	git clone --verify-signatures signed dst >out &&
+	test_i18ngrep "Commit [0-9a-f]\+ has a good GPG signature by " out
+'
+
+test_expect_success GPG 'clone signed tag with --verify-signatures' '
+	test_when_finished "rm -rf dst" &&
+	git clone -b v1-signed --verify-signatures signed dst >out &&
+	test_i18ngrep "Tag [0-9a-f]\+ has a good GPG signature by " out
+'
+
+test_expect_success GPG 'clone signed with clone.verifySignatures=true' '
+	test_when_finished "rm -rf dst" &&
+	test_config_global clone.verifySignatures true &&
+	git clone signed dst >out &&
+	test_i18ngrep "Commit [0-9a-f]\+ has a good GPG signature by " out
+'
+
+test_expect_success GPG 'clone signed with --depth=1 and clone.verifySignatures=true' '
+	test_when_finished "rm -rf dst" &&
+	test_config_global clone.verifySignatures true &&
+	git clone --depth=1 signed dst >out &&
+	test_i18ngrep "Commit [0-9a-f]\+ has a good GPG signature by " out
+'
+
+test_expect_success GPG 'clone signed with --no-checkout and clone.verifySignatures=true' '
+	test_when_finished "rm -rf dst" &&
+	test_config_global clone.verifySignatures true &&
+	git clone --no-checkout signed dst >out &&
+	test_i18ngrep "Commit [0-9a-f]\+ has a good GPG signature by " out
+'
+
+test_expect_success GPG 'clone signed with --mirror and clone.verifySignatures=true' '
+	test_when_finished "rm -rf dst" &&
+	test_config_global clone.verifySignatures true &&
+	git clone --mirror signed dst >out &&
+	test_i18ngrep "Commit [0-9a-f]\+ has a good GPG signature by " out
+'
+
+test_expect_success GPG 'clone signed without blobs and clone.verifySignatures=true' '
+	test_when_finished "rm -rf dst" &&
+	test_config_global clone.verifySignatures true &&
+	git clone --filter=blob:none signed dst >out &&
+	test_i18ngrep "Commit [0-9a-f]\+ has a good GPG signature by " out
+'
+
+test_expect_success GPG 'clone signed bare with clone.verifySignatures=true' '
+	test_when_finished "rm -rf dst" &&
+	test_config_global clone.verifySignatures true &&
+	git clone --bare signed dst >out &&
+	test_i18ngrep "Commit [0-9a-f]\+ has a good GPG signature by " out
+'
+
+test_expect_success GPG 'clone signed tag with clone.verifySignatures=true' '
+	test_when_finished "rm -rf dst" &&
+	test_config_global clone.verifySignatures true &&
+	git clone -b v1-signed signed dst >out &&
+	test_i18ngrep "Tag [0-9a-f]\+ has a good GPG signature by " out
+'
+
+test_expect_success GPG 'clone unsigned with defaults' '
+	test_when_finished "rm -rf dst" &&
+	git clone unsigned dst >out 2>&1 &&
+	! test_i18ngrep "GPG signature" out
+'
+
+test_expect_success GPG 'clone unsigned with clone.verifySignatures=true' '
+	test_config_global clone.verifySignatures true &&
+	test_must_fail git clone unsigned dst 2>out &&
+	test_path_is_missing dst &&
+	test_i18ngrep "Commit [0-9a-f]\+ does not have a GPG signature." out
+'
+
+test_expect_success GPG 'clone unsigned with --depth=1 and clone.verifySignatures=true' '
+	test_config_global clone.verifySignatures true &&
+	test_must_fail git clone --depth=1 unsigned dst 2>out &&
+	test_path_is_missing dst &&
+	test_i18ngrep "Commit [0-9a-f]\+ does not have a GPG signature." out
+'
+
+test_expect_success GPG 'clone unsigned with --no-checkout and clone.verifySignatures=true' '
+	test_config_global clone.verifySignatures true &&
+	test_must_fail git clone --no-checkout unsigned dst 2>out &&
+	test_path_is_missing dst &&
+	test_i18ngrep "Commit [0-9a-f]\+ does not have a GPG signature." out
+'
+
+test_expect_success GPG 'clone unsigned with --mirror and clone.verifySignatures=true' '
+	test_config_global clone.verifySignatures true &&
+	test_must_fail git clone --no-checkout unsigned dst 2>out &&
+	test_path_is_missing dst &&
+	test_i18ngrep "Commit [0-9a-f]\+ does not have a GPG signature." out
+'
+
+test_expect_success GPG 'clone unsigned without blobs and clone.verifySignatures=true' '
+	test_config_global clone.verifySignatures true &&
+	test_must_fail git clone --filter=blob:none unsigned dst 2>out &&
+	test_path_is_missing dst &&
+	test_i18ngrep "Commit [0-9a-f]\+ does not have a GPG signature." out
+'
+
+test_expect_success GPG 'clone unsigned bare with clone.verifySignatures=true' '
+	test_config_global clone.verifySignatures true &&
+	test_must_fail git clone --bare unsigned dst 2>out &&
+	test_path_is_missing dst &&
+	test_i18ngrep "Commit [0-9a-f]\+ does not have a GPG signature." out
+'
+
+test_expect_success GPG 'clone unsigned with --verify-signatures and clone.verifySignatures=false' '
+	test_config_global clone.verifySignatures false &&
+	test_must_fail git clone --verify-signatures unsigned dst 2>out &&
+	test_path_is_missing dst &&
+	test_i18ngrep "Commit [0-9a-f]\+ does not have a GPG signature." out
+'
+
+test_expect_success GPG 'clone unsigned with --no-verify-signatures and clone.verifySignatures=true' '
+	test_when_finished "rm -rf dst" &&
+	test_config_global clone.verifySignatures true &&
+	git clone --no-verify-signatures unsigned dst >out &&
+	! test_i18ngrep "GPG signature" out
+'
+
+test_expect_success GPG 'clone unsigned with --no-verify-signatures and gpg.verifySignatures=true' '
+	test_when_finished "rm -rf dst" &&
+	test_config_global gpg.verifySignatures true &&
+	git clone --no-verify-signatures unsigned dst >out &&
+	! test_i18ngrep "GPG signature" out
+'
+
+test_expect_success GPG 'clone unsigned with clone.verifySignatures=false and gpg.verifySignatures=true' '
+	test_when_finished "rm -rf dst" &&
+	test_config_global clone.verifySignatures false &&
+	test_config_global gpg.verifySignatures true &&
+	git clone unsigned dst >out &&
+	! test_i18ngrep "GPG signature" out
+'
+
+test_expect_success GPG 'clone unsigned with gpg.verifySignatures=true and clone.verifySignatures=false' '
+	test_when_finished "rm -rf dst" &&
+	test_config_global gpg.verifySignatures true &&
+	test_config_global clone.verifySignatures false &&
+	git clone unsigned dst >out &&
+	! test_i18ngrep "GPG signature" out
+'
+
+test_expect_success GPG 'clone unsigned with gpg.verifySignatures=true' '
+	test_config_global gpg.verifySignatures true &&
+	test_must_fail git clone unsigned dst 2>out &&
+	test_path_is_missing dst &&
+	test_i18ngrep "Commit [0-9a-f]\+ does not have a GPG signature." out
+'
+
+test_expect_success GPG 'clone bad signature with --verbose and clone.verifySignatures=true' '
+	test_config_global clone.verifySignatures true &&
+	test_must_fail git clone --verbose bad dst 2>out &&
+	test_path_is_missing dst &&
+	test_i18ngrep "gpg: BAD signature from " out
+'
+
+test_expect_success GPG 'clone bad signature with clone.verifySignatures=true' '
+	test_config_global clone.verifySignatures true &&
+	test_must_fail git clone bad dst 2>out &&
+	test_path_is_missing dst &&
+	test_i18ngrep "Commit [0-9a-f]\+ has a bad GPG signature " out
+'
+
+test_expect_success GPG 'clone untrusted with clone.verifySignatures=true' '
+	test_when_finished "rm -rf dst" &&
+	test_config_global clone.verifySignatures true &&
+	git clone untrusted dst >out &&
+	test_i18ngrep "Commit [0-9a-f]\+ has a good GPG signature by " out
+'
+
+test_expect_success GPG 'clone untrusted with clone.verifySignatures=true and gpg.minTrustLevel=fully' '
+	test_config_global clone.verifySignatures true &&
+	test_config_global gpg.minTrustLevel fully &&
+	test_must_fail git clone untrusted dst 2>out &&
+	test_path_is_missing dst &&
+	test_i18ngrep "Commit [0-9a-f]\+ has an untrusted GPG signature" out
+'
+
+test_expect_success GPG 'clone unsigned tip with clone.verifySignatures=true' '
+	test_config_global clone.verifySignatures true &&
+	test_must_fail git clone unsigned-tip dst 2>out &&
+	test_path_is_missing dst &&
+	test_i18ngrep "Commit [0-9a-f]\+ does not have a GPG signature." out
+'
+
+test_expect_success GPG 'clone unsigned tip tag with clone.verifySignatures=true' '
+	test_config_global clone.verifySignatures true &&
+	test_must_fail git clone -b v3-unsigned-tip unsigned-tip dst 2>out &&
+	test_path_is_missing dst &&
+	test_i18ngrep "Tag [0-9a-f]\+ does not have a GPG signature." out
+'
+
+test_expect_success GPG 'clone signed tag from unsigned tip tag with clone.verifySignatures=true' '
+	test_when_finished "rm -rf dst" &&
+	test_config_global clone.verifySignatures true &&
+	git clone -b v1-signed unsigned-tip dst >out &&
+	test_i18ngrep "Tag [0-9a-f]\+ has a good GPG signature by " out
+'
+
+test_expect_success GPG 'clone signed branch from unsigned tip tag with clone.verifySignatures=true' '
+	test_when_finished "rm -rf dst" &&
+	test_config_global clone.verifySignatures true &&
+	git clone -b signed-branch unsigned-tip dst >out &&
+	test_i18ngrep "Commit [0-9a-f]\+ has a good GPG signature by " out
+'
+
+test_expect_success GPG 'clone unsigned branch with clone.verifySignatures=true' '
+	test_config_global clone.verifySignatures true &&
+	test_must_fail git clone -b unsigned-branch unsigned-branch dst 2>out &&
+	test_path_is_missing dst &&
+	test_i18ngrep "Commit [0-9a-f]\+ does not have a GPG signature." out
+'
+
+test_expect_success GPG 'clone unsigned shallow tag with clone.verifySignatures=true' '
+	test_config_global clone.verifySignatures true &&
+	test_must_fail git clone -b v2-unsigned-shallow unsigned dst 2>out &&
+	test_path_is_missing dst &&
+	test_i18ngrep "Commit [0-9a-f]\+ does not have a GPG signature." out
+'
+
+test_expect_success GPG 'clone unsigned annotated tag with clone.verifySignatures=true' '
+	test_config_global clone.verifySignatures true &&
+	test_must_fail git clone -b v2-unsigned-annotated unsigned dst 2>out &&
+	test_path_is_missing dst &&
+	test_i18ngrep "Tag [0-9a-f]\+ does not have a GPG signature." out
+'
+
+test_expect_success GPG 'clone signed tag for unsigned commit with clone.verifySignatures=true' '
+	test_when_finished "rm -rf dst" &&
+	test_config_global clone.verifySignatures true &&
+	git clone -b v4-signed-tag-unsigned-commit signed-tag-unsigned-commit dst >out &&
+	test_i18ngrep "Tag [0-9a-f]\+ has a good GPG signature by " out
+'
+
+test_expect_success GPG 'clone unsigned detached HEAD with clone.verifySignatures=true' '
+	test_config_global clone.verifySignatures true &&
+	test_must_fail git clone unsigned-detached dst 2>out &&
+	test_path_is_missing dst &&
+	test_i18ngrep "Commit [0-9a-f]\+ does not have a GPG signature." out
+'
+
+test_expect_success GPG 'clone signed detached HEAD with clone.verifySignatures=true' '
+	test_when_finished "rm -rf dst" &&
+	test_config_global clone.verifySignatures true &&
+	git clone signed-detached dst >out &&
+	test_i18ngrep "Commit [0-9a-f]\+ has a good GPG signature by " out
+'
+
+test_expect_success GPG 'clone signed with unsigned submodules and clone.verifySignatures=true' '
+	test_when_finished "rm -rf dst" &&
+	test_config_global clone.verifySignatures true &&
+	git clone --recurse-submodules signed-with-unsigned-submodule dst >out &&
+	test_i18ngrep "Commit [0-9a-f]\+ has a good GPG signature by " out
+'
+
+test_expect_success GPG 'clone signed with unsigned submodules and --verify-signatures' '
+	test_when_finished "rm -rf dst" &&
+	git clone --recurse-submodules --verify-signatures \
+		signed-with-unsigned-submodule dst >out &&
+	test_i18ngrep "Commit [0-9a-f]\+ has a good GPG signature by " out
+'
+
+test_expect_success GPG 'clone signed with signed submodules and clone.verifySignatures=true' '
+	test_when_finished "rm -rf dst" &&
+	test_config_global clone.verifySignatures true &&
+	git clone --recurse-submodules signed-with-signed-submodule dst >out &&
+	test_i18ngrep "Commit [0-9a-f]\+ has a good GPG signature by " out
+'
+
+test_expect_success GPG 'clone unsigned with signed submodules and clone.verifySignatures=true' '
+	test_config_global clone.verifySignatures true &&
+	test_must_fail git clone --recurse-submodules unsigned-with-signed-submodule dst 2>out &&
+	test_path_is_missing dst &&
+	test_i18ngrep "Commit [0-9a-f]\+ does not have a GPG signature." out
+'
+
+test_expect_success GPG 'clone unsigned with signed submodules and --verify-signatures' '
+	test_must_fail git clone --recurse-submodules --verify-signatures \
+		unsigned-with-signed-submodule dst 2>out &&
+	test_path_is_missing dst &&
+	test_i18ngrep "Commit [0-9a-f]\+ does not have a GPG signature." out
+'
+
+test_expect_success GPG 'clone unsigned with unsigned submodules and clone.verifySignatures=true' '
+	test_config_global clone.verifySignatures true &&
+	test_must_fail git clone --recurse-submodules unsigned-with-unsigned-submodule dst 2>out &&
+	test_path_is_missing dst &&
+	test_i18ngrep "Commit [0-9a-f]\+ does not have a GPG signature." out
+'
+
+test_done
diff --git a/tag.c b/tag.c
index f6ad4171f9..deff55198a 100644
--- a/tag.c
+++ b/tag.c
@@ -13,17 +13,19 @@ const char *tag_type = "tag";
 static int run_gpg_verify(const struct object_id *oid, const char *buf,
 			  unsigned long size, unsigned flags)
 {
-	struct signature_check sigc;
+	struct signature_check sigc = { .result = 'N' };
 	size_t payload_size;
-	int ret;
-
-	memset(&sigc, 0, sizeof(sigc));
+	int ret = 1;
 
 	payload_size = parse_signature(buf, size);
 
 	if (size == payload_size) {
 		if (flags & GPG_VERIFY_VERBOSE)
 			write_in_full(1, buf, payload_size);
+
+		/* maybe print a detailed error message */
+		print_signature_buffer(oid, &sigc, ret, flags);
+
 		return error("no signature found");
 	}
 
-- 
2.25.0.rc1.302.gc71d20beed

Re: [PATCH 0/5] refactor gpg-interface and add gpg verification for clones

[Junio C Hamano]

Hans Jerry Illikainen <hji@dyntopia.com> writes:

> And finally, signature verification is added to the clone builtin.  It
> obeys --(no-)verify-signatures, clone.verifySignatures and
> gpg.verifySignatures (in decreasing order of significance).

It is clear for a merge or a pull what it means to verify
signature---you trust your local history, and you are willing to
merge in a commit only when it has a trusted signature (which
automatically means that you trust the hash function and also the
signer did some reasonable vetting of the history behind the tip
commit, or you never check out your intermediate state, depending
on your threat model).

I am not sure what it should mean to verify signature on clone.  I'd
assume that our threat model and verification policy are consistent
with what we use for a merge/pull, in that we trust all history
behind a commit that has a trusted signature, so it is clear that
you would want the tip commit of the default branch (or if you are
naming a single branch to clone, then the tip of that branch) to
carry a trusted signature.  But what about the commits that are
reachable from other branches and tags that are *not* contained
in the branch that is initially checked out?

Later in the proposed log message of 5/5 you allude to risk of
merely checking out a potentially untrustworthy contents to the
working tree.  This is far stricter than the usual threat model we
tend to think about as the developers of source code management
system, where checking out is perfectly OK but running "make" or its
equivalent is the first contact between the victim's system with
malicious contents.

Verifying the tip of the default/sole branch upon cloning before the
tree of the commit is checked out certainly would cover that single
case (i.e. the initial checkout after cloning), but I am not sure if
it is the best way, and I am reasonably certain it is insufficient
against your threat model.  After such a clone is made, when the
user checks out another branch obtained from the "origin" remote,
there is no mechanism that protects the user from the same risk you
just covered with the new signature verification mechanism upon
cloning, without validating the tip of that other branch, somewhere
between the time the clone is made and that other branch gets
checked out.

It almost makes me suspect that what you are trying to do with the
"verification upon cloning" may be much better done as a "verify the
tree for trustworthyness before checking it out to the working tree"
mechanism, where the trustworthyness of a tree-ish object may be
defined (and possibly made customizable by the policy of the project
the user is working on) like so:

 - A tree is trustworthy if it is the tree of a trustworthy commit.

 - A commit is trustworthy if

   . it carries a trusted signature, or
   . it is pointed by a tag that carries a trusted signature, or
   . it is reachable from a trustworthy commit.

Now, it is prohibitively expensive to compute the trusttworthiness
of a tree, defined like the above, when checking it out, UNLESS you
force each and every commit to carry a trusted signature, which is
not necessarily practical in the real world.

Another approach to ensure that any and all checkout would work only
on a trustworthy tree might be to make sure that tips of *all* the
refs are trustworthy commits immediately after cloning (instead of
verifying only the branch you are going to checkout, which is what
your patch does IIUC).  That way, any subsequent checkout in the
repository would either be checking out a commit that was

 - originally cloned from the remote, and is reachable from some ref
   that was validated back when the repository was cloned, or

 - created locally in this repository, which we trust, or

 - fetched from somewhere else, and is reachable from the commit
   that was validated back when "git pull" validated what was about
   to be integrated into the history of *this* repository.

Hmm?

Re: [PATCH 1/5] gpg-interface: conditionally show the result in print_signature_buffer()

[Junio C Hamano]

Hans Jerry Illikainen <hji@dyntopia.com> writes:

> The print_signature_buffer() function in gpg-interface.c is used to
> print the result of a GPG verified payload.  It takes a 'flags'
> parameter that determines what to print.
>
> Previously, the 'flags' parameter processed 2 flags:
>
> - GPG_VERIFY_RAW: to print the raw output from GPG instead of the
>   human(ish)-readable output.  One of these outputs were always
>   shown, irregardless of any other flags.
> - GPG_VERIFY_VERBOSE: to print the payload that was verified
>
> Interestingly, there was also a third flag defined in gpg-interface.h;
> GPG_VERIFY_OMIT_STATUS.  That flag wasn't used by the print function
> itself -- instead, callers would check for the presence of
> GPG_VERIFY_OMIT_STATUS before invoking print_signature_buffer().
>
> It seems reasonable that the GPG interface should handle all flags
> related to how the result should be (or shouldn't be) shown.  This patch
> implements that behavior by removing GPG_VERIFY_OMIT_STATUS and adding
> GPG_VERIFY_FULL.  If neither GPG_VERIFY_FULL nor GPG_VERIFY_VERBOSE is
> present, then nothing is printed.  This allows callers to invoke
> print_signature_buffer() unconditionally.

So in short, VERIFY_FULL is equivalent to !OMIT_STATUS?

As the direct callers of "print" are not the ones that set up bits
in flags, I think the proposed change makes the API easier to use.

Will queue.  Thanks.

> Signed-off-by: Hans Jerry Illikainen <hji@dyntopia.com>
> ---
>  builtin/tag.c           | 4 ++--
>  builtin/verify-commit.c | 2 +-
>  builtin/verify-tag.c    | 4 ++--
>  gpg-interface.c         | 2 +-
>  gpg-interface.h         | 6 +++---
>  tag.c                   | 4 +---
>  6 files changed, 10 insertions(+), 12 deletions(-)
>
> diff --git a/builtin/tag.c b/builtin/tag.c
> index e0a4c25382..8489e220e8 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -112,10 +112,10 @@ static int verify_tag(const char *name, const char *ref,
>  {
>  	int flags;
>  	const struct ref_format *format = cb_data;
> -	flags = GPG_VERIFY_VERBOSE;
> +	flags = GPG_VERIFY_FULL | GPG_VERIFY_VERBOSE;
>  
>  	if (format->format)
> -		flags = GPG_VERIFY_OMIT_STATUS;
> +		flags = 0;
>  
>  	if (gpg_verify_tag(oid, name, flags))
>  		return -1;
> diff --git a/builtin/verify-commit.c b/builtin/verify-commit.c
> index 40c69a0bed..2a099ec6ba 100644
> --- a/builtin/verify-commit.c
> +++ b/builtin/verify-commit.c
> @@ -63,7 +63,7 @@ static int git_verify_commit_config(const char *var, const char *value, void *cb
>  int cmd_verify_commit(int argc, const char **argv, const char *prefix)
>  {
>  	int i = 1, verbose = 0, had_error = 0;
> -	unsigned flags = 0;
> +	unsigned flags = GPG_VERIFY_FULL;
>  	const struct option verify_commit_options[] = {
>  		OPT__VERBOSE(&verbose, N_("print commit contents")),
>  		OPT_BIT(0, "raw", &flags, N_("print raw gpg status output"), GPG_VERIFY_RAW),
> diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
> index f45136a06b..bd5e99925b 100644
> --- a/builtin/verify-tag.c
> +++ b/builtin/verify-tag.c
> @@ -30,7 +30,7 @@ static int git_verify_tag_config(const char *var, const char *value, void *cb)
>  int cmd_verify_tag(int argc, const char **argv, const char *prefix)
>  {
>  	int i = 1, verbose = 0, had_error = 0;
> -	unsigned flags = 0;
> +	unsigned flags = GPG_VERIFY_FULL;
>  	struct ref_format format = REF_FORMAT_INIT;
>  	const struct option verify_tag_options[] = {
>  		OPT__VERBOSE(&verbose, N_("print tag contents")),
> @@ -53,7 +53,7 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
>  		if (verify_ref_format(&format))
>  			usage_with_options(verify_tag_usage,
>  					   verify_tag_options);
> -		flags |= GPG_VERIFY_OMIT_STATUS;
> +		flags = 0;
>  	}
>  
>  	while (i < argc) {
> diff --git a/gpg-interface.c b/gpg-interface.c
> index 2d538bcd6e..fc182d39be 100644
> --- a/gpg-interface.c
> +++ b/gpg-interface.c
> @@ -341,7 +341,7 @@ void print_signature_buffer(const struct signature_check *sigc, unsigned flags)
>  	if (flags & GPG_VERIFY_VERBOSE && sigc->payload)
>  		fputs(sigc->payload, stdout);
>  
> -	if (output)
> +	if (flags & GPG_VERIFY_FULL && output)
>  		fputs(output, stderr);
>  }
>  
> diff --git a/gpg-interface.h b/gpg-interface.h
> index f4e9b4f371..4631a91330 100644
> --- a/gpg-interface.h
> +++ b/gpg-interface.h
> @@ -3,9 +3,9 @@
>  
>  struct strbuf;
>  
> -#define GPG_VERIFY_VERBOSE		1
> -#define GPG_VERIFY_RAW			2
> -#define GPG_VERIFY_OMIT_STATUS	4
> +#define GPG_VERIFY_VERBOSE (1 << 0)
> +#define GPG_VERIFY_RAW (1 << 1)
> +#define GPG_VERIFY_FULL (1 << 2)
>  
>  enum signature_trust_level {
>  	TRUST_UNDEFINED,
> diff --git a/tag.c b/tag.c
> index 71b544467e..b8d6da81eb 100644
> --- a/tag.c
> +++ b/tag.c
> @@ -28,9 +28,7 @@ static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags)
>  
>  	ret = check_signature(buf, payload_size, buf + payload_size,
>  				size - payload_size, &sigc);
> -
> -	if (!(flags & GPG_VERIFY_OMIT_STATUS))
> -		print_signature_buffer(&sigc, flags);
> +	print_signature_buffer(&sigc, flags);
>  
>  	signature_check_clear(&sigc);
>  	return ret;

Re: [PATCH 2/5] gpg-interface: support one-line summaries in print_signature_buffer()

[Junio C Hamano]

Hans Jerry Illikainen <hji@dyntopia.com> writes:

> Most consumers of the GPG interface use print_signature_buffer() to show
> This patch moves the one-line summary from verify_merge_signature() to
> print_signature_buffer().  It also introduces a GPG_VERIFY_SHORT flag
> that determines whether the summary should be printed.

I think the motivation makes sense.

> -void print_signature_buffer(const struct signature_check *sigc, unsigned flags)
> +void print_signature_buffer(const struct object_id *oid,
> +			    const struct signature_check *sigc, int status,
> +			    unsigned flags)
>  {
>  	const char *output = flags & GPG_VERIFY_RAW ?
>  		sigc->gpg_status : sigc->gpg_output;
> +	char hex[GIT_MAX_HEXSZ + 1];
> +	char *type;
>  
>  	if (flags & GPG_VERIFY_VERBOSE && sigc->payload)
>  		fputs(sigc->payload, stdout);
>  
>  	if (flags & GPG_VERIFY_FULL && output)
>  		fputs(output, stderr);
> +
> +	if (flags & GPG_VERIFY_SHORT && oid) {

I am not sure about the wisdom of "&& oid" here.  Wouldn't it be a
bug for a caller who asks for _SHORT format to feed a NULL oid to
us?  I would understand

	if (flags & GPG_VERIFY_SHORT) {
		if (!oid)
			BUG("Caller asking for SHORT without oid???");

much better, but if there is a caller that has a legitimate need
tobe able to pass NULL and still request SHORT, let's see it and
discuss if it is a good idea.

For that matter, the two print routines we have immediately above
share the same issue.

> +		type = xstrdup_or_null(
> +			type_name(oid_object_info(the_repository, oid, NULL)));
> +		if (!type)
> +			type = xstrdup("object");

This is minor, but I wonder if this is easier to follow.

		type = type_name(oid_object_info(the_repository, oid, NULL));
		if (!type)
			type = "object";
		type = xstrdup(type);

> +		*type = toupper(*type);

This is not helpful at all for i18n/l10n, I am afraid.

> +		find_unique_abbrev_r(hex, oid, DEFAULT_ABBREV);
> +
> +		switch (sigc->result) {
> +		case 'G':
> +			if (status)
> +				error(_("%s %s has an untrusted GPG signature, "
> +					"allegedly by %s."),
> +				      type, hex, sigc->signer);

The original was of course

        die(_("Commit %s has an untrusted GPG signature, "
              "allegedly by %s."), hex, signature_check.signer);

so the message can properly localized including the typename.  That
is no longer true with this conversion.

You probably would need to prepare a 3-element array (one element
for each of <object, commit, tag>), each element of the array being
a 3-element array that holds the message for these three cases
(i.e. G, N and default) instead.

Re: [PATCH 3/5] commit: refactor signature verification helpers

[Junio C Hamano]

Hans Jerry Illikainen <hji@dyntopia.com> writes:

> +static unsigned gpg_flags = GPG_VERIFY_SHORT | GPG_VERIFY_COMPAT;
>  static struct strbuf merge_msg = STRBUF_INIT;
>  static struct strategy **use_strategies;
>  static size_t use_strategies_nr, use_strategies_alloc;
> @@ -633,7 +633,7 @@ static int git_merge_config(const char *k, const char *v, void *cb)
>  		sign_commit = git_config_bool(k, v) ? "" : NULL;
>  		return 0;
>  	} else if (!strcmp(k, "gpg.mintrustlevel")) {
> -		check_trust_level = 0;
> +		gpg_flags ^= GPG_VERIFY_COMPAT;

Did you really mean to "toggle the bit each time the variable
appears"?  Or is this better written as

		gpg_flags &= ~GPG_VERIFY_COMPAT;

instead?  There may be another instance of the same in this patch.

Re: [PATCH 4/5] merge: verify signatures if gpg.verifySignatures is true

[Junio C Hamano]

Hans Jerry Illikainen <hji@dyntopia.com> writes:

> @@ -610,6 +610,8 @@ static int git_merge_config(const char *k, const char *v, void *cb)
>  		show_diffstat = git_config_bool(k, v);
>  	else if (!strcmp(k, "merge.verifysignatures"))
>  		verify_signatures = git_config_bool(k, v);
> +	else if (!strcmp(k, "gpg.verifysignatures") && verify_signatures < 0)
> +		verify_signatures = git_config_bool(k, v);

Hmm, the next person who attempts to generalize the mechanism
further would have a hard time introducing a fallback configuration
that is even common than "gpg" when s/he has to start with this
code, no?  That is, this patch introduced "gpg.verifysignatures is
used when merge.verifysignatures is not defined" and with the two
level override the code works OK, but to implement "if neither gpg.*
or merge.* is defined, common.verifysignatures is used instead", the
above part needs to be dismantled and redone.

Keeping the "initialize verify_signatures to -1 (unspecified)" from
this patch, setting a separate gpg_verify_signatures variable upon
seeing gpg.verifysignatures, and consolidating the final finding
after git_config(git_merge_config, NULL) returns into verify_signatures
like so:

	init_diff_ui_defaults();
	git_config(git_merge_config, NULL);

+	if (verify_signatures < 0)
+		verify_signatures = (0 <= gpg_verify_signatures) 
+				  ? gpg_verify_signatures 
+				  : 0;

would be more in line with the way we arrange multiple configuration
variables to serve as fallback defaults.  And that is more easily
extensible.

Also with such an arrangement, "if (verify_signatures == 1)" we see
below will become unnecessary, which is another plus.

Thanks.

>  	else if (!strcmp(k, "pull.twohead"))
>  		return git_config_string(&pull_twohead, k, v);
>  	else if (!strcmp(k, "pull.octopus"))
> @@ -1399,7 +1401,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
>  		if (remoteheads->next)
>  			die(_("Can merge only exactly one commit into empty head"));
>  
> -		if (verify_signatures &&
> +		if (verify_signatures == 1 &&
>  		    gpg_verify_commit(&remoteheads->item->object.oid, NULL,
>  				      NULL, gpg_flags))
>  			die(_("Signature verification failed"));
> @@ -1423,7 +1425,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
>  		usage_with_options(builtin_merge_usage,
>  			builtin_merge_options);
>  
> -	if (verify_signatures) {
> +	if (verify_signatures == 1) {
>  		for (p = remoteheads; p; p = p->next) {
>  			if (gpg_verify_commit(&p->item->object.oid, NULL, NULL,
>  					      gpg_flags))

Re: [PATCH 0/5] refactor gpg-interface and add gpg verification for clones

[Hans Jerry Illikainen]

On Sun, Jan 05 2020, Junio C Hamano wrote:
> Hans Jerry Illikainen <hji@dyntopia.com> writes:
>
>> And finally, signature verification is added to the clone builtin.  It
>> obeys --(no-)verify-signatures, clone.verifySignatures and
>> gpg.verifySignatures (in decreasing order of significance).
>
> I am not sure what it should mean to verify signature on clone.  I'd
> assume that our threat model and verification policy are consistent
> with what we use for a merge/pull, in that we trust all history
> behind a commit that has a trusted signature, so it is clear that
> you would want the tip commit of the default branch (or if you are
> naming a single branch to clone, then the tip of that branch) to
> carry a trusted signature.

Yes, that's how it's implemented in v0 -- the tip of the branch/tag that
is to be checked out is verified.


> But what about the commits that are reachable from other branches and
> tags that are *not* contained in the branch that is initially checked
> out?

I thought about that and figured that adding signature verification for
the checkout builtin would be the next step after this series.  That
thought should probably have been mentioned in the cover letter for
criticism -- because now I'm not sure if that's a reasonable approach
anymore.


> Later in the proposed log message of 5/5 you allude to risk of
> merely checking out a potentially untrustworthy contents to the
> working tree.  This is far stricter than the usual threat model we
> tend to think about as the developers of source code management
> system, where checking out is perfectly OK but running "make" or its
> equivalent is the first contact between the victim's system with
> malicious contents.

Modern desktop environments perform enough magic that I think it makes
sense to assume that simply having malicious content on disk is enough
for a compromise -- even though no explicit user interaction takes place
with that content.  The NSF bug in GStreamer that made headlines a few
years back is a good example of that [1].  And the numerous AV bugs
published by taviso [2].


> Verifying the tip of the default/sole branch upon cloning before the
> tree of the commit is checked out certainly would cover that single
> case (i.e. the initial checkout after cloning), but I am not sure if
> it is the best way, and I am reasonably certain it is insufficient
> against your threat model.  After such a clone is made, when the
> user checks out another branch obtained from the "origin" remote,
> there is no mechanism that protects the user from the same risk you
> just covered with the new signature verification mechanism upon
> cloning, without validating the tip of that other branch, somewhere
> between the time the clone is made and that other branch gets
> checked out.

I agree.  Again, I should have mentioned my thoughts on adding signature
verification to the checkout builtin in the cover letter.


> It almost makes me suspect that what you are trying to do with the
> "verification upon cloning" may be much better done as a "verify the
> tree for trustworthyness before checking it out to the working tree"
> mechanism, where the trustworthyness of a tree-ish object may be
> defined (and possibly made customizable by the policy of the project
> the user is working on) like so:
>
>  - A tree is trustworthy if it is the tree of a trustworthy commit.
>
>  - A commit is trustworthy if
>
>    . it carries a trusted signature, or
>    . it is pointed by a tag that carries a trusted signature, or
>    . it is reachable from a trustworthy commit.
>
> Now, it is prohibitively expensive to compute the trusttworthiness
> of a tree, defined like the above, when checking it out, UNLESS you
> force each and every commit to carry a trusted signature, which is
> not necessarily practical in the real world.

Even though you mention that it's computationally expensive, I kind of
like this approach.  It seems generalized enough that it doesn't need to
be tied to a single operation like 'clone'.


> Another approach to ensure that any and all checkout would work only
> on a trustworthy tree might be to make sure that tips of *all* the
> refs are trustworthy commits immediately after cloning (instead of
> verifying only the branch you are going to checkout, which is what
> your patch does IIUC).  That way, any subsequent checkout in the
> repository would either be checking out a commit that was
>
>  - originally cloned from the remote, and is reachable from some ref
>    that was validated back when the repository was cloned, or
>
>  - created locally in this repository, which we trust, or
>
>  - fetched from somewhere else, and is reachable from the commit
>    that was validated back when "git pull" validated what was about
>    to be integrated into the history of *this* repository.

Wouldn't that still forgo signature verification when doing something
like:

    $ git fetch
    $ git checkout -b foo origin/branch-with-previously-unseen-commits

unless either fetch or checkout is equipped with signature verification?

Also, in case of a revoked key, this approach would require that tags
signed with that key be deleted on origin.  Maybe that's to be
considered best practice anyway, but distro maintainers might not
appreciate disappearing release tags.

Also, an interesting observation (but probably a "not our problem" as
far as Git is concerned) -- I have noticed that certain git forges might
create branches unexpectedly.  A few of my repos has dependabot/...
branches created by a GitHub bot that is enabled by default:

"""
Automated security updates are opened by Dependabot on behalf of
GitHub. The Dependabot GitHub App is automatically installed on every
repository where automated security updates are enabled.
"""

I can foresee a scenario where validating the tip of every ref on a
fresh clone would result in a DoS for lot of automated CI/CD-type
workflows when bots on GitHub (and other hosts) creates unexpected
branches in the users repos.

    $ git branch -r
      origin/HEAD -> origin/master
      origin/dependabot/pip/requirements/typed-ast-1.3.2
      origin/master


> Hmm?

I appreciate you taking the time to write your thoughts!  Unfortunately
there doesn't seem to be an obvious approach that is significantly
preferable to the alternatives.  I will experiment with the ideas you
mentioned above and see if I can come up with something that makes
sense.

Would you prefer that I break off the series before 5/5 in a new series
to fix the comments you made on the other patches that seem
almost-mergeable?

Thanks!


[1] https://scarybeastsecurity.blogspot.com/2016/11/0day-exploit-compromising-linux-desktop.html
[2] https://bugs.chromium.org/p/project-zero/issues/detail?id=769

-- 
hji

Re: [PATCH 0/5] refactor gpg-interface and add gpg verification for clones

[Junio C Hamano]

Hans Jerry Illikainen <hji@dyntopia.com> writes:

>> It almost makes me suspect that what you are trying to do with the
>> "verification upon cloning" may be much better done as a "verify the
>> tree for trustworthyness before checking it out to the working tree"
>> mechanism, where the trustworthyness of a tree-ish object may be
>> defined (and possibly made customizable by the policy of the project
>> the user is working on) like so:
>>
>>  - A tree is trustworthy if it is the tree of a trustworthy commit.
>>
>>  - A commit is trustworthy if
>>
>>    . it carries a trusted signature, or
>>    . it is pointed by a tag that carries a trusted signature, or
>>    . it is reachable from a trustworthy commit.
>>
>> Now, it is prohibitively expensive to compute the trusttworthiness
>> of a tree, defined like the above, when checking it out, UNLESS you
>> force each and every commit to carry a trusted signature, which is
>> not necessarily practical in the real world.
>
> Even though you mention that it's computationally expensive, I kind of
> like this approach.  It seems generalized enough that it doesn't need to
> be tied to a single operation like 'clone'.

Well, I don't ;-)  But even if you are not signing each and every
commit, it may be possible that the reachability information kept in
the commit-graph may help reduce the cost of the computation.  We'd
probably need a way to memoise which tags and commits have already
been verified earlier for trustworthiness for the scheme to work.

> Wouldn't that still forgo signature verification when doing something
> like:
>
>     $ git fetch
>     $ git checkout -b foo origin/branch-with-previously-unseen-commits
>
> unless either fetch or checkout is equipped with signature verification?

Yes, but I was assuming that "fetch", as an underlying mechanism for
"pull" would be what you'd teach the verification.

> Also, in case of a revoked key, this approach would require that tags
> signed with that key be deleted on origin.  

I am not sure if I follow.  An object that was signed back when a
key was OK and then the key gets revoked later---what should happen
to the trustworthiness of that object?  Another object that was
obtained from elsewhere was verified to be trustworthy, but later we
found out that the key we thought was trusted was already revoked---
what should happen to the trustworthiness of that object?

In either case, I think it is an unrelated matter if the tag object
should be removed either here or at the origin.  When spread *now*,
the key that was used to sign that tag object is known to have been
revoked, so while it is of no use to convey trustworthiness, if the
upstream project chooses to keep the tag (perhaps while re-issuing
a new version of the tag that points at the same object but signed
with a different key), that is perfectly fine, I would say.

> Maybe that's to be
> considered best practice anyway, but distro maintainers might not
> appreciate disappearing release tags.

I think that is best left as a policy decision for each project, and
we are not in the business of removing objects---we just should give
them tools to determine what key was used to sign an object, GPG or
whatever signing and key management infrastructure the projects
chooses to use gives them tools to decide if the key is what the
project trusts, and we give them tools to remove tags the project
finds unwanted.  Combining the tools according to the policy decision
the project makes is outside the scope of what we do here.