metamask/eth-ledger-bridge-keyring

A wrapper around LedgerJS libraries, to support the KeyringController protocol used by MetaMask

eth-ledger-bridge-keyring

An implementation of MetaMask's Keyring interface, that uses a Ledger hardware wallet for all cryptographic operations.

In most regards, it works in the same way as eth-hd-keyring, but using a Ledger device. However there are a number of differences:

• Because the keys are stored in the device, operations that rely on the device will fail if there is no Ledger device attached, or a different Ledger device is attached.

• It does not support the signMessage, signTypedData or exportAccount methods, because Ledger devices do not support these operations.

• Because extensions have limited access to browser features, there's no easy way to interact wth the Ledger Hardware wallet from the MetaMask extension. This library implements a workaround to those restrictions by injecting (on demand) an iframe to the background page of the extension, (which is hosted here.

The iframe is allowed to interact with the Ledger device (since U2F requires SSL and the iframe is hosted under https) using the libraries from LedgerJS hw-app-eth and hw-transport-u2f and establishes a two-way communication channel with the extension via postMessage.

The iframe code it's hosted in the same repo under the branch gh-pages and it's being served via github pages. In the future we might move it under the metamask.io domain.

Usage

In addition to all the known methods from the Keyring class protocol, there are a few others:

• isUnlocked : Returns true if we have the public key in memory, which allows to generate the list of accounts at any time

• unlock : Connects to the Ledger device and exports the extended public key, which is later used to read the available ethereum addresses inside the Ledger account.

• setAccountToUnlock : the index of the account that you want to unlock in order to use with the signTransaction and signPersonalMessage methods

• getFirstPage : returns the first ordered set of accounts from the Ledger account

• getNextPage : returns the next ordered set of accounts from the Ledger account based on the current page

• getPreviousPage : returns the previous ordered set of accounts from the Ledger account based on the current page

• forgetDevice : removes all the device info from memory so the next interaction with the keyring will prompt the user to connect the Ledger device and export the account information

Testing and Linting

Run yarn test to run the tests once. To run tests on file changes, run yarn test:watch.

Run yarn lint to run the linter, or run yarn lint:fix to run the linter and fix any automatically fixable issues.

Attributions

This code was inspired by eth-ledger-keyring and eth-hd-keyring

eth-ledger-bridge-keyring's Issues

useLedgerLive setting isn't always in sync

This is fairly simple to reproduce:

1. Connect a Ledger account to MetaMask
2. (eth-ledger-bridge-keyring injects the github pages iframe with correct useLedgerLive setting)
3. Use the test dapp to perform a signature
4. Remove the Ledger account from MetaMask
5. Add the account again

The useLedgerLive setting will be false in the IFrame, leading to the user's browser hanging (while the iframe incorrectly tries U2F.

Presently MetaMask sends the useLedgerLive setting down to the iframe when the user logs in, and when they toggle the setting. That isn't reliable enough in some cases.

Elaborate Serializing and Deserializing of Data

Some improvements can be made to the serializing and deserializing of data:

• this.hdk can/should be serialised and deserialised
  • Only the publicKey and chainCode needs to serialised/deserialised
  • Setting this.hdKey can be refactored via a setHdKey method
• this.page and this.unlockedAccount should be included in data serialization
• this.hdPath can be changed via deserialization without triggering the this.setHdPath method
• implementFullBIP44 is incorrectly serialised
  • Should be implementFullBIP44: this.implementFullBIP44 instead of implemntFullBIP44: false
• Some "resetting" of data can be refactored (i.e., see forgetDevice, setHdPath and the constructor)
• Some refactoring for filtering this.accounts can be done
• this.bridgeUrl can be changed via deserialisation without updating the iframe URL
  • This should either be only settable via init, or should trigger iframe deletion and recreation (less desireable)

Ensure thrown errors always bubble to the extension appropriately

