fix: In keybase writeInfo, enforce one name for address (gnolang#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 gnolang#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.

<details><summary>Contributors' checklist...</summary>

- [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
</details>

Signed-off-by: Jeff Thompson <[email protected]>

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
 }
 

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)

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