Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

allow postgres connections, that require ssl mode #404

Merged
merged 2 commits into from
Feb 27, 2023
Merged

allow postgres connections, that require ssl mode #404

merged 2 commits into from
Feb 27, 2023

Conversation

orbatschow
Copy link
Contributor

At the moment knex.js does not pass down connection query parameters to build the connection.
Related: knex/knex#2354

This can be bypassed by setting the corresponding environment variable PGSSLMODE.

This PR provides the corresponding documentation. I couldn't find the source code for the official website, so I put this into two places:

  • server/.env.sample
  • docker-compose.yml

So far there is a ticket open to allow connections to Postgres databases, that offer a self-signed certificate: #261

This PR will make a change on how the connection string is build, that should be backwards compatible and allows the usage of self-signed certificates.

@CLAassistant
Copy link

CLAassistant commented Feb 24, 2023

CLA assistant check
All committers have signed the CLA.

@orbatschow
Copy link
Contributor Author

orbatschow commented Feb 24, 2023

Aside from the patch provided within this PR, it seems that it is required to set the environment variable NODE_TLS_REJECT_UNAUTHORIZED to 0 in order to allow the usage of unsigned certificates.

Source: https://nodejs.org/api/cli.html#node_tls_reject_unauthorizedvalue

@meltyshev
Copy link
Member

Hi! Thank you for your contribution 🙏

I just have one question, is the problem only with knex? I meant that we only use knex for database creation and migration. At runtime, all queries are done through the built-in ORM in the sails framework. And as far as I understand it will just work with query parameters?

@orbatschow
Copy link
Contributor Author

orbatschow commented Feb 26, 2023

Hey,

I did a quick and dirty test:

Using the connection string:

postgresql://planka:REDACTED@localhost/planka?sslmode=require

Planka will successfully start up with only the connection string set (after running migrations). I definitely know, that the database migrations didn't work without the changes above, so I guess, that sails.js will just pass the connection string.

I found https://github.com/balderdashy/sails-postgresql/pull/62/files which indicates, that the string is just passed down to pg.

According to the documentation (https://github.com/brianc/node-postgres/blob/master/packages/pg-connection-string/README.md) pg accepts the string and configures everything regarding SSL in a correct manner.

@meltyshev meltyshev merged commit eea57ff into plankanban:master Feb 27, 2023
@meltyshev
Copy link
Member

Thank you for the test!

meltyshev added a commit that referenced this pull request Feb 27, 2023
@meltyshev
Copy link
Member

I had to revert the changes because of the «The server does not support SSL connections» error during the update. We need to check again for compatibility, maybe we missed something?

connection: process.env.DATABASE_URL,
connection: {
connectionString: process.env.DATABASE_URL,
ssl: {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems it will always try to connect using SSL, but this will cause an error if the server does not support SSL connections.

Copy link
Contributor Author

@orbatschow orbatschow Feb 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that setting the variable to false is equal to forcing an SSL connection, which explains, why non SSL connections are not working anymore.

Proposed fix:

function buildSSLConfig() {
  if (process.env.REJECT_UNAUTHORIZED_SSL_CERTIFICATE) {
    return {
      rejectUnauthorized: false,
    };
  }

  return false;
}

module.exports = {
  client: 'pg',
  connection: {
    connectionString: process.env.DATABASE_URL,
    ssl: buildSSLConfig(),
  },
  migrations: {
    tableName: 'migration',
    directory: path.join(__dirname, 'migrations'),
  },
  seeds: {
    directory: path.join(__dirname, 'seeds'),
  },
  wrapIdentifier: (value, origImpl) => origImpl(_.snakeCase(value)),
};

I will create a PR and also add documentation for environment variable within the .env.sample file. The default equals to true, so connections that don't require a SSL connection will be functional without any changes.

I also tested the changes locally with both variants, but it would be good if you do a second test on your own.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants