Add support for Satochip hardware wallet

[Toporin]

This is a pull request to add support for the Satochip hardware wallet.
The (optional) Satochip-2FA plugin allows to confirm transaction requests on a second android device.

The Satochip hardware wallet is based on a jacavard smartcard and is fully open-source.
The wallet is composed of a javacard applet (https://github.com/Toporin/SatochipApplet) that is to be loaded on the smartcard, and an Electrum client plugin (https://github.com/Toporin/electrum-satochip) that acts as the interface between the card and the network.

More info:
https://github.com/Toporin/ (official repository)
https://prezi.com/p/mpq-xhh3mxjl/satochip-gent-meetup/ (Slides from previous meetups in Belgium)

[SomberNight]

Thanks for the PR.

Please try to clean up your changes. I've highlighted some examples.

GitHub does not let me comment on some files; so further remarks here:

• electrum/electrum is a symlink; you seem to have removed and then re-added it? please clean history so git sees no change
• file permissions were changed for several files. please undo these changes

[SomberNight]

maybe undo changes in this file?? if you want a readme specific to your plugin, see e.g. https://github.com/spesmilo/electrum/blob/2a80f6a3ad1997762734b157a023f8dcd16f2f7f/electrum/plugins/coldcard/README.md

[SomberNight]

leftover by mistake??

[SomberNight]

same

[SomberNight]

is this not available on pypi/pip? i.e. https://pypi.org/project/pyscard/ ?
is the issue that they do not seem distribute a win32 wheel?
and we cannot build it ourselves?

[Toporin]

The maintainer of the project does not provide official releases for Windows 32-bit apparently. Since the project is open-source, it should be possible to build it from sources but it looks quite complex (LudovicRousseau/pyscard#55). The link provided in the script comes from the appveyor account for pyscard (https://ci.appveyor.com/project/LudovicRousseau/pyscard), and is the suggested workaround provided by the maintainer (see LudovicRousseau/pyscard#55 (comment))

[SomberNight]

please clean

[SomberNight]

Try not to reimplement existing code. Please do something like this instead:

from electrum.ecc import sig_string_from_der_sig, construct_sig65
from electrum.util import bfh, bh2u

sig_string = sig_string_from_der_sig(bfh(sigin))
return bh2u(construct_sig65(sig_string, recid, compressed))

[SomberNight]

please try to use existing functions instead of re-implementing them:

electrum/electrum/bip32.py

Line 276 in 2a80f6a

def convert_bip32_path_to_list_of_uint32(n: str) -> List[int]:

(and use e.g. int.to_bytes to convert the ints to bytes)

[SomberNight]

should this really be logged?

[SomberNight]

_() translates/localizes strings
maybe split this up, and remove the named parameters inside the string, as this seems really error-prone to translators
Or alternatively, do not translate it.

[Toporin]

Hello, Thank you for the detailed feedback. I will do the cleanup and provide clarifications to your remarks asap! I also noticed that the Travis CI check failed due to the Test_CardConnector.py file. This class performs basic unit tests for bip32 derivation in hardware and requires the Satochip card to succeed. So perhaps I should remove it? Best regards Le mar. 23 juil. 2019 à 21:18, ghost43 <[email protected]> a écrit :

…

***@***.**** requested changes on this pull request. Thanks for the PR. Please try to clean up your changes. I've highlighted some examples. GitHub does not let me comment on some files; so further remarks here: - electrum/electrum is a symlink; you seem to have removed and then re-added it? please clean history so git sees no change - file permissions were changed for several files. please undo these changes ------------------------------ In README-electrum.rst <#5523 (comment)>: > @@ -0,0 +1,115 @@ +Electrum - Lightweight Bitcoin client +===================================== + +:: maybe remove this file? ------------------------------ In README.rst <#5523 (comment)>: > -.. image:: https://travis-ci.org/spesmilo/electrum.svg?branch=master - :target: https://travis-ci.org/spesmilo/electrum - :alt: Build Status -.. image:: https://coveralls.io/repos/github/spesmilo/electrum/badge.svg?branch=master - :target: https://coveralls.io/github/spesmilo/electrum?branch=master - :alt: Test coverage statistics -.. image:: https://d322cqt584bo4o.cloudfront.net/electrum/localized.svg - :target: https://crowdin.com/project/electrum - :alt: Help translate Electrum online +This is a fork of Electrum modified for use with the Satochip Hardware Wallet. To use it, you need a device with the Satochip Javacard Applet installed. maybe undo changes in this file?? if you want a readme specific to your plugin, see e.g. https://github.com/spesmilo/electrum/blob/2a80f6a3ad1997762734b157a023f8dcd16f2f7f/electrum/plugins/coldcard/README.md ------------------------------ In contrib/build-wine/deterministic.spec <#5523 (comment)>: > @@ -121,7 +125,7 @@ exe_standalone = EXE( strip=None, upx=False, icon=home+'electrum/gui/icons/electrum.ico', - console=False) + console=True) #DebugSatochip True leftover by mistake?? ------------------------------ In contrib/build-wine/deterministic.spec <#5523 (comment)>: > @@ -134,7 +138,7 @@ exe_portable = EXE( strip=None, upx=False, icon=home+'electrum/gui/icons/electrum.ico', - console=False) + console=True) #DebugSatochip True same ------------------------------ In contrib/build-wine/prepare-wine.sh <#5523 (comment)>: > @@ -17,6 +17,14 @@ PYINSTALLER_REPO="https://github.com/SomberNight/pyinstaller.git" PYINSTALLER_COMMIT=d1cdd726d6a9edc70150d5302453fb90fdd09bf2 # ^ tag 3.4, plus a custom commit that fixes cross-compilation with MinGW +#DebugSatochip pyscard +#PYSCARD_FILENAME=pyscard-1.9.7-cp36-cp36m-win_amd64.whl #python64-bits +#PYSCARD_URL=https://sourceforge.net/projects/pyscard/files/pyscard/pyscard%201.9.7/pyscard-1.9.7-cp36-cp36m-win_amd64.whl/download +#PYSCARD_SHA256=c63a87e4e7c87ce4c1299a1d8e0cae4e43f27451ca210f2c54f2dcd7467565c5 +PYSCARD_FILENAME=pyscard-1.9.8-cp36-cp36m-win32.whl <https://sourceforge.net/projects/pyscard/files/pyscard/pyscard%201.9.7/pyscard-1.9.7-cp36-cp36m-win_amd64.whl/download+#PYSCARD_SHA256=c63a87e4e7c87ce4c1299a1d8e0cae4e43f27451ca210f2c54f2dcd7467565c5+PYSCARD_FILENAME=pyscard-1.9.8-cp36-cp36m-win32.whl> #python32-bits +PYSCARD_URL=https://ci.appveyor.com/api/buildjobs/j60tkykj6vh0ppiy/artifacts/dist%2Fpyscard-1.9.8-cp36-cp36m-win32.whl +PYSCARD_SHA256=4641b5db53fb3562671b7b7c685ddf8f715180e2809106fb2a9361dfad553b4b <https://ci.appveyor.com/api/buildjobs/j60tkykj6vh0ppiy/artifacts/dist%2Fpyscard-1.9.8-cp36-cp36m-win32.whl+PYSCARD_SHA256=4641b5db53fb3562671b7b7c685ddf8f715180e2809106fb2a9361dfad553b4b> is this not available on pypi/pip? i.e. https://pypi.org/project/pyscard/ ? is the issue that they do not seem distribute a win32 wheel? and we cannot build it ourselves? ------------------------------ In contrib/build-wine/prepare-wine.sh <#5523 (comment)>: > @@ -65,6 +73,12 @@ download_if_not_exist "$CACHEDIR/$ZBAR_FILENAME" "$ZBAR_URL" verify_hash "$CACHEDIR/$ZBAR_FILENAME" "$ZBAR_SHA256" wine "$CACHEDIR/$ZBAR_FILENAME" /S +#DebugSatochip install pyscard +info "Installing pyscard..." +download_if_not_exist $PYSCARD_FILENAME "$PYSCARD_URL" +verify_hash $PYSCARD_FILENAME "$PYSCARD_SHA256" +$PYTHON -m pip install "$PWD/$PYSCARD_FILENAME" instead of $PWD, use $CACHEDIR see surrounding lines for examples ------------------------------ In electrum/gui/kivy/data/fonts/tron/License.txt <#5523 (comment)>: > @@ -1,4 +1,4 @@ -Copyright (c) 2010-2011, Jeff Bell [www.randombell.com] | ***@***.***. +Copyright (c) 2010-2011, Jeff Bell [www.randombell.com] | ***@***.***. please clean this so that git sees no change ------------------------------ In electrum/gui/kivy/data/fonts/tron/Readme.txt <#5523 (comment)>: > @@ -10,7 +10,7 @@ UPDATE HISTORY: 1/22/11 - Made minor corrections to all previous letters and punctuation. Corrected issue with number 8's top filling in. -ABOUT THE AUTHOR: +ABOUT THE AUTHOR: please clean this so that git sees no change ------------------------------ In electrum/plugin.py <#5523 (comment)>: > @@ -596,7 +596,7 @@ def _scan_devices_with_hid(self): def scan_devices(self): self.logger.info("scanning devices...") - + please clean ------------------------------ In electrum/plugin.py <#5523 (comment)>: > @@ -625,5 +625,5 @@ def scan_devices(self): # Unpair disconnected devices for id_ in disconnected_ids: self.unpair_id(id_) - + please clean ------------------------------ In electrum/plugins/satochip/CardDataParser.py <#5523 (comment)>: > + if sig_size>0 and self.authentikey_coordx: + sig= response[offset:(offset+sig_size)] + pubkey= self.get_pubkey_from_signature(self.authentikey_coordx, sig_data, sig) + if pubkey != self.authentikey: + raise Exception("signing key is not authentikey!") + #todo: error checking + + return (tx_hash, needs_2fa) + + def parse_to_compact_sig(self, sigin, recid, compressed): + ''' convert a DER encoded signature to compact 65-byte format + input is hex string in DER format + output is hex string in compact 65-byteformat + http://bitcoin.stackexchange.com/questions/12554/why-the-signature-is-always-65-13232-bytes-long + https://bitcointalk.org/index.php?topic=215205.0 + ''' Try not to reimplement existing code. Please do something like this instead: from electrum.ecc import sig_string_from_der_sig, construct_sig65 from electrum.util import bfh, bh2u sig_string = sig_string_from_der_sig(bfh(sigin)) return bh2u(construct_sig65(sig_string, recid, compressed)) ------------------------------ In electrum/plugins/satochip/satochip.py <#5523 (comment)>: > +from .TxParser import TxParser + +from smartcard.sw.SWExceptions import SWException +from smartcard.Exceptions import CardConnectionException, CardRequestTimeoutException +from smartcard.CardType import AnyCardType +from smartcard.CardRequest import CardRequest + +_logger = get_logger(__name__) + +# debug: smartcard reader ids +SATOCHIP_VID= 0 #0x096E +SATOCHIP_PID= 0 #0x0503 + +MSG_USE_2FA= _("Do you want to use 2-Factor-Authentication (2FA)?\n\nWith 2FA, any transaction must be confirmed on a second device such as your smartphone. First you have to install the Satochip-2FA android app on google play. Then you have to pair your 2FA device with your Satochip by scanning the qr-code on the next screen. Warning: be sure to backup a copy of the qr-code in a safe place, in case you have to reinstall the app!") + +def bip32path2bytes(bip32path:str) -> (int, bytes): please try to use existing functions instead of re-implementing them: https://github.com/spesmilo/electrum/blob/2a80f6a3ad1997762734b157a023f8dcd16f2f7f/electrum/bip32.py#L276 (and use e.g. int.to_bytes to convert the ints to bytes) ------------------------------ In electrum/plugins/satochip/satochip.py <#5523 (comment)>: > + def __repr__(self): + return '<SatochipClient TODO>' + + def is_pairable(self): + return True + + def close(self): + _logger.info(f"[SatochipClient] close()")#debugSatochip + self.cc.card_disconnect() + self.cc.cardmonitor.deleteObserver(self.cc.cardobserver) + + def timeout(self, cutoff): + pass + + def is_initialized(self): + _logger.info(f"[SatochipClient] is_initialized(): TODO - currently set to true!")#debugSatochip should this really be logged? ------------------------------ In electrum/plugins/satochip/satochip.py <#5523 (comment)>: > + client = devmgr.client_by_id(device_id) + if client is None: + raise Exception(_('Failed to create a client for this device.') + '\n' + + _('Make sure it is in the correct state.')) + client.handler = self.create_handler(wizard) + + # check applet version + while(True): + (response, sw1, sw2, d)=client.cc.card_get_status() + if (sw1==0x90 and sw2==0x00): + v_supported= (CardConnector.SATOCHIP_PROTOCOL_MAJOR_VERSION<<8)+CardConnector.SATOCHIP_PROTOCOL_MINOR_VERSION + v_applet= (d["protocol_major_version"]<<8)+d["protocol_minor_version"] + strcmp= 'lower' if (v_applet<v_supported) else 'higher' + _logger.info(f"[SatochipPlugin] setup_device(): Satochip version={hex(v_applet)} Electrum supported version= {hex(v_supported)}")#debugSatochip + if (v_supported!=v_applet): + msg=_('The version of your Satochip (v{v_applet_maj:x}.{v_applet_min:x}) is {strcmp} than supported by Electrum (v{v_supported_maj:x}.{v_supported_min:x}). You should update Electrum to ensure correct function!').format(strcmp=strcmp, v_applet_maj=d["protocol_major_version"], v_applet_min=d["protocol_minor_version"], v_supported_maj=CardConnector.SATOCHIP_PROTOCOL_MAJOR_VERSION, v_supported_min=CardConnector.SATOCHIP_PROTOCOL_MINOR_VERSION) _() translates/localizes strings maybe split this up, and remove the named parameters inside the string, as this seems really error-prone to translators Or alternatively, do not translate it. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#5523?email_source=notifications&email_token=ACSTI2HFF3LQKAJRTV5MS7TQA5KRPA5CNFSM4IGBOTLKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB7KJUEQ#pullrequestreview-265591314>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACSTI2DNPK6ORVDRPRW4W3DQA5KRPANCNFSM4IGBOTLA> .

[SomberNight]

I also noticed that the Travis CI check failed due to the
Test_CardConnector.py file.

The failure is due to the smartcard module not being installed.
Your requirements should be specified in contrib/requirements-hw.txt. This should be done irrespective of whether the test file is kept.

If that test requires the hardware to be present, maybe it should be made conditional somehow, or it can just be excluded from the tests run by travis.

[Toporin]

Hello,

I introduced a new pull request from a dedicated branch and incorporating your suggestions: see #5533.