diff --git a/cmd/clef/datatypes.md b/cmd/clef/datatypes.md index e345a2bbd5..a26c1c9375 100644 --- a/cmd/clef/datatypes.md +++ b/cmd/clef/datatypes.md @@ -35,8 +35,7 @@ Response to SignDataRequest Example: ```json { - "approved": true, - "Password": "apassword" + "approved": true } ``` ### SignDataResponse - deny @@ -46,8 +45,7 @@ Response to SignDataRequest Example: ```json { - "approved": false, - "Password": "" + "approved": false } ``` ### SignTxRequest @@ -89,9 +87,9 @@ Example: } } ``` -### SignDataResponse - approve +### SignTxResponse - approve -Response to SignDataRequest. This response needs to contain the `transaction`, because the UI is free to make modifications to the transaction. +Response to request to sign a transaction. This response needs to contain the `transaction`, because the UI is free to make modifications to the transaction. Example: ```json @@ -105,19 +103,26 @@ Example: "nonce": "0x4", "data": "0x04030201" }, - "approved": true, - "password": "apassword" + "approved": true } ``` -### SignDataResponse - deny +### SignTxResponse - deny -Response to SignDataRequest. When denying a request, there's no need to provide the transaction in return +Response to SignTxRequest. When denying a request, there's no need to provide the transaction in return Example: ```json { - "approved": false, - "Password": "" + "transaction": { + "from": "0x", + "to": null, + "gas": "0x0", + "gasPrice": "0x0", + "value": "0x0", + "nonce": "0x0", + "data": null + }, + "approved": false } ``` ### OnApproved - SignTransactionResult @@ -164,7 +169,7 @@ Example: ``` ### UserInputResponse -Response to SignDataRequest +Response to UserInputRequest Example: ```json @@ -198,7 +203,7 @@ Example: } } ``` -### UserInputResponse +### ListResponse Response to list request. The response contains a list of all addresses to show to the caller. Note: the UI is free to respond with any address the caller, regardless of whether it exists or not diff --git a/cmd/clef/main.go b/cmd/clef/main.go index 088701eeee..dcc33d6d83 100644 --- a/cmd/clef/main.go +++ b/cmd/clef/main.go @@ -119,7 +119,7 @@ var ( ruleFlag = cli.StringFlag{ Name: "rules", Usage: "Enable rule-engine", - Value: "rules.json", + Value: "", } stdiouiFlag = cli.BoolFlag{ Name: "stdio-ui", @@ -371,17 +371,14 @@ func signer(c *cli.Context) error { log.Info("Loaded 4byte db", "signatures", db.Size(), "file", fourByteDb, "local", fourByteLocal) var ( - api core.ExternalAPI + api core.ExternalAPI + pwStorage storage.Storage = &storage.NoStorage{} ) configDir := c.GlobalString(configdirFlag.Name) if stretchedKey, err := readMasterKey(c, ui); err != nil { log.Info("No master seed provided, rules disabled", "error", err) } else { - - if err != nil { - utils.Fatalf(err.Error()) - } vaultLocation := filepath.Join(configDir, common.Bytes2Hex(crypto.Keccak256([]byte("vault"), stretchedKey)[:10])) // Generate domain specific keys @@ -390,30 +387,31 @@ func signer(c *cli.Context) error { confkey := crypto.Keccak256([]byte("config"), stretchedKey) // Initialize the encrypted storages - pwStorage := storage.NewAESEncryptedStorage(filepath.Join(vaultLocation, "credentials.json"), pwkey) + pwStorage = storage.NewAESEncryptedStorage(filepath.Join(vaultLocation, "credentials.json"), pwkey) jsStorage := storage.NewAESEncryptedStorage(filepath.Join(vaultLocation, "jsstorage.json"), jskey) configStorage := storage.NewAESEncryptedStorage(filepath.Join(vaultLocation, "config.json"), confkey) //Do we have a rule-file? - ruleJS, err := ioutil.ReadFile(c.GlobalString(ruleFlag.Name)) - if err != nil { - log.Info("Could not load rulefile, rules not enabled", "file", "rulefile") - } else { - hasher := sha256.New() - hasher.Write(ruleJS) - shasum := hasher.Sum(nil) - storedShasum := configStorage.Get("ruleset_sha256") - if storedShasum != hex.EncodeToString(shasum) { - log.Info("Could not validate ruleset hash, rules not enabled", "got", hex.EncodeToString(shasum), "expected", storedShasum) + if ruleFile := c.GlobalString(ruleFlag.Name); ruleFile != "" { + ruleJS, err := ioutil.ReadFile(c.GlobalString(ruleFile)) + if err != nil { + log.Info("Could not load rulefile, rules not enabled", "file", "rulefile") } else { - // Initialize rules - ruleEngine, err := rules.NewRuleEvaluator(ui, jsStorage, pwStorage) - if err != nil { - utils.Fatalf(err.Error()) + shasum := sha256.Sum256(ruleJS) + foundShaSum := hex.EncodeToString(shasum[:]) + storedShasum := configStorage.Get("ruleset_sha256") + if storedShasum != foundShaSum { + log.Info("Could not validate ruleset hash, rules not enabled", "got", foundShaSum, "expected", storedShasum) + } else { + // Initialize rules + ruleEngine, err := rules.NewRuleEvaluator(ui, jsStorage) + if err != nil { + utils.Fatalf(err.Error()) + } + ruleEngine.Init(string(ruleJS)) + ui = ruleEngine + log.Info("Rule engine configured", "file", c.String(ruleFlag.Name)) } - ruleEngine.Init(string(ruleJS)) - ui = ruleEngine - log.Info("Rule engine configured", "file", c.String(ruleFlag.Name)) } } } @@ -427,7 +425,7 @@ func signer(c *cli.Context) error { log.Info("Starting signer", "chainid", chainId, "keystore", ksLoc, "light-kdf", lightKdf, "advanced", advanced) am := core.StartClefAccountManager(ksLoc, nousb, lightKdf) - apiImpl := core.NewSignerAPI(am, chainId, nousb, ui, db, advanced) + apiImpl := core.NewSignerAPI(am, chainId, nousb, ui, db, advanced, pwStorage) // Establish the bidirectional communication, by creating a new UI backend and registering // it with the UI. @@ -798,7 +796,7 @@ func GenDoc(ctx *cli.Context) { } { // Sign plain text response add("SignDataResponse - approve", "Response to SignDataRequest", - &core.SignDataResponse{Password: "apassword", Approved: true}) + &core.SignDataResponse{Approved: true}) add("SignDataResponse - deny", "Response to SignDataRequest", &core.SignDataResponse{}) } @@ -833,9 +831,9 @@ func GenDoc(ctx *cli.Context) { } { // Sign tx response data := hexutil.Bytes([]byte{0x04, 0x03, 0x02, 0x01}) - add("SignDataResponse - approve", "Response to SignDataRequest. This response needs to contain the `transaction`"+ + add("SignTxResponse - approve", "Response to request to sign a transaction. This response needs to contain the `transaction`"+ ", because the UI is free to make modifications to the transaction.", - &core.SignTxResponse{Password: "apassword", Approved: true, + &core.SignTxResponse{Approved: true, Transaction: core.SendTxArgs{ Data: &data, Nonce: 0x4, @@ -846,9 +844,9 @@ func GenDoc(ctx *cli.Context) { Gas: 1000, Input: nil, }}) - add("SignDataResponse - deny", "Response to SignDataRequest. When denying a request, there's no need to "+ + add("SignTxResponse - deny", "Response to SignTxRequest. When denying a request, there's no need to "+ "provide the transaction in return", - &core.SignDataResponse{}) + &core.SignTxResponse{}) } { // WHen a signed tx is ready to go out desc := "SignTransactionResult is used in the call `clef` -> `OnApprovedTx(result)`" + @@ -874,7 +872,7 @@ func GenDoc(ctx *cli.Context) { { // User input add("UserInputRequest", "Sent when clef needs the user to provide data. If 'password' is true, the input field should be treated accordingly (echo-free)", &core.UserInputRequest{IsPassword: true, Title: "The title here", Prompt: "The question to ask the user"}) - add("UserInputResponse", "Response to SignDataRequest", + add("UserInputResponse", "Response to UserInputRequest", &core.UserInputResponse{Text: "The textual response from user"}) } { // List request @@ -888,7 +886,7 @@ func GenDoc(ctx *cli.Context) { {b, accounts.URL{Scheme: "keystore", Path: "/path/to/keyfile/b"}}}, }) - add("UserInputResponse", "Response to list request. The response contains a list of all addresses to show to the caller. "+ + add("ListResponse", "Response to list request. The response contains a list of all addresses to show to the caller. "+ "Note: the UI is free to respond with any address the caller, regardless of whether it exists or not", &core.ListResponse{ Accounts: []accounts.Account{ diff --git a/signer/core/api.go b/signer/core/api.go index 7020bedaa1..a489e1f0f7 100644 --- a/signer/core/api.go +++ b/signer/core/api.go @@ -21,8 +21,10 @@ import ( "encoding/json" "errors" "fmt" + "github.com/ethereum/go-ethereum/signer/storage" "math/big" "reflect" + "strings" "github.com/ethereum/go-ethereum/accounts" "github.com/ethereum/go-ethereum/accounts/keystore" @@ -92,11 +94,12 @@ type UIClientAPI interface { // SignerAPI defines the actual implementation of ExternalAPI type SignerAPI struct { - chainID *big.Int - am *accounts.Manager - UI UIClientAPI - validator *Validator - rejectMode bool + chainID *big.Int + am *accounts.Manager + UI UIClientAPI + validator *Validator + rejectMode bool + credentials storage.Storage } // Metadata about a request @@ -231,11 +234,11 @@ var ErrRequestDenied = errors.New("Request denied") // key that is generated when a new Account is created. // noUSB disables USB support that is required to support hardware devices such as // ledger and trezor. -func NewSignerAPI(am *accounts.Manager, chainID int64, noUSB bool, ui UIClientAPI, abidb *AbiDb, advancedMode bool) *SignerAPI { +func NewSignerAPI(am *accounts.Manager, chainID int64, noUSB bool, ui UIClientAPI, abidb *AbiDb, advancedMode bool, credentials storage.Storage) *SignerAPI { if advancedMode { log.Info("Clef is in advanced mode: will warn instead of reject") } - signer := &SignerAPI{big.NewInt(chainID), am, ui, NewValidator(abidb), !advancedMode} + signer := &SignerAPI{big.NewInt(chainID), am, ui, NewValidator(abidb), !advancedMode, credentials} if !noUSB { signer.startUSBListener() } @@ -356,24 +359,27 @@ func (api *SignerAPI) New(ctx context.Context) (common.Address, error) { if len(be) == 0 { return common.Address{}, errors.New("password based accounts not supported") } - var ( - resp NewAccountResponse - err error - ) + if resp, err := api.UI.ApproveNewAccount(&NewAccountRequest{MetadataFromContext(ctx)}); err != nil { + return common.Address{}, err + } else if !resp.Approved { + return common.Address{}, ErrRequestDenied + } + // Three retries to get a valid password for i := 0; i < 3; i++ { - resp, err = api.UI.ApproveNewAccount(&NewAccountRequest{MetadataFromContext(ctx)}) + resp, err := api.UI.OnInputRequired(UserInputRequest{ + "New account password", + fmt.Sprintf("Please enter a password for the new account to be created (attempt %d of 3)", i), + true}) if err != nil { - return common.Address{}, err + log.Warn("error obtaining password", "attempt", i, "error", err) + continue } - if !resp.Approved { - return common.Address{}, ErrRequestDenied - } - if pwErr := ValidatePasswordFormat(resp.Password); pwErr != nil { + if pwErr := ValidatePasswordFormat(resp.Text); pwErr != nil { api.UI.ShowError(fmt.Sprintf("Account creation attempt #%d failed due to password requirements: %v", (i + 1), pwErr)) } else { // No error - acc, err := be[0].(*keystore.KeyStore).NewAccount(resp.Password) + acc, err := be[0].(*keystore.KeyStore).NewAccount(resp.Text) return acc.Address, err } } @@ -427,6 +433,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) lookupOrQueryPassword(address common.Address, title, prompt string) (string, error) { + if pw := api.lookupPassword(address); pw != "" { + 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 + } +} + // SignTransaction signs the given Transaction and returns it both as json and rlp-encoded form func (api *SignerAPI) SignTransaction(ctx context.Context, args SendTxArgs, methodSelector *string) (*ethapi.SignTransactionResult, error) { var ( @@ -470,9 +494,14 @@ func (api *SignerAPI) SignTransaction(ctx context.Context, args SendTxArgs, meth } // Convert fields into a real transaction var unsignedTx = result.Transaction.toTransaction() - + // Get the password for the transaction + pw, err := api.lookupOrQueryPassword(acc.Address, "Account password", + fmt.Sprintf("Please enter the password for account %s", acc.Address.String())) + if err != nil { + return nil, err + } // The one to sign is the one that was returned from the UI - signedTx, err := wallet.SignTxWithPassphrase(acc, result.Password, unsignedTx, api.chainID) + signedTx, err := wallet.SignTxWithPassphrase(acc, pw, unsignedTx, api.chainID) if err != nil { api.UI.ShowError(err.Error()) return nil, err diff --git a/signer/core/api_test.go b/signer/core/api_test.go index 2b9b2f5f9d..f14e24f41c 100644 --- a/signer/core/api_test.go +++ b/signer/core/api_test.go @@ -19,8 +19,8 @@ package core import ( "bytes" "context" - "errors" "fmt" + "github.com/ethereum/go-ethereum/signer/storage" "io/ioutil" "math/big" "os" @@ -38,47 +38,45 @@ import ( ) //Used for testing -type HeadlessUI struct { - controller chan string +type headlessUi struct { + approveCh chan string // to send approve/deny + inputCh chan string // to send password } -func (ui *HeadlessUI) OnInputRequired(info UserInputRequest) (UserInputResponse, error) { - return UserInputResponse{}, errors.New("not implemented") +func (ui *headlessUi) OnInputRequired(info UserInputRequest) (UserInputResponse, error) { + input := <-ui.inputCh + return UserInputResponse{Text: input}, nil } -func (ui *HeadlessUI) OnSignerStartup(info StartupInfo) { -} -func (ui *HeadlessUI) RegisterUIServer(api *UIServerAPI) { -} +func (ui *headlessUi) OnSignerStartup(info StartupInfo) {} +func (ui *headlessUi) RegisterUIServer(api *UIServerAPI) {} +func (ui *headlessUi) OnApprovedTx(tx ethapi.SignTransactionResult) {} -func (ui *HeadlessUI) OnApprovedTx(tx ethapi.SignTransactionResult) { - fmt.Printf("OnApproved()\n") -} +func (ui *headlessUi) ApproveTx(request *SignTxRequest) (SignTxResponse, error) { -func (ui *HeadlessUI) ApproveTx(request *SignTxRequest) (SignTxResponse, error) { - - switch <-ui.controller { + switch <-ui.approveCh { case "Y": - return SignTxResponse{request.Transaction, true, <-ui.controller}, nil - case "M": //Modify + return SignTxResponse{request.Transaction, true}, nil + case "M": // modify + // The headless UI always modifies the transaction old := big.Int(request.Transaction.Value) newVal := big.NewInt(0).Add(&old, big.NewInt(1)) request.Transaction.Value = hexutil.Big(*newVal) - return SignTxResponse{request.Transaction, true, <-ui.controller}, nil + return SignTxResponse{request.Transaction, true}, nil default: - return SignTxResponse{request.Transaction, false, ""}, nil + return SignTxResponse{request.Transaction, false}, nil } } -func (ui *HeadlessUI) ApproveSignData(request *SignDataRequest) (SignDataResponse, error) { - if "Y" == <-ui.controller { - return SignDataResponse{true, <-ui.controller}, nil - } - return SignDataResponse{false, ""}, nil +func (ui *headlessUi) ApproveSignData(request *SignDataRequest) (SignDataResponse, error) { + approved := "Y" == <-ui.approveCh + return SignDataResponse{approved}, nil } -func (ui *HeadlessUI) ApproveListing(request *ListRequest) (ListResponse, error) { - switch <-ui.controller { +func (ui *headlessUi) ApproveListing(request *ListRequest) (ListResponse, error) { + approval := <-ui.approveCh + //fmt.Printf("approval %s\n", approval) + switch approval { case "A": return ListResponse{request.Accounts}, nil case "1": @@ -90,19 +88,19 @@ func (ui *HeadlessUI) ApproveListing(request *ListRequest) (ListResponse, error) } } -func (ui *HeadlessUI) ApproveNewAccount(request *NewAccountRequest) (NewAccountResponse, error) { - if "Y" == <-ui.controller { - return NewAccountResponse{true, <-ui.controller}, nil +func (ui *headlessUi) ApproveNewAccount(request *NewAccountRequest) (NewAccountResponse, error) { + if "Y" == <-ui.approveCh { + return NewAccountResponse{true}, nil } - return NewAccountResponse{false, ""}, nil + return NewAccountResponse{false}, nil } -func (ui *HeadlessUI) ShowError(message string) { +func (ui *headlessUi) ShowError(message string) { //stdout is used by communication fmt.Fprintln(os.Stderr, message) } -func (ui *HeadlessUI) ShowInfo(message string) { +func (ui *headlessUi) ShowInfo(message string) { //stdout is used by communication fmt.Fprintln(os.Stderr, message) } @@ -119,25 +117,20 @@ func tmpDirName(t *testing.T) string { return d } -func setup(t *testing.T) (*SignerAPI, chan string) { - - controller := make(chan string, 20) - +func setup(t *testing.T) (*SignerAPI, *headlessUi) { db, err := NewAbiDBFromFile("../../cmd/clef/4byte.json") if err != nil { t.Fatal(err.Error()) } - var ( - ui = &HeadlessUI{controller} - am = StartClefAccountManager(tmpDirName(t), true, true) - api = NewSignerAPI(am, 1337, true, ui, db, true) - ) - return api, controller -} -func createAccount(control chan string, api *SignerAPI, t *testing.T) { + ui := &headlessUi{make(chan string, 20), make(chan string, 20)} + am := StartClefAccountManager(tmpDirName(t), true, true) + api := NewSignerAPI(am, 1337, true, ui, db, true, &storage.NoStorage{}) + return api, ui - control <- "Y" - control <- "a_long_password" +} +func createAccount(ui *headlessUi, api *SignerAPI, t *testing.T) { + ui.approveCh <- "Y" + ui.inputCh <- "a_long_password" _, err := api.New(context.Background()) if err != nil { t.Fatal(err) @@ -146,14 +139,13 @@ func createAccount(control chan string, api *SignerAPI, t *testing.T) { time.Sleep(250 * time.Millisecond) } -func failCreateAccountWithPassword(control chan string, api *SignerAPI, password string, t *testing.T) { +func failCreateAccountWithPassword(ui *headlessUi, api *SignerAPI, password string, t *testing.T) { - control <- "Y" - control <- password - control <- "Y" - control <- password - control <- "Y" - control <- password + ui.approveCh <- "Y" + // We will be asked three times to provide a suitable password + ui.inputCh <- password + ui.inputCh <- password + ui.inputCh <- password addr, err := api.New(context.Background()) if err == nil { @@ -164,8 +156,8 @@ func failCreateAccountWithPassword(control chan string, api *SignerAPI, password } } -func failCreateAccount(control chan string, api *SignerAPI, t *testing.T) { - control <- "N" +func failCreateAccount(ui *headlessUi, api *SignerAPI, t *testing.T) { + ui.approveCh <- "N" addr, err := api.New(context.Background()) if err != ErrRequestDenied { t.Fatal(err) @@ -175,19 +167,20 @@ func failCreateAccount(control chan string, api *SignerAPI, t *testing.T) { } } -func list(control chan string, api *SignerAPI, t *testing.T) []common.Address { - control <- "A" - list, err := api.List(context.Background()) - if err != nil { - t.Fatal(err) - } - return list +func list(ui *headlessUi, api *SignerAPI, t *testing.T) ([]common.Address, error) { + ui.approveCh <- "A" + return api.List(context.Background()) + } func TestNewAcc(t *testing.T) { api, control := setup(t) verifyNum := func(num int) { - if list := list(control, api, t); len(list) != num { + list, err := list(control, api, t) + if err != nil { + t.Errorf("Unexpected error %v", err) + } + if len(list) != num { t.Errorf("Expected %d accounts, got %d", num, len(list)) } } @@ -200,18 +193,16 @@ func TestNewAcc(t *testing.T) { failCreateAccount(control, api, t) createAccount(control, api, t) failCreateAccount(control, api, t) - verifyNum(4) // Fail to create this, due to bad password failCreateAccountWithPassword(control, api, "short", t) failCreateAccountWithPassword(control, api, "longerbutbad\rfoo", t) - verifyNum(4) // Testing listing: // Listing one Account - control <- "1" + control.approveCh <- "1" list, err := api.List(context.Background()) if err != nil { t.Fatal(err) @@ -220,7 +211,7 @@ func TestNewAcc(t *testing.T) { t.Fatalf("List should only show one Account") } // Listing denied - control <- "Nope" + control.approveCh <- "Nope" list, err = api.List(context.Background()) if len(list) != 0 { t.Fatalf("List should be empty") @@ -257,7 +248,7 @@ func TestSignTx(t *testing.T) { api, control := setup(t) createAccount(control, api, t) - control <- "A" + control.approveCh <- "A" list, err = api.List(context.Background()) if err != nil { t.Fatal(err) @@ -267,8 +258,8 @@ func TestSignTx(t *testing.T) { methodSig := "test(uint)" tx := mkTestTx(a) - control <- "Y" - control <- "wrongpassword" + control.approveCh <- "Y" + control.inputCh <- "wrongpassword" res, err = api.SignTransaction(context.Background(), tx, &methodSig) if res != nil { t.Errorf("Expected nil-response, got %v", res) @@ -276,7 +267,7 @@ func TestSignTx(t *testing.T) { if err != keystore.ErrDecrypt { t.Errorf("Expected ErrLocked! %v", err) } - control <- "No way" + control.approveCh <- "No way" res, err = api.SignTransaction(context.Background(), tx, &methodSig) if res != nil { t.Errorf("Expected nil-response, got %v", res) @@ -284,8 +275,9 @@ func TestSignTx(t *testing.T) { if err != ErrRequestDenied { t.Errorf("Expected ErrRequestDenied! %v", err) } - control <- "Y" - control <- "a_long_password" + // Sign with correct password + control.approveCh <- "Y" + control.inputCh <- "a_long_password" res, err = api.SignTransaction(context.Background(), tx, &methodSig) if err != nil { @@ -298,8 +290,8 @@ func TestSignTx(t *testing.T) { if parsedTx.Value().Cmp(tx.Value.ToInt()) != 0 { t.Errorf("Expected value to be unchanged, expected %v got %v", tx.Value, parsedTx.Value()) } - control <- "Y" - control <- "a_long_password" + control.approveCh <- "Y" + control.inputCh <- "a_long_password" res2, err = api.SignTransaction(context.Background(), tx, &methodSig) if err != nil { @@ -310,8 +302,8 @@ func TestSignTx(t *testing.T) { } //The tx is modified by the UI - control <- "M" - control <- "a_long_password" + control.approveCh <- "M" + control.inputCh <- "a_long_password" res2, err = api.SignTransaction(context.Background(), tx, &methodSig) if err != nil { @@ -329,31 +321,3 @@ func TestSignTx(t *testing.T) { } } - -/* -func TestAsyncronousResponses(t *testing.T){ - - //Set up one account - api, control := setup(t) - createAccount(control, api, t) - - // Two transactions, the second one with larger value than the first - tx1 := mkTestTx() - newVal := big.NewInt(0).Add((*big.Int) (tx1.Value), big.NewInt(1)) - tx2 := mkTestTx() - tx2.Value = (*hexutil.Big)(newVal) - - control <- "W" //wait - control <- "Y" // - control <- "a_long_password" - control <- "Y" // - control <- "a_long_password" - - var err error - - h1, err := api.SignTransaction(context.Background(), common.HexToAddress("1111"), tx1, nil) - h2, err := api.SignTransaction(context.Background(), common.HexToAddress("2222"), tx2, nil) - - - } -*/ diff --git a/signer/core/cliui.go b/signer/core/cliui.go index 109fdd9a19..ad65246d9f 100644 --- a/signer/core/cliui.go +++ b/signer/core/cliui.go @@ -156,9 +156,9 @@ func (ui *CommandlineUI) ApproveTx(request *SignTxRequest) (SignTxResponse, erro showMetadata(request.Meta) fmt.Printf("-------------------------------------------\n") if !ui.confirm() { - return SignTxResponse{request.Transaction, false, ""}, nil + return SignTxResponse{request.Transaction, false}, nil } - return SignTxResponse{request.Transaction, true, ui.readPassword()}, nil + return SignTxResponse{request.Transaction, true}, nil } // ApproveSignData prompt the user for confirmation to request to sign data @@ -178,9 +178,9 @@ func (ui *CommandlineUI) ApproveSignData(request *SignDataRequest) (SignDataResp fmt.Printf("-------------------------------------------\n") showMetadata(request.Meta) if !ui.confirm() { - return SignDataResponse{false, ""}, nil + return SignDataResponse{false}, nil } - return SignDataResponse{true, ui.readPassword()}, nil + return SignDataResponse{true}, nil } // ApproveListing prompt the user for confirmation to list accounts @@ -217,9 +217,9 @@ func (ui *CommandlineUI) ApproveNewAccount(request *NewAccountRequest) (NewAccou fmt.Printf("and the address is returned to the external caller\n\n") showMetadata(request.Meta) if !ui.confirm() { - return NewAccountResponse{false, ""}, nil + return NewAccountResponse{false}, nil } - return NewAccountResponse{true, ui.readPassword()}, nil + return NewAccountResponse{true}, nil } // ShowError displays error message to user diff --git a/signer/core/signed_data.go b/signer/core/signed_data.go index 9df39ef59a..475a8837e2 100644 --- a/signer/core/signed_data.go +++ b/signer/core/signed_data.go @@ -139,8 +139,14 @@ func (api *SignerAPI) sign(addr common.MixedcaseAddress, req *SignDataRequest, l if err != nil { return nil, err } + pw, err := api.lookupOrQueryPassword(account.Address, + "Password for signing", + fmt.Sprintf("Please enter password for signing data with account %s", account.Address.Hex())) + if err != nil { + return nil, err + } // Sign the data with the wallet - signature, err := wallet.SignDataWithPassphrase(account, res.Password, req.ContentType, req.Rawdata) + signature, err := wallet.SignDataWithPassphrase(account, pw, req.ContentType, req.Rawdata) if err != nil { return nil, err } diff --git a/signer/core/signed_data_test.go b/signer/core/signed_data_test.go index 7d44bce2ce..76e1b72ee2 100644 --- a/signer/core/signed_data_test.go +++ b/signer/core/signed_data_test.go @@ -179,15 +179,15 @@ func TestSignData(t *testing.T) { //Create two accounts createAccount(control, api, t) createAccount(control, api, t) - control <- "1" + control.approveCh <- "1" list, err := api.List(context.Background()) if err != nil { t.Fatal(err) } a := common.NewMixedcaseAddress(list[0]) - control <- "Y" - control <- "wrongpassword" + control.approveCh <- "Y" + control.inputCh <- "wrongpassword" signature, err := api.SignData(context.Background(), TextPlain.Mime, a, hexutil.Encode([]byte("EHLO world"))) if signature != nil { t.Errorf("Expected nil-data, got %x", signature) @@ -195,7 +195,7 @@ func TestSignData(t *testing.T) { if err != keystore.ErrDecrypt { t.Errorf("Expected ErrLocked! '%v'", err) } - control <- "No way" + control.approveCh <- "No way" signature, err = api.SignData(context.Background(), TextPlain.Mime, a, hexutil.Encode([]byte("EHLO world"))) if signature != nil { t.Errorf("Expected nil-data, got %x", signature) @@ -204,8 +204,8 @@ func TestSignData(t *testing.T) { t.Errorf("Expected ErrRequestDenied! '%v'", err) } // text/plain - control <- "Y" - control <- "a_long_password" + control.approveCh <- "Y" + control.inputCh <- "a_long_password" signature, err = api.SignData(context.Background(), TextPlain.Mime, a, hexutil.Encode([]byte("EHLO world"))) if err != nil { t.Fatal(err) @@ -214,8 +214,8 @@ func TestSignData(t *testing.T) { t.Errorf("Expected 65 byte signature (got %d bytes)", len(signature)) } // data/typed - control <- "Y" - control <- "a_long_password" + control.approveCh <- "Y" + control.inputCh <- "a_long_password" signature, err = api.SignTypedData(context.Background(), a, typedData) if err != nil { t.Fatal(err) diff --git a/signer/rules/rules.go b/signer/rules/rules.go index 7ddf718034..5ebffa3af6 100644 --- a/signer/rules/rules.go +++ b/signer/rules/rules.go @@ -22,7 +22,6 @@ import ( "os" "strings" - "github.com/ethereum/go-ethereum/common" "github.com/ethereum/go-ethereum/internal/ethapi" "github.com/ethereum/go-ethereum/log" "github.com/ethereum/go-ethereum/signer/core" @@ -42,25 +41,23 @@ func consoleOutput(call otto.FunctionCall) otto.Value { for _, argument := range call.ArgumentList { output = append(output, fmt.Sprintf("%v", argument)) } - fmt.Fprintln(os.Stdout, strings.Join(output, " ")) + fmt.Fprintln(os.Stderr, strings.Join(output, " ")) return otto.Value{} } // rulesetUI provides an implementation of UIClientAPI that evaluates a javascript // file for each defined UI-method type rulesetUI struct { - next core.UIClientAPI // The next handler, for manual processing - storage storage.Storage - credentials storage.Storage - jsRules string // The rules to use + next core.UIClientAPI // The next handler, for manual processing + storage storage.Storage + jsRules string // The rules to use } -func NewRuleEvaluator(next core.UIClientAPI, jsbackend, credentialsBackend storage.Storage) (*rulesetUI, error) { +func NewRuleEvaluator(next core.UIClientAPI, jsbackend storage.Storage) (*rulesetUI, error) { c := &rulesetUI{ - next: next, - storage: jsbackend, - credentials: credentialsBackend, - jsRules: "", + next: next, + storage: jsbackend, + jsRules: "", } return c, nil @@ -153,18 +150,12 @@ func (r *rulesetUI) ApproveTx(request *core.SignTxRequest) (core.SignTxResponse, if approved { return core.SignTxResponse{ Transaction: request.Transaction, - Approved: true, - Password: r.lookupPassword(request.Transaction.From.Address()), - }, + Approved: true}, nil } return core.SignTxResponse{Approved: false}, err } -func (r *rulesetUI) lookupPassword(address common.Address) string { - return r.credentials.Get(strings.ToLower(address.String())) -} - func (r *rulesetUI) ApproveSignData(request *core.SignDataRequest) (core.SignDataResponse, error) { jsonreq, err := json.Marshal(request) approved, err := r.checkApproval("ApproveSignData", jsonreq, err) @@ -173,9 +164,9 @@ func (r *rulesetUI) ApproveSignData(request *core.SignDataRequest) (core.SignDat return r.next.ApproveSignData(request) } if approved { - return core.SignDataResponse{Approved: true, Password: r.lookupPassword(request.Address.Address())}, nil + return core.SignDataResponse{Approved: true}, nil } - return core.SignDataResponse{Approved: false, Password: ""}, err + return core.SignDataResponse{Approved: false}, err } // OnInputRequired not handled by rules diff --git a/signer/rules/rules_test.go b/signer/rules/rules_test.go index 6b537d2041..19d9049c72 100644 --- a/signer/rules/rules_test.go +++ b/signer/rules/rules_test.go @@ -84,11 +84,11 @@ func (alwaysDenyUI) OnSignerStartup(info core.StartupInfo) { } func (alwaysDenyUI) ApproveTx(request *core.SignTxRequest) (core.SignTxResponse, error) { - return core.SignTxResponse{Transaction: request.Transaction, Approved: false, Password: ""}, nil + return core.SignTxResponse{Transaction: request.Transaction, Approved: false}, nil } func (alwaysDenyUI) ApproveSignData(request *core.SignDataRequest) (core.SignDataResponse, error) { - return core.SignDataResponse{Approved: false, Password: ""}, nil + return core.SignDataResponse{Approved: false}, nil } func (alwaysDenyUI) ApproveListing(request *core.ListRequest) (core.ListResponse, error) { @@ -96,7 +96,7 @@ func (alwaysDenyUI) ApproveListing(request *core.ListRequest) (core.ListResponse } func (alwaysDenyUI) ApproveNewAccount(request *core.NewAccountRequest) (core.NewAccountResponse, error) { - return core.NewAccountResponse{Approved: false, Password: ""}, nil + return core.NewAccountResponse{Approved: false}, nil } func (alwaysDenyUI) ShowError(message string) { @@ -112,7 +112,7 @@ func (alwaysDenyUI) OnApprovedTx(tx ethapi.SignTransactionResult) { } func initRuleEngine(js string) (*rulesetUI, error) { - r, err := NewRuleEvaluator(&alwaysDenyUI{}, storage.NewEphemeralStorage(), storage.NewEphemeralStorage()) + r, err := NewRuleEvaluator(&alwaysDenyUI{}, storage.NewEphemeralStorage()) if err != nil { return nil, fmt.Errorf("failed to create js engine: %v", err) } @@ -248,8 +248,7 @@ func TestForwarding(t *testing.T) { js := "" ui := &dummyUI{make([]string, 0)} jsBackend := storage.NewEphemeralStorage() - credBackend := storage.NewEphemeralStorage() - r, err := NewRuleEvaluator(ui, jsBackend, credBackend) + r, err := NewRuleEvaluator(ui, jsBackend) if err != nil { t.Fatalf("Failed to create js engine: %v", err) } @@ -567,7 +566,7 @@ func TestContextIsCleared(t *testing.T) { } ` ui := &dontCallMe{t} - r, err := NewRuleEvaluator(ui, storage.NewEphemeralStorage(), storage.NewEphemeralStorage()) + r, err := NewRuleEvaluator(ui, storage.NewEphemeralStorage()) if err != nil { t.Fatalf("Failed to create js engine: %v", err) } diff --git a/signer/storage/storage.go b/signer/storage/storage.go index 60f4e3892a..50c55e455f 100644 --- a/signer/storage/storage.go +++ b/signer/storage/storage.go @@ -17,10 +17,6 @@ package storage -import ( - "fmt" -) - type Storage interface { // Put stores a value by key. 0-length keys results in no-op Put(key, value string) @@ -39,7 +35,7 @@ func (s *EphemeralStorage) Put(key, value string) { if len(key) == 0 { return } - fmt.Printf("storage: put %v -> %v\n", key, value) + //fmt.Printf("storage: put %v -> %v\n", key, value) s.data[key] = value } @@ -47,7 +43,7 @@ func (s *EphemeralStorage) Get(key string) string { if len(key) == 0 { return "" } - fmt.Printf("storage: get %v\n", key) + //fmt.Printf("storage: get %v\n", key) if v, exist := s.data[key]; exist { return v } @@ -60,3 +56,11 @@ func NewEphemeralStorage() Storage { } return s } + +// NoStorage is a dummy construct which doesn't remember anything you tell it +type NoStorage struct{} + +func (s *NoStorage) Put(key, value string) {} +func (s *NoStorage) Get(key string) string { + return "" +}