Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit b26d1c7

Browse filesBrowse files
Aditi-1400aduh95
Aditi-1400
authored and
aduh95
committed
crypto: make --use-system-ca per-env rather than per-process
PR-URL: #60678 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
1 parent e362635 commit b26d1c7
Copy full SHA for b26d1c7

18 files changed

+459-80Lines changed: 459 additions & 80 deletions
Expand file treeCollapse file tree
Open diff view settings
Collapse file

‎src/crypto/crypto_common.cc‎

Copy file name to clipboardExpand all lines: src/crypto/crypto_common.cc
+1-1Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ MaybeLocal<Value> GetValidationErrorReason(Environment* env, int err) {
6161
(err == X509_V_ERR_UNABLE_TO_VERIFY_LEAF_SIGNATURE) ||
6262
(err == X509_V_ERR_DEPTH_ZERO_SELF_SIGNED_CERT) ||
6363
((err == X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT) &&
64-
!per_process::cli_options->use_system_ca);
64+
!env->options()->use_system_ca);
6565

6666
if (suggest_system_ca) {
6767
reason.append("; if the root CA is installed locally, "
Collapse file

‎src/crypto/crypto_context.cc‎

Copy file name to clipboardExpand all lines: src/crypto/crypto_context.cc
+113-41Lines changed: 113 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -100,12 +100,39 @@ static thread_local X509_STORE* root_cert_store = nullptr;
100100
// copy generated by NewRootCertStore() will then contain the certificates
101101
// from this set.
102102
static thread_local std::unique_ptr<X509Set> root_certs_from_users;
103+
static thread_local bool has_cleanup_hook = false;
103104

104-
X509_STORE* GetOrCreateRootCertStore() {
105+
static void CleanupRootCertStore(void*) {
106+
if (root_cert_store != nullptr) {
107+
X509_STORE_free(root_cert_store);
108+
root_cert_store = nullptr;
109+
}
110+
111+
if (root_certs_from_users != nullptr) {
112+
for (X509* cert : *root_certs_from_users) {
113+
X509_free(cert);
114+
}
115+
root_certs_from_users.reset();
116+
}
117+
118+
has_cleanup_hook = false;
119+
}
120+
121+
static void EnsureRootCertStoreCleanupHook(Environment* env) {
122+
if (env == nullptr || has_cleanup_hook) {
123+
return;
124+
}
125+
126+
env->AddCleanupHook(CleanupRootCertStore, nullptr);
127+
has_cleanup_hook = true;
128+
}
129+
130+
X509_STORE* GetOrCreateRootCertStore(Environment* env) {
131+
EnsureRootCertStoreCleanupHook(env);
105132
if (root_cert_store != nullptr) {
106133
return root_cert_store;
107134
}
108-
root_cert_store = NewRootCertStore();
135+
root_cert_store = NewRootCertStore(env);
109136
return root_cert_store;
110137
}
111138

@@ -870,23 +897,22 @@ static void LoadCACertificates(void* data) {
870897
"Started loading extra root certificates off-thread\n");
871898
GetExtraCACertificates();
872899
}
900+
}
873901

874-
{
875-
Mutex::ScopedLock cli_lock(node::per_process::cli_options_mutex);
876-
if (!per_process::cli_options->use_system_ca) {
877-
return;
878-
}
879-
}
880-
902+
static void LoadSystemCACertificates(void* data) {
881903
per_process::Debug(DebugCategory::CRYPTO,
882904
"Started loading system root certificates off-thread\n");
883905
GetSystemStoreCACertificates();
884906
}
885907

886908
static std::atomic<bool> tried_cert_loading_off_thread = false;
887909
static std::atomic<bool> cert_loading_thread_started = false;
910+
static std::atomic<bool> tried_system_cert_loading_off_thread = false;
911+
static std::atomic<bool> system_cert_loading_thread_started = false;
888912
static Mutex start_cert_loading_thread_mutex;
913+
static Mutex start_system_cert_loading_thread_mutex;
889914
static uv_thread_t cert_loading_thread;
915+
static uv_thread_t system_cert_loading_thread;
890916

891917
void StartLoadingCertificatesOffThread(
892918
const FunctionCallbackInfo<Value>& args) {
@@ -906,23 +932,46 @@ void StartLoadingCertificatesOffThread(
906932
}
907933
}
908934

935+
Environment* env = Environment::GetCurrent(args);
936+
const bool use_system_ca = env != nullptr && env->options()->use_system_ca;
937+
per_process::Debug(
938+
DebugCategory::CRYPTO, "StartLoadingCertificatesOffThread env=%p\n", env);
909939
// Only try to start the thread once. If it ever fails, we won't try again.
910-
if (tried_cert_loading_off_thread.load()) {
911-
return;
912-
}
913-
{
940+
// Quick check, if it's already tried, no need to lock.
941+
if (!tried_cert_loading_off_thread.load()) {
914942
Mutex::ScopedLock lock(start_cert_loading_thread_mutex);
915-
// Re-check under the lock.
916-
if (tried_cert_loading_off_thread.load()) {
917-
return;
943+
// Check again under the lock.
944+
if (!tried_cert_loading_off_thread.load()) {
945+
tried_cert_loading_off_thread.store(true);
946+
int r =
947+
uv_thread_create(&cert_loading_thread, LoadCACertificates, nullptr);
948+
cert_loading_thread_started.store(r == 0);
949+
if (r != 0) {
950+
FPrintF(stderr,
951+
"Warning: Failed to load CA certificates off thread: %s\n",
952+
uv_strerror(r));
953+
}
918954
}
919-
tried_cert_loading_off_thread.store(true);
920-
int r = uv_thread_create(&cert_loading_thread, LoadCACertificates, nullptr);
921-
cert_loading_thread_started.store(r == 0);
922-
if (r != 0) {
923-
FPrintF(stderr,
924-
"Warning: Failed to load CA certificates off thread: %s\n",
925-
uv_strerror(r));
955+
}
956+
957+
// If the system CA list hasn't been loaded off-thread yet, allow a worker
958+
// enabling --use-system-ca to trigger its off-thread loading.
959+
// Quick check, if it's already tried, no need to lock.
960+
if (use_system_ca && !has_cached_system_root_certs.load() &&
961+
!tried_system_cert_loading_off_thread.load()) {
962+
Mutex::ScopedLock lock(start_system_cert_loading_thread_mutex);
963+
if (!has_cached_system_root_certs.load() &&
964+
!tried_system_cert_loading_off_thread.load()) {
965+
tried_system_cert_loading_off_thread.store(true);
966+
int r = uv_thread_create(
967+
&system_cert_loading_thread, LoadSystemCACertificates, nullptr);
968+
system_cert_loading_thread_started.store(r == 0);
969+
if (r != 0) {
970+
FPrintF(
971+
stderr,
972+
"Warning: Failed to load system CA certificates off thread: %s\n",
973+
uv_strerror(r));
974+
}
926975
}
927976
}
928977
}
@@ -947,13 +996,13 @@ void StartLoadingCertificatesOffThread(
947996
// with all the other flags.
948997
// 7. Certificates from --use-bundled-ca, --use-system-ca and
949998
// NODE_EXTRA_CA_CERTS are cached after first load. Certificates
950-
// from --use-system-ca are not cached and always reloaded from
999+
// from --use-openssl-ca are not cached and always reloaded from
9511000
// disk.
9521001
// 8. If users have reset the root cert store by calling
9531002
// tls.setDefaultCACertificates(), the store will be populated with
9541003
// the certificates provided by users.
9551004
// TODO(joyeecheung): maybe these rules need a bit of consolidation?
956-
X509_STORE* NewRootCertStore() {
1005+
X509_STORE* NewRootCertStore(Environment* env) {
9571006
X509_STORE* store = X509_STORE_new();
9581007
CHECK_NOT_NULL(store);
9591008

@@ -975,14 +1024,26 @@ X509_STORE* NewRootCertStore() {
9751024
}
9761025
#endif
9771026

978-
Mutex::ScopedLock cli_lock(node::per_process::cli_options_mutex);
979-
if (per_process::cli_options->ssl_openssl_cert_store) {
1027+
bool use_system_ca = false;
1028+
bool ssl_openssl_cert_store = false;
1029+
{
1030+
Mutex::ScopedLock cli_lock(node::per_process::cli_options_mutex);
1031+
ssl_openssl_cert_store = per_process::cli_options->ssl_openssl_cert_store;
1032+
if (env != nullptr) {
1033+
use_system_ca = env->options()->use_system_ca;
1034+
} else if (per_process::cli_options->per_isolate != nullptr &&
1035+
per_process::cli_options->per_isolate->per_env != nullptr) {
1036+
use_system_ca =
1037+
per_process::cli_options->per_isolate->per_env->use_system_ca;
1038+
}
1039+
}
1040+
if (ssl_openssl_cert_store) {
9801041
CHECK_EQ(1, X509_STORE_set_default_paths(store));
9811042
} else {
9821043
for (X509* cert : GetBundledRootCertificates()) {
9831044
CHECK_EQ(1, X509_STORE_add_cert(store, cert));
9841045
}
985-
if (per_process::cli_options->use_system_ca) {
1046+
if (use_system_ca) {
9861047
for (X509* cert : GetSystemStoreCACertificates()) {
9871048
CHECK_EQ(1, X509_STORE_add_cert(store, cert));
9881049
}
@@ -999,6 +1060,22 @@ X509_STORE* NewRootCertStore() {
9991060
}
10001061

10011062
void CleanupCachedRootCertificates() {
1063+
// Serialize with starters to avoid the race window.
1064+
{
1065+
Mutex::ScopedLock lock(start_cert_loading_thread_mutex);
1066+
if (tried_cert_loading_off_thread.load() &&
1067+
cert_loading_thread_started.load()) {
1068+
uv_thread_join(&cert_loading_thread);
1069+
}
1070+
}
1071+
{
1072+
Mutex::ScopedLock lock(start_system_cert_loading_thread_mutex);
1073+
if (tried_system_cert_loading_off_thread.load() &&
1074+
system_cert_loading_thread_started.load()) {
1075+
uv_thread_join(&system_cert_loading_thread);
1076+
}
1077+
}
1078+
10021079
if (has_cached_bundled_root_certs.load()) {
10031080
for (X509* cert : GetBundledRootCertificates()) {
10041081
X509_free(cert);
@@ -1015,13 +1092,6 @@ void CleanupCachedRootCertificates() {
10151092
X509_free(cert);
10161093
}
10171094
}
1018-
1019-
// Serialize with starter to avoid the race window.
1020-
Mutex::ScopedLock lock(start_cert_loading_thread_mutex);
1021-
if (tried_cert_loading_off_thread.load() &&
1022-
cert_loading_thread_started.load()) {
1023-
uv_thread_join(&cert_loading_thread);
1024-
}
10251095
}
10261096

10271097
void GetBundledRootCertificates(const FunctionCallbackInfo<Value>& args) {
@@ -1133,6 +1203,8 @@ void ResetRootCertStore(const FunctionCallbackInfo<Value>& args) {
11331203
Local<Context> context = args.GetIsolate()->GetCurrentContext();
11341204
CHECK(args[0]->IsArray());
11351205
Local<Array> cert_array = args[0].As<Array>();
1206+
Environment* env = Environment::GetCurrent(context);
1207+
EnsureRootCertStoreCleanupHook(env);
11361208

11371209
if (cert_array->Length() == 0) {
11381210
// If the array is empty, just clear the user certs and reset the store.
@@ -1187,9 +1259,7 @@ void ResetRootCertStore(const FunctionCallbackInfo<Value>& args) {
11871259
X509_STORE_free(root_cert_store);
11881260
}
11891261

1190-
// TODO(joyeecheung): we can probably just reset it to nullptr
1191-
// and let the next call to NewRootCertStore() create a new one.
1192-
root_cert_store = NewRootCertStore();
1262+
root_cert_store = nullptr;
11931263
}
11941264

11951265
void GetSystemCACertificates(const FunctionCallbackInfo<Value>& args) {
@@ -1700,11 +1770,12 @@ void SecureContext::SetX509StoreFlag(unsigned long flags) {
17001770
}
17011771

17021772
X509_STORE* SecureContext::GetCertStoreOwnedByThisSecureContext() {
1773+
Environment* env = this->env();
17031774
if (own_cert_store_cache_ != nullptr) return own_cert_store_cache_;
17041775

17051776
X509_STORE* cert_store = SSL_CTX_get_cert_store(ctx_.get());
1706-
if (cert_store == GetOrCreateRootCertStore()) {
1707-
cert_store = NewRootCertStore();
1777+
if (cert_store == GetOrCreateRootCertStore(env)) {
1778+
cert_store = NewRootCertStore(env);
17081779
SSL_CTX_set_cert_store(ctx_.get(), cert_store);
17091780
}
17101781

@@ -1777,7 +1848,8 @@ void SecureContext::AddCRL(const FunctionCallbackInfo<Value>& args) {
17771848

17781849
void SecureContext::SetRootCerts() {
17791850
ClearErrorOnReturn clear_error_on_return;
1780-
auto store = GetOrCreateRootCertStore();
1851+
Environment* env = this->env();
1852+
auto store = GetOrCreateRootCertStore(env);
17811853

17821854
// Increment reference count so global store is not deleted along with CTX.
17831855
X509_STORE_up_ref(store);
Collapse file

‎src/crypto/crypto_context.h‎

Copy file name to clipboardExpand all lines: src/crypto/crypto_context.h
+2-2Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,9 +19,9 @@ constexpr int kMaxSupportedVersion = TLS1_3_VERSION;
1919
void GetRootCertificates(
2020
const v8::FunctionCallbackInfo<v8::Value>& args);
2121

22-
X509_STORE* NewRootCertStore();
22+
X509_STORE* NewRootCertStore(Environment* env);
2323

24-
X509_STORE* GetOrCreateRootCertStore();
24+
X509_STORE* GetOrCreateRootCertStore(Environment* env);
2525

2626
ncrypto::BIOPointer LoadBIO(Environment* env, v8::Local<v8::Value> v);
2727

Collapse file

‎src/node.cc‎

Copy file name to clipboardExpand all lines: src/node.cc
-9Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -867,15 +867,6 @@ static ExitCode InitializeNodeWithArgsInternal(
867867
// default value.
868868
V8::SetFlagsFromString("--rehash-snapshot");
869869

870-
#if HAVE_OPENSSL
871-
// TODO(joyeecheung): make this a per-env option and move the normalization
872-
// into HandleEnvOptions.
873-
std::string use_system_ca;
874-
if (credentials::SafeGetenv("NODE_USE_SYSTEM_CA", &use_system_ca) &&
875-
use_system_ca == "1") {
876-
per_process::cli_options->use_system_ca = true;
877-
}
878-
#endif // HAVE_OPENSSL
879870
HandleEnvOptions(per_process::cli_options->per_isolate->per_env);
880871

881872
std::string node_options;
Collapse file

‎src/node_options.cc‎

Copy file name to clipboardExpand all lines: src/node_options.cc
+18-4Lines changed: 18 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -208,6 +208,13 @@ void EnvironmentOptions::CheckOptions(std::vector<std::string>* errors,
208208
"used, not both");
209209
}
210210

211+
#if HAVE_OPENSSL
212+
if (use_system_ca && per_process::cli_options->use_openssl_ca) {
213+
errors->push_back("either --use-openssl-ca or --use-system-ca can be "
214+
"used, not both");
215+
}
216+
#endif // HAVE_OPENSSL
217+
211218
if (heap_snapshot_near_heap_limit < 0) {
212219
errors->push_back("--heapsnapshot-near-heap-limit must not be negative");
213220
}
@@ -1052,6 +1059,13 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() {
10521059
&EnvironmentOptions::trace_env_native_stack,
10531060
kAllowedInEnvvar);
10541061

1062+
#if HAVE_OPENSSL
1063+
AddOption("--use-system-ca",
1064+
"use system's CA store",
1065+
&EnvironmentOptions::use_system_ca,
1066+
kAllowedInEnvvar);
1067+
#endif // HAVE_OPENSSL
1068+
10551069
AddOption(
10561070
"--trace-require-module",
10571071
"Print access to require(esm). Options are 'all' (print all usage) and "
@@ -1392,10 +1406,6 @@ PerProcessOptionsParser::PerProcessOptionsParser(
13921406
,
13931407
&PerProcessOptions::use_openssl_ca,
13941408
kAllowedInEnvvar);
1395-
AddOption("--use-system-ca",
1396-
"use system's CA store",
1397-
&PerProcessOptions::use_system_ca,
1398-
kAllowedInEnvvar);
13991409
AddOption("--use-bundled-ca",
14001410
"use bundled CA store"
14011411
#if !defined(NODE_OPENSSL_CERT_STORE)
@@ -2162,6 +2172,10 @@ void HandleEnvOptions(std::shared_ptr<EnvironmentOptions> env_options,
21622172

21632173
env_options->use_env_proxy = opt_getter("NODE_USE_ENV_PROXY") == "1";
21642174

2175+
#if HAVE_OPENSSL
2176+
env_options->use_system_ca = opt_getter("NODE_USE_SYSTEM_CA") == "1";
2177+
#endif // HAVE_OPENSSL
2178+
21652179
if (env_options->redirect_warnings.empty())
21662180
env_options->redirect_warnings = opt_getter("NODE_REDIRECT_WARNINGS");
21672181
}
Collapse file

‎src/node_options.h‎

Copy file name to clipboardExpand all lines: src/node_options.h
+1-1Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -223,6 +223,7 @@ class EnvironmentOptions : public Options {
223223
bool trace_env = false;
224224
bool trace_env_js_stack = false;
225225
bool trace_env_native_stack = false;
226+
bool use_system_ca = false;
226227
std::string trace_require_module;
227228
bool extra_info_on_fatal_exception = true;
228229
std::string unhandled_rejections;
@@ -360,7 +361,6 @@ class PerProcessOptions : public Options {
360361
bool ssl_openssl_cert_store = false;
361362
#endif
362363
bool use_openssl_ca = false;
363-
bool use_system_ca = false;
364364
bool use_bundled_ca = false;
365365
bool enable_fips_crypto = false;
366366
bool force_fips_crypto = false;
Collapse file

‎src/quic/endpoint.cc‎

Copy file name to clipboardExpand all lines: src/quic/endpoint.cc
+2-2Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -892,7 +892,7 @@ void Endpoint::Listen(const Session::Options& options) {
892892
"not what you want.");
893893
}
894894

895-
auto context = TLSContext::CreateServer(options.tls_options);
895+
auto context = TLSContext::CreateServer(env(), options.tls_options);
896896
if (!*context) {
897897
THROW_ERR_INVALID_STATE(
898898
env(), "Failed to create TLS context: %s", context->validation_error());
@@ -925,7 +925,7 @@ BaseObjectPtr<Session> Endpoint::Connect(
925925
config,
926926
session_ticket.has_value() ? "yes" : "no");
927927

928-
auto tls_context = TLSContext::CreateClient(options.tls_options);
928+
auto tls_context = TLSContext::CreateClient(env(), options.tls_options);
929929
if (!*tls_context) {
930930
THROW_ERR_INVALID_STATE(env(),
931931
"Failed to create TLS context: %s",

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.