cmd/clef, signer: refresh tutorial, fix noticed issues (#19774)

* cmd/clef, signer: refresh tutorial, fix noticed issues

* cmd/clef, signer: support removing stored keys (delpw + rules)

* cmd/clef: polishes + Geth integration in the tutorial

8 files changed, 129 insertions, 84 deletions

diff --git a/signer/core/api.go b/signer/core/api.go
index 251ee55dc..d68919c6f 100644
--- a/signer/core/api.go
+++ b/signer/core/api.go
@@ -24,7 +24,6 @@ import (
 	"math/big"
 	"os"
 	"reflect"
-	"strings"
 
 	"github.com/ethereum/go-ethereum/accounts"
 	"github.com/ethereum/go-ethereum/accounts/keystore"
@@ -44,7 +43,7 @@ const (
 	// ExternalAPIVersion -- see extapi_changelog.md
 	ExternalAPIVersion = "6.0.0"
 	// InternalAPIVersion -- see intapi_changelog.md
-	InternalAPIVersion = "6.0.0"
+	InternalAPIVersion = "7.0.0"
 )
 
 // ExternalAPI defines the external API through which signing requests are made.
@@ -234,7 +233,7 @@ type (
 		ContentType string                  `json:"content_type"`
 		Address     common.MixedcaseAddress `json:"address"`
 		Rawdata     []byte                  `json:"raw_data"`
-		Message     []*NameValueType        `json:"message"`
+		Messages    []*NameValueType        `json:"messages"`
 		Hash        hexutil.Bytes           `json:"hash"`
 		Meta        Metadata                `json:"meta"`
 	}
@@ -477,22 +476,24 @@ func logDiff(original *SignTxRequest, new *SignTxResponse) bool {
 	return modified
 }
 
-func (api *SignerAPI) lookupPassword(address common.Address) string {
-	return api.credentials.Get(strings.ToLower(address.String()))
+func (api *SignerAPI) lookupPassword(address common.Address) (string, error) {
+	return api.credentials.Get(address.Hex())
 }
+
 func (api *SignerAPI) lookupOrQueryPassword(address common.Address, title, prompt string) (string, error) {
-	if pw := api.lookupPassword(address); pw != "" {
+	// Look up the password and return if available
+	if pw, err := api.lookupPassword(address); err == nil {
 		return pw, nil
-	} else {
-		pwResp, err := api.UI.OnInputRequired(UserInputRequest{title, prompt, true})
-		if err != nil {
-			log.Warn("error obtaining password", "error", err)
-			// We'll not forward the error here, in case the error contains info about the response from the UI,
-			// which could leak the password if it was malformed json or something
-			return "", errors.New("internal error")
-		}
-		return pwResp.Text, nil
 	}
+	// Password unavailable, request it from the user
+	pwResp, err := api.UI.OnInputRequired(UserInputRequest{title, prompt, true})
+	if err != nil {
+		log.Warn("error obtaining password", "error", err)
+		// We'll not forward the error here, in case the error contains info about the response from the UI,
+		// which could leak the password if it was malformed json or something
+		return "", errors.New("internal error")
+	}
+	return pwResp.Text, nil
 }
 
 // SignTransaction signs the given Transaction and returns it both as json and rlp-encoded form
diff --git a/signer/core/cliui.go b/signer/core/cliui.go
index cf7101441..46a13f1e4 100644
--- a/signer/core/cliui.go
+++ b/signer/core/cliui.go
@@ -169,13 +169,12 @@ func (ui *CommandlineUI) ApproveSignData(request *SignDataRequest) (SignDataResp
 
 	fmt.Printf("-------- Sign data request--------------\n")
 	fmt.Printf("Account:  %s\n", request.Address.String())
-	fmt.Printf("message:\n")
-	for _, nvt := range request.Message {
+	fmt.Printf("messages:\n")
+	for _, nvt := range request.Messages {
 		fmt.Printf("%v\n", nvt.Pprint(1))
 	}
-	//fmt.Printf("message:  \n%v\n", request.Message)
 	fmt.Printf("raw data:  \n%q\n", request.Rawdata)
-	fmt.Printf("message hash:  %v\n", request.Hash)
+	fmt.Printf("data hash:  %v\n", request.Hash)
 	fmt.Printf("-------------------------------------------\n")
 	showMetadata(request.Meta)
 	if !ui.confirm() {
@@ -187,7 +186,6 @@ func (ui *CommandlineUI) ApproveSignData(request *SignDataRequest) (SignDataResp
 // ApproveListing prompt the user for confirmation to list accounts
 // the list of accounts to list can be modified by the UI
 func (ui *CommandlineUI) ApproveListing(request *ListRequest) (ListResponse, error) {
-
 	ui.mu.Lock()
 	defer ui.mu.Unlock()
 
diff --git a/signer/core/signed_data.go b/signer/core/signed_data.go
index 27eca9183..91e6cc7da 100644
--- a/signer/core/signed_data.go
+++ b/signer/core/signed_data.go
@@ -123,11 +123,10 @@ type TypedDataDomain struct {
 var typedDataReferenceTypeRegexp = regexp.MustCompile(`^[A-Z](\w*)(\[\])?$`)
 
 // sign receives a request and produces a signature
-
+//
 // Note, the produced signature conforms to the secp256k1 curve R, S and V values,
 // where the V value will be 27 or 28 for legacy reasons, if legacyV==true.
 func (api *SignerAPI) sign(addr common.MixedcaseAddress, req *SignDataRequest, legacyV bool) (hexutil.Bytes, error) {
-
 	// We make the request prior to looking up if we actually have the account, to prevent
 	// account-enumeration via the API
 	res, err := api.UI.ApproveSignData(req)
@@ -169,7 +168,6 @@ func (api *SignerAPI) SignData(ctx context.Context, contentType string, addr com
 	if err != nil {
 		return nil, err
 	}
-
 	signature, err := api.sign(addr, req, transformV)
 	if err != nil {
 		api.UI.ShowError(err.Error())
@@ -202,7 +200,7 @@ func (api *SignerAPI) determineSignatureFormat(ctx context.Context, contentType
 			return nil, useEthereumV, err
 		}
 		sighash, msg := SignTextValidator(validatorData)
-		message := []*NameValueType{
+		messages := []*NameValueType{
 			{
 				Name:  "This is a request to sign data intended for a particular validator (see EIP 191 version 0)",
 				Typ:   "description",
@@ -224,7 +222,7 @@ func (api *SignerAPI) determineSignatureFormat(ctx context.Context, contentType
 				Value: fmt.Sprintf("0x%x", msg),
 			},
 		}
-		req = &SignDataRequest{ContentType: mediaType, Rawdata: []byte(msg), Message: message, Hash: sighash}
+		req = &SignDataRequest{ContentType: mediaType, Rawdata: []byte(msg), Messages: messages, Hash: sighash}
 	case ApplicationClique.Mime:
 		// Clique is the Ethereum PoA standard
 		stringData, ok := data.(string)
@@ -251,7 +249,7 @@ func (api *SignerAPI) determineSignatureFormat(ctx context.Context, contentType
 		if err != nil {
 			return nil, useEthereumV, err
 		}
-		message := []*NameValueType{
+		messages := []*NameValueType{
 			{
 				Name:  "Clique header",
 				Typ:   "clique",
@@ -260,7 +258,7 @@ func (api *SignerAPI) determineSignatureFormat(ctx context.Context, contentType
 		}
 		// Clique uses V on the form 0 or 1
 		useEthereumV = false
-		req = &SignDataRequest{ContentType: mediaType, Rawdata: cliqueRlp, Message: message, Hash: sighash}
+		req = &SignDataRequest{ContentType: mediaType, Rawdata: cliqueRlp, Messages: messages, Hash: sighash}
 	default: // also case TextPlain.Mime:
 		// Calculates an Ethereum ECDSA signature for:
 		// hash = keccak256("\x19${byteVersion}Ethereum Signed Message:\n${message length}${message}")
@@ -272,21 +270,20 @@ func (api *SignerAPI) determineSignatureFormat(ctx context.Context, contentType
 				return nil, useEthereumV, err
 			} else {
 				sighash, msg := accounts.TextAndHash(textData)
-				message := []*NameValueType{
+				messages := []*NameValueType{
 					{
 						Name:  "message",
 						Typ:   accounts.MimetypeTextPlain,
 						Value: msg,
 					},
 				}
-				req = &SignDataRequest{ContentType: mediaType, Rawdata: []byte(msg), Message: message, Hash: sighash}
+				req = &SignDataRequest{ContentType: mediaType, Rawdata: []byte(msg), Messages: messages, Hash: sighash}
 			}
 		}
 	}
 	req.Address = addr
 	req.Meta = MetadataFromContext(ctx)
 	return req, useEthereumV, nil
-
 }
 
 // SignTextWithValidator signs the given message which can be further recovered
@@ -327,11 +324,11 @@ func (api *SignerAPI) SignTypedData(ctx context.Context, addr common.MixedcaseAd
 	}
 	rawData := []byte(fmt.Sprintf("\x19\x01%s%s", string(domainSeparator), string(typedDataHash)))
 	sighash := crypto.Keccak256(rawData)
-	message, err := typedData.Format()
+	messages, err := typedData.Format()
 	if err != nil {
 		return nil, err
 	}
-	req := &SignDataRequest{ContentType: DataTyped.Mime, Rawdata: rawData, Message: message, Hash: sighash}
+	req := &SignDataRequest{ContentType: DataTyped.Mime, Rawdata: rawData, Messages: messages, Hash: sighash}
 	signature, err := api.sign(addr, req, true)
 	if err != nil {
 		api.UI.ShowError(err.Error())
diff --git a/signer/rules/rules.go b/signer/rules/rules.go
index 5ebffa3af..f731dac40 100644
--- a/signer/rules/rules.go
+++ b/signer/rules/rules.go
@@ -74,12 +74,28 @@ func (r *rulesetUI) execute(jsfunc string, jsarg interface{}) (otto.Value, error
 
 	// Instantiate a fresh vm engine every time
 	vm := otto.New()
+
 	// Set the native callbacks
 	consoleObj, _ := vm.Get("console")
 	consoleObj.Object().Set("log", consoleOutput)
 	consoleObj.Object().Set("error", consoleOutput)
-	vm.Set("storage", r.storage)
 
+	vm.Set("storage", struct{}{})
+	storageObj, _ := vm.Get("storage")
+	storageObj.Object().Set("put", func(call otto.FunctionCall) otto.Value {
+		key, val := call.Argument(0).String(), call.Argument(1).String()
+		if val == "" {
+			r.storage.Del(key)
+		} else {
+			r.storage.Put(key, val)
+		}
+		return otto.NullValue()
+	})
+	storageObj.Object().Set("get", func(call otto.FunctionCall) otto.Value {
+		goval, _ := r.storage.Get(call.Argument(0).String())
+		jsval, _ := otto.ToValue(goval)
+		return jsval
+	})
 	// Load bootstrap libraries
 	script, err := vm.Compile("bignumber.js", BigNumber_JS)
 	if err != nil {
diff --git a/signer/rules/rules_test.go b/signer/rules/rules_test.go
index 19d9049c7..e1a23236c 100644
--- a/signer/rules/rules_test.go
+++ b/signer/rules/rules_test.go
@@ -301,23 +301,23 @@ func TestStorage(t *testing.T) {
 
 	js := `
 	function testStorage(){
-		storage.Put("mykey", "myvalue")
-		a = storage.Get("mykey")
+		storage.put("mykey", "myvalue")
+		a = storage.get("mykey")
 
-		storage.Put("mykey", ["a", "list"])  	// Should result in "a,list"
-		a += storage.Get("mykey")
+		storage.put("mykey", ["a", "list"])  	// Should result in "a,list"
+		a += storage.get("mykey")
 
 
-		storage.Put("mykey", {"an": "object"}) 	// Should result in "[object Object]"
-		a += storage.Get("mykey")
+		storage.put("mykey", {"an": "object"}) 	// Should result in "[object Object]"
+		a += storage.get("mykey")
 
 
-		storage.Put("mykey", JSON.stringify({"an": "object"})) // Should result in '{"an":"object"}'
-		a += storage.Get("mykey")
+		storage.put("mykey", JSON.stringify({"an": "object"})) // Should result in '{"an":"object"}'
+		a += storage.get("mykey")
 
-		a += storage.Get("missingkey")		//Missing keys should result in empty string
-		storage.Put("","missing key==noop") // Can't store with 0-length key
-		a += storage.Get("")				// Should result in ''
+		a += storage.get("missingkey")		//Missing keys should result in empty string
+		storage.put("","missing key==noop") // Can't store with 0-length key
+		a += storage.get("")				// Should result in ''
 
 		var b = new BigNumber(2)
 		var c = new BigNumber(16)//"0xf0",16)
@@ -337,7 +337,6 @@ func TestStorage(t *testing.T) {
 	if err != nil {
 		t.Errorf("Unexpected error %v", err)
 	}
-
 	retval, err := v.ToString()
 
 	if err != nil {
@@ -369,7 +368,7 @@ const ExampleTxWindow = `
 		var windowstart = new Date().getTime() - window;
 
 		var txs = [];
-		var stored = storage.Get('txs');
+		var stored = storage.get('txs');
 
 		if(stored != ""){
 			txs = JSON.parse(stored)
@@ -414,19 +413,18 @@ const ExampleTxWindow = `
 		var value = big(resp.tx.value)
 		var txs = []
 		// Load stored transactions
-		var stored = storage.Get('txs');
+		var stored = storage.get('txs');
 		if(stored != ""){
 			txs = JSON.parse(stored)
 		}
 		// Add this to the storage
 		txs.push({tstamp: new Date().getTime(), value: value});
-		storage.Put("txs", JSON.stringify(txs));
+		storage.put("txs", JSON.stringify(txs));
 	}
 
 `
 
 func dummyTx(value hexutil.Big) *core.SignTxRequest {
-
 	to, _ := mixAddr("000000000000000000000000000000000000dead")
 	from, _ := mixAddr("000000000000000000000000000000000000dead")
 	n := hexutil.Uint64(3)
@@ -448,28 +446,27 @@ func dummyTx(value hexutil.Big) *core.SignTxRequest {
 		Meta: core.Metadata{Remote: "remoteip", Local: "localip", Scheme: "inproc"},
 	}
 }
-func dummyTxWithV(value uint64) *core.SignTxRequest {
 
+func dummyTxWithV(value uint64) *core.SignTxRequest {
 	v := big.NewInt(0).SetUint64(value)
 	h := hexutil.Big(*v)
 	return dummyTx(h)
 }
+
 func dummySigned(value *big.Int) *types.Transaction {
 	to := common.HexToAddress("000000000000000000000000000000000000dead")
 	gas := uint64(21000)
 	gasPrice := big.NewInt(2000000)
 	data := make([]byte, 0)
 	return types.NewTransaction(3, to, value, gas, gasPrice, data)
-
 }
-func TestLimitWindow(t *testing.T) {
 
+func TestLimitWindow(t *testing.T) {
 	r, err := initRuleEngine(ExampleTxWindow)
 	if err != nil {
 		t.Errorf("Couldn't create evaluator %v", err)
 		return
 	}
-
 	// 0.3 ether: 429D069189E0000 wei
 	v := big.NewInt(0).SetBytes(common.Hex2Bytes("0429D069189E0000"))
 	h := hexutil.Big(*v)
@@ -496,7 +493,6 @@ func TestLimitWindow(t *testing.T) {
 	if resp.Approved {
 		t.Errorf("Expected check to resolve to 'Reject'")
 	}
-
 }
 
 // dontCallMe is used as a next-handler that does not want to be called - it invokes test failure
@@ -508,6 +504,7 @@ func (d *dontCallMe) OnInputRequired(info core.UserInputRequest) (core.UserInput
 	d.t.Fatalf("Did not expect next-handler to be called")
 	return core.UserInputResponse{}, nil
 }
+
 func (d *dontCallMe) RegisterUIServer(api *core.UIServerAPI) {
 }
 
@@ -589,7 +586,7 @@ func TestSignData(t *testing.T) {
 function ApproveSignData(r){
     if( r.address.toLowerCase() == "0x694267f14675d7e1b9494fd8d72fefe1755710fa")
     {
-        if(r.message[0].value.indexOf("bazonk") >= 0){
+        if(r.messages[0].value.indexOf("bazonk") >= 0){
             return "Approve"
         }
         return "Reject"
@@ -615,11 +612,11 @@ function ApproveSignData(r){
 		},
 	}
 	resp, err := r.ApproveSignData(&core.SignDataRequest{
-		Address: *addr,
-		Message: nvt,
-		Hash:    hash,
-		Meta:    core.Metadata{Remote: "remoteip", Local: "localip", Scheme: "inproc"},
-		Rawdata: []byte(rawdata),
+		Address:  *addr,
+		Messages: nvt,
+		Hash:     hash,
+		Meta:     core.Metadata{Remote: "remoteip", Local: "localip", Scheme: "inproc"},
+		Rawdata:  []byte(rawdata),
 	})
 	if err != nil {
 		t.Fatalf("Unexpected error %v", err)
diff --git a/signer/storage/aes_gcm_storage.go b/signer/storage/aes_gcm_storage.go
index 900831867..e6a8f145c 100644
--- a/signer/storage/aes_gcm_storage.go
+++ b/signer/storage/aes_gcm_storage.go
@@ -53,7 +53,7 @@ func NewAESEncryptedStorage(filename string, key []byte) *AESEncryptedStorage {
 	}
 }
 
-// Put stores a value by key. 0-length keys results in no-op
+// Put stores a value by key. 0-length keys results in noop.
 func (s *AESEncryptedStorage) Put(key, value string) {
 	if len(key) == 0 {
 		return
@@ -75,27 +75,41 @@ func (s *AESEncryptedStorage) Put(key, value string) {
 	}
 }
 
-// Get returns the previously stored value, or the empty string if it does not exist or key is of 0-length
-func (s *AESEncryptedStorage) Get(key string) string {
+// Get returns the previously stored value, or an error if it does not exist or
+// key is of 0-length.
+func (s *AESEncryptedStorage) Get(key string) (string, error) {
 	if len(key) == 0 {
-		return ""
+		return "", ErrZeroKey
 	}
 	data, err := s.readEncryptedStorage()
 	if err != nil {
 		log.Warn("Failed to read encrypted storage", "err", err, "file", s.filename)
-		return ""
+		return "", err
 	}
 	encrypted, exist := data[key]
 	if !exist {
 		log.Warn("Key does not exist", "key", key)
-		return ""
+		return "", ErrNotFound
 	}
 	entry, err := decrypt(s.key, encrypted.Iv, encrypted.CipherText, []byte(key))
 	if err != nil {
 		log.Warn("Failed to decrypt key", "key", key)
-		return ""
+		return "", err
+	}
+	return string(entry), nil
+}
+
+// Del removes a key-value pair. If the key doesn't exist, the method is a noop.
+func (s *AESEncryptedStorage) Del(key string) {
+	data, err := s.readEncryptedStorage()
+	if err != nil {
+		log.Warn("Failed to read encrypted storage", "err", err, "file", s.filename)
+		return
+	}
+	delete(data, key)
+	if err = s.writeEncryptedStorage(data); err != nil {
+		log.Warn("Failed to write entry", "err", err)
 	}
-	return string(entry)
 }
 
 // readEncryptedStorage reads the file with encrypted creds
diff --git a/signer/storage/aes_gcm_storage_test.go b/signer/storage/aes_gcm_storage_test.go
index a421a8449..0aaaf58d2 100644
--- a/signer/storage/aes_gcm_storage_test.go
+++ b/signer/storage/aes_gcm_storage_test.go
@@ -110,8 +110,8 @@ func TestEnd2End(t *testing.T) {
 	}
 
 	s1.Put("bazonk", "foobar")
-	if v := s2.Get("bazonk"); v != "foobar" {
-		t.Errorf("Expected bazonk->foobar, got '%v'", v)
+	if v, err := s2.Get("bazonk"); v != "foobar" || err != nil {
+		t.Errorf("Expected bazonk->foobar (nil error), got '%v' (%v error)", v, err)
 	}
 }
 
@@ -154,11 +154,11 @@ func TestSwappedKeys(t *testing.T) {
 		}
 	}
 	swap()
-	if v := s1.Get("k1"); v != "" {
+	if v, _ := s1.Get("k1"); v != "" {
 		t.Errorf("swapped value should return empty")
 	}
 	swap()
-	if v := s1.Get("k1"); v != "v1" {
+	if v, _ := s1.Get("k1"); v != "v1" {
 		t.Errorf("double-swapped value should work fine")
 	}
 }
diff --git a/signer/storage/storage.go b/signer/storage/storage.go
index 50c55e455..c1f593d96 100644
--- a/signer/storage/storage.go
+++ b/signer/storage/storage.go
@@ -17,11 +17,26 @@
 
 package storage
 
+import "errors"
+
+var (
+	// ErrZeroKey is returned if an attempt was made to inset a 0-length key.
+	ErrZeroKey = errors.New("0-length key")
+
+	// ErrNotFound is returned if an unknown key is attempted to be retrieved.
+	ErrNotFound = errors.New("not found")
+)
+
 type Storage interface {
-	// Put stores a value by key. 0-length keys results in no-op
+	// Put stores a value by key. 0-length keys results in noop.
 	Put(key, value string)
-	// Get returns the previously stored value, or the empty string if it does not exist or key is of 0-length
-	Get(key string) string
+
+	// Get returns the previously stored value, or an error if the key is 0-length
+	// or unknown.
+	Get(key string) (string, error)
+
+	// Del removes a key-value pair. If the key doesn't exist, the method is a noop.
+	Del(key string)
 }
 
 // EphemeralStorage is an in-memory storage that does
@@ -31,23 +46,29 @@ type EphemeralStorage struct {
 	namespace string
 }
 
+// Put stores a value by key. 0-length keys results in noop.
 func (s *EphemeralStorage) Put(key, value string) {
 	if len(key) == 0 {
 		return
 	}
-	//fmt.Printf("storage: put %v -> %v\n", key, value)
 	s.data[key] = value
 }
 
-func (s *EphemeralStorage) Get(key string) string {
+// Get returns the previously stored value, or an error if the key is 0-length
+// or unknown.
+func (s *EphemeralStorage) Get(key string) (string, error) {
 	if len(key) == 0 {
-		return ""
+		return "", ErrZeroKey
 	}
-	//fmt.Printf("storage: get %v\n", key)
-	if v, exist := s.data[key]; exist {
-		return v
+	if v, ok := s.data[key]; ok {
+		return v, nil
 	}
-	return ""
+	return "", ErrNotFound
+}
+
+// Del removes a key-value pair. If the key doesn't exist, the method is a noop.
+func (s *EphemeralStorage) Del(key string) {
+	delete(s.data, key)
 }
 
 func NewEphemeralStorage() Storage {
@@ -61,6 +82,7 @@ func NewEphemeralStorage() Storage {
 type NoStorage struct{}
 
 func (s *NoStorage) Put(key, value string) {}
-func (s *NoStorage) Get(key string) string {
-	return ""
+func (s *NoStorage) Del(key string)        {}
+func (s *NoStorage) Get(key string) (string, error) {
+	return "", errors.New("I forgot")
 }