Update packages/pg/lib/connection-parameters.js. Fix SSL issue (brianc#2723)

dapeleg-dn · dapeleg-dn · commit 57b17b0202f2 · 2023-06-05T17:11:21.000+03:00
diff --git a/packages/pg/lib/connection-parameters.js b/packages/pg/lib/connection-parameters.js
@@ -1,6 +1,7 @@
 'use strict'
 
 var dns = require('dns')
+const _ = require('lodash')
 
 var defaults = require('./defaults')
 
@@ -18,7 +19,7 @@ var val = function (key, config, envVar) {
   return config[key] || envVar || defaults[key]
 }
 
-var readSSLConfigFromEnvironment = function () {
+var readSSLModeFromEnvironment = function () {
   switch (process.env.PGSSLMODE) {
     case 'disable':
       return false
@@ -33,6 +34,27 @@ var readSSLConfigFromEnvironment = function () {
   return defaults.ssl
 }
 
+var fillInSSLVariablesFromEnvironment = function(existingSsl) {
+  var caFileName = val('sslrootcert', existingSsl)
+  var crtFileName = val('sslcert', existingSsl)
+  var keyFileName = val('sslkey', existingSsl)
+
+  // Only try to load fs if we expect to read from the disk
+  const fs = caFileName || crtFileName || keyFileName ? require('fs') : null
+  var result = {}
+  if (caFileName) {
+    result.ca = fs.readFileSync(caFileName).toString()
+  }
+  if (crtFileName) {
+    result.cert = fs.readFileSync(crtFileName).toString()
+  }
+  if (keyFileName) {
+    result.key = fs.readFileSync(keyFileName).toString()
+  }
+  
+  return result
+}
+
 // Convert arg to a string, surround in single quotes, and escape single quotes and backslashes
 var quoteParamValue = function (value) {
   return "'" + ('' + value).replace(/\\/g, '\\\\').replace(/'/g, "\\'") + "'"
@@ -78,7 +100,7 @@ class ConnectionParameters {
     this.binary = val('binary', config)
     this.options = val('options', config)
 
-    this.ssl = typeof config.ssl === 'undefined' ? readSSLConfigFromEnvironment() : config.ssl
+    this.ssl = typeof config.ssl === 'undefined' ? readSSLModeFromEnvironment() : config.ssl
 
     if (typeof this.ssl === 'string') {
       if (this.ssl === 'true') {
@@ -89,6 +111,23 @@ class ConnectionParameters {
     if (this.ssl === 'no-verify') {
       this.ssl = { rejectUnauthorized: false }
     }
+
+    // Fill in possibly missing SSL config parameters from environment variables. 
+    // This lets you mix SSL definitions between connection string, config object and environment variables.
+    if (this.ssl === true) {
+      var sslCandidate = fillInSSLVariablesFromEnvironment({})
+      
+      // Change the type of this.ssl from boolean to object only if relevant environment variables were defined
+      if (!_.isEmpty(sslCandidate)) {
+        this.ssl = sslCandidate
+      }
+    } else if (typeof this.ssl === 'object') {
+      var sslCandidate = fillInSSLVariablesFromEnvironment({})
+      this.ssl = Object.assign(this.ssl, sslCandidate)
+    }
+
+    // "hiding" the private key so it doesn't show up in stack traces
+    // or if the client is console.logged
     if (this.ssl && this.ssl.key) {
       Object.defineProperty(this.ssl, 'key', {
         enumerable: false,
diff --git a/packages/pg/test/integration/connection-pool/tls-tests.js b/packages/pg/test/integration/connection-pool/tls-tests.js
@@ -8,6 +8,8 @@ const pg = helper.pg
 const suite = new helper.Suite()
 
 if (process.env.PG_CLIENT_CERT_TEST) {
+
+  // Test SSL using "options object" constructor
   suite.testAsync('client certificate', async () => {
     const pool = new pg.Pool({
       ssl: {
@@ -20,4 +22,13 @@ if (process.env.PG_CLIENT_CERT_TEST) {
     await pool.query('SELECT 1')
     await pool.end()
   })
+
+  // Test SSL by only defining environment variables without any explicit reference to the cert files in the code
+  // to be compliant with lib-pg: https://www.postgresql.org/docs/current/libpq-envars.html
+  suite.testAsync('client certificate', async () => {
+    const pool = new pg.Pool({})
+
+    await pool.query('SELECT 1')
+    await pool.end()
+  })
 }
diff --git a/packages/pg/test/unit/connection-parameters/environment-variable-tests.js b/packages/pg/test/unit/connection-parameters/environment-variable-tests.js
@@ -3,6 +3,11 @@ var helper = require('../test-helper')
 const Suite = require('../../suite')
 
 var assert = require('assert')
+const fs = require('fs')
+
+const tmp = require('tmp')
+tmp.setGracefulCleanup()
+
 var ConnectionParameters = require('../../../lib/connection-parameters')
 var defaults = require('../../../lib').defaults
 
@@ -36,6 +41,38 @@ suite.test('ConnectionParameters initialized from environment variables', functi
   assert.equal(subject.port, 7890, 'env port')
   assert.equal(subject.database, 'allyerbase', 'env database')
   assert.equal(subject.password, 'open', 'env password')
+  assert.equal(subject.ssl, false, 'ssl')
+})
+
+suite.test('ConnectionParameters initialized from environment variables - ssl', function () {
+  createTempTlsFilesAndExecute(function(
+    certFilePath, keyFilePath, caFilePath, 
+    certFileContents, keyFileContents, caFileContents
+  ) {
+      clearEnv()
+      process.env['PGHOST'] = 'local'
+      process.env['PGUSER'] = 'bmc2'
+      process.env['PGPORT'] = 7890
+      process.env['PGDATABASE'] = 'allyerbase'
+      process.env['PGPASSWORD'] = 'open'
+    
+      process.env['PGSSLMODE'] = 'verify-full'
+      process.env['PGSSLCERT'] = certFilePath
+      process.env['PGSSLKEY'] = keyFilePath
+      process.env['PGSSLROOTCERT'] = caFilePath
+    
+      var subject = new ConnectionParameters()
+      assert.equal(subject.host, 'local', 'env host')
+      assert.equal(subject.user, 'bmc2', 'env user')
+      assert.equal(subject.port, 7890, 'env port')
+      assert.equal(subject.database, 'allyerbase', 'env database')
+      assert.equal(subject.password, 'open', 'env password')
+
+      assert.equal(typeof subject.ssl, 'object', 'env ssl')
+      assert.equal(subject.ssl.cert, certFileContents, 'env ssl cert')
+      assert.equal(subject.ssl.key, keyFileContents, 'env ssl key')
+      assert.equal(subject.ssl.ca, caFileContents, 'env ssl ca')
+    })
 })
 
 suite.test('ConnectionParameters initialized from mix', function () {
@@ -56,6 +93,77 @@ suite.test('ConnectionParameters initialized from mix', function () {
   assert.equal(subject.port, 7890, 'env port')
   assert.equal(subject.database, 'zugzug', 'config database')
   assert.equal(subject.password, defaults.password, 'defaults password')
+  assert.equal(subject.ssl, false, 'ssl')
+})
+
+suite.test('ConnectionParameters initialized from mix - ssl', function () {
+  createTempTlsFilesAndExecute(function(
+    certFilePath, keyFilePath, caFilePath, 
+    certFileContents, keyFileContents, caFileContents
+  ) {
+    clearEnv()
+    process.env['PGHOST'] = 'local'
+    process.env['PGUSER'] = 'bmc2'
+    process.env['PGPORT'] = 7890
+    process.env['PGDATABASE'] = 'allyerbase'
+    process.env['PGPASSWORD'] = 'open'
+    process.env['PGSSLMODE'] = 'verify-full'
+    process.env['PGSSLCERT'] = certFilePath
+    process.env['PGSSLKEY'] = keyFilePath
+    delete process.env['PGPASSWORD']
+    delete process.env['PGDATABASE']
+
+    var subject = new ConnectionParameters({
+      // The connection string will mostly override this config. See ConnectionParameters constructor.
+      user: 'testing',
+      database: 'zugzug',
+      ssl: {
+        ca: caFileContents
+      },
+      connectionString: "postgres://user2:pass2@host2:9999/db2"
+    })
+    assert.equal(subject.host, 'host2', 'string host')
+    assert.equal(subject.user, 'user2', 'string user')
+    assert.equal(subject.port, 9999, 'string port')
+    assert.equal(subject.database, 'db2', 'string database')
+    assert.equal(subject.password, 'pass2', 'string password')
+
+    assert.equal(typeof subject.ssl, 'object', 'env ssl')
+    assert.equal(subject.ssl.cert, certFileContents, 'env ssl cert')
+    assert.equal(subject.ssl.key, keyFileContents, 'env ssl key')
+    assert.equal(subject.ssl.ca, caFileContents, 'config ssl ca')
+  })
+})
+
+suite.test('ConnectionParameters initialized from config - ssl', function () {
+  createTempTlsFilesAndExecute(function(
+    certFilePath, keyFilePath, caFilePath, 
+    certFileContents, keyFileContents, caFileContents
+  ) {
+    clearEnv()
+    var subject = new ConnectionParameters({
+      host: 'local',
+      user: 'testing',
+      password: 'open',
+      port: 7890,
+      database: 'zugzug',
+      ssl: {
+        cert: certFileContents,
+        key: keyFileContents,
+        ca: caFileContents
+      }
+    })
+    assert.equal(subject.host, 'local', 'env host')
+    assert.equal(subject.user, 'testing', 'config user')
+    assert.equal(subject.port, 7890, 'env port')
+    assert.equal(subject.database, 'zugzug', 'config database')
+    assert.equal(subject.password, 'open', 'defaults password')
+
+    assert.equal(typeof subject.ssl, 'object', 'config ssl')
+    assert.equal(subject.ssl.cert, certFileContents, 'config ssl cert')
+    assert.equal(subject.ssl.key, keyFileContents, 'config ssl key')
+    assert.equal(subject.ssl.ca, caFileContents, 'config ssl ca')
+  })
 })
 
 suite.test('connection string parsing', function () {
@@ -67,6 +175,7 @@ suite.test('connection string parsing', function () {
   assert.equal(subject.password, 'pw', 'string password')
   assert.equal(subject.port, 381, 'string port')
   assert.equal(subject.database, 'lala', 'string database')
+  assert.equal(subject.ssl, false, 'ssl')
 })
 
 suite.test('connection string parsing - ssl', function () {
@@ -104,6 +213,33 @@ suite.test('ssl is false by default', function () {
   assert.equal(subject.ssl, false)
 })
 
+// Create temp TLS certificate-mock files and run test logic inside this context
+function createTempTlsFilesAndExecute(callback) {
+  tmp.dir(function _tempDirCreated(err, tmpdir) {
+    if (err) throw err;
+    
+    const certFilePath = tmpdir + '/client.crt'
+    const keyFilePath = tmpdir + '/client.key'
+    const caFilePath = tmpdir + '/ca.crt'
+
+    const certFileContents = 'client cert file'
+    const keyFileContents = 'client key file'
+    const caFileContents = 'CA cert file'
+
+    fs.appendFileSync(certFilePath, certFileContents, function (err) {
+      if (err) throw err;
+    })    
+    fs.appendFileSync(keyFilePath, keyFileContents, function (err) {
+      if (err) throw err;
+    })    
+    fs.appendFileSync(caFilePath, caFileContents, function (err) {
+      if (err) throw err;
+    })
+
+    callback(certFilePath, keyFilePath, caFilePath, certFileContents, keyFileContents, caFileContents)
+  })
+}
+
 var testVal = function (mode, expected) {
   suite.test('ssl is ' + expected + ' when $PGSSLMODE=' + mode, function () {
     clearEnv()
