PM-22038: Improve Wallet Seed, Key Pair, and Address Code Quality#1217
Open
PM-22038: Improve Wallet Seed, Key Pair, and Address Code Quality#1217
Conversation
Improve Wallet Seed, Key Pair, and Address Code Quality Made-with: Cursor
Address Least Authority audit Suggestion 3 (A2): - WalletSeed: remove Copy (required for ZeroizeOnDrop), add Zeroize+ZeroizeOnDrop, implement redacted Debug that prints WalletSeed::<variant>(***) instead of raw bytes. Keep Clone (needed for HashMap dual-ownership pattern) and Hash/PartialEq/Eq (required for HashMap<WalletSeed, Wallet<D>> key). - Keypair: remove Clone (zero callers — pure parsing wrapper). - MaintenanceUpdateBuilder::add_addresses: accept &[MaintenanceCounter] and use zip iterator instead of unchecked index loop, eliminating potential index out-of-bounds panic. - WalletSeed::try_from_lazy_hex: pre-validate hex string length (max 128 hex chars) before calling hex::decode, preventing memory allocation from oversized untrusted input. - Fix all Copy-removal compilation errors across ledger/helpers and util/toolkit by adding explicit .clone() at ownership transfer points. - Add unit tests for redacted Debug output, hex length validation, zip truncation safety, and HashMap key compatibility. Made-with: Cursor
WalletSeed no longer derives Copy (removed for security hardening). Two test helpers and one genesis utility need explicit .clone() to construct owned values from borrows. Made-with: Cursor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Harden
WalletSeed,KeyPair, and address-related types in the Midnight node ledger by implementing redacted Debug, removing Copy (required for ZeroizeOnDrop), adding Zeroize/ZeroizeOnDrop for automatic secret material cleanup, removing unused Clone from Keypair, replacing index-based panics inadd_addresseswith zip iteration, and enforcing input-size validation intry_from_lazy_hex— addressing Least Authority audit Suggestion 3 (A2).🎫 Ticket 📐 Engineering
Motivation
The Least Authority Node DIFF audit identified informational findings in
ledger/helpers/src/versions/common/types.rswhere cryptographic types derive traits that expose secret material.WalletSeedderivesDebug,Clone,Copy,Hash,PartialEq, andEq, enabling accidental logging of seed bytes, invisible copies via Copy, and non-constant-time comparisons.KeyPairderivesClone, encouraging copies of secret key material. Address generation code uses unchecked indexing that can panic, andtry_from_lazy_hexallocates memory from untrusted input before validating size.Key technical insight:
ZeroizeOnDropgenerates aDropimpl, and Rust prohibitsCopy + Drop. Therefore Copy removal is a technical requirement for zeroize support, not merely a style choice.Hash/PartialEq/Eqmust remain becauseWalletSeedserves as aHashMapkey inLedgerContext.Changes
Copy; addedZeroize+ZeroizeOnDropvia thezeroizecrate; implemented redactedfmt::Debug(printsWalletSeed(Medium(***))instead of raw bytes)Clonederivation (zero callers — pure parsing wrapper)add_addressesto usezipwith&[MaintenanceCounter], eliminating unchecked indexing panicstry_from_lazy_hexbeforehex::decodeallocationutil/toolkitupdated to use explicit.clone()whereWalletSeedpreviously relied on implicitCopy📌 Submission Checklist
🔱 Fork Strategy
🗹 TODO before merging