search

Eugeny / russh

Rust SSH client & server library
https://docs.rs/russh
952 stars 106 forks source link

Using ssh-key and ssh-enconding in russh-keys #140

Open simao opened 1 year ago

simao commented 1 year ago

Hi,

There is a lot of custom code to encode/decode keys and ssh payloads in russh-keys. I think this code is error prone and might have some bugs, for example the following key, used in the russh-keys test suite:

-----BEGIN OPENSSH PRIVATE KEY-----
b3BlbnNzaC1rZXktdjEAAAAACmFlczI1Ni1jdHIAAAAGYmNyeXB0AAAAGAAAABDTR2N0uf
rcvaVVRBhwc6eaAAAAEAAAAAEAAAAzAAAAC3NzaC1lZDI1NTE5AAAAIAFQLoDASJm1+tu5
qs/5pwCJs6o4fbBjGcYZHCTImVikAAAAkGdkZqVbBNP5V5f4dnkogFK6B8recS9mRgbq6i
9coTdA7URjKPqbeUZ/Lvpp4XHtXvYKTQ2gGreU17yAzl5WSIYqQ5lK6NM8ifFkhjvfilFR
rW5M84np6Ku0LaIqlQjV/JHN5eE7aGtFGOA/WO3oO0W9eLqXdxIAm8Dntq7LLv/KeaY3EX
BlJ6/YOScK4e3qBA==
-----END OPENSSH PRIVATE KEY-----

Cannot be parsed by my version of openssh (9.2), but can be parsed with russh-key. Presumably it was generated/encoded with russh? Possibly this is an error on my side in this case, but my point is, these kind of problems are more likely to exist with a custom implementation, IMO.

There is a dependency on openssl, to use rsa keys and a few other things. To use openssl we need to use unsafe code. Moving to a rust only implementation would go towards at some point stop using openssl and maybe eventually stop using unsafe code (the cryptovec issue would also have to be addressed).

ssh keys encoding/decoding is already done by other rust only crates like https://docs.rs/ssh-key/latest/ssh_key/. That crate also supports encryption and signature verification, so we could potentially avoid using openssl and just use pure rust, even when verifying signatures and encrypting payloads. ssh-keys is maintained by the RustCrypto project which has more people working on it and there is more scrutiny over the implementation.

The api to load openssh keys using russh-key is a bit awkward to use, for example, to parse an openssh key from a string, you need to split the string with spaces and then call parse_public_key_base64. Using ssh-key would give us a better api.

We could potentially at some point reexport the ssh-key types, which would also provide better integration with other crates, that might also use it. For example, ssh-key has a serde feature that provides Serializer/Deserializer instances for keys.

To be clear, I think there is still functionality worth keeping in russh-keys, this is just about using ssh-key where it can be used and keep using russh-keys for the rest.

That all said, changing to use ssh-keys would be a lot of work, so I wanted to ask before I work on it, would you consider a PR that implement this change, or you have a strong reason to keep using russh-keys?

Eugeny commented 1 year ago

Hey, I've already looked at ssh-keys previously too and I think it would be great to integrate it! (getting rid of the openssl dependency was on the roadmap for a long time too...)

I'll be happy to merge if you can work on it :v:

Eugeny commented 1 week ago

I've now almost replaced entire key handling with ssh-key (except for PKCS#8 which they don't have yet)