X509 (gpgsm) commit signing support

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

---
 gpg-interface.c | 2 +-
 gpg-interface.h | 2 --
 2 files changed, 1 insertion(+), 3 deletions(-)

gpg-interface.c +1 -1

index 0647bd634..09ddfbc26 100644
@@ -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;

gpg-interface.h +0 -2

index a5e6517ae..5ecff4aa0 100644
@@ -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

Quoted 2 lines · Henning Schild

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.

Quoted 34 lines

Signed-off-by Henning Schild <[email protected]>

---
 gpg-interface.c | 2 +-
 gpg-interface.h | 2 --
 2 files changed, 1 insertion(+), 3 deletions(-)

gpg-interface.c +1 -1

index 0647bd634..09ddfbc26 100644
@@ -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;

gpg-interface.h +0 -2

index a5e6517ae..5ecff4aa0 100644
@@ -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

Quoted 9 lines

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

index 6e2015ed9..7e1e9caf4 100755
@@ -227,4 +227,14 @@ test_expect_success GPG 'log.showsignature

behaves like --show-signature' ' grep "gpg: Good signature" actual
'

t/t7510-signed-commit.sh test_expect_success GPG 'log.showsignature behaves like --show-signature' ' · line 233

Quoted 4 lines

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

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

t/t7510-signed-commit.sh test_expect_success GPG 'log.showsignature behaves like --show-signature' ' · line 235

Quoted 2 lines

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

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

t/t7510-signed-commit.sh test_expect_success GPG 'log.showsignature behaves like --show-signature' ' · line 238

Quoted 3 lines

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

+	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

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

Signed-off-by Henning Schild <[email protected]>

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

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

index 6e2015ed9..7e1e9caf4 100755

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

+	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

Quoted 15 lines · Henning Schild

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

Signed-off-by Henning Schild <[email protected]>

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

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

index 6e2015ed9..7e1e9caf4 100755
@@ -227,4 +227,14 @@ test_expect_success GPG 'log.showsignature

behaves like --show-signature' ' grep "gpg: Good signature" actual
'

t/t7510-signed-commit.sh test_expect_success GPG 'log.showsignature behaves like --show-signature' ' · line 231

Quoted 2 lines

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

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

Quoted 9 lines

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

+	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  

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.

+enum gpgformats { PGP_FMT };

Quoted 5 lines

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.

gpg-interface.c line 15

Quoted 9 lines

static char *configured_signing_key;

gpg-interface.c +6 -2

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

gpg-interface.c line 26

Quoted 6 lines

gpg-interface.c +6 -0

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

Quoted 1 line

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

gpg-interface.c line 26

Quoted 6 lines

gpg-interface.c +6 -0

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

gpg-interface.c line 36

Quoted 8 lines

gpg-interface.c +8 -0

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

gpg-interface.c line 36

Quoted 13 lines

+static struct gpg_format_data *get_format_data_by_sig(const char *sig) +{

gpg-interface.c +3 -0

+	int i, j;
+	for (i = 0; i < ARRAY_SIZE(gpg_formats); i++)
+		for (j = 0; j < ARRAY_SIZE(gpg_formats[i].sigs);

j++)

gpg-interface.c +3 -0

+			if (gpg_formats[i].sigs[j] && 
+			    !strncmp(gpg_formats[i].sigs[j], sig,
+

strlen(gpg_formats[i].sigs[j])))

gpg-interface.c +3 -0

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

gpg-interface.c void print_signature_buffer(const struct signature_check *sigc, unsigned flags) · line 142

Quoted 6 lines

static int is_gpg_start(const char *line)
{

gpg-interface.c +1 -2

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

gpg-interface.c int git_gpg_config(const char *var, const · line 179

Quoted 9 lines

gpg-interface.c +1 -1

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

gpg-interface.c const char *get_signing_key(void) · line 204

Quoted 12 lines

gpg-interface.c +4 -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);  

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

gpg-interface.c int verify_signed_buffer(const char *payload, · line 261

Quoted 6 lines

gpg-interface.c +2 -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.

gpg-interface.c int verify_signed_buffer(const char *payload, size_t payload_size, · line 264

Quoted 2 lines

gpg-interface.c +2 -0

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

gpg-interface.c int verify_signed_buffer(const char *payload, size_t payload_size,

Quoted 5 lines

gpg-interface.c +4 -0

+	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

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

---
 gpg-interface.c | 74 ++++++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 58 insertions(+), 16 deletions(-)

gpg-interface.c +0 -0

index ed0e55917..0a8d1bff3 100644
@@ -7,12 +7,46 @@
 #include "tempfile.h"

gpg-interface.c +20 -0

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

gpg-interface.c +7 -2

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

gpg-interface.c +0 -2

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

Quoted 18 lines · 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 <[email protected]>

---
 gpg-interface.c | 74

++++++++++++++++++++++++++++++++++++++++++++------------- 1 file changed, 58 insertions(+), 16 deletions(-)

gpg-interface.c +0 -0

index ed0e55917..0a8d1bff3 100644
@@ -7,12 +7,46 @@
 #include "tempfile.h"
 

gpg-interface.c line 15

Quoted 9 lines

gpg-interface.c +6 -2

 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.

gpg-interface.c line 29

Quoted 13 lines

gpg-interface.c +10 -0

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

Quoted 26 lines

gpg-interface.c +19 -0

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

gpg-interface.c void print_signature_buffer(const struct · line 141

Quoted 6 lines

gpg-interface.c +1 -2

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

gpg-interface.c int git_gpg_config(const char *var, const · line 178

Quoted 9 lines

gpg-interface.c +1 -1

 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.

gpg-interface.c int git_gpg_config(const char *var, const char *value, void *cb) · line 183

Quoted 13 lines

gpg-interface.c +3 -7

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

Quoted 33 lines

gpg-interface.c +5 -1

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

gpg-interface.c +2 -1

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

gpg-interface.c int verify_signed_buffer(const char *payload, · line 269

Quoted 10 lines

gpg-interface.c +9 -0

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

Quoted 6 lines

gpg-interface.c +0 -2

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

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.

Quoted 5 lines

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.

gpg-interface.c static void parse_gpg_output(struct · line 93

Quoted 12 lines

gpg-interface.c +2 -1

index 0a8d1bff3..ac2df498d 100644
@@ -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...

gpg-interface.c static void parse_gpg_output(struct signature_check *sigc) · line 96

Quoted 5 lines

gpg-interface.c +1 -1

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

Quoted 8 lines

In the general case you need:

  found = *next ? next + 1 : next;

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

  found = next;

Quoted 7 lines

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.

Quoted 17 lines

Quoted 9 lines

In the general case you need:

  found = *next ? next + 1 : next;

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

  found = next;

Quoted 7 lines

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

Quoted 7 lines

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

Quoted 4 lines

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.

Quoted 11 lines

Quoted 5 lines

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

---
 gpg-interface.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

gpg-interface.c +0 -0

index bf8d567a4c..139b0f561e 100644

gpg-interface.c +0 -1

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

gpg-interface.c static void parse_gpg_output(struct signature_check · line 100

Quoted 10 lines

gpg-interface.c +1 -1

index bf8d567a4c..139b0f561e 100644
@@ -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

gpg-interface.c static void parse_gpg_output(struct signature_check · line 101

Quoted 1 line

gpg-interface.c +0 -0

 if (*next && sigc-> result != 'E')

also skipping the following strchrnul and xmemdupz

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.

Quoted 10 lines · Jeff King

Should we allow:

[gpg "OpenPGP"]
program = whatever

given that we allow:

[gpg]
format = OpenPGP

Quoted 3 lines

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

Quoted 16 lines · Jeff King

Quoted 13 lines · Jeff King

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.

Quoted 4 lines

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.

Quoted 20 lines · brian m. carlson

Quoted 19 lines · Jeff King

Quoted 5 lines · Jeff King

Should we allow:

[gpg "OpenPGP"]
program = whatever

Quoted 8 lines

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

Quoted 4 lines

Quoted 1 line

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

Quoted 5 lines

Quoted 2 lines

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

Quoted 9 lines · Junio C Hamano

Quoted 5 lines

Quoted 2 lines

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

Should we allow:

[gpg "OpenPGP"]
program = whatever

given that we allow:

[gpg]
format = OpenPGP

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

---
 Documentation/config.txt | 5 +++++
 gpg-interface.c          | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)

Documentation/config.txt +0 -0

index ac373e3f4..c0bd80954 100644
@@ -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.

Documentation/config.txt +2 -0

+gpg.<format>.program::
+	Use this to customize the program used for the signing

format you

Documentation/config.txt +1 -0

+	chose. (see gpg.program) gpg.openpgp.program is a synonym

for the

Documentation/config.txt +2 -0

+	legacy gpg.program.
+
 gui.commitMsgWidth::
 	Defines how wide the commit message window is in the
 	linkgit:git-gui[1]. "75" is the default.

gpg-interface.c +1 -1

index ac2df498d..65098430f 100644
@@ -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;

gpg-interface.c +1 -1

index 65098430f..bf8d567a4 100644
@@ -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 };

Quoted 15 lines

gpg-interface.c +4 -1

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

gpg-interface.c struct gpg_format_data { · line 30

Quoted 2 lines

gpg-interface.c +2 -0

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

Quoted 5 lines

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:

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

Quoted 7 lines

Documentation/config.txt +0 -0

index c0bd80954..b6f9b47d5 100644
@@ -1830,7 +1830,7 @@ gpg.program::
 

Quoted 7 lines

Documentation/config.txt +0 -0

 gpg.format::
 	Specifies which key format to use when signing with

--gpg-sign.

Documentation/config.txt +0 -1

-	Default is "openpgp", that is also the only supported

value.

Documentation/config.txt +1 -0

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

Quoted 11 lines

gpg-interface.c +1 -1

index 65098430f..bf8d567a4 100644
@@ -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 };

gpg-interface.c struct gpg_format_data { · line 30

Quoted 10 lines

gpg-interface.c +5 -0

+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

Quoted 5 lines

t/lib-gpg.sh +5 -0

+		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 [email protected] -o /dev/null --sign - 2>&1 &&

t/lib-gpg.sh then · line 48

Quoted 1 line

t/lib-gpg.sh +1 -0

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

Quoted 11 lines

t/lib-gpg.sh +0 -0

index a5d3b2cba..9dcb4e990 100755
@@ -38,7 +38,14 @@ then
 			"$TEST_DIRECTORY"/lib-gpg/ownertrust &&
 		gpg --homedir "${GNUPGHOME}" </dev/null >/dev/null

2>&1 \ --sign -u [email protected] &&

t/lib-gpg.sh +1 -1

-		test_set_prereq GPG
+		test_set_prereq GPG &&

Quoted 4 lines

t/lib-gpg.sh +1 -0

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

Quoted 9 lines

t/lib-gpg.sh +1 -0

+		gpgsm --homedir "${GNUPGHOME}" --import

"$TEST_DIRECTORY"/lib-gpg/gpgsm.crt.user &&

t/lib-gpg.sh +1 -0

+		gpgsm --homedir "${GNUPGHOME}" -K | grep

fingerprint: | cut -d" " -f4 | tr -d '\n' >
${GNUPGHOME}/trustlist.txt &&

t/lib-gpg.sh +3 -0

+		echo " S relax" >> ${GNUPGHOME}/trustlist.txt &&
+		(gpgconf --kill gpg-agent >/dev/null 2>&1 || : ) &&
+		echo hello | gpgsm --homedir "${GNUPGHOME}" -u

[email protected] -o /dev/null --sign - 2>&1 &&

t/lib-gpg.sh then · line 48

Quoted 1 line

t/lib-gpg.sh +1 -0

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

t/lib-gpg.sh

Quoted 11 lines

test_expect_success GPG 'log --graph --show-signature' '

t/lib-gpg.sh +5 -0

 	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

Quoted 2 lines

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.

Quoted 8 lines

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?

Quoted 10 lines

Quoted 3 lines

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

Quoted 11 lines

Quoted 8 lines

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

Quoted 5 lines

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.

Quoted 10 lines

Quoted 6 lines

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.

Quoted 10 lines

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

Quoted 5 lines

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.

Am Tue, 10 Jul 2018 13:09:01 -0400
schrieb Jeff King <[email protected]>:

Quoted 12 lines · Henning Schild

Quoted 11 lines

t/lib-gpg.sh +0 -0

index a5d3b2cba..9dcb4e990 100755
@@ -38,7 +38,14 @@ then
 			"$TEST_DIRECTORY"/lib-gpg/ownertrust &&
 		gpg --homedir "${GNUPGHOME}" </dev/null >/dev/null

2>&1 \ --sign -u [email protected] &&

t/lib-gpg.sh +1 -1

-		test_set_prereq GPG
+		test_set_prereq GPG &&

Quoted 8 lines

Quoted 4 lines

t/lib-gpg.sh +1 -0

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

Quoted 2 lines

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.

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 [email protected] and configure gpgsm to trust it.

Signed-off-by Henning Schild <[email protected]>

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

Quoted 13 lines · 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 [email protected] and configure gpgsm to trust it.

Signed-off-by Henning Schild <[email protected]>

---
 t/lib-gpg.sh               |  9 ++++++-
 t/lib-gpg/gpgsm-gen-key.in |  6 +++++
 t/t4202-log.sh             | 66

Quoted 7 lines

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

t/lib-gpg.sh +0 -0

index a5d3b2cba..9dcb4e990 100755
@@ -38,7 +38,14 @@ then
 			"$TEST_DIRECTORY"/lib-gpg/ownertrust &&
 		gpg --homedir "${GNUPGHOME}" </dev/null >/dev/null 2>&1 \

Quoted 24 lines

t/lib-gpg.sh +0 -0

index a5d3b2cba..9dcb4e990 100755
@@ -38,7 +38,14 @@ then
 			"$TEST_DIRECTORY"/lib-gpg/ownertrust &&
 		gpg --homedir "${GNUPGHOME}" </dev/null >/dev/null

2>&1 \ --sign -u [email protected] &&

t/lib-gpg.sh +2 -1

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

t/lib-gpg.sh +1 -0

+		gpgsm --homedir "${GNUPGHOME}" --import

"$TEST_DIRECTORY"/lib-gpg/gpgsm.crt.user &&

t/lib-gpg.sh +1 -0

+		gpgsm --homedir "${GNUPGHOME}" -K | grep

fingerprint: | cut -d" " -f4 | tr -d '\n' >
${GNUPGHOME}/trustlist.txt &&

t/lib-gpg.sh +3 -0

+		echo " S relax" >> ${GNUPGHOME}/trustlist.txt &&
+		(gpgconf --kill gpg-agent >/dev/null 2>&1 || : ) &&
+		echo hello | gpgsm --homedir "${GNUPGHOME}" -u

[email protected] -o /dev/null --sign - 2>&1 &&

t/lib-gpg.sh then · line 48

Quoted 1 line

t/lib-gpg.sh +1 -0

+		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

This series adds support for signing commits with gpgsm.

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.

Quoted 7 lines · Henning Schild

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.

Quoted 6 lines

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.