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

FIPS "OpenSSL only" mode #50

Open
connor4312 opened this issue Oct 12, 2022 · 4 comments · May be fixed by #52
Open

FIPS "OpenSSL only" mode #50

connor4312 opened this issue Oct 12, 2022 · 4 comments · May be fixed by #52

Comments

@connor4312
Copy link
Contributor

One cloud on the horizon for our usage of this library is the likelihood of needing to be FIPS certifiable. Long story short, this essentially means having all cryptography done by a dynamically linked OpenSSL library--which in turn is FIPS certified. In this case, we would use OpenSSL for the cryptography currently done by crates like aes and pbkdf2.

I will (attempt to) implement this in our fork of the library, but I was wondering if it's something there'd be interest in merging upstream as a feature flag. Understanding that dependency on OpenSSL is not greatly desirable and that this would increase the complexity of the library for something a minority of consumers will ever need 😛

@Eugeny
Copy link
Member

Eugeny commented Oct 12, 2022

Not sure how viable it is given that russh already uses RustCrypto primitives for e.g. Curve25519 / GCM / HMACs :)

I'm looking to eventually move away from OpenSSL to RustCrypto for faster builds and to avoid copying memory around, but that's very far away still. If you can get dynamically liked OpenSSL as a feature flag, I'm happy to merge it for the time being!

@connor4312
Copy link
Contributor Author

I figured that was the eventual goal, and I am definitely in favor of it as a random user (having gone through build hell and back to get everything compiling on all platforms VS Code targets 😅) but sadly it seems for naught. 30m-ish in it seems like I may be able to do this with minimal surgical changes, but we'll see. Should have a PR in today or tomorrow pending unexpected hurdles.

connor4312 added a commit to microsoft/vscode-russh that referenced this issue Oct 12, 2022
Fixes warp-tech#50

This introduces an on-by-default `rs-crypto` flag, which enables the
existing Rust-based crypto libraries (including aes and ED25519).
However, these implementations can be removed by disabling the flag.
If it's disabled, then openssl (when turned on) will stand in for them,
in a less performant way.

Note that while OpenSSL 3.x does have some ED25519 support, I have not
done the work to make that compatible as well--partly because ED25519
is not yet an approved algorithm for my company to use, and partly to
retain compatibility with OpenSSL 1.x
@connor4312 connor4312 linked a pull request Oct 12, 2022 that will close this issue
connor4312 added a commit to microsoft/vscode-russh that referenced this issue Oct 13, 2022
Fixes warp-tech#50

This introduces an on-by-default `rs-crypto` flag, which enables the
existing Rust-based crypto libraries (including aes and ED25519).
However, these implementations can be removed by disabling the flag.
If it's disabled, then openssl (when turned on) will stand in for them,
in a less performant way.

Note that while OpenSSL 3.x does have some ED25519 support, I have not
done the work to make that compatible as well--partly because ED25519
is not yet an approved algorithm for my company to use, and partly to
retain compatibility with OpenSSL 1.x
@dheater
Copy link

dheater commented Dec 19, 2022

Would native-tls not be a better choice? We would be leveraging the native OS FIPs certification. Maybe I'm missing something?

@connor4312
Copy link
Contributor Author

As far as I am aware, native-tls doesn't provide the primitives needed to implement SSH

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

Successfully merging a pull request may close this issue.

3 participants