Add client SSL authentication using key-file for Postgres, MySQL and MariaDB

[ThibsG]

Revival of #1166
Fixes #1162

Tests are separated whether client SSL or password is used to authenticate the user.
Database's configuration is changing (to configure database and not provide any password, and thus be more confident in the test),
but if you believe this is too heavy (too long, too many images etc.) let me know, I'll mix the Docker images to use the same,
either we want to connect using SSL or with a password.

[RabidFire]

Thanks for this, @ThibsG! We use YugabyteDB (it's PostgreSQL wire-compatible like CockroachDB) and need to use client certificates for authentication. We've been waiting for this support in order to shift from Diesel to SQLx.

Tagging @abonander since he reviewed the previous PR (#1166).

I'll do some testing on my end and report back. Thanks again!

[ThibsG]

Hi @RabidFire , did you have some time to run some tests?

[EnigmaCurry]

I get Error: Configuration("no keys found pem file") when I use an elliptic curve key type (-----BEGIN EC PRIVATE KEY-----). But maybe this type of key is not supported by rustls? It is tested working with psql.

[abonander]

I'd like to see the workflows running, not sure why they're not.

[ThibsG]

@abonander it seems that if I push more than 1 time per day-ish, it won't run tests automatically (too heavy I suppose), so it needs approval from maintainer (the link I see is this one: https://docs.github.com/actions/managing-workflow-runs/approving-workflow-runs-from-public-forks)

[abonander]

PRs from first-time contributors require approval every time a workflow wants to be run. It's a bit annoying but it seems to be a decent precaution as an attacker could open a PR and modify a workflow to submit some secret environment variables like auth tokens to a server they control.

[ThibsG]

Well thanks that makes perfect sense.

I just updated GA workflow to properly remove the previous container so the port is free to spawn the next container. Tests should run well now.

[fiadliel]

I've been playing with this a bit, and one potential issue I have (for the use case I'm looking at) is that configuration of the client certs is dependent on accept_invalid_certs being false.

It seems to me that client certificate configuration should be independent of server certificate verification? WDYT?

[ThibsG]

Indeed I totally agree, good catch thanks! 😉

[srinivasmohan]

@ThibsG it would also be nice to support combined pem files (cert + key in same file) as that would be rather convenient (and potentially reduce IO/reads in "heavy" envs).

something like.

async fn load_client_certs(client_cert: &CertificateInput) -> Result<(Vec<Certificate>, PrivateKey), Error> {
    let mut cursor = Cursor::new(client_cert.data().await?);

    let mut certs: Vec<Certificate> = vec! [];
    let mut keys: Vec<PrivateKey> = vec! [];

    loop {
        match rustls_pemfile::read_one(&mut cursor).expect("Could not parse cert/private keys from client cert") {
            Some(rustls_pemfile::Item::RSAKey(key)) => keys.push(PrivateKey(key)),
            Some(rustls_pemfile::Item::PKCS8Key(key)) => keys.push(PrivateKey(key)),
            Some(rustls_pemfile::Item::ECKey(key)) => keys.push(PrivateKey(key)),
            Some(rustls_pemfile::Item::X509Certificate(cert)) => certs.push(Certificate(cert)),
            None => break,
            _ => {}
        }
    }

    if keys.len() == 1 {
        Ok((certs, keys.into_iter().next().unwrap()))
    } else {
        Err(Error::Tls(format!("No private key in client cert").into()))
    }
}

[abonander]

@ThibsG current development is on the 0.7-dev branch which just had some major refactors merged. Do you mind rebasing?

[ThibsG]

@abonander done with rebasing 😉

[EnigmaCurry]

   Compiling sqlx-postgres v0.6.2 (https://github.com/ThibsG/sqlx?branch=postgres-native-tls-fixes#7ffc1330)
error[E0063]: missing fields `client_cert_path` and `client_key_path` in initializer of `TlsConfig<'_>`
  --> /home/ryan/.cargo/git/checkouts/sqlx-1c3a6f3a9c600558/7ffc133/sqlx-postgres/src/connection/tls.rs:56:18
   |
56 |     let config = TlsConfig {
   |                  ^^^^^^^^^ missing `client_cert_path` and `client_key_path`

[EnigmaCurry]

I'm testing this with setting PGSSLCERT, PGSSLKEY, and PGSSLROOTCERT environment vars. I'm getting this panic:

thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Tls(NotPkcs8)', src/main.rs:11:10

Heres my code and the full backtrace and how I set my env vars and my throwaway cert+key. I had suspected that my use of Elliptic Curve keys was causing this, so I switched my root CA, server, and client certs all to use RSA instead. AFAIK it did output a valid pkcs8 key file. It connects with psql correctly. I am generating the keys with step-ca in my script here.

In general, this is how I bring up postgres

[ThibsG]

The EC key variant was not handled, this has been fixed, thanks!

Also, I changed the target branch to merge this PR into 0.7-dev, it will be easier to review changes.

[EnigmaCurry]

FWIW, I'm still getting the same error Tls(NotPkcs8) with either EC or RSA keys. There must be something strange about my certs (maybe to do with step-ca vs openssl?), as I don't get this error if I use instead the ones provided in the tests folder.

[ThibsG]

I'm not a cert expert, but your client key seems to be in PKCS#1 format (the header is starting with BEGIN RSA PRIVATE KEY, not BEGIN PRIVATE KEY, in PKCS#8 the type is encoded within the key).

Looking at source code, it seems also that rust-native-tls does not support PKCS#1 keys (TL;DR: no need, the conversion to PKCS#8 is not that hard).

Try to convert using: openssl pkcs8 -topk8 -nocrypt -in cc-client-key -out cc-client-key.pk8.
I will test this with your setup tomorrow.

[EnigmaCurry]

Try to convert using: openssl pkcs8 -topk8 -nocrypt -in cc-client-key -out cc-client-key.pk8. I will test this with your setup tomorrow.

That fixed it! 🎉

I can confirm this is working now. End-to-End mutual TLS ::) Thank you!

[ThibsG]

@EnigmaCurry , you may also want to try runtime-tokio-rustls feature instead of runtime-tokio-native-tls, as Rustls supports PKCS#1 format. It avoids the conversion if you didn't want to use openssl at all.

[EnigmaCurry]

Rustls support PKCS#1 format.

Confirmed! I fixed my script so it outputs keyfiles in both formats, but now its nice to be able to use either one. Thanks again.

[abonander]

Looks like it's green, glad to finally land this!