Merge pull request ethereum#631 from Gustav-Simonsson/improve_key_store_crypto

Improve key store crypto

Author: obscuren

accounts/account_manager.go

@@ -33,14 +33,14 @@ and accounts persistence is derived from stored keys' addresses
 package accounts
 
 import (
-	"bytes"
 	"crypto/ecdsa"
 	crand "crypto/rand"
 	"errors"
 	"os"
 	"sync"
 	"time"
 
+	"github.com/ethereum/go-ethereum/common"
 	"github.com/ethereum/go-ethereum/crypto"
 )
 
@@ -50,12 +50,12 @@ var (
 )
 
 type Account struct {
-	Address []byte
+	Address common.Address
 }
 
 type Manager struct {
 	keyStore crypto.KeyStore2
-	unlocked map[string]*unlocked
+	unlocked map[common.Address]*unlocked
 	mutex    sync.RWMutex
 }
 
@@ -67,40 +67,40 @@ type unlocked struct {
 func NewManager(keyStore crypto.KeyStore2) *Manager {
 	return &Manager{
 		keyStore: keyStore,
-		unlocked: make(map[string]*unlocked),
+		unlocked: make(map[common.Address]*unlocked),
 	}
 }
 
-func (am *Manager) HasAccount(addr []byte) bool {
+func (am *Manager) HasAccount(addr common.Address) bool {
 	accounts, _ := am.Accounts()
 	for _, acct := range accounts {
-		if bytes.Compare(acct.Address, addr) == 0 {
+		if acct.Address == addr {
 			return true
 		}
 	}
 	return false
 }
 
-func (am *Manager) Primary() (addr []byte, err error) {
+func (am *Manager) Primary() (addr common.Address, err error) {
 	addrs, err := am.keyStore.GetKeyAddresses()
 	if os.IsNotExist(err) {
-		return nil, ErrNoKeys
+		return common.Address{}, ErrNoKeys
 	} else if err != nil {
-		return nil, err
+		return common.Address{}, err
 	}
 	if len(addrs) == 0 {
-		return nil, ErrNoKeys
+		return common.Address{}, ErrNoKeys
 	}
 	return addrs[0], nil
 }
 
-func (am *Manager) DeleteAccount(address []byte, auth string) error {
+func (am *Manager) DeleteAccount(address common.Address, auth string) error {
 	return am.keyStore.DeleteKey(address, auth)
 }
 
 func (am *Manager) Sign(a Account, toSign []byte) (signature []byte, err error) {
 	am.mutex.RLock()
-	unlockedKey, found := am.unlocked[string(a.Address)]
+	unlockedKey, found := am.unlocked[a.Address]
 	am.mutex.RUnlock()
 	if !found {
 		return nil, ErrLocked
@@ -111,7 +111,7 @@ func (am *Manager) Sign(a Account, toSign []byte) (signature []byte, err error)
 
 // TimedUnlock unlocks the account with the given address.
 // When timeout has passed, the account will be locked again.
-func (am *Manager) TimedUnlock(addr []byte, keyAuth string, timeout time.Duration) error {
+func (am *Manager) TimedUnlock(addr common.Address, keyAuth string, timeout time.Duration) error {
 	key, err := am.keyStore.GetKey(addr, keyAuth)
 	if err != nil {
 		return err
@@ -124,7 +124,7 @@ func (am *Manager) TimedUnlock(addr []byte, keyAuth string, timeout time.Duratio
 // Unlock unlocks the account with the given address. The account
 // stays unlocked until the program exits or until a TimedUnlock
 // timeout (started after the call to Unlock) expires.
-func (am *Manager) Unlock(addr []byte, keyAuth string) error {
+func (am *Manager) Unlock(addr common.Address, keyAuth string) error {
 	key, err := am.keyStore.GetKey(addr, keyAuth)
 	if err != nil {
 		return err
@@ -157,10 +157,10 @@ func (am *Manager) Accounts() ([]Account, error) {
 	return accounts, err
 }
 
-func (am *Manager) addUnlocked(addr []byte, key *crypto.Key) *unlocked {
+func (am *Manager) addUnlocked(addr common.Address, key *crypto.Key) *unlocked {
 	u := &unlocked{Key: key, abort: make(chan struct{})}
 	am.mutex.Lock()
-	prev, found := am.unlocked[string(addr)]
+	prev, found := am.unlocked[addr]
 	if found {
 		// terminate dropLater for this key to avoid unexpected drops.
 		close(prev.abort)
@@ -169,12 +169,12 @@ func (am *Manager) addUnlocked(addr []byte, key *crypto.Key) *unlocked {
 		// key, i.e. when Unlock was used.
 		zeroKey(prev.PrivateKey)
 	}
-	am.unlocked[string(addr)] = u
+	am.unlocked[addr] = u
 	am.mutex.Unlock()
 	return u
 }
 
-func (am *Manager) dropLater(addr []byte, u *unlocked, timeout time.Duration) {
+func (am *Manager) dropLater(addr common.Address, u *unlocked, timeout time.Duration) {
 	t := time.NewTimer(timeout)
 	defer t.Stop()
 	select {
@@ -186,9 +186,9 @@ func (am *Manager) dropLater(addr []byte, u *unlocked, timeout time.Duration) {
 		// was launched with. we can check that using pointer equality
 		// because the map stores a new pointer every time the key is
 		// unlocked.
-		if am.unlocked[string(addr)] == u {
+		if am.unlocked[addr] == u {
 			zeroKey(u.PrivateKey)
-			delete(am.unlocked, string(addr))
+			delete(am.unlocked, addr)
 		}
 		am.mutex.Unlock()
 	}
@@ -204,7 +204,7 @@ func zeroKey(k *ecdsa.PrivateKey) {
 
 // USE WITH CAUTION = this will save an unencrypted private key on disk
 // no cli or js interface
-func (am *Manager) Export(path string, addr []byte, keyAuth string) error {
+func (am *Manager) Export(path string, addr common.Address, keyAuth string) error {
 	key, err := am.keyStore.GetKey(addr, keyAuth)
 	if err != nil {
 		return err

cmd/geth/admin.go

@@ -126,7 +126,7 @@ func (js *jsre) pendingTransactions(call otto.FunctionCall) otto.Value {
 	// Add the accouns to a new set
 	accountSet := set.New()
 	for _, account := range accounts {
-		accountSet.Add(common.BytesToAddress(account.Address))
+		accountSet.Add(account.Address)
 	}
 
 	//ltxs := make([]*tx, len(txs))
@@ -391,7 +391,7 @@ func (js *jsre) unlock(call otto.FunctionCall) otto.Value {
 		}
 	}
 	am := js.ethereum.AccountManager()
-	err = am.TimedUnlock(common.FromHex(addr), passphrase, time.Duration(seconds)*time.Second)
+	err = am.TimedUnlock(common.HexToAddress(addr), passphrase, time.Duration(seconds)*time.Second)
 	if err != nil {
 		fmt.Printf("Unlock account failed '%v'\n", err)
 		return otto.FalseValue()
@@ -433,7 +433,7 @@ func (js *jsre) newAccount(call otto.FunctionCall) otto.Value {
 		fmt.Printf("Could not create the account: %v", err)
 		return otto.UndefinedValue()
 	}
-	return js.re.ToVal(common.ToHex(acct.Address))
+	return js.re.ToVal(acct.Address.Hex())
 }
 
 func (js *jsre) nodeInfo(call otto.FunctionCall) otto.Value {

cmd/geth/js.go

@@ -26,6 +26,7 @@ import (
 	"strings"
 
 	"github.com/ethereum/go-ethereum/cmd/utils"
+	"github.com/ethereum/go-ethereum/common"
 	"github.com/ethereum/go-ethereum/common/docserver"
 	"github.com/ethereum/go-ethereum/common/natspec"
 	"github.com/ethereum/go-ethereum/eth"
@@ -164,7 +165,7 @@ func (self *jsre) UnlockAccount(addr []byte) bool {
 		return false
 	}
 	// TODO: allow retry
-	if err := self.ethereum.AccountManager().Unlock(addr, pass); err != nil {
+	if err := self.ethereum.AccountManager().Unlock(common.BytesToAddress(addr), pass); err != nil {
 		return false
 	} else {
 		fmt.Println("Account is now unlocked for this session.")

cmd/geth/js_test.go

@@ -45,7 +45,7 @@ type testjethre struct {
 }
 
 func (self *testjethre) UnlockAccount(acc []byte) bool {
-	err := self.ethereum.AccountManager().Unlock(acc, "")
+	err := self.ethereum.AccountManager().Unlock(common.BytesToAddress(acc), "")
 	if err != nil {
 		panic("unable to unlock")
 	}
@@ -68,7 +68,7 @@ func testJEthRE(t *testing.T) (string, *testjethre, *eth.Ethereum) {
 	// set up mock genesis with balance on the testAddress
 	core.GenesisData = []byte(testGenesis)
 
-	ks := crypto.NewKeyStorePassphrase(filepath.Join(tmp, "keys"))
+	ks := crypto.NewKeyStorePassphrase(filepath.Join(tmp, "keystore"))
 	am := accounts.NewManager(ks)
 	ethereum, err := eth.New(&eth.Config{
 		DataDir:        tmp,

cmd/geth/main.go

@@ -365,11 +365,10 @@ func unlockAccount(ctx *cli.Context, am *accounts.Manager, account string) (pass
 	// Load startup keys. XXX we are going to need a different format
 	// Attempt to unlock the account
 	passphrase = getPassPhrase(ctx, "", false)
-	accbytes := common.FromHex(account)
-	if len(accbytes) == 0 {
+	if len(account) == 0 {
 		utils.Fatalf("Invalid account address '%s'", account)
 	}
-	err = am.Unlock(accbytes, passphrase)
+	err = am.Unlock(common.StringToAddress(account), passphrase)
 	if err != nil {
 		utils.Fatalf("Unlock account failed '%v'", err)
 	}
@@ -385,11 +384,11 @@ func startEth(ctx *cli.Context, eth *eth.Ethereum) {
 	account := ctx.GlobalString(utils.UnlockedAccountFlag.Name)
 	if len(account) > 0 {
 		if account == "primary" {
-			accbytes, err := am.Primary()
+			primaryAcc, err := am.Primary()
 			if err != nil {
 				utils.Fatalf("no primary account: %v", err)
 			}
-			account = common.ToHex(accbytes)
+			account = primaryAcc.Hex()
 		}
 		unlockAccount(ctx, am, account)
 	}

cmd/mist/gui.go

@@ -232,7 +232,7 @@ func (self *Gui) loadMergedMiningOptions() {
 func (gui *Gui) insertTransaction(window string, tx *types.Transaction) {
 	var inout string
 	from, _ := tx.From()
-	if gui.eth.AccountManager().HasAccount(common.Hex2Bytes(from.Hex())) {
+	if gui.eth.AccountManager().HasAccount(from) {
 		inout = "send"
 	} else {
 		inout = "recv"

cmd/utils/flags.go

@@ -346,7 +346,7 @@ func GetChain(ctx *cli.Context) (*core.ChainManager, common.Database, common.Dat
 
 func GetAccountManager(ctx *cli.Context) *accounts.Manager {
 	dataDir := ctx.GlobalString(DataDirFlag.Name)
-	ks := crypto.NewKeyStorePassphrase(filepath.Join(dataDir, "keys"))
+	ks := crypto.NewKeyStorePassphrase(filepath.Join(dataDir, "keystore"))
 	return accounts.NewManager(ks)
 }
 

common/natspec/natspec_e2e_test.go

@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"io/ioutil"
 	"os"
+	"strings"
 	"testing"
 
 	"github.com/ethereum/go-ethereum/accounts"
@@ -84,7 +85,7 @@ type testFrontend struct {
 }
 
 func (self *testFrontend) UnlockAccount(acc []byte) bool {
-	self.ethereum.AccountManager().Unlock(acc, "password")
+	self.ethereum.AccountManager().Unlock(common.BytesToAddress(acc), "password")
 	return true
 }
 
@@ -103,19 +104,19 @@ func testEth(t *testing.T) (ethereum *eth.Ethereum, err error) {
 
 	os.RemoveAll("/tmp/eth-natspec/")
 
-	err = os.MkdirAll("/tmp/eth-natspec/keys", os.ModePerm)
+	err = os.MkdirAll("/tmp/eth-natspec/keystore", os.ModePerm)
 	if err != nil {
 		panic(err)
 	}
 
 	// create a testAddress
-	ks := crypto.NewKeyStorePassphrase("/tmp/eth-natspec/keys")
+	ks := crypto.NewKeyStorePassphrase("/tmp/eth-natspec/keystore")
 	am := accounts.NewManager(ks)
 	testAccount, err := am.NewAccount("password")
 	if err != nil {
 		panic(err)
 	}
-	testAddress := common.Bytes2Hex(testAccount.Address)
+	testAddress := strings.TrimPrefix(testAccount.Address.Hex(), "0x")
 
 	// set up mock genesis with balance on the testAddress
 	core.GenesisData = []byte(`{

crypto/crypto.go

@@ -181,11 +181,11 @@ func Decrypt(prv *ecdsa.PrivateKey, ct []byte) ([]byte, error) {
 
 // Used only by block tests.
 func ImportBlockTestKey(privKeyBytes []byte) error {
-	ks := NewKeyStorePassphrase(common.DefaultDataDir() + "/keys")
+	ks := NewKeyStorePassphrase(common.DefaultDataDir() + "/keystore")
 	ecKey := ToECDSA(privKeyBytes)
 	key := &Key{
 		Id:         uuid.NewRandom(),
-		Address:    PubkeyToAddress(ecKey.PublicKey),
+		Address:    common.BytesToAddress(PubkeyToAddress(ecKey.PublicKey)),
 		PrivateKey: ecKey,
 	}
 	err := ks.StoreKey(key, "")
@@ -231,13 +231,13 @@ func decryptPreSaleKey(fileContent []byte, password string) (key *Key, err error
 	ecKey := ToECDSA(ethPriv)
 	key = &Key{
 		Id:         nil,
-		Address:    PubkeyToAddress(ecKey.PublicKey),
+		Address:    common.BytesToAddress(PubkeyToAddress(ecKey.PublicKey)),
 		PrivateKey: ecKey,
 	}
-	derivedAddr := common.Bytes2Hex(key.Address)
+	derivedAddr := hex.EncodeToString(key.Address.Bytes()) // needed because .Hex() gives leading "0x"
 	expectedAddr := preSaleKeyStruct.EthAddr
 	if derivedAddr != expectedAddr {
-		err = errors.New("decrypted addr not equal to expected addr")
+		err = errors.New(fmt.Sprintf("decrypted addr not equal to expected addr ", derivedAddr, expectedAddr))
 	}
 	return key, err
 }
@@ -252,7 +252,7 @@ func aesCBCDecrypt(key []byte, cipherText []byte, iv []byte) (plainText []byte,
 	decrypter.CryptBlocks(paddedPlainText, cipherText)
 	plainText = PKCS7Unpad(paddedPlainText)
 	if plainText == nil {
-		err = errors.New("Decryption failed: PKCS7Unpad failed after decryption")
+		err = errors.New("Decryption failed: PKCS7Unpad failed after AES decryption")
 	}
 	return plainText, err
 }