Eugeny / russh

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

Consider replacing OpenSSL with pure-rust alternative #225

Closed cipriancraciun closed 3 months ago

cipriancraciun commented 10 months ago

At the moment, from what I gather, the only requirement for the OpenSSL dependency is to support the RSA algorithms, mainly in the russh-keys crate, and a bit in russh itself.

However, the main issue with OpenSSL is cross-compilation, as for example in my own project the most cross compilation issues I had was related with the OpenSSL library.

Thus, it would be nice to either replace OpenSSL with a pure Rust implementation, or have it as an alternative. For example RusTLS depends on the https://crates.io/crates/rsa crate which is written in pure Rust.


Now, given that for Ed25519 keys this project uses Rust native implementations, my assumption is that OpenSSL was an initial dependency, and thus it remains to this day until the code in question is rewritten for an alternative.

cipriancraciun commented 10 months ago

Looking at the existing pull-requests, it's somewhat the opposite of #52, that wants to rely only on OpenSSL cryptography.

I think having both options would solve opposite requirements (those wanting a pure-Rust implementation, and those wanting OpenSSL).

However, implementing both, I think would complicate the code a bit, as it would litter the code with #[cfg(...)] items...

Perhaps, a sub-crate should be created, say russh-crypto that provides a wrapper for whatever backend the user enables via features (say, OpenSSL, or pure-Rust), and having the rest of the russh* crates not touch cryptographic libraries directly.


Interestingly, comment https://github.com/warp-tech/russh/issues/50#issuecomment-1276463955 on #50, states that russh would want to move away from OpenSSL, thus perhaps this is the proper direction.

Eugeny commented 10 months ago

Yep I'd like to integrate rust-crypto's RSA impl as well and have it as the default set. Cipher modules are actually fairly pluggable in the way how russh is structured so it'd just be a single #[cfg] per module. Just haven't had time to work on it.

cipriancraciun commented 10 months ago

@Eugeny although I don't promise anything (my "hobby budget" is already in the red), if I have the time I'll try to see if maybe I can provide a pull request.

One question though: should I remove the need for OpenSSL, or should pure-Rust RSA implementation be an alternative to OpenSSL? (If there isn't a good motive to keep OpenSSL in, I would suggest removing it, as it would keep the code simpler.)

Eugeny commented 10 months ago

That would be amazing! I want to keep openSSL in, as currently russh is used in VSCode where Microsoft maintains their own fork that uses OpenSSL RSA and also adds OpenSSL AES on top of russh for FIPS compliance.

To add native rsa, you'd basically need to add alternative cfg(not(feature(openssl))) branches in key.rs like here and probably a few other files - let me know if I can help you out with more info etc

cipriancraciun commented 10 months ago

To add native rsa, you'd basically need to add alternative cfg(not(feature(openssl))) branches in key.rs like here and probably a few other files [...]

I'm accustomed with conditional compilation (many of my Rust projects heavily use this to keep the code "flexible"). And it's from that experience that I know that too many features might lead to heavy to follow code.

let me know if I can help you out with more info etc

I was looking for suitable pure Rust alternatives. Would one of the following be acceptable?

Eugeny commented 10 months ago

I wasn't trying to be condescending, just pointing out the cfgs that are related to RSA specifically :)

Using rsa would be probably easier since using ssh-key's key parser would require branching at a higher level (at decode_openssh)

cipriancraciun commented 10 months ago

I wasn't trying to be condescending, just pointing out the cfgs that are related to RSA specifically :)

Oh, don't worry, I didn't interpret it as such (i.e. condescending). :) Perhaps my initial reply was a bit too blunt in this regard (as is usually the case with written internet communication).

With regard the places where #cfg are needed, I took a peek at the code earlier today and I have a good idea where the change is needed. However if you have the time, you could point them out, perhaps I've missed (or will miss) some when implementing.

Also, if you have any other suggestions (especially with regard to code style and other requirements), please let me know. Although I expect the changes to be quite minimal and in a similar style to what already exists in the current code.


Using rsa would be probably easier since using ssh-key's key parser would require branching at a higher level (at decode_openssh)

OK, I'll try to use the rsa crate. (Although, perhaps this is for another time, couldn't russh reuse ssh-key, thus reduce the maintenance effort?)

Eugeny commented 10 months ago

I considered using ssh-key previously, but moving to it completely would nullify any chance of FIPS compliance unfortunately.

darleybarreto commented 9 months ago

Hey @cipriancraciun, would you like some help here? I have some spare time this week and more in the next, happy to help :)

cipriancraciun commented 9 months ago

@darleybarreto thanks for the offer, but unfortunately I got swamped in other tasks / projects, and this remained at the bottom of the stack.

Let me check my schedule and I'll let you know. Would you be available for a call (Meet / Discord / etc.) so we can perhaps try pair-programming on this one?

darleybarreto commented 9 months ago

Thanks for you availability! I was thinking something more asynchronous, i.e., I would open a draft PR and you could follow the progress and review it in your own time.

What do you think?

cipriancraciun commented 9 months ago

I would open a draft PR and you could follow the progress and review it in your own time.

@darleybarreto, you can create a patch and open a pull-request, and I (or anyone following this issue) could take a look. However, I can' promise anything. :)

(Also, since I'm not the maintainer of this repository, it also depends on the maintainers to finally approve and merge it.)

In essence I think we need to create two patches:

Eugeny commented 9 months ago

Yes, openssl is already optional (a feature enabled by default)

darleybarreto commented 8 months ago

I opened a PR so you can check it out, but some tests are failing.

robertabcd commented 6 months ago

I have an separate implementation ported from my other fork of thrussh. See this commit: https://github.com/robertabcd/russh/commit/a63401f47b01ef02d793c8e181fc19a49b4e5c40 It completely removes all the openssl references and passes all tests. (It's chained after my ECDSA https://github.com/warp-tech/russh/pull/267 PR, so hopefully someone will look at that first)

Eugeny commented 3 months ago

Released in 0.44