From 86c21f3bffe3b10ce10a59ff7a5d48a1af3cb47d Mon Sep 17 00:00:00 2001 From: samialabri Date: Tue, 20 Oct 2026 22:35:13 +0900 Subject: [PATCH] Removed devices: reset the device key and id in place, then set up again DeviceKey implements kv-ffi's DeviceSigner.reset(): it deletes the key material (TPM key, DPAPI file, Secret Service / keychain entry, or key file) and the device-id and store files, then creates a new key with a new id in the best store available. The core calls it on device_revoked and retries a setup code's ticket once, so a fresh Add-a-device code signs a removed computer straight in. DeviceRevoked explains how to set it up again; the fingerprint shown refreshes. Removing a device warns that it must be set up again as a new one. Test: end to end through kv-sync's test server (new dev-dependency). The Windows TPM/DPAPI paths could not be compiled here (no Windows SDK for ring). Co-Authored-By: Claude Opus 5.5 --- src-tauri/Cargo.lock | 93 +++++++++++++++++++ src-tauri/Cargo.toml | 1 + src-tauri/src/device_key.rs | 177 ++++++++++++++++++++++++++++-------- src-tauri/src/sync.rs | 56 +++++++++++- src/main.ts | 2 +- 5 files changed, 289 insertions(+), 40 deletions(-) diff --git a/src-tauri/Cargo.lock b/src-tauri/Cargo.lock index e907fcc..8c133b4 100644 --- a/src-tauri/Cargo.lock +++ b/src-tauri/Cargo.lock @@ -245,6 +245,16 @@ dependencies = [ "nom", ] +[[package]] +name = "assert-json-diff" +version = "2.0.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "47e4f2b81832e72834d7518d8487a0396a28cc408186a2e8854c0f98011faf12" +dependencies = [ + "serde", + "serde_json", +] + [[package]] name = "async-broadcast" version = "0.7.2" @@ -1243,6 +1253,24 @@ dependencies = [ "windows-sys 0.61.2", ] +[[package]] +name = "deadpool" +version = "0.12.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0be2b1d1d6ec8d846f05e137292d0b89133caf95ef33695424c09568bdd39b1b" +dependencies = [ + "deadpool-runtime", + "lazy_static", + "num_cpus", + "tokio", +] + +[[package]] +name = "deadpool-runtime" +version = "0.1.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "092966b41edc516079bdf31ec78a2e0588d1d0c08f78b91d8307215928642b2b" + [[package]] name = "defmt" version = "1.2.0" @@ -1791,6 +1819,21 @@ dependencies = [ "autocfg", ] +[[package]] +name = "futures" +version = "0.3.34" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9a31d2a3fbaaeb2af2368bbdd904aa8e812d3c04a1ee10d3171f52d556e5d0a3" +dependencies = [ + "futures-channel", + "futures-core", + "futures-executor", + "futures-io", + "futures-sink", + "futures-task", + "futures-util", +] + [[package]] name = "futures-channel" version = "0.3.34" @@ -1798,6 +1841,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "b1f9e3d69d39e4862ffed03ed071a76f9a13ba1d9109d355b0f0aa6b15e393c4" dependencies = [ "futures-core", + "futures-sink", ] [[package]] @@ -1865,6 +1909,7 @@ version = "0.3.34" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "0d50a92467f8ba5dd6e3ee5d4bd04d73ab2e4e1c44474a0674821dfce14b79bc" dependencies = [ + "futures-channel", "futures-core", "futures-io", "futures-macro", @@ -2550,6 +2595,12 @@ version = "1.10.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "6dbf3de79e51f3d586ab4cb9d5c3e2c14aa28ed23d180cf89b4df0454a69cc87" +[[package]] +name = "httpdate" +version = "1.0.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "df3b46402a9d5adb4c86a0cf463f42e19994e3ee891101b1841f30a545cb49a9" + [[package]] name = "hybrid-array" version = "0.4.15" @@ -2573,6 +2624,7 @@ dependencies = [ "http", "http-body", "httparse", + "httpdate", "itoa", "pin-project-lite", "smallvec", @@ -3016,6 +3068,7 @@ dependencies = [ "image", "keyring", "kv-ffi", + "kv-sync", "libc", "p256", "qrcode", @@ -3205,9 +3258,16 @@ dependencies = [ "url", "uuid", "webpki-roots 0.26.11", + "wiremock", "zeroize", ] +[[package]] +name = "lazy_static" +version = "1.5.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "20870f649af7073d53e38067b2a84312175d56ea15217e1b15bc83506ec50afb" + [[package]] name = "libappindicator" version = "0.9.0" @@ -3565,6 +3625,16 @@ dependencies = [ "autocfg", ] +[[package]] +name = "num_cpus" +version = "1.17.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "91df4bbde75afed763b708b7eee1e8e7651e02d97f6d5dd763e89367e957b23b" +dependencies = [ + "hermit-abi", + "libc", +] + [[package]] name = "num_enum" version = "0.7.6" @@ -6888,6 +6958,29 @@ dependencies = [ "windows-sys 0.61.2", ] +[[package]] +name = "wiremock" +version = "0.6.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "08db1edfb05d9b3c1542e521aea074442088292f00b5f28e435c714a98f85031" +dependencies = [ + "assert-json-diff", + "base64 0.22.1", + "deadpool", + "futures", + "http", + "http-body-util", + "hyper", + "hyper-util", + "log", + "once_cell", + "regex", + "serde", + "serde_json", + "tokio", + "url", +] + [[package]] name = "wit-bindgen" version = "0.57.1" diff --git a/src-tauri/Cargo.toml b/src-tauri/Cargo.toml index eee9a2a..6adfcfd 100644 --- a/src-tauri/Cargo.toml +++ b/src-tauri/Cargo.toml @@ -77,6 +77,7 @@ core-foundation = "0.10" [dev-dependencies] tempfile = "3" +kv-sync = { path = "../../kestrove-core/crates/kv-sync", features = ["test-server"] } tokio = { version = "1", features = ["rt-multi-thread", "macros"] } [profile.release] diff --git a/src-tauri/src/device_key.rs b/src-tauri/src/device_key.rs index 114c313..ed5c921 100644 --- a/src-tauri/src/device_key.rs +++ b/src-tauri/src/device_key.rs @@ -8,10 +8,14 @@ //! //! The store that created the key is recorded next to the device id. A later launch uses only that store: if it //! is unavailable, signing fails instead of silently enrolling a new device. +//! +//! When the server removed this device (docs/03 "Removed devices"), the core calls [`DeviceKey`]'s `reset`: the old +//! key and id are deleted and a new key with a new id is created in place, in the best store available. use std::fmt; use std::io; use std::path::{Path, PathBuf}; +use std::sync::{RwLock, RwLockReadGuard}; use kv_ffi::CoreError; use p256::ecdsa::{signature::Signer as _, Signature, SigningKey}; @@ -71,16 +75,25 @@ enum Backend { Cng(cng::Key), } -pub struct DeviceKey { +/// One device identity: the id the server knows and its key. +struct Identity { id: String, public: Vec, backend: Backend, storage: KeyStorage, } +pub struct DeviceKey { + data_dir: PathBuf, + /// Stores to create a key in, best first (for the first key and after a reset). + preferred: Vec, + current: RwLock, +} + impl fmt::Debug for DeviceKey { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - f.debug_struct("DeviceKey").field("id", &self.id).field("storage", &self.storage).finish_non_exhaustive() + let cur = self.read(); + f.debug_struct("DeviceKey").field("id", &cur.id).field("storage", &cur.storage).finish_non_exhaustive() } } @@ -96,51 +109,61 @@ impl DeviceKey { } fn load_or_create_with(data_dir: &Path, preferred: &[KeyStorage]) -> io::Result { - let id_path = data_dir.join(ID_FILE); - let store_path = data_dir.join(STORE_FILE); - let id = read_text(&id_path)?; - let recorded = read_text(&store_path)?.and_then(|s| KeyStorage::from_tag(&s)); + let identity = Self::load_identity(data_dir)?.map_or_else(|| create_identity(data_dir, preferred), Ok)?; + Ok(Self { data_dir: data_dir.to_path_buf(), preferred: preferred.to_vec(), current: RwLock::new(identity) }) + } + + fn load_identity(data_dir: &Path) -> io::Result> { + let id = read_text(&data_dir.join(ID_FILE))?; + let recorded = read_text(&data_dir.join(STORE_FILE))?.and_then(|s| KeyStorage::from_tag(&s)); if let (Some(id), Some(storage)) = (id, recorded) { if let Some(key) = load(data_dir, &id, storage)? { - return Ok(key); + return Ok(Some(key)); } // The id is known but its key is gone (keyring wiped, folder copied from another machine): // this is a new device as far as the server is concerned. eprintln!("kestrove: the device key in {} is missing; creating a new device key", storage.describe()); } - let id = uuid::Uuid::new_v4().to_string(); - let mut last_err = io::Error::other("no key store available"); - for &storage in preferred { - match create(data_dir, &id, storage) { - Ok(key) => { - private_file::write(&id_path, id.as_bytes())?; - private_file::write(&store_path, storage.tag().as_bytes())?; - return Ok(key); - } - Err(e) => { - eprintln!("kestrove: device key store {} unavailable: {e}", storage.describe()); - last_err = e; - } - } - } - Err(last_err) + Ok(None) + } + + fn read(&self) -> RwLockReadGuard<'_, Identity> { + self.current.read().unwrap_or_else(|e| e.into_inner()) } pub fn storage(&self) -> KeyStorage { - self.storage + self.read().storage } /// Shown in settings when the key is less protected than it should be. pub fn warning(&self) -> Option<&'static str> { - (self.storage == KeyStorage::File).then_some( + (self.storage() == KeyStorage::File).then_some( "No system keyring was found, so this device's sync key is stored in a file only your user can read.", ) } - pub fn id(&self) -> &str { - &self.id + pub fn id(&self) -> String { + self.read().id.clone() } + /// The server removed this device for good: deletes the key and the device id, then creates a new key with a new + /// id. Afterwards the server sees a new device (pending approval, or approved by an enrollment ticket). + pub fn reset(&self) -> io::Result<()> { + let mut cur = self.current.write().unwrap_or_else(|e| e.into_inner()); + // Delete first: the file and DPAPI stores reuse one path, so a new key written first would be deleted. + destroy(&self.data_dir, &cur)?; + for f in [ID_FILE, STORE_FILE] { + match std::fs::remove_file(self.data_dir.join(f)) { + Err(e) if e.kind() != io::ErrorKind::NotFound => return Err(e), + _ => {} + } + } + *cur = create_identity(&self.data_dir, &self.preferred)?; + Ok(()) + } +} + +impl Identity { fn sign_raw(&self, message: &[u8]) -> io::Result> { match &self.backend { Backend::Software(k) => { @@ -155,15 +178,58 @@ impl DeviceKey { impl kv_ffi::DeviceSigner for DeviceKey { fn device_id(&self) -> String { - self.id.clone() + self.id() } fn public_key(&self) -> Vec { - self.public.clone() + self.read().public.clone() } fn sign(&self, message: Vec) -> Result, CoreError> { - self.sign_raw(&message).map_err(|e| CoreError::Internal { msg: format!("device key: {e}") }) + self.read().sign_raw(&message).map_err(|e| CoreError::Internal { msg: format!("device key: {e}") }) + } + + fn reset(&self) -> Result<(), CoreError> { + DeviceKey::reset(self).map_err(|e| CoreError::Internal { msg: format!("device key reset: {e}") }) + } +} + +/// A new key with a new device id in the first store that works; records both. +fn create_identity(data_dir: &Path, preferred: &[KeyStorage]) -> io::Result { + let id = uuid::Uuid::new_v4().to_string(); + let mut last_err = io::Error::other("no key store available"); + for &storage in preferred { + match create(data_dir, &id, storage) { + Ok(key) => { + private_file::write(&data_dir.join(ID_FILE), id.as_bytes())?; + private_file::write(&data_dir.join(STORE_FILE), storage.tag().as_bytes())?; + return Ok(key); + } + Err(e) => { + eprintln!("kestrove: device key store {} unavailable: {e}", storage.describe()); + last_err = e; + } + } + } + Err(last_err) +} + +/// Deletes `identity`'s key material from its store. Missing material is not an error. +fn destroy(data_dir: &Path, identity: &Identity) -> io::Result<()> { + let remove = |p: PathBuf| match std::fs::remove_file(p) { + Err(e) if e.kind() != io::ErrorKind::NotFound => Err(e), + _ => Ok(()), + }; + match identity.storage { + KeyStorage::File => remove(key_file(data_dir)), + #[cfg(not(windows))] + KeyStorage::Keychain | KeyStorage::SecretService => keyring_slot::delete(&identity.id), + #[cfg(windows)] + KeyStorage::Dpapi => remove(key_file(data_dir).with_extension("dpapi")), + #[cfg(windows)] + KeyStorage::Tpm => cng::Key::delete(&identity.id), + #[allow(unreachable_patterns)] + _ => Ok(()), } } @@ -184,9 +250,9 @@ fn platform_stores() -> Vec { } } -fn software(id: &str, key: SigningKey, storage: KeyStorage) -> DeviceKey { +fn software(id: &str, key: SigningKey, storage: KeyStorage) -> Identity { let public = key.verifying_key().to_encoded_point(false).as_bytes().to_vec(); - DeviceKey { id: id.to_string(), public, backend: Backend::Software(key), storage } + Identity { id: id.to_string(), public, backend: Backend::Software(key), storage } } fn parse_scalar(bytes: &[u8]) -> io::Result { @@ -208,7 +274,7 @@ fn key_file(data_dir: &Path) -> PathBuf { data_dir.join("device-key") } -fn load(data_dir: &Path, id: &str, storage: KeyStorage) -> io::Result> { +fn load(data_dir: &Path, id: &str, storage: KeyStorage) -> io::Result> { let secret = match storage { KeyStorage::File => private_file::read(&key_file(data_dir))?.map(Zeroizing::new), #[cfg(not(windows))] @@ -220,7 +286,7 @@ fn load(data_dir: &Path, id: &str, storage: KeyStorage) -> io::Result { - return Ok(cng::Key::open(id)?.map(|k| DeviceKey { + return Ok(cng::Key::open(id)?.map(|k| Identity { id: id.to_string(), public: k.public_key().unwrap_or_default(), backend: Backend::Cng(k), @@ -233,12 +299,12 @@ fn load(data_dir: &Path, id: &str, storage: KeyStorage) -> io::Result io::Result { +fn create(data_dir: &Path, id: &str, storage: KeyStorage) -> io::Result { #[cfg(windows)] if storage == KeyStorage::Tpm { let k = cng::Key::create(id)?; let public = k.public_key()?; - return Ok(DeviceKey { id: id.to_string(), public, backend: Backend::Cng(k), storage }); + return Ok(Identity { id: id.to_string(), public, backend: Backend::Cng(k), storage }); } let secret = new_scalar(); match storage { @@ -275,6 +341,13 @@ mod keyring_slot { } } + pub fn delete(id: &str) -> io::Result<()> { + match entry(id)?.delete_credential() { + Ok(()) | Err(keyring::Error::NoEntry) => Ok(()), + Err(e) => Err(io::Error::other(e)), + } + } + pub fn put(id: &str, secret: &[u8]) -> io::Result<()> { let e = entry(id)?; e.set_secret(secret).map_err(io::Error::other)?; @@ -361,7 +434,7 @@ mod cng { use sha2::{Digest, Sha256}; use windows_sys::Win32::Security::Cryptography::{ - NCryptCreatePersistedKey, NCryptExportKey, NCryptFinalizeKey, NCryptFreeObject, NCryptOpenKey, + NCryptCreatePersistedKey, NCryptDeleteKey, NCryptExportKey, NCryptFinalizeKey, NCryptFreeObject, NCryptOpenKey, NCryptOpenStorageProvider, NCryptSignHash, BCRYPT_ECCKEY_BLOB, BCRYPT_ECCPUBLIC_BLOB, BCRYPT_ECDSA_P256_ALGORITHM, BCRYPT_ECDSA_PUBLIC_P256_MAGIC, MS_PLATFORM_CRYPTO_PROVIDER, NCRYPT_KEY_HANDLE, NCRYPT_PROV_HANDLE, NCRYPT_SILENT_FLAG, @@ -443,6 +516,15 @@ mod cng { Ok(key) } + /// Deletes the persisted TPM key for `id` (the server removed this device). A missing key is fine. + pub fn delete(id: &str) -> io::Result<()> { + let Some(key) = Self::open(id)? else { return Ok(()) }; + // SAFETY: a live handle; NCryptDeleteKey also frees it, so `Drop` must not run afterwards. + let hr = unsafe { NCryptDeleteKey(key.0, NCRYPT_SILENT_FLAG) }; + std::mem::forget(key); + check(hr, "deleting the TPM key") + } + /// SEC1 uncompressed public key (0x04 || X || Y). pub fn public_key(&self) -> io::Result> { let mut buf = [0u8; 8 + 64]; @@ -533,6 +615,27 @@ mod tests { assert_ne!(a.device_id(), b.device_id()); } + /// docs/03 "Removed devices": a reset replaces the key and the id, on disk too, and leaves no old key behind. + #[test] + fn reset_replaces_key_and_id() { + let dir = tempfile::tempdir().unwrap(); + let a = DeviceKey::load_or_create_file(dir.path()).unwrap(); + let (old_id, old_pk) = (a.device_id(), a.public_key()); + a.reset().unwrap(); + assert_ne!(a.device_id(), old_id); + assert_ne!(a.public_key(), old_pk); + assert!(uuid::Uuid::parse_str(&a.device_id()).is_ok()); + let sig = a.sign(b"after".to_vec()).unwrap(); + VerifyingKey::from_sec1_bytes(&a.public_key()) + .unwrap() + .verify(b"after", &Signature::from_slice(&sig).unwrap()) + .unwrap(); + // The next launch loads the new identity. + let b = DeviceKey::load_or_create_file(dir.path()).unwrap(); + assert_eq!((b.device_id(), b.public_key()), (a.device_id(), a.public_key())); + assert_eq!(a.storage(), KeyStorage::File); + } + #[test] #[cfg(unix)] fn key_file_is_owner_only() { diff --git a/src-tauri/src/sync.rs b/src-tauri/src/sync.rs index cffbc67..7daae25 100644 --- a/src-tauri/src/sync.rs +++ b/src-tauri/src/sync.rs @@ -152,6 +152,9 @@ pub fn describe(e: &CoreError) -> String { CoreError::PendingApproval => "This computer is waiting for approval on one of your other devices.".into(), CoreError::NotConfigured => "No sync server is set up yet.".into(), CoreError::NotAuthenticated => "Sign in to the sync server again (lock and unlock with your password).".into(), + CoreError::DeviceRevoked => "This device was removed from your account. Set it up again: lock and unlock with \ + your master password, then approve this computer on another device." + .into(), CoreError::IntegrityAlarm { vault_id, reason } => format!( "The server sent vault data that failed verification (vault {vault_id}: {reason}). Syncing stopped; \ your local data is unchanged." @@ -426,8 +429,11 @@ impl SyncService { .map(|ds| ds.iter().filter(|d| !d.approved && !d.revoked && !d.current).count() as u32); } let err = result.as_ref().err().map(describe); + // A removed device got a new key inside the core (docs/03 "Removed devices"): show its fingerprint. + let fingerprint = self.core.device_key_fingerprint().ok(); self.update(|s| { s.syncing = false; + s.device_fingerprint = fingerprint; match &result { Ok(r) => { s.last_sync_at = Some(now_secs()); @@ -543,8 +549,11 @@ impl SyncService { pub async fn finish_setup(&self, master_password: Zeroizing) -> Result { self.require_key()?; let setup = lock(&self.pending_setup).clone().ok_or("Scan or paste a setup code first.")?; - let result = - self.core.login_with_setup(setup.clone(), master_password.to_string()).await.map_err(|e| describe(&e))?; + let result = self.core.login_with_setup(setup.clone(), master_password.to_string()).await; + // With a ticket the core already re-enrolled a removed device under a new key; refresh it either way. + let fingerprint = self.core.device_key_fingerprint().ok(); + self.update(|s| s.device_fingerprint = fingerprint); + let result = result.map_err(|e| describe(&e))?; self.update(|s| { s.server_url = Some(setup.server_url.clone()); s.pins = setup.spki_pins.clone(); @@ -655,6 +664,49 @@ mod tests { assert_eq!(again.status().device_fingerprint, svc.status().device_fingerprint); } + /// docs/03 "Removed devices", end to end through kv-sync's test server: this computer waits for approval, is + /// denied (revoked), and a fresh "Add a device" code signs it straight in under a new key and id. + #[tokio::test] + async fn removed_computer_reenrols_with_a_fresh_add_device_code() { + use kv_sync::test_server::{TestServer, SETUP_TOKEN}; + const PW: &str = "a long master password"; + let server = TestServer::start().await; + let phone_dir = tempfile::tempdir().unwrap(); + let phone = VaultCore::new_in_memory(); + phone.create_account(PW.into()).unwrap(); + phone.configure_server(server.url(), vec![]).unwrap(); + let phone_key: Arc = + Arc::new(DeviceKey::load_or_create_file(phone_dir.path()).unwrap()); + phone.set_device_signer(phone_key, "phone".into(), PlatformDto::Ios).unwrap(); + phone.register_account(SETUP_TOKEN.into()).await.unwrap(); + server.state().require_approval = true; + + let dir = tempfile::tempdir().unwrap(); + let svc = SyncService::new( + VaultCore::new_in_memory(), + dir.path(), + DeviceKey::load_or_create_file(dir.path()).map_err(|e| e.to_string()), + Box::new(|_| {}), + ); + svc.setup_from_text(&phone.setup_payload_for_kit().unwrap()).unwrap(); + assert_eq!(svc.finish_setup(PW.to_string().into()).await.unwrap(), SignIn::PendingApproval); + let old_fp = svc.status().device_fingerprint; + let pending = phone.list_devices().await.unwrap().into_iter().find(|d| !d.approved).unwrap(); + assert_eq!(Some(pending.key_fingerprint.clone()), old_fp); + let old_id = pending.device_id; + phone.revoke_device(old_id.clone()).await.unwrap(); + + svc.setup_from_text(&phone.add_device_payload().await.unwrap().payload).unwrap(); + assert_eq!(svc.finish_setup(PW.to_string().into()).await.unwrap(), SignIn::Ok); + let new_id = kv_ffi::DeviceSigner::device_id(&DeviceKey::load_or_create_file(dir.path()).unwrap()); + assert_ne!(new_id, old_id); + assert_ne!(svc.status().device_fingerprint, old_fp); + assert!(svc.status().logged_in); + let devices = phone.list_devices().await.unwrap(); + assert!(devices.iter().any(|d| d.device_id == old_id && d.revoked)); + assert!(devices.iter().any(|d| d.device_id == new_id && d.approved)); + } + #[tokio::test] async fn sync_needs_unlock() { let dir = tempfile::tempdir().unwrap(); diff --git a/src/main.ts b/src/main.ts index 4a39d90..d7e19bb 100644 --- a/src/main.ts +++ b/src/main.ts @@ -783,7 +783,7 @@ async function renderDevices(box: HTMLElement) { } if (!d.current) { actions.append(button(d.approved ? "Remove" : "Reject", "danger", () => confirmModal(`Remove “${d.name}”?`, - [p("kv-muted", "It will be signed out and can't sync again. Its local copy stays on it.")], "Remove device", + [p("kv-muted", "It will be signed out and can't sync again. Its local copy stays on it. To add it back later, set it up again as a new device.")], "Remove device", async () => { await invoke("revoke_device", { deviceId: d.device_id }); await reload(); }, true))); } list.append(h("div", "kv-frow",