Replace SSL backend to rustls #6244
Conversation
WalkthroughAdds rustls as the default TLS backend with mutually-exclusive backend features (ssl-rustls / ssl-openssl), implements a rustls↔OpenSSL compatibility layer, introduces extensive certificate/OID utilities and loaders, adds OpenSSL PyO3 Certificate bindings, and updates CI/build and feature wiring to support no-SSL checks. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Developer
participant Cargo as Cargo (features)
participant CI as CI
participant Runtime as RustPython
participant Rustls as ssl-rustls
participant OpenSSL as ssl-openssl
participant Compat as ssl compat layer
participant Certs as Cert loader / OID
Developer->>Cargo: build (default)
Cargo->>Runtime: enable feature "ssl-rustls"
CI->>Runtime: run checks (no-SSL matrix and ssl-rustls)
Runtime->>Rustls: initialize _ssl (ssl::make_module)
Rustls->>Compat: request TLS configs
Compat->>Certs: load certs / verify chain
Certs-->>Compat: verification result
Compat-->>Rustls: TLS config ready
note over Developer,Cargo: alternate backend
Developer->>Cargo: build --features ssl-openssl
Cargo->>Runtime: enable feature "ssl-openssl"
Runtime->>OpenSSL: initialize _ssl (openssl::make_module)
OpenSSL->>Certs: expose PySSLCertificate & helpers
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ 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 |
| /// Formats raw IP address bytes according to standard notation: | ||
| /// - IPv4: dotted decimal (e.g., "192.0.2.1") | ||
| /// - IPv6: colon-separated hex (e.g., "2001:DB8:0:0:0:0:0:1") | ||
| fn format_ip_address(ip: &[u8]) -> String { |
There was a problem hiding this comment.
Wouldn't make more sense to use one of:
There was a problem hiding this comment.
unfortunately ipv6 have different formatting.
Ipv6Addr format it like: "2001:DB8::1"
4d27e73 to
f0d6e63
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockLib/test/test_ssl.pyis excluded by!Lib/**
📒 Files selected for processing (14)
.cspell.dict/cpython.txt(4 hunks).cspell.dict/rust-more.txt(2 hunks).github/workflows/ci.yaml(4 hunks)Cargo.toml(2 hunks)README.md(1 hunks)src/lib.rs(1 hunks)stdlib/Cargo.toml(3 hunks)stdlib/build.rs(1 hunks)stdlib/src/lib.rs(2 hunks)stdlib/src/openssl/cert.rs(1 hunks)stdlib/src/ssl/cert.rs(1 hunks)stdlib/src/ssl/compat.rs(1 hunks)stdlib/src/ssl/oid.rs(1 hunks)vm/src/frame.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-28T16:31:03.991Z
Learnt from: arihant2math
Repo: RustPython/RustPython PR: 5790
File: build.rs:2-2
Timestamp: 2025-06-28T16:31:03.991Z
Learning: In Cargo build scripts (build.rs), the environment variable CARGO_CFG_TARGET_OS is guaranteed to exist and is automatically set by Cargo during the build process, making unwrap() safe to use when accessing this variable.
Applied to files:
.github/workflows/ci.yamlstdlib/build.rsstdlib/src/lib.rs
🧬 Code graph analysis (5)
stdlib/src/lib.rs (1)
stdlib/src/openssl.rs (2)
ssl(2078-2083)make_module(25-31)
vm/src/frame.rs (1)
crates/compiler-core/src/bytecode.rs (1)
display(1579-1591)
stdlib/src/openssl/cert.rs (3)
stdlib/src/openssl.rs (5)
convert_openssl_error(3262-3374)read(2026-2045)read(2687-2772)read(3130-3164)read(3530-3533)stdlib/src/ssl.rs (6)
e(1247-1247)public_bytes(4474-4494)get_info(4497-4500)_test_decode_cert(4395-4423)read(3231-3355)read(4096-4116)stdlib/src/ssl/cert.rs (2)
cert_to_dict(230-298)name_to_py(205-224)
stdlib/src/ssl/cert.rs (1)
stdlib/src/openssl/cert.rs (3)
name_to_py(103-120)name(104-118)cert_to_dict(123-208)
stdlib/src/ssl/compat.rs (4)
stdlib/src/socket.rs (1)
timeout_error_msg(2423-2425)stdlib/src/ssl.rs (9)
create_ssl_eof_error(449-454)create_ssl_want_read_error(423-434)create_ssl_want_write_error(436-447)create_ssl_zero_return_error(456-461)e(1247-1247)verify_flags(1035-1037)is_first_sni_read(2629-2631)timeout(4211-4214)time(4202-4208)stdlib/src/ssl/cert.rs (6)
new(566-574)new(993-1001)new(1257-1262)new(1427-1437)new(1510-1520)new_with_verifier(850-852)stdlib/src/openssl/ssl_data_31.rs (1)
encode_error_key(1768-1770)
🪛 Gitleaks (8.29.0)
stdlib/src/ssl/cert.rs
[high] 1117-1119: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
🔇 Additional comments (8)
.cspell.dict/rust-more.txt (1)
53-53: LGTM!Both dictionary additions are correctly alphabetically ordered and contextually appropriate for the PR's introduction of Rustls SSL/TLS backend support and PEM-based certificate handling. "pemfile" is correctly inserted between "peekable" and "powc", and "rustls" is correctly inserted between "rustfmt" and "rustyline".
Also applies to: 65-65
.cspell.dict/cpython.txt (1)
5-5: LGTM!The new spell-check dictionary entries (badcert, cadata, cafile, ticketer, webpki) are legitimate SSL/TLS technical terms directly related to the Rustls backend integration and certificate handling introduced in this PR. All entries are properly ordered alphabetically and will prevent false spell-check warnings.
Also applies to: 11-12, 53-53, 63-63
vm/src/frame.rs (1)
521-525: LGTM! Efficient logging optimization.Removing the
.to_string()call avoids an unnecessary allocation by passing theDisplayvalue directly to the trace formatter. This is a good optimization for trace logging..github/workflows/ci.yaml (1)
19-20: LGTM! CI configuration aligns with SSL backend refactoring.The introduction of
CARGO_ARGS_NO_SSLand the switch tossl-rustlsinCARGO_ARGScorrectly reflect the new modular SSL backend architecture. This allows CI to test both SSL-enabled and SSL-disabled configurations appropriately.stdlib/build.rs (1)
26-49: LGTM! Clean feature-gating of OpenSSL build logic.The OpenSSL-specific build script logic is now properly gated behind the
ssl-opensslfeature. This allows the build to proceed without OpenSSL dependencies when using thessl-rustlsbackend or no SSL at all.Cargo.toml (1)
13-13: LGTM! Well-designed SSL backend feature structure.The feature structure is well-designed:
ssl-rustlsas default provides out-of-the-box SSL without C dependenciessslremains as an empty feature for backward compatibilityssl-opensslandssl-vendorprovide OpenSSL backend options- Clear separation between backends
This aligns with the PR objectives to replace the hard-to-build OpenSSL with Rustls while maintaining OpenSSL as an option.
Also applies to: 23-26
stdlib/Cargo.toml (1)
18-24: LGTM! Comprehensive SSL backend dependency structure.The dependency structure properly supports both SSL backends:
- OpenSSL dependencies (openssl, openssl-sys, openssl-probe, foreign-types-shared) for
ssl-openssl- Rustls ecosystem dependencies (rustls, aws-lc-rs, x509-cert, etc.) for
ssl-rustls- All marked as optional and tied to appropriate features
- Includes FIPS variant (
ssl-rustls-fips)The dependency set is comprehensive and aligns with the requirements for a full-featured Rustls-based SSL implementation.
Also applies to: 117-134
src/lib.rs (1)
144-153: The original review comment is incorrect and based on a misunderstanding of the feature dependency structure.The condition at line 145 (
cfg!(feature = "ssl") && !any(ssl-rustls, ssl-openssl)) cannot occur at compile time. InCargo.toml, thesslfeature is empty (ssl = []), and both backend features explicitly enable it:
ssl-rustls = ["ssl", ...]ssl-openssl = ["ssl", ...]This means if
sslis enabled, it can only be because one of the backends enabled it. The TODO block is dead code and will never execute. No runtime validation issue exists—the compiler enforces this constraint at compile time.The code is safe as-is from a functional correctness perspective, though the unreachable TODO could be removed for clarity if it's no longer needed.
Likely an incorrect or invalid review comment.
stdlib/src/openssl/cert.rs
Outdated
| let format = format.unwrap_or(ENCODING_PEM); | ||
|
|
||
| match format { | ||
| x if x == ENCODING_DER => { |
There was a problem hiding this comment.
Can't it be just:
match format {
ENCODING_DER => ...,
ENCODING_PEM => ...,
}?
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib.rs (1)
144-153: Complete the SSL backend selection logic and update the error message.The nested check at lines 145-147 has a TODO placeholder but no actual implementation. When SSL is enabled but neither backend is available, this silently continues, which could lead to confusing runtime errors. Additionally, the error message at line 151 still references the old
--features=sslflag rather than the new backend-specific features (ssl-rustls,ssl-openssl).Consider implementing the missing logic or clarifying the intended behavior:
fn install_pip(installer: InstallPipMode, scope: Scope, vm: &VirtualMachine) -> PyResult<()> { if cfg!(feature = "ssl") { if cfg!(not(any(feature = "ssl-rustls", feature = "ssl-openssl"))) { - // TODO: something + return Err(vm.new_exception_msg( + vm.ctx.exceptions.system_error.to_owned(), + "install-pip requires rustpython be built with '--features=ssl-rustls' or '--features=ssl-openssl'".to_owned(), + )); } } else { return Err(vm.new_exception_msg( vm.ctx.exceptions.system_error.to_owned(), - "install-pip requires rustpython be build with '--features=ssl'".to_owned(), + "install-pip requires rustpython be built with '--features=ssl-rustls' or '--features=ssl-openssl'".to_owned(), )); }
♻️ Duplicate comments (3)
README.md (1)
69-82: Update SSL docs to reflect new backend features and drop vague TODO.This section still describes a single
sslfeature and OpenSSL/ssl-vendorsetup, while the manifest/CI now distinguishssl-rustls,ssl-openssl, andssl-vendorwith rustls as the primary backend. Please replace// TODO: fixmewith concrete wording that explains:
- which feature users should actually pass (
sslvsssl-rustls/ssl-openssl/ssl-vendor), and- how Windows setup differs now that rustls is available,
and adjust the
cargo install --features sslexample accordingly, or confirm thatsslis intentionally an alias that always enables a specific backend. Then remove the placeholder comment.stdlib/src/ssl/cert.rs (1)
1216-1236: The certificate-key match validation is incomplete.This function doesn't actually verify that the certificate and private key match—it only checks if the private key can be parsed (line 1228-1235). A mismatched cert/key pair will pass this validation but fail later during TLS handshake, making debugging difficult. CPython's
X509_check_private_keyperforms actual cryptographic comparison of the public and private key components.The comment note at line 1214-1215 acknowledges this is "simplified validation" but the function name
validate_cert_key_matchcreates a false expectation of actual validation. Consider either:
- Implementing proper validation by extracting the public key from the certificate and verifying it matches the private key (e.g., by signing and verifying a test message)
- Renaming to
validate_key_parseableto set correct expectations- Documenting prominently that this is a superficial check and actual validation happens during config building
Based on past review comments, this is a known issue but remains unaddressed.
stdlib/src/ssl/compat.rs (1)
778-818: Fix: StrictCertVerifier applied for any verification flag, not just VERIFY_X509_STRICT.Line 813 wraps the verifier with
StrictCertVerifierwheneververify_flags != 0, which incorrectly enables RFC 5280 strict validation (requiring Authority Key Identifier extension) for any verification flag. For example, settingVERIFY_CRL_CHECK(0x4) orVERIFY_X509_PARTIAL_CHAIN(0x80000) would unintentionally enforce AKI requirements, causing valid certificates without AKI to fail verification.CPython/OpenSSL only applies strict mode when
VERIFY_X509_STRICT(0x20) is explicitly set. TheStrictCertVerifierimplementation at lines 1245-1349 correctly checks for missing AKI extensions per RFC 5280, but it's being invoked too broadly.Apply this fix:
- // Wrap with StrictCertVerifier if other verify_flags are set - if verify_flags != 0 { + // Wrap with StrictCertVerifier only if VERIFY_X509_STRICT flag is set + const VERIFY_X509_STRICT: i32 = 0x00000020; + if verify_flags & VERIFY_X509_STRICT != 0 { Arc::new(super::cert::StrictCertVerifier::new(verifier, verify_flags)) } else { verifier }Note: The constant
VERIFY_X509_STRICTis already defined at line 1309 inStrictCertVerifier::verify_server_cert, so consider hoisting it to module level for reuse.
🧹 Nitpick comments (3)
.github/workflows/ci.yaml (1)
173-174: Add at least one CI job that compiles/tests with ssl-openssl (and optionally ssl-vendor).All these cross-target
cargo checksteps now use${{ env.CARGO_ARGS_NO_SSL }}, and the host builds usessl-rustlsviaCARGO_ARGS. There is no job in this workflow that ever enablesssl-opensslorssl-vendor, so the OpenSSL-backed paths can silently break.Consider adding a dedicated job (or matrix variant) that builds/tests with
--features ssl-openssl(plusssl-vendorif you want to cover vendored OpenSSL), to keep both backends exercised.Also applies to: 190-191, 197-198, 206-207, 213-214, 220-221, 227-228
crates/vm/src/frame.rs (1)
507-510: Use ofinstruction.display(...)in tracing is correct; consider mirroring it inflame_guard!.Passing
instruction.display(arg, &self.code.code)directly intotrace!is correct and avoids an unnecessaryStringallocation.For consistency (and the same small win), you could also drop the
.to_string()in theflame_guard!(format!("Frame::execute_instruction({})", …))above and rely on theDisplayimpl there as well, but that’s purely optional.Also applies to: 521-524
stdlib/src/openssl/cert.rs (1)
103-207: Refactor duplicated certificate conversion logic.The
name_to_py(lines 103-120) andcert_to_dict(lines 123-207) functions contain substantial code duplication with their counterparts instdlib/src/ssl/cert.rs(lines 205-224 and 230-298 respectively). Both implementations:
- Convert X509 names to Python nested tuples with identical structure
- Build Python dicts with the same fields (subject, issuer, version, serialNumber, notBefore, notAfter, subjectAltName)
- Parse SubjectAltName extensions with similar logic for DNS, IP, email, URI, DirName
- Format IPv6 addresses with identical uppercase hex formatting (lines 163-177 vs ssl/cert.rs lines 131-158)
This duplication increases maintenance burden and the risk of inconsistencies between backends.
Consider extracting the common certificate conversion logic into shared utility functions that both backends can use. For example:
- Create a common
CertificateInfostruct or trait that both OpenSSL'sX509Refand x509-parser'sX509Certificatecan convert to- Move the Python conversion logic (
name_to_py,cert_to_dict) to operate on this common representation- Keep only the backend-specific parsing code in each backend module
This would centralize the Python-facing representation and reduce the risk of divergent behavior.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockLib/test/test_ssl.pyis excluded by!Lib/**
📒 Files selected for processing (14)
.cspell.dict/cpython.txt(4 hunks).cspell.dict/rust-more.txt(2 hunks).github/workflows/ci.yaml(4 hunks)Cargo.toml(2 hunks)README.md(1 hunks)crates/vm/src/frame.rs(1 hunks)src/lib.rs(1 hunks)stdlib/Cargo.toml(3 hunks)stdlib/build.rs(1 hunks)stdlib/src/lib.rs(2 hunks)stdlib/src/openssl/cert.rs(1 hunks)stdlib/src/ssl/cert.rs(1 hunks)stdlib/src/ssl/compat.rs(1 hunks)stdlib/src/ssl/oid.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- stdlib/src/lib.rs
- stdlib/src/ssl/oid.rs
- Cargo.toml
- .cspell.dict/rust-more.txt
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-28T16:31:03.991Z
Learnt from: arihant2math
Repo: RustPython/RustPython PR: 5790
File: build.rs:2-2
Timestamp: 2025-06-28T16:31:03.991Z
Learning: In Cargo build scripts (build.rs), the environment variable CARGO_CFG_TARGET_OS is guaranteed to exist and is automatically set by Cargo during the build process, making unwrap() safe to use when accessing this variable.
Applied to files:
.github/workflows/ci.yamlstdlib/build.rs
🧬 Code graph analysis (5)
crates/vm/src/frame.rs (1)
crates/compiler-core/src/bytecode.rs (1)
display(1579-1591)
stdlib/src/openssl/cert.rs (2)
stdlib/src/openssl.rs (1)
convert_openssl_error(3262-3374)stdlib/src/ssl/cert.rs (2)
cert_to_dict(230-298)name_to_py(205-224)
stdlib/build.rs (2)
stdlib/src/ssl.rs (1)
version(3548-3561)stdlib/src/openssl.rs (1)
version(2282-2285)
stdlib/src/ssl/compat.rs (3)
stdlib/src/socket.rs (1)
timeout_error_msg(2423-2425)stdlib/src/ssl.rs (8)
create_ssl_eof_error(449-454)create_ssl_want_read_error(423-434)create_ssl_want_write_error(436-447)create_ssl_zero_return_error(456-461)e(1247-1247)is_first_sni_read(2629-2631)timeout(4211-4214)time(4202-4208)stdlib/src/openssl/ssl_data_31.rs (1)
encode_error_key(1768-1770)
stdlib/src/ssl/cert.rs (1)
stdlib/src/openssl/cert.rs (3)
name_to_py(103-120)name(104-118)cert_to_dict(123-208)
🔇 Additional comments (3)
.cspell.dict/cpython.txt (1)
5-5: Dictionary entries appropriately aligned with Rustls/TLS implementation.The additions of
badcert,cadata,cafile,ticketer, andwebpkiare all relevant to the SSL/TLS backend replacement work. The reordering ofItertoolcorrects the alphabetical sequence. All entries maintain proper dictionary ordering.Also applies to: 11-12, 30-30, 53-53, 63-63
.github/workflows/ci.yaml (1)
18-21: CARGO_ARGS / CARGO_ARGS_NO_SSL wiring matches new TLS feature layout.Using
ssl-rustlsinCARGO_ARGSand introducingCARGO_ARGS_NO_SSLas a no-TLS baseline is consistent with the new feature set and cleanly separates SSL-enabled from no-SSL builds. No issues here.stdlib/build.rs (1)
26-48: Correctly gate OpenSSL cfg/env handling on ssl-openssl; just confirm MSRV foris_ok_and.Wrapping all
DEP_OPENSSL_*handling in#[cfg(feature = "ssl-openssl")]is a good cleanup and prevents OpenSSL-specific cfgs from leaking into rustls-only or no-SSL builds. The vendored probe via:if std::env::var("DEP_OPENSSL_VENDORED").is_ok_and(|s| s == "1") { println!("cargo::rustc-cfg=openssl_vendored") }is also clear and avoids treating non-
"1"values as vendored.Since
Result::is_ok_andis a relatively new addition to the standard library, please ensure your workspacerust-versionand intended MSRV are high enough for this helper; otherwise a simplematches!(std::env::var("DEP_OPENSSL_VENDORED"), Ok(ref s) if s == "1")would be more conservative.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
README.md (1)
85-91: Document concrete feature-flag combinations for SSL backends.The text explains that
ssl-rustlsis the default andssl-openssl/ssl-vendorare alternatives, but it doesn’t show how to switch in practice. Consider adding shortcargoexamples (e.g.--no-default-features --features ssl-openssland--no-default-features --features ssl-vendor) so users know how to “replace” the default without hitting the mutual‑exclusion compile error.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.md(2 hunks)src/lib.rs(1 hunks)stdlib/src/lib.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib.rs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-28T16:31:03.991Z
Learnt from: arihant2math
Repo: RustPython/RustPython PR: 5790
File: build.rs:2-2
Timestamp: 2025-06-28T16:31:03.991Z
Learning: In Cargo build scripts (build.rs), the environment variable CARGO_CFG_TARGET_OS is guaranteed to exist and is automatically set by Cargo during the build process, making unwrap() safe to use when accessing this variable.
Applied to files:
stdlib/src/lib.rs
🧬 Code graph analysis (1)
stdlib/src/lib.rs (1)
stdlib/src/openssl.rs (2)
ssl(2078-2083)make_module(25-31)
⏰ 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). (1)
- GitHub Check: auto_format
🔇 Additional comments (2)
stdlib/src/lib.rs (2)
176-183:_sslmodule registration cleanly selects backend per feature.The two
_sslentries are now cfg-gated onssl-rustlsvsssl-opensslwith the samenot(target_arch = "wasm32")condition as the module declarations, so at most one backend is registered on host targets and wasm32 correctly gets no_sslmodule.
79-85: Mutual-exclusion guard betweenssl-opensslandssl-rustlsis correctly implemented.The cfg conditions for module declarations (lines 80, 82) precisely match the cfg conditions for
_sslregistrations (lines 177, 180). Thecompile_error!on line 84 triggers when both features are enabled, preventing compilation and eliminating the risk of duplicate_sslregistrations.
| You can install pip by | ||
| running: | ||
|
|
||
| ```bash | ||
| cargo install --git https://github.com/RustPython/RustPython --features ssl | ||
| cargo install --git https://github.com/RustPython/RustPython | ||
| rustpython --install-pip | ||
| ``` |
There was a problem hiding this comment.
Fix pip installation command; crate name is missing.
The cargo install example omits the crate name, so copy‑pasting line 73 will fail. It should mirror the earlier install example and include rustpython on the same line.
-```bash
-cargo install --git https://github.com/RustPython/RustPython
-rustpython --install-pip
-```
+```bash
+cargo install --git https://github.com/RustPython/RustPython rustpython
+rustpython --install-pip
+```🤖 Prompt for AI Agents
In README.md around lines 69 to 75, the cargo install example is missing the
crate name so the command will fail; update the install snippet to put the crate
name `rustpython` on the same line as `cargo install --git
https://github.com/RustPython/RustPython` and keep the subsequent `rustpython
--install-pip` line as shown in the suggested replacement so the two commands
can be copy-pasted and run successfully.
|
@ShaharNaveh I tried to review this myself, but it is really not easy. I know asking review for AI-generated dump is not fair. Let's just try. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
stdlib/src/ssl/compat.rs (3)
181-197: Error detection relies on fragile string matching.Lines 183-196 use debug format string matching (
contains("Hostname mismatch")) to distinguish error types. This is fragile because:
- Debug format output is not part of rustls's stable API
- Future rustls versions could change error messages, breaking this logic
- The
format!("{:?}", other_err)allocates unnecessarily on every errorConsider checking if rustls provides a more structured way to inspect
OtherErrorcontents, or document this as a known limitation requiring updates when rustls changes.
554-557: Potential panic in certificate verification error handling.Line 556 uses
.expect("unlikely to happen")when creating a certificate verification error. While attribute setting failures are rare (e.g., out of memory), if they do occur, this will panic the Rust runtime instead of gracefully returning an error to Python. Sincecreate_ssl_cert_verification_errorreturnsPyResult<PyBaseExceptionRef>, consider usingunwrap_or_elseto create a fallback exception without attributes.Apply this diff to handle the error gracefully:
SslError::CertVerification(cert_err) => { // Use the proper cert verification error creator - create_ssl_cert_verification_error(vm, &cert_err).expect("unlikely to happen") + create_ssl_cert_verification_error(vm, &cert_err).unwrap_or_else(|_| { + vm.new_exception_msg( + PySSLError::class(&vm.ctx).to_owned(), + format!("Certificate verification failed: {:?}", cert_err), + ) + }) }
1322-1325: Magic number for iteration limit.The hardcoded value
1000on line 1323 acts as a safety check against infinite loops. While the comment explains its purpose, consider defining this as a named constant (e.g.,MAX_HANDSHAKE_ITERATIONS) for better maintainability and to make the limit easily adjustable if needed.Apply this diff to use a named constant:
+// Maximum handshake iterations before giving up (safety check) +const MAX_HANDSHAKE_ITERATIONS: usize = 1000; + pub(super) fn ssl_do_handshake( conn: &mut TlsConnection, socket: &PySSLSocket, vm: &VirtualMachine, ) -> SslResult<()> { // ... existing code ... // Safety check: prevent truly infinite loops (should never happen) - if iteration_count > 1000 { + if iteration_count > MAX_HANDSHAKE_ITERATIONS { break; } }
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/lib.rs(2 hunks)stdlib/src/openssl/cert.rs(1 hunks)stdlib/src/ssl/cert.rs(1 hunks)stdlib/src/ssl/compat.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib.rs
- stdlib/src/openssl/cert.rs
🧰 Additional context used
🧬 Code graph analysis (2)
stdlib/src/ssl/cert.rs (1)
stdlib/src/openssl/cert.rs (3)
name_to_py(103-120)name(104-118)cert_to_dict(123-208)
stdlib/src/ssl/compat.rs (4)
stdlib/src/socket.rs (1)
timeout_error_msg(2423-2425)stdlib/src/ssl.rs (9)
create_ssl_eof_error(449-454)create_ssl_want_read_error(423-434)create_ssl_want_write_error(436-447)create_ssl_zero_return_error(456-461)e(1247-1247)verify_flags(1035-1037)is_first_sni_read(2629-2631)timeout(4211-4214)time(4202-4208)stdlib/src/ssl/cert.rs (6)
new(568-576)new(995-1003)new(1259-1264)new(1428-1438)new(1511-1521)new_with_verifier(852-854)stdlib/src/openssl/ssl_data_31.rs (1)
encode_error_key(1768-1770)
🪛 Gitleaks (8.29.0)
stdlib/src/ssl/cert.rs
[high] 1119-1121: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
🔇 Additional comments (1)
stdlib/src/ssl/cert.rs (1)
93-98: Disregard this review comment—errno preservation is not applicable here.The
decrypt_failedfunction correctly usesio::Error::other(). The decryption error comes frompkcs8::Error, which includes variants like Asn1, EncryptedPrivateKey, KeyMalformed, etc.—these are cryptographic/parsing errors, not OS-level errors with errno values. File I/O operations (certificate and key file reads at lines 1091, 1104) already preserve OS errors naturally viafs::read(). Usingio::Error::other()for non-OS errors is the correct approach.Likely an incorrect or invalid review comment.
youknowone
left a comment
There was a problem hiding this comment.
Good. I’ve taken a look. It’s hard to say that I’m fully satisfied with everything, but for now, since it’s not even clear whether we’ve implemented the SSL code safely, I’d rather not invest more effort into changing the code when we don’t fully understand the implementation. At the very least, pip install works and many tests are passing.
ShaharNaveh
left a comment
There was a problem hiding this comment.
The tests pass, that's what important. but I do worry about the maintenance of this code in the long run
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
stdlib/src/ssl/cert.rs (1)
178-184: Timestamp handling still uses expect() which can panic.Although the past review comment was marked as addressed, the current implementation still uses
expect()which will panic for out-of-range timestamps. While X.509 certificates typically have valid timestamps, malformed or crafted certificates could still trigger a panic.Consider using
unwrap_or_elsewith a fallback timestamp:fn format_asn1_time(time: &x509_parser::time::ASN1Time) -> String { let timestamp = time.timestamp(); DateTime::<Utc>::from_timestamp(timestamp, 0) - .expect("ASN1Time must be valid timestamp") + .unwrap_or_else(|| DateTime::<Utc>::from_timestamp(0, 0).unwrap()) .format("%b %e %H:%M:%S %Y GMT") .to_string() }stdlib/src/openssl/cert.rs (1)
26-51: FFI code still uses assert! which can panic.The past review comment correctly identified that
assert!on FFI return values (lines 31 and 43) can cause panics in production. While OpenSSL functions rarely fail in normal operation, returningResultwould be more robust.Consider changing the function signature to return
Result<Option<String>, String>and propagate errors instead of panicking:-pub(crate) fn obj2txt(obj: &Asn1ObjectRef, no_name: bool) -> Option<String> { +pub(crate) fn obj2txt(obj: &Asn1ObjectRef, no_name: bool) -> Result<Option<String>, String> { let no_name = i32::from(no_name); let ptr = obj.as_ptr(); let b = unsafe { let buflen = sys::OBJ_obj2txt(std::ptr::null_mut(), 0, ptr, no_name); - assert!(buflen >= 0); + if buflen < 0 { + return Err("OBJ_obj2txt failed to get buffer length".to_string()); + } if buflen == 0 { - return None; + return Ok(None); } // ... rest of the functionNote: This change would require updating all call sites.
🧹 Nitpick comments (1)
src/lib.rs (1)
62-68: Fix grammar in error message.The compile error message has a grammatical error. "to be enabled" should be removed since "either" already implies choice.
Apply this diff:
compile_error!( - "Feature \"ssl\" is now enabled by either \"ssl-rustls\" or \"ssl-openssl\" to be enabled. Do not manually pass \"ssl\" feature. To enable ssl-openssl, use --no-default-features to disable ssl-rustls" + "Feature \"ssl\" requires either \"ssl-rustls\" or \"ssl-openssl\" to be enabled. Do not manually pass \"ssl\" feature. To enable ssl-openssl, use --no-default-features to disable ssl-rustls" );
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
.cspell.dict/cpython.txt(4 hunks).github/workflows/ci.yaml(4 hunks)Cargo.toml(2 hunks)README.md(2 hunks)src/lib.rs(2 hunks)stdlib/Cargo.toml(3 hunks)stdlib/build.rs(1 hunks)stdlib/src/lib.rs(2 hunks)stdlib/src/openssl/cert.rs(1 hunks)stdlib/src/ssl/cert.rs(1 hunks)stdlib/src/ssl/compat.rs(1 hunks)stdlib/src/ssl/oid.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- stdlib/Cargo.toml
- .cspell.dict/cpython.txt
- Cargo.toml
- README.md
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-28T16:31:03.991Z
Learnt from: arihant2math
Repo: RustPython/RustPython PR: 5790
File: build.rs:2-2
Timestamp: 2025-06-28T16:31:03.991Z
Learning: In Cargo build scripts (build.rs), the environment variable CARGO_CFG_TARGET_OS is guaranteed to exist and is automatically set by Cargo during the build process, making unwrap() safe to use when accessing this variable.
Applied to files:
stdlib/build.rs.github/workflows/ci.yamlstdlib/src/lib.rs
🧬 Code graph analysis (4)
stdlib/build.rs (2)
stdlib/src/ssl.rs (1)
version(3521-3534)stdlib/src/openssl.rs (1)
version(2264-2267)
stdlib/src/lib.rs (1)
stdlib/src/openssl.rs (2)
ssl(2060-2065)make_module(25-31)
stdlib/src/openssl/cert.rs (3)
stdlib/src/openssl.rs (1)
convert_openssl_error(3239-3351)stdlib/src/ssl.rs (3)
e(1247-1247)public_bytes(4428-4448)get_info(4451-4454)stdlib/src/ssl/cert.rs (2)
cert_to_dict(298-345)name_to_py(273-292)
stdlib/src/ssl/compat.rs (4)
stdlib/src/socket.rs (1)
timeout_error_msg(2423-2425)stdlib/src/ssl.rs (10)
create_ssl_eof_error(449-454)create_ssl_want_read_error(423-434)create_ssl_want_write_error(436-447)create_ssl_zero_return_error(456-461)e(1247-1247)verify_flags(1035-1037)is_first_sni_read(2615-2617)timeout(4165-4168)time(4156-4162)protocol(1030-1032)stdlib/src/ssl/cert.rs (6)
new(609-617)new(1027-1035)new(1291-1296)new(1460-1470)new(1543-1553)new_with_verifier(892-894)stdlib/src/openssl/ssl_data_31.rs (1)
encode_error_key(1768-1770)
🪛 Gitleaks (8.29.0)
stdlib/src/ssl/cert.rs
[high] 1151-1153: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
⏰ 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 snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Check Rust code with clippy
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: auto_format
🔇 Additional comments (9)
src/lib.rs (1)
152-152: Verify semantic equivalence of the install_pip condition change.The condition changed from
cfg!(not(feature = "ssl"))to!cfg!(feature = "ssl")). While these should be semantically equivalent, the extra parenthesis in the new version is unnecessary.Apply this diff:
- if !cfg!(feature = "ssl") { + if !cfg!(feature = "ssl") {Actually, looking closer, the change is semantically equivalent. The extra closing parenthesis appears to be a display issue. No change needed.
.github/workflows/ci.yaml (1)
19-20: LGTM! Clear separation of SSL and no-SSL configurations.The new
CARGO_ARGS_NO_SSLenvironment variable provides a clean way to test builds without SSL support, which is appropriate for platforms like iOS and embedded targets where SSL may not be available or desired.stdlib/build.rs (1)
26-49: LGTM! Proper feature gating for OpenSSL build logic.The OpenSSL-specific build configuration is now correctly gated behind the
ssl-opensslfeature, which aligns with the PR's goal of supporting multiple TLS backends.stdlib/src/lib.rs (1)
79-84: LGTM! SSL backends are properly feature-gated with mutual exclusivity.The compile-time check ensures that both SSL backends cannot be enabled simultaneously, preventing duplicate
_sslmodule registration. This addresses the concern from the past review comment.stdlib/src/ssl/oid.rs (1)
1-464: LGTM! Well-structured OID lookup system with comprehensive coverage.The module provides a clean CPython-compatible OID lookup interface with:
- Efficient hash-based lookups for NID, OID string, and names
- Case-insensitive long name matching
- Mix of static (oid-registry) and string-based OIDs
- Comprehensive test coverage including duplicate detection
The design is solid and the implementation is correct.
stdlib/src/ssl/compat.rs (4)
57-63: LGTM! Thread-safe CryptoProvider initialization.The use of
Onceensures the default CryptoProvider is installed exactly once, even in multi-threaded scenarios. This is the correct approach for rustls 0.23+ process-level provider setup.
215-253: LGTM! Proper SSLCertVerificationError creation with CPython-compatible attributes.The function correctly:
- Maps rustls certificate errors to X509 verification codes
- Sets verify_code and verify_message attributes
- Handles error cases gracefully (continues even if attribute setting fails)
- Matches CPython's fill_and_set_sslerror() behavior
646-734: LGTM! Well-structured server config creation with proper client cert verification.The function handles multiple scenarios correctly:
- Deferred validation for TLS 1.3
- Immediate validation for TLS 1.2
- No client authentication when not requested
- Proper fallback when deferred error storage is not provided
- Session storage and ticketer configuration
The logic flow is clear and the error handling is appropriate.
828-967: LGTM! Comprehensive client config creation with multiple verifier types.The function correctly handles:
- Empty root store with EmptyRootStoreVerifier
- Hostname verification vs. hostname-ignoring verification
- CRL checking with proper flag handling
- Partial chain verification for self-signed certificates
- Strict verification when requested
- mTLS with client certificates
- Session resumption and ALPN configuration
The verifier wrapping logic is sound and matches OpenSSL behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
stdlib/Cargo.toml (1)
18-24: LGTM! Feature definitions match root Cargo.toml.The SSL backend feature split is consistent:
sslmarkerssl-rustlswith rustls dependenciesssl-rustls-fipsfor FIPS modessl-opensslwith OpenSSL dependenciesssl-vendorfor vendored OpenSSLstdlib/src/lib.rs (1)
79-84: LGTM! Mutual exclusion properly enforced.The compile_error at lines 83-84 prevents both ssl-rustls and ssl-openssl from being enabled simultaneously, addressing the past review concern.
stdlib/src/ssl/compat.rs (2)
807-811: LGTM! StrictCertVerifier properly gated by flag.Line 807 correctly checks
verify_flags & VERIFY_X509_STRICT != 0to apply StrictCertVerifier only when the specific flag is set, not for any non-zero flags. This addresses the past review concern.
826-965: LGTM! Client config builder handles all verification modes.The
create_client_configfunction properly handles:
- ✅ CERT_NONE mode with NoVerifier
- ✅ Empty root store with EmptyRootStoreVerifier
- ✅ Hostname verification vs check_hostname=False paths
- ✅ CRL support with build_webpki_verifier_with_crls
- ✅ Proper verifier wrapping (CRL, Partial, Strict)
- ✅ Session resumption and ALPN
Lines 919-926 correctly check the VERIFY_X509_STRICT bit specifically, matching the fix at line 807.
stdlib/src/openssl/cert.rs (1)
26-51: Critical: Replace assertions with proper error handling in FFI code.Lines 31 and 43 use
assert!on OpenSSL C function return values. If OpenSSL returns unexpected values (e.g., memory allocation failure), these will panic and crash the process. FFI boundaries should never panic.Change
obj2txtto returnResult<String, String>:-pub(crate) fn obj2txt(obj: &Asn1ObjectRef, no_name: bool) -> Option<String> { +pub(crate) fn obj2txt(obj: &Asn1ObjectRef, no_name: bool) -> Result<Option<String>, String> { let no_name = i32::from(no_name); let ptr = obj.as_ptr(); let b = unsafe { let buflen = sys::OBJ_obj2txt(std::ptr::null_mut(), 0, ptr, no_name); - assert!(buflen >= 0); + if buflen < 0 { + return Err("OBJ_obj2txt failed to get buffer length".to_string()); + } if buflen == 0 { - return None; + return Ok(None); } let buflen = buflen as usize; let mut buf = Vec::<u8>::with_capacity(buflen + 1); let ret = sys::OBJ_obj2txt( buf.as_mut_ptr() as *mut libc::c_char, buf.capacity() as _, ptr, no_name, ); - assert!(ret >= 0); + if ret < 0 { + return Err("OBJ_obj2txt failed".to_string()); + } // SAFETY: OBJ_obj2txt initialized the buffer successfully buf.set_len(buflen); buf }; let s = String::from_utf8(b) .unwrap_or_else(|e| String::from_utf8_lossy(e.as_bytes()).into_owned()); - Some(s) + Ok(Some(s)) }Update callers at lines 107 and elsewhere to handle the Result.
stdlib/src/ssl/cert.rs (1)
196-202: Critical: Handle invalid timestamps gracefully.Line 199 uses
.expect()which will panic ifDateTime::from_timestampreturnsNonefor out-of-range timestamps. While X.509 certificates typically have valid timestamps, malformed or crafted certificates could trigger this panic.Apply this diff to handle invalid timestamps:
fn format_asn1_time(time: &x509_parser::time::ASN1Time) -> String { let timestamp = time.timestamp(); DateTime::<Utc>::from_timestamp(timestamp, 0) - .expect("ASN1Time must be valid timestamp") + .unwrap_or_else(|| DateTime::<Utc>::from_timestamp(0, 0).expect("unix epoch is valid")) .format("%b %e %H:%M:%S %Y GMT") .to_string() }
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
.cspell.dict/cpython.txt(4 hunks).github/workflows/ci.yaml(4 hunks)Cargo.toml(2 hunks)README.md(2 hunks)src/lib.rs(2 hunks)stdlib/Cargo.toml(3 hunks)stdlib/build.rs(1 hunks)stdlib/src/lib.rs(2 hunks)stdlib/src/openssl/cert.rs(1 hunks)stdlib/src/ssl/cert.rs(1 hunks)stdlib/src/ssl/compat.rs(1 hunks)stdlib/src/ssl/oid.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- .github/workflows/ci.yaml
- README.md
- .cspell.dict/cpython.txt
- stdlib/build.rs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-28T16:31:03.991Z
Learnt from: arihant2math
Repo: RustPython/RustPython PR: 5790
File: build.rs:2-2
Timestamp: 2025-06-28T16:31:03.991Z
Learning: In Cargo build scripts (build.rs), the environment variable CARGO_CFG_TARGET_OS is guaranteed to exist and is automatically set by Cargo during the build process, making unwrap() safe to use when accessing this variable.
Applied to files:
stdlib/src/lib.rs
🧬 Code graph analysis (3)
stdlib/src/openssl/cert.rs (3)
stdlib/src/openssl.rs (1)
convert_openssl_error(3238-3350)stdlib/src/ssl.rs (2)
e(1244-1244)public_bytes(4422-4442)stdlib/src/ssl/cert.rs (2)
cert_to_dict(316-363)name_to_py(291-310)
stdlib/src/lib.rs (1)
stdlib/src/openssl.rs (2)
ssl(2059-2064)make_module(25-31)
stdlib/src/ssl/compat.rs (4)
stdlib/src/socket.rs (1)
timeout_error_msg(2423-2425)stdlib/src/ssl.rs (10)
create_ssl_eof_error(449-454)create_ssl_want_read_error(423-434)create_ssl_want_write_error(436-447)create_ssl_zero_return_error(456-461)e(1244-1244)verify_flags(1034-1036)is_first_sni_read(2610-2612)timeout(4159-4162)time(4150-4156)protocol(1029-1031)stdlib/src/ssl/cert.rs (6)
new(627-635)new(1033-1041)new(1297-1302)new(1454-1464)new(1537-1547)new_with_verifier(898-900)stdlib/src/openssl/ssl_data_31.rs (1)
encode_error_key(1768-1770)
🪛 Gitleaks (8.29.0)
stdlib/src/ssl/cert.rs
[high] 1157-1159: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
⏰ 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 (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Check the WASM package and demo
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Check Rust code with clippy
- GitHub Check: auto_format
🔇 Additional comments (18)
src/lib.rs (2)
62-68: LGTM! Compile-time guard prevents misconfiguration.The compile_error correctly enforces that users select exactly one SSL backend. The error message helpfully explains how to switch from the default ssl-rustls to ssl-openssl.
152-152: LGTM! Logically equivalent refactor.The negation is moved outside cfg!() with no functional change.
Cargo.toml (1)
13-26: LGTM! SSL backend features properly wired.The feature split is clean:
sslis now a marker featuressl-rustlsis the new default backendssl-opensslandssl-vendorprovide OpenSSL alternatives- Feature dependencies correctly propagate to rustpython-stdlib
This addresses the past review concerns about missing feature wiring at the root level.
stdlib/src/lib.rs (1)
176-183: LGTM! Backend initialization properly gated.Both backends correctly map to the
_sslPython module name, with feature gates ensuring only one is active at compile time.stdlib/src/ssl/oid.rs (1)
1-464: LGTM! Well-structured OID management system.This module provides a comprehensive OID lookup surface compatible with CPython's ssl module:
✅ Strengths:
- Clean separation between static (oid-registry) and string-based OIDs
- Multiple index types (NID, OID string, short name, long name) for fast lookups
- Case-insensitive long name matching
- Comprehensive OID coverage (X.509 DN, extensions, EKU, algorithms)
- Good test coverage validating lookups and uniqueness
✅ Design:
- LazyLock ensures one-time initialization
- HashMap indices provide O(1) lookups
- Macros reduce boilerplate for OID definitions
The implementation appears solid and production-ready.
stdlib/src/openssl/cert.rs (1)
76-94: LGTM! Match expression properly handles encoding formats.The match on ENCODING_DER and ENCODING_PEM is clean and properly maps OpenSSL encoding to bytes or PEM strings. Error handling via convert_openssl_error is appropriate.
stdlib/src/ssl/cert.rs (5)
53-132: LGTM! Well-organized error handling utilities.The
cert_errormodule provides consistent error creation functions that:
- Reduce code duplication
- Ensure uniform error messages
- Support proper error mapping to rustls and Python exceptions
The organization (pem, der, key submodules) is clean and maintainable.
270-286: LGTM! CA certificate detection properly implemented.The
is_ca_certificatefunction correctly:
- Parses the certificate
- Checks Basic Constraints extension
- Returns true only if CA=true
- Safely handles parse errors
This matches OpenSSL's X509_check_ca() behavior as documented.
605-840: LGTM! CertLoader provides robust certificate loading.The
CertLoaderimplementation:
- ✅ Auto-detects PEM vs DER format
- ✅ Handles duplicate certificates via HashSet
- ✅ Tracks CA statistics
- ✅ Supports files, directories, and bytes
- ✅ Validates X.509 structure after PEM decode
- ✅ Proper error messages
The separation of concerns (load_from_file, load_from_dir, load_from_bytes_ex) is clean.
1256-1276: Function correctly validates private key format.The past review concern about not checking cert/key match is noted, but the current implementation correctly validates that:
- The private key can be parsed by rustls
- A signing key can be created
Full cert/key matching would require signing a test message, which is complex. The current approach catches most configuration errors and matches the design constraints mentioned in past reviews regarding compatibility requirements.
1278-1620: LGTM! Comprehensive verifier implementations.The verifier suite provides:
- ✅ NoVerifier: CERT_NONE mode
- ✅ HostnameIgnoringVerifier: check_hostname=False
- ✅ StrictCertVerifier: RFC 5280 strict validation with AKI checks
- ✅ EmptyRootStoreVerifier: Proper error when no CAs loaded
- ✅ CRLCheckVerifier: CRL enforcement
- ✅ PartialChainVerifier: VERIFY_X509_PARTIAL_CHAIN support
- ✅ DeferredClientCertVerifier: TLS 1.3 deferred validation
Each verifier properly delegates to inner verifiers and handles edge cases. The wrapper pattern allows flexible composition.
stdlib/src/ssl/compat.rs (7)
39-46: LGTM! Verification flags properly defined.The constants
VERIFY_X509_STRICT(0x20) andVERIFY_X509_PARTIAL_CHAIN(0x80000) match OpenSSL's flag values for RFC 5280 strict validation and partial chain acceptance.
164-205: LGTM! Certificate error mapping comprehensive.The
rustls_cert_error_to_verify_infofunction properly maps rustls certificate errors to OpenSSL X509_V_ERR_* codes:
- UnknownIssuer → UNABLE_TO_GET_ISSUER_CERT_LOCALLY (20)
- Expired → CERT_HAS_EXPIRED (10)
- Revoked → CERT_REVOKED (23)
- Hostname/IP mismatch detection from error messages
This ensures Python code receives OpenSSL-compatible error codes.
373-562: LGTM! SslError enum provides comprehensive error handling.The error type properly covers:
- ✅ WantRead/WantWrite for non-blocking I/O
- ✅ Certificate verification with rustls::CertificateError
- ✅ TLS alerts mapped to OpenSSL error codes
- ✅ Timeout errors for socket.timeout
- ✅ SNI callback restart signal
- ✅ Proper error conversion to Python exceptions
The
from_rustlsmethod correctly maps rustls errors, including the special handling of CloseNotify as ZeroReturn and PeerIncompatible::NoCipherSuitesInCommon as SSL_R_NO_SHARED_CIPHER.
648-732: LGTM! Server config builder handles complex scenarios.The
create_server_configfunction properly:
- ✅ Handles client cert verification (optional, required, deferred)
- ✅ Supports custom cert resolvers for SNI
- ✅ Configures session storage and ticketer
- ✅ Applies ALPN with fallback
- ✅ Uses custom crypto provider for cipher/KX group control
The deferred validation support for TLS 1.3 is particularly well-designed.
1181-1325: LGTM! Handshake logic handles BIO and socket modes.The
ssl_do_handshakefunction:
- ✅ Handles first iteration forcing write for ClientHello/ServerHello
- ✅ Properly detects SNI callback needs and returns SniCallbackRestart
- ✅ Handles TLS 1.3 NewSessionTicket in both BIO and socket modes
- ✅ Prevents infinite loops with iteration counter
- ✅ Properly exits when handshake completes
The separation of BIO mode (one iteration, return WANT errors) vs socket mode (loop until complete) is correct.
1333-1430: LGTM! SSL_read properly handles blocking and non-blocking I/O.The
ssl_readfunction:
- ✅ Implements deadline-based timeout
- ✅ Loops to handle TLS records and post-handshake messages
- ✅ Tries plaintext read before fetching more TLS data
- ✅ Handles "message buffer full" gracefully
- ✅ Returns buffered plaintext before propagating errors
- ✅ Properly distinguishes BIO EOF from socket EOF
The CPython comment about suppress_ragged_eofs=True behavior (lines 1474-1486) is valuable context for the ZeroReturn handling.
1636-1786: LGTM! Helper functions provide OpenSSL compatibility.The helper functions properly:
- ✅ Normalize cipher names (TLS_AES_256_GCM_SHA384 → AES256-GCM-SHA384)
- ✅ Extract key bits from cipher names
- ✅ Map protocol versions (TLSv1.2, TLSv1.3)
- ✅ Convert curve names to KxGroups (prime256v1, secp384r1, X25519, etc.)
- ✅ Extract cipher info for Python exposure
These utilities bridge rustls and OpenSSL naming conventions.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
stdlib/Cargo.toml(3 hunks)
⏰ 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). (12)
- 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: Check the WASM package and demo
- GitHub Check: Run tests under miri
- GitHub Check: Check Rust code with clippy
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: auto_format
🔇 Additional comments (2)
stdlib/Cargo.toml (2)
18-23: Mutual exclusion between SSL backends is properly enforced.Verification confirms that a compile-time guard at
stdlib/src/lib.rs:84prevents bothssl-rustlsandssl-opensslfrom being enabled simultaneously with the error:"features \"ssl-openssl\" and \"ssl-rustls\" are mutually exclusive". No action required.
93-93: Chrono usage is justified—it handles certificate timestamp formatting in SSL modules.The addition of chrono is directly used in
stdlib/src/ssl/cert.rsfor converting ASN1 certificate timestamps to human-readable date strings via theformat_asn1_time()function. This is a core part of certificate handling and verification.
| # OpenSSL dependencies (optional, for ssl-openssl feature) | ||
| openssl = { version = "0.10.72", optional = true } | ||
| openssl-sys = { version = "0.9.110", optional = true } | ||
| openssl-probe = { version = "0.1.5", optional = true } | ||
| foreign-types-shared = { version = "0.1.1", optional = true } | ||
|
|
||
| # Rustls dependencies (optional, for ssl-rustls feature) | ||
| rustls = { version = "0.23.35", default-features = false, features = ["std", "tls12", "aws_lc_rs"], optional = true } | ||
| rustls-native-certs = { version = "0.8", optional = true } | ||
| rustls-pemfile = { version = "2.2", optional = true } | ||
| rustls-platform-verifier = { version = "0.6", optional = true } | ||
| x509-cert = { version = "0.2.5", features = ["pem", "builder"], optional = true } | ||
| x509-parser = { version = "0.16", optional = true } | ||
| der = { version = "0.7", features = ["alloc", "oid"], optional = true } | ||
| pem-rfc7468 = { version = "0.7", optional = true } | ||
| webpki-roots = { version = "0.26", optional = true } | ||
| aws-lc-rs = { version = "1.14.1", optional = true } | ||
| oid-registry = { version = "0.7", features = ["x509", "pkcs1", "nist_algs"], optional = true } | ||
| pkcs8 = { version = "0.10", features = ["encryption", "pkcs5", "pem"], optional = true } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify crypto/TLS dependency versions remain current (8 months post knowledge cutoff).
The dependency versions were corrected in response to prior review feedback (x509-cert → 0.2.5, aws-lc-rs → 1.14.1), but with knowledge cutoff at March 2025 and current date November 2025, verify these are still the recommended stable versions:
rustls = 0.23.35x509-cert = 0.2.5aws-lc-rs = 1.14.1openssl = 0.10.72
🌐 Web query:
What are the latest stable versions of rustls, x509-cert, aws-lc-rs, and openssl crates as of November 2025?
💡 Result:
As of November 15, 2025:
- rustls — 0.23.28. [1]
- x509-cert — 0.2.5. [2]
- aws-lc-rs — 1.14.1. [3]
- openssl (crate) — 0.10.72. [4]
Sources:
[1] rustls crate listing (lib.rs).
[2] rust-x509-cert package / crates listing (Fedora packages).
[3] aws-lc-rs docs/crate page (docs.rs).
[4] openssl crate update notices (crates.io / Fedora security advisory).
Downgrade rustls to 0.23.28 (current latest stable, not 0.23.35).
The latest stable version of rustls as of November 15, 2025 is 0.23.28, but line 127 declares 0.23.35. The other dependencies are correctly specified: x509-cert 0.2.5, aws-lc-rs 1.14.1, and openssl 0.10.72 are current.
Line 127:
- rustls = { version = "0.23.28", default-features = false, features = ["std", "tls12", "aws_lc_rs"], optional = true }
🤖 Prompt for AI Agents
In stdlib/Cargo.toml around lines 116 to 134, the rustls dependency is pinned to
version "0.23.35" but should be downgraded to "0.23.28"; update the rustls line
to use version = "0.23.28" (keeping default-features, features and optional
flags unchanged) so the Cargo.toml matches the current stable rustls version.
close #3839
close #5316
Summary by CodeRabbit
New Features
Chores