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

Operator key replacement #3863

Open
mdyring opened this issue Mar 12, 2019 · 7 comments

Comments

Projects
None yet
5 participants
@mdyring
Copy link

commented Mar 12, 2019

Summary

This is a feature request to introduce a replacement mechanism for validator operator keys.

It seems like a sensible place to introduce this would be in the "edit validator" code paths.

Problem Definition

With the fairly recent introduction of multisig it seems likely that validators will want to strengthen their security post launch, which will require the ability to replace the valoper key.

If a valoper key compromise is suspected it also makes sense to replace it.

Proposal

From a validator perspective it should be as simple as:

gaiacli edit-validator ... --from <existing key> --next-from <new key>

TX should be signed by the existing key and, for bonus points and to reduce risk, could required a signature with the new key as well.

Replacement should only be allowed if the new valoper account adheres to the minimum self bond.

Couple of ideas to solve scenario where validator can't finance 2 x min-self-bond:

  1. Respect a send message that happens in same TX, which moves >= min-self-bond from old account to new (my personal preference)
  2. Introduce explicit TX type (so not edit-validator) for this, which includes the logic to sufficiently fund the new account to level of min-self-bond.

For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@alexanderbez

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2019

Thanks @mdyring! This can be of great benefit to the validator community certainly. Unfortunately, this will not be so easy on the SDK as the validator operator key is used in many places throughout the SDK as a primary index key -- this will be a very costly tx.

@mdyring

This comment has been minimized.

Copy link
Author

commented Mar 12, 2019

I wouldn't expect this to be a common occurence and only done if required. So high cost seem OK.

@zmanian

This comment has been minimized.

Copy link
Member

commented Apr 17, 2019

One thing that would pair well with this is the idea of being able to move a bond between accounts without unbonding.

@rigelrozanski

This comment has been minimized.

Copy link
Member

commented May 16, 2019

I'd like to hear @sunnya97's thoughts on key management strategies here

@sunnya97

This comment has been minimized.

Copy link
Member

commented May 19, 2019

This seems like it could be relatively solved by use of SubKeys. Keep your primary key in the coldest of cold storages, and then you can rotate your "operator key" (the one you actually use) however often you want.

@mdyring

This comment has been minimized.

Copy link
Author

commented May 20, 2019

@sunnya97 @cwgoes looked at the SubKeys proposal and it looks like a step in the right direction!

I like the idea of ACL/permissioned subkeys, but I think it should be improved slightly:

  1. Enable key rotation of all keys, including the master key. I would suggest the modifications below to remove the need for a master key associated with SubKeyAccount while introducing a special wildcard entry ("*") for PermissionedRoutes, which signals the master permission. PubKeyIndex 0 consequentially has no special meaning.

  2. Per-subkey sequence, as independent processes might want to sign for same account with different subkeys. Overhead seems negligible.

A signature would then reference the account instead of master pubkey and point to SubKeyIndex.

// StdSignature represents a sig
type StdSignature struct {
  Address       AccAddress
  Signature     []byte
  SubKeyIndex   uint // References SubKeys, 0 indexed
}

Master PubKey removed from SubKeyAccount along with Sequence:

type SubKeyAccount struct {
  Address       AccAddress
  AccountNumber uint64
  Coins         Coins
  SubKeys       []SubKeyMetadata
}

Sequence tracked for each SubKey:

type SubKeyMetadata struct {
  PubKey               sdk.AccPubKey
  PermissionedRoutes   []string  // Route of "*" implies Master permission
  DailyFeeAllowance    sdk.Coins
  DailyFeeUsed         sdk.Coins
  Sequence             uint64
  Revoked              bool
}

It would probably make sense to migrate existing accounts to the new account structure, this seems to be straight-forward migrate by taking existing PubKey and adding as SubKey with "*" PermissionedRoutes.

@sunnya97

This comment has been minimized.

Copy link
Member

commented May 20, 2019

So the MasterKey is special because it is the key whose hash is the account address. This is important to enable account pruning in the future. See @zmanian's comments on this thread:
#4352

By the way, I reopened the SubKeys PR here:
#4380

@sunnya97 sunnya97 closed this May 20, 2019

@sunnya97 sunnya97 reopened this May 20, 2019

@mdyring mdyring referenced this issue May 22, 2019

Open

WIP: Subkeys Spec #4380

0 of 5 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.