Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

clef: make external signing work + follow up fixes #19003

Merged
merged 10 commits into from
Feb 12, 2019
39 changes: 22 additions & 17 deletions accounts/external/backend.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ import (
"github.com/ethereum/go-ethereum/common"
"github.com/ethereum/go-ethereum/common/hexutil"
"github.com/ethereum/go-ethereum/core/types"
"github.com/ethereum/go-ethereum/crypto"
"github.com/ethereum/go-ethereum/event"
"github.com/ethereum/go-ethereum/internal/ethapi"
"github.com/ethereum/go-ethereum/log"
Expand Down Expand Up @@ -154,13 +153,31 @@ func (api *ExternalSigner) signHash(account accounts.Account, hash []byte) ([]by

// SignData signs keccak256(data). The mimetype parameter describes the type of data being signed
func (api *ExternalSigner) SignData(account accounts.Account, mimeType string, data []byte) ([]byte, error) {
// TODO! Replace this with a call to clef SignData with correct mime-type for Clique, once we
// have that in place
return api.signHash(account, crypto.Keccak256(data))
var res hexutil.Bytes
var signAddress = common.NewMixedcaseAddress(account.Address)
if err := api.client.Call(&res, "account_signData",
mimeType,
&signAddress, // Need to use the pointer here, because of how MarshalJSON is defined
hexutil.Encode(data)); err != nil {
return nil, err
}
// If V is on 27/28-form, convert to to 0/1 for Clique
if mimeType == accounts.MimetypeClique && (res[64] == 27 || res[64] == 28) {
res[64] -= 27 // Transform V from 27/28 to 0/1 for Clique use
}
return res, nil
}

func (api *ExternalSigner) SignText(account accounts.Account, text []byte) ([]byte, error) {
return api.signHash(account, accounts.TextHash(text))
var res hexutil.Bytes
var signAddress = common.NewMixedcaseAddress(account.Address)
if err := api.client.Call(&res, "account_signData",
accounts.MimetypeTextPlain,
&signAddress, // Need to use the pointer here, because of how MarshalJSON is defined
hexutil.Encode(text)); err != nil {
return nil, err
}
return res, nil
}

func (api *ExternalSigner) SignTx(account accounts.Account, tx *types.Transaction, chainID *big.Int) (*types.Transaction, error) {
Expand Down Expand Up @@ -202,18 +219,6 @@ func (api *ExternalSigner) listAccounts() ([]common.Address, error) {
return res, nil
}

func (api *ExternalSigner) signCliqueBlock(a common.Address, rlpBlock hexutil.Bytes) (hexutil.Bytes, error) {
var sig hexutil.Bytes
if err := api.client.Call(&sig, "account_signData", core.ApplicationClique.Mime, a, rlpBlock); err != nil {
return nil, err
}
if sig[64] != 27 && sig[64] != 28 {
return nil, fmt.Errorf("invalid Ethereum signature (V is not 27 or 28)")
}
sig[64] -= 27 // Transform V from 27/28 to 0/1 for Clique use
return sig, nil
}

func (api *ExternalSigner) pingVersion() (string, error) {
var v string
if err := api.client.Call(&v, "account_version"); err != nil {
Expand Down
9 changes: 8 additions & 1 deletion cmd/clef/intapi_changelog.md
Original file line number Diff line number Diff line change
@@ -1,8 +1,15 @@
### Changelog for internal API (ui-api)

### 3.2.0

* Make `ShowError`, `OnApprovedTx`, `OnSignerStartup` be json-rpc [notifications](https://www.jsonrpc.org/specification#notification):

> A Notification is a Request object without an "id" member. A Request object that is a Notification signifies the Client's lack of interest in the corresponding Response object, and as such no Response object needs to be returned to the client. The Server MUST NOT reply to a Notification, including those that are within a batch request.
>
> Notifications are not confirmable by definition, since they do not have a Response object to be returned. As such, the Client would not be aware of any errors (like e.g. "Invalid params","Internal error"
### 3.1.0

* Add `ContentType string` to `SignDataRequest` to accommodate the latest EIP-191 and EIP-712 implementations.
* Add `ContentType` `string` to `SignDataRequest` to accommodate the latest EIP-191 and EIP-712 implementations.

### 3.0.0

Expand Down
14 changes: 12 additions & 2 deletions cmd/clef/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ import (
"github.com/ethereum/go-ethereum/crypto"
"github.com/ethereum/go-ethereum/log"
"github.com/ethereum/go-ethereum/node"
"github.com/ethereum/go-ethereum/params"
"github.com/ethereum/go-ethereum/rlp"
"github.com/ethereum/go-ethereum/rpc"
"github.com/ethereum/go-ethereum/signer/core"
Expand Down Expand Up @@ -83,6 +84,11 @@ var (
Value: DefaultConfigDir(),
Usage: "Directory for Clef configuration",
}
chainIdFlag = cli.Int64Flag{
Name: "chainid",
Value: params.MainnetChainConfig.ChainID.Int64(),
Usage: "Chain id to use for signing (1=mainnet, 3=ropsten, 4=rinkeby, 5=Goerli)",
}
rpcPortFlag = cli.IntFlag{
Name: "rpcport",
Usage: "HTTP-RPC server listening port",
Expand Down Expand Up @@ -178,7 +184,7 @@ func init() {
logLevelFlag,
keystoreFlag,
configdirFlag,
utils.NetworkIdFlag,
chainIdFlag,
utils.LightKDFFlag,
utils.NoUSBFlag,
utils.RPCListenAddrFlag,
Expand Down Expand Up @@ -402,9 +408,13 @@ func signer(c *cli.Context) error {
}
}
}
log.Info("Starting signer", "chainid", c.GlobalInt64(chainIdFlag.Name),
"keystore", c.GlobalString(keystoreFlag.Name),
"light-kdf", c.GlobalBool(utils.LightKDFFlag.Name),
"advanced", c.GlobalBool(advancedMode.Name))

apiImpl := core.NewSignerAPI(
c.GlobalInt64(utils.NetworkIdFlag.Name),
c.GlobalInt64(chainIdFlag.Name),
c.GlobalString(keystoreFlag.Name),
c.GlobalBool(utils.NoUSBFlag.Name),
ui, db,
Expand Down
73 changes: 73 additions & 0 deletions cmd/clef/tests/testsigner.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
// This file is a test-utility for testing clef-functionality
//
// Start clef with
//
// build/bin/clef --4bytedb=./cmd/clef/4byte.json --rpc
//
// Start geth with
//
// build/bin/geth --nodiscover --maxpeers 0 --signer http://localhost:8550 console --preload=cmd/clef/tests/testsigner.js
//
// and in the console simply invoke
//
// > test()
//
// You can reload the file via `reload()`

function reload(){
loadScript("./cmd/clef/tests/testsigner.js");
}

function init(){
if (typeof accts == 'undefined' || accts.length == 0){
accts = eth.accounts
console.log("Got accounts ", accts);
}
}
init()
function testTx(){
if( accts && accts.length > 0) {
var a = accts[0]
var txdata = eth.signTransaction({from: a, to: a, value: 1, nonce: 1, gas: 1, gasPrice: 1})
var v = parseInt(txdata.tx.v)
console.log("V value: ", v)
if (v == 37 || v == 38){
console.log("Mainnet 155-protected chainid was used")
}
if (v == 27 || v == 28){
throw new Error("Mainnet chainid was used, but without replay protection!")
}
}
}
function testSignText(){
if( accts && accts.length > 0){
var a = accts[0]
var r = eth.sign(a, "0x68656c6c6f20776f726c64"); //hello world
console.log("signing response", r)
}
}
function testClique(){
if( accts && accts.length > 0){
var a = accts[0]
var r = debug.testSignCliqueBlock(a, 0); // Sign genesis
console.log("signing response", r)
if( a != r){
throw new Error("Requested signing by "+a+ " but got sealer "+r)
}
}
}

function test(){
var tests = [
testTx,
testSignText,
testClique,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we also have tests to check the error conditions?

Copy link
Contributor Author

@holiman holiman Feb 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there any particular error conditions you're thinking of?
I mean, there are quite a lot of ways for signing to fail. This js-test is meant as an integration test, to test the whole geth-external-ui and back again flow.

It's not for CI, but for a dev to run manually to test things more easily.

Individual tests within signer/clef should test things like rejection and tx validation etc.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking of the 27/28 vs 0/1 in particular.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, added for transactions. For clique, the new debug endpoint will deliver the derived signer. And if the wrong v-type is used, it will derive another address as sealer of that block.

So the js-test already checks that the requested signer-address equals what the endpoint returns, thus checks that the geth and clef are in agreement (without explicitly checking v)

]
for( i in tests){
try{
tests[i]()
}catch(err){
console.log(err)
}
}
}
40 changes: 40 additions & 0 deletions internal/ethapi/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import (
"github.com/ethereum/go-ethereum/common"
"github.com/ethereum/go-ethereum/common/hexutil"
"github.com/ethereum/go-ethereum/common/math"
"github.com/ethereum/go-ethereum/consensus/clique"
"github.com/ethereum/go-ethereum/consensus/ethash"
"github.com/ethereum/go-ethereum/core"
"github.com/ethereum/go-ethereum/core/rawdb"
Expand Down Expand Up @@ -1481,6 +1482,45 @@ func (api *PublicDebugAPI) GetBlockRlp(ctx context.Context, number uint64) (stri
return fmt.Sprintf("%x", encoded), nil
}

// TestSignCliqueBlock fetches the given block number, and attempts to sign it as a clique header with the
// given address, returning the address of the recovered signature
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reminder: Peter said that this should be noted as TODO/to remove

//
// This is a temporary method to debug the externalsigner integration,
// TODO: Remove this method when the integration is mature
func (api *PublicDebugAPI) TestSignCliqueBlock(ctx context.Context, address common.Address, number uint64) (common.Address, error) {
block, _ := api.b.BlockByNumber(ctx, rpc.BlockNumber(number))
if block == nil {
return common.Address{}, fmt.Errorf("block #%d not found", number)
}
header := block.Header()
header.Extra = make([]byte, 32+65)
encoded := clique.CliqueRLP(header)

// Look up the wallet containing the requested signer
account := accounts.Account{Address: address}
wallet, err := api.b.AccountManager().Find(account)
if err != nil {
return common.Address{}, err
}

signature, err := wallet.SignData(account, accounts.MimetypeClique, encoded)
if err != nil {
return common.Address{}, err
}
sealHash := clique.SealHash(header).Bytes()
log.Info("test signing of clique block",
"Sealhash", fmt.Sprintf("%x", sealHash),
"signature", fmt.Sprintf("%x", signature))
pubkey, err := crypto.Ecrecover(sealHash, signature)
if err != nil {
return common.Address{}, err
}
var signer common.Address
copy(signer[:], crypto.Keccak256(pubkey[1:])[12:])

return signer, nil
}

// PrintBlock retrieves a block and returns its pretty printed form.
func (api *PublicDebugAPI) PrintBlock(ctx context.Context, number uint64) (string, error) {
block, _ := api.b.BlockByNumber(ctx, rpc.BlockNumber(number))
Expand Down
6 changes: 6 additions & 0 deletions internal/web3ext/web3ext.go
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,12 @@ web3._extend({
call: 'debug_getBlockRlp',
params: 1
}),
new web3._extend.Method({
name: 'testSignCliqueBlock',
call: 'debug_testSignCliqueBlock',
params: 2,
inputFormatters: [web3._extend.formatters.inputAddressFormatter, null],
}),
new web3._extend.Method({
name: 'setHead',
call: 'debug_setHead',
Expand Down
2 changes: 1 addition & 1 deletion signer/core/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ const (
// ExternalAPIVersion -- see extapi_changelog.md
ExternalAPIVersion = "5.0.0"
// InternalAPIVersion -- see intapi_changelog.md
InternalAPIVersion = "3.1.0"
InternalAPIVersion = "3.2.0"
)

// ExternalAPI defines the external API through which signing requests are made.
Expand Down
8 changes: 6 additions & 2 deletions signer/core/cliui.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,12 @@ package core

import (
"bufio"
"encoding/json"
"fmt"
"os"
"strings"
"sync"

"github.com/davecgh/go-spew/spew"
"github.com/ethereum/go-ethereum/common/hexutil"
"github.com/ethereum/go-ethereum/internal/ethapi"
"github.com/ethereum/go-ethereum/log"
Expand Down Expand Up @@ -264,7 +264,11 @@ func (ui *CommandlineUI) ShowInfo(message string) {

func (ui *CommandlineUI) OnApprovedTx(tx ethapi.SignTransactionResult) {
fmt.Printf("Transaction signed:\n ")
spew.Dump(tx.Tx)
if jsn, err := json.MarshalIndent(tx.Tx, " ", " "); err != nil {
fmt.Printf("WARN: marshalling error %v\n", err)
} else {
fmt.Println(string(jsn))
}
}

func (ui *CommandlineUI) OnSignerStartup(info StartupInfo) {
Expand Down
Loading