diff mbox series

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

Message ID 20200105135616.19102-5-hji@dyntopia.com (mailing list archive)
State New, archived
Headers show
Series refactor gpg-interface and add gpg verification for clones | expand

Commit Message

Hans Jerry Illikainen Jan. 5, 2020, 1:56 p.m. UTC 
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(-)

Comments

Junio C Hamano Jan. 6, 2020, 9:01 p.m. UTC | #1 
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))
diff mbox series

Patch

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