FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

crypto: make --use-system-ca per-env rather than per-process · nodejs/node@b26d1c7 · GitHub

/ node Public

Commit b26d1c7

Browse files
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

18 files changed

Lines changed: 459 additions & 80 deletions

‎src/crypto/crypto_common.cc‎

Lines 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, "

‎src/crypto/crypto_context.cc‎

Lines 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);

‎src/crypto/crypto_context.h‎

Lines 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

‎src/node.cc‎

Lines 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;

‎src/node_options.cc‎

Lines 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
}

‎src/node_options.h‎

Lines 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;

‎src/quic/endpoint.cc‎

Lines 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)
Back | FazBrowse Home | New Git URL