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
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
87 changes: 87 additions & 0 deletions .github/workflows/sqlx.yml
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,93 @@ jobs:
env:
DATABASE_URL: postgres://postgres:password@localhost:5432/sqlx?sslmode=verify-ca&sslrootcert=.%2Ftests%2Fcerts%2Fca.crt

postgres_native_cert:
name: Postgres Openssl Certificate Authentication
runs-on: ubuntu-20.04
strategy:
matrix:
postgres: [13]
runtime: [async-std-native-tls, tokio-native-tls, actix-native-tls]
needs: check
steps:
- uses: actions/checkout@v2

- uses: actions-rs/toolchain@v1
with:
profile: minimal
toolchain: stable
override: true

- uses: actions/cache@v2
with:
path: |
~/.cargo/registry
~/.cargo/git
target
key: ${{ runner.os }}-postgres-${{ matrix.runtime }}-${{ hashFiles('**/Cargo.lock') }}

- uses: actions-rs/cargo@v1
with:
command: build
args: >
--features postgres,openssl,all-types,runtime-${{ matrix.runtime }}

- run: docker-compose -f tests/docker-compose.yml run -d -p 5432:5432 postgres_${{ matrix.postgres }}_cert
- run: sleep 10

- uses: actions-rs/cargo@v1
with:
command: test
args: >
--no-default-features
--features any,postgres,openssl,macros,migrate,all-types,runtime-${{ matrix.runtime }}
env:
DATABASE_URL: postgres://postgres:password@localhost:5432/sqlx?sslmode=verify-ca&sslrootcert=.%2Ftests%2Fcerts%2Fca.crt&sslcert=.%2Ftests%2Fcerts%2Fclient.crt&sslkey=.%2Ftests%2Fkeys%2Fclient.key


postgres_cert:
name: Postgres Rustls Certificate Authentication
runs-on: ubuntu-20.04
strategy:
matrix:
postgres: [13]
runtime: [async-std-rustls, tokio-rustls, actix-rustls]
needs: check
steps:
- uses: actions/checkout@v2

- uses: actions-rs/toolchain@v1
with:
profile: minimal
toolchain: stable
override: true

- uses: actions/cache@v2
with:
path: |
~/.cargo/registry
~/.cargo/git
target
key: ${{ runner.os }}-postgres-${{ matrix.runtime }}-${{ hashFiles('**/Cargo.lock') }}

- uses: actions-rs/cargo@v1
with:
command: build
args: >
--features postgres,all-types,runtime-${{ matrix.runtime }}

- run: docker-compose -f tests/docker-compose.yml run -d -p 5432:5432 postgres_${{ matrix.postgres }}_cert
- run: sleep 10

- uses: actions-rs/cargo@v1
with:
command: test
args: >
--no-default-features
--features any,postgres,macros,migrate,all-types,runtime-${{ matrix.runtime }}
env:
DATABASE_URL: postgres://postgres:password@localhost:5432/sqlx?sslmode=verify-ca&sslrootcert=.%2Ftests%2Fcerts%2Fca.crt&sslcert=.%2Ftests%2Fcerts%2Fclient.crt&sslkey=.%2Ftests%2Fkeys%2Fclient.key

mysql:
name: MySQL
runs-on: ubuntu-20.04
Expand Down
23 changes: 17 additions & 6 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,9 @@ bit-vec = ["sqlx-core/bit-vec", "sqlx-macros/bit-vec"]
bstr = ["sqlx-core/bstr"]
git2 = ["sqlx-core/git2"]

# openssl
openssl = [ "sqlx-core/openssl-native" ]

[dependencies]
sqlx-core = { version = "0.5.7", path = "sqlx-core", default-features = false }
sqlx-macros = { version = "0.5.7", path = "sqlx-macros", default-features = false, optional = true }
Expand Down
7 changes: 6 additions & 1 deletion sqlx-core/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -93,11 +93,14 @@ _rt-actix = ["tokio-stream"]
_rt-async-std = []
_rt-tokio = ["tokio-stream"]
_tls-native-tls = []
_tls-rustls = ["rustls", "webpki", "webpki-roots"]
_tls-rustls = ["rustls", "rustls-pemfile", "webpki", "webpki-roots"]

# 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.


[dependencies]
ahash = "0.7.2"
atoi = "0.4.0"
Expand Down Expand Up @@ -139,12 +142,14 @@ md-5 = { version = "0.9.0", default-features = false, optional = true }
memchr = { version = "2.3.3", default-features = false }
num-bigint = { version = "0.3.1", default-features = false, optional = true, features = ["std"] }
once_cell = "1.5.2"
openssl = { version = "0.10.35", optional = true }
percent-encoding = "2.1.0"
parking_lot = "0.11.0"
rand = { version = "0.8.3", default-features = false, optional = true, features = ["std", "std_rng"] }
regex = { version = "1.3.9", optional = true }
rsa = { version = "0.4.0", optional = true }
rustls = { version = "0.19.0", features = ["dangerous_configuration"], optional = true }
rustls-pemfile = { version = "0.2.1", optional = true }
serde = { version = "1.0.106", features = ["derive", "rc"], optional = true }
serde_json = { version = "1.0.51", features = ["raw_value"], optional = true }
sha-1 = { version = "0.9.0", default-features = false, optional = true }
Expand Down
2 changes: 2 additions & 0 deletions sqlx-core/src/mysql/connection/tls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,8 @@ async fn upgrade(stream: &mut MySqlStream, options: &MySqlConnectOptions) -> Res
accept_invalid_certs,
accept_invalid_host_names,
options.ssl_ca.as_ref(),
None,
None,
)
.await?;

Expand Down
32 changes: 32 additions & 0 deletions sqlx-core/src/net/tls/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -80,11 +80,15 @@ where
accept_invalid_certs: bool,
accept_invalid_hostnames: bool,
root_cert_path: Option<&CertificateInput>,
client_cert_path: Option<&CertificateInput>,
client_key_path: Option<&CertificateInput>,
) -> Result<(), Error> {
let connector = configure_tls_connector(
accept_invalid_certs,
accept_invalid_hostnames,
root_cert_path,
client_cert_path,
client_key_path,
)
.await?;

Expand Down Expand Up @@ -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.

client_key_path: Option<&CertificateInput>,
) -> Result<sqlx_rt::TlsConnector, Error> {
#[cfg(feature = "openssl-native")]
use openssl::{pkcs12::Pkcs12, pkey::PKey, x509::X509};
#[cfg(feature = "openssl-native")]
use sqlx_rt::native_tls::Identity;
use sqlx_rt::native_tls::{Certificate, TlsConnector};
#[cfg(feature = "openssl-native")]
const PASSWORD: &str = "temp-pkcs12";

let mut builder = TlsConnector::builder();
builder
Expand All @@ -131,6 +143,26 @@ async fn configure_tls_connector(
let cert = Certificate::from_pem(&data)?;

builder.add_root_certificate(cert);

#[cfg(feature = "openssl-native")]
if let (Some(cert), Some(key)) = (client_cert_path, client_key_path) {
let cert_data = cert.data().await?;
let key_data = key.data().await?;
if let (Ok(pkey), Ok(cert)) = (
PKey::private_key_from_pem(&key_data),
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.

{
if let Ok(der) = pkcs.to_der() {
let identity = Identity::from_pkcs12(&der, PASSWORD)?;

builder.identity(identity);
}
}
}
}
}
}

Expand Down
45 changes: 44 additions & 1 deletion sqlx-core/src/net/tls/rustls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use rustls::{
Certificate, ClientConfig, RootCertStore, ServerCertVerified, ServerCertVerifier, TLSError,
WebPKIVerifier,
};
use std::io::Cursor;
use std::io::{BufReader, Cursor};
use std::sync::Arc;
use webpki::DNSNameRef;

Expand All @@ -13,6 +13,8 @@ pub async fn configure_tls_connector(
accept_invalid_certs: bool,
accept_invalid_hostnames: bool,
root_cert_path: Option<&CertificateInput>,
client_cert_path: Option<&CertificateInput>,
client_key_path: Option<&CertificateInput>,
) -> Result<sqlx_rt::TlsConnector, Error> {
let mut config = ClientConfig::new();

Expand All @@ -34,6 +36,21 @@ pub async fn configure_tls_connector(
.map_err(|_| Error::Tls(format!("Invalid certificate {}", ca).into()))?;
}

if let (Some(cert), Some(key)) = (client_cert_path, client_key_path) {
let key_data = key.data().await?;
let cert_data = cert.data().await?;
let certs = to_certs(cert_data);
let key = to_private_key(key_data)?;
match config.set_single_client_cert(certs, key) {
Ok(_) => (),
Err(err) => {
return Err(Error::Configuration(
format!("no keys found in: {:?}", err).into(),
))
}
}
}

if accept_invalid_hostnames {
config
.dangerous()
Expand Down Expand Up @@ -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.

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.

.iter()
.map(|v| rustls::Certificate(v.clone()))
.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()

let cur = Cursor::new(pem);
let mut reader = BufReader::new(cur);

loop {
match rustls_pemfile::read_one(&mut reader)? {
Some(rustls_pemfile::Item::RSAKey(key)) => return Ok(rustls::PrivateKey(key)),
Some(rustls_pemfile::Item::PKCS8Key(key)) => return Ok(rustls::PrivateKey(key)),
None => break,
_ => {}
}
}

Err(Error::Configuration("no keys found pem file".into()))
}
2 changes: 2 additions & 0 deletions sqlx-core/src/postgres/connection/tls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,8 @@ async fn upgrade(stream: &mut PgStream, options: &PgConnectOptions) -> Result<bo
accept_invalid_certs,
accept_invalid_hostnames,
options.ssl_root_cert.as_ref(),
options.ssl_client_cert.as_ref(),
options.ssl_client_key.as_ref(),
)
.await?;

Expand Down
Loading