Comprehensive remediation: fix ssh-agent control + UX cohesion + security (P0–P2) - #7
Merged
Conversation
Leaf-level robustness fixes from the comprehensive audit (stage P0-0): - EH-01: request_parser no longer panics on short/truncated SSH sign payloads; guard len < 6 before split_to and use try_get_u32 for the SSHSIG version. Adds unit tests for malformed inputs. - EH-02: agent confirm()/can_list() no longer panic via .expect when the UI request channel is closed (daemon shutdown); fail closed instead. - EH-05: PIN-dialog unlock fallback no longer panics on an empty decrypted cache; mirror the guarded sibling path and tolerate lock poisoning. - EH-08: cap daemon-side control-channel line reads at 64 KiB so a local process cannot flood the pipe/socket with an unbounded line. - SEC-07: remove the dead, permission-unaware VaultFile::save writer. - CFG-7: validate the version field when loading the local key cache, vault, and session files; reject unsupported versions with a clear error. - chore: declare the tokio "time" feature the agent crate actually uses so it builds and tests standalone (not only via workspace unification). All workspace tests pass; clippy clean.
…ce (P0-1) RT-01 (CRITICAL): on Windows the daemon could fail to take over \.\pipe\openssh-ssh-agent yet keep running as a zombie that still answered status/unlock while serving no SSH client — the core of "commands don't take effect on the running ssh-agent". - Claim the endpoint with first_pipe_instance(true); if another SSH agent (typically the Windows OpenSSH ssh-agent service) already owns the pipe, creation now fails loudly with actionable remediation instead of silently coexisting as a second instance that steals half the client connections. - Report transport failures over a watch channel the main loop observes; on failure the daemon shuts down (cancelling + removing its pid file) instead of running as a zombie. - status/details now reports `agent_running` so a dead transport is visible. - Replace a panicking tx.send().unwrap() in the listener with a graceful exit. RT-02 (HIGH): the single-instance guard only covered `sshwarden daemon`, so a bare foreground `sshwarden` started a second agent + control server competing for the same pipes, and a CLI lock/unlock could reach a different daemon than the one serving SSH. Move the is_daemon_running()/write_pid_file() guard to cover every run_foreground entry; the agent-pipe exclusivity above is the backstop when the pid file is stale. All workspace tests pass; clippy clean.
State-honesty fixes so status/sync no longer claim success the agent cannot back (the "it says it worked but ssh still fails" class): - lock-keystore: add agent.signable_key_count() and report it in status. After lock(), identities stay listable (ssh-add -l keeps working so auto-unlock-on-request can fire) but hold no private material; status now shows the signable count and "keys listed but not signable until unlock" instead of implying every listed key can sign. - RT-03/LOGIC-3/EH-04: a `sync` issued while the vault is locked no longer reports "Synced N SSH keys" as if the running agent was updated. It refreshes the cache, marks a pending sync, and says the keys load on next unlock. - LOGIC-2: a sign request arriving while locked with auto_unlock disabled is denied cleanly instead of being auto-approved under prompt_behavior=never against a key with no private material (an "approval" then broken signature). - LOGIC-7: warn when two vault items share a public key and collide in the keystore, explaining why status key_count can be lower than the vault count. Adds a unit test pinning the lock -> listable-but-not-signable behavior. All workspace tests pass; clippy clean.
Establish a local trust boundary and remove the offline-precompute weakness in
PIN unlock.
SEC-01 — control-channel caller authentication:
- Unix: reject any control connection whose peer uid differs from ours
(defence-in-depth over the existing 0600 socket).
- Windows: create the control pipe with a DACL restricting it to the current
user + LocalSystem (via create_with_security_attributes_raw + an SDDL built
from the process token SID), instead of the default DACL that grants Everyone
read access. Falls back to the default DACL if the descriptor can't be built.
SEC-04 — per-cache random PIN salt (format v3):
- crypto gains derive_pin_key_with_salt / pin_{encrypt,decrypt}_with_salt /
random_pin_salt / legacy_pin_salt; the old fixed-salt pin_encrypt/pin_decrypt
become thin legacy-salt shims so existing vault.enc / pre-v3 caches still
decrypt unchanged.
- LocalCacheKeySlots gains pin_salt; the envelope writer emits version 3 with a
fresh random salt. The 4-character minimum PIN is unchanged.
- A pre-v3 cache decrypts with the legacy salt and is then transparently
re-saved at v3 with a random salt (no biometric re-prompt; best-effort).
SEC-03 — PIN brute-force protection:
- Per-daemon (in-memory) failure counter with an escalating delay and a hard
lockout after PIN_MAX_ATTEMPTS, rejecting attempts during lockout without
running Argon2. Any successful unlock resets it.
CFG-7: local key cache load now accepts versions {2,3}.
Adds a crypto round-trip/migration-boundary test. Workspace tests pass; clippy
clean; rustfmt applied.
UX-1: human-facing command output was emitted via tracing (info!) to stderr with `INFO sshwarden:` prefixes, so `sshwarden keys | grep ...` returned nothing and "list" commands printed to different streams in different styles. - Add out_line()/err_line() helpers (carrying the workspace print_stdout/ print_stderr allow) so results go to stdout and errors to stderr; tracing is reserved for daemon/diagnostic logging. - Convert cmd_control (lock/unlock/status/sync/forget), non-json cmd_doctor, cmd_keys, cmd_set_pin, and the Config command to the helpers. - `keys` now prints a hint that it lists vault keys without changing the running agent (use `login` to load them) — partial UX-2. (cmd_login is converted in P1-2 where it is restructured to route through the daemon.) Workspace tests pass; clippy clean.
… effect (P1-2) UX-2 (login decision): `sshwarden login` now sends the master password over the control channel so the running daemon performs the login + sync and loads keys into the agent that actually serves SSH clients. If no daemon is running it falls back to a standalone login that lists keys and tells the user to start the daemon. `keys` stays an offline read-only lister (hint added in P1-1). CFG-2: `bindings add` was a silent no-op unless `ssh-config install` had been run first (the snippet was written but ~/.ssh/config never Included it). It now auto-installs the Include line after saving (idempotent), so a binding takes effect immediately. UX-7 (partial): a failed managed-snippet regeneration in bindings add/remove now fails loudly (non-zero) instead of warn-and-continue, since it would leave `ssh host` routing to the wrong key. Output goes through the stdout/stderr helpers from P1-1. (The deeper split-brain transport unification for bindings — RT-04/CFG-1 — is left as a follow-up; CFG-2 makes the local-file path work without a daemon, so the inconsistency is no longer user-breaking.) Workspace tests pass; clippy clean.
UX-3/UX-4/CFG-4/CFG-5: the docs disagreed three ways on where data lives and how many control commands exist. - status/status-json now report the resolved `data_dir` so users can see where their secrets actually live (the only code change here). - CLI guide: replace the "everything beside the exe (fully portable)" claim with the real 4-tier resolution (SSHWARDEN_HOME > SSHWARDEN_PORTABLE > [storage] portable > platform-standard default), matching config.toml.example. - IPC reference: correct "8 commands" to the 12 dispatched strings (adds unlock-native, status-json, forget, bind-hosts-dialog), document the `details` fields (incl. signable_key_count/agent_running/data_dir), note caller authentication, and fix stale source anchors. - README: mark standard storage, `sshwarden env`, the Unix control socket, and the envelope/PIN model as implemented (they were listed as TODO). Docs only (plus the status data_dir field); workspace tests pass; clippy clean.
…(P2-1)
XP-1/UX-6/LOGIC-5: fetch_status_details_for_doctor was hard-coded to bail
"IPC control is only supported on Windows" on Linux/macOS even though the Unix
control client is fully implemented, so doctor reported the daemon unreachable
on 2 of 3 platforms and skipped every dependent check. It now calls
send_control_command("status-json") on all platforms.
RT-05/XP-4/XP-6: doctor's only takeover check tested mere pipe *existence*,
which cannot tell SSHWarden's pipe from the OS ssh-agent service's. Added an
"agent.serving" check driven by the agent_running status field, so the zombie
case (control channel answers, but another agent owns the SSH endpoint) is
flagged with concrete remediation — no extra platform FFI. The Windows pipe
check is reworded to say it can't determine ownership and to point at it.
XP-3: added a non-Windows agent-endpoint check verifying the socket exists with
0600 perms and that SSH_AUTH_SOCK points at it (with an `eval "$(sshwarden env)"`
hint otherwise).
Workspace tests pass; clippy clean; rustfmt applied.
…lock during prompts (P2-2, partial) Concurrency fixes that do not touch the (runtime-only-verifiable) signing hot path internals: - CONC-1 (min): the standalone host-binding dialog response was awaited with no timeout inside the main select! loop, so a stuck dialog froze auto-lock, token refresh and notification handling. Bound it to 600s. - CONC-4 (mitigation): raise the UI-response broadcast capacity 32 -> 256 so a `Lagged` (which confirm()/can_list() treat as a denial) is effectively impossible under realistic concurrency. No hot-path logic change. - CONC-6: track in-flight UI requests and skip the inactivity auto-lock while a prompt is open, so the vault can't lock mid sign/unlock prompt. Deferred (need runtime verification of the SSH signing path, which can't be exercised by the test suite): the fully race-free per-request oneshot reply registry (CONC-4 full), single-flight auto-unlock (CONC-5), moving long control commands fully off the main loop (CONC-1 full), and spawn_blocking wrapping (CONC-7). Tracked as follow-ups. Workspace tests pass; clippy clean; rustfmt applied.
Residual hardening and cross-platform honesty: - EH-06: `forget` now accumulates on-disk deletion failures (local cache, legacy vault, native keyring material, session file) and returns an error naming what could not be removed, instead of always reporting success — a revocation that left secrets on disk is no longer reported as "forgotten". - SEC-05: BitwardenClient zeroizes its access/refresh tokens on drop (logout/forget/lock), alongside the existing ZeroizeOnDrop user key. - SEC-02: document prompt_behavior="never" as unsafe (any same-user process can obtain silent signatures; SSHWarden does not authenticate the calling process). - XP-2: macOS native unlock returned keys with no user-presence ceremony (violating ADR-0015). native_available() now returns false on macOS until a SecAccessControl/LAContext ceremony is implemented and verified, so it falls back to PIN/password instead of silently using the Keychain. - XP-5: Linux native_delete now checks the secret-tool exit status and reports failure (feeding EH-06) instead of discarding it. - UX-5: `sshwarden env` notes on Windows that OpenSSH uses the fixed pipe and ignores SSH_AUTH_SOCK (printed to stderr, so `eval` is unaffected). Deferred: end-to-end Zeroizing of the decrypted keys_json payload (SEC-06) — a wider plumbing change; the persistent SecureKeyCache copy is already zeroized. Workspace tests pass; clippy clean; rustfmt applied.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughHardens IPC and agent transport, adds fatal signaling, upgrades PIN crypto and local cache to v3 with per-cache salts and version checks/migration, implements PIN brute-force tracking and lockout, improves daemon single-instance and CLI output, and updates platform behavior and docs. ChangesPIN Encryption, File Versions, and Unlock Mechanics
Agent Transport Reliability and Control Channel Security
Daemon Single-Instance and Zombie Detection
Build, API, UI, and Docs
Estimated code review effort:
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main.rs (1)
1206-1236:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDaemon-routed login ignores the caller’s
--base-url.This branch prompts for or accepts overrides, but when the daemon is reachable it only sends
unlock-password:{password}. The daemon then logs in with its own configured account/server, sosshwarden login --email ... --base-url ...can succeed against the wrong target. Either reject those overrides in the daemon path or extend the control command to carry them through.🤖 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 `@src/main.rs` around lines 1206 - 1236, The daemon path currently sends only unlock-password via send_control_command (format!("unlock-password:{}", &*password)) which causes the daemon to ignore caller overrides; update the control command construction in the match branch that calls sshwarden_agent::control::send_control_command so it includes the caller-provided email and base_url (or explicitly detect when email/base_url were supplied and return an error rejecting them), e.g., build a command string like "unlock-password:{password};email:{email};base_url:{base_url}" (or similar key/value encoding) and pass that to send_control_command, ensuring any use of base_url.is_some() or config.auth.email is respected and preserved in the response handling code.
🧹 Nitpick comments (1)
crates/sshwarden-api/src/client.rs (1)
22-35: ⚡ Quick win
Droponly scrubs the final tokens — token rotation leaves stale copies in memory.The
Dropimpl zeroizes the tokens held at drop time, butself.access_token/self.refresh_tokenare also overwritten during the client's lifetime (refresh_access_tokenat Line 455-458 andlogin_passwordat Line 177-178). Reassigning theOption<String>drops the previousStringvia the normal allocator path without zeroizing, so each rotated token lingers in freed heap memory — partially undercutting the SEC-05 goal.Consider scrubbing before reassignment (or storing tokens in
Zeroizing<String>so every drop zeroizes):🔒️ Scrub-before-reassign on refresh
+ use zeroize::Zeroize; + if let Some(old) = self.access_token.as_mut() { + old.zeroize(); + } self.access_token = Some(token_resp.access_token); if let Some(new_refresh) = token_resp.refresh_token { + if let Some(old) = self.refresh_token.as_mut() { + old.zeroize(); + } self.refresh_token = Some(new_refresh); }🤖 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/sshwarden-api/src/client.rs` around lines 22 - 35, The Drop impl only zeroizes tokens at client drop, but rotated tokens are leaked when access_token/refresh_token (fields on BitwardenClient) are reassigned in methods like refresh_access_token and login_password; before assigning a new String, explicitly zeroize the previous value (e.g. if let Some(t) = self.access_token.as_mut() { t.zeroize(); } ) or change the field types to zeroize::Zeroizing<String> so every overwrite/drop zeroizes automatically; update all sites that assign to access_token/refresh_token (including refresh_access_token and login_password) to perform the scrub-before-reassign or use the Zeroizing wrapper so no stale token bytes remain on the heap.
🤖 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/sshwarden-agent/src/control.rs`:
- Around line 244-259: The current logic in start_control_server uses
ServerOptions::new().first_pipe_instance(false) and only falls back to
first_pipe_instance(true) on failure, which lets an existing attacker-created
pipe dictate the DACL; change the creation order to attempt the first instance
with first_pipe_instance(true) first when calling
create_with_security_attributes_raw(CONTROL_PIPE_NAME, sa_ptr) (using
ServerOptions::new()...), and after a successful first-instance creation set
first_pipe_instance(false) for subsequent instances; if claiming the first
instance (first_pipe_instance(true)) fails, return an error / fail closed
instead of attaching to an existing pipe so you don’t inherit another process’s
DACL.
In `@src/main.rs`:
- Around line 2313-2319: The persist_bind_payload() path currently only
regenerates the managed snippet while cmd_bindings_add() also ensures the user's
~/.ssh/config Include line via
write_sshwarden_include_line(managed_sshwarden_include_path(),
user_ssh_config_path()); centralize this behavior by invoking the same
include-install logic from persist_bind_payload() (or a new helper it calls) so
every binding persistence path (persist_bind_payload(), cmd_bindings_add(), UI
binding flows) calls write_sshwarden_include_line with
managed_sshwarden_include_path() and user_ssh_config_path(), and handle/report
any Err(e) the same way as cmd_bindings_add().
- Line 3252: The PIN lockout is only enforced in ControlAction::UnlockPin but
not in the SSH/dialog validators (make_pin_validator and the dialog closure
paths), allowing brute-force via UI flows; update the dialog/SSH PIN validation
code (the closures returned by make_pin_validator and any direct dialog
validation handlers used by the SSH request path) to consult the same
PinFailureHandle (pin_failures) before accepting attempts, reject validation
when the handle reports lockout, and ensure failed PIN attempts call the same
failure-accounting method on PinFailureHandle so that both
ControlAction::UnlockPin and dialog-based validators share identical gating and
increment behavior.
- Around line 2304-2308: The current flow calls
load_managed_keys_from_cache().unwrap_or_default() and then immediately calls
sync_managed_ssh_config_inner(&keys, true), which prunes offline bindings when
the cache is missing (keys == []). Change the logic in the places around the
sync_managed_ssh_config_inner calls (the instances using
load_managed_keys_from_cache, e.g., the call at
sync_managed_ssh_config_inner(&keys, true) and the similar block at the other
occurrence) to detect when the cache returned no key metadata and in that case
skip regeneration/pruning (either by not calling sync_managed_ssh_config_inner
or by calling it with pruning disabled) so existing bindings are preserved when
load_managed_keys_from_cache() yields an empty list; this maintains the “bind
ahead of first sync” behavior used by resolve_cipher_id().
- Around line 3154-3177: build_status_response() currently sets has_pin by only
checking pin_encrypted_keys, which misses v3 layout where the PIN is moved into
local_key_cache_data.local_cache_key.pin_encrypted during set-pin / migration;
change the has_pin calculation to true if either
pin_encrypted_keys.read().await.is_some() OR
local_key_cache_data.read().await.as_ref().and_then(|c|
c.local_cache_key.as_ref()).and_then(|k| k.pin_encrypted.as_ref()).is_some() (or
equivalent based on your Option types) so the status JSON reflects PIN
availability after v3 migration; update the reference to has_pin where it’s
produced in build_status_response().
- Around line 375-387: The check-then-act using is_daemon_running() followed by
write_pid_file() is racy; replace this with an atomic claim before proceeding
(e.g., attempt to create the PID file atomically using OpenOptions::create_new
or acquire an OS-level file lock/named mutex) so only one process succeeds, and
if the atomic create/lock fails treat it as "already running" and exit; update
the logic around is_daemon_running, write_pid_file, and startup (the branch that
calls run_foreground/detach_console) to use the atomic claim result, and ensure
the PID/lock is released/removed on clean shutdown or error.
---
Outside diff comments:
In `@src/main.rs`:
- Around line 1206-1236: The daemon path currently sends only unlock-password
via send_control_command (format!("unlock-password:{}", &*password)) which
causes the daemon to ignore caller overrides; update the control command
construction in the match branch that calls
sshwarden_agent::control::send_control_command so it includes the
caller-provided email and base_url (or explicitly detect when email/base_url
were supplied and return an error rejecting them), e.g., build a command string
like "unlock-password:{password};email:{email};base_url:{base_url}" (or similar
key/value encoding) and pass that to send_control_command, ensuring any use of
base_url.is_some() or config.auth.email is respected and preserved in the
response handling code.
---
Nitpick comments:
In `@crates/sshwarden-api/src/client.rs`:
- Around line 22-35: The Drop impl only zeroizes tokens at client drop, but
rotated tokens are leaked when access_token/refresh_token (fields on
BitwardenClient) are reassigned in methods like refresh_access_token and
login_password; before assigning a new String, explicitly zeroize the previous
value (e.g. if let Some(t) = self.access_token.as_mut() { t.zeroize(); } ) or
change the field types to zeroize::Zeroizing<String> so every overwrite/drop
zeroizes automatically; update all sites that assign to
access_token/refresh_token (including refresh_access_token and login_password)
to perform the scrub-before-reassign or use the Zeroizing wrapper so no stale
token bytes remain on the heap.
🪄 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: CHILL
Plan: Pro Plus
Run ID: 0a9c03b1-b0c6-430a-9f4a-c45e18f61a2e
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
README.mdconfig.toml.examplecrates/sshwarden-agent/Cargo.tomlcrates/sshwarden-agent/src/agent.rscrates/sshwarden-agent/src/control.rscrates/sshwarden-agent/src/named_pipe_listener_stream.rscrates/sshwarden-agent/src/request_parser.rscrates/sshwarden-agent/src/windows.rscrates/sshwarden-api/src/client.rscrates/sshwarden-api/src/crypto.rscrates/sshwarden-config/src/cache.rscrates/sshwarden-config/src/session.rscrates/sshwarden-config/src/vault.rscrates/sshwarden-ui/src/unlock/native.rsllmdoc/guides/how-to-use-cli-commands.mdllmdoc/reference/ipc-control-protocol.mdsrc/main.rs
Build: - main.rs: drop the `#[cfg(windows)]` gate on `use base64::Engine;` so the cross-platform encode/decode calls compile on macOS/Linux too. Review remediation: - control.rs (SEC-01): claim the FIRST named-pipe instance ourselves and fail closed if we cannot, so we never inherit a squatter's DACL. - main.rs (CFG-2): persist_bind_payload installs the ~/.ssh/config Include line, so UI/sign-flow bindings are no longer silent no-ops. - main.rs (SEC-03): UI/SSH PIN dialogs share the daemon-wide brute-force lockout via gate_pin_validator, not just ControlAction::UnlockPin. - main.rs (UX-7): skip orphan pruning when the key cache is empty so binding ahead of first sync no longer wipes existing bindings. - main.rs: build_status_response has_pin also reflects the v3 local-cache PIN slot. - main.rs (RT-02): claim the PID file atomically (create_new + stale reclaim) instead of a racy check-then-write. - main.rs (UX-2): cmd_login rejects --email/--base-url when a daemon will handle the login rather than silently ignoring them. - client.rs (SEC-05): scrub access/refresh tokens before reassigning so rotated tokens do not linger on the heap.
The Unix `doctor` agent-endpoint check built a throwaway PathBuf just to compare against the socket path, tripping `clippy::cmp_owned` under CI's `-D warnings`. This lint lived in a `#[cfg(not(windows))]` block, so it was invisible to a Windows clippy run and had been masked by the earlier base64 compile error. Compare borrowed `&Path`s instead (no allocation).
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main.rs (2)
3475-3481:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
unlockstill skips the v3 PIN slot after migration.This fallback builds the dialog from
get_pin_encrypted_data(), which only reads the legacy in-memory /vault.encPIN fields. Afterset-pinor legacy→v3 migration those slots are cleared and the PIN only lives underlocal_key_cache_data.local_cache_key.pin_encrypted, so plainsshwarden unlockfalls through even thoughunlock --pinand the SSH-request path can still unlock. Reuse the envelope-aware PIN validator path here.🤖 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 `@src/main.rs` around lines 3475 - 3481, The current fallback uses get_pin_encrypted_data which only reads legacy vault.enc PIN slots, causing v3/envelope-stored PINs (local_key_cache_data.local_cache_key.pin_encrypted) to be skipped; replace the legacy-only path so the unlock dialog builds the validator from the envelope-aware PIN data used by the `unlock --pin`/SSH request path: detect and prefer the envelope-stored pin (local_cache_key.pin_encrypted) and feed that envelope into the same validator constructor used by those paths (the make_pin_validator/gate_pin_validator chain) instead of calling get_pin_encrypted_data; ensure the validator creation reuses the envelope-aware code path so dialog unlocks can validate v3 PINs.
5643-5660:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPID reuse can make the singleton guard reject a healthy restart.
pid_file_owner_alive()treats any live process with the recorded PID as the daemon owner. After a crash, if the OS reuses that PID for an unrelated process,claim_pid_file()returnsOk(false)and SSHWarden stays locked out until that other process exits. The stale-owner check needs to verify it is actually SSHWarden (for example via exe/name/start-time metadata) or persist more than the raw PID in the file.Also applies to: 5670-5720
🤖 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 `@src/main.rs` around lines 5643 - 5660, The singleton guard in pid_file_owner_alive() is too permissive because it only checks whether any process still exists for the recorded PID, which can falsely block a healthy restart after PID reuse. Update the stale-owner validation used by claim_pid_file() to verify the process is actually SSHWarden, not just alive, by checking additional identity data such as exe/name/start-time or by persisting richer metadata alongside the PID. Apply the same ownership check consistently in the related pid-file handling paths around claim_pid_file() so stale PID files are rejected only when the recorded daemon is truly still running.
🤖 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/sshwarden-api/src/client.rs`:
- Around line 177-185: The code currently clones token_resp.refresh_token which
leaves the freshly issued token bytes in token_resp on the heap; instead move
the refresh token out of token_resp into self to avoid an extra unsanitized
copy. Replace the clone with a move (e.g. self.refresh_token =
token_resp.refresh_token) or, if token_resp is borrowed/mutably used later, use
std::mem::take(&mut token_resp.refresh_token) to transfer ownership; keep the
existing zeroize block for the previous self.refresh_token and do not call clone
so no plaintext copy remains on the heap.
In `@src/main.rs`:
- Around line 1208-1229: Compute email_override and base_url_override from the
CLI options first, then call is_daemon_running() and perform the bail check (the
existing anyhow::bail block) before you resolve/replace the email variable or
call prompt_password; in other words, move the email resolution match (the block
that uses prompt_email and config.auth.email) and the prompt_password("Master
password: ") call to after the daemon/override check so that
prompt_email/prompt_password are only invoked when the standalone login path
will proceed.
---
Outside diff comments:
In `@src/main.rs`:
- Around line 3475-3481: The current fallback uses get_pin_encrypted_data which
only reads legacy vault.enc PIN slots, causing v3/envelope-stored PINs
(local_key_cache_data.local_cache_key.pin_encrypted) to be skipped; replace the
legacy-only path so the unlock dialog builds the validator from the
envelope-aware PIN data used by the `unlock --pin`/SSH request path: detect and
prefer the envelope-stored pin (local_cache_key.pin_encrypted) and feed that
envelope into the same validator constructor used by those paths (the
make_pin_validator/gate_pin_validator chain) instead of calling
get_pin_encrypted_data; ensure the validator creation reuses the envelope-aware
code path so dialog unlocks can validate v3 PINs.
- Around line 5643-5660: The singleton guard in pid_file_owner_alive() is too
permissive because it only checks whether any process still exists for the
recorded PID, which can falsely block a healthy restart after PID reuse. Update
the stale-owner validation used by claim_pid_file() to verify the process is
actually SSHWarden, not just alive, by checking additional identity data such as
exe/name/start-time or by persisting richer metadata alongside the PID. Apply
the same ownership check consistently in the related pid-file handling paths
around claim_pid_file() so stale PID files are rejected only when the recorded
daemon is truly still running.
🪄 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: CHILL
Plan: Pro Plus
Run ID: 2511aaf3-9753-41c7-b3c9-62d066a74880
📒 Files selected for processing (3)
crates/sshwarden-agent/src/control.rscrates/sshwarden-api/src/client.rssrc/main.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/sshwarden-agent/src/control.rs
Follow-up review findings: - client.rs (SEC-05): move token_resp.refresh_token into self instead of cloning, so no extra plaintext copy of the freshly issued token lingers on the heap. - cmd_login (UX-2): run the daemon/override bail check BEFORE prompting for the password/email, and resolve the email only on the standalone path — no wasted prompt when the override will be rejected. - handle_control_command (SEC-04): the Hello-fallback PIN dialog now builds an envelope-aware validator (v3 local_cache_key.pin_encrypted) with a legacy vault.enc fallback, mirroring unlock --pin / the SSH path, so v3 PINs validate here too; the decrypted local cache key is held on success. - pid_file_owner_alive (RT-02): verify the recorded PID is actually this binary (exe path, then file name) before treating it as a live daemon, so PID reuse can't falsely block a healthy restart.
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
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.
Summary
Comprehensive, staged remediation from a full multi-dimension audit. Addresses the two reported pains — commands not taking effect on the running ssh-agent, and a fragmented/disjointed UX — plus the security/correctness issues found along the way.
Delivered as 10 independently-compilable, independently-tested commits (P0 → P1 → P2). Every stage passes
cargo build --workspace,cargo clippy --workspace --all-targets(clean), andcargo test --workspace(42 tests). Backward compatibility is preserved throughout: on-disk formats migrate transparently and no CLI command was removed.Root causes fixed
Pain B — "commands don't take effect on the running ssh-agent":
\.\pipe\openssh-ssh-agentyet keep running as a zombie that still answeredstatus/unlockwhile serving no SSH client. Now claims the endpoint withfirst_pipe_instance(true), reports failure over a watch channel the main loop observes (daemon shuts down instead of zombie-running), surfaces actionable remediation, andstatusreportsagent_running.sshwarden daemon; a bare foregroundsshwardenstarted a second competing agent/control server. The guard now covers everyrun_foregroundentry.syncno longer claims the agent was updated;statusreportssignable_key_count(a locked vault lists keys but can't sign); a locked sign request with auto-unlock off is denied cleanly instead of auto-approving a keyless signature.Pain A — "fragmented UX":
sshwarden keys | grepworks); logging stays on stderr.loginroutes through the daemon so it actually loads keys into the serving agent (falls back to a listing-only login if no daemon).bindings addauto-installs the~/.ssh/configInclude line, so it's no longer a silent no-op.statusprints the resolveddata_dir.Commits
2cae653b8eb7be402eca061ae144f7982e4847060ee19b5b8data_dirbf77a44e5b84c82b72a3fBackward compatibility
vault.encstill decrypt (legacy-salt shims) and are transparently re-saved at v3 after a successful unlock. The 4-character minimum PIN is unchanged.keysstays an offline lister,loginnow prefers the daemon.Deferred (need runtime verification of the SSH signing path, which the test suite can't exercise)
spawn_blockingwrapping (CONC-7) — mitigated here with a capacity bump + dialog timeout.Zeroizingof the decryptedkeys_jsonpayload (SEC-06) — wider plumbing; the persistentSecureKeyCachecopy is already zeroized.Manual verification needed on Windows (not exercisable in CI)
ssh-agentservice running, the daemon should error and exit (not run green);statusshowsagent_running=false.sshwardenwhile one runs should no-op.ssh-add -lcount matchessshwarden status;sshwarden keys | grep <name>returns rows.Summary by CodeRabbit
New Features
Bug Fixes & Improvements
Documentation