Lock a file the nonce store's own rename cannot unlink

keep-cli/src/signer/nonce_store.rs

@@ -5,7 +5,7 @@ use fs2::FileExt;
 use serde::{Deserialize, Serialize};
 use std::collections::HashMap;
 use std::fs::OpenOptions;
-use std::io::{Read, Write};
+use std::io::Write;
 use std::path::{Path, PathBuf};
 
 #[derive(Debug, Clone, Serialize, Deserialize)]
@@ -84,23 +84,57 @@ impl NonceStore {
             std::fs::create_dir_all(parent).context("Failed to create nonce store directory")?;
         }
 
-        let file = OpenOptions::new()
-            .read(true)
-            .write(true)
-            .create(true)
-            .truncate(false)
-            .open(&self.path)
-            .context("Failed to open nonce store")?;
-
-        file.lock_exclusive()
+        // Lock a dedicated sibling, never the store itself.
+        //
+        // Two things are wrong with locking the store, and they fail
+        // differently. The reader below used to read from the locked handle, so
+        // a caller that opened the store before another replaced it read the
+        // unlinked file and never saw the claim it had just waited for. That is
+        // the one the concurrency test reproduces. The second is not covered by
+        // any test here: once the store has been replaced, one caller can hold
+        // a lock on the unlinked inode while another holds a lock on the file
+        // that replaced it, so both are inside the critical section at once and
+        // exclusion has quietly stopped meaning anything. A sibling that is
+        // never renamed is what keeps the second case impossible.
+        //
+        // Locking the store meant locking an inode the write path then renamed
+        // over, which unlinks it. A second caller that opened the store before
+        // that rename held the old inode, waited on it, and was handed the lock
+        // once the first caller finished, at which point it read the unlinked
+        // file: the state from before the claim it was waiting for. It then
+        // re-issued the same round-1 commitment, and two shares over one nonce
+        // under different challenges recover the signer's key share, which is
+        // the outcome this guard exists to prevent. Serialising on a file that
+        // is never replaced is what makes the critical section mean anything.
+        let lock_path = self.path.with_extension("lock");
+        let lock_file = {
+            let mut opts = OpenOptions::new();
+            opts.create(true).write(true).truncate(false);
+            #[cfg(unix)]
+            {
+                use std::os::unix::fs::OpenOptionsExt;
+                opts.mode(0o600);
+            }
+            opts.open(&lock_path)
+                .context("Failed to open nonce store lock")?
+        };
+        lock_file
+            .lock_exclusive()
             .context("Failed to acquire nonce store lock")?;
 
