Skip to content
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

Consider replacing OpenSSL with pure-rust alternative #225

Closed
cipriancraciun opened this issue Jan 10, 2024 · 16 comments
Closed

Consider replacing OpenSSL with pure-rust alternative #225

cipriancraciun opened this issue Jan 10, 2024 · 16 comments

Comments

@cipriancraciun
Copy link
Contributor

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
Copy link
Contributor Author

cipriancraciun commented Jan 10, 2024

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 #50 (comment) on #50, states that russh would want to move away from OpenSSL, thus perhaps this is the proper direction.

@Eugeny
Copy link
Owner

Eugeny commented Jan 10, 2024

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
Copy link
Contributor Author

@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
Copy link
Owner

Eugeny commented Jan 10, 2024

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
Copy link
Contributor Author

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

@Eugeny
Copy link
Owner

Eugeny commented Jan 10, 2024

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
Copy link
Contributor Author

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
Copy link
Owner

Eugeny commented Jan 10, 2024

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

@darleybarreto
Copy link

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

@cipriancraciun
Copy link
Contributor Author

@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
Copy link

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
Copy link
Contributor Author

cipriancraciun commented Feb 9, 2024

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

@Eugeny
Copy link
Owner

Eugeny commented Feb 9, 2024

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

@darleybarreto
Copy link

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

@robertabcd
Copy link
Contributor

I have an separate implementation ported from my other fork of thrussh. See this commit: robertabcd@a63401f
It completely removes all the openssl references and passes all tests.
(It's chained after my ECDSA #267 PR, so hopefully someone will look at that first)

Eugeny pushed a commit that referenced this issue Apr 28, 2024
* SSH encoding of RSA keys is moved into `protocol` module.
* Decouple the SSH encoding into traits.
* When `openssl` feature is not enabled, the pure-rust RSA impl is used.

Alternative implementation for
#225
@Eugeny
Copy link
Owner

Eugeny commented Aug 4, 2024

Released in 0.44

@Eugeny Eugeny closed this as completed Aug 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants