From 81ffd00ba99d2d836060ddb15fa28b93c9aa9744 Mon Sep 17 00:00:00 2001 From: 2ro <17595647+2ro@users.noreply.github.com> Date: Mon, 6 Jul 2026 07:56:05 -0400 Subject: [PATCH] goblin: make password-change identity re-encryption all-or-nothing and rebind the live service reencrypt_all was best-effort per file: a partial failure left some identities on the old password and some on the new, a split the user cannot open with one password. Make it all-or-nothing: stage every re-encryption to a sibling temp file and only commit (rename into place) once every identity re-encrypts cleanly; any phase-1 failure deletes the temps and leaves every identity on the old password. Surface the failure: change_password now returns a hard error when the re-encryption fails instead of logging and reporting success. Keep the running NostrService in sync after a successful change by refreshing its in-memory identity blobs to the new password, so same-session gated ops (which re-unlock with the new password) work without a wallet reopen. The decrypted keys are unchanged by a password change, so listening and sending are untouched. Adds an all-or-nothing test. --- src/nostr/client.rs | 16 ++++++ src/nostr/identities.rs | 122 ++++++++++++++++++++++++++++++++-------- src/wallet/wallet.rs | 25 +++++--- 3 files changed, 131 insertions(+), 32 deletions(-) diff --git a/src/nostr/client.rs b/src/nostr/client.rs index 1f9486ae..519f4080 100644 --- a/src/nostr/client.rs +++ b/src/nostr/client.rs @@ -245,6 +245,22 @@ impl NostrService { } } + /// Re-encrypt every in-memory held identity's ncryptsec from `old` to `new`, + /// keeping the running service in sync with a wallet-password change without a + /// teardown. A password change does not alter the decrypted keys, so sending + /// and listening keep working untouched; this only refreshes the encrypted + /// blobs the in-memory copies carry, so a same-session gated op (which + /// re-unlocks the stored identity with the NEW password) and any later + /// identity-file save both use the new password rather than the stale old one. + /// Best-effort per copy: a copy that fails to re-encrypt is left as is (a + /// gated op on it may then need the app reopened), never a hard error. + pub fn reencrypt_in_memory(&self, old: &str, new: &str) { + for h in self.recv.write().iter_mut() { + let _ = h.identity.reencrypt(old, new); + } + let _ = self.identity.write().reencrypt(old, new); + } + /// Instant, purely-local identity switch: re-point the active keys/identity to /// a held identity already unlocked and already listening. No password, no /// teardown, no catch-up. `false` if `hex` is not held. diff --git a/src/nostr/identities.rs b/src/nostr/identities.rs index d89975f6..831a38d3 100644 --- a/src/nostr/identities.rs +++ b/src/nostr/identities.rs @@ -286,40 +286,66 @@ impl HeldIdentities { } /// Re-encrypt every held identity's ncryptsec from `old` to `new`, in place - /// on disk. Used by the wallet-password change so all front doors follow the - /// one password. Best-effort per file; returns the first error encountered - /// after attempting the rest. + /// on disk, ALL-OR-NOTHING. Used by the wallet-password change so all front + /// doors follow the one password. A partial move (some identities on the new + /// password, some still on the old) would be a split state the user cannot + /// recover with a single password, so this stages every re-encryption to a + /// sibling temp file first and only commits (renames into place) once EVERY + /// identity has re-encrypted cleanly. Any phase-1 failure deletes the temps + /// and leaves every identity untouched on the old password. pub fn reencrypt_all( &self, nostr_dir: &PathBuf, old: &str, new: &str, ) -> Result<(), HeldError> { - let mut first_err = None; + let suffix = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .map(|d| d.as_nanos()) + .unwrap_or(0); + // Phase 1: decrypt+re-encrypt every identity and stage it to a temp file + // beside its target. Nothing is committed yet, so a failure here (wrong old + // password, unreadable file, encrypt error) aborts the WHOLE change with + // every identity still untouched on the old password, never a split. + let mut staged: Vec<(PathBuf, PathBuf)> = Vec::new(); + let cleanup = |staged: &[(PathBuf, PathBuf)]| { + for (tmp, _) in staged { + let _ = fs::remove_file(tmp); + } + }; for entry in &self.identities { let abs = entry.abs_path(nostr_dir); - match NostrIdentity::load_at(&abs) { - Some(mut id) => { - if let Err(e) = id.reencrypt(old, new) { - first_err.get_or_insert(HeldError::Io(e.to_string())); - continue; - } - if let Err(e) = id.save_at(&abs) { - first_err.get_or_insert(HeldError::Io(e.to_string())); - } - } - None => { - first_err.get_or_insert(HeldError::Io(format!( - "identity file unreadable: {}", - entry.path - ))); - } + let Some(mut id) = NostrIdentity::load_at(&abs) else { + cleanup(&staged); + return Err(HeldError::Io(format!( + "identity file unreadable: {}", + entry.path + ))); + }; + if let Err(e) = id.reencrypt(old, new) { + cleanup(&staged); + return Err(HeldError::Io(e.to_string())); + } + let tmp = + abs.with_extension(format!("reencrypt-tmp-{}-{}", std::process::id(), suffix)); + if let Err(e) = id.save_at(&tmp) { + cleanup(&staged); + return Err(HeldError::Io(e.to_string())); + } + staged.push((tmp, abs)); + } + // Phase 2: every identity re-encrypted cleanly; commit by renaming each + // temp over its target. This window is small; a rename that still fails + // returns an explicit error rather than pretending the change succeeded. + for (tmp, abs) in &staged { + if let Err(e) = fs::rename(tmp, abs) { + return Err(HeldError::Io(format!( + "committing re-encrypted identity {}: {e}", + abs.display() + ))); } } - match first_err { - Some(e) => Err(e), - None => Ok(()), - } + Ok(()) } } @@ -540,6 +566,54 @@ mod tests { let _ = std::fs::remove_dir_all(&dir); } + #[test] + fn reencrypt_all_is_all_or_nothing() { + // Two held identities, one with a corrupted ncryptsec so its decrypt (and + // thus the whole re-encryption) fails. The batch must fail AND leave the + // other identity untouched on the OLD password (no partial move). + let dir = tmpdir("allornothing"); + let (legacy, _) = NostrIdentity::create_random("old").unwrap(); + legacy.save(&dir).unwrap(); + let legacy_hex = legacy.pubkey_hex().unwrap(); + let (mut idx, _) = HeldIdentities::load_or_migrate(&dir, &legacy).unwrap(); + let (second, _) = NostrIdentity::create_random("old").unwrap(); + let second_hex = idx.add(&dir, &second).unwrap(); + + // Corrupt the second identity's ncryptsec on disk. + let second_abs = idx.entry(&second_hex).unwrap().abs_path(&dir); + let mut broken = NostrIdentity::load_at(&second_abs).unwrap(); + broken.ncryptsec = "ncryptsec1qqqqqbroken".to_string(); + broken.save_at(&second_abs).unwrap(); + + // All-or-nothing: the batch fails... + assert!( + idx.reencrypt_all(&dir, "old", "new").is_err(), + "a failing identity must abort the whole re-encryption" + ); + // ...and the untouched legacy identity still opens under the OLD password, + // never having been committed to the new one. + let legacy_still = idx.entry(&legacy_hex).unwrap().load(&dir).unwrap(); + assert!( + legacy_still.unlock("old").is_ok(), + "untouched identity must still open under the old password" + ); + assert!( + legacy_still.unlock("new").is_err(), + "untouched identity must not have moved to the new password" + ); + // No temp files left behind after the aborted batch. + let leftovers: Vec<_> = std::fs::read_dir(&dir) + .unwrap() + .filter_map(|e| e.ok()) + .filter(|e| e.file_name().to_string_lossy().contains("reencrypt-tmp")) + .collect(); + assert!( + leftovers.is_empty(), + "aborted batch must clean up its temps" + ); + let _ = std::fs::remove_dir_all(&dir); + } + #[test] fn imported_nsec_adds_as_held_identity_and_unlocks() { // The multi-identity import path: a bare nsec becomes a held identity via diff --git a/src/wallet/wallet.rs b/src/wallet/wallet.rs index 386b8e60..d8269780 100644 --- a/src/wallet/wallet.rs +++ b/src/wallet/wallet.rs @@ -2277,15 +2277,24 @@ impl Wallet { } // The grin seed password changed; re-encrypt EVERY held nostr identity's // ncryptsec from old to new through the same NIP-49 path, so all front - // doors follow the one wallet password. Best-effort and non-fatal: a - // failure is logged (never swallowed as success), and the grin password - // change already committed above. Takes effect for the running service on - // the next wallet open (it reloads the active identity from disk). + // doors follow the one wallet password. This is ALL-OR-NOTHING: a partial + // move would leave some identities on the old password and some on the new + // (a split the user cannot open with one password), so on any failure we + // surface a hard error rather than silently continuing. Every identity is + // then left untouched on the old password. let nostr_dir = self.get_config().get_nostr_path(); - if let Some(index) = HeldIdentities::load(&nostr_dir) - && let Err(e) = index.reencrypt_all(&nostr_dir, &old, &new) - { - error!("nostr: re-encrypting held identities after password change: {e}"); + if let Some(index) = HeldIdentities::load(&nostr_dir) { + index.reencrypt_all(&nostr_dir, &old, &new).map_err(|e| { + error!("nostr: re-encrypting held identities after password change: {e}"); + Error::GenericError(format!("nostr identity re-encryption failed: {e}")) + })?; + // Keep the running service in sync so same-session gated ops (which + // re-unlock with the new password) work without a wallet reopen. The + // decrypted keys are unchanged by a password change, so this only + // refreshes the in-memory encrypted blobs. + if let Some(svc) = self.nostr_service() { + svc.reencrypt_in_memory(&old, &new); + } } Ok(()) }