Add SSL/TLS support for Postgres connections

[wseaton]

As discussed in #103, currently there's no way to connect to a postgres database that mandates a TLS/SSL connection.

What this PR does is make it so sslmode=required is functional, and if a root cert and no client certs are passed it falls back to the behavior of sslmode=verify-ca.

Example conn string:

conn = f"postgresql://{user}:{pw}@my.database.host:5439/db?sslmode=require&sslrootcert=/tmp/myroot.crt"
df = cx.read_sql(conn, "select 1;", return_type='pandas', protocol='cursor')

There is a lot of upstream discussion on the topic and I've tried to link to it in doc strings where appropriate. Tested it on Amazon Redshift, but looking for pointers on how to add functional tests for this. All of the code probably isn't idiomatic rust either, so looking for some feedback there too. Thanks!

[wangxiaoying]

Hi @wseaton, thanks for the PR! The logic of the connection construction looks pretty good to me and the docstrings make it very clear. There are only two things that I think might need some changes:

1. We already have the dependency on native-tls (for mysql), so it would be better if we use the same for postgres
2. Before the source construction (PostgresSource::new), we will also query the range of the partition key in the case that partition number is given and range is not (source_router::pg_get_partition_range). The connection construction here also need to be modified in order to support TLS.

I can make some modifications for the 2 issues above as well as add some tests. Let me know if you have any concerns : )

[wseaton]

Hey @wangxiaoying, thanks for the quick review!

1. We already have the dependency on `native-tls` (for mysql), so it would be better if we use the same for postgres

Hmm, I agree, although I could not find a simple method to add custom client certificates when using postgres-native-tls, only the addition of a trusted root ca. In addition I think the postgres docs mention requiring openssl for clients so it may be unavoidable, perhaps it could be gated behind a feature flag defaulting to NoTls if unset?

2. Before the source construction (`PostgresSource::new`), we will also query the range of the partition key in the case that partition number is given and range is not (`source_router::pg_get_partition_range`). The connection construction here also need to be modified in order to support TLS.

I can definitely fix this one.

I can make some modifications for the 2 issues above as well as add some tests. Let me know if you have any concerns : )

Sure, feel free to make any modifications you wish :)

One other thing I noticed was that the type alias for PgManager is not very ergonomic due to generics and may cause issues if we want to alternate between NoTls and a MakeTlsConnector implementation, not sure what to do about that.

[wangxiaoying]

@wseaton Thanks for the quick reply : )

Hmm, I agree, although I could not find a simple method to add custom client certificates when using postgres-native-tls, only the addition of a trusted root ca. In addition I think the postgres docs mention requiring openssl for clients so it may be unavoidable, perhaps it could be gated behind a feature flag defaulting to NoTls if unset?

Indeed, I also found that openssl is necessary if we need to support client authentication. But it seems like it is not a common need. According to this doc, client side authentication is used to defend against impersonation, which is typically due to insecure password management. Also there seems to be some compatibility issue for openssl (between 1.0 and 1.1). Maybe we can only support server authentication for now (sslmode>=require and sslrootcert) and add the client authentication if there are needs for this? (It seems like sslpassword is also related to client certificate, we can add support for this together if needed.)

One other thing I noticed was that the type alias for PgManager is not very ergonomic due to generics and may cause issues if we want to alternate between NoTls and a MakeTlsConnector implementation, not sure what to do about that.

Totally agreed. I'm trying to see whether there is a good way to make it more generic. It would be better if we could alter between NoTLS and MakeTlsConnector.

[wseaton]

@wangxiaoying refactored things a little bit, and moved the main logic for SSL construction into impl From<TlsConfig> for MakeTlsConnector, also fixed 2) (the source_router issue) from above

[dovahcrow]

Hi @wseaton we made a big refactor in the last 24 hours. I rebased your code to the refactored main branch and solved several conflicts and force pushed. You can do git fetch --all && git reset --hard origin/postgres_ssl to get the latest code. Be sure to save all your uncommitted changes first since this command will overwrite all the differences on your local branch with the remote one.

[wseaton]

@wangxiaoying just tested your latest postgres-native-tls refactor and everything is looking good w/ redshift via the connection strategy I was using above. Let me know if there's anything else I can do to help!

[wangxiaoying]

@wangxiaoying just tested your latest postgres-native-tls refactor and everything is looking good w/ redshift via the connection strategy I was using above. Let me know if there's anything else I can do to help!

@wseaton That's great! I cleaned up the code a little bit and tested with postgres on aws rds. If you think it's ok then we can merge it after review!

[frostpuppet]

Is there any possibility of adding support for the sslkey and sslcert for client auth?
Still somewhat new to rust but wondering how hard an addition like this is
https://www.cockroachlabs.com/docs/stable/build-a-rust-app-with-cockroachdb.html

[wangxiaoying]

Hi @frostpuppet , we currently only support server-side authentication since client auth is less common. Adding support for client auth may require openssl crate (current native-tls is not enough). Actually this initial commit supported it and we remove it later since we want to have less unnecessary dependencies.

May I ask what's your use case scenario for client auth?

[frostpuppet]

Thanks for replying so quickly. My use case really revolves around the way our database is secured. Typically a client will request access and certificates are issued through a Vault. So in my case I only have 3 certs and no password is issued.