brianc / node-postgres

PostgreSQL client for node.js.
https://node-postgres.com
MIT License
12.37k stars 1.23k forks source link

self signed certificate error #2009

Closed eric-elsholz closed 4 years ago

eric-elsholz commented 5 years ago

Hi. Sorry ahead of time, I don't have a lot of detail here. We updated our packages today and afterwards started seeing a connection error from Sequelize ORM: "self signed certificate". We do in fact use a self signed certificate but this has never been a problem for us in the past. We downgraded from pg@7.13.0 to pg@7.4.3 and then we were able to connect again.

ianwremmel commented 5 years ago

If I’m reading https://github.com/brianc/node-postgres/commit/bf029c827049ca16add0a862d40f4e60dfd9e602 correctly, I think there may have been a bunch of options (including rejectUnauthorized) being inadvertently set to falsy values. They are now not set at all unless you pass them in. The last release was a breaking change, but it’s probably easiest to fix by passing rejectUnauthorized: false to pg.

ianwremmel commented 5 years ago

Sorry, I think I actually meant checkServerIdentity.

charmander commented 5 years ago

I think there may have been a bunch of options (including rejectUnauthorized) being inadvertently set to falsy values

tls.connect treats rejectUnauthorized: undefined as rejectUnauthorized: false? … It does. Oh no. :(

Well, it’s a good thing that was fixed. We should probably add a test for it. It might also be worth a security announcement (@brianc).

powerc9000 commented 5 years ago

God I thought I was going crazy! Thanks for this info rolling back to 7.12 worked for me.

eldimious commented 5 years ago

I am facing the same issue. Rolling back to 7.12.1 worked for me too.

znarf commented 5 years ago

Spotted that in our staging system (heroku), happy to give more details.

BrandonZacharie commented 5 years ago

I'm using Sequelize as well and I got the "self signed certificate in chain" error until I rolled back to 7.12.1

Kinetic-Pulse commented 5 years ago

This one one of the most painful bugs I had to identify. Probably shouldnt do too many updates at once but ye.... Downgrading is the way for now ~7.12.1

ptpt commented 5 years ago

Same here. Very hard to identify. The only error message we got is:

Error: self signed certificate in certificate chain
    at TLSSocket.onConnectSecure (_tls_wrap.js:1321:34)
    at TLSSocket.emit (events.js:210:5)
    at TLSSocket.EventEmitter.emit (domain.js:476:20)
    at TLSSocket._finishInit (_tls_wrap.js:794:8)
    at TLSWrap.ssl.onhandshakedone (_tls_wrap.js:608:12) {
  code: 'SELF_SIGNED_CERT_IN_CHAIN'
}

And we noticed that it occurs when we query PSQL.

Downgraded to pg@7.4.3 and it works now.

jsanta commented 5 years ago

If it may help someone else, we just added the rejectUnauthorized: false, to our sequelize connection configuration:

sequelize: {
    uri:
      'postgres://postgres_user:postgres_user_password@postgres_server:5432/postgres_database',
    options: {
      database: 'postgres_database',
      dialect: 'postgres',
      host: 'postgres_server',
      port: 5432,
      username: 'your_postgres_user',
      password: 'your_postgres_user_password',
      pool: {
        max: 3,
        min: 1,
        idle: 10000,
      },
      dialectOptions: {
        ssl: {
          require: true,
          // Ref.: https://github.com/brianc/node-postgres/issues/2009
          rejectUnauthorized: false,
        },
        keepAlive: true,        
      },      
      ssl: true,
      define: {
        timestamps: false,
      },
    },
  },

lets say the error is still there, but we are not being notified, and the backend is not crashing anymore. Just adjust the configuration according to your own needs. Best regards from Chile.

brianc commented 5 years ago

Gah, I apologize for this! There aren't enough tests around self-signed cert connecting with node-postgres in travis so it was missed. Totally accidental on my part! I'm going to roll out a backwards-compatible (to the 7.x line) semver patch on top of this one to make sure it behaves exactly as it should.

@charmander what do you mean about security announcement? This makes things more secure now right? By forcing unauthorized cert rejection, where as before it was ignoring it? We should probably make the defaults be the same as the node.js defaults in v8.0. Do you mean announcing the current behavior is insecure in that it allows unauthorized certs unless you specifically set rejectUnauthorized to something to true? I agree that's worth calling out. We should also re-introduce this breaking change in 8.0 (w/ a deprecation notice in the 7.x line) so we can get folks to migrate to better ssl configurations.

brianc commented 5 years ago

Okay - released pg@7.14.0 which reverts pg@7.13.0 🤕 sorry about that!

I'd like to get self-signed and "proper" ssl connections tested in travis. Anyone have a docker file handy that sets both of those things up? Then we can be covered on this behavior going forward.

charmander commented 5 years ago

@charmander what do you mean about security announcement? This makes things more secure now right? By forcing unauthorized cert rejection, where as before it was ignoring it?

@brianc Yep, it does! I meant an announcement that pg 0.8.7 through 7.12.0 don’t check certificates.

(edit)

Do you mean announcing the current behavior is insecure in that it allows unauthorized certs unless you specifically set rejectUnauthorized to something to true? I agree that's worth calling out. We should also re-introduce this breaking change in 8.0 (w/ a deprecation notice in the 7.x line) so we can get folks to migrate to better ssl configurations.

Yes, exactly that.

sehrope commented 5 years ago

+1 to having the default in a new major being to validate so that it's an explicit action to disable things.

FYI, the default for libpq (ex: via psql) is not to validate either so the default behavior of this module till now is not totally crazy either. May be worth calling that out in any security announcement: https://www.postgresql.org/docs/current/libpq-ssl.html (Still a +1 to making a security note calling out the behavior)

sehrope commented 5 years ago

@brianc RE: a test setup, the defaults for a Debian / Ubuntu postgresql server install configures it to use a self signed "snake oil" cert. Setting it up in Docker container should be similar.

The PGJDBC driver has a whole bunch of SSL related configs and tests for running things on Travis. Includes overwriting the certs, setting up specific users, installing the sslinfo extension for verifying things. May be worth a peek: https://github.com/pgjdbc/pgjdbc

hghammoud commented 4 years ago

any news on this subject? any pg version that properly supports self signed certs?

brianc commented 4 years ago

I'll add this to pg@8.0 which is under active development & should be released in a week or two, so will look at this shortly sorry for the delay

charmander commented 4 years ago

any news on this subject? any pg version that properly supports self signed certs?

@hghammoud What’s “properly”? If you mean the behaviour of not verifying certificates, that was already restored in 7.14.0. If you want verification, you’ll have to specify options like:

{
    ssl: {
        rejectUnauthorized: true,
        ca: /* the certificate */,
    },
}

and that’s not going to change with 8.0 except for rejectUnauthorized: true being implied (maybe). It’s already done “properly” with respect to the difference between self-signed and trusted by the system.

brianc commented 4 years ago

@charmander - I was looking at the postgres docs...

https://www.postgresql.org/docs/9.1/libpq-ssl.html

Particularly at "Table 31-1. SSL Mode Descriptions" - I think specifying ssl: true and having rejectUnauthorized to false is actually pretty much in line w/ how libpq does things. I think what we should do is here is support being able to opt in to this behavior via the PGSSLMODE=verify-ca"...at that point instead of returningtruewe should return{ rejectUnauthorized: true }` to use it as the config.

Other than that I'm not even totally sure deprecating the ability to leave it undefined it is really the right thing as it behaves the same in libpq....undefined implies false. On one hand it's nice to let users know "hey, are you sure you don't want to reject unauthorized certificates?" but libpq doesn't do that either, it just happily goes along and doesn't verify unauthorized certs unless explicitly asked to do so. as far as I can see this is the final kinda dangling issue WRT me releasing a new semver minor 7.x w/ the warning included as well as doing (or not) doing any changes for 8.0.

charmander commented 4 years ago

It’s in line with how libpq does things, but I still think it’s better to be secure by default – in line with Node’s tls. I was surprised by it, at least, but maybe that was influenced by reading the code (which can make for misleading reading because the change to not verifying by default was an accident). Parsing libpq’s connection strings into the appropriate settings would be a good change, though, yeah. (And to be clear, the deprecating the ability to leave it undefined isn’t supposed to be permanent, right? It’ll default to rejectUnauthorized: false with the new major version.)

brianc commented 4 years ago

(And to be clear, the deprecating the ability to leave it undefined isn’t supposed to be permanent, right? It’ll default to rejectUnauthorized: false with the new major version.)

Well that's why I brought it up - I'm not sure we should default to rejectUnauthorized: false if you do something like

const client = new Client({ ssl: true })

I guess we could do that....you're right its more secure by default.

I think in that case we should make sure if you use the environment variables:

PGSSLMODE=require 
PGSSLMODE=verify-ca

We pass the proper value of either { rejectUnauthorized: false } or { rejectUnauthorized: true } into the ssl config so that stays in line with how the environment variable works. So, yeah in 8.0 I'll need to get some better tests around ssl handshakes in their various forms and then fix the behavior of the environment vars to do what's "expected" in all cases and make sure native and pure js line up in behavior...that'll be the last thing before 8.0! I'll start working on this this week...it's a bit more fiddly that the other changes because SSL is sorta complex by design so...it'll take a bit (but not too long!)

brianc commented 4 years ago

To elaborate on these changes a bit I think it should be like...

const client = new Client({ ssl: true })
// default ssl config = { rejectUnauthorized: true }
process.env.PGSSLMODE='require'
// default ssl config = { rejectUnauthorized: false }
const client = new Client() // does NOT reject unauthorized
const client = new Client({ ssl: true }) // rejects unauthorized
const client = new Client({ ssl: { rejectUnauthorized: false }}) // does NOT reject unauthorized
const client = new Client({ ssl: { rejectUnauthorized: true }}) // rejects unauthorized
process.env.PGSSLMODE = 'verify-ca'
// default ssl config = { rejectUnauthorized: true }
const client = new Client() // rejects unauthorized
const client = new Client({ ssl: true }) // rejects unauthorized
const client = new Client({ ssl: { rejectUnauthorized: false }}) // does NOT reject unauthorized
const client = new Client({ ssl: { rejectUnauthorized: true }}) // rejects unauthorized

I think in every case we want the connection parameters (which parse env vars) to generate a default ssl config, and in every case we want to allow any custom ssl: { ... } config passed to the client to overwrite the default config and extend it.

e.g.

PGSSLMODE=require
const client = new Client({
  ssl: {
     rejectUnauthorized: true
  }
})

should make it reject unauthorized. Anything you pass explicitly to { ssl: { ... } } takes precedence over environment variables and has the "final say" in the ssl config.

brianc commented 4 years ago

I'll document these permutations and also include a note to discourage the use of new Client({ ssl: true }) as the meaning is ambiguous. Perhaps we deprecate it in 8.0 in favor of environment variables or a more explicit full ssl: { ... } config

jylopez commented 4 years ago

In case it's not obvious (it wasn't to me), this is the solution:

const pool = new pg.Pool({
  connectionString: process.env.DATABASE_URL,
  ssl: {
    rejectUnauthorized: false
  }
});

Specifically, when trying to provision a heroku postgres server. Heroku's doc is not updated. It incorrectly says to put:

const pool = new Pool({
  connectionString: process.env.DATABASE_URL,
  ssl: true
});
fapdev77 commented 4 years ago

Also not obvious to me... Just a small correction use:

const pool = new Pool({ ....

instead of

const pool = new pg.Pool({ ...

Worked to me.

Thanks @jylopez

charmander commented 4 years ago

The original issue was fixed in pg 7.14.0, and the default change was made in 8.0. Support for the rest of the sslmode options is in #2105 (I think).

johnschoeman commented 4 years ago

In case it's not obvious (it wasn't to me), this is the solution:

const pool = new pg.Pool({
  connectionString: process.env.DATABASE_URL,
  ssl: {
    rejectUnauthorized: false
  }
});

Specifically, when trying to provision a heroku postgres server. Heroku's doc is not updated. It incorrectly says to put:

const pool = new Pool({
  connectionString: process.env.DATABASE_URL,
  ssl: true
});

For anyone coming here trying to fix a self-signed-certificate error on their Heroku + node + pg production instance, the recommended solution is to use pg < 8.0.0. and not turn off SSL altogether.

charmander commented 4 years ago

the recommended solution is to use pg < 8.0.0

@johnschoeman Whose recommended solution? I would recommend what @jylopez wrote (ssl: {rejectUnauthorized: false}) if you want the pg 7.x behaviour, not downgrading to pg 7.x.

johnschoeman commented 4 years ago

@charmander , heroku's recommendation. Their docs say the preferred way is to use pg < 8.0.0: https://devcenter.heroku.com/articles/getting-started-with-nodejs#provision-a-database, apologies for not referencing that in the initial comment.

I would recommend what @jylopez wrote (ssl: {rejectUnauthorized: false}) if you want the pg 7.x behaviour, not downgrading to pg 7.x.

Does this mean that these are equivalent? I may be confused with the API, but I read ssl: { rejectUnauthorized: false } as allowing requests without SSL certs.

DiegoRBaquero commented 4 years ago

They are not equivalent. Using ssl: { rejectUnauthorized: false } means enabling SSL but not rejecting self-signed.

We have different envs with different ssl/non-ssl settings, connection string is used, this broke things.

brianc commented 4 years ago

heroku's recommendation. Their docs say the preferred way is to use pg < 8.0.0:

oh no! First I heard about this... I'll email them about this. Not good to recommend an old version, particularly when it's less secure out of the box.

brianc commented 4 years ago

They are not equivalent. Using ssl: { rejectUnauthorized: false } means enabling SSL but not rejecting self-signed.

Yeah that's correct - that's what you want to do with heroku. This is exactly what the old behavior on pg@7.x did when you supplied { ssl: true }. What do you mean you have different envs with different ssl/non-ssl settings? Downgrading to 7.x is going to cause you problems as more fixes & features continue to be added to 8.x and beyond. I've reached out to heroku about this. I'm a bit annoyed they didn't mention anything here to me, but just updated their docs to have people install an old version....

What issues is it causing you exactly? I'd like to understand better so I can recommend a fix.

brianc commented 4 years ago

I sent in a support ticket on my paid heroku account, submitted feedback to the documentation article, and tweeted at them...hopefully I hear something back so I can help get this taken care of and clarity delivered to everyone.

brianc commented 4 years ago

In case it's not obvious (it wasn't to me), this is the solution:

Sorry about that! I tried to document the change here. I'm gonna try working with heroku to get their docs updated.

jylopez commented 4 years ago

@brianc Hey no problem man. You're doing a great job. Thanks for caring!

DiegoRBaquero commented 4 years ago

They are not equivalent. Using ssl: { rejectUnauthorized: false } means enabling SSL but not rejecting self-signed.

Yeah that's correct - that's what you want to do with heroku. This is exactly what the old behavior on pg@7.x did when you supplied { ssl: true }. What do you mean you have different envs with different ssl/non-ssl settings? Downgrading to 7.x is going to cause you problems as more fixes & features continue to be added to 8.x and beyond. I've reached out to heroku about this. I'm a bit annoyed they didn't mention anything here to me, but just updated their docs to have people install an old version....

What issues is it causing you exactly? I'd like to understand better so I can recommend a fix.

Hey, thanks for answering

Right, so first, I'm not using Heroku at all. We are using Aptible, GCP and AWS. We have different settings in different envs, some support SSL (self signed) and other don't. We use connection strings in env variables. So there's no way to remain using that right now whille passing this setting. We have over 50 microservices in over 6 different env, so it's a lot of work to put this setting where we have SSL, ignore it where not.

We have massive -> pg-promise > node-pg. pg-promise updated driver to 8 in a minor version and screwed us (https://github.com/vitaly-t/pg-promise/issues/711). Now we are stuck in massive 6.2.0 because of this.

x2 Thank you for caring!

brianc commented 4 years ago

hmmmm is there an environment variable you think I could add to make this controllable too or easier for you to roll out? I know psql supports PGSSLMODE with a bunch of different options. I could add something like PGSSLMODE=no-verify which would be the eqiv of passing { rejectUnauthorized: false } in as the ssl param? Then you could update env vars and not have to udpate code or do something like reach down into massive -> pg-promise -> node-pg to figure out how to thread the ssl config option down through all the layers.

They do have PGSSLMODE=require which can fallback to not checking if there is no CA present. I'm not sure how postgres checks for a root cert....I could likely dig in postgres client C code & find it...but w/ such limited time that's kinda lower on my priority list than making pg faster, which is what I have in the works coming up.

johnschoeman commented 4 years ago

Sorry about that! I tried to document the change here. I'm gonna try working with heroku to get their docs updated.

@brianc , thanks for the prompt response! This clears the confusion for me. 💯 I took a pass at updating the docs, if you're interested; I am new to node-postgres, and it wasn't clear to me to look in the announcements for this info. https://github.com/brianc/node-postgres-docs/pull/71

nabilfreeman commented 4 years ago

Is there a way to trust some self-signed certificates and not others?

Forgive me if I don't know SSL well enough, but the way I understand it is that my RDS instances from AWS have self signed certificate as part of their provisioning process and I've downloaded and trusted them on my server. This is the only option for SSL certs on RDS.

So what I would expect is that those certificates don't throw an error, but if another certificate appeared with a different hash that one would be rejected as a security risk.

Or is it trivial to spoof self-signed certs?

charmander commented 4 years ago

@nabilfreeman Specify the certificate as the ca option, like this:

new pg.Pool({
  ssl: {
    ca: fs.readFileSync('cert.pem', 'ascii'),
  },
})

(this can also be an array)

surferwat commented 4 years ago

In case it's not obvious (it wasn't to me), this is the solution:

const pool = new pg.Pool({
  connectionString: process.env.DATABASE_URL,
  ssl: {
    rejectUnauthorized: false
  }
});

Specifically, when trying to provision a heroku postgres server. Heroku's doc is not updated. It incorrectly says to put:

const pool = new Pool({
  connectionString: process.env.DATABASE_URL,
  ssl: true
});

After also adding PGSSLMODE=no-verify to .envworked for me.

brianc commented 4 years ago

After also adding PGSSLMODE=no-verify to .envworked for me.

👍 Working w/ heroku on getting the docs updated. I didn't ever intend to require code changes to heroku apps or configs when upgrading to pg@8.0....so trying to make it easier now by adding the PGSSLMODE=no-verify. It can also be added to the connection string ?ssl=no-verify

Kellym-Kainos commented 4 years ago

Does anyone know if it's possible to use nodes built in certs here:


  ssl: {
    rejectUnauthorized: true,
    ca: [path to ca]
  } ```

I've been able to use amazon rds cert but would be good to be able to use node certs so we dont have to update every 4/5 years
brianc commented 4 years ago

Does anyone know if it's possible to use nodes built in certs here:

Howdy! You should be able to pass any of the config options that node's ssl constructor takes directly through - pg will just pass them in unaltered if you pass an object as the ssl option to the pg client/pool constructor.

Kellym-Kainos commented 4 years ago

@brianc thanks however when i tried using one of these certs https://github.com/nodejs/node/blob/master/src/node_root_certs.h I'm still getting self signed certificate in chain error.

Do you ignore built in node certs?

boromisp commented 4 years ago

You can access the default root certificates as tls.rootCertificates and you can extend the default root certificate list with the NODE_EXTRA_CA_CERTS=file environment variable.

I don't think the AWS RDS server certificates are signed with any generally trusted root certificates, so your only option is to download and explicitly trust one of the root certificates provided by amazon.

Based on this serverfault answer on you can view your database's certificate chain with this command:

echo "" | openssl s_client -starttls postgres -connect EXAMPLE.COM:5432 -showcerts
valentincostam commented 4 years ago

This worked for me:

const isProduction = process.env.NODE_ENV === 'production'

const pool = new Pool({
  connectionString: process.env.DATABASE_URL,
  ssl: isProduction ? {rejectUnauthorized: false} : false
})

This way you prevent from getting the following error when your app is running on Heroku:

Error: self signed certificate

And also this error when it's running on localhost:

Error: The server does not support SSL connections

I'm using node v12.18.1, express v4.17.1, and pg v8.3.0.

koistya commented 4 years ago

When using a self-signed certificate, don't forget to specify servername. For example, in the case with Google Cloud SQL it would be {GOOGLE_GLOUD_SQL_PROJECT}:{GOOGLE_CLOUD_SQL_INSTANCE}:

new pg.Pool({
  ssl: env.PGSSLMODE === "verify-ca" && {
    cert: fs.readFileSync(env.PGSSLCERT, "ascii"),
    key: fs.readFileSync(env.PGSSLKEY, "ascii"),
    ca: fs.readFileSync(env.PGSSLROOTCERT, "ascii"),
    servername: env.PGSERVERNAME, // e.g. "example:pg12"
  }
});

Reference: Google Cloud SQL - Tips & Tricks, Node.js API Starter Kit

adrianhorning08 commented 3 years ago

I am using Digital Oceans App Platform and Managed Database using "pg": "^8.5.1", and this works: (specifying user, host, db, pw, and port)

const pool = new Pool({
  user: process.env.DB_USER,
  host: process.env.DB_HOST,
  database: process.env.DB,
  password: process.env.DB_PASSWORD,
  port: process.env.DB_PORT,
  ssl: {
    rejectUnauthorized: true,
    ca: fs.readFileSync("ca_cert.crt").toString(),
  },
});

but this doesn't (just putting the connection string)

const pool = new Pool({
  connectionString: ***sslmode=require,
  ssl: {
    rejectUnauthorized: true,
    ca: fs.readFileSync("ca_cert.crt").toString(),
  },
});

So yeah that sslmode=require appending on the connection string I think is messing it up.

Why would the first one work and the second doesn't?

Wanted to put this here in case some poor soul (like future me) needs it.

seamoss commented 3 years ago

I am using Digital Oceans App Platform and Managed Database using "pg": "^8.5.1", and this works: (specifying user, host, db, pw, and port)

const pool = new Pool({
  user: process.env.DB_USER,
  host: process.env.DB_HOST,
  database: process.env.DB,
  password: process.env.DB_PASSWORD,
  port: process.env.DB_PORT,
  ssl: {
    rejectUnauthorized: true,
    ca: fs.readFileSync("ca_cert.crt").toString(),
  },
});

but this doesn't (just putting the connection string)

const pool = new Pool({
  connectionString: ***sslmode=require,
  ssl: {
    rejectUnauthorized: true,
    ca: fs.readFileSync("ca_cert.crt").toString(),
  },
});

So yeah that sslmode=require appending on the connection string I think is messing it up.

Why would the first one work and the second doesn't?

Wanted to put this here in case some poor soul (like future me) needs it.

Also seeing this here. Refactored to parse strings to dicts for now, but a strange behavior, indeed.