[Feature Request]: Keys Management

[Bidon15]

Summary

Celestia Node can CRUD keys

Impact on users

It will be beneficial to produce a feature that allows Celestia Node to do CRUD on keys as this can allow signing Txs.

Currently, we have a merged ADR #369 that is proposing a state interaction for Celestia Node and a respective PR #411

Implementation ideas

As we are already using RPC approach lately, we can use it for this feature

Urgency

Medium

[liamsi]

The cosmos-sdk uses this for key storage: https://github.com/cosmos/cosmos-sdk/blob/master/crypto/keyring/doc.go
And this is also what lens uses under the hood I think.

[renaynay]

Yes @liamsi. I suggest if we continue w/ using lens, we should be consistent and use SDK's keyring.

[liamsi]

I think independent of lens we should be consistent and use SDK's keyring.

[renaynay]

We already do with #420

[Wondertan]

While reviewing #420 I went deeper into SDK's keyring. My observations are:

• They have some custom migration logic for amino and stuff that we don't need
• They introduce a keyring interface that is overloaded, not ergonomic and can be simplified
  • They treat Ledger as something that needs additional methods rather than an implementation detail
  • SupportedAlgorithms() (SigningAlgoList, SigningAlgoList) two lists instead of one?
  • Exporter interface whose responsibility is only ASCII armoring shouldn't really be an interface.
    • There can be only GetPubKey and GetPrivKey methods with armoring as separate utility functions and not an interface.
    • These getters will also remove the need for an Unsafe interface.
    • Importers could also use generic setters/writers with armoring treated elsewhere. Basically, armoring is a (de)serialization format that should not be a part of the interface.
  • Methods for migration
  • Multisig special methods can be implementation details
  • Why wallets are indexed by bother uid and address? Why only address or uid is not enough?
    • There might be a good reason for this, but it seems like a legacy
• They use github.com/99designs/keyring which looks sane.
  • The SDK's Keyring interface does not look sane and long term we should write our own generic wallet management which may wrap 99designs/keyring
  • We should look deeper into extending 99designs/keyring backend implementations with Ledger as a backend and possibly other hardware wallets. If its Keyring interface allows us to do so then we can remove our own Keystore interface, If not we should look for other generic wallets and/or improve on our Keystore

cc @liamsi @renaynay

[Wondertan]

I believe that all the Celestia wallets(like Kepler) should be super/light clients by default, so celestia-node(which implements those) has to contain first-class wallet support.

[renaynay]

Another requirement: when constructing keyring, allow users to pass custom userInput.

Ref: #420

[Wondertan]

@renaynay, this shouldn't be a requirement, but optional feature that tightens security for users. As otherwise, before sending any the user would need to write the password. But it can be undesirable:

• User has OS-level data encryption and he enters a similar purpose password every time he enters his PC
• User can have a desirable level of security just by entering the password to unlock his PR through a login screen. This does not involve encryption of the store private keys, but it is ok for the majority of users, IMO

Personally, I would like to have this feature, but I would not call this a requirement.

[renaynay]

"Allow users to specify a custom key name we should also allow custom path to the keyring, not necessary under store/keys, so that users can access keyrings elsewhere"

[tzdybal]

Any estimates on delivery of this feature?

[renaynay]

Allow users to indicate whether they want the mnemonic for generated key to be logged.

[renaynay]

Any estimates on delivery of this feature?

@tzdybal we already have a basic key utility that is taken from cosmos-sdk. It can be accessed via celestia <node_type> keys --help

[Bidon15]

Grooming 16/11/2022:

This issue is touching org wide decision making and we need help to understand how to resolve this org wise
cc: @MSevey

[MSevey]

is this a blocker for incentivized testnet or mainnet?

[Wondertan]

Not a blocker. We can live with what CosmosSDK provides for now and for mainnet

[liamsi]

Proposal

Current key-management and signing architecture

Currently the node tightly couples key-management and signing. The rationale is honourable: to not bother developers with either and instead let them focus on the things that matter most when developing rollups instead.

From the point of view of the developer, the architecture around key-management and signing is roughly this

  +----------------------+
  |        Node          |
  |                      |
  | +------------------+ |
  | | Key Management & | |
  | |    Signing       | |
  | +------------------+ |
  +----------+-----------+
             ^
             |
             v
         +-----------+
         |  node API |
         +-----------+
             ^
             |
             v
      +---------------+
      |   Developer   |
      +---------------+

Proposed Change in Architecture

The proposed change involves decoupling the key management and signing functionalities from the node and integrating a Clef-like or Tmkms-like system. The components would be decoupled as follows:

  +----------------------+                                     
  |        Node          |
  |                      |<-------------------+
  +----------+-----------+                    |
             ^                                |
             |                                v 
             |                       +------------------+
             |                       | Key Management & |
             |                       |    Signing       |
             |                       +------------------+
             |                               ^
             v                               |
         +-----------+                       |
         |  node API |                       |
         +-----------+                       |
             ^                               |
             |                               |
             v                               | 
      +---------------+                      | 
      |   Developer   |<---------------------+
      +---------------+

Note: It is completely optional for developers to interact with the signing API directly. Most users/devs won't and won't need to. The node API can still do the signing as before (by interacting with the signing API for the user).

Rationale

Decoupling key management and signing into its own component allows for more flexibility. Also, it could allow for one state-related endpoint in the node API that takes in signed any signed message (like in the cosmos-sdk) instead of hand-picking a few like in the current API. It can be completely optional for devlopers to interact with the key-management/signing API fwiw. The node will have to use it to sign things though.

[Wondertan]

How I understood the architecture: Power users can use the proposed KeyManagement API to sign transactions and send them over SubmitTx like API. Besides, the existing higher-level APIs(like BlobModule) are still accessible as an option(where the node uses the same API to sign itself). Is this correct?

[liamsi]

Besides, the existing higher-level APIs(like BlobModule) are still accessible as an option(where the node uses the same API to sign itself). Is this correct?

Yes. Though I do like the higher-level, public API to be independent of internal go-modules and go-iodiomatisms (like the notions of modules or the naming of functions etc). But that is for another discussion/issue. Just wanted to note that devs will interact with an higher-level API (where they don't need to bother about signing) but it won't be the same as the current existing API necessarily.

[liamsi]

The user/dev flow could be similar to this: https://github.com/ethereum/go-ethereum/blob/115d154392dc47eedbdfe9e598f2f4d9ae421976/cmd/clef/sign_flow.png (manually confirming txs should be optional obviously)

[Wondertan]

We can also make this Wallet/KeyManagement module a building block for modular chains by making sure it does not depend on concrete TX format. Copying the arguments from elsewhere:

It could be useful in different execution environments, so rollup devs don't need to have auth API for every TX format the new execution > environment has/invents and can reuse the identity of the light node. I think this is aligned with the execution environment agnostic vision

And generally, to any auth application, that doesn't have to be transactions, like oauth reusing light node indentity.

[liamsi]

We can also make this Wallet/KeyManagement module a building block for modular chains by making sure it does not depend on concrete TX format.

Don't you think this could be added later though?