From 5d32be4a907b8016622bc742ae5985ba33569750 Mon Sep 17 00:00:00 2001
From: Matthew Blewitt <blewitt.matt@gmail.com>
Date: Mon, 12 Feb 2018 21:35:21 +0000
Subject: [PATCH] Handle SSL negotiation errors more robustly

This commit adds some finer grained detail to handling the postmaster's
response to SSL negotiation packets, by accounting for the possibility
of an 'E' byte being sent back, and emitting an appropriate error.

In the naive case, the postmaster will respond with either 'S' (proceed
with an SSL connection) or 'N' (SSL is not supported). However, the
current if statement doesn't account for an 'E' byte being returned
by the postmaster, where an error is encountered (perhaps unable to
fork due to being out of memory).

By adding this case, we can prevent confusing error messages when SSL is
enforced and the postmaster returns an error after successful SSL
connections.

This also brings the connection handling further in line with
libpq, where 'E' is handled similarly as of this commit:

https://github.com/postgres/postgres/commit/a49fbaaf8d461ff91912c30b3563d54649474c80

Given that there are no longer pre-7.0 databases out in the wild, I
believe this is a safe change to make, and should not break backwards
compatibility (unless matching on error message content).

* Replace if statement with switch, to catch 'S', 'E' and 'N' bytes
  returned by the postmaster
* Return an Error for non 'S' or 'N' cases
* Expand and restructure unit tests for SSL negotiation packets
---
 lib/connection.js                   |  9 +++-
 test/unit/connection/error-tests.js | 66 ++++++++++++++++++++---------
 2 files changed, 53 insertions(+), 22 deletions(-)

diff --git a/lib/connection.js b/lib/connection.js
index d2cf69ec..208f0574 100644
--- a/lib/connection.js
+++ b/lib/connection.js
@@ -82,8 +82,13 @@ Connection.prototype.connect = function (port, host) {
 
   this.stream.once('data', function (buffer) {
     var responseCode = buffer.toString('utf8')
-    if (responseCode !== 'S') {
-      return self.emit('error', new Error('The server does not support SSL connections'))
+    switch (responseCode) {
+      case 'N':
+        return self.emit('error', new Error('The server does not support SSL connections'))
+      case 'S':
+        break
+      default:
+        return self.emit('error', new Error('There was an error establishing an SSL connection'))
     }
     var tls = require('tls')
     self.stream = tls.connect({
diff --git a/test/unit/connection/error-tests.js b/test/unit/connection/error-tests.js
index 329e1e31..f39187cf 100644
--- a/test/unit/connection/error-tests.js
+++ b/test/unit/connection/error-tests.js
@@ -37,26 +37,52 @@ suite.test('connection does not emit ECONNRESET errors during disconnect', funct
   done()
 })
 
+var SSLNegotiationPacketTests = [
+  {
+    testName: 'connection does not emit ECONNRESET errors during disconnect also when using SSL',
+    errorMessage: null,
+    response: 'S',
+    responseType: 'sslconnect'
+  },
+  {
+    testName: 'connection emits an error when SSL is not supported',
+    errorMessage: 'The server does not support SSL connections',
+    response: 'N',
+    responseType: 'error'
+  },
+  {
+    testName: 'connection emits an error when postmaster responds to SSL negotiation packet',
+    errorMessage: 'There was an error establishing an SSL connection',
+    response: 'E',
+    responseType: 'error'
+  }
+]
 
-suite.test('connection does not emit ECONNRESET errors during disconnect also when using SSL', function (done) {
-  // our fake postgres server, which just responds with 'S' to start SSL
-  var socket
-  var server = net.createServer(function (c) {
-    socket = c
-    c.once('data', function (data) {
-      c.write(Buffer.from('S'))
+for (var i = 0; i < SSLNegotiationPacketTests.length; i++) {
+  var tc = SSLNegotiationPacketTests[i]
+  suite.test(tc.testName, function (done) {
+    // our fake postgres server
+    var socket
+    var server = net.createServer(function (c) {
+      socket = c
+      c.once('data', function (data) {
+        c.write(Buffer.from(tc.response))
+      })
+    })
+
+    server.listen(7778, function () {
+      var con = new Connection({ssl: true})
+      con.connect(7778, 'localhost')
+      assert.emits(con, tc.responseType, function (err) {
+        if (err) {
+          assert.equal(err.message, tc.errorMessage)
+        }
+        con.end()
+        socket.destroy()
+        server.close()
+        done()
+      })
+      con.requestSsl()
     })
   })
-
-  server.listen(7778, function () {
-    var con = new Connection({ssl: true})
-    con.connect(7778, 'localhost')
-    assert.emits(con, 'sslconnect', function () {
-      con.end()
-      socket.destroy()
-      server.close()
-      done()
-    })
-    con.requestSsl()
-  })
-})
+}