Mark a FROST nonce used when it is claimed, not never - #933
Conversation
WalkthroughNonce storage now supports versioned migration, session-aware one-time claims, stale-handle protection, and atomic persistence. Hardware FROST signing passes the session ID when it registers a nonce. ChangesFROST nonce lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
1cf0233 to
8782b5b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
keep-cli/src/signer/nonce_store.rs (1)
87-96: 🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy liftKeep the nonce store lock tied to a stable pathname.
with_lockacquireslock_exclusive()on the file opened forself.path, butwrite_locked()replacesself.pathwith a new inode. A second opener can race to open the original inode before the rename and then acquire its own lock afterunlock()releases the old handle. Use a separate lock file whose pathname is never replaced byrename, or keep the store file in place for the duration ofwith_lock.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@keep-cli/src/signer/nonce_store.rs` around lines 87 - 96, Update with_lock and write_locked so the lock remains associated with a stable inode across nonce-store replacement: use a separate, never-renamed lock file for lock_exclusive, or avoid replacing the locked store inode while with_lock runs. Ensure all openers acquire the same stable pathname lock before accessing or renaming self.path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@keep-cli/src/signer/nonce_store.rs`:
- Around line 140-159: Update write_locked after std::fs::rename to open
nonce_store.json’s parent directory and call sync_all() on it, propagating
contextual errors consistently with the existing file fsync operations. Preserve
the current temp-file write, file sync, and atomic rename sequence.
---
Outside diff comments:
In `@keep-cli/src/signer/nonce_store.rs`:
- Around line 87-96: Update with_lock and write_locked so the lock remains
associated with a stable inode across nonce-store replacement: use a separate,
never-renamed lock file for lock_exclusive, or avoid replacing the locked store
inode while with_lock runs. Ensure all openers acquire the same stable pathname
lock before accessing or renaming self.path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: cf96e155-494e-4e32-9fdb-3e19205e67f5
📒 Files selected for processing (2)
keep-cli/src/commands/frost_network/hardware.rskeep-cli/src/signer/nonce_store.rs
| fn write_locked(path: &Path, content: &str) -> Result<()> { | ||
| let tmp_path = path.with_extension("tmp"); | ||
| let mut tmp = { | ||
| let mut opts = OpenOptions::new(); | ||
| opts.create(true).write(true).truncate(true); | ||
| #[cfg(unix)] | ||
| { | ||
| use std::os::unix::fs::OpenOptionsExt; | ||
| opts.mode(0o600); | ||
| } | ||
| opts.open(&tmp_path) | ||
| .context("Failed to open nonce store temp file")? | ||
| }; | ||
| tmp.write_all(content.as_bytes()) | ||
| .context("Failed to write nonce store")?; | ||
| writer.flush().context("Failed to flush nonce store")?; | ||
| tmp.sync_all().context("Failed to fsync nonce store")?; | ||
| drop(tmp); | ||
| std::fs::rename(&tmp_path, path).context("Failed to replace nonce store")?; | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files matching nonce_store:\n'
fd -a 'nonce_store\.rs$' . || true
file=$(fd 'nonce_store\.rs$' . | head -n 1 || true)
if [ -n "${file:-}" ]; then
printf '\nSelected file: %s\n' "$file"
wc -l "$file"
printf '\nLines 1-220:\n'
sed -n '1,220p' "$file" | cat -n
fi
printf '\nSearch for NonceStore/write_locked usages:\n'
rg -n "NonceStore|write_locked|claim|nonce|rename|sync_all" -S . || trueRepository: privkeyio/keep
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Relevant keep-web directory fsyncs:\n'
sed -n '444,795p' keep-web/src/state.rs | cat -n
printf '\nNonce Store check_and_add_nonce and remaining lines 220-330:\n'
sed -n '220,330p' keep-cli/src/signer/nonce_store.rs | cat -nRepository: privkeyio/keep
Length of output: 22564
Fsync the nonce store directory after the rename.
write_locked fsyncs the temp file, then renames it into place, but a power loss can persist the rename without persisting the containing directory entry. That can make self.path resolve to the old nonce store after reboot, so the claim can be replayed. Open nonce_store.json.parent() and call sync_all() after std::fs::rename, matching the other atomic writes in the workspace.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@keep-cli/src/signer/nonce_store.rs` around lines 140 - 159, Update
write_locked after std::fs::rename to open nonce_store.json’s parent directory
and call sync_all() on it, propagating contextual errors consistently with the
existing file fsync operations. Preserve the current temp-file write, file sync,
and atomic rename sequence.
1 participant
Summary
NonceStore::check_and_add_noncecould not reject a reused FROST nonce commitment. Its reject branch testede.used, and the only writer of that field wasmark_nonce_used, which had no callers anywhere in the workspace. No entry was everused, so a commitment seen a second time fell through to theOk(true)branch and was signed.keep-cli/src/commands/frost_network/hardware.rs:82reads as protected and is not: its"nonce has already been used - aborting to prevent key compromise"error was unreachable.mark_nonce_usedis deleted; it was the phantom second phase that made the guard look complete.Why it matters
Signing twice with one FROST nonce under two different challenges gives
s = (z1 - z2) / (c1 - c2)— the signer's key share, recoverable by anyone who sees both signature shares. This store is the host-side defence for the hardware-signer path, where the commitment comes back from an external device: a device that replays a persisted nonce checkpoint after a reboot, or whose own RNG has degraded, is exactly the case it exists to catch.What was subtle about it
The two-phase design is real and load-bearing, so the fix is not "reject on presence". The kind-21106 pre-commitment flow (
hardware.rs:416) registers commitments ahead of time viaadd_nonce, deliberately unused, andnonce_statsreports them as available. Rejecting any commitment already in the store would have broken that feature at first legitimate use. What was missing was only the transition from registered to claimed, which now happens insidecheck_and_add_noncerather than in a function nobody called.Test plan
check_and_add_nonce, three of the four fail. The one that passes (groups_do_not_share_commitments) never depended on the defectcargo test -p keep-cli --bins nonce_store: 4 passedcargo clippy -p keep-cli --all-targetsclean,cargo fmt --checkcleanProvenance
Found by an adversarial review pass over an unrelated RNG-hygiene change (#932), which swept the workspace for guards that cannot fire. Filed separately because it is a different bug class in a different subsystem, and mixing it into that PR would bury both.
Summary by CodeRabbit