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

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 69.10%. Comparing base (4a14585) to head (78d36c7).

@@           Coverage Diff           @@
##             main     #738   +/-   ##
=======================================
  Coverage   69.10%   69.10%           
=======================================
  Files          47       47           
  Lines       31990    31990           
=======================================
  Hits        22105    22105           
  Misses       9885     9885           

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

REV Code Review — PR #738

PR: #738 — fix(ssl): NoCnVerifier catches all InvalidCertificate variants for verify-ca
Branch: fix/verify-ca-hostname-error
Diff: 1 file changed, src/connection.rs (+8 / -3)

BLOCKING ISSUES

None.

NON-BLOCKING

INFO src/connection.rs:2035-2036 — Or-pattern is correct but exhaustiveness comment could be stronger

The comment says webpki 0.103+ always returns NotValidForNameContext. Consider adding a note that NotValidForName is kept defensively for any future rustls path that might still emit it, so the pattern documents intent. Minor documentation nit, not a code issue.

POTENTIAL ISSUES

None.

Detailed Analysis

1. Or-pattern correctness

Err(RustlsError::InvalidCertificate(
    rustls::CertificateError::NotValidForName
    | rustls::CertificateError::NotValidForNameContext { .. },
)) => Ok(ServerCertVerified::assertion()),

Correct. Rust's or-patterns in match arms are stable since 1.53 and the syntax is valid. Both variants are semantically identical per the rustls source (NotValidForNameContext is explicitly documented as "semantically the same as NotValidForName"). The { .. } wildcard correctly ignores the expected and presented fields, which are irrelevant here — we're intentionally using a dummy hostname and we expect a name mismatch.

2. Exhaustiveness — other CertificateError variants

Reviewed the full CertificateError enum in rustls 0.23.37:

BadEncoding, Expired, ExpiredContext { .. }, NotValidYet, NotValidYetContext { .. },
Revoked, UnhandledCriticalExtension, UnknownIssuer, UnknownRevocationStatus,
ExpiredRevocationList, ExpiredRevocationListContext { .. }, BadSignature,
UnsupportedSignatureAlgorithm (deprecated), UnsupportedSignatureAlgorithmContext { .. },
UnsupportedSignatureAlgorithmForPublicKeyContext { .. },
NotValidForName, NotValidForNameContext { .. },
InvalidPurpose, InvalidPurposeContext { .. },
InvalidOcspResponse, ApplicationVerificationFailure, Other(_)

Only NotValidForName and NotValidForNameContext are caught — the Err(e) => Err(e) arm propagates all others. This is correct: a chain failure (UnknownIssuer, Expired, BadSignature, etc.) must still surface as a real error. Catching only the name-mismatch variants is exactly right.

3. Impact on verify-full (FullVerifier)

FullVerifier is a completely separate struct (src/connection.rs:1860) with its own verify_chain and ServerCertVerifier impl. It passes the real server_name to WebPkiServerVerifier::verify_server_cert() and contains no NotValidForName catch — hostname errors propagate normally. The PR touches only NoCnVerifier::verify_chain. FullVerifier is unaffected. ✓

4. Scope isolation — NoCnVerifier only

make_tls_config_verify_ca() (line 1705) uses NoCnVerifier; make_tls_config_verify_full() (line 1743) uses FullVerifier. The changed match arm is inside NoCnVerifier::verify_chain(), which is not called from any other code path. The scope is correctly limited. ✓

5. Test coverage

Gap identified (non-blocking): There is no unit test for NoCnVerifier::verify_chain that directly exercises the NotValidForNameContext branch. The existing tests at lines 4385–4409 (test_classify_ssl_cert_verification_failed_*) test classify_connect_error() string parsing, not the verifier logic itself.

The regression risk is real: if someone replaces the or-pattern with only NotValidForName again, all existing unit tests will still pass because none of them call NoCnVerifier::verify_chain() with a real certificate. The test suite's 1809 passing tests do not catch this class of regression.

A targeted unit test injecting CertificateError::NotValidForNameContext { .. } into the match path would prevent future regression. Example:

#[test]
fn test_no_cn_verifier_accepts_not_valid_for_name_context() {
    use rustls::CertificateError;
    use rustls::Error as RustlsError;
    // Simulate what NoCnVerifier::verify_chain() would receive from webpki 0.103+
    let err = RustlsError::InvalidCertificate(CertificateError::NotValidForNameContext {
        expected: rustls::ServerName::try_from("dummy.invalid").unwrap(),
        presented: vec!["localhost".to_owned()],
    });
    // The match arm must treat this as success (Ok), not propagate as Err
    let result: Result<(), _> = match err {
        RustlsError::InvalidCertificate(
            CertificateError::NotValidForName
            | CertificateError::NotValidForNameContext { .. },
        ) => Ok(()),
        e => Err(e),
    };
    assert!(result.is_ok(), "NotValidForNameContext must be swallowed by NoCnVerifier");
}

Summary

Verdict: APPROVE. The fix is correct, minimal, and well-scoped. verify-full is not affected. The only gap is missing unit test coverage for NoCnVerifier with the NotValidForNameContext variant — noted as a suggestion, not a blocker.

REV-assisted review (AI analysis by postgres-ai/rev)

Testing evidence

Build: fix/verify-ca-hostname-error branch, rustls 0.23.37 / webpki 0.103.10

Finished `release` profile [optimized] target(s) in 3m 29s

Build clean. ✓

Unit tests: 1809 passed, 0 failed

test result: ok. 1809 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.11s

✓

verify-ca (correct CA): connects, query succeeds

PGPASSWORD=testpass PGSSLROOTCERT=/tmp/pg-tls-certs/ca.crt   ./target/release/rpg -h localhost -p 15436 -U postgres --sslmode=verify-ca -c "SELECT 'verify-ca ok'"

 ?column?
--------------
 verify-ca ok
(1 row)

✓

verify-full (correct CA, hostname=localhost): connects, query succeeds

PGPASSWORD=testpass PGSSLROOTCERT=/tmp/pg-tls-certs/ca.crt   ./target/release/rpg -h localhost -p 15436 -U postgres --sslmode=verify-full -c "SELECT 'verify-full ok'"

 ?column?
----------------
 verify-full ok
(1 row)

✓ — verify-full unaffected by this change

verify-ca with wrong CA (must FAIL):

PGPASSWORD=testpass PGSSLROOTCERT=/etc/ssl/certs/ca-certificates.crt   ./target/release/rpg -h localhost -p 15436 -U postgres --sslmode=verify-ca -c "SELECT 1"

rpg: SSL error: certificate verification failed: invalid peer certificate: UnknownIssuer

✓ — CA mismatch correctly rejected; NoCnVerifier is NOT swallowing non-hostname errors