Skip to content

minimize ssl lock #6376

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
youknowone merged 1 commit into from
Dec 9, 2025
Merged

minimize ssl lock #6376

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
210 changes: 117 additions & 93 deletions crates/stdlib/src/ssl.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1357,47 +1357,69 @@ mod _ssl {
);
}

// Get mutable references to store and ca_certs_der
// Parse arguments BEFORE acquiring locks to reduce lock scope
let cafile_path = if let OptionalArg::Present(Some(ref cafile_obj)) = args.cafile {
Some(Self::parse_path_arg(cafile_obj, vm)?)
} else {
None
};

let capath_dir = if let OptionalArg::Present(Some(ref capath_obj)) = args.capath {
Some(Self::parse_path_arg(capath_obj, vm)?)
} else {
None
};

let cadata_parsed = if let OptionalArg::Present(ref cadata_obj) = args.cadata
&& !vm.is_none(cadata_obj)
{
let is_string = PyStrRef::try_from_object(vm, cadata_obj.clone()).is_ok();
let data_vec = self.parse_cadata_arg(cadata_obj, vm)?;
Some((data_vec, is_string))
} else {
None
};

// Check for CRL before acquiring main locks
let (crl_opt, cafile_is_crl) = if let Some(ref path) = cafile_path {
let crl = self.load_crl_from_file(path, vm)?;
let is_crl = crl.is_some();
(crl, is_crl)
} else {
(None, false)
};

// If it's a CRL, just add it (separate lock, no conflict with root_store)
if let Some(crl) = crl_opt {
self.crls.write().push(crl);
}

// Now acquire write locks for certificate loading
let mut root_store = self.root_certs.write();
let mut ca_certs_der = self.ca_certs_der.write();

// Load from file
if let OptionalArg::Present(Some(ref cafile_obj)) = args.cafile {
let path = Self::parse_path_arg(cafile_obj, vm)?;

// Try to load as CRL first
if let Some(crl) = self.load_crl_from_file(&path, vm)? {
self.crls.write().push(crl);
} else {
// Not a CRL, load as certificate
let stats = self.load_certs_from_file_helper(
&mut root_store,
&mut ca_certs_der,
&path,
vm,
)?;
self.update_cert_stats(stats);
}
// Load from file (if not CRL)
if let Some(ref path) = cafile_path
&& !cafile_is_crl
{
// Not a CRL, load as certificate
let stats =
self.load_certs_from_file_helper(&mut root_store, &mut ca_certs_der, path, vm)?;
self.update_cert_stats(stats);
}

// Load from directory (don't add to ca_certs_der)
if let OptionalArg::Present(Some(ref capath_obj)) = args.capath {
let dir_path = Self::parse_path_arg(capath_obj, vm)?;
let stats = self.load_certs_from_dir_helper(&mut root_store, &dir_path, vm)?;
if let Some(ref dir_path) = capath_dir {
let stats = self.load_certs_from_dir_helper(&mut root_store, dir_path, vm)?;
self.update_cert_stats(stats);
}

// Load from bytes or str
if let OptionalArg::Present(cadata_obj) = args.cadata
&& !vm.is_none(&cadata_obj)
{
// Check if input is string or bytes
let is_string = PyStrRef::try_from_object(vm, cadata_obj.clone()).is_ok();
let data_vec = self.parse_cadata_arg(&cadata_obj, vm)?;
if let Some((ref data_vec, is_string)) = cadata_parsed {
let stats = self.load_certs_from_bytes_helper(
&mut root_store,
&mut ca_certs_der,
&data_vec,
data_vec,
is_string, // PEM only for strings
vm,
)?;
Expand Down Expand Up @@ -2547,48 +2569,51 @@ mod _ssl {
/// This simulates lazy loading behavior: capath certificates
/// are only added to get_ca_certs() after they're actually used in a handshake.
fn track_used_ca_from_capath(&self) -> Result<(), String> {
let context = self.context.read();
let capath_certs = context.capath_certs_der.read();

// No capath certs to track
if capath_certs.is_empty() {
return Ok(());
}

// Get peer certificate chain
let conn_guard = self.connection.lock();
let conn = conn_guard.as_ref().ok_or("No connection")?;

let peer_certs = conn.peer_certificates().ok_or("No peer certificates")?;
// Extract capath_certs, releasing context lock quickly
let capath_certs = {
let context = self.context.read();
let certs = context.capath_certs_der.read();
if certs.is_empty() {
return Ok(());
}
certs.clone()
};

if peer_certs.is_empty() {
return Ok(());
}
// Extract peer certificates, releasing connection lock quickly
let top_cert_der = {
let conn_guard = self.connection.lock();
let conn = conn_guard.as_ref().ok_or("No connection")?;
let peer_certs = conn.peer_certificates().ok_or("No peer certificates")?;
if peer_certs.is_empty() {
return Ok(());
}
peer_certs
.iter()
.map(|c| c.as_ref().to_vec())
.next_back()
.expect("is_empty checked above")
};

// Get the top certificate in the chain (closest to root)
// Note: Server usually doesn't send the root CA, so we check the last cert's issuer
let top_cert_der = peer_certs.last().unwrap();
let (_, top_cert) = x509_parser::parse_x509_certificate(top_cert_der)
let (_, top_cert) = x509_parser::parse_x509_certificate(&top_cert_der)
.map_err(|e| format!("Failed to parse top cert: {e}"))?;

let top_issuer = top_cert.issuer();

// Find matching CA in capath certs
for ca_der in capath_certs.iter() {
let (_, ca) = x509_parser::parse_x509_certificate(ca_der)
.map_err(|e| format!("Failed to parse CA: {e}"))?;
// Find matching CA in capath certs (skip unparseable certificates)
let matching_ca = capath_certs.iter().find_map(|ca_der| {
let (_, ca) = x509_parser::parse_x509_certificate(ca_der).ok()?;
// Check if this CA is self-signed (root CA) and matches the issuer
(ca.subject() == ca.issuer() && ca.subject() == top_issuer).then(|| ca_der.clone())
});

// Check if this CA is self-signed and matches the issuer
if ca.subject() == ca.issuer() // Self-signed (root CA)
&& ca.subject() == top_issuer
// Matches top cert's issuer
{
// Check if not already in ca_certs_der
let mut ca_certs_der = context.ca_certs_der.write();
if !ca_certs_der.iter().any(|c| c == ca_der) {
ca_certs_der.push(ca_der.clone());
}
break;
// Update ca_certs_der if we found a match
if let Some(ca_der) = matching_ca {
let context = self.context.read();
let mut ca_certs_der = context.ca_certs_der.write();
if !ca_certs_der.iter().any(|c| c == &ca_der) {
ca_certs_der.push(ca_der);
}
}

Expand Down Expand Up @@ -2675,6 +2700,7 @@ mod _ssl {

/// Check if SNI callback is configured
pub(crate) fn has_sni_callback(&self) -> bool {
// Nested read locks are safe
self.context.read().sni_callback.read().is_some()
}

Expand All @@ -2685,10 +2711,9 @@ mod _ssl {

/// Get the extracted SNI name from resolver
pub(crate) fn get_extracted_sni_name(&self) -> Option<String> {
self.sni_state
.read()
.as_ref()
.and_then(|arc| arc.lock().1.clone())
// Clone the Arc option to avoid nested lock (sni_state.read -> arc.lock)
let sni_state_opt = self.sni_state.read().clone();
sni_state_opt.as_ref().and_then(|arc| arc.lock().1.clone())
}

/// Invoke the Python SNI callback
Expand Down Expand Up @@ -3516,27 +3541,24 @@ mod _ssl {
return Err(vm.new_value_error("handshake not done yet"));
}

// Get peer certificates from TLS connection
let conn_guard = self.connection.lock();
let conn = conn_guard
.as_ref()
.ok_or_else(|| vm.new_value_error("No TLS connection established"))?;

let certs = conn.peer_certificates();
// Extract DER bytes from connection, releasing lock quickly
let der_bytes = {
let conn_guard = self.connection.lock();
let conn = conn_guard
.as_ref()
.ok_or_else(|| vm.new_value_error("No TLS connection established"))?;

// Return None if no peer certificate
let Some(certs) = certs else {
return Ok(None);
let Some(peer_certificates) = conn.peer_certificates() else {
return Ok(None);
};
let cert = peer_certificates
.first()
.ok_or_else(|| vm.new_value_error("No peer certificate available"))?;
cert.as_ref().to_vec()
};

// Get first certificate (peer's certificate)
let cert_der = certs
.first()
.ok_or_else(|| vm.new_value_error("No peer certificate available"))?;

if binary {
// Return DER-encoded certificate as bytes
let der_bytes = cert_der.as_ref().to_vec();
return Ok(Some(vm.ctx.new_bytes(der_bytes).into()));
}

Expand All @@ -3548,22 +3570,22 @@ mod _ssl {
return Ok(Some(vm.ctx.new_dict().into()));
}

// Parse DER certificate and convert to dict
let der_bytes = cert_der.as_ref();
let (_, cert) = x509_parser::parse_x509_certificate(der_bytes)
// Parse DER certificate and convert to dict (outside lock)
let (_, cert) = x509_parser::parse_x509_certificate(&der_bytes)
.map_err(|e| vm.new_value_error(format!("Failed to parse certificate: {e}")))?;

cert::cert_to_dict(vm, &cert).map(Some)
}

#[pymethod]
fn cipher(&self) -> Option<(String, String, i32)> {
let conn_guard = self.connection.lock();
let conn = conn_guard.as_ref()?;

let suite = conn.negotiated_cipher_suite()?;
// Extract cipher suite, releasing lock quickly
let suite = {
let conn_guard = self.connection.lock();
conn_guard.as_ref()?.negotiated_cipher_suite()?
};

// Extract cipher information using unified helper
// Extract cipher information outside the lock
let cipher_info = extract_cipher_info(&suite);

// Note: returns a 3-tuple (name, protocol_version, bits)
Expand All @@ -3577,11 +3599,13 @@ mod _ssl {

#[pymethod]
fn version(&self) -> Option<String> {
let conn_guard = self.connection.lock();
let conn = conn_guard.as_ref()?;

let suite = conn.negotiated_cipher_suite()?;
// Extract cipher suite, releasing lock quickly
let suite = {
let conn_guard = self.connection.lock();
conn_guard.as_ref()?.negotiated_cipher_suite()?
};

// Convert to string outside the lock
let version_str = match suite.version().version {
rustls::ProtocolVersion::TLSv1_2 => "TLSv1.2",
rustls::ProtocolVersion::TLSv1_3 => "TLSv1.3",
Expand Down
Loading