Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: make startTls code compatible with Bun #2119

Merged
merged 27 commits into from
Jul 10, 2023
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
bca1e28
uplift startTls code to be compatible with current bun
sidorares Jul 7, 2023
f4fb55e
more tls refactoring
sidorares Jul 7, 2023
69e3b2f
lint
sidorares Jul 7, 2023
e52b5d0
ci: enable tls in bun matrix
sidorares Jul 7, 2023
607a66d
fix ssl tests
sidorares Jul 7, 2023
0bc180c
lint
sidorares Jul 7, 2023
7d327a9
bun: only run 2 basic tests for now
sidorares Jul 7, 2023
47c2b5b
lint
sidorares Jul 7, 2023
84beaa5
don't enable ssl in test running against fake server
sidorares Jul 7, 2023
456a522
fix typo
sidorares Jul 7, 2023
99b0b85
debug failures
sidorares Jul 7, 2023
6c5879c
try bun canary
sidorares Jul 7, 2023
583a317
ci: add osx runner
sidorares Jul 8, 2023
709091b
ci: install docker for osx
sidorares Jul 8, 2023
9b2db14
ci: install docker for osx
sidorares Jul 8, 2023
f00b840
ci: install docker for osx
sidorares Jul 8, 2023
3036c51
ci: osx - don't mount config in docker
sidorares Jul 8, 2023
fe27ed1
handle ECONNREFUSED in waitDatabaseReady helper
sidorares Jul 8, 2023
585c3f1
comment out instead of early return
sidorares Jul 8, 2023
a73e90d
explicitly install lima
sidorares Jul 8, 2023
cbb2f94
use connection.end() instead of destroy
sidorares Jul 8, 2023
c4e45b8
debug Ssl_cipher assertion
sidorares Jul 8, 2023
7eb32af
more flexible Ssl_cipher assertion
sidorares Jul 8, 2023
1a594f2
initialize packet header befor writing
sidorares Jul 9, 2023
f065055
cleanup
sidorares Jul 9, 2023
4c64210
add compression to bun matrix
sidorares Jul 10, 2023
75ae090
only use bun v0.6.13 for non-ssl tests
sidorares Jul 10, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .github/workflows/ci-bun.yml
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,10 @@ jobs:
strategy:
fail-fast: false
matrix:
bun-version: [0.5.1]
bun-version: [0.6.13]
mysql-version: ["mysql:5.7", "mysql:8.0.18", "mysql:8.0.22"]
use-compression: [0]
use-tls: [0]
use-tls: [0,1]

name: Bun ${{ matrix.bun-version }} - DB ${{ matrix.mysql-version }} - SSL=${{matrix.use-tls}} Compression=${{matrix.use-compression}}

Expand Down
97 changes: 26 additions & 71 deletions lib/connection.js
Original file line number Diff line number Diff line change
Expand Up @@ -355,48 +355,46 @@ class Connection extends EventEmitter {
});
const rejectUnauthorized = this.config.ssl.rejectUnauthorized;
const verifyIdentity = this.config.ssl.verifyIdentity;
const host = this.config.host;
const servername = this.config.host

let secureEstablished = false;
const secureSocket = new Tls.TLSSocket(this.stream, {
rejectUnauthorized: rejectUnauthorized,
requestCert: true,
secureContext: secureContext,
isServer: false
this.stream.removeAllListeners('data');
const secureSocket = Tls.connect({
rejectUnauthorized,
requestCert: rejectUnauthorized,
secureContext,
isServer: false,
socket: this.stream,
servername
}, () => {
secureEstablished = true;
if (rejectUnauthorized) {
if (typeof servername === 'string' && verifyIdentity) {
const cert = secureSocket.getPeerCertificate(true);
const serverIdentityCheckError = Tls.checkServerIdentity(servername, cert);
if (serverIdentityCheckError) {
onSecure(serverIdentityCheckError);
return;
}
}
}
onSecure();
});
if (typeof host === 'string') {
secureSocket.setServername(host);
}
// error handler for secure socket
secureSocket.on('_tlsError', err => {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 )

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

secureSocket.on('error', err => {
if (secureEstablished) {
this._handleNetworkError(err);
} else {
onSecure(err);
}
});
secureSocket.on('secure', () => {
secureEstablished = true;
let callbackValue = null;
if (rejectUnauthorized) {
callbackValue = secureSocket.ssl.verifyError()
if (!callbackValue && typeof host === 'string' && verifyIdentity) {
const cert = secureSocket.ssl.getPeerCertificate(true);
callbackValue = Tls.checkServerIdentity(host, cert)
}
}
onSecure(callbackValue);
});
secureSocket.on('data', data => {
this.packetParser.execute(data);
});
this.write = buffer => {
secureSocket.write(buffer);
};
// start TLS communications
secureSocket._start();
this.write = buffer => secureSocket.write(buffer);
}

/*
pipe() {
if (this.stream instanceof Net.Stream) {
this.stream.ondata = (data, start, end) => {
Expand All @@ -412,6 +410,7 @@ class Connection extends EventEmitter {
});
}
}
*/

protocolError(message, code) {
// Starting with MySQL 8.0.24, if the client closes the connection
Expand Down Expand Up @@ -948,48 +947,4 @@ class Connection extends EventEmitter {
}
}

if (Tls.TLSSocket) {
// not supported
} else {
Connection.prototype.startTLS = function _startTLS(onSecure) {
if (this.config.debug) {
// eslint-disable-next-line no-console
console.log('Upgrading connection to TLS');
}
const crypto = require('crypto');
const config = this.config;
const stream = this.stream;
const rejectUnauthorized = this.config.ssl.rejectUnauthorized;
const credentials = crypto.createCredentials({
key: config.ssl.key,
cert: config.ssl.cert,
passphrase: config.ssl.passphrase,
ca: config.ssl.ca,
ciphers: config.ssl.ciphers
});
const securePair = Tls.createSecurePair(
credentials,
false,
true,
rejectUnauthorized
);

if (stream.ondata) {
stream.ondata = null;
}
stream.removeAllListeners('data');
stream.pipe(securePair.encrypted);
securePair.encrypted.pipe(stream);
securePair.cleartext.on('data', data => {
this.packetParser.execute(data);
});
this.write = function(buffer) {
securePair.cleartext.write(buffer);
};
securePair.on('secure', () => {
onSecure(rejectUnauthorized ? securePair.ssl.verifyError() : null);
});
};
}

module.exports = Connection;