Skip to content

Commit 1fcb5fc

Browse files
Aditi-1400Aditi-1400
committed
src: make --use-system-ca per-env rather than per-process
1 parent 17dc7de commit 1fcb5fc

File tree

9 files changed

+258
-32
lines changed

9 files changed

+258
-32
lines changed

‎src/crypto/crypto_context.cc‎

Lines changed: 15 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -101,11 +101,11 @@ static thread_local X509_STORE* root_cert_store = nullptr;
101101
// from this set.
102102
static thread_local std::unique_ptr<X509Set> root_certs_from_users;
103103

104-
X509_STORE* GetOrCreateRootCertStore() {
104+
X509_STORE* GetOrCreateRootCertStore(Environment* env) {
105105
if (root_cert_store != nullptr) {
106106
return root_cert_store;
107107
}
108-
root_cert_store = NewRootCertStore();
108+
root_cert_store = NewRootCertStore(env);
109109
return root_cert_store;
110110
}
111111

@@ -861,6 +861,7 @@ static std::vector<X509*>& GetExtraCACertificates() {
861861
}
862862

863863
static void LoadCACertificates(void* data) {
864+
Environment* env = static_cast<Environment*>(data);
864865
per_process::Debug(DebugCategory::CRYPTO,
865866
"Started loading bundled root certificates off-thread\n");
866867
GetBundledRootCertificates();
@@ -873,7 +874,7 @@ static void LoadCACertificates(void* data) {
873874

874875
{
875876
Mutex::ScopedLock cli_lock(node::per_process::cli_options_mutex);
876-
if (!per_process::cli_options->per_isolate->per_env->use_system_ca) {
877+
if (!env->options()->use_system_ca) {
877878
return;
878879
}
879880
}
@@ -917,7 +918,8 @@ void StartLoadingCertificatesOffThread(
917918
return;
918919
}
919920
tried_cert_loading_off_thread.store(true);
920-
int r = uv_thread_create(&cert_loading_thread, LoadCACertificates, nullptr);
921+
Environment* env = Environment::GetCurrent(args);
922+
int r = uv_thread_create(&cert_loading_thread, LoadCACertificates, env);
921923
cert_loading_thread_started.store(r == 0);
922924
if (r != 0) {
923925
FPrintF(stderr,
@@ -947,13 +949,13 @@ void StartLoadingCertificatesOffThread(
947949
// with all the other flags.
948950
// 7. Certificates from --use-bundled-ca, --use-system-ca and
949951
// NODE_EXTRA_CA_CERTS are cached after first load. Certificates
950-
// from --use-system-ca are not cached and always reloaded from
952+
// from --use-openssl-ca are not cached and always reloaded from
951953
// disk.
952954
// 8. If users have reset the root cert store by calling
953955
// tls.setDefaultCACertificates(), the store will be populated with
954956
// the certificates provided by users.
955957
// TODO(joyeecheung): maybe these rules need a bit of consolidation?
956-
X509_STORE* NewRootCertStore() {
958+
X509_STORE* NewRootCertStore(Environment* env) {
957959
X509_STORE* store = X509_STORE_new();
958960
CHECK_NOT_NULL(store);
959961

@@ -982,7 +984,7 @@ X509_STORE* NewRootCertStore() {
982984
for (X509* cert : GetBundledRootCertificates()) {
983985
CHECK_EQ(1, X509_STORE_add_cert(store, cert));
984986
}
985-
if (per_process::cli_options->per_isolate->per_env->use_system_ca) {
987+
if (env->options()->use_system_ca) {
986988
for (X509* cert : GetSystemStoreCACertificates()) {
987989
CHECK_EQ(1, X509_STORE_add_cert(store, cert));
988990
}
@@ -1189,7 +1191,7 @@ void ResetRootCertStore(const FunctionCallbackInfo<Value>& args) {
11891191

11901192
// TODO(joyeecheung): we can probably just reset it to nullptr
11911193
// and let the next call to NewRootCertStore() create a new one.
1192-
root_cert_store = NewRootCertStore();
1194+
root_cert_store = nullptr;
11931195
}
11941196

11951197
void GetSystemCACertificates(const FunctionCallbackInfo<Value>& args) {
@@ -1700,11 +1702,12 @@ void SecureContext::SetX509StoreFlag(unsigned long flags) {
17001702
}
17011703

17021704
X509_STORE* SecureContext::GetCertStoreOwnedByThisSecureContext() {
1705+
Environment* env = this->env();
17031706
if (own_cert_store_cache_ != nullptr) return own_cert_store_cache_;
17041707

17051708
X509_STORE* cert_store = SSL_CTX_get_cert_store(ctx_.get());
1706-
if (cert_store == GetOrCreateRootCertStore()) {
1707-
cert_store = NewRootCertStore();
1709+
if (cert_store == GetOrCreateRootCertStore(env)) {
1710+
cert_store = NewRootCertStore(env);
17081711
SSL_CTX_set_cert_store(ctx_.get(), cert_store);
17091712
}
17101713

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

17781781
void SecureContext::SetRootCerts() {
17791782
ClearErrorOnReturn clear_error_on_return;
1780-
auto store = GetOrCreateRootCertStore();
1783+
Environment* env = this->env();
1784+
auto store = GetOrCreateRootCertStore(env);
17811785

17821786
// Increment reference count so global store is not deleted along with CTX.
17831787
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/quic/endpoint.cc‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -895,7 +895,7 @@ void Endpoint::Listen(const Session::Options& options) {
895895
"not what you want.");
896896
}
897897

898-
auto context = TLSContext::CreateServer(options.tls_options);
898+
auto context = TLSContext::CreateServer(env(), options.tls_options);
899899
if (!*context) {
900900
THROW_ERR_INVALID_STATE(
901901
env(), "Failed to create TLS context: %s", context->validation_error());
@@ -928,7 +928,7 @@ BaseObjectPtr<Session> Endpoint::Connect(
928928
config,
929929
session_ticket.has_value() ? "yes" : "no");
930930

931-
auto tls_context = TLSContext::CreateClient(options.tls_options);
931+
auto tls_context = TLSContext::CreateClient(env(), options.tls_options);
932932
if (!*tls_context) {
933933
THROW_ERR_INVALID_STATE(env(),
934934
"Failed to create TLS context: %s",

‎src/quic/tlscontext.cc‎

Lines changed: 14 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -293,16 +293,18 @@ bool OSSLContext::ConfigureClient() const {
293293

294294
// ============================================================================
295295

296-
std::shared_ptr<TLSContext> TLSContext::CreateClient(const Options& options) {
297-
return std::make_shared<TLSContext>(Side::CLIENT, options);
296+
std::shared_ptr<TLSContext> TLSContext::CreateClient(Environment* env,
297+
const Options& options) {
298+
return std::make_shared<TLSContext>(env, Side::CLIENT, options);
298299
}
299300

300-
std::shared_ptr<TLSContext> TLSContext::CreateServer(const Options& options) {
301-
return std::make_shared<TLSContext>(Side::SERVER, options);
301+
std::shared_ptr<TLSContext> TLSContext::CreateServer(Environment* env,
302+
const Options& options) {
303+
return std::make_shared<TLSContext>(env, Side::SERVER, options);
302304
}
303305

304-
TLSContext::TLSContext(Side side, const Options& options)
305-
: side_(side), options_(options), ctx_(Initialize()) {}
306+
TLSContext::TLSContext(Environment* env, Side side, const Options& options)
307+
: side_(side), options_(options), env_(env), ctx_(Initialize()) {}
306308

307309
TLSContext::operator SSL_CTX*() const {
308310
DCHECK(ctx_);
@@ -460,14 +462,14 @@ SSLCtxPointer TLSContext::Initialize() {
460462
{
461463
ClearErrorOnReturn clear_error_on_return;
462464
if (options_.ca.empty()) {
463-
auto store = crypto::GetOrCreateRootCertStore();
465+
auto store = crypto::GetOrCreateRootCertStore(env_);
464466
X509_STORE_up_ref(store);
465467
SSL_CTX_set_cert_store(ctx.get(), store);
466468
} else {
467469
for (const auto& ca : options_.ca) {
468470
uv_buf_t buf = ca;
469471
if (buf.len == 0) {
470-
auto store = crypto::GetOrCreateRootCertStore();
472+
auto store = crypto::GetOrCreateRootCertStore(env_);
471473
X509_STORE_up_ref(store);
472474
SSL_CTX_set_cert_store(ctx.get(), store);
473475
} else {
@@ -477,8 +479,8 @@ SSLCtxPointer TLSContext::Initialize() {
477479
while (
478480
auto x509 = X509Pointer(PEM_read_bio_X509_AUX(
479481
bio.get(), nullptr, crypto::NoPasswordCallback, nullptr))) {
480-
if (cert_store == crypto::GetOrCreateRootCertStore()) {
481-
cert_store = crypto::NewRootCertStore();
482+
if (cert_store == crypto::GetOrCreateRootCertStore(env_)) {
483+
cert_store = crypto::NewRootCertStore(env_);
482484
SSL_CTX_set_cert_store(ctx.get(), cert_store);
483485
}
484486
CHECK_EQ(1, X509_STORE_add_cert(cert_store, x509.get()));
@@ -535,8 +537,8 @@ SSLCtxPointer TLSContext::Initialize() {
535537
}
536538

537539
X509_STORE* cert_store = SSL_CTX_get_cert_store(ctx.get());
538-
if (cert_store == crypto::GetOrCreateRootCertStore()) {
539-
cert_store = crypto::NewRootCertStore();
540+
if (cert_store == crypto::GetOrCreateRootCertStore(env_)) {
541+
cert_store = crypto::NewRootCertStore(env_);
540542
SSL_CTX_set_cert_store(ctx.get(), cert_store);
541543
}
542544

‎src/quic/tlscontext.h‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -229,10 +229,12 @@ class TLSContext final : public MemoryRetainer,
229229
std::string ToString() const;
230230
};
231231

232-
static std::shared_ptr<TLSContext> CreateClient(const Options& options);
233-
static std::shared_ptr<TLSContext> CreateServer(const Options& options);
232+
static std::shared_ptr<TLSContext> CreateClient(Environment* env,
233+
const Options& options);
234+
static std::shared_ptr<TLSContext> CreateServer(Environment* env,
235+
const Options& options);
234236

235-
TLSContext(Side side, const Options& options);
237+
TLSContext(Environment* env, Side side, const Options& options);
236238
DISALLOW_COPY_AND_MOVE(TLSContext)
237239

238240
// Each QUIC Session has exactly one TLSSession. Each TLSSession maintains
@@ -242,6 +244,7 @@ class TLSContext final : public MemoryRetainer,
242244

243245
inline Side side() const { return side_; }
244246
inline const Options& options() const { return options_; }
247+
inline Environment* env() const { return env_; }
245248
inline operator bool() const { return ctx_ != nullptr; }
246249
inline operator const ncrypto::SSLCtxPointer&() const { return ctx_; }
247250

@@ -269,6 +272,7 @@ class TLSContext final : public MemoryRetainer,
269272

270273
Side side_;
271274
Options options_;
275+
Environment* env_;
272276
ncrypto::X509Pointer cert_;
273277
ncrypto::X509Pointer issuer_;
274278
ncrypto::SSLCtxPointer ctx_;

‎test/cctest/test_node_crypto.cc‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@
1515
*/
1616
TEST(NodeCrypto, NewRootCertStore) {
1717
node::per_process::cli_options->ssl_openssl_cert_store = true;
18-
X509_STORE* store = node::crypto::NewRootCertStore();
18+
X509_STORE* store = node::crypto::NewRootCertStore(nullptr);
1919
ASSERT_TRUE(store);
2020
ASSERT_EQ(ERR_peek_error(), 0UL) << "NewRootCertStore should not have left "
2121
"any errors on the OpenSSL error stack\n";

‎test/parallel/test-cli-node-options.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@ if (common.hasCrypto) {
6868
if (!hasOpenSSL3)
6969
expectNoWorker('--openssl-config=_ossl_cfg', 'B\n');
7070
if (common.isMacOS) {
71-
expectNoWorker('--use-system-ca', 'B\n');
71+
expect('--use-system-ca', 'B\n');
7272
}
7373
}
7474

‎test/system-ca/test-use-system-ca-worker-disable.mjs‎

Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,103 @@
1+
// Flags: --use-system-ca
2+
3+
// Verify that a worker can disable the system CA store while the parent uses it.
4+
5+
import * as common from '../common/index.mjs';
6+
import assert from 'node:assert/strict';
7+
import https from 'node:https';
8+
import fixtures from '../common/fixtures.js';
9+
import { it, beforeEach, afterEach, describe } from 'node:test';
10+
import { once } from 'events';
11+
import {
12+
Worker,
13+
isMainThread,
14+
parentPort,
15+
workerData,
16+
} from 'node:worker_threads';
17+
18+
if (!common.hasCrypto) {
19+
common.skip('requires crypto');
20+
}
21+
22+
// To run this test, the system needs to be configured to trust
23+
// the CA certificate first (which needs an interactive GUI approval, e.g. TouchID):
24+
// see the README.md in this folder for instructions on how to do this.
25+
const handleRequest = (req, res) => {
26+
const path = req.url;
27+
switch (path) {
28+
case '/hello-world':
29+
res.writeHead(200);
30+
res.end('hello world\n');
31+
break;
32+
default:
33+
common.mustNotCall(`Unexpected path: ${path}`)();
34+
}
35+
};
36+
37+
describe('use-system-ca', function() {
38+
async function setupServer(key, cert) {
39+
const theServer = https.createServer(
40+
{
41+
key: fixtures.readKey(key),
42+
cert: fixtures.readKey(cert),
43+
},
44+
handleRequest,
45+
);
46+
theServer.listen(0);
47+
await once(theServer, 'listening');
48+
49+
return theServer;
50+
}
51+
52+
let server;
53+
54+
beforeEach(async function() {
55+
if (isMainThread) {
56+
server = await setupServer('agent8-key.pem', 'agent8-cert.pem');
57+
} else {
58+
server = undefined;
59+
}
60+
});
61+
62+
it('trusts a valid root certificate', async function() {
63+
if (isMainThread) {
64+
const worker = new Worker(new URL(import.meta.url), {
65+
execArgv: ['--no-use-system-ca'],
66+
workerData: {
67+
url: `https://localhost:${server.address().port}/hello-world`,
68+
},
69+
});
70+
await fetch(`https://localhost:${server.address().port}/hello-world`);
71+
72+
const [message] = await once(worker, 'message');
73+
assert.strictEqual(message.ok, false);
74+
assert(
75+
message.code === 'UNABLE_TO_VERIFY_LEAF_SIGNATURE' ||
76+
message.code === 'SELF_SIGNED_CERT_IN_CHAIN',
77+
`Expected certificate verification error but got: ${JSON.stringify(
78+
message,
79+
)}`,
80+
);
81+
82+
const [exitCode] = await once(worker, 'exit');
83+
assert.strictEqual(exitCode, 0);
84+
} else {
85+
const { url } = workerData;
86+
try {
87+
const res = await fetch(url);
88+
const text = await res.text();
89+
parentPort.postMessage({ ok: true, status: res.status, text });
90+
} catch (err) {
91+
parentPort.postMessage({
92+
ok: false,
93+
code: err?.cause?.code,
94+
message: err.message,
95+
});
96+
}
97+
}
98+
});
99+
100+
afterEach(async function() {
101+
server?.close();
102+
});
103+
});

0 commit comments

Comments
 (0)