Add implementation for mbedtls #261

zvolin · 2023-05-04T21:27:31Z

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

Cargo.toml

src/imp/mbedtls.rs

jethrogb

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

Cargo.toml

src/imp/mbedtls.rs

zvolin · 2023-05-09T18:29:59Z

src/imp/mbedtls.rs

+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(),
+        ))
+    }
+}


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

@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)

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

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

This is actually ongoing

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

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.

Resolves sfackler#211

jethrogb · 2023-05-19T15:07:53Z

@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?

Add implementation for mbedtls - Initial work by @MabezDev

fe55a47

Co-authored-by: Scott Mabin <scott@mabez.dev>

zvolin force-pushed the feature/mbedtls-imp branch from 612f281 to ecc5b15 Compare May 4, 2023 21:35

zvolin commented May 4, 2023

View reviewed changes

src/imp/mbedtls.rs Outdated Show resolved Hide resolved

sfackler reviewed May 4, 2023

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

sfackler reviewed May 4, 2023

View reviewed changes

src/imp/mbedtls.rs Outdated Show resolved Hide resolved

zvolin force-pushed the feature/mbedtls-imp branch from ecc5b15 to 045578a Compare May 4, 2023 22:12

zvolin commented May 4, 2023

View reviewed changes

src/imp/mbedtls.rs Outdated Show resolved Hide resolved

zvolin force-pushed the feature/mbedtls-imp branch 4 times, most recently from 5bc8555 to 50445dd Compare May 7, 2023 14:39

jethrogb mentioned this pull request May 9, 2023

Support using mbedtls #142

Closed

jethrogb reviewed May 9, 2023

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

src/imp/mbedtls.rs Outdated Show resolved Hide resolved

zvolin force-pushed the feature/mbedtls-imp branch from 50445dd to 9b22720 Compare May 9, 2023 18:27

zvolin commented May 9, 2023

View reviewed changes

zvolin added 2 commits May 15, 2023 09:47

Add implementation for mbedtls

4455c10

Resolves sfackler#211

Add CI for mbedtls implementation

3dce867

zvolin force-pushed the feature/mbedtls-imp branch from 9b22720 to 3dce867 Compare May 15, 2023 07:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add implementation for mbedtls #261

Add implementation for mbedtls #261

zvolin commented May 4, 2023 •

edited

Loading

jethrogb left a comment

zvolin May 9, 2023

zvolin May 11, 2023 •

edited

Loading

zvolin May 11, 2023

jethrogb May 15, 2023

zvolin May 16, 2023

zvolin May 19, 2023

jethrogb commented May 19, 2023

Add implementation for mbedtls #261

Are you sure you want to change the base?

Add implementation for mbedtls #261

Conversation

zvolin commented May 4, 2023 • edited Loading

jethrogb left a comment

Choose a reason for hiding this comment

zvolin May 9, 2023

Choose a reason for hiding this comment

zvolin May 11, 2023 • edited Loading

Choose a reason for hiding this comment

zvolin May 11, 2023

Choose a reason for hiding this comment

jethrogb May 15, 2023

Choose a reason for hiding this comment

zvolin May 16, 2023

Choose a reason for hiding this comment

zvolin May 19, 2023

Choose a reason for hiding this comment

jethrogb commented May 19, 2023

zvolin commented May 4, 2023 •

edited

Loading

zvolin May 11, 2023 •

edited

Loading