diff --git a/.github/workflows/build-test.yml b/.github/workflows/build-test.yml index f19afc3..785a962 100644 --- a/.github/workflows/build-test.yml +++ b/.github/workflows/build-test.yml @@ -944,3 +944,25 @@ jobs: run: make stubs - name: Stubs are current run: git diff --exit-code -- python/aerospike_native + + # Rust unit tests. The test binary links libpython, so this runs on the + # runner host against setup-python's interpreter; a manylinux container has + # no shared libpython to link. + rust-unit: + runs-on: ubuntu-22.04 + steps: + - uses: actions/checkout@v4 + - name: Setup Rust + uses: dtolnay/rust-toolchain@stable + with: + toolchain: 1.90.0 + - name: Setup Python + id: setup-python + uses: actions/setup-python@v6 + with: + python-version: '3.13' + - name: cargo test + env: + # PyO3 otherwise links whichever interpreter it finds first. + PYO3_PYTHON: ${{ steps.setup-python.outputs.python-path }} + run: make test-rust diff --git a/Cargo.lock b/Cargo.lock index 6218eb8..0f61afe 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -76,7 +76,6 @@ dependencies = [ "pyo3-stub-gen", "rand 0.8.6", "rustls", - "rustls-pemfile", "serde", "serde_json", "tokio", @@ -1217,15 +1216,6 @@ dependencies = [ "zeroize", ] -[[package]] -name = "rustls-pemfile" -version = "2.2.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "dce314e5fee3f39953d46bb63bb8a46d40c2f8fb7cc5a3b6cab2bde9721d6e50" -dependencies = [ - "rustls-pki-types", -] - [[package]] name = "rustls-pki-types" version = "1.14.1" @@ -1726,9 +1716,9 @@ dependencies = [ [[package]] name = "webpki-roots" -version = "1.0.8" +version = "1.0.11" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "bf85cb06032201fa7c6f829d7db5a7e5aa45bcc0655327713065f6f0576731bf" +checksum = "f5f280bbb2b287bc8e7de7e3852945a0ff50d1bbcb5ed42b6c7a568c546df171" dependencies = [ "rustls-pki-types", ] diff --git a/Cargo.toml b/Cargo.toml index cb0ad13..a1899b4 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -32,7 +32,6 @@ futures = "0.3" # at handshake unless a process-wide CryptoProvider is installed first. tokio-rustls = { version = "0.26", default-features = false, features = ["ring", "logging", "tls12"], optional = true } rustls = { version = "0.23", default-features = false, features = ["ring", "logging", "std", "tls12"], optional = true } -rustls-pemfile = { version = "2", optional = true } webpki-roots = { version = "1.0", optional = true } # v3 (through 3.3.2) segfaults once the thread that first imported the module # exits, and on macOS shares a fixed TLS slot with any other bundled v3. @@ -47,7 +46,7 @@ pyo3-build-config = "0.29" [features] default = ["extension-module", "tls"] extension-module = ["pyo3/extension-module"] -tls = ["aerospike-core/tls", "tokio-rustls", "rustls", "rustls-pemfile", "webpki-roots"] +tls = ["aerospike-core/tls", "tokio-rustls", "rustls", "webpki-roots"] [[bin]] name = "stub_gen" diff --git a/Makefile b/Makefile index 5d93585..8dec257 100644 --- a/Makefile +++ b/Makefile @@ -6,7 +6,7 @@ user ?= "" pass ?= "" ns ?= "test" -.PHONY: build test test-unit test-int install clean stubs local-cargo git-cargo lint +.PHONY: build test test-unit test-int test-rust install clean stubs local-cargo git-cargo lint all: lint dev build test install clean local-cargo: @@ -74,6 +74,11 @@ test-int: @python/clean_caches.sh || true source aerospike.env 2>/dev/null || source aerospike.env.example && python -m pytest python/tests/integration +test-rust: + # extension-module leaves Python symbols unresolved, so a test binary + # built with the default features cannot link; this links libpython. + cargo test --locked --no-default-features --features tls --lib + dev-test: dev stubs test build: diff --git a/python/aerospike_native/_native.pyi b/python/aerospike_native/_native.pyi index 8728898..a7306b2 100644 --- a/python/aerospike_native/_native.pyi +++ b/python/aerospike_native/_native.pyi @@ -6575,9 +6575,11 @@ class TlsConfig: Create a TlsConfig. Args: - cafile: Path to a CA certificate file (PEM). Optional -- when - omitted the server is verified against the system trust - store alone, which is what a ``tls_name``-only setup needs. + cafile: Path to a CA certificate file (PEM). When given, only + the CAs in this file are trusted. Optional -- when omitted + the server is verified against the public roots compiled + into the client (not the OS trust store), which is what a + ``tls_name``-only setup needs. protocols: Allowed TLS versions, e.g. ``["TLSv1.3"]``. Omit for the rustls default (TLS 1.2 and 1.3). ciphers: Allowed cipher suites by rustls name, e.g. @@ -6602,7 +6604,8 @@ class TlsConfig: Args: cafile: Path to a CA certificate file (PEM), or ``None`` to - verify against the system trust store alone. + verify against the public roots compiled into the client; + see :meth:`TlsConfig`. certfile: Path to the client certificate file (PEM). keyfile: Path to the client private key file (PEM, PKCS#8). protocols: Allowed TLS versions; see :meth:`TlsConfig`. diff --git a/python/tests/integration/tls_test.py b/python/tests/integration/tls_test.py index fe5b2ef..a9460ea 100644 --- a/python/tests/integration/tls_test.py +++ b/python/tests/integration/tls_test.py @@ -235,12 +235,12 @@ class TestTlsConfigOptions: caller restricting protocols or ciphers and getting neither, with no error. """ - def test_system_trust_store_needs_no_ca_file(self): + def test_bundled_roots_need_no_ca_file(self): """No CA file is a supported configuration, not "no TLS". - A `tls_name`-only setup verifies against the system trust store. This - used to be unreachable: the builder returned None without a CA file, - which silently produced a plaintext connection. + A `tls_name`-only setup verifies against the public roots compiled + into the client. A builder that returned None here would silently + produce a plaintext connection. """ assert TlsConfig() is not None diff --git a/src/tls.rs b/src/tls.rs index 018a42f..0dd788b 100644 --- a/src/tls.rs +++ b/src/tls.rs @@ -17,6 +17,10 @@ use pyo3::prelude::*; use pyo3_stub_gen::derive::{gen_stub_pyclass, gen_stub_pymethods}; use crate::IoError; +#[cfg(feature = "tls")] +use rustls::pki_types::pem::{self, PemObject}; +#[cfg(feature = "tls")] +use rustls::pki_types::{CertificateDer, PrivatePkcs8KeyDer}; //////////////////////////////////////////////////////////////////////////////////////////// // @@ -47,44 +51,75 @@ use crate::IoError; // ---------------------------------------------------------------------- // Shared construction // - // Both constructors used to build their own root store and config inline, - // which is why the surface stayed at "CA file, optionally with client - // auth": every new option meant another copy. These helpers hold the parts - // that do not vary so protocol and cipher selection can be added once. + // The parts both constructors share, so each TLS option is implemented + // once rather than per constructor. // ---------------------------------------------------------------------- - /// Build the trust anchors: the platform's webpki roots, plus *cafile* when - /// one is given. Passing no CA file is a supported configuration -- it - /// verifies the server against the system trust store alone, which is what - /// a `tls_name`-only setup needs. + /// Build the trust anchors. + /// + /// With *cafile*, only the certificates in that file are trusted: a + /// cluster signed by a private CA must not also accept a certificate from + /// every public one. Without it, the server is verified against the public + /// roots compiled into the client, which is what a `tls_name`-only setup + /// needs. Those roots are fixed at build time; OS certificate updates do + /// not reach them. #[cfg(feature = "tls")] fn build_root_store(cafile: Option<&str>) -> PyResult { - use std::fs::File; - use std::io::BufReader; - - let mut root_store = rustls::RootCertStore { - roots: webpki_roots::TLS_SERVER_ROOTS.into(), + let Some(cafile) = cafile else { + return Ok(rustls::RootCertStore { + roots: webpki_roots::TLS_SERVER_ROOTS.into(), + }); }; - if let Some(cafile) = cafile { - let ca_file = File::open(cafile).map_err(|e| { - PyErr::new::(format!("Cannot open CA file {}: {}", cafile, e)) - })?; - let mut ca_reader = BufReader::new(ca_file); - let certs: Result, _> = rustls_pemfile::certs(&mut ca_reader).collect(); - let certs = certs.map_err(|e| { - PyErr::new::(format!("Cannot parse CA file {}: {}", cafile, e)) + let mut root_store = rustls::RootCertStore::empty(); + for cert in load_pem_certs(cafile, "CA file")? { + root_store.add(cert).map_err(|e| { + PyErr::new::(format!("Cannot add CA certificate: {}", e)) })?; - for cert in certs { - root_store.add(cert).map_err(|e| { - PyErr::new::(format!("Cannot add CA certificate: {}", e)) - })?; - } + } + // An empty store fails every handshake with an unknown-issuer error + // that never mentions the file, so refuse it here instead. + if root_store.is_empty() { + return Err(PyErr::new::(format!( + "No CA certificates found in {}", + cafile + ))); } Ok(root_store) } + /// Read every certificate in the PEM file at *path*; *what* names the file + /// in error messages. + #[cfg(feature = "tls")] + fn load_pem_certs(path: &str, what: &str) -> PyResult>> { + CertificateDer::pem_file_iter(path) + .map_err(|e| pem_error(e, path, what))? + .collect::, _>>() + .map_err(|e| pem_error(e, path, what)) + } + + /// Read the first PKCS#8 private key in the PEM file at *path*. + #[cfg(feature = "tls")] + fn load_pem_pkcs8_key(path: &str) -> PyResult> { + PrivatePkcs8KeyDer::from_pem_file(path).map_err(|e| match e { + pem::Error::NoItemsFound => { + PyErr::new::(format!("No private key found in {}", path)) + } + e => pem_error(e, path, "key file"), + }) + } + + /// Convert a PEM read failure on the file at *path* into an `IoError`. + #[cfg(feature = "tls")] + fn pem_error(e: pem::Error, path: &str, what: &str) -> PyErr { + let message = match e { + pem::Error::Io(e) => format!("Cannot open {} {}: {}", what, path, e), + e => format!("Cannot parse {} {}: {}", what, path, e), + }; + PyErr::new::(message) + } + /// Resolve protocol-version names to rustls versions. /// /// Unknown names are rejected rather than ignored: silently dropping a @@ -207,9 +242,11 @@ use crate::IoError; /// Create a TlsConfig. /// /// Args: - /// cafile: Path to a CA certificate file (PEM). Optional -- when - /// omitted the server is verified against the system trust - /// store alone, which is what a ``tls_name``-only setup needs. + /// cafile: Path to a CA certificate file (PEM). When given, only + /// the CAs in this file are trusted. Optional -- when omitted + /// the server is verified against the public roots compiled + /// into the client (not the OS trust store), which is what a + /// ``tls_name``-only setup needs. /// protocols: Allowed TLS versions, e.g. ``["TLSv1.3"]``. Omit for /// the rustls default (TLS 1.2 and 1.3). /// ciphers: Allowed cipher suites by rustls name, e.g. @@ -259,7 +296,8 @@ use crate::IoError; /// /// Args: /// cafile: Path to a CA certificate file (PEM), or ``None`` to - /// verify against the system trust store alone. + /// verify against the public roots compiled into the client; + /// see :meth:`TlsConfig`. /// certfile: Path to the client certificate file (PEM). /// keyfile: Path to the client private key file (PEM, PKCS#8). /// protocols: Allowed TLS versions; see :meth:`TlsConfig`. @@ -279,45 +317,56 @@ use crate::IoError; ciphers: Option>, for_login_only: bool, ) -> PyResult { - use rustls::pki_types::{CertificateDer, PrivateKeyDer}; - use std::fs::File; - use std::io::BufReader; - let root_store = build_root_store(cafile.as_deref())?; - - let client_cert_file = File::open(&certfile).map_err(|e| { - PyErr::new::(format!( - "Cannot open client cert file {}: {}", certfile, e - )) - })?; - let mut client_cert_reader = BufReader::new(client_cert_file); - let client_certs: Result, _> = - rustls_pemfile::certs(&mut client_cert_reader).collect(); - let client_certs = client_certs.map_err(|e| { - PyErr::new::(format!( - "Cannot parse client cert file {}: {}", certfile, e - )) - })?; - - let key_file = File::open(&keyfile).map_err(|e| { - PyErr::new::(format!("Cannot open key file {}: {}", keyfile, e)) - })?; - let mut key_reader = BufReader::new(key_file); - let keys: Result, _> = - rustls_pemfile::pkcs8_private_keys(&mut key_reader).collect(); - let mut keys = keys.map_err(|e| { - PyErr::new::(format!("Cannot parse key file {}: {}", keyfile, e)) - })?; - let client_key = keys.pop().ok_or_else(|| { - PyErr::new::(format!("No private key found in {}", keyfile)) - })?; + let client_certs = load_pem_certs(&certfile, "client cert file")?; + let client_key = load_pem_pkcs8_key(&keyfile)?; let config = build_client_config( root_store, protocols.as_deref(), ciphers.as_deref(), - Some((client_certs, PrivateKeyDer::Pkcs8(client_key))), + Some((client_certs, client_key.into())), )?; Ok(TlsConfig { _as: std::sync::Arc::new(config), for_login_only }) } } + + // These never unwrap an `Err` or format a `PyErr`: its `Debug` needs a + // live interpreter, which this test binary does not start. + #[cfg(all(test, feature = "tls"))] + mod tests { + use super::{build_root_store, load_pem_pkcs8_key}; + + const TEST_CA: &str = concat!(env!("CARGO_MANIFEST_DIR"), "/testdata/test-ca.pem"); + + #[test] + fn cafile_is_the_only_trust_anchor() { + let Ok(store) = build_root_store(Some(TEST_CA)) else { + panic!("test CA must load"); + }; + assert_eq!(store.len(), 1); + } + + #[test] + fn bundled_roots_load_without_cafile() { + let Ok(store) = build_root_store(None) else { + panic!("bundled roots must load"); + }; + assert!(!store.is_empty()); + } + + #[test] + fn cafile_without_certificates_is_rejected() { + let path = std::env::temp_dir() + .join(format!("pnc-empty-ca-{}.pem", std::process::id())); + std::fs::write(&path, "").expect("write empty CA file"); + let result = build_root_store(path.to_str()); + let _ = std::fs::remove_file(&path); + assert!(result.is_err()); + } + + #[test] + fn key_file_without_a_key_is_rejected() { + assert!(load_pem_pkcs8_key(TEST_CA).is_err()); + } + } diff --git a/testdata/test-ca.pem b/testdata/test-ca.pem new file mode 100644 index 0000000..36babe2 --- /dev/null +++ b/testdata/test-ca.pem @@ -0,0 +1,10 @@ +-----BEGIN CERTIFICATE----- +MIIBcjCCARigAwIBAgIUNbjKJVSSWHfrHrKd0z2/Sjxf0nIwCgYIKoZIzj0EAwIw +FjEUMBIGA1UEAwwLUE5DIHRlc3QgQ0EwIBcNMjYxMDI3MTMyNzM5WhgPMjEyNjEw +MDMxMzI3MzlaMBYxFDASBgNVBAMMC1BOQyB0ZXN0IENBMFkwEwYHKoZIzj0CAQYI +KoZIzj0DAQcDQgAE9+bwp8zyGRpvn9N//7h8wq73Pp7r6VptWVE5Rh19CFU53wTA +iVXKsYG8XPzSZg66y2uuxrOAnpCzedqxgA2Cu6NCMEAwHQYDVR0OBBYEFBITGpta +kfE2/v0MtdxOXKcb1/7LMA8GA1UdEwEB/wQFMAMBAf8wDgYDVR0PAQH/BAQDAgEG +MAoGCCqGSM49BAMCA0gAMEUCIQDRPC4v5TAApDg2OF0nzDWsIhYKS0xxsz2zAE3E +Ir1bHwIgGGApsTNYtc3a8puiZMXQ5d2usdg8I2vntdGx+m7dBbk= +-----END CERTIFICATE-----