Make the nonce-store rename durable, not just its contents

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"
+        );
+    }
 }

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.

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<u64> {

keep-frost-net/src/nonce_store.rs

@@ -111,14 +111,28 @@ 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}")))?;
 
         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);