fix: In keybase writeInfo, enforce one name for address #3221
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Jeff Thompson <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅ 📢 Thoughts on this report? Let us know! |
jefft0
added
the
review/triage-pending
thehowl
approved these changes
Nov 27, 2024
jefft0
removed
the
review/triage-pending
|
Removed the |
zivkovicmilos
approved these changes
Dec 6, 2024
omarsy
pushed a commit
to TERITORI/gno
that referenced
this pull request
Dec 7, 2024
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]>
Villaquiranm
pushed a commit
to Villaquiranm/gno
that referenced
this pull request
Dec 9, 2024
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]>
r3v4s
pushed a commit
to gnoswap-labs/gno
that referenced
this pull request
Dec 10, 2024
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]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
Keybaseinterface 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 #2685 updatedwriteInfofor the case of inserting a new key (address) with the same name as an existing key with a different address. In this case,writeInfouses 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,
writeInfouses 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
Keybasedoc comments to this effect. However, some of the tests in keybase_test.go make this assumption, so this PR fixes them:TestSignVerifycreates 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.TestExportImportPubKeycreates 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...
BREAKING CHANGE: xxxmessage was included in the description