tls: make deprecated tls.createSecurePair() use public API #17882

addaleax · 2017-12-27T19:04:24Z

Make the deprecated tls.createSecurePair() method use other public
APIs only (TLSSocket in particular).

Since tls.createSecurePair() has been runtime-deprecated only
since Node 8, it probably isn’t quite time to remove it yet,
but this patch removes almost all of the code complexity that
is retained by it.

The API, as it is documented, is retained. However, it is very likely
that some users have come to rely on parts of undocumented API
of the SecurePair class, especially since some of the existing
tests checked for those. Therefore, this should definitely be
considered a breaking change.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

tls

addaleax · 2017-12-27T19:07:33Z

CI: https://ci.nodejs.org/job/node-test-commit/15097/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1176/
(both good)

I’m very open to just not considering this a good idea.

bnoordhuis

Nice! And that DuplexPair class is clever.

it is very likely that some users have come to rely on parts of undocumented API of the SecurePair class

Do you mean ._external? I have no problem with breaking that, that's always been an ill-considered hack.

bnoordhuis · 2017-12-27T23:00:17Z

src/node_crypto.cc

-  int flags = SSL_get_shutdown(ssl_);
-
-  if (flags & SSL_SENT_SHUTDOWN) {
-    Local<String> sent_shutdown_key = ssl_env()->sent_shutdown_string();


You can remove sent_shutdown_string, received_shutdown_string and onnewsessiondone_string from env.h, this is the only place they're used.

bnoordhuis · 2017-12-27T23:04:08Z

src/node_crypto.cc

-  }
-
-  Local<Object> obj = Object::New(env->isolate());
-  obj->Set(env->onselect_string(), args[0]);


onselect_string can be removed too.

addaleax · 2017-12-28T12:45:35Z

@bnoordhuis Thanks! I removed the unused strings here.

Nice! And that DuplexPair class is clever.

It’s from our test suite. :) I’ll think about maybe making it a public API in a follow-up PR, it seems like a useful primitive to me.

it is very likely that some users have come to rely on parts of undocumented API of the SecurePair class

Do you mean ._external? I have no problem with breaking that, that's always been an ill-considered hack.

At least one of the removed tests here used e.g. pair.ssl.setSNICallback. Also, there’s no real reason not to consider this semver-major, right? :)

BridgeAR

I think this is fantastic. As I don't do a full code review: rubberstamp LGTM.

maclover7 · 2017-12-31T17:24:36Z

Hmmm, I'm a little bit concerned about user breakage here. Since the function is deprecated and (fingers crossed?) can be removed in Node 10, is it worth it to rewrite the function if all of the deprecated code can "just" be removed soon-ish? Also, the irony of making a semver-major change on a deprecated function isn't lost on me yet 😄

addaleax · 2017-12-31T22:12:47Z

@maclover7 The thing is, this PR brings us into a position where we don’t have an incentive to remove this feature that early, because its maintenance cost is reduced by 95 % (or at least that’s what the diff size says).

And since stability is good for Node core & I think that means we should always take our time with removing/deprecating features, this PR allows us to do exactly that.

Make the deprecated `tls.createSecurePair()` method use other public APIs only (`TLSSocket` in particular). Since `tls.createSecurePair()` has been runtime-deprecated only since Node 8, it probably isn’t quite time to remove it yet, but this patch removes almost all of the code complexity that is retained by it. The API, as it is documented, is retained. However, it is very likely that some users have come to rely on parts of undocumented API of the `SecurePair` class, especially since some of the existing tests checked for those. Therefore, this should definitely be considered a breaking change.

TimothyGu

The code reduction is too good to pass up.

addaleax · 2018-01-14T13:54:17Z

Landed in 9301b8a

Make the deprecated `tls.createSecurePair()` method use other public APIs only (`TLSSocket` in particular). Since `tls.createSecurePair()` has been runtime-deprecated only since Node 8, it probably isn’t quite time to remove it yet, but this patch removes almost all of the code complexity that is retained by it. The API, as it is documented, is retained. However, it is very likely that some users have come to rely on parts of undocumented API of the `SecurePair` class, especially since some of the existing tests checked for those. Therefore, this should definitely be considered a breaking change. PR-URL: #17882 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Minwoo Jung <minwoo@nodesource.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

