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

Merged
merged 5 commits into from
Dec 21, 2024
Merged

Add various SSL modes #34

merged 5 commits into from
Dec 21, 2024

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. 😂

@ghivert ghivert force-pushed the feat/add-various-ssl-modes branch from 2540ee7 to 5d35e30 Compare August 24, 2024 15:55
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 ghivert force-pushed the feat/add-various-ssl-modes branch from 5d35e30 to 8ea17b4 Compare November 19, 2024 12:59
@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 3 times, most recently from da4c813 to 164cf1e Compare November 27, 2024 14:41
@ghivert ghivert requested a review from lpil November 27, 2024 14:43
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.

This is looking super nice! I've left 1 bike-sheddy question below.

Could you also add annotations to all the functions please, some are missing the return annotation.

SslEnabled
/// Disable SSL connection.
SslDisabled
}
Copy link
Owner

Choose a reason for hiding this comment

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

Bike shedding on names here!

It kinda sounds like "disabled" is more safe than "unsafe", but I would have thought that enabled > verified but unchecked > unverified. Is that right? Could the names get scarier and uglier as they get less secure?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea is SslEnabled > SslUnsafe > SslDisabled. Maybe we can use SslVerified > SslUnverified > SslDisabled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Implemented this.

@dandeduck
Copy link

Thank you for the work on this! Just wanted to add that it should fix issues like this one from squirrel with connecting to DBs that require SSL, since currently the sslmode=require query doesn't work as expected, they are kinda stuck (except for adding some additional parsing on top 😄)

Hope this can be added soon!

@ghivert ghivert force-pushed the feat/add-various-ssl-modes branch from 8e58f51 to e03115b Compare December 13, 2024 10:13
@ghivert ghivert force-pushed the feat/add-various-ssl-modes branch from e03115b to 486fd3a Compare December 13, 2024 10:16
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.

Thank you!!!

@lpil lpil merged commit 540aa69 into lpil:main Dec 21, 2024
1 check passed
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.

4 participants