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

feat(pubkey): Pubkey module #342

Merged
merged 9 commits into from
Oct 1, 2024
Merged

feat(pubkey): Pubkey module #342

merged 9 commits into from
Oct 1, 2024

Conversation

hacheigriega
Copy link
Member

@hacheigriega hacheigriega commented Aug 26, 2024

Explanation of Changes

This PR adds a new module x/pubkey, which will serve as the public key registry for various signing keys used in the SEDA Protocol. The module store follows the following scheme:

validator_address | index -> public_key

There is no application logic that prevents a validator operator from adding any public keys at any index. However, they should use the official, up-to-date CLI to generate the correct set of SEDA keys and send a transaction that would register their public keys at correct indices. In the initial implementation, the CLI generates a single secp256k1 key, whose public key is to be registered at index 0. The SEDA key file is saved in the same directory as the validator key file. By default, the location is $CHAIN_DIR/config/seda_keys.json.

To generate and register the SEDA keys:

sedad tx pubkey add-seda-keys --from <validator_operator_key> --gas-prices 1000000000aseda --gas auto --gas-adjustment 2.0

To use an existing SEDA key file:

sedad tx pubkey add-seda-keys --key-file ~/.sedad/config/seda_keys.json --from <validator_operator_key> --gas-prices 1000000000aseda --gas auto --gas-adjustment 2.0

To query a given validator's SEDA public keys:

sedad query pubkey validator-keys <validator_operator_addr>

@hacheigriega hacheigriega marked this pull request as ready for review September 5, 2024 17:35
@Thomasvdam
Copy link
Member

I guess it's not entirely clear for me how this module interfaces with modules that need access to the private keys, or how the dependencies are managed.

Maybe it helps to illustrate the following scenarios:

  1. We create a new module that needs to make use of the PKR, how does this new key get added?
  2. A module needs to have something signed, how does this happen?

It would be nice if we could leave all the details of how the keys are managed/stored/etc in the PKR module. I don't think it's possible to express this purely in terms of method/function calls and we'll have to keep a manual file somewhere that links a consumer module identifier to the kind of key it needs.

  • New module needs a keypair: add an entry to a list that consists of an ID (the index, should be unique) and the key implementation (how to generate, how to sign?)
  • On add-keys command the CLI iterates over the list, generates keys at the index of that module, publishes the new public keys in the TX. Private keys get stored in 1 file.
  • New module needs something signed, calls a method on the PKR module to sign data with the key under ID X, PKR returns signed bytes.

Pretty sure I'm simplifying things too much, but I feel like this should be possible.

@hacheigriega
Copy link
Member Author

  1. My thinking was that when we release a new binary that expects some new key to be registered, we would also update the CLI and have the validators use the updated CLI to generate a new set of keys and register their pubkeys.
  2. My thinking at the moment is that signing and verifying should be separated. Signing should be done using utility or context, whereas verifying should be supported by the pubkey module, which would expose the method through expected keepers. I think this separation makes sense because signing deals with the key file and is relevant only to validators. Would love to discuss more if you have doubts, but this is my current thinking.

@hacheigriega
Copy link
Member Author

I will remove unused code like VRF key or CLI endpoint for creating validator with VRF in a separate PR tomorrow

@hacheigriega hacheigriega requested a review from a team September 9, 2024 23:30
@Thomasvdam
Copy link
Member

  1. My thinking was that when we release a new binary that expects some new key to be registered, we would also update the CLI and have the validators use the updated CLI to generate a new set of keys and register their pubkeys.
  2. My thinking at the moment is that signing and verifying should be separated. Signing should be done using utility or context, whereas verifying should be supported by the pubkey module, which would expose the method through expected keepers. I think this separation makes sense because signing deals with the key file and is relevant only to validators. Would love to discuss more if you have doubts, but this is my current thinking.

Lets discuss this tomorrow, maybe we can do some pseudocode to see pros and cons to both approaches.

@hacheigriega hacheigriega changed the title feat: Pubkey module feat(pubkey): Pubkey module Sep 11, 2024
@hacheigriega
Copy link
Member Author

The key interface and the linting error are addressed in PR #365.

Copy link
Contributor

@gluax gluax left a comment

Choose a reason for hiding this comment

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

My comments may be me not understanding the cosmos SDK too well. But it's a security issue if we store private info in plain text in a file.

func LoadOrGenVRFKey(config *cfg.Config, loadPath string) (vrfPubKey sdkcrypto.PubKey, err error) {
var vrfKey *VRFKey
if loadPath != "" {
vrfKey, err = LoadVRFKey(loadPath)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this standard for cosmos? I'd say it's a risk to store a private key as plain text in a file. So we should additionally have the option to do it as an ENV var.

Copy link
Member Author

Choose a reason for hiding this comment

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

created an issue based on what we discussed: #368


// saveSEDAKeys saves a given list of IndexedPrivKey in the directory
// at dirPath.
func saveSEDAKeys(keys []IndexedPrivKey, dirPath string) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

These are private keys right? This is fine for devnet. Cosmos offers the file backend for a similar case, but they do specify you should NOT use file storage for an official deployment.

This security stuff could also done in another pr. Also maybe we could leverage the existing system cosmos has to use a different storage backed.

Copy link
Member Author

Choose a reason for hiding this comment

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

created an issue #368

type CLITestSuite struct {
suite.Suite

kr keyring.Keyring
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I do see this here, can we just use the keyring to store these private keys? In a dev environment that works cause they allow you to use the file backend.

Copy link
Member Author

Choose a reason for hiding this comment

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

Keyring is unsuitable for this purpose because we need repeated access to the key without the key owner's approval every time.

@hacheigriega hacheigriega requested review from gluax and a team September 30, 2024 13:25
DeshErBojhaa and others added 9 commits October 1, 2024 09:04
refactor: added integration tests

add query command

feat: pkr module implementation with add-key tx and query endpoints

feat: pkr module implemented with tests

fix: fix e2e tests and proto query api
chore: CHANGELOG update
chore: proto lint and error msg typo fix

chore: fix test name
chore: add missing proto files and lint

test: improve add key msg validation and unit tests

fix: use comet private key type to save to file

style: lint

refactor: replace some user defined errors with sdk ones
chore: regenerate proto and lint proto

chore: lint
@hacheigriega hacheigriega merged commit 1ddf2ec into main Oct 1, 2024
@hacheigriega hacheigriega deleted the feat/pkr branch October 1, 2024 13:05
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.

4 participants