fix(postgres): honor ssl_mode in build_connection_url#378
fix(postgres): honor ssl_mode in build_connection_url#378darkrideroffate wants to merge 1 commit into
Conversation
The sqlx-based connection path (DatabaseDriver::test_connection -> build_connection_url -> AnyConnection::connect) omitted the sslmode parameter, so sqlx defaulted to prefer and attempted TLS even when the user selected Disable. This diverged from the pool path (build_postgres_configurations), which honors ssl_mode, causing Load Databases to succeed while opening the connection failed against servers that reject the negotiated TLS (e.g. CloudNativePG forcing TLS 1.3): 'bad protocol version'. Map ssl_mode onto the libpq sslmode URL parameter, mirroring the MySQL driver, and add tests covering the disable case and the unset default.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Reviewed by kimi-k2.6-20260420 · Input: 213.9K · Output: 12.2K · Cached: 880.8K |
NewtTheWolf
left a comment
NewtTheWolf
left a comment
There was a problem hiding this comment.
Good, well-targeted fix. The root cause is correctly identified: build_connection_url never emitted sslmode, so the default test_connection path (driver_trait.rs:250 → AnyConnection::connect) fell back to sqlx's prefer and negotiated TLS even under Disable — diverging from the pool path, which already honored ssl_mode. Mapping the value onto the URL param fixes that, mirrors the MySQL driver, and the disable + default tests lock in the behavior. 👍
One non-blocking gap left inline for verify-ca/verify-full. Nothing blocking — it strictly improves the reported case.
| use urlencoding::encode; | ||
| let user = encode(params.username.as_deref().unwrap_or_default()); | ||
| let pass = encode(params.password.as_deref().unwrap_or_default()); | ||
| let sslmode = match params.ssl_mode.as_deref() { |
There was a problem hiding this comment.
Non-blocking follow-up: verify-ca/verify-full still won't actually work on this path — and now they fail instead of silently skipping verification. The default test_connection (driver_trait.rs:250) connects purely from this URL, which carries no sslrootcert, so params.ssl_ca (the custom CA that RDS / CloudNativePG users supply) is never propagated. Before this PR those modes silently ran as prefer (TLS, no verification); now they emit sslmode=verify-full against the system trust store only and will fail for any server needing a custom CA.
Arguably a safer fail-closed, but it means the verify-* modes stay broken on the sqlx test path, just differently. Options: append &sslrootcert=<ssl_ca> when a CA is configured, or leave it and note the limitation. The tests here cover only disable/default, so this path isn't exercised either way.
(Minor, and correct: the _ => "prefer" fallback matches build_postgres_configurations' default — nice consistency.)
2 participants
PostgreSQL's
build_connection_urlnever setsslmode, so the sqlx path (test_connection→build_connection_url→AnyConnection::connect) defaulted topreferand attempted TLS even when the user picked Disable. The pool path already honorsssl_mode, so Load Databases worked while opening the connection failed against servers that reject the negotiated TLS (e.g. CloudNativePG forcing TLS 1.3):bad protocol version.Now maps
ssl_modeonto thesslmodeURL param, mirroring the MySQL driver, plus tests for thedisableand default cases.