-
Notifications
You must be signed in to change notification settings - Fork 79
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ed25519-sk support? #71
Comments
I don't think the server needs |
The server would certainly not need The public key is the just the ssh-ed25519 key, plus an application string (typically
The signature is over the following blob:
Overall, I think the implementation would be quite small, not requiring any additional cryptographic primitives or additional dependencies. If there is enough interest in this, I would volunteer. Keep in mind though that most fido2 tokens don't properly support Ed25519, the newer ones (firmware>=5.2.3 released in 2019) from a certain well-known vendor with a |
This provides an initial version of security key support in tinyssh. The main change is a new file sshcrypto_key_sk_ed25519.c, which adds the relevant functions for parsing and putting public keys and signatures, as well as the sk_ed25519_open function, that performs the signature check using the existing crypto_sign_ed25519_open for the cryptographic operation. I had to update a few places, where the code did not distinguish between server side and client side crypto algorithms, as the server cannot create a keypair or perform a signing operation, as it doesn't have a fido key available. Limitations: * No end to end test yet, as I don't have a security key with ed25519 available yet. (I did create a test vector with a colleague though.) It might be that the crypto_sign_ed25519_open doesn't put the correct result into m and mlen yet, as I wasn't able to test this yet. * [email protected] supports an application identifier, which in practice is mostly "ssh:". This commit only allows this application identifier and will not accept public keys with a different one. This is because, I didn't have a good idea yet as to where to store it. * The code doesn't allow extensions in the signature yet. (Which are not defined yet, so it doesn't really matter).
I experimented a bit with this and came up with an initial patch: https://github.com/jo-bitsch/tinyssh/tree/sk-ed25519 The main change is a new file sshcrypto_key_sk_ed25519.c, which adds I had to update a few places, where the code did not distinguish between Limitations:
|
I currently don't have a yubikey with the right firmware available, so I'd be very thankful if someone could help in testing. I have a colleague who has one, but he's on vacation, so it's gonna take a few days till I can get around to it. I'm relatively confident though, that for an initial version there are only very minor things missing. Once I was able to do that, I'll raise a PR. |
This provides an initial version of security key support in tinyssh. The main change is a new file sshcrypto_key_sk_ed25519.c, which adds the relevant functions for parsing and putting public keys and signatures, as well as the sk_ed25519_open function, that performs the signature check using the existing crypto_sign_ed25519_open for the cryptographic operation. I had to update a few places, where the code did not distinguish between server side and client side crypto algorithms, as the server cannot create a keypair or perform a signing operation, as it doesn't have a fido key available. Limitations: * [email protected] supports an application identifier, which in practice is mostly "ssh:". This commit only allows this application identifier and will not accept public keys with a different one. This is because, I didn't have a good idea yet as to where to store it. * The code doesn't allow extensions in the signature yet. (Which are not defined yet, so it doesn't really matter).
I was able to test everything and it seems to work. I therefore raised a PR (#87). Testing or code review by other people is highly appreciated. |
I think the server don't needs libfido2 or anything like that. |
I agree, the server does not need libfido2. The patch I prepared also does not add libfido2 as a dependency. It allows clients that have their ed25519 private keys saved on a (fido2) security key to log in. So it's the same cryptographic primitives, that are already present with no additional dependencies. The main difference between [email protected] and ssh-ed25519 is that the signature covers a few additional fields as security keys don't produce raw ed25519 signatures. In the end, it's a strategic decision if tinyssh wants to support keys saved in (fido2) security keys or not. From a cryptographic view, I don't see downsides. From a security point of view, having private keys on the client side in a non-extractable storage seems to be a clear positive to me. As for keeping complexity low, there is an argument to be made. But then again, the implementation (see the raised PR) is quite small and aligns with the stated goals of tinyssh in my opinion. |
I recently bought different Security Keys, and found them to be useful with OpenSSH, so I installed tinyssh with this patch applied, but I was unable to login with my keys. See below. As this patch is for ed25519-sk only, I expected the Authenton key to fail (it is ecdsa only). Still I also included that key in my tests, to compare the resulting error messages: client commands used:
resulting server messages:
|
OK, seems it my application identifiers are to blame. When exporting resident keys with |
@leapfog: Thanks for testing my patch! I guess you already identified the issue: the application identifiers. My patch currently only supports the default application identifier: How do you use other application identifiers outside of testing? Is this a priority for you, that would make this more usable in your use case? Were you able to test successfully with the default application identifier? |
I'm still new to security keys (and all their details). Right after posting my first comment I came to think that my application identifier might be the problem and created a new key pair with a default identifier. And was indeed able to login. That is what I wanted to say in my second comment. And I wanted to explain why I had used non-default values in the first place: When downloading resident keys on another system, ssh-keygen creates the key pair with names that do not allow to see from what USB-token they origin. But in case the application identifier is set, its value is automatically used as part of the filenames. So I started using the manufacturer name as application identifier. In fact, the application identifier then becomes It would be great, if you could modify your patch and accept keys whenever their application identifier starts with "ssh:" |
I'm trying to look into where to best put the application identifier ( The maximum length according to the spec is
So I'm not sure what a good size should be. @leapfog's examples would fit into 16 bytes. 32 bytes also seems nicely aligned and would with a total size of the private key of 64 bytes, which incidentally is also the size of the ed25519 private key and it gives a bit of buffer. However, these numbers seem so arbitrary... |
This extends the security key support in tinyssh to allow arbitrary application ids. The application id is part of the public key in [email protected]. This is saved after the ed25519 public key bytes in sshcrypto_key.sign_publickey. Therefore, the previous limitation of only supporting application identifier "ssh:" is resolved. Remaining limitations: * The application identifier must be smaller than 32 bytes. * The code still doesn't allow extensions in the signature yet.
@leapfog I added support for arbitrary application identifiers smaller than 32 bytes. Could you try out if this works for you? |
Not sure if this are already been ruled out because it might need a dep on
libfido2
but if not... fido2 resident keys are pretty neatoGenerate a key that lives on the actual fido2 device with:
And then when working on a new computer you can load the keys into the current session with just:
The text was updated successfully, but these errors were encountered: