Open
Conversation
add explicit bootstrap trust config and fail-closed framing
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdded bootstrap configuration and getters, wired bootstrap values into rendezvous/API/update resolution; options now support encrypted storage/decryption; added transport-security detection APIs; introduced a 64 MiB default packet-length cap and related tests; updated protobufs to carry a licence_key and new register results. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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
🧹 Nitpick comments (2)
src/bytes_codec.rs (1)
302-315: Consider adding a boundary acceptance test.The rejection tests are good. For completeness, consider adding a test that verifies a packet exactly at
DEFAULT_MAX_PACKET_LENGTHis accepted (returnsOk(None)since there's no payload). This would confirm the boundary is inclusive.🧪 Optional boundary test
+ #[test] + fn test_codec_accepts_packet_at_default_limit() { + let mut codec = BytesCodec::new(); + let mut buf = encode_head_only(DEFAULT_MAX_PACKET_LENGTH); + // Should succeed (returns Ok(None) since payload is missing) + assert!(codec.decode(&mut buf).is_ok()); + } + #[test] fn test_codec_rejects_packet_above_default_limit() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bytes_codec.rs` around lines 302 - 315, Add a boundary acceptance test that verifies a packet with a header size exactly equal to DEFAULT_MAX_PACKET_LENGTH is accepted: create a BytesCodec via BytesCodec::new(), prepare a buffer with encode_head_only(DEFAULT_MAX_PACKET_LENGTH), call codec.decode(&mut buf) and assert it returns Ok(None) (no payload). Optionally add a second case for the custom limit by calling codec.set_max_packet_length(16) and testing encode_head_only(16) similarly; reference BytesCodec, DEFAULT_MAX_PACKET_LENGTH, set_max_packet_length, encode_head_only, and decode to locate where to add the test.src/websocket.rs (1)
390-400: Add test coverage for bootstrap api_server WebSocket scheme selection.The bootstrap-first api_server resolution is correctly implemented. However, the existing test_check_ws function does not verify that bootstrap api_server takes precedence over the fallback api-server option.
Consider adding a test case using
Config::set_bootstrap_config_for_test()with an https api_server to ensure the WebSocket scheme respects bootstrap priority:♻️ Suggested test addition
// In test_check_ws, add a test for bootstrap api_server: let saved_bootstrap = Config::BOOTSTRAP_CONFIG.read().unwrap().clone(); Config::set_bootstrap_config_for_test(BootstrapConfig { api_server: "https://bootstrap.example.com".to_owned(), ..Default::default() }); Config::set_option("api-server".to_string(), "http://fallback.example.com".to_string()); assert_eq!(check_ws("rustdesk.com:21116"), "wss://rustdesk.com/ws/id"); // Restore original bootstrap config Config::set_bootstrap_config_for_test(saved_bootstrap);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/websocket.rs` around lines 390 - 400, Add a unit test in test_check_ws that verifies bootstrap config overrides the fallback option: save the current Config::BOOTSTRAP_CONFIG, call Config::set_bootstrap_config_for_test(...) with api_server = "https://bootstrap.example.com", set the fallback via Config::set_option("api-server", "http://fallback.example.com"), then call check_ws("rustdesk.com:21116") and assert it returns the "wss://rustdesk.com/ws/id" result, and finally restore the saved bootstrap config; this ensures the WebSocket scheme selection logic (the api_server resolution used in check_ws) prefers bootstrap settings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/config.rs`:
- Around line 1308-1328: The code silently drops long passphrases because
encrypt_str_or_original returns an empty string for inputs beyond
ENCRYPT_MAX_LEN; update maybe_encrypt_option_value (and/or the caller
set_option) to validate length and handle encryption failures: if
is_encrypted_option(k) and v is non-empty, first check v.len() against
ENCRYPT_MAX_LEN and return an error or trim with a clear log/warning to the
user; after calling encrypt_str_or_original, detect an empty return as an
encryption failure and log an error/propagate a Result rather than replacing the
value silently. Reference functions: maybe_encrypt_option_value,
maybe_decrypt_option_value, is_encrypted_option, encrypt_str_or_original, and
set_option when adding validation/error propagation.
---
Nitpick comments:
In `@src/bytes_codec.rs`:
- Around line 302-315: Add a boundary acceptance test that verifies a packet
with a header size exactly equal to DEFAULT_MAX_PACKET_LENGTH is accepted:
create a BytesCodec via BytesCodec::new(), prepare a buffer with
encode_head_only(DEFAULT_MAX_PACKET_LENGTH), call codec.decode(&mut buf) and
assert it returns Ok(None) (no payload). Optionally add a second case for the
custom limit by calling codec.set_max_packet_length(16) and testing
encode_head_only(16) similarly; reference BytesCodec, DEFAULT_MAX_PACKET_LENGTH,
set_max_packet_length, encode_head_only, and decode to locate where to add the
test.
In `@src/websocket.rs`:
- Around line 390-400: Add a unit test in test_check_ws that verifies bootstrap
config overrides the fallback option: save the current Config::BOOTSTRAP_CONFIG,
call Config::set_bootstrap_config_for_test(...) with api_server =
"https://bootstrap.example.com", set the fallback via
Config::set_option("api-server", "http://fallback.example.com"), then call
check_ws("rustdesk.com:21116") and assert it returns the
"wss://rustdesk.com/ws/id" result, and finally restore the saved bootstrap
config; this ensures the WebSocket scheme selection logic (the api_server
resolution used in check_ws) prefers bootstrap settings.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3e7ff9eb-a0da-4182-9dee-3b0d0cb64a9d
📒 Files selected for processing (5)
src/bytes_codec.rssrc/config.rssrc/lib.rssrc/stream.rssrc/websocket.rs
Signed-off-by: Vlad (Kuzmin) Erium <libalias@gmail.com>
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 (1)
src/config.rs (1)
1255-1274:⚠️ Potential issue | 🟠 MajorAvoid stale snapshot when restoring rejected encrypted options.
Line 1255 snapshots
previous_optionsunder a read lock, but reconciliation happens later; a concurrentset_option()can update a key in between and then get overwritten with stale data. Reconcile rejected keys againstconfig.optionsafter acquiring the write lock.💡 Suggested fix
pub fn set_options(mut v: HashMap<String, String>) { Self::purify_options(&mut v); - let previous_options = CONFIG2.read().unwrap().options.clone(); let mut rejected_keys = Vec::new(); for (key, value) in v.iter_mut() { if !Self::maybe_encrypt_option_value(key, value) { rejected_keys.push(key.clone()); } } - for key in rejected_keys { - if let Some(old) = previous_options.get(&key) { - v.insert(key, old.clone()); - } else { - v.remove(&key); - } - } let mut config = CONFIG2.write().unwrap(); + for key in rejected_keys { + if let Some(old) = config.options.get(&key) { + v.insert(key, old.clone()); + } else { + v.remove(&key); + } + } if config.options == v { return; } config.options = v; config.store(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config.rs` around lines 1255 - 1274, The snapshot of previous_options taken from CONFIG2.read() is stale during reconciliation; change the flow to acquire CONFIG2.write() before reconciling rejected_keys so you reconcile against the live config.options instead of the earlier snapshot. Specifically: keep building v and rejected_keys using Self::maybe_encrypt_option_value, then acquire let mut config = CONFIG2.write().unwrap() and use config.options as the source of truth when restoring values for keys in rejected_keys (update v from config.options or remove if absent), then compare config.options == v, assign config.options = v and call config.store(). Ensure references to previous_options/read lock are removed and set_option race conditions are avoided by holding the write lock during reconciliation.
🧹 Nitpick comments (2)
src/config.rs (2)
923-924: Use the existing key constant instead of string literals.
"custom-rendezvous-server"is duplicated here whilekeys::OPTION_CUSTOM_RENDEZVOUS_SERVERalready exists. Using the constant avoids drift/typos.♻️ Suggested cleanup
- rendezvous_server = Self::get_option("custom-rendezvous-server"); + rendezvous_server = Self::get_option(keys::OPTION_CUSTOM_RENDEZVOUS_SERVER); ... - let s = Self::get_option("custom-rendezvous-server"); + let s = Self::get_option(keys::OPTION_CUSTOM_RENDEZVOUS_SERVER);Also applies to: 940-941
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config.rs` around lines 923 - 924, Replace the duplicated string literal "custom-rendezvous-server" with the existing constant keys::OPTION_CUSTOM_RENDEZVOUS_SERVER wherever it’s used (e.g., the call rendezvous_server = Self::get_option("custom-rendezvous-server")). Update all occurrences (also the similar use around the other block noted) to call Self::get_option(keys::OPTION_CUSTOM_RENDEZVOUS_SERVER) so the code uses the canonical constant and avoids drift/typos.
3562-3634: Make test global-state restoration panic-safe.These tests restore globals only at the end; if an assertion fails earlier, later tests can inherit mutated global state. Wrap restoration in a drop guard to guarantee cleanup.
Also applies to: 3638-3654, 3658-3676
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config.rs` around lines 3562 - 3634, The test mutates globals (BOOTSTRAP_CONFIG/EXE_RENDEZVOUS_SERVER/PROD_RENDEZVOUS_SERVER/CONFIG2) and currently restores them only at the end, which can leak state on panic; fix by creating a small RAII drop guard (e.g., RestoreGuard) that captures saved_bootstrap, saved_exe, saved_prod, saved_config2 and implements Drop to restore them (calling Config::set_bootstrap_config_for_test and writing back the other globals). Instantiate this guard before you mutate any globals (before calling Config::set_bootstrap_config_for_test with the test bootstrap and before modifying EXE_RENDEZVOUS_SERVER/PROD_RENDEZVOUS_SERVER/CONFIG2) so restoration happens even if assertions panic.
🤖 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 `@src/config.rs`:
- Around line 1255-1274: The snapshot of previous_options taken from
CONFIG2.read() is stale during reconciliation; change the flow to acquire
CONFIG2.write() before reconciling rejected_keys so you reconcile against the
live config.options instead of the earlier snapshot. Specifically: keep building
v and rejected_keys using Self::maybe_encrypt_option_value, then acquire let mut
config = CONFIG2.write().unwrap() and use config.options as the source of truth
when restoring values for keys in rejected_keys (update v from config.options or
remove if absent), then compare config.options == v, assign config.options = v
and call config.store(). Ensure references to previous_options/read lock are
removed and set_option race conditions are avoided by holding the write lock
during reconciliation.
---
Nitpick comments:
In `@src/config.rs`:
- Around line 923-924: Replace the duplicated string literal
"custom-rendezvous-server" with the existing constant
keys::OPTION_CUSTOM_RENDEZVOUS_SERVER wherever it’s used (e.g., the call
rendezvous_server = Self::get_option("custom-rendezvous-server")). Update all
occurrences (also the similar use around the other block noted) to call
Self::get_option(keys::OPTION_CUSTOM_RENDEZVOUS_SERVER) so the code uses the
canonical constant and avoids drift/typos.
- Around line 3562-3634: The test mutates globals
(BOOTSTRAP_CONFIG/EXE_RENDEZVOUS_SERVER/PROD_RENDEZVOUS_SERVER/CONFIG2) and
currently restores them only at the end, which can leak state on panic; fix by
creating a small RAII drop guard (e.g., RestoreGuard) that captures
saved_bootstrap, saved_exe, saved_prod, saved_config2 and implements Drop to
restore them (calling Config::set_bootstrap_config_for_test and writing back the
other globals). Instantiate this guard before you mutate any globals (before
calling Config::set_bootstrap_config_for_test with the test bootstrap and before
modifying EXE_RENDEZVOUS_SERVER/PROD_RENDEZVOUS_SERVER/CONFIG2) so restoration
happens even if assertions panic.
Signed-off-by: Vlad (Kuzmin) Erium <libalias@gmail.com>
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
1 participant
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.
This is a part of RustDesk client security hardening PR:
Summary
This PR adds the shared transport and bootstrap changes needed for a stricter client trust model.
Main changes:
trusted-peerssupport for preseeded peer identity trustWhy
The previous client behavior mixed mutable runtime config with implicit fallback behavior. In practice, that meant:
This PR moves the shared layer toward explicit trust and fail-closed behavior.
Design Decisions
1. Separate bootstrap trust roots from mutable runtime options
BootstrapConfigis loaded from a dedicated-bootstrapfile and is intended for values that should not silently drift at runtime:That keeps deployer-provided trust roots separate from ordinary mutable option state.
2. Remove implicit rendezvous fallback
When no explicit bootstrap or user-provided rendezvous server exists, shared config resolution now stays empty instead of reintroducing legacy fallback paths.
This is important for:
3. Preseed peer trust with bootstrap
trusted-peersBootstrap
trusted-peersallows a distributor to ship known peer signing keys up front. This supports first-contact trust without depending only on interactive trust-on-first-use.4. Bound framed message size
The frame decoder now rejects oversized packets before reserving memory. This is a direct hardening step against remote memory exhaustion.
5. Encrypt pairing passphrases at rest
Pairing passphrase options are now treated like other protected secrets and stored encrypted rather than plaintext.
Operational Impact
This PR changes shared config behavior in an intentional way:
Compatibility
This PR is intended to be consumed together with the matching
rustdesk-clientsecurity PR. The client PR uses:Example Bootstrap Config
The matching client PR adds an example bootstrap file showing the intended packaging model:
rendezvous-serverskeyapi-serverupdate-server[trusted-peers]Testing
Verified in the client workspace against the matching branch with:
Summary by CodeRabbit
New Features
Bug Fixes
Behavior Changes