Add sslmode verify-ca and verify-full

[uce]

@sfackler Thanks for the discussion in #768. I thought it's best to simply follow up with a proposal for what the proposed change might look like. I'd appreciate your feedback.

As noted in the issue, the general idea here is to mimic Postgres client behavior. I can see that the added modes might also be confusing as it's up to the user to do something useful with them (e.g. use the convenience from_tls_config constructor). Do you think this has potential to land in master?

I'm happy to drop the last commit 39d5178, because it touches quite a bit of infrastructure for maybe too little added coverage.

cc @petrosagg

[sfackler]

This note seems like a pretty big footgun.

[uce]

@sfackler Thanks for the reply. I fully agree and that's also why I was concerned about this.

Before I invest more time here, I would like to get your input on the following:

1. Extend TlsConnect with a sslmode check, e.g.:

   /// Returns whether the provided [`SslMode`] can be satisfied.
   fn supports_mode(&self, mode: SslMode) -> bool {
       match mode {
           SslMode::Disable | SslMode::Prefer | SslMode::Require => true,
           _ => false,
       }
   }

2. Implement for postgres-native-tls and postgres-openssl, e.g. for postgres-openssl:

   fn supports_mode(&self, mode: SslMode) -> bool {
       match mode {
           SslMode::Disable | SslMode::Prefer | SslMode::Require => true,
           SslMode::VerifyCa => self.ssl.verify_mode() == SslVerifyMode::PEER,
           // todo: check hostname verification configured
           SslMode::VerifyFull => self.ssl.verify_mode() == SslVerifyMode::PEER,
           _ => panic!("Unexpected sslmode `{:?}`", mode),
       }
   }

3. Check support in connect_tls(mut stream: S, mode: SslMode, tls: T), e.g.

   if !tls.supports_mode(mode) {
       return Err(Error::tls("client does not support sslmode".into()));
   }

As a first step, this would make misconfigurations of TlsConnect explicit.

Later, we could extend Config with other Postgres client-like options (sslcert, sslkey, sslrootcert) and provide convenience constructors in MakeTlsConnector in both postgres-native-tls and postgres-openssl.

That would make it convenient to get Postgres client-like behaviour and guard against misconfigurations.

Do you think this goes into a better direction? Would you be willing to accept such a contribution?

[uce]

@sfackler Any opinion here? (Sorry to ping you again, please let me know if I should just wait.)

[sfackler]

I'm not sure how doable it would be to adjust MakeTlsConnect and/or TlsConnect implementations to apply extra configuration after they were initially constructed.

[uce]

What about introducing convenience constructors that create MakeTlsConnect and/or TlsConnect with correct configurations (e.g. like here)? The checks (item 3 in the comment above) would guard against wrong configurations and the constructor would be easy to use, e.g. make_tls(config: &Config).

[sfackler]

But if a user does not realize they need to manually parse their config string up front and use that to create the connector things will silently not work as expected.

[uce]

things will silently not work as expected

In item 3 above, the idea was that it will actually fail (and not be silently ignored). Would that be an acceptable solution?

If yes, I could follow-up with an implementation. If no, I think we should close the PR and the linked issue.

[sfackler]

This seems like a pretty awkward interface. You may be better served just doing some pre-processing on the connection string to extract the TLS configuration, and then rewrite it if necessary (verify-full -> require e.g.) before passing it along to tokio-postgres.

[nyurik]

@sfackler looking into this more - it is fairly complicated to pre-parse sslmode because the connection string could come in many different forms (e.g. url-like or space separated), so every implementation would have to duplicate the whole config parsing code without any easy way to extend what you already have.

I propose pg::config::Config::from_str(conn_str) to support all ssl modes as defined by psql, but PostgresConnectionManager will continue to only support disable/prefer/require. If other ssl_mode is given, connect_tls will return an error.

This way, the code using this library will have consistent behavior:

• if the connection string has sslmode=verify-full, this library will parse it, but connect_tls will give a TLS error as kind=tls or kind=authentication (?)
• if the user of the lib want to support it, they will have to handle it while constructing the tls connector, and will modify config with config.clone_as_tls_required() -- which will keep all of the original config vals but replace ssl mode with required.

The benefit of this approach is that all existing code will continue working as before, and if the user doesn't handle new ssl modes, they will continue getting an error - its just that that error will happen a bit later in the process.

[nyurik]

See also an alternative implementation of this in #988 -- a far smaller implementation with just a few lines of code that should address the same need while maintaining backward compatibility