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

Adds macOS Keychain certs to default CA store #6588

Closed
wants to merge 2 commits into from

Conversation

ansg191
Copy link

@ansg191 ansg191 commented Oct 18, 2023

What does this PR do?

Adds support to bun-usockets to load trusted certificates from the native platform certificate store into the default CA store. This PR implements this for the macOS Keychain. Windows Certificate Stores could be added in the future.

Partially relevant: #271

Note: All added code is usockets internal. Also change does not update rootCertificates in internals/tls.

Implementation based on rustls-native-certs.

  • Documentation or TypeScript types (it's okay to leave the rest blank in this case)
  • Code changes

How did you verify your code works?

  • Tested locally
  • I included a test for the new code, or an existing test covers it
    • Not sure how to since it's platform specific

@Jarred-Sumner
Copy link
Collaborator

I think this is a great idea, but I think we should dynamically load Core Foundation and the Security framework instead of passing it as a linker flag. Loading more frameworks at start time impacts start time. So we should instead use dlopen() and dlsym() to lazily load the symbols as needed.

Uses dlopen & dlsym to load CoreFoundation & Security Frameworks only
when necessary (when first retrieving the default CA store).
@ansg191
Copy link
Author

ansg191 commented Oct 19, 2023

@Jarred-Sumner Just pushed a commit to do that.

The Core Foundation & Security Frameworks will be dynamically loaded on the first request to us_get_default_ca_store.

@@ -647,6 +949,7 @@ X509_STORE* us_get_default_ca_store() {
}

us_internal_init_root_certs();
us_internal_init_native_certs();
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we do this, we should just use the native certs and not both right? So on macOS, it would only load native certs and on Linux, it would only load root certs.

I think that we also should see if there's any perf difference in load time as I remember a 30ms perf hit a long time ago when loading native certs using a non-OpenSSL/BoringSSL library and loading from the system trust store.

Copy link
Author

Choose a reason for hiding this comment

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

I just did some profiling, and the results are not good. The 3 SecTrustSettingsCopyCertificates requests to get all the certificates took 70ms total & the SecTrustSettingsCopyTrustSettings on each certificate to determine whether the system trusts the cert took 78ms total. As of 1.0.6, bun only takes 6ms to load certificates.

That's really disappointing. I was not expecting these APIs to be so slow.

I think a better approach would be to follow what Deno did in denoland/deno#11491. On all platforms, default to embedded Mozilla certs, and have some flag/env variable/config to enable the use of native certs for those who want/need it and are willing to take the first request performance hit.

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