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

Check Send/Sync #8

Closed
jethrogb opened this issue Oct 30, 2018 · 2 comments · Fixed by #128
Closed

Check Send/Sync #8

jethrogb opened this issue Oct 30, 2018 · 2 comments · Fixed by #128
Labels
correctness

Comments

@jethrogb
Copy link
Member

We currently only implement Send on most structs when threading is enabled, but perhaps that restriction is unnecessary?
@jethrogb
Copy link
Member Author

I did some investigation:

Pk is Sync if:

  • It's an RSA key and mbedTLS threading is enabled.
  • It's an EC key of type ECKEY
    • In general: EC operations are not Sync, and in particular mbedtls_ecp_mul, mbedtls_ecp_muladd and by extension mbedtls_ecdsa_sign and mbedtls_ecdsa_verify.
    • The PK wrapper for keys of type ECKEY makes a stack-local copy of the necessary data (EC group) before invoking any of the above-mentioned functions
  • It's an EC key of type ECKEY_DH (because no operation can be called on such a key, only sign/verify/encrypt/decrypt are supported on Pk. DH operations are separate)

Pk is not Sync if:

  • It's an EC key of type ECDSA. There are no stack-local copies. It's very unlikely you'll have a Pk with an EC key type of ECDSA, because loading from DER/PEM always results in a ECKEY or ECKEY_DH (see mbedtls_oid_get_pk_alg).

Operations directly on the underlying key objects of a Pk of any EC type are probably not Sync.

All of Config's and Context's uses of a shared Pk are therefore probably Sync.

@jethrogb
Copy link
Member Author

jethrogb commented Oct 1, 2020

Problem: SSL Context is only Sync if the RNG is Sync

@AdrianCX AdrianCX mentioned this issue Nov 30, 2020
Merged
bors bot added a commit that referenced this issue Dec 17, 2020
@bors
Merge #128
3cf759a 
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 <adrian.cruceru@fortanix.com>
@bors bors bot closed this as completed in #128 Dec 17, 2020
@bors bors bot closed this as completed in ca38f0f Dec 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
correctness
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

MbedTLS Reference counted instead of lifetimes fortanix/rust-mbedtls
1 participant
@jethrogb