[RFC PATCH 0/2] cryptoSign flag & config

[RFC PATCH 0/2] cryptoSign flag & config

[Fabian Stelzer]

Since git now supports multiple methods for signing objects some of the 
existing flags and configuration variables can be misleading. Especially the 
flags could be understood as selecting a format (--gpg-sign), which they do 
not.

This series introduces the more generic name "cryptoSign". Using just "sign" 
could otherwise be too easily confused with "sign-off".

For now I have only adjusted the gpg-sign flag to a single command and added 
the new configuration prefix and would like to hear some feedback. If we can 
agree on the naming and implementation I will of course adjust all the other 
commands likewise and add some tests for the compatibility layer.
The `(commit|tag|push).gpgsign` var is still on my todo list as well. 

My earlier question to the list (<xmqqzgpn50l6.fsf@gitster.g>) did not 
receive much feedback so i'm sending this rfc patch.

Fabian Stelzer (2):
  crypto sign: add crypto-sign alias flag
  crypto sign: add cryptoSign.* config

 Documentation/config/gpg.txt | 31 ++++++++++++++++++++-----------
 Documentation/git-commit.txt | 15 ++++++++++-----
 builtin/commit.c             |  5 ++++-
 gpg-interface.c              | 30 ++++++++++++++++++++++--------
 4 files changed, 56 insertions(+), 25 deletions(-)

-- 
2.33.1

[RFC PATCH 1/2] crypto sign: add crypto-sign alias flag

[Fabian Stelzer]

Multiple commands allow passing `--gpg-sign` or `--no-gpg-sign` to
enable or disable object signing. Since git can now use other methods
for signing this flag could suggest that it selects `gpg` as the method
to use, which it does not.
Since just `--sign` would conflict with `--signoff` too easily we choose
`--crypto-sign` as a more general name.

Add the new flag to all affected commands as an alias to gpg-sign.
Move the `-S` shorthand to the new flag to indicate that this is the
recommended one to use.
Update the documentation to match.

This affects the commands: am, commit-tree, commit, merge, rebase and
revert.
---
 Documentation/git-commit.txt | 15 ++++++++++-----
 builtin/commit.c             |  5 ++++-
 2 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index 6c60bf98f9..b2c1d8bdb9 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -387,13 +387,18 @@ changes to tracked files.
 	default commit message.
 
 -S[<keyid>]::
+--crypto-sign[=<keyid>]::
+--no-crypto-sign::
 --gpg-sign[=<keyid>]::
 --no-gpg-sign::
-	GPG-sign commits. The `keyid` argument is optional and
-	defaults to the committer identity; if specified, it must be
-	stuck to the option without a space. `--no-gpg-sign` is useful to
-	countermand both `commit.gpgSign` configuration variable, and
-	earlier `--gpg-sign`.
+	Cryptographically sign commits. The `keyid` argument is optional and
+	its default depends on the configured `cryptoSign.format`; if specified,
+	it must be stuck to the option without a space. `--no-crypto-sign` is
+	useful to countermand both `commit.gpgSign` configuration variable, and
+	earlier `--crypto-sign`.
+	`--(no-)gpg-sign` is a compatibility alias and has no effect on which
+	cryptographic format will be used. This is determined by the
+	configuration variable cryptoSign.format (see linkgit:git-config[1]).
 
 \--::
 	Do not interpret any more arguments as options.
diff --git a/builtin/commit.c b/builtin/commit.c
index 883c16256c..2c789ff6f9 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1639,8 +1639,11 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		OPT_BOOL('e', "edit", &edit_flag, N_("force edit of commit")),
 		OPT_CLEANUP(&cleanup_arg),
 		OPT_BOOL(0, "status", &include_status, N_("include status in commit message template")),
