wallet: Ask for the record fingerprint before the cards - #104
BenWestgate wants to merge 9 commits into
Conversation
aa2d333 to
9f88b21
Compare
When RIPEMD-160 is unavailable, a valid standard Bails identifier cannot be checked. Preserve the Bails-alpha SHA-256 result and distinguish that inconclusive state from a completed identifier mismatch, so the no-record restore prompt does not claim the cards are wrong. Keep the independent fingerprint and explicit operator-confirmation boundary unchanged. Refs #79
The no-record restore flow can no longer claim a standard Bails identifier mismatch when RIPEMD-160 is unavailable. Record that platform-dependent inconclusive outcome in both the security model and invariant so reviewers can distinguish it from a completed comparison. Refs #79
An existing hex seed or codex32 master secret previously reached the wallet-record fingerprint check only after new recovery cards had been generated and confirmed. Check the typed record immediately after parsing the source, before any card output or ceremony. Preserve the explicit recordless path at the same early decision point, and pass the checked result through to wallet initialization so it is not prompted twice. Cover matching, mismatching, and recordless flows for both source encodings. Refs #30.
Translate Ctrl-C or EOF at the early wallet-record gate for ms32 create --existing into the existing wallet-setup interruption path. This keeps an operator from being told to invalidate a pre-existing recovery card before any new share ceremony has started. Add a focused regression proving the interruption occurs before share creation or output and preserves the valid-backup message.
Reassign the expected_fingerprint argument instead of copying it into a local, and give existing_secret its None default before the source checks instead of in an else branch. Behavior is unchanged. The installed package drops from 5161 to 5159 logical review lines, which keeps the integrated #7/#42/#57/#46/#80/#81 tip under the <5200 budget. Security: the record gate still runs before any card is generated or shown, and interrupts at that gate still raise _WalletSetupInterrupted. Validation: ruff check, ruff format --check, mypy src/codex32, and pytest (918 passed, with and without -O). Refs #81, #38. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018az69UX4773mYohXAtE8kD
A raw seed imported with create --existing was assigned a temporary random identifier for the no-record safety screen, then assigned a different random identifier when the new share set was created. Reuse the first identifier as the share-set identifier so the safety screen describes the backup that will actually be produced.\n\nExtend the recordless-creation regression to require the displayed, emitted, and imported identifiers to agree.\n\nRefs #30
9f88b21 to
1d5b6f5
Compare
Ben authorized raising the budget so #91 fits. The stack tip with the open fix PRs was at 5,197 of 5,200, and #91 adds 26 lines. Update the enforcing test and both places that document the number. Claude-Session: https://claude.ai/code/session_015CuLXqAvAovfoVcUmmogwa
`ms32 wallet` and `ms32 create --existing` hid the recovered master fingerprint while confirming a correction (#57), so a wrong correction was caught only after the operator accepted it and typed the record. Ask for the record first: before the shares in `ms32 wallet` and before the seed in `ms32 create --existing`. A correction that completes the secret then says whether it matches the record, without showing the fingerprint, and the record picks between equally likely corrections. The final identity check, the retry on mismatch and the Enter path for no record work as before; without a record nothing is shown until the recordless gate. Ctrl-C at the moved prompt still says the existing cards are valid. Closes #91 Claude-Session: https://claude.ai/code/session_015CuLXqAvAovfoVcUmmogwa
c785bf1 to
e8b84c5
Compare
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
AI-generated review (Codex), posted at the maintainer's request.
Not ACKing e8b84c5 yet: this raises the installed-code budget from 5,200 to 5,250 without explicit authorization. The record-before-cards/fingerprint-ranking change otherwise looks sound.
BenWestgate
added
area: cli
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
AI-generated focused security review (Codex), posted at the maintainer's request.
Security ACK e8b84c593ef2 for the recovery-identity change itself.
The typed wallet-record fingerprint is collected before recovery input. Correction preview uses a provisional recovered MasterSeed only to compare against that record; with a record present it prints only “matches” / “does not match” and never the recovered fingerprint value. The record matcher is applied only after capture-volume, Hamming-weight, and CRC-padding ranking, so it can narrow an otherwise tied correction set but cannot promote a less likely candidate.
The final restore path still calls verify_identity() through _recorded_fingerprint() before wallet initialization, and the tested mismatch/no-record paths preserve the existing verify-before-mutation contract. The new regressions also cover a corrected final share matching the typed record, record-before-shares ordering, retry on mismatch, recordless disclosure, and interruption before sharing.
No security blocker found in the recovery delta.
This is not an overall ACK for integration: the head still raises the installed-package budget from <5200 to <5250, and that change already has an unresolved current-head review NACK under the release checklist. Keep the PR draft until the cap/base issue is settled; human review/authorship policy still applies.
|
V1 disposition (Codex, 2026-10-04): defer this post-audit enhancement from the v1 candidate under the existing The recovery behavior itself has a current-head focused security ACK, but the feature takes the #81-based tree to 5,217 production logical lines. The original adversarial audit does not require the record-before-cards enhancement; its material restore-authentication finding is already addressed by the reviewed #57 → #105 → #80 → #81 → #95 stack and the GUI replay in #118. Keep this PR open/draft as post-v1 work. Do not weaken the v1 size gate or compress the security-sensitive recovery path just to land it. A maintainer can later choose a larger post-v1 budget or refactor with fresh review. Human authorship/review remains required whenever it is integrated. |
BenWestgate
removed
the
gate: adversarial review
1d5b6f5 to
b2aafde
Compare
This branch has not been deployed
2 participants
Requested by Ben · project thread
Before:
ms32 walletandms32 create --existinghid the recovered master fingerprint while a correction was being confirmed (#57). The wallet record was typed only after the cards, so a wrong correction was caught after the operator had accepted it.After: both commands ask for the record fingerprint first, before the cards in
walletand before the secret increate --existing. A suggested correction that completes the secret says “Master fingerprint matches your wallet record” or “does not match”, without showing the value, and the record picks between otherwise equally ranked corrections. The final identity check, re-ask on mismatch and explicit no-record route remain in place.How:
cli._recordasks for the record before input._cli_input._completedprovisionally recovers the last-neededmsshare only for fingerprint comparison._fingerprint_matcherapplies the record after capture-volume, Hamming-weight and CRC-padding ranking._recorded_fingerprintstill callsverify_identity()before wallet initialization, and the no-record route is the only path that discloses the recovered fingerprint.Security review: Codex ACKed current head
e8b84c5on 2026-10-04. The focused review verified that correction preview/ranking never exposes the typed/recovered fingerprint value, the record cannot promote a worse-ranked candidate, and the final restore path preserves verify-before-wallet-mutation. Regressions cover corrected final-share matching, record-before-shares ordering, retry on mismatch, no-record disclosure and interruption before sharing.Release blocker: this head also raises the installed-package budget from
<5200to<5250and measures 5,217 production lines on #81. The central release checklist still uses<5200; the prior current-head review NACK on that budget increase remains unresolved. The security ACK does not clear this policy blocker. Do not compress the recovery path merely to save lines; this enhancement is deferred from the v1 candidate as stated above.Conflicts with #101 in two
_confirm_correctioncalls; if both are integrated post-v1, preserve both the invalid-reason and record arguments.Closes #91
🤖 Generated with Claude Code
https://claude.ai/code/session_015CuLXqAvAovfoVcUmmogwa
Generated by Claude Code