stdlib/src/ssl.rs (2)

75-82: Unused import of PySSLCertificate in _ssl

This re-import is not needed for exposure; ssl_cert already registers the class via #[pyclass]. Keeping it requires allow(unused_imports). Remove for cleanliness.

-    // Re-export PySSLCertificate to make it available in the _ssl module
-    // It will be automatically exposed to Python via #[pyclass]
-    #[allow(unused_imports)]
-    use super::cert::PySSLCertificate;
+    // PySSLCertificate is registered by cert::ssl_cert; no import needed here.

---

188-195: Expose and support ENCODING_PEM_AUX consistently

ENCODING_PEM_AUX exists here but is private and unused. To match CPython’s accepted encodings and to let Certificate.public_bytes() accept PEM_AUX, make it pub(crate) and handle it in cert.rs as an alias of PEM.

-    #[pyattr]
-    const ENCODING_PEM_AUX: i32 = sys::X509_FILETYPE_PEM + 0x100;
+    #[pyattr]
+    pub(crate) const ENCODING_PEM_AUX: i32 = sys::X509_FILETYPE_PEM + 0x100;

Follow-up diff for cert.rs is included in that file’s comment.

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

24-24: Import ENCODING_PEM_AUX and accept it in public_bytes()

You added ENCODING_PEM_AUX in _ssl; mirror CPython by treating it like PEM. Also make the constant pub(crate) in ssl.rs.

-    use crate::ssl::_ssl::{ENCODING_DER, ENCODING_PEM, convert_openssl_error};
+    use crate::ssl::_ssl::{ENCODING_DER, ENCODING_PEM, ENCODING_PEM_AUX, convert_openssl_error};

And in public_bytes:

