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

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

[Henning Schild]

This series adds support for signing commits with gpgsm.

The first two patches are cleanups of gpg-interface, while the next
four prepare for the introduction of the actual feature in patch 7.
Finally patch 8 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 (8):
  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
  gpg-interface: introduce an abstraction for multiple gpg formats
  t/t7510: check the validation of the new config gpg.format
  gpg-interface: do not hardcode the key string len anymore
  gpg-interface: introduce new signature format "X509" using gpgsm
  gpg-interface t: extend the existing GPG tests with GPGSM

 Documentation/config.txt   |  7 ++++
 builtin/receive-pack.c     | 17 +--------
 gpg-interface.c            | 94 ++++++++++++++++++++++++++++++++++++++--------
 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, 321 insertions(+), 35 deletions(-)
 create mode 100644 t/lib-gpg/gpgsm-gen-key.in

-- 
2.16.4

[PATCH 1/8] 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 2/8] 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 3/8] 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 "PGP" 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..c88903399 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 "PGP", 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..1def1f131 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 = "PGP";
 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 (!strcmp(value, "PGP"))
+			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 4/8] 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 "PGP". 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 | 80 +++++++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 64 insertions(+), 16 deletions(-)

diff --git a/gpg-interface.c b/gpg-interface.c
index 1def1f131..cd3b1b568 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 = "PGP";
-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 = "PGP", .program = "gpg",
+	  .extra_args_verify = { "--keyid-format=long", },
+	  .sigs = { PGP_SIGNATURE, PGP_MESSAGE, },
+	},
+};
+static const char *gpg_format = "PGP";
+
+static struct gpg_format_data *get_format_data(void)
+{
+	int i;
+	for (i = 0; i < ARRAY_SIZE(gpg_formats); i++)
+		if (!strcmp(gpg_formats[i].format, gpg_format))
+			return gpg_formats + i;
+	assert(0);
+}
+
+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)
@@ -132,6 +165,8 @@ void set_signing_key(const char *key)
 
 int git_gpg_config(const char *var, const char *value, void *cb)
 {
+	int i, j;
+
 	if (!strcmp(var, "user.signingkey")) {
 		if (!value)
 			return config_error_nonbool(var);
@@ -140,18 +175,20 @@ int git_gpg_config(const char *var, const char *value, void *cb)
 	}
 
 	if (!strcmp(var, "gpg.format")) {
-		if (!strcmp(value, "PGP"))
+		j = 0;
+		for (i = 0; i < ARRAY_SIZE(gpg_formats); i++)
+			if (!strcmp(value, gpg_formats[i].format)) {
+				j++;
+				break;
+			}
+		if (!j)
 			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 +202,14 @@ 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();
 	argv_array_pushl(&gpg.args,
-			 gpg_program,
+			 fmt->program,
 			 "--status-fd=2",
 			 "-bsau", signing_key,
 			 NULL);
@@ -208,8 +247,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 +263,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 5/8] t/t7510: check the validation of the new config gpg.format

[Henning Schild]

Valid values are already covered by all tests that use GPG, now also
test what happens if we go for an invalid one.

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..cb523513f 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 gpg config for malformed values' '
+	mv .git/config .git/config.old &&
+	test_when_finished "mv .git/config.old .git/config" &&
+	git config gpg.format malformed &&
+	test_expect_code 128 git commit -S --amend -m "fail" 2>result &&
+	test_i18ngrep "malformed value for gpg.format: malformed" result &&
+	test_i18ngrep "fatal: .*\.git/config" result &&
+	test_i18ngrep "fatal: .*line 2" result
+'
+
 test_done
-- 
2.16.4

[PATCH 6/8] 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 cd3b1b568..aa747278e 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 7/8] 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 |  5 ++++-
 gpg-interface.c          | 10 +++++++++-
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index c88903399..337df6e48 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1828,9 +1828,12 @@ gpg.program::
 	signed, and the program is expected to send the result to its
 	standard output.
 
+gpg.programX509::
+	Just like gpg.program, here the default you override is "`gpgsm`".
+
 gpg.format::
 	Specifies which key format to use when signing with `--gpg-sign`.
-	Default is "PGP", that is also the only supported value.
+	Default is "PGP" and another possible value is "X509".
 
 gui.commitMsgWidth::
 	Defines how wide the commit message window is in the
diff --git a/gpg-interface.c b/gpg-interface.c
index aa747278e..85d721007 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 = "PGP", .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 = "PGP";
 
@@ -190,6 +195,9 @@ int git_gpg_config(const char *var, const char *value, void *cb)
 	if (!strcmp(var, "gpg.program"))
 		return git_config_string(&gpg_formats[PGP_FMT].program, var,
 					 value);
+	if (!strcmp(var, "gpg.programX509"))
+		return git_config_string(&gpg_formats[X509_FMT].program, var,
+					 value);
 	return 0;
 }
 
-- 
2.16.4

