threads / patch / 56928

patchssh signing: support non ssh-* keytypes

Subject: [PATCH] ssh signing: support non ssh-* keytypes

## tl;dr

12 messages between Nov 17, 2021 and Nov 19, 2021. Diffs are folded; open one to read it.

replies: 11people: 4as markdown or json

Fabian Stelzer· Nov 17, 2021, 16:27 UTC · lore

The user.signingKey config for ssh signing supports either a path to a file containing the key or for the sake of convenience a literal string with the ssh public key. To differentiate between those two cases we check if the first few characters contain "ssh-" which is unlikely to be the start of a path. ssh supports other key types which are not prefixed with "ssh-" and will currently be treated as a file path and therefore fail to load. To remedy this we move the prefix check into its own function and add the other key types. "ssh -Q key" can be used to show a list of all supported types.

Signed-off-by: Fabian Stelzer <fs@gigacodes.de>
---
 gpg-interface.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)
Show changes to gpg-interface.c +13 −3
diff --git a/gpg-interface.c b/gpg-interface.c
index 3e7255a2a9..dd1df9f4ee 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -707,6 +707,16 @@ int git_gpg_config(const char *var, const char *value, void *cb)
 	return 0;
 }
 
+/* Determines wether key contains a literal ssh key or a path to a file */
+static int is_literal_ssh_key(const char *key) {
+	return (
+		starts_with(key, "ssh-") ||
+		starts_with(key, "ecdsa-") ||
+		starts_with(key, "sk-ssh-") ||
+		starts_with(key, "sk-ecdsa-")
+	);
+}
+
 static char *get_ssh_key_fingerprint(const char *signing_key)
 {
 	struct child_process ssh_keygen = CHILD_PROCESS_INIT;
@@ -719,7 +729,7 @@ static char *get_ssh_key_fingerprint(const char *signing_key)
 	 * With SSH Signing this can contain a filename or a public key
 	 * For textual representation we usually want a fingerprint
 	 */
-	if (starts_with(signing_key, "ssh-")) {
+	if (is_literal_ssh_key(signing_key)) {
 		strvec_pushl(&ssh_keygen.args, "ssh-keygen", "-lf", "-", NULL);
 		ret = pipe_command(&ssh_keygen, signing_key,
 				   strlen(signing_key), &fingerprint_stdout, 0,
@@ -774,7 +784,7 @@ static const char *get_default_ssh_signing_key(void)
 
 	if (!ret) {
 		keys = strbuf_split_max(&key_stdout, '\n', 2);
-		if (keys[0] && starts_with(keys[0]->buf, "ssh-")) {
+		if (keys[0] && is_literal_ssh_key(keys[0]->buf)) {
 			default_key = strbuf_detach(keys[0], NULL);
 		} else {
 			warning(_("gpg.ssh.defaultKeyCommand succeeded but returned no keys: %s %s"),
@@ -894,7 +904,7 @@ static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature,
 		return error(
 			_("user.signingkey needs to be set for ssh signing"));
 
-	if (starts_with(signing_key, "ssh-")) {
+	if (is_literal_ssh_key(signing_key)) {
 		/* A literal ssh key */
 		key_file = mks_tempfile_t(".git_signing_key_tmpXXXXXX");
 		if (!key_file)

base-commit: cd3e606211bb1cf8bc57f7d76bab98cc17a150bc
-- 
2.31.1
Taylor Blau· Nov 17, 2021, 17:51 UTC · re: Fabian Stelzer · lore

Re: [PATCH] ssh signing: support non ssh-* keytypes

On Wed, Nov 17, 2021 at 05:27:27PM +0100, Fabian Stelzer wrote:
Show 24 quoted lines
> The user.signingKey config for ssh signing supports either a path to a
> file containing the key or for the sake of convenience a literal string
> with the ssh public key. To differentiate between those two cases we
> check if the first few characters contain "ssh-" which is unlikely to be
> the start of a path. ssh supports other key types which are not prefixed
> with "ssh-" and will currently be treated as a file path and therefore
> fail to load. To remedy this we move the prefix check into its own
> function and add the other key types. "ssh -Q key" can be used to show a
> list of all supported types.
>
> Signed-off-by: Fabian Stelzer <fs@gigacodes.de>
> ---
>  gpg-interface.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/gpg-interface.c b/gpg-interface.c
> index 3e7255a2a9..dd1df9f4ee 100644
> --- a/gpg-interface.c
> +++ b/gpg-interface.c
> @@ -707,6 +707,16 @@ int git_gpg_config(const char *var, const char *value, void *cb)
>  	return 0;
>  }
>
> +/* Determines wether key contains a literal ssh key or a path to a file */
Nit: s/wether/whether.

I had to re-read this comment before I realized that the "or a path to a file" isn't checked by this function, but is what we assume to be true if this function returns 0.

So I don't think anything you wrote there is wrong, but it may be clearer to just say "Returns 1 if `key` contains a literal SSH key, 0 otherwise."

Show 8 quoted lines
> +static int is_literal_ssh_key(const char *key) {
> +	return (
> +		starts_with(key, "ssh-") ||
> +		starts_with(key, "ecdsa-") ||
> +		starts_with(key, "sk-ssh-") ||
> +		starts_with(key, "sk-ecdsa-")
> +	);
> +}

The outer-most parenthesis are unnecessary, but help with line wrapping. Equally OK would have been:

    return starts_with(...) ||
      starts_with(...) ||
and so on, but it doesn't matter much one way or the other.
Show 10 quoted lines
> +
>  static char *get_ssh_key_fingerprint(const char *signing_key)
>  {
>  	struct child_process ssh_keygen = CHILD_PROCESS_INIT;
> @@ -719,7 +729,7 @@ static char *get_ssh_key_fingerprint(const char *signing_key)
>  	 * With SSH Signing this can contain a filename or a public key
>  	 * For textual representation we usually want a fingerprint
>  	 */
> -	if (starts_with(signing_key, "ssh-")) {
> +	if (is_literal_ssh_key(signing_key)) {

This (and all other replacements) are straightforward and exhaustive. It would be nice to see an additional test confirming that we treat, for e.g., literal ECDSA keys correctly.

Thanks, Taylor

Junio C Hamano· Nov 18, 2021, 03:09 UTC · re: Fabian Stelzer · lore

Re: [PATCH] ssh signing: support non ssh-* keytypes

Fabian Stelzer <fs@gigacodes.de> writes:
Show 9 quoted lines
> +/* Determines wether key contains a literal ssh key or a path to a file */
> +static int is_literal_ssh_key(const char *key) {
> +	return (
> +		starts_with(key, "ssh-") ||
> +		starts_with(key, "ecdsa-") ||
> +		starts_with(key, "sk-ssh-") ||
> +		starts_with(key, "sk-ecdsa-")
> +	);
> +}
A more forward looking thing you could do is to 
 (1) grandfather the convention "any string that begins with 'ssh-'
     is taken as a ssh literal key".
 (2) refrain from spreading such an unstructured mess by picking a
     reserved prefix, say "ssh-key::" and have all other kinds of
     ssh keys use the convention.
making the above function look more like
    static int is_literal_ssh_key(const char *string, const char **key)
    {
	if (skip_prefix(string, "ssh-key::", key)
	    return 1;
	if (starts_with(string, "ssh-")) {
	    key = string;
	    return 1;
	}
	return 0;
    }

so that the caller can extract the literal key from the string that specifies either the literal key or path to the file. This will futureproof us in two axis. When SSH adds types of keys using new algo, we do not have to add it to is_literal_ssh_key() function. Also when another crypto suite other than GPG and SSH comes, we won't repeat the "bare 'ssh-' prefix is reserved by ssh, and different kind in the same suite may have to consume more reserved prefixes" mistake---it would make it more natural for us to pick "literal keys from any variant of that new FOO crypto suite are written with 'foo-key::' prefix" if we did so right now. It would have been better if we didn't have to do the grandfathering, but I am assuming that the ship has already sailed?

Show 9 quoted lines
> @@ -719,7 +729,7 @@ static char *get_ssh_key_fingerprint(const char *signing_key)
>  	 * With SSH Signing this can contain a filename or a public key
>  	 * For textual representation we usually want a fingerprint
>  	 */
> -	if (starts_with(signing_key, "ssh-")) {
> +	if (is_literal_ssh_key(signing_key)) {
> 		strvec_pushl(&ssh_keygen.args, "ssh-keygen", "-lf", "-", NULL);
> 		ret = pipe_command(&ssh_keygen, signing_key,
> 				   strlen(signing_key), &fingerprint_stdout, 0,

This part needs a bit of adjustment if we go the "is_literal_ssh_key() is not just a boolean but is used to strip the prefix to signal the kind of key" route, but the necessary adjustment should be trivial.

Junio C Hamano· Nov 18, 2021, 06:39 UTC · re: Junio C Hamano · lore

Re: [PATCH] ssh signing: support non ssh-* keytypes

Junio C Hamano <gitster@pobox.com> writes:
Show 33 quoted lines
> Fabian Stelzer <fs@gigacodes.de> writes:
>
>> +/* Determines wether key contains a literal ssh key or a path to a file */
>> +static int is_literal_ssh_key(const char *key) {
>> +	return (
>> +		starts_with(key, "ssh-") ||
>> +		starts_with(key, "ecdsa-") ||
>> +		starts_with(key, "sk-ssh-") ||
>> +		starts_with(key, "sk-ecdsa-")
>> +	);
>> +}
>
> A more forward looking thing you could do is to 
>
>  (1) grandfather the convention "any string that begins with 'ssh-'
>      is taken as a ssh literal key".
>
>  (2) refrain from spreading such an unstructured mess by picking a
>      reserved prefix, say "ssh-key::" and have all other kinds of
>      ssh keys use the convention.
>
> making the above function look more like
>
>     static int is_literal_ssh_key(const char *string, const char **key)
>     {
> 	if (skip_prefix(string, "ssh-key::", key)
> 	    return 1;
> 	if (starts_with(string, "ssh-")) {
> 	    key = string;
> 	    return 1;
> 	}
> 	return 0;
>     }

Given that this ONLY gets called from ssh codepath, I think the special prefix can just be "key::", and when a new crypto suite is introduced to sit next to GPG and SSH, presumably the code structure to support it will be similar to that of ssh's, and it can also use "key::" prefix for their literal keys. That design may be cleaner.

Thanks.
Fabian Stelzer· Nov 18, 2021, 15:16 UTC · re: Junio C Hamano · lore

Re: [PATCH] ssh signing: support non ssh-* keytypes

On 17.11.2021 22:39, Junio C Hamano wrote:
Show 44 quoted lines
>Junio C Hamano <gitster@pobox.com> writes:
>
>> Fabian Stelzer <fs@gigacodes.de> writes:
>>
>>> +/* Determines wether key contains a literal ssh key or a path to a file */
>>> +static int is_literal_ssh_key(const char *key) {
>>> +	return (
>>> +		starts_with(key, "ssh-") ||
>>> +		starts_with(key, "ecdsa-") ||
>>> +		starts_with(key, "sk-ssh-") ||
>>> +		starts_with(key, "sk-ecdsa-")
>>> +	);
>>> +}
>>
>> A more forward looking thing you could do is to
>>
>>  (1) grandfather the convention "any string that begins with 'ssh-'
>>      is taken as a ssh literal key".
>>
>>  (2) refrain from spreading such an unstructured mess by picking a
>>      reserved prefix, say "ssh-key::" and have all other kinds of
>>      ssh keys use the convention.
>>
>> making the above function look more like
>>
>>     static int is_literal_ssh_key(const char *string, const char **key)
>>     {
>> 	if (skip_prefix(string, "ssh-key::", key)
>> 	    return 1;
>> 	if (starts_with(string, "ssh-")) {
>> 	    key = string;
>> 	    return 1;
>> 	}
>> 	return 0;
>>     }
>
>Given that this ONLY gets called from ssh codepath, I think the
>special prefix can just be "key::", and when a new crypto suite
>is introduced to sit next to GPG and SSH, presumably the code
>structure to support it will be similar to that of ssh's, and it
>can also use "key::" prefix for their literal keys.  That design
>may be cleaner.
>
>Thanks.

Thanks both for your review. I will use the key:: suggestion and also add tests for this. For now i guess we will have to keep the ssh- since it's already out there :/ Will reroll soon.

Fabian
Fabian Stelzer· Nov 18, 2021, 17:14 UTC · re: Fabian Stelzer · lore

[PATCH v2 1/2] ssh signing: support non ssh-* keytypes

The user.signingKey config for ssh signing supports either a path to a file containing the key or for the sake of convenience a literal string with the ssh public key. To differentiate between those two cases we check if the first few characters contain "ssh-" which is unlikely to be the start of a path. ssh supports other key types which are not prefixed with "ssh-" and will currently be treated as a file path and therefore fail to load. To remedy this we move the prefix check into its own function and introduce the prefix `key::` for literal ssh keys. This way we don't need to add new key types when they become available. The existing `ssh-` prefix is retained for compatibility with current user configs but removed from the official documentation to discourage its use.

Signed-off-by: Fabian Stelzer <fs@gigacodes.de>
---
When writing the tests for this i remembered why we had none for literal
keys. Those need a running ssh-agent with the private key present.
Please let me know if the tests provide a safer way to start the
additional agent and make sure it gets cleaned up correctly. I tried
grepping for spawn,kill and similar things but did not find much else.
 Documentation/config/user.txt | 14 +++++++-------
 gpg-interface.c               | 36 ++++++++++++++++++++++++++++-------
 t/lib-gpg.sh                  |  3 +++
 t/t7528-signed-commit-ssh.sh  | 24 ++++++++++++++++++++++-
 4 files changed, 62 insertions(+), 15 deletions(-)
Show changes to 4 files +62 −15

Documentation/config/user.txt, gpg-interface.c, t/lib-gpg.sh, t/t7528-signed-commit-ssh.sh

diff --git a/Documentation/config/user.txt b/Documentation/config/user.txt
index ad78dce9ec..4de700f651 100644
--- a/Documentation/config/user.txt
+++ b/Documentation/config/user.txt
@@ -36,10 +36,10 @@ user.signingKey::
 	commit, you can override the default selection with this variable.
 	This option is passed unchanged to gpg's --local-user parameter,
 	so you may specify a key using any method that gpg supports.
-	If gpg.format is set to "ssh" this can contain the literal ssh public
-	key (e.g.: "ssh-rsa XXXXXX identifier") or a file which contains it and
-	corresponds to the private key used for signing. The private key
-	needs to be available via ssh-agent. Alternatively it can be set to
-	a file containing a private key directly. If not set git will call
-	gpg.ssh.defaultKeyCommand (e.g.: "ssh-add -L") and try to use the first
-	key available.
+	If gpg.format is set to `ssh` this can contain the path to either
+	your private ssh key or the public key when ssh-agent is used.
+	Alternatively it can contain a public key prefixed with `key::`
+	directly (e.g.: "key::ssh-rsa XXXXXX identifier"). The private key
+	needs to be available via ssh-agent. If not set git will call
+	gpg.ssh.defaultKeyCommand (e.g.: "ssh-add -L") and try to use the
+	first key available.
diff --git a/gpg-interface.c b/gpg-interface.c
index 3e7255a2a9..73554ea998 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -707,6 +707,21 @@ int git_gpg_config(const char *var, const char *value, void *cb)
 	return 0;
 }
 
+/*
+ * Returns 1 if `string` contains a literal ssh key, 0 otherwise
+ * `key` will be set to the start of the actual key if a prefix is present.
+ */
+static int is_literal_ssh_key(const char *string, const char **key)
+{
+	if (skip_prefix(string, "key::", key))
+		return 1;
+	if (starts_with(string, "ssh-")) {
+		*key = string;
+		return 1;
+	}
+	return 0;
+}
+
 static char *get_ssh_key_fingerprint(const char *signing_key)
 {
 	struct child_process ssh_keygen = CHILD_PROCESS_INIT;
@@ -714,15 +729,16 @@ static char *get_ssh_key_fingerprint(const char *signing_key)
 	struct strbuf fingerprint_stdout = STRBUF_INIT;
 	struct strbuf **fingerprint;
 	char *fingerprint_ret;
+	const char *literal_key = NULL;
 
 	/*
 	 * With SSH Signing this can contain a filename or a public key
 	 * For textual representation we usually want a fingerprint
 	 */
-	if (starts_with(signing_key, "ssh-")) {
+	if (is_literal_ssh_key(signing_key, &literal_key)) {
 		strvec_pushl(&ssh_keygen.args, "ssh-keygen", "-lf", "-", NULL);
-		ret = pipe_command(&ssh_keygen, signing_key,
-				   strlen(signing_key), &fingerprint_stdout, 0,
+		ret = pipe_command(&ssh_keygen, literal_key,
+				   strlen(literal_key), &fingerprint_stdout, 0,
 				   NULL, 0);
 	} else {
 		strvec_pushl(&ssh_keygen.args, "ssh-keygen", "-lf",
@@ -757,6 +773,7 @@ static const char *get_default_ssh_signing_key(void)
 	const char **argv;
 	int n;
 	char *default_key = NULL;
+	const char *literal_key = NULL;
 
 	if (!ssh_default_key_command)
 		die(_("either user.signingkey or gpg.ssh.defaultKeyCommand needs to be configured"));
@@ -774,7 +791,11 @@ static const char *get_default_ssh_signing_key(void)
 
 	if (!ret) {
 		keys = strbuf_split_max(&key_stdout, '\n', 2);
-		if (keys[0] && starts_with(keys[0]->buf, "ssh-")) {
+		if (keys[0] && is_literal_ssh_key(keys[0]->buf, &literal_key)) {
+			/*
+			 * We only use `is_literal_ssh_key` here to check validity
+			 * The prefix will be stripped when the key is used.
+			 */
 			default_key = strbuf_detach(keys[0], NULL);
 		} else {
 			warning(_("gpg.ssh.defaultKeyCommand succeeded but returned no keys: %s %s"),
@@ -889,19 +910,20 @@ static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature,
 	struct tempfile *key_file = NULL, *buffer_file = NULL;
 	char *ssh_signing_key_file = NULL;
 	struct strbuf ssh_signature_filename = STRBUF_INIT;
+	const char *literal_key = NULL;
 
 	if (!signing_key || signing_key[0] == '\0')
 		return error(
 			_("user.signingkey needs to be set for ssh signing"));
 
-	if (starts_with(signing_key, "ssh-")) {
+	if (is_literal_ssh_key(signing_key, &literal_key)) {
 		/* A literal ssh key */
 		key_file = mks_tempfile_t(".git_signing_key_tmpXXXXXX");
 		if (!key_file)
 			return error_errno(
 				_("could not create temporary file"));
-		keylen = strlen(signing_key);
-		if (write_in_full(key_file->fd, signing_key, keylen) < 0 ||
+		keylen = strlen(literal_key);
+		if (write_in_full(key_file->fd, literal_key, keylen) < 0 ||
 		    close_tempfile_gently(key_file) < 0) {
 			error_errno(_("failed writing ssh signing key to '%s'"),
 				    key_file->filename.buf);
diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh
index a3f285f515..6434feb6c1 100644
--- a/t/lib-gpg.sh
+++ b/t/lib-gpg.sh
@@ -91,6 +91,7 @@ GPGSSH_KEY_PRIMARY="${GNUPGHOME}/ed25519_ssh_signing_key"
 GPGSSH_KEY_SECONDARY="${GNUPGHOME}/rsa_2048_ssh_signing_key"
 GPGSSH_KEY_UNTRUSTED="${GNUPGHOME}/untrusted_ssh_signing_key"
 GPGSSH_KEY_WITH_PASSPHRASE="${GNUPGHOME}/protected_ssh_signing_key"
+GPGSSH_KEY_ECDSA="${GNUPGHOME}/ecdsa_ssh_signing_key"
 GPGSSH_KEY_PASSPHRASE="super_secret"
 GPGSSH_ALLOWED_SIGNERS="${GNUPGHOME}/ssh.all_valid.allowedSignersFile"
 
@@ -119,6 +120,8 @@ test_lazy_prereq GPGSSH '
 	echo "\"principal with number 2\" $(cat "${GPGSSH_KEY_SECONDARY}.pub")" >> "${GPGSSH_ALLOWED_SIGNERS}" &&
 	ssh-keygen -t ed25519 -N "${GPGSSH_KEY_PASSPHRASE}" -C "git ed25519 encrypted key" -f "${GPGSSH_KEY_WITH_PASSPHRASE}" >/dev/null &&
 	echo "\"principal with number 3\" $(cat "${GPGSSH_KEY_WITH_PASSPHRASE}.pub")" >> "${GPGSSH_ALLOWED_SIGNERS}" &&
+	ssh-keygen -t ecdsa -N "" -f "${GPGSSH_KEY_ECDSA}" >/dev/null
+	echo "\"principal with number 4\" $(cat "${GPGSSH_KEY_ECDSA}.pub")" >> "${GPGSSH_ALLOWED_SIGNERS}" &&
 	ssh-keygen -t ed25519 -N "" -f "${GPGSSH_KEY_UNTRUSTED}" >/dev/null
 '
 
diff --git a/t/t7528-signed-commit-ssh.sh b/t/t7528-signed-commit-ssh.sh
index badf3ed320..455eafa15c 100755
--- a/t/t7528-signed-commit-ssh.sh
+++ b/t/t7528-signed-commit-ssh.sh
@@ -73,7 +73,29 @@ test_expect_success GPGSSH 'create signed commits' '
 	git tag eleventh-signed $(cat oid) &&
 	echo 12 | git commit-tree --gpg-sign="${GPGSSH_KEY_UNTRUSTED}" HEAD^{tree} >oid &&
 	test_line_count = 1 oid &&
-	git tag twelfth-signed-alt $(cat oid)
+	git tag twelfth-signed-alt $(cat oid) &&
+
+	echo 13>file && test_tick && git commit -a -m thirteenth -S"${GPGSSH_KEY_ECDSA}" &&
+	git tag thirteenth-signed-ecdsa
+'
+
+test_expect_success GPGSSH 'sign commits using literal public keys with ssh-agent' '
+	test_when_finished "test_unconfig commit.gpgsign" &&
+	test_config gpg.format ssh &&
+	eval $(ssh-agent) &&
+	test_when_finished "kill ${SSH_AGENT_PID}" &&
+	ssh-add "${GPGSSH_KEY_PRIMARY}" &&
+	echo 1 >file && git add file &&
+	git commit -a -m rsa-inline -S"$(cat "${GPGSSH_KEY_PRIMARY}.pub")" &&
+	echo 2 >file &&
+	test_config user.signingkey "$(cat "${GPGSSH_KEY_PRIMARY}.pub")" &&
+	git commit -a -m rsa-config -S &&
+	ssh-add "${GPGSSH_KEY_ECDSA}" &&
+	echo 3 >file &&
+	git commit -a -m ecdsa-inline -S"key::$(cat "${GPGSSH_KEY_ECDSA}.pub")" &&
+	echo 4 >file &&
+	test_config user.signingkey "key::$(cat "${GPGSSH_KEY_ECDSA}.pub")" &&
+	git commit -a -m ecdsa-config -S
 '
 
 test_expect_success GPGSSH 'verify and show signatures' '

base-commit: cd3e606211bb1cf8bc57f7d76bab98cc17a150bc
-- 
2.31.1
Fabian Stelzer· Nov 18, 2021, 17:14 UTC · re: Fabian Stelzer · lore

[PATCH v2 2/2] ssh signing: make sign/amend test more resilient

The test `amending already signed commit` is using git checkout to select a specific commit to amend. In case an earlier test fails and leaves behind a dirty index/worktree this test would fail as well. Using `checkout -f` will avoid interference by most other tests.

Signed-off-by: Fabian Stelzer <fs@gigacodes.de>
---
 t/t7510-signed-commit.sh     | 2 +-
 t/t7528-signed-commit-ssh.sh | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)
Show changes to 2 files +2 −2

t/t7510-signed-commit.sh, t/t7528-signed-commit-ssh.sh

diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
index d65a0171f2..9882b69ae2 100755
--- a/t/t7510-signed-commit.sh
+++ b/t/t7510-signed-commit.sh
@@ -228,7 +228,7 @@ test_expect_success GPG 'detect fudged signature with NUL' '
 '
 
 test_expect_success GPG 'amending already signed commit' '
-	git checkout fourth-signed^0 &&
+	git checkout -f fourth-signed^0 &&
 	git commit --amend -S --no-edit &&
 	git verify-commit HEAD &&
 	git show -s --show-signature HEAD >actual &&
diff --git a/t/t7528-signed-commit-ssh.sh b/t/t7528-signed-commit-ssh.sh
index 455eafa15c..0ec5a6d764 100755
--- a/t/t7528-signed-commit-ssh.sh
+++ b/t/t7528-signed-commit-ssh.sh
@@ -239,7 +239,7 @@ test_expect_success GPGSSH 'amending already signed commit' '
 	test_config gpg.format ssh &&
 	test_config user.signingkey "${GPGSSH_KEY_PRIMARY}" &&
 	test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" &&
-	git checkout fourth-signed^0 &&
+	git checkout -f fourth-signed^0 &&
 	git commit --amend -S --no-edit &&
 	git verify-commit HEAD &&
 	git show -s --show-signature HEAD >actual &&
-- 
2.31.1
Eric Sunshine· Nov 18, 2021, 22:14 UTC · re: Fabian Stelzer · lore

Re: [PATCH v2 1/2] ssh signing: support non ssh-* keytypes

On Thu, Nov 18, 2021 at 12:14 PM Fabian Stelzer <fs@gigacodes.de> wrote:
Show 12 quoted lines
> The user.signingKey config for ssh signing supports either a path to a
> file containing the key or for the sake of convenience a literal string
> with the ssh public key. To differentiate between those two cases we
> check if the first few characters contain "ssh-" which is unlikely to be
> the start of a path. ssh supports other key types which are not prefixed
> with "ssh-" and will currently be treated as a file path and therefore
> fail to load. To remedy this we move the prefix check into its own
> function and introduce the prefix `key::` for literal ssh keys. This way
> we don't need to add new key types when they become available. The
> existing `ssh-` prefix is retained for compatibility with current user
> configs but removed from the official documentation to discourage its
> use.

I think we usually avoid removing documentation for something which is still supported (even if deprecated) for the very real reason that people will still encounter the old form in the wild, whether in configuration files, in blogs, or elsewhere, and may be perplexed to discover that the form is not documented (thus not understand how or why it seems to be working). Instead, we can discourage its use by mentioning clearly that it is deprecated and that `key::` should be used instead.

Show 20 quoted lines
> Signed-off-by: Fabian Stelzer <fs@gigacodes.de>
> ---
> diff --git a/Documentation/config/user.txt b/Documentation/config/user.txt
> @@ -36,10 +36,10 @@ user.signingKey::
>         This option is passed unchanged to gpg's --local-user parameter,
>         so you may specify a key using any method that gpg supports.
> -       If gpg.format is set to "ssh" this can contain the literal ssh public
> -       key (e.g.: "ssh-rsa XXXXXX identifier") or a file which contains it and
> -       corresponds to the private key used for signing. The private key
> -       needs to be available via ssh-agent. Alternatively it can be set to
> -       a file containing a private key directly. If not set git will call
> -       gpg.ssh.defaultKeyCommand (e.g.: "ssh-add -L") and try to use the first
> -       key available.
> +       If gpg.format is set to `ssh` this can contain the path to either
> +       your private ssh key or the public key when ssh-agent is used.
> +       Alternatively it can contain a public key prefixed with `key::`
> +       directly (e.g.: "key::ssh-rsa XXXXXX identifier"). The private key
> +       needs to be available via ssh-agent. If not set git will call
> +       gpg.ssh.defaultKeyCommand (e.g.: "ssh-add -L") and try to use the
> +       first key available.
Thus, perhaps this text could end with:
    For backward compatibility, a raw key which begins with "ssh-",
    such as "ssh-rsa XXXXXX identifier", is treated as "key::ssh-rsa
    XXXXXX identifier", but this form is deprecated; use the `key::`
    form instead.
Fabian Stelzer· Nov 19, 2021, 09:05 UTC · re: Eric Sunshine · lore

Re: [PATCH v2 1/2] ssh signing: support non ssh-* keytypes

On 18.11.2021 17:14, Eric Sunshine wrote:
Show 23 quoted lines
>On Thu, Nov 18, 2021 at 12:14 PM Fabian Stelzer <fs@gigacodes.de> wrote:
>> The user.signingKey config for ssh signing supports either a path to a
>> file containing the key or for the sake of convenience a literal string
>> with the ssh public key. To differentiate between those two cases we
>> check if the first few characters contain "ssh-" which is unlikely to be
>> the start of a path. ssh supports other key types which are not prefixed
>> with "ssh-" and will currently be treated as a file path and therefore
>> fail to load. To remedy this we move the prefix check into its own
>> function and introduce the prefix `key::` for literal ssh keys. This way
>> we don't need to add new key types when they become available. The
>> existing `ssh-` prefix is retained for compatibility with current user
>> configs but removed from the official documentation to discourage its
>> use.
>
>I think we usually avoid removing documentation for something which is
>still supported (even if deprecated) for the very real reason that
>people will still encounter the old form in the wild, whether in
>configuration files, in blogs, or elsewhere, and may be perplexed to
>discover that the form is not documented (thus not understand how or
>why it seems to be working). Instead, we can discourage its use by
>mentioning clearly that it is deprecated and that `key::` should be
>used instead.
>

I thought since this only existed in the docs since the 2.34 release a few days ago i could get away with it ;) But since we keep the support for `ssh-` in the code we should document it as such. I'll add your suggestion below to it. Thanks for your help.

Show 27 quoted lines
>> Signed-off-by: Fabian Stelzer <fs@gigacodes.de>
>> ---
>> diff --git a/Documentation/config/user.txt b/Documentation/config/user.txt
>> @@ -36,10 +36,10 @@ user.signingKey::
>>         This option is passed unchanged to gpg's --local-user parameter,
>>         so you may specify a key using any method that gpg supports.
>> -       If gpg.format is set to "ssh" this can contain the literal ssh public
>> -       key (e.g.: "ssh-rsa XXXXXX identifier") or a file which contains it and
>> -       corresponds to the private key used for signing. The private key
>> -       needs to be available via ssh-agent. Alternatively it can be set to
>> -       a file containing a private key directly. If not set git will call
>> -       gpg.ssh.defaultKeyCommand (e.g.: "ssh-add -L") and try to use the first
>> -       key available.
>> +       If gpg.format is set to `ssh` this can contain the path to either
>> +       your private ssh key or the public key when ssh-agent is used.
>> +       Alternatively it can contain a public key prefixed with `key::`
>> +       directly (e.g.: "key::ssh-rsa XXXXXX identifier"). The private key
>> +       needs to be available via ssh-agent. If not set git will call
>> +       gpg.ssh.defaultKeyCommand (e.g.: "ssh-add -L") and try to use the
>> +       first key available.
>
>Thus, perhaps this text could end with:
>
>    For backward compatibility, a raw key which begins with "ssh-",
>    such as "ssh-rsa XXXXXX identifier", is treated as "key::ssh-rsa
>    XXXXXX identifier", but this form is deprecated; use the `key::`
>    form instead.
Fabian Stelzer· Nov 19, 2021, 15:07 UTC · re: Fabian Stelzer · lore

[PATCH v3 0/2] ssh signing: support non ssh-* keytypes

support generic ssh keytypes using a `key::` prefix instead of relying on ssh- prefixes.

changes since v2:
 - no longer hide that we still support `ssh-` prefixes for literal
   keys.
Fabian Stelzer (2):
  ssh signing: support non ssh-* keytypes
  ssh signing: make sign/amend test more resilient
 Documentation/config/user.txt | 17 ++++++++++-------
 gpg-interface.c               | 36 ++++++++++++++++++++++++++++-------
 t/lib-gpg.sh                  |  3 +++
 t/t7510-signed-commit.sh      |  2 +-
 t/t7528-signed-commit-ssh.sh  | 26 +++++++++++++++++++++++--
 5 files changed, 67 insertions(+), 17 deletions(-)
Range-diff against v2:
1:  ea032f98f0 ! 1:  865a32d37c ssh signing: support non ssh-* keytypes
    @@ Documentation/config/user.txt: user.signingKey::
     +	directly (e.g.: "key::ssh-rsa XXXXXX identifier"). The private key
     +	needs to be available via ssh-agent. If not set git will call
     +	gpg.ssh.defaultKeyCommand (e.g.: "ssh-add -L") and try to use the
    -+	first key available.
    ++	first key available. For backward compatibility, a raw key which
    ++	begins with "ssh-", such as "ssh-rsa XXXXXX identifier", is treated
    ++	as "key::ssh-rsa XXXXXX identifier", but this form is deprecated;
    ++	use the `key::` form instead.
     
      ## gpg-interface.c ##
     @@ gpg-interface.c: int git_gpg_config(const char *var, const char *value, void *cb)
2:  5054ff0e76 = 2:  868cb7f524 ssh signing: make sign/amend test more resilient
base-commit: cd3e606211bb1cf8bc57f7d76bab98cc17a150bc
-- 
2.31.1
Fabian Stelzer· Nov 19, 2021, 15:07 UTC · re: Fabian Stelzer · lore

[PATCH v3 1/2] ssh signing: support non ssh-* keytypes

The user.signingKey config for ssh signing supports either a path to a file containing the key or for the sake of convenience a literal string with the ssh public key. To differentiate between those two cases we check if the first few characters contain "ssh-" which is unlikely to be the start of a path. ssh supports other key types which are not prefixed with "ssh-" and will currently be treated as a file path and therefore fail to load. To remedy this we move the prefix check into its own function and introduce the prefix `key::` for literal ssh keys. This way we don't need to add new key types when they become available. The existing `ssh-` prefix is retained for compatibility with current user configs but removed from the official documentation to discourage its use.

Signed-off-by: Fabian Stelzer <fs@gigacodes.de>
---
 Documentation/config/user.txt | 17 ++++++++++-------
 gpg-interface.c               | 36 ++++++++++++++++++++++++++++-------
 t/lib-gpg.sh                  |  3 +++
 t/t7528-signed-commit-ssh.sh  | 24 ++++++++++++++++++++++-
 4 files changed, 65 insertions(+), 15 deletions(-)
Show changes to 4 files +65 −15

Documentation/config/user.txt, gpg-interface.c, t/lib-gpg.sh, t/t7528-signed-commit-ssh.sh

diff --git a/Documentation/config/user.txt b/Documentation/config/user.txt
index ad78dce9ec..ec9233b060 100644
--- a/Documentation/config/user.txt
+++ b/Documentation/config/user.txt
@@ -36,10 +36,13 @@ user.signingKey::
 	commit, you can override the default selection with this variable.
 	This option is passed unchanged to gpg's --local-user parameter,
 	so you may specify a key using any method that gpg supports.
-	If gpg.format is set to "ssh" this can contain the literal ssh public
-	key (e.g.: "ssh-rsa XXXXXX identifier") or a file which contains it and
-	corresponds to the private key used for signing. The private key
-	needs to be available via ssh-agent. Alternatively it can be set to
-	a file containing a private key directly. If not set git will call
-	gpg.ssh.defaultKeyCommand (e.g.: "ssh-add -L") and try to use the first
-	key available.
+	If gpg.format is set to `ssh` this can contain the path to either
+	your private ssh key or the public key when ssh-agent is used.
+	Alternatively it can contain a public key prefixed with `key::`
+	directly (e.g.: "key::ssh-rsa XXXXXX identifier"). The private key
+	needs to be available via ssh-agent. If not set git will call
+	gpg.ssh.defaultKeyCommand (e.g.: "ssh-add -L") and try to use the
+	first key available. For backward compatibility, a raw key which
+	begins with "ssh-", such as "ssh-rsa XXXXXX identifier", is treated
+	as "key::ssh-rsa XXXXXX identifier", but this form is deprecated;
+	use the `key::` form instead.
diff --git a/gpg-interface.c b/gpg-interface.c
index 3e7255a2a9..73554ea998 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -707,6 +707,21 @@ int git_gpg_config(const char *var, const char *value, void *cb)
 	return 0;
 }
 
+/*
+ * Returns 1 if `string` contains a literal ssh key, 0 otherwise
+ * `key` will be set to the start of the actual key if a prefix is present.
+ */
+static int is_literal_ssh_key(const char *string, const char **key)
+{
+	if (skip_prefix(string, "key::", key))
+		return 1;
+	if (starts_with(string, "ssh-")) {
+		*key = string;
+		return 1;
+	}
+	return 0;
+}
+
 static char *get_ssh_key_fingerprint(const char *signing_key)
 {
 	struct child_process ssh_keygen = CHILD_PROCESS_INIT;
@@ -714,15 +729,16 @@ static char *get_ssh_key_fingerprint(const char *signing_key)
 	struct strbuf fingerprint_stdout = STRBUF_INIT;
 	struct strbuf **fingerprint;
 	char *fingerprint_ret;
+	const char *literal_key = NULL;
 
 	/*
 	 * With SSH Signing this can contain a filename or a public key
 	 * For textual representation we usually want a fingerprint
 	 */
-	if (starts_with(signing_key, "ssh-")) {
+	if (is_literal_ssh_key(signing_key, &literal_key)) {
 		strvec_pushl(&ssh_keygen.args, "ssh-keygen", "-lf", "-", NULL);
-		ret = pipe_command(&ssh_keygen, signing_key,
-				   strlen(signing_key), &fingerprint_stdout, 0,
+		ret = pipe_command(&ssh_keygen, literal_key,
+				   strlen(literal_key), &fingerprint_stdout, 0,
 				   NULL, 0);
 	} else {
 		strvec_pushl(&ssh_keygen.args, "ssh-keygen", "-lf",
@@ -757,6 +773,7 @@ static const char *get_default_ssh_signing_key(void)
 	const char **argv;
 	int n;
 	char *default_key = NULL;
+	const char *literal_key = NULL;
 
 	if (!ssh_default_key_command)
 		die(_("either user.signingkey or gpg.ssh.defaultKeyCommand needs to be configured"));
@@ -774,7 +791,11 @@ static const char *get_default_ssh_signing_key(void)
 
 	if (!ret) {
 		keys = strbuf_split_max(&key_stdout, '\n', 2);
-		if (keys[0] && starts_with(keys[0]->buf, "ssh-")) {
+		if (keys[0] && is_literal_ssh_key(keys[0]->buf, &literal_key)) {
+			/*
+			 * We only use `is_literal_ssh_key` here to check validity
+			 * The prefix will be stripped when the key is used.
+			 */
 			default_key = strbuf_detach(keys[0], NULL);
 		} else {
 			warning(_("gpg.ssh.defaultKeyCommand succeeded but returned no keys: %s %s"),
@@ -889,19 +910,20 @@ static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature,
 	struct tempfile *key_file = NULL, *buffer_file = NULL;
 	char *ssh_signing_key_file = NULL;
 	struct strbuf ssh_signature_filename = STRBUF_INIT;
+	const char *literal_key = NULL;
 
 	if (!signing_key || signing_key[0] == '\0')
 		return error(
 			_("user.signingkey needs to be set for ssh signing"));
 
-	if (starts_with(signing_key, "ssh-")) {
+	if (is_literal_ssh_key(signing_key, &literal_key)) {
 		/* A literal ssh key */
 		key_file = mks_tempfile_t(".git_signing_key_tmpXXXXXX");
 		if (!key_file)
 			return error_errno(
 				_("could not create temporary file"));
-		keylen = strlen(signing_key);
-		if (write_in_full(key_file->fd, signing_key, keylen) < 0 ||
+		keylen = strlen(literal_key);
+		if (write_in_full(key_file->fd, literal_key, keylen) < 0 ||
 		    close_tempfile_gently(key_file) < 0) {
 			error_errno(_("failed writing ssh signing key to '%s'"),
 				    key_file->filename.buf);
diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh
index a3f285f515..6434feb6c1 100644
--- a/t/lib-gpg.sh
+++ b/t/lib-gpg.sh
@@ -91,6 +91,7 @@ GPGSSH_KEY_PRIMARY="${GNUPGHOME}/ed25519_ssh_signing_key"
 GPGSSH_KEY_SECONDARY="${GNUPGHOME}/rsa_2048_ssh_signing_key"
 GPGSSH_KEY_UNTRUSTED="${GNUPGHOME}/untrusted_ssh_signing_key"
 GPGSSH_KEY_WITH_PASSPHRASE="${GNUPGHOME}/protected_ssh_signing_key"
+GPGSSH_KEY_ECDSA="${GNUPGHOME}/ecdsa_ssh_signing_key"
 GPGSSH_KEY_PASSPHRASE="super_secret"
 GPGSSH_ALLOWED_SIGNERS="${GNUPGHOME}/ssh.all_valid.allowedSignersFile"
 
@@ -119,6 +120,8 @@ test_lazy_prereq GPGSSH '
 	echo "\"principal with number 2\" $(cat "${GPGSSH_KEY_SECONDARY}.pub")" >> "${GPGSSH_ALLOWED_SIGNERS}" &&
 	ssh-keygen -t ed25519 -N "${GPGSSH_KEY_PASSPHRASE}" -C "git ed25519 encrypted key" -f "${GPGSSH_KEY_WITH_PASSPHRASE}" >/dev/null &&
 	echo "\"principal with number 3\" $(cat "${GPGSSH_KEY_WITH_PASSPHRASE}.pub")" >> "${GPGSSH_ALLOWED_SIGNERS}" &&
+	ssh-keygen -t ecdsa -N "" -f "${GPGSSH_KEY_ECDSA}" >/dev/null
+	echo "\"principal with number 4\" $(cat "${GPGSSH_KEY_ECDSA}.pub")" >> "${GPGSSH_ALLOWED_SIGNERS}" &&
 	ssh-keygen -t ed25519 -N "" -f "${GPGSSH_KEY_UNTRUSTED}" >/dev/null
 '
 
diff --git a/t/t7528-signed-commit-ssh.sh b/t/t7528-signed-commit-ssh.sh
index badf3ed320..455eafa15c 100755
--- a/t/t7528-signed-commit-ssh.sh
+++ b/t/t7528-signed-commit-ssh.sh
@@ -73,7 +73,29 @@ test_expect_success GPGSSH 'create signed commits' '
 	git tag eleventh-signed $(cat oid) &&
 	echo 12 | git commit-tree --gpg-sign="${GPGSSH_KEY_UNTRUSTED}" HEAD^{tree} >oid &&
 	test_line_count = 1 oid &&
-	git tag twelfth-signed-alt $(cat oid)
+	git tag twelfth-signed-alt $(cat oid) &&
+
+	echo 13>file && test_tick && git commit -a -m thirteenth -S"${GPGSSH_KEY_ECDSA}" &&
+	git tag thirteenth-signed-ecdsa
+'
+
+test_expect_success GPGSSH 'sign commits using literal public keys with ssh-agent' '
+	test_when_finished "test_unconfig commit.gpgsign" &&
+	test_config gpg.format ssh &&
+	eval $(ssh-agent) &&
+	test_when_finished "kill ${SSH_AGENT_PID}" &&
+	ssh-add "${GPGSSH_KEY_PRIMARY}" &&
+	echo 1 >file && git add file &&
+	git commit -a -m rsa-inline -S"$(cat "${GPGSSH_KEY_PRIMARY}.pub")" &&
+	echo 2 >file &&
+	test_config user.signingkey "$(cat "${GPGSSH_KEY_PRIMARY}.pub")" &&
+	git commit -a -m rsa-config -S &&
+	ssh-add "${GPGSSH_KEY_ECDSA}" &&
+	echo 3 >file &&
+	git commit -a -m ecdsa-inline -S"key::$(cat "${GPGSSH_KEY_ECDSA}.pub")" &&
+	echo 4 >file &&
+	test_config user.signingkey "key::$(cat "${GPGSSH_KEY_ECDSA}.pub")" &&
+	git commit -a -m ecdsa-config -S
 '
 
 test_expect_success GPGSSH 'verify and show signatures' '
-- 
2.31.1
Fabian Stelzer· Nov 19, 2021, 15:07 UTC · re: Fabian Stelzer · lore

[PATCH v3 2/2] ssh signing: make sign/amend test more resilient

The test `amending already signed commit` is using git checkout to select a specific commit to amend. In case an earlier test fails and leaves behind a dirty index/worktree this test would fail as well. Using `checkout -f` will avoid interference by most other tests.

Signed-off-by: Fabian Stelzer <fs@gigacodes.de>
---
 t/t7510-signed-commit.sh     | 2 +-
 t/t7528-signed-commit-ssh.sh | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)
Show changes to 2 files +2 −2

t/t7510-signed-commit.sh, t/t7528-signed-commit-ssh.sh

diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
index d65a0171f2..9882b69ae2 100755
--- a/t/t7510-signed-commit.sh
+++ b/t/t7510-signed-commit.sh
@@ -228,7 +228,7 @@ test_expect_success GPG 'detect fudged signature with NUL' '
 '
 
 test_expect_success GPG 'amending already signed commit' '
-	git checkout fourth-signed^0 &&
+	git checkout -f fourth-signed^0 &&
 	git commit --amend -S --no-edit &&
 	git verify-commit HEAD &&
 	git show -s --show-signature HEAD >actual &&
diff --git a/t/t7528-signed-commit-ssh.sh b/t/t7528-signed-commit-ssh.sh
index 455eafa15c..0ec5a6d764 100755
--- a/t/t7528-signed-commit-ssh.sh
+++ b/t/t7528-signed-commit-ssh.sh
@@ -239,7 +239,7 @@ test_expect_success GPGSSH 'amending already signed commit' '
 	test_config gpg.format ssh &&
 	test_config user.signingkey "${GPGSSH_KEY_PRIMARY}" &&
 	test_config gpg.ssh.allowedSignersFile "${GPGSSH_ALLOWED_SIGNERS}" &&
-	git checkout fourth-signed^0 &&
+	git checkout -f fourth-signed^0 &&
 	git commit --amend -S --no-edit &&
 	git verify-commit HEAD &&
 	git show -s --show-signature HEAD >actual &&
-- 
2.31.1

← back to recent threads