Emission of secure events changed across versions of node

[tufosa]

• Version: v8.16.1, v10.16.3, v12.8.1
• Platform: Linux tufopad 4.15.0-58-generic transform streams: unpipe or ignore future writes after .push(null) #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux
• Subsystem: tls

It seems to be a bit of discordance in the emission of the secure event across versions of node and also if you use the deprecated createSecurePair vs new tls.TLSSocket.

Steps to reproduce

First, you'll need to clone this repo containing the sample code.

This program starts a tls.Server, and then two clients, one using the deprecated createSecurePair and the other using tls.TLSSocket. It just opens the connections and wait for the different events to trigger. Each event leaves a trace. It reproduces 2 cases per client (4 in total): one just opening the connection (write: false) and the other opening the connection and immediately writing an empty string to the socket (write: true). Therefore the 4 cases are:

• SecurePair-write-false
• SecurePair-write-true
• TLSSocket-write-false
• TLSSocket-write-true

In order to reproduce the bug, run node main.js using different versions of nodejs and observe the differences in behaviour through the traces left by the program. It seems like both the secure and secureConnection events are emitted with different criteria depending on the version of node used.

I undestand that createSecurePair is deprecated and therefore should not be used, but I believe that the new way of doing things (tls.TLSSocket) should have the same behaviour as the old createSecurePair in order to make the transition easier. It seems reasonable to wait for the secure event before attempting to write anything. This is how the old createSecurePair behaved in node v8.16.1, but this behaviour has been changed in node 10 and node 12.

Also the documented secureConnect event is never emitted. An undocumented secure event is emitted instead. I see that this has been reported before (at least here and here), but the docs haven't been fixed.

node v8.16.1

Creating SecurePair write false client...
(node:31271) [DEP0064] DeprecationWarning: tls.createSecurePair() is deprecated. Please use tls.TLSSocket instead.
secureConnection
secure

Creating TLSSocket write false client...
ERROR:  TLSSocket with write false did NOT receive secure event

Creating SecurePair write true client...
secureConnection
secure

Creating TLSSocket write true client...
secureConnection
secure

Finished

The only case not emitting any event is TLSSocket-write-false.

node v10.16.3

Creating SecurePair write false client...
(node:31533) [DEP0064] DeprecationWarning: tls.createSecurePair() is deprecated. Please use tls.TLSSocket instead.
ERROR:  SecurePair with write false did NOT receive secure event

Creating TLSSocket write false client...
ERROR:  TLSSocket with write false did NOT receive secure event

Creating SecurePair write true client...
secureConnection
secure

Creating TLSSocket write true client...
secureConnection
secure

Finished

All write-false cases fail (as in do not emit events) and all write-true cases
pass.

node v12.8.1

Creating SecurePair write false client...
(node:31828) [DEP0064] DeprecationWarning: tls.createSecurePair() is deprecated. Please use tls.TLSSocket instead.
ERROR:  SecurePair with write false did NOT receive secure event

Creating TLSSocket write false client...
ERROR:  TLSSocket with write false did NOT receive secure event

Creating SecurePair write true client...
secure

Creating TLSSocket write true client...
secureConnection
secure

Finished

All write-false cases fail and the write-true cases behave slightly different,
as one of them (SecurePair-write-true) never emit the secureConnection event.

[sam-github]

You aren't pinning the TLS version to a particular value, the 12.x at least should be doing TLS1.3, it wouldn't surprise me at all if it was different.

Could you print out some info about the versions and cipher suites negotiated and see if that explains the differences you see?

[tufosa]

Good point, @sam-github!! I've checked which versions of TLS and ciphers were being used in each case, and both node v8.16.1 and node v10.16.3 were using TLSv1.2 and cipher ECDHE-RSA-AES128-GCM-SHA256, while node v12.8.1 was using TLSv1.3 and cipher TLS_AES_256_GCM_SHA384. This could be the cause (and indeed seems to be) of the different behaviour shown by node v12.8.1 in the SecurePair-write-true case. If I pin both TLS version and cipher using the --tls-cipher-list running main.js like this:

$ node --tls-cipher-list="ECDHE-RSA-AES128-GCM-SHA256" main.js

then the weird behaviour of node v12.8.1 in the SecurePair-write-true case disappears (it emits the secureConnection event as expected). I've changed the code in the repo, and it now prints the info about the TLS version and cipher.

My concerns remain the same, though. There are undocumented differences in the behaviour of the old (and deprecated) createSecurePair and its replacement tls.TLSSocket. It still makes sense to me waiting for the secure event before writing anything to the socket. And regarding the weird behaviour of node v12.8.1 in the SecurePair-write-true (apparently caused by TLSv1.3), shouldn't nodejs abstract that from the developer? what I mean is: shouldn't tls.Server emit the event secureConnection regardless of the TLS version used underneath?

[sam-github]

createSecurePair has been deprecated for a long, long time. I've personally never used it or tried to understand its purpose. I run into it in the tests and code and wish it could be deleted already, so I've very little to say about it specifically.

I took a shot at documenting some of this, #10846, it got rejected. I might try again sometime, or I welcome a PR if you would like to take it up, feel free to steal any text from there as your own. Specifically, I tried to document the 'secure' event, since it must be used when starting a TLS connection over an existing TCP socket, which I believe is what you are noticing.

As far as TLS1.3 being noticably different - it is different, the ordering of messages across a TLS setup changed from 1.2, its a pretty major protocol change (despite that the minor just got bumped). While we might wish that was abstracted away, it couldn't be. Its possible to write code robust across 1.2 and 1.3, see the docs, but code making assumptions that (in retrospect) were only true for 1.2 is going to have to be updated.

[tufosa]

Thanks a lot for your time and effort, @sam-github!! createSecurePair was (and still is) the only way to implement a pure "TLS building block" that encrypts whatever is coming "from the left" and decrypts whatever comes "from the right". This building block can be connected to another "TCP building block", but also to a "file bulding block" or to any other. I know it might not be the most popular use of TLS, but for us at Devo (www.devo.com), it's quite crucial. I was currently getting rid of createSecurePair in one of our implementations, and that's how I came up with the bug.

[sam-github]

@tufosa I thought that was possible by using the TLSSocket constructor. It was certainly intended that way, despite the lack of docs.

Instead of describing how they are different (the difference may be an annoyance, but isn't obviously a bug), could you provide a running sample of code that achieves something with socketpair that cannot be converted to use TLSSocket? If so, I think we either prematurely deprecated socketpair, or have a bug, missing feature, or missing docs on the TLSSocket.

[tufosa]

@sam-github, consider the following example (included as a separate working file here):

const fs = require('fs');      
const path = require('path');  
const tls = require('tls');

const MSG = 'Once unpon a time in the West...';
  
const key = fs.readFileSync(path.join(__dirname,'./agent1-key.pem'));
const cert = fs.readFileSync(path.join(__dirname,'./agent1-cert.pem'));
const ca = fs.readFileSync(path.join(__dirname,'./ca1-cert.pem'));

const creds = tls.createSecureContext({key, cert, ca});

const clientPair = tls.createSecurePair(creds, false);

const serverPair = tls.createSecurePair(creds, true, true, true);

clientPair.encrypted.pipe(serverPair.encrypted);
serverPair.encrypted.pipe(clientPair.encrypted);

serverPair.cleartext.on('data', data => {
  console.log('Data received on the clear side of the server:',data.toString());
  process.exit(0);
});

clientPair.on('secure', () => {
  console.log('client secured');
  clientPair.cleartext.write(MSG);
});

This example creates two simple TLS building blocks and connects them through their encrypted side. When I connect them, they share their credentials and try to complete the TLS handshake. When this is done, the secure event is emitted. If I write something to the clear side of one of the pairs, it will get encrypted, then piped to the encrypted side of the other pair and then it will get decrypted again. As simple as that. No TCP or net.Socket involved.

This example will only successfully run using node 8, because if you use node 10 or 12, the secure event will never be emitted.

Now, could this be replicated using tls.TLSSocket?. Well, it seems that it can, but I couldn't think of any other way other than the one used in nodejs' source code. The problem is that, if you do it that way, the secure event is only emitted after you write some data to the socket. And, sorry if I explained myself poorly previously, but this is what this bug intends to report: that the replacement for the deprecated createSecurePair behaves differently. And this difference in behaviour breaks some of our aplications because we wait for the secure event to be emitted before attempting to write anything into the stream (which I think it is pretty reasonable).

[sam-github]

OK, that's enough info for me to take a look at it, thanks.

[jasnell]

There's been no further discussion or action here in many years. Is there anything remaining to do here or should we just move on? None of the impacted versions are even supported any longer.

[tufosa]

yep, I guess this could be closed. It wouldn't hurt to run this script and check if the "secure" event is emitted consistently in the last versions of node, though.

[jasnell]

That script appears to be producing consistent results for me.