Make the nonce-store rename durable, not just its contents - #950
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
WalkthroughThe change adds a public ChangesNonce store directory durability
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 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 |
|
Four taken, and two of them correct things I asserted rather than checked. The claim that no test could discriminate was wrong. I reasoned from "crash durability needs fault injection", which is true of the crash and not of the property that matters: the caller signs on a successful claim, so a sync that fails has to fail the claim. Making the directory writable and searchable but not readable produces that failure with nothing simulated. Removing the sync now fails that test with the message it was written for. Shipping a security fix on the strength of an argument for why it could not be tested is worse than shipping it untested, because the argument discourages the next person from looking. The missing-docs risk is real, and I verified it rather than taking it on faith: removing the doc comment from the other arm makes the crate fail to compile, so the lint does fire through the re-export and the undocumented Windows arm would have broken the release build. Pull-request CI cannot catch it because Windows only builds on push. Restructured into one documented function rather than a cfg pair, which removes the failure mode instead of patching it. The observation about which path was hardened is the sharpest of the set. The rewrite in the network store runs only after a hundred thousand entries; the append that creates the store runs on the first session. I fixed the rare one and left the common one, and the common one loses the entire store rather than one record. Now synced when the record creates the file, with the reasoning at the call site so the next reader does not have to rediscover which path fires when. Also taken: the empty-parent case, matching the guard the signing path already applies, and the Windows wording, which said the guarantee was covered by the rename primitive when it is simply absent there. Not taken here, deliberately: the lock-versus-rename inode race. It is pre-existing, this change does not touch it, and it is a larger hole in the same guard than anything fixed here. It is filed and should be the next change rather than a rider on this one. |
1 participant
Summary
Both nonce stores made their contents durable and not the name pointing at them. Syncing a temp file and renaming it leaves the rename itself unflushed, so a power loss can put the previous file back and silently undo the write.
For a nonce store that undo is the failure the store exists to prevent. A commitment claimed just before the loss reads as unclaimed on the next boot, the device signs a second time under the same nonce, and two shares over one nonce with different challenges recover the signer's key share. An earlier change closed the case where the contents never reached the disk; this closes the case where they did and the directory entry did not.
Three paths, not one. The rewrite path in each store, and the append path that creates the store on its first use. That last one is the one that matters: the rewrite in the network store only runs once a hundred thousand entries have accumulated, while the first record 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. Hardening the rare path and leaving the first-use path was the wrong half.
The helper already existed for key rotation and is now exported rather than copied a third and fourth time, since a duplicated durability primitive is how one copy ends up subtly different. Folded into that: it is a single documented function instead of a cfg pair, because with the crate denying missing docs and the item now publicly reachable, the undocumented Windows arm would have failed to compile there. Pull-request CI never builds Windows, so that would have landed on main and surfaced in a release build. It also now handles a single-component relative path, where
parent()yields an empty string rather than nothing and opening it fails, matching a guard the signing path already applies to the same case.The Windows behaviour is stated honestly as absent rather than provided elsewhere: a directory handle cannot be opened there through the standard library, and the rename primitive orders its metadata updates without promising they have reached the disk when it returns.
Test plan
The first version of this description said no test could discriminate, on the grounds that crash durability needs fault injection. That was wrong, and the correction is the useful part: the crash is untestable, but the property that matters is not. The caller signs on a successful claim, so a directory sync that fails has to fail the claim rather than be swallowed. Making the store directory writable and searchable but not readable produces exactly that, with nothing simulated, because opening the directory to sync it is what breaks.
Falsified: removing the sync makes that test fail with the message it was written for, and restoring it passes. The test skips itself if the mode does not actually take, since as root it would otherwise assert into a setup that never applied and pass regardless of the code.
All three crates' suites pass, the workspace builds, formatter and clippy clean.
Filed rather than fixed here
The larger hole in the same guard, found while reviewing this: the lock protecting the store is taken on the file that the rename then unlinks, so a second caller can hold a lock on a doomed inode, read the pre-claim state, and hand out the same commitment. It is pre-existing, unaffected by this change, and deserves its own change. Also filed: five further rename sites elsewhere with the same missing sync, and a credential writer that syncs but discards the error while documenting that it must not.
Summary by CodeRabbit