I haven't been able to determine if this problem is on the extension or keyring side, but it seems as though every error that occurs with Ledger integration makes its way to the extension UI as Internal JSON-RPC error., which is incredibly unhelpful for the dev teams and users being able to report issues.

This should be a top priority moving forward with Ledger work.

Add LICENSE file

This project is missing a LICENSE file

Handling Multiple Init's

Issues:

1. LedgerBridgeKeyring can accidentally be initialised twice on the same client, resulting in silent failures (e.g. messages be either duplicated or resolved prematurely/incorrectly).
2. Now that set-up logic is being moved to a separate init function, there's a possibility that the same instance is init'ed twice, and thus requires appropriate handling and fail-safes

• setupIframe to throw an error if it detects a duplicate iframe
• this.currentMessageId to be removed and replaced with nanoid
  • Ensures that two different clients sending messages to the same bridge URL will always have a unique (instead of being 0-indexed and incrementing)
  • Reduces code complexity
• Prevent this.bridgeUrl from being null before the iframe is created, or at least update the iframe if the keyring is deserialized with a new bridgeUrl
  • See this ticket for more details: #161
• Safe-proofing delayed promise: #162
• this._eventListener will be mutated if the keyring is init'ed twice, which would interfere with:
  • window.removeEventListener('message', this._eventListener)
  • Solution to this may be to just remove this._eventListener completely, and replace it with something like this:

type WindowEventHandlerTuple = readonly [
    keyof WindowEventHandlersEventMap,
    (this: WindowEventHandlers, ev: WindowEventHandlersEventMap[keyof WindowEventHandlersEventMap]) => any
];

class LedgerBridgeKeyring extends EventEmitter {
    _windowEventListeners: ReadonlyArray<WindowEventHandlerTuple> = [];


  _setupListener () {
    const messageListener = ({ origin, data }) => {
      // ...
    }
    
    this._addWindowEventListener(['message', messageListener])
  }
  
  _addWindowEventListener(windowEventHandler:  WindowEventHandlerTuple){
     window.addListener(...windowEventHandler);
     this._windowEventListeners = [...this._windowEventListeners, windowEventHandler];
  }
  
  destroy(){
    for(const listener of this._windowEventListeners){
      window.removeListener(...listener);
    }
  }
} 

Ledger keyring should implement Keyring type

The Ledger keyring should extend the Keyring type as other keyrings.

`updateTransportMethod` doesn't resolve when called multiple times successively before iframe loads

If updateTransportMethod is called a second time before the iframe loads and before the first call resolves, the first call will never resolve.

We should queue up delayed promises to ensure none of them fail to resolve. Maybe with some timeout handling or max queue size to handle bizarre usage correctly.

Migrate from Yarn v1 to v3

The version of Yarn that this repo relies upon is v1. We want to this repo to v3 to bring this repo in alignment with our other repos.

• Run yarn set version stable to install Yarn v3.
• Copy .yarnrc.yml and .yarn/plugins from the module template.
• Run yarn install to generate a new version of yarn.lock.
• As our Yarn v3 assumes that @lavamoat/allow-scripts is installed, we will want to add that as a development dependency. In addition, we will want to add a lavamoat section to package.json. Again, see the module template for the correct version of allow-scripts to use as well as where the new lavamoat section should go.

Safe-Proofing delayedPromise

If someone init's the keyring twice, or calls updateTransportMethod twice quickly while the iframe is loading then you could run into situations where this if block runs (see here) but when this.delayedPromise.resolve(result) or this.delayedPromise.reject(e) is called the object is deleted by a separate execution context.

Ways that this could be fixed:

• change this.delayedPromise to this.delayedPromises: Array<Object> = [], and then run something like:

for (const delayedPromise of this.delayedPromises) {
    this.updateTransportMethod(delayedPromise.transportType)
         .then((res) => delayedPromise.resolve(res))
         .catch(e => delayedPromise.reject(e))
}

this.delayedPromises = [];

// and then when the promise is added we do 
this.delayedPromises.push({
 // ...
})

