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

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

[Fabian Stelzer]

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

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

[Taylor Blau]

On Wed, Nov 17, 2021 at 05:27:27PM +0100, Fabian Stelzer 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 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."

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

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

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

[Junio C Hamano]

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

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?

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

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

[Junio C Hamano]

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.

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

[Fabian Stelzer]

On 17.11.2021 22:39, Junio C Hamano wrote:
>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

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

[Fabian Stelzer]

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

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

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

[Fabian Stelzer]

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

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

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

[Eric Sunshine]

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.

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

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

[Fabian Stelzer]

On 18.11.2021 17:14, Eric Sunshine wrote:
>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.

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

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

[Fabian Stelzer]

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

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

[Fabian Stelzer]

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

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

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

[Fabian Stelzer]

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

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