node-postgres icon indicating copy to clipboard operation
node-postgres copied to clipboard

Allow users to pass certs when PG environment variable PGSSLMODE=require/verify-ca/verify-full

Open RichardJCai opened this issue 4 years ago • 4 comments

Previously if readSSLConfigFromEnvironment was used to generate the SSL config, the ssl object would not be created to create certs.

I ran into this issue when trying to execute yarn tests to a secure cluster (SSLMODE=require), certs would never be read even when I specified PGSSLCERT, PGSSLKEY, PGSSLROOTCERT.

This fix allows the user to pass in certs through PG env variables.

I'm not great with JS so apologies in advance

Why this is necessary.

To highlight this issue in a simple case

Running the script I attached below through PGSSLMODE=require PGSSLCERT=/home/ubuntu/certs/client.testuser.crt PGSSLROOTCERT=/home/ubuntu/certs/ca.crt PGSSLKEY=/home/ubuntu/certs/client.testuser.key PGHOST=localhost PGPORT=26257 PGUSER=root node test.js results in the following error:

const {Client, Pool} = require("./packages/pg")

const client = new Client()
client.connect(err => {
if (err) {
    console.error('error connecting', err.stack)
} else {
    console.log('connected')
    client.end()
}
})

const pool = new Pool()
pool
.connect()
.then(client => {
    console.log('connected')
    client.release()
})
.catch(err => console.error('error connecting', err.stack))
.then(() => pool.end())
error connecting Error: unable to verify the first certificate
    at TLSSocket.onConnectSecure (_tls_wrap.js:1497:34)
    at TLSSocket.emit (events.js:315:20)
    at TLSSocket._finishInit (_tls_wrap.js:932:8)
    at TLSWrap.ssl.onhandshakedone (_tls_wrap.js:706:12)
error connecting Error: unable to verify the first certificate
    at TLSSocket.onConnectSecure (_tls_wrap.js:1497:34)
    at TLSSocket.emit (events.js:315:20)
    at TLSSocket._finishInit (_tls_wrap.js:932:8)
    at TLSWrap.ssl.onhandshakedone (_tls_wrap.js:706:12)

This is because the ssl field in ConnectionParameters is simply set to true and the certs fields are not populated

ConnectionParameters {
  user: 'root',
  database: 'root',
  port: 26257,
  host: 'localhost',
  binary: false,
  options: undefined,
  ssl: true,
  client_encoding: '',
  replication: undefined,
  isDomainSocket: false,
  application_name: undefined,
  fallback_application_name: undefined,
  statement_timeout: false,
  idle_in_transaction_session_timeout: false,
  query_timeout: false,
  connect_timeout: 0
}

RichardJCai avatar Apr 10 '21 00:04 RichardJCai

They shouldn’t be required, just supported.

Gotcha, thought erroring would be better since if certs aren't passed in with the SSLMODEs then they won't be able to connect and getting a somewhat vague

error connecting Error: unable to verify the first certificate
    at TLSSocket.onConnectSecure (_tls_wrap.js:1497:34)
    at TLSSocket.emit (events.js:315:20)
    at TLSSocket._finishInit (_tls_wrap.js:932:8)
    at TLSWrap.ssl.onhandshakedone (_tls_wrap.js:706:12)
error connecting Error: unable to verify the first certificate
    at TLSSocket.onConnectSecure (_tls_wrap.js:1497:34)
    at TLSSocket.emit (events.js:315:20)
    at TLSSocket._finishInit (_tls_wrap.js:932:8)
    at TLSWrap.ssl.onhandshakedone (_tls_wrap.js:706:12)

is confusing since theres actually no certs to update. As far as I can tell, certs won't be populated anywhere when readSSLConfigFromEnvironment is called.

Do you think logging a warning is appropriate? It certainly would've helped me out.

RichardJCai avatar Apr 10 '21 02:04 RichardJCai

Updated so that it'll return an object with the cert properties.

Please let me know if this is reasonable, mostly the part about returning undefined if the environment variable is not present for the certs.

RichardJCai avatar Apr 19 '21 00:04 RichardJCai

I just tried passing a cert via env var PGSSLROOTCERT and it doesn't work. From reading the node-postgres docs it seems like it should.

node-postgres uses the same environment variables as libpq to connect to a PostgreSQL server.

dyllandry avatar Nov 25 '21 18:11 dyllandry

The CI here is failing because https://github.com/brianc/node-postgres/pull/3110 needs to be merged (cc @brianc)

But this PR is ready for review.

rafiss avatar Jan 05 '24 18:01 rafiss