[PATCH 8/8] 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..a2f234053 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..9d5029fcf 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..69b9e05d6 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..a76bcf333 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..ef742aa16 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 4/8] gpg-interface: introduce an abstraction for multiple gpg formats

[Martin Ågren]

Hi Henning,

On 3 July 2018 at 14:38, Henning Schild <henning.schild@siemens.com> wrote:
> Create a struct that holds the format details for the supported formats.
> At the moment that is still just "PGP". 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>

Welcome to the mailing list! :-)

I'll just comment on a few thoughts I had while skimming this.

>  static char *configured_signing_key;
> -static const char *gpg_format = "PGP";
> -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 = "PGP", .program = "gpg",
> +         .extra_args_verify = { "--keyid-format=long", },
> +         .sigs = { PGP_SIGNATURE, PGP_MESSAGE, },
> +       },
> +};

I think those trailing commas are ok now, but I'm not sure...

I had the same thought about designated initializers. Those should be ok
now, c.f. cbc0f81d96 (strbuf: use designated initializers in STRBUF_INIT,
2017-07-10) and a73b3680c4 (Add and use generic name->id mapping code
for color slot parsing, 2018-05-26).

> +static const char *gpg_format = "PGP";
> +
> +static struct gpg_format_data *get_format_data(void)
> +{
> +       int i;
> +       for (i = 0; i < ARRAY_SIZE(gpg_formats); i++)
> +               if (!strcmp(gpg_formats[i].format, gpg_format))
> +                       return gpg_formats + i;
> +       assert(0);

This might be better written as `BUG("bad gpg_format '%s'",
gpg_format);` or something like that.

(It's not supposed to be triggered, not even by invalid data from the
user, right?)

>         if (!strcmp(var, "gpg.format")) {
> -               if (!strcmp(value, "PGP"))

This line was added in patch 3. It errors out precisely when gpg.format
is "PGP", no? That this doesn't break the whole series is explained by
1) it being removed in this patch 4, and 2) there being no tests. It
makes me wonder if something like patch 5 (test gpg.format) could be
part of patch 3, both with negative ("= malformed") and positive ("=
PGP") tests?

> +               j = 0;
> +               for (i = 0; i < ARRAY_SIZE(gpg_formats); i++)
> +                       if (!strcmp(value, gpg_formats[i].format)) {
> +                               j++;
> +                               break;
> +                       }
> +               if (!j)
>                         return error("malformed value for %s: %s", var, value);

`if (i == ARRAY_SIZE(gpg_formats))` and drop `j`?

Or check whether `get_format_data()` returns NULL? Hmm, well you can't,
since it takes its "input" from a global variable...

If you want to keep that global nature, the duplication of search-logic
could perhaps be avoided by having a helper function for returning the
index of a gpg_format (or -1).

>                 return git_config_string(&gpg_format, var, value);
>         }

Martin

Re: [PATCH 4/8] gpg-interface: introduce an abstraction for multiple gpg formats

[Henning Schild]

Am Wed, 4 Jul 2018 09:10:17 +0200
schrieb Martin Ågren <martin.agren@gmail.com>:

> Hi Henning,
> 
> On 3 July 2018 at 14:38, Henning Schild <henning.schild@siemens.com>
> wrote:
> > Create a struct that holds the format details for the supported
> > formats. At the moment that is still just "PGP". 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>  
> 
> Welcome to the mailing list! :-)

Thanks!

> I'll just comment on a few thoughts I had while skimming this.
> 
> >  static char *configured_signing_key;
> > -static const char *gpg_format = "PGP";
> > -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 = "PGP", .program = "gpg",
> > +         .extra_args_verify = { "--keyid-format=long", },
> > +         .sigs = { PGP_SIGNATURE, PGP_MESSAGE, },
> > +       },
> > +};  
> 
> I think those trailing commas are ok now, but I'm not sure...
> 
> I had the same thought about designated initializers. Those should be
> ok now, c.f. cbc0f81d96 (strbuf: use designated initializers in
> STRBUF_INIT, 2017-07-10) and a73b3680c4 (Add and use generic name->id
> mapping code for color slot parsing, 2018-05-26).

Ok, i did not actually check coding style yet. I could run it through a
tool, given there is a suggestion. Or i could address issues someone
points out in the review.
What i get from your comment is that it might be ok to leave the code
as is, others have introduces similar constructs before me.

> > +static const char *gpg_format = "PGP";
> > +
> > +static struct gpg_format_data *get_format_data(void)
> > +{
> > +       int i;
> > +       for (i = 0; i < ARRAY_SIZE(gpg_formats); i++)
> > +               if (!strcmp(gpg_formats[i].format, gpg_format))
> > +                       return gpg_formats + i;
> > +       assert(0);  
> 
> This might be better written as `BUG("bad gpg_format '%s'",
> gpg_format);` or something like that.
> 
> (It's not supposed to be triggered, not even by invalid data from the
> user, right?)

Yes that is code that can not (should not) be reached. I agree that an
assert(0) is not very expressive and will fix that in v2.

> 
> >         if (!strcmp(var, "gpg.format")) {
> > -               if (!strcmp(value, "PGP"))  
> 
> This line was added in patch 3. It errors out precisely when
> gpg.format is "PGP", no? That this doesn't break the whole series is
> explained by 1) it being removed in this patch 4, and 2) there being
> no tests. It makes me wonder if something like patch 5 (test
> gpg.format) could be part of patch 3, both with negative ("=
> malformed") and positive ("= PGP") tests?

I will pull the tests from patch 5 before touching that code and fix up
issues inbetween. The whole series saw a "make test" inbetween all
commits.

> > +               j = 0;
> > +               for (i = 0; i < ARRAY_SIZE(gpg_formats); i++)
> > +                       if (!strcmp(value, gpg_formats[i].format)) {
> > +                               j++;
> > +                               break;
> > +                       }
> > +               if (!j)
> >                         return error("malformed value for %s: %s",
> > var, value);  
> 
> `if (i == ARRAY_SIZE(gpg_formats))` and drop `j`?
> 
> Or check whether `get_format_data()` returns NULL? Hmm, well you
> can't, since it takes its "input" from a global variable...
> 
> If you want to keep that global nature, the duplication of
> search-logic could perhaps be avoided by having a helper function for
> returning the index of a gpg_format (or -1).

True, the two are almost the same and should be merged. Will do in v2.

Henning

> >                 return git_config_string(&gpg_format, var, value);
> >         }  
> 
> Martin

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

[brian m. carlson]

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

On Tue, Jul 03, 2018 at 02:38:15PM +0200, Henning Schild wrote:
> Add "gpg.format" where the user can specify which type of signature to
> use for commits. At the moment only "PGP" is supported and the value is
> not even used. This commit prepares for a new types of signatures.

We typically prefer to have option values specified in lower case.  I
also think "openpgp" might be better than "PGP", since that's the name
of the specification and it would avoid any potential unhappiness about
compatibility with PGP or trademarks.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

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

Re: [PATCH 7/8] gpg-interface: introduce new signature format "X509" using gpgsm

[brian m. carlson]

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

On Tue, Jul 03, 2018 at 02:38:19PM +0200, Henning Schild wrote:
> 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 |  5 ++++-
>  gpg-interface.c          | 10 +++++++++-
>  2 files changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index c88903399..337df6e48 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1828,9 +1828,12 @@ gpg.program::
>  	signed, and the program is expected to send the result to its
>  	standard output.
>  
> +gpg.programX509::

I'm not super excited about this name.  It seems to indicate we want a
level of hierarchy involved.

A hierarchy like sign.openpgp.program (falling back to gpg.program) and
sign.x509.program might be more logical.

> diff --git a/gpg-interface.c b/gpg-interface.c
> index aa747278e..85d721007 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 = "PGP", .program = "gpg",
>  	  .extra_args_verify = { "--keyid-format=long", },
>  	  .sigs = { PGP_SIGNATURE, PGP_MESSAGE, },
>  	},
> +	{ .format = "X509", .program = "gpgsm",

Similarly to my comment about "PGP", I think this would do well as
"x509".
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

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

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

[brian m. carlson]

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

On Tue, Jul 03, 2018 at 02:38:20PM +0200, Henning Schild wrote:
> 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

It looks like the GPGSM prerequisite will only be set if the GPG
prerequisite is set as well.  Do we want to consider the case when the
user might have gpgsm but not gpg?
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

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

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

[brian m. carlson]

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

On Tue, Jul 03, 2018 at 02:38:12PM +0200, Henning Schild wrote:
> This series adds support for signing commits with gpgsm.
> 
> The first two patches are cleanups of gpg-interface, while the next
> four prepare for the introduction of the actual feature in patch 7.
> Finally patch 8 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.

Overall, I think this is heading in a good direction.  I left a few
comments, but it seemed pretty sane.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

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

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

[Henning Schild]

Am Fri, 6 Jul 2018 01:18:35 +0000
schrieb "brian m. carlson" <sandals@crustytoothpaste.net>:

> On Tue, Jul 03, 2018 at 02:38:12PM +0200, Henning Schild wrote:
> > This series adds support for signing commits with gpgsm.
> > 
> > The first two patches are cleanups of gpg-interface, while the next
> > four prepare for the introduction of the actual feature in patch 7.
> > Finally patch 8 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.  
> 
> Overall, I think this is heading in a good direction.  I left a few
> comments, but it seemed pretty sane.

Thanks, i hope others think so too and that will eventually get merged.

Henning

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

[Henning Schild]

Am Fri, 6 Jul 2018 01:14:47 +0000
schrieb "brian m. carlson" <sandals@crustytoothpaste.net>:

> On Tue, Jul 03, 2018 at 02:38:20PM +0200, Henning Schild wrote:
> > 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  
> 
> It looks like the GPGSM prerequisite will only be set if the GPG
> prerequisite is set as well.  Do we want to consider the case when the
> user might have gpgsm but not gpg?

Nice finding, i should have tried to hide that better ;).

