[DO NOT MERGE] Almost complete rustls integration rewrite#8007
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR implements a comprehensive TLS migration from OpenSSL to Rustls with AWS-LC/FIPS support, adds Rust-to-Python Serde serialization infrastructure with module-loaded hooks, includes cryptographic OID reference data, and performs socket module cleanup. ChangesTLS/Rustls Migration with Error Handling & Provider Support
VM Serde Infrastructure and Module-Loaded Hooks
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [x] lib: cpython/Lib/ssl.py dependencies:
dependent tests: (51 tests)
[x] lib: cpython/Lib/asyncio dependencies:
dependent tests: (5 tests)
[x] lib: cpython/Lib/http dependencies:
dependent tests: (47 tests)
Legend:
|
|
I wonder if @coderabbitai will review a PR in draft status if @coderabbitai is tagged to review it. |
|
By default, CodeRabbit skips automatic reviews on draft PRs, but an explicit mention overrides that behavior. You can also configure this via the reviews:
auto_review:
drafts: trueWould you like me to go ahead and review this PR now? ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
crates/stdlib/src/ssl/compat.rs (2)
368-381: ⚡ Quick winUsing
.expect()on fallible operations in error handling path.Lines 369-370 use
.expect("unlikely to happen")oncreate_pem_ssl_error. While attribute-setting failures are indeed rare (typically only on OOM), panicking in an error-handling path is undesirable. Consider using.unwrap_or_else()to fall back to a simpler error without attributes, matching the pattern used elsewhere in this file (e.g., line 386).♻️ Proposed fix to avoid panic in error path
- Self::PemLib(msg) => Self::create_pem_ssl_error(vm, format!("PEM lib: {msg}")) - .expect("unlikely to happen"), + Self::PemLib(msg) => Self::create_pem_ssl_error(vm, format!("PEM lib: {msg}")) + .unwrap_or_else(|_| Self::create_plain_ssl_error(vm, format!("PEM lib: {msg}"))),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/stdlib/src/ssl/compat.rs` around lines 368 - 381, The PemLib arm panics by calling .expect(...) on the fallible create_pem_ssl_error; change it to handle failure like other arms by using unwrap_or_else to fall back to a simpler error (e.g., call Self::create_plain_ssl_error with a "PEM lib: {msg}" message) so create_pem_ssl_error(vm, format!("PEM lib: {msg}")).unwrap_or_else(|_| Self::create_plain_ssl_error(vm, format!("PEM lib: {msg}"))). Ensure you update the Self::PemLib match arm to use unwrap_or_else and reference create_pem_ssl_error and create_plain_ssl_error accordingly.
225-252: 💤 Low valueAudit
rustls::PeerIncompatiblecoverage vs rustls 0.23 variants (crates/stdlib/src/ssl/compat.rs,from_rustlsmatch ~225-252)The mapping only covers some
rustls::PeerIncompatiblevariants in rustls 0.23; the_ =>arm currently handles other existing variants (e.g.,ExtendedMasterSecretExtensionRequired,IncorrectCertificateTypeExtension,NullCompressionRequired,ServerSentHelloRetryRequestWithUnknownExtension,UnsolicitedCertificateTypeExtension,ServerRejectedEncryptedClientHello(...)) by falling back toSelf::Ssl(format!("peer is incompatible: {peer_err:?}"))rather than an OpenSSL reason code. Add explicit arms for the remaining variants or document that this generic fallback is intentional (since the enum is non-exhaustive).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/stdlib/src/ssl/compat.rs` around lines 225 - 252, The match over rustls::PeerIncompatible in from_rustls currently falls through to a generic Ssl(...) fallback for many 0.23 variants; update the match to explicitly handle the remaining PeerIncompatible variants (e.g., ExtendedMasterSecretExtensionRequired, IncorrectCertificateTypeExtension, NullCompressionRequired, ServerSentHelloRetryRequestWithUnknownExtension, UnsolicitedCertificateTypeExtension, ServerRejectedEncryptedClientHello, etc.) and map each to the appropriate OpenSSL reason constant using Self::OpenSslReason(SSL_R_...) where sensible, or if you intentionally want a generic fallback keep the fallback but add a comment documenting that the enum is non_exhaustive and the generic Self::Ssl(format!(...)) is deliberate; touch the match in from_rustls so all current PeerIncompatible variants are either explicitly mapped to an SSL_R_* constant (using the same style as existing arms) or a documented fallback is left.crates/vm/src/vm/mod.rs (1)
2378-2401: ⚡ Quick winCover the first-load hook path too.
This test only exercises the
sys.modulescache-hit case becausesysis already imported before Line 2383. The downstreamsslhook incrates/stdlib/src/rustls.rs:164-177is registered during module execution and depends on the initial_find_and_loadpath, so a regression there would currently slip through.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/vm/src/vm/mod.rs` around lines 2378 - 2401, The test only covers the cache-hit path; update module_loaded_hook_can_patch_imported_module to also exercise the initial load path by registering the hook (mark_module_loaded) before ensuring the target module is not present in sys.modules and then importing it so the interpreter takes the _find_and_load path; specifically, in the vm.enter closure call vm.register_module_loaded_hook("sys", mark_module_loaded) then remove the "sys" entry from sys.modules (via the VM sys module/object APIs) or choose a fresh module name that is guaranteed not to be pre-imported, then call vm.import(...) to trigger the initial-load hook and assert the "__rustpython_module_loaded_hook_ran__" attribute as the existing test does (referencing module_loaded_hook_can_patch_imported_module, mark_module_loaded, vm.register_module_loaded_hook, and vm.import).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/vm/src/convert/rust_py_serde.rs`:
- Around line 13-17: The code currently uses unimplemented!() for unit and
unit-struct handling inside RustPySerDe which causes a panic; instead implement
proper Serde handling that returns a Result (so errors are converted to Python
exceptions via with_serde_conf()) and treat unit/unit-struct as Python None: in
the serializer paths for unit/unit_struct return Ok(vm.ctx.none().into()) (or
the equivalent PyObject/Value representing None) and in the deserializer/visitor
paths accept Python None and return Ok(()) (or the appropriate unit value)
rather than panicking; update the functions/match arms that reference
unit/unit_struct in RustPySerDe so they return Result-based values and use the
existing with_serde_conf/error helpers to map failures to Python exceptions.
---
Nitpick comments:
In `@crates/stdlib/src/ssl/compat.rs`:
- Around line 368-381: The PemLib arm panics by calling .expect(...) on the
fallible create_pem_ssl_error; change it to handle failure like other arms by
using unwrap_or_else to fall back to a simpler error (e.g., call
Self::create_plain_ssl_error with a "PEM lib: {msg}" message) so
create_pem_ssl_error(vm, format!("PEM lib: {msg}")).unwrap_or_else(|_|
Self::create_plain_ssl_error(vm, format!("PEM lib: {msg}"))). Ensure you update
the Self::PemLib match arm to use unwrap_or_else and reference
create_pem_ssl_error and create_plain_ssl_error accordingly.
- Around line 225-252: The match over rustls::PeerIncompatible in from_rustls
currently falls through to a generic Ssl(...) fallback for many 0.23 variants;
update the match to explicitly handle the remaining PeerIncompatible variants
(e.g., ExtendedMasterSecretExtensionRequired, IncorrectCertificateTypeExtension,
NullCompressionRequired, ServerSentHelloRetryRequestWithUnknownExtension,
UnsolicitedCertificateTypeExtension, ServerRejectedEncryptedClientHello, etc.)
and map each to the appropriate OpenSSL reason constant using
Self::OpenSslReason(SSL_R_...) where sensible, or if you intentionally want a
generic fallback keep the fallback but add a comment documenting that the enum
is non_exhaustive and the generic Self::Ssl(format!(...)) is deliberate; touch
the match in from_rustls so all current PeerIncompatible variants are either
explicitly mapped to an SSL_R_* constant (using the same style as existing arms)
or a documented fallback is left.
In `@crates/vm/src/vm/mod.rs`:
- Around line 2378-2401: The test only covers the cache-hit path; update
module_loaded_hook_can_patch_imported_module to also exercise the initial load
path by registering the hook (mark_module_loaded) before ensuring the target
module is not present in sys.modules and then importing it so the interpreter
takes the _find_and_load path; specifically, in the vm.enter closure call
vm.register_module_loaded_hook("sys", mark_module_loaded) then remove the "sys"
entry from sys.modules (via the VM sys module/object APIs) or choose a fresh
module name that is guaranteed not to be pre-imported, then call vm.import(...)
to trigger the initial-load hook and assert the
"__rustpython_module_loaded_hook_ran__" attribute as the existing test does
(referencing module_loaded_hook_can_patch_imported_module, mark_module_loaded,
vm.register_module_loaded_hook, and vm.import).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 0ca7babc-3895-4256-8a71-a9ba266cfb79
⛔ Files ignored due to path filters (5)
Cargo.lockis excluded by!**/*.lockLib/test/test_asyncio/test_events.pyis excluded by!Lib/**Lib/test/test_asyncio/test_sendfile.pyis excluded by!Lib/**Lib/test/test_asyncio/test_ssl.pyis excluded by!Lib/**Lib/test/test_ssl.pyis excluded by!Lib/**
📒 Files selected for processing (22)
Cargo.tomlcrates/stdlib/Cargo.tomlcrates/stdlib/src/lib.rscrates/stdlib/src/rustls-data/obj_mac.numcrates/stdlib/src/rustls-data/objects.txtcrates/stdlib/src/rustls.rscrates/stdlib/src/socket.rscrates/stdlib/src/ssl.rscrates/stdlib/src/ssl/cert.rscrates/stdlib/src/ssl/compat.rscrates/stdlib/src/ssl/error.rscrates/stdlib/src/ssl/oid.rscrates/stdlib/src/ssl/providers.rscrates/vm/Cargo.tomlcrates/vm/src/convert/mod.rscrates/vm/src/convert/rust_py_serde.rscrates/vm/src/import.rscrates/vm/src/vm/interpreter.rscrates/vm/src/vm/mod.rsexamples/custom_tls_providers.rsextra_tests/snippets/stdlib_ssl_bio_unencrypted_trailer.pysrc/interpreter.rs
💤 Files with no reviewable changes (3)
- crates/stdlib/src/ssl/error.rs
- crates/stdlib/src/ssl/oid.rs
- crates/stdlib/src/ssl/cert.rs
| /// Panics on unit values and unit structures. | ||
| pub struct RustPySerDe<'a> { | ||
| vm: &'a VirtualMachine, | ||
| conf: RustPySerDeConf, | ||
| } |
There was a problem hiding this comment.
Don't panic on valid unit values.
Unit and unit-struct values are normal Serde inputs. unimplemented!() here will unwind the VM instead of producing a Python exception, because with_serde_conf() only translates Result errors.
Suggested fix
-/// Panics on unit values and unit structures.
pub struct RustPySerDe<'a> {
vm: &'a VirtualMachine,
conf: RustPySerDeConf,
}
@@
fn serialize_unit(self) -> Result<Self::Ok, Self::Error> {
- unimplemented!("BUG: Unit value cannot be serialized into a Python object")
+ Ok(self.vm.ctx.none())
}
fn serialize_unit_struct(self, _name: &'static str) -> Result<Self::Ok, Self::Error> {
- unimplemented!("BUG: Unit struct value cannot be serialized into a Python object")
+ Ok(self.vm.ctx.none())
}Also applies to: 120-125
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/vm/src/convert/rust_py_serde.rs` around lines 13 - 17, The code
currently uses unimplemented!() for unit and unit-struct handling inside
RustPySerDe which causes a panic; instead implement proper Serde handling that
returns a Result (so errors are converted to Python exceptions via
with_serde_conf()) and treat unit/unit-struct as Python None: in the serializer
paths for unit/unit_struct return Ok(vm.ctx.none().into()) (or the equivalent
PyObject/Value representing None) and in the deserializer/visitor paths accept
Python None and return Ok(()) (or the appropriate unit value) rather than
panicking; update the functions/match arms that reference unit/unit_struct in
RustPySerDe so they return Result-based values and use the existing
with_serde_conf/error helpers to map failures to Python exceptions.
There was a problem hiding this comment.
This will not work for Option<()>.
There was a problem hiding this comment.
@im-0, you're right. Some(()) would call serialize_some → ().serialize(serializer) → serialize_unit(), which with my fix would produce Python None — making it indistinguishable from an actual None. The unimplemented!() is a deliberate guard against this ambiguity, and the struct-level doc comment already documents the limitation. I'll withdraw the suggestion.
🐇 (˵ •̀ ω •́ ˵)
|
For @im-0:
|
It is broken (and not by me): However, I did run
Not yet. Because I started working on this before it was written. In my own defense I can say that I started working on this EXACTLY because previous version was vibe coded (and did not work for my cases). I used LLM for some simple fixes (mostly to make reported errors into what cpython tests expect and for some debugging). Most of
I needed cpython test suite running on macos and windows because I do not have access to those at the moment. |
Those test suites will still run even if the PR is in draft status. |
a3ef4a9 to
1ccd777
Compare
|
Thank you so much for this try! Currently we dont have a reviewer to properly review this topic. Lets try to make this better. |
136989d to
b6afc8e
Compare
| h.close() | ||
| self.assertIn('text/html', content_type) | ||
|
|
||
| @unittest.skipIf("rustls" in __import__('ssl').OPENSSL_VERSION, "TODO: RUSTPYTHON; rustls does not support server host name verification by CN") |
There was a problem hiding this comment.
If this doesn't crash/hangs Id prefer to have it as
| @unittest.skipIf("rustls" in __import__('ssl').OPENSSL_VERSION, "TODO: RUSTPYTHON; rustls does not support server host name verification by CN") | |
| @unittest.expectedFailureIf("rustls" in __import__('ssl').OPENSSL_VERSION, "TODO: RUSTPYTHON; rustls does not support server host name verification by CN") |
There was a problem hiding this comment.
@youknowone There is a hack for a case when server certificate is loaded into a certificate store:
RustPython/crates/stdlib/src/ssl/cert.rs
Line 1594 in c5f555e
It is possible to closer replicate OpenSSL behavior but for this it will better to just fork https://github.com/rustls/webpki instead of adding more hacks around it (I added some too and I do not like it). And it will not work with rustls_platform_verifier anyway as its behavior is not configurable (whether we need to use rustls_platform_verifier at all is a different question).
ShaharNaveh
left a comment
There was a problem hiding this comment.
TYSM for working on this!
This will become a lot easier to maintain:)
I have few nitpicks
| h.close() | ||
| self.assertIn('text/html', content_type) | ||
|
|
||
| @unittest.skipIf("rustls" in __import__('ssl').OPENSSL_VERSION, "TODO: RUSTPYTHON; rustls does not support server host name verification by CN") |
There was a problem hiding this comment.
If this doesn't crash/hangs Id prefer to have it as
| @unittest.skipIf("rustls" in __import__('ssl').OPENSSL_VERSION, "TODO: RUSTPYTHON; rustls does not support server host name verification by CN") | |
| @unittest.expectedFailureIf("rustls" in __import__('ssl').OPENSSL_VERSION, "TODO: RUSTPYTHON; rustls does not support server host name verification by CN") |
b6afc8e to
d74a56d
Compare
|
Changes:
|
It checks that BIO mode does not read more data than required for TLS session.
This can be used to alter Python-native modules imported from cpython in runtime.
Currently parsing is done in run time but this really can be moved to compile time. Imported from OpenSSL 4.0.0: * https://github.com/openssl/openssl/raw/11b7b6ea3b65a584e1d31408ed1bdb139465cffd/crypto/objects/objects.txt * https://github.com/openssl/openssl/raw/11b7b6ea3b65a584e1d31408ed1bdb139465cffd/crypto/objects/obj_mac.num
Enabled tests: * test_create_server_ssl_verified * test_create_connection_memory_leak * test_handshake_timeout_handler_leak * test_tls_unique_channel_binding - now correctly skipped because we monkey-patch ssl.CHANNEL_BINDING_TYPES * test_sni_callback_alert * test_sni_callback_raising * test_sni_callback_wrong_return_type Tests skipped with rustls implementation specifically (should work with OpenSSL): * test_load_dh_params - rustls does not support Diffie-Hellman key exchange (it uses elliptic curve Diffie-Hellman) * test_get_ca_certs_capath - test expects that certificates are loaded lazily which is hard to implement with rustls * test_verify_strict - rustls does not perform some cert checks that OpenSSL does in strict mode Previously disbled tests changed to rustls-specific skip: * test_session - current rustls does not expose session info * test_session_handling * test_msg_callback_tls12 - _msg_callback cannot be implemented with rustls
This reverts commit e1d9a11.
This reverts commit 52305c0.
Main changes: * Now it contains understandable connection state machine instead of a handful of entangled variables. * OID and NID mappings are generated from OpenSSL data files. Some mappings are still hardcoded but those are much smaller than before and are actually testable. * OpenSSL-compatible cipher list string (`man openssl-ciphers`) is parsed correctly (I hope so). * Difference between socket IO and buffered IO handling is minimal. Please check the module-level doc comment in crates/stdlib/src/rustls.rs for additional information. What is missing: * ssl timeout support. Normal socket timeouts still work just fine but Python's ssl module implementation handles timeouts for whole SSL/TLS operations, like handshake. This does not break anything and just makes timeouts imprecise so I believe that proper timeouts can be implemented later. * Socket IO still uses _socket.socket.send/recv instead of methods of the socket object itself. Otherwise everything hangs and this must be investigated. * See other TODO items in crates/stdlib/src/rustls.rs
…s used rustls does not support server host name verification by CN. Only Subject Alternative Names are supported.
I misunderstood it and thought that it has something to do with locking.
d74a56d to
bd7cfca
Compare
Why do not merge?
test.test_ssl,test.test_asyncio,extra_testsand some very basic examples that userequests.rustls integration rewrite
Main changes:
man openssl-ciphers) is parsed correctly (I hope so).Please check the module-level doc comment in crates/stdlib/src/rustls.rsfor additional information.
What is missing:
Changed tests
Enabled:
Skipped with rustls implementation specifically (should work with OpenSSL):
Previously disabled tests changed to rustls-specific skip:
I also added a test for BIO mode in
extra_tests/.Related changes
ssl.CHANNEL_BINDING_TYPEShardcoded by Python-native module imported from cpython.crates/vm/src/convert/rust_py_serde.rs.Summary by CodeRabbit
New Features
Bug Fixes
Chores