-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
feat: add customized Ledger support #12935
Changes from all commits
4d28f24
9ebdbc7
f147f07
158e14b
1da973d
820e738
5b385e2
6b9221f
0f3dc68
d171440
0b4fc3c
9d6791f
412cdcb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -144,6 +144,15 @@ type Options struct { | |
SupportedAlgos SigningAlgoList | ||
// supported signing algorithms for Ledger | ||
SupportedAlgosLedger SigningAlgoList | ||
// define Ledger Derivation function | ||
LedgerDerivation func() (ledger.SECP256K1, error) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can't ledger derive keys that aren't on the SECP curve? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Currently, Ledger devices are represented using the |
||
// define Ledger key generation function | ||
LedgerCreateKey func([]byte) types.PubKey | ||
// define Ledger app name | ||
LedgerAppName string | ||
// indicate whether Ledger should skip DER Conversion on signature, | ||
// depending on which format (DER or BER) the Ledger app returns signatures | ||
LedgerSigSkipDERConv bool | ||
} | ||
|
||
// NewInMemory creates a transient keyring useful for testing | ||
|
@@ -213,6 +222,22 @@ func newKeystore(kr keyring.Keyring, cdc codec.Codec, backend string, opts ...Op | |
optionFn(&options) | ||
} | ||
|
||
if options.LedgerDerivation != nil { | ||
ledger.SetDiscoverLedger(options.LedgerDerivation) | ||
} | ||
|
||
if options.LedgerCreateKey != nil { | ||
ledger.SetCreatePubkey(options.LedgerCreateKey) | ||
} | ||
|
||
if options.LedgerAppName != "" { | ||
ledger.SetAppName(options.LedgerAppName) | ||
} | ||
|
||
if options.LedgerSigSkipDERConv { | ||
ledger.SetSkipDERConversion() | ||
} | ||
|
||
return keystore{ | ||
db: kr, | ||
cdc: cdc, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,16 +14,19 @@ import ( | |
"github.com/cosmos/cosmos-sdk/crypto/types" | ||
) | ||
|
||
// discoverLedger defines a function to be invoked at runtime for discovering | ||
// a connected Ledger device. | ||
var discoverLedger discoverLedgerFn | ||
// options stores the Ledger Options that can be used to customize Ledger usage | ||
var options Options | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is there a way to not use a private global? Or is this inherent to how the ledger is setup and used? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The implementation of the Ledger functions depends on the options provided, so there must be some form of local state. Let me know if there is a way to improve it, I am open to any ideas. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we use dependency injection here @kocubinski? |
||
|
||
type ( | ||
// discoverLedgerFn defines a Ledger discovery function that returns a | ||
// connected device or an error upon failure. Its allows a method to avoid CGO | ||
// dependencies when Ledger support is potentially not enabled. | ||
discoverLedgerFn func() (SECP256K1, error) | ||
|
||
// createPubkeyFn supports returning different public key types that implement | ||
// types.PubKey | ||
createPubkeyFn func([]byte) types.PubKey | ||
|
||
// SECP256K1 reflects an interface a Ledger API must implement for SECP256K1 | ||
SECP256K1 interface { | ||
Close() error | ||
|
@@ -35,6 +38,15 @@ type ( | |
SignSECP256K1([]uint32, []byte) ([]byte, error) | ||
} | ||
|
||
// Options hosts customization options to account for differences in Ledger | ||
// signing and usage across chains. | ||
Options struct { | ||
discoverLedger discoverLedgerFn | ||
createPubkey createPubkeyFn | ||
appName string | ||
skipDERConversion bool | ||
} | ||
|
||
// PrivKeyLedgerSecp256k1 implements PrivKey, calling the ledger nano we | ||
// cache the PubKey from the first call to use it later. | ||
PrivKeyLedgerSecp256k1 struct { | ||
|
@@ -46,6 +58,35 @@ type ( | |
} | ||
) | ||
|
||
// Initialize the default options values for the Cosmos Ledger | ||
func initOptionsDefault() { | ||
options.createPubkey = func(key []byte) types.PubKey { | ||
return &secp256k1.PubKey{Key: key} | ||
} | ||
options.appName = "Cosmos" | ||
options.skipDERConversion = false | ||
} | ||
|
||
// Set the discoverLedger function to use a different Ledger derivation | ||
func SetDiscoverLedger(fn discoverLedgerFn) { | ||
options.discoverLedger = fn | ||
} | ||
|
||
// Set the createPubkey function to use a different public key | ||
func SetCreatePubkey(fn createPubkeyFn) { | ||
options.createPubkey = fn | ||
} | ||
|
||
// Set the Ledger app name to use a different app name | ||
func SetAppName(appName string) { | ||
options.appName = appName | ||
} | ||
|
||
// Set the DER Conversion requirement to true (false by default) | ||
func SetSkipDERConversion() { | ||
options.skipDERConversion = true | ||
} | ||
|
||
// NewPrivKeySecp256k1Unsafe will generate a new key and store the public key for later use. | ||
// | ||
// This function is marked as unsafe as it will retrieve a pubkey without user verification. | ||
|
@@ -178,11 +219,11 @@ func convertDERtoBER(signatureDER []byte) ([]byte, error) { | |
} | ||
|
||
func getDevice() (SECP256K1, error) { | ||
if discoverLedger == nil { | ||
if options.discoverLedger == nil { | ||
return nil, errors.New("no Ledger discovery function defined") | ||
} | ||
|
||
device, err := discoverLedger() | ||
device, err := options.discoverLedger() | ||
if err != nil { | ||
return nil, errors.Wrap(err, "ledger nano S") | ||
} | ||
|
@@ -220,6 +261,10 @@ func sign(device SECP256K1, pkl PrivKeyLedgerSecp256k1, msg []byte) ([]byte, err | |
return nil, err | ||
} | ||
|
||
if options.skipDERConversion { | ||
return sig, nil | ||
} | ||
|
||
return convertDERtoBER(sig) | ||
} | ||
|
||
|
@@ -234,7 +279,7 @@ func sign(device SECP256K1, pkl PrivKeyLedgerSecp256k1, msg []byte) ([]byte, err | |
func getPubKeyUnsafe(device SECP256K1, path hd.BIP44Params) (types.PubKey, error) { | ||
publicKey, err := device.GetPublicKeySECP256K1(path.DerivationPath()) | ||
if err != nil { | ||
return nil, fmt.Errorf("please open Cosmos app on the Ledger device - error: %v", err) | ||
return nil, fmt.Errorf("please open the %v app on the Ledger device - error: %v", options.appName, err) | ||
} | ||
|
||
// re-serialize in the 33-byte compressed format | ||
|
@@ -246,7 +291,7 @@ func getPubKeyUnsafe(device SECP256K1, path hd.BIP44Params) (types.PubKey, error | |
compressedPublicKey := make([]byte, secp256k1.PubKeySize) | ||
copy(compressedPublicKey, cmp.SerializeCompressed()) | ||
|
||
return &secp256k1.PubKey{Key: compressedPublicKey}, nil | ||
return options.createPubkey(compressedPublicKey), nil | ||
} | ||
|
||
// getPubKeyAddr reads the pubkey and the address from a ledger device. | ||
|
@@ -270,5 +315,5 @@ func getPubKeyAddrSafe(device SECP256K1, path hd.BIP44Params, hrp string) (types | |
compressedPublicKey := make([]byte, secp256k1.PubKeySize) | ||
copy(compressedPublicKey, cmp.SerializeCompressed()) | ||
|
||
return &secp256k1.PubKey{Key: compressedPublicKey}, addr, nil | ||
return options.createPubkey(compressedPublicKey), addr, nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@austinchandra Just to understand the context of this PR: Is Evmos creating a custom ledger app just for Evmos?
The SDK team is working on SIGN_MODE_TEXTUAL, which improves ledger signing in general, and comes with a Cosmos-wide Ledger app. https://github.com/cosmos/cosmos-sdk/blob/main/docs/architecture/adr-050-sign-mode-textual.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Evmos plans to create a custom Ledger app in the future, which will probably attempt to integrate some variant of SIGN_MODE_TEXTUAL. For now, we are using the Ethereum app to sign Cosmos payloads using EIP-712 Typed Messages, which this PR helps to enable.