diff --git a/keep-cli/src/signer/nonce_store.rs b/keep-cli/src/signer/nonce_store.rs index c0b29c48..1fadfe6b 100644 --- a/keep-cli/src/signer/nonce_store.rs +++ b/keep-cli/src/signer/nonce_store.rs @@ -155,6 +155,12 @@ impl NonceStore { tmp.sync_all().context("Failed to fsync nonce store")?; drop(tmp); std::fs::rename(&tmp_path, path).context("Failed to replace nonce store")?; + // Syncing the temp file above makes its contents durable; it does not + // make the name pointing at them durable. Without this a power loss + // after the rename can leave the previous store in place, which is a + // claimed nonce silently becoming unclaimed: exactly the reboot replay + // this store exists to stop, and the case the file sync was added for. + keep_core::fsync_dir(path).context("Failed to fsync nonce store directory")?; Ok(()) } @@ -390,4 +396,44 @@ mod tests { "a reboot must not hand the nonce back" ); } + + /// A claim that cannot be made durable must not be reported as made. + /// + /// I had written that this change was untestable because crash durability + /// needs fault injection. That is true of the crash, and it is not true of + /// the property that matters here: the caller signs on `Ok(true)`, so a + /// directory sync that fails has to fail the claim rather than be swallowed. + /// An unreadable directory produces exactly that failure without simulating + /// anything, because opening the directory to sync it is what breaks. + /// + /// Unix only, and skipped as root, where the mode is not enforced. + #[cfg(unix)] + #[test] + fn a_claim_that_cannot_be_synced_is_not_reported_as_claimed() { + use std::os::unix::fs::PermissionsExt; + + let dir = tempfile::tempdir().unwrap(); + let mut s = store(dir.path()); + // Establish the store while the directory is still readable. + assert!(s.check_and_add_nonce("group", "aabb", None).unwrap()); + + // Write and search, but not read: the rename still succeeds, opening + // the directory to sync it does not. + std::fs::set_permissions(dir.path(), std::fs::Permissions::from_mode(0o300)).unwrap(); + // Root ignores the mode, and so do some filesystems. Check that the + // restriction actually took rather than asserting into a setup that + // never applied, which would pass whatever the code did. + if std::fs::File::open(dir.path()).is_ok() { + std::fs::set_permissions(dir.path(), std::fs::Permissions::from_mode(0o700)).unwrap(); + return; + } + let result = s.check_and_add_nonce("group", "ccdd", None); + std::fs::set_permissions(dir.path(), std::fs::Permissions::from_mode(0o700)).unwrap(); + + assert!( + result.is_err(), + "a claim whose record cannot be made durable must not return success, \ + or the caller signs against a claim that may not survive a reboot" + ); + } } diff --git a/keep-core/src/lib.rs b/keep-core/src/lib.rs index 599999a0..5b562a1f 100644 --- a/keep-core/src/lib.rs +++ b/keep-core/src/lib.rs @@ -71,6 +71,7 @@ pub(crate) mod rate_limit; /// Relay configuration for FROST shares. pub mod relay; mod rotation; +pub use rotation::fsync_dir; /// Arbitrary-secret records (passwords, API tokens, notes) stored in the vault. pub mod secret; /// Persistent encrypted storage backend. diff --git a/keep-core/src/rotation.rs b/keep-core/src/rotation.rs index 85d5f4b3..2c530591 100644 --- a/keep-core/src/rotation.rs +++ b/keep-core/src/rotation.rs @@ -73,18 +73,42 @@ fn secure_delete(path: &Path) -> std::io::Result<()> { fs::remove_file(path) } -#[cfg(not(windows))] -fn fsync_dir(path: &Path) -> std::io::Result<()> { - let parent = path - .parent() - .ok_or_else(|| std::io::Error::other("path has no parent directory"))?; - let dir = File::open(parent)?; - dir.sync_all() -} - -#[cfg(windows)] -fn fsync_dir(_path: &Path) -> std::io::Result<()> { - Ok(()) +/// Flushes the directory entry for `path` so a rename into it survives power +/// loss. +/// +/// Syncing a freshly written file makes its contents durable; it does not make +/// the name pointing at them durable. Without this a crash after an atomic +/// rename can leave the previous file in place, which is the difference between +/// a store that survives a reboot and one that quietly rolls back. +/// +/// Takes the file path, not the directory: this exists to pair with the +/// write-temp-then-rename idiom, and the caller already holds the destination. +/// +/// On Windows this is a no-op, and the guarantee is therefore absent rather +/// than provided elsewhere. A directory handle cannot be opened through the +/// standard library there, and the rename primitive orders its metadata +/// updates without promising they have reached the disk when it returns. +pub fn fsync_dir(path: &Path) -> std::io::Result<()> { + #[cfg(windows)] + { + let _ = path; + Ok(()) + } + #[cfg(not(windows))] + { + // `parent` is `Some("")` for a single-component relative path, not + // `None`, and opening "" is ENOENT. Treat it as the current directory, + // matching the guard the signing path already applies to the same case. + let parent = match path.parent() { + Some(p) if !p.as_os_str().is_empty() => p, + Some(_) => Path::new("."), + None => { + return Err(std::io::Error::other("path has no parent directory")); + } + }; + let dir = File::open(parent)?; + dir.sync_all() + } } fn copy_with_retry(from: &Path, to: &Path) -> std::io::Result { diff --git a/keep-frost-net/src/nonce_store.rs b/keep-frost-net/src/nonce_store.rs index 248903cb..6465a080 100644 --- a/keep-frost-net/src/nonce_store.rs +++ b/keep-frost-net/src/nonce_store.rs @@ -111,6 +111,14 @@ impl NonceStore for FileNonceStore { use std::os::unix::fs::OpenOptionsExt; opts.mode(0o600); } + // Whether this call is creating the store decides whether its directory + // entry needs flushing below. Appends to an existing file are covered by + // syncing the file itself; the very first record also creates a name, + // and a name that never reaches the disk takes the whole store with it, + // so every session id reads as unconsumed after the next boot. That is + // the first session, not the hundred-thousandth, so it is the case that + // matters most. + let is_new = !self.path.exists(); let mut file = opts .open(&self.path) .map_err(|e| FrostNetError::Session(format!("Failed to open nonce store: {e}")))?; @@ -118,7 +126,13 @@ impl NonceStore for FileNonceStore { let hex_id = hex::encode(session_id); let write_result = writeln!(file, "{hex_id}"); let sync_result = if write_result.is_ok() { - file.sync_all() + file.sync_all().and_then(|()| { + if is_new { + keep_core::fsync_dir(&self.path) + } else { + Ok(()) + } + }) } else { Ok(()) }; @@ -205,7 +219,12 @@ fn rewrite_nonce_file<'a>( } file.sync_all()?; - std::fs::rename(&tmp_path, path) + std::fs::rename(&tmp_path, path)?; + // The sync above makes the contents durable, not the name pointing at + // them. Without this a power loss after the rename can restore the + // previous file, which for a nonce store means a consumed nonce reads + // as available again on the next boot. + keep_core::fsync_dir(path) })(); let _ = FileExt::unlock(&lock_file);