Add Keycard support#451
Add Keycard support#451jonesmarvin8 wants to merge 28 commits intomarvin/refactor-wallet-pub-accfrom
Conversation
jonesmarvin8
changed the base branch from
main
to
marvin/refactor-wallet-pub-acc
| let current_dir = env::current_dir().expect("Failed to get current working directory"); | ||
|
|
||
| let paths_to_add: Vec<PathBuf> = vec
There was a problem hiding this comment.
two issues in the python integration:
current_dir().join("python") puts cwd-relative python/ at sys.path[0]. after cargo install --path wallet --force the binary is global, so running it from any directory containing a malicious python/keycard_wallet.py executes that file in-process with PIN and mnemonic access.
wallet_with_keycard.sh:14 does git clone --branch lee-schnorr of bitgamma/keycard-py unpinned. whoever pushes there controls signing for every install.
both need fixing before this lands. resolve the path from current_exe(), and either vendor keycard-py properly or pin a commit hash and verify in CI.
|
|
||
| import keycard | ||
|
|
||
| PIN = '123456' |
There was a problem hiding this comment.
let's drop these. setup_communication(pin=PIN, …) and load_mnemonic(mnemonic=DEFAULT_MNEMONIC, …) use them as defaults, so a future caller forgetting an arg silently signs with 123456 or loads "fashion degree mountain...". if test fixtures need a known mnemonic, move it to a test-only file.
| #[arg(short, long)] | ||
| mnemonic: Option<String>, | ||
| #[arg(short, long)] | ||
| pin: Option<String>, |
There was a problem hiding this comment.
--pin on argv leaks to shell history, ps, xtrace, and stays in heap unzeroed. for a hardware key the PIN is the whole moat once the card is taken. let's read it from a TTY prompt (rpassword) or env var, and store it in Zeroizing<String>. same applies to the other subcommands taking pin: Option<String> (programs/native_token_transfer.rs:40,73, pinata.rs:31, account.rs:42, token.rs:80,111).
| .call_method1("get_public_key_for_path", (path,))? | ||
| .extract()?; | ||
|
|
||
| let public_key: [u8; 32] = public_key.try_into().expect("Expect 32 bytes"); |
There was a problem hiding this comment.
wrong PIN, removed card, secure-channel timeout, all the everyday failure modes hit .expect() and panic the tokio worker. these aren't invariants. let's propagate PyResult (or a typed KeycardError) up to the CLI handler and reserve expect for things that can't fail.
(also Expect a valid public key1 / key2 look like leftover debug.)
same applies to the matching expects in program_facades/native_token_transfer/public.rs:60,72,113,118,122.
| let mut nonces = self | ||
| .0 | ||
| .get_accounts_nonces(account_ids.clone()) | ||
| .get_accounts_nonces(vec![from]) |
There was a problem hiding this comment.
silent semantic change unrelated to keycard. pre-PR fetched both [from, to] nonces unconditionally; now from only, with to extended only if its key is local. when to is foreign (the normal case), account_ids.len() == 2 but nonces.len() == 1 and Message::try_new(...).unwrap() either panics or builds a malformed tx.
let's revert to get_accounts_nonces(account_ids.clone()) and put any conditional-nonce logic in its own PR with a foreign-recipient test.
| )) | ||
| })?; | ||
|
|
||
| Ok(Signature { value: signature }) |
There was a problem hiding this comment.
the 64 bytes from the card go straight into Signature { value } with no rust-side verify. if keycard_wallet.py (or keycard-py) ever returns a wrong signature, the only signal is a sequencer rejection, nonce burned, message digest leaked.
cheap to add: VerifyingKey::from_bytes(pub_key.value())?.verify_prehash(message, &sig)? after sign, refuse on failure.
| } | ||
|
|
||
| #[must_use] | ||
| pub fn get_public_key_for_path_with_connect(pin: &str, path: &str) -> PublicKey { |
There was a problem hiding this comment.
each _with_connect does select/pair/open_secure_channel/verify_pin/.../unpair. with get_public_key_for_path_with_connect + sign_message_for_path_with_connect, that's two PIN verifications per send. the card locks after 3 bad PINs.
let's collapse to one session: connect → get_pubkey → sign → disconnect. or longer-term, hold pairing state across ops in a process-global mutex with keycard load setting it up once.
There was a problem hiding this comment.
Good point. I can collapse this into a single session. Thanks!
Ideally, I would hold pairing across ops state. Unfortunately, each wallet command in CLI is treated as its own instance. So, I cannot retain the python state in memory (the original approach I wanted to use). Storing the python class on disk seems problematic to me as a keycard may be removed between usage.
2 participants
🎯 Purpose
Integrates Keycard for public transactions for
auth-transfers.⚙️ Approach
auth-transferandpinatato use Keycard🧪 How to Test
Follow setup for Keycard before this.
In root:
wallet_with_keycard.shto install Wallet and the necessary python packages.keycard_tests.sh(in root)🔗 Dependencies
PR 452 (and PR 457) must be merged first.
🔜 Future Work
keycard-pyfrom being included in the repo; currently necessary due to updates tokeycard-pythat is incompatible with the firmware.📋 PR Completion Checklist
Mark only completed items. A complete PR should have all boxes ticked.