ssh signing: Support ECDSA as literal SSH keys #1272
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Welcome to GitGitGadgetHi @alindeman, and welcome to GitGitGadget, the GitHub App to send patch series to the Git mailing list from GitHub Pull Requests. Please make sure that your Pull Request has a good description, as it will be used as cover letter. You can CC potential reviewers by adding a footer to the PR description with the following syntax: Also, it is a good idea to review the commit messages one last time, as the Git project expects them in a quite specific form:
It is in general a good idea to await the automated test ("Checks") in this Pull Request before contributing the patches, e.g. to avoid trivial issues such as unportable code. Contributing the patchesBefore you can contribute the patches, your GitHub username needs to be added to the list of permitted users. Any already-permitted user can do that, by adding a comment to your PR of the form Both the person who commented An alternative is the channel Once on the list of permitted usernames, you can contribute the patches to the Git mailing list by adding a PR comment If you want to see what email(s) would be sent for a After you submit, GitGitGadget will respond with another comment that contains the link to the cover letter mail in the Git mailing list archive. Please make sure to monitor the discussion in that thread and to address comments and suggestions (while the comments and suggestions will be mirrored into the PR by GitGitGadget, you will still want to reply via mail). If you do not want to subscribe to the Git mailing list just to be able to respond to a mail, you can download the mbox from the Git mailing list archive (click the curl -g --user "<EMailAddress>:<Password>" \
--url "imaps://imap.gmail.com/INBOX" -T /path/to/raw.txtTo iterate on your change, i.e. send a revised patch or patch series, you will first want to (force-)push to the same branch. You probably also want to modify your Pull Request description (or title). It is a good idea to summarize the revision by adding something like this to the cover letter (read: by editing the first comment on the PR, i.e. the PR description): To send a new iteration, just add another PR comment with the contents: Need help?New contributors who want advice are encouraged to join git-mentoring@googlegroups.com, where volunteers who regularly contribute to Git are willing to answer newbie questions, give advice, or otherwise provide mentoring to interested contributors. You must join in order to post or view messages, but anyone can join. You may also be able to find help in real time in the developer IRC channel, |
|
There are issues in commit 4c57eb5: |
alindeman
force-pushed
the
ecdsa-sha2-keys
branch
from
4c57eb5
to
79c02d7
Compare
|
/allow |
|
User alindeman is now allowed to use GitGitGadget. |
Would you mind editing this comment? It will be sent as cover letter (for single-letter contributions, it will be inserted between the commit message and the diffstat). |
|
@dscho Sure, does that look any better? Is there a specific format I need to follow? |
Looks good to me! |
|
/preview |
|
Preview email sent as pull.1272.git.git.1653930662659.gitgitgadget@gmail.com |
Keys generated using `ssh-keygen -t ecdsa` or similar are being rejected as literal SSH keys because the prefix is `ecdsa-sha2-nistp256`, `ecdsa-sha2-nistp384` or `ecdsa-sha2-nistp521`. This was acknowledged as an issue [1] in the past, but hasn't yet been fixed. [1]: git#1041 (comment) Signed-off-by: Andy Lindeman <andy@lindeman.io>
alindeman
force-pushed
the
ecdsa-sha2-keys
branch
from
79c02d7
to
9f908c3
Compare
|
/submit |
|
Submitted as pull.1272.git.git.1653932705097.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Fabian Stelzer wrote (reply to this): On 30.05.2022 17:45, Andy Lindeman via GitGitGadget wrote:
>From: Andy Lindeman <andy@lindeman.io>
>
>Keys generated using `ssh-keygen -t ecdsa` or similar are being rejected
>as literal SSH keys because the prefix is `ecdsa-sha2-nistp256`,
>`ecdsa-sha2-nistp384` or `ecdsa-sha2-nistp521`.
>
>This was acknowledged as an issue [1] in the past, but hasn't yet been
>fixed.
Hi Andy,
thanks for your report. We have decided in the past to not explicitly cater to every key prefix and instead use `key::` for literal keys.
See https://git-scm.com/docs/git-config#Documentation/git-config.txt-usersigningKey
`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.`
>
>[1]: https://github.com/git/git/pull/1041#issuecomment-971425601
>
>Signed-off-by: Andy Lindeman <andy@lindeman.io>
>---
> ssh signing: Support ECDSA as literal SSH keys
>
> Keys generated using ssh-keygen -t ecdsa or similar will currently be
> rejected as literal SSH keys because the prefix is ecdsa-sha2-nistp256,
> ecdsa-sha2-nistp384 or ecdsa-sha2-nistp521.
>
> This was acknowledged as an issue in the past, but hasn't yet been
> fixed.
>
> https://github.com/git/git/pull/1041#issuecomment-971425601
>
>Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1272%2Falindeman%2Fecdsa-sha2-keys-v1
>Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1272/alindeman/ecdsa-sha2-keys-v1
>Pull-Request: https://github.com/git/git/pull/1272
>
> gpg-interface.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/gpg-interface.c b/gpg-interface.c
>index 280f1fa1a58..086bd03b51d 100644
>--- a/gpg-interface.c
>+++ b/gpg-interface.c
>@@ -779,7 +779,7 @@ 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-")) {
>+ if (starts_with(string, "ssh-") || starts_with(string, "ecdsa-sha2-")) {
> *key = string;
> return 1;
> }
>
>base-commit: 8ddf593a250e07d388059f7e3f471078e1d2ed5c
>-- >gitgitgadget |
|
On the Git mailing list, Andy Lindeman wrote (reply to this): On Tue, May 31, 2022 at 3:34 AM Fabian Stelzer <fs@gigacodes.de> wrote:
> On 30.05.2022 17:45, Andy Lindeman via GitGitGadget wrote:
> >From: Andy Lindeman <andy@lindeman.io>
> >
> >Keys generated using `ssh-keygen -t ecdsa` or similar are being rejected
> >as literal SSH keys because the prefix is `ecdsa-sha2-nistp256`,
> >`ecdsa-sha2-nistp384` or `ecdsa-sha2-nistp521`.
> >
> >This was acknowledged as an issue [1] in the past, but hasn't yet been
> >fixed.
>
> Hi Andy,
> thanks for your report. We have decided in the past to not explicitly cater
> to every key prefix and instead use `key::` for literal keys.
> See
> https://git-scm.com/docs/git-config#Documentation/git-config.txt-usersigningKey
>
> `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.`
Thanks for replying, Fabian.
My main issue is that ecdsa-sha2-* keys currently seem incompatible
with `gpg.ssh.defaultKeyCommand = "ssh-add -L"`
The git-config documentation of `gpg.ssh.defaultKeyCommand` says:
> To automatically use the first available key from your ssh-agent set this to "ssh-add -L".
But this does not work with ecdsa keys because each line of the output
of the command is checked against `is_literal_ssh_key`. Because of
that check, keys that do not begin with `ssh-` are skipped.
I could certainly write my own shell script for `defaultKeyCommand`
that did something like `ssh-add -L | sed 's/^/key::/'` but it's a bit
awkward.
The code that runs `defaultKeyCommand` states:
> /*
> * We only use `is_literal_ssh_key` here to check validity
> * The prefix will be stripped when the key is used.
> */
but this is clearly not true because it is rejecting valid SSH keys.
Do you have thoughts on how to improve `gpg.ssh.defaultKeyCommand` for
keys whose prefix is not `ssh-` ? |
|
On the Git mailing list, Fabian Stelzer wrote (reply to this): On 31.05.2022 09:28, Andy Lindeman wrote:
>On Tue, May 31, 2022 at 3:34 AM Fabian Stelzer <fs@gigacodes.de> wrote:
>> On 30.05.2022 17:45, Andy Lindeman via GitGitGadget wrote:
>> >From: Andy Lindeman <andy@lindeman.io>
>> >
>> >Keys generated using `ssh-keygen -t ecdsa` or similar are being rejected
>> >as literal SSH keys because the prefix is `ecdsa-sha2-nistp256`,
>> >`ecdsa-sha2-nistp384` or `ecdsa-sha2-nistp521`.
>> >
>> >This was acknowledged as an issue [1] in the past, but hasn't yet been
>> >fixed.
>>
>> Hi Andy,
>> thanks for your report. We have decided in the past to not explicitly cater
>> to every key prefix and instead use `key::` for literal keys.
>> See
>> https://git-scm.com/docs/git-config#Documentation/git-config.txt-usersigningKey
>>
>> `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.`
>
>Thanks for replying, Fabian.
>
>My main issue is that ecdsa-sha2-* keys currently seem incompatible
>with `gpg.ssh.defaultKeyCommand = "ssh-add -L"`
>
>The git-config documentation of `gpg.ssh.defaultKeyCommand` says:
>
>> To automatically use the first available key from your ssh-agent set this to "ssh-add -L".
>
>But this does not work with ecdsa keys because each line of the output
>of the command is checked against `is_literal_ssh_key`. Because of
>that check, keys that do not begin with `ssh-` are skipped.
True, this is a bug.
>
>I could certainly write my own shell script for `defaultKeyCommand`
>that did something like `ssh-add -L | sed 's/^/key::/'` but it's a bit
>awkward.
I think this is at least a valid workaround for now.
>
>The code that runs `defaultKeyCommand` states:
>
>> /*
>> * We only use `is_literal_ssh_key` here to check validity
>> * The prefix will be stripped when the key is used.
>> */
>
>but this is clearly not true because it is rejecting valid SSH keys.
>
>Do you have thoughts on how to improve `gpg.ssh.defaultKeyCommand` for
>keys whose prefix is not `ssh-` ?
The problem is that we do not want to maintain all ssh keytypes in the git code. Thats why the `key::` was added.
I'll have to think what we could do besides just skipping the check completely and just assuming the defaultKeyCommand will return a valid key.
ssh-add -L is not necessarily defined as having a parsable output and any additional messages it might print (or some pkcs11 provider) would at least be skipped with the ssh- prefix check. |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Fabian Stelzer <fs@gigacodes.de> writes:
>>Thanks for replying, Fabian.
>>
>>My main issue is that ecdsa-sha2-* keys currently seem incompatible
>>with `gpg.ssh.defaultKeyCommand = "ssh-add -L"`
>>
>>The git-config documentation of `gpg.ssh.defaultKeyCommand` says:
>>
>>> To automatically use the first available key from your ssh-agent set this to "ssh-add -L".
This is puzzling. One chooses the key to use when signing, and the
key should go to the gpg.ssh.defaultkey, and also "ssh-add" is told
about the key for convenient access. Asking "ssh-add -L" about the
keys it knows about and randomly pick the first one it happens to
tell you about sounds totally backwards to me.
I may have a key I use to sign, and one key each to go to various
destinations, all of which "ssh-add -L" may know about. It alone
cannot fundamentally tell because it does not know what you intend
to use each key for.
Of course, as your own custom script, defaultKeyCommand may know
which keys you intend to use for connecting and which keys you
intend to use for signing. It may even need to know which key you
intend to use for each project you work with and your .git/config
may have something to tell the script what "trait" the key to be
used that appear in "ssh-add -L" output should have (perhaps the key
is rotated very often so you cannot write the exact key in your
configuration, but perhaps the comment at the end of each line have
sufficient cue to tell them apart). So, the custom script would
need to go line by line to find the key to use in the first place,
and if it is computationally capable enough to do so, it should be
easy to prefix key:: in front. IIRC, we designed the system in such
a way that it is not an error to prefix key:: in front of ssh-* keys.
In any case, perhaps we should extend the documentation a bit. It
generally is not sensible to just use "ssh-add -L" and pick one
random key out of it, so we shouldn't be encouraging such a use, I
suspect.
|
|
On the Git mailing list, Fabian Stelzer wrote (reply to this): On 01.06.2022 00:05, Junio C Hamano wrote:
>Fabian Stelzer <fs@gigacodes.de> writes:
>
>>>Thanks for replying, Fabian.
>>>
>>>My main issue is that ecdsa-sha2-* keys currently seem incompatible
>>>with `gpg.ssh.defaultKeyCommand = "ssh-add -L"`
>>>
>>>The git-config documentation of `gpg.ssh.defaultKeyCommand` says:
>>>
>>>> To automatically use the first available key from your ssh-agent set this to "ssh-add -L".
>
>This is puzzling. One chooses the key to use when signing, and the
>key should go to the gpg.ssh.defaultkey, and also "ssh-add" is told
>about the key for convenient access.
I think you mean `user.siningKey` but yes, this is the best way to do this.
> Asking "ssh-add -L" about the
>keys it knows about and randomly pick the first one it happens to
>tell you about sounds totally backwards to me.
>
>I may have a key I use to sign, and one key each to go to various
>destinations, all of which "ssh-add -L" may know about. It alone
>cannot fundamentally tell because it does not know what you intend
>to use each key for.
>
>Of course, as your own custom script, defaultKeyCommand may know
>which keys you intend to use for connecting and which keys you
>intend to use for signing. It may even need to know which key you
>intend to use for each project you work with and your .git/config
>may have something to tell the script what "trait" the key to be
>used that appear in "ssh-add -L" output should have (perhaps the key
>is rotated very often so you cannot write the exact key in your
>configuration, but perhaps the comment at the end of each line have
>sufficient cue to tell them apart). So, the custom script would
>need to go line by line to find the key to use in the first place,
>and if it is computationally capable enough to do so, it should be
>easy to prefix key:: in front. IIRC, we designed the system in such
>a way that it is not an error to prefix key:: in front of ssh-* keys.
>
>In any case, perhaps we should extend the documentation a bit. It
>generally is not sensible to just use "ssh-add -L" and pick one
>random key out of it, so we shouldn't be encouraging such a use, I
>suspect.
Yes, I think that reasonable. The script can do some advanced decision making / key lookup if needed. The use-case for me was to enforce/encourage use of the correct users keys on a shared development server in a corporate environment (i have a global directory of all the users keys and want to make sure everyone uses their correct one when signing).
I'll take a look at the docs and suggest a patch in a bit.
|
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Fabian Stelzer <fs@gigacodes.de> writes:
> On 01.06.2022 00:05, Junio C Hamano wrote:
>>Fabian Stelzer <fs@gigacodes.de> writes:
>>
>>>>Thanks for replying, Fabian.
>>>>
>>>>My main issue is that ecdsa-sha2-* keys currently seem incompatible
>>>>with `gpg.ssh.defaultKeyCommand = "ssh-add -L"`
>>>>
>>>>The git-config documentation of `gpg.ssh.defaultKeyCommand` says:
>>>>
>>>>> To automatically use the first available key from your ssh-agent set this to "ssh-add -L".
>>
>>This is puzzling. One chooses the key to use when signing, and the
>>key should go to the gpg.ssh.defaultkey, and also "ssh-add" is told
>>about the key for convenient access.
>
> I think you mean `user.siningKey` but yes, this is the best way to do this.
Thanks for seeing my intention through my mistake.
>> Asking "ssh-add -L" about the
>>keys it knows about and randomly pick the first one it happens to
>>tell you about sounds totally backwards to me.
>>
>>I may have a key I use to sign, and one key each to go to various
>>destinations, all of which "ssh-add -L" may know about. It alone
>>cannot fundamentally tell because it does not know what you intend
>>to use each key for.
>> ...
>>In any case, perhaps we should extend the documentation a bit. It
>>generally is not sensible to just use "ssh-add -L" and pick one
>>random key out of it, so we shouldn't be encouraging such a use, I
>>suspect.
>
> Yes, I think that reasonable. The script can do some advanced decision
> making / key lookup if needed.
OK.
> The use-case for me was to enforce/encourage use of the correct
> users keys on a shared development server in a corporate
> environment (i have a global directory of all the users keys and
> want to make sure everyone uses their correct one when signing).
I actually wanted to hear more about the reasoning along that line.
IOW, "sure, theoretically, you should start from 'this is the key I
want to use' and you shouldn't be asking 'ssh-add -L' about it, but
here is a real-world workflow that makes it cumbersome" was what I
wanted to see, both in the discussion *and* in the documentation
update.
For example, there may be corporate environment where key is
frequently rotated, e.g. every morning an employee may have to "corp
login" to talk to a central key server and get the ssh key stored in
their hardware token refreshed. In such an environment, it would
not be surprising if the employee does not even know what the
fingerprint or the public part of the key looks like before asking
'ssh-add -L' to query the hardware token, so it may be impractical
to follow the "set your key to user.signingKey and add that to the
agent". Asking the agent about the key may make perfect sense (but
you'd probably need to find which key among its output lines) in
such a case.
|
|
On the Git mailing list, Fabian Stelzer wrote (reply to this): Using `ssh-add -L` for gpg.ssh.defaultKeyCommand is not a good
recommendation. It might switch keys depending on the order of known
keys and it only supports ssh-* and no ecdsa or other keys.
Clarify that we expect a literal key prefixed by `key::`, give valid
example use cases and refer to `user.signingKey` as the preferred
option.
Signed-off-by: Fabian Stelzer <fs@gigacodes.de>
---
Documentation/config/gpg.txt | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/Documentation/config/gpg.txt b/Documentation/config/gpg.txt
index 86892ada77..86f6308c4c 100644
--- a/Documentation/config/gpg.txt
+++ b/Documentation/config/gpg.txt
@@ -36,9 +36,12 @@ gpg.minTrustLevel::
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".
+ signature is requested. On successful exit a valid ssh public key
+ prefixed with `key::` is expected in the first line of its output.
+ This allows for a script doing a dynamic lookup of the correct public
+ key when it is impractical to statically configure `user.signingKey`.
+ For example when keys or SSH Certificates are rotated frequently or
+ selection of the right key depends on external factors unknown to git.
gpg.ssh.allowedSignersFile::
A file containing ssh public keys which you are willing to trust.
--
2.35.3
|
|
On the Git mailing list, Andy Lindeman wrote (reply to this): On Wed, Jun 8, 2022 at 11:24 AM Fabian Stelzer <fs@gigacodes.de> wrote:
>
> Using `ssh-add -L` for gpg.ssh.defaultKeyCommand is not a good
> recommendation. It might switch keys depending on the order of known
> keys and it only supports ssh-* and no ecdsa or other keys.
> Clarify that we expect a literal key prefixed by `key::`, give valid
> example use cases and refer to `user.signingKey` as the preferred
> option.
>
> Signed-off-by: Fabian Stelzer <fs@gigacodes.de>
> ---
> Documentation/config/gpg.txt | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/config/gpg.txt b/Documentation/config/gpg.txt
> index 86892ada77..86f6308c4c 100644
> --- a/Documentation/config/gpg.txt
> +++ b/Documentation/config/gpg.txt
> @@ -36,9 +36,12 @@ gpg.minTrustLevel::
>
> 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".
> + signature is requested. On successful exit a valid ssh public key
> + prefixed with `key::` is expected in the first line of its output.
> + This allows for a script doing a dynamic lookup of the correct public
> + key when it is impractical to statically configure `user.signingKey`.
> + For example when keys or SSH Certificates are rotated frequently or
> + selection of the right key depends on external factors unknown to git.
>
> gpg.ssh.allowedSignersFile::
> A file containing ssh public keys which you are willing to trust.
> --
> 2.35.3
>
Nice. This makes sense to me. |
|
User |
|
If not too late, may I suggest to extend this patch to recognize diff --git a/gpg-interface.c b/gpg-interface.c
index 280f1fa1a58233..086bd03b51d6a7 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -779,7 +779,7 @@ 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-")) {
+ if (starts_with(string, "ssh-") || starts_with(string, "sk-ssh-") || starts_with(string, "ecdsa-sha2-")) {
*key = string;
return 1;
} |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Keys generated using
ssh-keygen -t ecdsaor similar will currently be rejected as literal SSH keys because the prefix isecdsa-sha2-nistp256,ecdsa-sha2-nistp384orecdsa-sha2-nistp521.This was acknowledged as an issue in the past, but hasn't yet been fixed.
#1041 (comment)
CC: Fabian Stelzer fs@gigacodes.de
cc: Andy Lindeman andy@lindeman.io