accounts: ledger and HD review fixes

- Handle a data race where a Ledger drops between list and open
- Prolong Ledger tx confirmation window to 30 days from 1 minute
- Simplify Ledger chainid-signature calculation and validation
- Simplify Ledger USB APDU request chunking algorithm
- Silence keystore account cache notifications for manual actions
- Only enable self derivations if wallet open succeeds
This commit is contained in:
Péter Szilágyi 2017-02-11 17:02:00 +02:00
parent c7022c1a0c
commit e99c788155
No known key found for this signature in database
GPG Key ID: E9AE538CEDF8293D
5 changed files with 39 additions and 60 deletions

View File

@ -113,11 +113,6 @@ func (ac *accountCache) add(newAccount accounts.Account) {
copy(ac.all[i+1:], ac.all[i:]) copy(ac.all[i+1:], ac.all[i:])
ac.all[i] = newAccount ac.all[i] = newAccount
ac.byAddr[newAccount.Address] = append(ac.byAddr[newAccount.Address], newAccount) ac.byAddr[newAccount.Address] = append(ac.byAddr[newAccount.Address], newAccount)
select {
case ac.notify <- struct{}{}:
default:
}
} }
// note: removed needs to be unique here (i.e. both File and Address must be set). // note: removed needs to be unique here (i.e. both File and Address must be set).
@ -131,10 +126,6 @@ func (ac *accountCache) delete(removed accounts.Account) {
} else { } else {
ac.byAddr[removed.Address] = ba ac.byAddr[removed.Address] = ba
} }
select {
case ac.notify <- struct{}{}:
default:
}
} }
func removeAccount(slice []accounts.Account, elem accounts.Account) []accounts.Account { func removeAccount(slice []accounts.Account, elem accounts.Account) []accounts.Account {

View File

@ -140,7 +140,7 @@ func TestCacheInitialReload(t *testing.T) {
} }
func TestCacheAddDeleteOrder(t *testing.T) { func TestCacheAddDeleteOrder(t *testing.T) {
cache, notify := newAccountCache("testdata/no-such-dir") cache, _ := newAccountCache("testdata/no-such-dir")
cache.watcher.running = true // prevent unexpected reloads cache.watcher.running = true // prevent unexpected reloads
accs := []accounts.Account{ accs := []accounts.Account{
@ -176,20 +176,10 @@ func TestCacheAddDeleteOrder(t *testing.T) {
for _, a := range accs { for _, a := range accs {
cache.add(a) cache.add(a)
} }
select {
case <-notify:
default:
t.Fatalf("notifications didn't fire for adding new accounts")
}
// Add some of them twice to check that they don't get reinserted. // Add some of them twice to check that they don't get reinserted.
cache.add(accs[0]) cache.add(accs[0])
cache.add(accs[2]) cache.add(accs[2])
select {
case <-notify:
t.Fatalf("notifications fired for adding existing accounts")
default:
}
// Check that the account list is sorted by filename. // Check that the account list is sorted by filename.
wantAccounts := make([]accounts.Account, len(accs)) wantAccounts := make([]accounts.Account, len(accs))
copy(wantAccounts, accs) copy(wantAccounts, accs)
@ -213,11 +203,6 @@ func TestCacheAddDeleteOrder(t *testing.T) {
} }
cache.delete(accounts.Account{Address: common.HexToAddress("fd9bd350f08ee3c0c19b85a8e16114a11a60aa4e"), URL: accounts.URL{Scheme: KeyStoreScheme, Path: "something"}}) cache.delete(accounts.Account{Address: common.HexToAddress("fd9bd350f08ee3c0c19b85a8e16114a11a60aa4e"), URL: accounts.URL{Scheme: KeyStoreScheme, Path: "something"}})
select {
case <-notify:
default:
t.Fatalf("notifications didn't fire for deleting accounts")
}
// Check content again after deletion. // Check content again after deletion.
wantAccountsAfterDelete := []accounts.Account{ wantAccountsAfterDelete := []accounts.Account{
wantAccounts[1], wantAccounts[1],

View File

@ -286,7 +286,7 @@ func TestWalletNotifications(t *testing.T) {
// Randomly add and remove account and make sure events and wallets are in sync // Randomly add and remove account and make sure events and wallets are in sync
live := make(map[common.Address]accounts.Account) live := make(map[common.Address]accounts.Account)
for i := 0; i < 256; i++ { for i := 0; i < 1024; i++ {
// Execute a creation or deletion and ensure event arrival // Execute a creation or deletion and ensure event arrival
if create := len(live) == 0 || rand.Int()%4 > 0; create { if create := len(live) == 0 || rand.Int()%4 > 0; create {
// Add a new account and ensure wallet notifications arrives // Add a new account and ensure wallet notifications arrives

View File

@ -192,6 +192,9 @@ func (w *ledgerWallet) Open(passphrase string) error {
if err != nil { if err != nil {
return err return err
} }
if len(devices) == 0 {
return accounts.ErrUnknownWallet
}
// Device opened, attach to the input and output endpoints // Device opened, attach to the input and output endpoints
device := devices[0] device := devices[0]
@ -767,7 +770,7 @@ func (w *ledgerWallet) ledgerDerive(derivationPath []uint32) (common.Address, er
func (w *ledgerWallet) ledgerSign(derivationPath []uint32, address common.Address, tx *types.Transaction, chainID *big.Int) (*types.Transaction, error) { func (w *ledgerWallet) ledgerSign(derivationPath []uint32, address common.Address, tx *types.Transaction, chainID *big.Int) (*types.Transaction, error) {
// We need to modify the timeouts to account for user feedback // We need to modify the timeouts to account for user feedback
defer func(old time.Duration) { w.device.ReadTimeout = old }(w.device.ReadTimeout) defer func(old time.Duration) { w.device.ReadTimeout = old }(w.device.ReadTimeout)
w.device.ReadTimeout = time.Minute w.device.ReadTimeout = time.Hour * 24 * 30 // Timeout requires a Ledger power cycle, only if you must
// Flatten the derivation path into the Ledger request // Flatten the derivation path into the Ledger request
path := make([]byte, 1+4*len(derivationPath)) path := make([]byte, 1+4*len(derivationPath))
@ -823,7 +826,7 @@ func (w *ledgerWallet) ledgerSign(derivationPath []uint32, address common.Addres
signer = new(types.HomesteadSigner) signer = new(types.HomesteadSigner)
} else { } else {
signer = types.NewEIP155Signer(chainID) signer = types.NewEIP155Signer(chainID)
signature[64] = (signature[64]-34)/2 - byte(chainID.Uint64()) signature[64] = signature[64] - byte(chainID.Uint64()*2+35)
} }
// Inject the final signature into the transaction and sanity check the sender // Inject the final signature into the transaction and sanity check the sender
signed, err := tx.WithSignature(signer, signature) signed, err := tx.WithSignature(signer, signature)
@ -875,45 +878,42 @@ func (w *ledgerWallet) ledgerSign(derivationPath []uint32, address common.Addres
// Optional APDU data | arbitrary // Optional APDU data | arbitrary
func (w *ledgerWallet) ledgerExchange(opcode ledgerOpcode, p1 ledgerParam1, p2 ledgerParam2, data []byte) ([]byte, error) { func (w *ledgerWallet) ledgerExchange(opcode ledgerOpcode, p1 ledgerParam1, p2 ledgerParam2, data []byte) ([]byte, error) {
// Construct the message payload, possibly split into multiple chunks // Construct the message payload, possibly split into multiple chunks
var chunks [][]byte apdu := make([]byte, 2, 7+len(data))
for left := data; len(left) > 0 || len(chunks) == 0; {
// Create the chunk header binary.BigEndian.PutUint16(apdu, uint16(5+len(data)))
var chunk []byte apdu = append(apdu, []byte{0xe0, byte(opcode), byte(p1), byte(p2), byte(len(data))}...)
apdu = append(apdu, data...)
if len(chunks) == 0 {
// The first chunk encodes the length and all the opcodes
chunk = []byte{0x00, 0x00, 0xe0, byte(opcode), byte(p1), byte(p2), byte(len(data))}
binary.BigEndian.PutUint16(chunk, uint16(5+len(data)))
}
// Append the data blob to the end of the chunk
space := 64 - len(chunk) - 5 // 5 == header size
if len(left) > space {
chunks, left = append(chunks, append(chunk, left[:space]...)), left[space:]
continue
}
chunks, left = append(chunks, append(chunk, left...)), nil
}
// Stream all the chunks to the device // Stream all the chunks to the device
for i, chunk := range chunks { header := []byte{0x01, 0x01, 0x05, 0x00, 0x00} // Channel ID and command tag appended
chunk := make([]byte, 64)
space := len(chunk) - len(header)
for i := 0; len(apdu) > 0; i++ {
// Construct the new message to stream // Construct the new message to stream
header := []byte{0x01, 0x01, 0x05, 0x00, 0x00} // Channel ID and command tag appended chunk = append(chunk[:0], header...)
binary.BigEndian.PutUint16(header[3:], uint16(i)) binary.BigEndian.PutUint16(chunk[3:], uint16(i))
msg := append(header, chunk...)
if len(apdu) > space {
chunk = append(chunk, apdu[:space]...)
apdu = apdu[space:]
} else {
chunk = append(chunk, apdu...)
apdu = nil
}
// Send over to the device // Send over to the device
if glog.V(logger.Detail) { if glog.V(logger.Detail) {
glog.Infof("-> %03d.%03d: %x", w.device.Bus, w.device.Address, msg) glog.Infof("-> %03d.%03d: %x", w.device.Bus, w.device.Address, chunk)
} }
if _, err := w.input.Write(msg); err != nil { if _, err := w.input.Write(chunk); err != nil {
return nil, err return nil, err
} }
} }
// Stream the reply back from the wallet in 64 byte chunks // Stream the reply back from the wallet in 64 byte chunks
var reply []byte var reply []byte
chunk = chunk[:64] // Yeah, we surely have enough space
for { for {
// Read the next chunk from the Ledger wallet // Read the next chunk from the Ledger wallet
chunk := make([]byte, 64)
if _, err := io.ReadFull(w.output, chunk); err != nil { if _, err := io.ReadFull(w.output, chunk); err != nil {
return nil, err return nil, err
} }
@ -925,17 +925,19 @@ func (w *ledgerWallet) ledgerExchange(opcode ledgerOpcode, p1 ledgerParam1, p2 l
return nil, errReplyInvalidHeader return nil, errReplyInvalidHeader
} }
// If it's the first chunk, retrieve the total message length // If it's the first chunk, retrieve the total message length
var payload []byte
if chunk[3] == 0x00 && chunk[4] == 0x00 { if chunk[3] == 0x00 && chunk[4] == 0x00 {
reply = make([]byte, 0, int(binary.BigEndian.Uint16(chunk[5:7]))) reply = make([]byte, 0, int(binary.BigEndian.Uint16(chunk[5:7])))
chunk = chunk[7:] payload = chunk[7:]
} else { } else {
chunk = chunk[5:] payload = chunk[5:]
} }
// Append to the reply and stop when filled up // Append to the reply and stop when filled up
if left := cap(reply) - len(reply); left > len(chunk) { if left := cap(reply) - len(reply); left > len(payload) {
reply = append(reply, chunk...) reply = append(reply, payload...)
} else { } else {
reply = append(reply, chunk[:left]...) reply = append(reply, payload[:left]...)
break break
} }
} }

View File

@ -273,8 +273,9 @@ func startNode(ctx *cli.Context, stack *node.Node) {
for _, wallet := range stack.AccountManager().Wallets() { for _, wallet := range stack.AccountManager().Wallets() {
if err := wallet.Open(""); err != nil { if err := wallet.Open(""); err != nil {
glog.V(logger.Warn).Infof("Failed to open wallet %s: %v", wallet.URL(), err) glog.V(logger.Warn).Infof("Failed to open wallet %s: %v", wallet.URL(), err)
} else {
wallet.SelfDerive(accounts.DefaultBaseDerivationPath, stateReader)
} }
wallet.SelfDerive(accounts.DefaultBaseDerivationPath, stateReader)
} }
// Listen for wallet event till termination // Listen for wallet event till termination
for event := range events { for event := range events {
@ -283,8 +284,8 @@ func startNode(ctx *cli.Context, stack *node.Node) {
glog.V(logger.Info).Infof("New wallet appeared: %s, failed to open: %s", event.Wallet.URL(), err) glog.V(logger.Info).Infof("New wallet appeared: %s, failed to open: %s", event.Wallet.URL(), err)
} else { } else {
glog.V(logger.Info).Infof("New wallet appeared: %s, %s", event.Wallet.URL(), event.Wallet.Status()) glog.V(logger.Info).Infof("New wallet appeared: %s, %s", event.Wallet.URL(), event.Wallet.Status())
event.Wallet.SelfDerive(accounts.DefaultBaseDerivationPath, stateReader)
} }
event.Wallet.SelfDerive(accounts.DefaultBaseDerivationPath, stateReader)
} else { } else {
glog.V(logger.Info).Infof("Old wallet dropped: %s", event.Wallet.URL()) glog.V(logger.Info).Infof("Old wallet dropped: %s", event.Wallet.URL())
event.Wallet.Close() event.Wallet.Close()