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

R4R: PrivKeyLedgerSecp256k1: lost connection #3431

Merged
merged 20 commits into from
Feb 5, 2019

Conversation

jleni
Copy link
Member

@jleni jleni commented Jan 29, 2019

This PR solves issue #3335

This PR upgrades ledger-cosmos-go and brings the following improvements:

  • Migrates USB HID library to a better maintained implementation
  • Fixes a problem in MacOS that was resulting in "lost connection" messages
  • Fixes issues in Windows clients
  • Additional unit tests in each of the layers
  • better resource handling in gaia. Ledger connections are not kept open, etc.
  • signature fixes: conversion from DER to BER encoding
  • additional tests
  • new make target to run ledger integration tests (with real device) make test_ledger

Technical description

This upgrade also results in multiple changes/fixes in several dependencies:

HID https://github.com/ZondaX/hid

Ledger-go https://github.com/ZondaX/ledger-go

Ledger-cosmos-go https://github.com/ZondaX/ledger-cosmos-go


  • Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
  • Wrote tests
  • Updated relevant documentation (docs/)
  • Added entries in PENDING.md with issue #
  • rereviewed Files changed in the github PR explorer

For Admin Use:

  • Added appropriate labels to PR (ex. wip, ready-for-review, docs)
  • Reviewers Assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@jleni jleni added the C:Ledger Issues and features related Ledger integration and functionality label Jan 29, 2019
@jackzampolin
Copy link
Member

Hey @jleni can you also link to a PR to ZondaX/ledger-cosmos-go with the changes that you are importing into the SDK? We can't really reason about the changes if we don't have that. Also this PR needs a rebase.

@jleni
Copy link
Member Author

jleni commented Jan 30, 2019

Yes, the conflict appeared once another PR was merged last night. No worries, the PR is still WIP.

@jackzampolin I have now included a longer technical explanation for all the changes that were made with links to each respective layer and PR.

@jleni jleni added the wip label Jan 30, 2019
@codecov
Copy link

codecov bot commented Jan 31, 2019

Codecov Report

Merging #3431 into develop will increase coverage by 6.23%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #3431      +/-   ##
===========================================
+ Coverage    52.97%   59.21%   +6.23%     
===========================================
  Files          142      134       -8     
  Lines        10363     9940     -423     
===========================================
+ Hits          5490     5886     +396     
+ Misses        4550     3680     -870     
- Partials       323      374      +51

@jleni
Copy link
Member Author

jleni commented Feb 1, 2019

As discussed with @cwgoes @jackzampolin

I have noticed something that may result in problems. At the moment, when a key is requested to a ledger device, a PrivKeyLedgerSecp256k1 object keeps the device together with the keys. However, this object is actually a USB resource that is never released.

At the moment, there are a few problems in gaia and still in this PR:

  • discovery opens the resource but never closes it... calling discovery twice fails as the resource has not been released.
  • the cached usb resource can fail in many ways (disconnects, etc.)
  • we might need to add code to reconnect, verify that it is the same device, etc.
  • PrivKeyLedgerSecp256k1 gets serialized to amino I am sure it is skipping the connection object, but still.. it feels odd.

We agreed on refactoring gaia's code, so resources are released ASAP and pks are rechecked before signing. As PKs are checked, it should be possible to detect if devices are swapped or there are multiple devices and the incorrect one is re-discovered.

@jleni
Copy link
Member Author

jleni commented Feb 1, 2019

In addition to these changes, I have noticed that we have a mix of DER vs BER signatures. I will try to unify this a bit too to avoid confusion.

@jackzampolin
Copy link
Member

This PR just updated dependancies.

@jleni
Copy link
Member Author

jleni commented Feb 1, 2019

True, for now the changes are in other layers as described above.
I will soon push all the other local changes I made that apply to the SDK.
Once the fixes in the dependencies got integrated, there are other reasons why there might be connection issues. Also unit tests for ledger in the SDK are quite weak. Actually, they are skipped at the moment.

@jackzampolin
Copy link
Member

@jleni Looking forward to unit testing for this really crucial usecase. Thank you for adding them!

@jleni
Copy link
Member Author

jleni commented Feb 2, 2019

I would say this is not the PR/issue for that.. but once #3461 and #3431 are merged, I would open another issue to add cli tests that check a real ledger device too.

@jleni jleni changed the title WIP: PrivKeyLedgerSecp256k1: lost connection R4R: PrivKeyLedgerSecp256k1: lost connection Feb 2, 2019
@jleni
Copy link
Member Author

jleni commented Feb 2, 2019

@jackzampolin Given that these fixes have uncovered an existing incompatibility between DER and BER signatures, I would suggest considering this PR for prelaunch: without these changes, signatures will be invalid.

Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

Thanks @jleni! I left a few remarks 👍

crypto/ledger_integration_test.go Outdated Show resolved Hide resolved
crypto/ledger_integration_test.go Outdated Show resolved Hide resolved
crypto/ledger_secp256k1.go Show resolved Hide resolved
crypto/ledger_secp256k1.go Outdated Show resolved Hide resolved
crypto/ledger_secp256k1.go Outdated Show resolved Hide resolved
@jleni
Copy link
Member Author

jleni commented Feb 4, 2019

It seems an unrelated issue hit integration_tests here:

    gobash.go:80: Running gaiad start --home=/tmp/gaia_integration_TestGaiaCLISubmitProposal_210313979/.gaiad --rpc.laddr=tcp://0.0.0.0:32893 --p2p.laddr=tcp://0.0.0.0:38125
panic: Get http://localhost:32893/block: read tcp 127.0.0.1:53250->127.0.0.1:32893: read: connection reset by peer [recovered]
	panic: Get http://localhost:32893/block: read tcp 127.0.0.1:53250->127.0.0.1:32893: read: connection reset by peer

Should I open another issue for that? I can try rerunning but there is a panic to analyse there anyway.

@jleni
Copy link
Member Author

jleni commented Feb 4, 2019

Here is the link to the CI error:
https://circleci.com/gh/ZondaX/cosmos-sdk/343

@jleni
Copy link
Member Author

jleni commented Feb 4, 2019

Running integration_tests again and they passed. There is definitely a sporadic unrelated issue in integration_tests that needs to be looked at.

@jleni
Copy link
Member Author

jleni commented Feb 5, 2019

@jackzampolin @alessio Fixed a conflict in Pending.md.
Hopefully, this is also ready to go.

Copy link
Contributor

@alessio alessio left a comment

Choose a reason for hiding this comment

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

Very good indeed. Nihil obstat from me. @cwgoes fancy taking a look at this?

@jleni
Copy link
Member Author

jleni commented Feb 5, 2019

@jackzampolin @alessio This PR and #3461 have changes in similar areas. Whichever you merge first will result in changes on the other side.
I would say, we merge #3461 first before it goes stale and I adjust this PR accordingly.

@jackzampolin
Copy link
Member

@jleni Per your suggestion I've merged #3461 first. This now has merge conflicts.

@jleni
Copy link
Member Author

jleni commented Feb 5, 2019

Yes, thanks! I will adjust this in the next hour

Copy link
Member

@jackzampolin jackzampolin left a comment

Choose a reason for hiding this comment

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

Thank you @jleni! LGTM

@jackzampolin jackzampolin merged commit 174ec0c into cosmos:develop Feb 5, 2019
@jleni jleni deleted the fix/lost-conn branch February 5, 2019 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:Ledger Issues and features related Ledger integration and functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants