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

Interchangable PrivKey implementations in keybase #5278

Merged
merged 27 commits into from
Dec 12, 2019

Conversation

austinabell
Copy link
Contributor

@austinabell austinabell commented Nov 5, 2019

closes: #4941

This PR allows for the keybase to be configured to override the implementation of the key that is saved to the keybase. I made this PR to be non-breaking changes and as unintrusive as possible, but can structure it differently or suggest different ways of going about this if you are open to these kinds of changes.

Summary of what this changes:

  • Allows for keybase options to be passed into initializing each type of keybase, where the only option included now is to be able to override the key type that is saved to the keybase
    • Changes mainly around this function: here and the function gets defaulted if not provided.
    • I used this pattern because I personally think it's better than using the .With overrides, but I have noticed you guys use that one and I can change it if wanted
  • Exposes the keys commands individually, because an application that wants to override the add command cannot use the bundled keys commands
  • Exposes necessary codecs around keys to be able to override them to include the new key implementation types

The other part of this is the key signing, which the only change is overriding the codec in Tendermint used for decoding a private key from bytes, change I made was this. After the key is decoded it simply uses the Sign function of the tendermint PrivKey interface

Let me know if you guys are open to these changes and I can clean up the PR and do necessary documentation. I will try to think of a better way to modify the codecs being referenced without exposing them publicly.

Here is an example of the keys commands being used with the overriden add command to use the keybase options. Here is the key type that is being saved, and it is usable because it satisfies the tendermint PrivKey interface. Here is the PR in ethermint that uses these changes but the main utility of these changes is to be able to override the addressing and signing of the existing secp256k1 curve, but does not solve the issue of allowing multiple curves.

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
  • Wrote tests
  • Updated relevant documentation (docs/)
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed 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)

@alexanderbez
Copy link
Contributor

Thanks @austinabell! Unfortunately, odds are we won't have any time in the near future to review this in-depth.

Copy link
Collaborator

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

Thanks @austinabell! Minor comments only.

return err
}

return RunAddCmd(cmd, args, kb)
Copy link
Collaborator

Choose a reason for hiding this comment

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

this looks odd. Shouldn't a public function call a private one instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I agree, the rationale was that I did not want to require an application to duplicate all of the command logic just to add options to the keybase. Ideally I would have liked to restructure the flow of this command, to split the logic into generating the key bytes from the command parameters and saving the key, so that instead of options passed to the keybase, the key type that is saved could just be defined in the command. I didn't want to make breaking changes though, so I will try to think of a better way of doing this without making breaking changes

Copy link
Contributor

Choose a reason for hiding this comment

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

I see and appreciate the rationale, though I'm not convinced about the implementation. I like see a suboptimal design flow better than a potentially broken one.

Copy link
Contributor Author

@austinabell austinabell Dec 12, 2019

Choose a reason for hiding this comment

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

Oh I'm sorry, I missed this comment. I'll explain what I meant by a different flow by some vague pseudocode and hopefully this will be sufficient to get the point across:

currently the code vaguely does:

addcommand() {
  info ... = kb.CreateAccount(...)
}
...) CreateAccount(..) {
 ...
  derivedBz = ComputeDerivedKey(seed,...)
  writeKey(...derivedBz)
}

Where it would be more ideal to pass the bytes back to the addcommand to be able to derive whatever implementation there instead of having to add keybase options to do this deep in the function calls. It doesn't make much sense to me why the key is derived and written all in one flow anyway. This would have removed the need for this PR altogether because I could have just overriden the add command and been done with it. Example in pseudo:

addcommand() {
  derivedBz = ComputeDerivedKey(...)
  writeKey(...derivedBz)
}

client/keys/add.go Show resolved Hide resolved
client/keys/delete.go Show resolved Hide resolved
crypto/keys/keyring.go Outdated Show resolved Hide resolved
crypto/keys/keyring.go Outdated Show resolved Hide resolved
crypto/keys/keybase_base.go Outdated Show resolved Hide resolved
crypto/keys/keybase_base.go Outdated Show resolved Hide resolved
crypto/keys/keybase_base.go Outdated Show resolved Hide resolved
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.

This is very nice indeed, thanks!
Changes in the Keybase on ongoing, so I think this could be merged after #5180 is merged first.

I'll review this in-depth ASAP anyway, thanks!

