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)
✅ 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 |
Sorry, something went wrong.
![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.
Sorry, something went wrong.
youknowone
merged commit 898fe85
into
RustPython:main
Dec 20, 2025
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.