Interchangable PrivKey implementations in keybase #5278

austinabell · 2019-11-05T19:46:50Z

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)

crypto/keys/keybase_base.go

alexanderbez · 2019-11-05T20:10:51Z

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

fedekunze

Thanks @austinabell! Minor comments only.

fedekunze · 2019-11-05T20:44:59Z

client/keys/add.go

+		return err
+	}
+
+	return RunAddCmd(cmd, args, kb)


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

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

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.

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

client/keys/delete.go

crypto/keys/keyring.go

crypto/keys/keybase_base.go

alessio

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!

austinabell · 2019-11-06T13:45:13Z

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 · 2019-11-06T17:18:19Z

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).

fedekunze

LGTM. Pending a Changelog entry and minor godoc changes

client/keys/add_ledger_test.go

crypto/keys/keybase_base.go

client/keys/utils.go

client/keys/add.go

client/keys/codec.go

client/keys/utils.go

alessio

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 · 2019-12-12T17:21:07Z

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 · 2019-12-12T17:39:07Z

@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.

crypto/keys/lazy_keybase_test.go

alessio · 2019-12-12T18:40:26Z

crypto/keys/types.go

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

 	"github.com/tendermint/tendermint/crypto"
+	tmcrypto "github.com/tendermint/tendermint/crypto"


This is already imported, see the preceding line

this is not yet resolved

alessio

Few suggestions

crypto/keys/types.go

crypto/keys/lazy_keybase_test.go

…to austin/kbsigning

alessio · 2019-12-12T19:09:33Z

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

austinabell · 2019-12-12T19:18:10Z

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 · 2019-12-12T19:41:06Z

@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 · 2019-12-12T19:44:42Z

+       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

alessio

Nihil obstat from me - pending CI

Thanks!

codecov · 2019-12-12T21:03:49Z

Codecov Report

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

@@            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

Allow for the keybase to be configured to override the implementation of the key that is saved to the keybase. Closes: cosmos#4941

austinabell requested review from alessio, alexanderbez, fedekunze, jackzampolin and rigelrozanski as code owners November 5, 2019 19:46

golangcibot reviewed Nov 5, 2019

View reviewed changes

crypto/keys/keybase_base.go Outdated Show resolved Hide resolved

fedekunze suggested changes Nov 6, 2019

View reviewed changes

alessio suggested changes Nov 6, 2019

View reviewed changes

fedekunze requested a review from jleni November 6, 2019 10:16

austinabell added 7 commits November 20, 2019 11:42

Added keybase options to be able to override key algo

9d3227a

merge

71b0c89

Refactor

5bff74e

Fix typo

91f072d

merge

80ed922

Updates keybase options function and added function documentation

3b5f8c0

Changes necessary to apply options to keyring keybase

d378296

austinabell force-pushed the austin/kbsigning branch from 48600d8 to d378296 Compare November 21, 2019 17:03

Merge branch 'master' into austin/kbsigning

de4a612

austinabell mentioned this pull request Nov 21, 2019

Cosmos PR changes cosmos/ethermint#158

Merged

fedekunze requested a review from alessio November 22, 2019 11:55

fedekunze added the R4R label Nov 22, 2019

fedekunze approved these changes Nov 22, 2019

View reviewed changes

fedekunze added the C:Keys Keybase, KMS and HSMs label Nov 22, 2019

address PR comments

d0c2800

alessio reviewed Nov 25, 2019

View reviewed changes

client/keys/add.go Outdated Show resolved Hide resolved

alessio reviewed Nov 25, 2019

View reviewed changes

client/keys/codec.go Show resolved Hide resolved

alessio reviewed Nov 25, 2019

View reviewed changes

client/keys/utils.go Show resolved Hide resolved

alessio suggested changes Dec 12, 2019

View reviewed changes

address comments other than test

b13d540

Add keygen override test

f0c82cf

golangcibot reviewed Dec 12, 2019

View reviewed changes

crypto/keys/lazy_keybase_test.go Outdated Show resolved Hide resolved

misspell lol

d54277e

alessio reviewed Dec 12, 2019

View reviewed changes

crypto/keys/types.go Outdated Show resolved Hide resolved

crypto/keys/types.go Outdated Show resolved Hide resolved

Alessio Treglia and others added 2 commits December 12, 2019 18:44

