feat(postgresql)/issue 209 verify ca and verify full

[VincentZhangy]

Extend the SSL mode support in libpq to include 'verify-ca' and 'verify-full',
bringing it in line with PostgreSQL's libpq behavior.

• verify-ca: verify server certificate is signed by a trusted CA
• verify-full: additionally verify the server hostname matches the certificate CN or SAN

Fix issues in the existing SSL implementation where only 'require' mode was
properly handled. Refactor the SSL context configuration to correctly set
certificate verification flags and hostname validation callbacks.

Optimize the certificate verification logic by:

• Reusing the same verification callback for both modes
• Reducing redundant certificate chain traversals
• Improving error messages for certificate validation failures

This change ensures secure TLS connections with proper certificate checks,
preventing man-in-the-middle attacks when using the stricter SSL modes.

close #209

[ymadd]

Thanks for tackling #209! I'm not a maintainer — just a user who read through the diff out of interest. Sharing a few observations below in case they're useful to you or the maintainers. Take or leave any of these.

Things that stood out

1. webpki (rustls-webpki 0.101) looks unused

src-tauri/Cargo.toml adds webpki = { package = "rustls-webpki", version = "0.101" }, but I couldn't find a webpki:: reference anywhere in src-tauri/src/. As a side effect, Cargo.lock now carries both rustls-webpki 0.101.7 and 0.103.9. Might be worth dropping if it's truly unused.

2. The verify-ca fallback in VerifyCaCertVerifier::verify_server_cert is hard to reason about

pool_manager.rs:382-408:

Err(e) if is_hostname_error(&e) => {
    self.inner.verify_server_cert(
        end_entity, intermediates,
        &ServerName::try_from("localhost").unwrap(),
        ocsp_response, now,
    )
    .or(Ok(ServerCertVerified::assertion()))
}

A couple of concerns from a reader's perspective:

• .or(Ok(ServerCertVerified::assertion())) converts any error from the second call into success. In practice the chain was already validated by the first call so this is probably benign today, but it relies on a specific internal ordering of WebPkiServerVerifier. If that ever changes, an unverified chain could slip through silently.
• The "localhost" placeholder + a second full verification means the chain gets validated twice per handshake.

rustls 0.23 exposes rustls::client::verify_server_cert_signed_by_trust_anchor and rustls::client::verify_server_name as public API (see rustls-0.23.40/src/lib.rs:615-618). A more direct shape might be:

struct VerifyCaCertVerifier {
    roots: Arc<RootCertStore>,
    supported: WebPkiSupportedAlgorithms,
}

impl ServerCertVerifier for VerifyCaCertVerifier {
    fn verify_server_cert(&self, end_entity, intermediates, _server_name, _ocsp, now)
        -> Result<ServerCertVerified, TlsError>
    {
        let cert = ParsedCertificate::try_from(end_entity)?;
        verify_server_cert_signed_by_trust_anchor(&cert, &self.roots, intermediates, now, self.supported.all)?;
        Ok(ServerCertVerified::assertion())
    }
    // verify_tls{12,13}_signature delegate to rustls::crypto::verify_tls{12,13}_signature
}

This makes "we intentionally skip hostname verification" explicit, avoids the .or(Ok(...)), and only validates the chain once.

3. WebPkiServerVerifier::builder(...).build().expect(...) can panic

pool_manager.rs:367. builder().build() returns Err(VerifierBuilderError::NoRootAnchors) for an empty RootCertStore, and load_platform_roots() can return empty on macOS if rustls_native_certs::load_native_certs() partially fails. Propagating via Result would avoid a hard panic.

4. The verify-ca default path may regress what PR #167 fixed on macOS

load_platform_roots() uses rustls_native_certs::load_native_certs() and feeds them straight into WebPkiServerVerifier. PR #167 explicitly moved away from this pattern because of two macOS-specific problems:

• Secure Transport's strict id-kp-serverAuth EKU check on user-supplied root anchors rejects valid CA certs (including the AWS RDS bundle).
• The macOS keychain doesn't trust regional Amazon RDS roots out of the box.

The existing docstring at pool_manager.rs:188-214 mentions this design decision. With this change, verify-ca without a user CA might fail for RDS users on macOS who currently work with the rustls-platform-verifier path. One option could be to keep rustls-platform-verifier for the no-user-CA branch of verify-ca, or to require an explicit CA file for verify-ca (which is effectively how libpq is used anyway). Either way the docstring would want a refresh.

5. require semantics change is a behavior shift worth flagging