• or, to allow for more flexibility, this.delayedPromise could be changed to something like this.iframeOnloadHooks: Array<Promise> and the whole this.updateTransportMethod could be contained in the array, in case someone needs to add in a different onload call in later?

Bring linting pipeline in sync with module template

Our ESLint config is out of date with our other repos, and this repo is missing Prettier as well as depcheck.

• Copy the versions of our ESLint config packages that the module template is using. If this issue is addressed before #166 and #23, we would want to omit Jest and TypeScript packages.
• Ensure that .eslintrc.js is up to date. If this issue is addressed before #166 and #23, we would want to omit Jest and TypeScript-related config.
• Add Prettier and Prettier-related packages as development dependencies, and copy .prettierrc.js from the module template
• Ensure that all lint:* scripts in the module template are reflected in this repo.
• Add depcheck as a development dependency, and copy .depcheckrc.json from the module template.
• Fix lint violations caused by the upgrade. Running yarn lint:fix should automatically fix most of them, but we will need to manually fill in missing JSDocs.

Upgrade Ledger Dependencies and Fix Build Task Alias Issue

There is a need to upgrade Ledger dependencies to the latest versions and fix an issue with the build task where the alias for "@ledgerhq/devices": "@ledgerhq/devices/lib-es", is required for the new dependencies.

Fix typing of `IFrameMessageResponse` and refactor into modular types

IFrameMessageResponse is currently defined with a generic parameter TAction, which is constrained as a subtype of IFrameMessageActions. This parameter doesn't usefully narrow or widen the type, and causes two problems:

1. Type errors on assignment operations that should be valid.

• e.g. as type casting: failing because it's trying to assign a supertype to a subtype i.e. (response: IFrameMessageResponse<TAction>) => void to (response: IFrameMessageResponse<IFrameMessageActions>) => void.
  • In general, (x: SuperType) => void is a subtype of (x: SubType) => void.
• This error indicates an underlying issue with the typing and shouldn't be silenced.
• Removing TAction puts (response: SomeSpecificIFrameMessageResponse) => void on the LHS (assignee, supertype) and (response: IFrameMessageResponse) => void on the RHS (assigned, subtype), resolving this logical error.

2. TAction also interferes with type narrowing based on action value, and is silencing type errors. Some union members of IFrameMessageResponse do not include a success, error, payload, or payload.error property, but because of the action: TAction property, TypeScript doesn't alert us that we should be performing in checks on them in addition to null checks. This can cause some of the existing destructuring expressions to unexpectedly fail at runtime.

Constraining IFrameMessageResponse['action'] to IFrameMessageActions both resolves these issues and guides us towards writing type-safe logic about these actions that conform to their specific type signatures. It appears that this was the original intention of writing IFrameMessageActions as a discriminated union instead of a wider type encompassing all of the actions.

Tasks

• To resolve these issues, This make IFrameMessageResponse non-generic and removes as casting.
• For improved readability and maintainability, refactor IFrameMessageResponse into a union of named types.
• Define a IFrameMessageResponseBase type for modularity and better visibility of IFrameMessageResponse types with atypical shapes.

[Ledger] - Implement eth-ledger-bridge-keyring library MMM

Info

There is a ledger keyring library @metamask/eth-ledger-bridge-keyring, it is currently being implemented in MetaMask Extension

MM team wanna us to extend this library to enable the integration of ledger keyring in mobile

In result, we have to replace the existing ledger-keyring library in mobile from @consensys/ledgerhq-metamask-keyring to @metamask/eth-ledger-bridge-keyring

Tasks:

1. decouple PR in @metamask/eth-ledger-bridge-keyring into [Stanley]

• A PR to existing software architecture to enable multiple options for connection bridge
• A PR to implement the mobile connection bridge into the updated software architecture

Replacing original zenhub ticket https://app.zenhub.com/workspaces/cet-metamask-hardware-wallets-6566ee6aedc46007d5a260bb/issues/zh/106

Ledger dependencies bump to 5.36.0

LedgerHQ has released 5.36.0 of all dependencies which includes support for chainIDs above 108. Please bump dependencies on https://github.com/MetaMask/eth-ledger-bridge-keyring/tree/gh-pages . CC @brunobar79

Use shared ESLint config

This project should use the org's shared ESLint config.

Sometimes messages to injected iframe don't get a reply

When we send a message to the injected iframe, we attach a message handler that waits for a reply, assuming that the only reply will be the one that it expects. If it receives a different reply than expected, the message handler is removed and the callback is never called, leaving that method pending forever. This can happen when making multiple calls to the keyring in quick succession.

We should keep the reply listener attached until we see a reply, and ignore unexpected messages that might be responses to different calls. We should also consider adding a timeout, in case the reply never comes.

Message replies are sometimes mixed up

When we send a message to the injected iframe, we assume the first reply we see is the response to that message. That might not be the case if we call the same method multiple times in quick succession.

To ensure we don't get our replies mixed up, we should attach a unique identifier to each message and look for it in the reply.

Please upgrade hw-app-eth to make it support EIP1559 and EIP2930

Hello,

We have recently merged LedgerHQ/ledgerjs#637
which brings support for EIP1559 and EIP2930 in combination with Nano App v1.8.9 (currently available as a rc1 in Experimental Manager Provider 4 on Ledger Live)

I noticed that you will have to upgrade the library https://github.com/MetaMask/eth-ledger-bridge-keyring/blob/gh-pages/package.json#L23 to make it possible to do such transaction. This needs to be at least v6.3.0 of hw-app-eth (see https://github.com/LedgerHQ/ledgerjs/releases)

the v5 -> v6 shouldn't be breaking. It was made a major bump when we migrated from FlowType to TypeScript but we still target to JS bundle that should work without you to change anything beyond that.

It would be great if you have the ability to test this end to end, but if you can't, we would be happy to test on the matter (if you tell us the combination of what we need to do with MetaMask plugin for it to produce EIP1559 transactions)

Thanks and sorry for such short notice.

Ledger nano S can`t interact with Eth2 Staking Smart Contract (Metamask)

Ledger Live Version and Operating System
tested on Ledger Live Ledger nano X, firmware 1.2.4-5 , ETH 1.6.1
Platform and version: Windows 10
Expected behavior
Actual behavior
Steps to reproduce the behavior
Ledger cannot interact with the Eth2 SmartContrat (launchpad), the contract is not signed.

Contract data: Allow contract data in transactions -> Allowed
Ledger nano X,
Firmware 1.2.4-5 ,
App ETH 1.6.1
I have tried with different browsers, with Brave, Chrome + Metamask and with another usb cable.

the problem is happening to many people with the latest firmware.

MetaMask - RPC Error: Error: TransportStatusError: Ledger device: Condition of use not satisfied (denied by the user?) (0x6985) {code: -32603, message: "Error: TransportStatusError: Ledger device: Condit… use not satisfied (denied by the user?) (0x6985)", stack: "Error: Error: TransportStatusError: Ledger device:… use not satisfied (denied by the user?) (0x6985)"}

LedgerHQ/app-ethereum#116

There is already a PR and has been for months
#45

Add security policy

See https://help.github.com/en/github/managing-security-vulnerabilities/adding-a-security-policy-to-your-repository

Add support for getEIP1024PublicEncryptionKey

Add support for getEIP1024PublicEncryptionKey since the latest hw-app-eth now supports it.

This issue relates to MetaMask/metamask-extension#16716

v1 Release

This issue tracks the progress of a v1 release.

Tasks:

• Migrate to TypeScript (#23)
• Add a CHANGELOG (#22)
• Add a SECURITY.md file (#16)

[Clear Signing] Bundler not compatible with subpath exports in `gh-pages`

Unfortunately, after trying to implement the new EIP712 clear signed method from @ledgerhq/hw-app-eth, we discovered that the bundler used in the gh-pages branch isn't compatible with subpath exports used by the Ledger librairies in order to provide both commonjs and esmodules versions.

Documentation talking about it is very hard to find, I could only find 1 issue talking about it here.

Bundling with Parcel or Browserify
These bundlers do not (yet) support package.json exports, so --moduleResolution node is still a reasonably good fit.

Parcel on its end has recently been updated in May to support this: https://parceljs.org/features/dependency-resolution/#package-exports.

Without a removal of Browserify, it doesn't look possible for Ledger to update its packages to the latest version and include the new features built into it.

Migrate from Mocha/Chai to Jest

We've standardized on using Jest instead of Mocha for all of our libraries.

• Remove mocha, chai, and chai-spies as dependencies and replace it with jest
• Copy jest.config.js from the module template
• Remove the resolutions field from package.json as we don't need it
• Convert existing tests from Mocha/Chai syntax to Jest
• Replace the test script in package.json with the equivalent jest line (see the module template), and add a test:watch script
• If #164 has already been completed, then add our eslint-config-jest package to package.json and add a Jest section to .eslintrc.js (see the module template)
• Run yarn test. If Jest does not report that 100% test coverage has not been achieved, then add jest-it-up as a development dependency; otherwise adjust the coverage thresholds in jest.config.js to match the numbers reported.

Tasks:

• Migrate to 'jest'
• Add jest-it-up

Definition of Done

• Whenever applies, any change unit tested, reviewed(approved) and documented(JSDOC at least)
• We have changelog entries for any related changes
• Any changed APIs have comprehensive inline documentation
• Any changed public APIs are well covered by unit tests
• Other items
• If a planning/research ticket, then the plan has been reviewed and approved by at least 1 team members

Migrate to TypeScript

(Edited) We've standardized on using TypeScript for our libraries.

• Add typescript as a development dependencies, and copy tsconfig.json and tsconfig.build.json from the module template.
• Convert existing JavaScript syntax to TypeScript
• Update the build script in package.json to use tsc
• Add a build:clean script
• Add typedoc as development dependencies, copy the build:docs package script from the module template, and copy typedoc.json
• Add ts-node and @types/node as development dependencies
• If this issue is started after #164 is complete, then:
  • Add @metamask/eslint-config-typescript and @typescript-eslint/* packages
  • Ensure that the lint:eslint package script includes ts files
  • Add TypeScript config to .eslintrc.js (again, see the module template)
• If this issue is started after #166 is complete, then:
  • Add @types/jest and ts-jest as development dependencies

Notice of possible breaking change for dapps relying on the result of EIP-712 signatures

In as soon as 4 weeks time, MetaMask will release a fix to our EIP-712 signing code for ledger devices, and this has the possibility of breaking dapps which explicitly relied on our raw signature return values.

As per the Ethereum Yellow Paper, the recovery identifier v - the last byte of the signature - should be either 27 (0x1b) or 28 (0x1c). However, since Metamask started supporting signatures for EIP-712 message, we have (incorrectly) returned signatures with a v value of either 0 or 1. While our signing logic is correct, and messages have been signed with the correct keys, the signature as returned to your dapp would have ended with the incorrect byte. To further illustrate the problem, some projects have had to explicitly work around this by explicitly checking the v value on the returned signature and if it matches 0 or 1, adding 27 to it.

A fix to this issue was made in a PR to our ledger keyring repo last summer: https://github.com/MetaMask/eth-ledger-bridge-keyring/pull/152/files This fix will be included in an upcoming release. With this fix, the last byte of the signature returned to your dapp, when a user with a Ledger account signs an EIP-712 message, will change.

For example, given the exact same inputs and signed with the exact same account, a signature that previously was returned as this 0x72d4e38a0e582e09a620fd38e236fe687a1ec782206b56d576f579c026a7e5b946759735981cd0c3efb02d36df28bb2feedfec3d90e408efc93f45b894946e3200 would now be returned as 0x72d4e38a0e582e09a620fd38e236fe687a1ec782206b56d576f579c026a7e5b946759735981cd0c3efb02d36df28bb2feedfec3d90e408efc93f45b894946e321b

Note that recovery will not be impacted: if you could recover an address from any given signature before this change, you will still be able to recover an address after this change.

But this could break functionality of your dapp, for Ledger users, if you relied on the explicit signature hex string returned by MetaMask. In such a case, you may need to either migrate saved data or add logic that checks the last two bytes for either scenario (v being 0 or 1, OR v being 27 or 28).

This change will be released as early as 4 weeks from this posting.

Illegal EIP712 signature for signTypedData

Hi, there.

We are receiving some illegal EIP712 signatures from the ledger wallet, the v is 0 or 1. As we know, v should be 27 or 28 as a valid EIP712 signature.

So I'm checking the source code as follows:

eth-ledger-bridge-keyring/index.js

Lines 417 to 430 in 9a87fac

	let v = payload.v - 27
	v = v.toString(16)
	if (v.length < 2) {
	v = `0${v}`
	}
	const signature = `0x${payload.r}${payload.s}${v}`
	const addressSignedWith = sigUtil.recoverTypedSignature_v4({
	data,
	sig: signature,
	})
	if (ethUtil.toChecksumAddress(addressSignedWith) !== ethUtil.toChecksumAddress(withAccount)) {
	throw new Error('Ledger: The signature doesnt match the right address')
	}
	return signature

I guest it minus 27 because it wants to recover the signer, but it forgot to return the original signature. The signature should be return 0x${payload.r}${payload.s}${payload.v}, not return signature

According to this MetaMask/metamask-extension#10240 (comment), I found some projects also have this issue.

• https://github.com/poap-xyz/poap-fun/pull/62
• gallery-so/go-gallery#164

So I think maybe we can fix the root cause?

Replace CircleCI workflow with GitHub Actions + other standardizations

This repo uses CircleCI for running tests against pull requests and the main branch. We have standardized on using GitHub Actions for our other libraries.

• Copy the .github/ and scripts/ directories from the module template.
• Add typedoc as a development dependency.
• Copy build:docs script from the module template.
• Push up a PR to verify that the workflow is run successfully
• Copy .editorconfig, .gitignore, and .gitattributes from the module template
• Add @metamask/auto-changelog as a development dependency
• Copy prepack script from the module template

Our workflows assume that the repo is using Yarn v3 and that the lint, test, build, and build:docs package scripts exist. Therefore, this issue should be started after #23, #164, and #165 are complete.

Update node engine to v14

This package is still using Node.js v12, while our module template uses v16.

We should:

• Use v16 in our .nvmrc file
• Update engines section in package.json file with:

{
    "node": ">=14.0.0"
} 

• Update nodejs version used by circle-ci workflows

This is needed also to add @lavamoat/allow-scripts and update to yarn v3 (See #165)

can not connect to ledger wallet

found this error in background console:

looks like Chrome update cross-origin policy in Chrome 91.0.4472.77, and actually same issue with trezor, trace: trezor/trezor-suite#4878

Add CHANGELOG

Add a CHANGELOG

Standardize repo

This repo is quite out of date with the current standards of MetaMask modules. We should update this repository to follow the conventions used in the module template.

BIP44 derivation mode should stop at first empty account

per bip44 spec, the derivation of accounts should stop at first account without transaction:

Software should prevent a creation of an account if a previous account does not have a transaction history (meaning none of its addresses have been used before).

do you plan to support this? I know it can be challenging in that it requires you to asynchronously fetch if a given address have transactions.

The potential problem when not implementing this however is that we'll end up with accounts that are not discovered by Ledger Live (or any other application that strictly follow bip44) if people start selecting accounts without respecting the order (which happened a lot with MEW and that's why we forced to scan at least 10 accounts in advance).