Merge branch 'master' into austin/kbsigning

df1d4f4

remove duplicate dep ref

9ef070b

alessio reviewed Dec 12, 2019

View reviewed changes

crypto/keys/lazy_keybase_test.go Outdated Show resolved Hide resolved

Merge branch 'austin/kbsigning' of github.com:ChainSafe/cosmos-sdk in…

bae2c1b

…to austin/kbsigning

austinabell mentioned this pull request Dec 12, 2019

Interchangeable key implementations SDK PR cosmos/gaia#224

Closed

Merge branch 'master' into austin/kbsigning

e211980

change example test

e87e815

alessio reviewed Dec 12, 2019

View reviewed changes

crypto/keys/types.go Outdated Show resolved Hide resolved

alessio reviewed Dec 12, 2019

View reviewed changes

crypto/keys/types.go Outdated Show resolved Hide resolved

remove duplicate dep ref

4d68fb2

alessio approved these changes Dec 12, 2019

View reviewed changes

alessio merged commit 0e28da2 into cosmos:master Dec 12, 2019

austinabell mentioned this pull request Dec 13, 2019

Apply options on missed keyring types #5404

Merged

11 tasks

alessio mentioned this pull request Dec 13, 2019

Refresh dependencies to incorporate sdk's latest changes cosmos/gaia#227

Merged

11 tasks

TraderWithPython mentioned this pull request Jun 30, 2024

Supporting Multiple Signature Algorithms in Cosmos SDK for Compatibility with Major Wallets #20822

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Interchangable PrivKey implementations in keybase #5278

Interchangable PrivKey implementations in keybase #5278

austinabell commented Nov 5, 2019 •

edited by fedekunze

Loading

alexanderbez commented Nov 5, 2019

fedekunze left a comment

fedekunze Nov 5, 2019

austinabell Nov 6, 2019

alessio Nov 25, 2019

austinabell Dec 12, 2019 •

edited

Loading

alessio left a comment

austinabell commented Nov 6, 2019

austinabell commented Nov 6, 2019

fedekunze left a comment

alessio left a comment

austinabell commented Dec 12, 2019 •

edited

Loading

alessio commented Dec 12, 2019

alessio Dec 12, 2019

alessio Dec 12, 2019

alessio left a comment

alessio commented Dec 12, 2019

austinabell commented Dec 12, 2019 •

edited

Loading

alessio commented Dec 12, 2019

austinabell commented Dec 12, 2019

alessio left a comment

codecov bot commented Dec 12, 2019 •

edited

Loading

Interchangable PrivKey implementations in keybase #5278

Interchangable PrivKey implementations in keybase #5278

Conversation

austinabell commented Nov 5, 2019 • edited by fedekunze Loading

alexanderbez commented Nov 5, 2019

fedekunze left a comment

Choose a reason for hiding this comment

fedekunze Nov 5, 2019

Choose a reason for hiding this comment

austinabell Nov 6, 2019

Choose a reason for hiding this comment

alessio Nov 25, 2019

Choose a reason for hiding this comment

austinabell Dec 12, 2019 • edited Loading

Choose a reason for hiding this comment

alessio left a comment

Choose a reason for hiding this comment

austinabell commented Nov 6, 2019

austinabell commented Nov 6, 2019

fedekunze left a comment

Choose a reason for hiding this comment

alessio left a comment

Choose a reason for hiding this comment

austinabell commented Dec 12, 2019 • edited Loading

alessio commented Dec 12, 2019

alessio Dec 12, 2019

Choose a reason for hiding this comment

alessio Dec 12, 2019

Choose a reason for hiding this comment

alessio left a comment

Choose a reason for hiding this comment

alessio commented Dec 12, 2019

austinabell commented Dec 12, 2019 • edited Loading

alessio commented Dec 12, 2019

austinabell commented Dec 12, 2019

alessio left a comment

Choose a reason for hiding this comment

codecov bot commented Dec 12, 2019 • edited Loading

Codecov Report

austinabell commented Nov 5, 2019 •

edited by fedekunze

Loading

austinabell Dec 12, 2019 •

edited

Loading

austinabell commented Dec 12, 2019 •

edited

Loading

austinabell commented Dec 12, 2019 •

edited

Loading

codecov bot commented Dec 12, 2019 •

edited

Loading