More openssl impl - #6464
Conversation
Sorry, something went wrong.
|
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)
📜 Recent review details Configuration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro 📥 CommitsReviewing files that changed from the base of the PR and between 7bcb91e and 57c3f4e. 📒 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. ❤️ ShareComment @coderabbitai help to get the list of available commands and usage tips. |
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)crates/stdlib/src/openssl/cert.rs (1)📜 Review details108-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_eq is 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))
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📥 CommitsReviewing files that changed from the base of the PR and between 6660170 and 7bcb91e.
📒 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)
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style by running cargo fmt to 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.rs
- crates/stdlib/src/openssl.rs
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
crates/stdlib/src/openssl.rs (4)⏰ 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)
- convert_openssl_error (3664-3767)
- new (537-541)
- new (4093-4103)
- value (3025-3026)
- 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
.github/workflows/ci.yaml (1)crates/stdlib/src/openssl/cert.rs (6)150-154: LGTM - OpenSSL build verification step.
This CI step ensures the ssl-openssl feature compiles correctly on Linux. The restriction to Linux is appropriate given cross-platform OpenSSL complexities.
crates/stdlib/src/openssl.rs (11)137-143: LGTM - Hashable implementation.
Using X509_subject_name_hash is 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 ssl module 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 ssl module output.
260-276: Union field access for GEN_RID is implemented correctly.
The code properly accesses the GENERAL_NAME union field d for GEN_RID type. After verifying the type with (*ptr).type_ == sys::GEN_RID, it correctly casts (*ptr).d to *const sys::ASN1_OBJECT. The implementation includes proper null checking and uses the standard OpenSSL obj2txt() conversion function. The union access pattern is safe and follows C FFI conventions for discriminated union handling.
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 on msg_callback_data_free to 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_chain excludes 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_ca returns non-zero for any CA certificate type (not just 1), so the != 0 check 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 vm and pw_obj by 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 via RefCell is correctly implemented.
Sorry, something went wrong.
| #[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.
⚠️ Potential issue | 🔴 Critical
🧩 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 AgentsIn 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.
Sorry, something went wrong.
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.