Skip to content
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.

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

Update tls.md #46224

Merged
merged 1 commit into from
Jan 18, 2023
Merged

Update tls.md #46224

merged 1 commit into from
Jan 18, 2023

Conversation

tgerk
Copy link
Contributor

@tgerk tgerk commented Jan 16, 2023

tls.createServer() and new tls.Server() ignore secureContext option

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. tls Issues and PRs related to the tls subsystem. labels Jan 16, 2023
@Trott
Copy link
Member

Trott commented Jan 16, 2023

@nodejs/crypto @nodejs/http Putting aside formatting for a moment, is the content of this addition correct?

@tgerk
Copy link
Contributor Author

tgerk commented Jan 16, 2023

Yes, as of version 18.12.1. In brief, secureContext exists only to tls.Socket constructor options, not tls.Server.

I'm writing an FTP service that constructs a new TLS server for the data channel for each client session and believed there was some optimization in creating secureContext once. The documentation error, at least, needs to be fixed.

I reviewed source code to confirm. /lib/_tls_wrap.js line 1211 passes the whole tls.Server constructor options object to tls.Server.setSecureContext(). Those options are masticated and passed to tls.createSecureContext() to initialize tls.Server._sharedCreds. It's this property that is passed in secureContext option when constructing tls.Socket, same file line 1096 in function tlsConnectionListener

I'm not sure this is the correct or complete resolution. It's surprising that tls.Server options validation does not fail. The client error when connecting to a server constructed with secureContext fails cryptically: "Error: Client network socket disconnected before secure TLS connection was established"

@tgerk
Copy link
Contributor Author

tgerk commented Jan 16, 2023

Putting aside formatting for a moment

Totally agree--I had a vague hope the github editor would apply lint, pretty, etal on commit.

@Trott
Copy link
Member

Trott commented Jan 16, 2023

Putting aside formatting for a moment

Totally agree--I had a vague hope the github editor would apply lint, pretty, etal on commit.

Yeah, no problem there, I'm happy to fix the formatting etc. I just want confirmation from the relevant subsystem maintainers that the information is correct and complete, this is not a bug, etc.

@lpinca
Copy link
Member

lpinca commented Jan 16, 2023

I think f11f180 meant tls.connect() instead of tls.createServer(). See also 96a986d.

tls.createServer() and new tls.Server() ignore secureContext option.
@Trott Trott added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. labels Jan 16, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 18, 2023
@nodejs-github-bot nodejs-github-bot merged commit 6ecbd57 into nodejs:main Jan 18, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 6ecbd57

RafaelGSS pushed a commit that referenced this pull request Jan 20, 2023
tls.createServer() and new tls.Server() ignore secureContext option.

PR-URL: #46224
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Jan 20, 2023
juanarbol pushed a commit that referenced this pull request Jan 26, 2023
tls.createServer() and new tls.Server() ignore secureContext option.

PR-URL: #46224
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@juanarbol juanarbol mentioned this pull request Jan 28, 2023
juanarbol pushed a commit that referenced this pull request Jan 31, 2023
tls.createServer() and new tls.Server() ignore secureContext option.

PR-URL: #46224
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. doc Issues and PRs related to the documentations. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants