tls.createSecurePair is being runtime-deprecated

[ChALkeR]

Ref: nodejs/node#11349

Used in:

tedious-1.14.0.tgz/lib/message-io.js:117:      this.securePair = tls.createSecurePair(credentials);

See also #135.
See e.g. sidorares/node-mysql2#367 for a feature detection example.

[tvrprasad]

@ChALkeR Thanks for the pointer. This will be helpful when we get to this. Feel free to send a PR if you feel inspired :-)

[arthurschreiber]

@tvrprasad: @ChALkeR is part of the Node.js team and this post is mostly intended as a heads-up for us to know that this feature is deprecated. I don't think he actually uses tedious. 😄

@ChALkeR: I've known about this deprecation for a while, but the documentation around this deprecation is really sparse. The examples I've seen so far (like the PR for node-mysql) focus on "simple" TLS upgrades, where the connection is just switched from non-TLS to TLS.

In TDS, the TLS upgrade is a lot more involved, all the handshaking packets need to be wrapped into TDS messages, and then once the connection is established, all further TLS packets don't need to be wrapped anymore. Implementing this via tls.createSecurePair was pretty straightforward, but the last time I tried this via TLSSocket, I could not get it to work. 😞

I'll try again when I get to it, but i don't think much has changed.

Another issue I ran into is that wrapping a socket from the readable-stream package into a TLSSocket would cause a segfault (!) of the Node.js process. Unfortunately, I don't have reproduction steps handy right now. 😞

[tvrprasad]

@arthurschreiber Thanks for the info :-)

but the last time I tried this via TLSSocket, I could not get it to work.

If this work is on github, please send me a pointer. I'm curious to check it out.

[ariskemper]

@tvrprasad @arthurschreiber Here is information on how to replace tls.createSecurePair https://nodejs.org/dist/latest-v6.x/docs/api/tls.html#tls_class_securepair

[olange]

Encoutered the same issue on Node v8.0.1. My app reports following error, after trying to establish an encrypted connexion:

(node:6344) [DEP0064] DeprecationWarning: tls.createSecurePair() is deprecated. Please use tls.Socket instead.

I am using patriksimek/node-mssql and the tedious driver (the default driver of node-mssql) to access our Microsoft SQL Server database.

[deluxor]

Same issue as @olange using node-mssql with tedious driver.

Listening on port 4000
(node:4402) [DEP0064] DeprecationWarning: tls.createSecurePair() is deprecated. Please use tls.Socket instead.

Please don't take me wrong and I really appreciate your efforts keeping this module up but Is there any prediction when it will be fixed?

[CunhaGus]

Using the package 'mssql' and using options:
{
encrypt: true
}

(node:13508) [DEP0064] DeprecationWarning: tls.createSecurePair() is deprecated. Please use tls.Socket instead.

[akanieski]

When Node team deprecates something like this whats the timeframe for removal in Node? Just curious.

[bminer]

Probably will be removed in Node 9 or 10. The feature has been doc-deprecated for at least 4 years. :-/ This is one of the only popular NPM packages still using it.

[iamvijaydev]

Does anyone from collaborators have any timeline on when the change will be released?

[Iiridayn]

Getting this today since I just updated our systems to node 8.9.0 lts. I've got two heavy projects right now, but if they both ease up, I'll see if I can spare some time to fixing this.

[Suraiya-Hameed]

@Iiridayn That would be awesome!
I did have a look at it, but couldn't figure out how to get the handshake packet from TLSSocket, stuck at same point as #515 (comment) 😕
Would love to hear if you have ideas on that.

[Jorgeee1]

Any progress on that? I'm facing the same issue trying to connect to Azure, just if it helps.
I would be grateful if someone explained quickly to me what is the consequence of running an app while this is deprecated, the SQL queries I request aren't executed and I need to know if this is the cause, but the texts on this I've read aren't very eloquent.

Thank you in advance.

[jhard]

@Jorgeee1 This is only a warning, it is unlikely to be the reason for your trouble. I am using tedious to connect to Azure DB, too, and it's working fine.

[joux3]

I wrote a quick proof of concept of replacing createSecurePair with TLSSocket+ a TCP server to get the raw handshake data. You can take a look at it here. The integration tests that I could run passed.

This is obviously not a real solution to the issue at hand (unless eating extra sockets etc is OK). This is more of a discussion starter. It would be good to have actual suggestions and discussion on nodejs/help#1010.

[Suraiya-Hameed]

@joux3 that's awesome 😃 Can you create a PR? It will be easier to review and discuss there.

[Iiridayn]

Heavy projects have not eased up, a third has been added, and we've migrated to MySQL as well. Sorry I won't be able to fix this one - best of luck to you :).

[saostad]

Please fix it, it is important problem

[arthurschreiber]

Hey y'all,

let me give a quick update on the current status here. I understand that the deprecation message might be a nuisance, but it is not really a problem. tls.createSecurePair might be deprecated, but for the short- and medium-term future, it is not going to go anywhere. According to nodejs/node#17882, it won't be removed soon.

Also, nodejs/node#17882 proposes a re-implementation of tls.createSecurePair in terms of public node.js API, introducing a (for now) internal DuplexSocket class. The basics of this implementation are exactly what we need for tedious to move away from tls.createSecurePair, without resorting to interesting 😁 but not really viable hacks as proposed by @joux3. We'll probably have to create our own DuplexSocket version for now, but at least I now have a good idea on how to solve this in tedious once it is clear when tls.createSecurePair will actually be removed.

Please let us know what you think! 😄

[addaleax]

@arthurschreiber Fwiw, I also just published that DuplexPair implementation at https://www.npmjs.com/package/duplexpair. I guess you could build a wrapper around that to have a tls.createSecurePair() lookalike pretty easily?

[arthurschreiber]

@addaleax Sweet! ❤️ This is great and I'll see if I can hook this up and get us moved away from tls.createSecurePair sooner rather than later! Thank you so much! 🙇

[addaleax]

@arthurschreiber I think a patch for this could look as simple as this:

diff --git a/package.json b/package.json
index bca5b266849d..8cc0ed957577 100644
--- a/package.json
+++ b/package.json
@@ -43,6 +43,7 @@
     "babel-runtime": "^6.26.0",
     "big-number": "0.3.1",
     "bl": "^1.2.0",
+    "duplexpair": "^1.0.1",
     "iconv-lite": "^0.4.11",
     "readable-stream": "^2.2.6",
     "sprintf": "0.1.5"
diff --git a/src/message-io.js b/src/message-io.js
index 6c2a15c32a9f..a1de3a1e91c0 100644
--- a/src/message-io.js
+++ b/src/message-io.js
@@ -1,7 +1,9 @@
+'use strict';
 const tls = require('tls');
 const crypto = require('crypto');
 const EventEmitter = require('events').EventEmitter;
 const Transform = require('readable-stream').Transform;
+const DuplexPair = require('duplexpair');
 
 const Packet = require('./packet').Packet;
 const TYPE = require('./packet').TYPE;
@@ -84,10 +86,14 @@ module.exports = class MessageIO extends EventEmitter {
   startTls(credentialsDetails, hostname, trustServerCertificate) {
     const credentials = tls.createSecureContext ? tls.createSecureContext(credentialsDetails) : crypto.createCredentials(credentialsDetails);
 
-    this.securePair = tls.createSecurePair(credentials);
+    const duplexpair = new DuplexPair();
+    this.securePair = {
+      cleartext: new tls.TLSSocket(duplexpair.socket1),
+      encrypted: duplexpair.socket2
+    };
     this.tlsNegotiationComplete = false;
 
-    this.securePair.on('secure', () => {
+    this.securePair.cleartext.on('secure', () => {
       const cipher = this.securePair.cleartext.getCipher();
 
       if (!trustServerCertificate) {

But this doesn’t seem to be tested as part of the regular test suite, so I’m not sure how to go about that…

[joux3]

I gave a yet another shot at replacing SecurePair. The integration tests seem to be fine, tested on Node 9. joux3@6d255fb

However I have some observations (but maybe the patch is good enough for a PR to discuss the issues?):

• Node.js 6 seems to totally crash on Assertion failed: (uv__stream_fd(stream) >= 0), function uv_read_start, file ../deps/uv/src/unix/stream.c, line 1517.
• I had to remove timer faking from retry timeout test, the fake timer seems to break DuplexPair internally
• I didn't test unencrypted connections. Those might be affected by the logic flow changes (signals didn't seem to propagate correctly through pipes so there's a manual close handler on the socket)'

Huge thanks to @addaleax for your work!

[jimmont]

@joux3 is your fork ready for use with Node 8--the current LTS release? and is it possibly to open a pull request? I'd like to see if it resolves an issue I have using it in conjunction with https://github.com/patriksimek/node-mssql/ thank you!

[addaleax]

I had to remove timer faking from retry timeout test, the fake timer seems to break DuplexPair internally

@joux3 Which test would that be? (I’m sorry, I’m not particularly familiar with this code base beyond what I’ve looked at for this issue)

[joux3]

@addaleax, the failing test is

tedious/test/integration/connection-retry-test.js

Lines 73 to 104 in f5a0f95

	'no retries if connection timeout fires': function(test) {
	const config = getConfig();
	config.options.connectTimeout = config.options.connectionRetryInterval / 2;
	const clock = this.sinon.useFakeTimers();
	test.expect(1);
	this.sinon.stub(TransientErrorLookup.prototype, 'isTransientError', (error) => {
	return error === this.invalidLoginError;
	});
	const connection = new Connection(config);
	connection.on('retry', () => {
	test.ok(false);
	});
	connection.on('errorMessage', () => {
	// Forward clock past connectTimeout which is less than retry interval.
	clock.tick(config.options.connectTimeout + 1);
	});
	connection.on('connect', (err) => {
	test.ok(err);
	});
	connection.on('end', (info) => {
	clock.restore();
	test.done();
	});
	},

Disabling the timer's here: https://github.com/tediousjs/tedious/pull/689/files#diff-ed1dc3ba8a10a8fb4b18a528b35bad00.
Edit: I restored timer faking by only faking setTimeout ff5d5eb.

@jimmont, I opened a pull request at #689 (looks like referencing the issue id in the PR name is not enough...). Integration tests pass on Node 8, but there's still some work to do.

[justy]

👍 To fixing this ASAP!

[raymondwinarto]

Any progress on this one? It'll be good if we can get an update on this issue.

[Suraiya-Hameed]

Linking PR #689 for tracking.

[Strandedpirate]

In a world where warnings are sent to stderr instead of stdout. <fail/>

[CoolRice]

any update?

[arthurschreiber]

#738 contains a fix and will be merged and released once reviewed by someone from the @tediousjs/microsoft-contributors team.

[arthurschreiber]

After a long time of back and forth, this was finally (and properly) fixed via #738. tedious@2.6.4 is now available on npm. It's currently tagged as next and I'll make sure it will be tagged as latest soon-ish.

Thanks everyone for your patience and help in getting this fixed! 🙇