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

feat: add customized Ledger support #12935

Merged
merged 13 commits into from
Sep 7, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* (x/authz) [#13047](https://github.com/cosmos/cosmos-sdk/pull/13047) Add a GetAuthorization function to the keeper.
* (cli) [#13064](https://github.com/cosmos/cosmos-sdk/pull/13064) Add `debug prefixes` to list supported HRP prefixes via .
* (cli) [#12742](https://github.com/cosmos/cosmos-sdk/pull/12742) Add the `prune` CLI cmd to manually prune app store history versions based on the pruning options.
* (ledger) [#12935](https://github.com/cosmos/cosmos-sdk/pull/12935) Generalize Ledger integration to allow for different apps or keytypes that use SECP256k1.

### Improvements

Expand Down
6 changes: 6 additions & 0 deletions UPGRADING.md
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,12 @@ By default, the new `MinInitialDepositRatio` parameter is set to zero during mig
feature is disabled. If chains wish to utilize the minimum proposal deposits at time of submission, the migration logic needs to be
modified to set the new parameter to the desired value.

### Ledger

Ledger support has been generalized to enable use of different apps and keytypes that use `secp256k1`. The Ledger interface remains the same, but it can now be provided through the Keyring `Options`, allowing higher-level chains to connect to different Ledger apps or use custom implementations. In addition, higher-level chains can provide custom key implementations around the Ledger public key, to enable greater flexibility with address generation and signing.

This is not a breaking change, as all values will default to use the standard Cosmos app implementation unless specified otherwise.

## [v0.46.x](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.46.0)

### Go API Changes
Expand Down
2 changes: 1 addition & 1 deletion client/cmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,7 @@ func readTxCommandFlags(clientCtx Context, flagSet *pflag.FlagSet) (Context, err
// If the `from` signer account is a ledger key, we need to use
// SIGN_MODE_AMINO_JSON, because ledger doesn't support proto yet.
// ref: https://github.com/cosmos/cosmos-sdk/issues/8109
if keyType == keyring.TypeLedger && clientCtx.SignModeStr != flags.SignModeLegacyAminoJSON {
if keyType == keyring.TypeLedger && clientCtx.SignModeStr != flags.SignModeLegacyAminoJSON && !clientCtx.LedgerHasProtobuf {
fmt.Println("Default sign-mode 'direct' not supported by Ledger, using sign-mode 'amino-json'.")
clientCtx = clientCtx.WithSignModeStr(flags.SignModeLegacyAminoJSON)
}
Expand Down
8 changes: 8 additions & 0 deletions client/context.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ type Context struct {
FeePayer sdk.AccAddress
FeeGranter sdk.AccAddress
Viper *viper.Viper
LedgerHasProtobuf bool
PreprocessTxHook PreprocessTxFn

// IsAux is true when the signer is an auxiliary signer (e.g. the tipper).
Expand Down Expand Up @@ -266,6 +267,13 @@ func (ctx Context) WithAux(isAux bool) Context {
return ctx
}

// WithLedgerHasProto returns the context with the provided boolean value, indicating
// whether the target Ledger application can support Protobuf payloads.
func (ctx Context) WithLedgerHasProtobuf(val bool) Context {
Copy link
Contributor

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

Copy link
Contributor Author

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.

ctx.LedgerHasProtobuf = val
return ctx
}

// WithPreprocessTxHook returns the context with the provided preprocessing hook, which
// enables chains to preprocess the transaction using the builder.
func (ctx Context) WithPreprocessTxHook(preprocessFn PreprocessTxFn) Context {
Expand Down
25 changes: 25 additions & 0 deletions crypto/keyring/keyring.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't ledger derive keys that aren't on the SECP curve?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently, Ledger devices are represented using the SECP256k1 interface. This PR intended only to generalize Ledger support within the realm of secp256k1 keys, but it's possible that generalizing it even further might be more appropriate. Do you think that would be the case?

// 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
Expand Down Expand Up @@ -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,
Expand Down
4 changes: 3 additions & 1 deletion crypto/ledger/ledger_mock.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,11 @@ import (
// set the discoverLedger function which is responsible for loading the Ledger
// device at runtime or returning an error.
func init() {
discoverLedger = func() (SECP256K1, error) {
options.discoverLedger = func() (SECP256K1, error) {
return LedgerSECP256K1Mock{}, nil
}

initOptionsDefault()
}

type LedgerSECP256K1Mock struct{}
Expand Down
4 changes: 3 additions & 1 deletion crypto/ledger/ledger_notavail.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,9 @@ import (
// set the discoverLedger function which is responsible for loading the Ledger
// device at runtime or returning an error.
func init() {
discoverLedger = func() (SECP256K1, error) {
options.discoverLedger = func() (SECP256K1, error) {
return nil, errors.New("support for ledger devices is not available in this executable")
}

initOptionsDefault()
}
10 changes: 8 additions & 2 deletions crypto/ledger/ledger_real.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,18 +3,24 @@

package ledger

import ledger "github.com/cosmos/ledger-cosmos-go"
import (
"github.com/cosmos/cosmos-sdk/crypto/keys/secp256k1"
"github.com/cosmos/cosmos-sdk/crypto/types"
ledger "github.com/cosmos/ledger-cosmos-go"
)

// If ledger support (build tag) has been enabled, which implies a CGO dependency,
// set the discoverLedger function which is responsible for loading the Ledger
// device at runtime or returning an error.
func init() {
discoverLedger = func() (SECP256K1, error) {
options.discoverLedger = func() (SECP256K1, error) {
device, err := ledger.FindLedgerCosmosUserApp()
if err != nil {
return nil, err
}

return device, nil
}

initOptionsDefault()
}
61 changes: 53 additions & 8 deletions crypto/ledger/ledger_secp256k1.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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
Expand All @@ -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 {
Expand All @@ -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.
Expand Down Expand Up @@ -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")
}
Expand Down Expand Up @@ -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)
}

Expand All @@ -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
Expand All @@ -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.
Expand All @@ -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
}