Pre-PR require did strict verification (PR #167 made that intentional); post-PR require skips verification (which matches libpq). That's the right direction IMO, but users who currently rely on require + CA bundle on RDS will silently lose verification on upgrade. A CHANGELOG note pointing them at verify-full (or verify-ca) might save some confusion.

---

A few smaller notes I'll keep out of this comment unless useful: only en + zh of the 7 locales got verify-ca / verify-full strings; CryptoProvider::get_default().unwrap() is called per-handshake inside NoCertVerifier::supported_verify_schemes; the test_tls_connector_* tests assert is_ok() rather than verifier semantics (a NoCertVerifier in every branch would still pass them).

Hope some of this helps — and thanks again for the work on this.

[kilo-code-bot]

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Resolved Issues (2)

File	Line	Issue
src/components/modals/NewConnectionModal.tsx	1016	i18n translation key mismatch for verify-ca — fixed
src/components/modals/NewConnectionModal.tsx	1017	i18n translation key mismatch for verify-full — fixed

Files Reviewed (1 file)

• src/components/modals/NewConnectionModal.tsx

---

_{Reviewed by kimi-k2.6 · 146,779 tokens}

[VincentZhangy]

@ymadd Thank you again for the extremely thorough review – it was incredibly helpful.
I’ve gone through every point you raised, and most of them have been addressed exactly as you suggested. Below is a quick summary of the fixes already in place, followed by one small remaining issue I spotted while double‑checking the code.

What has been fixed (matching your feedback)

1. Unused webpki 0.101 – removed from Cargo.toml; Cargo.lock now only carries rustls-webpki 0.103.x. ✅
2. VerifyCaCertVerifier rewritten – now uses verify_server_cert_signed_by_trust_anchor directly, drops the "localhost" placeholder, validates the chain once, and makes the “skip hostname check” intent explicit. No more .or(Ok(...)). ✅
3. No more panics – VerifyCaCertVerifier::new() returns Result<Self, String> and errors propagate through build_postgres_tls_connector. WebPkiServerVerifier::builder().build() uses map_err with ? instead of expect. ✅
4. verify-ca now requires an explicit CA file – we no longer fall back to platform roots, matching libpq’s sslmode=verify-ca and avoiding macOS Secure Transport EKU issues. The docstring has been updated accordingly. ✅
5. require semantics change – documented in CHANGELOG as a breaking change, pointing users toward verify-ca/verify-full for validation. ✅
6. Smaller notes – i18n strings added for all five missing locales, NoCertVerifier caches WebPkiSupportedAlgorithms at construction, dead code (is_hostname_error, load_platform_roots) removed, tests updated to cover the new verify-ca behaviour. ✅

One minor issue I noticed while reviewing the code

In load_roots_from_pem (pool_manager.rs), the current implementation writes:

let mut cursor = std::io::Cursor::new(&pem[..]);
for cert in rustls_pemfile::certs(&mut cursor) {
    let cert = cert.map_err(...)?;
    ...
}

However, rustls_pemfile::certs returns a Result<impl Iterator<...>, ...>. The for loop cannot iterate directly over a Result – the function call should be followed by ? and then the iterator should be used, e.g.:

let cert_iter = rustls_pemfile::certs(&mut cursor)
    .map_err(|e| format!("Failed to parse ssl_ca '{}': {}", path, e))?;
for cert in cert_iter {
    let cert = cert.map_err(|e| format!("..."))?;
    roots.add(cert).map_err(|e| format!("..."))?;
}

I believe this is just a copy‑paste oversight in the snippet you shared – the actual code in your repository may already be correct. Could you double‑check it? Apart from that, everything else looks solid.

Final note

Your review was exceptionally detailed and saved us from several subtle pitfalls. Thank you again for taking the time to write it up. If you spot anything else, please don’t hesitate to point it out – I’ll be happy to follow up.

Best regards

[ymadd]

@VincentZhangy Thanks for the thorough revisions! All the Critical items look cleanly addressed, and the rewrite of VerifyCaCertVerifier on top of verify_server_cert_signed_by_trust_anchor is exactly the right shape. Nicely done.

[debba]

Looks a very good work!
Thanks @VincentZhangy and @ymadd for the revision :)
I will take a look and if no problems I will merge it asap.

[debba]

Hi @VincentZhangy,

Thanks for the detailed write-up — the changes look good to me, everything you listed lines up with the diff.

About the load_roots_from_pem snippet: if I'm not mistaken, the code in the repo is actually fine. The signature you're describing (Result<impl Iterator<...>, ...>) is the old rustls-pemfile 1.x API. Since 2.0, certs() was reworked to return the iterator directly:

pub fn certs<R: BufRead>(rd: &mut R)
    -> impl Iterator<Item = Result<CertificateDer<'static>, io::Error>>

So each item yielded by the loop is already a Result, and the inner cert.map_err(...)? is the idiomatic way to handle it. Cargo.toml pins rustls-pemfile = "2" (currently resolving to 2.2.0), which is why the current code compiles and CI is green. Applying ? to the certs() call itself would actually fail to compile against 2.x, since the return value isn't a Result anymore.

@ymadd could you confirm this on your side as well? If we're aligned, I'd say no further changes are needed and we can move toward merging.

By the way, if you'd like to chat with the maintainers or get more involved with the project, feel free to join our Discord: https://discord.com/invite/K2hmhfHRSt — would be great to have you there.

[ymadd]

Confirmed on my side as well — the current code is correct.

I checked the local rustls-pemfile 2.2.0 source and the signature is:

pub fn certs(rd: &mut dyn io::BufRead)
    -> impl Iterator<Item = Result<CertificateDer<'static>, io::Error>>

So the iterator is returned directly and each item is a Result. The existing
for cert in rustls_pemfile::certs(&mut cursor) { let cert = cert.map_err(...)?; ... }
pattern is the idiomatic shape against 2.x. Applying ? to certs() itself would actually break against this version, as @debba pointed out.
The pattern @VincentZhangy was thinking of is the rustls-pemfile 1.x API.

No further changes needed from my side — this looks good to merge.