From f64e1f5302c2238c26ef90203378c363d31e09bd Mon Sep 17 00:00:00 2001 From: Jeff Thompson Date: Fri, 6 Dec 2024 14:24:31 +0100 Subject: [PATCH] fix: In keybase writeInfo, enforce one name for address (#3221) The `Keybase` interface was written with the assumption of a one-to-one correspondence between a key's name and address. But this needs to be enforced by the code. PR https://github.com/gnolang/gno/pull/2685 updated `writeInfo` for the case of inserting a new key (address) with the same name as an existing key with a different address. In this case, `writeInfo` uses the name to look up the existing address and deletes the address entry. This PR does the same for the other case: Inserting a key with the same address as an existing key, but a new name. In this case, `writeInfo` uses the address to look up the existing key's name, and deletes the name entry. This is not a breaking change because none of the production code expects to add a key a second time with the same address but a different name. We update the `Keybase` doc comments to this effect. However, some of the tests in keybase_test.go make this assumption, so this PR fixes them: * `TestSignVerify` creates a key with name n2 and exports its public key. It wants to re-import the public key with name n3 and get the public key to check that it is the same public key as n2. We change this test to re-import the public key into a fresh in-memory key store. * `TestExportImportPubKey` creates a key with name "john", exports its public key, then re-imports it with the new name "john-pubkey-only" (but the same address). The current test checks that the key can still be fetched under the old name "john". But this breaks the one-to-one correspondence of key name and address. So the test is changed to confirm that the key with the old name is replaced by the new name.
Contributors' checklist... - [x] Added new tests, or not needed, or not feasible - [x] Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory - [x] Updated the official documentation or not needed - [x] No breaking changes were made, or a `BREAKING CHANGE: xxx` message was included in the description - [x] Added references to related issues and PRs - [ ] Provided any useful hints for running manual tests
Signed-off-by: Jeff Thompson --- tm2/pkg/crypto/keys/keybase.go | 9 ++++++++- tm2/pkg/crypto/keys/keybase_test.go | 15 ++++++++------- tm2/pkg/crypto/keys/types.go | 3 +++ 3 files changed, 19 insertions(+), 8 deletions(-) diff --git a/tm2/pkg/crypto/keys/keybase.go b/tm2/pkg/crypto/keys/keybase.go index 7f1e152c79c..23c4237151a 100644 --- a/tm2/pkg/crypto/keys/keybase.go +++ b/tm2/pkg/crypto/keys/keybase.go @@ -523,10 +523,17 @@ func (kb dbKeybase) writeInfo(name string, info Info) error { kb.db.DeleteSync(addrKey(oldInfo.GetAddress())) } + addressKey := addrKey(info.GetAddress()) + nameKeyForAddress := kb.db.Get(addressKey) + if len(nameKeyForAddress) > 0 { + // Enforce 1-to-1 name to address. Remove the info by the old name with the same address + kb.db.DeleteSync(nameKeyForAddress) + } + serializedInfo := writeInfo(info) kb.db.SetSync(key, serializedInfo) // store a pointer to the infokey by address for fast lookup - kb.db.SetSync(addrKey(info.GetAddress()), key) + kb.db.SetSync(addressKey, key) return nil } diff --git a/tm2/pkg/crypto/keys/keybase_test.go b/tm2/pkg/crypto/keys/keybase_test.go index bfb21b46fad..25306e62635 100644 --- a/tm2/pkg/crypto/keys/keybase_test.go +++ b/tm2/pkg/crypto/keys/keybase_test.go @@ -149,11 +149,12 @@ func TestSignVerify(t *testing.T) { i2, err := cstore.CreateAccount(n2, mn2, bip39Passphrase, p2, 0, 0) require.Nil(t, err) - // Import a public key + // Import a public key into a new store armor, err := cstore.ExportPubKey(n2) require.Nil(t, err) - cstore.ImportPubKey(n3, armor) - i3, err := cstore.GetByName(n3) + cstore2 := NewInMemory() + cstore2.ImportPubKey(n3, armor) + i3, err := cstore2.GetByName(n3) require.NoError(t, err) require.Equal(t, i3.GetName(), n3) @@ -174,6 +175,7 @@ func TestSignVerify(t *testing.T) { s21, pub2, err := cstore.Sign(n2, p2, d1) require.Nil(t, err) require.Equal(t, i2.GetPubKey(), pub2) + require.Equal(t, i3.GetPubKey(), pub2) s22, pub2, err := cstore.Sign(n2, p2, d2) require.Nil(t, err) @@ -282,11 +284,10 @@ func TestExportImportPubKey(t *testing.T) { require.NoError(t, err) // Compare the public keys require.True(t, john.GetPubKey().Equals(john2.GetPubKey())) - // Ensure the original key hasn't changed - john, err = cstore.GetByName("john") + // Ensure that storing with the address of "john-pubkey-only" removed the entry for "john" + has, err := cstore.HasByName("john") require.NoError(t, err) - require.Equal(t, john.GetPubKey().Address(), addr) - require.Equal(t, john.GetName(), "john") + require.False(t, has) // Ensure keys cannot be overwritten err = cstore.ImportPubKey("john-pubkey-only", armor) diff --git a/tm2/pkg/crypto/keys/types.go b/tm2/pkg/crypto/keys/types.go index 3865951168e..bdaf39caa54 100644 --- a/tm2/pkg/crypto/keys/types.go +++ b/tm2/pkg/crypto/keys/types.go @@ -27,10 +27,12 @@ type Keybase interface { // CreateAccount creates an account based using the BIP44 path (44'/118'/{account}'/0/{index} // Encrypt the key to disk using encryptPasswd. + // If an account exists with the same address but a different name, it is replaced by the new name. // See https://github.com/tendermint/classic/sdk/issues/2095 CreateAccount(name, mnemonic, bip39Passwd, encryptPasswd string, account uint32, index uint32) (Info, error) // Like CreateAccount but from general bip44 params. + // If an account exists with the same address but a different name, it is replaced by the new name. CreateAccountBip44(name, mnemonic, bip39Passwd, encryptPasswd string, params hd.BIP44Params) (Info, error) // CreateLedger creates, stores, and returns a new Ledger key reference @@ -43,6 +45,7 @@ type Keybase interface { CreateMulti(name string, pubkey crypto.PubKey) (info Info, err error) // The following operations will *only* work on locally-stored keys + // In all import operations, if an account exists with the same address but a different name, it is replaced by the new name. Rotate(name, oldpass string, getNewpass func() (string, error)) error Import(name string, armor string) (err error) ImportPrivKey(name, armor, decryptPassphrase, encryptPassphrase string) error