From 11ab1daaddd6d77238e4ea5bbbeb7f3a9041746c Mon Sep 17 00:00:00 2001 From: Brian C Date: Wed, 29 Jan 2020 10:18:20 -0600 Subject: [PATCH] Close connection on SSL connection errors (#2082) * Close connection on SSL connection errors Fixes #2079 * Fix test * Remove console.log * Fix tests, implement same behavior for native client * Fix tests --- packages/pg-pool/test/error-handling.js | 8 +-- packages/pg/lib/connection.js | 21 ++++++- packages/pg/lib/native/client.js | 5 +- .../test/integration/gh-issues/2056-tests.js | 1 + .../test/integration/gh-issues/2079-tests.js | 56 +++++++++++++++++++ 5 files changed, 83 insertions(+), 8 deletions(-) create mode 100644 packages/pg/test/integration/gh-issues/2079-tests.js diff --git a/packages/pg-pool/test/error-handling.js b/packages/pg-pool/test/error-handling.js index 72d97ede..90de4ec4 100644 --- a/packages/pg-pool/test/error-handling.js +++ b/packages/pg-pool/test/error-handling.js @@ -211,14 +211,14 @@ describe('pool error handling', function () { const pool = new Pool({ max: 1, port: closeServer.address().port, host: 'localhost' }) pool.connect((err) => { expect(err).to.be.an(Error) - if (err.errno) { - expect(err.errno).to.be('ECONNRESET') + if (err.code) { + expect(err.code).to.be('ECONNRESET') } }) pool.connect((err) => { expect(err).to.be.an(Error) - if (err.errno) { - expect(err.errno).to.be('ECONNRESET') + if (err.code) { + expect(err.code).to.be('ECONNRESET') } closeServer.close(() => { pool.end(done) diff --git a/packages/pg/lib/connection.js b/packages/pg/lib/connection.js index 435c1a96..6fa0696c 100644 --- a/packages/pg/lib/connection.js +++ b/packages/pg/lib/connection.js @@ -85,11 +85,13 @@ Connection.prototype.connect = function (port, host) { this.stream.once('data', function (buffer) { var responseCode = buffer.toString('utf8') switch (responseCode) { - case 'N': // Server does not support SSL connections - return self.emit('error', new Error('The server does not support SSL connections')) case 'S': // Server supports SSL connections, continue with a secure connection break + case 'N': // Server does not support SSL connections + self.stream.end() + return self.emit('error', new Error('The server does not support SSL connections')) default: // Any other response byte, including 'E' (ErrorResponse) indicating a server error + self.stream.end() return self.emit('error', new Error('There was an error establishing an SSL connection')) } var tls = require('tls') @@ -112,9 +114,18 @@ Connection.prototype.connect = function (port, host) { options.servername = host } self.stream = tls.connect(options) - self.attachListeners(self.stream) self.stream.on('error', reportStreamError) + // send SSLRequest packet + const buff = Buffer.alloc(8) + buff.writeUInt32BE(8) + buff.writeUInt32BE(80877103, 4) + if (self.stream.writable) { + self.stream.write(buff) + } + + self.attachListeners(self.stream) + self.emit('sslconnect') }) } @@ -345,6 +356,10 @@ Connection.prototype.end = function () { // 0x58 = 'X' this.writer.add(emptyBuffer) this._ending = true + if (!this.stream.writable) { + this.stream.end() + return + } return this.stream.write(END_BUFFER, () => { this.stream.end() }) diff --git a/packages/pg/lib/native/client.js b/packages/pg/lib/native/client.js index 6859bc2c..581ef72d 100644 --- a/packages/pg/lib/native/client.js +++ b/packages/pg/lib/native/client.js @@ -89,7 +89,10 @@ Client.prototype._connect = function (cb) { this.connectionParameters.getLibpqConnectionString(function (err, conString) { if (err) return cb(err) self.native.connect(conString, function (err) { - if (err) return cb(err) + if (err) { + self.native.end() + return cb(err) + } // set internal states to connected self._connected = true diff --git a/packages/pg/test/integration/gh-issues/2056-tests.js b/packages/pg/test/integration/gh-issues/2056-tests.js index f2912fc6..e025a1ad 100644 --- a/packages/pg/test/integration/gh-issues/2056-tests.js +++ b/packages/pg/test/integration/gh-issues/2056-tests.js @@ -5,6 +5,7 @@ var assert = require('assert') const suite = new helper.Suite() + suite.test('All queries should return a result array', (done) => { const client = new helper.pg.Client() client.connect() diff --git a/packages/pg/test/integration/gh-issues/2079-tests.js b/packages/pg/test/integration/gh-issues/2079-tests.js new file mode 100644 index 00000000..bec8e481 --- /dev/null +++ b/packages/pg/test/integration/gh-issues/2079-tests.js @@ -0,0 +1,56 @@ + +"use strict" +var helper = require('./../test-helper') +var assert = require('assert') + +const suite = new helper.Suite() + +// makes a backend server that responds with a non 'S' ssl response buffer +let makeTerminatingBackend = (byte) => { + const { createServer } = require('net') + + const server = createServer((socket) => { + // attach a listener so the socket can drain + // https://www.postgresql.org/docs/9.3/protocol-message-formats.html + socket.on('data', (buff) => { + const code = buff.readInt32BE(4) + // I don't see anything in the docs about 80877104 + // but libpq is sending it... + if (code === 80877103 || code === 80877104) { + const packet = Buffer.from(byte, 'utf-8') + socket.write(packet) + } + }) + socket.on('close', () => { + server.close() + }) + }) + + server.listen() + const { port } = server.address() + return port +} + +suite.test('SSL connection error allows event loop to exit', (done) => { + const port = makeTerminatingBackend('N') + const client = new helper.pg.Client({ ssl: 'require', port }) + // since there was a connection error the client's socket should be closed + // and the event loop will have no refs and exit cleanly + client.connect((err) => { + assert(err instanceof Error) + done() + }) +}) + + +suite.test('Non "S" response code allows event loop to exit', (done) => { + const port = makeTerminatingBackend('X') + const client = new helper.pg.Client({ ssl: 'require', port }) + // since there was a connection error the client's socket should be closed + // and the event loop will have no refs and exit cleanly + client.connect((err) => { + assert(err instanceof Error) + done() + }) +}) +