From 5f94f8c7e7cfc6d037accfad379ce2ab0df6bf47 Mon Sep 17 00:00:00 2001 From: Martin Holst Swende Date: Thu, 7 Mar 2019 10:56:08 +0100 Subject: [PATCH] signer: change the stdio jsonrpc to use legacy namespace conventions (#19047) This PR will will break existing UIs, since it changes all calls like ApproveSignTransaction to be on the form ui_approveSignTransaction. This is to make it possible for the UI to reuse the json-rpc library from go-ethereum, which uses this convention. Also, this PR removes some unused structs, after import/export were removed from the external api (so no longer needs internal methods for approval) One more breaking change is introduced, removing passwords from the ApproveSignTxResponse and the likes. This makes the manual interface more like the rulebased interface, and integrates nicely with the credential storage. Thus, the way it worked before, it would be tempting for the UI to implement 'remember password' functionality. The way it is now, it will be easy instead to tell clef to store passwords and use them. If a pw is not found in the credential store, the user is prompted to provide the password. --- cmd/clef/README.md | 56 +++----- cmd/clef/datatypes.md | 33 +++-- cmd/clef/intapi_changelog.md | 35 +++++ cmd/clef/main.go | 228 ++++++++++++++++++++------------ signer/core/api.go | 100 +++++++------- signer/core/api_test.go | 186 ++++++++++---------------- signer/core/cliui.go | 53 ++------ signer/core/signed_data.go | 8 +- signer/core/signed_data_test.go | 16 +-- signer/core/stdioui.go | 40 ++---- signer/rules/rules.go | 50 ++----- signer/rules/rules_test.go | 45 +------ signer/storage/storage.go | 16 ++- 13 files changed, 399 insertions(+), 467 deletions(-) diff --git a/cmd/clef/README.md b/cmd/clef/README.md index 036e71b28..956957f3d 100644 --- a/cmd/clef/README.md +++ b/cmd/clef/README.md @@ -661,7 +661,7 @@ OBS! A slight deviation from `json` standard is in place: every request and resp Whereas the `json` specification allows for linebreaks, linebreaks __should not__ be used in this communication channel, to make things simpler for both parties. -### ApproveTx +### ApproveTx / `ui_approveTx` Invoked when there's a transaction for approval. @@ -673,13 +673,13 @@ Here's a method invocation: curl -i -H "Content-Type: application/json" -X POST --data '{"jsonrpc":"2.0","method":"account_signTransaction","params":[{"from":"0x694267f14675d7e1b9494fd8d72fefe1755710fa","gas":"0x333","gasPrice":"0x1","nonce":"0x0","to":"0x07a565b7ed7d7a678680a4c162885bedbb695fe0", "value":"0x0", "data":"0x4401a6e40000000000000000000000000000000000000000000000000000000000000012"},"safeSend(address)"],"id":67}' http://localhost:8550/ ``` - +Results in the following invocation on the UI: ```json { "jsonrpc": "2.0", "id": 1, - "method": "ApproveTx", + "method": "ui_approveTx", "params": [ { "transaction": { @@ -724,7 +724,7 @@ curl -i -H "Content-Type: application/json" -X POST --data '{"jsonrpc":"2.0","me { "jsonrpc": "2.0", "id": 1, - "method": "ApproveTx", + "method": "ui_approveTx", "params": [ { "transaction": { @@ -767,7 +767,7 @@ One which has missing `to`, but with no `data`: { "jsonrpc": "2.0", "id": 3, - "method": "ApproveTx", + "method": "ui_approveTx", "params": [ { "transaction": { @@ -796,33 +796,7 @@ One which has missing `to`, but with no `data`: } ``` -### ApproveExport - -Invoked when a request to export an account has been made. - -#### Sample call - -```json - -{ - "jsonrpc": "2.0", - "id": 7, - "method": "ApproveExport", - "params": [ - { - "address": "0x0000000000000000000000000000000000000000", - "meta": { - "remote": "signer binary", - "local": "main", - "scheme": "in-proc" - } - } - ] -} - -``` - -### ApproveListing +### ApproveListing / `ui_approveListing` Invoked when a request for account listing has been made. @@ -833,7 +807,7 @@ Invoked when a request for account listing has been made. { "jsonrpc": "2.0", "id": 5, - "method": "ApproveListing", + "method": "ui_approveListing", "params": [ { "accounts": [ @@ -860,7 +834,7 @@ Invoked when a request for account listing has been made. ``` -### ApproveSignData +### ApproveSignData / `ui_approveSignData` #### Sample call @@ -868,7 +842,7 @@ Invoked when a request for account listing has been made. { "jsonrpc": "2.0", "id": 4, - "method": "ApproveSignData", + "method": "ui_approveSignData", "params": [ { "address": "0x123409812340981234098123409812deadbeef42", @@ -886,7 +860,7 @@ Invoked when a request for account listing has been made. ``` -### ShowInfo +### ShowInfo / `ui_showInfo` The UI should show the info to the user. Does not expect response. @@ -896,7 +870,7 @@ The UI should show the info to the user. Does not expect response. { "jsonrpc": "2.0", "id": 9, - "method": "ShowInfo", + "method": "ui_showInfo", "params": [ { "text": "Tests completed" @@ -906,7 +880,7 @@ The UI should show the info to the user. Does not expect response. ``` -### ShowError +### ShowError / `ui_showError` The UI should show the info to the user. Does not expect response. @@ -925,7 +899,7 @@ The UI should show the info to the user. Does not expect response. ``` -### OnApproved +### OnApprovedTx / `ui_onApprovedTx` `OnApprovedTx` is called when a transaction has been approved and signed. The call contains the return value that will be sent to the external caller. The return value from this method is ignored - the reason for having this callback is to allow the ruleset to keep track of approved transactions. @@ -933,7 +907,7 @@ When implementing rate-limited rules, this callback should be used. TLDR; Use this method to keep track of signed transactions, instead of using the data in `ApproveTx`. -### OnSignerStartup +### OnSignerStartup / `ui_onSignerStartup` This method provide the UI with information about what API version the signer uses (both internal and external) aswell as build-info and external api, in k/v-form. @@ -944,7 +918,7 @@ Example call: { "jsonrpc": "2.0", "id": 1, - "method": "OnSignerStartup", + "method": "ui_onSignerStartup", "params": [ { "info": { diff --git a/cmd/clef/datatypes.md b/cmd/clef/datatypes.md index e345a2bbd..a26c1c937 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/intapi_changelog.md b/cmd/clef/intapi_changelog.md index db3353bb7..b7d4937cd 100644 --- a/cmd/clef/intapi_changelog.md +++ b/cmd/clef/intapi_changelog.md @@ -1,5 +1,40 @@ ### Changelog for internal API (ui-api) +### 6.0.0 + +Removed `password` from responses to operations which require them. This is for two reasons, + +- Consistency between how rulesets operate and how manual processing works. A rule can `Approve` but require the actual password to be stored in the clef storage. +With this change, the same stored password can be used even if rulesets are not enabled, but storage is. +- It also removes the usability-shortcut that a UI might otherwise want to implement; remembering passwords. Since we now will not require the +password on every `Approve`, there's no need for the UI to cache it locally. + - In a future update, we'll likely add `clef_storePassword` to the internal API, so the user can store it via his UI (currently only CLI works). + +Affected datatypes: +- `SignTxResponse` +- `SignDataResponse` +- `NewAccountResponse` + +If `clef` requires a password, the `OnInputRequired` will be used to collect it. + + +### 5.0.0 + +Changed the namespace format to adhere to the legacy ethereum format: `name_methodName`. Changes: + +* `ApproveTx` -> `ui_approveTx` +* `ApproveSignData` -> `ui_approveSignData` +* `ApproveExport` -> `removed` +* `ApproveImport` -> `removed` +* `ApproveListing` -> `ui_approveListing` +* `ApproveNewAccount` -> `ui_approveNewAccount` +* `ShowError` -> `ui_showError` +* `ShowInfo` -> `ui_showInfo` +* `OnApprovedTx` -> `ui_onApprovedTx` +* `OnSignerStartup` -> `ui_onSignerStartup` +* `OnInputRequired` -> `ui_onInputRequired` + + ### 4.0.0 * Bidirectional communication implemented, so the UI can query `clef` via the stdin/stdout RPC channel. Methods implemented are: diff --git a/cmd/clef/main.go b/cmd/clef/main.go index 088701eee..a90031245 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. @@ -640,68 +638,124 @@ func testExternalUI(api *core.SignerAPI) { ctx := context.WithValue(context.Background(), "remote", "clef binary") ctx = context.WithValue(ctx, "scheme", "in-proc") ctx = context.WithValue(ctx, "local", "main") - errs := make([]string, 0) - api.UI.ShowInfo("Testing 'ShowInfo'") - api.UI.ShowError("Testing 'ShowError'") + a := common.HexToAddress("0xdeadbeef000000000000000000000000deadbeef") - checkErr := func(method string, err error) { - if err != nil && err != core.ErrRequestDenied { - errs = append(errs, fmt.Sprintf("%v: %v", method, err.Error())) + queryUser := func(q string) string { + resp, err := api.UI.OnInputRequired(core.UserInputRequest{ + Title: "Testing", + Prompt: q, + }) + if err != nil { + errs = append(errs, err.Error()) + } + return resp.Text + } + expectResponse := func(testcase, question, expect string) { + if got := queryUser(question); got != expect { + errs = append(errs, fmt.Sprintf("%s: got %v, expected %v", testcase, got, expect)) } } - var err error - - cliqueHeader := types.Header{ - common.HexToHash("0000H45H"), - common.HexToHash("0000H45H"), - common.HexToAddress("0000H45H"), - common.HexToHash("0000H00H"), - common.HexToHash("0000H45H"), - common.HexToHash("0000H45H"), - types.Bloom{}, - big.NewInt(1337), - big.NewInt(1337), - 1338, - 1338, - big.NewInt(1338), - []byte("Extra data Extra data Extra data Extra data Extra data Extra data Extra data Extra data"), - common.HexToHash("0x0000H45H"), - types.BlockNonce{}, - } - cliqueRlp, err := rlp.EncodeToBytes(cliqueHeader) - if err != nil { - utils.Fatalf("Should not error: %v", err) - } - addr, err := common.NewMixedcaseAddressFromString("0x0011223344556677889900112233445566778899") - if err != nil { - utils.Fatalf("Should not error: %v", err) - } - _, err = api.SignData(ctx, "application/clique", *addr, cliqueRlp) - checkErr("SignData", err) - - _, err = api.SignTransaction(ctx, core.SendTxArgs{From: common.MixedcaseAddress{}}, nil) - checkErr("SignTransaction", err) - _, err = api.SignData(ctx, "text/plain", common.MixedcaseAddress{}, common.Hex2Bytes("01020304")) - checkErr("SignData", err) - //_, err = api.SignTypedData(ctx, common.MixedcaseAddress{}, core.TypedData{}) - //checkErr("SignTypedData", err) - _, err = api.List(ctx) - checkErr("List", err) - _, err = api.New(ctx) - checkErr("New", err) - - api.UI.ShowInfo("Tests completed") - - if len(errs) > 0 { - log.Error("Got errors") - for _, e := range errs { - log.Error(e) + expectApprove := func(testcase string, err error) { + if err == nil || err == accounts.ErrUnknownAccount { + return } - } else { - log.Info("No errors") + errs = append(errs, fmt.Sprintf("%v: expected no error, got %v", testcase, err.Error())) } + expectDeny := func(testcase string, err error) { + if err == nil || err != core.ErrRequestDenied { + errs = append(errs, fmt.Sprintf("%v: expected ErrRequestDenied, got %v", testcase, err)) + } + } + + // Test display of info and error + { + api.UI.ShowInfo("If you see this message, enter 'yes' to next question") + expectResponse("showinfo", "Did you see the message? [yes/no]", "yes") + api.UI.ShowError("If you see this message, enter 'yes' to the next question") + expectResponse("showerror", "Did you see the message? [yes/no]", "yes") + } + { // Sign data test - clique header + api.UI.ShowInfo("Please approve the next request for signing a clique header") + cliqueHeader := types.Header{ + common.HexToHash("0000H45H"), + common.HexToHash("0000H45H"), + common.HexToAddress("0000H45H"), + common.HexToHash("0000H00H"), + common.HexToHash("0000H45H"), + common.HexToHash("0000H45H"), + types.Bloom{}, + big.NewInt(1337), + big.NewInt(1337), + 1338, + 1338, + big.NewInt(1338), + []byte("Extra data Extra data Extra data Extra data Extra data Extra data Extra data Extra data"), + common.HexToHash("0x0000H45H"), + types.BlockNonce{}, + } + cliqueRlp, err := rlp.EncodeToBytes(cliqueHeader) + if err != nil { + utils.Fatalf("Should not error: %v", err) + } + addr, _ := common.NewMixedcaseAddressFromString("0x0011223344556677889900112233445566778899") + _, err = api.SignData(ctx, accounts.MimetypeClique, *addr, hexutil.Encode(cliqueRlp)) + expectApprove("signdata - clique header", err) + } + { // Sign data test - plain text + api.UI.ShowInfo("Please approve the next request for signing text") + addr, _ := common.NewMixedcaseAddressFromString("0x0011223344556677889900112233445566778899") + _, err := api.SignData(ctx, accounts.MimetypeTextPlain, *addr, hexutil.Encode([]byte("hello world"))) + expectApprove("signdata - text", err) + } + { // Sign data test - plain text reject + api.UI.ShowInfo("Please deny the next request for signing text") + addr, _ := common.NewMixedcaseAddressFromString("0x0011223344556677889900112233445566778899") + _, err := api.SignData(ctx, accounts.MimetypeTextPlain, *addr, hexutil.Encode([]byte("hello world"))) + expectDeny("signdata - text", err) + } + { // Sign transaction + + api.UI.ShowInfo("Please reject next transaction") + data := hexutil.Bytes([]byte{}) + to := common.NewMixedcaseAddress(a) + tx := core.SendTxArgs{ + Data: &data, + Nonce: 0x1, + Value: hexutil.Big(*big.NewInt(6)), + From: common.NewMixedcaseAddress(a), + To: &to, + GasPrice: hexutil.Big(*big.NewInt(5)), + Gas: 1000, + Input: nil, + } + _, err := api.SignTransaction(ctx, tx, nil) + expectDeny("signtransaction [1]", err) + expectResponse("signtransaction [2]", "Did you see any warnings for the last transaction? (yes/no)", "no") + } + { // Listing + api.UI.ShowInfo("Please reject listing-request") + _, err := api.List(ctx) + expectDeny("list", err) + } + { // Import + api.UI.ShowInfo("Please reject new account-request") + _, err := api.New(ctx) + expectDeny("newaccount", err) + } + { // Metadata + api.UI.ShowInfo("Please check if you see the Origin in next listing (approve or deny)") + api.List(context.WithValue(ctx, "Origin", "origin.com")) + expectResponse("metadata - origin", "Did you see origin (origin.com)? [yes/no] ", "yes") + } + + for _, e := range errs { + log.Error(e) + } + result := fmt.Sprintf("Tests completed. %d errors:\n%s\n", len(errs), strings.Join(errs, "\n")) + api.UI.ShowInfo(result) + } // getPassPhrase retrieves the password associated with clef, either fetched @@ -798,7 +852,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 +887,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 +900,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 +928,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 +942,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 837b7266c..f499b3451 100644 --- a/signer/core/api.go +++ b/signer/core/api.go @@ -23,6 +23,7 @@ import ( "fmt" "math/big" "reflect" + "strings" "github.com/ethereum/go-ethereum/accounts" "github.com/ethereum/go-ethereum/accounts/keystore" @@ -32,6 +33,7 @@ import ( "github.com/ethereum/go-ethereum/internal/ethapi" "github.com/ethereum/go-ethereum/log" "github.com/ethereum/go-ethereum/rlp" + "github.com/ethereum/go-ethereum/signer/storage" ) const ( @@ -40,7 +42,7 @@ const ( // ExternalAPIVersion -- see extapi_changelog.md ExternalAPIVersion = "6.0.0" // InternalAPIVersion -- see intapi_changelog.md - InternalAPIVersion = "4.0.0" + InternalAPIVersion = "6s.0.0" ) // ExternalAPI defines the external API through which signing requests are made. @@ -68,10 +70,6 @@ type UIClientAPI interface { ApproveTx(request *SignTxRequest) (SignTxResponse, error) // ApproveSignData prompt the user for confirmation to request to sign data ApproveSignData(request *SignDataRequest) (SignDataResponse, error) - // ApproveExport prompt the user for confirmation to export encrypted Account json - ApproveExport(request *ExportRequest) (ExportResponse, error) - // ApproveImport prompt the user for confirmation to import Account json - ApproveImport(request *ImportRequest) (ImportResponse, error) // ApproveListing prompt the user for confirmation to list accounts // the list of accounts to list can be modified by the UI ApproveListing(request *ListRequest) (ListResponse, error) @@ -96,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 @@ -187,25 +186,6 @@ type ( //The UI may make changes to the TX Transaction SendTxArgs `json:"transaction"` Approved bool `json:"approved"` - Password string `json:"password"` - } - // ExportRequest info about query to export accounts - ExportRequest struct { - Address common.Address `json:"address"` - Meta Metadata `json:"meta"` - } - // ExportResponse response to export-request - ExportResponse struct { - Approved bool `json:"approved"` - } - // ImportRequest info about request to import an Account - ImportRequest struct { - Meta Metadata `json:"meta"` - } - ImportResponse struct { - Approved bool `json:"approved"` - OldPassword string `json:"old_password"` - NewPassword string `json:"new_password"` } SignDataRequest struct { ContentType string `json:"content_type"` @@ -217,14 +197,12 @@ type ( } SignDataResponse struct { Approved bool `json:"approved"` - Password string } NewAccountRequest struct { Meta Metadata `json:"meta"` } NewAccountResponse struct { - Approved bool `json:"approved"` - Password string `json:"password"` + Approved bool `json:"approved"` } ListRequest struct { Accounts []accounts.Account `json:"accounts"` @@ -240,8 +218,8 @@ type ( Info map[string]interface{} `json:"info"` } UserInputRequest struct { - Prompt string `json:"prompt"` Title string `json:"title"` + Prompt string `json:"prompt"` IsPassword bool `json:"isPassword"` } UserInputResponse struct { @@ -256,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() } @@ -381,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 } } @@ -452,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 ( @@ -495,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 bf4e91eb9..3225c10ea 100644 --- a/signer/core/api_test.go +++ b/signer/core/api_test.go @@ -19,7 +19,6 @@ package core import ( "bytes" "context" - "errors" "fmt" "io/ioutil" "math/big" @@ -35,62 +34,49 @@ import ( "github.com/ethereum/go-ethereum/core/types" "github.com/ethereum/go-ethereum/internal/ethapi" "github.com/ethereum/go-ethereum/rlp" + "github.com/ethereum/go-ethereum/signer/storage" ) //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) ApproveExport(request *ExportRequest) (ExportResponse, error) { - return ExportResponse{<-ui.controller == "Y"}, nil - -} - -func (ui *HeadlessUI) ApproveImport(request *ImportRequest) (ImportResponse, error) { - if "Y" == <-ui.controller { - return ImportResponse{true, <-ui.controller, <-ui.controller}, nil - } - return ImportResponse{false, "", ""}, 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": @@ -102,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) } @@ -131,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) @@ -158,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 { @@ -176,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) @@ -187,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)) } } @@ -212,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) @@ -232,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") @@ -269,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) @@ -279,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) @@ -288,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) @@ -296,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 { @@ -310,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 { @@ -322,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 { @@ -341,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 a6c0bdb16..cf7101441 100644 --- a/signer/core/cliui.go +++ b/signer/core/cliui.go @@ -87,9 +87,10 @@ func (ui *CommandlineUI) readPasswordText(inputstring string) string { } func (ui *CommandlineUI) OnInputRequired(info UserInputRequest) (UserInputResponse, error) { - fmt.Println(info.Title) - fmt.Println(info.Prompt) + + fmt.Printf("## %s\n\n%s\n", info.Title, info.Prompt) if info.IsPassword { + fmt.Printf("> ") text, err := terminal.ReadPassword(int(os.Stdin.Fd())) if err != nil { log.Error("Failed to read password", "err", err) @@ -156,9 +157,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,40 +179,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 -} - -// ApproveExport prompt the user for confirmation to export encrypted Account json -func (ui *CommandlineUI) ApproveExport(request *ExportRequest) (ExportResponse, error) { - ui.mu.Lock() - defer ui.mu.Unlock() - - fmt.Printf("-------- Export Account request--------------\n") - fmt.Printf("A request has been made to export the (encrypted) keyfile\n") - fmt.Printf("Approving this operation means that the caller obtains the (encrypted) contents\n") - fmt.Printf("\n") - fmt.Printf("Account: %x\n", request.Address) - //fmt.Printf("keyfile: \n%v\n", request.file) - fmt.Printf("-------------------------------------------\n") - showMetadata(request.Meta) - return ExportResponse{ui.confirm()}, nil -} - -// ApproveImport prompt the user for confirmation to import Account json -func (ui *CommandlineUI) ApproveImport(request *ImportRequest) (ImportResponse, error) { - ui.mu.Lock() - defer ui.mu.Unlock() - - fmt.Printf("-------- Import Account request--------------\n") - fmt.Printf("A request has been made to import an encrypted keyfile\n") - fmt.Printf("-------------------------------------------\n") - showMetadata(request.Meta) - if !ui.confirm() { - return ImportResponse{false, "", ""}, nil - } - return ImportResponse{true, ui.readPasswordText("Old password"), ui.readPasswordText("New password")}, nil + return SignDataResponse{true}, nil } // ApproveListing prompt the user for confirmation to list accounts @@ -248,21 +218,20 @@ 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 func (ui *CommandlineUI) ShowError(message string) { - fmt.Printf("-------- Error message from Clef-----------\n") - fmt.Println(message) + fmt.Printf("## Error \n%s\n", message) fmt.Printf("-------------------------------------------\n") } // ShowInfo displays info message to user func (ui *CommandlineUI) ShowInfo(message string) { - fmt.Printf("Info: %v\n", message) + fmt.Printf("## Info \n%s\n", message) } func (ui *CommandlineUI) OnApprovedTx(tx ethapi.SignTransactionResult) { diff --git a/signer/core/signed_data.go b/signer/core/signed_data.go index 9df39ef59..475a8837e 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 7d44bce2c..76e1b72ee 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/core/stdioui.go b/signer/core/stdioui.go index cb25bc2d0..0edb72def 100644 --- a/signer/core/stdioui.go +++ b/signer/core/stdioui.go @@ -65,71 +65,59 @@ func (ui *StdIOUI) notify(serviceMethod string, args interface{}) error { func (ui *StdIOUI) ApproveTx(request *SignTxRequest) (SignTxResponse, error) { var result SignTxResponse - err := ui.dispatch("ApproveTx", request, &result) + err := ui.dispatch("ui_approveTx", request, &result) return result, err } func (ui *StdIOUI) ApproveSignData(request *SignDataRequest) (SignDataResponse, error) { var result SignDataResponse - err := ui.dispatch("ApproveSignData", request, &result) - return result, err -} - -func (ui *StdIOUI) ApproveExport(request *ExportRequest) (ExportResponse, error) { - var result ExportResponse - err := ui.dispatch("ApproveExport", request, &result) - return result, err -} - -func (ui *StdIOUI) ApproveImport(request *ImportRequest) (ImportResponse, error) { - var result ImportResponse - err := ui.dispatch("ApproveImport", request, &result) + err := ui.dispatch("ui_approveSignData", request, &result) return result, err } func (ui *StdIOUI) ApproveListing(request *ListRequest) (ListResponse, error) { var result ListResponse - err := ui.dispatch("ApproveListing", request, &result) + err := ui.dispatch("ui_approveListing", request, &result) return result, err } func (ui *StdIOUI) ApproveNewAccount(request *NewAccountRequest) (NewAccountResponse, error) { var result NewAccountResponse - err := ui.dispatch("ApproveNewAccount", request, &result) + err := ui.dispatch("ui_approveNewAccount", request, &result) return result, err } func (ui *StdIOUI) ShowError(message string) { - err := ui.notify("ShowError", &Message{message}) + err := ui.notify("ui_showError", &Message{message}) if err != nil { - log.Info("Error calling 'ShowError'", "exc", err.Error(), "msg", message) + log.Info("Error calling 'ui_showError'", "exc", err.Error(), "msg", message) } } func (ui *StdIOUI) ShowInfo(message string) { - err := ui.notify("ShowInfo", Message{message}) + err := ui.notify("ui_showInfo", Message{message}) if err != nil { - log.Info("Error calling 'ShowInfo'", "exc", err.Error(), "msg", message) + log.Info("Error calling 'ui_showInfo'", "exc", err.Error(), "msg", message) } } func (ui *StdIOUI) OnApprovedTx(tx ethapi.SignTransactionResult) { - err := ui.notify("OnApprovedTx", tx) + err := ui.notify("ui_onApprovedTx", tx) if err != nil { - log.Info("Error calling 'OnApprovedTx'", "exc", err.Error(), "tx", tx) + log.Info("Error calling 'ui_onApprovedTx'", "exc", err.Error(), "tx", tx) } } func (ui *StdIOUI) OnSignerStartup(info StartupInfo) { - err := ui.notify("OnSignerStartup", info) + err := ui.notify("ui_onSignerStartup", info) if err != nil { - log.Info("Error calling 'OnSignerStartup'", "exc", err.Error(), "info", info) + log.Info("Error calling 'ui_onSignerStartup'", "exc", err.Error(), "info", info) } } func (ui *StdIOUI) OnInputRequired(info UserInputRequest) (UserInputResponse, error) { var result UserInputResponse - err := ui.dispatch("OnInputRequired", info, &result) + err := ui.dispatch("ui_onInputRequired", info, &result) if err != nil { - log.Info("Error calling 'OnInputRequired'", "exc", err.Error(), "info", info) + log.Info("Error calling 'ui_onInputRequired'", "exc", err.Error(), "info", info) } return result, err } diff --git a/signer/rules/rules.go b/signer/rules/rules.go index 1f81e21bd..5ebffa3af 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,28 +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 -} - -func (r *rulesetUI) ApproveExport(request *core.ExportRequest) (core.ExportResponse, error) { - jsonreq, err := json.Marshal(request) - approved, err := r.checkApproval("ApproveExport", jsonreq, err) - if err != nil { - log.Info("Rule-based approval error, going to manual", "error", err) - return r.next.ApproveExport(request) - } - if approved { - return core.ExportResponse{Approved: true}, nil - } - return core.ExportResponse{Approved: false}, err -} - -func (r *rulesetUI) ApproveImport(request *core.ImportRequest) (core.ImportResponse, error) { - // This cannot be handled by rules, requires setting a password - // dispatch to next - return r.next.ApproveImport(request) + 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 eca4b29a0..19d9049c7 100644 --- a/signer/rules/rules_test.go +++ b/signer/rules/rules_test.go @@ -84,19 +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 -} - -func (alwaysDenyUI) ApproveExport(request *core.ExportRequest) (core.ExportResponse, error) { - return core.ExportResponse{Approved: false}, nil -} - -func (alwaysDenyUI) ApproveImport(request *core.ImportRequest) (core.ImportResponse, error) { - return core.ImportResponse{Approved: false, OldPassword: "", NewPassword: ""}, nil + return core.SignDataResponse{Approved: false}, nil } func (alwaysDenyUI) ApproveListing(request *core.ListRequest) (core.ListResponse, error) { @@ -104,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) { @@ -120,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) } @@ -225,16 +217,6 @@ func (d *dummyUI) ApproveSignData(request *core.SignDataRequest) (core.SignDataR return core.SignDataResponse{}, core.ErrRequestDenied } -func (d *dummyUI) ApproveExport(request *core.ExportRequest) (core.ExportResponse, error) { - d.calls = append(d.calls, "ApproveExport") - return core.ExportResponse{}, core.ErrRequestDenied -} - -func (d *dummyUI) ApproveImport(request *core.ImportRequest) (core.ImportResponse, error) { - d.calls = append(d.calls, "ApproveImport") - return core.ImportResponse{}, core.ErrRequestDenied -} - func (d *dummyUI) ApproveListing(request *core.ListRequest) (core.ListResponse, error) { d.calls = append(d.calls, "ApproveListing") return core.ListResponse{}, core.ErrRequestDenied @@ -266,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) } @@ -276,17 +257,15 @@ func TestForwarding(t *testing.T) { } r.ApproveSignData(nil) r.ApproveTx(nil) - r.ApproveImport(nil) r.ApproveNewAccount(nil) r.ApproveListing(nil) - r.ApproveExport(nil) r.ShowError("test") r.ShowInfo("test") //This one is not forwarded r.OnApprovedTx(ethapi.SignTransactionResult{}) - expCalls := 8 + expCalls := 6 if len(ui.calls) != expCalls { t.Errorf("Expected %d forwarded calls, got %d: %s", expCalls, len(ui.calls), strings.Join(ui.calls, ",")) @@ -545,16 +524,6 @@ func (d *dontCallMe) ApproveSignData(request *core.SignDataRequest) (core.SignDa return core.SignDataResponse{}, core.ErrRequestDenied } -func (d *dontCallMe) ApproveExport(request *core.ExportRequest) (core.ExportResponse, error) { - d.t.Fatalf("Did not expect next-handler to be called") - return core.ExportResponse{}, core.ErrRequestDenied -} - -func (d *dontCallMe) ApproveImport(request *core.ImportRequest) (core.ImportResponse, error) { - d.t.Fatalf("Did not expect next-handler to be called") - return core.ImportResponse{}, core.ErrRequestDenied -} - func (d *dontCallMe) ApproveListing(request *core.ListRequest) (core.ListResponse, error) { d.t.Fatalf("Did not expect next-handler to be called") return core.ListResponse{}, core.ErrRequestDenied @@ -597,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 60f4e3892..50c55e455 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 "" +}