diff --git a/Cargo.lock b/Cargo.lock index 937a39aff..2249a2423 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -123,6 +123,17 @@ version = "1.0.104" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "330a5ed07fa54e4702c9d6c4174f74427fc0ef6e214bbd677ae50a5099946470" +[[package]] +name = "apple-native-keyring-store" +version = "1.0.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2b350bfd03649e07aa05c0a81b3e15934374e585c98204a57e20b9d49f49bb9a" +dependencies = [ + "keyring-core", + "log", + "security-framework", +] + [[package]] name = "arbitrary" version = "1.5.0" @@ -1600,6 +1611,7 @@ dependencies = [ "lock_api", "once_cell", "parking_lot_core", + "serde", ] [[package]] @@ -4585,6 +4597,8 @@ dependencies = [ "dirs 6.0.0", "flate2", "ide-core", + "keyring", + "keyring-core", "parking_lot", "russh", "russh-config", @@ -4595,6 +4609,7 @@ dependencies = [ "tempfile", "tokio", "tracing", + "uuid", ] [[package]] @@ -5216,6 +5231,33 @@ dependencies = [ "serde", ] +[[package]] +name = "keyring" +version = "4.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2270074a3d26bcac93c1dc5d2845eb4c089e8d761ccf6e0ea266a16004640627" +dependencies = [ + "apple-native-keyring-store", + "keyring-core", + "windows-native-keyring-store", + "zbus-secret-service-keyring-store", +] + +[[package]] +name = "keyring-core" +version = "1.0.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "29fbf4b551298c67925f38cf341a1ada797a75f1837bdf8ae1fc8e2ebc6efb59" +dependencies = [ + "chrono", + "dashmap", + "log", + "regex", + "ron", + "serde", + "uuid", +] + [[package]] name = "kqueue" version = "1.2.1" @@ -5679,6 +5721,30 @@ dependencies = [ "unicode-segmentation", ] +[[package]] +name = "num" +version = "0.4.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "35bd024e8b2ff75562e5f34e7f4905839deb4b22955ef5e73d2fea1b9813cb23" +dependencies = [ + "num-bigint 0.4.8", + "num-complex", + "num-integer", + "num-iter", + "num-rational", + "num-traits", +] + +[[package]] +name = "num-bigint" +version = "0.4.8" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c89e69e7e0f03bea5ef08013795c25018e101932225a656383bd384495ecc367" +dependencies = [ + "num-integer", + "num-traits", +] + [[package]] name = "num-bigint" version = "0.5.1" @@ -5691,6 +5757,15 @@ dependencies = [ "rand_core 0.10.1", ] +[[package]] +name = "num-complex" +version = "0.4.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "73f88a1307638156682bada9d7604135552957b7818057dcef22705b4d509495" +dependencies = [ + "num-traits", +] + [[package]] name = "num-conv" version = "0.2.2" @@ -5706,6 +5781,27 @@ dependencies = [ "num-traits", ] +[[package]] +name = "num-iter" +version = "0.1.46" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c92800bd69a1eac91786bcfe9da64a897eb72911b8dc3095decbd07429e8048b" +dependencies = [ + "num-integer", + "num-traits", +] + +[[package]] +name = "num-rational" +version = "0.4.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f83d14da390562dca69fc84082e73e548e1ad308d24accdedd2720017cb37824" +dependencies = [ + "num-bigint 0.4.8", + "num-integer", + "num-traits", +] + [[package]] name = "num-traits" version = "0.2.19" @@ -7208,6 +7304,19 @@ dependencies = [ "windows-sys 0.52.0", ] +[[package]] +name = "ron" +version = "0.12.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "97b3fe97c18ff24299ea56f802c5693eb34266fb0bc8317f4e101debc8a3ceaf" +dependencies = [ + "bitflags 2.13.2", + "serde", + "serde_derive", + "typeid", + "unicode-ident", +] + [[package]] name = "ropey" version = "1.6.1" @@ -7274,7 +7383,7 @@ dependencies = [ "md5", "ml-kem", "module-lattice", - "num-bigint", + "num-bigint 0.5.1", "p256", "p384", "p521", @@ -7655,6 +7764,25 @@ dependencies = [ "zeroize", ] +[[package]] +name = "secret-service" +version = "5.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5107b24b91445dd2aa449a258a1807b63240942157292354dc5bfdbeb8bc6db8" +dependencies = [ + "aes", + "cbc", + "futures-util", + "getrandom 0.4.3", + "hkdf", + "hybrid-array", + "num", + "once_cell", + "serde", + "sha2 0.11.1", + "zbus", +] + [[package]] name = "security-framework" version = "3.7.0" @@ -9143,7 +9271,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "32497e9a4c7b38532efcdebeef879707aa9f794296a4f0244f6f69e9bc8574bd" dependencies = [ "fastrand", - "getrandom 0.3.4", + "getrandom 0.4.3", "once_cell", "rustix", "windows-sys 0.61.2", @@ -10952,6 +11080,19 @@ version = "0.2.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "f0805222e57f7521d6a62e36fa9163bc891acd422f971defe97d64e70d0a4fe5" +[[package]] +name = "windows-native-keyring-store" +version = "1.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "063426e76fdec7438d56bb777f67e318a84a25c707b07e575cb8b78e10c028f8" +dependencies = [ + "byteorder", + "keyring-core", + "regex", + "windows-sys 0.61.2", + "zeroize", +] + [[package]] name = "windows-numerics" version = "0.3.1" @@ -11488,6 +11629,17 @@ dependencies = [ "zvariant", ] +[[package]] +name = "zbus-secret-service-keyring-store" +version = "1.0.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "74801d001b9e7729adb4f1825b67b398185fed424749aa3d8bacf70417137d9a" +dependencies = [ + "keyring-core", + "secret-service", + "zbus", +] + [[package]] name = "zbus_macros" version = "5.19.0" diff --git a/Cargo.toml b/Cargo.toml index 6976798e9..f51c48953 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -106,6 +106,9 @@ sqlx = { version = "0.9", default-features = false, features = ["runtime-tokio", tiberius = { version = "0.13", default-features = false, features = ["tds73", "rustls", "sql-browser-tokio"] } bb8 = "0.9" uuid = { version = "1", features = ["v4"] } +# OS credential stores +keyring = "4.2" +keyring-core = "1" # testing insta = { version = "1", features = ["yaml"] } diff --git a/crates/ide-db/Cargo.toml b/crates/ide-db/Cargo.toml index 8f9853930..e72eccf23 100644 --- a/crates/ide-db/Cargo.toml +++ b/crates/ide-db/Cargo.toml @@ -29,3 +29,4 @@ uuid.workspace = true [dev-dependencies] tokio = { workspace = true, features = ["rt-multi-thread", "macros"] } tempfile.workspace = true +ide-ssh = { workspace = true, features = ["testing"] } diff --git a/crates/ide-db/src/lib.rs b/crates/ide-db/src/lib.rs index 27717f282..71545ecbb 100644 --- a/crates/ide-db/src/lib.rs +++ b/crates/ide-db/src/lib.rs @@ -1161,9 +1161,49 @@ impl DbManager { #[cfg(test)] mod tests { use super::*; + use ide_ssh::secrets::testing::files_containing; + use std::path::Path; + + fn manager(dir: &Path) -> DbManager { + ide_ssh::secrets::testing::use_memory_store(); + let data = dir.join("data"); + let ssh = Arc::new(SshManager::new(data.join("ssh-passwords.json"))); + DbManager::new(dir.join("project"), data.join("db-passwords.json"), data.join("db-history.json"), ssh) + } + + #[tokio::test] + async fn saved_passwords_stay_off_disk() { + let tmp = tempfile::tempdir().unwrap(); + manager(tmp.path()).set_password("s1", Some("hunter2-db".into()), true).await.unwrap(); + manager(tmp.path()).set_ssh_secret("s1", Some("hunter2-tunnel".into()), true).await.unwrap(); + assert_eq!(files_containing(tmp.path(), "hunter2-db"), Vec::::new()); + assert_eq!(files_containing(tmp.path(), "hunter2-tunnel"), Vec::::new()); + let reopened = manager(tmp.path()); + assert_eq!(reopened.secret(&DataSource::default(), "s1".into(), true).await.as_deref(), Some("hunter2-db")); + assert_eq!( + reopened.secret(&DataSource::default(), secrets::ssh_key("s1"), true).await.as_deref(), + Some("hunter2-tunnel") + ); + } + + #[tokio::test] + async fn moves_plaintext_passwords_of_older_versions_off_disk() { + let tmp = tempfile::tempdir().unwrap(); + let project = tmp.path().join("project").to_string_lossy().into_owned(); + let legacy = serde_json::json!({ project: { "s1": "old-db-pw" }, "/other": { "s2": "other-pw" } }); + std::fs::create_dir_all(tmp.path().join("data")).unwrap(); + std::fs::write(tmp.path().join("data/db-passwords.json"), legacy.to_string()).unwrap(); + let m = manager(tmp.path()); + assert_eq!(m.secret(&DataSource::default(), "s1".into(), true).await.as_deref(), Some("old-db-pw")); + assert_eq!(files_containing(tmp.path(), "old-db-pw"), Vec::::new()); + assert_eq!(files_containing(tmp.path(), "other-pw"), Vec::::new()); + let other = SecretStore::new(tmp.path().join("data/db-passwords.json"), Path::new("/other")); + assert_eq!(other.get("s2").as_deref(), Some("other-pw")); + } #[tokio::test] async fn global_data_sources_are_shared_by_projects_with_their_passwords() { + ide_ssh::secrets::testing::use_memory_store(); let tmp = tempfile::tempdir().unwrap(); let (app, one, two) = (tmp.path().join("app"), tmp.path().join("one"), tmp.path().join("two")); let manager = |root: &std::path::Path| { diff --git a/crates/ide-db/src/secrets.rs b/crates/ide-db/src/secrets.rs index 43a571eb0..9889232a6 100644 --- a/crates/ide-db/src/secrets.rs +++ b/crates/ide-db/src/secrets.rs @@ -1,20 +1,15 @@ -//! Saved data source passwords, in the app's data directory rather than the OS -//! credential store (no system prompts). One JSON file for all projects, keyed -//! by project root, then credential key; owner-only on Unix. +//! Saved data source passwords, in the OS credential store like the other saved +//! passwords ([`SecretFile`]). One index file for all projects; each credential is +//! keyed by project root, then credential key. -use ide_core::{IdeError, IdeResult}; -use parking_lot::Mutex; +use ide_core::IdeResult; +use ide_ssh::secrets::SecretFile; use std::collections::BTreeMap; use std::path::{Path, PathBuf}; -/// Project root → credential key → secret. -type Contents = BTreeMap>; - -/// Serialises read-modify-write across the projects sharing the file. -static LOCK: Mutex<()> = Mutex::new(()); - pub struct SecretStore { path: PathBuf, + file: SecretFile, project: String, } @@ -23,9 +18,24 @@ pub fn ssh_key(id: &str) -> String { format!("{id}#ssh") } +fn file_key(project: &str, key: &str) -> String { + format!("{project}|{key}") +} + +/// Older versions' file: project root → credential key → secret. +fn legacy(value: serde_json::Value) -> Option> { + let all: BTreeMap> = serde_json::from_value(value).ok()?; + Some( + all.into_iter() + .flat_map(|(project, keys)| keys.into_iter().map(move |(k, s)| (file_key(&project, &k), s))) + .collect(), + ) +} + impl SecretStore { pub fn new(path: PathBuf, root: &Path) -> Self { - Self { path, project: root.to_string_lossy().into_owned() } + let file = SecretFile::with_legacy(path.clone(), legacy); + Self { path, file, project: root.to_string_lossy().into_owned() } } /// The same file, keyed by `scope` instead (global data sources: the app data directory). @@ -34,55 +44,15 @@ impl SecretStore { } pub fn get(&self, key: &str) -> Option { - let _g = LOCK.lock(); - self.read().remove(&self.project)?.remove(key) + self.file.get(&file_key(&self.project, key)) } pub fn set(&self, key: &str, secret: &str) -> IdeResult<()> { - let _g = LOCK.lock(); - let mut all = self.read(); - all.entry(self.project.clone()).or_default().insert(key.to_owned(), secret.to_owned()); - self.write(&all) + self.file.set(&file_key(&self.project, key), secret) } pub fn delete(&self, key: &str) { - let _g = LOCK.lock(); - let mut all = self.read(); - let Some(project) = all.get_mut(&self.project) else { return }; - if project.remove(key).is_none() { - return; - } - if project.is_empty() { - all.remove(&self.project); - } - if let Err(e) = self.write(&all) { - tracing::warn!("saved passwords: {}", e.message); - } - } - - fn read(&self) -> Contents { - match std::fs::read(&self.path) { - Ok(bytes) => serde_json::from_slice(&bytes).unwrap_or_else(|e| { - tracing::warn!("saved passwords: {}: {e}", self.path.display()); - Contents::new() - }), - Err(_) => Contents::new(), - } - } - - fn write(&self, all: &Contents) -> IdeResult<()> { - let err = |e: std::io::Error| IdeError::unavailable(format!("saved passwords: {e}")); - if let Some(dir) = self.path.parent() { - std::fs::create_dir_all(dir).map_err(err)?; - } - let tmp = self.path.with_extension("json.tmp"); - let mut options = std::fs::OpenOptions::new(); - options.write(true).create(true).truncate(true); - #[cfg(unix)] - std::os::unix::fs::OpenOptionsExt::mode(&mut options, 0o600); - let bytes = serde_json::to_vec_pretty(all).map_err(|e| IdeError::internal(e.to_string()))?; - std::io::Write::write_all(&mut options.open(&tmp).map_err(err)?, &bytes).map_err(err)?; - std::fs::rename(&tmp, &self.path).map_err(err) + self.file.delete(&file_key(&self.project, key)) } } @@ -92,6 +62,7 @@ mod tests { #[test] fn keeps_projects_apart_and_drops_empty_ones() { + ide_ssh::secrets::testing::use_memory_store(); let tmp = tempfile::tempdir().unwrap(); let file = tmp.path().join("db-passwords.json"); let a = SecretStore::new(file.clone(), Path::new("/p/a")); diff --git a/crates/ide-host/src/lib.rs b/crates/ide-host/src/lib.rs index b0eded265..eb3d08f67 100644 --- a/crates/ide-host/src/lib.rs +++ b/crates/ide-host/src/lib.rs @@ -25,7 +25,7 @@ use ide_language::{LanguageRegistry, Syntax}; use ide_run::ProcessManager; use ide_terminal::TerminalManager; use ide_workspace::SettingsStore; -use parking_lot::RwLock; +use parking_lot::{Mutex, RwLock}; use std::collections::HashMap; use std::path::PathBuf; use std::sync::{Arc, OnceLock}; @@ -74,6 +74,8 @@ pub struct Host { pub environment: environment::Environment, /// The HTTP proxy's saved password (see `proxy.rs`). proxy_password: ide_ssh::secrets::SecretFile, + /// The proxy password when it could not be saved (no credential store). + proxy_session_password: Mutex>, pub(crate) sink_for: SinkFactory, owner: fn(&str) -> &str, pub runtime: tokio::runtime::Handle, @@ -103,6 +105,7 @@ impl Host { let host = Self { trust: trust::TrustStore::load(&config_dir, &data_dir), proxy_password: ide_ssh::secrets::SecretFile::new(data_dir.join("proxy-password.json")), + proxy_session_password: Mutex::default(), remote: Arc::new(ide_remote::RemoteService::new(ssh.clone(), data_dir.join("remote"))), ssh, environment: environment::Environment::new(project_environment, local_data_dir.clone()), diff --git a/crates/ide-host/src/proxy.rs b/crates/ide-host/src/proxy.rs index 1ce2ed021..6644d250b 100644 --- a/crates/ide-host/src/proxy.rs +++ b/crates/ide-host/src/proxy.rs @@ -1,6 +1,7 @@ //! System Settings › HTTP Proxy on the host: the global settings made current for //! connections and child processes, the password kept in the saved passwords -//! (owner-only `proxy-password.json`) instead of `settings.toml`. +//! (the OS credential store) instead of `settings.toml`, or for the session +//! when there is no credential store. use crate::Host; use ide_core::IdeResult; @@ -12,7 +13,9 @@ impl Host { /// `settings` with the saved proxy password filled in, as the Settings dialog edits it. pub fn with_proxy_password(&self, mut settings: Settings) -> Settings { // A password typed into settings.toml by hand counts too, unless one was saved. - if let Some(password) = self.proxy_password.get(PASSWORD_KEY) { + let session = || self.proxy_session_password.lock().clone(); + // The session copy is the latest write that could not be saved. + if let Some(password) = session().or_else(|| self.proxy_password.get(PASSWORD_KEY)) { settings.proxy.password = password; } settings @@ -40,11 +43,70 @@ impl Host { _ => return Ok(Some(value)), }; if let Some(password) = password { - match password.as_str().filter(|p| !p.is_empty()) { - Some(p) => self.proxy_password.set(PASSWORD_KEY, p)?, + let password = password.as_str().filter(|p| !p.is_empty()); + let mut session = self.proxy_session_password.lock(); + *session = None; + match password { + Some(p) => { + if let Err(e) = self.proxy_password.set(PASSWORD_KEY, p) { + tracing::warn!("the HTTP proxy password is kept for this session only: {}", e.message); + *session = Some(p.to_owned()); + } + } None => self.proxy_password.delete(PASSWORD_KEY), } } Ok((key != "proxy.password").then_some(value)) } } + +#[cfg(test)] +mod tests { + use super::*; + use ide_ssh::secrets::{SecretFile, StoreError, StoreResult, Vault}; + use std::sync::Arc; + + /// A machine without a credential store (a headless Linux host). + struct NoStore; + + impl Vault for NoStore { + fn available(&self) -> bool { + false + } + fn get(&self, _: &str, _: &str) -> StoreResult { + Err(StoreError::NoDefaultStore) + } + fn set(&self, _: &str, _: &str, _: &str) -> StoreResult<()> { + Err(StoreError::NoDefaultStore) + } + fn delete(&self, _: &str, _: &str) -> StoreResult<()> { + Err(StoreError::NoDefaultStore) + } + } + + #[tokio::test] + async fn without_a_credential_store_the_password_lasts_the_session() { + let tmp = tempfile::tempdir().unwrap(); + let mut host = Host::new(crate::HostConfig { + config_dir: tmp.path().join("config"), + data_dir: tmp.path().join("data"), + local_data_dir: tmp.path().join("data"), + languages: Arc::new(ide_language::LanguageRegistry::load(None).unwrap()), + syntax: Arc::default(), + sink_for: Box::new(|_| Arc::new(ide_core::NullSink)), + owner: |w| w, + runtime: tokio::runtime::Handle::current(), + project_environment: false, + }); + let path = tmp.path().join("data/proxy-password.json"); + std::fs::create_dir_all(path.parent().unwrap()).unwrap(); + std::fs::write(&path, r#"{"httpProxy": "old"}"#).unwrap(); + host.proxy_password = SecretFile::with_vault(path, |v| serde_json::from_value(v).ok(), Arc::new(NoStore)); + + let left = host.take_proxy_password("proxy.password", serde_json::json!("s3cret")).unwrap(); + assert_eq!(left, None); + assert_eq!(host.with_proxy_password(Settings::default()).proxy.password, "s3cret"); + host.take_proxy_password("proxy.password", serde_json::json!("")).unwrap(); + assert_eq!(host.with_proxy_password(Settings::default()).proxy.password, ""); + } +} diff --git a/crates/ide-host/tests/proxy.rs b/crates/ide-host/tests/proxy.rs index cf74d5dd5..91fbb42d1 100644 --- a/crates/ide-host/tests/proxy.rs +++ b/crates/ide-host/tests/proxy.rs @@ -43,6 +43,7 @@ fn walk(dir: &std::path::Path) -> Vec { #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn manual_proxy_applies_and_the_password_is_saved_apart() { + ide_ssh::secrets::testing::use_memory_store(); let tmp = tempfile::tempdir().unwrap(); let host = host(tmp.path()); assert_eq!(proxy().mode, ProxyMode::Auto); @@ -61,7 +62,7 @@ async fn manual_proxy_applies_and_the_password_is_saved_apart() { assert!(!settings_files(tmp.path()).contains("s3cret"), "no password in settings.toml"); let saved = std::fs::read_to_string(tmp.path().join("data/proxy-password.json")).unwrap(); - assert!(saved.contains("s3cret")); + assert!(!saved.contains("s3cret"), "the password is in the credential store, not on disk: {saved}"); assert_eq!(ops::project::settings(&host, "main").proxy.password, "s3cret"); // A whole-table write keeps the password out of the file too; an empty one forgets it. diff --git a/crates/ide-ssh/Cargo.toml b/crates/ide-ssh/Cargo.toml index feb93e126..3d99cdf0d 100644 --- a/crates/ide-ssh/Cargo.toml +++ b/crates/ide-ssh/Cargo.toml @@ -14,15 +14,20 @@ tokio.workspace = true tracing.workspace = true parking_lot.workspace = true dirs.workspace = true +uuid.workspace = true +keyring.workspace = true +keyring-core.workspace = true russh = { version = "0.64", default-features = false, features = ["ring", "rsa", "flate2"] } russh-config = "0.58.0" russh-sftp = "2.4" flate2 = "1" [features] -# `testing`: an in-process SSH / SFTP server for tests of crates built on this one. -testing = [] +# `testing`: an in-process SSH / SFTP server for tests of crates built on this one, +# and an in-memory credential store in place of the OS one. +testing = ["keyring-core/sample"] [dev-dependencies] tokio = { workspace = true, features = ["rt-multi-thread", "macros"] } tempfile.workspace = true +keyring-core = { workspace = true, features = ["sample"] } diff --git a/crates/ide-ssh/src/git_creds.rs b/crates/ide-ssh/src/git_creds.rs index bdc818e13..4e839aa40 100644 --- a/crates/ide-ssh/src/git_creds.rs +++ b/crates/ide-ssh/src/git_creds.rs @@ -216,6 +216,7 @@ mod tests { use super::*; fn store() -> (tempfile::TempDir, GitCredentials) { + crate::secrets::testing::use_memory_store(); let tmp = tempfile::tempdir().unwrap(); let path = tmp.path().join("git-credentials.json"); (tmp, GitCredentials { saved: Arc::new(SecretFile::new(path)), sessions: Arc::default() }) diff --git a/crates/ide-ssh/src/secrets.rs b/crates/ide-ssh/src/secrets.rs index 4cd6a9cb8..7359f1571 100644 --- a/crates/ide-ssh/src/secrets.rs +++ b/crates/ide-ssh/src/secrets.rs @@ -1,61 +1,289 @@ -//! Saved passwords (SSH passwords / key passphrases by configuration id, the HTTP -//! proxy's), in the app's data directory rather than the OS credential store (no -//! system prompts). One JSON object, key → secret; owner-only on Unix. +//! Saved passwords (SSH passwords / key passphrases by configuration id, git HTTPS +//! credentials, the HTTP proxy's) in the OS credential store: Windows Credential +//! Manager, the macOS Keychain, the Secret Service on Linux. The file at `path` only +//! indexes them (an id and the keys, never a secret); owner-only on Unix. +//! +//! Older versions kept the secrets themselves in that file: they are moved to the +//! credential store on first use. Without a credential store (a headless Linux +//! host) such a file is still read and forgetting a secret rewrites it without +//! that one, but no new secret is written to disk: saving fails and callers keep +//! the secret for the session only. use ide_core::{IdeError, IdeResult}; +pub use keyring_core::{Error as StoreError, Result as StoreResult}; use parking_lot::Mutex; -use std::collections::BTreeMap; +use serde::{Deserialize, Serialize}; +use std::collections::{BTreeMap, BTreeSet}; use std::path::PathBuf; +use std::sync::Arc; + +const SERVICE: &str = "ai.l0ng.openidea"; + +/// Serialises read-modify-write of the index files (several stores may share one). +static LOCK: Mutex<()> = Mutex::new(()); + +/// Reads the secrets of an older version's file, by key (`None`: not that format). +pub type Legacy = fn(serde_json::Value) -> Option>; + +/// Where the secrets are kept: the default credential store, or a stand-in in tests. +pub trait Vault: Send + Sync { + fn available(&self) -> bool; + fn get(&self, service: &str, key: &str) -> StoreResult; + fn set(&self, service: &str, key: &str, secret: &str) -> StoreResult<()>; + fn delete(&self, service: &str, key: &str) -> StoreResult<()>; +} + +/// `keyring_core`'s default store: the OS's, unless a test installed another one. +struct DefaultStore; + +impl DefaultStore { + fn entry(&self, service: &str, key: &str) -> StoreResult { + if !self.available() { + return Err(StoreError::NoDefaultStore); + } + keyring_core::Entry::new(service, key) + } +} + +impl Vault for DefaultStore { + fn available(&self) -> bool { + if keyring_core::get_default_store().is_some() { + return true; + } + static WARNED: std::sync::Once = std::sync::Once::new(); + match keyring::Entry::store_status() { + Ok(()) => true, + Err(e) => { + WARNED.call_once(|| tracing::warn!("saved passwords: no credential store: {e}")); + false + } + } + } + + fn get(&self, service: &str, key: &str) -> StoreResult { + self.entry(service, key)?.get_password() + } + + fn set(&self, service: &str, key: &str, secret: &str) -> StoreResult<()> { + self.entry(service, key)?.set_password(secret) + } + + fn delete(&self, service: &str, key: &str) -> StoreResult<()> { + self.entry(service, key)?.delete_credential() + } +} pub struct SecretFile { path: PathBuf, - lock: Mutex<()>, + legacy: Legacy, + vault: Arc, +} + +/// The file's contents. `id` keeps the credentials of different data directories +/// (and of tests) apart in the one OS store. +#[derive(Serialize, Deserialize)] +struct Index { + id: String, + keys: BTreeSet, +} + +enum Contents { + Index(Index), + /// An older version's file that could not be moved, and why. + Plaintext(BTreeMap, String), +} + +fn service(id: &str) -> String { + format!("{SERVICE}/{id}") +} + +fn new_id() -> String { + uuid::Uuid::new_v4().to_string() } impl SecretFile { pub fn new(path: PathBuf) -> Self { - Self { path, lock: Mutex::new(()) } + Self::with_legacy(path, |v| serde_json::from_value(v).ok()) + } + + pub fn with_legacy(path: PathBuf, legacy: Legacy) -> Self { + Self::with_vault(path, legacy, Arc::new(DefaultStore)) + } + + pub fn with_vault(path: PathBuf, legacy: Legacy, vault: Arc) -> Self { + Self { path, legacy, vault } } pub fn get(&self, key: &str) -> Option { - let _g = self.lock.lock(); - self.read().remove(key) + let _g = LOCK.lock(); + match self.load() { + Contents::Index(mut index) if index.keys.contains(key) => match self.vault.get(&service(&index.id), key) { + Ok(secret) => Some(secret), + Err(StoreError::NoEntry) => { + // Removed outside the app: stop listing it. + index.keys.remove(key); + self.write_logged(&index); + None + } + Err(e) => { + tracing::warn!("saved passwords: {key}: {e}"); + None + } + }, + Contents::Index(_) => None, + Contents::Plaintext(mut old, _) => old.remove(key), + } } pub fn set(&self, key: &str, secret: &str) -> IdeResult<()> { - let _g = self.lock.lock(); - let mut all = self.read(); - all.insert(key.to_owned(), secret.to_owned()); - self.write(&all) + let _g = LOCK.lock(); + let mut index = match self.load() { + Contents::Index(index) => index, + Contents::Plaintext(_, why) => return Err(unavailable(&why)), + }; + let service = service(&index.id); + self.vault.set(&service, key, secret).map_err(|e| unavailable(&e.to_string()))?; + if index.keys.insert(key.to_owned()) + && let Err(e) = self.write(&index) + { + let _ = self.vault.delete(&service, key); + return Err(e); + } + Ok(()) } pub fn delete(&self, key: &str) { - let _g = self.lock.lock(); - let mut all = self.read(); - if all.remove(key).is_some() - && let Err(e) = self.write(&all) - { - tracing::warn!("{}", e.message); + let _g = LOCK.lock(); + match self.load() { + Contents::Index(mut index) => { + if !index.keys.contains(key) { + return; + } + match self.vault.delete(&service(&index.id), key) { + Ok(()) | Err(StoreError::NoEntry) => {} + Err(e) => { + // Kept in the index so that the next delete tries again. + tracing::warn!("saved passwords: could not forget {key}: {e}"); + return; + } + } + index.keys.remove(key); + self.write_logged(&index); + } + Contents::Plaintext(mut old, _) => { + if old.remove(key).is_none() { + return; + } + // A move halfway may have copied it already. + if let Some(id) = self.pending_id() + && let Err(e) = self.vault.delete(&service(&id), key) + && !matches!(e, StoreError::NoEntry) + { + tracing::warn!("saved passwords: could not forget {key}: {e}"); + return; + } + self.write_logged(&old); + } } } /// Every stored key, sorted (the secrets themselves stay private). pub fn keys(&self) -> Vec { - let _g = self.lock.lock(); - self.read().into_keys().collect() - } - - fn read(&self) -> BTreeMap { - match std::fs::read(&self.path) { - Ok(bytes) => serde_json::from_slice(&bytes).unwrap_or_else(|e| { - tracing::warn!("saved passwords: {}: {e}", self.path.display()); - BTreeMap::new() - }), - Err(_) => BTreeMap::new(), + let _g = LOCK.lock(); + match self.load() { + Contents::Index(index) => index.keys.into_iter().collect(), + Contents::Plaintext(old, _) => old.into_keys().collect(), } } - fn write(&self, all: &BTreeMap) -> IdeResult<()> { + fn load(&self) -> Contents { + let value = match std::fs::read(&self.path) { + Err(_) => return Contents::Index(Index { id: new_id(), keys: BTreeSet::new() }), + Ok(bytes) => match serde_json::from_slice::(&bytes) { + Ok(v) => v, + Err(e) => return self.set_aside(&e.to_string()), + }, + }; + if let Ok(index) = serde_json::from_value::(value.clone()) { + return Contents::Index(index); + } + match (self.legacy)(value) { + Some(old) => self.migrate(old), + None => self.set_aside("unknown format"), + } + } + + /// An unreadable file is renamed rather than overwritten (it may index + /// credentials, or hold secrets); the store starts over. + fn set_aside(&self, why: &str) -> Contents { + let aside = self.path.with_extension(format!("json.unreadable-{}", new_id())); + tracing::warn!("saved passwords: {}: {why}; moved to {}", self.path.display(), aside.display()); + if let Err(e) = std::fs::rename(&self.path, &aside) { + // Not overwritten either: nothing is saved until it can be moved. + return Contents::Plaintext(BTreeMap::new(), format!("{}: {e}", self.path.display())); + } + Contents::Index(Index { id: new_id(), keys: BTreeSet::new() }) + } + + fn pending_path(&self) -> PathBuf { + self.path.with_extension("json.migrating") + } + + /// The id of a move that has not finished (see [`SecretFile::migrate`]). + fn pending_id(&self) -> Option { + let id = std::fs::read_to_string(self.pending_path()).ok()?; + Some(id.trim().to_owned()).filter(|id| !id.is_empty()) + } + + /// Moves an older version's secrets to the credential store, key by key; a secret + /// the store refuses for good (too long, say) is forgotten, and the user is asked + /// for it again. The old file is replaced only when everything else was moved. + /// Until then the id waits in `.migrating`, so a retry after a failure or a + /// crash overwrites the same credentials instead of copying them again. + fn migrate(&self, old: BTreeMap) -> Contents { + if !self.vault.available() { + return Contents::Plaintext(old, "no credential store".into()); + } + let pending = self.pending_path(); + let id = match self.pending_id() { + Some(id) => id, + None => { + let id = new_id(); + if let Err(e) = std::fs::write(&pending, &id) { + return Contents::Plaintext(old, format!("{}: {e}", pending.display())); + } + id + } + }; + let mut index = Index { id, keys: BTreeSet::new() }; + for (key, secret) in &old { + match self.vault.set(&service(&index.id), key, secret) { + Ok(()) => { + index.keys.insert(key.clone()); + } + Err(e @ (StoreError::TooLong(..) | StoreError::Invalid(..))) => { + tracing::warn!("saved passwords: {key} does not fit the credential store and is forgotten: {e}"); + } + Err(e) => { + tracing::warn!("saved passwords: could not move {key} to the credential store: {e}"); + return Contents::Plaintext(old, e.to_string()); + } + } + } + if let Err(e) = self.write(&index) { + return Contents::Plaintext(old, e.message); + } + let _ = std::fs::remove_file(&pending); + Contents::Index(index) + } + + fn write_logged(&self, contents: &impl Serialize) { + if let Err(e) = self.write(contents) { + tracing::warn!("{}", e.message); + } + } + + fn write(&self, contents: &impl Serialize) -> IdeResult<()> { let err = |e: std::io::Error| IdeError::unavailable(format!("saved passwords: {}: {e}", self.path.display())); if let Some(dir) = self.path.parent() { std::fs::create_dir_all(dir).map_err(err)?; @@ -65,18 +293,258 @@ impl SecretFile { options.write(true).create(true).truncate(true); #[cfg(unix)] std::os::unix::fs::OpenOptionsExt::mode(&mut options, 0o600); - let bytes = serde_json::to_vec_pretty(all).map_err(|e| IdeError::internal(e.to_string()))?; + let bytes = serde_json::to_vec_pretty(contents).map_err(|e| IdeError::internal(e.to_string()))?; std::io::Write::write_all(&mut options.open(&tmp).map_err(err)?, &bytes).map_err(err)?; std::fs::rename(&tmp, &self.path).map_err(err) } } +fn unavailable(why: &str) -> IdeError { + IdeError::unavailable(format!("saved passwords: {why}")) +} + +/// Helpers for tests of crates that save secrets (feature `testing`). +#[cfg(any(test, feature = "testing"))] +pub mod testing { + use std::path::{Path, PathBuf}; + + /// Makes an in-memory credential store the default, so that the test never + /// touches the user's keychain. Call it before the first secret is saved. + pub fn use_memory_store() { + static ONCE: std::sync::Once = std::sync::Once::new(); + ONCE.call_once(|| keyring_core::set_default_store(keyring_core::sample::Store::new().expect("memory store"))); + } + + /// Every file under `dir` whose bytes contain `needle`. + pub fn files_containing(dir: &Path, needle: &str) -> Vec { + let mut found = Vec::new(); + let mut pending = vec![dir.to_path_buf()]; + while let Some(d) = pending.pop() { + for entry in std::fs::read_dir(&d).into_iter().flatten().flatten() { + let path = entry.path(); + if path.is_dir() { + pending.push(path); + } else if std::fs::read(&path).is_ok_and(|b| b.windows(needle.len()).any(|w| w == needle.as_bytes())) { + found.push(path); + } + } + } + found + } +} + #[cfg(test)] mod tests { + use super::testing::{files_containing, use_memory_store}; use super::*; + use std::collections::HashMap; + + /// A credential store that can be absent, or fail on chosen keys. + #[derive(Default)] + struct FakeVault { + absent: bool, + entries: Mutex>, + failing: Mutex StoreError>>, + } + + impl FakeVault { + fn fail(&self, key: &str, error: fn() -> StoreError) { + self.failing.lock().insert(key.to_owned(), error); + } + + fn heal(&self) { + self.failing.lock().clear(); + } + + fn check(&self, key: &str) -> StoreResult<()> { + if self.absent { + return Err(StoreError::NoDefaultStore); + } + self.failing.lock().get(key).map_or(Ok(()), |e| Err(e())) + } + + fn secrets(&self) -> Vec { + let mut all: Vec<_> = self.entries.lock().values().cloned().collect(); + all.sort(); + all + } + } + + impl Vault for FakeVault { + fn available(&self) -> bool { + !self.absent + } + fn get(&self, service: &str, key: &str) -> StoreResult { + self.check(key)?; + self.entries.lock().get(&(service.to_owned(), key.to_owned())).cloned().ok_or(StoreError::NoEntry) + } + fn set(&self, service: &str, key: &str, secret: &str) -> StoreResult<()> { + self.check(key)?; + self.entries.lock().insert((service.to_owned(), key.to_owned()), secret.to_owned()); + Ok(()) + } + fn delete(&self, service: &str, key: &str) -> StoreResult<()> { + self.check(key)?; + self.entries.lock().remove(&(service.to_owned(), key.to_owned())).map(|_| ()).ok_or(StoreError::NoEntry) + } + } + + fn platform() -> StoreError { + StoreError::PlatformFailure("locked".into()) + } + + fn with_fake(path: &std::path::Path, vault: &Arc) -> SecretFile { + SecretFile::with_vault(path.to_path_buf(), |v| serde_json::from_value(v).ok(), vault.clone()) + } + + const OLD: &str = r#"{"https://a.example": "me\nold-a", "https://b.example": "me\nold-b"}"#; + + #[test] + fn saved_secrets_never_reach_disk() { + use_memory_store(); + let tmp = tempfile::tempdir().unwrap(); + let path = tmp.path().join("ssh-passwords.json"); + SecretFile::new(path.clone()).set("a", "hunter2-ssh").unwrap(); + assert_eq!(files_containing(tmp.path(), "hunter2-ssh"), Vec::::new()); + let reopened = SecretFile::new(path); + assert_eq!(reopened.get("a").as_deref(), Some("hunter2-ssh")); + assert_eq!(reopened.keys(), ["a"]); + } + + #[test] + fn moves_plaintext_files_of_older_versions_off_disk() { + use_memory_store(); + let tmp = tempfile::tempdir().unwrap(); + let path = tmp.path().join("git-credentials.json"); + std::fs::write(&path, r#"{"https://example.com": "me\nold-pw"}"#).unwrap(); + let file = SecretFile::new(path.clone()); + assert_eq!(file.keys(), ["https://example.com"]); + assert_eq!(files_containing(tmp.path(), "old-pw"), Vec::::new()); + assert_eq!(SecretFile::new(path).get("https://example.com").as_deref(), Some("me\nold-pw")); + } + + #[test] + fn a_failed_move_keeps_the_old_file_and_the_retry_converges() { + let tmp = tempfile::tempdir().unwrap(); + let path = tmp.path().join("git-credentials.json"); + std::fs::write(&path, OLD).unwrap(); + let vault = Arc::new(FakeVault::default()); + vault.fail("https://b.example", platform); + + let file = with_fake(&path, &vault); + assert_eq!(file.get("https://a.example").as_deref(), Some("me\nold-a")); + assert_eq!(std::fs::read_to_string(&path).unwrap(), OLD, "the old file is untouched"); + assert!(file.set("https://c.example", "me\nnew").unwrap_err().message.contains("locked")); + + vault.heal(); + assert_eq!(file.keys(), ["https://a.example", "https://b.example"]); + assert_eq!(files_containing(tmp.path(), "old-a"), Vec::::new()); + assert_eq!(vault.secrets(), ["me\nold-a", "me\nold-b"], "the retry overwrote, not copied"); + assert!(!path.with_extension("json.migrating").exists()); + assert_eq!(with_fake(&path, &vault).get("https://b.example").as_deref(), Some("me\nold-b")); + } + + #[test] + fn forgetting_during_a_pending_move_forgets_the_copy_too() { + let tmp = tempfile::tempdir().unwrap(); + let path = tmp.path().join("git-credentials.json"); + std::fs::write(&path, OLD).unwrap(); + let vault = Arc::new(FakeVault::default()); + vault.fail("https://b.example", platform); + + let file = with_fake(&path, &vault); + file.delete("https://a.example"); + vault.heal(); + assert_eq!(file.keys(), ["https://b.example"]); + assert_eq!(vault.secrets(), ["me\nold-b"]); + } + + #[test] + fn a_secret_the_store_refuses_for_good_is_forgotten_and_does_not_block_saving() { + let tmp = tempfile::tempdir().unwrap(); + let path = tmp.path().join("git-credentials.json"); + std::fs::write(&path, OLD).unwrap(); + let vault = Arc::new(FakeVault::default()); + vault.fail("https://b.example", || StoreError::TooLong("password".into(), 2560)); + + let file = with_fake(&path, &vault); + assert_eq!(file.keys(), ["https://a.example"]); + assert_eq!(files_containing(tmp.path(), "old-b"), Vec::::new()); + file.set("https://c.example", "me\nnew").unwrap(); + assert_eq!(file.get("https://c.example").as_deref(), Some("me\nnew")); + } + + #[test] + fn without_a_credential_store_the_old_file_is_read_and_nothing_new_is_saved() { + let tmp = tempfile::tempdir().unwrap(); + let path = tmp.path().join("git-credentials.json"); + std::fs::write(&path, OLD).unwrap(); + let vault = Arc::new(FakeVault { absent: true, ..Default::default() }); + + let file = with_fake(&path, &vault); + assert_eq!(file.get("https://a.example").as_deref(), Some("me\nold-a")); + assert!(file.set("https://c.example", "me\nnew").unwrap_err().message.contains("no credential store")); + assert_eq!(files_containing(tmp.path(), "me\nnew"), Vec::::new()); + assert!(!path.with_extension("json.migrating").exists()); + + file.delete("https://a.example"); + assert_eq!(file.keys(), ["https://b.example"]); + assert_eq!(files_containing(tmp.path(), "old-a"), Vec::::new()); + } + + #[test] + fn forgets_only_what_the_store_forgot() { + let tmp = tempfile::tempdir().unwrap(); + let vault = Arc::new(FakeVault::default()); + let file = with_fake(&tmp.path().join("ssh-passwords.json"), &vault); + file.set("a", "pw").unwrap(); + vault.fail("a", platform); + file.delete("a"); + assert_eq!(file.keys(), ["a"], "still listed, so that forgetting can be retried"); + vault.heal(); + file.delete("a"); + assert_eq!(file.keys(), Vec::::new()); + assert_eq!(vault.secrets(), Vec::::new()); + } + + #[test] + fn a_secret_removed_outside_the_app_is_no_longer_listed() { + let tmp = tempfile::tempdir().unwrap(); + let vault = Arc::new(FakeVault::default()); + let file = with_fake(&tmp.path().join("ssh-passwords.json"), &vault); + file.set("a", "pw").unwrap(); + vault.entries.lock().clear(); + assert_eq!(file.get("a"), None); + assert_eq!(file.keys(), Vec::::new()); + } + + #[test] + fn a_failed_index_write_takes_the_secret_back_out_of_the_store() { + let tmp = tempfile::tempdir().unwrap(); + let path = tmp.path().join("ssh-passwords.json"); + std::fs::create_dir(&path).unwrap(); + let vault = Arc::new(FakeVault::default()); + assert!(with_fake(&path, &vault).set("a", "pw").is_err()); + assert_eq!(vault.secrets(), Vec::::new()); + } + + #[test] + fn an_unreadable_file_is_set_aside_not_overwritten() { + let tmp = tempfile::tempdir().unwrap(); + let path = tmp.path().join("ssh-passwords.json"); + std::fs::write(&path, "{ not json").unwrap(); + let vault = Arc::new(FakeVault::default()); + let file = with_fake(&path, &vault); + assert_eq!(file.keys(), Vec::::new()); + file.set("a", "pw").unwrap(); + std::fs::write(&path, "{ not json either").unwrap(); + assert_eq!(file.keys(), Vec::::new()); + assert_eq!(files_containing(tmp.path(), "{ not json").len(), 2, "both kept, neither overwritten"); + } #[test] fn stores_and_forgets_secrets() { + use_memory_store(); let tmp = tempfile::tempdir().unwrap(); let file = SecretFile::new(tmp.path().join("ssh-passwords.json")); file.set("a", "pw").unwrap(); diff --git a/crates/ide-ssh/tests/os_credential_store.rs b/crates/ide-ssh/tests/os_credential_store.rs new file mode 100644 index 000000000..5382b6d74 --- /dev/null +++ b/crates/ide-ssh/tests/os_credential_store.rs @@ -0,0 +1,44 @@ +//! Saved passwords against the real OS credential store (Windows Credential +//! Manager, macOS Keychain, Secret Service). + +use ide_ssh::secrets::SecretFile; + +/// Forgets every key of the file, also when the test panics. +struct Forget<'a>(&'a SecretFile); + +impl Drop for Forget<'_> { + fn drop(&mut self) { + for key in self.0.keys() { + self.0.delete(&key); + } + } +} + +#[test] +#[ignore = "uses the OS credential store"] +fn saves_moves_and_forgets_in_the_os_store() { + assert!(keyring::Entry::store_status().is_ok(), "no OS credential store"); + let tmp = tempfile::tempdir().unwrap(); + let path = tmp.path().join("git-credentials.json"); + std::fs::write(&path, r#"{"https://old.example": "me\nold-os-pw"}"#).unwrap(); + let file = SecretFile::new(path.clone()); + let forget = Forget(&file); + file.set("https://new.example", "me\nnew-os-pw").unwrap(); + let on_disk = std::fs::read_to_string(&path).unwrap(); + assert!(!on_disk.contains("old-os-pw") && !on_disk.contains("new-os-pw"), "{on_disk}"); + + let reopened = SecretFile::new(path); + assert_eq!(reopened.get("https://old.example").as_deref(), Some("me\nold-os-pw")); + assert_eq!(reopened.get("https://new.example").as_deref(), Some("me\nnew-os-pw")); + #[cfg(windows)] + { + let index: serde_json::Value = serde_json::from_str(&on_disk).unwrap(); + let service = format!("ai.l0ng.openidea/{}", index["id"].as_str().unwrap()); + let listed = std::process::Command::new("cmdkey").arg("/list").output().unwrap(); + let listed = String::from_utf8_lossy(&listed.stdout); + assert_eq!(listed.matches(service.as_str()).count(), 2, "{listed}"); + } + drop(forget); + assert_eq!(reopened.keys(), Vec::::new()); + assert_eq!(reopened.get("https://old.example"), None); +} diff --git a/src/features/database/SourceDialog.test.tsx b/src/features/database/SourceDialog.test.tsx new file mode 100644 index 000000000..e2cce12f3 --- /dev/null +++ b/src/features/database/SourceDialog.test.tsx @@ -0,0 +1,21 @@ +import { fireEvent, render, screen, within } from "@testing-library/react"; +import { describe, expect, it } from "vitest"; +import { SourceDialog } from "./SourceDialog"; + +const savePassword = () => screen.getByRole("checkbox", { name: "Save password" }) as HTMLInputElement; + +describe("new data source", () => { + it("does not save the database password unless asked to", () => { + render(); + expect(savePassword().checked).toBe(false); + }); + + it("does not save the SSH tunnel password unless asked to", () => { + render(); + fireEvent.click(screen.getByRole("tab", { name: "SSH/SSL" })); + fireEvent.click(screen.getByRole("checkbox", { name: "Use SSH tunnel" })); + fireEvent.click(screen.getByRole("button", { name: "SSH authentication type" })); + fireEvent.click(within(screen.getByRole("listbox")).getByText("Password")); + expect(savePassword().checked).toBe(false); + }); +}); diff --git a/src/features/database/SourceDialog.tsx b/src/features/database/SourceDialog.tsx index b50c16399..e42153cbf 100644 --- a/src/features/database/SourceDialog.tsx +++ b/src/features/database/SourceDialog.tsx @@ -110,7 +110,7 @@ const blankSsh = (): DbSsh => ({ user: "", auth: "openSsh", keyFile: "", - saveSecret: true, + saveSecret: false, }); const blankOptions = (): DbOptions => ({ @@ -155,7 +155,7 @@ function blank(): DataSource { user: "", file: null, ssl: "prefer", - savePassword: true, + savePassword: false, readOnly: false, color: null, comment: "",