-            match format {
+            match format {
                 x if x == ENCODING_DER => {
                     // DER encoding
                     let der = self
                         .cert
                         .to_der()
                         .map_err(|e| convert_openssl_error(vm, e))?;
                     Ok(vm.ctx.new_bytes(der).into())
                 }
-                x if x == ENCODING_PEM => {
+                x if x == ENCODING_PEM || x == ENCODING_PEM_AUX => {
                     // PEM encoding
                     let pem = self
                         .cert
                         .to_pem()
                         .map_err(|e| convert_openssl_error(vm, e))?;
                     Ok(vm.ctx.new_bytes(pem).into())
                 }
                 _ => Err(vm.new_value_error("Unsupported format".to_owned())),
             }

---

157-187: Prefer std::net for SAN IP formatting

Manual IPv6 formatting is non‑canonical (uppercase, no compression). Use Ipv4Addr/Ipv6Addr for correct presentation.

-                    } else if let Some(ip) = gen_name.ipaddress() {
-                        // Parse IP address properly (IPv4 or IPv6)
-                        let ip_str = if ip.len() == 4 {
-                            // IPv4
-                            format!("{}.{}.{}.{}", ip[0], ip[1], ip[2], ip[3])
-                        } else if ip.len() == 16 {
-                            // IPv6 - format like: "X:X:X:X:X:X:X:X" (not compressed)
-                            format!(
-                                "{:X}:{:X}:{:X}:{:X}:{:X}:{:X}:{:X}:{:X}",
-                                (ip[0] as u16) << 8 | ip[1] as u16,
-                                (ip[2] as u16) << 8 | ip[3] as u16,
-                                (ip[4] as u16) << 8 | ip[5] as u16,
-                                (ip[6] as u16) << 8 | ip[7] as u16,
-                                (ip[8] as u16) << 8 | ip[9] as u16,
-                                (ip[10] as u16) << 8 | ip[11] as u16,
-                                (ip[12] as u16) << 8 | ip[13] as u16,
-                                (ip[14] as u16) << 8 | ip[15] as u16
-                            )
-                        } else {
-                            // Fallback for unexpected length
-                            String::from_utf8_lossy(ip).into_owned()
-                        };
-                        vm.new_tuple((ascii!("IP Address"), ip_str)).into()
+                    } else if let Some(ip) = gen_name.ipaddress() {
+                        use std::net::{IpAddr, Ipv4Addr, Ipv6Addr};
+                        let ip_str = match ip.len() {
+                            4 => IpAddr::V4(Ipv4Addr::new(ip[0], ip[1], ip[2], ip[3])).to_string(),
+                            16 => {
+                                let mut seg = [0u16; 8];
+                                for i in 0..8 { seg[i] = u16::from_be_bytes([ip[2*i], ip[2*i+1]]); }
+                                IpAddr::V6(Ipv6Addr::from(seg)).to_string()
+                            }
+                            _ => String::from_utf8_lossy(ip).into_owned(),
+                        };
+                        vm.new_tuple((ascii!("IP Address"), ip_str)).into()

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

stdlib/src/ssl.rs (2)

1220-1231: Propagate certificate conversion errors instead of silently dropping certificates.

The code still swallows errors with .ok(), which can mask VM failures and yield incomplete chains. This matches the unresolved past review comment.

Apply this diff to propagate errors:

-        fn get_unverified_chain(&self, vm: &VirtualMachine) -> Option<PyObjectRef> {
+        fn get_unverified_chain(&self, vm: &VirtualMachine) -> PyResult<Option<PyObjectRef>> {
             let stream = self.stream.read();
             let chain = stream.ssl().peer_cert_chain()?;
 
-            // Return Certificate objects
-            let certs: Vec<PyObjectRef> = chain
-                .iter()
-                .filter_map(|cert| {
-                    // Clone the X509 certificate to create an owned copy
-                    unsafe {
-                        sys::X509_up_ref(cert.as_ptr());
-                        let owned_cert = X509::from_ptr(cert.as_ptr());
-                        cert_to_certificate(vm, owned_cert).ok()
-                    }
-                })
-                .collect();
-
-            Some(vm.ctx.new_list(certs).into())
+            let certs: Vec<PyObjectRef> = chain
+                .iter()
+                .map(|cert| unsafe {
+                    sys::X509_up_ref(cert.as_ptr());
+                    let owned_cert = X509::from_ptr(cert.as_ptr());
+                    cert_to_certificate(vm, owned_cert)
+                })
+                .collect::<PyResult<_>>()?;
+            Ok(Some(vm.ctx.new_list(certs).into()))
         }

---

1248-1259: Propagate certificate conversion errors in verified chain construction.

The code still swallows errors with if let Ok(), which can silently drop certificates. This matches the unresolved past review comment.

Apply this diff to propagate errors:

-        fn get_verified_chain(&self, vm: &VirtualMachine) -> Option<PyListRef> {
+        fn get_verified_chain(&self, vm: &VirtualMachine) -> PyResult<Option<PyListRef>> {
             let stream = self.stream.read();
             unsafe {
                 let chain = sys::SSL_get0_verified_chain(stream.ssl().as_ptr());
                 if chain.is_null() {
-                    return None;
+                    return Ok(None);
                 }
 
                 let num_certs = sys::OPENSSL_sk_num(chain as *const _);
-                let mut certs = Vec::new();
+                let mut certs: Vec<PyObjectRef> = Vec::with_capacity(num_certs as usize);
 
-                // Return Certificate objects
                 for i in 0..num_certs {
                     let cert_ptr = sys::OPENSSL_sk_value(chain as *const _, i) as *mut sys::X509;
                     if cert_ptr.is_null() {
                         continue;
                     }
-                    // Clone the X509 certificate to create an owned copy
                     sys::X509_up_ref(cert_ptr);
                     let owned_cert = X509::from_ptr(cert_ptr);
-                    if let Ok(cert_obj) = cert_to_certificate(vm, owned_cert) {
-                        certs.push(cert_obj);
-                    }
+                    let cert_obj = cert_to_certificate(vm, owned_cert)?;
+                    certs.push(cert_obj);
                 }
 
                 if certs.is_empty() {
-                    None
+                    Ok(None)
                 } else {
-                    Some(vm.ctx.new_list(certs))
+                    Ok(Some(vm.ctx.new_list(certs)))
                 }
             }
         }

stdlib/src/ssl.rs (1)

2073-2102: Replace magic numbers with named constants and use consistent exception class access.

The code uses magic numbers 20 and 294 for OpenSSL error codes, and inconsistently retrieves the SSLEOFError class via vm.class("_ssl", "SSLEOFError") instead of PySslEOFError::class(&vm.ctx).

Define constants for the error codes and use the consistent exception class pattern:

+    // OpenSSL 3.0 error codes
+    const ERR_LIB_SSL: u32 = 20;
+    const SSL_R_UNEXPECTED_EOF_WHILE_READING: u32 = 294;
+
     #[track_caller]
     fn convert_ssl_error(
         vm: &VirtualMachine,
         e: impl std::borrow::Borrow<ssl::Error>,
     ) -> PyBaseExceptionRef {
         let e = e.borrow();
         let (cls, msg) = match e.code() {
             // ... existing code ...
             ssl::ErrorCode::SSL => {
-                // Check for OpenSSL 3.0 SSL_R_UNEXPECTED_EOF_WHILE_READING
                 if let Some(ssl_err) = e.ssl_error() {
-                    // SSL_R_UNEXPECTED_EOF_WHILE_READING = 294 (0x126)
-                    // In OpenSSL 3.0+, unexpected EOF is reported as SSL_ERROR_SSL
-                    // with this specific reason code instead of SSL_ERROR_SYSCALL
                     unsafe {
                         let err_code = sys::ERR_peek_last_error();
                         let reason = sys::ERR_GET_REASON(err_code);
                         let lib = sys::ERR_GET_LIB(err_code);
-                        // ERR_LIB_SSL = 20, SSL_R_UNEXPECTED_EOF_WHILE_READING = 294
-                        if lib == 20 && reason == 294 {
+                        if lib == ERR_LIB_SSL && reason == SSL_R_UNEXPECTED_EOF_WHILE_READING {
                             return vm.new_exception(
-                                vm.class("_ssl", "SSLEOFError"),
+                                PySslEOFError::class(&vm.ctx).to_owned(),
                                 vec![
                                     vm.ctx.new_int(SSL_ERROR_EOF).into(),
                                     vm.ctx
                                         .new_str("EOF occurred in violation of protocol")
                                         .into(),
                                 ],
                             );
                         }
                     }
                     return convert_openssl_error(vm, ssl_err.clone());
                 }

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

68-95: Consider handling ENCODING_PEM_AUX or documenting why it's unsupported.

The parent module exposes ENCODING_PEM_AUX as a constant, but this method doesn't handle it. Either implement PEM_AUX encoding support or document the limitation.

If PEM_AUX support is intended:

                 x if x == ENCODING_PEM => {
                     // PEM encoding
                     let pem = self
                         .cert
                         .to_pem()
                         .map_err(|e| convert_openssl_error(vm, e))?;
                     Ok(vm.ctx.new_bytes(pem).into())
                 }
+                x if x == super::_ssl::ENCODING_PEM_AUX => {
+                    // PEM encoding with auxiliary info
+                    // Note: X509::to_pem() doesn't support AUX format
+                    // Would need to use d2i_X509_AUX_bio or similar
+                    Err(vm.new_not_implemented_error(
+                        "ENCODING_PEM_AUX is not yet supported".to_owned()
+                    ))
+                }
                 _ => Err(vm.new_value_error("Unsupported format".to_owned())),

---

167-186: Consider using std::net::Ipv6Addr for simpler IPv6 formatting.

The manual bit-shifting works but is verbose. Using the standard library could improve readability.

                         let ip_str = if ip.len() == 4 {
                             // IPv4
                             format!("{}.{}.{}.{}", ip[0], ip[1], ip[2], ip[3])
                         } else if ip.len() == 16 {
-                            // IPv6 - format like: "X:X:X:X:X:X:X:X" (not compressed)
-                            format!(
-                                "{:X}:{:X}:{:X}:{:X}:{:X}:{:X}:{:X}:{:X}",
-                                (ip[0] as u16) << 8 | ip[1] as u16,
-                                (ip[2] as u16) << 8 | ip[3] as u16,
-                                (ip[4] as u16) << 8 | ip[5] as u16,
-                                (ip[6] as u16) << 8 | ip[7] as u16,
-                                (ip[8] as u16) << 8 | ip[9] as u16,
-                                (ip[10] as u16) << 8 | ip[11] as u16,
-                                (ip[12] as u16) << 8 | ip[13] as u16,
-                                (ip[14] as u16) << 8 | ip[15] as u16
-                            )
+                            // IPv6
+                            let octets: [u8; 16] = ip.try_into().expect("length checked");
+                            std::net::Ipv6Addr::from(octets).to_string()
                         } else {
                             // Fallback for unexpected length
                             String::from_utf8_lossy(ip).into_owned()
                         };

Note: If CPython compatibility requires uppercase hex without compression, keep the current implementation.

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

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

107-128: Extract duplicate name_to_py closure to reduce code duplication.

The name_to_py closure is defined identically at lines 107-128 and 195-218, violating DRY principles.

Also applies to: 195-218

stdlib/src/ssl.rs (1)

1220-1234: Propagate errors instead of swallowing them with .ok().

Errors from cert_to_certificate are silently dropped with .ok(), which can mask allocation or VM failures and yield truncated chains. This was flagged in a previous review but not yet addressed.

Change the function signature to return PyResult<Option<PyObjectRef>> and propagate errors:

-        fn get_unverified_chain(&self, vm: &VirtualMachine) -> Option<PyObjectRef> {
+        fn get_unverified_chain(&self, vm: &VirtualMachine) -> PyResult<Option<PyObjectRef>> {
             let stream = self.stream.read();
             let chain = stream.ssl().peer_cert_chain()?;
 
-            // Return Certificate objects
-            let certs: Vec<PyObjectRef> = chain
-                .iter()
-                .filter_map(|cert| {
-                    // Clone the X509 certificate to create an owned copy
-                    unsafe {
-                        sys::X509_up_ref(cert.as_ptr());
-                        let owned_cert = X509::from_ptr(cert.as_ptr());
-                        cert_to_certificate(vm, owned_cert).ok()
-                    }
-                })
-                .collect();
-
-            Some(vm.ctx.new_list(certs).into())
+            let certs: Vec<PyObjectRef> = chain
+                .iter()
+                .map(|cert| unsafe {
+                    sys::X509_up_ref(cert.as_ptr());
+                    let owned = X509::from_ptr(cert.as_ptr());
+                    cert_to_certificate(vm, owned)
+                })
+                .collect::<PyResult<_>>()?;
+            Ok(Some(vm.ctx.new_list(certs).into()))
         }

stdlib/src/ssl.rs (1)

2072-2101: Consider defining constants for OpenSSL error codes.

The magic numbers 20 (ERR_LIB_SSL) and 294 (SSL_R_UNEXPECTED_EOF_WHILE_READING) are hardcoded but well-documented in comments. For better maintainability, consider defining them as constants.

Example:

const ERR_LIB_SSL: u32 = 20;
const SSL_R_UNEXPECTED_EOF_WHILE_READING: u32 = 294;

// Then use in the condition:
if lib == ERR_LIB_SSL && reason == SSL_R_UNEXPECTED_EOF_WHILE_READING {
    // ...
}

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

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

26-51: Consider defensive error handling over assertions.

The function uses assert! for OpenSSL error conditions (lines 31, 43). While OpenSSL should not return errors for these calls in normal operation, replacing assertions with proper error returns would be more defensive and prevent potential panics.

Consider returning Result<String, ErrorStack> and propagating errors:

pub(crate) fn obj2txt(obj: &Asn1ObjectRef, no_name: bool) -> Result<String, ErrorStack> {
    let no_name = i32::from(no_name);
    let ptr = obj.as_ptr();
    let b = unsafe {
        let buflen = sys::OBJ_obj2txt(std::ptr::null_mut(), 0, ptr, no_name);
        if buflen <= 0 {
            return Err(ErrorStack::get());
        }
        // ... rest of implementation
    };
    // ...
}

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro