fix: ssl_mode=require/verify-ca/verify-full silently allows plaintext - #45
Merged
Merged
Conversation
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.
Version suggestionBased on this PR's title (
This is informational only — no tag or release is created automatically yet. |
This was referenced Aug 19, 2026
aesslinger
added a commit
that referenced
this pull request
Aug 19, 2026
build_tls_connector's own doc comment already said require should force TLS "without certificate validation", but needs_cert_validation only matched verify-ca/verify-full -- require fell through to the final with_platform_verifier() fallback, which does validate against the OS trust store, defeating the entire point of require vs. verify-full (the standard require use case is self-signed certs or private CAs the user hasn't configured ssl_ca for). Ported NoCertVerifier from the builtin driver's src-tauri/src/pool_manager.rs::NoCertVerifier -- accepts any certificate unconditionally, including bypassing TLS 1.2/1.3 signature verification, not just chain/hostname checks (more permissive than VerifyCaCertVerifier's chain-only bypass, but that's the builtin's own deliberate choice for this mode, not something to improve on silently). Routed require mode to it, threading client_auth through the same pattern already used by the verify-ca/verify-full branches so mTLS + require still work together. Proved the bug and the fix live against a real self-signed-cert SSL-enabled PostgreSQL instance (separate from the non-SSL fixture used for #43): require failed the TLS handshake before this fix, and now connects successfully. Confirmed no regression in verify-ca/ verify-full (still correctly validate -- existing VerifyCaCertVerifier unit tests unaffected) or require against a non-SSL server (still correctly fails, per #43/#45). Separately found while testing this fix that verify-ca without an explicit ssl_ca file silently falls back to platform-trust validation instead of erroring like the builtin does -- a distinct, smaller discrepancy, filed as #46 and left out of scope here. Fixes #44.
aesslinger
added a commit
that referenced
this pull request
Aug 19, 2026
Continuation of the beta line after PRs #45/#47/#48 (ssl_mode=require/ verify-ca/verify-full silently allowing plaintext, ssl_mode=require validating against the platform trust store instead of skipping validation, and verify-ca silently falling back to platform trust instead of requiring an explicit CA file). Also corrected three README claims that had gone stale across this session's TLS work: the ssl_mode value list was missing allow/prefer in two places, ssl_ca's description didn't note it's now required (not just optional) for verify-ca, and the pool-caching description still said host:port:database:user, missing startup_script and every TLS param folded in by #37. Verified: .tabularium re-validated clean against the live registry schema; cargo build/test (114/114)/clippy/fmt all pass; markdownlint clean; manual live-TLS smoke tests against real non-SSL and self-signed-cert SSL-enabled PostgreSQL instances covering every ssl_mode value and error path all behave as documented.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
build_poolnever calledcfg.ssl_mode(...)on thedeadpool_postgres::Config. Left asNone, the underlyingtokio_postgres::Configdefaults toSslMode::Prefer(confirmed intokio-postgres-0.7.18/src/config.rs:255) — negotiate TLS if offered, silently accept plaintext otherwise. Sossl_mode=require/verify-ca/verify-fullbuilt the right rustls verifier (that's what #35/#37/#40 fixed) but never actually enforced TLS at the protocol level.This is security-relevant, not just a correctness bug: a user opting into "TLS or nothing" got an unencrypted connection with no error if the server couldn't/wouldn't negotiate TLS.
Fixes #43.
Discovery context
Found during a post-merge sanity pass on
mainafter #35/#37/#40/#41/#42 landed. Confirmed viagit log/git showthat this predates all six recently-merged PRs — present sinceclient.rswas first staged in #3, not a regression from any of them.Fix
Added
resolve_ssl_mode, mapping this plugin'sssl_modestrings todeadpool_postgres::SslMode, matching the builtin driver'sbuild_postgres_configurations(src-tauri/src/pool_manager.rs) mapping exactly:disable→Disableallow/prefer→Prefer(no behavior change — alreadytokio_postgres's default)require/verify-ca/verify-full→Require(the actual fix)Wired into
build_poolright aftercfg.manager = Some(...).Testing — exact local setup and results against a real SSL-enabled server
Automated tests only prove the mapping function is correct and that the bug reproduces against a non-SSL server. To be sure the fix doesn't break real TLS connections, I stood up a second local instance with actual TLS enabled and drove the built binary against it directly, alongside the existing non-SSL fixture.
Setup (both via Podman, run locally, not part of the repo or CI):
postgres:16, no SSL — matches CI'slive-db-integrationfixture exactly.postgres:16with a real self-signed cert generated viaopenssl req -x509 -newkey rsa:2048,ssl = onset inpostgresql.conf, restarted, confirmed viaSHOW ssl;→on.Manual repro against the built binary, piping raw JSON-RPC requests into
./target/debug/postgresql-plugin, run against the fix (current code):ssl_modedisable{"id":1,"jsonrpc":"2.0","result":null}✅Prefer, unchanged){"id":1,"jsonrpc":"2.0","result":null}✅prefer{"id":1,"jsonrpc":"2.0","result":null}✅require{"error":{"code":-32603,"message":"Connection failed: ... error performing TLS handshake"},...}✅require{"error":{"code":-32603,"message":"... error performing TLS handshake"}}❌ — see "Related finding" belowBefore/after on the actual bug (
requireagainst the non-SSL server) — done via the automated live-DB test, not a second manual binary run: temporarily commented out the one new line (cfg.ssl_mode = resolve_ssl_mode(...)) inclient.rs, rebuilt, ran the new test — confirmedssl_mode_require_fails_against_a_server_without_tlsfails against that reverted code (connects over plaintext, matching the original bug). Restored the line, rebuilt, confirmed the test passes.Automated tests:
resolve_ssl_mode_*inclient_tests.rs): every mapping case, written first and confirmed failing to compile before the function existed.ssl_mode_require_fails_against_a_server_without_tlsintests/live_db.rs): runs against CI's realpostgres:16fixture (no SSL). Confirmed Red before the fix, Green after (see above).Related finding, filed separately (explains the one row above that didn't pass)
Row 5 above (
requireagainst the real SSL-enabled server) failed with a TLS handshake error — expected to succeed, sincerequireshould accept any cert without validating it. Root-caused:build_tls_connector'sneeds_cert_validationcheck only matchesverify-ca/verify-full, sorequirefalls through to the finalwith_platform_verifier()branch, which does validate against the OS trust store — contradicting the function's own doc comment ("requireforces TLS without certificate validation") and the builtin's actual behavior (NoCertVerifier, no validation at all forrequire). Confirmed viagit show 9986b1ethat this predates every PR from this session, including this one — a second, distinct, pre-existing bug (different root cause: cert validation vs. protocol-level enforcement) found only because I tested this fix against a real TLS server rather than stopping at the non-SSL repro. Filed as #44, intentionally out of scope here.Test plan
cargo test --lib --bins— 111/111 pass (107 previous + 4 new)cargo test --test live_db(against a live non-SSL PostgreSQL 16 instance) — 8/8 pass, including the new regression test, verified Red-then-Green against a temporarily revertedclient.rscargo clippy --all-targets -- -D warnings— cleancargo fmt --all -- --check— cleancargo build --release— cleannpx markdownlint CHANGELOG.md— clean