crypto: KeyStore improvement ideas

[fjl]

The key management infrastructure is currently split into several layers:

package crypto

// the current interface
type KeyStore interface {
    GenerateNewKey(io.Reader, string) (*Key, error)
    GetKey(common.Address, string) (*Key, error) // get key from addr and auth string
    GetKeyAddresses() ([]common.Address, error)  // get all addresses
    StoreKey(*Key, string) error                 // store key optionally using auth string
    DeleteKey(common.Address, string) error      // delete key by addr and auth string
    Cleanup(keyAddr common.Address) (err error)
}

          ┌───────────────────────────┐              
          │  accounts.AccountManager  │              
          └───────────────────────────┘              
                        │                            
           ┌────────────┴──────────────┐             
           ▼                           ▼             
┌────────────────────┐   ┌──────────────────────────┐
│crypto.keyStorePlain│   │crypto.keyStorePassphrase │
└────────────────────┘   └──────────────────────────┘

The general reasoning behind this design was to ensure that people wouldn't use
unencrypted keys. It was also supposed to anticipate an implementation of KeyStore that
talks to a hardware crypto device.

The interface has grown over time and isn't as flexible as it could be.
One particular example where the split hurts is that using KeyStorePlain still requires
'unlocking' the account with an empty passphrase because the account manager doesn't
know whether keys are encrypted. We could also use an in-memory KeyStore for testing,
but implementing one requires satisfying the whole interface.

I propose the following changes:

• Move the KeyStore interface to package accounts.
  There are no other users of the interface apart from the
  account manager.
• Move encryption/decryption of key data into the account manager.
  This will shrink the interface to a mapping between addresses and keys.

package accounts

// proposed interface
type KeyStore interface {
    GetKeyAddresses() ([]common.Address, error)
    GetKey(common.Address) (*crypto.Key, error)
    StoreKey(*crypto.Key, string) error
    DeleteKey(common.Address) error
}

[Gustav-Simonsson]

@fjl How would one get a *crypto.Key from only common.Address if the key is encrypted? It's not possible to fit the encrypted key storage data into crypto.Key without modifying it.

[fjl]

Sure, *crypto.Key is not the right type for this.
The return value would need to be a type that contains the ciphertext, encryption parameters, etc.

[karalabe]

I like the idea of this refactoring. I would modify the interface a bit though :P

type KeyStore interface {
    Keys() ([]common.Address, error)
    Store(*crypto.Key, string) error
    Fetch(common.Address) (*crypto.Key, error)
    Delete(common.Address) error
}

i.e. Since the interface is a KeyStore, I don't see the need to have all methods contain "Key" in their name.

However, the issue with this interface design is that I need the AccountManager to be able to decrypt anything a KeyStore might throw at it. As in I won't be able to implement some arbitrary weird key encryption mechanism as my hands are tied with regard to the format. This may or may not be a problem, but thought I'd mention it.

[fjl]

One part of this plan has been implemented in #2284. The keyStore interface is now located in package accounts, hidden from the API and looks like this:

type keyStore interface {
    // Loads and decrypts the key from disk.
    GetKey(addr common.Address, filename string, auth string) (*Key, error)
    // Writes and encrypts the key.
    StoreKey(filename string, k *Key, auth string) error
    // Joins filename with the key directory unless it is already absolute.
    JoinPath(filename string) string
}

[fjl]

This doesn't apply anymore.