crates/stdlib/src/ssl.rs (2)

1532-1541: Inconsistent error handling between certificate loading calls.

load_system_certificates errors are silently ignored with let _ = ..., while try_load_from_python_environ propagates errors with ?. This inconsistency could mask legitimate failures.

Consider either:

1. Using ? for both to propagate errors consistently
2. Using let _ = ... for both if best-effort loading is intended
3. Adding a comment explaining why one can fail silently but not the other

-            // Load system certificates first (before creating loader to avoid borrow conflicts)
-            let _ = self.load_system_certificates(&mut store, vm);
+            // Load system certificates first (before creating loader to avoid borrow conflicts)
+            // Silently ignore errors - fallback to webpki-roots below if no certs loaded
+            let _ = self.load_system_certificates(&mut store, vm);

---

4504-4556: enum_crls uses unsafe Windows API - review carefully.

The implementation correctly:

1. Opens the store and checks for null
2. Iterates CRLs with proper null termination check
3. Closes the store at the end

However, there's a potential issue: if store_name.as_str().encode_utf16() produces an invalid store name that opens successfully but causes CertEnumCRLsInStore to behave unexpectedly, the loop would still safely terminate on null.

One concern: the CertCloseStore at line 4553 is always called, but if early returns were added in the future (e.g., for error handling in the loop), the store could leak. Consider using a RAII wrapper or defer! pattern.

For improved safety, consider wrapping the store handle in a struct with Drop:

struct CertStoreHandle(windows_sys::Win32::Security::Cryptography::HCERTSTORE);
impl Drop for CertStoreHandle {
    fn drop(&mut self) {
        if !self.0.is_null() {
            unsafe { CertCloseStore(self.0, 0) };
        }
    }
}

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

crates/stdlib/src/ssl.rs (4)

1532-1554: Document platform-specific certificate loading order.

The implementation has different certificate loading strategies for Windows vs. non-Windows:

• Windows: System certs first, then environment variables (additively)
• Non-Windows: Environment variables first, system certs only as fallback

This could be confusing for users expecting consistent behavior. Additionally, the Windows path silently ignores errors from load_system_certificates (line 1536), while the non-Windows path checks success (line 1549).

Consider adding a module-level doc comment explaining this platform difference, or align the behavior if there's no strong reason for the difference. If the difference is intentional (e.g., to match CPython's test expectations), document it clearly:

// Platform-specific certificate loading strategies:
// - Windows: Always loads system store + env vars (matches test_load_default_certs_env_windows)
// - Unix: Prefers env vars over system (matches test_load_default_certs_env)

---

3443-3497: Consider implications of chunked write implementation.

The write method now processes data in 16KB chunks to avoid filling rustls's internal buffer. However, there are potential issues:

1. Partial write semantics: If a chunk fails to flush to the socket (line 3469-3495), the method returns an error, but some chunks may have already been written to the TLS layer. This differs from traditional socket write semantics where the return value indicates how much was written.

2. Performance impact: The method now makes multiple flush attempts (one per chunk) rather than buffering everything. For small writes, this could increase overhead.

3. Non-blocking behavior: In non-blocking mode, if a chunk fails with SSLWantWriteError, previously written chunks are lost from the caller's perspective.

Consider either:

• Tracking written bytes and returning partial success instead of error
• Documenting that this matches SSL_write_ex behavior (all-or-nothing in SSL layer)
• Adding a comment explaining why chunking is necessary for rustls

// Write data in chunks to avoid filling rustls's internal buffer.
// rustls has a limited internal buffer (~16KB), so we need to flush periodically.
// Note: This follows SSL_write_ex semantics - either all data is accepted or an error is returned.

---

4488-4514: Consider error handling consistency for certificate usage.

The error handling for c.valid_uses() (lines 4499-4511) catches all errors and falls back to returning true, which could mask genuine errors. This differs from the similar code in the openssl.rs context snippet (lines 3661-3666 in relevant_code_snippets), which propagates errors:

// openssl.rs pattern:
let usage: PyObjectRef = match c.valid_uses().map_err(|e| e.to_pyexception(vm))? {
    ValidUses::All => vm.ctx.new_bool(true).into(),
    // ...
};

vs. current implementation:

