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.

Sign up for GitHub

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

crypto/keyring: fix offline keys migration #8639

Merged
merged 43 commits into from
Mar 1, 2021
Merged

crypto/keyring: fix offline keys migration #8639

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
43 commits
Select commit Hold shift + click to select a range
5610039
add fix for multisig keys
sahith-narahari Feb 19, 2021
f599220
update tests
sahith-narahari Feb 19, 2021
48286a3
Merge branch 'master' into sahith/fix-key-multi
Feb 19, 2021
ca120e8
fix lint
sahith-narahari Feb 19, 2021
266c5f5
Merge branch 'master' into sahith/fix-key-multi
Feb 22, 2021
03e84ec
Merge branch 'master' into sahith/fix-key-multi
jgimeno Feb 23, 2021
88e1eed
rename function legacy
jgimeno Feb 23, 2021
48068ae
Merge branch 'master' into sahith/fix-key-multi
sahith-narahari Feb 23, 2021
3288631
fix linter
jgimeno Feb 23, 2021
da71b04
update keys
sahith-narahari Feb 23, 2021
4b9f9d9
test offline
jgimeno Feb 23, 2021
42682ce
temp commit
jgimeno Feb 23, 2021
9208860
Merge branch 'sahith/fix-key-multi' of github.com:cosmos/cosmos-sdk i…
sahith-narahari Feb 23, 2021
1afcbe3
update ledger keys
sahith-narahari Feb 23, 2021
8944c17
Merge branch 'master' into sahith/fix-key-multi
jgimeno Feb 23, 2021
a34d140
Apply suggestions from code review
Feb 23, 2021
5a65cf6
Merge branch 'master' into sahith/fix-key-multi
Feb 23, 2021
a95da25
Fix `keys migrate` command (#8703)
amaury1093 Feb 25, 2021
ea0807a
Merge branch 'master' into sahith/fix-key-multi
Feb 25, 2021
f212a49
Merge branch 'master' into sahith/fix-key-multi
Feb 26, 2021
eb03b8f
Jonathan/remove unused import pub (#8704)
jgimeno Feb 26, 2021
9e8e55d
update godoc
sahith-narahari Feb 26, 2021
3ce470c
Merge branch 'master' into sahith/fix-key-multi
Feb 26, 2021
5168b53
revert api breaking changes
sahith-narahari Feb 26, 2021
85700ae
Merge branch 'sahith/fix-key-multi' of github.com:cosmos/cosmos-sdk i…
sahith-narahari Feb 26, 2021
b53bc9e
update docs
sahith-narahari Feb 26, 2021
5a048ab
add changelog
sahith-narahari Feb 26, 2021
98416d1
Merge branch 'master' into sahith/fix-key-multi
sahith-narahari Feb 26, 2021
4e328c3
fix end message
Feb 26, 2021
32aeb7b
Merge branch 'master' into sahith/fix-key-multi
Feb 26, 2021
17b606e
try display race detector while running
Feb 26, 2021
e10efd0
turn --old-home into a positional argument
Feb 26, 2021
28070f2
Update CHANGELOG.md
Feb 26, 2021
f7d907d
remove redundant comment
Feb 26, 2021
5478cb6
try fix test
Feb 26, 2021
cbee3fd
Merge branch 'master' into sahith/fix-key-multi
Feb 26, 2021
a7babac
fixing the test case
Feb 26, 2021
455ccce
new testdata
Feb 27, 2021
a49440e
Merge branch 'master' into sahith/fix-key-multi
Feb 27, 2021
d6fca2b
crypto/keyring: reinstate the InfoImporter interface
Feb 27, 2021
8b85b5c
Update error message
sahith-narahari Feb 27, 2021
72229d2
Merge branch 'master' into sahith/fix-key-multi
sahith-narahari Mar 1, 2021
f5df8f1
Update client/keys/migrate.go
Mar 1, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion client/keys/migrate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ func runMigrateCmd(cmd *cobra.Command, args []string) error {
return err
}

if err := migrator.ImportPubKey(keyName, pubkeyArmor); err != nil {
if err := migrator.ImportPubKey(keyName, pubkeyArmor, keyType); err != nil {
return err
}

Expand Down
Binary file added client/keys/testdata/keys/keys.db/000008.ldb
View file
Open in desktop
Binary file not shown.
Binary file added client/keys/testdata/keys/keys.db/000013.ldb
View file
Open in desktop
Binary file not shown.
Binary file added client/keys/testdata/keys/keys.db/000026.ldb
View file
Open in desktop
Binary file not shown.
2 changes: 1 addition & 1 deletion client/keys/testdata/keys/keys.db/CURRENT
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
MANIFEST-000005
MANIFEST-000062
2 changes: 1 addition & 1 deletion client/keys/testdata/keys/keys.db/CURRENT.bak
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
MANIFEST-000003
MANIFEST-000060
354 changes: 324 additions & 30 deletions client/keys/testdata/keys/keys.db/LOG
View file
Open in desktop

Large diffs are not rendered by default.

Binary file removed client/keys/testdata/keys/keys.db/MANIFEST-000005
View file
Open in desktop
Binary file not shown.
Binary file added client/keys/testdata/keys/keys.db/MANIFEST-000062
View file
Open in desktop
Binary file not shown.
7 changes: 7 additions & 0 deletions codec/legacy/codec.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package legacy
import (
"github.com/cosmos/cosmos-sdk/codec"
cryptocodec "github.com/cosmos/cosmos-sdk/crypto/codec"
"github.com/cosmos/cosmos-sdk/crypto/keys/multisig"
cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types"
)

Expand All @@ -29,3 +30,9 @@ func PubKeyFromBytes(pubKeyBytes []byte) (pubKey cryptotypes.PubKey, err error)
err = Cdc.UnmarshalBinaryBare(pubKeyBytes, &pubKey)
return
}

// AminoPubKeyFromBytes unmarshals public key bytes and returns a PubKey
func AminoPubKeyFromBytes(pubKeyBytes []byte) (pubKey multisig.LegacyAminoPubKey, err error) {
err = Cdc.UnmarshalBinaryBare(pubKeyBytes, &pubKey)
return
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we want this function. Could you explain why adding it?
My point is that we shouldn't create functions for specific types. It won't save amount of code:

// instead of calling AminoPubKeyFromBytes we can just write normally:
var pubkey multisig.LegacyAminoPubKey
if err = Cdc.UnmarshalBinaryBare(pubKeyBytes, &pubKey); err != nil

Copy link
Contributor

Choose a reason for hiding this comment

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

The brutal, untested removal of this function caused the mess explained in #8633. Yes, your solution could work indeed. And yet, I believe that there is value in placing and documenting all amino-related legacy code in the codec/legacy package. So that when the time comes for us to remove all legacy stuff, we'll find it more easily. Wdyt?

16 changes: 14 additions & 2 deletions crypto/keyring/keyring.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ type Importer interface {
ImportPrivKey(uid, armor, passphrase string) error

// ImportPubKey imports ASCII armored public keys.
ImportPubKey(uid string, armor string) error
ImportPubKey(uid string, armor string, keyType KeyType) error
}

// Exporter is implemented by key stores that support export of public and private keys.
Expand Down Expand Up @@ -295,7 +295,7 @@ func (ks keystore) ImportPrivKey(uid, armor, passphrase string) error {
return nil
}

func (ks keystore) ImportPubKey(uid string, armor string) error {
func (ks keystore) ImportPubKey(uid string, armor string, keyType KeyType) error {
if _, err := ks.Key(uid); err == nil {
return fmt.Errorf("cannot overwrite key: %s", uid)
}
Expand All @@ -305,6 +305,18 @@ func (ks keystore) ImportPubKey(uid string, armor string) error {
return err
}

if keyType == TypeMulti {
pubKey, err := legacy.AminoPubKeyFromBytes(pubBytes)
if err != nil {
return err
}

_, err = ks.writeMultisigKey(uid, &pubKey)
if err != nil {
return err
}
return nil
}
pubKey, err := legacy.PubKeyFromBytes(pubBytes)
if err != nil {
return err
Expand Down
30 changes: 15 additions & 15 deletions crypto/keyring/keyring_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ func TestSignVerifyKeyRing(t *testing.T) {
require.NoError(t, err)
require.NoError(t, kb.Delete(n2))

require.NoError(t, kb.ImportPubKey(n3, armor))
require.NoError(t, kb.ImportPubKey(n3, armor, TypeLocal))
i3, err := kb.Key(n3)
require.NoError(t, err)
require.Equal(t, i3.GetName(), n3)
Expand Down Expand Up @@ -260,7 +260,7 @@ func TestExportImportPubKeyKeyRing(t *testing.T) {
require.NoError(t, err)

// Import it under a different name
err = kb.ImportPubKey("john-pubkey-only", armor)
err = kb.ImportPubKey("john-pubkey-only", armor, TypeLocal)
require.NoError(t, err)

// Ensure consistency
Expand All @@ -271,7 +271,7 @@ func TestExportImportPubKeyKeyRing(t *testing.T) {
require.True(t, john.GetPubKey().Equals(john2.GetPubKey()))

// Ensure keys cannot be overwritten
err = kb.ImportPubKey("john-pubkey-only", armor)
err = kb.ImportPubKey("john-pubkey-only", armor, TypeLocal)
require.NotNil(t, err)
}

Expand Down Expand Up @@ -302,11 +302,11 @@ func TestAdvancedKeyManagementKeyRing(t *testing.T) {
require.NoError(t, err)

// import succeeds
err = kb.ImportPubKey(n2, exported)
err = kb.ImportPubKey(n2, exported, TypeLocal)
require.NoError(t, err)

// second import fails
err = kb.ImportPubKey(n2, exported)
err = kb.ImportPubKey(n2, exported, TypeLocal)
require.NotNil(t, err)
}

Expand Down Expand Up @@ -549,7 +549,7 @@ func TestInMemorySignVerify(t *testing.T) {
require.Nil(t, err)
err = cstore.Delete(n2)
require.NoError(t, err)
err = cstore.ImportPubKey(n3, armor)
err = cstore.ImportPubKey(n3, armor, TypeLocal)
require.NoError(t, err)
i3, err := cstore.Key(n3)
require.NoError(t, err)
Expand Down Expand Up @@ -580,7 +580,7 @@ func TestInMemoryExportImport(t *testing.T) {
err = cstore.Delete("john")
require.NoError(t, err)

err = cstore.ImportPubKey("john2", armor)
err = cstore.ImportPubKey("john2", armor, TypeLocal)
require.NoError(t, err)

john2, err := cstore.Key("john2")
Expand Down Expand Up @@ -641,7 +641,7 @@ func TestInMemoryExportImportPubKey(t *testing.T) {
require.NoError(t, err)

// Import it under a different name
err = cstore.ImportPubKey("john-pubkey-only", armor)
err = cstore.ImportPubKey("john-pubkey-only", armor, TypeLocal)
require.NoError(t, err)
// Ensure consistency
john2, err := cstore.Key("john-pubkey-only")
Expand All @@ -650,7 +650,7 @@ func TestInMemoryExportImportPubKey(t *testing.T) {
require.True(t, john.GetPubKey().Equals(john2.GetPubKey()))

// Ensure keys cannot be overwritten
err = cstore.ImportPubKey("john-pubkey-only", armor)
err = cstore.ImportPubKey("john-pubkey-only", armor, TypeLocal)
require.NotNil(t, err)
}

Expand Down Expand Up @@ -681,11 +681,11 @@ func TestInMemoryAdvancedKeyManagement(t *testing.T) {
require.NoError(t, err)

// import succeeds
err = cstore.ImportPubKey(n2, exported)
err = cstore.ImportPubKey(n2, exported, TypeLocal)
require.NoError(t, err)

// second import fails
err = cstore.ImportPubKey(n2, exported)
err = cstore.ImportPubKey(n2, exported, TypeLocal)
require.NotNil(t, err)
}

Expand Down Expand Up @@ -1063,11 +1063,11 @@ func TestAltKeyring_ImportExportPubKey(t *testing.T) {
require.NoError(t, err)

newUID := otherID
err = keyring.ImportPubKey(newUID, armor)
err = keyring.ImportPubKey(newUID, armor, TypeLocal)
require.NoError(t, err)

// Should fail importing private key on existing key.
err = keyring.ImportPubKey(newUID, armor)
err = keyring.ImportPubKey(newUID, armor, TypeLocal)
require.EqualError(t, err, fmt.Sprintf("cannot overwrite key: %s", newUID))
}

Expand All @@ -1085,11 +1085,11 @@ func TestAltKeyring_ImportExportPubKey_ByAddress(t *testing.T) {
require.NoError(t, err)

newUID := otherID
err = keyring.ImportPubKey(newUID, armor)
err = keyring.ImportPubKey(newUID, armor, TypeLocal)
require.NoError(t, err)

// Should fail importing private key on existing key.
err = keyring.ImportPubKey(newUID, armor)
err = keyring.ImportPubKey(newUID, armor, TypeLocal)
require.EqualError(t, err, fmt.Sprintf("cannot overwrite key: %s", newUID))
}

Expand Down