I thought about it when writing the code. There might be distributions
where you can install one without the other. I also introduces a few
tests that rely on the implication, where GPGSM tests on top of GPG.
(i.e. t7030 "create signed tags x509")
The implication is really just there for the tests, not for end-users.
Dropping it would create more variations in testing (make it more
expensive).

I would say it is not worth it at the moment.

Implementing the gpg.format detection by actually calling the "other"
program to find which one knows the key, would shine another light on
that one. But i kind of doubt that idea is a good one.

Henning

Re: [PATCH 7/8] gpg-interface: introduce new signature format "X509" using gpgsm

[Henning Schild]

Am Fri, 6 Jul 2018 01:10:13 +0000
schrieb "brian m. carlson" <sandals@crustytoothpaste.net>:

> On Tue, Jul 03, 2018 at 02:38:19PM +0200, Henning Schild wrote:
> > 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 |  5 ++++-
> >  gpg-interface.c          | 10 +++++++++-
> >  2 files changed, 13 insertions(+), 2 deletions(-)
> > 
> > diff --git a/Documentation/config.txt b/Documentation/config.txt
> > index c88903399..337df6e48 100644
> > --- a/Documentation/config.txt
> > +++ b/Documentation/config.txt
> > @@ -1828,9 +1828,12 @@ gpg.program::
> >  	signed, and the program is expected to send the result to
> > its standard output.
> >  
> > +gpg.programX509::  
> 
> I'm not super excited about this name.  It seems to indicate we want a
> level of hierarchy involved.
> 
> A hierarchy like sign.openpgp.program (falling back to gpg.program)
> and sign.x509.program might be more logical.
> 
> > diff --git a/gpg-interface.c b/gpg-interface.c
> > index aa747278e..85d721007 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 = "PGP", .program = "gpg",
> >  	  .extra_args_verify = { "--keyid-format=long", },
> >  	  .sigs = { PGP_SIGNATURE, PGP_MESSAGE, },
> >  	},
> > +	{ .format = "X509", .program = "gpgsm",  
> 
> Similarly to my comment about "PGP", I think this would do well as
> "x509".

Another naming discussion, lets keep discussing and i will implement it
once settled.

Henning

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

[Henning Schild]

Am Fri, 6 Jul 2018 01:01:48 +0000
schrieb "brian m. carlson" <sandals@crustytoothpaste.net>:

> On Tue, Jul 03, 2018 at 02:38:15PM +0200, Henning Schild wrote:
> > Add "gpg.format" where the user can specify which type of signature
> > to use for commits. At the moment only "PGP" is supported and the
> > value is not even used. This commit prepares for a new types of
> > signatures.  
> 
> We typically prefer to have option values specified in lower case.  I
> also think "openpgp" might be better than "PGP", since that's the name
> of the specification and it would avoid any potential unhappiness
> about compatibility with PGP or trademarks.

Thanks for your input. I was assuming the names to start a discussion
and i do not have a preference here.
Let us wait for further comments on naming, i will then implement
whatever the consensus is or what the maintainer requests.

In fact "gpg.format" could be dropped and we could try both "gpg" and
"gpgsm" and see where the signing-key is available. I decided to not
implement that because gpgsm (unlike) gpg does not return an error when
"--list-secret-keys" does not find anything. And we could have the odd
case where both would find a matching key. In which case we would need
"gpg.format" again to specify which one we prefer.

Henning

Re: [PATCH 4/8] gpg-interface: introduce an abstraction for multiple gpg formats

[Junio C Hamano]

Martin Ågren <martin.agren@gmail.com> writes:

>> +enum gpgformats { PGP_FMT };
>> +struct gpg_format_data gpg_formats[] = {
>> +       { .format = "PGP", .program = "gpg",
>> +         .extra_args_verify = { "--keyid-format=long", },
>> +         .sigs = { PGP_SIGNATURE, PGP_MESSAGE, },
>> +       },
>> +};
>
> I think those trailing commas are ok now, but I'm not sure...
>
> I had the same thought about designated initializers. Those should be ok
> now, c.f. cbc0f81d96 (strbuf: use designated initializers in STRBUF_INIT,
> 2017-07-10) and a73b3680c4 (Add and use generic name->id mapping code
> for color slot parsing, 2018-05-26).

As you said, we dipped our toes in designated initializers in both
struct and array, i.e. { .field = init }, { [offset] = init } last
summer and we haven't got complaints from minor platforms so far.

The "comma" thing you are wondering is something else.  The comma we
see above is after the last element in an array's initializer (and
the last element in a struct's initializer), which we have been
happily using from very early days (and they are kosher ANSI C).

What we've been avoiding was the comma after the last element in the
enum (in other words, if PGP_FMT had ',' after it in the above
quoted addition, that would have been violation of that rule), as
having such a trailing comma used to be ANSI C violation as well.  I
do not recall offhand if we loosened that deliberately.

4b05548f ("enums: omit trailing comma for portability", 2010-05-14),
c9b6782a ("enums: omit trailing comma for portability", 2011-03-16)

Re: [PATCH 1/8] builtin/receive-pack: use check_signature from gpg-interface

[Junio C Hamano]

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

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

Makes sense.  

When d05b9618 ("receive-pack: GPG-validate push certificates",
2014-08-14) implemented the check, there wasn't check_signature()
available.  The commit probably should have done what a4cc18f2
("verify-tag: share code with verify-commit", 2015-06-21) later did
to introduce the check_signature() function by factoring it out of
commit.c::check_commit_signature() as a preparatory step.

Will queue.  Thanks.

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

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

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> On Tue, Jul 03, 2018 at 02:38:15PM +0200, Henning Schild wrote:
>> Add "gpg.format" where the user can specify which type of signature to
>> use for commits. At the moment only "PGP" is supported and the value is
>> not even used. This commit prepares for a new types of signatures.
>
> We typically prefer to have option values specified in lower case.  I
> also think "openpgp" might be better than "PGP", since that's the name
> of the specification and it would avoid any potential unhappiness about
> compatibility with PGP or trademarks.

I do not know about trademarks, but the suggested "'openpgp' in
lowercase" sounds sensible.  Some people might even favor doing
strcasecmp() on the value; I do not have a strong opinion on that
yet.

Re: [PATCH 5/8] t/t7510: check the validation of the new config gpg.format

[Junio C Hamano]

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

> Valid values are already covered by all tests that use GPG, now also
> test what happens if we go for an invalid one.
>
> 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..cb523513f 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 gpg config for malformed values' '
> +	mv .git/config .git/config.old &&
> +	test_when_finished "mv .git/config.old .git/config" &&

Hmmmmm.  

Is the damage caused by throwing a bad value at gpg.format designed
to be so severe that "test_when_finished test_unconfig ..." cannot
recover from?  This test script is not about how "git config" is
implemented and works, so it would be a good idea for it to be even
oblivious to the fact that .git/config is the file being mucked with
when we do "git config".

I have a suspicion that you can just use test_config (which would
arrange "test_when_finished test_unconfig ..." for free).

> +	git config gpg.format malformed &&
> +	test_expect_code 128 git commit -S --amend -m "fail" 2>result &&

Is this 128 something we document and have users rely on?  Or should
we rather say

	test_must_fail git commit ...

here instead?

> +	test_i18ngrep "malformed value for gpg.format: malformed" result &&
> +	test_i18ngrep "fatal: .*\.git/config" result &&
> +	test_i18ngrep "fatal: .*line 2" result
> +'
> +
>  test_done

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

[Junio C Hamano]

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

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

Nicely explained and nicely implemented.

> diff --git a/gpg-interface.c b/gpg-interface.c
> index cd3b1b568..aa747278e 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);
>  			}