// Current pattern:
let usage: PyObjectRef = match c.valid_uses() {
    Ok(ValidUses::All) => vm.ctx.new_bool(true).into(),
    Ok(ValidUses::Oids(oids)) => { /* ... */ },
    Err(_) => vm.ctx.new_bool(true).into(),  // Masks error
};

Consider propagating errors for better diagnostics, or document why errors should be silently ignored. If this matches CPython's behavior of treating invalid usage as "all uses", add a comment explaining the rationale.

---

4517-4569: Improve safety and error handling in enum_crls.

The function uses extensive unsafe code with raw Windows APIs. While the implementation appears correct, there are safety and maintenance concerns:

1. Unsafe pointer operations (lines 4550-4552): Dereferencing raw pointers and creating slices without validation
2. Resource cleanup (line 4566): Store cleanup happens at the end, but if a panic occurs during iteration, the store won't be closed
3. Inconsistency: Uses windows_sys FFI directly, while enum_certificates uses the higher-level schannel crate

Consider these improvements:

1. Use RAII for store cleanup:

// Add a guard struct
struct StoreGuard(windows_sys::Win32::Security::Cryptography::HCERTSTORE);
impl Drop for StoreGuard {
    fn drop(&mut self) {
        unsafe { CertCloseStore(self.0, 0) };
    }
}

let _store_guard = StoreGuard(store);

2. Add safety comments documenting the invariants for each unsafe block:

// SAFETY: crl_context is valid because CertEnumCRLsInStore returned non-null
let crl = unsafe { &*crl_context };

3. Consider using schannel for consistency with enum_certificates, if it provides CRL enumeration APIs.

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

crates/stdlib/src/ssl.rs (1)

4346-4353: Windows get_default_verify_paths() still returns "" instead of None (same concern as earlier review).

On Windows you keep returning Some("") for both default_cafile and default_capath, which becomes empty strings in the namedtuple rather than None. CPython’s ssl.get_default_verify_paths() returns None for those fields when there is no actual default path. If downstream Python code or tests rely on is None semantics rather than falsy-string checks, this remains a behavioral divergence.

If you decide to keep the empty-string approach (to avoid None being passed into os.path.isfile, etc.), please document that deviation clearly in a comment here or in the Python-side wrapper.

crates/stdlib/src/ssl.rs (5)

1931-1953: Reuse validate_hostname() in _wrap_socket to keep semantics consistent.

_wrap_socket() currently reimplements hostname checks inline (empty, leading dot, NUL) and comments that IPs are allowed, but it:

• Doesn’t enforce the 253-character limit enforced by validate_hostname().
• Duplicates logic now shared by _wrap_bio and set_server_hostname().

Consider replacing the inline checks with a call to validate_hostname(hostname, vm)?; to keep all entry points (wrap_socket, wrap_bio, set_server_hostname) in sync, and so any future hostname rules change in one place.

---

1450-1521: Windows vs non-Windows system cert loading looks reasonable; consider error reporting consistency.

The new load_system_certificates() splits Windows and non-Windows paths cleanly:

• Windows: enumerates ROOT and CA via schannel stores for both CurrentUser and LocalMachine, counting total and CA certs.
• Non-Windows: uses rustls_native_certs::load_native_certs() and similarly updates stats.

On Windows, you convert “no certs loaded” into OSError("Failed to load certificates from Windows store"), but callers (load_default_certs) immediately ignore that error and fall back to env/webpki. On non-Windows, you only raise when there are both 0 certs and non-empty errors. If you want consistent diagnostics, you might either:

• Make Windows also return details when available (e.g., last OS error), or
• Treat the “0 certs loaded” case the same way as non-Windows and only raise when there’s an explicit error from the loader.

Not strictly required, but it would make behavior and error reporting more uniform across platforms.

---

1532-1554: load_default_certs Windows/non-Windows ordering matches intent; consider surfacing failure explicitly in logs/tests.

The new split in load_default_certs():

• Windows: always attempts system store first (via schannel), then optionally adds env-based certs.
• Non-Windows: prefers env-based SSL_CERT_FILE/SSL_CERT_DIR, falling back to system store only if env doesn’t yield anything.

