russh icon indicating copy to clipboard operation
russh copied to clipboard

Consider replacing OpenSSL with pure-rust alternative

Open cipriancraciun opened this issue 1 year ago • 15 comments

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 avatar Jan 10 '24 16:01 cipriancraciun

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.

cipriancraciun avatar Jan 10 '24 16:01 cipriancraciun

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.

Eugeny avatar Jan 10 '24 16:01 Eugeny

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

cipriancraciun avatar Jan 10 '24 17:01 cipriancraciun

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

Eugeny avatar Jan 10 '24 18:01 Eugeny

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?

  • https://crates.io/crates/rsa (part of the RustCrypto project); (this one is currently vulnerable to the "Marvin attack", but I'm sure they'll fix it soon;)
  • https://crates.io/crates/ssh-key, also from the RustCrypto project, that seems to implement some utility functions specifically for SSH; (might help reduce the code;) (this one relies upon the previous rsa one;)

cipriancraciun avatar Jan 10 '24 18:01 cipriancraciun

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)

Eugeny avatar Jan 10 '24 18:01 Eugeny

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

cipriancraciun avatar Jan 10 '24 19:01 cipriancraciun

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

Eugeny avatar Jan 10 '24 19:01 Eugeny

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

darleybarreto avatar Feb 07 '24 14:02 darleybarreto

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

cipriancraciun avatar Feb 07 '24 19:02 cipriancraciun

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?

darleybarreto avatar Feb 08 '24 10:02 darleybarreto

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:

  • one to make the OpenSSL dependency optional via Rust features; (maybe this is already supported? I can't remember at this moment;)
  • one adding support (in addition to OpenSSL) for a pure-Rust RSA implementation; (see previous comments about the suggested library);

cipriancraciun avatar Feb 09 '24 11:02 cipriancraciun

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

Eugeny avatar Feb 09 '24 15:02 Eugeny

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

darleybarreto avatar Feb 17 '24 14:02 darleybarreto

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)

robertabcd avatar Apr 22 '24 00:04 robertabcd

Released in 0.44

Eugeny avatar Aug 04 '24 09:08 Eugeny