ssl module for windows by youknowone · Pull Request #6332 · RustPython/RustPython

youknowone

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.