bitcoin-core / HWI

Bitcoin Hardware Wallet Interface
MIT License
493 stars 195 forks source link

Trezor: it should be possible to use an empty passphrase #639

Closed prusnak closed 1 year ago

prusnak commented 1 year ago

In case a user enters empty passphrase on Trezor, it should work and a passphrase-less wallet should be open (the same as on a Trezor with passphrase protection off).

It seems that HWI disallows empty passphrases leading to confusions such as:

moneymanolis commented 1 year ago

@achow101 - any update / input on this from your side?

achow101 commented 1 year ago

It's not clear to me what the actual issue is, can you provide a reproduction for HWI?

I don't see anything in HWI that prevents using the empty passphrase. It's not explicitly disallowed; it should just be passing the empty passphrase straight through to the device.

achow101 commented 1 year ago

Ah, I think I see the issue now. enumerate is expecting non-empty strings for the passphrase and thus returning an error. However the actual client itself can deal with empty passphrases. If you ignore enumerate and just execute a command with a device with the empty passphrase, it should work.

achow101 commented 1 year ago

@moneymanolis Can you test #644

moneymanolis commented 1 year ago

Will do asap. Thanks for responding so quickly with a PR.

moneymanolis commented 1 year ago

Initial testing results @achow101: On hwi master this does NOT work: ./hwi.py -t "trezor" -d "webusb:020:2:4" --password "" enumerate

Result is: [{"type": "trezor", "path": "webusb:020:2:4", "label": "temp", "model": "trezor_1", "needs_pin_sent": false, "needs_passphrase_sent": true, "error": "Could not open client or get fingerprint information: Passphrase needs to be specified before the fingerprint information can be retrieved", "code": -12}]

On https://github.com/bitcoin-core/HWI/pull/644 it works, result is sth. like: [{"type": "trezor", "path": "webusb:020:2:4", "label": "temp", "model": "trezor_1", "needs_pin_sent": false, "needs_passphrase_sent": false, "fingerprint": "59967cfd"}]

Update: But if I run ./hwi.py -t "trezor" -d "webusb:020:2:4" enumerate

I get [{"type": "trezor", "path": "webusb:020:2:4", "label": "temp", "model": "trezor_1", "needs_pin_sent": false, "needs_passphrase_sent": true, "error": "Could not open client or get fingerprint information: Passphrase needs to be specified before the fingerprint information can be retrieved", "code": -12}] on #644

Shouldn't that work?

Just as a sidenote if you send the pin and then go straight to signtx: master and #644 work. My expectation was, that I'd had to provide the (empty) passphrase. But okay.

I am working now on testing #644 in conjunction with Specter, too.

achow101 commented 1 year ago

But if I run ./hwi.py -t "trezor" -d "webusb:020:2:4" enumerate

I get [{"type": "trezor", "path": "webusb:020:2:4", "label": "temp", "model": "trezor_1", "needs_pin_sent": false, "needs_passphrase_sent": true, "error": "Could not open client or get fingerprint information: Passphrase needs to be specified before the fingerprint information can be retrieved", "code": -12}] on #644

Shouldn't that work?

Not necessarily. I think that's just an issue of what users expect. Do they expect it to work if passphrase protection is enabled but no passphrase is provided? Or should they expect to always need to provide a passphrase, even if it's the empty string?

I guess either way the behavior should be unified as it currently is not as you point out.

prusnak commented 1 year ago

@matejcik Can you please weigh in and tell us what is the behaviour that makes most sense (so it is aligned with how we do stuff in python-trezor/trezorctl/Suite)?

moneymanolis commented 1 year ago

Using those changes in Specter https://github.com/cryptoadvance/specter-desktop/pull/1977 and the PR from hwi (https://github.com/bitcoin-core/HWI/pull/644) works for me!

moneymanolis commented 1 year ago

We have a bunch of Specter Desktop users who at some point had the passphrase feature enabled but didn't use it in the sense that they used an empty string only. Technically, and the Trezor teams knows this much better, the part of the PBKDF2 function (the salt part) from BIP39 where the passphrase goes is never really empty. It is always filled with the string "mnemonic" and if there is a passphrase this string is concatenated with the passphrase. So, I'd say the passphrase can only be an empty string or a non-empty string.

matejcik commented 1 year ago

what is the behaviour that makes most sense (so it is aligned with how we do stuff in python-trezor/trezorctl/Suite)?

Right now there's actually a split between trezorctl and Suite. trezorctl does the easy thing and if passphrase is enabled, always asks for it. Suite enables passphrase by default even for people who never use the hidden wallet feature. By selecting the "default wallet", you auto-open the "" passphrase.

The Suite behavior is the more correct one UX-wise. So I believe that if the passphrase is not explicitly specified, HWI should default to the "" passphrase.