project-chip / rs-matter

Rust implementation of the Matter protocol. Status: Experimental
Apache License 2.0
329 stars 45 forks source link

fix: for mbedtls update #58

Closed thekuwayama closed 1 year ago

thekuwayama commented 1 year ago

Hi! This PR would modify matter/ for interface change due to mbedtls update.

I have not added rev for mbedtls in Cargo.toml . Please comments if you need to add it.

Thanks.

ivmarkov commented 1 year ago

@thekuwayama Two notes:

thekuwayama commented 1 year ago

@ivmarkov Thank you for comments.

Can you rebase your PR against the no_std branch?

OK!

"mbedtls update"

The related commit is the following:

Some functions now require his RNG argument.

-   pub fn private_from_ec_components(
+   pub fn private_from_ec_components<F: Random>(
+       rng: &mut F, 
        mut curve: EcGroup,
        private_key: Mpi
    ) -> Result<Pk> {
-   pub fn mul(
+   pub fn mul<F: crate::rng::Random>(
        &self,
        group: &mut EcGroup,
        k: &Mpi
+       rng: &mut F
    ) -> Result<EcPoint> {

And authenticated encryption/decryption functions have been updated the arguments.

    pub fn decrypt_auth_inplace(
        mut self,
        ad: &[u8],
-       data: &mut [u8],
-       tag: &[u8],
+       data_with_tag: &mut [u8],
+       tag_len: usize,
    ) -> Result<(usize, Cipher<Decryption, Authenticated, Finished>)> {

matter-iot does not specify mbedtls revision in Cargo.toml.

If we try to build matter-iot with the latest mbedtls, it will fail. This PR would update according to the mbedtls function updates.

ivmarkov commented 1 year ago

matter-iot does not specify mbedtls revision in Cargo.toml.

Of course! In retrospective, this is so obvious - sorry for asking dumb questions!

However the crux is we should actually not depend on the latest unstable main branch of any crate. Besides unanticipated breakages, depending on git repos means we cannot publish matter-rs on crates.io.

@kedars Was there any specific reason why we depend on the latest, unreleased mbedtls main branch? Perhaps there used to be valid reasons for that in the past, but this is no longer the case?

@thekuwayama Shall we - as part of this PR - also switch to the latest published mbedtls? I think this is V0.11, right? I would assume we nevertheless still need the changes in this PR?

Last but not least, let's also remind ourselves that the need to actually use mbedtls and/or openssl is much lower now that we have rust-crypto working. Aside from perhaps some performance regressions (would these even matter on non-MCU hardware?) and the fact that rust-crypto still has some allocations (but so does mbedtls with its Arc-based API), I actually am not sure, that we need to continue carrying the burden of supporting anything but rust-crypto.

ivmarkov commented 1 year ago

Maybe one important detail: I think depending on GIT crates was OK, as long as when you publish, the build that you do during publishing does not depend on these crates (i.e., these crates are hidden behind non-default features, which is exactly the case with mbedtls in no_std).

Still - fixing matter-rs to depend on a published version of mbedtls, or even further - removing mbedtls and openssl support altogether are valid topics, I think.

thekuwayama commented 1 year ago

I would assume we nevertheless still need the changes in this PR?

Yes, I think so. This update is needed even if you specify mbedtls v0.11 in your Cargo.toml. Because fortanix/rust-mbedtls@efd9d18 is already in v0.10 .

thekuwayama commented 1 year ago

@ivmarkov

Shall we - as part of this PR - also switch to the latest published mbedtls?

If it is no longer the case, I also think better to specify mbedtls version. 👍

kedars commented 1 year ago

@ivmarkov the reason to depend on mbedtls git repo was that for 2 years the mbedtls crate hadn't had an update and a number of my fixes for making Matter work were part of the mbedtls git repo (ref: https://github.com/fortanix/rust-mbedtls/issues/196) Same was the case with OpenSSL. I see that mbedtls has had a number of updates recently, so yes, we can now move to the create instead of the git repo

@ivmarkov @thekuwayama @bjoernQ having said that, I think yeah, we could move to Rust Crypto instead of the these crates and reduce the support burden. The only thing I wanted to check was that when different crypto crates were used (as opposed to mbedTLS and OpenSSL), some of the common things like SHA ended up getting pulled in from various sources, depending on what the next level dependencies were of the individual crypto libraries. While most of this is flash overhead, some was in-memory as well.

thekuwayama commented 1 year ago

@kedars cc: @ivmarkov Thank you very much for the response! Then,

And we can close this PR. What do you guys think?