Implement more SSL methods - #6210
Conversation
Sorry, something went wrong.
WalkthroughBumps openssl-sys to 0.9.110 and expands the SSL/TLS implementation: adds TLS1.1/1.2, enables ECDH, introduces new OpenSSL symbol shims, and exposes multiple new PySslContext/PySslSocket APIs for ciphers, ECDH curve, verify flags, verified chains, ALPN, channel binding, post-handshake verification, and shutdown. 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)
📜 Recent review details Configuration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro 📥 CommitsReviewing files that changed from the base of the PR and between c3dad1d and 91df67c. ⛔ Files ignored due to path filters (1)
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
derive/src/lib.rs (1)⏰ 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)
stdlib/src/ssl.rs (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. ❤️ ShareComment @coderabbitai help to get the list of available commands and usage tips. |
Sorry, something went wrong.
ShaharNaveh
left a comment
There was a problem hiding this comment.
Can you please remove the old certs like
Sorry, something went wrong.
|
@ShaharNaveh I cut the commits to find what's breaking CI. test_ssl and the test files will be included in next patch |
Sorry, something went wrong.
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 when pos > 0 and returns the wrong slice. Use pos..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 true can 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);
stdlib/src/ssl.rs (1)🧹 Nitpick comments (5)1256-1279: selected_alpn_protocol: LGTM after fixing the slice bug above.
API usage is correct; returns None when not negotiated.
stdlib/src/ssl.rs (5)📜 Review details585-614: get_ciphers: OK overall; minor safety/robustness nits.
Looks good. Optionally cast num_ciphers to i32 and guard against negative returns from OPENSSL_sk_num (defensive).
616-641: set_ecdh_curve behavior on OpenSSL ≥1.1.0.
set_tmp_ecdh is 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 looping SSL_shutdown twice for bidirectional shutdown per OpenSSL guidance.
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📥 CommitsReviewing files that changed from the base of the PR and between 1d23f6e and c3dad1d.
📒 Files selected for processing (2)- stdlib/Cargo.toml (1 hunks)
- stdlib/src/ssl.rs (13 hunks)
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Format Rust code with the default rustfmt style (run cargo 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
📄 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
derive-impl/src/util.rs (1)🔇 Additional comments (5)vm/src/exceptions.rs (1)
- with (305-317)
- cstring_error (1143-1145)
stdlib/Cargo.toml (1)stdlib/src/ssl.rs (4)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.
1147-1176: get_verified_chain: LGTM.
Uses SSL_get0_verified_chain and 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_ciphers is present on all supported OpenSSL/LibreSSL builds.
- SSL_get_client_ciphers and SSL_verify_client_post_handshake remain under proper cfgs.
Please verify CI matrix includes LibreSSL (if supported) or guard with additional cfgs to avoid link errors.
Sorry, something went wrong.
Summary by CodeRabbit
New Features
Chores