Skip to content

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

Merged
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
pimterry:fix-tls-1.3-client-cert-resume
Aug 8, 2026
Merged

tls: fix authorized state on no-cert TLS1.3 client cert resumption#64677
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
pimterry:fix-tls-1.3-client-cert-resume

Conversation

@pimterry

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/crypto
  • @nodejs/net

@nodejs-github-botnodejs-github-bot added needs-ci PRs that need a full CI run. tls Issues and PRs related to the tls subsystem. labels Jul 22, 2026
@pimterry
pimterryforce-pushed the fix-tls-1.3-client-cert-resume branch from 005f950 to 8ec1d60CompareJuly 22, 2026 14:59
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
the resumed session would report authorized=true. This doesn't match TLS
1.2 behaviour or make any sense, and was purely an artifact of our
internal logic for handling TLS 1.3 resumption details. We now correctly
report the authorization state and/or error from the original connection
in all cases, with a matrix test that fully checks the invariant:
authorized state after resume should always match the initial state.
Signed-off-by: Tim Perry <pimterry@gmail.com>
@pimterry
pimterryforce-pushed the fix-tls-1.3-client-cert-resume branch from 8ec1d60 to 428a444CompareJuly 22, 2026 17:13
@pimterry

Copy link
Copy Markdown
MemberAuthor

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

codecovBot commented Jul 22, 2026

Copy link
Copy Markdown

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 linesCoverage Δ
lib/internal/tls/wrap.js95.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.

@pimterrypimterry added commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. request-ci Add this label to start a Jenkins CI on a PR. labels Jul 23, 2026
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 23, 2026
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@trivikr

Copy link
Copy Markdown
Member

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

@pimterrypimterry added the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 7, 2026
@pimterry

Copy link
Copy Markdown
MemberAuthor

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

@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 7, 2026
@panvapanva added the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 8, 2026
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 8, 2026
@nodejs-github-bot
nodejs-github-bot merged commit c59cd6b into nodejs:mainAug 8, 2026
88 of 89 checks passed
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Landed in c59cd6b

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

commit-queue-squashAdd this label to instruct the Commit Queue to squash all the PR commits into the first one.needs-ciPRs that need a full CI run.tlsIssues and PRs related to the tls subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Reloading page bypasses TLS client authentication

5 participants

@pimterry@nodejs-github-bot@trivikr@jasnell@panva