-
Notifications
You must be signed in to change notification settings - Fork 28
Add configurable and extendable ssl options #52
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
base: main
Are you sure you want to change the base?
Conversation
lpil
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! I've left a note below about the configuration design.
src/pog.gleam
Outdated
| /// Used to fine-tune SSL connection behavior, such as SNI (Server Name Indication) | ||
| /// which is important for proper certificate verification when connecting to | ||
| /// databases with virtual hosting or multi-domain certificates. | ||
| ssl_options: SslOptions, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think having this be not-part of the Ssl type makes it possible to have invalid configuration. That is, you can disable SSL and enable SNI, but as I understand it that is not actually possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True! Right now it would just ignore it. I could add a runtime validation on it? Or would you like me to try out some types magic to get this covered?
Edit: Nvm, I see you want it to be part of the Ssl type.
This would be a breaking change though (i think), is that a problem?
Made the changes accordingly, hope this looks more to your liking. The thing im a little wary of in this implementation is that it forces people to understand what sni is and make the choice or have to dive into what it is in order to know wether to put this on true or false. I'd personally prefer it more if it is taken care of in best practice unless specified otherwise. If you have any idea how to make that happen, let me know. Another option would be something like this: pub opaque type Ssl {
SslVerified(sni_enabled: Bool)
SslUnverified(sni_enabled: Bool)
SslDisabled
}
/// ...
pub fn ssl(config: Config, ssl: Ssl) -> Config {
Config(..config, ssl:)
}
pub fn ssl_verified(sni_enabled: Option(Bool)) -> Ssl {
case sni_enabled {
option.None -> SslVerified(sni_enabled: True)
option.Some(se) -> SslVerified(sni_enabled: se)
}
}
pub fn ssl_unverified(sni_enabled: Option(Bool)) -> Ssl {
case sni_enabled {
option.None -> SslUnverified(sni_enabled: True)
option.Some(se) -> SslUnverified(sni_enabled: se)
}
}
pub fn ssl_disabled() -> Ssl {
SslDisabled
}In this case you would be able to use |> pog.ssl(pog.ssl_unverified())Without manually setting any ssl options, wdyt? |
lpil
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lovely!! Thank you!
lpil
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops sorry, I was too hasty. Few tiny notes inline
| - `SslUnverified`: Enables SSL without certificate verification (use with caution) | ||
| - `SslDisabled`: No SSL encryption (not recommended for production) | ||
|
|
||
| Both `SslVerified` and `SslUnverified` modes support Server Name Indication (SNI), which is essential for proper certificate verification when connecting to databases using virtual hosting or multi-domain certificates. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a note saying it's recommended to set this to true please
| } | ||
| ``` | ||
|
|
||
| The `sni_enabled` parameter (defaults to `True`) helps ensure proper SSL certificate verification by sending the server name during the SSL handshake. This is particularly important for: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it default to true? How is that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that was my original plan but realised default are not that simple in Gleam, I'll remove it, good catch
| /// Never ignore CA certificate checking _unless you know exactly what you are | ||
| /// doing_. | ||
| SslVerified | ||
| SslVerified(sni_enabled: Bool) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Document the sni_enabled property and recommend to set this to true please 🙏
| /// doing._ In case `pog` can not find the proper CA certificate, take a look | ||
| /// at the README to get some help to inject the CA certificate in your OS. | ||
| SslUnverified | ||
| SslUnverified(sni_enabled: Bool) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Document the sni_enabled property and recommend to set this to true please 🙏
Add SSL Options support
This PR adds support for configuring SSL options in
pog, starting with Server Name Indication (SNI) support.As mentioned: #51
Changes
SslOptionstype to handle SSL-specific configurationssni_enabledflag (default:True)ssl_optionsfunction to configure SSL optionsExample
The changes improve SSL connection handling by making it more configurable and secure by default, particularly for scenarios involving virtual hosting or multi-domain SSL certificates.
Note to maintainer(s):
Please let me know if this makes sense to you or you'd like to see it differently. I thought this could be an extendable way of adding options.
It is also possible to give people the option to chose the host instead of a bool, however I've yet to find a reason why you'd ever want to use a different host for sni than the host you are connecting to.
I'm very much a beginner of gleam or functional languages in general.
I have 0 experience with erlang. So keep this in mind if you have comments.
Ran all tests succesfully with
gleam test. However there are currently no tests for anything Ssl related.