From 6937a2428b6e54c4ebe93fa54899631bae861ec3 Mon Sep 17 00:00:00 2001 From: Ben Salili-James Date: Tue, 5 May 2020 13:24:11 +0100 Subject: [PATCH 1/5] Add `PGSSLMODE=noverify` support to opt-out of rejecting self-signed certs --- packages/pg/lib/connection-parameters.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/pg/lib/connection-parameters.js b/packages/pg/lib/connection-parameters.js index b34e0df5f..71161e2c8 100644 --- a/packages/pg/lib/connection-parameters.js +++ b/packages/pg/lib/connection-parameters.js @@ -34,6 +34,8 @@ var useSsl = function () { case 'verify-ca': case 'verify-full': return true + case 'no-verify': + return { rejectUnauthorized: false } } return defaults.ssl } From b89eb0f81df36b32d28bf94d8cbc31186ed66574 Mon Sep 17 00:00:00 2001 From: "Brian M. Carlson" Date: Tue, 5 May 2020 10:58:34 -0500 Subject: [PATCH 2/5] Write tests & unify treatment of no-verify --- packages/pg/lib/connection-parameters.js | 14 ++++- .../environment-variable-tests.js | 59 ++++++++++++------- 2 files changed, 48 insertions(+), 25 deletions(-) diff --git a/packages/pg/lib/connection-parameters.js b/packages/pg/lib/connection-parameters.js index 71161e2c8..eead3c39f 100644 --- a/packages/pg/lib/connection-parameters.js +++ b/packages/pg/lib/connection-parameters.js @@ -25,8 +25,15 @@ var val = function (key, config, envVar) { return config[key] || envVar || defaults[key] } -var useSsl = function () { - switch (process.env.PGSSLMODE) { +var useSsl = function (modeFromConfig) { + // if the ssl parameter passed to config is not a string, just return it + // directly (it will be passed directly to tls.connect) + if (modeFromConfig !== undefined && typeof modeFromConfig !== 'string') { + return modeFromConfig + } + const mode = modeFromConfig || process.env.PGSSLMODE + + switch (mode) { case 'disable': return false case 'prefer': @@ -70,7 +77,8 @@ var ConnectionParameters = function (config) { }) this.binary = val('binary', config) - this.ssl = typeof config.ssl === 'undefined' ? useSsl() : config.ssl + // this.ssl = typeof config.ssl === 'undefined' ? useSsl() : config.ssl + this.ssl = useSsl(config.ssl) this.client_encoding = val('client_encoding', config) this.replication = val('replication', config) // a domain socket begins with '/' diff --git a/packages/pg/test/unit/connection-parameters/environment-variable-tests.js b/packages/pg/test/unit/connection-parameters/environment-variable-tests.js index 45d481e30..c64edee87 100644 --- a/packages/pg/test/unit/connection-parameters/environment-variable-tests.js +++ b/packages/pg/test/unit/connection-parameters/environment-variable-tests.js @@ -1,5 +1,7 @@ 'use strict' var helper = require(__dirname + '/../test-helper') +const Suite = require('../../suite') + var assert = require('assert') var ConnectionParameters = require(__dirname + '/../../../lib/connection-parameters') var defaults = require(__dirname + '/../../../lib').defaults @@ -11,7 +13,17 @@ for (var key in process.env) { delete process.env[key] } -test('ConnectionParameters initialized from environment variables', function (t) { +const suite = new Suite('ConnectionParameters') + +const clearEnv = () => { + // clear process.env + for (var key in process.env) { + delete process.env[key] + } +} + +suite.test('ConnectionParameters initialized from environment variables', function () { + clearEnv() process.env['PGHOST'] = 'local' process.env['PGUSER'] = 'bmc2' process.env['PGPORT'] = 7890 @@ -26,7 +38,13 @@ test('ConnectionParameters initialized from environment variables', function (t) assert.equal(subject.password, 'open', 'env password') }) -test('ConnectionParameters initialized from mix', function (t) { +suite.test('ConnectionParameters initialized from mix', function () { + clearEnv() + process.env['PGHOST'] = 'local' + process.env['PGUSER'] = 'bmc2' + process.env['PGPORT'] = 7890 + process.env['PGDATABASE'] = 'allyerbase' + process.env['PGPASSWORD'] = 'open' delete process.env['PGPASSWORD'] delete process.env['PGDATABASE'] var subject = new ConnectionParameters({ @@ -40,12 +58,8 @@ test('ConnectionParameters initialized from mix', function (t) { assert.equal(subject.password, defaults.password, 'defaults password') }) -// clear process.env -for (var key in process.env) { - delete process.env[key] -} - -test('connection string parsing', function (t) { +suite.test('connection string parsing', function () { + clearEnv() var string = 'postgres://brian:pw@boom:381/lala' var subject = new ConnectionParameters(string) assert.equal(subject.host, 'boom', 'string host') @@ -55,7 +69,10 @@ test('connection string parsing', function (t) { assert.equal(subject.database, 'lala', 'string database') }) -test('connection string parsing - ssl', function (t) { +suite.test('connection string parsing - ssl', function () { + // clear process.env + clearEnv() + var string = 'postgres://brian:pw@boom:381/lala?ssl=true' var subject = new ConnectionParameters(string) assert.equal(subject.ssl, true, 'ssl') @@ -75,27 +92,24 @@ test('connection string parsing - ssl', function (t) { string = 'postgres://brian:pw@boom:381/lala' subject = new ConnectionParameters(string) assert.equal(!!subject.ssl, false, 'ssl') -}) -// clear process.env -for (var key in process.env) { - delete process.env[key] -} + string = 'postgres://brian:pw@boom:381/lala?ssl=no-verify' + subject = new ConnectionParameters(string) + assert.deepStrictEqual(subject.ssl, { rejectUnauthorized: false }, 'ssl') +}) -test('ssl is false by default', function () { +suite.test('ssl is false by default', function () { + clearEnv() var subject = new ConnectionParameters() assert.equal(subject.ssl, false) }) var testVal = function (mode, expected) { - // clear process.env - for (var key in process.env) { - delete process.env[key] - } - process.env.PGSSLMODE = mode - test('ssl is ' + expected + ' when $PGSSLMODE=' + mode, function () { + suite.test('ssl is ' + expected + ' when $PGSSLMODE=' + mode, function () { + clearEnv() + process.env.PGSSLMODE = mode var subject = new ConnectionParameters() - assert.equal(subject.ssl, expected) + assert.deepStrictEqual(subject.ssl, expected) }) } @@ -106,6 +120,7 @@ testVal('prefer', true) testVal('require', true) testVal('verify-ca', true) testVal('verify-full', true) +testVal('no-verify', { rejectUnauthorized: false }) // restore process.env for (var key in realEnv) { From e9073f5a00f225670899b2a466fe18b5b047201d Mon Sep 17 00:00:00 2001 From: "Brian M. Carlson" Date: Tue, 5 May 2020 11:03:29 -0500 Subject: [PATCH 3/5] Cleanup & comments --- packages/pg/lib/connection-parameters.js | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/packages/pg/lib/connection-parameters.js b/packages/pg/lib/connection-parameters.js index eead3c39f..83aa1e4e3 100644 --- a/packages/pg/lib/connection-parameters.js +++ b/packages/pg/lib/connection-parameters.js @@ -25,9 +25,11 @@ var val = function (key, config, envVar) { return config[key] || envVar || defaults[key] } -var useSsl = function (modeFromConfig) { +var normalizeSSLConfig = function (modeFromConfig) { // if the ssl parameter passed to config is not a string, just return it // directly (it will be passed directly to tls.connect) + // this way you can pass all the ssl params in via constructor: + // new Client({ ssl: { minDHSize: 1024 } }) etc if (modeFromConfig !== undefined && typeof modeFromConfig !== 'string') { return modeFromConfig } @@ -41,6 +43,11 @@ var useSsl = function (modeFromConfig) { case 'verify-ca': case 'verify-full': return true + // no-verify is not standard to libpq but allows specifying + // you require ssl but want to bypass server certificate validation. + // this is a very common way to connect in heroku so we support it + // vai both environment variables (PGSSLMODE=no-verify) as well + // as in connection string params ?ssl=no-verify case 'no-verify': return { rejectUnauthorized: false } } @@ -77,8 +84,8 @@ var ConnectionParameters = function (config) { }) this.binary = val('binary', config) - // this.ssl = typeof config.ssl === 'undefined' ? useSsl() : config.ssl - this.ssl = useSsl(config.ssl) + + this.ssl = normalizeSSLConfig(config.ssl) this.client_encoding = val('client_encoding', config) this.replication = val('replication', config) // a domain socket begins with '/' From 18649107782196d67b16871bcf2172241c80dda7 Mon Sep 17 00:00:00 2001 From: "Brian M. Carlson" Date: Tue, 5 May 2020 11:08:05 -0500 Subject: [PATCH 4/5] Add test for no-verify string config option --- packages/pg/test/unit/client/configuration-tests.js | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/packages/pg/test/unit/client/configuration-tests.js b/packages/pg/test/unit/client/configuration-tests.js index e6cbc0dcc..d4b16d101 100644 --- a/packages/pg/test/unit/client/configuration-tests.js +++ b/packages/pg/test/unit/client/configuration-tests.js @@ -1,5 +1,6 @@ 'use strict' require(__dirname + '/test-helper') +var assert = require('assert') var pguser = process.env['PGUSER'] || process.env.USER var pgdatabase = process.env['PGDATABASE'] || process.env.USER @@ -43,6 +44,11 @@ test('client settings', function () { assert.equal(client.ssl, true) }) + test('ssl no-verify', function () { + var client = new Client({ ssl: 'no-verify' }) + assert.deepStrictEqual(client.ssl, { rejectUnauthorized: false }) + }) + test('custom ssl force off', function () { var old = process.env.PGSSLMODE process.env.PGSSLMODE = 'prefer' From 7929f6ae44a63b76b2ea58e5d9fc016a2d3f14df Mon Sep 17 00:00:00 2001 From: "Brian M. Carlson" Date: Thu, 7 May 2020 11:36:39 -0500 Subject: [PATCH 5/5] Make change less invasive and fully backwards compatible for native binding config --- packages/pg/lib/connection-parameters.js | 26 +++++++------------ .../test/unit/client/configuration-tests.js | 5 ---- 2 files changed, 9 insertions(+), 22 deletions(-) diff --git a/packages/pg/lib/connection-parameters.js b/packages/pg/lib/connection-parameters.js index 83aa1e4e3..e1d838929 100644 --- a/packages/pg/lib/connection-parameters.js +++ b/packages/pg/lib/connection-parameters.js @@ -25,17 +25,8 @@ var val = function (key, config, envVar) { return config[key] || envVar || defaults[key] } -var normalizeSSLConfig = function (modeFromConfig) { - // if the ssl parameter passed to config is not a string, just return it - // directly (it will be passed directly to tls.connect) - // this way you can pass all the ssl params in via constructor: - // new Client({ ssl: { minDHSize: 1024 } }) etc - if (modeFromConfig !== undefined && typeof modeFromConfig !== 'string') { - return modeFromConfig - } - const mode = modeFromConfig || process.env.PGSSLMODE - - switch (mode) { +var readSSLConfigFromEnvironment = function () { + switch (process.env.PGSSLMODE) { case 'disable': return false case 'prefer': @@ -43,11 +34,6 @@ var normalizeSSLConfig = function (modeFromConfig) { case 'verify-ca': case 'verify-full': return true - // no-verify is not standard to libpq but allows specifying - // you require ssl but want to bypass server certificate validation. - // this is a very common way to connect in heroku so we support it - // vai both environment variables (PGSSLMODE=no-verify) as well - // as in connection string params ?ssl=no-verify case 'no-verify': return { rejectUnauthorized: false } } @@ -85,7 +71,13 @@ var ConnectionParameters = function (config) { this.binary = val('binary', config) - this.ssl = normalizeSSLConfig(config.ssl) + this.ssl = typeof config.ssl === 'undefined' ? readSSLConfigFromEnvironment() : config.ssl + + // support passing in ssl=no-verify via connection string + if (this.ssl === 'no-verify') { + this.ssl = { rejectUnauthorized: false } + } + this.client_encoding = val('client_encoding', config) this.replication = val('replication', config) // a domain socket begins with '/' diff --git a/packages/pg/test/unit/client/configuration-tests.js b/packages/pg/test/unit/client/configuration-tests.js index d4b16d101..e604513bf 100644 --- a/packages/pg/test/unit/client/configuration-tests.js +++ b/packages/pg/test/unit/client/configuration-tests.js @@ -44,11 +44,6 @@ test('client settings', function () { assert.equal(client.ssl, true) }) - test('ssl no-verify', function () { - var client = new Client({ ssl: 'no-verify' }) - assert.deepStrictEqual(client.ssl, { rejectUnauthorized: false }) - }) - test('custom ssl force off', function () { var old = process.env.PGSSLMODE process.env.PGSSLMODE = 'prefer'