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

feat: added support for reading certificates from macOS system store #56599

Open
wants to merge 33 commits into
base: main
Choose a base branch
from

Conversation

timja
Copy link

@timja timja commented Jan 14, 2025

Fixes #39657

Builds on #44532 but for macOS

TODO:

  • Make it work, it works 🥳
  • Review that all CF resources are being appropriately released, I think its right now
  • Review whether and where tests are appropriate - Added although disabled by default

I can take a look at the Windows one after, resolving the conflicts and addressing the review comments as well.


Happy to refactor heavily, I haven't used c++ before and I wrote it initially in objective c and ported it across.
This is heavily based upon chromium and some of OpenJDK along with a PR I have open with OpenJDK


Testing

I'm using https://github.com/timja/openjdk-intermediate-ca-reproducer as a reproducer:

docker compose up --build

Install the certificates, either by adding to keychain manually (see README) or using /usr/bin/security (see what the test is doing in this PR.

main.js

let resp = await fetch("https://localhost:8443");
console.log(resp.status); // 200
console.log(resp.headers.get("Content-Type")); // "text/html"
console.log(await resp.text()); // "Hello, World!"
/Users/$USER/projects/node/out/Release/node --use-system-ca main.js

I've also tested this through a ZScaler MiTM setup.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/crypto
  • @nodejs/gyp

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Jan 14, 2025
@timja timja force-pushed the macos-system-ca-support branch from 8fd32ce to f3c212c Compare January 14, 2025 16:32
src/crypto/crypto_context.cc Outdated Show resolved Hide resolved
src/crypto/crypto_context.cc Outdated Show resolved Hide resolved
src/crypto/crypto_context.cc Outdated Show resolved Hide resolved
@anonrig anonrig requested a review from jasnell January 14, 2025 17:29
@timja
Copy link
Author

timja commented Jan 15, 2025

Would it be possible for someone to re-open the feature request please? #39657. It was closed due to being stale / no progress on it.

doc/api/tls.md Outdated Show resolved Hide resolved
src/crypto/crypto_context.cc Outdated Show resolved Hide resolved
src/crypto/crypto_context.cc Outdated Show resolved Hide resolved
src/crypto/crypto_context.cc Outdated Show resolved Hide resolved
@timja timja requested review from joyeecheung and addaleax January 15, 2025 17:03
@timja timja marked this pull request as ready for review January 16, 2025 15:22
key: fixtures.readKey('agent1-key.pem'),
cert: fixtures.readKey('agent1-cert.pem')
}, handleRequest);
httpsServer.listen(0, common.mustCall(async () => {
Copy link
Member

Choose a reason for hiding this comment

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

Passing an async function to httpsServer.listen(...) is unexpected here.

Copy link
Author

Choose a reason for hiding this comment

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

is it a problem? It seems to work fine, I tried a few other approaches including using the node test framework but I struggled to get the test to wait for the https server to be listening.

Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

Adjusted, let me know if any issues with the new one

// where the trustSettings parameter returns NULL.
// No trust-settings array means
// “this certificate must be verifiable using a known trusted certificate”.
if (trustSettings == nullptr && !trustEvaluated) {
Copy link
Member

@joyeecheung joyeecheung Jan 20, 2025

Choose a reason for hiding this comment

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

Isn't this going against what the docs says? The certificate is only to be trusted as root certificate if trustSettings is not null but points to an empty array? i.e. this should already be covered in IsTrustSettingsTrustedForPolicy?

Copy link
Author

Choose a reason for hiding this comment

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

No is trust settings won’t find it as there’s no trust settings. The validate is implementing the part of the statement must be validated by another certificate. This is for intermediate certificates that are not sent by the web server but are present in the OS keychain

Copy link
Member

Choose a reason for hiding this comment

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

In that case shouldn't this be removed from the global root certificate array? And verified using SecPolicyCreateSSL instead on a per-connection basis? IIUC this is taking all the intermediate certificates, doing a basic check with SecPolicyCreateBasicX509, and if it looks X509 compliant, add it to the global root certificate array, which seems unsafe..

Copy link
Author

Choose a reason for hiding this comment

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

SecTrustEvaluateWithError is validating that this is a trusted certificate.

Potentially, I suspect that’s what Chromium is doing although I didn’t find the code, intermediate certificates do work there

Copy link
Author

@timja timja Jan 22, 2025

Choose a reason for hiding this comment

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

As far as I can tell chromium stores intermediates here:
https://github.com/chromium/chromium/blob/main/net/cert/internal/trust_store_mac.cc#L823

The code is quite hard to follow.


When evaluating client certificates they do use SecTrustEvaluateWithError in order to get the full chain:
https://github.com/chromium/chromium/blob/98f89988c9774d0e138a0724aa64c46187203a77/net/ssl/client_cert_store_mac.cc#L83-L84

but I can't see the same for regular certificate validation.


I think this is compliant but let me know if you think there's a better way of doing it

@timja
Copy link
Author

timja commented Jan 20, 2025

Thanks for the reviews all I'll continue actioning tomorrow.

@timja timja requested review from jasnell and joyeecheung January 22, 2025 10:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow Node to use certificates from the macOS Keychain when making HTTPS requests
5 participants