BridgeAR · 2018-01-14T13:57:38Z

I think it is fine but this landed with only one @nodejs/tsc member giving a LG.

addaleax · 2018-01-14T14:01:40Z

Oh, right, thanks for catching … I kinda still had it in my mind that @bnoordhuis was one of the TSC people. Want me to rebase it out of master?

BridgeAR · 2018-01-14T14:15:07Z

No, as I said: I personally think it is fine and I know that it is sometimes difficult to get enough people having a look.

mcollina · 2018-01-14T15:30:21Z

LGTM after the fact

addaleax added 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. labels Dec 27, 2017

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Dec 27, 2017

bnoordhuis approved these changes Dec 27, 2017

View reviewed changes

jasnell approved these changes Dec 27, 2017

View reviewed changes

JungMinu approved these changes Dec 28, 2017

View reviewed changes

BridgeAR approved these changes Dec 28, 2017

View reviewed changes

addaleax force-pushed the tls-remove-legacy branch from fbb6e4e to 6123768 Compare December 31, 2017 21:39

addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 2, 2018

arthurschreiber mentioned this pull request Jan 6, 2018

tls.createSecurePair is being runtime-deprecated tediousjs/tedious#515

Closed

addaleax force-pushed the tls-remove-legacy branch from 6123768 to eeae308 Compare January 6, 2018 12:11

TimothyGu approved these changes Jan 13, 2018

View reviewed changes

addaleax removed lib / src Issues and PRs related to general changes in the lib or src directory. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Jan 14, 2018

addaleax closed this Jan 14, 2018

addaleax deleted the tls-remove-legacy branch January 14, 2018 13:54

joyeecheung mentioned this pull request Jan 15, 2018

tls_wrap: migrate synchronous errors #18125

Closed

4 tasks

This was referenced Apr 20, 2018

Add an easy way to start a TLS communication on top of a plain *server side* Socket #13368

Closed

Performance Regression: tls.createSecurePair() Not Buffering Connections in v10 #20263

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tls: make deprecated tls.createSecurePair() use public API #17882

tls: make deprecated tls.createSecurePair() use public API #17882

addaleax commented Dec 27, 2017

addaleax commented Dec 27, 2017 •

edited

Loading

bnoordhuis left a comment

bnoordhuis Dec 27, 2017

bnoordhuis Dec 27, 2017

addaleax commented Dec 28, 2017

BridgeAR left a comment

maclover7 commented Dec 31, 2017

addaleax commented Dec 31, 2017

TimothyGu left a comment

addaleax commented Jan 14, 2018

BridgeAR commented Jan 14, 2018

addaleax commented Jan 14, 2018

BridgeAR commented Jan 14, 2018

mcollina commented Jan 14, 2018

tls: make deprecated tls.createSecurePair() use public API #17882

tls: make deprecated tls.createSecurePair() use public API #17882

Conversation

addaleax commented Dec 27, 2017

Checklist

Affected core subsystem(s)

addaleax commented Dec 27, 2017 • edited Loading

bnoordhuis left a comment

Choose a reason for hiding this comment

bnoordhuis Dec 27, 2017

Choose a reason for hiding this comment

bnoordhuis Dec 27, 2017

Choose a reason for hiding this comment

addaleax commented Dec 28, 2017

BridgeAR left a comment

Choose a reason for hiding this comment

maclover7 commented Dec 31, 2017

addaleax commented Dec 31, 2017

TimothyGu left a comment

Choose a reason for hiding this comment

addaleax commented Jan 14, 2018

BridgeAR commented Jan 14, 2018

addaleax commented Jan 14, 2018

BridgeAR commented Jan 14, 2018

mcollina commented Jan 14, 2018

addaleax commented Dec 27, 2017 •

edited

Loading