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

Add implementation for mbedtls #261

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

zvolin
Copy link

@zvolin zvolin commented May 4, 2023

Yo,
this is my attempt to resolve #211. The implementation is based on the initial work done by @MabezDev.

There are a few things worth mentioning, where I'd like to use some guidance:

  • no longer relevant after bump to mbedtls 0.9 this breaks api compatibility. I've changed fn get_ref(&self) -> &S and fn get_mut(&mut self) -> &mut S to fn get_ref(&self) -> impl Deref<Target = S> and fn get_mut(&mut self) -> impl DerefMut<Target = S>. I'm not sure if that's required, I started with a bit different implementation, but it's the non-unsafe way to have TlsStream impl Sync. I've checked 3 top dependant crates: Reqwest, hyper-tls and tokio-native-tls and only the last one failed, on tests of smoke.rs. If we decide to go this way I'm happy to run tests on all dependant crates and create the issues that'll warn about bumping this. If not, I can try the other ways. The other possible solutions would be to make this change only on mbedtls targets (but not uniform api is a painpoint) / try to implement this differently (but likely with unsafe).
  • I'm not sure what would be the way to get 'default' ca root certs list. My use case are Intel SGX enclaves which do not have (simple) access to the filesystem. I know there is interest in running those on some esp platforms etc, but I'm unfamiliar with them. We could maybe hardcode the ca root certificates from Mozilla but I decided that for now I'll start with the mindset that the user needs to provide them himself in case of using mbedtls.
  • What other targets to add? I will add the ones mentioned in the issue, are there maybe more 'obvious ones'?
  • Should I include builds for those targets in the CI? Will require installing additional toolchains and can extend the overall time / usage

A disclaimer, I'm not really familiar with HTTPS/TLS/SSL spec and implementations and also with networking in general 😅 so I'll be thankful for a careful review

Co-authored-by: Scott Mabin <scott@mabez.dev>
src/imp/mbedtls.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
src/imp/mbedtls.rs Outdated Show resolved Hide resolved
src/imp/mbedtls.rs Outdated Show resolved Hide resolved
@zvolin zvolin force-pushed the feature/mbedtls-imp branch 4 times, most recently from 5bc8555 to 50445dd Compare May 7, 2023 14:39
@jethrogb jethrogb mentioned this pull request May 9, 2023
Copy link

@jethrogb jethrogb left a comment

Choose a reason for hiding this comment

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

Let's investigate if MbedTLS can have more things Sync.

Cargo.toml Outdated Show resolved Hide resolved
src/imp/mbedtls.rs Outdated Show resolved Hide resolved
Comment on lines +105 to +127
fn pkcs12_decode_key_bag<B: AsRef<[u8]>>(
key_bag: &p12::EncryptedPrivateKeyInfo,
pass: B,
) -> Result<Vec<u8>, Error> {
// try to decrypt the key with algorithms supported by p12 crate
if let Some(decrypted) = key_bag.decrypt(pass.as_ref()) {
Ok(decrypted)
// try to decrypt the key with algorithms supported by pkcs5 standard
} else if let p12::AlgorithmIdentifier::OtherAlg(_) = key_bag.encryption_algorithm {
// write the algorithm identifier back to DER format
let algorithm_der =
yasna::construct_der(|writer| key_bag.encryption_algorithm.write(writer));
// and construct pkcs5 decoder from it
let scheme = pkcs5::EncryptionScheme::try_from(&algorithm_der[..])?;

Ok(scheme.decrypt(pass.as_ref(), &key_bag.encrypted_data)?)
} else {
Err(Error::Custom(
"Unsupported key encryption algorithm".to_owned(),
))
}
}
Copy link
Author

Choose a reason for hiding this comment

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

Custom pkcs12 treatment is there only because rust-mbedtls implementation doesn't support pbes2 encrypted keys (which is openssl's default). I'll try to provide support for it directly in mbedtls instead

Copy link
Author

@zvolin zvolin May 11, 2023

Choose a reason for hiding this comment

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

@sfackler @jethrogb
I've checked and we could add an implementation of pbes2 to rust-mbedtls, however from encryption schemes defined by pkcs5 rfc, MbedTls has support only for des-cbc and des-ede3-cbc.
rc2-cbc, rc5-cbc and aes-cbc are missing and aes-cbc is openssl's default when running openssl pkcs12 -export.
rc2 and rc5 are also missing in pkcs5 crate, so it seems to be all about AES.
Moreover, des-cbc is considered insecure: 'Although its short key length of 56 bits makes it too insecure for modern applications' wiki

Here are the options:

  • Add native (MbedTls) support for pbes2 in rust-mbedtls and use it there, only allowing des-cbc and des-ede3-cbc encryption schemes and providing compatible .p12 for rust-native-tls tests
  • Add non-native (using pkcs5 crate) suppert for pbes2 in rust-mbedtls and maybe hide it behind feature flag, allowing also aes-cbc
  • Keep this implementation and don't use rust-mbedtls for pbes2 (however if I'll have some spare time I'll likely issue a PR for rust-mbedtls anyway as I've already written most of the implementation)

Copy link
Author

Choose a reason for hiding this comment

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

Mbed-TLS/mbedtls#7024
Mbed-TLS/mbedtls#7038
It's possible AES will get support in MbedTls at some point, but it'll probably require rust-mbedtls to update to 3.x then, which can be major effort

Choose a reason for hiding this comment

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

it'll probably require rust-mbedtls to update to 3.x

This is actually ongoing

Copy link
Author

Choose a reason for hiding this comment

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

The preferred way of solving this would be to add support for AES in MbedTls: Mbed-TLS/mbedtls#7604
then pull it into rust-mbedtls and use it directly here

Copy link
Author

Choose a reason for hiding this comment

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

we've been discussing this with @jethrogb and it seems that rust-mbedtls is rather going to take steps towards removing pkcs12 support than extending it to pbes2. So it is up to you @sfackler whether we leave this implemented as is now using p12 and pkcs5 crates or just mark it as unimplemented.

@jethrogb
Copy link

@sfackler you wrote

I'd probably just recommend people not use PKCS#12 anymore.

#245 (comment)

Can we deprecate the p12 API and not require it for new platform implementations?

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.

mbedtls implementation
3 participants