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 support for SSL options #22

Merged
merged 2 commits into from
Jul 17, 2024
Merged

Add support for SSL options #22

merged 2 commits into from
Jul 17, 2024

Conversation

ghivert
Copy link
Contributor

@ghivert ghivert commented May 14, 2024

That PR add support for SSL options.
This is required since OTP 26.

PR to merge after arrays has been merged. #22

@lpil
Copy link
Owner

lpil commented May 15, 2024

Hello! I was under the impression that since OTP26 this is no longer required due to the default being to use system cacerts. This is why HTTPC et al can now be safely used without further configuration, while before you needed to manually provide this configuration for each connection.

Have you been having issues with TLS? Thanks

@ghivert
Copy link
Contributor Author

ghivert commented May 16, 2024

That's weird, because I effectively had no problem with HTTPC without further configuration.

However, when trying to reach a Render (render.com) PG instance through HTTPS, I had to explicitly ask to disable peer verification. I can provide you an example if you need. And a non-working HTTPS endpoint too!


Quoting the docs, that's what I did:

In OTP 26, the default value for the verify option is now verify_peer instead of verify_none. Host verification requires trusted CA certificates to be supplied using one of the options cacerts or cacertsfile. Therefore, a connection attempt with an empty option list will fail in OTP 26:

Erlang/OTP 26 . . .

Eshell V14.0 (press Ctrl+G to abort, type help(). for help)
1> application:ensure_all_started(ssl).
{ok,[crypto,asn1,public_key,ssl]}
2> ssl:connect("www.erlang.org", 443, []).
{error,{options,incompatible,
                [{verify,verify_peer},{cacerts,undefined}]}}
The default value for the cacerts option is undefined, which is not compatible with the {verify,verify_peer} option.

To make the connection succeed, the recommended way is to use the cacerts option to supply CA certificates to be used for verifying. For example:

1> application:ensure_all_started(ssl).
{ok,[crypto,asn1,public_key,ssl]}
2> ssl:connect("www.erlang.org", 443, [{cacerts, public_key:cacerts_get()}]).
{ok,{sslsocket,{gen_tcp,#Port<0.5>,tls_connection,undefined},
               [<0.137.0>,<0.136.0>]}}
Alternatively, host verification can be explicitly disabled. For example:

1> application:ensure_all_started(ssl).
{ok,[crypto,asn1,public_key,ssl]}
2> ssl:connect("www.erlang.org", 443, [{verify,verify_none}]).
{ok,{sslsocket,{gen_tcp,#Port<0.6>,tls_connection,undefined},
               [<0.143.0>,<0.142.0>]}}

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.

I had to explicitly ask to disable peer verification. I can provide you an example if you need.

And your application had access to appropriate cacerts via the host? Huh.

OK supporting this configuration sounds good, but we should have a properly design API rather than copying the Erlang API which permits many redundant and invalid configurations. It needs to only support configurations that make sense and are valid.

@ghivert
Copy link
Contributor Author

ghivert commented Jun 14, 2024

Can confirm it's not working anymore with OTP26. Maybe we can wait for pgo issue to handle it correctly.

@ghivert ghivert force-pushed the fix/ssl-options branch from 56396bb to f6d5155 Compare July 4, 2024 09:36
@ghivert
Copy link
Contributor Author

ghivert commented Jul 4, 2024

Maybe we can have an API like

pub type Config {
  /// Other fields omitted for brevity
  Config(ssl: Ssl)
}

pub type Ssl {
  Disabled
  Enabled
  VerifyPeers(Cacerts)
}

pub type Cacerts {
  /// List of certificates to verify against.
  Cacerts(certs: List(BitArray))
  /// Filename of certificate to verify against.
  Cacertfile(filename: String)
}

That way, the SSL is always in a correct state:

  • SSL is disabled.
  • SSL is enabled, and it implies it does not verify peers.
  • SSL is enabled, and verify peers, so you have to provide your Cacerts.

The only required part would be to map that types to the proper pgo tuples used by pgo during initialization.
Maybe we could do better and directly fetch the CA cert on the host?
I can work on that if that can help to ship it faster. 😊 Meanwhile I'm forced to use my (dummy) implementation for SSL, and I suspect users that target OTP 26+ have to use a similar hack.

@lpil
Copy link
Owner

lpil commented Jul 5, 2024

Wouldn't we always want to verify peers? What does enabled do if it doesn't verify?

@ghivert
Copy link
Contributor Author

ghivert commented Jul 5, 2024

In some environments, the certificate can be self-issued, and you'd have to add the CA certificate to the chain. In this case, verification will fail if using standard certificates. Skipping verification would solve this (in an unsound way). It seems fair enough if you're running in a secured environment that requires you to use SSL (like, I don't know, AWS).
I'm not sure about that argument, mainly making assumptions here.

@lpil
Copy link
Owner

lpil commented Jul 5, 2024

Which option is skipping verification? Wouldn't that be a 4th option?

@ghivert ghivert force-pushed the fix/ssl-options branch from f6d5155 to 7ab4d91 Compare July 7, 2024 13:58
@ghivert ghivert requested a review from lpil July 7, 2024 13:58
@ghivert
Copy link
Contributor Author

ghivert commented Jul 7, 2024

I tried another way, and it's working like a charm! I'm curious to have your thoughts on that flow. In the end, I took decision to do an opinionated checking, otherwise SSL is useless. gleam_pgo now grabs the cacerts from the OS, and check everything correctly.
The SSL module and some help from Erlang Forums helped me to have the correct defaults!


I'm also trying to see if providing defaults on pgo directly is not a better choice. Meanwhile, having defaults for gleam_pgo could be a nice way to have a solution right away, and to not be dependent of pgo. I let you see what seems best for you. 🙂

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.

Wonderful! This looks fantastic. Could you update the changelog also please? 🙏

I think it would be excellent if this was included in PGO too

@ghivert ghivert force-pushed the fix/ssl-options branch from 7ab4d91 to b094931 Compare July 9, 2024 09:25
@ghivert
Copy link
Contributor Author

ghivert commented Jul 9, 2024

It's done! Feel free to update the branch or merge whenever you want! For history, I'm trying to see what I can do for pgo in the erlang/pgo#80 issue. :)

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.

Hero!!!

@lpil lpil merged commit 2ee84fb into lpil:main Jul 17, 2024
1 check passed
@ghivert ghivert mentioned this pull request Aug 24, 2024
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.

2 participants