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.

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

Migrate Keyring to Protobuf #7108

Closed
aaronc opened this issue Aug 19, 2020 · 17 comments · Fixed by #7987 or #9695
Closed

Migrate Keyring to Protobuf #7108

aaronc opened this issue Aug 19, 2020 · 17 comments · Fixed by #7987 or #9695
Assignees
Labels
C:Keys Keybase, KMS and HSMs

Comments

@aaronc
Copy link
Member

aaronc commented Aug 19, 2020

This got missed in the migration. I don't consider it a blocker for Stargate so marking this for 0.40.1. Does that sound okay @alexanderbez ?

It would be good if we can do this in a backwards compatible way. i.e. if a keyring is in amino format, it gets read as amino but reserialized to proto after that. We should either make a backup when doing this, or even better just use different files for proto keys and keep the amino ones untouched.

@aaronc aaronc added the C:Keys Keybase, KMS and HSMs label Aug 19, 2020
@aaronc aaronc added this to the v0.40.1 milestone Aug 19, 2020
@alexanderbez
Copy link
Contributor

alexanderbez commented Aug 19, 2020

Yes, that's fine. In fact, I think I have a comment in a thread somewhere that alludes to this very idea -- use Amino for legacy key material and use Proto for new key material while also having a built-in and opt-in command to migrate anything that is legacy so we can ultimately phase out Amino entirely from the Keyring logic.

@aaronc aaronc added ice-box S:blocked Status: Blocked and removed S:blocked Status: Blocked labels Oct 14, 2020
@alessio
Copy link
Contributor

alessio commented Nov 16, 2020

IMHO keys material does not necessarily have to be encoded with proto. The keyring is a Cosmos SDK pure Go tool and is not meant to be used in conjunction with messages over the wire (e.g. private keys don't travel over the wire). I think the objective of this issue should be stop using amino for keyring's keys material.

@amaury1093
Copy link
Contributor

Could you explain a little bit more @alessio?

From what I understand, if we stop using amino for keyring's keys material, then we will use protobuf for serialization between the keyring and the keyring-backend, which is the objective of this issue.

@alessio
Copy link
Contributor

alessio commented Nov 16, 2020

protobuf for serialization between the keyring and the keyring-backend, that's generally ok - as long as client developers don't need to write proto files specific to the keys the use.

Keyring just handle bytes-to-disk serialization of a few internal structures (see *info types in crypto/keyring). For that, gob should suffice?

@amaury1093
Copy link
Contributor

amaury1093 commented Nov 16, 2020

I'm not familiar with gob, how does gob encode interfaces? Say I want to serialize localInfo:

type localInfo struct {
Name string `json:"name"`
PubKey cryptotypes.PubKey `json:"pubkey"`
PrivKeyArmor string `json:"privkey.armor"`
Algo hd.PubKeyType `json:"algo"`
}

How will the PubKey field (an interface) be encoded? Proto uses Any, amino also has its own mechanism.

@fedekunze
Copy link
Collaborator

We are seeing another issue on Ethermin due to the fact that the SDK uses keyring.CryptoCdc and legacy.Cdc.Amino inconsistently, see v0.40.0-rc3...chengwenxi:vincent-v0.40.0-rc3

@amaury1093
Copy link
Contributor

This is not closed by #7987

@amaury1093 amaury1093 reopened this Dec 2, 2020
@aaronc aaronc modified the milestones: v0.40.1, v0.42 Jan 6, 2021
@robert-zaremba
Copy link
Collaborator

Scope

Create a migration for keyring keys:

  • don't use legacy codec
  • remove crypto/keyring.multiInfo.PubKeys field

@robert-zaremba
Copy link
Collaborator

robert-zaremba commented Mar 12, 2021

Problem

Currently, all user keys in keyring are serialized using Amino. This blocks adding new key types to keyring unless we implement Amino codec interface to that types.

[outdated] Proposed Migration Algorithm

See #7108 (comment) below for the algorithm we picked.

Add keyring migration for user keys:

  1. Add migration to the constructor
  2. In the constructor check if keyring migration was already done (see below how we do it)
  3. If done, return new kerying
  4. If not, go through all keyring records and migrate from Amino to Protobuf
  5. Save an extra record to the keyring: {Key: "migration", Data: versionNumber}, where version number for this migration is 1.
  6. Return keyring.

All keyring functions must use protobuf for serialization.

Ref: discussion on Discord

@robert-zaremba
Copy link
Collaborator

robert-zaremba commented Apr 14, 2021

Algorithm v2

The logic we are implementing is:

  1. Add migration to the constructor
  2. When constructing the keyring, check a version (using a special "keyring-cosmos-version" key), if the key doesn't exist then migrate:
    1. Iterate through all keys
    2. try proto deserialization
    3. if it fails -> migrate the from amino to proto
    4. if it stil fails -> return warning, and continue with other keys.
    5. set a version to 1.
  3. A user access a key
    • Try proto deserialization
    • if it fails - try amino and and migrate the single key.

Using a version will allow us to do future migrations if needed.

@cyberbono3 cyberbono3 self-assigned this Apr 22, 2021
This was referenced Apr 28, 2021
@mergify mergify bot closed this as completed in #9695 Sep 20, 2021
mergify bot pushed a commit that referenced this issue Sep 20, 2021
<!--
The default pull request template is for types feat, fix, or refactor.
For other templates, add one of the following parameters to the url:
- template=docs.md
- template=other.md
-->

## Description

The draft PR #9222 
Closes: #7108

<!-- Add a description of the changes that this PR introduces and the files that
are the most critical to review. -->

- implement proto definition for `Record` 
- rename `Info.go` to `legacyInfo.go` within `keyring` package
- implement CLI `migrate` command that migrates all keys from  legacyInfo to proto according to @robert-zaremba migration [algorithm](#9222)
- remove legacy keybase entirely.
- add `Migrate` and `MigrateAll` functions  in `keyring.go` for single key and all keys migration
- add tests
- fix tests

---

### Author Checklist

*All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.*

I have...

- [ ] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] added `!` to the type prefix if API or client breaking change
- [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting))
- [ ] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules)
- [x] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing)
- [ ] added a changelog entry to `CHANGELOG.md`
- [ ] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [ ] updated the relevant documentation or specification
- [x] reviewed "Files changed" and left comments if necessary
- [ ] confirmed all CI checks have passed

