⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 55.94406% with 63 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.79%. Comparing base (389b876) to head (90785c4).

@@            Coverage Diff             @@
##             main     #711      +/-   ##
==========================================
- Coverage   68.84%   68.79%   -0.05%     
==========================================
  Files          46       46              
  Lines       30870    30992     +122     
==========================================
+ Hits        21251    21318      +67     
- Misses       9619     9674      +55     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

REV Review — PR #711: fix(ssl): sslmode=require without cert verification

Summary

Correctly implements sslmode=require semantics (encrypt-only, no cert verification) by introducing a NoVerifier custom ServerCertVerifier, replacing the previous behavior that used the system CA store and broke against self-signed/private-CA server certs. Also refactors error classification into a testable classify_connect_error function and adds fine-grained SSL error variants matching psql wording.

Blocking findings

None.

Non-blocking findings

src/connection.rs — NoVerifier::new() hardcodes ring::default_provider()

NoVerifier::new() (line ~1584) instantiates rustls::crypto::ring::default_provider() for its provider field, while main.rs installs aws_lc_rs as the global process-level provider. This is the same pattern used by the pre-existing NoCnVerifier (line ~1655), so this is not a regression — but both structs carry a latent inconsistency: if ring is ever dropped as a Cargo dependency (it is currently in Cargo.lock only because rustls pulls it in for optional features), the crypto::ring module would disappear. Consider using rustls::crypto::CryptoProvider::get_default().cloned().unwrap_or_else(|| Arc::new(rustls::crypto::aws_lc_rs::default_provider())) in both verifiers, or extracting provider construction into a shared helper. Low urgency because both providers are currently compiled in.

src/connection.rs — no test for the partial cert/key WARNING path in make_tls_config_require

The branch (Some(_), None) | (None, Some(_)) emits a WARNING to stderr and falls back to with_no_client_auth(). This behavior is documented in the docstring and matches the pre-existing make_tls_config_verify_ca/full functions (which also lack this test), but for a security-sensitive path it would be worth a unit test that confirms the fallback (e.g., constructing a ConnParams with only ssl_cert set and asserting the returned config has no client auth). Not blocking because the pattern is identical to the verified CA functions.

src/connection.rs — classify_connect_error string matching is fragile for future rustls versions

classify_connect_error matches on the literal substrings "invalid peer certificate" and "certificate verify failed" (line ~2258–2261). These are rustls Display strings and could change across minor rustls bumps. A comment citing the specific rustls error type (rustls::Error::InvalidCertificate) and the rustls version where the string was verified would make future maintenance easier. Again, the surrounding comment for "server does not support TLS" already does this correctly — same approach should be applied here.

CLAUDE.md — security rules addition is appropriate but placement could be more discoverable

The new "Security rules" section is a useful addition. Consider whether it belongs in a dedicated SECURITY.md or docs/contributing.md rather than CLAUDE.md, so it's visible to contributors who may not be using Claude Code. This is cosmetic.

Potential issues

map_connect_error error chain walk may pick up an intermediate, less-specific message

The new map_connect_error walks std::error::Error::source() and replaces best with any non-empty, non-duplicate string it finds, always preferring the last such source message. In practice, tokio-postgres wraps errors as Kind::Tls → rustls error → (nothing deeper), so the innermost message is what we want. However, if a future tokio-postgres version adds another wrapping layer, the walk could surface an internal implementation detail. This is an acceptable trade-off and matches the documented intent; just worth flagging for awareness.

Test coverage

Strong. The PR adds 7 targeted unit tests for classify_connect_error covering:

• SslNotSupported from the tokio-postgres "server does not support TLS" string
• SslCertVerificationFailed from both "invalid peer certificate: UnknownIssuer" and "invalid peer certificate: NotValidForName"
• SslRequired (server-side enforcement)
• Generic TlsError fallthrough
• AuthenticationFailed guard
• ConnectionFailed guard

Tests are well-structured, use assert!(matches!(...)) correctly with {err:?} in failure messages, and verify both the variant and the Display string where relevant.

Gaps (non-blocking, noted above):

• No test for the partial ssl_cert/ssl_key warning path in make_tls_config_require
• No integration test for the happy path (sslmode=require against a real server with a self-signed cert), but this is impractical in unit tests and likely covered by manual/CI integration tests

Verdict

APPROVE