@fedekunze fedekunze requested a review from jleni November 6, 2019 10:16
@austinabell
Copy link
Contributor Author

This is very nice indeed, thanks!
Changes in the Keybase on ongoing, so I think this could be merged after #5180 is merged first.

I'll review this in-depth ASAP anyway, thanks!

Awesome, yeah I had noticed that which is why I was trying to delay making a PR but I opened anyway now since it would be beneficial for Ethermint to migrate back to the Cosmos keybase. Now that I know you guys are open to this I will see how I can make the changes work with your changes and clean up the PR after this has been merged in :)

@austinabell
Copy link
Contributor Author

After that PR is merged and you guys get a chance to rereview, let me know if you want me to unseal and not change the codecs to be public, something like: ChainSafe@f5e01d4

I don't think there is another option other than just allowing new types to be registered or overwriting the module codecs unless there are other changes. One idea I did have is to possibly attach the additional account and key types to the module manager and just regenerate the codec with the types that the module manages.

For example: the genutil module codec requires any new key types to be registered where the auth module codec requires new key types and account types to be registered in it's codec. So what my proposition is to add a function to regenerate the module codecs with the new registered types, and use types added to the module manager to update the necessary codecs and reseal them if necessary. The reason this would be useful is for a developer it is unknown which modules require which types to be registered, and is not clear how to update (if even possible).

for example, with these changes in Ethermint, the module codecs have to be updated individually:

	tmamino.RegisterKeyType(PubKeySecp256k1{}, PubKeyAminoName)
	tmamino.RegisterKeyType(PrivKeySecp256k1{}, PrivKeyAminoName)
	authtypes.RegisterAccountTypeCodec(PubKeySecp256k1{}, PubKeyAminoName)
	authtypes.RegisterAccountTypeCodec(PrivKeySecp256k1{}, PrivKeyAminoName)
	clientkeys.RegisterKeyTypeCodec(PubKeySecp256k1{}, PubKeyAminoName)
	clientkeys.RegisterKeyTypeCodec(PrivKeySecp256k1{}, PrivKeyAminoName)
	cryptokeys.RegisterKeyTypeCodec(PubKeySecp256k1{}, PubKeyAminoName)
	cryptokeys.RegisterKeyTypeCodec(PrivKeySecp256k1{}, PrivKeyAminoName)
	genutiltypes.RegisterKeyTypeCodec(PubKeySecp256k1{}, PubKeyAminoName)
	genutiltypes.RegisterKeyTypeCodec(PrivKeySecp256k1{}, PrivKeyAminoName)

and possibly more will be needed once other modules are tested. Another option would be to have a crypto codec that can be modified and resealed that is referenced by all modules, instead of referencing the tendermint amino codec (where this cannot be updated as a dependency).

Copy link
Collaborator

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

LGTM. Pending a Changelog entry and minor godoc changes

client/keys/add_ledger_test.go Outdated Show resolved Hide resolved
client/keys/add_ledger_test.go Outdated Show resolved Hide resolved
crypto/keys/keybase_base.go Outdated Show resolved Hide resolved
crypto/keys/keybase_base.go Show resolved Hide resolved
client/keys/utils.go Outdated Show resolved Hide resolved
@fedekunze fedekunze added the C:Keys Keybase, KMS and HSMs label Nov 22, 2019
client/keys/add.go Outdated Show resolved Hide resolved
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.

Good progress so far! Few things need to be addressed. Would you @austinabell mind to open a PR in cosmos/gaia to test these gaia aganst these changes please?

@austinabell
Copy link
Contributor Author

austinabell commented Dec 12, 2019

Good progress so far! Few things need to be addressed. Would you @austinabell mind to open a PR in cosmos/gaia to test these gaia aganst these changes please?

There are no changes required for Gaia? I'm confused about what you mean specifically

Edit: do you just mean a PR with just replacing the Cosmos-sdk dependency with this commit?

@alessio
Copy link
Contributor

alessio commented Dec 12, 2019

@austinabell dixit:

Edit: do you just mean a PR with just replacing the Cosmos-sdk dependency with this commit?

Exactly. We need to run gaia integration test suites against this.

@@ -4,6 +4,7 @@ import (
"fmt"

"github.com/tendermint/tendermint/crypto"
tmcrypto "github.com/tendermint/tendermint/crypto"
Copy link
Contributor

Choose a reason for hiding this comment

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

This is already imported, see the preceding line

Copy link
Contributor

Choose a reason for hiding this comment

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

this is not yet resolved

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.

Few suggestions

crypto/keys/types.go Outdated Show resolved Hide resolved
crypto/keys/types.go Outdated Show resolved Hide resolved
@alessio
Copy link
Contributor

alessio commented Dec 12, 2019

Just last bit: can you please drop an example somewhere as per #5278 (comment)?

@austinabell
Copy link
Contributor Author

austinabell commented Dec 12, 2019

Just last bit: can you please drop an example somewhere as per #5278 (comment)?

Okay I was somewhat confused by this, because I assumed all logic for this is tested in the test I added, and these tests are testing usually printable data, the ExampleNew() function tests all of this, and there is nothing that gets printed about the overriden keygen function so I don't know what specifically you are looking for me to test or show here.

edit: also sorry I missed your comment before, I responded: #5278 (comment)

@alessio
Copy link
Contributor

alessio commented Dec 12, 2019

@austinabell I meant something like the following to illustrate the usage of the function by example:

diff --git a/crypto/keys/keybase_test.go b/crypto/keys/keybase_test.go
index 2d4752fe3..7fd744c3b 100644
--- a/crypto/keys/keybase_test.go
+++ b/crypto/keys/keybase_test.go
@@ -7,6 +7,7 @@ import (
 
        "github.com/stretchr/testify/assert"
        "github.com/stretchr/testify/require"
+       "github.com/tendermint/tendermint/crypto/secp256k1"
 
        "github.com/tendermint/tendermint/crypto"
        "github.com/tendermint/tendermint/crypto/ed25519"
@@ -401,7 +402,8 @@ func TestSeedPhrase(t *testing.T) {
 
 func ExampleNew() {
        // Select the encryption and storage for your cryptostore
-       cstore := NewInMemory()
+       customKeyGenFunc := func(bz [32]byte) crypto.PrivKey { return secp256k1.PrivKeySecp256k1(bz) }
+       cstore := NewInMemory(WithKeygenFunc(customKeyGenFunc))
 
        sec := Secp256k1
 

However, it's not a blocker

@austinabell
Copy link
Contributor Author

+       customKeyGenFunc := func(bz [32]byte) crypto.PrivKey { return secp256k1.PrivKeySecp256k1(bz) }
+       cstore := NewInMemory(WithKeygenFunc(customKeyGenFunc))

Assumed that would be confusing, since it makes it look like it is a required parameter in the example, but will commit this change

crypto/keys/types.go Outdated Show resolved Hide resolved
crypto/keys/types.go Outdated Show resolved Hide resolved
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.

Nihil obstat from me - pending CI

Thanks!

@codecov
Copy link

codecov bot commented Dec 12, 2019

Codecov Report

Merging #5278 into master will increase coverage by 0.02%.
The diff coverage is 80.95%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5278      +/-   ##
==========================================
+ Coverage   54.53%   54.55%   +0.02%     
==========================================
  Files         311      311              
  Lines       18740    18756      +16     
==========================================
+ Hits        10219    10233      +14     
- Misses       7741     7744       +3     
+ Partials      780      779       -1
Impacted Files Coverage Δ
client/keys/migrate.go 42% <100%> (ø) ⬆️
client/keys/root.go 100% <100%> (ø) ⬆️
client/keys/codec.go 100% <100%> (ø) ⬆️
crypto/keys/types.go 83.33% <100%> (ø) ⬆️
crypto/keys/keybase.go 71.59% <100%> (ø) ⬆️
client/keys/show.go 83.9% <100%> (ø) ⬆️
client/keys/import.go 68.42% <100%> (ø) ⬆️
client/keys/delete.go 77.27% <100%> (ø) ⬆️
client/keys/update.go 73.33% <100%> (ø) ⬆️
crypto/keys/codec.go 100% <100%> (ø) ⬆️
... and 10 more

@alessio alessio merged commit 0e28da2 into cosmos:master Dec 12, 2019
xiangjianmeng pushed a commit to xiangjianmeng/cosmos-sdk that referenced this pull request Dec 18, 2019
Allow for the keybase to be configured to override the implementation
of the key that is saved to the keybase.

Closes: cosmos#4941
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:Keys Keybase, KMS and HSMs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Interchangeable key signing and addressing
5 participants