tls: fix authorized state on no-cert TLS1.3 client cert resumption

[pimterry]

Previously if you used TLS 1.3 and the server requested a client cert, but the client didn't send one, and you used rejectUnauthorized: false (e.g. to manually verify state with some extra conditions) then the initial session would report authorized=false, but resuming the session later would report authorized=true.

This doesn't match TLS 1.2 behaviour (which correctly preserves authorization state & errors across resumption). It's not inevitable and it wasn't intentional - this is an artifact of our internal logic for handling PSKs in TLS 1.3 inside ncrypto (here) that wasn't properly validated on this side.

We now correctly report the authorization state and/or error from the original connection in all cases. This also adds a matrix test that fully checks the invariant over all combinations of TLS 1.2/1.3 + valid-cert/invalid-cert/no-cert: authorized state after resume should always match the initial state.

Fixes #35317 - note there is some debate in there (cc @bnoordhuis) but I think there's confusion in some of the descriptions and it's actually just a clear bug.

See also #64584 which was opened just now, to simply document the current state.

This PR is either a fix for the current state (in which case we could close #64584) or a semver-major change (in which case we can keep the document PR for now, and merge this to clean that up to change the behaviour as a major bump). Opinions from @nodejs/crypto would be helpful. This is arguably a security fix but it has been directly described in public in that issue as intended behaviour for nearly 6 years.

The scope is limited to rejectUnauthorized: false cases, which does imply a degree of taking your own responsibility for validation - but even if you enable that option we still shouldn't lie to you unnecessarily, as we do today.

[nodejs-github-bot]

Review requested:

• @nodejs/crypto
• @nodejs/net

[pimterry]

Boringssl is an actual failure here... Seems TLS resumption works quite differently over there and so the tests stall. I'll dig into it tomorrow.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.13%. Comparing base (fbab458) to head (892a748).
⚠️ Report is 258 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #64677      +/-   ##
==========================================
- Coverage   90.15%   90.13%   -0.02%     
==========================================
  Files         741      741              
  Lines      242207   242119      -88     
  Branches    45606    45598       -8     
==========================================
- Hits       218354   218228     -126     
- Misses      15350    15388      +38     
  Partials     8503     8503              

Files with missing lines	Coverage Δ
lib/internal/tls/wrap.js	95.12% <100.00%> (+0.01%)	⬆️

... and 37 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/75086/

[trivikr]

@pimterry Did you mean to add commit-queue label too?

[pimterry]

Thanks for the reminder @trivikr! Lost this somewhere, added to the queue.

[nodejs-github-bot]

Landed in c59cd6b

[tgies]

FYI: This caused #65579 by tripping over the pre-existing yet unreported bug that getPeerX509Certificate() actually mutates the certificate chain object owned by the TLS stack as it walks it. Filed #65602 which fixes getPeerX509Certificate() and adds a new cheaper hasPeerCertificate() to use here rather than materializing the whole cert chain just to test if it's null.