fix: fold TLS params into pool cache key - #37
Merged
Conversation
aesslinger
added
the
prerelease:beta
Version suggestionBased on this PR's title (
This is informational only — no tag or release is created automatically yet. |
connection_key matched only on host:port:database:user:startup_script, ignoring ssl_mode/ssl_ca/ssl_cert/ssl_key entirely. So two connections to the same target differing only in TLS configuration -- require vs. verify-full, or two different client certs -- could incorrectly share a cached pool and its already-negotiated TLS setup. This wasn't inherited from the builtin driver: the builtin's build_connection_key already keyed on ssl_mode/ssl_ca before this plugin's client.rs was even staged (2026-08-11, over two months after tabularis#278 added that keying upstream on 2026-06-04). So this was a parity miss during extraction, not a later upstream change -- surfaced while reviewing #34/#35's client-cert fix, which touches the same TLS config but not the cache key. Folded all four TLS params into connection_key, matching the builtin's TLS-param keying. Updated CLAUDE.md's description of the pool cache key to match. Fixes #36.
aesslinger
force-pushed
the
fix/pool-key-tls-params
branch
from
August 19, 2026 13:27
3a35631 to
560cdbb
Compare
aesslinger
added a commit
that referenced
this pull request
Aug 19, 2026
build_tls_connector wrapped rustls::client::WebPkiServerVerifier for verify-ca, whose verify_server_cert unconditionally checks the hostname via verify_server_name with no way to opt out -- making verify-ca behave identically to verify-full. verify-ca is supposed to validate the certificate chain but skip hostname verification -- that's the entire distinction from verify-full, matching libpq sslmode=verify-ca semantics. Ported VerifyCaCertVerifier from the builtin driver's src-tauri/src/pool_manager.rs, which deliberately avoids WebPkiServerVerifier for this exact reason and instead calls rustls::client::verify_server_cert_signed_by_trust_anchor directly (never reads server_name at all). Proved the bug and the fix with a chain-valid cert whose hostname deliberately doesn't match the connection target -- rejected before this fix, accepted after -- while confirming a CA-untrusted cert is still correctly rejected, and verify-full still correctly rejects the same hostname mismatch. Rebasing onto main (after #35 and #37 merged) surfaced a real merge hazard: naively combining #38's verify-ca branch with #35's client-auth logic would have dropped client_auth entirely on the verify-ca path, always calling .with_no_client_auth() regardless of ssl_cert/ssl_key. Fixed during this rebase so verify-ca now attaches a configured client cert the same way verify-full and the default branch already do. Added build_tls_connector_verify_ca_attaches_a_configured_client_cert, checking ClientConfig::client_auth_cert_resolver.has_certs() (not just "builds successfully", which the buggy version also did) -- confirmed it fails against the naive merge and passes against this fix. Fixes #38.
aesslinger
added a commit
that referenced
this pull request
Aug 19, 2026
build_tls_connector wrapped rustls::client::WebPkiServerVerifier for verify-ca, whose verify_server_cert unconditionally checks the hostname via verify_server_name with no way to opt out -- making verify-ca behave identically to verify-full. verify-ca is supposed to validate the certificate chain but skip hostname verification -- that's the entire distinction from verify-full, matching libpq sslmode=verify-ca semantics. Ported VerifyCaCertVerifier from the builtin driver's src-tauri/src/pool_manager.rs, which deliberately avoids WebPkiServerVerifier for this exact reason and instead calls rustls::client::verify_server_cert_signed_by_trust_anchor directly (never reads server_name at all). Proved the bug and the fix with a chain-valid cert whose hostname deliberately doesn't match the connection target -- rejected before this fix, accepted after -- while confirming a CA-untrusted cert is still correctly rejected, and verify-full still correctly rejects the same hostname mismatch. Rebasing onto main (after #35 and #37 merged) surfaced a real merge hazard: naively combining #38's verify-ca branch with #35's client-auth logic would have dropped client_auth entirely on the verify-ca path, always calling .with_no_client_auth() regardless of ssl_cert/ssl_key. Fixed during this rebase so verify-ca now attaches a configured client cert the same way verify-full and the default branch already do. Added build_tls_connector_verify_ca_attaches_a_configured_client_cert, checking ClientConfig::client_auth_cert_resolver.has_certs() (not just "builds successfully", which the buggy version also did) -- confirmed it fails against the naive merge and passes against this fix. Fixes #38.
4 tasks
aesslinger
added a commit
that referenced
this pull request
Aug 19, 2026
build_tls_connector wrapped rustls::client::WebPkiServerVerifier for verify-ca, whose verify_server_cert unconditionally checks the hostname via verify_server_name with no way to opt out -- making verify-ca behave identically to verify-full. verify-ca is supposed to validate the certificate chain but skip hostname verification -- that's the entire distinction from verify-full, matching libpq sslmode=verify-ca semantics. Ported VerifyCaCertVerifier from the builtin driver's src-tauri/src/pool_manager.rs, which deliberately avoids WebPkiServerVerifier for this exact reason and instead calls rustls::client::verify_server_cert_signed_by_trust_anchor directly (never reads server_name at all). Proved the bug and the fix with a chain-valid cert whose hostname deliberately doesn't match the connection target -- rejected before this fix, accepted after -- while confirming a CA-untrusted cert is still correctly rejected, and verify-full still correctly rejects the same hostname mismatch. Rebasing onto main (after #35 and #37 merged) surfaced a real merge hazard: naively combining #38's verify-ca branch with #35's client-auth logic would have dropped client_auth entirely on the verify-ca path, always calling .with_no_client_auth() regardless of ssl_cert/ssl_key. Fixed during this rebase so verify-ca now attaches a configured client cert the same way verify-full and the default branch already do. Added build_tls_connector_verify_ca_attaches_a_configured_client_cert, checking ClientConfig::client_auth_cert_resolver.has_certs() (not just "builds successfully", which the buggy version also did) -- confirmed it fails against the naive merge and passes against this fix. Fixes #38.
This was referenced Aug 19, 2026
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
1 participant
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
connection_key(src/client.rs) matched only onhost:port:database:user:startup_script, ignoringssl_mode/ssl_ca/ssl_cert/ssl_keyentirely. So two connections to the same target differing only in TLS configuration —requirevs.verify-full, or two different client certs — could incorrectly share a cached pool and its already-negotiated TLS setup.Fixes #36.
Not a builtin-parity gap — a plugin-specific miss
Surfaced while reviewing #34/#35's client-cert fix, which touches the same TLS config path but not the cache key.
This one is not inherited from the builtin driver. The builtin's
build_connection_keyalready keyed onssl_mode/ssl_castarting tabularis#278 (2026-06-04). This plugin'sclient.rswasn't staged until 2026-08-11 — over two months later — so the gap was introduced during extraction, not something that arrived upstream afterward. Full timeline in #36.Changes
ssl_mode,ssl_ca,ssl_cert,ssl_keyintoconnection_key's format string, matching the builtin's TLS-param keying shape.connection_key's doc comment andclient.rs's module-level "Pool caching" doc comment.CLAUDE.md's description of the pool cache key so it doesn't go stale.Relationship to #35
Independent of #35 (client cert not honored in the TLS connector) — different function (
connection_keyvs.build_tls_connector), no functional overlap. Either can merge first; both are intended for the same upcoming beta tag.TDD approach
Added
connection_key_differs_by_ssl_mode/_ssl_ca/_ssl_cert/_ssl_key_alonetosrc/client_tests.rsfirst (mirroring the builtin'spostgres_pool_key_changes_when_ssl_*_changestest names), confirmed all four failed against the oldconnection_key(same key regardless of TLS params), then implemented the fix until green.Test plan
cargo test --lib --bins— 92/92 pass (88 previous + 4 new)cargo clippy --all-targets -- -D warnings— cleancargo fmt --all -- --check— cleannpx markdownlint CHANGELOG.md CLAUDE.md— clean