DRAFT: Pluggable crypto #462
Conversation
There was a problem hiding this comment.
Took a quick look, some comments
- I think Object safety is pretty important, given the computed types are fixed width it should be possible to use
[u8; 32]or similar to avoid allocations (although given the operations involved an allocation is likely irrelevant) - We should avoid changing the signature of
newas this will break lots of code, instead we could add a new constructor, e.g. new_with_crypto or something
src/crypto.rs
Outdated
| type DynError = Box<dyn std::error::Error + Send + Sync>; | ||
|
|
||
| #[derive(Debug)] | ||
| pub struct Error { |
There was a problem hiding this comment.
I think it'd be better to just use object_store::Error, whilst it contains variants that aren't relevant, I don't think this matters
src/crypto.rs
Outdated
| pub trait CryptoProvider: Send + Sync + Debug + 'static { | ||
| fn digest_sha256(bytes: &[u8]) -> Result<impl AsRef<[u8]>, Error>; | ||
| fn hmac_sha256(secret: &[u8], bytes: &[u8]) -> Result<impl AsRef<[u8]>, Error>; | ||
| fn hex_digest(bytes: &[u8]) -> Result<String, Error> { |
There was a problem hiding this comment.
Probably don't need this to be here unless you expect people to override it
| } | ||
| } | ||
|
|
||
| pub mod openssl_crypto { |
There was a problem hiding this comment.
I would personally prefer people to add additional providers out of tree if they need them. I'm not massively excited about adding a dependency on openssl, optional or not.
There was a problem hiding this comment.
Yep, will take this out - Was just trying out a couple of providers to see how they compare and if the trait was flexible enough
|
@tustvold I have a pretty complete implementation here. Other than more docs and bits of cleanup along those lines, the last major bit I haven't accounted for is the Rsa key stuff for GCP credentials. This is (today) specific to GCP and appears at first glance to be quite a bit more complicated to put into an object safe trait due to the requirement to create an intermediate I'm assuming we'd want to also encapsulate that into the trait, but I just want to raise it for discussion. Maybe there's a straightforward way to do this that I don't know, I only have a few months experience with rust. Usually I'd have something in the interface that returns Let me know what your thoughts are and thanks again for working with me on this! I know it's a lot of mental overhead, so I appreciate it! |
JakeDern
force-pushed
the
pluggable-crypto
branch
from
e6ee69b to
624c3e3
Compare
2 participants
Which issue does this PR close?
Closes #413.
Rationale for this change
Many companies, including my current employer, are picky about what cryptographic implementations may be used. This PR seeks to accomplish two things:
What changes are included in this PR?
This is heavily WIP - I'm just opening this to make it clear how the approach with generic types would work vs an implementation that would use
dyninstead at the expense of having to allocate and copy the outputs of various crypto implementations.CryptoProvidertraitRingProviderimplementation based on theringfeatureAre there any user-facing changes?
Yes, several including:
default::Defaultimplementations dependent on theringfeaturenew()require a crypto provider