diff --git a/docs/guide/chapters/part5-yours.tex b/docs/guide/chapters/part5-yours.tex index ef8d714..ab69659 100644 --- a/docs/guide/chapters/part5-yours.tex +++ b/docs/guide/chapters/part5-yours.tex @@ -85,7 +85,8 @@ after you change a profile, every ten minutes and whenever you click \ui{Sync No \begin{checklist} \item[\color{green}\iCheck] Edited on two machines? The newer change wins, profile by profile. \item[\color{green}\iCheck] Deleted on one machine? It goes from the others too (unless someone edited it afterwards). - \item[\color{rose}\ph{E4F6}] Passwords, keys and tokens never sync. Each machine keeps its own in its keychain or vault. + \item[\color{rose}\ph{E4F6}] Saved passwords, keys and tokens never sync. Each machine keeps its own in its keychain or vault. + (Anything a login script \emph{sends} is part of the profile, so that does travel.) \end{checklist} \crop{settings-security.png}{581}{253}{576}{285}{0.8\linewidth}{Profile sync lives on the Security page, under your stored credentials.} diff --git a/docs/user-manual.md b/docs/user-manual.md index 51748fc..5afbd5c 100644 --- a/docs/user-manual.md +++ b/docs/user-manual.md @@ -434,7 +434,8 @@ Connections, disconnections, profile changes, credential access, imports and exp - for a profile changed on both sides, the more recent change wins; - a profile deleted on one machine is deleted on the others (unless it was edited after the deletion); -- passwords, keys and other credentials are never synced; each machine keeps its own in its keychain or vault. +- saved passwords, keys and tokens are never synced; each machine keeps its own in its keychain or vault. Values that a login script `send`s (including a password typed after a `wait "Password:"`) are part of the profile and do travel, as they do in an export; +- timestamps in the shared file are never trusted beyond the local clock, so a file edited to claim a future date can't override later local changes. - **Encrypted export**: **File → Export profiles…**, the sidebar footer, the command palette, or **Export…** on a profile's right-click menu. Choose the profiles, set a password and save a `.nexus` file. Credentials saved in the keychain or vault are not included. diff --git a/rust/app/src/settings.rs b/rust/app/src/settings.rs index 6931c17..e4c7c4b 100644 --- a/rust/app/src/settings.rs +++ b/rust/app/src/settings.rs @@ -452,7 +452,8 @@ impl Settings { widgets::hint( ui, "Keep connection profiles in step across machines through a folder they share (a cloud drive, a network share, a git checkout). \ - Profiles are stored there encrypted with the sync password; passwords and keys are never synced.", + Profiles are stored there encrypted with the sync password. Saved passwords and keys stay in each machine's keychain or vault; \ + values a login script sends travel with their profile.", ); form(ui, "sync-form", |ui| { field(ui, "Folder"); diff --git a/rust/core/src/clients/ssh.rs b/rust/core/src/clients/ssh.rs index a839b94..fe5f807 100644 --- a/rust/core/src/clients/ssh.rs +++ b/rust/core/src/clients/ssh.rs @@ -840,7 +840,7 @@ pub fn generate_key_pair(dir: &std::path::Path, name: &str, kind: &str, bits: u3 let comment = format!("{name}@nexus-terminal"); let key = russh::keys::PrivateKey::new(data, comment.as_str()).map_err(|e| e.to_string())?; let key = if passphrase.is_empty() { key } else { key.encrypt(&mut rng, passphrase.as_bytes()).map_err(|e| e.to_string())? }; - std::fs::create_dir_all(dir)?; + crate::config::create_private_dir(dir)?; let path = dir.join(&name); if path.exists() || path.with_extension("pub").exists() { return Err(format!("key {name:?} already exists").into()); @@ -904,7 +904,11 @@ mod tests { let public = std::fs::read_to_string(dir.join("id_test.pub")).unwrap(); assert!(public.starts_with("ssh-ed25519 ") && public.trim_end().ends_with("id_test@nexus-terminal"), "{public}"); #[cfg(unix)] - assert_eq!(std::os::unix::fs::PermissionsExt::mode(&std::fs::metadata(&path).unwrap().permissions()) & 0o777, 0o600); + { + use std::os::unix::fs::PermissionsExt; + assert_eq!(std::fs::metadata(&path).unwrap().permissions().mode() & 0o777, 0o600); + assert_eq!(std::fs::metadata(&dir).unwrap().permissions().mode() & 0o777, 0o700, "a fresh ~/.ssh is private"); + } assert!(crate::keyfiles::is_likely_private_key(&path)); // Encrypted: unreadable without the passphrase, readable with it. let p = path.to_string_lossy().into_owned(); diff --git a/rust/core/src/secrets.rs b/rust/core/src/secrets.rs index 9d5e4f7..69c3fa1 100644 --- a/rust/core/src/secrets.rs +++ b/rust/core/src/secrets.rs @@ -236,12 +236,13 @@ impl Vault { write_private(&self.path(), &data) } - pub fn get(&self, key: &str) -> Result { + pub fn get(&self, key: &str) -> Result> { let st = self.state.lock(); let u = st.as_ref().ok_or("secrets: vault is locked")?; let enc = u.vault.secrets.get(key).ok_or_else(|| format!("secrets: key {key:?} not found"))?; let data = B64.decode(enc).map_err(|e| format!("failed to decode secret: {e}"))?; - Ok(String::from_utf8_lossy(&open(&u.key, &data)?).into_owned()) + let plain = Zeroizing::new(open(&u.key, &data)?); + Ok(Zeroizing::new(String::from_utf8_lossy(&plain).into_owned())) } pub fn set(&self, key: &str, value: &str) -> Result<()> { @@ -269,17 +270,26 @@ impl Vault { /// Writes a file readable only by the owner, atomically: the data goes to a sibling temp file /// that replaces `path` only once it is fully written and synced, so a crash can't leave a -/// truncated vault or export behind (and an existing file's wider mode is never kept). +/// truncated vault or export behind (and an existing file's wider mode is never kept). The temp +/// file is created exclusively under an unguessable name, so nothing pre-placed in the folder (a +/// symlink in a shared sync folder, say) can be followed or overwritten. pub fn write_private(path: &std::path::Path, data: &[u8]) -> Result<()> { use std::io::Write; - let name = path.file_name().map(|n| n.to_string_lossy().into_owned()).unwrap_or_default(); - let tmp = path.with_file_name(format!(".{name}.tmp-{}", std::process::id())); + let name = path.file_name().ok_or("write_private: path has no file name")?.to_string_lossy().into_owned(); let mut opts = std::fs::OpenOptions::new(); - opts.write(true).create(true).truncate(true); + opts.write(true).create_new(true); #[cfg(unix)] std::os::unix::fs::OpenOptionsExt::mode(&mut opts, 0o600); + let mut tmp = path.to_path_buf(); let written = (|| { - let mut f = opts.open(&tmp)?; + let mut f = loop { + let r = random_bytes::<8>(); + tmp = path.with_file_name(format!(".{name}.tmp-{}", r.iter().map(|b| format!("{b:02x}")).collect::())); + match opts.open(&tmp) { + Err(e) if e.kind() == std::io::ErrorKind::AlreadyExists => continue, + r => break r?, + } + }; f.write_all(data)?; f.sync_all()?; std::fs::rename(&tmp, path) @@ -321,9 +331,9 @@ impl Keychain { off_runtime(move || matches!(entry(&service, "__nexus_keychain_probe__").and_then(|e| e.get_password()), Ok(_) | Err(keyring::Error::NoEntry))) } - pub fn get(&self, key: &str) -> Option { + pub fn get(&self, key: &str) -> Option> { let (service, key) = (self.service.clone(), key.to_string()); - off_runtime(move || entry(&service, &key).and_then(|e| e.get_password())).ok() + off_runtime(move || entry(&service, &key).and_then(|e| e.get_password())).ok().map(Zeroizing::new) } pub fn set(&self, key: &str, value: &str) -> Result<()> { @@ -352,7 +362,7 @@ impl Secrets { Secrets { vault, keychain, keychain_ok } } - pub fn get(&self, key: &str) -> Option { + pub fn get(&self, key: &str) -> Option> { if self.keychain_ok && let Some(v) = self.keychain.get(key).filter(|v| !v.is_empty()) { @@ -423,7 +433,7 @@ mod tests { v.lock(); assert!(v.get("a").is_err()); v.unlock("pw").unwrap(); - assert_eq!(v.get("a").unwrap(), "1"); + assert_eq!(*v.get("a").unwrap(), "1"); std::fs::remove_dir_all(dir).unwrap(); } @@ -459,7 +469,7 @@ mod tests { v.lock(); assert!(v.unlock("old").is_err(), "the old password no longer opens it"); v.unlock("new").unwrap(); - assert_eq!((v.get("a").unwrap(), v.get("profile:p:password").unwrap()), ("1".to_string(), "hunter2".to_string())); + assert_eq!((v.get("a").unwrap().as_str(), v.get("profile:p:password").unwrap().as_str()), ("1", "hunter2")); assert_eq!(v.keys(), ["a", "profile:p:password"]); std::fs::remove_dir_all(dir).unwrap(); } @@ -496,7 +506,30 @@ mod tests { s.vault.set("profile:p2:password", "keep").unwrap(); s.delete_profile("p1"); assert!(SecretKind::ALL.iter().all(|k| s.get(&k.key("p1")).is_none())); - assert_eq!(s.get("profile:p2:password").as_deref(), Some("keep")); + assert_eq!(s.get("profile:p2:password").as_deref().map(String::as_str), Some("keep")); + std::fs::remove_dir_all(dir).unwrap(); + } + + #[cfg(unix)] + #[test] + fn write_private_never_follows_links_or_leaves_temp_files() { + use std::os::unix::fs::PermissionsExt; + let dir = std::env::temp_dir().join(format!("nexus-wp-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + let canary = dir.join("canary"); + std::fs::write(&canary, b"untouched").unwrap(); + // A pre-placed symlink at the destination is replaced, not followed. + let target = dir.join("out"); + std::os::unix::fs::symlink(&canary, &target).unwrap(); + write_private(&target, b"secret").unwrap(); + assert_eq!(std::fs::read(&canary).unwrap(), b"untouched"); + assert!(std::fs::symlink_metadata(&target).unwrap().file_type().is_file()); + assert_eq!(std::fs::read(&target).unwrap(), b"secret"); + assert_eq!(std::fs::metadata(&target).unwrap().permissions().mode() & 0o777, 0o600); + // No temp file is left behind, and the temp name isn't the old predictable one. + let names: Vec = std::fs::read_dir(&dir).unwrap().map(|e| e.unwrap().file_name().to_string_lossy().into_owned()).collect(); + assert!(names.iter().all(|n| !n.contains(".tmp-")), "{names:?}"); std::fs::remove_dir_all(dir).unwrap(); } } diff --git a/rust/core/src/service.rs b/rust/core/src/service.rs index 7478762..f68aa46 100644 --- a/rust/core/src/service.rs +++ b/rust/core/src/service.rs @@ -43,6 +43,9 @@ pub struct Service { pub store: Arc, pub secrets: Arc, pub sessions: Arc, + /// Whether a sync password is stored, probed at most every few seconds: the Settings page + /// asks every frame, and each probe is a keychain round trip that fetches the real value. + sync_password: parking_lot::Mutex>, } #[derive(Clone, Debug)] @@ -58,7 +61,7 @@ impl Service { let store = Arc::new(Store::open(&config.db_path)?); let secrets = Arc::new(Secrets::new(Vault::new(config.vault_dir.clone()), Keychain::new(KEYCHAIN_SERVICE))); let sessions = Manager::new(store.clone(), secrets.clone(), config.recordings_dir.clone()); - Ok(Service { config, store, secrets, sessions }) + Ok(Service { config, store, secrets, sessions, sync_password: Default::default() }) } // --- profiles ----------------------------------------------------------------------- @@ -115,6 +118,7 @@ impl Service { /// Sets (or, when empty, forgets) the profile-sync password. pub fn set_sync_password(&self, password: &str) -> Result<()> { + *self.sync_password.lock() = None; if password.is_empty() { self.secrets.delete(SYNC_PASSWORD); return Ok(()); @@ -140,7 +144,15 @@ impl Service { } pub fn has_sync_password(&self) -> bool { - self.secrets.get(SYNC_PASSWORD).is_some() + let mut c = self.sync_password.lock(); + if let Some((at, v)) = *c + && at.elapsed() < std::time::Duration::from_secs(5) + { + return v; + } + let v = self.secrets.get(SYNC_PASSWORD).is_some(); + *c = Some((std::time::Instant::now(), v)); + v } /// Merges profiles with the shared sync file; see [`crate::sync`]. @@ -431,7 +443,7 @@ impl Service { path.as_mut_os_string().push(format!(".{}", exportimport::EXTENSION)); } exportimport::encrypt_and_export(&ps, password, &path)?; - self.store.add_audit("export", "", "", &format!("Exported {} profile(s) without secrets", ps.len())); + self.store.add_audit("export", "", "", &format!("Exported {} profile(s) (saved credentials not included)", ps.len())); Ok(path) } diff --git a/rust/core/src/sessions/credentials.rs b/rust/core/src/sessions/credentials.rs index 748cd85..49c4294 100644 --- a/rust/core/src/sessions/credentials.rs +++ b/rust/core/src/sessions/credentials.rs @@ -62,7 +62,7 @@ impl Manager { { st.used.insert(key.clone()); store.add_audit("secret.access", &pid, "", &format!("Accessed {kind} credential")); - return Ok(Zeroizing::new(v)); + return Ok(v); } } if mode == SecretMode::SessionOnly diff --git a/rust/core/src/sessions/mod.rs b/rust/core/src/sessions/mod.rs index 6e7317c..fbf5c2a 100644 --- a/rust/core/src/sessions/mod.rs +++ b/rust/core/src/sessions/mod.rs @@ -803,7 +803,7 @@ mod tests { // Typed again and accepted: stored. resolve(&m, "s1"); m.finish_connect("s1", e, Ok(Client::None), |_| {}); - assert_eq!(m.secrets.get(&key).as_deref(), Some("hunter2")); + assert_eq!(m.secrets.get(&key).as_deref().map(String::as_str), Some("hunter2")); assert_eq!(asked.load(Ordering::SeqCst), 3); // The saved value is reused; once the server rejects it, the next attempt asks again. diff --git a/rust/core/src/sessions/protocols.rs b/rust/core/src/sessions/protocols.rs index deb7a1a..40bb120 100644 --- a/rust/core/src/sessions/protocols.rs +++ b/rust/core/src/sessions/protocols.rs @@ -17,7 +17,7 @@ impl Manager { let passphrase = self.secret_fn(id, &p.id, SecretKind::Passphrase, sc.key_secret_mode, prompt); let c = SshClient::new(&p, self.terminal_callbacks(id, epoch)); c.set_authorities(self.cert_authorities()); - let stored = |k: SecretKind| self.secrets.get(&k.key(&p.id)).unwrap_or_default(); + let stored = |k: SecretKind| self.secrets.get(&k.key(&p.id)).map(|v| v.to_string()).unwrap_or_default(); c.set_stored_secrets(stored(SecretKind::Totp), stored(SecretKind::ProxyPassword)); c.connect(self.host_key_check(id, sc.host_key_policy), Some(password), Some(passphrase))?; for pf in sc.port_forwards.iter().filter(|f| f.enabled) { @@ -347,7 +347,7 @@ impl Manager { host: rc.proxy_host.clone(), port: rc.proxy_port as u16, username: rc.proxy_username.clone(), - password: self.secrets.get(&SecretKind::ProxyPassword.key(&p.id)).unwrap_or_default(), + password: self.secrets.get(&SecretKind::ProxyPassword.key(&p.id)).map(|v| v.to_string()).unwrap_or_default(), }; let h = host.clone(); Arc::new(move || proxy.connect(&h, port, timeout)) diff --git a/rust/core/src/sync.rs b/rust/core/src/sync.rs index caec54d..bb54b53 100644 --- a/rust/core/src/sync.rs +++ b/rust/core/src/sync.rs @@ -2,7 +2,12 @@ //! one encrypted file, `nexus-profiles.nexus`, in the export format (Argon2id + AES-256-GCM) under //! a sync password. Each side merges profile by profile — the newer `updated_at` wins — and //! deletions travel as dated tombstones, so a profile removed on one machine goes away on the -//! others. Credentials are never synced: they live in each machine's keychain or vault. +//! others. Saved credentials are never synced: they live in each machine's keychain or vault +//! (values a login script sends travel with their profile, as in an export). +//! +//! Timestamps in the shared file are clipped to the merging machine's clock: a record stamped in +//! the future would otherwise win over every later local edit (or keep deleting a profile) for +//! as long as the forged date, and the sync password is shared by every machine. use crate::Result; use crate::models::{Profile, Time}; @@ -37,14 +42,23 @@ pub struct Merged { /// Merges local state with the shared file. pub fn merge(local: &[Profile], local_deleted: &BTreeMap, remote: &SyncFile, now: Time) -> Merged { - let mut deleted = remote.deleted.clone(); + let mut deleted: BTreeMap = remote.deleted.iter().map(|(id, t)| (id.clone(), (*t).min(now))).collect(); for (id, t) in local_deleted { let e = deleted.entry(id.clone()).or_insert(*t); - *e = (*e).max(*t); + *e = (*e).max(*t).min(now); } deleted.retain(|_, t| (now - *t).num_days() < TOMBSTONE_DAYS); - let remote_by: HashMap<&str, &Profile> = remote.profiles.iter().map(|p| (p.id.as_str(), p)).collect(); + let remote_profiles: Vec = remote + .profiles + .iter() + .map(|p| { + let mut p = p.clone(); + p.updated_at = p.updated_at.min(now); + p + }) + .collect(); + let remote_by: HashMap<&str, &Profile> = remote_profiles.iter().map(|p| (p.id.as_str(), p)).collect(); let local_by: HashMap<&str, &Profile> = local.iter().map(|p| (p.id.as_str(), p)).collect(); let mut out = Merged::default(); let mut ids: Vec<&str> = local_by.keys().chain(remote_by.keys()).copied().collect(); @@ -64,8 +78,8 @@ pub fn merge(local: &[Profile], local_deleted: &BTreeMap, remote: (None, Some(r)) => *r, (None, None) => continue, }; - // A deletion newer than the last edit wins; an edit after the deletion brings it back. - if deleted.get(id).is_some_and(|t| *t >= winner.updated_at) { + // A deletion newer than the last edit wins; an edit at or after the deletion brings it back. + if deleted.get(id).is_some_and(|t| *t > winner.updated_at) { if local_by.contains_key(id) { out.delete.push(id.to_string()); } @@ -138,6 +152,25 @@ mod tests { assert!(merge(&[], &BTreeMap::new(), &remote, at(TOMBSTONE_DAYS * 86_400 + 10)).file.deleted.is_empty()); } + #[test] + fn future_dated_records_cannot_win_forever() { + let now = at(1_000_000); + // A forged remote edit "from the future" is clipped to now, so the user's later local edit + // (also at now) keeps winning, and the rewritten file no longer carries the forged date. + let remote = SyncFile { version: 1, profiles: vec![p("a", "forged", 9_000_000)], deleted: BTreeMap::new() }; + let m = merge(&[p("a", "mine", 1_000_000)], &BTreeMap::new(), &remote, now); + assert!(m.upsert.is_empty()); + assert_eq!(m.file.profiles[0].host, "mine"); + // A brand-new remote profile stamped in the future is accepted, but with a sane time. + let remote = SyncFile { version: 1, profiles: vec![p("b", "new", 9_000_000)], deleted: BTreeMap::new() }; + let m = merge(&[], &BTreeMap::new(), &remote, now); + assert_eq!(m.upsert[0].updated_at, now); + // A future-dated tombstone can't keep deleting a profile the user re-creates afterwards. + let remote = SyncFile { version: 1, profiles: vec![], deleted: [("c".to_string(), at(9_000_000))].into() }; + let m = merge(&[p("c", "recreated", 1_000_001)], &BTreeMap::new(), &remote, at(1_000_001)); + assert!(m.delete.is_empty() && m.file.profiles.len() == 1 && m.file.deleted.is_empty()); + } + #[test] fn file_round_trips_under_the_password() { let dir = std::env::temp_dir().join(format!("nexus-sync-{}", std::process::id()));