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

fix: for mbedtls update #58

Closed

Conversation

thekuwayama
Copy link
Contributor

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
Copy link
Contributor

@thekuwayama Two notes:

  • Can you rebase your PR against the no_std branch? As per the note in the repo README, all development is now ongoing in the no_std and sequential branches, with the no_std branch to be merged into main within the next days (main would remain under a different name but would no longer see any active development)
  • Can you explain in more details here what exactly you mean by "mbedtls update". As in what the PR is fixing?

@thekuwayama thekuwayama changed the base branch from main to no_std July 1, 2023 19:53
@thekuwayama
Copy link
Contributor Author

@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 encrypt_auth_inplace(
        mut self,
        ad: &[u8],
-       data: &mut [u8],
-       tag: &mut [u8],
+      data_with_tag: &mut [u8],
+      tag_len: usize,
    ) -> Result<(usize, Cipher<Encryption, Authenticated, Finished>)> {
    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
Copy link
Contributor

ivmarkov commented Jul 1, 2023

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.

  • @kedars @bjoernQ what do you guys think?
  • @thekuwayama - do you have any reason to actually not switch to rust-crypto (which is the default in no_std / sequential anyway)?

@ivmarkov
Copy link
Contributor

ivmarkov commented Jul 1, 2023

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
Copy link
Contributor Author

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
Copy link
Contributor Author

@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
Copy link
Collaborator

kedars commented Jul 2, 2023

@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: fortanix/rust-mbedtls#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
Copy link
Contributor Author

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

  • I believe it is better to be able to build the main branch, even before merging the no_std branch. Therefore, I will create a pull request to the main branch to specify the version 0.9 of mbedtls.
  • We remember to check that matter-iot does not depend on multiple crates for the same purpose (e.g., SHA) when merging no_std branch.
    • I do not know how to check it using CI.

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

@kedars kedars closed this in #64 Jul 3, 2023
@thekuwayama thekuwayama deleted the update__with_mbedtls branch July 3, 2023 07:35
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