From 7de35dda5b32a8108e28fc10d3f7265dae52eb13 Mon Sep 17 00:00:00 2001 From: Hunter Bown Date: Wed, 6 May 2026 15:08:56 -0500 Subject: [PATCH] fix(onboarding): make chmod hardening best-effort (#897) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When the onboarding flow writes the API key under `~/.deepseek/`, the parent-dir and config-file chmod are propagated as hard errors if they fail. On Docker-on-Windows where the container mounts `$USERPROFILE\ .deepseek` to `/home/deepseek/.deepseek`, the bind-mounted NTFS volume can't accept Unix chmod, so `set_permissions` returns EPERM and the user sees `Failed to save API key: Failed to set permissions on /home/deepseek/.deepseek` — even though the directory and the secret file were already created successfully. The chmod is a hardening pass: the dir already lives under the user's home and the file is created with `O_CREAT | mode(0o600)` via OpenOptions. On filesystems where Unix permissions don't apply at all (Docker bind-mount of NTFS, network shares, FAT, certain CI volumes), the host's native ACL model is doing access control regardless. So demote the chmod to best-effort on Unix: warn loudly via `tracing::warn!` for security-sensitive operators who run with `RUST_LOG=warn`, then continue. Three sites: - `config::ensure_parent_dir` — parent-dir 0o700 hardening - `config::write_config_file_secure` — file 0o600 hardening - `secrets::FileKeyringStore::store_unlocked` — file 0o600 hardening (the parent-dir chmod here was already best-effort via `let _ =`) Tests: - `cargo test -p deepseek-secrets --all-features --locked` → 16/16 pass (including `file_store_round_trips_with_secure_perms` which still asserts mode 0o600 on a normal Linux test FS) - `cargo test -p deepseek-tui --bin deepseek-tui --all-features save_api_key --locked` → 5/5 pass - `cargo clippy -p deepseek-tui -p deepseek-secrets --all-features --locked -- -D warnings` clean - `cargo fmt --all -- --check` clean The GitBash paste failure mode reported in the same issue is a terminal quirk (GitBash on Windows doesn't reliably forward Ctrl+V to TUI apps); PowerShell + Shift+Insert work, as the reporter discovered. Not in scope here. Co-Authored-By: Claude Opus 4.7 --- crates/secrets/src/lib.rs | 14 ++++++++--- crates/tui/src/config.rs | 53 ++++++++++++++++++++++++++++++++------- 2 files changed, 55 insertions(+), 12 deletions(-) diff --git a/crates/secrets/src/lib.rs b/crates/secrets/src/lib.rs index ed4cb32d..c500bf77 100644 --- a/crates/secrets/src/lib.rs +++ b/crates/secrets/src/lib.rs @@ -266,9 +266,17 @@ impl FileKeyringStore { #[cfg(unix)] { use std::os::unix::fs::PermissionsExt; - let mut perms = fs::metadata(&self.path)?.permissions(); - perms.set_mode(0o600); - fs::set_permissions(&self.path, perms)?; + // Best-effort 0o600 — matches the parent-dir chmod above which + // is also `let _ = ...`. Filesystems that don't support Unix + // chmod (Docker bind-mounts of NTFS, network shares — #897) + // would otherwise fail the whole save here even though the + // blob already wrote successfully. The host's native ACLs + // are doing access control in those environments. + if let Ok(meta) = fs::metadata(&self.path) { + let mut perms = meta.permissions(); + perms.set_mode(0o600); + let _ = fs::set_permissions(&self.path, perms); + } } Ok(()) } diff --git a/crates/tui/src/config.rs b/crates/tui/src/config.rs index 114ff447..6e195fca 100644 --- a/crates/tui/src/config.rs +++ b/crates/tui/src/config.rs @@ -2289,14 +2289,32 @@ pub fn ensure_parent_dir(path: &Path) -> Result<()> { .with_context(|| format!("Failed to create directory: {}", parent.display()))?; #[cfg(unix)] { - let mut perms = fs::metadata(parent) - .with_context(|| format!("Failed to read metadata for {}", parent.display()))? - .permissions(); - if perms.mode() & 0o077 != 0 { - perms.set_mode(perms.mode() & !0o077); - fs::set_permissions(parent, perms).with_context(|| { - format!("Failed to set permissions on {}", parent.display()) - })?; + // Tighten group/other bits on the parent dir as a hardening pass. + // The dir lives under the user's home, so the chmod is best-effort: + // filesystems that don't accept Unix permission bits (Docker + // bind-mounts of NTFS, network shares, FAT, certain CI volumes — + // see #897) return EPERM/ENOTSUP. The dir already exists by the + // time we get here, so failing the whole save just because we + // couldn't tighten perms strands the user mid-onboarding. Warn + // loudly so a security-sensitive operator can still notice via + // `RUST_LOG=warn`, then continue. + if let Ok(meta) = fs::metadata(parent) { + let mode = meta.permissions().mode(); + if mode & 0o077 != 0 { + let mut perms = meta.permissions(); + perms.set_mode(mode & !0o077); + if let Err(err) = fs::set_permissions(parent, perms) { + tracing::warn!( + target: "deepseek::config", + path = %parent.display(), + error = %err, + "could not tighten parent dir permissions; \ + filesystem may not support Unix chmod \ + (Docker bind-mount, NTFS, network share). \ + Continuing — the file will still be written." + ); + } + } } } } @@ -2315,7 +2333,24 @@ fn write_config_file_secure(path: &Path, content: &str) -> Result<()> { .mode(0o600) .open(path)?; file.write_all(content.as_bytes())?; - file.set_permissions(fs::Permissions::from_mode(0o600))?; + // The file was already opened with mode 0o600; the explicit + // set_permissions re-asserts that on filesystems where mode-at-open + // didn't take effect (or where the file already existed with broader + // bits). Filesystems that don't accept Unix chmod at all (Docker + // bind-mounts of NTFS, network shares — #897) return EPERM. Treat + // that as a warning rather than failing the whole save: the file + // contents are written, and on Windows/macOS hosts the parent file + // system's native ACL model is doing the access control. + if let Err(err) = file.set_permissions(fs::Permissions::from_mode(0o600)) { + tracing::warn!( + target: "deepseek::config", + path = %path.display(), + error = %err, + "could not enforce 0o600 on config file; filesystem may \ + not support Unix chmod. File contents written; rely on \ + host ACLs for access control." + ); + } } #[cfg(not(unix))] {