arthurschreiber
commented
1 year ago I think MessageIO.prototype.startTls also does not handle correctly cases where the network socket is closed by the remote side during the TLS negotiation. I think this eventually gets handled correctly by the close and error events we define here on the socket:
But I don't think this is the correct place to handle this. Essentially, it would be best if startTls would allow simply "forgetting" about the original network socket, same as with tls.connect.
Because the TDS 7.x protocol performs TLS negotiation wrapped into TDS messages exchanged between client and server, we can't use
tls.connectdirectly to wrap our tcp sockets into tls sockets. (We can and do this with TDS 8.x).For TDS 7.x the TLS negotiation is implemented in
MessageIO.prototype.startTls. We use a library callednative-duplexpairto give us access to both the cleartext as well the encrypted sides of the tls connection, so we can take the handshake data, wrap it into TDS messages, and send it to the remote server.We pass the cleartext side of the of the secure pair to
tls.connect.tls.connectwraps this "stream" and sets up events to e.g. get notified if the stream is closed to close the tls socket accordingly.This can be seen e.g. here: https://github.com/nodejs/node/blob/v16.x/lib/_tls_wrap.js#L632
Unfortunately, this logic works correctly if
tls.connectis used to wrap anet.Socketinstance, as we for example do when using in the TDS 8.x case.But in the TDS 7.x case, this doesn't work right. If the underlying
net.Socketis closed, this information is not passed through to the the duplexpair stream.So we have cases where the
net.Socketis closed, but thetls.Socketdoes not get notified. I think we also have cases the other way round - thetls.Socketcould be or run into an error, but thenet.Socketwe have does not get closed correctly.We need to look at how
tls.connectpasses this information around, and replicate this correctly inMessageIO.prototype.startTls.