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

src: fix off-thread cert loading in bundled cert mode · nodejs/node@3f9c0e0 · GitHub

/ node Public

Commit 3f9c0e0

Browse files
joyeecheung
authored and
RafaelGSS
committed
src: fix off-thread cert loading in bundled cert mode
https://redirect.github.com/nodejs/node/pull/59856 had an typo/mistake in the skip conditions so that it is skipping when --use-openssl-ca or --openssl-system-ca-path (configure time) are NOT used, even though they should be skipped only when those ARE used (which is not the default for default builds). This change fixes that so that the perf numbers in that PR is true for the default build. PR-URL: #60764 Reviewed-By: Aditi Singh <aditisingh1400@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
1 parent 4e442b2 commit 3f9c0e0

6 files changed

Lines changed: 180 additions & 1 deletion

File tree

‎src/crypto/crypto_context.cc‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -901,7 +901,7 @@ void StartLoadingCertificatesOffThread(
901901
// loading.
902902
{
903903
Mutex::ScopedLock cli_lock(node::per_process::cli_options_mutex);
904-
if (!per_process::cli_options->ssl_openssl_cert_store) {
904+
if (per_process::cli_options->ssl_openssl_cert_store) {
905905
return;
906906
}
907907
}

‎test/common/tls.js‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -194,6 +194,7 @@ function extractMetadata(cert) {
194194
subject: x509.subject,
195195
};
196196
}
197+
exports.extractMetadata = extractMetadata;
197198

198199
// To compare two certificates, we can just compare serialNumber, issuer,
199200
// and subject like X509_comp(). We can't just compare two strings because
@@ -219,3 +220,12 @@ exports.includesCert = function includesCert(certs, cert) {
219220
};
220221

221222
exports.TestTLSSocket = TestTLSSocket;
223+
224+
// Dumps certs into a file to pass safely into test/fixtures/list-certs.js
225+
exports.writeCerts = function writeCerts(certs, filename) {
226+
const fs = require('fs');
227+
for (const cert of certs) {
228+
const x509 = new crypto.X509Certificate(cert);
229+
fs.appendFileSync(filename, x509.toString());
230+
}
231+
};

‎test/fixtures/list-certs.js‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
const assert = require('assert');
2+
const EXPECTED_CERTS_PATH = process.env.EXPECTED_CERTS_PATH;
3+
let expectedCerts = [];
4+
if (EXPECTED_CERTS_PATH) {
5+
const fs = require('fs');
6+
const file = fs.readFileSync(EXPECTED_CERTS_PATH, 'utf-8');
7+
const expectedCerts = file.split('-----END CERTIFICATE-----\n')
8+
.filter(line => line.trim() !== '')
9+
.map(line => line + '-----END CERTIFICATE-----\n');
10+
}
11+
12+
const tls = require('tls');
13+
const { includesCert, extractMetadata } = require('../common/tls');
14+
15+
const CERTS_TYPE = process.env.CERTS_TYPE || 'default';
16+
const actualCerts = tls.getCACertificates(CERTS_TYPE);
17+
for (const cert of expectedCerts) {
18+
assert(includesCert(actualCerts, cert), 'Expected certificate not found: ' + JSON.stringify(extractMetadata(cert)));
19+
}

‎test/parallel/test-tls-off-thread-cert-loading-disabled.js‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
'use strict';
2+
// This tests that when --use-openssl-ca is specified, no off-thread cert loading happens.
3+
4+
const common = require('../common');
5+
if (!common.hasCrypto) {
6+
common.skip('missing crypto');
7+
}
8+
const { spawnSyncAndAssert } = require('../common/child_process');
9+
const fixtures = require('../common/fixtures');
10+
const assert = require('assert');
11+
12+
spawnSyncAndAssert(
13+
process.execPath,
14+
[ '--use-openssl-ca', fixtures.path('list-certs.js') ],
15+
{
16+
env: {
17+
...process.env,
18+
NODE_DEBUG_NATIVE: 'crypto',
19+
NODE_EXTRA_CA_CERTS: fixtures.path('keys', 'fake-startcom-root-cert.pem'),
20+
CERTS_TYPE: 'default',
21+
}
22+
},
23+
{
24+
stderr(output) {
25+
assert.doesNotMatch(
26+
output,
27+
/Started loading bundled root certificates off-thread/
28+
);
29+
assert.doesNotMatch(
30+
output,
31+
/Started loading extra root certificates off-thread/
32+
);
33+
assert.doesNotMatch(
34+
output,
35+
/Started loading system root certificates off-thread/
36+
);
37+
return true;
38+
}
39+
}
40+
);

‎test/parallel/test-tls-off-thread-cert-loading-system.js‎

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
'use strict';
2+
3+
// This test verifies that system root certificates loading is loaded off-thread if
4+
// --use-system-ca is used.
5+
6+
const common = require('../common');
7+
if (!common.hasCrypto) {
8+
common.skip('missing crypto');
9+
}
10+
const tmpdir = require('../common/tmpdir');
11+
const { spawnSyncAndAssert } = require('../common/child_process');
12+
const fixtures = require('../common/fixtures');
13+
const assert = require('assert');
14+
const { writeCerts } = require('../common/tls');
15+
const tls = require('tls');
16+
17+
tmpdir.refresh();
18+
writeCerts([
19+
// Check that the extra cert is loaded.
20+
fixtures.readKey('fake-startcom-root-cert.pem'),
21+
// Check that the system certs are loaded.
22+
...tls.getCACertificates('system'),
23+
// Check that the bundled certs are loaded.
24+
...tls.getCACertificates('bundled'),
25+
], tmpdir.resolve('check-cert.pem'));
26+
27+
spawnSyncAndAssert(
28+
process.execPath,
29+
[ '--use-system-ca', '--use-bundled-ca', fixtures.path('list-certs.js') ],
30+
{
31+
env: {
32+
...process.env,
33+
NODE_DEBUG_NATIVE: 'crypto',
34+
NODE_EXTRA_CA_CERTS: fixtures.path('keys', 'fake-startcom-root-cert.pem'),
35+
EXPECTED_CERTS_PATH: tmpdir.resolve('check-cert.pem'),
36+
CERTS_TYPE: 'default',
37+
}
38+
},
39+
{
40+
stderr(output) {
41+
assert.match(
42+
output,
43+
/Started loading bundled root certificates off-thread/
44+
);
45+
assert.match(
46+
output,
47+
/Started loading extra root certificates off-thread/
48+
);
49+
assert.match(
50+
output,
51+
/Started loading system root certificates off-thread/
52+
);
53+
return true;
54+
}
55+
}
56+
);

‎test/parallel/test-tls-off-thread-cert-loading.js‎

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
'use strict';
2+
3+
// This test verifies that when --use-bundled-ca is used (default to true in default builds),
4+
// the loading of extra and bundled root certificates happens off the main thread.
5+
6+
const common = require('../common');
7+
if (!common.hasCrypto) {
8+
common.skip('missing crypto');
9+
}
10+
const tmpdir = require('../common/tmpdir');
11+
const { spawnSyncAndAssert } = require('../common/child_process');
12+
const fixtures = require('../common/fixtures');
13+
const assert = require('assert');
14+
const { writeCerts } = require('../common/tls');
15+
const tls = require('tls');
16+
17+
tmpdir.refresh();
18+
writeCerts([
19+
// Check that the extra cert is loaded.
20+
fixtures.readKey('fake-startcom-root-cert.pem'),
21+
// Check that the bundled certs are loaded.
22+
...tls.getCACertificates('bundled'),
23+
], tmpdir.resolve('check-cert.pem'));
24+
25+
spawnSyncAndAssert(
26+
process.execPath,
27+
[ '--no-use-system-ca', '--use-bundled-ca', fixtures.path('list-certs.js') ],
28+
{
29+
env: {
30+
...process.env,
31+
NODE_DEBUG_NATIVE: 'crypto',
32+
NODE_EXTRA_CA_CERTS: fixtures.path('keys', 'fake-startcom-root-cert.pem'),
33+
EXPECTED_CERTS_PATH: tmpdir.resolve('check-cert.pem'),
34+
CERTS_TYPE: 'default',
35+
}
36+
},
37+
{
38+
stderr(output) {
39+
assert.match(
40+
output,
41+
/Started loading bundled root certificates off-thread/
42+
);
43+
assert.match(
44+
output,
45+
/Started loading extra root certificates off-thread/
46+
);
47+
assert.doesNotMatch(
48+
output,
49+
/Started loading system root certificates off-thread/
50+
);
51+
return true;
52+
}
53+
}
54+
);

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL