REV Code Review — PR #734

Reviewing src/connection.rs diff (two fixes: chain-complete SSL for verify-ca/verify-full, and sslmode=prefer double-timeout).

PRAISE — Two-attempt chain completion algorithm

NoCnVerifier::verify_server_cert and FullVerifier::verify_server_cert both use the same clean two-attempt pattern: try what the server sent, then on UnknownIssuer retry with certs from sslrootcert prepended as intermediates. This matches what OpenSSL/psql do internally and is the correct algorithm. The guard !self.extra_intermediates.is_empty() means the fast path is unchanged when no bundle is configured.

Security analysis: augmenting the intermediate list does not lower the trust bar — the chain must still terminate at a trust anchor in roots. No security regression.

PRAISE — FullVerifier hostname check preserved

The new FullVerifier passes the real server_name (not dummy.invalid) to WebPkiServerVerifier::verify_server_cert, so hostname validation is fully intact for sslmode=verify-full. Clean separation from NoCnVerifier's dummy-name trick.

PRAISE — Timeout fix is correctly scoped

is_timeout_error is only checked on the sslmode=prefer TLS→plain fallback path. Other sslmodes are unaffected. The function correctly limits its match to ConnectionError::ConnectionFailed, so TLS errors (SslNotSupported, TlsError, etc.) are never misidentified as timeouts.

MINOR — is_timeout_error() relies on string matching (src/connection.rs, is_timeout_error)

fn is_timeout_error(e: &ConnectionError) -> bool {
    match e {
        ConnectionError::ConnectionFailed { reason, .. } => {
            let r = reason.to_lowercase();
            r.contains("timeout") || r.contains("timed out")
        }
        _ => false,
    }
}

This works for the current tokio-postgres 0.7.16, which explicitly constructs:

io::Error::new(io::ErrorKind::TimedOut, "connection timed out")

(confirmed in connect_socket.rs). It also catches OS-level ETIMEDOUT ("Connection timed out").

The problem: the match depends on a human-readable string from an external crate. A future tokio-postgres patch that changes the message (e.g., to "connect timed out" or "deadline elapsed") would silently break the fix — the fallback would proceed and double-timeout again, with no error at the call site.

Suggestion: Add a comment citing the exact tokio-postgres source location and version:

// Matches the literal string tokio-postgres constructs for connect_timeout
// expiry (connect_socket.rs:62, v0.7.x: "connection timed out") and
// OS-level ETIMEDOUT ("Connection timed out").  If tokio-postgres changes
// this string the fix degrades gracefully (fallback proceeds, slower) but
// silently — revisit if #723 reappears after a tokio-postgres upgrade.

Longer term, thread io::ErrorKind through ConnectionError::ConnectionFailed so the check can be kind == TimedOut instead of a string scan.

MINOR — PGCONNECT_TIMEOUT=0s when no timeout is configured (src/connection.rs:2426)

reason: format!("connection timed out (PGCONNECT_TIMEOUT={timeout}s)",
    timeout = params.connect_timeout.unwrap_or(0)),

When connect_timeout is None (no application-level timeout configured), an OS-level TCP timeout (ETIMEDOUT) can still fire. is_timeout_error would catch it (the OS message "Connection timed out" matches), and the error displayed to the user would be:

connection timed out (PGCONNECT_TIMEOUT=0s)

0s implies the user configured a zero-second timeout, which is incorrect and confusing. When connect_timeout is None, the message should not mention PGCONNECT_TIMEOUT at all, or should omit the value.

Suggestion:

reason: match params.connect_timeout {
    Some(t) => format!("connection timed out (PGCONNECT_TIMEOUT={t}s)"),
    None    => "connection timed out".to_string(),
},

MINOR — WebPkiServerVerifier rebuilt on every call in FullVerifier::verify_chain (src/connection.rs, FullVerifier)

fn verify_chain(&self, ...) -> Result<ServerCertVerified, RustlsError> {
    let verifier = rustls::client::WebPkiServerVerifier::builder_with_provider(
        Arc::new(self.roots.clone()),   // clones entire RootCertStore each call
        Arc::clone(&self.provider),
    )
    .build()
    .map_err(|e| RustlsError::General(...))?;

    verifier.verify_server_cert(...)
}

build() validates all trust anchors; self.roots.clone() copies potentially hundreds of roots (e.g. the webpki_roots bundle). This runs once per TLS handshake, and twice per handshake on the chain-completion retry path.

NoCnVerifier::verify_chain has the same pattern (pre-existing, not new in this PR), so fixing only FullVerifier would be inconsistent, but FullVerifier is entirely new code.

