Skip to content

accounts/usbwallet: trezor signed message support #19831

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 5 commits into from
Closed

accounts/usbwallet: trezor signed message support #19831

wants to merge 5 commits into from

Conversation

gballet
Copy link
Member

@gballet gballet commented Jul 12, 2019 •
edited
Loading

Implements signData for trezor. This has been tested with both the pre-1.8 version of the firmware and the latest. Fixes #19812

To work, this PR needs #19827

@gballet gballet requested review from karalabe and holiman July 12, 2019 06:58
@gballet gballet force-pushed the clef-signdata-with-trezor branch from 3e2094c to 9c9734e Compare July 12, 2019 07:04
karalabe
karalabe requested changes Jul 17, 2019
View reviewed changes
accounts/usbwallet/ledger.go Outdated
@@ -168,6 +168,12 @@ func (w *ledgerDriver) SignTx(path accounts.DerivationPath, tx *types.Transactio
return w.ledgerSign(path, tx, chainID)
}

// SignData sends a blob of data to the USB device and waits for the user to confirm
// or deny the transaction.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

transaction -> signing request.

accounts/usbwallet/trezor.go Outdated
@@ -185,6 +185,29 @@ func (w *trezorDriver) SignTx(path accounts.DerivationPath, tx *types.Transactio
return w.trezorSign(path, tx, chainID)
}

// SignData sends a blob of data to the xUSB device and waits for the user to confirm
// or deny the transaction.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

transaction -> signing request.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

xUSB -> USB?

accounts/usbwallet/wallet.go Outdated
@@ -67,6 +67,10 @@ type driver interface {
// SignTx sends the transaction to the USB device and waits for the user to confirm
// or deny the transaction.
SignTx(path accounts.DerivationPath, tx *types.Transaction, chainID *big.Int) (common.Address, *types.Transaction, error)

// SignData sends a blob of data to the USB device and waits for the user to confirm
// or deny the transaction.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

transaction -> signing request.

accounts/usbwallet/wallet.go

// SignData sends a blob of data to the USB device and waits for the user to confirm
// or deny the transaction.
SignData(path accounts.DerivationPath, hash []byte) (common.Address, []byte, error)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm unsure about passing a hash vs. the data itself. Won't some hardware wallet or Clef itself need the entire data?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually the message itself is passed, I'm going to change the variable name.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Parameter name is still hash

signer/core/signed_data.go Outdated
@@ -144,7 +144,7 @@ func (api *SignerAPI) sign(addr common.MixedcaseAddress, req *SignDataRequest, l
}
pw, err := api.lookupOrQueryPassword(account.Address,
"Password for signing",
fmt.Sprintf("Please enter password for signing data with account %s", account.Address.Hex()))
fmt.Sprintf("Please enter password for signing data with account %s, or just press 'RETURN'", account.Address.Hex()))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit ugly :P. A cleaner solution perhaps would be to look at the account.URL and if it's a keystore, ask for password, for everything else just blindly forward?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quoting you (from memory) when you asked me to look into the Trezor stuff: "I would like a quick and ugly solution". Boom, delivered.

karalabe
karalabe reviewed Jul 18, 2019
View reviewed changes
accounts/usbwallet/wallet.go
@@ -514,7 +518,44 @@ func (w *wallet) SelfDerive(bases []accounts.DerivationPath, chain ethereum.Chai
// signHash implements accounts.Wallet, however signing arbitrary data is not
// supported for hardware wallets, so this method will always return an error.
func (w *wallet) signHash(account accounts.Account, hash []byte) ([]byte, error) {
return nil, accounts.ErrNotSupported
w.stateLock.RLock() // Comms have their own mutex
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this method should be renamed signData? Also please rename the hash variables everywhere to data.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please read the comment and update it too accordingly :P

@karalabe karalabe added this to the 1.9.1 milestone Jul 18, 2019
@gballet gballet force-pushed the clef-signdata-with-trezor branch from 7bcd24e to 407128b Compare July 18, 2019 14:36
karalabe
karalabe reviewed Jul 18, 2019
View reviewed changes
accounts/usbwallet/wallet.go Outdated
@@ -518,7 +517,7 @@ func (w *wallet) SelfDerive(bases []accounts.DerivationPath, chain ethereum.Chai

// signHash implements accounts.Wallet, however signing arbitrary data is not
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:P

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

grmbl

karalabe
karalabe reviewed Jul 19, 2019
View reviewed changes
accounts/usbwallet/ledger.go Outdated
@@ -168,6 +168,12 @@ func (w *ledgerDriver) SignTx(path accounts.DerivationPath, tx *types.Transactio
return w.ledgerSign(path, tx, chainID)
}

// SignData sends a blob of data to the USB device and waits for the user to confirm
// or deny the signing request.
func (w *ledgerDriver) SignData(path accounts.DerivationPath, hash []byte) (common.Address, []byte, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Parameter name is still hash

karalabe
karalabe reviewed Jul 19, 2019
View reviewed changes
accounts/usbwallet/trezor.go
if addr := response.GetAddressHex(); len(addr) > 0 { // Newer firmwares use hexadecimal fomats
address = common.HexToAddress(addr)
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pls drop empty line.

karalabe
karalabe reviewed Jul 19, 2019
View reviewed changes
accounts/usbwallet/wallet.go
@@ -531,7 +571,7 @@ func (w *wallet) SignDataWithPassphrase(account accounts.Account, passphrase, mi
}

func (w *wallet) SignText(account accounts.Account, text []byte) ([]byte, error) {
return w.signHash(account, accounts.TextHash(text))
return w.signData(account, accounts.TextHash(text))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be wrong now. If we changed the signature to sign data, then we can't hash the text here now. What does sign text even do? It's not my method. Ping @holiman ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's an extra catch here that I've spotted yesterday during a debug session: the Trezor actually adds the prefix no matter what, so right now the prefix is added twice. So the whole prefix management has to be changed, I'm still untangling some bits, and it would be interesting to discuss it.

karalabe
karalabe reviewed Jul 19, 2019
View reviewed changes
signer/core/signed_data.go Outdated
return nil, err
// Only ask for the passphrase if the account is keystore based
pw := ""
if strings.Compare(wallet.URL().Scheme, "keystore") == 0 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wallet.URL().Scheme == "keystore"

@karalabe
Copy link
Member

Putting this into the 1.9.x backlog. We need Trezor firmware support before we can continue (trezor/trezor-firmware#148).

@karalabe karalabe modified the milestones: 1.9.1, 1.9.x Jul 22, 2019
@karalabe karalabe removed this from the 1.9.x milestone Nov 12, 2019
@lexuansang93
Copy link

1

@karalabe
Copy link
Member

Closing this as it's not supported by Trezor. We can only pick it up if they implement it upstream. Closing until then.

@karalabe karalabe closed this Jun 25, 2020
@holiman
Copy link
Contributor

holiman commented Jun 25, 2020

reference: trezor/trezor-firmware#148

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@karalabe karalabe karalabe requested changes

@holiman holiman Awaiting requested review from holiman

Assignees
No one assigned
Labels
status:on-hold
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

cmd/clef: support trezor signing
5 participants
@gballet @karalabe @lexuansang93 @holiman @adamschmideg