-		{ OPTION_STRING, 'S', "gpg-sign", &sign_commit, N_("key-id"),
+		{ OPTION_STRING, 'S', "crypto-sign", &sign_commit, N_("key-id"),
+		  N_("cryptographically sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
+		{ OPTION_STRING, 0, "gpg-sign", &sign_commit, N_("key-id"),
 		  N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
+
 		/* end commit message options */
 
 		OPT_GROUP(N_("Commit contents options")),
-- 
2.33.1

[RFC PATCH 2/2] crypto sign: add cryptoSign.* config

[Fabian Stelzer]

Since git now supports multiple cryptographic methods/formats to sign
objects, the `gpg.` configuration prefix is misleading.
Add `cryptoSign.`, but keep `gpg.` as a compatibility alias at least for
all existing options.
`gpg.mintrustlevel` is moved to `cryptosign.gpg.mintrustlevel` while
also still allowing the former.
---
 Documentation/config/gpg.txt | 31 ++++++++++++++++++++-----------
 gpg-interface.c              | 30 ++++++++++++++++++++++--------
 2 files changed, 42 insertions(+), 19 deletions(-)

diff --git a/Documentation/config/gpg.txt b/Documentation/config/gpg.txt
index 4f30c7dbdd..ef21eb8249 100644
--- a/Documentation/config/gpg.txt
+++ b/Documentation/config/gpg.txt
@@ -1,6 +1,17 @@
 gpg.program::
-	Use this custom program instead of "`gpg`" found on `$PATH` when
-	making or verifying a PGP signature. The program must support the
+	Deprecated alias for `cryptoSign.<format>.program`.
+
+cryptoSign.format::
+gpg.format::
+	Specifies which key format to use when signing with `--crypto-sign`.
+	Default is "openpgp". Other possible values are "x509", "ssh".
+
+cryptoSign.<format>.program::
+gpg.<format>.program::
+	Use this to customize the program used for the signing format you
+	chose (see `cryptoSign.format`). The default value for
+	`gpg.x509.program` is "gpgsm" and `gpg.ssh.program` is "ssh-keygen".
+	With the format set to "opengpg" or "x509" the program must support the
 	same command-line interface as GPG, namely, to verify a detached
 	signature, "`gpg --verify $signature - <$file`" is run, and the
 	program is expected to signal a good signature by exiting with
@@ -8,17 +19,12 @@ gpg.program::
 	standard input of "`gpg -bsau $key`" is fed with the contents to be
 	signed, and the program is expected to send the result to its
 	standard output.
+	If the format is "ssh", then the configured program must implement the
+	`ssh-keygen -Y find-principals|check-novalidate|verify|sign` commands
+	(see ssh-keygen(1) man page).
 
-gpg.format::
-	Specifies which key format to use when signing with `--gpg-sign`.
-	Default is "openpgp". Other possible values are "x509", "ssh".
-
-gpg.<format>.program::
-	Use this to customize the program used for the signing format you
-	chose. (see `gpg.program` and `gpg.format`) `gpg.program` can still
-	be used as a legacy synonym for `gpg.openpgp.program`. The default
-	value for `gpg.x509.program` is "gpgsm" and `gpg.ssh.program` is "ssh-keygen".
 
+crpytoSign.gpg.minTrustLevel::
 gpg.minTrustLevel::
 	Specifies a minimum trust level for signature verification.  If
 	this option is unset, then signature verification for merge
@@ -34,12 +40,14 @@ gpg.minTrustLevel::
 * `fully`
 * `ultimate`
 
+cryptoSign.ssh.defaultKeyCommand::
 gpg.ssh.defaultKeyCommand:
 	This command that will be run when user.signingkey is not set and a ssh
 	signature is requested. On successful exit a valid ssh public key is
 	expected in the	first line of its output. To automatically use the first
 	available key from your ssh-agent set this to "ssh-add -L".
 
+cryptoSign.ssh.allowedSignersFile::
 gpg.ssh.allowedSignersFile::
 	A file containing ssh public keys which you are willing to trust.
 	The file consists of one or more lines of principals followed by an ssh
@@ -67,6 +75,7 @@ This way only committers with an already valid key can add or change keys in the
 Using a SSH CA key with the cert-authority option
 (see ssh-keygen(1) "CERTIFICATES") is also valid.
 
+cryptoSign.ssh.revocationFile::
 gpg.ssh.revocationFile::
 	Either a SSH KRL or a list of revoked public keys (without the principal prefix).
 	See ssh-keygen(1) for details.
diff --git a/gpg-interface.c b/gpg-interface.c
index 3e7255a2a9..eacafcd56e 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -638,6 +638,7 @@ int git_gpg_config(const char *var, const char *value, void *cb)
 	struct gpg_format *fmt = NULL;
 	char *fmtname = NULL;
 	char *trust;
+	const char *crypto_var = NULL;
 	int ret;
 
 	if (!strcmp(var, "user.signingkey")) {
@@ -647,7 +648,17 @@ int git_gpg_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 
-	if (!strcmp(var, "gpg.format")) {
+	/*
+	 * `gpg.` is a backwards compatibility prefix alias for `cryptosign.`
+	 * All following vars expect a prefix so we can return early if
+	 * there is none
+	 */
+	if (!skip_prefix(var, "gpg.", &crypto_var) &&
+	    !skip_prefix(var, "cryptosign.", &crypto_var))
+		return 0;
+
+
+	if (!strcmp(crypto_var, "format")) {
 		if (!value)
 			return config_error_nonbool(var);
 		fmt = get_format_by_name(value);
@@ -658,7 +669,9 @@ int git_gpg_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 
-	if (!strcmp(var, "gpg.mintrustlevel")) {
+	/* `gpg.mintrustlevel` moved to `cryptosign.gpg.mintrustlevel` */
+	if (!strcmp(crypto_var, "mintrustlevel") ||
+	    !strcmp(crypto_var, "gpg.mintrustlevel")) {
 		if (!value)
 			return config_error_nonbool(var);
 
@@ -672,31 +685,32 @@ int git_gpg_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 
-	if (!strcmp(var, "gpg.ssh.defaultkeycommand")) {
+	if (!strcmp(crypto_var, "ssh.defaultkeycommand")) {
 		if (!value)
 			return config_error_nonbool(var);
 		return git_config_string(&ssh_default_key_command, var, value);
 	}
 
-	if (!strcmp(var, "gpg.ssh.allowedsignersfile")) {
+	if (!strcmp(crypto_var, "ssh.allowedsignersfile")) {
 		if (!value)
 			return config_error_nonbool(var);
 		return git_config_pathname(&ssh_allowed_signers, var, value);
 	}
 
-	if (!strcmp(var, "gpg.ssh.revocationfile")) {
+	if (!strcmp(crypto_var, "ssh.revocationfile")) {
 		if (!value)
 			return config_error_nonbool(var);
 		return git_config_pathname(&ssh_revocation_file, var, value);
 	}
 
-	if (!strcmp(var, "gpg.program") || !strcmp(var, "gpg.openpgp.program"))
+	if (!strcmp(crypto_var, "program") ||
+	    !strcmp(crypto_var, "openpgp.program"))
 		fmtname = "openpgp";
 
-	if (!strcmp(var, "gpg.x509.program"))
+	if (!strcmp(crypto_var, "x509.program"))
 		fmtname = "x509";
 
-	if (!strcmp(var, "gpg.ssh.program"))
+	if (!strcmp(crypto_var, "ssh.program"))
 		fmtname = "ssh";
 
 	if (fmtname) {
-- 
2.33.1

Re: [RFC PATCH 1/2] crypto sign: add crypto-sign alias flag

[Junio C Hamano]

Fabian Stelzer <fs@gigacodes.de> writes:

> +--crypto-sign[=<keyid>]::
> +--no-crypto-sign::
>  --gpg-sign[=<keyid>]::
>  --no-gpg-sign::
> -	GPG-sign commits. The `keyid` argument is optional and
> -	defaults to the committer identity; if specified, it must be
> -	stuck to the option without a space. `--no-gpg-sign` is useful to
> -	countermand both `commit.gpgSign` configuration variable, and
> -	earlier `--gpg-sign`.
> +	Cryptographically sign commits. The `keyid` argument is optional and
> +	its default depends on the configured `cryptoSign.format`; if specified,
> +	it must be stuck to the option without a space. `--no-crypto-sign` is
> +	useful to countermand both `commit.gpgSign` configuration variable, and
> +	earlier `--crypto-sign`.
> +	`--(no-)gpg-sign` is a compatibility alias and has no effect on which
> +	cryptographic format will be used. This is determined by the
> +	configuration variable cryptoSign.format (see linkgit:git-config[1]).

I'd make the last three lines into a separate paragraph and nudge
users toward the new spelling if I were doing this change, e.g.

	...
	earlier `--crypto-sign`.
+
The option was originally called `--[no-]gpg-sign` and is still
supported as a synonym, but it is encouraged to migrate to use
the `--crypto-sign` option.

Not the problem with this patch, but can the format be inferred from
<keyid>?

If so, `--crypt-sign=<keyid of format X>` can choose the format X
and specify what exact key to use at the same time without the
cryptosign.format configuration variable.  But if not, the interface
leaves us in an awkward place by letting different keys easily
specified from the command line while making it impossible to
switch between GPG and SSH from the command line.

If --gpg-sign is not a mere synonym, but also implies GPG is
preferred when cryptoSign.format is not specified, that is a
different story, of course.  That makes it unnecessary to deprecate
`--gpg-sign` and in addition we need to add `--ssh-sign` option that
works similarly, which may not scale well but I do not expect we'd
add many more next to GPG and SSH, hopefully?  I dunno.

> diff --git a/builtin/commit.c b/builtin/commit.c
> index 883c16256c..2c789ff6f9 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -1639,8 +1639,11 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>  		OPT_BOOL('e', "edit", &edit_flag, N_("force edit of commit")),
>  		OPT_CLEANUP(&cleanup_arg),
>  		OPT_BOOL(0, "status", &include_status, N_("include status in commit message template")),
> -		{ OPTION_STRING, 'S', "gpg-sign", &sign_commit, N_("key-id"),
> +		{ OPTION_STRING, 'S', "crypto-sign", &sign_commit, N_("key-id"),
> +		  N_("cryptographically sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
> +		{ OPTION_STRING, 0, "gpg-sign", &sign_commit, N_("key-id"),
>  		  N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" },

Leaving this explained as "GPG sign commit" contradicts "it is a
mere alias that does not even imply GPG is preferred when no
preference is given by the cryptoSign.format variable".

> +

Unwanted blank line before the "this is the last line of these
options" marker?

>  		/* end commit message options */
>  
>  		OPT_GROUP(N_("Commit contents options")),

Re: [RFC PATCH 2/2] crypto sign: add cryptoSign.* config

[Eric Sunshine]

`On Mon, Dec 20, 2021 at 9:09 AM Fabian Stelzer <fs@gigacodes.de> wrote:
> Since git now supports multiple cryptographic methods/formats to sign
> objects, the `gpg.` configuration prefix is misleading.
> Add `cryptoSign.`, but keep `gpg.` as a compatibility alias at least for
> all existing options.
> `gpg.mintrustlevel` is moved to `cryptosign.gpg.mintrustlevel` while
> also still allowing the former.
> ---
> diff --git a/Documentation/config/gpg.txt b/Documentation/config/gpg.txt
> @@ -1,6 +1,17 @@
> +cryptoSign.format::
> +gpg.format::
> +       Specifies which key format to use when signing with `--crypto-sign`.
> +       Default is "openpgp". Other possible values are "x509", "ssh".
> +
> +cryptoSign.<format>.program::
> +gpg.<format>.program::
> +       Use this to customize the program used for the signing format you
> +       chose (see `cryptoSign.format`). The default value for

This is a somewhat minor comment, but I find that grouping these
config keys together like this gives too much weight to the old
`gpg.foo` ones, making it seem as if they're still first-class
citizens which people can use freely. If you instead organize them as
below, then it is easier to see at a glance that the old keys
shouldn't be used:

    cryptoSign.format::
        Specifies which key format to use when signing...

    cryptoSign.<format>.program::
        Use this to customize the program used...

    ...

    gpg.format::
        Deprecated synonym of `cryptoSign.format`.

    gpg.<format>.program::
        Deprecated synonym of `cryptoSign.<format>.program`.

The same observation about grouping of config keys applies to the
remainder of the documentation changes in this patch.

Re: [RFC PATCH 1/2] crypto sign: add crypto-sign alias flag

[Fabian Stelzer]

On 20.12.2021 13:54, Junio C Hamano wrote:
>Fabian Stelzer <fs@gigacodes.de> writes:
>
>> +--crypto-sign[=<keyid>]::
>> +--no-crypto-sign::
>>  --gpg-sign[=<keyid>]::
>>  --no-gpg-sign::
>> -	GPG-sign commits. The `keyid` argument is optional and
>> -	defaults to the committer identity; if specified, it must be
>> -	stuck to the option without a space. `--no-gpg-sign` is useful to
>> -	countermand both `commit.gpgSign` configuration variable, and
>> -	earlier `--gpg-sign`.
>> +	Cryptographically sign commits. The `keyid` argument is optional and
>> +	its default depends on the configured `cryptoSign.format`; if specified,
>> +	it must be stuck to the option without a space. `--no-crypto-sign` is
>> +	useful to countermand both `commit.gpgSign` configuration variable, and
>> +	earlier `--crypto-sign`.
>> +	`--(no-)gpg-sign` is a compatibility alias and has no effect on which
>> +	cryptographic format will be used. This is determined by the
>> +	configuration variable cryptoSign.format (see linkgit:git-config[1]).
>
>I'd make the last three lines into a separate paragraph and nudge
>users toward the new spelling if I were doing this change, e.g.
>
>	...
>	earlier `--crypto-sign`.
>+
>The option was originally called `--[no-]gpg-sign` and is still
>supported as a synonym, but it is encouraged to migrate to use
>the `--crypto-sign` option.

Will do.

>
>Not the problem with this patch, but can the format be inferred from
><keyid>?
>
>If so, `--crypt-sign=<keyid of format X>` can choose the format X
>and specify what exact key to use at the same time without the
>cryptosign.format configuration variable.  But if not, the interface
>leaves us in an awkward place by letting different keys easily
>specified from the command line while making it impossible to
>switch between GPG and SSH from the command line.
>

I thought about doing this when we added the key:: prefix to differentiate 
between literal keys and file paths for ssh. Something like 
ssh-key::/ssh-file::/gpg-key::
I decided against it since I think this could lead to different to 
understand behaviour for the user. It is quite clear that a flag takes 
precedence over a config var, but what if i set `format=gpg` and then 
`user.signingkey=ssh-key::...`?
If we would start this from a green field and did not have the format 
setting, then just inferring from the key would be ok. But with the backward 
compatibility I think this is too confusing.

>If --gpg-sign is not a mere synonym, but also implies GPG is
>preferred when cryptoSign.format is not specified, that is a
>different story, of course.  That makes it unnecessary to deprecate
>`--gpg-sign` and in addition we need to add `--ssh-sign` option that
>works similarly, which may not scale well but I do not expect we'd
>add many more next to GPG and SSH, hopefully?  I dunno.
>

This is indeed a path we could take. Here the flag would have precedence 
over the config. However it would not make sense to just specify --ssh-sign 
when `user.signingkey` is set to a gpg key id. So the user would always have 
to specify the key or we would need to move the signing key config into the 
format specific blocks leading to even more compatibility code paths :/

I'm going to make some bold assumptions here, so please correct me if I'm 
wrong.
Most users will not need to switch signing methods within repositories, if 
at all.  Corporate users will probably adopt ssh, if anything at all. I have 
never seen gpg consistently used in a corporate setup. Open source 
development is heavily leaning on gpg. So the most common case will probably 
be users having to switch between work (ssh) & oss (gpg) development which 
can either be configured per repository or by using `includeIf "gitdir:"` in 
your .gitconfig.

So I think the extra flags or extended key prefixes to infer the format are 
not needed and we can choose the simple option of only having the single 
cryptosign.format config.

>> diff --git a/builtin/commit.c b/builtin/commit.c
>> index 883c16256c..2c789ff6f9 100644
>> --- a/builtin/commit.c
>> +++ b/builtin/commit.c
>> @@ -1639,8 +1639,11 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>>  		OPT_BOOL('e', "edit", &edit_flag, N_("force edit of commit")),
>>  		OPT_CLEANUP(&cleanup_arg),
>>  		OPT_BOOL(0, "status", &include_status, N_("include status in commit message template")),
>> -		{ OPTION_STRING, 'S', "gpg-sign", &sign_commit, N_("key-id"),
>> +		{ OPTION_STRING, 'S', "crypto-sign", &sign_commit, N_("key-id"),
>> +		  N_("cryptographically sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
>> +		{ OPTION_STRING, 0, "gpg-sign", &sign_commit, N_("key-id"),
>>  		  N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
>
>Leaving this explained as "GPG sign commit" contradicts "it is a
>mere alias that does not even imply GPG is preferred when no
>preference is given by the cryptoSign.format variable".
>

True, will fix.

>> +
>
>Unwanted blank line before the "this is the last line of these
>options" marker?
>
>>  		/* end commit message options */
>>
>>  		OPT_GROUP(N_("Commit contents options")),

Re: [RFC PATCH 2/2] crypto sign: add cryptoSign.* config

[Fabian Stelzer]

On 20.12.2021 17:07, Eric Sunshine wrote:
>`On Mon, Dec 20, 2021 at 9:09 AM Fabian Stelzer <fs@gigacodes.de> wrote:
>> Since git now supports multiple cryptographic methods/formats to sign
>> objects, the `gpg.` configuration prefix is misleading.
>> Add `cryptoSign.`, but keep `gpg.` as a compatibility alias at least for
>> all existing options.
>> `gpg.mintrustlevel` is moved to `cryptosign.gpg.mintrustlevel` while
>> also still allowing the former.
>> ---
>> diff --git a/Documentation/config/gpg.txt b/Documentation/config/gpg.txt
>> @@ -1,6 +1,17 @@
>> +cryptoSign.format::
>> +gpg.format::
>> +       Specifies which key format to use when signing with `--crypto-sign`.
>> +       Default is "openpgp". Other possible values are "x509", "ssh".
>> +
>> +cryptoSign.<format>.program::
>> +gpg.<format>.program::
>> +       Use this to customize the program used for the signing format you
>> +       chose (see `cryptoSign.format`). The default value for
>
>This is a somewhat minor comment, but I find that grouping these
>config keys together like this gives too much weight to the old
>`gpg.foo` ones, making it seem as if they're still first-class
>citizens which people can use freely. If you instead organize them as
>below, then it is easier to see at a glance that the old keys
>shouldn't be used:
>
>    cryptoSign.format::
>        Specifies which key format to use when signing...
>
>    cryptoSign.<format>.program::
>        Use this to customize the program used...
>
>    ...
>
>    gpg.format::
>        Deprecated synonym of `cryptoSign.format`.
>
>    gpg.<format>.program::
>        Deprecated synonym of `cryptoSign.<format>.program`.
>
>The same observation about grouping of config keys applies to the
>remainder of the documentation changes in this patch.

I wasn't sure how much we want to already deprecate the `gpg.` keys so I 
tried a gentle approach :)
But I would be in favor of your variant.

Thanks