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

MbedTLS Reference counted instead of lifetimes #128

Merged
merged 2 commits into from
Dec 17, 2020

Conversation

AdrianCX
Copy link
Contributor

@AdrianCX AdrianCX commented Nov 17, 2020

Moving from referene counting allows simpler move to native-tls / hyper.

Arc Changes:

  • Each Config/Context/... will hold Arcs towards items it holds pointers to.
  • This forces objects to live as long as needed, once no longer used they get destroyed by reference counting.

This allows passing the objects to multiple threads without worrying about lifetime.
I've also added notes why classes are Sync where used. Let me know if I missed any classes.

Usage example of an intermediate mbed-hyper integration is at:

There I added a crate to wrap hyper - similar to native-tls. (that will be moved to native-tls layer soon)
That crate can be considered an integration test that I will raise a separate PR for.

Edit:

Changes after initial review:

  • Added forward_mbedtls_calloc / forward_mbedtls_free functions so we can pass certificates to and from mbedtls without allocator mismatches/corruptions.
  • Switched to MbedtlsList and Certificate. A MbedtlsBox is pending for this PR as well.
  • Fixed most comments.

Still pending:

  • Update define! macros
  • Add MbedtlsBox

Fixes #1
Partial progress on #3
Fixes #4
Fixes #8
Partially addresses #9

Copy link
Member

@jethrogb jethrogb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not done with the review, but let's start here

mbedtls-sys/Cargo.toml Outdated Show resolved Hide resolved
mbedtls-sys/vendor/include/mbedtls/ssl.h Outdated Show resolved Hide resolved
mbedtls-sys/build/config.rs Outdated Show resolved Hide resolved
mbedtls/src/wrapper_macros.rs Outdated Show resolved Hide resolved
mbedtls/src/wrapper_macros.rs Outdated Show resolved Hide resolved
mbedtls/src/pk/dhparam.rs Outdated Show resolved Hide resolved
mbedtls/src/pk/mod.rs Outdated Show resolved Hide resolved
mbedtls/src/pk/rfc6979.rs Outdated Show resolved Hide resolved
mbedtls/src/rng/ctr_drbg.rs Outdated Show resolved Hide resolved
mbedtls/src/rng/hmac_drbg.rs Outdated Show resolved Hide resolved
@AdrianCX AdrianCX force-pushed the acruceru/mbed-reference-counted branch 4 times, most recently from 6e41bdc to 2ce3aa1 Compare November 23, 2020 08:48
Copy link
Member

@jethrogb jethrogb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add the following text in the issue description (if appropriate):
Fixes #1
Fixes #3
Fixes #4
Fixes #8
Fixes #9

mbedtls/src/lib.rs Outdated Show resolved Hide resolved
mbedtls/src/pk/dhparam.rs Outdated Show resolved Hide resolved
mbedtls/src/pk/mod.rs Outdated Show resolved Hide resolved
mbedtls/src/rng/hmac_drbg.rs Outdated Show resolved Hide resolved
mbedtls/src/rng/hmac_drbg.rs Outdated Show resolved Hide resolved
mbedtls/src/rng/ctr_drbg.rs Show resolved Hide resolved
mbedtls/src/rng/rdrand.rs Outdated Show resolved Hide resolved
mbedtls/src/self_test.rs Outdated Show resolved Hide resolved
mbedtls/src/ssl/ciphersuites.rs Outdated Show resolved Hide resolved
mbedtls/src/x509/mod.rs Outdated Show resolved Hide resolved
@AdrianCX AdrianCX force-pushed the acruceru/mbed-reference-counted branch 2 times, most recently from 0be7829 to cd9eb8e Compare December 1, 2020 13:00
mbedtls/src/x509/mbedtls_wrappers.rs Outdated Show resolved Hide resolved
mbedtls/src/x509/mbedtls_wrappers.rs Outdated Show resolved Hide resolved
mbedtls/src/x509/mbedtls_wrappers.rs Outdated Show resolved Hide resolved
mbedtls/src/x509/mbedtls_wrappers.rs Outdated Show resolved Hide resolved
mbedtls/src/x509/mbedtls_wrappers.rs Outdated Show resolved Hide resolved
mbedtls/src/x509/mbedtls_wrappers.rs Outdated Show resolved Hide resolved
mbedtls/src/x509/certificate.rs Outdated Show resolved Hide resolved
mbedtls/src/x509/certificate.rs Outdated Show resolved Hide resolved
mbedtls-sys/Cargo.toml Outdated Show resolved Hide resolved
mbedtls/build.rs Outdated Show resolved Hide resolved
mbedtls/Cargo.toml Outdated Show resolved Hide resolved
mbedtls/build.rs Outdated Show resolved Hide resolved
mbedtls/examples/client.rs Outdated Show resolved Hide resolved
mbedtls/src/x509/certificate.rs Outdated Show resolved Hide resolved
mbedtls/src/x509/certificate.rs Outdated Show resolved Hide resolved
mbedtls/src/x509/certificate.rs Outdated Show resolved Hide resolved
mbedtls/src/x509/certificate.rs Outdated Show resolved Hide resolved
mbedtls/src/x509/certificate.rs Outdated Show resolved Hide resolved
@AdrianCX AdrianCX force-pushed the acruceru/mbed-reference-counted branch 4 times, most recently from f741aeb to 1715f09 Compare December 8, 2020 11:25
Copy link
Member

@jethrogb jethrogb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this only partially addresses #9, please update the PR description so it doesn't close the issue on merge.

mbedtls/Cargo.toml Outdated Show resolved Hide resolved
mbedtls/Cargo.toml Outdated Show resolved Hide resolved
mbedtls/src/lib.rs Show resolved Hide resolved
mbedtls/src/rng/ctr_drbg.rs Show resolved Hide resolved
mbedtls/src/rng/ctr_drbg.rs Outdated Show resolved Hide resolved
mbedtls/src/x509/certificate.rs Outdated Show resolved Hide resolved
mbedtls/src/x509/certificate.rs Outdated Show resolved Hide resolved
mbedtls/src/x509/certificate.rs Show resolved Hide resolved
mbedtls/src/x509/certificate.rs Outdated Show resolved Hide resolved
mbedtls/valgrind_unittests.sh Show resolved Hide resolved
@AdrianCX AdrianCX force-pushed the acruceru/mbed-reference-counted branch from 3344b8d to 241be71 Compare December 10, 2020 15:19
ct.sh Outdated Show resolved Hide resolved
mbedtls/src/ssl/config.rs Outdated Show resolved Hide resolved
mbedtls/src/rng/os_entropy.rs Outdated Show resolved Hide resolved
mbedtls/src/rng/os_entropy.rs Show resolved Hide resolved
mbedtls/src/self_test.rs Outdated Show resolved Hide resolved
mbedtls/src/ssl/context.rs Show resolved Hide resolved
mbedtls/src/ssl/context.rs Outdated Show resolved Hide resolved
mbedtls/src/ssl/context.rs Outdated Show resolved Hide resolved
mbedtls/src/ssl/context.rs Outdated Show resolved Hide resolved
mbedtls/src/ssl/context.rs Outdated Show resolved Hide resolved
mbedtls/src/pk/dhparam.rs Outdated Show resolved Hide resolved
Arc Changes:
-    Each Config/Context/... will hold Arcs towards items it holds pointers to.
-    This forces objects to live as long as needed, once no longer used they get destroyed by reference counting.

This allows passing the objects to multiple threads without worrying about lifetime.
I've also added notes why classes are Sync where used. Let me know if I missed any classes.

Usage example of an intermediate mbed-hyper integration is at:
-    https://github.com/fortanix/rust-mbedtls/tree/acruceru/wip-mbed-hyper-v2/mbedtls-hyper/examples/integrations

There I added a crate to wrap hyper - similar to native-tls. (that will be moved to native-tls layer soon)
That crate can be considered an integration test that I will raise a separate PR for.
@AdrianCX AdrianCX force-pushed the acruceru/mbed-reference-counted branch from 65ea801 to afecf09 Compare December 15, 2020 16:30
@jethrogb
Copy link
Member

bors try

bors bot added a commit that referenced this pull request Dec 16, 2020
@bors
Copy link
Contributor

bors bot commented Dec 16, 2020

try

Build succeeded:

@jethrogb
Copy link
Member

bors r+

bors bot added a commit that referenced this pull request Dec 17, 2020
128: MbedTLS Reference counted instead of lifetimes r=jethrogb a=AdrianCX

Moving from referene counting allows simpler move to native-tls / hyper.

Arc Changes:
- Each Config/Context/... will hold Arcs towards items it holds pointers to.
- This forces objects to live as long as needed, once no longer used they get destroyed by reference counting.

This allows passing the objects to multiple threads without worrying about lifetime.
I've also added notes why classes are Sync where used. Let me know if I missed any classes.

Usage example of an intermediate mbed-hyper integration is at: 
- https://github.com/fortanix/rust-mbedtls/tree/acruceru/wip-mbed-hyper-v2/mbedtls-hyper/examples/integrations

There I added a crate to wrap hyper - similar to native-tls. (that will be moved to native-tls layer soon)
That crate can be considered an integration test that I will raise a separate PR for.


Edit:

Changes after initial review:
-    Added forward_mbedtls_calloc / forward_mbedtls_free functions so we can pass certificates to and from mbedtls without allocator mismatches/corruptions.
-    Switched to MbedtlsList<Certificate> and Certificate. A MbedtlsBox is pending for this PR as well.
-    Fixed most comments.

Still pending:
-    Update define! macros
-    Add MbedtlsBox<Certificate>


Fixes #1
Partial progress on #3
Fixes #4
Fixes #8
Partially addresses #9

Co-authored-by: Adrian Cruceru <[email protected]>
@bors
Copy link
Contributor

bors bot commented Dec 17, 2020

Timed out.

@jethrogb
Copy link
Member

bors r+

@bors
Copy link
Contributor

bors bot commented Dec 17, 2020

Build succeeded:

@bors bors bot merged commit ca38f0f into master Dec 17, 2020
mcr pushed a commit to mcr/rust-mbedtls that referenced this pull request Aug 10, 2023
Gets rid of synthetic_error, and makes the various send_* methods return `Result<Response, Error>`.
Introduces a new error type "HTTP", which represents an error due to status codes 4xx or 5xx.
The HTTP error type contains a boxed Response, so users can read the actual response if they want.
Adds an `error_for_status` setting to disable the functionality of treating 4xx and 5xx as errors.
Adds .unwrap() to a lot of tests.

Fixes fortanix#128.
@Taowyoo Taowyoo deleted the acruceru/mbed-reference-counted branch October 20, 2023 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants