fix: make startTls code compatible with Bun

[sidorares]

• raw stream 'data' events were not disconnected before starting TLS socket ( looks like node disconnects them for us, bun does not )
• use TLS.connect() instead of new TLS.Socket() + secureSocket._start()
• move logic from 'secure' handler to a TLS.connect(options, callback) callback. Note: if handled via event, correct document name should be "secureConnection" - see https://nodejs.org/dist/latest-v20.x/docs/api/tls.html#event-secureconnection
• fixes in verifyIdentity logic

Note: requires bun >= 0.6.13 due to functionality added in oven-sh/bun#3457

[github-actions]

Coverage report

The coverage rate is 89.42658744362517%

The branch rate is 84.63687150837988%

100% of new lines are covered.

[Jarred-Sumner]

raw stream 'data' events were not disconnected before starting TLS socket ( looks like node disconnects them for us, bun does not )

We should fix this on our end, more packages will run into it

[Jarred-Sumner]

we probably need to emit this error too or else more people will run into this issue

[sidorares]

I had to change event name from "connect" to "secureConnect" as well, looks like node emits both and the first one is deprecated. TLS docs only mention secureConnect - https://nodejs.org/dist/latest-v20.x/docs/api/tls.html#event-secureconnection ( later moved code to the Tls.connect() callback to avoid referencing event name )

[sidorares]

I might need to investigate further if this is how I should handle errors. _tlsError event is still in latest node - https://github.com/nodejs/node/blob/d9438ccbd8f6cbd6c8ebfae84b0f2782d2c1fca7/lib/_tls_wrap.js#L991-L1010 , not sure if documented.

[sidorares]

ok, this was a bit more than I expected. We were not running tls tests for quite some time, I re-enabled them and also added OSX runner

[huangyq23]

@sidorares
I believe this is breaking the ssl: {rejectUnauthorized: true, verifyIdentity: false} scenario.

There is a subtle difference between the new TLS.connect() flow and the old new TLS.Socket() + secureSocket._start() flow.

The newer flow will always call the configured checkServerIdentity for you, which means our custom logic for verifyIdentity: false is nullified because it would have been called regardless by Node itself.

A potential fix to achieve the proper behavior is to pass in a no-op checkServerIdentity: ()=>null; to TLS.connect() when verifyIdentity: false.

Ref:

• Node TLS module: https://github.com/nodejs/node/blob/b53c51995380b1f8d642297d848cab6010d2909c/lib/_tls_wrap.js#L1612-L1618
• Missing Node documentation clarification: How to use new tls.TLSSocket(...) to establish a secure connection? nodejs/node#43994

[sidorares]

thanks @huangyq23 , I think you are correct and I agree checkServerIdentity: ()=>null; might be a good fix. I need to think what's the best scenario to test this

[juanheyns]

@sidorares I believe this is breaking the ssl: {rejectUnauthorized: true, verifyIdentity: false} scenario.

There is a subtle difference between the new TLS.connect() flow and the old new TLS.Socket() + secureSocket._start() flow.

The newer flow will always call the configured checkServerIdentity for you, which means our custom logic for verifyIdentity: false is nullified because it would have been called regardless by Node itself.

A potential fix to achieve the proper behavior is to pass in a no-op checkServerIdentity: ()=>null; to TLS.connect() when verifyIdentity: false.

Ref:

• Node TLS module: https://github.com/nodejs/node/blob/b53c51995380b1f8d642297d848cab6010d2909c/lib/_tls_wrap.js#L1612-L1618
• Missing Node documentation clarification: How to use new tls.TLSSocket(...) to establish a secure connection? nodejs/node#43994

I have submitted a PR which addresses this:
#2376