From fc24f5f6c7dc74808f0419b79bd94db77027bfcf Mon Sep 17 00:00:00 2001 From: Adam J Esslinger Date: Wed, 19 Aug 2026 10:38:28 -0400 Subject: [PATCH] fix: ssl_mode=require/verify-ca/verify-full silently allows plaintext build_pool never called cfg.ssl_mode(...) on the deadpool_postgres Config, so the underlying tokio_postgres::Config kept its own default (SslMode::Prefer: negotiate TLS if offered, but accept plaintext otherwise) regardless of what this plugin's ssl_mode was actually set to. A user opting into "TLS or nothing" got an unencrypted connection with no error if the server couldn't or wouldn't negotiate TLS -- a security-relevant gap, not just a correctness one. Added resolve_ssl_mode, mapping this plugin's ssl_mode strings to deadpool_postgres::SslMode to match the builtin driver's build_postgres_configurations mapping exactly (disable->Disable, allow/prefer->Prefer, require/verify-ca/verify-full->Require), and wired it into build_pool. Proved the bug and the fix with a live-database test against a real non-SSL PostgreSQL instance (matches CI's postgres:16 fixture, which also runs without SSL): confirmed the new test fails against the pre-fix code (ssl_mode=require connects successfully over plaintext) and passes after the fix. Also manually verified disable/prefer/unset modes against both a non-SSL and a self-signed-cert SSL-enabled instance to confirm no regression in the modes this change doesn't touch. Separately found (while testing this fix against the SSL-enabled instance) that ssl_mode=require validates the server cert against the platform trust store instead of skipping validation entirely -- a distinct, pre-existing bug in build_tls_connector unrelated to this one. Filed as #44, left out of scope here. Fixes #43. --- CHANGELOG.md | 15 +++++++++++++++ src/client.rs | 20 +++++++++++++++++++- src/client_tests.rs | 38 +++++++++++++++++++++++++++++++++++++- tests/live_db.rs | 20 ++++++++++++++++++++ 4 files changed, 91 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 81bbd78..2d9203e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -53,6 +53,21 @@ the same 8-byte big-endian i64 wire format `INT8` uses and reuses the existing `i64_to_json` JS-safe-integer stringification, matching the builtin driver's `extract/advanced_types.rs::Money`. +- `ssl_mode=require`/`verify-ca`/`verify-full` silently allowed plaintext + connections — `build_pool` never called `cfg.ssl_mode(...)` on the + `deadpool_postgres::Config`, so the underlying `tokio_postgres::Config` + kept its own default (`SslMode::Prefer`: negotiate TLS if offered, but + accept plaintext otherwise) regardless of what this plugin's `ssl_mode` + was actually set to. A user opting into "TLS or nothing" got an + unencrypted connection with no error if the server couldn't/wouldn't + negotiate TLS — a security-relevant gap, not just a correctness one. + Added `resolve_ssl_mode`, mapping this plugin's `ssl_mode` strings to + `deadpool_postgres::SslMode` to match the builtin driver's + `build_postgres_configurations` mapping exactly (`disable`→`Disable`, + `allow`/`prefer`→`Prefer`, `require`/`verify-ca`/`verify-full`→`Require`), + and wired it into `build_pool`. Proved the bug and the fix with a live + test against a real non-SSL PostgreSQL instance: `ssl_mode=require` + connected successfully before this fix, and now correctly fails. ## [1.0.0-beta.7] - 2026-08-17 diff --git a/src/client.rs b/src/client.rs index d3af4c5..c5d3517 100644 --- a/src/client.rs +++ b/src/client.rs @@ -22,7 +22,7 @@ use std::collections::HashMap; use std::str::FromStr; use std::sync::{LazyLock, Mutex}; -use deadpool_postgres::{Config, ManagerConfig, Pool, RecyclingMethod, Runtime}; +use deadpool_postgres::{Config, ManagerConfig, Pool, RecyclingMethod, Runtime, SslMode}; use tokio_postgres::types::{ToSql, Type}; use tokio_postgres::{NoTls, Row}; use tokio_postgres_rustls::MakeRustlsConnect; @@ -311,6 +311,7 @@ async fn build_pool(params: &ConnectionParams) -> Result { cfg.manager = Some(ManagerConfig { recycling_method: RecyclingMethod::Fast, }); + cfg.ssl_mode = resolve_ssl_mode(params.ssl_mode.as_deref()); let script = params .startup_script @@ -417,6 +418,23 @@ fn needs_tls(params: &ConnectionParams) -> bool { ) } +/// Map this plugin's `ssl_mode` strings to `deadpool_postgres::SslMode`, +/// so `require`/`verify-ca`/`verify-full` actually force TLS at the protocol +/// level instead of leaving `tokio_postgres`'s own default (`SslMode::Prefer`) +/// in effect, which silently accepts a plaintext connection when the server +/// doesn't offer TLS. Matches the builtin driver's `ssl_mode` mapping in +/// `build_postgres_configurations` (`src-tauri/src/pool_manager.rs`) exactly. +/// Certificate/hostname verification is unaffected — that's handled +/// separately by `build_tls_connector`. +fn resolve_ssl_mode(ssl_mode: Option<&str>) -> Option { + match ssl_mode { + Some("disable") => Some(SslMode::Disable), + Some("allow" | "prefer") => Some(SslMode::Prefer), + Some("require" | "verify-ca" | "verify-full") => Some(SslMode::Require), + _ => None, + } +} + /// Build a rustls ClientConfig. `verify-ca`/`verify-full` validate the /// server's certificate chain — against a caller-supplied CA bundle /// (`ssl_ca`) when present, or the platform trust store otherwise. diff --git a/src/client_tests.rs b/src/client_tests.rs index 8b97beb..0955887 100644 --- a/src/client_tests.rs +++ b/src/client_tests.rs @@ -5,9 +5,10 @@ use tokio::sync::Mutex; use super::{ build_tls_connector, cleanup_idle_pools, connection_key, get_or_create_pool, - load_client_cert_from_pem, load_roots_from_pem, VerifyCaCertVerifier, POOLS, + load_client_cert_from_pem, load_roots_from_pem, resolve_ssl_mode, VerifyCaCertVerifier, POOLS, }; use crate::models::ConnectionParams; +use deadpool_postgres::SslMode; // `POOLS` is a single process-wide static, and Rust's test harness runs // `#[tokio::test]` fns concurrently on separate threads — without this, @@ -592,3 +593,38 @@ fn build_tls_connector_verify_ca_attaches_a_configured_client_cert() { "verify-ca must attach the configured client cert, not silently drop it" ); } + +// Coverage for #43: build_pool never called cfg.ssl_mode(...), so +// tokio_postgres's own default (SslMode::Prefer) applied regardless of +// ssl_mode=require/verify-ca/verify-full, letting connections silently +// fall back to plaintext instead of enforcing TLS at the protocol level. +// resolve_ssl_mode maps this plugin's ssl_mode strings to +// tokio_postgres::config::SslMode, matching the builtin driver's +// build_postgres_configurations mapping exactly. + +#[test] +fn resolve_ssl_mode_maps_disable() { + assert_eq!(resolve_ssl_mode(Some("disable")), Some(SslMode::Disable)); +} + +#[test] +fn resolve_ssl_mode_maps_allow_and_prefer_to_prefer() { + assert_eq!(resolve_ssl_mode(Some("allow")), Some(SslMode::Prefer)); + assert_eq!(resolve_ssl_mode(Some("prefer")), Some(SslMode::Prefer)); +} + +#[test] +fn resolve_ssl_mode_maps_require_verify_ca_and_verify_full_to_require() { + assert_eq!(resolve_ssl_mode(Some("require")), Some(SslMode::Require)); + assert_eq!(resolve_ssl_mode(Some("verify-ca")), Some(SslMode::Require)); + assert_eq!( + resolve_ssl_mode(Some("verify-full")), + Some(SslMode::Require) + ); +} + +#[test] +fn resolve_ssl_mode_leaves_unset_or_unknown_values_unmapped() { + assert_eq!(resolve_ssl_mode(None), None); + assert_eq!(resolve_ssl_mode(Some("bogus")), None); +} diff --git a/tests/live_db.rs b/tests/live_db.rs index e084ef2..ba0249b 100644 --- a/tests/live_db.rs +++ b/tests/live_db.rs @@ -311,3 +311,23 @@ fn broken_startup_script_fails_fast_with_clear_attribution() { "error should be clearly attributed to the startup script, got: {error}" ); } + +// Coverage for #43: build_pool never called cfg.ssl_mode(...), so +// tokio_postgres's own default (SslMode::Prefer) applied regardless of the +// plugin's ssl_mode value, letting ssl_mode=require silently connect over +// plaintext instead of failing. CI's live-db-integration fixture runs a +// plain `postgres:16` container with no SSL configured (see +// .github/workflows/ci.yml), so this must fail here just like it would +// against any server that hasn't been configured to offer TLS. +#[test] +fn ssl_mode_require_fails_against_a_server_without_tls() { + let mut plugin = Plugin::spawn(); + let mut params = conn_params(); + params["ssl_mode"] = json!("require"); + + let response = plugin.call("test_connection", json!({ "params": params })); + assert!( + response.get("error").is_some(), + "ssl_mode=require must fail against a server with no TLS, not silently connect over plaintext" + ); +}