Keybase: Multiple Signature Algorithms #5439

sunnya97 · 2019-12-20T21:56:52Z

Description

This PR further modularizes the work started by @austinabell in #4941, in order to allow multiple signature algorithms to be supported by the same keybase. Required by #4789.

For contributor use:

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.
Code follows the module structure standards.
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added relevant godoc comments.
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, R4R, docs, etc)
Reviewers assigned
Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

client/keys/import.go

client/keys/add.go

crypto/keys/lazy_keybase.go

crypto/keys/types.go

crypto/keys/keybase.go

alessio

It all makes sense and looks good. I'd just like to see two things being taken care of:

Coverage
CHANGELOG entry cause multiple API breaking changes are introduced

CHANGELOG.md

crypto/keys/keybase_test.go

fedekunze

Thanks @sunnya97. A lot of errors are not being checked. Also, the provided hdPath is not validated.

Nit, I personally don't like the term algo as it means "something" in Spanish. Better use the full word algorithm.

client/keys/import.go

client/keys/list_test.go

client/keys/show_test.go

x/auth/ante/sigverify.go

crypto/keys/mintkey/mintkey.go

CHANGELOG.md

crypto/keys/mintkey/mintkey.go

crypto/keys/keybase_base.go

sunnya97 · 2020-01-03T20:38:03Z

There was already a type called SigningAlgo being used in the keybase, so I kept with that terminology. Do you think it's worth going through and changing it everywhere? Algorithm is too wordy imo

crypto/keys/keybase_base.go

fedekunze

utACK. Will wait for @bez and @AdityaSripal review too before merging

crypto/keys/keybase_base.go

alexanderbez

Changes look fine. I left some feedback, mainly questions 👍

alexanderbez · 2020-01-09T22:04:57Z

client/keys/add.go

