X509 (gpgsm) commit signing support

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

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

---
 Documentation/config.txt | 4 ++++
 gpg-interface.c          | 7 +++++++
 2 files changed, 11 insertions(+)

Documentation/config.txt +4 -0

index 1cc18a828..ac373e3f4 100644
@@ -1828,6 +1828,10 @@ gpg.program::
 	signed, and the program is expected to send the result to its
 	standard output.
 
+gpg.format::
+	Specifies which key format to use when signing with `--gpg-sign`.
+	Default is "openpgp", that is also the only supported value.
+
 gui.commitMsgWidth::
 	Defines how wide the commit message window is in the
 	linkgit:git-gui[1]. "75" is the default.

gpg-interface.c +1 -0

index 09ddfbc26..960c40086 100644
@@ -7,6 +7,7 @@
 #include "tempfile.h"
 
 static char *configured_signing_key;
+static const char *gpg_format = "openpgp";
 static const char *gpg_program = "gpg";
 
 #define PGP_SIGNATURE "-----BEGIN PGP SIGNATURE-----"

gpg-interface.c +2 -0

+	}
+
 	if (!strcmp(var, "gpg.program")) {
 		if (!value)
 			return config_error_nonbool(var);

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

Quoted 9 lines

#define PGP_SIGNATURE "-----BEGIN PGP SIGNATURE-----"

gpg-interface.c +4 -0

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

I may be mistaken (sorry, I read so many topics X-<) but I think the consensus was to accept only "openpgp" so that we can ensure that

[gpg "openpgp"] program = foo

etc. can be parsed more naturally without having to manually special case the subsection name to be case insensitive. In other words, strcasecmp() should just be strcmp(). Otherwise, people would get a wrong expectation that

[gpg] format = OpenPGP
[gpg "openpgp"] program = foo

should refer to the same and single OpenPGP, but that would violate the usual configuration syntax rules.

The structure of checking the error looks quite suboptimal even when we initially support the single "openpgp" at this step. I would have expected you to be writing the above like so:

	if (!value)
		return config_error_nonbool(var);
	if (strcmp(value, "openpgp"))
		return error("unsupported value for %s: %s", var, value);
	return git_config_string(...);

That would make it more clear that the variable is "nonbool", which does not change across additional support for new formats in later steps.

Quoted 40 lines · Henning Schild

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

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

---
 Documentation/config.txt | 4 ++++
 gpg-interface.c          | 7 +++++++
 2 files changed, 11 insertions(+)

Documentation/config.txt +0 -0

index 1cc18a828..ac373e3f4 100644
@@ -1828,6 +1828,10 @@ gpg.program::
 	signed, and the program is expected to send the result to

its standard output.

Documentation/config.txt +2 -0

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

--gpg-sign.

Documentation/config.txt +1 -0

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

value. +
gui.commitMsgWidth::

Documentation/config.txt +0 -0

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

gpg-interface.c +1 -0

index 09ddfbc26..960c40086 100644
@@ -7,6 +7,7 @@
 #include "tempfile.h"
 
 static char *configured_signing_key;
+static const char *gpg_format = "openpgp";
 static const char *gpg_program = "gpg";
 
 #define PGP_SIGNATURE "-----BEGIN PGP SIGNATURE-----"

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

Quoted 9 lines

gpg-interface.c +4 -0

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

I may be mistaken (sorry, I read so many topics X-<) but I think the consensus was to accept only "openpgp" so that we can ensure that

[gpg "openpgp"] program = foo

etc. can be parsed more naturally without having to manually special case the subsection name to be case insensitive. In other words, strcasecmp() should just be strcmp(). Otherwise, people would get a wrong expectation that

[gpg] format = OpenPGP
[gpg "openpgp"] program = foo

should refer to the same and single OpenPGP, but that would violate the usual configuration syntax rules.

The structure of checking the error looks quite suboptimal even when we initially support the single "openpgp" at this step. I would have expected you to be writing the above like so:

	if (!value)
		return config_error_nonbool(var);
	if (strcmp(value, "openpgp"))
		return error("unsupported value for %s: %s", var,

value); return git_config_string(...);

That would make it more clear that the variable is "nonbool", which does not change across additional support for new formats in later steps.

Quoted 5 lines

gpg-interface.c +2 -0

+	}
+
 	if (!strcmp(var, "gpg.program")) {

gpg-interface.c +0 -0

 		if (!value)
 			return config_error_nonbool(var);  

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

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

---
 Documentation/config.txt | 4 ++++
 gpg-interface.c          | 7 +++++++
 2 files changed, 11 insertions(+)

Documentation/config.txt +0 -0

index 1cc18a828..ac373e3f4 100644

Documentation/config.txt gpg.program:: · line 1833

Quoted 3 lines

Documentation/config.txt +3 -0

+gpg.format::
+	Specifies which key format to use when signing with `--gpg-sign`.
+	Default is "openpgp", that is also the only supported value.

I think, as discussed in the other thread, perhaps a different prefix for these options is in order if we'd like to plan for the future. Maybe this could be "signature.format", "sign.format", "signing.format", or "signingtool.format" (I tend to prefer the former, but not too strongly).

I anticipate that some projects will prefer other formats, and it makes it easier if we don't have to maintain two sets of legacy names.

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

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

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

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

index 6e2015ed9..1b6a1dd90 100755
@@ -227,4 +227,13 @@ test_expect_success GPG 'log.showsignature behaves like --show-signature' '
 	grep "gpg: Good signature" actual
 '
 

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 | 84 ++++++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 63 insertions(+), 21 deletions(-)

gpg-interface.c +0 -0

index 960c40086..699651fd9 100644
@@ -7,11 +7,46 @@
 #include "tempfile.h"
 
 static char *configured_signing_key;

gpg-interface.c +3 -2

+			return gpg_formats + i;
+	return NULL;
+}
 
