Skip to content

tls: improve performance of getAllowUnauthorized #54297

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

tls: improve performance of getAllowUnauthorized #54297

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions benchmark/tls/get-allow-unauthorized.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
'use strict';

const common = require('../common.js');
const assert = require('assert');

const options = {
flags: ['--expose-internals'],
};

const bench = common.createBenchmark(main, {
n: [1024 * 1024 * 16],
}, options);

function main({ n }) {
const { getAllowUnauthorized } = require('internal/options');

const length = 1024;
const array = new Array(length).fill(false);

bench.start();

for (let i = 0; i < n; ++i) {
const index = i % length;
array[index] = getAllowUnauthorized();
}

bench.end(n);

// Verify the entries to prevent dead code elimination from making
// the benchmark invalid.
for (let i = 0; i < length; ++i) {
assert.strictEqual(typeof array[i], 'boolean');
}
}
18 changes: 10 additions & 8 deletions lib/internal/options.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ const {
} = internalBinding('options');

let warnOnAllowUnauthorized = true;
let allowUnauthorized = false;

let optionsDict;
let cliInfo;
Expand Down Expand Up @@ -47,15 +48,16 @@ function getOptionValue(optionName) {
}

function getAllowUnauthorized() {
const allowUnauthorized = process.env.NODE_TLS_REJECT_UNAUTHORIZED === '0';

if (allowUnauthorized && warnOnAllowUnauthorized) {
if (warnOnAllowUnauthorized) {
warnOnAllowUnauthorized = false;
process.emitWarning(
'Setting the NODE_TLS_REJECT_UNAUTHORIZED ' +
'environment variable to \'0\' makes TLS connections ' +
'and HTTPS requests insecure by disabling ' +
'certificate verification.');
allowUnauthorized = process.env.NODE_TLS_REJECT_UNAUTHORIZED === '0';
if (allowUnauthorized) {
Comment on lines 52 to +54
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warnOnAllowUnauthorized should only be set false when both conditions are true, otherwise we will not emit the warning when the NODE_TLS_REJECT_UNAUTHORIZED is turned on.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But this is exactly what it does?!

on initial warnOnAllowUnauthorized is set to true. so the if in line 51 is entered. we set warnOnAllowUnauthorized to false, as we are only interested into warning once. We check if allowUnauthorized is set to 0, and cache it.. If it is true, we enter the if block in line 54. we emit the warning. anyhow we cached the value and return it.

The second time we cal getAllowUnauthorized warnOnAllowUnauthorized is set to false. We dont need to check anything else as we dont need to emit a warning anymore. we return the cached result.

ANd imho NODE_TLS_REJECT_UNAUTHORIZED should only be set, at start up and not at runtime, as this could mean, that a malicious package could turn off the tls check.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we set warnOnAllowUnauthorized to false, as we are only interested into warning once.

This code basically emits a warning when that flag is enabled for the first time, whether it is during startup or runtime.

ANd imho NODE_TLS_REJECT_UNAUTHORIZED should only be set, at start up and not at runtime

Ideally, yes, practically, people do things very differently than we expect, anyway, I don't feel comfortable approving in the current way (breaking the old behavior) but let's see if we can get more people who have more knowledge on this topic.

Somehow, this is also something related to security, so I don't think hiding the warning when the flag is enabled runtime is a good thing.

process.emitWarning(
'Setting the NODE_TLS_REJECT_UNAUTHORIZED ' +
'environment variable to \'0\' makes TLS connections ' +
'and HTTPS requests insecure by disabling ' +
'certificate verification.');
}
}
return allowUnauthorized;
}
Expand Down
Loading