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.

Sign up for GitHub

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

Jump to bottom

Update version of Mbed TLS #622

Merged
merged 3 commits into from
Jun 29, 2023
Merged

Update version of Mbed TLS #622

merged 3 commits into from
Jun 29, 2023

Conversation

egrimley-arm
Copy link
Contributor

@egrimley-arm egrimley-arm commented Jun 9, 2023 •
edited
Loading

third-party/rust-mbedtls is now based on the upstream branch v0.10, tag mbedtls_v0.10.0.

(It was initially based on fortanix/rust-mbedtls#278, which has now been merged.)

@mathias-arm
Copy link
Contributor

The upgrade to bindgen version 0.65.1 brings additional dependency changes. It would be interesting to see if we can downgrade to 0.64 to avoid that version bump on syn. I think there is an issue with psa-attestation if we try to upgrade bindgen in that crate.

@egrimley-arm egrimley-arm self-assigned this Jun 13, 2023
@egrimley-arm
Copy link
Contributor Author

I can confirm that mbedtls-sys does not work with bindgen 0.64, and psa-attestation does not work with bindgen 0.65.1. (I haven't tried to make sense of the errors generated in either case.) The semantic versioning of Rust crates treats the left-most non-zero number as major, so we can't expect compatibility between these versions of bindgen, unfortunately. I hope Cargo keeps build-dependencies separate from ordinary dependencies so the damage is at least limited.

@mathias-arm
Copy link
Contributor

Your new branch of mbedtls did not pick those changes:
veracruz-project/rust-mbedtls@7cac229
veracruz-project/rust-mbedtls@0b62e75

@egrimley-arm
Copy link
Contributor Author

Your new branch of mbedtls did not pick those changes:
veracruz-project/rust-mbedtls@7cac229
veracruz-project/rust-mbedtls@0b62e75

I've rescued those two: just the Cargo.toml changes as the Cargo.lock changes would get in the way of the merging and rebasing that I will have to do.

Are the following commits still of interest?

veracruz-project/rust-mbedtls@7a72ec0
veracruz-project/rust-mbedtls@394e7ae

@mathias-arm
Copy link
Contributor

Your new branch of mbedtls did not pick those changes:
veracruz-project/rust-mbedtls@7cac229
veracruz-project/rust-mbedtls@0b62e75

I've rescued those two: just the Cargo.toml changes as the Cargo.lock changes would get in the way of the merging and rebasing that I will have to do.

Are the following commits still of interest?

veracruz-project/rust-mbedtls@7a72ec0 veracruz-project/rust-mbedtls@394e7ae

We need to test on AArch64 to see if those are still needed.

@egrimley-arm
Copy link
Contributor Author

egrimley-arm commented Jun 15, 2023 •
edited
Loading

Since the change made by veracruz-project/rust-mbedtls@7cac229 is in a section called [target.x86_64-fortanix-unknown-sgx.dependencies], presumably it makes no real difference to us, so the only purpose of the change is to keep Dependabot happy?

EDIT: That change really does make a difference to our Cargo.lock files. Cargo is very mysterious.

@mathias-arm
Copy link
Contributor

Are the following commits still of interest?

veracruz-project/rust-mbedtls@7a72ec0
veracruz-project/rust-mbedtls@394e7ae

Probably, as far as I can remember they relate to building on AArch64 and a toolchain that was returning an empty sysroot.

@mathias-arm
Copy link
Contributor

Looking at the branch in rust-mbedtls there is some back and forth (things done and then reverted), maybe some squashing would be helpful.

@egrimley-arm
Copy link
Contributor Author

I've rebased, squashed and improved the log messages.

We could wait for the upstream branch to get merged or released or for development of the upstream branch to slow down before merging this, or we could not wait for that.

@mathias-arm
Copy link
Contributor

I am generally happy with the state of the PR, but before we merge it:

  • I would prefer to have Updates after runtime split #627 be merged first in Veracruz.
  • There might be enhancement that can be done to reduce the duplication in crates (I guess its mostly bindgen at this point) resulting from this PR.
  • We should merge your work into main branch of rust-mbedtls.
  • Optionally wait for merged and released upstream.

@egrimley-arm
Copy link
Contributor Author

  • We should merge your work into main branch of rust-mbedtls.

We've been using the veracruz branch, though it's not mentioned in veracruz/.gitmodules: perhaps it should be and I could add that in this PR?

@mathias-arm
Copy link
Contributor

We've been using the veracruz branch, though it's not mentioned in veracruz/.gitmodules: perhaps it should be and I could add that in this PR?

Good idea. Based on today's discussion, I think we should not wait for an upstream merge and/or release.

@egrimley-arm egrimley-arm marked this pull request as ready for review June 22, 2023 22:07
@egrimley-arm egrimley-arm changed the title WIP: Update version of Mbed TLS Update version of Mbed TLS Jun 22, 2023
@mathias-arm
Copy link
Contributor

Can you remove the Remove unused features "icecap-lkvm" and "icecap-qemu". commit from the PR, this might create an issue for #555?

@egrimley-arm
Copy link
Contributor Author

Can you remove the Remove unused features "icecap-lkvm" and "icecap-qemu". commit from the PR, this might create an issue for #555?

OK. By the way, there seems to be an "approved" on the upstream PR (fortanix/rust-mbedtls#278). I don't know if that means an imminent merge. I might rebase again on Monday.

mathias-arm
mathias-arm previously approved these changes Jun 26, 2023
View reviewed changes
@egrimley-arm
Copy link
Contributor Author

The upstream PR has just been merged into master, 49 minutes ago, but it's been rebased so the commit I'm basing this on is not in master. So I think it's worth me rebasing this again before we merge.

@egrimley-arm
Copy link
Contributor Author

Done. Almost nothing changed: git diff --submodule=diff 81be98c..e11872e

@mathias-arm, if you can approve again I'll merge this time.

@egrimley-arm
Copy link
Contributor Author

I've just rebased again.

@egrimley-arm
Upgrade mbedtls to 3.4.0 using upstream/v0.10 (mbedtls_v0.10.0).
01b99ee 
Most of the changes in third-party/rust-mbedtls are for IceCap
and are now clearly labelled as such.

The monitor_getrandom feature has been disabled, for now.
@egrimley-arm
.gitmodules: Add "branch = veracruz" for third-party/rust-mbedtls.
646d9ff
@egrimley-arm egrimley-arm merged commit abab18a into veracruz-project:main Jun 29, 2023
@egrimley-arm egrimley-arm deleted the pr-update-mbedtls branch June 29, 2023 14:36
@mathias-arm mathias-arm added trusted-veracruz-runtime Something related to the trusted Veracruz runtime dependencies Pull requests that update a dependency file labels Jul 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dreemkiller dreemkiller dreemkiller approved these changes

@mathias-arm mathias-arm mathias-arm left review comments

Assignees

@egrimley-arm egrimley-arm

Labels
dependencies Pull requests that update a dependency file trusted-veracruz-runtime Something related to the trusted Veracruz runtime
Projects
Veracruz 23.07
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@egrimley-arm @mathias-arm @dreemkiller