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

Ledger integration into KMS #172

Merged
merged 12 commits into from Feb 20, 2019

Conversation

Projects
None yet
5 participants
@adrianbrink
Copy link
Contributor

commented Feb 14, 2019

This PR adds the ledger integration as a backend to the KMS. There is
still more work required to ensure that the Ledger application knows how
to correctly decode/encode Tendermint votes.

adrianbrink added some commits Feb 14, 2019

Ledger integration into KMS
This PR adds the ledger integration as a backend to the KMS. There is
still more work required to ensure that the Ledger application knows how
to correctly decode/encode Tendermint votes.
@adrianbrink

This comment has been minimized.

Copy link
Contributor Author

commented Feb 14, 2019

We can update the signatory dependencies as soon as we can release 0.11 of signatory-cosmos-val.

Cargo.toml Outdated
signatory = { git = "https://github.com/cryptiumlabs/signatory" }
signatory-dalek = { git = "https://github.com/cryptiumlabs/signatory" }
signatory-yubihsm = { git = "https://github.com/cryptiumlabs/signatory" }
signatory-ledger-cosval = { git = "https://github.com/cryptiumlabs/signatory" }

This comment has been minimized.

Copy link
@Liamsi

Liamsi Feb 15, 2019

Member

please do not introduce external versions/forks of these crates.

This comment has been minimized.

Copy link
@adrianbrink

adrianbrink Feb 15, 2019

Author Contributor

This is waiting on the release of the correct upstream crates, specifically the new release of signatory-ledger-cosval.

This comment has been minimized.

Copy link
@tarcieri
@@ -41,7 +41,7 @@ jobs:
command: |
rustc --version
cargo --version
cargo test --all --all-features
cargo test --all --features "default softsign yubihsm yubihsm-mock"

This comment has been minimized.

Copy link
@Liamsi

Liamsi Feb 15, 2019

Member

Why is this change necessary?

This comment has been minimized.

Copy link
@adrianbrink

adrianbrink Feb 15, 2019

Author Contributor

The test harness does not work with the Ledger since I haven't implemented a ledger-mock.

Show resolved Hide resolved src/keyring/ed25519/ledger.rs Outdated
@jleni

This comment has been minimized.

Copy link
Contributor

commented Feb 15, 2019

@adrianbrink @Liamsi
I have a kind of continuation of this integration that it depends on changes made here tendermint/signatory#141, other layers and it requires Ledger Validator app v0.6.0

@adrianbrink we can work together on this, I can rebase over your PR (so your commits in this PR are not lost) and apply all the other changes that are necessary for the complete integration.

Would that make sense?

@jleni

This comment has been minimized.

Copy link
Contributor

commented Feb 15, 2019

Let me know what you prefer. In the meantime, I need to wait until tendermint/signatory#141 gets merged.

@tarcieri

This comment has been minimized.

Copy link
Collaborator

commented Feb 15, 2019

@jleni merged

@adrianbrink

This comment has been minimized.

Copy link
Contributor Author

commented Feb 15, 2019

@adrianbrink @Liamsi
I have a kind of continuation of this integration that it depends on changes made here tendermint/signatory#141, other layers and it requires Ledger Validator app v0.6.0

@adrianbrink we can work together on this, I can rebase over your PR (so your commits in this PR are not lost) and apply all the other changes that are necessary for the complete integration.

Would that make sense?

Rebasing sounds fine!

@jleni

This comment has been minimized.

Copy link
Contributor

commented Feb 15, 2019

I have multiple changes that will make this PR incompatible, specially once @tony-iqlusion releases the new crates. signatory-ledger-cosval is obsolete and will be replaced.

@adrianbrink I think the best option is that:

  • I rebase on this PR
  • submit another one with all the additional changes on top.
  • give you access to the PR branch so you can also work there if you want

agree?

@jleni

This comment has been minimized.

Copy link
Contributor

commented Feb 15, 2019

We also need to open a couple of new additional issues to track improvements that need to be made with respect to reconnections, timeout handling, extending command line support, etc.

@adrianbrink

This comment has been minimized.

Copy link
Contributor Author

commented Feb 15, 2019

I have multiple changes that will make this PR incompatible, specially once @tony-iqlusion releases the new crates. signatory-ledger-cosval is obsolete and will be replaced.

@adrianbrink I think the best option is that:

* I rebase on this PR

* submit another one with all the additional changes on top.

* give you access to the PR branch so you can also work there if you want

agree?

That sounds good

@jleni jleni referenced this pull request Feb 15, 2019

Closed

Gaiad cannot reconnect KMS unless restart gaiad #3190

0 of 4 tasks complete
@adrianbrink

This comment has been minimized.

Copy link
Contributor Author

commented Feb 15, 2019

@jleni I just tested everything with the latest Ledger application and the updated dependencies and it works :-)

@jleni

This comment has been minimized.

Copy link
Contributor

commented Feb 15, 2019

There are several linked issues that are pending. Here is another one: #173

@jleni

This comment has been minimized.

Copy link
Contributor

commented Feb 15, 2019

If you give me write access to adrianbrink:adrian/ledger_integration I can push my additional KMS changes directly here.

I also need to add automatic reconnections in signatory.

@adrianbrink

This comment has been minimized.

Copy link
Contributor Author

commented Feb 15, 2019

I've added you as a contributor to adrianbrink/kms

jleni and others added some commits Feb 15, 2019

Merge pull request #1 from ZondaX/zondax/ledger-tm
Refactoring and adjusting to new ledger-tm library
Merge pull request #2 from ZondaX/ledger_integration
Upgrading crates + cargo fmt fixes
Merge pull request #4 from ZondaX/ledger_integration
Disabling ledgertm tests until a ledgermock is available
@jleni

This comment has been minimized.

Copy link
Contributor

commented Feb 19, 2019

@Liamsi Any news about this PR? It would be good to integrate this alpha version.

@Liamsi

This comment has been minimized.

Copy link
Member

commented Feb 19, 2019

I'll look into it in a bit!

@Liamsi
Copy link
Member

left a comment

The changes look OK. Do we want to include code snippets that aren't currently used though? I would suggest to remove the currently unused boilerplate code and add it together with the functionality that comes with followup PRs.

It is not clear to me what needs to be done to get the ledger sign anything with these changes. Just add [[providers.ledgertm]] to the config and we are done?

impl Callable for DetectCommand {
/// Detect all Ledger devices running the Tendermint app
fn call(&self) {
println!("This feature will be soon available");

This comment has been minimized.

Copy link
@Liamsi

Liamsi Feb 19, 2019

Member

Why include the detect command if it does not do anything yet?

@@ -17,3 +17,5 @@ adapter = { type = "usb" }
auth = { key = 1, password = "password" } # Default YubiHSM admin credentials. Change ASAP!
keys = [{ id = "gaia-9000", key = 1 }]
#serial_number = "0123456789" # identify serial number of a specific YubiHSM to connect to

[[providers.ledgertm]]

This comment has been minimized.

Copy link
@Liamsi

Liamsi Feb 19, 2019

Member

Is this everything that is necessary to get the ledger up and running? If not providing (all) existing options with some exemplary defaults would be cool.

};

pub const LEDGER_TM_PROVIDER_LABEL: &str = "ledgertm";
pub const LEDGER_TM_ID: &str = "ledgertm";

This comment has been minimized.

Copy link
@Liamsi

Liamsi Feb 19, 2019

Member

Shouldn't the ID come from the config? An ID that is always the same (and the same as the constant LEDGER_TM_PROVIDER_LABEL), feels wrong.

static ref HSM_CLIENT: Mutex<Ed25519LedgerTmAppSigner> = Mutex::new(create_hsm_client());
}

fn create_hsm_client() -> Ed25519LedgerTmAppSigner {

This comment has been minimized.

Copy link
@Liamsi

Liamsi Feb 19, 2019

Member

It looks like this is not used anywhere?

use signatory_ledger_tm::Ed25519LedgerTmAppSigner;
use std::sync::Mutex;

// This instance is only used by CLI commands or tests

This comment has been minimized.

Copy link
@Liamsi

Liamsi Feb 19, 2019

Member

Currently not used anywhere?

@Liamsi

This comment has been minimized.

Copy link
Member

commented Feb 20, 2019

Thanks a lot @jleni @adrianbrink!

I would suggest to apply the following changes to this PR:

  • remove all unused / incomplete / boilerplate code:
    The commands in src/commands/ledgertm do not provide any functionality yet (help prints about the detect command: "detect connected Ledger devices running the Tendermint app" and detect does only print "This feature will be soon available"), please submit a follow-up PR

  • revert changes from 562109d:
    (error[E0658]: use of unstable library feature 'duration_as_u128' (see issue #50202), also error: Could not compile tmkms.), different concern and not properly formatted (please fix and submit a follow up PR)

  • remove src/ledgertm.rs: this code is not used anywhere yet and is not useful yet

  • add a comment about the config options and used defaults (if any)

I've submitted a follow up PR here which contains all your changes: #176, PTAL

@jleni plans to submit a follow up PR which completes this PR and uses completes the commands etc.

@Liamsi Liamsi merged commit 562109d into tendermint:master Feb 20, 2019

1 check failed

ci/circleci Your tests failed on CircleCI
Details
@chengwenxi

This comment has been minimized.

Copy link

commented on 562109d Feb 22, 2019

I get some error when trying to compile on the latest macOS with rust 1.32.0.

   --> src/session.rs:105:43
    |
105 |                 let end = start.elapsed().as_millis();
    |                                           ^^^^^^^^^
@Liamsi

This comment has been minimized.

Copy link
Member

commented Feb 22, 2019

Please do use the latest master branch instead of this one. I've removed a bunch of code and fixed other parts in #173. You should still be able to experiment with the ledger integration.

@cwgoes cwgoes deleted the cryptiumlabs:adrian/ledger_integration branch Feb 23, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.