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

Acruceru/mbedtls8 async support #163

Closed
wants to merge 3 commits into from

Conversation

AdrianCX
Copy link
Contributor

This is broken into 2 commits, please review per commit.

First one is the preparation needed to port the async support (plus a migration mode feature to allow both mbedtls7 and 8 to coexists)
Second is the actual async support ported from Mohsen's branch (with some changes due to reference counting)

Let me know if this should be merged (either first/both commits or none), I can also keep it as a separate branch for now similar to the 0.7 one.

@AdrianCX
Copy link
Contributor Author

AdrianCX commented Feb 3, 2022

todo: pick up: #171 (comment)

@AdrianCX
Copy link
Contributor Author

AdrianCX commented Feb 8, 2022

Going to split this off into multiple PRs.

First is the preparation work: #182

Pending:

bors bot added a commit that referenced this pull request Feb 10, 2022
182: Initial changes for async support r=jethrogb a=AdrianCX

Splitting #163 into separate parts.

Also rebased since `@raoulstrackx` already helped with some pieces of the PR above.

Code does the following:
- Minor update to comments on Pk (revisited the latest code to check if any issues, none found)
- verify with sig len 0 makes mbedtls assume default siglen and potentially overflow instead of stopping.
- EntropyHolder can have different sources, either shared or not (request from another older PR)
- Ported ALPN changes from `@mzohreva` branch - NullTerminatedList
- Split off HandshakeContext and made it a sort of base class to Context so we can use it safely. (lots of ideas from `@jethrogb` here)
- Made Context < T >  as opposed to using Any, makes life easier for async and a lot of other use cases.

Co-authored-by: Adrian Cruceru <adrian.cruceru@fortanix.com>
@Taowyoo
Copy link
Collaborator

Taowyoo commented Mar 14, 2023

Hi @AdrianCX,
I am working on cleaning up old PR & branches in this repo includes adding async support into master.
Are you still willing to update this PR? Otherwise, I will continue on it.
I create a PR based on your changes and rebased it on to master and fixing all problem and CI now.
Also it would be very helpful if you could give any info about pending tasks for this PR. THX!!

@Taowyoo Taowyoo mentioned this pull request Mar 15, 2023
2 tasks
@AdrianCX
Copy link
Contributor Author

AdrianCX commented Mar 15, 2023

@Taowyoo please feel free to pick it up, I can likely help if you run into issues or want to talk (email in profile)

This was done quite some time ago and code needs to be revisited a bit, it's mostly adjusting async code from @mzohreva branch on 0.7 to 0.8. Unfortunately I kept adding things to the branch since PR was raised.

Some notes on what might be needed:

  1. The 'migration_mode' feature I remember not being wanted so you might need to remove that.

  2. Changing 'sign' from '&mut self' to '&self' was something that was in-progress.

I was changing C code to const everywhere to make sure it's true but I'm not sure if I finished that, so you either need to revert to '&mut self' or check that assumption is valid.

Regarding this:

    pub fn sign<F: Random>(
        &self,
  1. Performance tests (compare to 0.7 under same scenario, should give similar numbers)

  2. Additional unit tests / run tests with valgrind.

When this is merged then additional work on integrating with rust-nativetls might be useful.

A old branch that had that: https://github.com/AdrianCX/rust-native-tls/tree/acruceru/rust-mbedtls

@Taowyoo
Copy link
Collaborator

Taowyoo commented Mar 15, 2023

@AdrianCX Thank you for providing these information. Helps a lot!

bors bot added a commit that referenced this pull request Mar 15, 2023
226: Rebased Acruceru/mbedtls8 async support r=Taowyoo a=Taowyoo

This is the successor of #163, except the commits in original PR.
Following changes are added:
- Add async tests to CI

2023-03-15 update:
According to #163 (comment)
Here are some to-dos in this PR:
- [x] remove 'migration_mode' feature
- [x] Since changing 'sign' from '&mut self' to '&self' was something that was in-progress, need to either revert to '&mut self' or check that assumption is valid. Regarding this:
	```rust
	    pub fn sign<F: Random>(
	        &self,
	```
Following two tasks could be put in another PR&issue
- Performance tests (compare to 0.7 under same scenario, should give similar numbers)
- Additional unit tests / run tests with valgrind. 





Co-authored-by: Adrian Cruceru <adrian.cruceru@fortanix.com>
Co-authored-by: Yuxiang Cao <yuxiang.cao@fortanix.com>
@Taowyoo
Copy link
Collaborator

Taowyoo commented Apr 19, 2023

Closed because this is done by #239

@Taowyoo Taowyoo closed this Apr 19, 2023
@Taowyoo Taowyoo deleted the acruceru/mbedtls8-async-support branch October 20, 2023 16:25
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.

2 participants