Re: [PATCH 7/8] gpg-interface: introduce new signature format "X509" using gpgsm

[Junio C Hamano]

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

> -enum gpgformats { PGP_FMT };
> +enum gpgformats { PGP_FMT, X509_FMT };
>  struct gpg_format_data gpg_formats[] = {
>  	{ .format = "PGP", .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, }

Missing SP between "{X" is a bit irritating.

Also the trailing comma (the issue is shared with the PGP side) when
the initializer is smashed on a single line feels pretty much
pointless.  If it were multi-line, then such a trailing comma would 
help future developers to add a new entry, i.e.

	 .sigs = { 
	 	PGP_SIGNATURE,
	 	PGP_MESSAGE,
	+	PGP_SOMETHING_NEW,
	 }

without touching the last existing entry.  But on a single line?

	-.sigs = { PGP_SIGNATURE, PGP_MESSAGE }
	+.sigs = { PGP_SIGNATURE, PGP_MESSAGE, PGP_SOMETHING_NEW }

is probably prettier without such a trailing comma.

> @@ -190,6 +195,9 @@ int git_gpg_config(const char *var, const char *value, void *cb)
>  	if (!strcmp(var, "gpg.program"))
>  		return git_config_string(&gpg_formats[PGP_FMT].program, var,
>  					 value);
> +	if (!strcmp(var, "gpg.programX509"))
> +		return git_config_string(&gpg_formats[X509_FMT].program, var,
> +					 value);

This is a git_config() callback, isn't it?  A two-level variable
name is given to a callback after downcasing, so nothing will match
"gpg.programX509", I suspect.  I see Brian already commented on the
name and the better organization being

 - gpg.format defines 'openpgp' or whatever other values;
 - gpg.<format>.program defines the actual program

where <format> is the value gpg.format would take
(e.g. "gpg.openpgp.program = gnupg").  And I agree with these
suggestions.

Re: [PATCH 1/8] builtin/receive-pack: use check_signature from gpg-interface

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Henning Schild <henning.schild@siemens.com> writes:
>
>> 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>
>> ---
>
> Makes sense.  
>
> When d05b9618 ("receive-pack: GPG-validate push certificates",
> 2014-08-14) implemented the check, there wasn't check_signature()
> available.  The commit probably should have done what a4cc18f2
> ("verify-tag: share code with verify-commit", 2015-06-21) later did
> to introduce the check_signature() function by factoring it out of
> commit.c::check_commit_signature() as a preparatory step.
>
> Will queue.  Thanks.

Well, I guess I won't queue this version that would waste others'
time, as you'd be rerolling to update variable names and such, so
I'd wait for that (and you in turn would wait for the names and
other discussions to settle).

