diff --git a/accounts/scwallet/hub.go b/accounts/scwallet/hub.go index 26aa6fc79..996de75eb 100644 --- a/accounts/scwallet/hub.go +++ b/accounts/scwallet/hub.go @@ -36,14 +36,12 @@ import ( "encoding/json" "io/ioutil" "os" - "reflect" "sync" "time" "github.com/ebfe/scard" "github.com/ethereum/go-ethereum/accounts" "github.com/ethereum/go-ethereum/common" - "github.com/ethereum/go-ethereum/common/hexutil" "github.com/ethereum/go-ethereum/event" "github.com/ethereum/go-ethereum/log" ) @@ -71,9 +69,10 @@ type smartcardPairing struct { type Hub struct { scheme string // Protocol scheme prefixing account and wallet URLs. - context *scard.Context - datadir string - pairings map[string]smartcardPairing + context *scard.Context + datadir string + pairings map[string]smartcardPairing + refreshed time.Time // Time instance when the list of wallets was last refreshed wallets map[string]*Wallet // Mapping from reader names to wallet instances updateFeed event.Feed // Event feed to notify wallet additions/removals @@ -82,11 +81,9 @@ type Hub struct { quit chan chan error - stateLock sync.Mutex // Protects the internals of the hub from racey access + stateLock sync.RWMutex // Protects the internals of the hub from racey access } -var HubType = reflect.TypeOf(&Hub{}) - func (hub *Hub) readPairings() error { hub.pairings = make(map[string]smartcardPairing) pairingFile, err := os.Open(hub.datadir + "/smartcards.json") @@ -158,7 +155,6 @@ func NewHub(scheme string, datadir string) (*Hub, error) { if err != nil { return nil, err } - hub := &Hub{ scheme: scheme, context: context, @@ -166,28 +162,31 @@ func NewHub(scheme string, datadir string) (*Hub, error) { wallets: make(map[string]*Wallet), quit: make(chan chan error), } - if err := hub.readPairings(); err != nil { return nil, err } - hub.refreshWallets() return hub, nil } -// Wallets implements accounts.Backend, returning all the currently tracked -// devices that appear to be hardware wallets. +// Wallets implements accounts.Backend, returning all the currently tracked smart +// cards that appear to be hardware wallets. func (hub *Hub) Wallets() []accounts.Wallet { // Make sure the list of wallets is up to date - hub.stateLock.Lock() - defer hub.stateLock.Unlock() - hub.refreshWallets() + hub.stateLock.RLock() + defer hub.stateLock.RUnlock() + cpy := make([]accounts.Wallet, 0, len(hub.wallets)) for _, wallet := range hub.wallets { - if wallet != nil { - cpy = append(cpy, wallet) + cpy = append(cpy, wallet) + } + for i := 0; i < len(cpy); i++ { + for j := i + 1; j < len(cpy); j++ { + if cpy[i].URL().Cmp(cpy[j].URL()) > 0 { + cpy[i], cpy[j] = cpy[j], cpy[i] + } } } return cpy @@ -196,67 +195,77 @@ func (hub *Hub) Wallets() []accounts.Wallet { // refreshWallets scans the devices attached to the machine and updates the // list of wallets based on the found devices. func (hub *Hub) refreshWallets() { + // Don't scan the USB like crazy it the user fetches wallets in a loop + hub.stateLock.RLock() elapsed := time.Since(hub.refreshed) + hub.stateLock.RUnlock() + if elapsed < refreshThrottling { return } - + // Retrieve all the smart card reader to check for cards readers, err := hub.context.ListReaders() if err != nil { - log.Error("Error listing readers", "err", err) + // This is a perverted hack, the scard library returns an error if no card + // readers are present instead of simply returning an empty list. We don't + // want to fill the user's log with errors, so filter those out. + if err.Error() != "scard: Cannot find a smart card reader." { + log.Error("Failed to enumerate smart card readers", "err", err) + } } + // Transform the current list of wallets into the new one + hub.stateLock.Lock() events := []accounts.WalletEvent{} seen := make(map[string]struct{}) + for _, reader := range readers { - if wallet, ok := hub.wallets[reader]; ok { - // We already know about this card; check it's still present - if err := wallet.ping(); err != nil { - log.Debug("Got error pinging wallet", "reader", reader, "err", err) - } else { - seen[reader] = struct{}{} - } - continue - } + // Mark the reader as present seen[reader] = struct{}{} + // If we alreay know about this card, skip to the next reader, otherwise clean up + if wallet, ok := hub.wallets[reader]; ok { + if err := wallet.ping(); err == nil { + continue + } + wallet.Close() + events = append(events, accounts.WalletEvent{Wallet: wallet, Kind: accounts.WalletDropped}) + delete(hub.wallets, reader) + } + // New card detected, try to connect to it card, err := hub.context.Connect(reader, scard.ShareShared, scard.ProtocolAny) if err != nil { - log.Debug("Error opening card", "reader", reader, "err", err) + log.Debug("Failed to open smart card", "reader", reader, "err", err) continue } - wallet := NewWallet(hub, card) - err = wallet.connect() - if err != nil { - log.Debug("Error connecting to wallet", "reader", reader, "err", err) + if err = wallet.connect(); err != nil { + log.Debug("Failed to connect to smart card", "reader", reader, "err", err) card.Disconnect(scard.LeaveCard) continue } - + // Card connected, start tracking in amongs the wallets hub.wallets[reader] = wallet events = append(events, accounts.WalletEvent{Wallet: wallet, Kind: accounts.WalletArrived}) - log.Info("Found new smartcard wallet", "reader", reader, "publicKey", hexutil.Encode(wallet.PublicKey[:4])) } - - // Remove any wallets we no longer see - for k, wallet := range hub.wallets { - if _, ok := seen[k]; !ok { - log.Info("Wallet disconnected", "pubkey", hexutil.Encode(wallet.PublicKey[:4]), "reader", k) + // Remove any wallets no longer present + for reader, wallet := range hub.wallets { + if _, ok := seen[reader]; !ok { wallet.Close() events = append(events, accounts.WalletEvent{Wallet: wallet, Kind: accounts.WalletDropped}) - delete(hub.wallets, k) + delete(hub.wallets, reader) } } + hub.refreshed = time.Now() + hub.stateLock.Unlock() for _, event := range events { hub.updateFeed.Send(event) } - hub.refreshed = time.Now() } // Subscribe implements accounts.Backend, creating an async subscription to -// receive notifications on the addition or removal of wallets. +// receive notifications on the addition or removal of smart card wallets. func (hub *Hub) Subscribe(sink chan<- accounts.WalletEvent) event.Subscription { // We need the mutex to reliably start/stop the update loop hub.stateLock.Lock() @@ -274,16 +283,18 @@ func (hub *Hub) Subscribe(sink chan<- accounts.WalletEvent) event.Subscription { } // updater is responsible for maintaining an up-to-date list of wallets managed -// by the hub, and for firing wallet addition/removal events. +// by the smart card hub, and for firing wallet addition/removal events. func (hub *Hub) updater() { for { + // TODO: Wait for a USB hotplug event (not supported yet) or a refresh timeout + // <-hub.changes time.Sleep(refreshCycle) // Run the wallet refresher - hub.stateLock.Lock() hub.refreshWallets() // If all our subscribers left, stop the updater + hub.stateLock.Lock() if hub.updateScope.Count() == 0 { hub.updating = false hub.stateLock.Unlock() diff --git a/accounts/scwallet/wallet.go b/accounts/scwallet/wallet.go index 2f40afbaa..b85f41187 100644 --- a/accounts/scwallet/wallet.go +++ b/accounts/scwallet/wallet.go @@ -93,10 +93,11 @@ type Wallet struct { Hub *Hub // A handle to the Hub that instantiated this wallet. PublicKey []byte // The wallet's public key (used for communication and identification, not signing!) - lock sync.Mutex // Lock that gates access to struct fields and communication with the card - card *scard.Card // A handle to the smartcard interface for the wallet. - session *Session // The secure communication session with the card - log log.Logger // Contextual logger to tag the base with its id + lock sync.Mutex // Lock that gates access to struct fields and communication with the card + card *scard.Card // A handle to the smartcard interface for the wallet. + session *Session // The secure communication session with the card + log log.Logger // Contextual logger to tag the base with its id + deriveNextPath accounts.DerivationPath // Next derivation path for account auto-discovery deriveNextAddr common.Address // Next derived account address for auto-discovery deriveChain ethereum.ChainStateReader // Blockchain state reader to discover used account with @@ -273,7 +274,7 @@ func (w *Wallet) Unpair(pin []byte) error { func (w *Wallet) URL() accounts.URL { return accounts.URL{ Scheme: w.Hub.scheme, - Path: fmt.Sprintf("%x", w.PublicKey[1:3]), + Path: fmt.Sprintf("%x", w.PublicKey[1:5]), // Byte #0 isn't unique; 1:5 covers << 64K cards, bump to 1:9 for << 4M } } @@ -327,7 +328,7 @@ func (w *Wallet) Open(passphrase string) error { if err := w.session.authenticate(*pairing); err != nil { return fmt.Errorf("failed to authenticate card %x: %s", w.PublicKey[:4], err) } - return nil + return ErrPINNeeded } // If no passphrase was supplied, request the PUK from the user if passphrase == "" { @@ -389,8 +390,8 @@ func (w *Wallet) Close() error { // selfDerive is an account derivation loop that upon request attempts to find // new non-zero accounts. func (w *Wallet) selfDerive() { - w.log.Debug("Smartcard wallet self-derivation started") - defer w.log.Debug("Smartcard wallet self-derivation stopped") + w.log.Debug("Smart card wallet self-derivation started") + defer w.log.Debug("Smart card wallet self-derivation stopped") // Execute self-derivations until termination or error var ( @@ -418,21 +419,22 @@ func (w *Wallet) selfDerive() { // Device lock obtained, derive the next batch of accounts var ( - paths []accounts.DerivationPath - nextAccount accounts.Account - nextAddr = w.deriveNextAddr - nextPath = w.deriveNextPath + paths []accounts.DerivationPath + nextAcc accounts.Account + + nextAddr = w.deriveNextAddr + nextPath = w.deriveNextPath context = context.Background() ) for empty := false; !empty; { // Retrieve the next derived Ethereum account if nextAddr == (common.Address{}) { - if nextAccount, err = w.session.derive(nextPath); err != nil { + if nextAcc, err = w.session.derive(nextPath); err != nil { w.log.Warn("Smartcard wallet account derivation failed", "err", err) break } - nextAddr = nextAccount.Address + nextAddr = nextAcc.Address } // Check the account's status against the current chain state var ( @@ -459,8 +461,8 @@ func (w *Wallet) selfDerive() { paths = append(paths, path) // Display a log message to the user for new (or previously empty accounts) - if _, known := pairing.Accounts[nextAddr]; !known || (!empty && nextAddr == w.deriveNextAddr) { - w.log.Info("Smartcard wallet discovered new account", "address", nextAccount.Address, "path", path, "balance", balance, "nonce", nonce) + if _, known := pairing.Accounts[nextAddr]; !known || !empty || nextAddr != w.deriveNextAddr { + w.log.Info("Smartcard wallet discovered new account", "address", nextAddr, "path", path, "balance", balance, "nonce", nonce) } pairing.Accounts[nextAddr] = path @@ -470,12 +472,10 @@ func (w *Wallet) selfDerive() { nextPath[len(nextPath)-1]++ } } - // If there are new accounts, write them out if len(paths) > 0 { err = w.Hub.setPairing(w, pairing) } - // Shift the self-derivation forward w.deriveNextAddr = nextAddr w.deriveNextPath = nextPath @@ -524,6 +524,13 @@ func (w *Wallet) Accounts() []accounts.Account { for address, path := range pairing.Accounts { ret = append(ret, w.makeAccount(address, path)) } + for i := 0; i < len(ret); i++ { + for j := i + 1; j < len(ret); j++ { + if ret[i].URL.Cmp(ret[j].URL) > 0 { + ret[i], ret[j] = ret[j], ret[i] + } + } + } return ret } return nil