[PATCH v2 0/9] X509 (gpgsm) commit signing support

[PATCH v2 0/9] X509 (gpgsm) commit signing support

[Henning Schild]

Changes in v2:
 - removed trailing commas in array initializers and add leading space
 - replaced assert(0) with BUG in p5
 - consolidated 2 format lookups reusing get_format_data p5
 - changed from format "PGP" to "openpgp", later X509 to "x509"
 - use strcasecmp instead of strcmp for format matching
 - introduce gpg.<format>.program in p8, no gpg.programX509 anymore
 - moved t/7510 patch inbetween the two patches changing validation code
 - changed t/7510 patch to use test_config and test_must_fail

This series adds support for signing commits with gpgsm.

The first two patches are cleanups of gpg-interface, they are already
close to being merged. But since they have not been pulled to next i am
resending them.
The following 5 patches (p3-p7) prepare for the introduction of the
actual feature in patch 8.
Finally patch 9 extends the testsuite to cover the new feature.

This series can be seen as a follow up of a series that appeared under
the name "gpg-interface: Multiple signing tools" in april 2018 [1]. After
that series was not merged i decided to get my patches ready. The
original series aimed at being generic for any sort of signing tool, while
this series just introduced the X509 variant of gpg. (gpgsm)
I collected authors and reviewers of that first series and already put them
on cc.

[1] https://public-inbox.org/git/20180409204129.43537-1-mastahyeti@gmail.com/

Henning Schild (9):
  builtin/receive-pack: use check_signature from gpg-interface
  gpg-interface: make parse_gpg_output static and remove from interface
    header
  gpg-interface: add new config to select how to sign a commit
  t/t7510: check the validation of the new config gpg.format
  gpg-interface: introduce an abstraction for multiple gpg formats
  gpg-interface: do not hardcode the key string len anymore
  gpg-interface: introduce new config to select per gpg format program
  gpg-interface: introduce new signature format "x509" using gpgsm
  gpg-interface t: extend the existing GPG tests with GPGSM

 Documentation/config.txt   |  9 +++++
 builtin/receive-pack.c     | 17 ++-------
 gpg-interface.c            | 88 ++++++++++++++++++++++++++++++++++++++--------
 gpg-interface.h            |  2 --
 t/lib-gpg.sh               |  9 ++++-
 t/lib-gpg/gpgsm-gen-key.in |  6 ++++
 t/t4202-log.sh             | 66 ++++++++++++++++++++++++++++++++++
 t/t5534-push-signed.sh     | 52 +++++++++++++++++++++++++++
 t/t7003-filter-branch.sh   | 15 ++++++++
 t/t7030-verify-tag.sh      | 47 +++++++++++++++++++++++--
 t/t7510-signed-commit.sh   | 10 ++++++
 t/t7600-merge.sh           | 31 ++++++++++++++++
 12 files changed, 317 insertions(+), 35 deletions(-)
 create mode 100644 t/lib-gpg/gpgsm-gen-key.in

-- 
2.16.4

[PATCH v2 1/9] builtin/receive-pack: use check_signature from gpg-interface

[Henning Schild]

The combination of verify_signed_buffer followed by parse_gpg_output is
available as check_signature. Use that instead of implementing it again.

