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 all 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
31 changes: 24 additions & 7 deletions .github/workflows/ci-bun.yml
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,19 @@ jobs:
strategy:
fail-fast: false
matrix:
bun-version: [0.5.1]
bun-version: [canary]
mysql-version: ["mysql:5.7", "mysql:8.0.18", "mysql:8.0.22"]
use-compression: [0]
use-tls: [0]
use-compression: [0, 1]
use-tls: [0,1]
include:
- bun-version: "0.6.13"
use-compression: 1
use-tls: 0
mysql-version: "mysql:8.0.18"
- bun-version: "0.6.13"
use-compression: 1
use-tls: 0
mysql-version: "mysql:8.0.22"

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

Expand All @@ -33,7 +42,7 @@ jobs:
run: docker run -d -e MYSQL_ALLOW_EMPTY_PASSWORD=1 -e MYSQL_DATABASE=${{ env.MYSQL_DATABASE }} -v $PWD/mysqldata:/var/lib/mysql/ -v $PWD/examples/custom-conf:/etc/mysql/conf.d -v $PWD/examples/ssl/certs:/certs -p ${{ env.MYSQL_PORT }}:3306 ${{ matrix.mysql-version }}

- name: Set up Bun ${{ matrix.bun-version }}
uses: oven-sh/setup-bun@v0.1.8
uses: oven-sh/setup-bun@v1
with:
bun-version: ${{ matrix.bun-version }}

Expand All @@ -57,6 +66,14 @@ jobs:
- name: Wait mysql server is ready
run: node tools/wait-up.js

- name: Run tests
# todo: run full test suite once test createServer is implemented using Bun.listen
run: FILTER=test-select MYSQL_PORT=3306 bun run test
# todo: run full test suite once test createServer is implemented using Bun.listen
- name: run tests
env:
MYSQL_USER: ${{ env.MYSQL_USER }}
MYSQL_DATABASE: ${{ env.MYSQL_DATABASE }}
MYSQL_PORT: ${{ env.MYSQL_PORT }}
MYSQL_USE_COMPRESSION: ${{ matrix.use-compression }}
MYSQL_USE_TLS: ${{ matrix.use-tls }}
run: |
bun test/integration/connection/test-select-1.js
bun test/integration/connection/test-select-ssl.js
81 changes: 81 additions & 0 deletions .github/workflows/ci-osx.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
name: CI - OSX

on:
pull_request:
push:
branches: [ main ]

workflow_dispatch:

env:
MYSQL_PORT: 3306
MYSQL_USER: root
MYSQL_DATABASE: test

jobs:
tests-osx:
runs-on: macos-13
strategy:
fail-fast: false
matrix:
node-version: [18.x, 20.x]
mysql-version: ["mysql:8.0.22", "mysql:8.0.33"]
use-compression: [0, 1]
use-tls: [0]
mysql_connection_url_key: [""]
# TODO - add mariadb to the matrix. currently few tests are broken due to mariadb incompatibilities
include:
# 20.x
- node-version: "20.x"
mysql-version: "mysql:8.0.33"
use-compression: 1
use-tls: 0
use-builtin-test-runner: 1
- node-version: "20.x"
mysql-version: "mysql:8.0.33"
use-compression: 0
use-tls: 1
use-builtin-test-runner: 1
env:
MYSQL_CONNECTION_URL: ${{ secrets[matrix.mysql_connection_url_key] }}

name: Node.js ${{ matrix.node-version }} - DB ${{ matrix.mysql-version }}${{ matrix.mysql_connection_url_key }} - SSL=${{matrix.use-tls}} Compression=${{matrix.use-compression}}

steps:
- uses: actions/checkout@v3

- name: install lima
run: brew install lima

- name: Setup Docker on macOS
uses: douglascamata/setup-docker-macos-action@v1-alpha

- name: Set up MySQL
if: ${{ matrix.mysql-version }}
run: docker run -d -e MYSQL_ALLOW_EMPTY_PASSWORD=1 -e MYSQL_DATABASE=${{ env.MYSQL_DATABASE }} -p ${{ env.MYSQL_PORT }}:3306 ${{ matrix.mysql-version }}

- name: Set up Node.js ${{ matrix.node-version }}
uses: actions/setup-node@v3
with:
node-version: ${{ matrix.node-version }}

- name: Cache dependencies
uses: actions/cache@v3
with:
path: ~/.npm
key: npm-${{ hashFiles('package-lock.json') }}
restore-keys: npm-

- name: Install npm dependencies
run: npm ci

- name: Wait mysql server is ready
if: ${{ matrix.mysql-version }}
run: node tools/wait-up.js

- name: Run tests
run: FILTER=${{matrix.filter}} MYSQL_USE_TLS=${{ matrix.use-tls }} MYSQL_USE_COMPRESSION=${{ matrix.use-compression }} npm run coverage-test

- name: Run tests with built-in node test runner
if: ${{ matrix.use-builtin-test-runner }}
run: FILTER=${{matrix.filter}} MYSQL_USE_TLS=${{ matrix.use-tls }} MYSQL_USE_COMPRESSION=${{ matrix.use-compression }} npm run test:builtin-node-runner
111 changes: 24 additions & 87 deletions lib/connection.js
Original file line number Diff line number Diff line change
Expand Up @@ -355,62 +355,43 @@ 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();
}

pipe() {
if (this.stream instanceof Net.Stream) {
this.stream.ondata = (data, start, end) => {
this.packetParser.execute(data, start, end);
};
} else {
this.stream.on('data', data => {
this.packetParser.execute(
data.parent,
data.offset,
data.offset + data.length
);
});
}
this.write = buffer => secureSocket.write(buffer);
}

protocolError(message, code) {
Expand Down Expand Up @@ -948,48 +929,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;
8 changes: 6 additions & 2 deletions test/builtin-runner/regressions/2052.test.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ describe(
() => {
it('should report 0 actual parameters when 1 placeholder is used in ORDER BY ?', (t, done) => {
const connection = {
sequenceId: 1,
constructor: {
statementKey: () => 0,
},
Expand All @@ -23,9 +24,10 @@ describe(
},
writePacket: (packet) => {
// client -> server COM_PREPARE
packet.writeHeader(1);
assert.equal(
packet.buffer.toString('hex'),
'000000001673656c656374202a2066726f6d207573657273206f72646572206279203f'
'1f0000011673656c656374202a2066726f6d207573657273206f72646572206279203f'
);
},
};
Expand Down Expand Up @@ -68,7 +70,9 @@ describe(
}
);

describe('E2E Prepare result with number of parameters incorrectly reported by the server', { timeout: 1000 }, () => {
describe('E2E Prepare result with number of parameters incorrectly reported by the server',
{ timeout: 1000 },
() => {
let connection;

function isNewerThan8_0_22() {
Expand Down
11 changes: 7 additions & 4 deletions test/common.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ const config = {
port: process.env.MYSQL_PORT || 3306
};

if (process.env.MYSQL_USE_TLS) {
if (process.env.MYSQL_USE_TLS === '1') {
config.ssl = {
rejectUnauthorized: false,
ca: fs.readFileSync(
Expand All @@ -32,7 +32,7 @@ exports.waitDatabaseReady = function(callback) {
const tryConnect = function() {
const conn = exports.createConnection({ database: 'mysql', password: process.env.MYSQL_PASSWORD });
conn.once('error', err => {
if (err.code !== 'PROTOCOL_CONNECTION_LOST' && err.code !== 'ETIMEDOUT') {
if (err.code !== 'PROTOCOL_CONNECTION_LOST' && err.code !== 'ETIMEDOUT' && err.code !== 'ECONNREFUSED') {
console.log('Unexpected error waiting for connection', err);
process.exit(-1);
}
Expand Down Expand Up @@ -84,6 +84,7 @@ exports.createConnection = function(args) {
typeCast: args && args.typeCast,
namedPlaceholders: args && args.namedPlaceholders,
connectTimeout: args && args.connectTimeout,
ssl: (args && args.ssl) ?? config.ssl,
};

const conn = driver.createConnection(params);
Expand Down Expand Up @@ -164,10 +165,12 @@ exports.createServer = function(onListening, handler) {
const server = require('../index.js').createServer();
server.on('connection', conn => {
conn.on('error', () => {
// we are here when client drops connection
// server side of the connection
// ignore disconnects
});
// remove ssl bit from the flags
let flags = 0xffffff;
flags = flags ^ ClientFlags.COMPRESS;
flags = flags ^ (ClientFlags.COMPRESS | ClientFlags.SSL);

conn.serverHandshake({
protocolVersion: 10,
Expand Down
3 changes: 2 additions & 1 deletion test/integration/connection/test-disconnects.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@ const server = common.createServer(
// different host provided via MYSQL_HOST that identifies a real MySQL
// server instance.
host: 'localhost',
port: server._port
port: server._port,
ssl: false
});
connection.query('SELECT 123', (err, _rows, _fields) => {
if (err) {
Expand Down
3 changes: 2 additions & 1 deletion test/integration/connection/test-protocol-errors.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@ const server = common.createServer(
// different host provided via MYSQL_HOST that identifies a real MySQL
// server instance.
host: 'localhost',
port: server._port
port: server._port,
ssl: false
});
connection.query(query, (err, _rows, _fields) => {
if (err) {
Expand Down
3 changes: 2 additions & 1 deletion test/integration/connection/test-quit.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@ const server = common.createServer(
// different host provided via MYSQL_HOST that identifies a real MySQL
// server instance.
host: 'localhost',
port: server._port
port: server._port,
ssl: false
});

connection.query(queryCli, (err, _rows, _fields) => {
Expand Down
15 changes: 15 additions & 0 deletions test/integration/connection/test-select-ssl.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
'use strict';

const assert = require('assert');
const common = require('../../common');
const connection = common.createConnection();

connection.query(`SHOW STATUS LIKE 'Ssl_cipher'`, (err, rows) => {
assert.ifError(err);
if (process.env.MYSQL_USE_TLS === '1') {
assert.equal(rows[0].Value.length > 0, true);
} else {
assert.deepEqual(rows, [{ Variable_name: 'Ssl_cipher', Value: '' }]);
}
connection.end();
});
Loading
Loading