From 5c17f8fcd2be1bceb70c004fd6236422bda3f580 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Mon, 12 Oct 2026 18:46:12 +0000 Subject: [PATCH] Resolve libpq environment defaults in one place The `PG*` fallbacks for the DSN lived in `mod.rs` and the `PGSSL*` ones in `tls.rs`, each with its own env helper. Put both behind one `resolve_connection_defaults`, which the pool calls once, so the two cannot drift apart and `tls::build` stays a pure function of its params. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01ULdieJyQoHNzeHq42H2M1p --- rust/src/database/postgres/mod.rs | 51 ++++++++++++++++++++++++++++++ rust/src/database/postgres/pool.rs | 8 ++--- rust/src/database/postgres/tls.rs | 43 +------------------------ 3 files changed, 54 insertions(+), 48 deletions(-) diff --git a/rust/src/database/postgres/mod.rs b/rust/src/database/postgres/mod.rs index 925703ece5..b98f95c9a7 100644 --- a/rust/src/database/postgres/mod.rs +++ b/rust/src/database/postgres/mod.rs @@ -11,6 +11,8 @@ use anyhow::{Context, Error}; use pyo3::prelude::*; use pyo3::types::PyModule; +use crate::database::postgres::tls::TlsParams; + mod connection; mod cursor_state; mod errors; @@ -105,6 +107,55 @@ pub(crate) fn fixup_config_defaults(dsn: &str) -> Result Result<(tokio_postgres::Config, TlsParams), Error> { + Ok((fixup_config_defaults(dsn)?, tls_env_defaults(tls_params))) +} + +/// Fill unset [`TlsParams`] fields from libpq's `PGSSL*` environment +/// variables, as libpq does for keywords the connection string omits. +/// +/// Called at pool/connection construction (see `ConnectionManager::from_dsn`, +/// next to the other `PG*` env handling in `fixup_config_defaults`) rather +/// than inside [`build`], which stays a pure function of its params. +/// +/// Deliberately more conservative than libpq where a stray variable could +/// break a working setup: +/// - a set-but-empty variable is treated as unset, as libpq does; +/// - `PGSSLCERT`/`PGSSLKEY` are imported only as a complete pair, and only +/// when neither is configured explicitly. libpq resolves a lone sslcert's +/// key lazily (`~/.postgresql/postgresql.key`) and only fails if the +/// server actually requests a client certificate; we build the TLS config +/// eagerly, so a leftover `PGSSLCERT` from some other tool must not abort +/// every connection. +fn tls_env_defaults(params: &TlsParams) -> TlsParams { + fn fallback(value: &Option, var: &str) -> Option { + value.clone().or_else(|| env_nonempty(var)) + } + + let (sslcert, sslkey) = match (¶ms.sslcert, ¶ms.sslkey) { + (None, None) => match (env_nonempty("PGSSLCERT"), env_nonempty("PGSSLKEY")) { + (Some(cert), Some(key)) => (Some(cert), Some(key)), + _ => (None, None), + }, + (cert, key) => (cert.clone(), key.clone()), + }; + + TlsParams { + sslmode: fallback(¶ms.sslmode, "PGSSLMODE"), + sslrootcert: fallback(¶ms.sslrootcert, "PGSSLROOTCERT"), + sslcert, + sslkey, + sslpassword: fallback(¶ms.sslpassword, "PGSSLPASSWORD"), + } +} + fn env_nonempty(var: &str) -> Option { std::env::var(var).ok().filter(|value| !value.is_empty()) } diff --git a/rust/src/database/postgres/pool.rs b/rust/src/database/postgres/pool.rs index f13d8cd0c7..d2906c126f 100644 --- a/rust/src/database/postgres/pool.rs +++ b/rust/src/database/postgres/pool.rs @@ -32,8 +32,8 @@ use tokio_postgres::{Client, Config, Connection as PgConnection, NoTls, Socket, use crate::database::postgres::connection::Connection; use crate::database::postgres::errors::{pg_err_to_py, OperationalError}; -use crate::database::postgres::fixup_config_defaults; use crate::database::postgres::helpers::BlockingPostgres; +use crate::database::postgres::resolve_connection_defaults; use crate::database::postgres::tls::{self, TlsConnector, TlsParams}; use crate::runtime::RustRuntime; @@ -123,11 +123,7 @@ impl ConnectionManager { session: SessionConfig, tls_params: &TlsParams, ) -> Result { - let mut config = fixup_config_defaults(dsn)?; - // Resolve the `PGSSL*` env fallbacks here, next to the other `PG*` - // env handling in `fixup_config_defaults`, keeping `tls::build` (and - // its unit tests) a pure function of its params. - let tls_params = tls::with_env_defaults(tls_params); + let (mut config, tls_params) = resolve_connection_defaults(dsn, tls_params)?; let (ssl_mode, connector) = tls::build(&tls_params)?; config.ssl_mode(ssl_mode); Ok(Self { diff --git a/rust/src/database/postgres/tls.rs b/rust/src/database/postgres/tls.rs index ea3db87e09..5cd57a92a8 100644 --- a/rust/src/database/postgres/tls.rs +++ b/rust/src/database/postgres/tls.rs @@ -83,51 +83,10 @@ enum Verification { Full, } -/// Fill unset [`TlsParams`] fields from libpq's `PGSSL*` environment -/// variables, as libpq does for keywords the connection string omits. -/// -/// Called at pool/connection construction (see `ConnectionManager::from_dsn`, -/// next to the other `PG*` env handling in `fixup_config_defaults`) rather -/// than inside [`build`], which stays a pure function of its params. -/// -/// Deliberately more conservative than libpq where a stray variable could -/// break a working setup: -/// - a set-but-empty variable is treated as unset, as libpq does; -/// - `PGSSLCERT`/`PGSSLKEY` are imported only as a complete pair, and only -/// when neither is configured explicitly. libpq resolves a lone sslcert's -/// key lazily (`~/.postgresql/postgresql.key`) and only fails if the -/// server actually requests a client certificate; we build the TLS config -/// eagerly, so a leftover `PGSSLCERT` from some other tool must not abort -/// every connection. -pub(crate) fn with_env_defaults(params: &TlsParams) -> TlsParams { - fn env_nonempty(var: &str) -> Option { - std::env::var(var).ok().filter(|value| !value.is_empty()) - } - fn fallback(value: &Option, var: &str) -> Option { - value.clone().or_else(|| env_nonempty(var)) - } - - let (sslcert, sslkey) = match (¶ms.sslcert, ¶ms.sslkey) { - (None, None) => match (env_nonempty("PGSSLCERT"), env_nonempty("PGSSLKEY")) { - (Some(cert), Some(key)) => (Some(cert), Some(key)), - _ => (None, None), - }, - (cert, key) => (cert.clone(), key.clone()), - }; - - TlsParams { - sslmode: fallback(¶ms.sslmode, "PGSSLMODE"), - sslrootcert: fallback(¶ms.sslrootcert, "PGSSLROOTCERT"), - sslcert, - sslkey, - sslpassword: fallback(¶ms.sslpassword, "PGSSLPASSWORD"), - } -} - /// Resolve [`TlsParams`] into the `tokio-postgres` [`SslMode`] to set on the /// connection config and the [`TlsConnector`] to connect with. A pure /// function of its params: the `PGSSL*` env fallbacks are applied by the -/// caller (see [`with_env_defaults`]). +/// caller (see [`super::resolve_connection_defaults`]). pub fn build(params: &TlsParams) -> anyhow::Result<(SslMode, TlsConnector)> { let mode = params.sslmode.as_deref().unwrap_or("prefer"); let (ssl_mode, verification) = match mode {