This mirrors CPython’s documented/tested behavior. Since both paths eventually fall back to webpki_roots when x509_cert_count == 0, the user never sees a hard failure. If you want easier debugging of “why are my system CAs ignored?”, consider emitting a warning or debug log when system-store loading fails but you silently proceed to the Mozilla bundle.

---

3443-3499: Chunked writes + explicit TLS flushing look correct; watch for performance overhead.

The new write() path:

• Writes in 16KB chunks into conn.writer().
• After each chunk, flushes pending TLS data via write_pending_tls() in BIO mode or write_tls + sock_send in socket mode, honoring timeouts and mapping blocking errors to WANT_WRITE.

Semantically this should avoid overfilling rustls’ internal buffers and better match OpenSSL-style behavior. The main downside is per-chunk allocation of a Vec<u8> in the socket mode flush path; if this proves hot under large writes, consider reusing a single buffer or using a stack-allocated array instead of reallocating each iteration.

---

4466-4515: enum_certificates implementation looks solid; consider reducing unsafe surface.

The Windows-only enum_certificates():

• Opens both CurrentUser and LocalMachine stores for the given name.
• Returns an OSError if neither can be opened.
• Emits tuples of (der_bytes, enc_type, usage) where enc_type is "x509_asn", "pkcs_7_asn", or the raw encoding integer, and usage is either True or a frozenset of OID strings based on ValidUses.

The unsafe block only dereferences the CERT_CONTEXT pointer to read dwCertEncodingType. If the schannel crate ever exposes the encoding type safely, it would be preferable to use that API to avoid direct struct casting via windows_sys::Win32::Security::Cryptography::CERT_CONTEXT. For now this looks correct but slightly brittle across crate-version changes.

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

crates/stdlib/src/ssl.rs (1)

4346-4351: Inconsistent with CPython: Should return None instead of empty strings.

This issue was previously raised in review comments. CPython's ssl.get_default_verify_paths() returns None for cafile and capath fields when Windows certificate store is used (rather than empty strings). The current implementation returning Some("") deviates from this behavior.

Apply this diff to match CPython behavior:

 #[cfg(windows)]
 let (default_cafile, default_capath) = {
     // Windows uses certificate store, not file paths
-    // Return empty strings to avoid None being passed to os.path.isfile()
-    (Some(""), Some(""))
+    (None, None)
 };

The Python ssl.py module handles None values correctly in its logic.

crates/stdlib/src/ssl.rs (4)

1459-1491: Consider logging certificate load failures for diagnostics.

The current implementation silently skips certificates when store.add() fails (line 1476). While this approach is pragmatic for continuing the load process, diagnostic information about why specific certificates were rejected could be valuable for troubleshooting.

If rustls provides error details, consider logging them:

                                 let is_ca = cert::is_ca_certificate(cert.as_ref());
