-
Notifications
You must be signed in to change notification settings - Fork 324
Expose cryptography backends via CryptoProvider #452
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
base: master
Are you sure you want to change the base?
Conversation
4483fd7 to
370f904
Compare
|
@Keats any chance of a review on this? |
|
Would this allow applications to use rustls rather than openssl? |
|
@drusellers As far as I know rustls doesn't implement the cryptography directly, rather it relies on other crates like |
Keats
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can be simplified
| Algorithm::PS384 => Box::new(rsa::RsaPss384Signer::new(key)?) as Box<dyn JwtSigner>, | ||
| Algorithm::PS512 => Box::new(rsa::RsaPss512Signer::new(key)?) as Box<dyn JwtSigner>, | ||
| Algorithm::EdDSA => Box::new(eddsa::EdDSASigner::new(key)?) as Box<dyn JwtSigner>, | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that will get more complex if we start supporting some alg in some backend like #461
| pub(crate) fn get_default() -> Option<&'static Arc<CryptoProvider>> { | ||
| PROCESS_DEFAULT_PROVIDER.get() | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need that? We should be able to define a DEFAULT_PROVIDER using cargo features I think without needing the dance around that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC you mean that it automatically sets PROCESS_DEFAULT_PROVIDER to aws_lc::CryptoProvider if the aws feature is enabled? Yes that could be done, but similar to the JWK utility functions, that would mean you have to replace everything when you implement a custom provider, because otherwise if you select one of the built in features, you'd be locked into that provider.
This way you can implement whatever you want to, enable both aws and custom-provider, and fallback to aws for everything you haven't implemented.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding of custom providers is that you do not want to add the rust-crypto/aws-lc dependencies? That feels a bit awkward otherwise
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel that way, yes, others may not, I do not know. I did it this way because rustls did it this way, but I can change it if you want me to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All that said while it could be assigned automatically for the rust_crypto and aws_lc_rs features, it still needs to provide a set method for outside crypto providers, and I'm honestly not sure how to do it any other way.
Unless you want a
#[cfg(feature = "custom-provider")]
static PROCESS_DEFAULT_PROVIDER: OnceLock<Arc<CryptoProvider>> = OnceLock::new();
#[cfg(not(feature = "custom-provider")]
# assign from crate featureswhich I personally think is pretty ugly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the way i was thinking about it is that people would create crates like jsonwebtoken-ring, jsonwebtoken-botan that are self contained
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a fan of that to be honest. I can get behind separate crates as "companion crates" for jsonwebtoken, but doing a passthrough crate is imo a bad idea.
If my project requires two crates that both depend on jsonwebtoken, but use different implementations, i.e. jsonwebtoken-botan and jsonwebtoken-ring, I need to compile two crypto backends, and more importantly I need to trust two crypto backends.
I'd much rather they both require the base jsonwebtoken, and let me select the crypto provider at my crate level, be it a builtin one or one of the custom ones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not familiar with that pattern of selecting providers like apparently rustls does.
If my project requires two crates that both depend on jsonwebtoken, but use different implementations, i.e. jsonwebtoken-botan and jsonwebtoken-ring, I need to compile two crypto backends, and more importantly I need to trust two crypto backends.
I'd much rather they both require the base jsonwebtoken, and let me select the crypto provider at my crate level, be it a builtin one or one of the custom ones.
What does it look like from the pov of let's say jsonwebtoken-botan implementer and user?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
38698b3 to
3344881
Compare
|
@Keats I made the JWK functions private after all. For the In regards to the macro, I also left it as is, if #461 goes through and does gate the implementation itself behind a feature, I guess the best would be to remove the macro and write the functions by hand for each built-in provider. As for the example, I used the |
| pub extract_ec_public_key_coordinates: | ||
| fn(&[u8], Algorithm) -> Result<(EllipticCurve, Vec<u8>, Vec<u8>)>, | ||
| /// Given some data and a name of a hash function, compute hash_function(data) | ||
| pub compute_digest: fn(&[u8], ThumbprintHash) -> Vec<u8>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems I forgot to bring this up before, but it would really be great if we could do a breaking change to turn this into Result<Vec<u8>>. Everything else lets you return an error should it happen in your custom provider (e.g. botan can technically error here, because it does go through an FFI, and while it shouldn't error, I still need to call unwrap here)
Adds a
CryptoProviderstruct that allows replacing the built-in providers with something custom.All the details from this implementation that could be considered "interesting" are stolen straight from
rustls's CryptoProvider.I've marked the
new_signer,new_verifierand JWK functions from the two built in backends aspub, so you can do stuff like this:i.e. overwrite just specific algorithms.
One area I'm a little unsure about is
JwkUtils, 1) about the name and 2) about theDefaultimplementation. TheCryptoProvider::signer_andCryptoProvider::verifier_factoryfunctions are obviously mandatory for a custom provider, but not everyone uses JWK, so the default just uses dummy functions withunimplemented!().