Implement more SSL methods #6210
Conversation
WalkthroughBumps Changes
Sequence Diagram(s)sequenceDiagram
participant Py as Python code
participant Ctx as PySslContext
participant Sock as PySslSocket
participant OpenSSL as OpenSSL FFI
Note over Ctx,OpenSSL #D6EAF8: Context operations
Py->>Ctx: get_ciphers()
Ctx->>OpenSSL: SSL_get_ciphers / EVP calls
OpenSSL-->>Ctx: cipher list
Ctx-->>Py: list of cipher dicts
Py->>Ctx: set_ecdh_curve(curve_name)
Ctx->>OpenSSL: OBJ_txt2nid / EC_KEY_new_by_curve_name
OpenSSL-->>Ctx: set temp ECDH key
Ctx-->>Py: OK / error
Note over Sock,OpenSSL #E8F8F5: Socket runtime ops
Py->>Sock: get_verified_chain()
Sock->>OpenSSL: Access verified chain (X509_verify_cert / chain retrieval)
OpenSSL-->>Sock: DER certs
Sock-->>Py: list[bytes] | None
Py->>Sock: shared_ciphers()
Sock->>OpenSSL: SSL_get_client_ciphers / SSL_get_ciphers
OpenSSL-->>Sock: client & server lists
Sock-->>Py: intersection list
Py->>Sock: selected_alpn_protocol()
Sock->>OpenSSL: SSL_get0_alpn_selected
OpenSSL-->>Sock: protocol | null
Sock-->>Py: protocol | None
Py->>Sock: get_channel_binding("tls-unique")
Sock->>OpenSSL: SSL_get_finished / TLS APIs
OpenSSL-->>Sock: binding bytes
Sock-->>Py: bytes | None
Py->>Sock: verify_client_post_handshake()
Sock->>OpenSSL: SSL_verify_client_post_handshake
OpenSSL-->>Sock: result / error
Sock-->>Py: OK / raises
Py->>Sock: shutdown()
Sock->>OpenSSL: SSL_shutdown sequence
OpenSSL-->>Sock: closed / error
Sock-->>Py: socket | raises
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (2)**/*.rs📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
{vm,stdlib}/**/*.rs📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
🧬 Code graph analysis (1)stdlib/src/ssl.rs (5)
⏰ 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)
🔇 Additional comments (17)
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 |
youknowone
force-pushed
the
ssl
branch
5 times, most recently
from
8a06226 to
2f9375a
Compare
youknowone
force-pushed
the
ssl
branch
4 times, most recently
from
9c870a9 to
a2c04ed
Compare
|
@ShaharNaveh I cut the commits to find what's breaking CI. test_ssl and the test files will be included in next patch |
![coderabbitai[bot]](https://waps.l3s.uni-hannover.de/live/im_/https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
stdlib/src/ssl.rs (3)
492-500: Protocol constants added but not enforced in context creation.PROTOCOL_TLSv1 / TLSv1_1 / TLSv1_2 map to SslMethod::tls(), but min/max versions aren’t restricted. As-is, TLS 1.3 (or higher) may still negotiate, breaking CPython parity. Set explicit min/max (or disable via options) per selected protocol.
Apply one of the following (preferred: min/max API):
@@ - let method = match proto { + let method = match proto { // SslVersion::Ssl3 => unsafe { ssl::SslMethod::from_ptr(sys::SSLv3_method()) }, SslVersion::Tls => ssl::SslMethod::tls(), - SslVersion::Tls1 => ssl::SslMethod::tls(), - SslVersion::Tls1_1 => ssl::SslMethod::tls(), - SslVersion::Tls1_2 => ssl::SslMethod::tls(), + SslVersion::Tls1 | SslVersion::Tls1_1 | SslVersion::Tls1_2 => ssl::SslMethod::tls(), SslVersion::TlsClient => ssl::SslMethod::tls_client(), SslVersion::TlsServer => ssl::SslMethod::tls_server(), _ => return Err(vm.new_value_error("invalid protocol version")), }; @@ - let mut options = SslOptions::ALL & !SslOptions::DONT_INSERT_EMPTY_FRAGMENTS; + let mut options = SslOptions::ALL & !SslOptions::DONT_INSERT_EMPTY_FRAGMENTS; @@ options |= SslOptions::NO_COMPRESSION; options |= SslOptions::CIPHER_SERVER_PREFERENCE; options |= SslOptions::SINGLE_DH_USE; options |= SslOptions::SINGLE_ECDH_USE; builder.set_options(options); + + // Enforce exact protocol selection for legacy constants. + match proto { + SslVersion::Tls1 => { + builder.set_min_proto_version(Some(ssl::SslVersion::TLS1)).unwrap(); + builder.set_max_proto_version(Some(ssl::SslVersion::TLS1)).unwrap(); + } + SslVersion::Tls1_1 => { + builder.set_min_proto_version(Some(ssl::SslVersion::TLS1_1)).unwrap(); + builder.set_max_proto_version(Some(ssl::SslVersion::TLS1_1)).unwrap(); + } + SslVersion::Tls1_2 => { + builder.set_min_proto_version(Some(ssl::SslVersion::TLS1_2)).unwrap(); + builder.set_max_proto_version(Some(ssl::SslVersion::TLS1_2)).unwrap(); + } + _ => {} + }Also applies to: 514-526, 125-129
767-773: ALPN callback slice bug (start..end range is wrong).
&client[pos..proto.len()]will panic whenpos > 0and returns the wrong slice. Usepos..pos + proto.len().- let pos = memchr::memmem::find(client, proto) + let pos = memchr::memmem::find(client, proto) .expect("selected alpn proto should be present in client protos"); - Ok(&client[pos..proto.len()]) + Ok(&client[pos..pos + proto.len()])
165-175: HAS_SSLv2/SSLv3 constants are misleading on modern OpenSSL.On OpenSSL ≥1.1.0 these protocols are removed/disabled; hardcoding
truecan mislead feature detection in Python code. Consider compile-time gating to reflect real support.- const HAS_SSLv2: bool = true; - const HAS_SSLv3: bool = true; + // Reflect actual availability (CPython-style semantics) + const HAS_SSLv2: bool = false; + const HAS_SSLv3: bool = cfg!(ossl101);
♻️ Duplicate comments (1)
stdlib/src/ssl.rs (1)
1256-1279: selected_alpn_protocol: LGTM after fixing the slice bug above.API usage is correct; returns None when not negotiated.
🧹 Nitpick comments (5)
stdlib/src/ssl.rs (5)
585-614: get_ciphers: OK overall; minor safety/robustness nits.Looks good. Optionally cast
num_cipherstoi32and guard against negative returns fromOPENSSL_sk_num(defensive).
616-641: set_ecdh_curve behavior on OpenSSL ≥1.1.0.
set_tmp_ecdhis effectively a no-op from 1.1.0 onward (auto ECDH). Consider documenting this or gating to older OpenSSL to avoid surprising users.
1193-1254: shared_ciphers O(N×M) but acceptable; consider set-based lookup if needed.Works under ossl110+. If performance becomes an issue, build a HashSet of client cipher pointers for O(N).
1281-1323: Channel binding (tls-unique): LGTM.XOR of session_reused and is_client matches CPython. Consider returning NotImplementedError for other cb types to align messages.
1347-1374: shutdown semantics vs CPython unwrap; potential reuse after unwrap.This returns the underlying socket without detaching/invalidating the SSLSocket. Consider aligning with CPython’s
unwrap()(or clearly documenting semantics), and ensure subsequent operations on the SSLSocket error cleanly. Also consider loopingSSL_shutdowntwice for bidirectional shutdown per OpenSSL guidance.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
stdlib/Cargo.toml(1 hunks)stdlib/src/ssl.rs(13 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Format Rust code with the default rustfmt style (runcargo fmt)
Run clippy and fix any warnings or lints introduced by your changes
Follow Rust best practices for error handling and memory management
Files:
stdlib/src/ssl.rs
{vm,stdlib}/**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use RustPython macros (
pyclass,pymodule,pyfunction, etc.) when implementing Python functionality in Rust
Files:
stdlib/src/ssl.rs
🧬 Code graph analysis (1)
stdlib/src/ssl.rs (2)
derive-impl/src/util.rs (1)
with(305-317)vm/src/exceptions.rs (1)
cstring_error(1143-1145)
🔇 Additional comments (5)
stdlib/Cargo.toml (1)
110-110: openssl-sys bump looks fine; please verify link across all CI targets.0.10.x openssl crate works with 0.9.110 openssl-sys. Still, ensure CI covers Linux, macOS, Windows, and vendored builds to catch symbol availability regressions.
stdlib/src/ssl.rs (4)
1147-1176: get_verified_chain: LGTM.Uses
SSL_get0_verified_chainand DER-encodes. Null/empty handling is correct.
1324-1345: verify_client_post_handshake: LGTM.Correctly gated on ossl111; returns NotImplemented otherwise.
1587-1587: Minor: Good fix truncating read buffer before returning.Prevents trailing zeros; matches Python semantics.
1683-1697: Extern FFI symbols: gate and test across providers.Declaring missing symbols locally is fine; ensure:
SSL_get_ciphersis present on all supported OpenSSL/LibreSSL builds.SSL_get_client_ciphersandSSL_verify_client_post_handshakeremain under proper cfgs.
Please verify CI matrix includes LibreSSL (if supported) or guard with additional cfgs to avoid link errors.
2 participants
Summary by CodeRabbit
New Features
Chores