Add sslmode verify-ca and verify-full (take 2)

[nyurik]

Implement additional support for postgres ssl_mode parsing. The new modes will still result in a TLS error, but this approach allows users to handle additional cert validation modes before calling connect_tls().

Note that this approach is mostly a noop/backward compatible for all those who don't care about this because if the ssl mode is set to one of the verify modes, existing user code will continue to error-out because connect_tls() will return an error. The only difference is that the error will happen a bit later while trying to connect, instead of failing during the config parse.

Expected usage:

let mut cfg = pg::config::Config::from_str(conn_str)?;

// create a TLS connector

let ssl_mode = cfg.get_ssl_mode();
if ssl_mode == SslMode::VerifyCa || ssl_mode == SslMode::VerifyFull {
  cfg.ssl_mode(SslMode::Required);
  // configure TLS connector to handle verification
}

let manager = PostgresConnectionManager::new(cfg, connector);

This fixes #768 and uses some ideas from #774

[sfackler]

I don't feel great about including options in a library's config that it inherently does not support. I think it would be instead preferable to have the external library have a sort of wrapper parser that adjusts the sslmode before passing the string along.

[jbg]

I wonder if it would be possible for this crate to somehow pass the sslmode through to the TlsConnect impl? At the moment the user of the library needs to manually set up the chosen TLS implementation config (e.g. rustls::ClientConfig in the case of tokio-postgres-rustls) for their desired sslmode, whereas with PG client libraries on other platforms they can simply set sslmode and have this handled for them.

If tokio-postgres-rustls could find out the sslmode from the connection string, it could provide predefined configurations for each mode, while still allowing a ClientConfig to be passed in for users with advanced requirements.

[nyurik]

@sfackler technically, all except the Disable ssl modes are not "supported" by the current implementation, and instead rely on the external SSL code, which in turn does not get the initial options that the user specified. So I think this solution would be the most "sane" - as it would be backwards compatible, while providing the needed functionality. I would love to come up with some other solution to this, but at the end of the day it is either the tokio-postgres that does connection string parsing, or some external crate that we all rely on, and tokio-postgres simply requires as a parameter (this would be even messier)

[deankarn]

First of Thank you for this amazing crate! I ran into this issue recently and decided to chime in on the convo.

FWIW this is a real blocker for implementing a more seamless TLS experience or automatic handling thereof.

Even if the library itself did eventually some day handle via rustls by default, the Config changes would still be necessary in order to setup the TLSConnector properly.

I would argue that adding this to the Config would be ok so long as the user is notified when something is misconfigured, which it appears that this PR does. Additionally it will facilitate the next steps of a more seamless experience and ability for custom configuration until then.

[jbg]

I think any approach which makes this library appear to support verify-ca and verify-full should actually thread those values through to the TLS connector. Otherwise it's just exchanging one type of confusion for another type of confusion, it's not really a meaningful improvement.

One option would be to add a method to the MakeTlsConnect trait, something like:

fn make_tls_connect_with_mode(&mut self, domain: &str, mode: SslMode) -> Result<Self::TlsConnect, Self::Error> {
  self.make_tls_connect(domain)
}

Since it has a default impl, this would not break existing code using the trait (I think the possibility of name collisions can be discounted in this situation).

The crates implementing TLS support (e.g. tokio-postgres-rustls) would then add their own impl of the new method and use it to set up the TLS configuration according to the provided mode.

[nyurik]

I am ok with any approach that would get us towards the more seamless SSL usage. I think it is fundamentally bad for the community adaption to have this amazing library that omits the most critical component - ability to connect to every correctly-configured PostgreSQL server.

Currently, each developer must do a deep dive into SSL configurations and the error-prone TLS library setup. With so much copy/paste required, lots of implementations will have security bugs, giving bad name to the whole ecosystem.

[nyurik]

@sfackler per above, would it possible to move forward with this? I am sure we all would love to get a cleaner SSL solution for all users, and it is clearly required. This solution offers a path forward without backward compat breaking.

[deankarn]

To also add onto @nyurik’s comment no matter the path forward it seems like the config will need to be able to parse these additional values. Required is already present and not supported out of the box also and so see no harm in the other two also being present.

I just had to hack the connection string in my project just to support there other two modes and ensure the Config parsing didn’t fail on them.

Even if your not ready to proceed with any next steps beyond adding to the config I for one would be super grateful to not have to hack the connection string in my projects like this even if just a first step 🙏

[nyurik]

@sfackler friendly ping. I know we are all busy with our favorite projects, but this is really blocking a lot of us, and seems to be the easiest path forward. Thanks! ❤️

[sfackler]

@sfackler technically, all except the Disable ssl modes are not "supported" by the current implementation, and instead rely on the external SSL code, which in turn does not get the initial options that the user specified.

That is not an accurate description of the library's handling of sslmode.

fn make_tls_connect_with_mode(&mut self, domain: &str, mode: SslMode) -> Result<Self::TlsConnect, Self::Error> {
  self.make_tls_connect(domain)
}

This seems at a high level like an reasonable approach, but I don't think it would be a good idea to have a default implementation that ignores the setting. There are also other bits of ssl config that would presumably need to be passed through like sslcert, sslkey, etc.

I am also a bit concerned about opening up built in support for verify-ca. In TYOOL 2023, it is time for people to figure out how to issue certificates with a proper SAN and stick it in their postgres. Every TLS backend I'm aware of for this crate acts as if verify-full was passed unless you explicitly configure things to disable that.

[jbg]

@nyurik

I think it is fundamentally bad for the community adaption to have this amazing library that omits the most critical component - ability to connect to every correctly-configured PostgreSQL server.

The current status quo does in fact have the ability, without any manual TLS configuration, to connect to any correctly-configured PostgreSQL server — you don't need to do any manual configuration of TLS libraries on the client if the server is correctly configured, since the defaults of tokio-postgres-rustls, postgres-native-tls and postgres-openssl should all be equivalent to verify-full, which will work with any correctly-configured PostgreSQL server.

this is really blocking a lot of us

Can you share a little bit more detail about your use case and how this is blocking you, to help ensure that the design of any potential solution here does in fact solve your problem?