[RFC PATCH 0/2] cryptoSign flag & config

STALE1724d

5 messages, 2 authors, 2021-12-21 · open the first message on its own page

[RFC PATCH 0/2] cryptoSign flag & config

From: Fabian Stelzer <hidden>
Date: 2021-12-20 14:09:43

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

From: Fabian Stelzer <hidden>
Date: 2021-12-20 14:09:44

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

From: Fabian Stelzer <hidden>
Date: 2021-12-20 14:09:45

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 2/2] crypto sign: add cryptoSign.* config

From: Eric Sunshine <hidden>
Date: 2021-12-20 22:08:01

`On Mon, Dec 20, 2021 at 9:09 AM Fabian Stelzer [off-list ref] wrote:
quoted hunk
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 2/2] crypto sign: add cryptoSign.* config

From: Fabian Stelzer <hidden>
Date: 2021-12-21 09:39:46

On 20.12.2021 17:07, Eric Sunshine wrote:
`On Mon, Dec 20, 2021 at 9:09 AM Fabian Stelzer [off-list ref] wrote:
quoted
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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help