Harden local RC authentication, host key validation, and command IPC - #44
Conversation
|
Warning Review limit reached
Next review available in: 39 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (16)
📝 Walkthrough📝 Walkthrough🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/mountmate-core/src/rclone.rs (1)
100-154: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse the normalized port value for
known_hosts_markerin rclone mode
validate_port(port)?;rejects invalid ports but discards the checked numeric value, soknown_hosts_marker(host, port)uses the raw port string. SSH handling normalizes022/08022to22/8022before computing markers, so this path can reject a valid existing binding due to leading-zeros formatting. Share/import the SSH normalize/validate helper and pass its normalized port tooptions,openssh_command, andknown_hosts_marker.🤖 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 `@crates/mountmate-core/src/rclone.rs` around lines 100 - 154, Update the port handling in the rclone configuration flow around validate_port and known_hosts_marker to retain the normalized numeric port value instead of the raw string. Reuse the existing SSH normalize/validate helper, and use its normalized result consistently for the port option, openssh_command, and known_hosts_marker so leading-zero ports resolve identically across paths.
🧹 Nitpick comments (1)
crates/mountmate-core/src/storage.rs (1)
602-612: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winCreate the temp file already-private instead of chmod-after-open.
On unix the temporary file exists with umask permissions between
openandrestrict_private_path. Content is only written afterwards, so nothing leaks directly, but a concurrent opener in the parent directory can grab a readable fd during that window and read the secret after the write. Setting the mode at creation closes it; the post-renamecall is then also redundant sincerenamepreserves the mode.🔒 Proposed hardening
let result = (|| { - let mut file = OpenOptions::new() - .create(true) - .truncate(true) - .write(true) - .open(&temporary)?; - restrict_private_path(&temporary, false)?; + let mut options = OpenOptions::new(); + options.create(true).truncate(true).write(true); + #[cfg(unix)] + { + use std::os::unix::fs::OpenOptionsExt; + options.mode(0o600); + } + let mut file = options.open(&temporary)?; + restrict_private_path(&temporary, false)?; file.write_all(content)?;🤖 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 `@crates/mountmate-core/src/storage.rs` around lines 602 - 612, Update the temporary-file creation in the atomic write closure around OpenOptions to request private permissions at creation time using the platform-appropriate mode-setting option, before any content is written. Remove the now-redundant restrict_private_path(&temporary, false) call, while preserving the existing post-rename behavior only if needed for the destination.
🤖 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 `@crates/mountmate-core/src/app_command.rs`:
- Around line 513-520: Update the unauthenticated handling around
read_message_until and the error response write so the response’s
write_all/flush cannot outlive UNAUTHENTICATED_DEADLINE. Reuse the same deadline
for the pre-auth response, or apply a substantially shorter write timeout,
ensuring total worker occupancy remains bounded near the authentication deadline
while preserving the existing error-response behavior.
In `@crates/mountmate-core/src/runtime.rs`:
- Around line 652-659: Update write_rc_htpasswd and RC_BCRYPT_COST to avoid
bcrypt cost 10 for repeated per-request loopback RC authentication. Use the
lower bcrypt cost established for this private 64-hex secret, while preserving
the existing password hashing, atomic write, and RuntimeError mapping behavior.
In `@docs/development-roadmap.md`:
- Line 29: Update the completion date in the roadmap entry beginning
“Implemented and independently reviewed” so it is not future-dated: use the
actual completion/review date, or revise it after the review has completed.
- Around line 35-37: Update the PID-reuse/stale cleanup description in the
roadmap to state that rc_htpasswd_file is retained when PID identity changes or
process ownership is unverifiable, and removed only during verified
readiness-failure cleanup or stale cleanup.
---
Outside diff comments:
In `@crates/mountmate-core/src/rclone.rs`:
- Around line 100-154: Update the port handling in the rclone configuration flow
around validate_port and known_hosts_marker to retain the normalized numeric
port value instead of the raw string. Reuse the existing SSH normalize/validate
helper, and use its normalized result consistently for the port option,
openssh_command, and known_hosts_marker so leading-zero ports resolve
identically across paths.
---
Nitpick comments:
In `@crates/mountmate-core/src/storage.rs`:
- Around line 602-612: Update the temporary-file creation in the atomic write
closure around OpenOptions to request private permissions at creation time using
the platform-appropriate mode-setting option, before any content is written.
Remove the now-redundant restrict_private_path(&temporary, false) call, while
preserving the existing post-rename behavior only if needed for the destination.
🪄 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 Plus
Run ID: d226de57-8914-4af6-bd3b-1910ec203ddf
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
Cargo.tomlREADME.mdREADME.zh-CN.mdcrates/mountmate-core/Cargo.tomlcrates/mountmate-core/src/app_command.rscrates/mountmate-core/src/credential.rscrates/mountmate-core/src/paths.rscrates/mountmate-core/src/rclone.rscrates/mountmate-core/src/runtime.rscrates/mountmate-core/src/ssh.rscrates/mountmate-core/src/storage.rscrates/mountmate-core/tests/legacy_migration.rscrates/mountmate-platform/src/lib.rsdocs/development-roadmap.mddocs/rust-rewrite-audit.mdlicenses/RUST-THIRD-PARTY.txt
| stream | ||
| .set_nonblocking(false) | ||
| .map_err(|error| error.to_string())?; | ||
| stream | ||
| .set_read_timeout(Some(Duration::from_secs(2))) | ||
| .map_err(|error| error.to_string())?; | ||
| stream | ||
| .set_write_timeout(Some(Duration::from_secs(2))) | ||
| .map_err(|error| error.to_string())?; | ||
| let request: CommandRequest = | ||
| read_message(&mut stream).map_err(|error| error.to_string())?; | ||
| read_message_until(&mut stream, deadline).map_err(|error| error.to_string())?; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
The 2 s response write is outside the pre-auth budget, so workers can still be monopolized.
UNAUTHENTICATED_DEADLINE only bounds read_message_until. An unauthenticated client that never drains its receive buffer keeps a worker busy for up to 500 ms (read) plus 2 s (write_all/flush under the 2 s write timeout). Four such sockets occupy all AUTH_WORKER_COUNT workers for ~2.5 s at a time, and further connections are silently dropped once the queue fills — the starvation this PR targets is reduced, not eliminated. Bounding the write by the same deadline (or using a much shorter write timeout for the pre-auth/error response) keeps total per-connection worker occupancy near the deadline.
🛡️ Proposed bound on the response write
stream
- .set_write_timeout(Some(Duration::from_secs(2)))
+ .set_write_timeout(Some(UNAUTHENTICATED_DEADLINE))
.map_err(|error| error.to_string())?;Also applies to: 533-542
🤖 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 `@crates/mountmate-core/src/app_command.rs` around lines 513 - 520, Update the
unauthenticated handling around read_message_until and the error response write
so the response’s write_all/flush cannot outlive UNAUTHENTICATED_DEADLINE. Reuse
the same deadline for the pre-auth response, or apply a substantially shorter
write timeout, ensuring total worker occupancy remains bounded near the
authentication deadline while preserving the existing error-response behavior.
| const RC_BCRYPT_COST: u32 = 10; | ||
|
|
||
| fn write_rc_htpasswd(path: &Path, username: &str, password: &str) -> Result<(), RuntimeError> { | ||
| let password_hash = bcrypt::hash(password, RC_BCRYPT_COST) | ||
| .map_err(|error| RuntimeError::RcAuthentication(error.to_string()))?; | ||
| atomic_write(path, format!("{username}:{password_hash}\n").as_bytes())?; | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does rclone --rc-htpasswd cache bcrypt credential verification per request?
💡 Result:
No, rclone does not currently cache bcrypt credential verification results per request when using the --rc-htpasswd flag [1]. When using --rc-htpasswd, rclone relies on a library to parse and verify credentials against the provided htpasswd file [1]. Because bcrypt is intentionally computationally expensive to protect against brute-force attacks, the absence of an internal authentication cache can lead to performance degradation, particularly when an rclone interface (such as a WebDAV server or remote control API) is subjected to high-frequency requests, such as directory listings containing many subdirectories [1]. Technical discussions within the rclone project have identified this limitation [1]. It has been noted that the underlying mechanism often performs file system operations (like STAT) on the htpasswd file for every request, and there is no implemented middleware to cache successful authentication results in memory to skip the expensive hash verification process for subsequent requests [1]. As a workaround, users experiencing performance issues have been advised to place their htpasswd files on a RAM disk to mitigate the file system overhead, although this does not address the CPU cost of the bcrypt verification itself [1].
Citations:
🌐 Web query:
rclone issue 7892 rc-htpasswd bcrypt cache performance issue
💡 Result:
Rclone issue 7892 identifies a performance bottleneck when using htpasswd with bcrypt for authentication, particularly in rclone serve webdav [1]. The issue arises because the underlying library used to parse and read the htpasswd file frequently performs a STAT operation on the file for every request [1]. Additionally, the computational cost of bcrypt, which is designed to be intentionally slow, further degrades performance when a large number of requests are processed [1]. Proposed mitigations discussed in the issue include: - Caching the authentication result (user/realm to basic auth cache) within rclone to avoid redundant bcrypt computations [1]. - Moving the htpasswd file to a RAM disk to mitigate the overhead of repeated file system STAT operations [1]. Historically, rclone has balanced security and performance regarding bcrypt usage. For instance, in other internal modules (such as proxy/webdav), rclone has opted to reduce bcrypt cost settings from the default to a minimum level to maintain adequate security for in-memory storage while improving transaction speed [2]. As of the current date, using --rc-htpasswd (or --htpasswd for serve commands) remains a recommended best practice for securing the rclone remote control (RC) API [3][4]. Users experiencing performance degradation are advised to monitor the issue thread for potential updates or implement the suggested workarounds [1].
Citations:
- 1: serve webdav: htpasswd+bcrypt cause poor performance when listing or writing dirs with many subdirs /files rclone/rclone#7892
- 2: sjpotter/rclone@9115752
- 3: https://www.endorlabs.com/vulnerability/cve-2026-49980
- 4: https://rclone.org/rc/
🌐 Web query:
github rclone auth rc htpasswd bcrypt cost default --rc-htpasswd
💡 Result:
Rclone’s remote control (RC) and HTTP serving commands support the standard Apache htpasswd file format for basic authentication, which includes support for Bcrypt [1][2][3][4]. Regarding the Bcrypt cost, Rclone utilizes the Go standard library's golang.org/x/crypto/bcrypt package [5][6]. The default cost used by this library is bcrypt.DefaultCost, which is currently 10 [6]. While users often inquire about manual control over this cost, Rclone’s official command-line interface does not provide a specific flag to configure the Bcrypt cost factor used when validating passwords via the --rc-htpasswd file. The Bcrypt cost is handled internally by the implementation that parses and validates the htpasswd file format [1][2]. In specific internal Rclone implementations (such as the proxy handler), there have been instances where the cost was intentionally adjusted (e.g., lowered to bcrypt.MinCost in specific contexts) to manage performance during high-frequency operations, though this is an internal implementation detail and not a user-configurable setting for the --rc-htpasswd mechanism [5].
Citations:
- 1: https://forum.rclone.org/t/no-support-for-sha512crypt-in-htpasswd/20837
- 2: http://www-rclone-org.7d7d567e.cdn.memsites.com/commands/rclone_rcd/
- 3: https://rclone.org/commands/rclone_rcd/
- 4: https://github.com/rclone/rclone/blob/b5a81dab/docs/content/commands/rclone_serve_http.md
- 5: sjpotter/rclone@9115752
- 6: proposal: x/crypto/bcrypt: DefaultCost should be increased to 12 golang/go#54573
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "tracked runtime files:"
git ls-files | rg '(^|/)runtime\.rs$|rc.*password|htpasswd|auth' || true
echo
echo "context around write_rc_htpasswd and rc start:"
if [ -f crates/mountmate-core/src/runtime.rs ]; then
wc -l crates/mountmate-core/src/runtime.rs
sed -n '1,90p' crates/mountmate-core/src/runtime.rs
sed -n '600,680p' crates/mountmate-core/src/runtime.rs
echo
echo "rc argument usages:"
rg -n -- "--rc|--rc-addr|--rc-password|--rc-htpasswd" crates/mountmate-core/src/runtime.rs crates -g '*.rs' || true
fiRepository: Stardust0831/ssh-mountmate
Length of output: 8321
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "rclone rc command construction:"
sed -n '360,430p' crates/mountmate-core/src/rclone.rs
sed -n '540,585p' crates/mountmate-core/src/rclone.rs
echo
echo "status/polling code:"
fd -t f '.*\.rs$' crates | while read -r f; do
if rg -n "status|transfer|monitor|poll|mountmate|read_json|MountState|Ready|Running|Transferred|BytesTransferred" "$f" > /tmp/rg_matches.txt 2>/dev/null; then
echo "--- $f"
fi
done || trueRepository: Stardust0831/ssh-mountmate
Length of output: 5287
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "credential generation:"
sed -n '1,160p' crates/mountmate-core/src/credential.rs
sed -n '800,980p' crates/mountmate-core/src/runtime.rs
echo
echo "RC client / polling call sites:"
sed -n '1,260p' crates/mountmate-core/src/rc.rs
sed -n '1,220p' crates/mountmate-core/src/transfer.rs
sed -n '1,220p' crates/mountmate-core/src/connection.rs
rg -n "HttpRcClient|rc_status|status|transfers|poll|monitor|periodic|interval|tokio::time|Interval::" crates/mountmate-core/src crates/mountmate-app/src -g '*.rs' | head -n 220Repository: Stardust0831/ssh-mountmate
Length of output: 50382
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Main polling/status loop locations:"
sed -n '1640,1720p' crates/mountmate-app/src/main.rs
rg -n --max-count 240 "status_task|polling|poll_task|tick|synced_polls|status_generation|RefreshMountStatus|status_for|refresh_transfers|transfer_snapshot|HttpRcClient::with_credentials" crates/mountmate-app/src/main.rs crates -g '*.rs'Repository: Stardust0831/ssh-mountmate
Length of output: 13102
Avoid bcrypt 10 for per-request RC auth.
--rc-htpasswd is used with a private loopback-bound 64-hex password, but rclone does not cache basic-auth verification and every RC status/transfer poll sends credentials. Cost 10 can add tens to hundreds of milliseconds per rc request, multiplying across mounts during polling. Use a lower bcrypt cost here or an auth scheme without repeated bcrypt checks for this loopback-only secret.
🤖 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 `@crates/mountmate-core/src/runtime.rs` around lines 652 - 659, Update
write_rc_htpasswd and RC_BCRYPT_COST to avoid bcrypt cost 10 for repeated
per-request loopback RC authentication. Use the lower bcrypt cost established
for this private 64-hex secret, while preserving the existing password hashing,
atomic write, and RuntimeError mapping behavior.
|
|
||
| ## Security hardening in progress: issues #41-#43 | ||
|
|
||
| Implemented and independently reviewed on 2026-07-31. Merge and Release remain gated on the |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the future-dated review date.
Line 29 says the work was implemented and reviewed on July 31, 2026, but the current date is July 30, 2026. Use the actual completion date or update this after the review completes.
🤖 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 `@docs/development-roadmap.md` at line 29, Update the completion date in the
roadmap entry beginning “Implemented and independently reviewed” so it is not
future-dated: use the actual completion/review date, or revise it after the
review has completed.
e362e68 to
b2f5a98
Compare
|
Verification complete for commit b2f5a98. Authoritative CI: https://github.com/Stardust0831/ssh-mountmate/actions/runs/30567904626 Passed jobs:
The platform jobs include packaged update/rollback checks, real SFTP lifecycle tests where applicable, and real macOS FUSE and NFS mount lifecycle tests on both architectures. Fresh local verification before push also passed:
Independent security review found no remaining merge blocker. |
1 participant
Summary
Local verification
cargo fmt --all -- --checkcargo clippy --workspace --all-targets --all-features -- -D warningscargo test --workspace --all-featuresAuthoritative six-platform CI will be dispatched manually for this branch before merge.
Closes #41
Closes #42
Closes #43
Summary by CodeRabbit
Security Enhancements
Reliability
Documentation