fix: Fix Windows Schannel TLS and make it independent of OpenSSL - #1005
Open
emmettlu wants to merge 5 commits into
Open
fix: Fix Windows Schannel TLS and make it independent of OpenSSL#1005emmettlu wants to merge 5 commits into
emmettlu wants to merge 5 commits into
Conversation
Drain leftover handshake records, handle TLS 1.3 SEC_I_RENEGOTIATE, send close_notify, and acquire credentials with SCH_CREDENTIALS. Add a Schannel server acceptor so client tests no longer need OpenSSL, expose the ntex schannel feature on ClientBuilder and http::schannel, and keep Schannel usable without openssl or ws.
There was a problem hiding this comment.
🟡 Changes recommended
The new Schannel test performs a real network call to https://example.com/, which is likely to be flaky in CI and should be made opt-in (e.g., #[ignore]) or replaced with a deterministic local test.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR strengthens Windows Schannel TLS support in ntex-tls (client fixes + new server acceptor) and wires Schannel through ntex so Windows TLS can be used without OpenSSL, while also making ws optional for builds and tests.
Changes:
- Fix Schannel client behavior (leftover handshake record draining, TLS 1.3 post-handshake
SEC_I_RENEGOTIATE, sendclose_notify, useSCH_CREDENTIALS). - Add Schannel server support (
ServerConfig,TlsAcceptor,http::schannel) and update tests to use Schannel instead of OpenSSL on Windows. - Expose Schannel on
ClientBuilderandWsClient, and gate websocket-related test helpers behind thewsfeature.
File summaries
| File | Description |
|---|---|
| ntex/tests/http_client_schannel.rs | Switch Schannel test to Schannel server acceptor; add a public HTTPS smoke test. |
| ntex/tests/connect.rs | Update Schannel test to use Schannel TlsAcceptor/ServerConfig and simplify cert checks. |
| ntex/src/ws/client.rs | Add WsClient::schannel(...) constructor for Windows Schannel. |
| ntex/src/web/test.rs | Make websocket test client configuration conditional on ws; add Schannel path for WSS when OpenSSL is off. |
| ntex/src/web/error_default.rs | Gate websocket-specific imports/conversions behind ws. |
| ntex/src/lib.rs | Re-export Schannel connectors/configs under connect::schannel and server::schannel. |
| ntex/src/http/test.rs | Gate websocket client config in HTTP test server helpers behind ws. |
| ntex/src/http/mod.rs | Add http::schannel(...) service wrapper and include Schannel in TLS feature gating. |
| ntex/src/client/builder.rs | Add ClientBuilder::schannel(...) and default to Schannel on Windows when it’s the only TLS backend. |
| ntex/CHANGES.md | Document new schannel feature wiring and “build without ws” capability. |
| ntex/Cargo.toml | Add schannel feature and adjust dev-dependencies for TLS testing on Windows. |
| ntex-tls/src/schannel/mod.rs | Core Schannel implementation updates (client fixes + server-side handshake path + close_notify). |
| ntex-tls/src/schannel/connect.rs | Add TlsConnector::connect(...) for wrapping existing Io and refactor Service impl to use it. |
| ntex-tls/src/schannel/cert.rs | Add Schannel ServerConfig that loads PEM cert/key for inbound TLS. |
| ntex-tls/src/schannel/accept.rs | Add Schannel TlsAcceptor service with handshake timeout + connection limiting. |
| ntex-tls/CHANGES.md | Document Schannel client fixes and new server acceptor. |
| ntex-tls/Cargo.toml | Update crate description and adjust dev-dependencies for new test setup. |
Review details
- Files reviewed: 17/17 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+68
to
+69
| /// Load a PEM-encoded certificate and PKCS#8 private key. | ||
| pub fn from_pem(cert_pem: &str, key_pem: &str) -> io::Result<Self> { |
Comment on lines
+52
to
+54
| #[ntex::test] | ||
| async fn test_schannel_public_https() { | ||
| let tls = TlsConnector::<ntex::connect::Connector<ntex::http::Uri>>::new(); |
Incomplete TLS records were left in ntex's read source, which paused further socket reads. Hosts such as cn.bing.com then hit the 5s handshake timeout. Keep unread ciphertext in Context.enc_buf, drain the ntex source on every process_read_buf, and add a Bing HTTPS client test.
TlsAcceptor::map_err comes from ntex::service::Service. Dropping that import broke every CI job that compiles ntex tests with rustls. Restore ntex-tls's workspace ntex dev-dependency with openssl/rustls so examples keep compiling.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1005 +/- ##
==========================================
- Coverage 89.61% 89.61% -0.01%
==========================================
Files 250 250
Lines 35804 35804
==========================================
- Hits 32085 32084 -1
- Misses 3719 3720 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
Author
|
so what to fix next? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Drain leftover handshake records, handle TLS 1.3 SEC_I_RENEGOTIATE, send close_notify, and acquire credentials with SCH_CREDENTIALS.
Add a Schannel server acceptor so client tests no longer need OpenSSL, expose the ntex schannel feature on ClientBuilder and http::schannel, and keep Schannel usable without openssl or ws.