Thanks anyway.

Re: [PATCH 1/8] builtin/receive-pack: use check_signature from gpg-interface

[Henning Schild]

Am Fri, 6 Jul 2018 14:35:29 -0700
schrieb Junio C Hamano <gitster@pobox.com>:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Henning Schild <henning.schild@siemens.com> writes:
> >  
> >> 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>
> >> ---  
> >
> > Makes sense.  
> >
> > When d05b9618 ("receive-pack: GPG-validate push certificates",
> > 2014-08-14) implemented the check, there wasn't check_signature()
> > available.  The commit probably should have done what a4cc18f2
> > ("verify-tag: share code with verify-commit", 2015-06-21) later did
> > to introduce the check_signature() function by factoring it out of
> > commit.c::check_commit_signature() as a preparatory step.
> >
> > Will queue.  Thanks.  
> 
> Well, I guess I won't queue this version that would waste others'
> time, as you'd be rerolling to update variable names and such, so
> I'd wait for that (and you in turn would wait for the names and
> other discussions to settle).
> 
> Thanks anyway.

I think 1 and 2 can be seen as somewhat unrelated to the gpgsm feature,
they are more general refactoring. So i think picking them is a good
idea. It will make the series shorter and ease review in the next round.

Henning

Re: [PATCH 4/8] gpg-interface: introduce an abstraction for multiple gpg formats

[Henning Schild]

Am Fri, 6 Jul 2018 10:24:58 -0700
schrieb Junio C Hamano <gitster@pobox.com>:

> Martin Ågren <martin.agren@gmail.com> writes:
> 
> >> +enum gpgformats { PGP_FMT };
> >> +struct gpg_format_data gpg_formats[] = {
> >> +       { .format = "PGP", .program = "gpg",
> >> +         .extra_args_verify = { "--keyid-format=long", },
> >> +         .sigs = { PGP_SIGNATURE, PGP_MESSAGE, },
> >> +       },
> >> +};  
> >
> > I think those trailing commas are ok now, but I'm not sure...
> >
> > I had the same thought about designated initializers. Those should
> > be ok now, c.f. cbc0f81d96 (strbuf: use designated initializers in
> > STRBUF_INIT, 2017-07-10) and a73b3680c4 (Add and use generic
> > name->id mapping code for color slot parsing, 2018-05-26).  
> 
> As you said, we dipped our toes in designated initializers in both
> struct and array, i.e. { .field = init }, { [offset] = init } last
> summer and we haven't got complaints from minor platforms so far.
> 
> The "comma" thing you are wondering is something else.  The comma we
> see above is after the last element in an array's initializer (and
> the last element in a struct's initializer), which we have been
> happily using from very early days (and they are kosher ANSI C).
> 
> What we've been avoiding was the comma after the last element in the
> enum (in other words, if PGP_FMT had ',' after it in the above
> quoted addition, that would have been violation of that rule), as
> having such a trailing comma used to be ANSI C violation as well.  I
> do not recall offhand if we loosened that deliberately.
> 
> 4b05548f ("enums: omit trailing comma for portability", 2010-05-14),
> c9b6782a ("enums: omit trailing comma for portability", 2011-03-16)

I guess that means the style is acceptable and does not require
changes, please correct me if i am wrong.

Henning

Re: [PATCH 5/8] t/t7510: check the validation of the new config gpg.format

[Henning Schild]

Am Fri, 6 Jul 2018 13:21:10 -0700
schrieb Junio C Hamano <gitster@pobox.com>:

> Henning Schild <henning.schild@siemens.com> writes:
> 
> > Valid values are already covered by all tests that use GPG, now also
> > test what happens if we go for an invalid one.
> >
> > 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..cb523513f 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 gpg config for malformed values' '
> > +	mv .git/config .git/config.old &&
> > +	test_when_finished "mv .git/config.old .git/config" &&  
> 
> Hmmmmm.  
> 
> Is the damage caused by throwing a bad value at gpg.format designed
> to be so severe that "test_when_finished test_unconfig ..." cannot
> recover from?  This test script is not about how "git config" is
> implemented and works, so it would be a good idea for it to be even
> oblivious to the fact that .git/config is the file being mucked with
> when we do "git config".
> 
> I have a suspicion that you can just use test_config (which would
> arrange "test_when_finished test_unconfig ..." for free).
> 
> > +	git config gpg.format malformed &&
> > +	test_expect_code 128 git commit -S --amend -m "fail"
> > 2>result &&  
> 
> Is this 128 something we document and have users rely on?  Or should
> we rather say
> 
> 	test_must_fail git commit ...
> 
> here instead?

