Support Postgres environment variables for SSL configuration

[mrooding]

Changes

#2967 introduced additional environment variables to configure SSL on Postgres. The plugin-server didn't support these yet. This PR adds support for the same environment variables. This fixes #440

There's 1 caveat, which is that the sslmode cannot be set using the environment variable. It can only be set through the DATABASE_URL (brianc/node-postgres#2345). I did implement POSTHOG_POSTGRES_SSL_MODE to easily detect whether to include SSL settings. I could remove this check altogether and simply always add the ssl {} config block but I'm unsure how the underlying TLS Socket will behave when receiving empty values.

I'll do an additional PR to the docs repo to clarify the usage of POSTHOG_POSTGRES_SSL_MODE properly.

I didn't run the jest tests since I couldn't get the test setup to work properly. I couldn't find any instructions on how to run the tests. Tried starting the docker-compose.dev.yml from the Posthog repo which failed to authenticate, then I found the setup:test:postgres script but that seems to depend on Postgres being installed on my local machine instead of using Docker. What's the right approach to test the plugin server? Would be a great addition to the README for other contributors.

Checklist

• Updated Settings section in README.md, if settings are affected
• Jest tests

[neilkakkar]

Thanks, Marc! About the development instructions, indeed, plugin server depends on running posthog locally, instructions for which are in the posthog repo and here: https://posthog.com/docs/developing-locally - good shout though, I'll add this to the docs too.

[mrooding]

Thanks @neilkakkar for the link. I'll have a look at it asap 👍

[mrooding]

Hi @neilkakkar

The link you gave me helped set up Posthog locally but I'm still unsure on how to get the plugin-server tests to execute properly. The setup:test:postgres still assumes that you have Postgres locally installed in order to execute createdb. It would be nice to have a setup script which is compatible with running Postgres only through Docker. For example, by adding another container in docker-compose.dev.yml that executes the dropdb and createdb. I'd be happy to give this a try.

Anyway, I was able to run the tests by creating the db manually in the container and then exporting the DATABASE_URL. Some tests were failing but they seemed unrelated to my changes.

[neilkakkar]

I think it makes sense to follow what the main PostHog repo does here: https://github.com/PostHog/posthog/blob/master/posthog/settings.py#L383-L387 - that is, passing the certs as env vars instead of files.

The same variables are used in both places

[neilkakkar]

I've made these changes, let me know if they don't work for you?

[neilkakkar]

.. My bad, looks like these are supposed to be PATHs, and the database URL takes a path, but when inserted into as options into node-pg client, taken in as the actual cert strings. Will revert to reading the file here.

[neilkakkar]

Maybe we should support these env vars on Heroku, too? Not familiar with why Heroku is setup this way, @Twixes ?

[Twixes]

That should be good if ssl also has rejectUnauthorized: false then. Though I was under the impression that an SSL setup with the certs correctly specified shouldn't need rejectUnauthorized: false. Unfortunately on Heroku Postgres (for which process.env.DYNO is a proxy) we don't know the certs due to the way these DBs are provisioned, but someone deliberately configuring this would be safer with rejectUnauthorized: true – as far as I know, I may be wrong.

[neilkakkar]

Ah, this makes perfect sense! @mrooding I'm guessing this would work for you as well? i.e. setting rejectUnauthorized to true when users are providing their own certs and keys.

[mrooding]

rejectUnauthorized should indeed be true. Skipping verification should definitely not be the default. Maybe it would be good to allow the user to set this using an environment variable. If anyone's using self-signed certs they wouldn't be able to get the connection working

[mrooding]

@neilkakkar I added a new config property for the rejectUnauthorized which defaults to true. @Twixes I left the heroku setting hardcoded to true since you mentioned that Heroku manages its own certs and I'm assuming that it's impossible to define your own self-signed certs on Heroku.

[Twixes]

To merge or not to merge is up to you @neilkakkar :)

[neilkakkar]

I think before I merge I'd really like a test for this - it feels too unsafe otherwise.

[yakkomajuri]

Hey @mrooding! Could you potentially write a test for this and take a look at the code quality errors?

@neilkakkar is working on non plugin server tasks for this cycle so he unfortunately won't have time to pick this up.

[yakkomajuri]

Friendly ping @mrooding

[mrooding]

It's been a crazy week @yakkomajuri. I'd be happy to add a test.

If we want to have a test that verifies whether the SSL connection works properly then we'll have to adjust the Posthog docker-compose script to start Postgres with a (self-signed) certificate. The same cert would have to be included in the plugin-server repository to create and verify the connection.

Let me know if you agree with this approach before I take a stab at it.

[mrooding]

Hi @yakkomajuri

It just dawned on me that this was still open. Could you let me know what you thought of my proposal? Would be happy to finish this off

[yakkomajuri]

Yeah I think this should be alright - could you take a look at the merge conflicts?

[yakkomajuri]

Closing for now as we're moving to a monorepo soon. Happy to revisit though.