-                                if store.add(cert).is_ok() {
+                                match store.add(cert) {
+                                    Ok(_) => {
                                     *self.x509_cert_count.write() += 1;
                                     if is_ca {
                                         *self.ca_cert_count.write() += 1;
                                     }
+                                    }
+                                    Err(e) => {
+                                        // Log certificate rejection for diagnostics
+                                        eprintln!("Warning: Failed to add certificate from {} store: {:?}", store_name, e);
+                                    }
                                 }

---

3443-3497: Verify chunk flushing frequency for performance.

The implementation chunks writes into 16KB blocks and flushes after each chunk. While this prevents internal buffer overflow, it may introduce unnecessary system calls for large writes.

Consider flushing less frequently or only when the internal buffer indicates it's needed:

const CHUNK_SIZE: usize = 16384;
const FLUSH_THRESHOLD: usize = CHUNK_SIZE * 4; // Flush every 64KB
let mut bytes_since_flush = 0;

while written < data.len() {
    let chunk_end = std::cmp::min(written + CHUNK_SIZE, data.len());
    let chunk = &data[written..chunk_end];
    
    {
        let mut writer = conn.writer();
        use std::io::Write;
        writer.write_all(chunk)
            .map_err(|e| vm.new_os_error(format!("Write failed: {e}")))?;
    }
    
    written = chunk_end;
    bytes_since_flush += chunk.len();
    
    // Flush when threshold reached or on last chunk
    if bytes_since_flush >= FLUSH_THRESHOLD || written >= data.len() {
        if conn.wants_write() {
            // ... existing flush logic ...
        }
        bytes_since_flush = 0;
    }
}

This reduces the number of flushes while still preventing buffer overflow. However, verify with benchmarks whether this optimization is beneficial.

---

4488-4513: Verify safety of unsafe pointer operations.

The code performs unsafe pointer dereferences on certificate contexts (line 4490-4493) without explicit null checks. While schannel's API should guarantee valid pointers, documenting this assumption would improve safety.

Add a safety comment or assertion:

             let cert = vm.ctx.new_bytes(c.to_der().to_owned());
             let enc_type = unsafe {
+                // Safety: schannel's CertContext::as_ptr() always returns a valid pointer
+                // to an initialized CERT_CONTEXT structure
                 let ptr = c.as_ptr() as *const Cryptography::CERT_CONTEXT;
+                debug_assert!(!ptr.is_null(), "Certificate context pointer should never be null");
                 (*ptr).dwCertEncodingType
             };

Additionally, verify with the schannel crate documentation that cert_context.as_ptr() guarantees non-null pointers.

---

4517-4569: Consider RAII wrapper to prevent resource leaks.

The function manually manages the certificate store handle with CertOpenSystemStoreW and CertCloseStore. If a panic occurs between opening and closing (e.g., during memory allocation or Python object creation), the store handle will leak.

Implement a RAII guard to ensure cleanup:

struct StoreHandle(windows_sys::Win32::Security::Cryptography::HCERTSTORE);

impl Drop for StoreHandle {
    fn drop(&mut self) {
        if !self.0.is_null() {
            unsafe { CertCloseStore(self.0, 0) };
        }
    }
}

#[cfg(windows)]
#[pyfunction]
fn enum_crls(store_name: PyStrRef, vm: &VirtualMachine) -> PyResult<Vec<PyObjectRef>> {
    use windows_sys::Win32::Security::Cryptography::{
        CRL_CONTEXT, CertCloseStore, CertEnumCRLsInStore, CertOpenSystemStoreW,
        X509_ASN_ENCODING,
    };

    let store_name_wide: Vec<u16> = store_name
        .as_str()
        .encode_utf16()
        .chain(std::iter::once(0))
        .collect();

    let store = unsafe { CertOpenSystemStoreW(0, store_name_wide.as_ptr()) };
    
    if store.is_null() {
        return Err(vm.new_os_error(format!(
            "failed to open certificate store {:?}",
            store_name.as_str()
        )));
    }
    
    let _store_guard = StoreHandle(store);  // Ensures cleanup on panic or early return
    
    // ... rest of implementation ...
}

This ensures the store is always closed, even if an exception is raised during processing.

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

crates/stdlib/src/ssl.rs (1)

4345-4352: Unresolved: Return None instead of empty strings for Windows paths.

This was flagged in a previous review but remains unchanged. CPython's ssl.get_default_verify_paths() returns None (not empty strings) when default paths don't exist on Windows.

The comment "Return empty strings to avoid None being passed to os.path.isfile()" is not the correct rationale—os.path.isfile(None) should raise TypeError when no valid path exists, which is the expected behavior.

Apply this diff to match CPython behavior:

         #[cfg(windows)]
         let (default_cafile, default_capath) = {
             // Windows uses certificate store, not file paths
-            // Return empty strings to avoid None being passed to os.path.isfile()
-            (Some(""), Some(""))
+            // Return None to match CPython behavior when no file paths exist
+            (None, None)
         };

Based on past review comments at lines 4345-4352.

crates/stdlib/src/ssl.rs (1)

4465-4514: Consider logging warnings for silently skipped certificates.

The function uses filter_map and flat_map (line 4487) which silently skips certificates that fail to process. While this may be intentional (system stores can contain unsupported formats), silently dropping certificates could hide issues during debugging.

Consider one of these optional improvements:

• Log a warning when certificates are skipped (if logging is available)
• Add a comment documenting that invalid/unsupported certificates are intentionally skipped
• Collect and return error information for troubleshooting

This is a recommended enhancement for observability, not a functional issue.

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro