Ledger support #26

bargst · 2018-06-05T20:51:13Z

What was wrong?

Issue #25

How was it fixed?

Create a class LedgerAccount

Cute Animal Picture

pipermerriam

Some preliminary feedback.

pipermerriam · 2018-06-05T22:12:09Z

eth_account/ledger.py

+ if len(element) == 1:
+ result = result + struct.pack(">I", int(element[0])) 
+ else:
+ result = result + struct.pack(">I", 0x80000000 | int(element[0]))


Any chance we can get an explanation of what this code is doing and why with some inline comments?

pipermerriam · 2018-06-05T22:13:56Z

eth_account/ledger.py

+ offset = 1 + result[0]
+ address = result[offset + 1 : offset + 1 + result[offset]]
+
+ return f'0x{address.decode()}'


This should use eth_utils.to_normalized_address (assuming address is a bytes value.

pipermerriam · 2018-06-05T22:14:15Z

eth_account/ledger.py

+ result = result + struct.pack(">I", 0x80000000 | int(element[0]))
+ return result
+
+ def get_addresses(self, offset):


The name of this function suggests that it will return multiple addresses, but it seems to only return a single one.

pipermerriam · 2018-06-05T22:15:47Z

eth_account/ledger.py

+ """
+ offset = 0
+ while address != self.get_addresses(offset):
+ offset += 1


Instead of this mutation pattern, how about:

import itertools for offset in itertools.count(): ...

Also, lets set an upper bound on this to some reasonable value (like 20?).

Also, lets set an upper bound on this to some reasonable value (like 20?).

I'm not sure we can define what maximum number of addresses should be allowed against a wallet. I guess we can start with something like: how many addresses can we check in one second? At the least, we would need an escape hatch for people who might have tons of addresses, like:

def get_offset(self, address, maximum_search_length=100):

(but with a better name)

Good plan, like the kwarg. how about search_limit? We should probably also allow for specifying the starting offset value.

bargst · 2018-06-06T22:04:09Z

Thanks for the feedback, I updated accordingly !

I will probably change the offset name to something more explicit like account_id.

Next is signTransaction() then encrypt() and signHash()

pipermerriam · 2018-06-07T01:21:03Z

@carver probably something for us to do separately, but this feels like the time to create an abstract base class for accounts so the public required API is explicitly defined.

carver · 2018-06-07T18:06:26Z

Note that encrypt() is probably not possible to implement (I hope!), so just the other two are sufficient. A draft of the spec is in #28.

Edit: see #29 for a preview of BaseAccount, which defines the appropriate interface

dylanjw · 2018-06-07T21:47:30Z

eth_account/ledger.py

 class LedgerAccount:
- """
+ '''


Have a look at the PEP docstring format https://www.python.org/dev/peps/pep-0257/, I think it allows the docstring to get parsed correctly.

Nevermind, I just noticed all the docstrings in the project use triple single quotes.

dylanjw · 2018-06-07T22:05:57Z

eth_account/ledger.py

+
+ reply = self.device.exchange(apdu, timeout=60)
+ except:
+ raise Exception('Something went wrong when communicating with device')


Im not too keen on masking the underlying exception in all cases. Are there specific exceptions raised by the exchange method that could be made more friendly?

Ok, I will remove the try catch

carver · 2018-06-07T22:21:13Z

After a rebase on master, you can subclass BaseAccount.

bargst · 2018-06-08T20:39:03Z

@carver signHash(), as currently defined, is not doable with the ledger: what is sent to the device is not the hash of the message, it is the whole message. The message is then hashed directly in the ledger and vrs signature are then returned.

Is it possible to have the message passed to signHash() in BaseAccount ?

bargst · 2018-06-12T15:44:29Z

@pipermerriam @carver I consider dev done, could you review ?
What's missing is commit history clean-up and a decision about signHash() issue.

carver · 2018-06-12T20:59:15Z

decision about signHash() issue.

Yeah, signing is ugly right now. The process for hashing messages is not well standardized. It's baked into the hardware keys, so we can't have a single signMessage implemented by all the different signers as a drop-in replacement. They all do it a little differently.

So to support ledger signing, it should probably just be a custom method. ledgerSignMessage() is an option for the name. Alternatively, you could name it defunctSignMessage() (if it's the exact same hashing process as geth). It's called the defunct style elsewhere in eth-account.

You can raise a NotImplementedError from signHash saying that the Ledger doesn't support signing opaque hashes.

What's missing is commit history clean-up and a decision about signHash() issue.

Also, passing CI tests :)

dylanjw · 2018-06-12T21:01:53Z

Because ledgerblue depends on pycrypto, and some of the other dependencies (eth-keyfile) have replaced it with pycryptodome, we are getting errors on this branch, like the following:

ImportError while importing test module '/home/dwilson/Develop/Projects/ethereum/eth-account/tests/core/test_ledger.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
tests/core/test_ledger.py:5: in <module>
    from eth_account import (
eth_account/__init__.py:1: in <module>
    from eth_account.account import Account  # noqa: F401
eth_account/account.py:10: in <module>
    from eth_keyfile import (
venv/lib/python3.6/site-packages/eth_keyfile/__init__.py:7: in <module>
    from eth_keyfile.keyfile import (  # noqa: F401
venv/lib/python3.6/site-packages/eth_keyfile/keyfile.py:6: in <module>
    from Crypto.Cipher import AES
venv/lib/python3.6/site-packages/Crypto/Cipher/__init__.py:3: in <module>
    from Crypto.Cipher._mode_ecb import _create_ecb_cipher
venv/lib/python3.6/site-packages/Crypto/Cipher/_mode_ecb.py:29: in <module>
    from Crypto.Util._raw_api import (load_pycryptodome_raw_lib,
venv/lib/python3.6/site-packages/Crypto/Util/_raw_api.py:32: in <module>
    from Crypto.Util.py3compat import byte_string
E   ImportError: cannot import name 'byte_string'

There is an open issue on ledgerblue: LedgerHQ/blue-loader-python#44

carver · 2018-06-12T21:12:26Z

Hm, maybe we should use pycryptodomex in eth-hash, eth-keys, eth-keyfile, etc, as recommended here: http://pycryptodome.readthedocs.io/en/latest/src/introduction.html

Or maybe just upgrade ledger software to use pycryptodome :)

pipermerriam

If I understand correctly, test_ledger.py won't be runnable in CI. If so, can you move it to a new tests/integration/ledger/... directory. In addition, will you add a README to that directory which explains how to run the tests and what the per-requisits are.

carver · 2018-06-12T21:48:28Z

Right now the tests are failing for fixable reasons, so I want to see those get resolved before we hide them in a test that's skipped by CI.

pipermerriam · 2018-06-12T22:08:58Z

eth_account/signers/ledger.py

+
+ '''
+
+ def __init__(self, account_id=0, address='', path_prefix=ETH_DERIVATION_PATH_PREFIX):


I'd prefer None as the default value for address here.

pipermerriam · 2018-06-12T22:09:03Z

eth_account/signers/ledger.py

+
+ for account_id in itertools.count(start=search_account_id):
+ if account_id > search_limit:
+ raise Exception(f'Address {address} not found' +


Prefer use of ValueError or even a custom exception type here rather than a bare Exception

pipermerriam · 2018-06-12T22:09:05Z

eth_account/signers/ledger.py

+ raise Exception(f'Address {address} not found' +
+ f'(search_limit={search_limit}, ' +
+ f'search_account_id={search_account_id})')
+ if address == self.get_address(account_id):


This should probably use eth_utils.is_same_address

pipermerriam · 2018-06-12T22:10:50Z

eth_account/signers/ledger.py

+ '''
+ List Ethereum HD wallet adrress of the ledger device
+ '''
+ return list(map(lambda account_id: self.get_address(account_id),


I think the lambda isn't needed here, you should be able to just pass in self.get_address.

Alternate way to do this which I think is more readable

return [self.get_address(account_id) for account_id in range(...)]

pipermerriam · 2018-06-12T22:11:14Z

eth_account/signers/ledger.py

+ if address == self.get_address(account_id):
+ return account_id
+
+ def list(self, limit=5, page=0):


Alternate name suggestion get_addresses

pipermerriam · 2018-06-12T22:11:32Z

eth_account/signers/ledger.py

+ '''
+ Not available with a Ledger
+ '''
+ raise Exception('signHash() is not available within ledger devices')


Can you use NotImplementedError here.

pipermerriam · 2018-06-12T22:12:29Z

eth_account/signers/ledger.py

+ r = int.from_bytes(result[1:1 + 32], 'big')
+ s = int.from_bytes(result[1 + 32: 1 + 32 + 32], 'big')
+
+ rlp_encoded = encode_transaction(unsigned_transaction, vrs=(v, r, s))


I'm curious if we should have a bit of validation here to sanity check that the returned signature does indeed belong to the expected signing address?

The sanity test has been added

pipermerriam · 2018-06-12T22:13:07Z

setup.py

@@ -6,6 +6,9 @@
 )

 extras_require={
+ 'ledger': [
+ "ledgerblue==0.1.17",


Should we change this to be a version range, maybe just pinned within the minor version? ledgerblue>=0.1.17,<0.2.0

bargst · 2018-06-13T21:27:05Z

I will now try to:

~~implement defunctSignMessage()~~
make a PR to ledgerblue to update to pycryptodomex => Replace pycrypto with pycryptodomex LedgerHQ/blue-loader-python#45
look at the complexity to just extract the dongle interface to the device from ledgerblue to import it in eth-account directly

What's your thought on the third point ?

carver · 2018-06-13T21:41:59Z

look at the complexity to just extract the dongle interface to the device from ledgerblue to import it in eth-account directly

Sorry, I don't know much about the internals here of the dongle interface. Can you show how this proposal would change the eth-account API?

bargst · 2018-06-14T13:47:16Z

@carver I see it as copying and adapting HIDDongleHIDAPI class from https://github.com/LedgerHQ/blue-loader-python/blob/master/ledgerblue/comm.py#L56 and inserting it in ledger.py directly.
Then in LedgerAccount._send_to_device() use this new class instead of ledgerblue.comm.getDongle()

This HIDDongleHIDAPI class only has a dependency on 'hidapi>=0.7.99' and so we can get rid of ledgerbue requirement, but off course I need to make a small POC ...

pipermerriam · 2018-06-15T03:56:48Z

so we can get rid of ledgerbue requirement

This would be great in theory. Will have to see how it looks in practice.

bargst · 2018-06-16T21:38:13Z

I don't know why but I can't find a way to make the defunctSignMessage() work with my ledger and I don't think it is a requirement for supporting ledger in eth-account so I won't dig deeper and instead focus on ledegerblue dependency.

bargst · 2018-06-20T15:47:16Z

I will try to simplify USBDevice class to be cleaner. Maybe we can do like geth where they have implemented a usbwallet.driver so it can be used by other hardware device: https://github.com/ethereum/go-ethereum/blob/master/accounts/usbwallet/ledger.go

pipermerriam

The two wrap and unwrap APDU methods are quite long and don't read well. Do you think you could add some comments to explain what exactly is happening in them?
All of the method and variable names should be updated to use snake_casing instead of camelCasing
All Exception and BaseException usage should be updated to use either an appropriate standard library exception or a custom exception class.

pipermerriam · 2018-06-20T20:24:33Z

eth_account/signers/ledger.py

+ time.sleep(0.0001)
+ return bytearray(data)
+
+ def wrapCommandAPDU(self, channel, command, packetSize):


This method doesn't appear to use self. What do you think about extracting it to a utility function so that it can have some basic tests written for it (which will also allow it to be easily tested in isolation)

pipermerriam · 2018-06-20T20:25:25Z

eth_account/signers/ledger.py

+ def waitFirstResponse(self, timeout):
+ start = time.time()
+ data = ""
+ while len(data) == 0:


Maybe a cleaner way to do this

start_at = time.time() while True: data = self.device.read(65) if data: break else: # check timeout, sleep, etc

pipermerriam · 2018-06-20T20:27:30Z

eth_account/signers/ledger.py

+ result += b"\x00"
+ return bytearray(result)
+
+ def unwrapResponseAPDU(self, channel, data, packetSize):


Same with this method. No self usage so it should be able to be extracted into a utility method and tested in isolation.

pipermerriam · 2018-06-20T20:31:50Z

eth_account/signers/ledger.py

+ possibleCause = "Condition of use not satisfied (denied by the user?)"
+ if sw == 0x6a84 or sw == 0x6a85:
+ possibleCause = "Not enough space?"
+ if sw == 0x6484:


These should probably all be elif clauses.

pipermerriam · 2018-06-20T20:31:59Z

eth_account/signers/ledger.py

+ if self.debug:
+ print("HID <= %s %.2x" % (to_hex(response), sw))
+ if sw != 0x9000:
+ possibleCause = "Unknown reason"


Can you move this to a final else clause.

pipermerriam · 2018-06-20T20:33:08Z

eth_account/signers/ledger.py

+ result.extend(bytearray(self.device.read(65)))
+ sw = (result[swOffset] << 8) + result[swOffset + 1]
+ response = result[dataStart : dataLength + dataStart]
+ if self.debug:


Can you drop the debug flag and instead use the standard python logging facilities. We don't ship with print statements.

class LedgerUsbDevice: logger = logging.getLogger('eth_account.signers.ledger.LedgerUsbDevices') def some_method(self): self.logging.debug('the message')

bargst · 2018-06-24T22:14:35Z

I think the use of the hidapi is the way to go, so I spent some time cleanly implementing it with comments. Hope it looks like it could be possibly merged this way ? :)

I still have to move test_ledger.py to tests/integration/ledger/... directory and make a README for tests usage.

dylanjw

Mostly optional nitpicky stuff. The only big question is it doesnt look like there is handling for transactions larger than 255 bytes.

dylanjw · 2018-06-26T17:40:25Z

eth_account/signers/ledger.py

+ packet = header + commands[packet_id]
+
+ # Add padding to the packet to make it exactly PACKET_SIZE long
+ packet += bytes([0x0] * (PACKET_SIZE - len(packet)))


could this be simplified with ljust?

packet.ljust(PACKET_SIZE, bytes([0x0]))

dylanjw · 2018-06-26T17:49:15Z

eth_account/signers/ledger.py

+ command = struct.pack('>H', len(command)) + command
+
+ # Split command into at max PACKET_FREE sized chunks
+ commands = [command[i:i + PACKET_FREE] for i in range(0, len(command), PACKET_FREE)]


The var name commands was confusing the first couple times reading through. Could it be something like command_chunks, command_packets, or simply chunks?

dylanjw · 2018-06-26T18:00:07Z

eth_account/signers/ledger.py

+class LedgerUsbDevice:
+ '''
+ References:
+ - https://github.com/LedgerHQ/blue-loader-python/blob/master/ledgerblue/comm.py#L56


Should the comm reference be taken out now that it is no longer used, or is it an important example for how the ledger works?

I would rather let it as is because part of the comm module are still present (device detection for ex.)

dylanjw · 2018-06-26T18:20:26Z

eth_account/signers/ledger.py

+ for hidDevice in hid.enumerate(0, 0):
+ if hidDevice['vendor_id'] == 0x2c97:
+ if ('interface_number' in hidDevice and hidDevice['interface_number'] == 0) \
+ or ('usage_page' in hidDevice and hidDevice['usage_page'] == 0xffa0):


Could these hex values be put in global constants? LEDGER_VENDOR_ID and LEDGER_USAGE_PAGE_ID?

dylanjw · 2018-06-26T18:26:39Z

eth_account/signers/ledger.py

+ if not channel:
+ if reply_start + timeout < time.time():
+ raise LedgerUsbException(f'Timeout waiting for a device' +
+ f'response (timeout={timeout}s)')


TIL about f-strings!

Yeah, they're pretty cool. I can't wait to use them more broadly! Unfortunately f-strings weren't added until 3.6, but Web3.py (and therefore eth-account) has to support py3.5. So this will have to be switched out for % or .format style instead. :(

That explain why circleCI lint was failing ! Too bad we are not on 3.6 :)
I will update to remove all f-strings.

dylanjw · 2018-06-26T18:28:46Z

eth_account/signers/ledger.py

+ # Status is stored at then end of the reply
+ (status,) = struct.unpack('>H', reply[-2:])
+
+ if status == 0x9000:


What does 0x9000 mean? A constant var could explain this.

dylanjw · 2018-06-26T18:37:39Z

eth_account/signers/ledger.py

+ rlp_encoded_tx = rlp.encode(unsigned_transaction)
+
+ # https://github.com/LedgerHQ/blue-app-eth/blob/master/doc/ethapp.asc#sign-eth-transaction
+ apdu = bytes.fromhex('e0040000')


'e0040000' could be saved to a constant, with a descriptive name like: ADPU_SIGN_TX_FIRST_DATA_BLOCK

dylanjw · 2018-06-26T18:40:17Z

eth_account/signers/ledger.py

+ apdu = bytes.fromhex('e0040000')
+ apdu += bytes([len(bip32_path) + len(rlp_encoded_tx)])
+ apdu += bip32_path
+ apdu += rlp_encoded_tx


What happens if the rlp_encoded_tx is greater than the 255 byte limit? Should this be getting chunked and sent with the next data block adpu ('e0048000')?

The chunk is done by the exchange() function, but I will add a test to see if it is working as expected.

bargst · 2018-06-30T16:43:25Z

@dylanjw you were right ! Transaction larger than 255 would not be signed. Now fixed !

bargst · 2018-07-02T10:52:41Z

I finally found a way to make defunctSignMessage() work and sign message on the ledger !

I think the PR is now worth reviewing @pipermerriam @carver @dylanjw but do you want me to squash history before or after review ?

This PR now use hidapi python module and do not rely anymore on ledgerblue.

carver · 2018-07-10T00:36:26Z

Squashing can happen after. I should be able to do another review tomorrow.

carver · 2018-07-13T23:34:42Z

... and by tomorrow I mean next week. Sorry for the delay. I've been pretty heads-down on getting the next trinity release out the door.

carver

Not a full review, but at least something to get started on.

carver · 2018-07-20T21:43:19Z