+        // Opened inside the critical section, so this always resolves to the
+        // file the previous holder left behind rather than one captured before
+        // they replaced it.
         let mut data = {
             let mut content = String::new();
-            let mut reader = &file;
-            reader
-                .read_to_string(&mut content)
-                .context("Failed to read nonce store")?;
+            match std::fs::read_to_string(&self.path) {
+                Ok(c) => content = c,
+                Err(e) if e.kind() == std::io::ErrorKind::NotFound => {}
+                Err(e) => {
+                    let _ = FileExt::unlock(&lock_file);
+                    return Err(anyhow::Error::from(e).context("Failed to read nonce store"));
+                }
+            }
             if content.is_empty() {
                 NonceStoreData {
                     version: STORE_VERSION,
@@ -122,7 +156,7 @@ impl NonceStore {
 
         self.data = data;
 
-        FileExt::unlock(&file).context("Failed to release nonce store lock")?;
+        FileExt::unlock(&lock_file).context("Failed to release nonce store lock")?;
 
         Ok(result)
     }
@@ -436,4 +470,103 @@ mod tests {
              or the caller signs against a claim that may not survive a reboot"
         );
     }
+
+    /// Concurrent handles must not both hand out the same commitment.
+    ///
+    /// The sequential test above cannot reach this. It claims and then claims
+    /// again, so the second call opens the store after the first replaced it and
+    /// naturally sees the claim. The dangerous interleaving is the one where a
+    /// caller opens the store, waits on the lock while the holder replaces the
+    /// file, and is then handed a lock on the file that was replaced.
+    ///
+    /// Detection is probabilistic in the direction that is safe: on the fixed
+    /// code exactly one winner is guaranteed, so this cannot fail spuriously.
+    /// On the broken code it needs the threads to overlap, which repeating the
+    /// round makes very likely without making a green run a lie.
+    ///
+    /// It covers the stale-read half only. Reverting just the lock leaves this
+    /// green, because with the read taken by path there is no stale handle to
+    /// read from. The exclusion half is pinned separately, below, by asserting
+    /// the locked inode is the one thing the write path never replaces.
+    #[test]
+    fn concurrent_handles_hand_out_a_commitment_exactly_once() {
+        for round in 0..40 {
+            let dir = tempfile::tempdir().unwrap();
+            // Establish the store so every thread races on replacing it rather
+            // than on creating it.
+            store(dir.path())
+                .check_and_add_nonce("g", "seed", None)
+                .unwrap();
+
+            let path = dir.path().to_path_buf();
+            let barrier = std::sync::Arc::new(std::sync::Barrier::new(8));
+            let handles: Vec<_> = (0..8)
+                .map(|_| {
+                    let path = path.clone();
+                    let barrier = std::sync::Arc::clone(&barrier);
+                    std::thread::spawn(move || {
+                        let mut s = NonceStore::open(&path).unwrap();
+                        barrier.wait();
+                        s.check_and_add_nonce("g", "contested", None)
+                    })
+                })
+                .collect();
+
+            // Every thread must succeed, not merely not-win. Mapping errors to
+            // "not granted" would let an implementation that fails under
+            // contention for seven of eight callers still show one winner.
+            let outcomes: Vec<bool> = handles
+                .into_iter()
+                .map(|h| h.join().unwrap().expect("a contended claim must not error"))
+                .collect();
+            let winners = outcomes.iter().filter(|granted| **granted).count();
+
+            assert_eq!(
+                winners, 1,
+                "round {round}: exactly one caller may be handed a commitment; \
+                 more than one means two signature shares under one nonce"
+            );
+        }
+    }
+
+    /// The locked file must be the one thing a claim does not replace.
+    ///
+    /// This is the invariant the whole fix rests on, and unlike the race above
+    /// it is directly observable: a claim replaces the store, so the store's
+    /// inode changes, and it must not touch the lock's. Locking the store made
+    /// those the same inode, so the lock was destroyed by the very operation it
+    /// was meant to be protecting.
+    ///
+    /// Deterministic and thread-free. It also fails if someone later deletes
+    /// the lock file between calls as a cleanup, which is the realistic way
+    /// this regresses.
+    #[cfg(unix)]
+    #[test]
+    fn a_claim_replaces_the_store_and_never_the_lock() {
+        use std::os::unix::fs::MetadataExt;
+
+        let dir = tempfile::tempdir().unwrap();
+        let mut s = store(dir.path());
+        s.check_and_add_nonce("g", "first", None).unwrap();
+
+        let store_path = dir.path().join("nonce_store.json");
+        let lock_path = dir.path().join("nonce_store.lock");
+        let store_before = std::fs::metadata(&store_path).unwrap().ino();
+        let lock_before = std::fs::metadata(&lock_path).unwrap().ino();
+
+        s.check_and_add_nonce("g", "second", None).unwrap();
+
+        assert_ne!(
+            store_before,
+            std::fs::metadata(&store_path).unwrap().ino(),
+            "a claim is written by replacing the store, so its inode must change"
+        );
+        assert_eq!(
+            lock_before,
+            std::fs::metadata(&lock_path).unwrap().ino(),
+            "the locked file must survive the claim; if the write path replaces \
+             it, callers end up holding locks on different inodes and exclusion \
+             silently stops applying"
+        );
+    }
 }