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.

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

Jump to bottom

tls: deprecate undocumented tlsSocket.ssl property, add docs #23915

Closed
wants to merge 3 commits into from
Closed

tls: deprecate undocumented tlsSocket.ssl property, add docs #23915

wants to merge 3 commits into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Oct 26, 2018

Updated take on #10846

Fixes: #10555

/cc @sam-github

Checklist

@nodejs-github-bot
Copy link
Collaborator

@jasnell build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1385/pipeline

@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 Oct 26, 2018
jasnell
jasnell commented Oct 26, 2018
View reviewed changes
doc/api/tls.md Outdated Show resolved Hide resolved
jasnell
jasnell commented Oct 26, 2018
View reviewed changes
doc/api/tls.md Outdated Show resolved Hide resolved
vsemozhetbyt
vsemozhetbyt reviewed Oct 27, 2018
View reviewed changes
doc/api/tls.md Outdated Show resolved Hide resolved
vsemozhetbyt
vsemozhetbyt reviewed Oct 27, 2018
View reviewed changes
doc/api/tls.md Outdated Show resolved Hide resolved
addaleax
addaleax reviewed Oct 27, 2018
View reviewed changes
doc/api/tls.md Outdated Show resolved Hide resolved
doc/api/tls.md Outdated Show resolved Hide resolved
vsemozhetbyt
vsemozhetbyt reviewed Oct 27, 2018
View reviewed changes
doc/api/tls.md Outdated Show resolved Hide resolved
vsemozhetbyt
vsemozhetbyt reviewed Oct 27, 2018
View reviewed changes
doc/api/tls.md Outdated Show resolved Hide resolved
jasnell
jasnell commented Oct 29, 2018
View reviewed changes
doc/api/tls.md Outdated Show resolved Hide resolved
jasnell
jasnell commented Oct 29, 2018
View reviewed changes
doc/api/tls.md Outdated Show resolved Hide resolved
jasnell
jasnell commented Oct 29, 2018
View reviewed changes
doc/api/tls.md Outdated Show resolved Hide resolved
@jasnell jasnell force-pushed the tls-socket-take2 branch 2 times, most recently from 02e937e to 66849e7 Compare October 30, 2018 15:44
@jasnell jasnell changed the title doc: tls API for direct TLS socket use tls: deprecate undocumented tlsSocket.ssl property, add docs Oct 30, 2018
@jasnell jasnell added semver-major PRs that contain breaking changes and should be released in the next major version. deprecations Issues and PRs related to deprecations. labels Oct 30, 2018
@jasnell
Copy link
Member Author

jasnell commented Oct 30, 2018

@addaleax @sam-github @bnoordhuis @nodejs/tsc ... please take another look over. This has changed significantly and is now a semver-major deprecation.

@sam-github
Copy link
Contributor

Are the things people were using .ssl for possible via public APIs?

@jasnell
Copy link
Member Author

jasnell commented Oct 30, 2018

I've added verifyError() as a function on TLSSocket.prototype but I'm really not sure what else folks have been using beyond that. We can address those cases as needed.

vsemozhetbyt
vsemozhetbyt reviewed Oct 30, 2018
View reviewed changes
doc/api/tls.md Outdated Show resolved Hide resolved
@jasnell
Copy link
Member Author

jasnell commented Nov 1, 2018

Ping @nodejs/tsc

@jasnell jasnell requested a review from addaleax November 1, 2018 14:35
addaleax
addaleax approved these changes Nov 1, 2018
View reviewed changes
lib/_tls_wrap.js Outdated Show resolved Hide resolved
@jasnell
tls: deprecate undocumented .ssl property, add docs
4ca0e4c 
deprecate the legacy undocumented `.ssl` alias for the
`TLSSocket._handle` and document alternatives. Document
how to properly use the `TLSSocket` constructor directly.

Updated take on nodejs#10846

Fixes: nodejs#10555
@jasnell
[Squash] use kHandle
c82010d
Trott
Trott reviewed Nov 2, 2018
View reviewed changes
doc/api/deprecations.md Outdated Show resolved Hide resolved
Trott
Trott reviewed Nov 2, 2018
View reviewed changes
doc/api/tls.md
@@ -805,6 +856,21 @@ and their processing can be delayed due to packet loss or reordering. However,
smaller fragments add extra TLS framing bytes and CPU overhead, which may
decrease overall server throughput.

### tlsSocket.verifyError()
Copy link
Member

Choose a reason for hiding this comment

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

Sorry if these questions are naïve or otherwise ill-informed: Is this the best name for this? I know it's what we used previously, but it wasn't exposed publicly, was it? Might `validatePeerCertificate() or something like that be a better name?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm fine with whatever name folks would like to see.

@Trott @jasnell
[Squash] suggestion
5116f58 
Co-Authored-By: jasnell <[email protected]>
@jasnell
Copy link
Member Author

jasnell commented Nov 19, 2018

Ping @nodejs/tsc

@rvagg
Copy link
Member

rvagg commented Nov 19, 2018

there's an outstanding question from @Trott inline

lgtm I think, although the commit description doesn't hint at the addition of features. So it might be best to split out verifyError() to a separate commit.

@jasnell jasnell closed this Jan 28, 2019
@armanbilge armanbilge mentioned this pull request Jul 26, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bnoordhuis bnoordhuis bnoordhuis left review comments

@Trott Trott Trott left review comments

@vsemozhetbyt vsemozhetbyt vsemozhetbyt left review comments

@sam-github sam-github sam-github approved these changes

@addaleax addaleax addaleax approved these changes

Assignees
No one assigned
Labels
deprecations Issues and PRs related to deprecations. doc Issues and PRs related to the documentations. semver-major PRs that contain breaking changes and should be released in the next major version. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@jasnell @nodejs-github-bot @sam-github @rvagg @bnoordhuis @Trott @addaleax @vsemozhetbyt