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

RustCrypto implementation of DPE crypto. #287

Merged
merged 2 commits into from
Dec 14, 2023
Merged

Conversation

esmusick
Copy link
Contributor

@esmusick esmusick commented Dec 14, 2023

Adds a RustCrypto implementation as an alternative to openssl. Openssl is set as the default, and for platform/simulator the two are mutually exclusive.

For the Crypto trait impl, I generally did not use generics across curves even though it is possible, because the trait bounds needed were often longer than the actual shared logic.
There is some shared logic with the openssl impl using the hkdf crate, which I've moved into a shared module.

Adds a RustCrypto implementation as an alternative to openssl.
Openssl is set as the default, and for platform/simulator the two are
mutually exclusive.

For the Cryto trait impl, I generally did not use generics across curves even
though it is possible, because the trait bounds needed were often longer than
the actual shared logic.
There is some shared logic with the openssl impl using the hkdf crate,
which I've moved into a shared module.
@esmusick esmusick marked this pull request as ready for review December 14, 2023 20:27
jhand2
jhand2 previously approved these changes Dec 14, 2023
Copy link
Collaborator

@jhand2 jhand2 left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

Most builds/test run with default openssl, but the full rustcrypto impl
is exercised with the verification tests.
The cfg_if usage means rustcrypto and openssl are no longer mututally
exclusive for the default platform.
@jhand2 jhand2 merged commit 229cafb into chipsalliance:main Dec 14, 2023
1 check passed
Copy link
Contributor

@zhalvorsen zhalvorsen left a comment

Choose a reason for hiding this comment

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

Nice job! This is awesome!


#[cfg(feature = "openssl")]
pub use crate::openssl::*;
pub use signer::*;

#[cfg(feature = "rustcrypto")]
pub use crate::rustcrypto::*;
pub use signer::*;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this line is duplicated

Suggested change
pub use signer::*;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sent #288

crypto/src/rustcrypto.rs Show resolved Hide resolved
Comment on lines +37 to +79
impl DefaultPlatform {
cfg_if! {
if #[cfg(feature = "openssl")] {
fn parse_issuer_name() -> Vec<u8> {
X509::from_pem(TEST_CERT_PEM)
.unwrap()
.subject_name()
.to_der()
.unwrap()
}} else if #[cfg(feature = "rustcrypto")] {
fn parse_issuer_name() -> Vec<u8> {
Certificate::from_pem(TEST_CERT_PEM)
.unwrap()
.tbs_certificate
.subject
.to_der()
.unwrap()
}
}}

cfg_if! {
if #[cfg(feature = "openssl")] {
fn parse_issuer_sn() -> Vec<u8> {
X509::from_pem(TEST_CERT_PEM)
.unwrap()
.serial_number()
.to_bn()
.unwrap()
.to_vec()
}
} else if #[cfg(feature = "rustcrypto")] {
fn parse_issuer_sn() -> Vec<u8> {
Certificate::from_pem(TEST_CERT_PEM)
.unwrap()
.tbs_certificate
.serial_number
.as_bytes()
.to_vec()
}
}
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

optional: this might just be personal preference, but I think if they are separated into distinct modules it might be a little more readable.

Suggested change
impl DefaultPlatform {
cfg_if! {
if #[cfg(feature = "openssl")] {
fn parse_issuer_name() -> Vec<u8> {
X509::from_pem(TEST_CERT_PEM)
.unwrap()
.subject_name()
.to_der()
.unwrap()
}} else if #[cfg(feature = "rustcrypto")] {
fn parse_issuer_name() -> Vec<u8> {
Certificate::from_pem(TEST_CERT_PEM)
.unwrap()
.tbs_certificate
.subject
.to_der()
.unwrap()
}
}}
cfg_if! {
if #[cfg(feature = "openssl")] {
fn parse_issuer_sn() -> Vec<u8> {
X509::from_pem(TEST_CERT_PEM)
.unwrap()
.serial_number()
.to_bn()
.unwrap()
.to_vec()
}
} else if #[cfg(feature = "rustcrypto")] {
fn parse_issuer_sn() -> Vec<u8> {
Certificate::from_pem(TEST_CERT_PEM)
.unwrap()
.tbs_certificate
.serial_number
.as_bytes()
.to_vec()
}
}
}
}
pub use default::DefaultPlatform
#[cfg(feature = "openssl")]
mod default {
use super::*;
use openssl::x509::X509;
pub struct DefaultPlatform;
impl DefaultPlatform {
pub fn parse_issuer_name() -> Vec<u8> {
X509::from_pem(TEST_CERT_PEM)
.unwrap()
.subject_name()
.to_der()
.unwrap()
}
pub fn parse_issuer_sn() -> Vec<u8> {
X509::from_pem(TEST_CERT_PEM)
.unwrap()
.serial_number()
.to_bn()
.unwrap()
.to_vec()
}
}
}
#[cfg(feature = "rustcrypto")]
mod default {
use super::*;
use x509_cert::{
certificate::Certificate,
der::{DecodePem, Encode},
};
impl DefaultPlatform {
pub fn parse_issuer_name() -> Vec<u8> {
Certificate::from_pem(TEST_CERT_PEM)
.unwrap()
.tbs_certificate
.subject
.to_der()
.unwrap()
}
pub fn parse_issuer_sn() -> Vec<u8> {
Certificate::from_pem(TEST_CERT_PEM)
.unwrap()
.tbs_certificate
.serial_number
.as_bytes()
.to_vec()
}
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sent #288 (left cfg_if in to allow clippy/fmt to run on both impls by default)

@esmusick esmusick deleted the rc-impl branch December 15, 2023 17:13
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.

3 participants