-#define PGP_SIGNATURE "-----BEGIN PGP SIGNATURE-----"
-#define PGP_MESSAGE "-----BEGIN PGP MESSAGE-----"

gpg-interface.c +0 -2

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

gpg-interface.c +5 -0

+	if (fmtname) {
+		fmt = get_format_by_name(fmtname);
+		return git_config_string(&fmt->program, var, value);
+	}
+
 	return 0;
 }

Quoted 20 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 | 84

++++++++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 63 insertions(+), 21 deletions(-)

gpg-interface.c +0 -1

index 960c40086..699651fd9 100644
@@ -7,11 +7,46 @@
 #include "tempfile.h"
 
 static char *configured_signing_key;
-static const char *gpg_format = "openpgp";

gpg-interface.c line 18

Quoted 10 lines

gpg-interface.c +8 -1

-static const char *gpg_program = "gpg";
+struct gpg_format {
+	const char *name;
+	const char *program;
+	const char **extra_args_verify;
+	const char **sigs;
+};
+
+static const char *openpgp_verify_args[] =
 { "--keyid-format=long", NULL };  

Let's write it like this, even if the current line is short enough:

static const char *openpgp_verify_args[] = {
	"--keyid-format=long",
	NULL
};

Ditto for the next one. Even if you do not expect these two arrays to get longer, people tend to copy & paste to imitate existing code when making new things, and we definitely would not want them to be doing so on a code like the above (or below). When they need to add new elements to their arrays, having the terminating NULL on its own line means they will have to see less patch noise.

gpg-interface.c line 24

Quoted 7 lines

gpg-interface.c +7 -0

+static const char *openpgp_sigs[] = {
+	"-----BEGIN PGP SIGNATURE-----",
+	"-----BEGIN PGP MESSAGE-----", NULL };
+
+static struct gpg_format gpg_formats[] = {
+	{ .name = "openpgp", .program = "gpg",
+	  .extra_args_verify = openpgp_verify_args,  

If the underlying aray is "verify_args" (not "extra"), perhaps the field name should also be .verify_args to match.

Giving an array of things a name "things[]" is a disease, unless the array is very often passed around as a whole to represent the collection of everything. When the array is mostly accessed one element at a time, being able to say thing[3] to mean the third thing is much more pleasant.

So, from that point of view, I pretty much agree with openpgp_verify_args[] and openpgp_sigs[], but am against gpg_formats[]. The last one should be gpg_format[], for which we happen to have only one variant right now.

gpg-interface.c line 28

Quoted 4 lines

gpg-interface.c +4 -0

+	  .sigs = openpgp_sigs
+	},
+};
+static struct gpg_format *current_format = &gpg_formats[0];  

Have a blank line before the above one.

What does "current_format" mean? Is it different from "format to be used"? As we will overwrite the variable upon reading the config, we cannot afford to call it default_format, but somehow "current" feels misleading, at least to me. I expected to find "old format" elsewhere and there may be some format conversion or something from the variable name.

    static struct gpg_format *use_format = &gpg_format[0];

perhaps?

gpg-interface.c line 35

Quoted 7 lines

gpg-interface.c +7 -0

+
+static struct gpg_format *get_format_by_name(const char *str)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(gpg_formats); i++)
+		if (!strcasecmp(gpg_formats[i].name, str))  

As [1/7], this would become strcmp(), I presume?

