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 "" +}