Signed-off-by: Henning Schild <henning.schild@siemens.com>
---
 builtin/receive-pack.c | 17 ++---------------
 1 file changed, 2 insertions(+), 15 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 68d36e0a5..9f0583deb 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -629,8 +629,6 @@ static void prepare_push_cert_sha1(struct child_process *proc)
 		return;
 
 	if (!already_done) {
-		struct strbuf gpg_output = STRBUF_INIT;
-		struct strbuf gpg_status = STRBUF_INIT;
 		int bogs /* beginning_of_gpg_sig */;
 
 		already_done = 1;
@@ -639,22 +637,11 @@ static void prepare_push_cert_sha1(struct child_process *proc)
 			oidclr(&push_cert_oid);
 
 		memset(&sigcheck, '\0', sizeof(sigcheck));
-		sigcheck.result = 'N';
 
 		bogs = parse_signature(push_cert.buf, push_cert.len);
-		if (verify_signed_buffer(push_cert.buf, bogs,
-					 push_cert.buf + bogs, push_cert.len - bogs,
-					 &gpg_output, &gpg_status) < 0) {
-			; /* error running gpg */
-		} else {
-			sigcheck.payload = push_cert.buf;
-			sigcheck.gpg_output = gpg_output.buf;
-			sigcheck.gpg_status = gpg_status.buf;
-			parse_gpg_output(&sigcheck);
-		}
+		check_signature(push_cert.buf, bogs, push_cert.buf + bogs,
+				push_cert.len - bogs, &sigcheck);
 
-		strbuf_release(&gpg_output);
-		strbuf_release(&gpg_status);
 		nonce_status = check_nonce(push_cert.buf, bogs);
 	}
 	if (!is_null_oid(&push_cert_oid)) {
-- 
2.16.4

[PATCH v2 2/9] gpg-interface: make parse_gpg_output static and remove from interface header

[Henning Schild]

This commit turns parse_gpg_output into an internal function, the only
outside user was migrated in an earlier commit.

Signed-off-by: Henning Schild <henning.schild@siemens.com>
---
 gpg-interface.c | 2 +-
 gpg-interface.h | 2 --
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/gpg-interface.c b/gpg-interface.c
index 0647bd634..09ddfbc26 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -35,7 +35,7 @@ static struct {
 	{ 'R', "\n[GNUPG:] REVKEYSIG "},
 };
 
-void parse_gpg_output(struct signature_check *sigc)
+static void parse_gpg_output(struct signature_check *sigc)
 {
 	const char *buf = sigc->gpg_status;
 	int i;
diff --git a/gpg-interface.h b/gpg-interface.h
index a5e6517ae..5ecff4aa0 100644
--- a/gpg-interface.h
+++ b/gpg-interface.h
@@ -33,8 +33,6 @@ void signature_check_clear(struct signature_check *sigc);
  */
 size_t parse_signature(const char *buf, size_t size);
 
-void parse_gpg_output(struct signature_check *);
-
 /*
  * Create a detached signature for the contents of "buffer" and append
  * it after "signature"; "buffer" and "signature" can be the same
-- 
2.16.4

[PATCH v2 3/9] gpg-interface: add new config to select how to sign a commit

[Henning Schild]

Add "gpg.format" where the user can specify which type of signature to
use for commits. At the moment only "openpgp" is supported and the value is
not even used. This commit prepares for a new types of signatures.

Signed-off-by: Henning Schild <henning.schild@siemens.com>
---
 Documentation/config.txt | 4 ++++
 gpg-interface.c          | 7 +++++++
 2 files changed, 11 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1cc18a828..ac373e3f4 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1828,6 +1828,10 @@ gpg.program::
 	signed, and the program is expected to send the result to its
 	standard output.
 
+gpg.format::
+	Specifies which key format to use when signing with `--gpg-sign`.
+	Default is "openpgp", that is also the only supported value.
+
 gui.commitMsgWidth::
 	Defines how wide the commit message window is in the
 	linkgit:git-gui[1]. "75" is the default.
diff --git a/gpg-interface.c b/gpg-interface.c
index 09ddfbc26..ed0e55917 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -7,6 +7,7 @@
 #include "tempfile.h"
 
 static char *configured_signing_key;
+static const char *gpg_format = "openpgp";
 static const char *gpg_program = "gpg";
 
 #define PGP_SIGNATURE "-----BEGIN PGP SIGNATURE-----"
@@ -138,6 +139,12 @@ int git_gpg_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 
+	if (!strcmp(var, "gpg.format")) {
+		if (strcasecmp(value, "openpgp"))
+			return error("malformed value for %s: %s", var, value);
+		return git_config_string(&gpg_format, var, value);
+	}
+
 	if (!strcmp(var, "gpg.program")) {
 		if (!value)
 			return config_error_nonbool(var);
-- 
2.16.4

[PATCH v2 4/9] t/t7510: check the validation of the new config gpg.format

[Henning Schild]

Test setting gpg.format to both invalid and valid values.

Signed-off-by: Henning Schild <henning.schild@siemens.com>
---
 t/t7510-signed-commit.sh | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
index 6e2015ed9..7e1e9caf4 100755
--- a/t/t7510-signed-commit.sh
+++ b/t/t7510-signed-commit.sh
@@ -227,4 +227,14 @@ test_expect_success GPG 'log.showsignature behaves like --show-signature' '
 	grep "gpg: Good signature" actual
 '
 
+test_expect_success GPG 'check config gpg.format values' '
+	rm .git/config &&
+	test_config gpg.format openpgp &&
+	git commit -S --amend -m "success" &&
+	test_config gpg.format OpEnPgP &&
+	git commit -S --amend -m "success" &&
+	test_config gpg.format malformed &&
+	test_must_fail git commit -S --amend -m "fail" 2>result
+'
+
 test_done
-- 
2.16.4

[PATCH v2 5/9] gpg-interface: introduce an abstraction for multiple gpg formats

[Henning Schild]

Create a struct that holds the format details for the supported formats.
At the moment that is still just "openpgp". This commit prepares for the
introduction of more formats, that might use other programs and match
other signatures.

Signed-off-by: Henning Schild <henning.schild@siemens.com>
---
 gpg-interface.c | 74 ++++++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 58 insertions(+), 16 deletions(-)

diff --git a/gpg-interface.c b/gpg-interface.c
index ed0e55917..0a8d1bff3 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -7,12 +7,46 @@
 #include "tempfile.h"
 
 static char *configured_signing_key;
-static const char *gpg_format = "openpgp";
-static const char *gpg_program = "gpg";
+struct gpg_format_data {
+	const char *format;
+	const char *program;
+	const char *extra_args_verify[1];
+	const char *sigs[2];
+};
 
 #define PGP_SIGNATURE "-----BEGIN PGP SIGNATURE-----"
 #define PGP_MESSAGE "-----BEGIN PGP MESSAGE-----"
 
+enum gpgformats { PGP_FMT };
+struct gpg_format_data gpg_formats[] = {
+	{ .format = "openpgp", .program = "gpg",
+	  .extra_args_verify = { "--keyid-format=long" },
+	  .sigs = { PGP_SIGNATURE, PGP_MESSAGE }
+	},
+};
+static const char *gpg_format = "openpgp";
+
+static struct gpg_format_data *get_format_data(const char *str)
+{
+	int i;
+	for (i = 0; i < ARRAY_SIZE(gpg_formats); i++)
+		if (!strcasecmp(gpg_formats[i].format, str))
+			return gpg_formats + i;
+	return NULL;
+}
+
+static struct gpg_format_data *get_format_data_by_sig(const char *sig)
+{
+	int i, j;
+	for (i = 0; i < ARRAY_SIZE(gpg_formats); i++)
+		for (j = 0; j < ARRAY_SIZE(gpg_formats[i].sigs); j++)
+			if (gpg_formats[i].sigs[j] && 
+			    !strncmp(gpg_formats[i].sigs[j], sig,
+				     strlen(gpg_formats[i].sigs[j])))
+				return gpg_formats + i;
+	return NULL;
+}
+
 void signature_check_clear(struct signature_check *sigc)
 {
 	FREE_AND_NULL(sigc->payload);
@@ -104,8 +138,7 @@ void print_signature_buffer(const struct signature_check *sigc, unsigned flags)
 
 static int is_gpg_start(const char *line)
 {
-	return starts_with(line, PGP_SIGNATURE) ||
-		starts_with(line, PGP_MESSAGE);
+	return (get_format_data_by_sig(line) != NULL);
 }
 
 size_t parse_signature(const char *buf, size_t size)
@@ -140,18 +173,14 @@ int git_gpg_config(const char *var, const char *value, void *cb)
 	}
 
 	if (!strcmp(var, "gpg.format")) {
-		if (strcasecmp(value, "openpgp"))
+		if (!get_format_data(value))
 			return error("malformed value for %s: %s", var, value);
 		return git_config_string(&gpg_format, var, value);
 	}
 
-	if (!strcmp(var, "gpg.program")) {
-		if (!value)
-			return config_error_nonbool(var);
-		gpg_program = xstrdup(value);
-		return 0;
-	}
-
+	if (!strcmp(var, "gpg.program"))
+		return git_config_string(&gpg_formats[PGP_FMT].program, var,
+					 value);
 	return 0;
 }
 
@@ -165,12 +194,16 @@ const char *get_signing_key(void)
 int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *signing_key)
 {
 	struct child_process gpg = CHILD_PROCESS_INIT;
+	struct gpg_format_data *fmt;
 	int ret;
 	size_t i, j, bottom;
 	struct strbuf gpg_status = STRBUF_INIT;
 
+	fmt = get_format_data(gpg_format);
+	if (!fmt)
+		BUG("bad gpg_format '%s'", gpg_format);
 	argv_array_pushl(&gpg.args,
-			 gpg_program,
+			 fmt->program,
 			 "--status-fd=2",
 			 "-bsau", signing_key,
 			 NULL);
@@ -208,8 +241,9 @@ int verify_signed_buffer(const char *payload, size_t payload_size,
 			 struct strbuf *gpg_output, struct strbuf *gpg_status)
 {
 	struct child_process gpg = CHILD_PROCESS_INIT;
+	struct gpg_format_data *fmt;
 	struct tempfile *temp;
-	int ret;
+	int ret, i;
 	struct strbuf buf = STRBUF_INIT;
 
 	temp = mks_tempfile_t(".git_vtag_tmpXXXXXX");
@@ -223,10 +257,18 @@ int verify_signed_buffer(const char *payload, size_t payload_size,
 		return -1;
 	}
 
+	fmt = get_format_data_by_sig(signature);
+	assert(fmt);
+
+	argv_array_pushl(&gpg.args,
+			 fmt->program, NULL);
+	for (i = 0; i < ARRAY_SIZE(fmt->extra_args_verify); i++)
+		if (fmt->extra_args_verify[i])
+			argv_array_pushl(&gpg.args,
+					 fmt->extra_args_verify[i], NULL);
+
 	argv_array_pushl(&gpg.args,
-			 gpg_program,
 			 "--status-fd=1",
-			 "--keyid-format=long",
 			 "--verify", temp->filename.buf, "-",
 			 NULL);
 
-- 
2.16.4

[PATCH v2 6/9] gpg-interface: do not hardcode the key string len anymore

[Henning Schild]

gnupg does print the keyid followed by a space and the signer comes
next. The same pattern is also used in gpgsm, but there the key length
would be 40 instead of 16. Instead of hardcoding the expected length,
find the first space and calculate it.

Signed-off-by: Henning Schild <henning.schild@siemens.com>
---
 gpg-interface.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/gpg-interface.c b/gpg-interface.c
index 0a8d1bff3..ac2df498d 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -88,10 +88,11 @@ static void parse_gpg_output(struct signature_check *sigc)
 		sigc->result = sigcheck_gpg_status[i].result;
 		/* The trust messages are not followed by key/signer information */
 		if (sigc->result != 'U') {
-			sigc->key = xmemdupz(found, 16);
+			next = strchrnul(found, ' ');
+			sigc->key = xmemdupz(found, next - found);
 			/* The ERRSIG message is not followed by signer information */
 			if (sigc-> result != 'E') {
-				found += 17;
+				found = next + 1;
 				next = strchrnul(found, '\n');
 				sigc->signer = xmemdupz(found, next - found);
 			}
-- 
2.16.4

[PATCH v2 7/9] gpg-interface: introduce new config to select per gpg format program

[Henning Schild]

Supporting multiple signing formats we will have the need to configure a
custom program each. Add a new config value to cater for that.

Signed-off-by: Henning Schild <henning.schild@siemens.com>
---
 Documentation/config.txt | 5 +++++
 gpg-interface.c          | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index ac373e3f4..c0bd80954 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1832,6 +1832,11 @@ gpg.format::
 	Specifies which key format to use when signing with `--gpg-sign`.
 	Default is "openpgp", that is also the only supported value.
 
+gpg.<format>.program::
+	Use this to customize the program used for the signing format you
+	chose. (see gpg.program) gpg.openpgp.program is a synonym for the
+	legacy gpg.program.
+
 gui.commitMsgWidth::
 	Defines how wide the commit message window is in the
 	linkgit:git-gui[1]. "75" is the default.
diff --git a/gpg-interface.c b/gpg-interface.c
index ac2df498d..65098430f 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -179,7 +179,7 @@ int git_gpg_config(const char *var, const char *value, void *cb)
 		return git_config_string(&gpg_format, var, value);
 	}
 
-	if (!strcmp(var, "gpg.program"))
+	if (!strcmp(var, "gpg.program") || !strcmp(var, "gpg.openpgp.program"))
 		return git_config_string(&gpg_formats[PGP_FMT].program, var,
 					 value);
 	return 0;
-- 
2.16.4

[PATCH v2 8/9] gpg-interface: introduce new signature format "x509" using gpgsm

[Henning Schild]

This commit allows git to create and check x509 type signatures using
gpgsm.

Signed-off-by: Henning Schild <henning.schild@siemens.com>
---
 Documentation/config.txt |  2 +-
 gpg-interface.c          | 10 +++++++++-
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index c0bd80954..b6f9b47d5 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1830,7 +1830,7 @@ gpg.program::
 
 gpg.format::
 	Specifies which key format to use when signing with `--gpg-sign`.
-	Default is "openpgp", that is also the only supported value.
+	Default is "opengpg" and another possible value is "x509".
 
 gpg.<format>.program::
 	Use this to customize the program used for the signing format you
diff --git a/gpg-interface.c b/gpg-interface.c
index 65098430f..bf8d567a4 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -16,13 +16,18 @@ struct gpg_format_data {
 
 #define PGP_SIGNATURE "-----BEGIN PGP SIGNATURE-----"
 #define PGP_MESSAGE "-----BEGIN PGP MESSAGE-----"
+#define X509_SIGNATURE "-----BEGIN SIGNED MESSAGE-----"
 
-enum gpgformats { PGP_FMT };
+enum gpgformats { PGP_FMT, X509_FMT };
 struct gpg_format_data gpg_formats[] = {
 	{ .format = "openpgp", .program = "gpg",
 	  .extra_args_verify = { "--keyid-format=long" },
 	  .sigs = { PGP_SIGNATURE, PGP_MESSAGE }
 	},
+	{ .format = "x509", .program = "gpgsm",
+	  .extra_args_verify = { NULL },
+	  .sigs = { X509_SIGNATURE, NULL }
+	},
 };
 static const char *gpg_format = "openpgp";
 
@@ -182,6 +187,9 @@ int git_gpg_config(const char *var, const char *value, void *cb)
 	if (!strcmp(var, "gpg.program") || !strcmp(var, "gpg.openpgp.program"))
 		return git_config_string(&gpg_formats[PGP_FMT].program, var,
 					 value);
+	if (!strcmp(var, "gpg.x509.program"))
+		return git_config_string(&gpg_formats[X509_FMT].program, var,
+					 value);
 	return 0;
 }
 
-- 
2.16.4

[PATCH v2 9/9] gpg-interface t: extend the existing GPG tests with GPGSM

[Henning Schild]

Add test cases to cover the new X509/gpgsm support. Most of them
resemble existing ones. They just switch the format to x509 and set the
signingkey when creating signatures. Validation of signatures does not
need any configuration of git, it does need gpgsm to be configured to
trust the key(-chain).
We generate a self-signed key for committer@example.com and configure
gpgsm to trust it.

Signed-off-by: Henning Schild <henning.schild@siemens.com>
---
 t/lib-gpg.sh               |  9 ++++++-
 t/lib-gpg/gpgsm-gen-key.in |  6 +++++
 t/t4202-log.sh             | 66 ++++++++++++++++++++++++++++++++++++++++++++++
 t/t5534-push-signed.sh     | 52 ++++++++++++++++++++++++++++++++++++
 t/t7003-filter-branch.sh   | 15 +++++++++++
 t/t7030-verify-tag.sh      | 47 +++++++++++++++++++++++++++++++--
 t/t7600-merge.sh           | 31 ++++++++++++++++++++++
 7 files changed, 223 insertions(+), 3 deletions(-)
 create mode 100644 t/lib-gpg/gpgsm-gen-key.in

diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh
index a5d3b2cba..9dcb4e990 100755
--- a/t/lib-gpg.sh
+++ b/t/lib-gpg.sh
@@ -38,7 +38,14 @@ then
 			"$TEST_DIRECTORY"/lib-gpg/ownertrust &&
 		gpg --homedir "${GNUPGHOME}" </dev/null >/dev/null 2>&1 \
 			--sign -u committer@example.com &&
-		test_set_prereq GPG
+		test_set_prereq GPG &&
+		echo | gpgsm --homedir "${GNUPGHOME}" -o "$TEST_DIRECTORY"/lib-gpg/gpgsm.crt.user --passphrase-fd 0 --pinentry-mode loopback --generate-key --batch "$TEST_DIRECTORY"/lib-gpg/gpgsm-gen-key.in &&
+		gpgsm --homedir "${GNUPGHOME}" --import "$TEST_DIRECTORY"/lib-gpg/gpgsm.crt.user &&
+		gpgsm --homedir "${GNUPGHOME}" -K | grep fingerprint: | cut -d" " -f4 | tr -d '\n' > ${GNUPGHOME}/trustlist.txt &&
+		echo " S relax" >> ${GNUPGHOME}/trustlist.txt &&
+		(gpgconf --kill gpg-agent >/dev/null 2>&1 || : ) &&
+		echo hello | gpgsm --homedir "${GNUPGHOME}" -u committer@example.com -o /dev/null --sign - 2>&1 &&
+		test_set_prereq GPGSM
 		;;
 	esac
 fi
diff --git a/t/lib-gpg/gpgsm-gen-key.in b/t/lib-gpg/gpgsm-gen-key.in
new file mode 100644
index 000000000..3470b9dc7
--- /dev/null
+++ b/t/lib-gpg/gpgsm-gen-key.in
@@ -0,0 +1,6 @@
+Key-Type: RSA
+Key-Length: 2048
+Key-Usage: sign
+Serial: random
+Name-DN: CN=C O Mitter, O=Example, SN=C O, GN=Mitter
+Name-Email: committer@example.com
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 25b1f8cc7..abbfa648c 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -1556,12 +1556,30 @@ test_expect_success GPG 'setup signed branch' '
 	git commit -S -m signed_commit
 '
 
+test_expect_success GPGSM 'setup signed branch x509' '
+	test_when_finished "git reset --hard && git checkout master" &&
+	git checkout -b signed-x509 master &&
+	echo foo >foo &&
+	git add foo &&
+	git config gpg.format x509 &&
+	git config user.signingkey $GIT_COMMITTER_EMAIL &&
+	git commit -S -m signed_commit &&
+	git config --unset gpg.format &&
+	git config --unset user.signingkey
+'
+
 test_expect_success GPG 'log --graph --show-signature' '
 	git log --graph --show-signature -n1 signed >actual &&
 	grep "^| gpg: Signature made" actual &&
 	grep "^| gpg: Good signature" actual
 '
 
+test_expect_success GPGSM 'log --graph --show-signature x509' '
+	git log --graph --show-signature -n1 signed-x509 >actual &&
+	grep "^| gpgsm: Signature made" actual &&
+	grep "^| gpgsm: Good signature" actual
+'
+
 test_expect_success GPG 'log --graph --show-signature for merged tag' '
 	test_when_finished "git reset --hard && git checkout master" &&
 	git checkout -b plain master &&
@@ -1581,11 +1599,39 @@ test_expect_success GPG 'log --graph --show-signature for merged tag' '
 	grep "^| | gpg: Good signature" actual
 '
 
+test_expect_success GPGSM 'log --graph --show-signature for merged tag x509' '
+	test_when_finished "git reset --hard && git checkout master" &&
+	git config gpg.format x509 &&
+	git config user.signingkey $GIT_COMMITTER_EMAIL &&
+	git checkout -b plain-x509 master &&
+	echo aaa >bar &&
+	git add bar &&
+	git commit -m bar_commit &&
+	git checkout -b tagged-x509 master &&
+	echo bbb >baz &&
+	git add baz &&
+	git commit -m baz_commit &&
+	git tag -s -m signed_tag_msg signed_tag_x509 &&
+	git checkout plain-x509 &&
+	git merge --no-ff -m msg signed_tag_x509 &&
+	git log --graph --show-signature -n1 plain-x509 >actual &&
+	grep "^|\\\  merged tag" actual &&
+	grep "^| | gpgsm: Signature made" actual &&
+	grep "^| | gpgsm: Good signature" actual &&
+	git config --unset gpg.format &&
+	git config --unset user.signingkey
+'
+
 test_expect_success GPG '--no-show-signature overrides --show-signature' '
 	git log -1 --show-signature --no-show-signature signed >actual &&
 	! grep "^gpg:" actual
 '
 
+test_expect_success GPGSM '--no-show-signature overrides --show-signature x509' '
+	git log -1 --show-signature --no-show-signature signed-x509 >actual &&
+	! grep "^gpgsm:" actual
+'
+
 test_expect_success GPG 'log.showsignature=true behaves like --show-signature' '
 	test_config log.showsignature true &&
 	git log -1 signed >actual &&
@@ -1593,12 +1639,25 @@ test_expect_success GPG 'log.showsignature=true behaves like --show-signature' '
 	grep "gpg: Good signature" actual
 '
 
+test_expect_success GPGSM 'log.showsignature=true behaves like --show-signature x509' '
+	test_config log.showsignature true &&
+	git log -1 signed-x509 >actual &&
+	grep "gpgsm: Signature made" actual &&
+	grep "gpgsm: Good signature" actual
+'
+
 test_expect_success GPG '--no-show-signature overrides log.showsignature=true' '
 	test_config log.showsignature true &&
 	git log -1 --no-show-signature signed >actual &&
 	! grep "^gpg:" actual
 '
 
+test_expect_success GPGSM '--no-show-signature overrides log.showsignature=true x509' '
+	test_config log.showsignature true &&
+	git log -1 --no-show-signature signed-x509 >actual &&
+	! grep "^gpgsm:" actual
+'
+
 test_expect_success GPG '--show-signature overrides log.showsignature=false' '
 	test_config log.showsignature false &&
 	git log -1 --show-signature signed >actual &&
@@ -1606,6 +1665,13 @@ test_expect_success GPG '--show-signature overrides log.showsignature=false' '
 	grep "gpg: Good signature" actual
 '
 
+test_expect_success GPGSM '--show-signature overrides log.showsignature=false x509' '
+	test_config log.showsignature false &&
+	git log -1 --show-signature signed-x509 >actual &&
+	grep "gpgsm: Signature made" actual &&
+	grep "gpgsm: Good signature" actual
+'
+
 test_expect_success 'log --graph --no-walk is forbidden' '
 	test_must_fail git log --graph --no-walk
 '
diff --git a/t/t5534-push-signed.sh b/t/t5534-push-signed.sh
index 1cea758f7..ef2789afa 100755
--- a/t/t5534-push-signed.sh
+++ b/t/t5534-push-signed.sh
@@ -218,4 +218,56 @@ test_expect_success GPG 'fail without key and heed user.signingkey' '
 	test_cmp expect dst/push-cert-status
 '
 
+test_expect_success GPGSM 'fail without key and heed user.signingkey x509' '
+	git config gpg.format x509 &&
+	env | grep GIT > envfile &&
+	prepare_dst &&
+	mkdir -p dst/.git/hooks &&
+	git -C dst config receive.certnonceseed sekrit &&
+	write_script dst/.git/hooks/post-receive <<-\EOF &&
+	# discard the update list
+	cat >/dev/null
+	# record the push certificate
+	if test -n "${GIT_PUSH_CERT-}"
+	then
+		git cat-file blob $GIT_PUSH_CERT >../push-cert
+	fi &&
+
+	cat >../push-cert-status <<E_O_F
+	SIGNER=${GIT_PUSH_CERT_SIGNER-nobody}
+	KEY=${GIT_PUSH_CERT_KEY-nokey}
+	STATUS=${GIT_PUSH_CERT_STATUS-nostatus}
+	NONCE_STATUS=${GIT_PUSH_CERT_NONCE_STATUS-nononcestatus}
+	NONCE=${GIT_PUSH_CERT_NONCE-nononce}
+	E_O_F
+
+	EOF
+	unset GIT_COMMITTER_EMAIL &&
+	git config user.email hasnokey@nowhere.com &&
+	git config user.signingkey "" &&
+	test_must_fail git push --signed dst noop ff +noff &&
+	git config user.signingkey committer@example.com &&
+	git push --signed dst noop ff +noff &&
+
+	(
+		cat <<-\EOF &&
+		SIGNER=/CN=C O Mitter/O=Example/SN=C O/GN=Mitter
+		KEY=
+		STATUS=G
+		NONCE_STATUS=OK
+		EOF
+		sed -n -e "s/^nonce /NONCE=/p" -e "/^$/q" dst/push-cert
+	) >expect.in &&
+	key=$(cat "${GNUPGHOME}/trustlist.txt" | cut -d" " -f1 | tr -d ":") &&
+	sed -e "s/^KEY=/KEY=${key}/" expect.in > expect &&
+
+	noop=$(git rev-parse noop) &&
+	ff=$(git rev-parse ff) &&
+	noff=$(git rev-parse noff) &&
+	grep "$noop $ff refs/heads/ff" dst/push-cert &&
+	grep "$noop $noff refs/heads/noff" dst/push-cert &&
+	test_cmp expect dst/push-cert-status
+'
+
+
 test_done
diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh
index ec4b160dd..d1ce7a189 100755
--- a/t/t7003-filter-branch.sh
+++ b/t/t7003-filter-branch.sh
@@ -309,6 +309,21 @@ test_expect_success GPG 'Filtering retains message of gpg signed commit' '
 	test_cmp expect actual
 '
 
+test_expect_success GPGSM 'Filtering retains message of gpgsm signed commit' '
+	mkdir gpgsm &&
+	touch gpgsm/foo &&
+	git add gpgsm &&
+	git config gpg.format x509 &&
+	git config user.signingkey $GIT_COMMITTER_EMAIL &&
+	test_tick &&
+	git commit -S -m "Adding gpgsm" &&
+
+	git log -1 --format="%s" > expect &&
+	git filter-branch -f --msg-filter "cat" &&
+	git log -1 --format="%s" > actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'Tag name filtering allows slashes in tag names' '
 	git tag -m tag-with-slash X/1 &&
 	git cat-file tag X/1 | sed -e s,X/1,X/2, > expect &&
diff --git a/t/t7030-verify-tag.sh b/t/t7030-verify-tag.sh
index 291a1e2b0..510e643cf 100755
--- a/t/t7030-verify-tag.sh
+++ b/t/t7030-verify-tag.sh
@@ -41,6 +41,13 @@ test_expect_success GPG 'create signed tags' '
 	git tag -uB7227189 -m eighth eighth-signed-alt
 '
 
+test_expect_success GPGSM 'create signed tags x509 ' '
+	git config gpg.format x509 &&
+	git config user.signingkey $GIT_COMMITTER_EMAIL &&
+	echo 9 >file && test_tick && git commit -a -m "nineth gpgsm-signed" &&
+	git tag -s -m nineth nineth-signed-x509
+'
+
 test_expect_success GPG 'verify and show signatures' '
 	(
 		for tag in initial second merge fourth-signed sixth-signed seventh-signed
@@ -72,6 +79,18 @@ test_expect_success GPG 'verify and show signatures' '
 	)
 '
 
+test_expect_success GPGSM 'verify and show signatures x509' '
+	(
+		for tag in nineth-signed-x509
+		do
+			git verify-tag $tag 2>actual &&
+			grep "Good signature from" actual &&
+			! grep "BAD signature from" actual &&
+			echo $tag OK || exit 1
+		done
+	)
+'
+
 test_expect_success GPG 'detect fudged signature' '
 	git cat-file tag seventh-signed >raw &&
 	sed -e "/^tag / s/seventh/7th forged/" raw >forged1 &&
@@ -112,8 +131,32 @@ test_expect_success GPG 'verify signatures with --raw' '
 	)
 '
 
-test_expect_success GPG 'verify multiple tags' '
-	tags="fourth-signed sixth-signed seventh-signed" &&
+test_expect_success GPGSM 'verify signatures with --raw x509' '
+	(
+		for tag in nineth-signed-x509
+		do
+			git verify-tag --raw $tag 2>actual &&
+			grep "GOODSIG" actual &&
+			! grep "BADSIG" actual &&
+			echo $tag OK || exit 1
+		done
+	)
+'
+test_expect_success GPGSM 'verify multiple tags' '
+	tags="fourth-signed sixth-signed seventh-signed nineth-signed-x509" &&
+	for i in $tags
+	do
+		git verify-tag -v --raw $i || return 1
+	done >expect.stdout 2>expect.stderr.1 &&
+	grep "^.GNUPG:." <expect.stderr.1 >expect.stderr &&
+	git verify-tag -v --raw $tags >actual.stdout 2>actual.stderr.1 &&
+	grep "^.GNUPG:." <actual.stderr.1 >actual.stderr &&
+	test_cmp expect.stdout actual.stdout &&
+	test_cmp expect.stderr actual.stderr
+'
+
+test_expect_success GPGSM 'verify multiple tags x509' '
+	tags="fourth-signed sixth-signed seventh-signed nineth-signed-x509" &&
 	for i in $tags
 	do
 		git verify-tag -v --raw $i || return 1
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index 6736d8d13..db18b6c31 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -747,6 +747,21 @@ test_expect_success GPG 'merge --ff-only tag' '
 	git rev-parse HEAD >actual &&
 	test_cmp expect actual
 '
+test_expect_success GPGSM 'merge --ff-only tag x509' '
+	git reset --hard c0 &&
+	git commit --allow-empty -m "A newer commit" &&
+	git config gpg.format x509 &&
+	git config user.signingkey $GIT_COMMITTER_EMAIL &&
+	git tag -s -m "A newer commit" signed-x509 &&
+	git reset --hard c0 &&
+
+	git merge --ff-only signed &&
+	git rev-parse signed^0 >expect &&
+	git rev-parse HEAD >actual &&
+	test_cmp expect actual &&
+	git config --unset gpg.format &&
+	git config --unset user.signingkey
+'
 
 test_expect_success GPG 'merge --no-edit tag should skip editor' '
 	git reset --hard c0 &&
@@ -760,6 +775,22 @@ test_expect_success GPG 'merge --no-edit tag should skip editor' '
 	test_cmp expect actual
 '
 
+test_expect_success GPGSM 'merge --no-edit tag should skip editor x509' '
+	git reset --hard c0 &&
+	git commit --allow-empty -m "A newer commit" &&
+	git config gpg.format x509 &&
+	git config user.signingkey $GIT_COMMITTER_EMAIL &&
+	git tag -f -s -m "A newer commit" signed &&
+	git reset --hard c0 &&
+
+	EDITOR=false git merge --no-edit --no-ff signed-x509 &&
+	git rev-parse signed^0 >expect &&
+	git rev-parse HEAD^2 >actual &&
+	test_cmp expect actual &&
+	git config --unset gpg.format &&
+	git config --unset user.signingkey
+'
+
 test_expect_success 'set up mod-256 conflict scenario' '
 	# 256 near-identical stanzas...
 	for i in $(test_seq 1 256); do
-- 
2.16.4

Re: [PATCH v2 6/9] gpg-interface: do not hardcode the key string len anymore

[Jeff King]

On Tue, Jul 10, 2018 at 10:52:28AM +0200, Henning Schild wrote:

> gnupg does print the keyid followed by a space and the signer comes
> next. The same pattern is also used in gpgsm, but there the key length
> would be 40 instead of 16. Instead of hardcoding the expected length,
> find the first space and calculate it.

Sounds good, but I think there's an off-by-one in the patch.

> diff --git a/gpg-interface.c b/gpg-interface.c
> index 0a8d1bff3..ac2df498d 100644
> --- a/gpg-interface.c
> +++ b/gpg-interface.c
> @@ -88,10 +88,11 @@ static void parse_gpg_output(struct signature_check *sigc)
>  		sigc->result = sigcheck_gpg_status[i].result;
>  		/* The trust messages are not followed by key/signer information */
>  		if (sigc->result != 'U') {
> -			sigc->key = xmemdupz(found, 16);
> +			next = strchrnul(found, ' ');
> +			sigc->key = xmemdupz(found, next - found);

Here "next" may point to the trailing NUL of the string...

>  			/* The ERRSIG message is not followed by signer information */
>  			if (sigc-> result != 'E') {
> -				found += 17;
> +				found = next + 1;
>  				next = strchrnul(found, '\n');

...in which case "found" points past the end of the string, and we
search random memory. That's presumably impossible with well-formed gpg
output (you don't get 'E' without an extra message), but we should be
robust against bogus input.

In the general case you need:

  found = *next ? next + 1 : next;

or similar. In this case, you can actually do:

  found = next;

because we know that it's OK to search over the literal space again. But
that's pretty subtle, so we're probably better off just doing the
conditional above.

(And yes, looking at the existing code, I think it's even worse, as
there does not seem to be a guarantee that we even have 16 characters in
the string).

-Peff

Re: [PATCH v2 4/9] t/t7510: check the validation of the new config gpg.format

[Jeff King]

On Tue, Jul 10, 2018 at 10:52:26AM +0200, Henning Schild wrote:

> diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
> index 6e2015ed9..7e1e9caf4 100755
> --- a/t/t7510-signed-commit.sh
> +++ b/t/t7510-signed-commit.sh
> @@ -227,4 +227,14 @@ test_expect_success GPG 'log.showsignature behaves like --show-signature' '
>  	grep "gpg: Good signature" actual
>  '
>  
> +test_expect_success GPG 'check config gpg.format values' '
> +	rm .git/config &&
> +	test_config gpg.format openpgp &&
> +	git commit -S --amend -m "success" &&

You shouldn't need this "rm" here. test_config will add your config, and
then delete it after the test finishes.

I know you probably saw that in t1300 or nearby tests, but IMHO they are
wrong to do so. It's a historical wart that should be cleaned up.

> +	test_config gpg.format OpEnPgP &&
> +	git commit -S --amend -m "success" &&

A bit of a funny side effect is that we'll unset gpg.format three times
at the end of the test, since each test_config doesn't know that the
earlier invocations touched the same variable.

It's probably not worth addressing, but we could do it with an explicit:

  test_when_finished "test_unconfig gpg.format" &&
  git config gpg.format openpgp &&
  ...
  git config gpg.format OpEnPgP &&

Or alternatively, this could be three independent tests, which would
give the opportunity to describe each.

> +	test_config gpg.format malformed &&
> +	test_must_fail git commit -S --amend -m "fail" 2>result
> +'

If you're not going to look at the saved "result", we are better to just
leave stderr un-redirected. It will go to /dev/null by default, or to
the user-visible output of the test is run in verbose mode.

-Peff

Re: [PATCH v2 3/9] gpg-interface: add new config to select how to sign a commit

[Jeff King]

On Tue, Jul 10, 2018 at 10:52:25AM +0200, Henning Schild wrote:

> @@ -138,6 +139,12 @@ int git_gpg_config(const char *var, const char *value, void *cb)
>  		return 0;
>  	}
>  
> +	if (!strcmp(var, "gpg.format")) {
> +		if (strcasecmp(value, "openpgp"))
> +			return error("malformed value for %s: %s", var, value);
> +		return git_config_string(&gpg_format, var, value);
> +	}

I know we discussed names and case-sensitivity a little bit off-list.
FWIW, this name looks good to me.

-Peff

Re: [PATCH v2 5/9] gpg-interface: introduce an abstraction for multiple gpg formats

[Jeff King]

On Tue, Jul 10, 2018 at 10:52:27AM +0200, Henning Schild wrote:

> Create a struct that holds the format details for the supported formats.
> At the moment that is still just "openpgp". This commit prepares for the
> introduction of more formats, that might use other programs and match
> other signatures.

Great, this looks like a good incremental step.

>  static char *configured_signing_key;
> -static const char *gpg_format = "openpgp";
> -static const char *gpg_program = "gpg";
> +struct gpg_format_data {
> +	const char *format;
> +	const char *program;
> +	const char *extra_args_verify[1];
> +	const char *sigs[2];
> +};

These magic numbers are at a weird distance from where we fill them in:

> +struct gpg_format_data gpg_formats[] = {
> +	{ .format = "openpgp", .program = "gpg",
> +	  .extra_args_verify = { "--keyid-format=long" },
> +	  .sigs = { PGP_SIGNATURE, PGP_MESSAGE }
> +	},
> +};

I'm not sure if we can easily do any better in C, though. Declaring the
struct with an open-ended "[]" would make the compiler unhappy. We could
do something like:

  struct gpg_format_data {
	...
	const char **extra_args_verify;
  };
  ...
  static const char *openpgp_verify_args[] = {
	"--key-id-format=long"
  };
  ...
  static struct gpg_format_data gpg_formats[] = {
	{ ...
	  .extra_args_verify = openpgp_verify_args
	}
  };

I'm not sure if that's more horrible or less. It's worse to write in the
first place, but it's slightly easier to maintain going forward. I
dunno.

> +enum gpgformats { PGP_FMT };

Looks like we use this only for indexing the gpg_formats array. I know
that C guarantees 0-indexing, but if we're depending on it, it might be
worth writing out "PGP_FMT = 0" explicitly. And probably adding a
comment that this needs to remain in sync with the array.

The other alternative is that we could simply use
get_format_data("openpgp"), though that does add a minor runtime cost.

> +struct gpg_format_data gpg_formats[] = {
> +	{ .format = "openpgp", .program = "gpg",
> +	  .extra_args_verify = { "--keyid-format=long" },
> +	  .sigs = { PGP_SIGNATURE, PGP_MESSAGE }
> +	},
> +};

This array should be marked static, I think.

> +static struct gpg_format_data *get_format_data(const char *str)
> +{
> +	int i;
> +	for (i = 0; i < ARRAY_SIZE(gpg_formats); i++)
> +		if (!strcasecmp(gpg_formats[i].format, str))
> +			return gpg_formats + i;
> +	return NULL;
> +}

This looks much nicer than the assert()-ing version from v1.

> +static struct gpg_format_data *get_format_data_by_sig(const char *sig)
> +{
> +	int i, j;
> +	for (i = 0; i < ARRAY_SIZE(gpg_formats); i++)
> +		for (j = 0; j < ARRAY_SIZE(gpg_formats[i].sigs); j++)
> +			if (gpg_formats[i].sigs[j] && 
> +			    !strncmp(gpg_formats[i].sigs[j], sig,
> +				     strlen(gpg_formats[i].sigs[j])))
> +				return gpg_formats + i;
> +	return NULL;
> +}

This might be a little more readable with:

  starts_with(sig, gpg_formats[i].sigs[j])

instead of the strncmp. It may also be more efficient, as we don't have
to compute the strlen of the prefix for each non-matching line (the
compiler _might_ be smart enough to realize these are all string
literals, but it's pretty buried).

I also wondered if our prefix matching here is overly loose. We have to
do a prefix match, since "sig" isn't terminated at the line buffer. So I
think we'd match:

  --- BEGIN PGP MESSAGE --- AND SOME OTHER STUFF ---

on a line. But I think that's no different than the current code. If we
care, I guess we could look for '\n' or '\0' immediately after.

>  static int is_gpg_start(const char *line)
>  {
> -	return starts_with(line, PGP_SIGNATURE) ||
> -		starts_with(line, PGP_MESSAGE);
> +	return (get_format_data_by_sig(line) != NULL);
>  }

I don't know if we've ever discussed this style explicitly, but we'd
usually omit the unnecessary parentheses for the return here.

> @@ -140,18 +173,14 @@ int git_gpg_config(const char *var, const char *value, void *cb)
>  	}
>  
>  	if (!strcmp(var, "gpg.format")) {
> -		if (strcasecmp(value, "openpgp"))
> +		if (!get_format_data(value))
>  			return error("malformed value for %s: %s", var, value);
>  		return git_config_string(&gpg_format, var, value);
>  	}

Much nicer than v1.

> @@ -165,12 +194,16 @@ const char *get_signing_key(void)
>  int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *signing_key)
>  {
>  	struct child_process gpg = CHILD_PROCESS_INIT;
> +	struct gpg_format_data *fmt;
>  	int ret;
>  	size_t i, j, bottom;
>  	struct strbuf gpg_status = STRBUF_INIT;
>  
> +	fmt = get_format_data(gpg_format);
> +	if (!fmt)
> +		BUG("bad gpg_format '%s'", gpg_format);

This makes sense as a BUG, because we would already have validated it
when parsing gpg.format earlier. That does make me wonder if we should
simply be storing a "struct gpg_format_data" instead of a string,
though. I.e., at the top-level:

  /* default to signing with openpgp */
  static struct gpg_format_data *gpg_format = &gpg_formats[0];

> @@ -223,10 +257,18 @@ int verify_signed_buffer(const char *payload, size_t payload_size,
>  		return -1;
>  	}
>  
> +	fmt = get_format_data_by_sig(signature);
> +	assert(fmt);

Is this assert() right? The signature data comes from the user. I guess
to get here we'll already have matched their signature via
is_gpg_start(), and this is just a cross-check? If so, then it's OK to
assert, but a BUG() with a descriptive message would be better still.

I also wonder if whoever parses the signature should get back a
gpg_format_data and just pass it in here, so we don't have to reparse.
That's what my earlier series did. It requires tweaking the function
signatures, but IMHO the result was a lot more obvious.

> +	argv_array_pushl(&gpg.args,
> +			 fmt->program, NULL);

If you're just pushing one thing, you don't need pushl(). You can just:

  argv_array_push(&gpg.args, fmt->program);

> +	for (i = 0; i < ARRAY_SIZE(fmt->extra_args_verify); i++)
> +		if (fmt->extra_args_verify[i])
> +			argv_array_pushl(&gpg.args,
> +					 fmt->extra_args_verify[i], NULL);

Likewise here. Though if you made extra_args_verify a NULL-terminated
list, this whole loop could become:

  argv_array_pushv(&gpg.args, fmt->extra_args_verify);

It's not _that_ much code, but I think using NULL-terminated lists in a
situation like this is more idiomatic for our code base.

-Peff

Re: [PATCH v2 2/9] gpg-interface: make parse_gpg_output static and remove from interface header

[Junio C Hamano]

Henning Schild <henning.schild@siemens.com> writes:

> This commit turns parse_gpg_output into an internal function, the only
> outside user was migrated in an earlier commit.

It is not too big a deal but as we prefer to see our history speak
in consistent voice, we would usually phrase the above as if we are
giving an order to "make it so" to the codebase, e.g.

	Turn parse_gpg_output() into a static function, as the only
	outside user was removed in the previous step.

or something like that.

These two steps, as you said earlier, are nice clean-up patches
whose goodness can be measured independently, regardless of the
gpgsm support which is the primary focus of this series.

Thanks.

>
> Signed-off-by: Henning Schild <henning.schild@siemens.com>
> ---
>  gpg-interface.c | 2 +-
>  gpg-interface.h | 2 --
>  2 files changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/gpg-interface.c b/gpg-interface.c
> index 0647bd634..09ddfbc26 100644
> --- a/gpg-interface.c
> +++ b/gpg-interface.c
> @@ -35,7 +35,7 @@ static struct {
>  	{ 'R', "\n[GNUPG:] REVKEYSIG "},
>  };
>  
> -void parse_gpg_output(struct signature_check *sigc)
> +static void parse_gpg_output(struct signature_check *sigc)
>  {
>  	const char *buf = sigc->gpg_status;
>  	int i;
> diff --git a/gpg-interface.h b/gpg-interface.h
> index a5e6517ae..5ecff4aa0 100644
> --- a/gpg-interface.h
> +++ b/gpg-interface.h
> @@ -33,8 +33,6 @@ void signature_check_clear(struct signature_check *sigc);
>   */
>  size_t parse_signature(const char *buf, size_t size);
>  
> -void parse_gpg_output(struct signature_check *);
> -
>  /*
>   * Create a detached signature for the contents of "buffer" and append
>   * it after "signature"; "buffer" and "signature" can be the same

Re: [PATCH v2 7/9] gpg-interface: introduce new config to select per gpg format program

[Jeff King]

On Tue, Jul 10, 2018 at 10:52:29AM +0200, Henning Schild wrote:

> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index ac373e3f4..c0bd80954 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1832,6 +1832,11 @@ gpg.format::
>  	Specifies which key format to use when signing with `--gpg-sign`.
>  	Default is "openpgp", that is also the only supported value.
>  
> +gpg.<format>.program::
> +	Use this to customize the program used for the signing format you
> +	chose. (see gpg.program) gpg.openpgp.program is a synonym for the
> +	legacy gpg.program.

This seems like a good step forward. This is similar to the
signingtool.$name.program I proposed earlier, but keeping it specific to
gpg, which makes sense.

On the other hand, do we anticipate the user ever being able to add
gpg.foo.program? I don't think so; we'll provide a limited set of
options. So we _could_ go with "gpg.openpgpProgram" or similar, and
later add "gpg.x509Program".

And one reason to do so might be...

> diff --git a/gpg-interface.c b/gpg-interface.c
> index ac2df498d..65098430f 100644
> --- a/gpg-interface.c
> +++ b/gpg-interface.c
> @@ -179,7 +179,7 @@ int git_gpg_config(const char *var, const char *value, void *cb)
>  		return git_config_string(&gpg_format, var, value);
>  	}
>  
> -	if (!strcmp(var, "gpg.program"))
> +	if (!strcmp(var, "gpg.program") || !strcmp(var, "gpg.openpgp.program"))
>  		return git_config_string(&gpg_formats[PGP_FMT].program, var,
>  					 value);

We normally match config keys with strcmp() because the config machinery
will have already normalized them to lowercase. But in Git's config
format, the subsection (the middle in a three-dot name) is less
restricted and is case-sensitive.

Should we allow:

  [gpg "OpenPGP"]
  program = whatever

given that we allow:

  [gpg]
  format = OpenPGP

? I think just using strcasecmp() here would be sufficient. But I wonder
if it is a symptom of using the wrong tool (subsections) when we don't
need it.

-Peff

Re: [PATCH v2 4/9] t/t7510: check the validation of the new config gpg.format

[Junio C Hamano]

Henning Schild <henning.schild@siemens.com> writes:

> Test setting gpg.format to both invalid and valid values.
>
> Signed-off-by: Henning Schild <henning.schild@siemens.com>
> ---
>  t/t7510-signed-commit.sh | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
> index 6e2015ed9..7e1e9caf4 100755
> --- a/t/t7510-signed-commit.sh
> +++ b/t/t7510-signed-commit.sh
> @@ -227,4 +227,14 @@ test_expect_success GPG 'log.showsignature behaves like --show-signature' '
>  	grep "gpg: Good signature" actual
>  '
>  
> +test_expect_success GPG 'check config gpg.format values' '
> +	rm .git/config &&

Please don't.  .git/config has stuff that are more important than
just collection of random configuration these days, and we can even
expect that future versions of Git may not store its config in a
flat file .git/config but in a different mechanism "git config"
command knows how to access.  A low-level test for "git config"
command's operation may have to be implemented by inspecting the
resulting .git/config, but as this test is not about "git config"'s
inner workins but is about one feature "git commit" command has,
we prefer not to depend too much on the internal implementation
detail such as "local config is stored in .git/config file".

Let's hear why you want to remove this file; what things that have
previously been placed in the file do you want not to see, before
performing the following actions?  Once we know that, we can suggest
a way to do so better than removing the entire file.

> +	test_config gpg.format openpgp &&
> +	git commit -S --amend -m "success" &&
> +	test_config gpg.format OpEnPgP &&
> +	git commit -S --amend -m "success" &&
> +	test_config gpg.format malformed &&
> +	test_must_fail git commit -S --amend -m "fail" 2>result
> +'
> +
>  test_done

Re: [PATCH v2 7/9] gpg-interface: introduce new config to select per gpg format program

[Jeff King]

On Tue, Jul 10, 2018 at 12:54:13PM -0400, Jeff King wrote:

> Should we allow:
> 
>   [gpg "OpenPGP"]
>   program = whatever
> 
> given that we allow:
> 
>   [gpg]
>   format = OpenPGP
> 
> ? I think just using strcasecmp() here would be sufficient. But I wonder
> if it is a symptom of using the wrong tool (subsections) when we don't
> need it.

I did just read the discussion in response to v1, where everybody told
you the opposite. ;)

So I guess my question/points are more for brian and Junio.

-Peff

Re: [PATCH v2 8/9] gpg-interface: introduce new signature format "x509" using gpgsm

[Jeff King]

On Tue, Jul 10, 2018 at 10:52:30AM +0200, Henning Schild wrote:

> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index c0bd80954..b6f9b47d5 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1830,7 +1830,7 @@ gpg.program::
>  
>  gpg.format::
>  	Specifies which key format to use when signing with `--gpg-sign`.
> -	Default is "openpgp", that is also the only supported value.
> +	Default is "opengpg" and another possible value is "x509".

opengpg?

Since we're having so much fun with naming discussions, let's talk about
"x509". :)

That's the cert format. I think of these signatures as S/MIME, but
really that's the mail-oriented parts of the standard. I think
technically this is "CMS".

That said, we should pick what most people will find natural when
referring to it. So maybe x509 isn't the worst choice, as I doubt most
people know the term CMS. Probably the term they know _most_ is "gpgsm",
but I think the point is that one does not have to be using gpgsm in the
first place.

So I dunno. I think I talked myself back into x509. ;)

> diff --git a/gpg-interface.c b/gpg-interface.c
> index 65098430f..bf8d567a4 100644
> --- a/gpg-interface.c
> +++ b/gpg-interface.c
> @@ -16,13 +16,18 @@ struct gpg_format_data {
>  
>  #define PGP_SIGNATURE "-----BEGIN PGP SIGNATURE-----"
>  #define PGP_MESSAGE "-----BEGIN PGP MESSAGE-----"
> +#define X509_SIGNATURE "-----BEGIN SIGNED MESSAGE-----"
>  
> -enum gpgformats { PGP_FMT };
> +enum gpgformats { PGP_FMT, X509_FMT };
>  struct gpg_format_data gpg_formats[] = {
>  	{ .format = "openpgp", .program = "gpg",
>  	  .extra_args_verify = { "--keyid-format=long" },
>  	  .sigs = { PGP_SIGNATURE, PGP_MESSAGE }
>  	},
> +	{ .format = "x509", .program = "gpgsm",
> +	  .extra_args_verify = { NULL },
> +	  .sigs = { X509_SIGNATURE, NULL }
> +	},

Extremely minor nit, but if there are no other uses of PGP_SIGNATURE etc
outside of this array (as I hope there wouldn't be after this series),
would it make more sense to just include the literals inline in the
array definition? That's one less layer of indirection when somebody is
reading the code.

-Peff

Re: [PATCH v2 9/9] gpg-interface t: extend the existing GPG tests with GPGSM

[Jeff King]

On Tue, Jul 10, 2018 at 10:52:31AM +0200, Henning Schild wrote:

> diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh
> index a5d3b2cba..9dcb4e990 100755
> --- a/t/lib-gpg.sh
> +++ b/t/lib-gpg.sh
> @@ -38,7 +38,14 @@ then
>  			"$TEST_DIRECTORY"/lib-gpg/ownertrust &&
>  		gpg --homedir "${GNUPGHOME}" </dev/null >/dev/null 2>&1 \
>  			--sign -u committer@example.com &&
> -		test_set_prereq GPG
> +		test_set_prereq GPG &&
> +		echo | gpgsm --homedir "${GNUPGHOME}" -o "$TEST_DIRECTORY"/lib-gpg/gpgsm.crt.user --passphrase-fd 0 --pinentry-mode loopback --generate-key --batch "$TEST_DIRECTORY"/lib-gpg/gpgsm-gen-key.in &&

Is this --generate-key going to require high-quality entropy? That could
slow the test suite down (and maybe even stall it quite a bit on servers
doing automated CI).

Can we save a dummy generated key and just import it? That's what we do
for the regular gpg case.

> +		gpgsm --homedir "${GNUPGHOME}" --import "$TEST_DIRECTORY"/lib-gpg/gpgsm.crt.user &&
> +		gpgsm --homedir "${GNUPGHOME}" -K | grep fingerprint: | cut -d" " -f4 | tr -d '\n' > ${GNUPGHOME}/trustlist.txt &&
> +		echo " S relax" >> ${GNUPGHOME}/trustlist.txt &&
> +		(gpgconf --kill gpg-agent >/dev/null 2>&1 || : ) &&
> +		echo hello | gpgsm --homedir "${GNUPGHOME}" -u committer@example.com -o /dev/null --sign - 2>&1 &&
> +		test_set_prereq GPGSM

This &&-chain means we can't have GPGSM without GPG. In theory the two
could be tested independently. I don't know if it's worth the trouble to
make that work, though. I wouldn't be surprised if there are some subtle
dependencies within the test scripts, and I'm not sure how common it is
for somebody to have gpgsm and not gpg. So it may make sense to just
punt on it until such a person appears.

>  test_expect_success GPG 'log --graph --show-signature' '
>  	git log --graph --show-signature -n1 signed >actual &&
>  	grep "^| gpg: Signature made" actual &&
>  	grep "^| gpg: Good signature" actual
>  '
>  
> +test_expect_success GPGSM 'log --graph --show-signature x509' '
> +	git log --graph --show-signature -n1 signed-x509 >actual &&
> +	grep "^| gpgsm: Signature made" actual &&
> +	grep "^| gpgsm: Good signature" actual
> +'

We're going to have a lot of duplicated tests here. That's a maintenance
burden when one of them needs fixes later. And when new tests are added,
we won't automatically get them tested under each format.

Can we move the battery of tests into a function that takes a few
parameters (prereq name, branch to look at, etc) and then call it for
both the gpg/gpgsm cases?

-Peff

Re: [PATCH v2 0/9] X509 (gpgsm) commit signing support

[Jeff King]

On Tue, Jul 10, 2018 at 10:52:22AM +0200, Henning Schild wrote:

> This series adds support for signing commits with gpgsm.

Thanks for working on this. I left a bunch of comments, but overall the
direction looks good.

We talked about this a bit off-list, but just for the public record:

> This series can be seen as a follow up of a series that appeared under
> the name "gpg-interface: Multiple signing tools" in april 2018 [1]. After
> that series was not merged i decided to get my patches ready. The
> original series aimed at being generic for any sort of signing tool, while
> this series just introduced the X509 variant of gpg. (gpgsm)
> I collected authors and reviewers of that first series and already put them
> on cc.

This series is a fine replacement for that earlier work. It's flexible
enough to allow what we really wanted out of that series (gpgsm support,
or another drop-in tool that uses the same interface). It doesn't lay
any groundwork for further tools (like signify), but I think the
consensus on the list was to punt on that until somebody had more
concrete plans for adding such a tool.

-Peff

Re: [PATCH v2 9/9] gpg-interface t: extend the existing GPG tests with GPGSM

[Jeff King]

On Tue, Jul 10, 2018 at 01:09:01PM -0400, Jeff King wrote:

> > +		gpgsm --homedir "${GNUPGHOME}" --import "$TEST_DIRECTORY"/lib-gpg/gpgsm.crt.user &&
> > +		gpgsm --homedir "${GNUPGHOME}" -K | grep fingerprint: | cut -d" " -f4 | tr -d '\n' > ${GNUPGHOME}/trustlist.txt &&
> > +		echo " S relax" >> ${GNUPGHOME}/trustlist.txt &&
> > +		(gpgconf --kill gpg-agent >/dev/null 2>&1 || : ) &&
> > +		echo hello | gpgsm --homedir "${GNUPGHOME}" -u committer@example.com -o /dev/null --sign - 2>&1 &&
> > +		test_set_prereq GPGSM
> 
> This &&-chain means we can't have GPGSM without GPG. In theory the two
> could be tested independently. I don't know if it's worth the trouble to
> make that work, though. I wouldn't be surprised if there are some subtle
> dependencies within the test scripts, and I'm not sure how common it is
> for somebody to have gpgsm and not gpg. So it may make sense to just
> punt on it until such a person appears.

Oof, sorry, I thought I had read all of the v1 review, but I just
noticed I missed a similar comment from brian. So it sounds like you
already considered this, and just ignore what I wrote here.

-Peff

Re: [PATCH v2 5/9] gpg-interface: introduce an abstraction for multiple gpg formats

[Junio C Hamano]

Henning Schild <henning.schild@siemens.com> writes:

> Create a struct that holds the format details for the supported formats.
> At the moment that is still just "openpgp". This commit prepares for the
> introduction of more formats, that might use other programs and match
> other signatures.
>
> Signed-off-by: Henning Schild <henning.schild@siemens.com>
> ---
>  gpg-interface.c | 74 ++++++++++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 58 insertions(+), 16 deletions(-)
>
> diff --git a/gpg-interface.c b/gpg-interface.c
> index ed0e55917..0a8d1bff3 100644
> --- a/gpg-interface.c
> +++ b/gpg-interface.c
> @@ -7,12 +7,46 @@
>  #include "tempfile.h"
>  
>  static char *configured_signing_key;
> -static const char *gpg_format = "openpgp";
> -static const char *gpg_program = "gpg";
> +struct gpg_format_data {
> +	const char *format;
> +	const char *program;
> +	const char *extra_args_verify[1];
> +	const char *sigs[2];
> +};

Do you use identifier "gpg_format" elsewhere as a structure name?  A
structure is there always to hold "data", so often "_data" suffix is
meaningless in a structure type name like this.

>  #define PGP_SIGNATURE "-----BEGIN PGP SIGNATURE-----"
>  #define PGP_MESSAGE "-----BEGIN PGP MESSAGE-----"
>  
> +enum gpgformats { PGP_FMT };
> +struct gpg_format_data gpg_formats[] = {
> +	{ .format = "openpgp", .program = "gpg",
> +	  .extra_args_verify = { "--keyid-format=long" },
> +	  .sigs = { PGP_SIGNATURE, PGP_MESSAGE }
> +	},
> +};
> +static const char *gpg_format = "openpgp";
> +
> +static struct gpg_format_data *get_format_data(const char *str)

get_format_data_by_name() or something like that, for consistency
with the next function, perhaps?

> +{
> +	int i;
> +	for (i = 0; i < ARRAY_SIZE(gpg_formats); i++)
> +		if (!strcasecmp(gpg_formats[i].format, str))
> +			return gpg_formats + i;
> +	return NULL;
> +}
> +
> +static struct gpg_format_data *get_format_data_by_sig(const char *sig)
> +{
> +	int i, j;
> +	for (i = 0; i < ARRAY_SIZE(gpg_formats); i++)
> +		for (j = 0; j < ARRAY_SIZE(gpg_formats[i].sigs); j++)
> +			if (gpg_formats[i].sigs[j] && 
> +			    !strncmp(gpg_formats[i].sigs[j], sig,
> +				     strlen(gpg_formats[i].sigs[j])))
> +				return gpg_formats + i;
> +	return NULL;
> +}
> +
>  void signature_check_clear(struct signature_check *sigc)
>  {
>  	FREE_AND_NULL(sigc->payload);
> @@ -104,8 +138,7 @@ void print_signature_buffer(const struct signature_check *sigc, unsigned flags)
>  
>  static int is_gpg_start(const char *line)
>  {
> -	return starts_with(line, PGP_SIGNATURE) ||
> -		starts_with(line, PGP_MESSAGE);
> +	return (get_format_data_by_sig(line) != NULL);

If this is still called "is_gpg_start()", then shouldn't the
implementation be more like this?

	struct gpg_format *found = get_format_data_by_sig(line);

	return found && found == &gpg_formats[PGP_FMT];

It probably does not make much difference in the end, as you won't
have functions is_(gpg|gpgsm|somethingelse|yetanother)_start() but
either have a single function "does some x509 looking block start
here?" or "I am expecting gpg and nothing else, does a block for
that start here?" and at that point this will no longer be called
is_gpg_start(), I would expect.

>  size_t parse_signature(const char *buf, size_t size)
> @@ -140,18 +173,14 @@ int git_gpg_config(const char *var, const char *value, void *cb)
>  	}
>  
>  	if (!strcmp(var, "gpg.format")) {
> -		if (strcasecmp(value, "openpgp"))
> +		if (!get_format_data(value))
>  			return error("malformed value for %s: %s", var, value);
>  		return git_config_string(&gpg_format, var, value);

This is a bug introduced in an earlier step in the series, but you
will segfault when given a configuration that looks like this:

	[gpg]
		format

Imitate the way how !value is diagnosed upfront for "gpg.program"
below before this patch.

>  	}
>  
> -	if (!strcmp(var, "gpg.program")) {
> -		if (!value)
> -			return config_error_nonbool(var);
> -		gpg_program = xstrdup(value);
> -		return 0;
> -	}
> -
> +	if (!strcmp(var, "gpg.program"))
> +		return git_config_string(&gpg_formats[PGP_FMT].program, var,
> +					 value);

git_config_string() has its own "do not feed boolean 'true' to me"
check, so this change does not introduce a regression.

> @@ -165,12 +194,16 @@ const char *get_signing_key(void)
>  int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *signing_key)
>  {
>  	struct child_process gpg = CHILD_PROCESS_INIT;
> +	struct gpg_format_data *fmt;
>  	int ret;
>  	size_t i, j, bottom;
>  	struct strbuf gpg_status = STRBUF_INIT;
>  
> +	fmt = get_format_data(gpg_format);
> +	if (!fmt)
> +		BUG("bad gpg_format '%s'", gpg_format);
>  	argv_array_pushl(&gpg.args,
> -			 gpg_program,
> +			 fmt->program,
>  			 "--status-fd=2",
>  			 "-bsau", signing_key,
>  			 NULL);
> @@ -208,8 +241,9 @@ int verify_signed_buffer(const char *payload, size_t payload_size,
>  			 struct strbuf *gpg_output, struct strbuf *gpg_status)
>  {
>  	struct child_process gpg = CHILD_PROCESS_INIT;
> +	struct gpg_format_data *fmt;
>  	struct tempfile *temp;
> -	int ret;
> +	int ret, i;
>  	struct strbuf buf = STRBUF_INIT;
>  
>  	temp = mks_tempfile_t(".git_vtag_tmpXXXXXX");
> @@ -223,10 +257,18 @@ int verify_signed_buffer(const char *payload, size_t payload_size,
>  		return -1;
>  	}
>  
> +	fmt = get_format_data_by_sig(signature);
> +	assert(fmt);
> +
> +	argv_array_pushl(&gpg.args,
> +			 fmt->program, NULL);
> +	for (i = 0; i < ARRAY_SIZE(fmt->extra_args_verify); i++)
> +		if (fmt->extra_args_verify[i])
> +			argv_array_pushl(&gpg.args,
> +					 fmt->extra_args_verify[i], NULL);

This loop allows us to extend .extra_args_verify when we need to
support another variant that needs to take two arguments by making
.extra_args field larger for all variants and stuffing a NULL to
unused slots, and it even allows a nonsense like this

	.extra_args_verify = { NULL, "--keyid-format=long" },

I cannot quite shake the feeling that .extra_args_verify and .sigs
fields should not be of fixed sized array of strings, but rather be
of type "const char **" that is NULL terminated.  Such a clean-up
will make it harder to write initializers, so it may not be worth
it, I guess.

>  	argv_array_pushl(&gpg.args,
> -			 gpg_program,
>  			 "--status-fd=1",
> -			 "--keyid-format=long",
>  			 "--verify", temp->filename.buf, "-",
>  			 NULL);

Re: [PATCH v2 7/9] gpg-interface: introduce new config to select per gpg format program

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Should we allow:
>
>   [gpg "OpenPGP"]
>   program = whatever
>
> given that we allow:
>
>   [gpg]
>   format = OpenPGP

If the latter is allowed then we should allow the former.  

But because allowing the former is cumbersome, we may be better off
not parsing the value case-insensitively like an earlier step in
this series did.

Re: [PATCH v2 8/9] gpg-interface: introduce new signature format "x509" using gpgsm

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> @@ -16,13 +16,18 @@ struct gpg_format_data {
>>  
>>  #define PGP_SIGNATURE "-----BEGIN PGP SIGNATURE-----"
>>  #define PGP_MESSAGE "-----BEGIN PGP MESSAGE-----"
>> +#define X509_SIGNATURE "-----BEGIN SIGNED MESSAGE-----"
>>  
>> -enum gpgformats { PGP_FMT };
>> +enum gpgformats { PGP_FMT, X509_FMT };
>>  struct gpg_format_data gpg_formats[] = {
>>  	{ .format = "openpgp", .program = "gpg",
>>  	  .extra_args_verify = { "--keyid-format=long" },
>>  	  .sigs = { PGP_SIGNATURE, PGP_MESSAGE }
>>  	},
>> +	{ .format = "x509", .program = "gpgsm",
>> +	  .extra_args_verify = { NULL },
>> +	  .sigs = { X509_SIGNATURE, NULL }
>> +	},
>
> Extremely minor nit, but if there are no other uses of PGP_SIGNATURE etc
> outside of this array (as I hope there wouldn't be after this series),
> would it make more sense to just include the literals inline in the
> array definition? That's one less layer of indirection when somebody is
> reading the code.

It is good design-sense to shoot for fewer levels of indirection,
but I suspect that "'const char **' instead of maximally-sized fixed
array of strings" would require a named array and constants like
this:

	static const char *gpg_verify_args[] = {
		"--verify",
		"--status-fd=1",
		"--keyid-format=long",
		NULL
	};
	static const char *gpg_sigs[] = {
		"-----BEGIN PGP SIGNATURE-----",
		"-----BEGIN PGP MESSAGE-----",
		NULL
	};

	struct gpg_format {
		const char *name;
		const char *program;
		const char * const *verify_args;
		const char * const *sigs;
	} gpg_format[] = {
		{
			.name = "openpgp",
			.program = "gpg',
			.verify_args = gpg_verify_args,
			.sigs = gpg_sigs,
		},
		{
			...
		},
	};

so we may end up having the same number of levels of indirection
anyway in the long-term final form.

As readers may be able to read from the above, I also have a
suspicion that it is a mistake to pretend that "--verify" etc.,
which merely happen to be common across the variants the series
covers, will stay forever to be common across _all_ variants and
that is why the field no longer is called "extra" args but is meant
to contain the full args.

Re: [PATCH v2 8/9] gpg-interface: introduce new signature format "x509" using gpgsm

[Jeff King]

On Tue, Jul 10, 2018 at 10:40:22AM -0700, Junio C Hamano wrote:

> > Extremely minor nit, but if there are no other uses of PGP_SIGNATURE etc
> > outside of this array (as I hope there wouldn't be after this series),
> > would it make more sense to just include the literals inline in the
> > array definition? That's one less layer of indirection when somebody is
> > reading the code.
> 
> It is good design-sense to shoot for fewer levels of indirection,
> but I suspect that "'const char **' instead of maximally-sized fixed
> array of strings" would require a named array and constants like
> this:

Yes, I agree with that direction (because it drops the magic numbers and
lets us use existing argv_array helpers).

> [...]
> so we may end up having the same number of levels of indirection
> anyway in the long-term final form.

True, but at least this level of indirection is buying us something. :)

> As readers may be able to read from the above, I also have a
> suspicion that it is a mistake to pretend that "--verify" etc.,
> which merely happen to be common across the variants the series
> covers, will stay forever to be common across _all_ variants and
> that is why the field no longer is called "extra" args but is meant
> to contain the full args.

I'd be fine going in that direction, too. But I don't actually foresee
adding new variants in the future. The point of this series versus the
signingtool one is that it's limited to gpg and gpg-alikes. And I doubt
we're likely to see more than the two that exist.

So even if we do end up adding support for more tools in the long run, I
think it will outgrow this config scheme.

-Peff

Re: [PATCH v2 9/9] gpg-interface t: extend the existing GPG tests with GPGSM

[Junio C Hamano]

Henning Schild <henning.schild@siemens.com> writes:

> Add test cases to cover the new X509/gpgsm support. Most of them
> resemble existing ones. They just switch the format to x509 and set the
> signingkey when creating signatures. Validation of signatures does not
> need any configuration of git, it does need gpgsm to be configured to
> trust the key(-chain).
> We generate a self-signed key for committer@example.com and configure
> gpgsm to trust it.
>
> Signed-off-by: Henning Schild <henning.schild@siemens.com>
> ---
>  t/lib-gpg.sh               |  9 ++++++-
>  t/lib-gpg/gpgsm-gen-key.in |  6 +++++
>  t/t4202-log.sh             | 66 ++++++++++++++++++++++++++++++++++++++++++++++
>  t/t5534-push-signed.sh     | 52 ++++++++++++++++++++++++++++++++++++
>  t/t7003-filter-branch.sh   | 15 +++++++++++
>  t/t7030-verify-tag.sh      | 47 +++++++++++++++++++++++++++++++--
>  t/t7600-merge.sh           | 31 ++++++++++++++++++++++
>  7 files changed, 223 insertions(+), 3 deletions(-)
>  create mode 100644 t/lib-gpg/gpgsm-gen-key.in

I saw my post-integration tests taking forever to finish for 'pu'
and "ps x" output showed that many tests (I ran tests in parallel)
were stuck and all of them were running gpgsm. 

For now, I've ejected the topic out of 'pu', as I want to finish
today's integration run with all the other topics first before
coming back to this topic.

Re: [PATCH v2 4/9] t/t7510: check the validation of the new config gpg.format

[Henning Schild]

Am Tue, 10 Jul 2018 11:55:36 -0400
schrieb Jeff King <peff@peff.net>:

> On Tue, Jul 10, 2018 at 10:52:26AM +0200, Henning Schild wrote:
> 
> > diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
> > index 6e2015ed9..7e1e9caf4 100755
> > --- a/t/t7510-signed-commit.sh
> > +++ b/t/t7510-signed-commit.sh
> > @@ -227,4 +227,14 @@ test_expect_success GPG 'log.showsignature
> > behaves like --show-signature' ' grep "gpg: Good signature" actual
> >  '
> >  
> > +test_expect_success GPG 'check config gpg.format values' '
> > +	rm .git/config &&
> > +	test_config gpg.format openpgp &&
> > +	git commit -S --amend -m "success" &&  
> 
> You shouldn't need this "rm" here. test_config will add your config,
> and then delete it after the test finishes.

Right, got rid of it.

> I know you probably saw that in t1300 or nearby tests, but IMHO they
> are wrong to do so. It's a historical wart that should be cleaned up.
> 
> > +	test_config gpg.format OpEnPgP &&
> > +	git commit -S --amend -m "success" &&  
> 
> A bit of a funny side effect is that we'll unset gpg.format three
> times at the end of the test, since each test_config doesn't know
> that the earlier invocations touched the same variable.
> 
> It's probably not worth addressing, but we could do it with an
> explicit:

I think it is not worth addressing. Manual unsetting would mess with
test_config and both alternatives would just bloat the code. The
additional two unsets do not hurt, imho.

>   test_when_finished "test_unconfig gpg.format" &&
>   git config gpg.format openpgp &&
>   ...
>   git config gpg.format OpEnPgP &&
> 
> Or alternatively, this could be three independent tests, which would
> give the opportunity to describe each.
> 
> > +	test_config gpg.format malformed &&
> > +	test_must_fail git commit -S --amend -m "fail" 2>result
> > +'  
> 
> If you're not going to look at the saved "result", we are better to
> just leave stderr un-redirected. It will go to /dev/null by default,
> or to the user-visible output of the test is run in verbose mode.

Right, dropped the redirect.

Henning
 
> -Peff

Re: [PATCH v2 2/9] gpg-interface: make parse_gpg_output static and remove from interface header

[Henning Schild]

Am Tue, 10 Jul 2018 09:47:26 -0700
schrieb Junio C Hamano <gitster@pobox.com>:

> Henning Schild <henning.schild@siemens.com> writes:
> 
> > This commit turns parse_gpg_output into an internal function, the
> > only outside user was migrated in an earlier commit.  
> 
> It is not too big a deal but as we prefer to see our history speak
> in consistent voice, we would usually phrase the above as if we are
> giving an order to "make it so" to the codebase, e.g.
> 
> 	Turn parse_gpg_output() into a static function, as the only
> 	outside user was removed in the previous step.
> 
> or something like that.
> 
> These two steps, as you said earlier, are nice clean-up patches
> whose goodness can be measured independently, regardless of the
> gpgsm support which is the primary focus of this series.

I updated the comment of the second one and send them as a seperate
series. I do not expect them to change since there was -- so far -- no
feedback suggesting that. So i am tempted to promise that ;).

Henning

> Thanks.
> 
> >
> > Signed-off-by: Henning Schild <henning.schild@siemens.com>
> > ---
> >  gpg-interface.c | 2 +-
> >  gpg-interface.h | 2 --
> >  2 files changed, 1 insertion(+), 3 deletions(-)
> >
> > diff --git a/gpg-interface.c b/gpg-interface.c
> > index 0647bd634..09ddfbc26 100644
> > --- a/gpg-interface.c
> > +++ b/gpg-interface.c
> > @@ -35,7 +35,7 @@ static struct {
> >  	{ 'R', "\n[GNUPG:] REVKEYSIG "},
> >  };
> >  
> > -void parse_gpg_output(struct signature_check *sigc)
> > +static void parse_gpg_output(struct signature_check *sigc)
> >  {
> >  	const char *buf = sigc->gpg_status;
> >  	int i;
> > diff --git a/gpg-interface.h b/gpg-interface.h
> > index a5e6517ae..5ecff4aa0 100644
> > --- a/gpg-interface.h
> > +++ b/gpg-interface.h
> > @@ -33,8 +33,6 @@ void signature_check_clear(struct signature_check
> > *sigc); */
> >  size_t parse_signature(const char *buf, size_t size);
> >  
> > -void parse_gpg_output(struct signature_check *);
> > -
> >  /*
> >   * Create a detached signature for the contents of "buffer" and
> > append
> >   * it after "signature"; "buffer" and "signature" can be the same  

Re: [PATCH v2 4/9] t/t7510: check the validation of the new config gpg.format

[Henning Schild]

Am Tue, 10 Jul 2018 09:54:59 -0700
schrieb Junio C Hamano <gitster@pobox.com>:

> Henning Schild <henning.schild@siemens.com> writes:
> 
> > Test setting gpg.format to both invalid and valid values.
> >
> > Signed-off-by: Henning Schild <henning.schild@siemens.com>
> > ---
> >  t/t7510-signed-commit.sh | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> >
> > diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
> > index 6e2015ed9..7e1e9caf4 100755
> > --- a/t/t7510-signed-commit.sh
> > +++ b/t/t7510-signed-commit.sh
> > @@ -227,4 +227,14 @@ test_expect_success GPG 'log.showsignature
> > behaves like --show-signature' ' grep "gpg: Good signature" actual
> >  '
> >  
> > +test_expect_success GPG 'check config gpg.format values' '
> > +	rm .git/config &&  
> 
> Please don't.  .git/config has stuff that are more important than
> just collection of random configuration these days, and we can even
> expect that future versions of Git may not store its config in a
> flat file .git/config but in a different mechanism "git config"
> command knows how to access.  A low-level test for "git config"
> command's operation may have to be implemented by inspecting the
> resulting .git/config, but as this test is not about "git config"'s
> inner workins but is about one feature "git commit" command has,
> we prefer not to depend too much on the internal implementation
> detail such as "local config is stored in .git/config file".
> 
> Let's hear why you want to remove this file; what things that have
> previously been placed in the file do you want not to see, before
> performing the following actions?  Once we know that, we can suggest
> a way to do so better than removing the entire file.

That was a leftover of parsing the result and expecting the malformed
value in a certain line. Now i do not look at result anymore. If the
first two do succeed and the third fails it is probably fair enough to
assume that the format "malformed" triggerd the config validation path.

That rm is gone from what will be v3.

Henning

> > +	test_config gpg.format openpgp &&
> > +	git commit -S --amend -m "success" &&
> > +	test_config gpg.format OpEnPgP &&
> > +	git commit -S --amend -m "success" &&
> > +	test_config gpg.format malformed &&
> > +	test_must_fail git commit -S --amend -m "fail" 2>result
> > +'
> > +
> >  test_done  

Re: [PATCH v2 6/9] gpg-interface: do not hardcode the key string len anymore

[Henning Schild]

Am Tue, 10 Jul 2018 11:49:31 -0400
schrieb Jeff King <peff@peff.net>:

> On Tue, Jul 10, 2018 at 10:52:28AM +0200, Henning Schild wrote:
> 
> > gnupg does print the keyid followed by a space and the signer comes
> > next. The same pattern is also used in gpgsm, but there the key
> > length would be 40 instead of 16. Instead of hardcoding the
> > expected length, find the first space and calculate it.  
> 
> Sounds good, but I think there's an off-by-one in the patch.
> 
> > diff --git a/gpg-interface.c b/gpg-interface.c
> > index 0a8d1bff3..ac2df498d 100644
> > --- a/gpg-interface.c
> > +++ b/gpg-interface.c
> > @@ -88,10 +88,11 @@ static void parse_gpg_output(struct
> > signature_check *sigc) sigc->result = sigcheck_gpg_status[i].result;
> >  		/* The trust messages are not followed by
> > key/signer information */ if (sigc->result != 'U') {
> > -			sigc->key = xmemdupz(found, 16);
> > +			next = strchrnul(found, ' ');
> > +			sigc->key = xmemdupz(found, next -
> > found);  
> 
> Here "next" may point to the trailing NUL of the string...
> 
> >  			/* The ERRSIG message is not followed by
> > signer information */ if (sigc-> result != 'E') {
> > -				found += 17;
> > +				found = next + 1;
> >  				next = strchrnul(found, '\n');  
> 
> ...in which case "found" points past the end of the string, and we
> search random memory. That's presumably impossible with well-formed
> gpg output (you don't get 'E' without an extra message), but we
> should be robust against bogus input.
> 
> In the general case you need:
> 
>   found = *next ? next + 1 : next;
> 
> or similar. In this case, you can actually do:
> 
>   found = next;
> 
> because we know that it's OK to search over the literal space again.
> But that's pretty subtle, so we're probably better off just doing the
> conditional above.
> 
> (And yes, looking at the existing code, I think it's even worse, as
> there does not seem to be a guarantee that we even have 16 characters
> in the string).

The existing code works only on expected output and the same is true
for the version after this patch. Making the parser robust against
random input would imho be a sort of cleanup patch on top of my
series. .. or before, in which case i would become responsible for
making sure that still works after my modification.
This argument is twofold. I do not really want to fix that as well and
it might be a good idea to separate concerns anyways.

Henning

> -Peff

Re: [PATCH v2 8/9] gpg-interface: introduce new signature format "x509" using gpgsm

[Henning Schild]

Am Tue, 10 Jul 2018 13:01:10 -0400
schrieb Jeff King <peff@peff.net>:

> On Tue, Jul 10, 2018 at 10:52:30AM +0200, Henning Schild wrote:
> 
> > diff --git a/Documentation/config.txt b/Documentation/config.txt
> > index c0bd80954..b6f9b47d5 100644
> > --- a/Documentation/config.txt
> > +++ b/Documentation/config.txt
> > @@ -1830,7 +1830,7 @@ gpg.program::
> >  
> >  gpg.format::
> >  	Specifies which key format to use when signing with
> > `--gpg-sign`.
> > -	Default is "openpgp", that is also the only supported
> > value.
> > +	Default is "opengpg" and another possible value is
> > "x509".  
> 
> opengpg?

Right, thanks!

> Since we're having so much fun with naming discussions, let's talk
> about "x509". :)
> 
> That's the cert format. I think of these signatures as S/MIME, but
> really that's the mail-oriented parts of the standard. I think
> technically this is "CMS".
> 
> That said, we should pick what most people will find natural when
> referring to it. So maybe x509 isn't the worst choice, as I doubt most
> people know the term CMS. Probably the term they know _most_ is
> "gpgsm", but I think the point is that one does not have to be using
> gpgsm in the first place.

Ok, but now that you mention it, i will include the string "gpgsm" into
Documentation/config.txt somewhere. Maybe other documentation bits
could use hints that gpg is not the only kid in town anymore.

> So I dunno. I think I talked myself back into x509. ;)

Ok, will stick to it.

Henning

> > diff --git a/gpg-interface.c b/gpg-interface.c
> > index 65098430f..bf8d567a4 100644
> > --- a/gpg-interface.c
> > +++ b/gpg-interface.c
> > @@ -16,13 +16,18 @@ struct gpg_format_data {
> >  
> >  #define PGP_SIGNATURE "-----BEGIN PGP SIGNATURE-----"
> >  #define PGP_MESSAGE "-----BEGIN PGP MESSAGE-----"
> > +#define X509_SIGNATURE "-----BEGIN SIGNED MESSAGE-----"
> >  
> > -enum gpgformats { PGP_FMT };
> > +enum gpgformats { PGP_FMT, X509_FMT };
> >  struct gpg_format_data gpg_formats[] = {
> >  	{ .format = "openpgp", .program = "gpg",
> >  	  .extra_args_verify = { "--keyid-format=long" },
> >  	  .sigs = { PGP_SIGNATURE, PGP_MESSAGE }
> >  	},
> > +	{ .format = "x509", .program = "gpgsm",
> > +	  .extra_args_verify = { NULL },
> > +	  .sigs = { X509_SIGNATURE, NULL }
> > +	},  
> 
> Extremely minor nit, but if there are no other uses of PGP_SIGNATURE
> etc outside of this array (as I hope there wouldn't be after this
> series), would it make more sense to just include the literals inline
> in the array definition? That's one less layer of indirection when
> somebody is reading the code.
> 
> -Peff

Re: [PATCH v2 9/9] gpg-interface t: extend the existing GPG tests with GPGSM

[Henning Schild]

Am Tue, 10 Jul 2018 14:12:57 -0700
schrieb Junio C Hamano <gitster@pobox.com>:

> Henning Schild <henning.schild@siemens.com> writes:
> 
> > Add test cases to cover the new X509/gpgsm support. Most of them
> > resemble existing ones. They just switch the format to x509 and set
> > the signingkey when creating signatures. Validation of signatures
> > does not need any configuration of git, it does need gpgsm to be
> > configured to trust the key(-chain).
> > We generate a self-signed key for committer@example.com and
> > configure gpgsm to trust it.
> >
> > Signed-off-by: Henning Schild <henning.schild@siemens.com>
> > ---
> >  t/lib-gpg.sh               |  9 ++++++-
> >  t/lib-gpg/gpgsm-gen-key.in |  6 +++++
> >  t/t4202-log.sh             | 66
> > ++++++++++++++++++++++++++++++++++++++++++++++
> > t/t5534-push-signed.sh     | 52
> > ++++++++++++++++++++++++++++++++++++ t/t7003-filter-branch.sh   |
> > 15 +++++++++++ t/t7030-verify-tag.sh      | 47
> > +++++++++++++++++++++++++++++++-- t/t7600-merge.sh           | 31
> > ++++++++++++++++++++++ 7 files changed, 223 insertions(+), 3
> > deletions(-) create mode 100644 t/lib-gpg/gpgsm-gen-key.in  
> 
> I saw my post-integration tests taking forever to finish for 'pu'
> and "ps x" output showed that many tests (I ran tests in parallel)
> were stuck and all of them were running gpgsm. 
> 
> For now, I've ejected the topic out of 'pu', as I want to finish
> today's integration run with all the other topics first before
> coming back to this topic.

That must have been the key generation, see my reply to one of
Jeffs mails.

Henning

Re: [PATCH v2 9/9] gpg-interface t: extend the existing GPG tests with GPGSM

[Henning Schild]

Am Tue, 10 Jul 2018 13:09:01 -0400
schrieb Jeff King <peff@peff.net>:

> On Tue, Jul 10, 2018 at 10:52:31AM +0200, Henning Schild wrote:
> 
> > diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh
> > index a5d3b2cba..9dcb4e990 100755
> > --- a/t/lib-gpg.sh
> > +++ b/t/lib-gpg.sh
> > @@ -38,7 +38,14 @@ then
> >  			"$TEST_DIRECTORY"/lib-gpg/ownertrust &&
> >  		gpg --homedir "${GNUPGHOME}" </dev/null >/dev/null
> > 2>&1 \ --sign -u committer@example.com &&
> > -		test_set_prereq GPG
> > +		test_set_prereq GPG &&
> > +		echo | gpgsm --homedir "${GNUPGHOME}" -o
> > "$TEST_DIRECTORY"/lib-gpg/gpgsm.crt.user --passphrase-fd 0
> > --pinentry-mode loopback --generate-key --batch
> > "$TEST_DIRECTORY"/lib-gpg/gpgsm-gen-key.in &&  
> 
> Is this --generate-key going to require high-quality entropy? That
> could slow the test suite down (and maybe even stall it quite a bit
> on servers doing automated CI).

Yes, i also saw that getting "stuck" on one machine. But i blamed an
experimental kernel.

> Can we save a dummy generated key and just import it? That's what we
> do for the regular gpg case.

I will look into storing a binary and leaving notes how it was
generated, just like regular gpg does. The reason i did not do that in
the first place is that x509 certs have a validity and we introduce
time into the picture. But i will see if i can generate epoch->infinity
to get the host clock or just the future out of the picture.

> > +		gpgsm --homedir "${GNUPGHOME}" --import
> > "$TEST_DIRECTORY"/lib-gpg/gpgsm.crt.user &&
> > +		gpgsm --homedir "${GNUPGHOME}" -K | grep
> > fingerprint: | cut -d" " -f4 | tr -d '\n' >
> > ${GNUPGHOME}/trustlist.txt &&
> > +		echo " S relax" >> ${GNUPGHOME}/trustlist.txt &&
> > +		(gpgconf --kill gpg-agent >/dev/null 2>&1 || : ) &&
> > +		echo hello | gpgsm --homedir "${GNUPGHOME}" -u
> > committer@example.com -o /dev/null --sign - 2>&1 &&
> > +		test_set_prereq GPGSM  
> 
> This &&-chain means we can't have GPGSM without GPG. In theory the two
> could be tested independently. I don't know if it's worth the trouble
> to make that work, though. I wouldn't be surprised if there are some
> subtle dependencies within the test scripts, and I'm not sure how
> common it is for somebody to have gpgsm and not gpg. So it may make
> sense to just punt on it until such a person appears.

As you found later, i already commented on that and do not think it is
worth the effort. Users will be able to just have gpgsm, testers will
need gpg to test gpgsm.
 
> >  test_expect_success GPG 'log --graph --show-signature' '
> >  	git log --graph --show-signature -n1 signed >actual &&
> >  	grep "^| gpg: Signature made" actual &&
> >  	grep "^| gpg: Good signature" actual
> >  '
> >  
> > +test_expect_success GPGSM 'log --graph --show-signature x509' '
> > +	git log --graph --show-signature -n1 signed-x509 >actual &&
> > +	grep "^| gpgsm: Signature made" actual &&
> > +	grep "^| gpgsm: Good signature" actual
> > +'  
> 
> We're going to have a lot of duplicated tests here. That's a
> maintenance burden when one of them needs fixes later. And when new
> tests are added, we won't automatically get them tested under each
> format.
> 
> Can we move the battery of tests into a function that takes a few
> parameters (prereq name, branch to look at, etc) and then call it for
> both the gpg/gpgsm cases?

I guess this is part of the earlier "allow GPGSM without GPG" and i
can ignore it if we agree that this is not needed?

Henning
 
> -Peff

Re: [PATCH v2 6/9] gpg-interface: do not hardcode the key string len anymore

[Jeff King]

On Wed, Jul 11, 2018 at 10:54:59AM +0200, Henning Schild wrote:

> > In the general case you need:
> > 
> >   found = *next ? next + 1 : next;
> > 
> > or similar. In this case, you can actually do:
> > 
> >   found = next;
> > 
> > because we know that it's OK to search over the literal space again.
> > But that's pretty subtle, so we're probably better off just doing the
> > conditional above.
> > 
> > (And yes, looking at the existing code, I think it's even worse, as
> > there does not seem to be a guarantee that we even have 16 characters
> > in the string).
> 
> The existing code works only on expected output and the same is true
> for the version after this patch. Making the parser robust against
> random input would imho be a sort of cleanup patch on top of my
> series. .. or before, in which case i would become responsible for
> making sure that still works after my modification.
> This argument is twofold. I do not really want to fix that as well and
> it might be a good idea to separate concerns anyways.

I think it's worth addressing in the near term, if only because this
kind of off-by-one is quite subtle, and I don't want to forget to deal
with it. Whether that happens as part of this patch, or as a cleanup
before or after, I'm not picky. :)

-Peff

Re: [PATCH v2 9/9] gpg-interface t: extend the existing GPG tests with GPGSM

[Jeff King]

On Wed, Jul 11, 2018 at 12:38:24PM +0200, Henning Schild wrote:

> > Can we save a dummy generated key and just import it? That's what we
> > do for the regular gpg case.
> 
> I will look into storing a binary and leaving notes how it was
> generated, just like regular gpg does. The reason i did not do that in
> the first place is that x509 certs have a validity and we introduce
> time into the picture. But i will see if i can generate epoch->infinity
> to get the host clock or just the future out of the picture.

That would be preferable. But even if we can just get something like a
10-year expiration, that may be enough. Somebody dealing with failing
tests and regenerating keys in ten years is probably not the end of the
world.

It could hurt people with drastically incorrect system clocks, but I
suspect there are other tests with similar problems (especially if your
clock is in the past).

> > We're going to have a lot of duplicated tests here. That's a
> > maintenance burden when one of them needs fixes later. And when new
> > tests are added, we won't automatically get them tested under each
> > format.
> > 
> > Can we move the battery of tests into a function that takes a few
> > parameters (prereq name, branch to look at, etc) and then call it for
> > both the gpg/gpgsm cases?
> 
> I guess this is part of the earlier "allow GPGSM without GPG" and i
> can ignore it if we agree that this is not needed?

I think it's orthogonal. Even if GPGSM requires GPG, you'd still want to
make sure that whatever exercise we give to the GPG code is also
exercised using GPGSM.

I will note, though, that _some_ tests are not really exercising
gpg-specific bits, but more how we react to it (e.g., how we format %G
placeholders). And it's probably OK for those to just be run once.

So in an ideal world, the test script would probably look something
like:

  # this function holds tests which exercise the interactions
  # with the gpg binary itself
  type_specific_tests() {
	prereq=$1
	branch=$2

	test_expect_success $prereq "test whatever ($prereq)" '
		some test using $branch here
	'
  }

  test_expect_success 'setup' '
	set up both openpgp and x509 branches here
  '

  type_specific_tests GPG openpgp
  type_specific_tests GPGSM x509

  # and now tests that generically care about getting _some_ signature
  # result (e.g., the way we format signature info)

  # and then probably a few tests specific to how the config is handled,
  # like your new gpg.format coverage

But in practice, the type-specific bits are often muddled together with
the type-independent ones (e.g., we happen to test the parsing of a
failed signature from gpg by checking how %G? is formatted or similar).

So it may be simplest to just run most of the tests twice, once with gpg
and once with gpgsm. I kind of wonder if all of t7510 could just be
bumped into a function. Or even into a sourced file and run from two
different scripts. See the way that t8001 and t8002 use
annotate-tests.sh for an example.

-Peff

Re: [PATCH v2 9/9] gpg-interface t: extend the existing GPG tests with GPGSM

[Henning Schild]

Am Wed, 11 Jul 2018 08:51:10 -0400
schrieb Jeff King <peff@peff.net>:

> On Wed, Jul 11, 2018 at 12:38:24PM +0200, Henning Schild wrote:
> 
> > > Can we save a dummy generated key and just import it? That's what
> > > we do for the regular gpg case.  
> > 
> > I will look into storing a binary and leaving notes how it was
> > generated, just like regular gpg does. The reason i did not do that
> > in the first place is that x509 certs have a validity and we
> > introduce time into the picture. But i will see if i can generate
> > epoch->infinity to get the host clock or just the future out of the
> > picture.  
> 
> That would be preferable. But even if we can just get something like a
> 10-year expiration, that may be enough. Somebody dealing with failing
> tests and regenerating keys in ten years is probably not the end of
> the world.
> 
> It could hurt people with drastically incorrect system clocks, but I
> suspect there are other tests with similar problems (especially if
> your clock is in the past).

I now have a working version with a pregenerated certificate from
1970-3000. I would be surprised if git is still around at that time ;).

> > > We're going to have a lot of duplicated tests here. That's a
> > > maintenance burden when one of them needs fixes later. And when
> > > new tests are added, we won't automatically get them tested under
> > > each format.
> > > 
> > > Can we move the battery of tests into a function that takes a few
> > > parameters (prereq name, branch to look at, etc) and then call it
> > > for both the gpg/gpgsm cases?  
> > 
> > I guess this is part of the earlier "allow GPGSM without GPG" and i
> > can ignore it if we agree that this is not needed?  
> 
> I think it's orthogonal. Even if GPGSM requires GPG, you'd still want
> to make sure that whatever exercise we give to the GPG code is also
> exercised using GPGSM.
> 
> I will note, though, that _some_ tests are not really exercising
> gpg-specific bits, but more how we react to it (e.g., how we format %G
> placeholders). And it's probably OK for those to just be run once.
> 
> So in an ideal world, the test script would probably look something
> like:
> 
>   # this function holds tests which exercise the interactions
>   # with the gpg binary itself
>   type_specific_tests() {
> 	prereq=$1
> 	branch=$2
> 
> 	test_expect_success $prereq "test whatever ($prereq)" '
> 		some test using $branch here
> 	'
>   }
> 
>   test_expect_success 'setup' '
> 	set up both openpgp and x509 branches here
>   '
> 
>   type_specific_tests GPG openpgp
>   type_specific_tests GPGSM x509
> 
>   # and now tests that generically care about getting _some_ signature
>   # result (e.g., the way we format signature info)
> 
>   # and then probably a few tests specific to how the config is
> handled, # like your new gpg.format coverage
> 
> But in practice, the type-specific bits are often muddled together
> with the type-independent ones (e.g., we happen to test the parsing
> of a failed signature from gpg by checking how %G? is formatted or
> similar).
> 
> So it may be simplest to just run most of the tests twice, once with
> gpg and once with gpgsm. I kind of wonder if all of t7510 could just
> be bumped into a function. Or even into a sourced file and run from
> two different scripts. See the way that t8001 and t8002 use
> annotate-tests.sh for an example.

I do not agree and would like to leave the tests as they are. Instead
of introducing a whole lot of very similar copies, i added just a few.
The original ones are even very similar between each other.
We are again talking about two problems. 1. we need test cases for
gpgsm if we want to merge gpgsm 2. the testsuite is very repetitive

While addressing 1 make 2 obvious and worse, addressing 2 is a whole
different story and should probably be discussed outside of this
thread. And i would not like to inherit responsibility for 2. In
fact the whole discussion emphasizes that it was a good idea to make
GPGSM depend on GPG, because it allows to somewhat reuse existing tests.

Henning

> -Peff

Re: [PATCH v2 6/9] gpg-interface: do not hardcode the key string len anymore

[Henning Schild]

Am Wed, 11 Jul 2018 08:34:25 -0400
schrieb Jeff King <peff@peff.net>:

> On Wed, Jul 11, 2018 at 10:54:59AM +0200, Henning Schild wrote:
> 
> > > In the general case you need:
> > > 
> > >   found = *next ? next + 1 : next;
> > > 
> > > or similar. In this case, you can actually do:
> > > 
> > >   found = next;
> > > 
> > > because we know that it's OK to search over the literal space
> > > again. But that's pretty subtle, so we're probably better off
> > > just doing the conditional above.
> > > 
> > > (And yes, looking at the existing code, I think it's even worse,
> > > as there does not seem to be a guarantee that we even have 16
> > > characters in the string).  
> > 
> > The existing code works only on expected output and the same is true
> > for the version after this patch. Making the parser robust against
> > random input would imho be a sort of cleanup patch on top of my
> > series. .. or before, in which case i would become responsible for
> > making sure that still works after my modification.
> > This argument is twofold. I do not really want to fix that as well
> > and it might be a good idea to separate concerns anyways.  
> 
> I think it's worth addressing in the near term, if only because this
> kind of off-by-one is quite subtle, and I don't want to forget to deal
> with it. Whether that happens as part of this patch, or as a cleanup
> before or after, I'm not picky. :)

I get that and if anyone is willing to write that code, i will base my
patches on it. What i want to avoid is taking responsibility for
problems i did not introduce, just because i happen to work on that
code at the moment. Keeping track of that (not forgetting) is also not
for the random contributor like myself.

Henning

> -Peff

Re: [PATCH v2 6/9] gpg-interface: do not hardcode the key string len anymore

[Jeff King]

On Wed, Jul 11, 2018 at 03:46:19PM +0200, Henning Schild wrote:

> > I think it's worth addressing in the near term, if only because this
> > kind of off-by-one is quite subtle, and I don't want to forget to deal
> > with it. Whether that happens as part of this patch, or as a cleanup
> > before or after, I'm not picky. :)
> 
> I get that and if anyone is willing to write that code, i will base my
> patches on it. What i want to avoid is taking responsibility for
> problems i did not introduce, just because i happen to work on that
> code at the moment. Keeping track of that (not forgetting) is also not
> for the random contributor like myself.

It doesn't make sense to do a patch before your series, since it would
just be:

  if (strlen(found) > 16)
    ...

which would get obliterated by your patch. The patch after is shown
below. But frankly, it seems a lot easier to just handle this while you
are rewriting the code.

-- >8 --
Subject: [PATCH] gpg-interface: handle off-by-one parsing gpg output

When parsing gpg's VALIDSIG lines, we look for a space
followed by the signer information. Because we use
strchrnul(), though, if the space is missing we'll end up
pointing to the trailing NUL. When we try to move past that
space, we have to handle the NUL case separately to avoid
accidentally stepping out of the string entirely.

Signed-off-by: Jeff King <peff@peff.net>
---
 gpg-interface.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gpg-interface.c b/gpg-interface.c
index bf8d567a4c..139b0f561e 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -97,7 +97,7 @@ static void parse_gpg_output(struct signature_check *sigc)
 			sigc->key = xmemdupz(found, next - found);
 			/* The ERRSIG message is not followed by signer information */
 			if (sigc-> result != 'E') {
-				found = next + 1;
+				found = *next ? next + 1 : next;
 				next = strchrnul(found, '\n');
 				sigc->signer = xmemdupz(found, next - found);
 			}
-- 
2.18.0.400.g702e398724

Re: [PATCH v2 9/9] gpg-interface t: extend the existing GPG tests with GPGSM

[Jeff King]

On Tue, Jul 10, 2018 at 10:52:31AM +0200, Henning Schild wrote:

> diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh
> index a5d3b2cba..9dcb4e990 100755
> --- a/t/lib-gpg.sh
> +++ b/t/lib-gpg.sh
> @@ -38,7 +38,14 @@ then
>  			"$TEST_DIRECTORY"/lib-gpg/ownertrust &&
>  		gpg --homedir "${GNUPGHOME}" </dev/null >/dev/null 2>&1 \
>  			--sign -u committer@example.com &&
> -		test_set_prereq GPG
> +		test_set_prereq GPG &&
> +		echo | gpgsm --homedir "${GNUPGHOME}" -o "$TEST_DIRECTORY"/lib-gpg/gpgsm.crt.user --passphrase-fd 0 --pinentry-mode loopback --generate-key --batch "$TEST_DIRECTORY"/lib-gpg/gpgsm-gen-key.in &&
> +		gpgsm --homedir "${GNUPGHOME}" --import "$TEST_DIRECTORY"/lib-gpg/gpgsm.crt.user &&
> +		gpgsm --homedir "${GNUPGHOME}" -K | grep fingerprint: | cut -d" " -f4 | tr -d '\n' > ${GNUPGHOME}/trustlist.txt &&
> +		echo " S relax" >> ${GNUPGHOME}/trustlist.txt &&
> +		(gpgconf --kill gpg-agent >/dev/null 2>&1 || : ) &&
> +		echo hello | gpgsm --homedir "${GNUPGHOME}" -u committer@example.com -o /dev/null --sign - 2>&1 &&
> +		test_set_prereq GPGSM

By the way, I think these gpgsm invocations need some redirection of
their stderr. This isn't inside a test_expect block, so they spew to the
terminal even in non-verbose mode.

Ideally they'd redirect to descriptor 4, which then respects "-v"
properly (though the existing gpg invocations just go to 2).

-Peff

Re: [PATCH v2 9/9] gpg-interface t: extend the existing GPG tests with GPGSM

[Jeff King]

On Wed, Jul 11, 2018 at 03:40:19PM +0200, Henning Schild wrote:

> > So it may be simplest to just run most of the tests twice, once with
> > gpg and once with gpgsm. I kind of wonder if all of t7510 could just
> > be bumped into a function. Or even into a sourced file and run from
> > two different scripts. See the way that t8001 and t8002 use
> > annotate-tests.sh for an example.
> 
> I do not agree and would like to leave the tests as they are. Instead
> of introducing a whole lot of very similar copies, i added just a few.

I'm not sure I understand why you added the ones you did, though. For
instance, "--no-show-signature overrides --show-signature x509" seems
like it has nothing to do with the gpg/gpgsm distinction.

So I'd have expected that to be _outside_ of the shared battery of
tests.

> The original ones are even very similar between each other.
> We are again talking about two problems. 1. we need test cases for
> gpgsm if we want to merge gpgsm 2. the testsuite is very repetitive
> 
> While addressing 1 make 2 obvious and worse, addressing 2 is a whole
> different story and should probably be discussed outside of this
> thread. And i would not like to inherit responsibility for 2. In
> fact the whole discussion emphasizes that it was a good idea to make
> GPGSM depend on GPG, because it allows to somewhat reuse existing tests.

IMHO there is a big difference between inheriting responsibility for
something, and not making it worse. But I'm not all that interested in
fighting about it.

-Peff

Re: [PATCH v2 9/9] gpg-interface t: extend the existing GPG tests with GPGSM

[Henning Schild]

Am Wed, 11 Jul 2018 10:35:54 -0400
schrieb Jeff King <peff@peff.net>:

> On Wed, Jul 11, 2018 at 03:40:19PM +0200, Henning Schild wrote:
> 
> > > So it may be simplest to just run most of the tests twice, once
> > > with gpg and once with gpgsm. I kind of wonder if all of t7510
> > > could just be bumped into a function. Or even into a sourced file
> > > and run from two different scripts. See the way that t8001 and
> > > t8002 use annotate-tests.sh for an example.  
> > 
> > I do not agree and would like to leave the tests as they are.
> > Instead of introducing a whole lot of very similar copies, i added
> > just a few.  
> 
> I'm not sure I understand why you added the ones you did, though. For
> instance, "--no-show-signature overrides --show-signature x509" seems
> like it has nothing to do with the gpg/gpgsm distinction.
> 
> So I'd have expected that to be _outside_ of the shared battery of
> tests.

True, it took my quite some time to figure out a way to configure gpgsm
non-interactively. Generate the key etc. without even a single popup of
the gpg-agent... After that i just added random tests to create
"coverage", without much focus. I would be happy to revisit that and
drop test cases, and add some that are missing.

Henning

> > The original ones are even very similar between each other.
> > We are again talking about two problems. 1. we need test cases for
> > gpgsm if we want to merge gpgsm 2. the testsuite is very repetitive
> > 
> > While addressing 1 make 2 obvious and worse, addressing 2 is a whole
> > different story and should probably be discussed outside of this
> > thread. And i would not like to inherit responsibility for 2. In
> > fact the whole discussion emphasizes that it was a good idea to make
> > GPGSM depend on GPG, because it allows to somewhat reuse existing
> > tests.  
> 
> IMHO there is a big difference between inheriting responsibility for
> something, and not making it worse. But I'm not all that interested in
> fighting about it.
>
> -Peff

Re: [PATCH v2 6/9] gpg-interface: do not hardcode the key string len anymore

[Henning Schild]

Am Wed, 11 Jul 2018 10:27:52 -0400
schrieb Jeff King <peff@peff.net>:

> On Wed, Jul 11, 2018 at 03:46:19PM +0200, Henning Schild wrote:
> 
> > > I think it's worth addressing in the near term, if only because
> > > this kind of off-by-one is quite subtle, and I don't want to
> > > forget to deal with it. Whether that happens as part of this
> > > patch, or as a cleanup before or after, I'm not picky. :)  
> > 
> > I get that and if anyone is willing to write that code, i will base
> > my patches on it. What i want to avoid is taking responsibility for
> > problems i did not introduce, just because i happen to work on that
> > code at the moment. Keeping track of that (not forgetting) is also
> > not for the random contributor like myself.  
> 
> It doesn't make sense to do a patch before your series, since it would
> just be:
> 
>   if (strlen(found) > 16)
>     ...

Instead of randomly crashing on unexpected input, we would now silently
ignore it.

> which would get obliterated by your patch. The patch after is shown
> below. But frankly, it seems a lot easier to just handle this while
> you are rewriting the code.
> 
> -- >8 --  
> Subject: [PATCH] gpg-interface: handle off-by-one parsing gpg output
> 
> When parsing gpg's VALIDSIG lines, we look for a space
> followed by the signer information. Because we use
> strchrnul(), though, if the space is missing we'll end up
> pointing to the trailing NUL. When we try to move past that
> space, we have to handle the NUL case separately to avoid
> accidentally stepping out of the string entirely.

True.

> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  gpg-interface.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/gpg-interface.c b/gpg-interface.c
> index bf8d567a4c..139b0f561e 100644
> --- a/gpg-interface.c
> +++ b/gpg-interface.c
> @@ -97,7 +97,7 @@ static void parse_gpg_output(struct signature_check
> *sigc) sigc->key = xmemdupz(found, next - found);
>  			/* The ERRSIG message is not followed by
> signer information */ if (sigc-> result != 'E') {
> -				found = next + 1;
> +				found = *next ? next + 1 : next;

This would keep us in bounds of the unexpected string. But ignore the
line instead of "complaining" or crashing.

But you are right, it is easy enough and ignoring the line is probably
the best way of dealing with it.

i will change the condition to
> if (*next && sigc-> result != 'E')

also skipping the following strchrnul and xmemdupz

Henning

>  				next = strchrnul(found, '\n');
>  				sigc->signer = xmemdupz(found, next
> - found); }

Re: [PATCH v2 9/9] gpg-interface t: extend the existing GPG tests with GPGSM

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> While addressing 1 make 2 obvious and worse, addressing 2 is a whole
>> different story and should probably be discussed outside of this
>> thread. And i would not like to inherit responsibility for 2. In
>> fact the whole discussion emphasizes that it was a good idea to make
>> GPGSM depend on GPG, because it allows to somewhat reuse existing tests.
>
> IMHO there is a big difference between inheriting responsibility for
> something, and not making it worse.

Well said.

Re: [PATCH v2 9/9] gpg-interface t: extend the existing GPG tests with GPGSM

[Henning Schild]

Am Wed, 11 Jul 2018 10:33:52 -0400
schrieb Jeff King <peff@peff.net>:

> On Tue, Jul 10, 2018 at 10:52:31AM +0200, Henning Schild wrote:
> 
> > diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh
> > index a5d3b2cba..9dcb4e990 100755
> > --- a/t/lib-gpg.sh
> > +++ b/t/lib-gpg.sh
> > @@ -38,7 +38,14 @@ then
> >  			"$TEST_DIRECTORY"/lib-gpg/ownertrust &&
> >  		gpg --homedir "${GNUPGHOME}" </dev/null >/dev/null
> > 2>&1 \ --sign -u committer@example.com &&
> > -		test_set_prereq GPG
> > +		test_set_prereq GPG &&
> > +		echo | gpgsm --homedir "${GNUPGHOME}" -o
> > "$TEST_DIRECTORY"/lib-gpg/gpgsm.crt.user --passphrase-fd 0
> > --pinentry-mode loopback --generate-key --batch
> > "$TEST_DIRECTORY"/lib-gpg/gpgsm-gen-key.in &&
> > +		gpgsm --homedir "${GNUPGHOME}" --import
> > "$TEST_DIRECTORY"/lib-gpg/gpgsm.crt.user &&
> > +		gpgsm --homedir "${GNUPGHOME}" -K | grep
> > fingerprint: | cut -d" " -f4 | tr -d '\n' >
> > ${GNUPGHOME}/trustlist.txt &&
> > +		echo " S relax" >> ${GNUPGHOME}/trustlist.txt &&
> > +		(gpgconf --kill gpg-agent >/dev/null 2>&1 || : ) &&
> > +		echo hello | gpgsm --homedir "${GNUPGHOME}" -u
> > committer@example.com -o /dev/null --sign - 2>&1 &&
> > +		test_set_prereq GPGSM  
> 
> By the way, I think these gpgsm invocations need some redirection of
> their stderr. This isn't inside a test_expect block, so they spew to
> the terminal even in non-verbose mode.
> 
> Ideally they'd redirect to descriptor 4, which then respects "-v"
> properly (though the existing gpg invocations just go to 2).

Thanks for pointing that out. I decided to go for 2 as well, for
consistency with gpg and not having to change that.

Henning

> -Peff

Re: [PATCH v2 6/9] gpg-interface: do not hardcode the key string len anymore

[Jeff King]

On Wed, Jul 11, 2018 at 06:15:05PM +0200, Henning Schild wrote:

> > diff --git a/gpg-interface.c b/gpg-interface.c
> > index bf8d567a4c..139b0f561e 100644
> > --- a/gpg-interface.c
> > +++ b/gpg-interface.c
> > @@ -97,7 +97,7 @@ static void parse_gpg_output(struct signature_check
> > *sigc) sigc->key = xmemdupz(found, next - found);
> >  			/* The ERRSIG message is not followed by
> > signer information */ if (sigc-> result != 'E') {
> > -				found = next + 1;
> > +				found = *next ? next + 1 : next;
> 
> This would keep us in bounds of the unexpected string. But ignore the
> line instead of "complaining" or crashing.
> 
> But you are right, it is easy enough and ignoring the line is probably
> the best way of dealing with it.
> 
> i will change the condition to
> > if (*next && sigc-> result != 'E')
> 
> also skipping the following strchrnul and xmemdupz

That sounds good to me. Thanks.

-Peff

Re: [PATCH v2 5/9] gpg-interface: introduce an abstraction for multiple gpg formats

[Henning Schild]

Am Tue, 10 Jul 2018 10:16:39 -0700
schrieb Junio C Hamano <gitster@pobox.com>:

> Henning Schild <henning.schild@siemens.com> writes:
> 
> > Create a struct that holds the format details for the supported
> > formats. At the moment that is still just "openpgp". This commit
> > prepares for the introduction of more formats, that might use other
> > programs and match other signatures.
> >
> > Signed-off-by: Henning Schild <henning.schild@siemens.com>
> > ---
> >  gpg-interface.c | 74
> > ++++++++++++++++++++++++++++++++++++++++++++------------- 1 file
> > changed, 58 insertions(+), 16 deletions(-)
> >
> > diff --git a/gpg-interface.c b/gpg-interface.c
> > index ed0e55917..0a8d1bff3 100644
> > --- a/gpg-interface.c
> > +++ b/gpg-interface.c
> > @@ -7,12 +7,46 @@
> >  #include "tempfile.h"
> >  
> >  static char *configured_signing_key;
> > -static const char *gpg_format = "openpgp";
> > -static const char *gpg_program = "gpg";
> > +struct gpg_format_data {
> > +	const char *format;
> > +	const char *program;
> > +	const char *extra_args_verify[1];
> > +	const char *sigs[2];
> > +};  
> 
> Do you use identifier "gpg_format" elsewhere as a structure name?  A
> structure is there always to hold "data", so often "_data" suffix is
> meaningless in a structure type name like this.

i renamed the struct to "gpg_format" and what was "gpg_format" before,
a const char* holding the format we use for signing, is now a pointer
into gpg_formats and called "current_format"
 
> >  #define PGP_SIGNATURE "-----BEGIN PGP SIGNATURE-----"
> >  #define PGP_MESSAGE "-----BEGIN PGP MESSAGE-----"
> >  
> > +enum gpgformats { PGP_FMT };
> > +struct gpg_format_data gpg_formats[] = {
> > +	{ .format = "openpgp", .program = "gpg",
> > +	  .extra_args_verify = { "--keyid-format=long" },
> > +	  .sigs = { PGP_SIGNATURE, PGP_MESSAGE }
> > +	},
> > +};
> > +static const char *gpg_format = "openpgp";
> > +
> > +static struct gpg_format_data *get_format_data(const char *str)  
> 
> get_format_data_by_name() or something like that, for consistency
> with the next function, perhaps?

I went for get_format_by_name and also renamed "format" inside the
struct to be "name"

> > +{
> > +	int i;
> > +	for (i = 0; i < ARRAY_SIZE(gpg_formats); i++)
> > +		if (!strcasecmp(gpg_formats[i].format, str))
> > +			return gpg_formats + i;
> > +	return NULL;
> > +}
> > +
> > +static struct gpg_format_data *get_format_data_by_sig(const char
> > *sig) +{
> > +	int i, j;
> > +	for (i = 0; i < ARRAY_SIZE(gpg_formats); i++)
> > +		for (j = 0; j < ARRAY_SIZE(gpg_formats[i].sigs);
> > j++)
> > +			if (gpg_formats[i].sigs[j] && 
> > +			    !strncmp(gpg_formats[i].sigs[j], sig,
> > +
> > strlen(gpg_formats[i].sigs[j])))
> > +				return gpg_formats + i;
> > +	return NULL;
> > +}
> > +
> >  void signature_check_clear(struct signature_check *sigc)
> >  {
> >  	FREE_AND_NULL(sigc->payload);
> > @@ -104,8 +138,7 @@ void print_signature_buffer(const struct
> > signature_check *sigc, unsigned flags) 
> >  static int is_gpg_start(const char *line)
> >  {
> > -	return starts_with(line, PGP_SIGNATURE) ||
> > -		starts_with(line, PGP_MESSAGE);
> > +	return (get_format_data_by_sig(line) != NULL);  
> 
> If this is still called "is_gpg_start()", then shouldn't the
> implementation be more like this?
> 
> 	struct gpg_format *found = get_format_data_by_sig(line);
> 
> 	return found && found == &gpg_formats[PGP_FMT];
> 
> It probably does not make much difference in the end, as you won't
> have functions is_(gpg|gpgsm|somethingelse|yetanother)_start() but
> either have a single function "does some x509 looking block start
> here?" or "I am expecting gpg and nothing else, does a block for
> that start here?" and at that point this will no longer be called
> is_gpg_start(), I would expect.

I dropped the whole thing since it is now a one-liner and only called
on one place.

> >  size_t parse_signature(const char *buf, size_t size)
> > @@ -140,18 +173,14 @@ int git_gpg_config(const char *var, const
> > char *value, void *cb) }
> >  
> >  	if (!strcmp(var, "gpg.format")) {
> > -		if (strcasecmp(value, "openpgp"))
> > +		if (!get_format_data(value))
> >  			return error("malformed value for %s: %s",
> > var, value); return git_config_string(&gpg_format, var, value);  
> 
> This is a bug introduced in an earlier step in the series, but you
> will segfault when given a configuration that looks like this:
> 
> 	[gpg]
> 		format
> 
> Imitate the way how !value is diagnosed upfront for "gpg.program"
> below before this patch.

The next version will take care of this.

> >  	}
> >  
> > -	if (!strcmp(var, "gpg.program")) {
> > -		if (!value)
> > -			return config_error_nonbool(var);
> > -		gpg_program = xstrdup(value);
> > -		return 0;
> > -	}
> > -
> > +	if (!strcmp(var, "gpg.program"))
> > +		return
> > git_config_string(&gpg_formats[PGP_FMT].program, var,
> > +					 value);  
> 
> git_config_string() has its own "do not feed boolean 'true' to me"
> check, so this change does not introduce a regression.
> 
> > @@ -165,12 +194,16 @@ const char *get_signing_key(void)
> >  int sign_buffer(struct strbuf *buffer, struct strbuf *signature,
> > const char *signing_key) {
> >  	struct child_process gpg = CHILD_PROCESS_INIT;
> > +	struct gpg_format_data *fmt;
> >  	int ret;
> >  	size_t i, j, bottom;
> >  	struct strbuf gpg_status = STRBUF_INIT;
> >  
> > +	fmt = get_format_data(gpg_format);
> > +	if (!fmt)
> > +		BUG("bad gpg_format '%s'", gpg_format);
> >  	argv_array_pushl(&gpg.args,
> > -			 gpg_program,
> > +			 fmt->program,
> >  			 "--status-fd=2",
> >  			 "-bsau", signing_key,
> >  			 NULL);
> > @@ -208,8 +241,9 @@ int verify_signed_buffer(const char *payload,
> > size_t payload_size, struct strbuf *gpg_output, struct strbuf
> > *gpg_status) {
> >  	struct child_process gpg = CHILD_PROCESS_INIT;
> > +	struct gpg_format_data *fmt;
> >  	struct tempfile *temp;
> > -	int ret;
> > +	int ret, i;
> >  	struct strbuf buf = STRBUF_INIT;
> >  
> >  	temp = mks_tempfile_t(".git_vtag_tmpXXXXXX");
> > @@ -223,10 +257,18 @@ int verify_signed_buffer(const char *payload,
> > size_t payload_size, return -1;
> >  	}
> >  
> > +	fmt = get_format_data_by_sig(signature);
> > +	assert(fmt);
> > +
> > +	argv_array_pushl(&gpg.args,
> > +			 fmt->program, NULL);
> > +	for (i = 0; i < ARRAY_SIZE(fmt->extra_args_verify); i++)
> > +		if (fmt->extra_args_verify[i])
> > +			argv_array_pushl(&gpg.args,
> > +
> > fmt->extra_args_verify[i], NULL);  
> 
> This loop allows us to extend .extra_args_verify when we need to
> support another variant that needs to take two arguments by making
> .extra_args field larger for all variants and stuffing a NULL to
> unused slots, and it even allows a nonsense like this
> 
> 	.extra_args_verify = { NULL, "--keyid-format=long" },
> 
> I cannot quite shake the feeling that .extra_args_verify and .sigs
> fields should not be of fixed sized array of strings, but rather be
> of type "const char **" that is NULL terminated.  Such a clean-up
> will make it harder to write initializers, so it may not be worth
> it, I guess.

I now have NULL terminated variable size arrays.

Thanks,
Henning
 
> >  	argv_array_pushl(&gpg.args,
> > -			 gpg_program,
> >  			 "--status-fd=1",
> > -			 "--keyid-format=long",
> >  			 "--verify", temp->filename.buf, "-",
> >  			 NULL);  

Re: [PATCH v2 7/9] gpg-interface: introduce new config to select per gpg format program

[Henning Schild]

Replies to this one have been ignored for v3. I do not know how to
proceed here.

Henning

Am Tue, 10 Jul 2018 10:52:29 +0200
schrieb Henning Schild <henning.schild@siemens.com>:

> Supporting multiple signing formats we will have the need to
> configure a custom program each. Add a new config value to cater for
> that.
> 
> Signed-off-by: Henning Schild <henning.schild@siemens.com>
> ---
>  Documentation/config.txt | 5 +++++
>  gpg-interface.c          | 2 +-
>  2 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index ac373e3f4..c0bd80954 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1832,6 +1832,11 @@ gpg.format::
>  	Specifies which key format to use when signing with
> `--gpg-sign`. Default is "openpgp", that is also the only supported
> value. 
> +gpg.<format>.program::
> +	Use this to customize the program used for the signing
> format you
> +	chose. (see gpg.program) gpg.openpgp.program is a synonym
> for the
> +	legacy gpg.program.
> +
>  gui.commitMsgWidth::
>  	Defines how wide the commit message window is in the
>  	linkgit:git-gui[1]. "75" is the default.
> diff --git a/gpg-interface.c b/gpg-interface.c
> index ac2df498d..65098430f 100644
> --- a/gpg-interface.c
> +++ b/gpg-interface.c
> @@ -179,7 +179,7 @@ int git_gpg_config(const char *var, const char
> *value, void *cb) return git_config_string(&gpg_format, var, value);
>  	}
>  
> -	if (!strcmp(var, "gpg.program"))
> +	if (!strcmp(var, "gpg.program") || !strcmp(var,
> "gpg.openpgp.program")) return
> git_config_string(&gpg_formats[PGP_FMT].program, var, value);
>  	return 0;

Re: [PATCH v2 5/9] gpg-interface: introduce an abstraction for multiple gpg formats

[Henning Schild]

Am Tue, 10 Jul 2018 12:23:32 -0400
schrieb Jeff King <peff@peff.net>:

> On Tue, Jul 10, 2018 at 10:52:27AM +0200, Henning Schild wrote:
> 
> > Create a struct that holds the format details for the supported
> > formats. At the moment that is still just "openpgp". This commit
> > prepares for the introduction of more formats, that might use other
> > programs and match other signatures.  
> 
> Great, this looks like a good incremental step.
> 
> >  static char *configured_signing_key;
> > -static const char *gpg_format = "openpgp";
> > -static const char *gpg_program = "gpg";
> > +struct gpg_format_data {
> > +	const char *format;
> > +	const char *program;
> > +	const char *extra_args_verify[1];
> > +	const char *sigs[2];
> > +};  
> 
> These magic numbers are at a weird distance from where we fill them
> in:
> 
> > +struct gpg_format_data gpg_formats[] = {
> > +	{ .format = "openpgp", .program = "gpg",
> > +	  .extra_args_verify = { "--keyid-format=long" },
> > +	  .sigs = { PGP_SIGNATURE, PGP_MESSAGE }
> > +	},
> > +};  
> 
> I'm not sure if we can easily do any better in C, though. Declaring
> the struct with an open-ended "[]" would make the compiler unhappy.
> We could do something like:
> 
>   struct gpg_format_data {
> 	...
> 	const char **extra_args_verify;
>   };
>   ...
>   static const char *openpgp_verify_args[] = {
> 	"--key-id-format=long"
>   };
>   ...
>   static struct gpg_format_data gpg_formats[] = {
> 	{ ...
> 	  .extra_args_verify = openpgp_verify_args
> 	}
>   };
> 
> I'm not sure if that's more horrible or less. It's worse to write in
> the first place, but it's slightly easier to maintain going forward. I
> dunno.

I switched to that, looks good but i am also not sure which one is
better.

> > +enum gpgformats { PGP_FMT };  
> 
> Looks like we use this only for indexing the gpg_formats array. I know
> that C guarantees 0-indexing, but if we're depending on it, it might
> be worth writing out "PGP_FMT = 0" explicitly. And probably adding a
> comment that this needs to remain in sync with the array.
> 
> The other alternative is that we could simply use
> get_format_data("openpgp"), though that does add a minor runtime cost.

Thanks, i got rid of the enum and use get_format_data with two fixed
strings for the two possible matches.

> > +struct gpg_format_data gpg_formats[] = {
> > +	{ .format = "openpgp", .program = "gpg",
> > +	  .extra_args_verify = { "--keyid-format=long" },
> > +	  .sigs = { PGP_SIGNATURE, PGP_MESSAGE }
> > +	},
> > +};  
> 
> This array should be marked static, I think.

Yes.

> > +static struct gpg_format_data *get_format_data(const char *str)
> > +{
> > +	int i;
> > +	for (i = 0; i < ARRAY_SIZE(gpg_formats); i++)
> > +		if (!strcasecmp(gpg_formats[i].format, str))
> > +			return gpg_formats + i;
> > +	return NULL;
> > +}  
> 
> This looks much nicer than the assert()-ing version from v1.
> 
> > +static struct gpg_format_data *get_format_data_by_sig(const char
> > *sig) +{
> > +	int i, j;
> > +	for (i = 0; i < ARRAY_SIZE(gpg_formats); i++)
> > +		for (j = 0; j < ARRAY_SIZE(gpg_formats[i].sigs);
> > j++)
> > +			if (gpg_formats[i].sigs[j] && 
> > +			    !strncmp(gpg_formats[i].sigs[j], sig,
> > +
> > strlen(gpg_formats[i].sigs[j])))
> > +				return gpg_formats + i;
> > +	return NULL;
> > +}  
> 
> This might be a little more readable with:
> 
>   starts_with(sig, gpg_formats[i].sigs[j])
> 
> instead of the strncmp. It may also be more efficient, as we don't
> have to compute the strlen of the prefix for each non-matching line
> (the compiler _might_ be smart enough to realize these are all string
> literals, but it's pretty buried).

Thanks, will do.

> I also wondered if our prefix matching here is overly loose. We have
> to do a prefix match, since "sig" isn't terminated at the line
> buffer. So I think we'd match:
> 
>   --- BEGIN PGP MESSAGE --- AND SOME OTHER STUFF ---
> 
> on a line. But I think that's no different than the current code. If
> we care, I guess we could look for '\n' or '\0' immediately after.

Let us not care, just like current code.

> >  static int is_gpg_start(const char *line)
> >  {
> > -	return starts_with(line, PGP_SIGNATURE) ||
> > -		starts_with(line, PGP_MESSAGE);
> > +	return (get_format_data_by_sig(line) != NULL);
> >  }  
> 
> I don't know if we've ever discussed this style explicitly, but we'd
> usually omit the unnecessary parentheses for the return here.

Will remove those braces.

> > @@ -140,18 +173,14 @@ int git_gpg_config(const char *var, const
> > char *value, void *cb) }
> >  
> >  	if (!strcmp(var, "gpg.format")) {
> > -		if (strcasecmp(value, "openpgp"))
> > +		if (!get_format_data(value))
> >  			return error("malformed value for %s: %s",
> > var, value); return git_config_string(&gpg_format, var, value);
> >  	}  
> 
> Much nicer than v1.
> 
> > @@ -165,12 +194,16 @@ const char *get_signing_key(void)
> >  int sign_buffer(struct strbuf *buffer, struct strbuf *signature,
> > const char *signing_key) {
> >  	struct child_process gpg = CHILD_PROCESS_INIT;
> > +	struct gpg_format_data *fmt;
> >  	int ret;
> >  	size_t i, j, bottom;
> >  	struct strbuf gpg_status = STRBUF_INIT;
> >  
> > +	fmt = get_format_data(gpg_format);
> > +	if (!fmt)
> > +		BUG("bad gpg_format '%s'", gpg_format);  
> 
> This makes sense as a BUG, because we would already have validated it
> when parsing gpg.format earlier. 
> That does make me wonder if we should
> simply be storing a "struct gpg_format_data" instead of a string,
> though. I.e., at the top-level:
> 
>   /* default to signing with openpgp */
>   static struct gpg_format_data *gpg_format = &gpg_formats[0];

Good idea, implemented that.

> > @@ -223,10 +257,18 @@ int verify_signed_buffer(const char *payload,
> > size_t payload_size, return -1;
> >  	}
> >  
> > +	fmt = get_format_data_by_sig(signature);
> > +	assert(fmt);  
> 
> Is this assert() right? The signature data comes from the user. I
> guess to get here we'll already have matched their signature via
> is_gpg_start(), and this is just a cross-check? If so, then it's OK to
> assert, but a BUG() with a descriptive message would be better still.

Turned that into a BUG(). I knew there where 2 such asserts but must
have been looking for assert(0) when i fixed the first.

> I also wonder if whoever parses the signature should get back a
> gpg_format_data and just pass it in here, so we don't have to reparse.
> That's what my earlier series did. It requires tweaking the function
> signatures, but IMHO the result was a lot more obvious.

It would make sense to return the type of signature right with
parse_signature, and would probably safe a few lookups. I started the
refactoring and it turned out to become a pretty big change. That is
why i would prefer to do that on top of the series or leave it the way
it is now.

> > +	argv_array_pushl(&gpg.args,
> > +			 fmt->program, NULL);  
> 
> If you're just pushing one thing, you don't need pushl(). You can
> just:
> 
>   argv_array_push(&gpg.args, fmt->program);
> 
> > +	for (i = 0; i < ARRAY_SIZE(fmt->extra_args_verify); i++)
> > +		if (fmt->extra_args_verify[i])
> > +			argv_array_pushl(&gpg.args,
> > +
> > fmt->extra_args_verify[i], NULL);  
> 
> Likewise here. Though if you made extra_args_verify a NULL-terminated
> list, this whole loop could become:
> 
>   argv_array_pushv(&gpg.args, fmt->extra_args_verify);
> 
> It's not _that_ much code, but I think using NULL-terminated lists in
> a situation like this is more idiomatic for our code base.

Thanks, good point!

> -Peff

Re: [PATCH v2 7/9] gpg-interface: introduce new config to select per gpg format program

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 820 bytes --]

On Tue, Jul 10, 2018 at 12:56:38PM -0400, Jeff King wrote:
> On Tue, Jul 10, 2018 at 12:54:13PM -0400, Jeff King wrote:
> 
> > Should we allow:
> > 
> >   [gpg "OpenPGP"]
> >   program = whatever
> > 
> > given that we allow:
> > 
> >   [gpg]
> >   format = OpenPGP
> > 
> > ? I think just using strcasecmp() here would be sufficient. But I wonder
> > if it is a symptom of using the wrong tool (subsections) when we don't
> > need it.
> 
> I did just read the discussion in response to v1, where everybody told
> you the opposite. ;)
> 
> So I guess my question/points are more for brian and Junio.

I'm okay with us forcing "openpgp".  That seems sane enough for now, and
if people scream loudly, we can loosen it.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 867 bytes --]

Re: [PATCH v2 9/9] gpg-interface t: extend the existing GPG tests with GPGSM

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1964 bytes --]

On Wed, Jul 11, 2018 at 12:38:24PM +0200, Henning Schild wrote:
> Am Tue, 10 Jul 2018 13:09:01 -0400
> schrieb Jeff King <peff@peff.net>:
> 
> > On Tue, Jul 10, 2018 at 10:52:31AM +0200, Henning Schild wrote:
> > 
> > > diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh
> > > index a5d3b2cba..9dcb4e990 100755
> > > --- a/t/lib-gpg.sh
> > > +++ b/t/lib-gpg.sh
> > > @@ -38,7 +38,14 @@ then
> > >  			"$TEST_DIRECTORY"/lib-gpg/ownertrust &&
> > >  		gpg --homedir "${GNUPGHOME}" </dev/null >/dev/null
> > > 2>&1 \ --sign -u committer@example.com &&
> > > -		test_set_prereq GPG
> > > +		test_set_prereq GPG &&
> > > +		echo | gpgsm --homedir "${GNUPGHOME}" -o
> > > "$TEST_DIRECTORY"/lib-gpg/gpgsm.crt.user --passphrase-fd 0
> > > --pinentry-mode loopback --generate-key --batch
> > > "$TEST_DIRECTORY"/lib-gpg/gpgsm-gen-key.in &&  
> > 
> > Is this --generate-key going to require high-quality entropy? That
> > could slow the test suite down (and maybe even stall it quite a bit
> > on servers doing automated CI).
> 
> Yes, i also saw that getting "stuck" on one machine. But i blamed an
> experimental kernel.
> 
> > Can we save a dummy generated key and just import it? That's what we
> > do for the regular gpg case.
> 
> I will look into storing a binary and leaving notes how it was
> generated, just like regular gpg does. The reason i did not do that in
> the first place is that x509 certs have a validity and we introduce
> time into the picture. But i will see if i can generate epoch->infinity
> to get the host clock or just the future out of the picture.

Yeah, I agree that would be good.  If gpgsm does anything like what gpg
does, it will require the use of /dev/random, which means this will
definitely be slow on build servers and other noninteractive systems.
We have this problem at $DAYJOB when automating generation of gpg keys.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 867 bytes --]

Re: [PATCH v2 0/9] X509 (gpgsm) commit signing support

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1619 bytes --]

On Tue, Jul 10, 2018 at 01:12:24PM -0400, Jeff King wrote:
> On Tue, Jul 10, 2018 at 10:52:22AM +0200, Henning Schild wrote:
> > This series can be seen as a follow up of a series that appeared under
> > the name "gpg-interface: Multiple signing tools" in april 2018 [1]. After
> > that series was not merged i decided to get my patches ready. The
> > original series aimed at being generic for any sort of signing tool, while
> > this series just introduced the X509 variant of gpg. (gpgsm)
> > I collected authors and reviewers of that first series and already put them
> > on cc.
> 
> This series is a fine replacement for that earlier work. It's flexible
> enough to allow what we really wanted out of that series (gpgsm support,
> or another drop-in tool that uses the same interface). It doesn't lay
> any groundwork for further tools (like signify), but I think the
> consensus on the list was to punt on that until somebody had more
> concrete plans for adding such a tool.

I actually think this moves in a nice direction for adding support for
minisign/signify and other schemes.  There's a way to look up what
algorithm is in use in a particular context based on the first line and
a general interface for deciding what format to write.  Granted, it
currently still is very specific to gpg-style tools, but I think this is
an improvement in that regard.

As an OpenPGP user, I have no interest in adding support for other
tools, but I think this should make it easier if someone else wants to
do that.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 867 bytes --]

Re: [PATCH v2 0/9] X509 (gpgsm) commit signing support

[Jeff King]

On Sat, Jul 14, 2018 at 06:33:12PM +0000, brian m. carlson wrote:

> > This series is a fine replacement for that earlier work. It's flexible
> > enough to allow what we really wanted out of that series (gpgsm support,
> > or another drop-in tool that uses the same interface). It doesn't lay
> > any groundwork for further tools (like signify), but I think the
> > consensus on the list was to punt on that until somebody had more
> > concrete plans for adding such a tool.
> 
> I actually think this moves in a nice direction for adding support for
> minisign/signify and other schemes.  There's a way to look up what
> algorithm is in use in a particular context based on the first line and
> a general interface for deciding what format to write.  Granted, it
> currently still is very specific to gpg-style tools, but I think this is
> an improvement in that regard.

My issue with this for helping with signify is that it creates a new
gpg.<tool>.* hierarchy with two slots (openpgp and x509). But we would
not want gpg.signify.program, would we?  That makes no sense, as neither
the signature-matching nor the program invocation are gpg-like.

But if we later moved to "signingtool.<tool>.*", now we have an extra
layer of compatibility to deal with. E.g., signingtool.openpgp.program
is the same as gpg.openpgp.program which is the same as gpg.program.

I think we can do that, but it means more historical baggage.

I'm OK with that since signify support is purely hypothetical at this
point.  But that's why I say that this doesn't lay the groundwork in the
way that the other series did.

> As an OpenPGP user, I have no interest in adding support for other
> tools, but I think this should make it easier if someone else wants to
> do that.

I don't plan to work on signify (or other tools) anytime soon either. My
interest here is in x509, since that's what enterprises would use over
pgp.

I actually dislike pgp for this application, too, because I find the key
management kind of complicated and tedious. But at least it's a standard
among open source folks.

-Peff

Re: [PATCH v2 7/9] gpg-interface: introduce new config to select per gpg format program

[Jeff King]

On Sat, Jul 14, 2018 at 06:13:47PM +0000, brian m. carlson wrote:

> On Tue, Jul 10, 2018 at 12:56:38PM -0400, Jeff King wrote:
> > On Tue, Jul 10, 2018 at 12:54:13PM -0400, Jeff King wrote:
> > 
> > > Should we allow:
> > > 
> > >   [gpg "OpenPGP"]
> > >   program = whatever
> > > 
> > > given that we allow:
> > > 
> > >   [gpg]
> > >   format = OpenPGP
> > > 
> > > ? I think just using strcasecmp() here would be sufficient. But I wonder
> > > if it is a symptom of using the wrong tool (subsections) when we don't
> > > need it.
> > 
> > I did just read the discussion in response to v1, where everybody told
> > you the opposite. ;)
> > 
> > So I guess my question/points are more for brian and Junio.
> 
> I'm okay with us forcing "openpgp".  That seems sane enough for now, and
> if people scream loudly, we can loosen it.

Well, I specifically meant "are you sure subsections like this are a
good idea". But it seems like people think so?

If so, then I think the best route is just dictating that yes,
gpg.format is case-sensitive because it is referencing
gpg.<format>.program, which is itself case-sensitive (and "openpgp" is
the right spelling).

-Peff

Re: [PATCH v2 7/9] gpg-interface: introduce new config to select per gpg format program

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Sat, Jul 14, 2018 at 06:13:47PM +0000, brian m. carlson wrote:
>
>> On Tue, Jul 10, 2018 at 12:56:38PM -0400, Jeff King wrote:
>> > On Tue, Jul 10, 2018 at 12:54:13PM -0400, Jeff King wrote:
>> > 
>> > > Should we allow:
>> > > 
>> > >   [gpg "OpenPGP"]
>> > >   program = whatever
>> > > 
>> > > given that we allow:
>> > > 
>> > >   [gpg]
>> > >   format = OpenPGP
>> > > 
>> > > ? I think just using strcasecmp() here would be sufficient. But I wonder
>> > > if it is a symptom of using the wrong tool (subsections) when we don't
>> > > need it.
>> > 
>> > I did just read the discussion in response to v1, where everybody told
>> > you the opposite. ;)
>> > 
>> > So I guess my question/points are more for brian and Junio.
>> 
>> I'm okay with us forcing "openpgp".  That seems sane enough for now, and
>> if people scream loudly, we can loosen it.
>
> Well, I specifically meant "are you sure subsections like this are a
> good idea". But it seems like people think so?

I admit that I did not even consider that there may be better tool
than using subsections to record this information.  What are the
possibilities you have in mind (if you have one)?

>
> If so, then I think the best route is just dictating that yes,
> gpg.format is case-sensitive because it is referencing
> gpg.<format>.program, which is itself case-sensitive (and "openpgp" is
> the right spelling).
>
> -Peff

Re: [PATCH v2 7/9] gpg-interface: introduce new config to select per gpg format program

[Jeff King]

On Mon, Jul 16, 2018 at 02:56:34PM -0700, Junio C Hamano wrote:

> >> I'm okay with us forcing "openpgp".  That seems sane enough for now, and
> >> if people scream loudly, we can loosen it.
> >
> > Well, I specifically meant "are you sure subsections like this are a
> > good idea". But it seems like people think so?
> 
> I admit that I did not even consider that there may be better tool
> than using subsections to record this information.  What are the
> possibilities you have in mind (if you have one)?

I don't think there is another tool except two-level options, like
"gpg.openpgpprogram" and "gpg.x509program".

Although those are a bit ugly, I just wondered if they might make things
simpler, since AFAIK we are not planning to add more config options
here. Like gpg.x509.someotherflag, nor gpg.someothertool.program.

Of course one reason _for_ the tri-level is that we might one day add
gpg.x509.someotherflag, and this gives us room to do it with less
awkwardness (i.e., a proliferation of gpg.x509someflag options).

-Peff

Re: [PATCH v2 7/9] gpg-interface: introduce new config to select per gpg format program

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Jul 16, 2018 at 02:56:34PM -0700, Junio C Hamano wrote:
>
>> >> I'm okay with us forcing "openpgp".  That seems sane enough for now, and
>> >> if people scream loudly, we can loosen it.
>> >
>> > Well, I specifically meant "are you sure subsections like this are a
>> > good idea". But it seems like people think so?
>> 
>> I admit that I did not even consider that there may be better tool
>> than using subsections to record this information.  What are the
>> possibilities you have in mind (if you have one)?
>
> I don't think there is another tool except two-level options, like
> "gpg.openpgpprogram" and "gpg.x509program".
>
> Although those are a bit ugly, I just wondered if they might make things
> simpler, since AFAIK we are not planning to add more config options
> here. Like gpg.x509.someotherflag, nor gpg.someothertool.program.
>
> Of course one reason _for_ the tri-level is that we might one day add
> gpg.x509.someotherflag, and this gives us room to do it with less
> awkwardness (i.e., a proliferation of gpg.x509someflag options).

Yes, and signingtool.<name>.<key> is probably a good (ultra-)long
term direction.  Preparing the code may be quite a lot of work that
nobody may be interested in, and nothing other than the GPG family
might materialize for a long time, but if we can cheaply prepare
external interface less dependent on GPG/PGP, that by itself would
be a good thing to have, I would guess.

Thanks.