More openssl impl #6464
More openssl impl #6464
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds OpenSSL-specific CI build step and extensive OpenSSL/ssl changes: PSK client/server callback storage and APIs, callable password support for certificate loading, stricter DER/PEM handling, certificate trait/visibility updates, ALPN and select/handshake refinements, and related ex_data lifecycle logic. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/stdlib/src/openssl/cert.rs (1)
108-134: Equality comparison logic uses unusual pattern.The comparison at line 133 uses
eq.cmp(&true)which is an unusual way to express boolean equality. While it works, it's less readable than a direct approach.🔎 Suggested simplification
- let eq = self_der == other_der; - Ok(op.eval_ord(eq.cmp(&true)).into()) + let result = self_der == other_der; + Ok(PyComparisonValue::Implemented(op.eval_eq(result)))Alternatively, if
eval_eqis not available, consider:- Ok(op.eval_ord(eq.cmp(&true)).into()) + let matches = match op { + PyComparisonOp::Eq => result, + PyComparisonOp::Ne => !result, + _ => unreachable!(), + }; + Ok(PyComparisonValue::Implemented(matches))
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/ci.yaml(1 hunks)crates/stdlib/src/openssl.rs(24 hunks)crates/stdlib/src/openssl/cert.rs(8 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style by runningcargo fmtto format Rust code
Always run clippy to lint Rust code (cargo clippy) before completing tasks and fix any warnings or lints introduced by changes
Follow Rust best practices for error handling and memory management
Use the macro system (pyclass,pymodule,pyfunction, etc.) when implementing Python functionality in Rust
Files:
crates/stdlib/src/openssl/cert.rscrates/stdlib/src/openssl.rs
🧠 Learnings (1)
📚 Learning: 2025-11-29T12:17:28.606Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-29T12:17:28.606Z
Learning: Run `cargo test --workspace --exclude rustpython_wasm` to execute Rust unit tests
Applied to files:
.github/workflows/ci.yaml
🧬 Code graph analysis (1)
crates/stdlib/src/openssl/cert.rs (1)
crates/stdlib/src/openssl.rs (4)
convert_openssl_error(3664-3767)new(537-541)new(4093-4103)value(3025-3026)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run tests under miri
- GitHub Check: Check the WASM package and demo
- GitHub Check: Check Rust code with clippy
🔇 Additional comments (18)
.github/workflows/ci.yaml (1)
150-154: LGTM - OpenSSL build verification step.This CI step ensures the
ssl-opensslfeature compiles correctly on Linux. The restriction to Linux is appropriate given cross-platform OpenSSL complexities.crates/stdlib/src/openssl/cert.rs (6)
137-143: LGTM - Hashable implementation.Using
X509_subject_name_hashis a reasonable approach for certificate hashing, consistent with how OpenSSL identifies certificates.
145-166: LGTM - Representable implementation.The representation logic correctly handles edge cases like missing short names and non-UTF8 data, providing a readable certificate description.
88-97: LGTM - PEM encoding returns string.Returning PEM as a string aligns with CPython's
sslmodule behavior where PEM-encoded certificates are text.
201-209: LGTM - Serial number even-length padding.Ensuring the serial number hex string has even length (byte-aligned) matches CPython's certificate representation.
287-339: LGTM - Certificate extension fields.The additions for OCSP responders, CA Issuers, and CRL distribution points correctly populate the certificate dictionary, matching CPython's
sslmodule output.
260-276: Union field access for GEN_RID is implemented correctly.The code properly accesses the GENERAL_NAME union field
dfor GEN_RID type. After verifying the type with(*ptr).type_ == sys::GEN_RID, it correctly casts(*ptr).dto*const sys::ASN1_OBJECT. The implementation includes proper null checking and uses the standard OpenSSLobj2txt()conversion function. The union access pattern is safe and follows C FFI conventions for discriminated union handling.crates/stdlib/src/openssl.rs (11)
605-636: LGTM - Message callback ex_data management.The dedicated ex_data index for msg_callback and the free function properly manage the Python object reference lifecycle, preventing memory leaks and use-after-free issues.
1126-1146: LGTM - Options update with clear/set delta.The logic correctly computes which options to clear and set, ensuring the final options match the requested value rather than just ORing new options.
1367-1373: LGTM - ALPN protocol slice bounds fix.Using
pos..pos + proto.len()correctly returns only the selected protocol bytes, preventing potential buffer overread.
1485-1500: LGTM - Empty cadata error handling.The differentiated error messages for PEM vs DER format help users understand why their certificate data wasn't loaded.
1960-1977: LGTM - Password bytes extraction helper.The helper correctly handles all expected password types (str, bytes, bytearray) with clear error messaging.
2117-2134: LGTM - ex_data setup with proper reference counting.The pattern of cloning, calling
into_raw(), and relying onmsg_callback_data_freeto decrement the refcount correctly manages the Python object lifecycle.
2555-2578: LGTM - Server-side peer cert chain fix.Correctly handles the OpenSSL behavior where
SSL_get_peer_cert_chainexcludes the peer certificate for server-side sockets, matching CPython's behavior.
2844-2880: LGTM - DER certificate loading with improved EOF detection.The enhanced logic correctly distinguishes between:
- Clean EOF (all data consumed as valid certificates)
- Parse error (garbage data after valid certificates)
- Empty/invalid data
This matches CPython's certificate loading behavior.
1599-1601: LGTM - X509_check_ca return value fix.
X509_check_careturns non-zero for any CA certificate type (not just 1), so the!= 0check is correct.
886-889: LGTM - PSK callback field additions.The new fields for PSK callbacks and identity hint are properly initialized with
PyMutex<Option<_>>for thread-safe optional storage.
1872-1921: Password callback implementation is safe.The callback closure captures
vmandpw_objby reference within a synchronous operation. OpenSSL's PEM_read_bio_PrivateKey invokes the callback immediately during the read operation to request the password, and the closure is not stored for later use. The references remain valid throughout the function's execution, and error propagation viaRefCellis correctly implemented.
| #[pymethod] | ||
| fn set_psk_client_callback( | ||
| &self, | ||
| callback: PyObjectRef, | ||
| vm: &VirtualMachine, | ||
| ) -> PyResult<()> { | ||
| // Cannot add PSK client callback to a server context | ||
| if self.protocol == SslVersion::TlsServer { | ||
| return Err(vm | ||
| .new_os_subtype_error( | ||
| PySSLError::class(&vm.ctx).to_owned(), | ||
| None, | ||
| "Cannot add PSK client callback to a PROTOCOL_TLS_SERVER context" | ||
| .to_owned(), | ||
| ) | ||
| .upcast()); | ||
| } | ||
|
|
||
| if vm.is_none(&callback) { | ||
| *self.psk_client_callback.lock() = None; | ||
| unsafe { | ||
| sys::SSL_CTX_set_psk_client_callback(self.builder().as_ptr(), None); | ||
| } | ||
| } else { | ||
| if !callback.is_callable() { | ||
| return Err(vm.new_type_error("callback must be callable".to_owned())); | ||
| } | ||
| *self.psk_client_callback.lock() = Some(callback); | ||
| // Note: The actual callback will be invoked via SSL app_data mechanism | ||
| // when do_handshake is called | ||
| } | ||
| Ok(()) | ||
| } | ||
|
|
||
| #[pymethod] | ||
| fn set_psk_server_callback( | ||
| &self, | ||
| callback: PyObjectRef, | ||
| identity_hint: OptionalArg<PyStrRef>, | ||
| vm: &VirtualMachine, | ||
| ) -> PyResult<()> { | ||
| // Cannot add PSK server callback to a client context | ||
| if self.protocol == SslVersion::TlsClient { | ||
| return Err(vm | ||
| .new_os_subtype_error( | ||
| PySSLError::class(&vm.ctx).to_owned(), | ||
| None, | ||
| "Cannot add PSK server callback to a PROTOCOL_TLS_CLIENT context" | ||
| .to_owned(), | ||
| ) | ||
| .upcast()); | ||
| } | ||
|
|
||
| if vm.is_none(&callback) { | ||
| *self.psk_server_callback.lock() = None; | ||
| *self.psk_identity_hint.lock() = None; | ||
| unsafe { | ||
| sys::SSL_CTX_set_psk_server_callback(self.builder().as_ptr(), None); | ||
| } | ||
| } else { | ||
| if !callback.is_callable() { | ||
| return Err(vm.new_type_error("callback must be callable".to_owned())); | ||
| } | ||
| *self.psk_server_callback.lock() = Some(callback); | ||
| if let OptionalArg::Present(hint) = identity_hint { | ||
| *self.psk_identity_hint.lock() = Some(hint.as_str().to_owned()); | ||
| } | ||
| // Note: The actual callback will be invoked via SSL app_data mechanism | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for where psk_client_callback and psk_server_callback are actually used
rg -n "psk_client_callback|psk_server_callback" --type rust -B 3 -A 3 crates/stdlib/src/openssl.rsRepository: RustPython/RustPython
Length of output: 2720
🏁 Script executed:
#!/bin/bash
# Search for do_handshake implementation and any usage of stored callbacks
rg -n "do_handshake|psk_client_callback\|psk_server_callback" --type rust crates/stdlib/src/openssl.rs | head -50Repository: RustPython/RustPython
Length of output: 820
🏁 Script executed:
#!/bin/bash
# Get the do_handshake implementation around line 2866
sed -n '2860,2920p' crates/stdlib/src/openssl.rsRepository: RustPython/RustPython
Length of output: 2828
🏁 Script executed:
#!/bin/bash
# Search for where psk_client_callback and psk_server_callback are USED (not just stored)
# Look for any invocations or retrievals of these stored callbacks
rg -n "psk_client_callback\|psk_server_callback" --type rust crates/stdlib/src/openssl.rs | grep -v "fn set_psk" | grep -v "\.lock()"Repository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Search more broadly for callback usage and wrapper functions
rg -n "psk_client_callback|psk_server_callback|raw_client_psk|raw_server_psk" --type rust crates/stdlib/src/openssl.rsRepository: RustPython/RustPython
Length of output: 852
🏁 Script executed:
#!/bin/bash
# Check if there are any wrapper functions defined for PSK callbacks
rg -n "extern.*psk|fn.*psk.*callback|unsafe.*psk" --type rust crates/stdlib/src/openssl.rs | head -20Repository: RustPython/RustPython
Length of output: 146
🏁 Script executed:
#!/bin/bash
# Final check: see if callbacks are ever retrieved from the lock anywhere
rg -n "\.psk_client_callback\|\.psk_server_callback" --type rust crates/stdlib/src/openssl.rsRepository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Confirm: search for any actual invocation/retrieval of these callbacks
rg -n "lock()" crates/stdlib/src/openssl.rs | grep -i pskRepository: RustPython/RustPython
Length of output: 482
PSK callbacks stored but not registered with OpenSSL—no callback wrapper function is invoked.
The methods store Python callbacks in psk_client_callback/psk_server_callback, but fail to register C-compatible wrapper functions with OpenSSL. When a non-None callback is set, the code should call sys::SSL_CTX_set_psk_client_callback(self.builder().as_ptr(), Some(wrapper_fn)) to register an actual C callback. Currently this only happens when clearing (setting to None). The stored callbacks are never retrieved or invoked—they're unreachable from both OpenSSL and the handshake code. Either implement the wrapper function and OpenSSL registration, or clarify if this is an intentional partial implementation pending follow-up work.
🤖 Prompt for AI Agents
In crates/stdlib/src/openssl.rs around lines 1384-1454, the PSK client/server
setters store Python callbacks but do not register C-compatible wrapper
functions with OpenSSL, so OpenSSL cannot call into the stored Python callbacks;
fix by implementing extern "C" wrapper functions for client and server PSK
callbacks that retrieve the Rust context (via SSL_CTX app_data or ex_data),
acquire the GIL, call the stored PyObject callback and translate return values
into the OpenSSL-required types (handle errors by returning appropriate failure
codes), and when setting a non-None callback call
sys::SSL_CTX_set_psk_client_callback(self.builder().as_ptr(),
Some(psk_client_wrapper)) or sys::SSL_CTX_set_psk_server_callback(...,
Some(psk_server_wrapper)); also ensure you set the context pointer in OpenSSL
(app_data/ex_data) so the wrapper can find self, and when clearing (None)
unregister the callback by passing None and clear the stored app_data/ex_data;
include proper locking, lifetime and error handling around the Python call.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.