add pluggable crypto provider - #450
Conversation
|
Thanks for picking this up.
I think we should probably model this like rustls @djc Are we on the same page there? |
djc
left a comment
There was a problem hiding this comment.
Thanks! This looks directionally right to me.
| fn load_private_key( | ||
| &self, | ||
| key_der: PrivateKeyDer<'static>, | ||
| algorithm: Option<&'static SignatureAlgorithm>, |
There was a problem hiding this comment.
i think this is for compatibility with from_der_and_sign_algo. the old KeyPair was already bound to SignatureAlgorithim. this could be changed to have the signing algorithim desicion happen at the time of actual signing though.
hey! thanks for the quick review. just before i change anything, are you in agreement with this as well @djc? |
|
I agree that we should not have a default provider or enable any provider by default, and API that needs it should take one explicitly. I don't think it's worth splitting the providers out into separate crates at this point? We're not close to API stability and I feel like rcgen is much less likely than rustls to end up deeper in dependency graphs. |
That's fine with me 👍 |
| public_key: &impl PublicKeyData, | ||
| public_key: &(impl PublicKeyData + ?Sized), | ||
| issuer: &Issuer<'_, impl SigningKey>, | ||
| #[cfg(feature = "crypto")] provider: &dyn CryptoProvider, |
There was a problem hiding this comment.
I think we've kind of made a mess of this already with the crypto feature but I'd like to avoid making more pub API surface that has feature-gated params/fields. Unification makes it unpleasant for consumers who can have working code fail to compile after adding a dep that activates the feature under them.
There was a problem hiding this comment.
Agreed. Also given the existence of an impl SigningKey there must be a CryptoProvider in play somewhere?
Feels to me like we should just get rid of the crypto feature in this PR because the crypto dependencies are instead abstracted via traits, and the aws-lc-rs and ring features should in principle only be used for their respective provider implementations?
| public_key: &impl PublicKeyData, | ||
| public_key: &(impl PublicKeyData + ?Sized), | ||
| issuer: &Issuer<'_, impl SigningKey>, | ||
| #[cfg(feature = "crypto")] provider: &dyn CryptoProvider, |
There was a problem hiding this comment.
Agreed. Also given the existence of an impl SigningKey there must be a CryptoProvider in play somewhere?
Feels to me like we should just get rid of the crypto feature in this PR because the crypto dependencies are instead abstracted via traits, and the aws-lc-rs and ring features should in principle only be used for their respective provider implementations?
| - run: cargo clippy --no-default-features --features aws_lc_rs_unstable,pem,x509-parser --all-targets | ||
| - run: cargo clippy --no-default-features --features aws_lc_rs --all-targets | ||
| - run: cargo clippy --no-default-features --features fips,pem,x509-parser --all-targets | ||
| - run: cargo clippy --no-default-features --features aws_lc_rs,fips,pem,x509-parser --all-targets |
There was a problem hiding this comment.
i coupled aws_lc_rs + fips together as flags in this PR, but it's ultimately irrelevant to the actual provider abstration. since we're going to rewrite that in the future anyways, should i just revert to previous behaviour?
There was a problem hiding this comment.
I went ahead and removed this for now, I'm not sure if the way the flag works right now is how it's intended to?
| fn generate( | ||
| &self, | ||
| algorithm: &'static SignatureAlgorithm, | ||
| key_size: Option<RsaKeySize>, |
There was a problem hiding this comment.
Instead of passing in a separate key size, suggest we first (in a separate PR) change it so there are separate SignatureAlgorithm values for different RSA key sizes.
There was a problem hiding this comment.
Sure, happy to make a PR for this
| #[cfg(all( | ||
| test, | ||
| feature = "x509-parser", | ||
| any(feature = "ring", feature = "aws_lc_rs") |
There was a problem hiding this comment.
yeah, there are some other lines where this was added too. took those out as well
There was a problem hiding this comment.
the test module in certificate.rs pulls in a provider through crate::test_provider(), so we actually do have to keep this.
| &self, | ||
| writer: &mut DERWriterSeq, | ||
| pub_key_spki: Option<&[u8]>, | ||
| provider: Option<&dyn CryptoProvider>, |
There was a problem hiding this comment.
The separate Options for pub_key_spki and provider don't make sense.
There was a problem hiding this comment.
do you mean combining them into one parameter? the only thing with that is that it's different from the shape of the other functions
| }) | ||
| } | ||
|
|
||
| fn validate(&self, issuer: &Issuer<'_, impl SigningKey>) -> Result<(), Error> { |
There was a problem hiding this comment.
Why move this into a separate method?
If you're only going to get your LLM to iterate on this PR we might as well stop reviewing this here, or your employer can pay me for my time.
There was a problem hiding this comment.
Yeah, that change doesn't really make sense, will revert. I try to review the commits, but clearly some nonsensical changes slipped in. After reviewing the rest of your comments, I'll go through and make sure there aren't any more unrelated additions here, sorry about that. Thanks so much for the reviews.
There was a problem hiding this comment.
It is really not great that you're getting volunteers to review something that you haven't even reviewed yourself.
|
Reverted many of the unnecessary changes, remaining edits should be only crypto provider related Changes:
|
| } | ||
|
|
||
| /// Obtains the key pair from a DER formatted key | ||
| /// Obtains the key pair from a PEM formatted key |
There was a problem hiding this comment.
this change isn't relevant, but i do think this comment is wrong? (copy pasted from from_pkcs8_der_and_sign_algo maybe). but could break this change off into another PR
ec38547 to
c08332c
Compare
Adds a pluggable cryptography provider API to rcgen, allowing applications to use custom backends without depending on Ring or AWS-LC. This design is inspired by rustls and jsonwebtoken.
This PR is a follow up on #414. The goal is to support our own external SymCrypt provider crate.
Ring and AWS-LC remain available through built-in provider implementations.
CryptoProvidertrait providing:KeyPairbackend-neutral by storing aSigningKeytrait object instead of concrete Ring/AWS-LC key types.CryptoProvider::hash.CryptoProvider::verifyinstead ofx509-parsercrypto features.Built-in providers must now be enabled and selected explicitly:
ringfeature exposes the built-in Ring provider.aws_lc_rsfeature exposes the built-in AWS-LC provider.Providers are passed directly to APIs:
Resolves #228