From d17a51829da1d545463433d62525a9cfeaad945f Mon Sep 17 00:00:00 2001 From: Martin Holst Swende Date: Mon, 4 Dec 2017 11:57:01 +0100 Subject: [PATCH] cmd/signer: made possible for UI to modify tx parameters --- cmd/signer/README.md | 19 ++++++++------- cmd/signer/abihelper.go | 2 +- cmd/signer/api.go | 18 +++++++------- cmd/signer/api_test.go | 53 ++++++++++++++++++++++++++++++++++++----- cmd/signer/auditlog.go | 26 ++++++++++++++++++++ cmd/signer/cliui.go | 17 +++++++------ cmd/signer/main.go | 45 +++++++++++++++++++++------------- cmd/signer/types.go | 4 ++-- 8 files changed, 136 insertions(+), 48 deletions(-) create mode 100644 cmd/signer/auditlog.go diff --git a/cmd/signer/README.md b/cmd/signer/README.md index d3694c25aa..513e08dbc1 100644 --- a/cmd/signer/README.md +++ b/cmd/signer/README.md @@ -136,14 +136,17 @@ None Signs a transactions and responds with the signed transaction in RLP encoded form. #### Arguments - - from [address]: account to send the transaction from - - Transaction object: - - transaction.to [address]: receiver account - - gas [number]: maximum amount of gas to burn - - gasPrice [number]: gas price - - value [number:optional]: amount of Wei to send with the transaction - - data [data:optional]: input data - - transaction.nonce [number]: account nonce + 1. from [address]: account to send the transaction from + 2. transaction object: + - `to` [address]: receiver account + - `gas` [number]: maximum amount of gas to burn + - `gasPrice` [number]: gas price + - `value` [number:optional]: amount of Wei to send with the transaction + - `data` [data:optional]: input data + - `nonce` [number]: account nonce + 3. method signature [string:optional] + - The method signature, if present, is to aid decoding the calldata. Should consist of `methodname(paramtype,...)`, e.g. `transfer(uint256,address)`. The signer may use this data to parse the supplied calldata, and show the user. The data, however, is considered totally untrusted, and reliability is not expected. + #### Result - signed transaction in RLP encoded form [data] diff --git a/cmd/signer/abihelper.go b/cmd/signer/abihelper.go index 96b6106d3b..4e48055dc0 100644 --- a/cmd/signer/abihelper.go +++ b/cmd/signer/abihelper.go @@ -133,6 +133,6 @@ func (db *abiDb) LookupABI(id []byte) (string, error) { } return "", fmt.Errorf("Signature %v not found", sig) } -func (db *abiDb) Size() int{ +func (db *abiDb) Size() int { return len(db.db) } diff --git a/cmd/signer/api.go b/cmd/signer/api.go index 30541d43d0..b14a8e4faf 100644 --- a/cmd/signer/api.go +++ b/cmd/signer/api.go @@ -53,15 +53,16 @@ type Metadata struct { type ( // SignTxRequest contains info about a transaction to sign SignTxRequest struct { - transaction *types.Transaction + transaction types.Transaction from accounts.Account callinfo fmt.Stringer } // SignTxResponse result from SignTxRequest SignTxResponse struct { - hash common.Hash - approved bool - pw string + //The UI may make changes to the TX + transaction types.Transaction + approved bool + pw string } // ExportRequest info about query to export accounts ExportRequest struct { @@ -244,7 +245,7 @@ func (api *SignerAPI) SignTransaction(ctx context.Context, from common.Address, tx = types.NewTransaction(uint64(*args.Nonce), *args.To, (*big.Int)(args.Value), (*big.Int)(args.Gas), (*big.Int)(args.GasPrice), args.Data) } - req := SignTxRequest{transaction: tx, from: acc} + req := SignTxRequest{transaction: *tx, from: acc} if len(tx.Data()) > 3 { // Try to make sense of the data var abidata string @@ -269,10 +270,11 @@ func (api *SignerAPI) SignTransaction(ctx context.Context, from common.Address, if result := <-ch; result.approved { //Sanity check - if result.hash != tx.Hash() { - return nil, fmt.Errorf("Transaction hash mismatch") + if result.transaction.Hash() != tx.Hash() { + api.ui.ShowInfo("Transaction modified by UI") } - signedTx, err := wallet.SignTxWithPassphrase(acc, result.pw, tx, api.chainID) + // The one to sign is the one that was returned from the UI + signedTx, err := wallet.SignTxWithPassphrase(acc, result.pw, &result.transaction, api.chainID) if err != nil { api.ui.ShowError(err.Error()) return nil, err diff --git a/cmd/signer/api_test.go b/cmd/signer/api_test.go index f83c042f64..fa553a2af6 100644 --- a/cmd/signer/api_test.go +++ b/cmd/signer/api_test.go @@ -1,12 +1,14 @@ package main import ( + "bytes" "context" "fmt" "github.com/ethereum/go-ethereum/accounts/keystore" "github.com/ethereum/go-ethereum/cmd/utils" "github.com/ethereum/go-ethereum/common" "github.com/ethereum/go-ethereum/common/hexutil" + "github.com/ethereum/go-ethereum/core/types" "io/ioutil" "math/big" "os" @@ -21,10 +23,17 @@ type HeadlessUI struct { } func (ui *HeadlessUI) ApproveTx(request *SignTxRequest, metadata Metadata, ch chan SignTxResponse) { - if "Y" == <-ui.controller { - ch <- SignTxResponse{request.transaction.Hash(), true, <-ui.controller} - } else { - ch <- SignTxResponse{request.transaction.Hash(), false, ""} + + switch <-ui.controller { + case "Y": + ch <- SignTxResponse{request.transaction, true, <-ui.controller} + case "M": //Modify + old := request.transaction + newVal := big.NewInt(0).Add(old.Value(), big.NewInt(1)) + tx := types.NewTransaction(old.Nonce(), *old.To(), newVal, old.Gas(), old.GasPrice(), old.Data()) + ch <- SignTxResponse{*tx, true, <-ui.controller} + default: + ch <- SignTxResponse{request.transaction, false, ""} } } func (ui *HeadlessUI) ApproveSignData(request *SignDataRequest, metadata Metadata, ch chan SignDataResponse) { @@ -250,10 +259,18 @@ func mkTestTx() TransactionArg { } func TestSignTx(t *testing.T) { + + var ( + list Accounts + h []byte + h2 []byte + err error + ) + api, control := setup(t) createAccount(control, api, t) control <- "A" - list, err := api.List(context.Background()) + list, err = api.List(context.Background()) if err != nil { t.Fatal(err) } @@ -264,7 +281,7 @@ func TestSignTx(t *testing.T) { control <- "Y" control <- "wrongpassword" - h, err := api.SignTransaction(context.Background(), a, tx, &methodSig) + h, err = api.SignTransaction(context.Background(), a, tx, &methodSig) if h != nil { t.Errorf("Expected nil-data, got %h", h) } @@ -291,4 +308,28 @@ func TestSignTx(t *testing.T) { if h == nil || len(h) != 118 { t.Errorf("Expected 181 byte rlp-data (got %d bytes)", len(h)) } + //The tx is NOT modified by the UI + control <- "Y" + control <- "apassword" + + h2, err = api.SignTransaction(context.Background(), a, tx, &methodSig) + if err != nil { + t.Fatal(err) + } + if !bytes.Equal(h, h2) { + t.Error("Expected tx to be unmodified by UI") + } + + //The tx is modified by the UI + control <- "M" + control <- "apassword" + + h2, err = api.SignTransaction(context.Background(), a, tx, &methodSig) + if err != nil { + t.Fatal(err) + } + if bytes.Equal(h, h2) { + t.Error("Expected tx to be modified by UI") + } + } diff --git a/cmd/signer/auditlog.go b/cmd/signer/auditlog.go new file mode 100644 index 0000000000..42122cfaa3 --- /dev/null +++ b/cmd/signer/auditlog.go @@ -0,0 +1,26 @@ +package main + +import ( + "bufio" + "fmt" + "github.com/ethereum/go-ethereum/rpc" + "io" + "time" +) + +type AuditLogger struct { + writer *bufio.Writer +} + +func (l AuditLogger) Store(record *rpc.RPCInvocationRecord) { + l.writer.WriteString(fmt.Sprintf("%v\n%v\n", time.Now().Format(time.RFC3339), record.Method)) + for i, arg := range record.Args { + l.writer.WriteString(fmt.Sprintf("\t%d: %v\n", i, arg)) + } + l.writer.WriteString(fmt.Sprintf("%v\n", record.Response)) + l.writer.Flush() +} + +func NewAuditLogger(writer io.Writer) *AuditLogger { + return &AuditLogger{bufio.NewWriter(writer)} +} diff --git a/cmd/signer/cliui.go b/cmd/signer/cliui.go index 6bb8ccca3d..3898bac917 100644 --- a/cmd/signer/cliui.go +++ b/cmd/signer/cliui.go @@ -56,10 +56,13 @@ func (ui *CommandlineUI) readString() string { func (ui *CommandlineUI) readPassword() string { fmt.Printf("Enter password to approve:\n") fmt.Printf("> ") - text, err := terminal.ReadPassword(int(os.Stdin.Fd())) - if err != nil { - log.Crit("Failed to read password", "err", err) - } + //TODO; remove this, only for debuggging within IDE + text := "foobar" + //TODO: Use this + // text, err := terminal.ReadPassword(int(os.Stdin.Fd())) + //if err != nil { + // log.Crit("Failed to read password", "err", err) + //} fmt.Println() fmt.Println("-----------------------") return string(text) @@ -114,7 +117,7 @@ func (ui *CommandlineUI) ApproveTx(request *SignTxRequest, metadata Metadata, ch showMetadata(metadata) fmt.Printf("-------------------------------------------\n") - ch <- SignTxResponse{request.transaction.Hash(), true ,ui.readPassword()} + ch <- SignTxResponse{request.transaction, true, ui.readPassword()} } // ApproveSignData prompt the user for confirmation to request to sign data @@ -157,9 +160,9 @@ func (ui *CommandlineUI) ApproveImport(request *ImportRequest, metadata Metadata fmt.Printf("A request has been made to import an encrypted keyfile\n") fmt.Printf("-------------------------------------------\n") showMetadata(metadata) - if ui.confirm(){ + if ui.confirm() { ch <- ImportResponse{true, ui.readPasswordText("Old password"), ui.readPasswordText("New password")} - }else{ + } else { ch <- ImportResponse{false, "", ""} } } diff --git a/cmd/signer/main.go b/cmd/signer/main.go index 02eff8e96b..ced6892ef0 100644 --- a/cmd/signer/main.go +++ b/cmd/signer/main.go @@ -20,7 +20,6 @@ package main import ( "fmt" - "io" "net" "os" "path/filepath" @@ -70,9 +69,14 @@ func main() { }, cli.StringFlag{ Name: "auditlog", - Usage: "File used to emit audit logs. Set to '' to disable", + Usage: "File used to emit audit logs. Set to \"\" to disable", Value: "audit.log", }, + cli.StringFlag{ + Name: "requestfile", + Usage: "File containing requests to handle", + Value: "", + }, } app.Action = func(c *cli.Context) error { @@ -88,21 +92,34 @@ func main() { var ( server = rpc.NewServer() - api = NewSignerAPI( + + api = NewSignerAPI( c.Int64(utils.NetworkIdFlag.Name), c.String("keystore"), c.Bool(utils.NoUSBFlag.Name), NewCommandlineUI(), db, c.Bool(utils.LightKDFFlag.Name)) listener net.Listener - //err error ) - + if logfile := c.String("auditlog"); logfile != "" { + f, err := os.OpenFile(logfile, os.O_CREATE|os.O_APPEND|os.O_WRONLY, 0600) + if err != nil { + utils.Fatalf("Could not open %v for audit logging", logfile) + } + server.SetAuditLogger(NewAuditLogger(f)) + log.Info("Writing audit logs to %v", logfile) + } // register signer API with server if err = server.RegisterName("account", api); err != nil { utils.Fatalf("Could not register signer API: %v", err) } + // Import from file + if rfile := c.String("requestfile"); rfile != "" { + //Each line of file represents one request + log.Warn("Import from file not yet implemented") + } + // start http server endpoint := fmt.Sprintf("%s:%d", c.String(utils.RPCListenAddrFlag.Name), c.Int("rpcport")) if listener, err = net.Listen("tcp", endpoint); err != nil { @@ -112,6 +129,7 @@ func main() { cors := []string{"*"} rpc.NewHTTPServer(cors, server).Serve(listener) + return nil } app.Run(os.Args) @@ -127,16 +145,11 @@ func main() { // Make transaction // safeSend(0x12) // 4401a6e40000000000000000000000000000000000000000000000000000000000000012 -// curl -i -H "Content-Type: application/json" -X POST --data '{"jsonrpc":"2.0","method":"account_signTransaction","params":["0x82A2A876D39022B3019932D30Cd9c97ad5616813","pw",{"gas":"0x333","gasPrice":"0x123","nonce":"0x0","to":"0x07a565b7ed7d7a678680a4c162885bedbb695fe0", "value":"0x10", "input":"0x4401a6e40000000000000000000000000000000000000000000000000000000000000012"}],"id":67}' http://localhost:8550/ -type rwc struct { - io.Reader - io.Writer -} +/* -func (r *rwc) Close() error { - if err := os.Stdin.Close(); err != nil { - return err - } - return os.Stdout.Close() -} +curl -i -H "Content-Type: application/json" -X POST --data '{"jsonrpc":"2.0","method":"account_signTransaction","params":["0x82A2A876D39022B3019932D30Cd9c97ad5616813",{"gas":"0x333","gasPrice":"0x123","nonce":"0x0","to":"0x07a565b7ed7d7a678680a4c162885bedbb695fe0", "value":"0x10", "input":"0x4401a6e40000000000000000000000000000000000000000000000000000000000000012"}],"id":67}' http://localhost:8550/ + + +curl -i -H "Content-Type: application/json" -X POST --data '{"jsonrpc":"2.0","method":"account_signTransaction","params":["0x82A2A876D39022B3019932D30Cd9c97ad5616813",{"gas":"0x333","gasPrice":"0x123","nonce":"0x0","to":"0x07a565b7ed7d7a678680a4c162885bedbb695fe0", "value":"0x10", "input":"0x4401a6e40000000000000000000000000000000000000000000000000000000000000012"},"test"],"id":67}' http://localhost:8550/ +*/ diff --git a/cmd/signer/types.go b/cmd/signer/types.go index 699742d1ee..8ac693ab19 100644 --- a/cmd/signer/types.go +++ b/cmd/signer/types.go @@ -26,9 +26,9 @@ import ( type Accounts []Account -func (as Accounts) String() string{ +func (as Accounts) String() string { var output []string - for _,a := range as{ + for _, a := range as { output = append(output, a.String()) } return strings.Join(output, "\n")