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

Added support for postgres certificate authentication for native tls #1166

Closed

Conversation

linuxuser586
Copy link
Contributor

@linuxuser586 linuxuser586 commented Apr 10, 2021

Fixes #1162 Adds support for postgres certificate authentication for native tls with openssl or rustls.

This allows using the uri format postgres://postgres:password@localhost:5432/sqlx?sslmode=verify-full&sslrootcert=./ca.crt&sslcert=./client.crt&sslkey=./client.key or the following environment variables

  • PGSSLROOTCERT
  • PGSSLCERT
  • PGSSLKEY

This can be used for better security by disabling password login. It using the same patters used with psql.

Example

# pg_hba.conf
# omit host to prevent password login
hostssl all all all cert

@mehcode
Copy link
Member

mehcode commented Apr 27, 2021

I'm thinking it might be best to allow a native_tls::TlsConnector or rustls::Config to be be passed to the connection options directly to allow anything.

@linuxuser586
Copy link
Contributor Author

@mehcode I'm not sure if rusttls supports certificate authentication in the current implementation. I was trying to make the connection string match the way psql works. I had to use some openssl functions to handle the pkcs12 format. One way to get around this would be to do the pkcs12 conversion and specify that. I did that in my first test and it worked so seems possible for rustls. I will have to investigate more to see how to make it works with both tls supported options.

@linuxuser586
Copy link
Contributor Author

@mehcode I finally found time to add support for certificate authentication using rustls. I updated the github workflows to test native tls using openssl, which is required for pkcs12 to identity transformation. I added a separate matrix for rustls without openssl since it does not need to use the pkcs12 format. I made openssl optional with native tls since the openssl library is only needed to support creating the identity from pkcs12 when using certificate authentication.

@abonander
Copy link
Collaborator

@linuxuser586 sorry for the delay.

We had some recent changes to our Github Actions matrices that need to be rebased into this PR. Do you mind taking care of that?

@linuxuser586
Copy link
Contributor Author

@abonander Done

Copy link
Collaborator

@abonander abonander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was giving this a more in-depth review before merging it and came across a number of issues.

let cur = Cursor::new(pem);
let mut reader = BufReader::new(cur);
rustls_pemfile::certs(&mut reader)
.unwrap()
Copy link
Collaborator

@abonander abonander Sep 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docs for rustls_pemfile::certs() is a bit misleading; it does propagate I/O errors but it will also return I/O errors if there's any syntax errors in the data. We probably shouldn't panic here in that case.

@@ -77,3 +94,29 @@ impl ServerCertVerifier for NoHostnameTlsVerifier {
}
}
}

fn to_certs(pem: Vec<u8>) -> Vec<rustls::Certificate> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This name isn't really idiomatic as it doesn't take &self.

I think certs_from_pem() would be better.

.collect()
}

fn to_private_key(pem: Vec<u8>) -> Result<rustls::PrivateKey, Error> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, private_key_from_pem()

X509::from_pem(&cert_data),
) {
if let Ok(pkcs) =
Pkcs12::builder().build(PASSWORD, PASSWORD, pkey.as_ref(), cert.as_ref())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a fan of this hack, though I do understand that the Identity API doesn't really give you any other option.

I would like to see this PR merged in native-tls but it's been stale for over a year now so I dunno what we wanna do here: sfackler/rust-native-tls#147

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If nothing else I would like to see comments added to this code explaining exactly why it's doing this and what it needs to be better.

I also don't really like how the errors are discarded here.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly I would probably consider requiring the openssl crate for this, even as an optional feature, to be a non-starter. I would prefer to wait for the above PR to be merged, though it's not clear when that's going to happen.

@@ -117,8 +121,16 @@ async fn configure_tls_connector(
accept_invalid_certs: bool,
accept_invalid_hostnames: bool,
root_cert_path: Option<&CertificateInput>,
client_cert_path: Option<&CertificateInput>,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we fix these warnings?

If the native-tls feature is enabled but not openssl-native, we should check if these are not None and if so, log a warning that these do nothing without the openssl-native feature.


# support offline/decoupled building (enables serialization of `Describe`)
offline = ["serde", "either/serde"]

# openssl
openssl-native = [ "openssl" ]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really see the reasoning behind giving this a different feature name. Why not just openssl like in the sqlx crate?

If you tried to define an openssl feature but got an error, it's because you don't need to explicitly define features for optional = true dependencies.

@abonander
Copy link
Collaborator

Keep an eye on sfackler/rust-native-tls#209, that looks promising.

@emk
Copy link

emk commented Dec 28, 2021

Hello! Thank you for working on client certificates!

I'm the maintainer of https://github.com/emk/rust-musl-builder, a moderately popular tool for statically-linking Rust apps that use OpenSSL and libpq against libmusl. I'm trying to migrate away from that approach and encourage people to migrate to rustls, which is much easier to cross-compile and statically link.

Here is the code that I'm using for "pure Rust" TLS support, including some (as yet untested) client certificate support: https://github.com/dbcrossbar/dbcrossbar/pull/188/files. This uses rustls and rustls-pemfile to set up a single client certificate in a fashion much like that of libpq.

I would be happy to contribute any part of this code to help with this PR, or to prepare a draft PR based on this approach.

Google's hosted PostgreSQL servers actually require client certs to connect using TLS. So this is a potentially very useful feature.

@abonander
Copy link
Collaborator

Google's hosted PostgreSQL servers actually require client certs to connect using TLS. So this is a potentially very useful feature.

Which ones? Because we use PostgreSQL servers hosted by Google Cloud and it works fine without client certs. It just requires TLS in general.

@larksoftware
Copy link

Google's hosted PostgreSQL servers actually require client certs to connect using TLS. So this is a potentially very useful feature.

As a new user of GCP I was under this impression as well. But after trying to configure sqlx with TLS and failing I did some more reading and found there is another method:

https://cloud.google.com/sql/docs/mysql/connect-admin-proxy

Using the Cloud SQL Auth proxy is the recommended method for connecting to a Cloud SQL instance. The Cloud SQL Auth proxy:

Works with both public and private IP endpoints
Validates connections using credentials for a user or service account
Wraps the connection in a SSL/TLS layer that's authorized for a Cloud SQL instance

So using this auth proxy seems to be the recommended solution for connecting. I'm going to give this a try now and hope I can then use sqlx with my GCP postgres db.

@linuxuser586
Copy link
Contributor Author

@abonander @larksoftware I experienced this very issue and is why I created the https://github.com/linuxuser586/sqlx/tree/postgres-native-tls-fixes fork. Unfortunately, my changes were never merged into the official version and I don't have the time to address the concerns to make it in to the official version; however, the forked version does meet my needs and may meet yours. It is easy to add the git version in rust and so for me, not using the official version is not an issue. I hope this helps you solve your problem.

@abonander
Copy link
Collaborator

I don't have the time to address the concerns to make it in to the official version

I guess we can close this PR then. My review comments still stand, as I would rather see proper support in native-tls than the round-tripping through OpenSSL here.

Note to anyone using this fork, it doesn't properly bubble up errors if there's something wrong with the certificate, it'll just panic: #1166 (comment)

@abonander abonander closed this Jan 28, 2022
@abonander
Copy link
Collaborator

Code quality aside, the solution for rustls was acceptable so if anyone wants to open a PR based on this one with just the rustls changes and address the nits, we'd gladly merge it.

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.

Postgres native tls does not support client certificates.
5 participants