Skip to content

Commit ceac736

Browse files
tests/ledger signing (#194)
By Default, the `SignText` function in the geth usbwallet package returns NotSupported. To resolve this, we fork that package and enable that function to be used and use that here. Also, this PR also updates some of the `Sign` logic in `Signable`: * `SigningHash()` always returns the hash with the EIP 191 prefix * `SigningMessage()` only returns the hash without the EIP 191 prefix * The `Sign` method in `Signable` is updated to always use `SigningMessage` instead of `SigningHash` (this is because the EIP 191 prefix is automatically added in the ledger case) * The `Sign` function on the private key signer is updated to always add an EIP 191 prefix before signing This fixes two issues: * The `NotSupported` error when using ledger * The mismatching recovered address in the ledger E2E test --------- Co-authored-by: ajaskolski <[email protected]> Co-authored-by: ajaskolski <[email protected]>
1 parent 5fbf3e8 commit ceac736

14 files changed

+1627
-25
lines changed

.golangci.yml

+1
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,7 @@ issues:
9393
# Exclude the directory from linting because it will soon be removed and only contains
9494
# generated code. Can be removed once the gethwrappers directory is removed.
9595
- sdk/evm/bindings
96+
- sdk/usbwallet
9697
exclude-rules:
9798
- path: _test\.go
9899
linters:

e2e/ledger/ledger_test.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,6 @@ func TestManualLedgerSigning(t *testing.T) { //nolint:paralleltest
9595
recoveredAddr, err := signature.Recover(hash)
9696
require.NoError(t, err, "Failed to recover signer address")
9797

98-
require.Equal(t, accountPublicKey, recoveredAddr, "Signature verification failed")
98+
require.Equal(t, accountPublicKey, recoveredAddr.Hex(), "Signature verification failed")
9999
t.Logf("Signature verified successfully. Signed by: %s\n", recoveredAddr.Hex())
100100
}

go.mod

+1-1
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ require (
66
github.com/ethereum/go-ethereum v1.14.12
77
github.com/go-playground/validator/v10 v10.23.0
88
github.com/joho/godotenv v1.5.1
9+
github.com/karalabe/hid v1.0.1-0.20240306101548-573246063e52
910
github.com/smartcontractkit/chain-selectors v1.0.34
1011
github.com/smartcontractkit/chainlink-testing-framework/framework v0.3.6
1112
github.com/spf13/cast v1.7.0
@@ -82,7 +83,6 @@ require (
8283
github.com/holiman/uint256 v1.3.1 // indirect
8384
github.com/huin/goupnp v1.3.0 // indirect
8485
github.com/jackpal/go-nat-pmp v1.0.2 // indirect
85-
github.com/karalabe/hid v1.0.1-0.20240306101548-573246063e52 // indirect
8686
github.com/klauspost/compress v1.17.9 // indirect
8787
github.com/kr/pretty v0.3.1 // indirect
8888
github.com/kr/text v0.2.0 // indirect

proposal.go

+2-11
Original file line numberDiff line numberDiff line change
@@ -184,13 +184,13 @@ func (p *Proposal) SigningHash() (common.Hash, error) {
184184
return common.Hash{}, err
185185
}
186186

187-
return toEthSignedMessageHash(msg), nil
187+
return toEthSignedMessageHash(msg.Bytes()), nil
188188
}
189189

190190
// SigningMessage generates a signing message without the EIP191 prefix.
191191
// This function is used for ledger contexts where the ledger itself will apply the EIP191 prefix.
192192
// Corresponds to the input here https://github.com/smartcontractkit/ccip-owner-contracts/blob/main/src/ManyChainMultiSig.sol#L202
193-
func (p *Proposal) SigningMessage() ([32]byte, error) {
193+
func (p *Proposal) SigningMessage() (common.Hash, error) {
194194
tree, err := p.MerkleTree()
195195
if err != nil {
196196
return common.Hash{}, err
@@ -263,15 +263,6 @@ func (p *Proposal) GetEncoders() (map[types.ChainSelector]sdk.Encoder, error) {
263263
return encoders, nil
264264
}
265265

266-
func toEthSignedMessageHash(messageHash common.Hash) common.Hash {
267-
// Add the Ethereum signed message prefix
268-
prefix := []byte("\x19Ethereum Signed Message:\n32")
269-
data := append(prefix, messageHash.Bytes()...)
270-
271-
// Hash the prefixed message
272-
return crypto.Keccak256Hash(data)
273-
}
274-
275266
// proposalValidateBasic basic validation for an MCMS proposal
276267
func proposalValidateBasic(proposalObj Proposal) error {
277268
validUntil := time.Unix(int64(proposalObj.ValidUntil), 0)

sdk/usbwallet/README.md

+10
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
# Notes
2+
3+
- Lifted from Geth v1.14.7 source code accounts/usbwallet directory.
4+
- Note that Geth < 1.14 version suffers from this issue described and patched here https://github.com/ethereum/go-ethereum/pull/28945.
5+
- Removed trezor
6+
- Modified to add EIP 191 support (SignPersonalMessage). The Geth library does not implement EIP 191
7+
intentionally, as it is less secure than its successor EIP 712. However, in the case of MCMS we explicitly
8+
want the cross chain replayability possibility of EIP 191. Luckily the ledger communication
9+
protocol is already fully supported. Aside from adding the new methods to the hub and wallet interfaces,
10+
the diff is localized to eip191.go.

sdk/usbwallet/eip191.go

+113
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,113 @@
1+
package usbwallet
2+
3+
import (
4+
"encoding/binary"
5+
"fmt"
6+
7+
"github.com/ethereum/go-ethereum/accounts"
8+
"github.com/ethereum/go-ethereum/crypto"
9+
)
10+
11+
var (
12+
// Add support for ledger sign personal message.
13+
ledgerP1InitPersonalMessageData ledgerParam1 = 0x00 // First chunk of Personal Message data
14+
ledgerP1ContPersonalMessageData ledgerParam1 = 0x80 // Next chunk of Personal Message data
15+
ledgerOpSignPersonalMessage ledgerOpcode = 0x08 // Signs an ethereum personal message
16+
)
17+
18+
// signMessage implements accounts.Wallet
19+
// Used for EIP191 (personal signing).
20+
func (w *wallet) signMessage(account accounts.Account, message []byte) ([]byte, error) {
21+
w.stateLock.RLock() // Comms have own mutex, this is for the state fields
22+
defer w.stateLock.RUnlock()
23+
24+
// If the wallet is closed, abort
25+
if w.device == nil {
26+
return nil, accounts.ErrWalletClosed
27+
}
28+
// Make sure the requested account is contained within
29+
path, ok := w.paths[account.Address]
30+
if !ok {
31+
return nil, accounts.ErrUnknownAccount
32+
}
33+
// All infos gathered and metadata checks out, request signing
34+
<-w.commsLock
35+
defer func() { w.commsLock <- struct{}{} }()
36+
37+
// Ensure the device isn't screwed with while user confirmation is pending
38+
// TODO(karalabe): remove if hotplug lands on Windows
39+
w.hub.commsLock.Lock()
40+
w.hub.commsPend++
41+
w.hub.commsLock.Unlock()
42+
43+
defer func() {
44+
w.hub.commsLock.Lock()
45+
w.hub.commsPend--
46+
w.hub.commsLock.Unlock()
47+
}()
48+
signature, err := w.driver.SignPersonalMessage(path, message)
49+
if err != nil {
50+
return nil, err
51+
}
52+
return signature, nil
53+
}
54+
55+
func (w *ledgerDriver) SignPersonalMessage(path accounts.DerivationPath, message []byte) ([]byte, error) {
56+
// If the Ethereum app doesn't run, abort
57+
if w.offline() {
58+
return nil, accounts.ErrWalletClosed
59+
}
60+
// Ensure the wallet is capable of signing the given transaction
61+
if w.version[0] < 1 && w.version[1] < 2 {
62+
//lint:ignore ST1005 brand name displayed on the console
63+
return nil, fmt.Errorf("Ledger version >= 1.2.0 required for personal signing (found version v%d.%d.%d)", w.version[0], w.version[1], w.version[2])
64+
}
65+
return w.ledgerSignPersonalMessage(path, message)
66+
}
67+
68+
func (w *ledgerDriver) ledgerSignPersonalMessage(derivationPath []uint32, message []byte) ([]byte, error) {
69+
// Flatten the derivation path into the Ledger request
70+
path := make([]byte, 1+4*len(derivationPath))
71+
path[0] = byte(len(derivationPath))
72+
for i, component := range derivationPath {
73+
binary.BigEndian.PutUint32(path[1+4*i:], component)
74+
}
75+
var messageLength [4]byte
76+
binary.BigEndian.PutUint32(messageLength[:], uint32(len(message)))
77+
payload := append(path, messageLength[:]...)
78+
payload = append(payload, message...)
79+
// Send the request and wait for the response
80+
var (
81+
op = ledgerP1InitPersonalMessageData
82+
reply []byte
83+
err error
84+
)
85+
fmt.Println("Derivation path: ", path)
86+
// Chunk size selection to mitigate an underlying RLP deserialization issue on the ledger app.
87+
// https://github.com/LedgerHQ/app-ethereum/issues/409
88+
chunk := 255
89+
for ; len(payload)%chunk <= ledgerEip155Size; chunk-- {
90+
}
91+
92+
for len(payload) > 0 {
93+
// Calculate the size of the next data chunk
94+
if chunk > len(payload) {
95+
chunk = len(payload)
96+
}
97+
// Send the chunk over, ensuring it's processed correctly
98+
reply, err = w.ledgerExchange(ledgerOpSignPersonalMessage, op, 0, payload[:chunk])
99+
if err != nil {
100+
return nil, err
101+
}
102+
// Shift the payload and ensure subsequent chunks are marked as such
103+
payload = payload[chunk:]
104+
op = ledgerP1ContPersonalMessageData
105+
}
106+
107+
// Extract the Ethereum signature and do a sanity validation
108+
if len(reply) != crypto.SignatureLength {
109+
return nil, fmt.Errorf("reply lacks signature: reploy %v", reply)
110+
}
111+
signature := append(reply[1:], reply[0])
112+
return signature, nil
113+
}

0 commit comments

Comments
 (0)