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

key assignment feature #144

Merged
merged 9 commits into from
May 3, 2023
Merged

key assignment feature #144

merged 9 commits into from
May 3, 2023

Conversation

agouin
Copy link
Member

@agouin agouin commented May 2, 2023

Splits up keys to require separate key file for each chain ID

Changes --keyfile flag to --key-dir, as there can be multiple keys. Keys must be named {chain_id}_share.json for threshold signer mode, or {chain_id}_priv_validator_key.json for single signer mode.

Breaks up cosigner RSA keys into separate rsa_keys.json key file. RSA config is one private key per cosigner, but ed25519 config is potentially many key private key shards per cosigner.

  • Sharding a priv-validator key is now done with horcrux create-ed25519-shares
  • RSA keys must be created for the cluster separately with horcrux create-rsa-shares

Adds horcrux config migrate command to migrate cosigner share files and config file from v2 to v3:

  • Splits up share.json into {chain_id}_share.json (using chain ID from v2 config) and rsa_keys.json.
  • Removes chain-id from config and writes new config.
  • Removes old share.json file.

Resolves #135

@agouin agouin marked this pull request as ready for review May 2, 2023 18:46
Copy link
Member

@mark-rushakoff mark-rushakoff left a comment

Choose a reason for hiding this comment

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

This was a fairly large PR, so I didn't read it as closely as I would for a smaller PR. Noticed a few small style things, but nothing seems wrong as far as I can see.

cmd/horcrux/cmd/cosigner.go Outdated Show resolved Hide resolved
cmd/horcrux/cmd/key2shares.go Outdated Show resolved Hide resolved
cmd/horcrux/cmd/key2shares.go Outdated Show resolved Hide resolved
cmd/horcrux/cmd/key2shares.go Outdated Show resolved Hide resolved
cmd/horcrux/cmd/key2shares.go Outdated Show resolved Hide resolved
cmd/horcrux/cmd/key2shares_test.go Outdated Show resolved Hide resolved
cmd/horcrux/cmd/signer.go Outdated Show resolved Hide resolved
signer/cosigner_key.go Outdated Show resolved Hide resolved
signer/threshold_validator_test.go Outdated Show resolved Hide resolved
cmd/horcrux/cmd/migrate_test.go Outdated Show resolved Hide resolved
@DavidNix
Copy link
Contributor

DavidNix commented May 2, 2023

I'm taking a look. I'll have some comments. Give me a little bit because this is a large PR.

Copy link
Contributor

@DavidNix DavidNix left a comment

Choose a reason for hiding this comment

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

I ❤️ the design here as well as giving users a migrate command.

I forgot where this was suggested (or maybe I dreamed it): In the future, splitting a PR like this into two, key dir refactor and migrate command, would probably help with code review.

I missed this, but we save keys into 1 dir and name files with a chain-id suffix? Or are there subdirs also named after the chain-id?

cmd/horcrux/cmd/cosigner.go Outdated Show resolved Hide resolved
cmd/horcrux/cmd/fixtures/config-migrated.yaml Outdated Show resolved Hide resolved
cmd/horcrux/cmd/key2shares.go Outdated Show resolved Hide resolved
signer/threshold_validator.go Outdated Show resolved Hide resolved
@mark-rushakoff
Copy link
Member

mark-rushakoff commented May 2, 2023 via email

@agouin agouin requested a review from DavidNix May 3, 2023 00:12
@agouin
Copy link
Member Author

agouin commented May 3, 2023

I missed this, but we save keys into 1 dir and name files with a chain-id suffix?

Sharding will create a dir for each cosigner. The files can be copied directly to the relative cosigners without renaming any files now.

$ ls
priv_validator_key.json

$ horcrux create-ed25519-shares --chain-id cosmoshub-4 --key-file priv_validator_key.json --threshold 2 --shares 3
Created Ed25519 Share cosigner_1/cosmoshub-4_share.json
Created Ed25519 Share cosigner_2/cosmoshub-4_share.json
Created Ed25519 Share cosigner_3/cosmoshub-4_share.json

$ horcrux create-rsa-shares --shares 3
Created RSA Share cosigner_1/rsa_keys.json
Created RSA Share cosigner_2/rsa_keys.json
Created RSA Share cosigner_3/rsa_keys.json

$ ls -R
.:
cosigner_1  cosigner_2  cosigner_3 priv_validator_key.json

./cosigner_1:
cosmoshub-4_share.json  rsa_keys.json

./cosigner_2:
cosmoshub-4_share.json  rsa_keys.json

./cosigner_3:
cosmoshub-4_share.json  rsa_keys.json

@agouin agouin merged commit 219ce9f into main May 3, 2023
@agouin agouin deleted the andrew/key_assignment branch May 3, 2023 15:27
agouin added a commit that referenced this pull request May 3, 2023
* key assignment feature

* lint

* Add migrate command

* Simplify key2shares. Make TestMultipleChainHorcrux use different priv keys for each chain

* fix key2shares test

* handle feedback

* Fix non-out-dir, update docs

* Fix test

* lint
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replicated Security - support key assignment feature
3 participants