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

Allow trait-based dynamic crypto implementations #36

Merged
merged 2 commits into from Aug 14, 2019
Merged

Conversation

eoger
Copy link
Contributor

@eoger eoger commented Aug 12, 2019

This is more or less what @thomcc did in taskcluster/rust-hawk#23 and should enable us to use rust-ece without pulling openssl at all in application-services.

@@ -3,32 +3,31 @@
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file is probably where the review is needed, I've left comments to guide the review a little bit.

/// Export the key component in the
/// binary uncompressed point representation.
fn as_raw(&self) -> Result<Vec<u8>>;
/// For downcasting purposes.
fn as_any(&self) -> &Any;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cryptographer::compute_ecdh_secret takes RemotePublicKey and LocalKeyPair, but might need to downcast them to use specific implemented methods. So that's why I came up with this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

}

pub trait LocalKeyPair {
/// Generate a random local key pair.
fn generate_random() -> Result<Self>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here and below, removed the Result<Self> methods to make these traits object-safe.

@@ -81,17 +80,33 @@ impl EcKeyComponents {
}
}

pub trait Crypto: Sized {
type RemotePublicKey: RemotePublicKey;
Copy link
Contributor Author

@eoger eoger Aug 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the associated types to make Cryptographer object-safe.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: which is also why we end up with #36 (comment).

fn generate_ephemeral_keypair() -> Result<Self::LocalKeyPair>;
pub trait Cryptographer: Send + Sync + 'static {
/// Generate a random ephemeral local key pair.
fn generate_ephemeral_keypair(&self) -> Result<Box<dyn LocalKeyPair>>;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See previous comment, moved these methods to make LocalKeyPair and RemotePublicKey traits object-safe.

fn compute_ecdh_secret(
remote: &Self::RemotePublicKey,
local: &Self::LocalKeyPair,
&self,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here and below I'm making these methods take self.

) -> Result<Vec<u8>> {
let local_any = local.as_any();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That why :/

data: &[u8],
tag: &[u8],
) -> Result<Vec<u8>>;
fn random(&self, dest: &mut [u8]) -> Result<()>;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note for self:

Suggested change
fn random(&self, dest: &mut [u8]) -> Result<()>;
fn random_bytes(&self, dest: &mut [u8]) -> Result<()>;

Copy link
Contributor

@rfk rfk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks 👍 to me FWIW, although I don't feel sufficiently proficient with trait stuff to give an r+.

src/crypto/holder.rs Show resolved Hide resolved
src/crypto/mod.rs Outdated Show resolved Hide resolved
src/crypto/mod.rs Outdated Show resolved Hide resolved
src/crypto/openssl.rs Outdated Show resolved Hide resolved
Copy link

@thomcc thomcc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One nit really, which is optional. I suspect the Any could be avoided if I were a bit more familiar with what's going on, but it's probably not that bad. Glad to see this happen in either case, though.

src/aes128gcm.rs Outdated Show resolved Hide resolved
/// Export the raw components of the keypair.
fn raw_components(&self) -> Result<(EcKeyComponents)>;
/// For downcasting purposes.
fn as_any(&self) -> &Any;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunate.

@@ -122,31 +128,34 @@ mod aes128gcm_tests {
#[test]
fn test_e2e() {
let (local_key, remote_key) = generate_keys().unwrap();
let plaintext = "When I grow up, I want to be a watermelon".as_bytes();
let plaintext = b"When I grow up, I want to be a watermelon";
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Critical secret insight into eoger's psyche.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🍉

src/lib.rs Outdated Show resolved Hide resolved
@eoger eoger merged commit f95f28a into master Aug 14, 2019
@eoger eoger deleted the dyn-crypto branch August 14, 2019 17:26
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 this pull request may close these issues.

None yet

3 participants