tls: improve performance of getAllowUnauthorized

[Uzlopak]

Improves the performance of getAllowUnauthorized. It is called on every call of tls connect.

before:

aras@aras-Lenovo-Legion-5-17ARH05H:~/workspace/node$ node ./benchmark/tls/get-allow-unauthorized.js 
tls/get-allow-unauthorized.js n=16777216: 4,760,304.7689979095

after:

aras@aras-Lenovo-Legion-5-17ARH05H:~/workspace/node$ ./node ./benchmark/tls/get-allow-unauthorized.js 
tls/get-allow-unauthorized.js n=16777216: 169,041,177.4754851

[H4ad]

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.

[Uzlopak]

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.

[H4ad]

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.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.09%. Comparing base (3cbeed8) to head (f2bddcc).
Report is 5 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #54297      +/-   ##
==========================================
- Coverage   87.11%   87.09%   -0.02%     
==========================================
  Files         647      647              
  Lines      181773   181799      +26     
  Branches    34889    34885       -4     
==========================================
- Hits       158347   158335      -12     
- Misses      16739    16773      +34     
- Partials     6687     6691       +4     

Files	Coverage Δ
lib/internal/options.js	100.00% <100.00%> (ø)

... and 30 files with indirect coverage changes

[Uzlopak]

@nodejs/security

Can this be discussed on the security perspective?