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

chore(electric): Flip the default value of DATABASE_REQUIRE_SSL to true #767

Merged
merged 8 commits into from
Dec 22, 2023

Conversation

alco
Copy link
Member

@alco alco commented Dec 17, 2023

This is a better default for any environment other than, perhaps, dev.

Until now, connecting to a Postgres that was configured to only accept SSL connection would result in a cryptic invalid_authorization_specification error (FWIW the same error may be logged when Postgres is configured to only accept unencrypted connection, but this is a rare edge case). That was the case with Crunchy Bridge and AWS RDS.

With this new default, more hosted databases should be supported out of the box. One prominent exception is Fly Postgres (at least prior to its takeover by Supabase): since Fly machine all connect to the same encrypted private network, there is no benefit of an additional encryption for database connections on top of that. For cases like this, we need to print a friendly error message instructing the developer to set DATABASE_REQUIRE_SSL=false in their Electric configuration. This will be addressed in #762.

Note that Electric also needs to be able to verify the database host's authenticity to prevent man-in-the-middle attacks. I've filed VAX-1446 to address this at a later time.

Also, the added burden of setting DATABASE_REQUIRE_SSL=false in dev is not lost on me. I'm implementing support for .env files in #770. That will remove some configuration-related woes in dev.

@alco alco force-pushed the alco/sslmode branch 2 times, most recently from b1e2e0e to e2296cf Compare December 19, 2023 15:06
@alco alco changed the base branch from main to alco/database-use-ipv6 December 19, 2023 15:06
@alco alco changed the title Alco/sslmode chore(electric): Flip the default value of DATABASE_REQUIRE_SSL to true Dec 19, 2023
@alco alco marked this pull request as ready for review December 19, 2023 15:45
Copy link
Contributor

@magnetised magnetised left a comment

Choose a reason for hiding this comment

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

I'm sure you've thought about this but isn't there an "optimistic" version of this setting that will try ssl then fallback if it doesn't work? wouldn't that work everywhere? in this pr you refuse use of the sslmode param but isn't sslmode=prefer exactly the behaviour we want? i.e. the client sends an SSLRequest message and the server responds with either S or N

@alco
Copy link
Member Author

alco commented Dec 21, 2023

@magnetised This PR bundles a couple of independent changes:

  • enforcement of SSL connections by default. What you're saying about sslmode=prefer can be achieved by setting DATABASE_REQUIRE_SSL=false. But I don't think it should be the default for production deployments. So far we've roughly followed the "secure by default" approach with AUTH_MODE being set to secure by default.

    Of course, it can be argued, that a database is probably always hidden from the Internet in a private network in production deployments, but I'm not sure that's valid anymore. Hosted PGs like Supabase and Crunchy Bridge make it easy to connect to them over the public Internet.

  • rejection of DATABASE_URL if it has the sslmode query param. This is because we don't intend to support all of the sslmode values that psql supports. I remember originally asking for this option to be supported but I've reversed my position on this topic after doing some research into the use of SSL with Postgres and how the pg_hba.conf file can be used to restrict incoming connections based on host and encryption rules.

@magnetised
Copy link
Contributor

@alco thanks for the clarification.

Base automatically changed from alco/database-use-ipv6 to main December 22, 2023 09:47
This is an edge case that's not handled by epgsql.

Epgsql can handle the case where it tries to establish an SSL connection
but gets back a "no encryption" response from Postgres. It will keep the
connection open, though it will be unencrypted.

However, Fly Postgres seems to always close its end of the connection
if it's not configured to run in SSL mode. Epgsql does not try to
reconnect in this case, so we do this ourselves after removing the SSL
connection option.
We override the value of "replication" anyway, so it doesn't make sense
to allow it to be passed in.

As for the sslmode, we do not support all the options that psql does.
Our rules are simpler: use SSL by default but allow falling back to
unencrypted connections by setting DATABASE_REQUIRE_SSL to false.
@alco
Copy link
Member Author

alco commented Dec 22, 2023

FYI I had to implement SSL support in the proxy because it wouldn't be able to establish an upstream connection when the database was configured to require SSL. I'm baffled at the fact that we haven't noticed this until now.

I've seen Postgres reject connections when this setting was set to its default value.
This authentication method is used by Neon. And it's related to the SSL implementation here
because there's no way Postgres would be requesting a cleartext password over an unencrypted
connection.
@alco alco merged commit e8bb9a8 into main Dec 22, 2023
4 of 5 checks passed
@alco alco deleted the alco/sslmode branch December 22, 2023 12:52
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.

None yet

2 participants