@@ -112,6 +116,14 @@ func RunAddCmd(cmd *cobra.Command, args []string, kb keys.Keybase, inBuf *bufio.
 	interactive := viper.GetBool(flagInteractive)
 	showMnemonic := !viper.GetBool(flagNoBackup)

+	algo := keys.SigningAlgo(viper.GetString(flagKeyAlgo))


Secp256k1 is already the default. Why manually do this?

Setting Secp256k1 as the default wasn't working for me, something weird with cobra.

alexanderbez · 2020-01-09T22:14:33Z

crypto/keys/keybase_base.go

-func baseSecpPrivKeyGen(bz [32]byte) tmcrypto.PrivKey {
-	return secp256k1.PrivKeySecp256k1(bz)
+// StdPrivKeyGen generates a secp256k1 private key from the given bytes
+func StdPrivKeyGen(bz []byte, algo SigningAlgo) tmcrypto.PrivKey {


This seems...hacky. Why do we need this? For testing purposes only? The method name is not indicative of it only supporting secp256k1 keys. Can we at least expand it to support all other known key types too?

Well, by default, I think we only want to support secp256k1, as that's what the current x/auth module in the Cosmos SDK does. But I agree, it's probably a good ideas to add SecpPrivKeyGen as separate function, and have StdPrivKeyGen use it. That was when we add other keys as the Std defaults, we just add them to the Std function.

But I agree, it's probably a good idea to add SecpPrivKeyGen as a separate function, and have StdPrivKeyGen use it. That was when we add other keys as the Std defaults, we just add them to the Std function.

I see you've already done this! Nice. But I see no harm in supporting all the algos here. Also, we should return an error if the provided algo is not supported or recognized, so the caller doesn't blindly receive nil in such a case.

Do we not want the standard default to be only supporting secp keys?

alexanderbez · 2020-01-09T22:15:37Z

crypto/keys/keybase_base.go

-	masterPriv, ch := hd.ComputeMastersFromSeed(seed)
-	return hd.DerivePrivateKeyForPath(masterPriv, ch, fullHdPath)
+// StdDeriveKey derives and returns the secp256k1 private key for the given seed and HD path.
+func StdDeriveKey(mnemonic string, bip39Passphrase, hdPath string, algo SigningAlgo) ([]byte, error) {


Same comment as StdPrivKeyGen. I'd recommend updating the godoc to be more general and support all known key types. There isn't even an error returned if the algo isn't SECP256K1.

crypto/keys/lazy_keybase.go

crypto/keys/mintkey/mintkey.go

alexanderbez · 2020-01-13T15:48:38Z

crypto/keys/keybase_base.go

-func baseSecpPrivKeyGen(bz [32]byte) tmcrypto.PrivKey {
-	return secp256k1.PrivKeySecp256k1(bz)
+// StdPrivKeyGen generates a secp256k1 private key from the given bytes
+func StdPrivKeyGen(bz []byte, algo SigningAlgo) tmcrypto.PrivKey {


But I agree, it's probably a good idea to add SecpPrivKeyGen as a separate function, and have StdPrivKeyGen use it. That was when we add other keys as the Std defaults, we just add them to the Std function.

I see you've already done this! Nice. But I see no harm in supporting all the algos here. Also, we should return an error if the provided algo is not supported or recognized, so the caller doesn't blindly receive nil in such a case.

alexanderbez · 2020-01-13T15:48:58Z

crypto/keys/keybase_base.go

-	masterPriv, ch := hd.ComputeMastersFromSeed(seed)
-	return hd.DerivePrivateKeyForPath(masterPriv, ch, fullHdPath)
+// StdDeriveKey derives and returns the secp256k1 private key for the given seed and HD path.
+func StdDeriveKey(mnemonic string, bip39Passphrase, hdPath string, algo SigningAlgo) ([]byte, error) {


crypto/keys/mintkey/mintkey.go

alexanderbez

These changes break keys stored in the current master version of keybase requiring a keymigration. But being how we have yet to release v0.38 and users will have to migrate anyway, this may be moot.

Can you please confirm @alessio that users have to migrate all keys in their local keybase with the advent of v0.38 release?

alessio · 2020-01-14T12:06:59Z

On Mon, 13 Jan 2020, 23:06 Alexander Bezobchuk, ***@***.***> wrote: Can you please confirm @alessio <https://github.com/alessio> that users have to migrate all keys in their local keybase with the advent of v0.38 release

Yes, I confirm

…

alexanderbez

testedACK

codecov · 2020-01-14T16:17:39Z

Codecov Report

Merging #5439 into master will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #5439      +/-   ##
==========================================
+ Coverage   54.35%   54.36%   +0.01%     
==========================================
  Files         313      313              
  Lines       18908    18908              
==========================================
+ Hits        10277    10279       +2     
+ Misses       7844     7842       -2     
  Partials      787      787

Impacted Files	Coverage Δ
x/mock/app.go	`64.18% <0%> (+1.35%)`	⬆️

sunnya97 added R4R C:Keys Keybase, KMS and HSMs labels Dec 20, 2019

sunnya97 requested review from alessio, alexanderbez and jackzampolin December 20, 2019 21:56

sunnya97 requested review from fedekunze and rigelrozanski as code owners December 20, 2019 21:56

sunnya97 added WIP and removed R4R labels Dec 20, 2019

golangcibot reviewed Dec 20, 2019

View reviewed changes

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

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

sunnya97 added R4R and removed WIP labels Dec 20, 2019

sunnya97 force-pushed the keybase_multi_algo branch from 27f4fe7 to ae68ce9 Compare December 21, 2019 07:32

alessio reviewed Dec 23, 2019

View reviewed changes

crypto/keys/lazy_keybase.go Show resolved Hide resolved

alessio reviewed Dec 23, 2019

View reviewed changes

crypto/keys/types.go Show resolved Hide resolved

alessio reviewed Dec 23, 2019

View reviewed changes

crypto/keys/keybase.go Show resolved Hide resolved

alessio suggested changes Dec 23, 2019

View reviewed changes

sunnya97 force-pushed the keybase_multi_algo branch from 45070ea to 48562f7 Compare December 23, 2019 23:41

alessio reviewed Dec 24, 2019

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

golangcibot reviewed Dec 24, 2019

View reviewed changes

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

sunnya97 added 10 commits December 24, 2019 23:55

in progress

7970983

in progress

90b1f5c

in progress

a16e40f

in progress

2923b54

in progress

97fc60a

fixed defaults

4b056de

fixed unconvert

8934b61

with functions

1a7c67b

modularize full key derivation process

ef203f6

exposed StdDerive and StdGenerate functions

f32a1a4

fedekunze suggested changes Jan 3, 2020

View reviewed changes

address some comments review

08059ce

Merge branch 'master' into keybase_multi_algo

6e50c29

fedekunze reviewed Jan 6, 2020

View reviewed changes

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

fedekunze approved these changes Jan 6, 2020

View reviewed changes

crypto/keys/keybase_base.go Show resolved Hide resolved

fedekunze and others added 3 commits January 6, 2020 20:25

Update crypto/keys/keybase_base.go

8faaca3

added err check

6777c23

added return type info to godoc of UnarmorDecryptPrivKey

906a5ee

sunnya97 requested review from AdityaSripal and removed request for jackzampolin and rigelrozanski January 7, 2020 01:38

Merge branch 'master' into keybase_multi_algo

318c145

sunnya97 requested a review from jackzampolin as a code owner January 9, 2020 05:19

Cleanup changelog

959575a

alexanderbez suggested changes Jan 9, 2020

View reviewed changes

address @alexanderbez comments

2c3aa4e

alexanderbez suggested changes Jan 13, 2020

View reviewed changes

sunnya97 added 2 commits January 13, 2020 15:43

added constants for header keys

e10933a

added errors

59874b4

alexanderbez added the T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine). label Jan 13, 2020

alexanderbez reviewed Jan 13, 2020

View reviewed changes

Merge branch 'master' into keybase_multi_algo

930c3be

Merge branch 'master' into keybase_multi_algo

fe70610

alexanderbez approved these changes Jan 14, 2020

View reviewed changes

alexanderbez merged commit f367087 into master Jan 14, 2020

alexanderbez deleted the keybase_multi_algo branch January 14, 2020 15:40

migueldingli1997 mentioned this pull request Aug 11, 2020

General: CLI Transactions should not accept a private key ixofoundation/ixo-blockchain#149

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Keybase: Multiple Signature Algorithms #5439

Keybase: Multiple Signature Algorithms #5439

sunnya97 commented Dec 20, 2019 •

edited by alexanderbez

Loading

alessio left a comment

fedekunze left a comment •

edited

Loading

sunnya97 commented Jan 3, 2020 •

edited

Loading

fedekunze left a comment

alexanderbez left a comment

alexanderbez Jan 9, 2020

sunnya97 Jan 11, 2020

alexanderbez Jan 9, 2020

sunnya97 Jan 11, 2020

alexanderbez Jan 13, 2020

sunnya97 Jan 13, 2020

alexanderbez Jan 9, 2020

alexanderbez Jan 13, 2020

alexanderbez Jan 13, 2020

alexanderbez Jan 13, 2020

alexanderbez left a comment

alessio commented Jan 14, 2020 via email

alexanderbez left a comment

codecov bot commented Jan 14, 2020

Keybase: Multiple Signature Algorithms #5439

Keybase: Multiple Signature Algorithms #5439

Conversation

sunnya97 commented Dec 20, 2019 • edited by alexanderbez Loading

Description

alessio left a comment

Choose a reason for hiding this comment

fedekunze left a comment • edited Loading

Choose a reason for hiding this comment

sunnya97 commented Jan 3, 2020 • edited Loading

fedekunze left a comment

Choose a reason for hiding this comment

alexanderbez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexanderbez left a comment

Choose a reason for hiding this comment

alessio commented Jan 14, 2020 via email

alexanderbez left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 14, 2020

Codecov Report

sunnya97 commented Dec 20, 2019 •

edited by alexanderbez

Loading

fedekunze left a comment •

edited

Loading

sunnya97 commented Jan 3, 2020 •

edited

Loading