Suggestion: Build the inner verifier once in FullVerifier::new() and store Arc<WebPkiServerVerifier>:

struct FullVerifier {
    inner: Arc<rustls::client::WebPkiServerVerifier>,
    extra_intermediates: Vec<CertificateDer<'static>>,
}

If build() can fail, propagate the error through a Result-returning new() (callers already handle Result<ClientConfig, ConnectionError>).

NIT — Silent failure in load_certs_as_intermediates (src/connection.rs:1644–1651)

let Ok(pem) = std::fs::read(p) else { return vec![] };
rustls_pemfile::certs(&mut pem.as_slice())
    .filter_map(Result::ok)   // silently drops parse errors
    .collect()

In practice the file was already read successfully by load_root_cert_store moments earlier, so both silent branches are unlikely. The filter_map(Result::ok) is the more concerning one: a partially-malformed bundle (e.g. mixed PEM/DER) would yield fewer intermediates than expected without any diagnostic.

This doesn't affect chain verification correctness (the first attempt still runs; only the retry path is impacted), but it could make debugging issue recurrences harder.

Suggestion: Not worth restructuring for this PR, but log a debug/trace message on parse error, or at minimum document the silent-failure intent in a comment.

NIT — ring provider hardcoded in both verifiers

Both NoCnVerifier::new and FullVerifier::new do:

provider: Arc::new(rustls::crypto::ring::default_provider()),

If the crate later adds an aws-lc-rs feature or makes the provider configurable at the ClientConfig level, both verifiers will need updating. This matches the pre-existing NoCnVerifier pattern so no regression here, but worth a // TODO if a provider-config story is planned.

Overall verdict: APPROVE (with MINOR issues noted)

Both fixes are logically correct. The chain-completion two-attempt approach matches psql/OpenSSL semantics and introduces no security regression. The timeout propagation is correctly scoped. The two MINOR issues (PGCONNECT_TIMEOUT=0s display and WebPkiServerVerifier rebuild cost) are real but non-blocking; the string-matching fragility is low risk given the explicit tokio-postgres source pin. All findings above are NON-BLOCKING.

Copyright 2026.

REV Code Review — PR #734

Reviewing src/connection.rs diff (two fixes: chain-complete SSL for verify-ca/verify-full, and sslmode=prefer double-timeout).

PRAISE — Two-attempt chain completion algorithm

NoCnVerifier::verify_server_cert and FullVerifier::verify_server_cert both use the same clean two-attempt pattern: try what the server sent, then on UnknownIssuer retry with certs from sslrootcert prepended as intermediates. This matches what OpenSSL/psql do internally and is the correct algorithm. The guard !self.extra_intermediates.is_empty() means the fast path is unchanged when no bundle is configured.

Security analysis: augmenting the intermediate list does not lower the trust bar — the chain must still terminate at a trust anchor in roots. No security regression.

PRAISE — FullVerifier hostname check preserved

The new FullVerifier passes the real server_name (not dummy.invalid) to WebPkiServerVerifier::verify_server_cert, so hostname validation is fully intact for sslmode=verify-full. Clean separation from NoCnVerifier's dummy-name trick.

PRAISE — Timeout fix is correctly scoped

is_timeout_error is only checked on the sslmode=prefer TLS→plain fallback path. Other sslmodes are unaffected. The function correctly limits its match to ConnectionError::ConnectionFailed, so TLS errors (SslNotSupported, TlsError, etc.) are never misidentified as timeouts.

MINOR — is_timeout_error() relies on string matching (src/connection.rs, is_timeout_error)

fn is_timeout_error(e: &ConnectionError) -> bool {
    match e {
        ConnectionError::ConnectionFailed { reason, .. } => {
            let r = reason.to_lowercase();
            r.contains("timeout") || r.contains("timed out")
        }
        _ => false,
    }
}

This works for the current tokio-postgres 0.7.16, which explicitly constructs:

io::Error::new(io::ErrorKind::TimedOut, "connection timed out")

(confirmed in connect_socket.rs). It also catches OS-level ETIMEDOUT ("Connection timed out").

The problem: the match depends on a human-readable string from an external crate. A future tokio-postgres patch that changes the message (e.g., to "connect timed out" or "deadline elapsed") would silently break the fix — the fallback would proceed and double-timeout again, with no error at the call site.

Suggestion: Add a comment citing the exact tokio-postgres source location and version:

// Matches the literal string tokio-postgres constructs for connect_timeout
// expiry (connect_socket.rs:62, v0.7.x: "connection timed out") and
// OS-level ETIMEDOUT ("Connection timed out").  If tokio-postgres changes
// this string the fix degrades gracefully (fallback proceeds, slower) but
// silently — revisit if #723 reappears after a tokio-postgres upgrade.

Longer term, thread io::ErrorKind through ConnectionError::ConnectionFailed so the check can be kind == TimedOut instead of a string scan.

MINOR — PGCONNECT_TIMEOUT=0s when no timeout is configured (src/connection.rs:2426)

reason: format!("connection timed out (PGCONNECT_TIMEOUT={timeout}s)",
    timeout = params.connect_timeout.unwrap_or(0)),

When connect_timeout is None (no application-level timeout configured), an OS-level TCP timeout (ETIMEDOUT) can still fire. is_timeout_error would catch it (the OS message "Connection timed out" matches), and the error displayed to the user would be:

connection timed out (PGCONNECT_TIMEOUT=0s)

0s implies the user configured a zero-second timeout, which is incorrect and confusing. When connect_timeout is None, the message should not mention PGCONNECT_TIMEOUT at all, or should omit the value.

Suggestion:

reason: match params.connect_timeout {
    Some(t) => format!("connection timed out (PGCONNECT_TIMEOUT={t}s)"),
    None    => "connection timed out".to_string(),
},

MINOR — WebPkiServerVerifier rebuilt on every call in FullVerifier::verify_chain (src/connection.rs, FullVerifier)

fn verify_chain(&self, ...) -> Result<ServerCertVerified, RustlsError> {
    let verifier = rustls::client::WebPkiServerVerifier::builder_with_provider(
        Arc::new(self.roots.clone()),   // clones entire RootCertStore each call
        Arc::clone(&self.provider),
    )
    .build()
    .map_err(|e| RustlsError::General(...))?;

    verifier.verify_server_cert(...)
}

build() validates all trust anchors; self.roots.clone() copies potentially hundreds of roots (e.g. the webpki_roots bundle). This runs once per TLS handshake, and twice per handshake on the chain-completion retry path.

NoCnVerifier::verify_chain has the same pattern (pre-existing, not new in this PR), so fixing only FullVerifier would be inconsistent, but FullVerifier is entirely new code.

Suggestion: Build the inner verifier once in FullVerifier::new() and store Arc<WebPkiServerVerifier>:

struct FullVerifier {
    inner: Arc<rustls::client::WebPkiServerVerifier>,
    extra_intermediates: Vec<CertificateDer<'static>>,
}

If build() can fail, propagate the error through a Result-returning new() (callers already handle Result<ClientConfig, ConnectionError>).

NIT — Silent failure in load_certs_as_intermediates (src/connection.rs:1644–1651)

let Ok(pem) = std::fs::read(p) else { return vec![] };
rustls_pemfile::certs(&mut pem.as_slice())
    .filter_map(Result::ok)   // silently drops parse errors
    .collect()

In practice the file was already read successfully by load_root_cert_store moments earlier, so both silent branches are unlikely. The filter_map(Result::ok) is the more concerning one: a partially-malformed bundle (e.g. mixed PEM/DER) would yield fewer intermediates than expected without any diagnostic.

This doesn't affect chain verification correctness (the first attempt still runs; only the retry path is impacted), but it could make debugging issue recurrences harder.

Suggestion: Not worth restructuring for this PR, but log a debug/trace message on parse error, or at minimum document the silent-failure intent in a comment.

NIT — ring provider hardcoded in both verifiers

Both NoCnVerifier::new and FullVerifier::new do:

provider: Arc::new(rustls::crypto::ring::default_provider()),

If the crate later adds an aws-lc-rs feature or makes the provider configurable at the ClientConfig level, both verifiers will need updating. This matches the pre-existing NoCnVerifier pattern so no regression here, but worth a // TODO if a provider-config story is planned.

Overall verdict: APPROVE (with MINOR issues noted)

Both fixes are logically correct. The chain-completion two-attempt approach matches psql/OpenSSL semantics and introduces no security regression. The timeout propagation is correctly scoped. The two MINOR issues (PGCONNECT_TIMEOUT=0s display and WebPkiServerVerifier rebuild cost) are real but non-blocking; the string-matching fragility is low risk given the explicit tokio-postgres source pin. All findings above are NON-BLOCKING.

Copyright 2026.

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

Codecov Report

❌ Patch coverage is 5.07246% with 131 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.38%. Comparing base (fd54b19) to head (b0d655c).

@@            Coverage Diff             @@
##             main     #734      +/-   ##
==========================================
- Coverage   68.65%   68.38%   -0.27%     
==========================================
  Files          46       46              
  Lines       31132    31265     +133     
==========================================
+ Hits        21372    21379       +7     
- Misses       9760     9886     +126     

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