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

Add various SSL modes #34

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

ghivert
Copy link
Contributor

@ghivert ghivert commented Aug 24, 2024

Hi!

Continuing on my work on PGO, I think it would be better to support the diverse options supported by PG, to try to get a 1:1 support of connection strings, but also to solve common problems when people don't know how to handle CA certificates. More arguments:

As I suspected here, but it seems to be a common issue across PG users. For example, node-pg assumes user is in mode sslmode=require when activating SSL. Digital Ocean too recommend to use sslmode=require by default. If we don't support the different modes, I'm afraid we'll have a lot of issues because providers assumes whether users are powerful DevOps, able to manage CA certificates by hand, or they will just bypass SSL security. Or we'll have a lot of education to do.

Talking about education, while I think it's important to let users be able to do what they want, I think it's important to do things right on security. README now includes a full section on SSL, and how it should be configured. I'm not sure it's desired here. Maybe we should have it in a separated doc, and reference it in gleam.toml docs section? I'm afraid people won't read it then. 😂

Copy link
Owner

@lpil lpil left a comment

Choose a reason for hiding this comment

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

Hello! Thank you for this and the detail you've gone into here.

After reading the documentation I no longer know what the correct configuration is. Why is it that it's called verify and enabled when the others talk of a "require"? How do they match up? What's the optimal configuration?

@ghivert
Copy link
Contributor Author

ghivert commented Aug 26, 2024

If you're not able to understand the correct configuration by reading the README, it means the README is badly worded! I'll cook another version and come back with it 🙂


To answer your question, psql defines some sslmode which seems to can't be mapped to Erlang easily, and which does not make much sense in my opinion. require is "SSL activated, not checking CA certificate", verify-full and verify-ca are "SSL activated, checking CA certificate", but there's some subtleties. Documentation is here, table 34.1. I find the difference between the two rather unclear, and I think verify-ca and verify-full can't be easily distinguished with Erlang.
Reworded, require => {verify, verify_none} while verify-full => {verify, verify_peers}. The best configuration is always verify-full. But require allow to skip CA cert validation (and so, injection of CA cert in your OS).

I wanted to stick with standard PG connection URI, so that's why there's require and other stuff.

@lpil
Copy link
Owner

lpil commented Sep 1, 2024

Gosh that's confusing, I feel both Erlang and PostgreSQL could have done better here!

I think the most important thing for me is that it's clear what the optimal and most secure option is, and that it's the default to prevent people accidentally being insecure.

Thanks again

@TorstenDittmann
Copy link

Is there any progress on this? 👀 Using hosted postgres services like neon.tech or xata.io requires developers to set ssl options 👍🏻

@ghivert
Copy link
Contributor Author

ghivert commented Nov 19, 2024

It took me long time, but I think it's finally in a much better way than before. Tell me @lpil if it seems better to you, or if you think there's still things to change.

@ghivert ghivert force-pushed the feat/add-various-ssl-modes branch 2 times, most recently from 59704be to da4c813 Compare November 27, 2024 14:39
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