This is basically an adopted copy of t1308 'check line errors for
malformed values'.

I will have a look at test_config.

Henning

> > +	test_i18ngrep "malformed value for gpg.format: malformed"
> > result &&
> > +	test_i18ngrep "fatal: .*\.git/config" result &&
> > +	test_i18ngrep "fatal: .*line 2" result
> > +'
> > +
> >  test_done  

Re: [PATCH 4/8] gpg-interface: introduce an abstraction for multiple gpg formats

[Eric Sunshine]

On Mon, Jul 9, 2018 at 4:22 AM Henning Schild
<henning.schild@siemens.com> wrote:
> Am Fri, 6 Jul 2018 10:24:58 -0700
> schrieb Junio C Hamano <gitster@pobox.com>:
> > Martin Ågren <martin.agren@gmail.com> writes:
> > >> +struct gpg_format_data gpg_formats[] = {
> > >> +       { .format = "PGP", .program = "gpg",
> > >> +         .extra_args_verify = { "--keyid-format=long", },
> > >> +         .sigs = { PGP_SIGNATURE, PGP_MESSAGE, },
> > >> +       },
> > >> +};
> > >
> > > I think those trailing commas are ok now, but I'm not sure...
> >
> > What we've been avoiding was the comma after the last element in the
> > enum (in other words, if PGP_FMT had ',' after it in the above
> > quoted addition, that would have been violation of that rule), as
> > having such a trailing comma used to be ANSI C violation as well.
>
> I guess that means the style is acceptable and does not require
> changes, please correct me if i am wrong.

The trailing comma in the 'sigs' initializer is bothersome because
'sigs' is declared as a 2-element array, and this initializer already
has two elements. Therefore, the comma is misleading to anyone reading
the code, making it appears as if additional items can be added. For
that reason, alone, it would be nice to see the unnecessary comma
removed.

Ditto with regard to the trailing comma in the 'extra_args_verify'
initializer since 'extra_args_verify' is declared as a 1-element
array.

Re: [PATCH 4/8] gpg-interface: introduce an abstraction for multiple gpg formats

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Mon, Jul 9, 2018 at 4:22 AM Henning Schild
> <henning.schild@siemens.com> wrote:
>> Am Fri, 6 Jul 2018 10:24:58 -0700
>> schrieb Junio C Hamano <gitster@pobox.com>:
>> > Martin Ågren <martin.agren@gmail.com> writes:
>> > >> +struct gpg_format_data gpg_formats[] = {
>> > >> +       { .format = "PGP", .program = "gpg",
>> > >> +         .extra_args_verify = { "--keyid-format=long", },
>> > >> +         .sigs = { PGP_SIGNATURE, PGP_MESSAGE, },
>> > >> +       },
>> > >> +};
>> > >
>> > > I think those trailing commas are ok now, but I'm not sure...
>> >
>> > What we've been avoiding was the comma after the last element in the
>> > enum (in other words, if PGP_FMT had ',' after it in the above
>> > quoted addition, that would have been violation of that rule), as
>> > having such a trailing comma used to be ANSI C violation as well.
>>
>> I guess that means the style is acceptable and does not require
>> changes, please correct me if i am wrong.
>
> The trailing comma in the 'sigs' initializer is bothersome because
> 'sigs' is declared as a 2-element array, and this initializer already
> has two elements. Therefore, the comma is misleading to anyone reading
> the code, making it appears as if additional items can be added. For
> that reason, alone, it would be nice to see the unnecessary comma
> removed.
>
> Ditto with regard to the trailing comma in the 'extra_args_verify'
> initializer since 'extra_args_verify' is declared as a 1-element
> array.

I am not sure I agree with that reasoning.  The primary benefit we
gain from the convention to allow trailing comma in struct/array
initializers like these is that we can add a new field with a patch
like this:

	 struct {
	 	char *foo;
	 	char *bar;
	+	char *baz;
	 } xyzzy = {
	 	"foo",
	 	"bar",
	+	"baz",
	 };

without having to touch the initialization of the field that used to
be at the end, i.e. .bar="bar", and just can add the new stuff at
the end.  But that is only possible as long as we allow the trailing
comma after the last element.  The same argument applies to an array
initialization (i.e. when we need to allow more elements to the
.extra_args_to_verify[] array).

All of the above is somewhat moot for the other reason I already
stated in the thread, though.  And that may be a good reason why we
would want to lose these trailing commas from the last elements
(i.e.  if it does not prepare the current code to avoid having to
touch the last line when we add a new element at the end, then
trailing comma is a visual hiccup that does not serve any useful
purpose).

Re: [PATCH 1/8] builtin/receive-pack: use check_signature from gpg-interface

[Junio C Hamano]

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

> I think 1 and 2 can be seen as somewhat unrelated to the gpgsm feature,
> they are more general refactoring. So i think picking them is a good
> idea. It will make the series shorter and ease review in the next round.

Surely, resending from patch 3 and upwards labelled as "add support
for gpgsm", saying that the topic depends on a different topic
branch named $X in my tree (after $X actually gets pushed out,
preferrably as part of 'next'---which is a promise that the changes
won't see any more drastic rewrites), is a good approach.

Thanks.

Re: [PATCH 4/8] gpg-interface: introduce an abstraction for multiple gpg formats

[Jeff King]

On Fri, Jul 06, 2018 at 10:24:58AM -0700, Junio C Hamano wrote:

> What we've been avoiding was the comma after the last element in the
> enum (in other words, if PGP_FMT had ',' after it in the above
> quoted addition, that would have been violation of that rule), as
> having such a trailing comma used to be ANSI C violation as well.  I
> do not recall offhand if we loosened that deliberately.
> 
> 4b05548f ("enums: omit trailing comma for portability", 2010-05-14),
> c9b6782a ("enums: omit trailing comma for portability", 2011-03-16)

I think we accidentally did a weather-balloon in e1327023ea (grep:
refactor the concept of "grep source" into an object, 2012-02-02).
It's still there and nobody has complained about it yet.

So I think we can consider that requirement loosened at this point.

-Peff

Re: [PATCH 4/8] gpg-interface: introduce an abstraction for multiple gpg formats

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Jul 06, 2018 at 10:24:58AM -0700, Junio C Hamano wrote:
>
>> What we've been avoiding was the comma after the last element in the
>> enum (in other words, if PGP_FMT had ',' after it in the above
>> quoted addition, that would have been violation of that rule), as
>> having such a trailing comma used to be ANSI C violation as well.  I
>> do not recall offhand if we loosened that deliberately.
>> 
>> 4b05548f ("enums: omit trailing comma for portability", 2010-05-14),
>> c9b6782a ("enums: omit trailing comma for portability", 2011-03-16)
>
> I think we accidentally did a weather-balloon in e1327023ea (grep:
> refactor the concept of "grep source" into an object, 2012-02-02).
> It's still there and nobody has complained about it yet.
>
> So I think we can consider that requirement loosened at this point.

#leftoverbits: update CodingGuidelines and refer to these three
commits.

Takers?

 

Re: [PATCH 4/8] gpg-interface: introduce an abstraction for multiple gpg formats

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Jul 06, 2018 at 10:24:58AM -0700, Junio C Hamano wrote:
>
>> What we've been avoiding was the comma after the last element in the
>> enum (in other words, if PGP_FMT had ',' after it in the above
>> quoted addition, that would have been violation of that rule), as
>> having such a trailing comma used to be ANSI C violation as well.  I
>> do not recall offhand if we loosened that deliberately.
>> 
>> 4b05548f ("enums: omit trailing comma for portability", 2010-05-14),
>> c9b6782a ("enums: omit trailing comma for portability", 2011-03-16)
>
> I think we accidentally did a weather-balloon in e1327023ea (grep:
> refactor the concept of "grep source" into an object, 2012-02-02).
> It's still there and nobody has complained about it yet.
>
> So I think we can consider that requirement loosened at this point.
>
> -Peff

Yup, thanks for digging it out.  It seems that I did the same
digging some time ago but forgot about it.

Re: [PATCH 4/8] gpg-interface: introduce an abstraction for multiple gpg formats

[Jeff King]

On Tue, Jul 10, 2018 at 08:58:32AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > On Fri, Jul 06, 2018 at 10:24:58AM -0700, Junio C Hamano wrote:
> >
> >> What we've been avoiding was the comma after the last element in the
> >> enum (in other words, if PGP_FMT had ',' after it in the above
> >> quoted addition, that would have been violation of that rule), as
> >> having such a trailing comma used to be ANSI C violation as well.  I
> >> do not recall offhand if we loosened that deliberately.
> >> 
> >> 4b05548f ("enums: omit trailing comma for portability", 2010-05-14),
> >> c9b6782a ("enums: omit trailing comma for portability", 2011-03-16)
> >
> > I think we accidentally did a weather-balloon in e1327023ea (grep:
> > refactor the concept of "grep source" into an object, 2012-02-02).
> > It's still there and nobody has complained about it yet.
> >
> > So I think we can consider that requirement loosened at this point.
> >
> > -Peff
> 
> Yup, thanks for digging it out.  It seems that I did the same
> digging some time ago but forgot about it.

I actually cheated and just searched for "enum, comma" in my list
archive and found your message. ;)

-Peff