refactor!: consolidate rustls forks via [patch.crates-io]#1395
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughCargo overrides for ChangesTLS migration and Cargo overrides
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
clash-lib/src/proxy/transport/reality.rs (1)
18-20: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winConsider sharing the root store with the rest of the crate.
Now that this file uses the same
rustls::RootCertStoreasshadow_tls/mod.rs, the localinit_roots()+ per-ClientOnceLock<Arc<RootCertStore>>duplicates whatcrate::common::tls::GLOBAL_ROOT_STOREalready provides. Consolidating would remove DRY drift, save the per-instance allocation, and ensure a single source of truth for trusted roots across TLS transports.♻️ Sketch
-use rustls::{ - ClientConfig, RootCertStore, client::RealityConfig, pki_types::ServerName, -}; +use rustls::{ClientConfig, client::RealityConfig, pki_types::ServerName}; use tokio_rustls::{TlsConnector, client::TlsStream}; use std::{ io, ops::Deref, - sync::{Arc, OnceLock, atomic::AtomicBool}, + sync::{Arc, atomic::AtomicBool}, }; -use crate::proxy::{ - AnyStream, - transport::{Transport, VisionOptions, splice_tls::SplicableTlsStream}, -}; - -fn init_roots() -> Arc<RootCertStore> { - Arc::new(webpki_roots::TLS_SERVER_ROOTS.iter().cloned().collect()) -} +use crate::{ + common::tls::GLOBAL_ROOT_STORE, + proxy::{ + AnyStream, + transport::{Transport, VisionOptions, splice_tls::SplicableTlsStream}, + }, +}; @@ - let tls_config = ClientConfig::builder() - .with_root_certificates(self.roots.get_or_init(init_roots).clone()) - .with_reality(reality) - .with_no_client_auth(); + let tls_config = ClientConfig::builder() + .with_root_certificates(GLOBAL_ROOT_STORE.clone()) + .with_reality(reality) + .with_no_client_auth();The
roots: OnceLock<Arc<RootCertStore>>field onClientInnercan then be removed.🤖 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 `@clash-lib/src/proxy/transport/reality.rs` around lines 18 - 20, The local init_roots() and the per-Client OnceLock<Arc<RootCertStore>> duplicate the crate-wide ROOT store; replace uses of init_roots() and the ClientInner field roots: OnceLock<Arc<RootCertStore>> with the shared crate::common::tls::GLOBAL_ROOT_STORE (clone the Arc where needed), remove the init_roots() function and the roots field from ClientInner, and update any client construction or TLS setup code to fetch GLOBAL_ROOT_STORE.clone() so all transports use the single global RootCertStore.
🤖 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 `@Cargo.toml`:
- Around line 48-50: The Cargo.toml patch entries for rustls and tokio-rustls
currently use branch= which tracks moving tips; change them to pin specific
commit SHAs by replacing branch = "watfaq/0.23.40" and branch = "watfaq/0.26.4"
with rev = "<commit-sha>" for the corresponding commits in the Watfaq forks
(update both rustls and tokio-rustls entries), ensuring you pick the exact
commit SHAs you want to lock to and run cargo update to refresh Cargo.lock.
---
Outside diff comments:
In `@clash-lib/src/proxy/transport/reality.rs`:
- Around line 18-20: The local init_roots() and the per-Client
OnceLock<Arc<RootCertStore>> duplicate the crate-wide ROOT store; replace uses
of init_roots() and the ClientInner field roots: OnceLock<Arc<RootCertStore>>
with the shared crate::common::tls::GLOBAL_ROOT_STORE (clone the Arc where
needed), remove the init_roots() function and the roots field from ClientInner,
and update any client construction or TLS setup code to fetch
GLOBAL_ROOT_STORE.clone() so all transports use the single global RootCertStore.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 98ea0b6e-9ce7-474e-9176-443226d21da7
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
Cargo.tomlclash-lib/Cargo.tomlclash-lib/src/lib.rsclash-lib/src/proxy/transport/reality.rsclash-lib/src/proxy/transport/shadow_tls/mod.rsclash-lib/src/proxy/transport/splice_tls.rs
💤 Files with no reviewable changes (1)
- clash-lib/src/lib.rs
| [patch.crates-io] | ||
| rustls = { git = "https://github.com/Watfaq/rustls.git", branch = "watfaq/0.23.40" } | ||
| tokio-rustls = { git = "https://github.com/Watfaq/tokio-rustls.git", branch = "watfaq/0.26.4" } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- Watfaq/rustls @ watfaq/0.23.40 ---"
git ls-remote https://github.com/Watfaq/rustls.git refs/heads/watfaq/0.23.40
echo "--- Watfaq/tokio-rustls @ watfaq/0.26.4 ---"
git ls-remote https://github.com/Watfaq/tokio-rustls.git refs/heads/watfaq/0.26.4Repository: Watfaq/clash-rs
Length of output: 275
🏁 Script executed:
# First, verify the root Cargo.toml has the patch section mentioned
cat -n Cargo.toml | sed -n '45,55p'Repository: Watfaq/clash-rs
Length of output: 352
🏁 Script executed:
# Find and examine clash-lib/Cargo.toml for git dependencies
fd -name "Cargo.toml" -path "*/clash-lib/*"Repository: Watfaq/clash-rs
Length of output: 290
🏁 Script executed:
# Search for all git dependencies in Cargo.toml files to verify the pattern
rg "git = " --type toml -B1 -A1Repository: Watfaq/clash-rs
Length of output: 3035
Pin patched rustls/tokio-rustls to specific commits, not branches.
Using branch = makes cargo update (or a fresh Cargo.lock) silently follow whatever the branch tip becomes, and the branch can be force-pushed or rewritten in the fork. The workspace predominantly pins git dependencies to rev = (e.g. shadowsocks-rust, boring-noise, smoltcp, shadowquic, unix-udp-sock, sock2proc) — patches replacing core TLS crates should follow this pattern.
🔒 Suggested change: pin to commit SHAs
[patch.crates-io]
-rustls = { git = "https://github.com/Watfaq/rustls.git", branch = "watfaq/0.23.40" }
-tokio-rustls = { git = "https://github.com/Watfaq/tokio-rustls.git", branch = "watfaq/0.26.4" }
+rustls = { git = "https://github.com/Watfaq/rustls.git", rev = "8211697ce28686d72c92c2fc4b440b7ffc9a3ee3" }
+tokio-rustls = { git = "https://github.com/Watfaq/tokio-rustls.git", rev = "b26e3e2b7a0161d505fd12d6e545b16463f1a45f" }📝 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.
| [patch.crates-io] | |
| rustls = { git = "https://github.com/Watfaq/rustls.git", branch = "watfaq/0.23.40" } | |
| tokio-rustls = { git = "https://github.com/Watfaq/tokio-rustls.git", branch = "watfaq/0.26.4" } | |
| [patch.crates-io] | |
| rustls = { git = "https://github.com/Watfaq/rustls.git", rev = "8211697ce28686d72c92c2fc4b440b7ffc9a3ee3" } | |
| tokio-rustls = { git = "https://github.com/Watfaq/tokio-rustls.git", rev = "b26e3e2b7a0161d505fd12d6e545b16463f1a45f" } |
🤖 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 48 - 50, The Cargo.toml patch entries for rustls and
tokio-rustls currently use branch= which tracks moving tips; change them to pin
specific commit SHAs by replacing branch = "watfaq/0.23.40" and branch =
"watfaq/0.26.4" with rev = "<commit-sha>" for the corresponding commits in the
Watfaq forks (update both rustls and tokio-rustls entries), ensuring you pick
the exact commit SHAs you want to lock to and run cargo update to refresh
Cargo.lock.
ibigbug
changed the title
📊 Proxy Throughput ResultsShadowsocks
Trojan
VMess
VLESS
SOCKS5
AnyTLS
Hysteria2
TUIC
ShadowQUIC
SSH
Netem Tests (50 ms delay, 1% packet loss)Shadowsocks
Trojan
Hysteria2
TUIC
ShadowQUIC
Ran 34 variant(s) in parallel; each direction transfers the full payload. 🖥️ Test Environment
📎 View full workflow run and download artifacts Full test logDownload the |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
1 participant
Breaking Change
Removes
watfaq-rustlsandtokio-watfaq-rustlsgit dependencies in favour of patching crates-iorustlsandtokio-rustlswith the Watfaq forks via[patch.crates-io].This means there is now a single rustls instance in the binary — no more duplicate global crypto provider state.
Changes
[patch.crates-io]forrustls→ Watfaq/rustls@watfaq/0.23.40[patch.crates-io]fortokio-rustls→ Watfaq/tokio-rustls@watfaq/0.26.4watfaq_rustls::/tokio_watfaq_rustls::imports withrustls::/tokio_rustls::crypto provider install_default()call inlib.rs(no longer two separate rustls copies)watfaq-dns/aws-lc-rsandwatfaq-dns/ringfeature forwards accidentally dropped in previous commitFork branches
Both fork branches are clean — created directly from the upstream release tag with only the custom patches applied (0 commits behind, minimal ahead):
watfaq/0.23.40new_with_session_id_generatorwatfaq/0.26.4connect_with_session_id_generatorfor Shadow-TLS V3