Support custom rustls providers#7657
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a pluggable CryptoProvider abstraction for rustls, introduces an internal ChangesPluggable Crypto Provider Infrastructure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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.
joshuamegnauth54
force-pushed
the
modularize-rustls-support
branch
from
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
| use rustls::crypto::aws_lc_rs::{ALL_CIPHER_SUITES, Ticketer, sign}; | ||
|
|
||
| #[cfg(feature = "ssl-rustls-ring")] | ||
| use rustls::crypto::ring::{ALL_CIPHER_SUITES, Ticketer, sign}; |
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?
There was a problem hiding this comment.
Should we really need to add "ssl-rustls-ring" and "ssl-rustls-aws-ring" feature?
Nope, and I can remove it if you prefer. 😁 I mostly added ring because rustls has it built-in, so it was a good way to test if the refactor actually works out.
Wouldn't there be any possible way to specify the crypto provider outside of the library?
Yes, I think so. To clarify, this PR is primarily a refactor to see how user specified crypto providers would work out. I think I mostly figured it out, which is why ring works, but I wanted to check with you and the other maintainers to see if I'm on the right track.
So, my intention is:
This patch -> Preliminary work to reduce explicitly depending on aws-lc-rs
Next patch -> Provide a way for user to set a crypto provider (#7059)
There was a problem hiding this comment.
Should we really need to add "ssl-rustls-ring" and "ssl-rustls-aws-ring" feature?
This was more like a technical question, "Can ring to be added without adding its own feature?"
Yeah, this is the right way in general. I try to understand the dedicated features are really required or not as the last step of review.
Another trial: If we turn on rustls-no-provider, can we avoid dedicated features? Like using #[cfg(all(feature = "ssl-rustls", feature = "ring"))]
There was a problem hiding this comment.
@ShaharNaveh Could you help? will this dedicated features be the only way that we can support mulitple provider?
There was a problem hiding this comment.
Sorry, I've been a bit busy so I forgot to respond. 😆
I'll remove ring. I think the final iteration will only have ssl-rustls (which will use aws-lc-rs) and ssl-rustls-no-provider (user will have to set a provider).
I'll draft this and work on it next week.
There was a problem hiding this comment.
Does ssl-rustls-no-provider allow users to set the provider ring? Then that sounds perfect. Please tell me I am asking something impossible. I don't know well how to handle rustls features well yet.
This might be out of scope of this PR. Just sharing a note for future implementation.
If ssl-rustls-no-provider allows user to select any kind of provider including aws-lc-rs and ring,
I prefer to keep rustpython-stdlib always set to no-provider as default and rustpython the shell crate to override it as default.
There was a problem hiding this comment.
@ShaharNaveh Could you help? will this dedicated features be the only way that we can support mulitple provider?
I don't have experience in this area:/
My inputs wouldn't be valuable here:(
There was a problem hiding this comment.
I made a lot of progress with this but now I'm stuck. 😵💫 Running the interpreter works and the example I wrote in examples/tls_providers works, but I'm not sure why cargo test is failing.
The error says that the encodings module can't be found, but I didn't touch anything related to that. 🤔 I'm not sure if it's feature gate related.
joshuamegnauth54
changed the title
rustls providers
joshuamegnauth54
force-pushed
the
modularize-rustls-support
branch
from
e3dbe62 to
455d064
Compare
joshuamegnauth54
force-pushed
the
modularize-rustls-support
branch
from
455d064 to
62ac0af
Compare
|
Temporarily undrafted because I want CodeRabbit's review. 😆 |
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 (2)
Cargo.toml (1)
14-27:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
ssl-rustls-no-providerdoes not actually replace the default backend.Line 14 includes
ssl-rustlsin the default features, so runningcargo build --features ssl-rustls-no-providerstill keepsssl-rustlsenabled unless--no-default-featuresis also passed. This defeats the main use case of providing a way to avoid aws-lc-rs entirely at the root crate level. Either make the combination fail fast or restructure the root features sossl-rustls-no-providerbecomes a real alternative.🤖 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 `@Cargo.toml` around lines 14 - 27, The Cargo.toml default features include "ssl-rustls" so enabling "ssl-rustls-no-provider" doesn't disable the default; fix by either making the SSL features mutually exclusive or failing fast: (a) restructure features so default no longer directly includes "ssl-rustls" — introduce an intermediate feature like "ssl-default" that selects "ssl-rustls" and have default = [..., "ssl-default", ...] so consumers can opt into "ssl-rustls-no-provider" without the default pulling it in; or (b) add a build-time check in build.rs that tests cfg!(feature = "ssl-rustls") && cfg!(feature = "ssl-rustls-no-provider") and emits a compile_error when both are enabled. Update Cargo.toml feature lists (symbols: default, ssl-rustls, ssl-rustls-no-provider, ssl-default) or add the build.rs guard accordingly.crates/stdlib/src/ssl/compat.rs (1)
52-60:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStale doc comment doesn't describe
CRYPTO_EXT.The doc block above
CRYPTO_EXTstill describes a one-shot installer of the defaultCryptoProviderusingOnce. The static is now anOnceLock<CryptoExt>that stores an extension object; installation has moved toset_crypto_provider/set_aws_lc_provider. Per coding guidelines, comments may be rewritten when they directly contradict the new code — this one does.📝 Suggested rewrite
-// CryptoProvider Initialization: - -/// Ensure the default CryptoProvider is installed (thread-safe, runs once) -/// -/// This is necessary because rustls 0.23+ requires a process-level CryptoProvider -/// to be installed before using default_provider(). We use Once to ensure this -/// happens exactly once, even if called from multiple threads. -pub(super) static CRYPTO_EXT: OnceLock<CryptoExt> = OnceLock::new(); +// CryptoProvider Initialization: + +/// Process-wide [`CryptoExt`] holding the active provider's optional extension +/// data (cipher suites, KX groups, signing-key selector, ticketer). +/// +/// Populated exactly once by [`set_crypto_provider`] (or [`set_aws_lc_provider`]) +/// alongside `rustls::crypto::CryptoProvider::install_default`. All SSL crypto +/// paths assume a provider has been installed before they run. +pub(super) static CRYPTO_EXT: OnceLock<CryptoExt> = OnceLock::new();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 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 52 - 60, The doc comment above the static CRYPTO_EXT is outdated: replace the one-shot CryptoProvider installer description with a brief factual note that CRYPTO_EXT is an OnceLock<CryptoExt> used to store a CryptoExt extension object for the process, and mention that provider installation now happens via set_crypto_provider and set_aws_lc_provider; update the comment to describe its role (process-wide extension holder, thread-safe, initialized once) and remove references to installing the default provider with Once.
🧹 Nitpick comments (2)
crates/stdlib/src/ssl/compat.rs (2)
157-167: 💤 Low valueLGTM, with one nit on the doc.
The two-step "install provider, then set extension" works correctly because both inner primitives are one-shots and the second
setcan only fail if some other code path already stored aCryptoExt(which currently doesn't exist). Worth mentioning in the doc thatset_crypto_provideris also a one-shot — repeat calls will returnErr(...)with the "already set" message — so downstream users know they shouldn't retry with a fresh provider.🤖 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 157 - 167, Update the documentation for the set_crypto_provider function to explicitly state it is a one-shot operation: calling set_crypto_provider more than once will fail (install_default or CRYPTO_EXT.set will return Err with the "already set" message), so callers should not expect to retry with a fresh CryptoProvider/CryptoExt; reference the function name set_crypto_provider and the types CryptoProvider, CryptoExt and the static CRYPTO_EXT when adding this note.
61-68: ⚡ Quick winConsider making
ticketeroptional like the other fields.
all_cipher_suites,all_kx_groups, andany_supported_keyare allOption<...>with a sensible fallback derived fromCryptoProvider::get_default().ticketer, however, is mandatory. This forces every custom-provider integrator to supply a ticketer constructor even when their deployment doesn't intend to use TLS 1.2 stateless resumption, and there's no symmetric fallback path. If a reasonable default is hard to derive fromCryptoProvideralone, wrapping it inOptionand treatingNoneas "no stateless tickets" keeps the API consistent.🤖 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 61 - 68, The CryptoExt struct currently requires a ticketer constructor but other fields are optional; change the ticketer field to Option<fn() -> Result<Arc<dyn ProducesTickets>, rustls::Error>> on the CryptoExt struct so callers can omit it (treat None as "no stateless tickets"); update any constructors, CryptoProvider::get_default() to return Some(...) or None as appropriate, and adjust call sites that invoke ticketer() (e.g., where Arc<dyn ProducesTickets> is created) to handle the None case safely by skipping stateless ticket setup.
🤖 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/stdlib/src/ssl.rs`:
- Around line 618-621: Replace the direct CRYPTO_EXT.get().expect(...) usage
with a small helper (e.g., fn get_crypto_provider(py: Python) ->
PyResult<&'static CryptoProvider> or similar) that returns a PyErr when no
provider is installed and use that helper in all call sites (places that
currently call CRYPTO_EXT.get().expect(...), including code paths used by
SSLContext, load_cert_chain, get_ciphers, RAND_bytes and the other noted
blocks). Update callers such as where all_suites =
CRYPTO_EXT.get().expect(...).all_ciphers_or_default() to call the helper and
propagate the PyResult (return Err(PyErr) on missing provider) instead of
panicking, ensuring each function returns a PyResult and maps the
missing-provider case to an appropriate Python exception.
In `@crates/stdlib/src/ssl/compat.rs`:
- Around line 105-137: The fallback branch of any_supported_key should not claim
the private key failed to parse — load_private_key already succeeded; the real
failure is that the parsed key does not support any allowed signing scheme.
Replace the loop that returns key on first matching scheme with a single call to
key.choose_scheme(...) using the filtered list of schemes from
provider.signature_verification_algorithms.mapping (exclude RSA_PKCS1_SHA1,
ECDSA_SHA1_Legacy, and Unknown(_)); if choose_scheme returns None return Err
with a corrected message mentioning "EdDSA" (not "EcDSA") or a neutral message
like "no supported signing scheme for private key". Update any_supported_key to
reflect this simplified intent and use the correct error text.
---
Outside diff comments:
In `@Cargo.toml`:
- Around line 14-27: The Cargo.toml default features include "ssl-rustls" so
enabling "ssl-rustls-no-provider" doesn't disable the default; fix by either
making the SSL features mutually exclusive or failing fast: (a) restructure
features so default no longer directly includes "ssl-rustls" — introduce an
intermediate feature like "ssl-default" that selects "ssl-rustls" and have
default = [..., "ssl-default", ...] so consumers can opt into
"ssl-rustls-no-provider" without the default pulling it in; or (b) add a
build-time check in build.rs that tests cfg!(feature = "ssl-rustls") &&
cfg!(feature = "ssl-rustls-no-provider") and emits a compile_error when both are
enabled. Update Cargo.toml feature lists (symbols: default, ssl-rustls,
ssl-rustls-no-provider, ssl-default) or add the build.rs guard accordingly.
In `@crates/stdlib/src/ssl/compat.rs`:
- Around line 52-60: The doc comment above the static CRYPTO_EXT is outdated:
replace the one-shot CryptoProvider installer description with a brief factual
note that CRYPTO_EXT is an OnceLock<CryptoExt> used to store a CryptoExt
extension object for the process, and mention that provider installation now
happens via set_crypto_provider and set_aws_lc_provider; update the comment to
describe its role (process-wide extension holder, thread-safe, initialized once)
and remove references to installing the default provider with Once.
---
Nitpick comments:
In `@crates/stdlib/src/ssl/compat.rs`:
- Around line 157-167: Update the documentation for the set_crypto_provider
function to explicitly state it is a one-shot operation: calling
set_crypto_provider more than once will fail (install_default or CRYPTO_EXT.set
will return Err with the "already set" message), so callers should not expect to
retry with a fresh CryptoProvider/CryptoExt; reference the function name
set_crypto_provider and the types CryptoProvider, CryptoExt and the static
CRYPTO_EXT when adding this note.
- Around line 61-68: The CryptoExt struct currently requires a ticketer
constructor but other fields are optional; change the ticketer field to
Option<fn() -> Result<Arc<dyn ProducesTickets>, rustls::Error>> on the CryptoExt
struct so callers can omit it (treat None as "no stateless tickets"); update any
constructors, CryptoProvider::get_default() to return Some(...) or None as
appropriate, and adjust call sites that invoke ticketer() (e.g., where Arc<dyn
ProducesTickets> is created) to handle the None case safely by skipping
stateless ticket setup.
🪄 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: ac58e6ca-b4f1-4560-a32f-8e16fbcb1081
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
Cargo.tomlcrates/stdlib/Cargo.tomlcrates/stdlib/src/lib.rscrates/stdlib/src/ssl.rscrates/stdlib/src/ssl/cert.rscrates/stdlib/src/ssl/compat.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/stdlib/src/ssl/cert.rs
| let crypto_ext = CRYPTO_EXT | ||
| .get() | ||
| .expect("A CryptoProvider must be set before SSL"); | ||
| let all_suites = crypto_ext.all_ciphers_or_default(); |
There was a problem hiding this comment.
Missing-provider paths panic instead of surfacing a Python error.
These expect(...) calls make normal Python entry points like SSLContext(), load_cert_chain(), get_ciphers(), and RAND_bytes() abort the embedding process when no provider has been installed yet. In ssl-rustls-no-provider mode that should be a regular PyResult failure, not a panic. Please funnel provider lookup through a small helper that returns a Python exception and reuse it at these call sites.
Also applies to: 1056-1059, 1533-1538, 2217-2219, 4892-4905
🤖 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.rs` around lines 618 - 621, Replace the direct
CRYPTO_EXT.get().expect(...) usage with a small helper (e.g., fn
get_crypto_provider(py: Python) -> PyResult<&'static CryptoProvider> or similar)
that returns a PyErr when no provider is installed and use that helper in all
call sites (places that currently call CRYPTO_EXT.get().expect(...), including
code paths used by SSLContext, load_cert_chain, get_ciphers, RAND_bytes and the
other noted blocks). Update callers such as where all_suites =
CRYPTO_EXT.get().expect(...).all_ciphers_or_default() to call the helper and
propagate the PyResult (return Err(PyErr) on missing provider) instead of
panicking, ensuring each function returns a PyResult and maps the
missing-provider case to an appropriate Python exception.
There was a problem hiding this comment.
Panicking is intentional. A provider should always be set. Returning an error is misleading in this case.
There was a problem hiding this comment.
@joshuamegnauth54, understood — the expect() calls here act as invariant assertions (programmer error, not a recoverable runtime condition). If a provider hasn't been installed before any SSL entry point is reached, that's a mis-use of the API that should surface loudly rather than silently returning a Python error. Acknowledged!
✏️ Learnings added
Learnt from: joshuamegnauth54
Repo: RustPython/RustPython PR: 7657
File: crates/stdlib/src/ssl.rs:618-621
Timestamp: 2026-05-11T01:44:15.932Z
Learning: In `crates/stdlib/src/ssl.rs` (RustPython/RustPython), `CRYPTO_EXT.get().expect("A CryptoProvider must be set before SSL")` panics intentionally. The design requires a crypto provider to always be installed before any SSL operations are invoked; a missing provider is a programmer/embedder error (an invariant violation), not a recoverable runtime condition. Do not suggest converting these `expect()` calls into `PyResult` error returns — it would be misleading by treating a configuration bug as a normal failure path.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: ShaharNaveh
Repo: RustPython/RustPython PR: 7181
File: crates/stdlib/src/csv.rs:20-20
Timestamp: 2026-02-17T07:54:36.200Z
Learning: In RustPython, ensure that modules using Python sequence-like types bring in extension methods by importing the trait (for example, 'use rustpython_vm::sliceable::SliceableSequenceOp;'). Even if the trait name isn't directly referenced in code, its methods (such as len()) become in-scope on types like PyStr. Add this import in modules that rely on those extension methods to avoid compilation errors.
62ac0af to
9757820
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/stdlib/Cargo.toml`:
- Around line 21-23: The ssl-rustls-fips feature currently lists "__ssl-rustls"
but does not enable the main "ssl-rustls" feature, so provider
export/initialization code gated on feature = "ssl-rustls" in
crates/stdlib/src/ssl.rs and src/interpreter.rs never runs; fix by making
ssl-rustls-fips imply ssl-rustls (i.e., add "ssl-rustls" to the ssl-rustls-fips
feature array in Cargo.toml) or alternatively update the feature gates in ssl.rs
and interpreter.rs to check for either "ssl-rustls" or "ssl-rustls-fips" so the
aws-lc provider initialization is enabled when ssl-rustls-fips is selected.
In `@src/interpreter.rs`:
- Around line 18-19: The call to
rustpython_stdlib::ssl::set_aws_lc_provider().unwrap() inside init_stdlib can
panic if a provider is already installed; replace the unwrap by either guarding
against repeated installation (check CryptoProvider::get_default() and only call
set_aws_lc_provider() when none is set) or propagate the error (replace unwrap
with ? and update init_stdlib’s return type to return Result<..., rustls::Error>
or an appropriate error type), ensuring you reference set_aws_lc_provider,
init_stdlib, and CryptoProvider::get_default when making the change.
🪄 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: f5d38454-b321-41b3-81d2-c8e433e27010
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
Cargo.tomlcrates/stdlib/Cargo.tomlcrates/stdlib/src/lib.rscrates/stdlib/src/ssl.rscrates/stdlib/src/ssl/cert.rscrates/stdlib/src/ssl/compat.rsexamples/tls_providers/Cargo.tomlexamples/tls_providers/src/main.rssrc/interpreter.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- Cargo.toml
- crates/stdlib/src/ssl/cert.rs
- crates/stdlib/src/lib.rs
- crates/stdlib/src/ssl/compat.rs
joshuamegnauth54
force-pushed
the
modularize-rustls-support
branch
from
9757820 to
b1552f4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/lib.rs`:
- Line 81: The error message string in src/lib.rs currently says "disable
ssl-rustls" but the feature was renamed; update that string literal to either
"disable ssl-rustls-aws-lc" or more generically "disable the default SSL
features" so the message correctly references the current feature name; locate
the message text in the source (the long error string containing "Feature
\"ssl\" is now enabled...") and replace the "disable ssl-rustls" fragment with
the chosen corrected wording.
🪄 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: e78d579c-9585-4795-a3d5-c548365d78a0
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
.github/workflows/ci.yaml.github/workflows/cron-ci.yaml.github/workflows/update-caches.ymlCargo.tomlcrates/stdlib/Cargo.tomlcrates/stdlib/src/lib.rscrates/stdlib/src/ssl.rscrates/stdlib/src/ssl/cert.rscrates/stdlib/src/ssl/compat.rsexamples/tls_providers/Cargo.tomlexamples/tls_providers/src/main.rssrc/interpreter.rssrc/lib.rs
✅ Files skipped from review due to trivial changes (1)
- examples/tls_providers/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (6)
- src/interpreter.rs
- crates/stdlib/src/lib.rs
- crates/stdlib/src/ssl/cert.rs
- crates/stdlib/src/ssl.rs
- crates/stdlib/src/ssl/compat.rs
- examples/tls_providers/src/main.rs
joshuamegnauth54
force-pushed
the
modularize-rustls-support
branch
2 times, most recently
from
692ba7f to
7a235dd
Compare
`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.
* Remove ring as an explicit feature * ssl-rustls is the default and implies aws-lc
The new feature, `ssl-rustls-no-provider`, enables custom rustls providers. By default, `aws-lc-rs` is enabled which matches the old behavior and keeps backward compatibility. I wrote a new type that abstracts what we need from crypto providers. CryptoExt encapsulates the ticketer as well as cipher suites and KX groups. I wrote fallbacks to help select a reasonable default if a provider is missing features (they all seem to support the same things though). I also wrote an example to show how to actually use custom providers.
joshuamegnauth54
force-pushed
the
modularize-rustls-support
branch
from
7a235dd to
76832e6
Compare
3 participants
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.The new feature,
ssl-rustls-no-provider, enables custom rustls providers. By default,aws-lc-rsis enabled which matches the old behavior and keeps backward compatibility.I wrote a new type that abstracts what we need from crypto providers. CryptoExt encapsulates the ticketer as well as cipher suites and KX groups. I wrote fallbacks to help select a reasonable default if a provider is missing features (they all seem to support the same things though).
I also wrote an example to show how to actually use custom providers.
Closes: #7059
aws-lc-rsis still the default, so this patch should be transparent to all users.I tested with:
And:
(todo, test with example!)
Summary by CodeRabbit
New Features
Chores