Skip to content

Add support for mbedtls to rust-native-tls #1

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

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

jack-fortanix
Copy link

This PR is not for merging to this branch but just for review/commentary before I try opening a PR with upstream. There is a lot of unsafe, a lot of raw pointer manipulation in here. I do not believe upstream will take this PR as is.

I certainly think the rust-mbedtls API could be improved, such that it would be possible to implement this without all the unsafe. This would of course benefit any other current or potential users. See fortanix/rust-mbedtls#4. However I have no idea how to go about fixing our crates API. Suggestions very welcome.

@jack-fortanix
Copy link
Author

@jethrogb @Goirad ready for another look

Box::from_raw(self.config);
Box::from_raw(self.rng);
Box::from_raw(self.entropy);

Copy link
Member

Choose a reason for hiding this comment

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

Maybe if !self.cred_cert_list.is_null()?

let trust_roots = if builder.root_certificates.len() > 0 {
builder.root_certificates.clone()
} else {
load_ca_certs("/usr/share/ca-certificates/mozilla")?
Copy link
Member

Choose a reason for hiding this comment

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

This seems to imply that this only works on *nix systems, so the mbedtls feature should only be available on those systems.

Copy link
Author

Choose a reason for hiding this comment

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

This is already Unix specific since on Windows and macOS/iOS rust-native-tls always uses the platform native APIs. Actually not just Unix specific but Ubuntu specific, as the location of the trust roots varies. This is just where Ubuntu sticks them.

So I guess this can be improved somewhat - first try several commonly used paths so things nominally work on RHEL etc. If none of the paths work, return an error telling the user they must provide the trust roots manually.

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