### Reviewers Checklist

*All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.*

I have...

- [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] confirmed `!` in the type prefix if API or client breaking change
- [ ] confirmed all author checklist items have been addressed 
- [ ] reviewed state machine logic
- [ ] reviewed API design and naming
- [ ] reviewed documentation is accurate
- [ ] reviewed tests and test coverage
- [ ] manually tested (if applicable)
larry0x pushed a commit to larry0x/cosmos-sdk that referenced this issue May 22, 2023
<!--
The default pull request template is for types feat, fix, or refactor.
For other templates, add one of the following parameters to the url:
- template=docs.md
- template=other.md
-->

## Description

The draft PR cosmos#9222 
Closes: cosmos#7108

<!-- Add a description of the changes that this PR introduces and the files that
are the most critical to review. -->

- implement proto definition for `Record` 
- rename `Info.go` to `legacyInfo.go` within `keyring` package
- implement CLI `migrate` command that migrates all keys from  legacyInfo to proto according to @robert-zaremba migration [algorithm](cosmos#9222)
- remove legacy keybase entirely.
- add `Migrate` and `MigrateAll` functions  in `keyring.go` for single key and all keys migration
- add tests
- fix tests

---

### Author Checklist

*All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.*

I have...

- [ ] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] added `!` to the type prefix if API or client breaking change
- [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting))
- [ ] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules)
- [x] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing)
- [ ] added a changelog entry to `CHANGELOG.md`
- [ ] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [ ] updated the relevant documentation or specification
- [x] reviewed "Files changed" and left comments if necessary
- [ ] confirmed all CI checks have passed

### Reviewers Checklist

*All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.*

I have...

- [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] confirmed `!` in the type prefix if API or client breaking change
- [ ] confirmed all author checklist items have been addressed 
- [ ] reviewed state machine logic
- [ ] reviewed API design and naming
- [ ] reviewed documentation is accurate
- [ ] reviewed tests and test coverage
- [ ] manually tested (if applicable)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment