Modularize rustls as work towards supporting custom providers#7657
Modularize rustls as work towards supporting custom providers#7657joshuamegnauth54 wants to merge 1 commit intoRustPython:mainfrom
Conversation
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/lib.rs (1)
80-82:⚠️ Potential issue | 🟡 MinorUpdate the stale error message.
The
not(any(...))clause was extended to includessl-rustls-ring, but the message still only mentionsssl-rustls/ssl-openssl. A user who hits this by enabling onlysslwill be told about options that don't reflect the new backend split (and the sentence is grammatically awkward).📝 Proposed wording
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 an SSL backend: enable one of \"ssl-rustls\" (aws-lc-rs), \"ssl-rustls-ring\", or \"ssl-openssl\". Do not pass \"ssl\" alone. To switch to ssl-openssl, use --no-default-features to disable the default ssl-rustls." );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib.rs` around lines 80 - 82, The compile_error! message in src/lib.rs is stale and must be updated to reflect the new backend split; modify the string passed to compile_error! (the multi-line message currently referencing "ssl-rustls" / "ssl-openssl") to mention all valid feature options ("ssl-rustls", "ssl-rustls-ring", and "ssl-openssl"), fix the grammar to read clearly (e.g., "Feature \"ssl\" is now provided by one of: \"ssl-rustls\", \"ssl-rustls-ring\", or \"ssl-openssl\"; do not enable \"ssl\" directly."), and keep the guidance about using --no-default-features to select ssl-openssl if needed.crates/stdlib/src/ssl/compat.rs (1)
2370-2376:⚠️ Potential issue | 🟡 Minor
curve_name_to_kx_groupassumes a provider has already been installed.Previously this used
aws_lc_rs's statics directly and didn't depend on global state. With.expect("A CryptoProvider should have been set earlier"), any future call site that reaches this helper beforecreate_client_config/create_server_confighave run will panic at runtime instead of producing an SSL error. Consider callingensure_default_provider()at the top of this function (idempotent viaOnce) to make it independent of call order.🛡️ Proposed fix
pub(super) fn curve_name_to_kx_group( curve: &str, ) -> Result<Vec<&'static dyn SupportedKxGroup>, String> { - // Get the default crypto provider's key exchange groups - let provider = rustls::crypto::CryptoProvider::get_default() - .expect("A CryptoProvider should have been set earlier"); + // Ensure a CryptoProvider is installed regardless of call order. + ensure_default_provider(); + let provider = rustls::crypto::CryptoProvider::get_default() + .expect("ensure_default_provider installs one of the enabled backends"); let all_groups = &provider.kx_groups;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/stdlib/src/ssl/compat.rs` around lines 2370 - 2376, The function curve_name_to_kx_group currently calls CryptoProvider::get_default().expect(...) which can panic if no provider was installed; instead call ensure_default_provider() at the start of curve_name_to_kx_group (the idempotent initializer suggested in the review) to guarantee a provider is set, then retrieve the provider and return an Err(String) if get_default() still fails; update references in curve_name_to_kx_group (and related error path) to use ensure_default_provider() and avoid any .expect() so the function returns an SSL error instead of panicking.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/stdlib/src/lib.rs`:
- Around line 112-115: The compile-time guard using compile_error! is too
narrow: update the cfg around the compile_error! (the compile_error! invocation
that references ssl-openssl and __ssl-rustls) to detect any mutually-enabled
pair among ssl-openssl, ssl-rustls-aws-lc-rs, and ssl-rustls-ring (for example
use cfg(any(all(feature="ssl-openssl", feature="__ssl-rustls"),
all(feature="ssl-rustls-aws-lc-rs", feature="ssl-rustls-ring"),
all(feature="ssl-openssl", feature="ssl-rustls-aws-lc-rs"),
all(feature="ssl-openssl", feature="ssl-rustls-ring")))) so the compile_error!
fires when more than one backend is enabled; alternatively (as noted) instead
change the imports in cert.rs (the conflicting use
rustls::crypto::aws_lc_rs::sign and use rustls::crypto::ring::sign) and the
branches in compat.rs::ensure_default_provider/install_default to use cfg(...,
not(...)) precedence so only one backend's code is compiled.
In `@crates/stdlib/src/ssl.rs`:
- Around line 88-92: Add a compile-time guard that fails if both rustls
providers are enabled so ALL_CIPHER_SUITES, Ticketer, and sign aren't defined
twice: add a top-level #[cfg(all(feature = "ssl-rustls-ring", feature =
"ssl-rustls-aws-lc-rs"))] compile_error!("Enable exactly one of ssl-rustls-ring
or ssl-rustls-aws-lc-rs"); keep the existing #[cfg(feature = "...")] use
statements for the imports of ALL_CIPHER_SUITES, Ticketer, and sign so only one
provider's symbols are compiled.
- Around line 2303-2304: The code currently panics by calling
Ticketer::new().expect(...) inside py_new; instead propagate the error as a
PyResult by replacing the expect with Ticketer::new().map_err(|e| /* convert to
Python error */ ) so py_new returns Err on failure. Update the server_ticketer
initialization in py_new to use .map_err(...) that converts the Ticketer
creation error into a Python exception (matching the existing conversion pattern
used elsewhere in this module), ensuring py_new returns the mapped PyErr rather
than panicking.
In `@crates/stdlib/src/ssl/compat.rs`:
- Around line 62-74: Add a compile-time guard that forbids enabling both rustls
backends together: add a cfg(all(feature = "ssl-rustls-ring", feature =
"ssl-rustls-aws-lc-rs")) compile_error! in the crate root (same place you
already check for ssl-openssl/__ssl-rustls) so the Cargo feature combination is
rejected; also make compat.rs conditional installs explicit (use mutually
exclusive cfgs around each rustls::crypto::CryptoProvider::install_default call
in ensure_default_provider / INIT_PROVIDER) so only the selected backend's
install_default is ever compiled.
---
Outside diff comments:
In `@crates/stdlib/src/ssl/compat.rs`:
- Around line 2370-2376: The function curve_name_to_kx_group currently calls
CryptoProvider::get_default().expect(...) which can panic if no provider was
installed; instead call ensure_default_provider() at the start of
curve_name_to_kx_group (the idempotent initializer suggested in the review) to
guarantee a provider is set, then retrieve the provider and return an
Err(String) if get_default() still fails; update references in
curve_name_to_kx_group (and related error path) to use ensure_default_provider()
and avoid any .expect() so the function returns an SSL error instead of
panicking.
In `@src/lib.rs`:
- Around line 80-82: The compile_error! message in src/lib.rs is stale and must
be updated to reflect the new backend split; modify the string passed to
compile_error! (the multi-line message currently referencing "ssl-rustls" /
"ssl-openssl") to mention all valid feature options ("ssl-rustls",
"ssl-rustls-ring", and "ssl-openssl"), fix the grammar to read clearly (e.g.,
"Feature \"ssl\" is now provided by one of: \"ssl-rustls\", \"ssl-rustls-ring\",
or \"ssl-openssl\"; do not enable \"ssl\" directly."), and keep the guidance
about using --no-default-features to select ssl-openssl if needed.
🪄 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: 7d7c738d-71a1-4533-a139-97a83ec37467
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
Cargo.tomlcrates/stdlib/Cargo.tomlcrates/stdlib/src/lib.rscrates/stdlib/src/ssl.rscrates/stdlib/src/ssl/cert.rscrates/stdlib/src/ssl/compat.rssrc/lib.rs
| server_ticketer: Ticketer::new() | ||
| .expect("Failed to create shared ticketer for TLS 1.2 session resumption"), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find fallible Ticketer::new() call sites and rustls lockfile version.
rg -n -C3 '\bTicketer::new\s*\(' --type rust
fd -e lock Cargo.lock --exec sh -c 'grep -n -A2 '\''name = "rustls"'\'' "$1"' sh {}Repository: RustPython/RustPython
Length of output: 792
🏁 Script executed:
#!/bin/bash
# Check the containing function context around line 2303
sed -n '2270,2320p' crates/stdlib/src/ssl.rs | cat -nRepository: RustPython/RustPython
Length of output: 2815
🏁 Script executed:
#!/bin/bash
# Look for Ticketer type definition and imports in ssl.rs
rg -n 'Ticketer|use.*ticketer' crates/stdlib/src/ssl.rs | head -20Repository: RustPython/RustPython
Length of output: 263
🏁 Script executed:
#!/bin/bash
# Check if vm.new_os_error() is used elsewhere in the codebase
rg -n 'new_os_error' --type rust | head -10Repository: RustPython/RustPython
Length of output: 1182
🏁 Script executed:
#!/bin/bash
# Search for Ticketer in the rustls crate to understand its API
rg -n 'pub.*Ticketer' --type rustRepository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Find the function signature containing this code
sed -n '2250,2310p' crates/stdlib/src/ssl.rs | head -70 | cat -nRepository: RustPython/RustPython
Length of output: 3800
🏁 Script executed:
#!/bin/bash
# Get more context to find the function name and PyResult
sed -n '2200,2260p' crates/stdlib/src/ssl.rs | tail -70 | cat -nRepository: RustPython/RustPython
Length of output: 3133
🏁 Script executed:
#!/bin/bash
# Search for pyfunction and PySSLContext in ssl.rs to identify the function
rg -n 'fn.*py_new|#\[pyfunction\]|impl.*PySSLContext' crates/stdlib/src/ssl.rs | grep -A2 -B2 '22[0-9][0-9]'Repository: RustPython/RustPython
Length of output: 414
🏁 Script executed:
#!/bin/bash
# Check what Ticketer::new() returns in rustls source
# First, let's check if there's any documentation or type hints in imports/usage
rg -n 'Ticketer::new' -A2 -B2 crates/stdlib/src/ssl.rsRepository: RustPython/RustPython
Length of output: 348
🏁 Script executed:
#!/bin/bash
# Check rustls documentation/crates.io for Ticketer::new() signature in 0.23.37
# First, let's see if there's any cargo metadata or docs
curl -s 'https://docs.rs/rustls/0.23.37/rustls/crypto/aws_lc_rs/struct.Ticketer.html' | grep -A5 'fn new' 2>/dev/null | head -20 || echo "Could not fetch online docs"Repository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Try to find any type annotations or usages that might hint at Ticketer::new() return type
# Look at what type server_ticketer is expected to be
rg -n 'server_ticketer\s*:' crates/stdlib/src/ssl.rs -A1 -B1Repository: RustPython/RustPython
Length of output: 395
🏁 Script executed:
#!/bin/bash
# Search for Ticketer struct definition in cargo dependencies
# Try to find the actual error type that Ticketer::new() can return
python3 << 'PYTHON_EOF'
import subprocess
import re
# Run cargo to get the actual version and path
result = subprocess.run(['cargo', 'metadata', '--format-version', '1'],
capture_output=True, text=True, cwd='.')
if result.returncode == 0:
import json
metadata = json.loads(result.stdout)
for pkg in metadata.get('packages', []):
if 'rustls' in pkg.get('name', ''):
print(f"Package: {pkg['name']} v{pkg['version']}")
print(f"Manifest path: {pkg['manifest_path']}")
PYTHON_EOFRepository: RustPython/RustPython
Length of output: 151
🏁 Script executed:
#!/bin/bash
# Look at the error handling pattern - see if there's a Result type used with Ticketer
rg -n 'Result.*Ticketer|Ticketer.*Result' crates/stdlib/src/ssl.rsRepository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Check if there are any issues or comments in the PR/code about Ticketer errors
rg -n 'Ticketer|ticketer' crates/stdlib/src/ssl.rs | grep -i 'error\|fail\|result'Repository: RustPython/RustPython
Length of output: 165
🏁 Script executed:
#!/bin/bash
# Check the actual rustls dependency to confirm Ticketer::new() signature
# Look at Cargo.toml to see the exact rustls version constraint
grep -A5 'rustls' Cargo.toml | head -20Repository: RustPython/RustPython
Length of output: 698
🏁 Script executed:
#!/bin/bash
# Check if there are type hints or documentation about Ticketer in the imports
sed -n '80,95p' crates/stdlib/src/ssl.rs | cat -nRepository: RustPython/RustPython
Length of output: 765
🏁 Script executed:
#!/bin/bash
# Verify that Ticketer::new() indeed returns a Result by checking if it's documented
# Search for any comments or documentation about the return type
rg -n 'Ticketer' crates/stdlib/src/ssl.rs -B3 -A3 | grep -E '(Result|Error|expect)'Repository: RustPython/RustPython
Length of output: 165
🏁 Script executed:
#!/bin/bash
# Look for the struct definition of Ticketer or any type documentation
# Check if there are any fallback implementations or handling
rg -n 'impl.*Ticketer|struct.*Ticketer' --type rustRepository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Verify vm.new_os_error() signature and return type
sed -n '120,130p' crates/vm/src/vm/vm_new.rs | cat -nRepository: RustPython/RustPython
Length of output: 441
🏁 Script executed:
#!/bin/bash
# Double-check by looking at the actual error type that would be returned
# Search for similar error patterns in the SSL module
rg -n '\.map_err.*vm\.new' crates/stdlib/src/ssl.rs | head -5Repository: RustPython/RustPython
Length of output: 510
🏁 Script executed:
#!/bin/bash
# Verify the proposed fix syntax - check if ? can be used in struct literals
# Look for examples of error handling before struct construction
rg -n 'let.*=.*\.map_err.*\?;' crates/stdlib/src/ssl.rs -A2 | head -20Repository: RustPython/RustPython
Length of output: 204
🏁 Script executed:
#!/bin/bash
# Check the exact pattern used in the codebase for pre-computing values before struct construction
sed -n '2250,2310p' crates/stdlib/src/ssl.rs | grep -n 'let.*=\|Ok(Py'Repository: RustPython/RustPython
Length of output: 411
🏁 Script executed:
#!/bin/bash
# Verify if the proposed fix syntax is valid by checking Rust syntax rules
# The key question: can you use ? operator inside struct literals?
# Let me check if there are any examples in the codebase of error handling before Ok(Struct { ... })
rg -n 'let.*=.*\?;' crates/stdlib/src/ssl.rs -A5 | grep -B5 'Ok(' | head -30Repository: RustPython/RustPython
Length of output: 1234
🏁 Script executed:
#!/bin/bash
# Check the exact pattern - looking for map_err followed by ? before struct construction
sed -n '2200,2320p' crates/stdlib/src/ssl.rs | grep -n '?' | head -10Repository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Test if ? can be used directly in struct literal - let's look for examples
# Search for struct literals with ? operator or error handling inside them
rg -n 'Struct\s*{|Ok\(.*{' crates/stdlib/src/ssl.rs -A20 | head -60Repository: RustPython/RustPython
Length of output: 175
🏁 Script executed:
#!/bin/bash
# More specifically, look at the exact structure around line 2303 to see the pattern
# Get more lines to see the full struct literal
sed -n '2295,2312p' crates/stdlib/src/ssl.rs | cat -nRepository: RustPython/RustPython
Length of output: 1003
🏁 Script executed:
#!/bin/bash
# Check the Rust compiler documentation pattern - can you use ? inside struct field initializers?
# Look for any extraction patterns before Ok() in similar code
rg -n 'let.*Ticketer|let.*=.*\.expect|let.*=.*\.map_err' crates/stdlib/src/ssl.rs -B2 -A2Repository: RustPython/RustPython
Length of output: 2262
🏁 Script executed:
#!/bin/bash
# Check the Rust edition to verify if ? operator works in struct field initializers
grep -n 'edition\|rust-version' Cargo.toml | head -5Repository: RustPython/RustPython
Length of output: 172
🏁 Script executed:
#!/bin/bash
# Look for examples of using ? directly in struct field initializers in the codebase
rg -n 'field.*:.*\?' crates/stdlib/src/ssl.rs -A1 -B1Repository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Check if the pattern in the proposed fix (using ? in struct initializer) exists anywhere
rg -n ':\s*.*\.map_err.*\?' crates/stdlib/src/ --type rust -A1 -B1 | head -20Repository: RustPython/RustPython
Length of output: 1533
🏁 Script executed:
#!/bin/bash
# Verify the proposed syntax is valid by checking similar patterns in the codebase
# Look for .map_err(...)?; patterns in field initializers
rg -n 'map_err.*\?,' crates/ --type rust | head -10Repository: RustPython/RustPython
Length of output: 1214
🏁 Script executed:
#!/bin/bash
# Final check: verify Ticketer::new() definitely returns Result by checking if .expect() compiles
# If it's being called with .expect(), it MUST return Result
# Let's confirm the error type by looking at any documentation or type hints
sed -n '2290,2310p' crates/stdlib/src/ssl.rsRepository: RustPython/RustPython
Length of output: 1098
Replace .expect() with proper error handling in py_new.
Since py_new returns PyResult<Self>, cryptographic provider initialization failures should propagate as exceptions, not panic. Use .map_err() to convert to a Python exception following the pattern established throughout the codebase.
- server_ticketer: Ticketer::new()
- .expect("Failed to create shared ticketer for TLS 1.2 session resumption"),
+ server_ticketer: Ticketer::new().map_err(|_| {
+ vm.new_os_error("Failed to create shared ticketer for TLS 1.2 session resumption")
+ })?,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| server_ticketer: Ticketer::new() | |
| .expect("Failed to create shared ticketer for TLS 1.2 session resumption"), | |
| server_ticketer: Ticketer::new().map_err(|_| { | |
| vm.new_os_error("Failed to create shared ticketer for TLS 1.2 session resumption") | |
| })?, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/stdlib/src/ssl.rs` around lines 2303 - 2304, The code currently panics
by calling Ticketer::new().expect(...) inside py_new; instead propagate the
error as a PyResult by replacing the expect with Ticketer::new().map_err(|e| /*
convert to Python error */ ) so py_new returns Err on failure. Update the
server_ticketer initialization in py_new to use .map_err(...) that converts the
Ticketer creation error into a Python exception (matching the existing
conversion pattern used elsewhere in this module), ensuring py_new returns the
mapped PyErr rather than panicking.
`rustls`'s architecture is very clean and trait-driven. There are many providers for `rustls` including the built-in `aws-lc-rs` and `ring` as well as backends for `boringssl`, `graviola`, `openssl`, `mbedtls`, etc. This commit removes the hard dependency on `aws-lc-rs` and adds support for `ring`. It works towards RustPython#7059 as well.
26e4d8f to
e3dbe62
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/stdlib/src/ssl.rs (1)
4881-4908:⚠️ Potential issue | 🟡 MinorRefresh the RAND comments for provider-agnostic rustls.
Lines 4883 and 4888 still say aws-lc-rs handles randomness, but
RAND_bytesnow routes through the selected/defaultCryptoProvider.Proposed comment cleanup
#[pyfunction] fn RAND_status() -> i32 { - 1 // Always have good randomness with aws-lc-rs + 1 // Always have good randomness from the active rustls CryptoProvider } #[pyfunction] fn RAND_add(_string: PyObjectRef, _entropy: f64) { - // No-op: aws-lc-rs handles its own entropy + // No-op: the active rustls CryptoProvider handles its own entropy // Accept any type (str, bytes, bytearray) }As per coding guidelines, do not delete or rewrite existing comments unless they are factually wrong or directly contradict the new code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/stdlib/src/ssl.rs` around lines 4881 - 4908, Update the misleading comments in RAND_status and RAND_add that reference aws-lc-rs so they reflect the current provider-agnostic design: change the RAND_status comment to state that randomness is provided by the configured CryptoProvider (not aws-lc-rs), and update the RAND_add comment to note it is a no-op because the selected/default CryptoProvider (used by RAND_bytes via CryptoProvider::get_default() and its secure_random.fill) manages entropy; keep the explanatory intent but remove the specific aws-lc-rs reference so comments match RAND_bytes behavior.src/lib.rs (1)
72-82:⚠️ Potential issue | 🟡 MinorUpdate the
sslfeature error to mentionssl-rustls-ring.Line 81 now contradicts the accepted backend set in lines 74-78, so users enabling bare
sslwon’t see the new ring option in the guidance.Proposed wording update
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\" must be enabled through a backend feature: \"ssl-rustls\", \"ssl-rustls-ring\", or \"ssl-openssl\". Do not pass \"ssl\" directly. To switch away from the default rustls backend, use --no-default-features." );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib.rs` around lines 72 - 82, The compile_error! message triggered when the cfg for features (feature = "ssl" and not any of "ssl-rustls", "ssl-rustls-ring", "ssl-openssl") is misleading because it omits "ssl-rustls-ring"; update the error string in the compile_error! call to list "ssl-rustls-ring" alongside "ssl-rustls" and "ssl-openssl" (and adjust wording to match the existing sentence structure) so users enabling bare "ssl" see the correct guidance referencing ssl-rustls-ring.
♻️ Duplicate comments (1)
crates/stdlib/src/ssl.rs (1)
2276-2307:⚠️ Potential issue | 🟠 MajorAvoid panicking while constructing
_SSLContext.This is still a fallible rustls initialization path inside
py_new, which already returnsPyResult<Self>; propagate a Python exception instead of aborting the interpreter with.expect(...).Proposed error-propagating shape
+ let server_ticketer = Ticketer::new().map_err(|_| { + vm.new_os_error("Failed to create shared ticketer for TLS 1.2 session resumption") + })?; + Ok(PySSLContext { protocol, check_hostname: PyRwLock::new(protocol == PROTOCOL_TLS_CLIENT), verify_mode: PyRwLock::new(default_verify_mode), @@ rustls_server_session_store: rustls::server::ServerSessionMemoryCache::new( SSL_SESSION_CACHE_SIZE, ), - server_ticketer: Ticketer::new() - .expect("Failed to create shared ticketer for TLS 1.2 session resumption"), + server_ticketer, accept_count: AtomicUsize::new(0),Verification:
#!/bin/bash # Verify the remaining panicking ticketer initialization and the rustls version in use. rg -n -C4 '\bserver_ticketer:\s*Ticketer::new\(\)' --type rust rg -n -C2 'name = "rustls"|version = "0\.23\.37"' Cargo.lockrustls 0.23.37 aws_lc_rs Ticketer::new signature Result rustls 0.23.37 ring Ticketer::new signature Result🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/stdlib/src/ssl.rs` around lines 2276 - 2307, The construction of PySSLContext currently panics on Ticketer::new().expect(...) inside py_new; change this to propagate the error as a PyResult by replacing the expect with handling the Result from Ticketer::new() and returning Err(...) on failure so py_new returns a Python exception instead of aborting; locate the server_ticketer field initialization in PySSLContext (and the py_new function that builds it) and map the Ticketer::new() error into an appropriate PyErr (e.g., PyRuntimeError or converted from the underlying error) so py_new returns Err(PyErr) on failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@crates/stdlib/src/ssl.rs`:
- Around line 4881-4908: Update the misleading comments in RAND_status and
RAND_add that reference aws-lc-rs so they reflect the current provider-agnostic
design: change the RAND_status comment to state that randomness is provided by
the configured CryptoProvider (not aws-lc-rs), and update the RAND_add comment
to note it is a no-op because the selected/default CryptoProvider (used by
RAND_bytes via CryptoProvider::get_default() and its secure_random.fill) manages
entropy; keep the explanatory intent but remove the specific aws-lc-rs reference
so comments match RAND_bytes behavior.
In `@src/lib.rs`:
- Around line 72-82: The compile_error! message triggered when the cfg for
features (feature = "ssl" and not any of "ssl-rustls", "ssl-rustls-ring",
"ssl-openssl") is misleading because it omits "ssl-rustls-ring"; update the
error string in the compile_error! call to list "ssl-rustls-ring" alongside
"ssl-rustls" and "ssl-openssl" (and adjust wording to match the existing
sentence structure) so users enabling bare "ssl" see the correct guidance
referencing ssl-rustls-ring.
---
Duplicate comments:
In `@crates/stdlib/src/ssl.rs`:
- Around line 2276-2307: The construction of PySSLContext currently panics on
Ticketer::new().expect(...) inside py_new; change this to propagate the error as
a PyResult by replacing the expect with handling the Result from Ticketer::new()
and returning Err(...) on failure so py_new returns a Python exception instead
of aborting; locate the server_ticketer field initialization in PySSLContext
(and the py_new function that builds it) and map the Ticketer::new() error into
an appropriate PyErr (e.g., PyRuntimeError or converted from the underlying
error) so py_new returns Err(PyErr) on failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 92c70991-b60c-4e5e-a436-4c13048a8af0
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
Cargo.tomlcrates/stdlib/Cargo.tomlcrates/stdlib/src/lib.rscrates/stdlib/src/ssl.rscrates/stdlib/src/ssl/cert.rscrates/stdlib/src/ssl/compat.rssrc/lib.rs
✅ Files skipped from review due to trivial changes (2)
- crates/stdlib/src/ssl/compat.rs
- crates/stdlib/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/stdlib/src/ssl/cert.rs
There was a problem hiding this comment.
every line is repeating the other line and the only difference is submodule name. doesn't rustls really support something like use rustls::crypto::{ALL_CIPHER_SUITES, Ticketer, sign}; ?
There was a problem hiding this comment.
Nope. 😆 The platform itself has to implement those. Also, both of rustls' built-in platforms can be enabled at the same time so the crate can't export the items like that.
There was a problem hiding this comment.
That's unfortune. Then I'd like to ask one more thing. Should we really need to add "ssl-rustls-ring" and "ssl-rustls-aws-ring" feature? There are also many third party providers for rustls. We cannot add all the features for those providers. Wouldn't there be any possible way to specify the crypto provider outside of the library?

rustls's architecture is very clean and trait-driven. There are many providers forrustlsincluding the built-inaws-lc-rsandringas well as backends forboringssl,graviola,openssl,mbedtls, etc.This commit removes the hard dependency on
aws-lc-rsand adds optional support forring. It works towards #7059 as well.aws-lc-rsis still the default, so this patch should be transparent to all users.I tested with:
And:
Summary by CodeRabbit
New Features
ssl-rustls-ringoption and expanded configurable Rustls TLS backends (AWS‑LC‑RS or Ring).Chores