Quoted 7 lines

gpg-interface.c +4 -2

+			return gpg_formats + i;
+	return NULL;
+}
 
-#define PGP_SIGNATURE "-----BEGIN PGP SIGNATURE-----"
-#define PGP_MESSAGE "-----BEGIN PGP MESSAGE-----"
+static struct gpg_format *get_format_by_sig(const char *sig)

gpg-interface.c line 49

Quoted 10 lines

gpg-interface.c +9 -0

+{
+	int i, j;
+
+	for (i = 0; i < ARRAY_SIZE(gpg_formats); i++)
+		for (j = 0; gpg_formats[i].sigs[j]; j++)
+			if (starts_with(sig,
 gpg_formats[i].sigs[j]))
+				return gpg_formats + i;
+	return NULL;
+}  

OK.

Quoted 14 lines

gpg-interface.c +0 -7

@@ -140,18 +172,23 @@ int git_gpg_config(const char *var, const
 char *value, void *cb) }
 
 	if (!strcmp(var, "gpg.format")) {
-		if (value && strcasecmp(value, "openpgp"))
-			return error("malformed value for %s: %s",
 var, value);
-		return git_config_string(&gpg_format, var, value);
-	}
-
-	if (!strcmp(var, "gpg.program")) {
 		if (!value)
 			return config_error_nonbool(var);
-		gpg_program = xstrdup(value);

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

Quoted 4 lines

gpg-interface.c +3 -0

+		fmt = get_format_by_name(value);
+		if (!fmt)
+			return error("malformed value for %s: %s",
 var, value);  

If I say "opongpg", that probably is not "malformed" (it is all lowercase ascii alphabet that is very plausible-looking string), but simply "bad value".

But other than the minor nit, yes, this structure is what I expected to see from the very first step 1/7.

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

Quoted 7 lines

gpg-interface.c +3 -0

+		current_format = fmt;
 		return 0;
 	}  
 
 
+	if (!strcmp(var, "gpg.program"))
+		fmtname = "openpgp";  

OK, this is a backward compatibility measure to treat gpg.program as if it were gpg.openpgp.program, which makes sense.

Quoted 8 lines

gpg-interface.c +3 -0

+	if (fmtname) {
+		fmt = get_format_by_name(fmtname);
+		return git_config_string(&fmt->program, var,

value);

gpg-interface.c +2 -0

+	}
+
 	return 0;
 }  

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.
Input that does not match the expected format will be ignored now, before we jumped to found+17 which might have been behind the end of an unexpected string.

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

---
 gpg-interface.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

gpg-interface.c +4 -3

index 699651fd9..93bd0fb32 100644
@@ -89,10 +89,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;
+			if (*next && sigc-> result != 'E') {
+				found = next + 1;
 				next = strchrnul(found, '\n');
 				sigc->signer = xmemdupz(found, next - found);
 			}

gui.commitMsgWidth::

Documentation/config.txt +0 -0

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

gpg-interface.c +1 -1

index 93bd0fb32..f3c22b551 100644
@@ -182,7 +182,7 @@ int git_gpg_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 
-	if (!strcmp(var, "gpg.program"))
+	if (!strcmp(var, "gpg.program") || !strcmp(var, "gpg.openpgp.program"))
 		fmtname = "openpgp";
 
 	if (fmtname) {

Documentation/config.txt gpg.format:: · line 1838

Quoted 6 lines · Henning Schild

(quoted diff) +2 -0

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

format you

(quoted diff) +1 -0

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

for the

Documentation/config.txt +1 -0

+	legacy gpg.program.  

I _think_ you meant "see gpg.format", but I am not 100% sure.

When X is a synonym for Y, Y is also a synonym for X, so technically speaking this does not matter, but when we talk about backward compatibility fallback, I think we say "OLDway is retained as a legacy synonym for NEWway", i.e. the other way around.

Also, typeset in tt what end-users would type literally, like configuration variable names, i.e.

Use this to customize the rpogram used for the signing format you chose (see gpg.format). gpg.program can still be used as a legacy synonym for gpg.openpgp.program.

Quoted 16 lines

gui.commitMsgWidth::

Documentation/config.txt +0 -0

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

gpg-interface.c +1 -1

index 93bd0fb32..f3c22b551 100644
@@ -182,7 +182,7 @@ int git_gpg_config(const char *var, const char
 *value, void *cb) return 0;
 	}
 
-	if (!strcmp(var, "gpg.program"))
+	if (!strcmp(var, "gpg.program") || !strcmp(var,
 "gpg.openpgp.program")) fmtname = "openpgp";
 
 	if (fmtname) {