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: remove NPN (next protocol negotation) support #19403

Merged
merged 2 commits into from
Mar 27, 2018
Merged

tls: remove NPN (next protocol negotation) support #19403

merged 2 commits into from
Mar 27, 2018

Conversation

bnoordhuis
Copy link
Member

NPN has been superseded by ALPN. Chrome and Firefox removed support for
NPN in 2016 and 2017 respectively to no ill effect.

Fixes: #14602

cc @nodejs/crypto

@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Mar 17, 2018
@bnoordhuis bnoordhuis mentioned this pull request Mar 17, 2018
Closed
vsemozhetbyt
vsemozhetbyt reviewed Mar 17, 2018
View reviewed changes
doc/api/deprecations.md Outdated
@@ -950,6 +950,13 @@ deprecated if the assigned value is not a string, boolean, or number. In the
future, such assignment may result in a thrown error. Please convert the
property to a string before assigning it to `process.env`.

<a id="DEP0105"></a>
Copy link
Contributor

Choose a reason for hiding this comment

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

A nit: DEP0105 -> DEP00XX?

vsemozhetbyt
vsemozhetbyt reviewed Mar 17, 2018
View reviewed changes
doc/api/tls.md Outdated

When ALPN has no selected protocol, `tlsSocket.alpnProtocol` returns `false`.
The `tlsSocket.alpnProtocol` property is a string that contains the selected
ALPN protocol. When ALPN has no selected protocol, `tlsSocket.alpnProtocol`
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt Mar 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

A nit: excess space in protocol. When

@vsemozhetbyt vsemozhetbyt added tls Issues and PRs related to the tls subsystem. crypto Issues and PRs related to the crypto subsystem. https Issues or PRs related to the https subsystem. labels Mar 17, 2018
@mscdex mscdex added the semver-major PRs that contain breaking changes and should be released in the next major version. label Mar 17, 2018
@bnoordhuis
Copy link
Member Author

@nodejs/crypto Can I get a review, please?

@nodejs/tsc You should probably also weigh in, if not on the exact implementation than at least on the overall direction. See discussion in #14602 for rationale on expediting this.

tniessen
tniessen approved these changes Mar 19, 2018
View reviewed changes
Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

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

Mostly LGTM, I personally cannot estimate the impact of ALPN/NPN support, but the linked issue seems to have discussed that already.

doc/api/deprecations.md Outdated

Type: Runtime

This was never a documented feature.
Copy link
Member

Choose a reason for hiding this comment

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

Nit: It would be great if you could add a sentence explaining why we decided to remove the API instead of documenting it.

Copy link
Member

Choose a reason for hiding this comment

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

Agree. NPN is a documented feature making this comment a bit confusing.

@Trott
Copy link
Member

Trott commented Mar 19, 2018

@nodejs/tsc You should probably also weigh in, if not on the exact implementation than at least on the overall direction.

No comments on the implementation, but on overall concept, SGTM.

jasnell
jasnell reviewed Mar 19, 2018
View reviewed changes
doc/api/deprecations.md Outdated
<a id="DEP0105"></a>
### DEP0105: tls.convertNPNProtocols()

Type: Runtime
Copy link
Member

@jasnell jasnell Mar 19, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

If the capability is being removed entirely, this should be Type: End-of-Life Nevermind, I see that this specific function is retained for the time being.

jasnell
jasnell reviewed Mar 19, 2018
View reviewed changes
lib/tls.js Outdated
// If protocols is Array - translate it into buffer
if (Array.isArray(protocols)) {
out.NPNProtocols = convertProtocols(protocols);
} else if (isUint8Array(protocols)) {
// Copy new buffer not to be modified by user.
out.NPNProtocols = Buffer.from(protocols);
}
};
}, 'tls.convertNPNProtocols() is deprecated.', 'DEP0105');
Copy link
Member

Choose a reason for hiding this comment

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

s/DEP0105/DEP00XX ... the code is to be assigned when the PR is landed.

@bnoordhuis
Copy link
Member Author

Incorporated feedback. CI: https://ci.nodejs.org/job/node-test-pull-request/13877/

@bnoordhuis
Copy link
Member Author

With mkssldef tweak: https://ci.nodejs.org/job/node-test-pull-request/13884/

@bnoordhuis
tls: drop NPN (next protocol negotiation) support
5bfbe5c 
NPN has been superseded by ALPN.  Chrome and Firefox removed support for
NPN in 2016 and 2017 respectively to no ill effect.

Fixes: nodejs#14602
PR-URL: nodejs#19403
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@bnoordhuis
tls: runtime-deprecate tls.convertNPNProtocols()
9204a0d 
Fixes: nodejs#14602
PR-URL: nodejs#19403
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@bnoordhuis bnoordhuis closed this Mar 27, 2018
@bnoordhuis bnoordhuis deleted the fix14602 branch March 27, 2018 14:23
@bnoordhuis bnoordhuis merged commit 9204a0d into nodejs:master Mar 27, 2018
@bnoordhuis
Copy link
Member Author

Landed in b3f2391...9204a0d.

@ChALkeR ChALkeR mentioned this pull request Apr 13, 2018
Closed
4 tasks
@tniessen tniessen mentioned this pull request Apr 24, 2018
Closed
2 tasks
tniessen added a commit that referenced this pull request Apr 24, 2018
@tniessen
doc: fix spelling of API name in 10.0.0 changelog
ad5307f 
PR-URL: #20257
Refs: #19403
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins pushed a commit that referenced this pull request May 4, 2018
@tniessen @MylesBorins
doc: fix spelling of API name in 10.0.0 changelog
3bc5353 
PR-URL: #20257
Refs: #19403
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@SimonSchick SimonSchick mentioned this pull request Jul 5, 2019
Merged
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vsemozhetbyt vsemozhetbyt vsemozhetbyt left review comments

@tniessen tniessen tniessen approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
crypto Issues and PRs related to the crypto subsystem. https Issues or PRs related to the https subsystem. lib / src Issues and PRs related to general changes in the lib or src directory. 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.
7 participants
@bnoordhuis @Trott @jasnell @tniessen @vsemozhetbyt @mscdex @nodejs-github-bot