crypto: support --use-system-ca on non-Windows and non-macOS #57009
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Review requested:
|
nodejs-github-bot
added
c++
joyeecheung
force-pushed
the
certs-others
branch
from
4a711b4 to
c455c04
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #57009 +/- ##
==========================================
- Coverage 89.10% 89.09% -0.02%
==========================================
Files 665 665
Lines 193203 193245 +42
Branches 37220 37223 +3
==========================================
+ Hits 172158 172167 +9
- Misses 13771 13807 +36
+ Partials 7274 7271 -3
|
joyeecheung
force-pushed
the
certs-others
branch
from
c455c04 to
2c44065
Compare
github-actions
bot
removed
the
request-ci
joyeecheung
commented
Feb 13, 2025
joyeecheung
force-pushed
the
certs-others
branch
2 times, most recently
from
c78e1bb to
0d6c04a
Compare
On other platforms, load from the OpenSSL default certificate file and diretory. This is different from --use-openssl-ca in that it caches the certificates on first load, instead of always reading from disk every time a new root store is needed. When used together with the statically-linked OpenSSL, the default configuration usually leads to this behavior: - If SSL_CERT_FILE is used, load from SSL_CERT_FILE. Otherwise load from /etc/ssl/cert.pem - If SSL_CERT_DIR is used, load from all the files under SSL_CERT_DIR. Otherwise, load from all the files under /etc/ssl/certs
joyeecheung
force-pushed
the
certs-others
branch
from
0d6c04a to
891de46
Compare
github-actions
bot
removed
the
request-ci
richardlau
reviewed
Feb 14, 2025
joyeecheung
force-pushed
the
certs-others
branch
from
20cff99 to
822e288
Compare
joyeecheung
force-pushed
the
certs-others
branch
from
822e288 to
803ce60
Compare
richardlau
approved these changes
Feb 14, 2025
github-actions
bot
removed
the
request-ci
jasnell
reviewed
Feb 15, 2025
jasnell
reviewed
Feb 15, 2025
|
|
||
| uv_fs_t stats_req; | ||
| auto cleanup_stats = | ||
| OnScopeLeave([&stats_req]() { uv_fs_req_cleanup(&stats_req); }); |
There was a problem hiding this comment.
Not for this PR, just thinking out loud... it would be nice if we had smart pointer type wrappers for these so we could simplify this kind of code.
jasnell
approved these changes
Feb 15, 2025
|
Looks like there's a minor linting issue but otherwise LGTM |
|
Fixed the linter error (TIL "UNIX" is a trademark but "Unix" isnt?!) 🤯 |
github-actions
bot
removed
the
request-ci
richardlau
approved these changes
Feb 15, 2025
joyeecheung
added
commit-queue
nodejs-github-bot
removed
the
commit-queue
|
Landed in 579fc67 |
targos
pushed a commit
that referenced
this pull request
Feb 17, 2025
On other platforms, load from the OpenSSL default certificate file and diretory. This is different from --use-openssl-ca in that it caches the certificates on first load, instead of always reading from disk every time a new root store is needed. When used together with the statically-linked OpenSSL, the default configuration usually leads to this behavior: - If SSL_CERT_FILE is used, load from SSL_CERT_FILE. Otherwise load from /etc/ssl/cert.pem - If SSL_CERT_DIR is used, load from all the files under SSL_CERT_DIR. Otherwise, load from all the files under /etc/ssl/certs PR-URL: #57009 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: James M Snell <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
crypto: support --use-system-ca on non-Windows and non-macOS
On other platforms, load from the OpenSSL default certificate
file and diretory.
This is different from --use-openssl-ca in that it caches
the certificates on first load, instead of always reading
from disk every time a new root store is needed.
When used together with the statically-linked OpenSSL, the
default configuration usually leads to this behavior:
load from /etc/ssl/cert.pem
SSL_CERT_DIR. Otherwise, load from all the files under
/etc/ssl/certs
I've only checked Ubuntu and RHEL so far. It may be worth checking whether this works on other popular distributions - from what I can tell, though, it seems the hard-coded configurations always leads to default locations under
/etc/ssl/, which seems to be a location that most Linux distros would either use or link to anyway - for example, Ubuntu symlinks all the managed certificates to files under/etc/ssl/certs, while RHEL 9 just bundles all managed certificates to/etc/ssl/certs/ca-bundle.crt, so the current approach would usually just work.If this somehow turns out to be sufficient, we can come back adding specific fallbacks for other systems similar to what go does which is basically listing all the directories that are known to be used for this purpose - but from what I can tell this can be slow and lead to duplicates since they might get symlinked to each other, so ideally if
X509_get_default_cert_dir()andX509_get_default_cert_file()works on a platform, it's better not to look further to avoid unnecessary costs.There is also the question of whether we should use NSS shared DB, like what Chrome does, but since most command line tools rely on the directory/file-based system-wide storage instead (like go listed above, and e.g. every rust tool using the rustls-native-certs crate), and it seems to be losing the point if we statically link NSS, I didn't go with this route. In any case it seems various distros already consolidates the NSS shared DB somehow into these default locations when they manage their certificate stores, or at least NSS DB can be loaded via environment variable overrides, so the current approach looks good enough.