feat: add `gnoland secrets` command suite #1593

zivkovicmilos · 2024-01-26T16:41:10Z

Description

This PR introduces the secret management command suite as a the gnoland subcommand -- gnoland secrets.

It is part of a series of PRs I plan to do on improving the chain initialization flow, with subsequent PRs focusing on the config file and its manipulation.

Secrets being managed:

Validator private key (consensus)
Node p2p key (networking)
Validator last signed state (consensus optimization)

Available commands:

secrets init - Initializes the Gno node secrets locally, including the validator key, validator state and node key
secrets verify - Verifies the Gno node secrets locally, including the validator key, validator state and node key
secrets get - Shows the Gno node secrets locally, including the validator key, validator state and node key

---
title: secrets command suite
---
flowchart LR
    subgraph init
        A[init] --> B[--data-dir]
        
        B -.-> C1[ValidatorPrivateKey]
        B -.-> C2[ValidatorState]
        B -.-> C3[NodeKey]
    end
    subgraph verify
        D[verify] --> E[--data-dir]

        E -.-> E1[ValidatorPrivateKey]
        E -.-> E2[ValidatorState]
        E -.-> E3[NodeKey]
    end
    subgraph get
        G[get] --> H[--data-dir]

        H -.-> H1[ValidatorPrivateKey]
        H -.-> H2[ValidatorState]
        H -.-> H3[NodeKey]
    end

Contributors' checklist...

Added new tests, or not needed, or not feasible
Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
Updated the official documentation or not needed
No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
Added references to related issues and PRs
Provided any useful hints for running manual tests
Added new benchmarks to generated graphs, if any. More info here.

codecov · 2024-01-26T16:44:21Z

Codecov Report

Attention: Patch coverage is 79.32817% with 80 lines in your changes are missing coverage. Please review.

Project coverage is 45.04%. Comparing base (6760265) to head (cf5abb6).

Files	Patch %	Lines
gno.land/cmd/gnoland/secrets_get.go	60.71%	27 Missing and 17 partials ⚠️
gno.land/cmd/gnoland/secrets_init.go	81.05%	9 Missing and 9 partials ⚠️
gno.land/cmd/gnoland/secrets_common.go	82.43%	9 Missing and 4 partials ⚠️
gno.land/cmd/gnoland/secrets_verify.go	93.82%	3 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1593      +/-   ##
==========================================
- Coverage   47.54%   45.04%   -2.51%     
==========================================
  Files         388      464      +76     
  Lines       61242    67975    +6733     
==========================================
+ Hits        29117    30617    +1500     
- Misses      29686    34785    +5099     
- Partials     2439     2573     +134

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

gfanton

Looks great, nothing to say about the code. 👍

Just one suggestion: is it possible to change the name of the package? I feel that secrets is way too generic for a command name. Even though I understand that it might seem a bit redundant, I think gnosecrets would be clearer and more appropriate here.

ajnavarro · 2024-01-29T08:22:38Z

Looks nice, could you add some usage examples to better understand how these commands will be used by the users, and how they will interact with 3rd party commands? Thanks.

moul · 2024-01-29T12:46:13Z

Can we rename secrets for the binary? How about using gnoland config ... as a subcommand instead?

Also, we can change secrets init to gnoland init, and then have gnoland config for verification and gnoland config --show for displaying.

@albttx, what are your thoughts on this API proposal?

moul · 2024-01-29T13:11:01Z

Just a reminder on my vision for node management in the future:

For local development, we should strive for extreme simplicity. Currently, the all-in-one gnoland start command (including initialization) was great for this purpose. However, since we introduced gnodev, I believe we should concentrate our efforts on improving the developer experience with gnodev.
"Junior node operators" (apologies for the term "junior," but it will become clear in point 3): these are individuals who want to join an existing network. They would likely download the genesis and configuration from an official repository and follow tutorials. Right now, I think our primary focus for the gnoland binary should be on this group.
"Advanced node operators" and "network architects" are individuals who are more independent and capable of debugging, among other things. While they still require some tools and documentation, we can consider this an advanced section. Our main emphasis should be on point 2, which should be our primary promotion.

I am mainly referring to "gno.land" and not TM2 for building other blockchains, where we can adopt a different DX strategy.

If we proceed in this direction, I suggest removing features related to secrets and keeping only the essential steps in the gnoland binary to set up a production node that connects to an existing network. Essentially, it would be something like:
gnoland start --with-parameters or possibly gnoland init --params followed by gnoland start. Additionally, we can have gnoland status/doctor/info to verify everything and provide valuable information for support.

As for the new binary, I believe it would be more suitable for advanced users at the moment. We can relocate it to contribs/gnolandtools <secrets> and include various new helpers that could be useful for advanced scenarios.

This approach would give us a binary focused on being a production node (gnoland) and avoid unnecessary updates just because of a new initialization flag. Instead, we want to update the node only if there is a clear advantage for a running node to be updated.

albttx · 2024-01-29T13:18:39Z

Hello, I'm not really sure to understand where is the need of this command.

As a node runner on gnoland and cosmos-sdk based chain, i don't get where i could need this.

I believe what we need is:

gnoland init to generate the priv_validator_key.json + node_key.json + genesis.json
gnoland genesis verify to verify the genesis file, it could also check what you set.

When i need new keys on a cosmos-sdk based chain i do (which is quite rare...)

gaiad init --moniker osef --home /tmp/osef
cp /tmp/osef/config/{node_key.json,priv_validator_key.json} .

I made a PR on a proposal for the gnoland cli here: #1102

zivkovicmilos · 2024-01-29T13:48:08Z

Hello, I'm not really sure to understand where is the need of this command.

As a node runner on gnoland and cosmos-sdk based chain, i don't get where i could need this.

I believe what we need is:

gnoland init to generate the priv_validator_key.json + node_key.json + genesis.json

gnoland genesis verify to verify the genesis file, it could also check what you set.

When i need new keys on a cosmos-sdk based chain i do (which is quite rare...)
gaiad init --moniker osef --home /tmp/osef
cp /tmp/osef/config/{node_key.json,priv_validator_key.json} .
I made a PR on a proposal for the gnoland cli here: #1102

@albttx

To start a node and connect to a new or existing network, users essentially need 4 things:

the network genesis file (genesis.json), that outlines the state for block 0 (and the initial validator set). This file is the same for all network participants, and is shared via whatever medium
the validator private key (used for consensus), even if the node is not a validator (they can become one at any time)
the validator's last sign state (this is a consensus optimization, but in the current implementation required)
the configuration file for the node (in the current implementation required, will fix this in another PR). This file outlines info like bootnodes (for multinode connectivity), path to the genesis.json and so on. Essentially holds info all modules will node to make the node run correctly

gnoland start does this entire generation process for you, without letting you specify anything apart from the output directory on where it should dump this data.

This PR introduces commands that let you generate and manage your secrets, outside of the lazy load gnoland start context, because you need to be able to reuse existing keys (if you're migrating them) or simply generate new ones easily. We don't have any ability in gno to generate or manage these secrets, outside of this PR. You shouldn't need to start the node in order to obtain the validator / p2p key (that you need regardless, in order to construct a valid config.toml in a multinode environment). I wholeheartedly believe that having a dedicated command suite for these simple operations is better than manually trying to copy and figure out where the files are on disk.

With @moul's suggestion, your chain connection process would look like this:

generate genesis.json, or obtain it from somewhere else (genesis generate ... / genesis ...) <---- We have this after feat: add genesis command suite #1252
generate the node secrets, or use existing ones (gnoland init) <---- We have this in this PR
generate the configuration file, if any (gnoland config ...) <---- This is still missing, I will open a PR on it
start the node with the configuration file, secrets and genesis (gnoland start ...) <---- This is already present in the codebase

Please note that the configuration file you provide will contain the list of initial bootnodes that you'll connect to upon startup, and your node will start syncing / communicating with other peers in the network

We've already solved genesis manipulation with #1252 (and child PRs), meaning you can use those subcommands to add validators, change the initial state and so on.

zivkovicmilos · 2024-01-29T14:18:57Z

Can we rename secrets for the binary? How about using gnoland config ... as a subcommand instead?

Also, we can change secrets init to gnoland init, and then have gnoland config for verification and gnoland config --show for displaying.

@albttx, what are your thoughts on this API proposal?

@moul

I plan to integrate a config command suite in my next PR, under gnoland config, so having this functionality labelled as such will just cause confusion (this suite is working with node secrets, not the config itself).

What do you think about swapping secrets -> keys? As in, gnoland keys init ..., and so on.

I am fully in favor of integrating this into the gnoland binary, either through gnoland <secrets> or directly with gnoland.

What option seems better?

gnoland <secrets> all ..., gnoland <secrets> verify ... gnoland <secrets> show ...
gnoland init ..., gnoland verify ..., gnoland show ...

I'm leaning much more towards the first option, because there is a clear separation of functionality.

Keep in mind we will have a config management suite as well, so it would need to follow this pattern as well (gnoland config or directly with gnoland)

cc @gfanton @ajnavarro

zivkovicmilos · 2024-01-29T18:33:22Z

After discussing with @albttx internally, we've converged on a possibly best solution:

The gnoland binary should:

manage the config generation / management (gnoland config ...) (will open up a PR on it)
manage the genesis generation / management (gnoland genesis ...) (currently this is an external binary)
manage the secrets generation / management (gnoland <secrets> ...) (currently this is an external binary, in this PR)

But we also want to introduce a new command: gnoland init, which will be a wombo combo of gnoland config init + gnoland <secrets> init.
This way, the flow for any user to join a Gno chain can be:

gnoland init - initializes the default config and secrets into a data directory
gnoland config p2p --permanent-peers ... (if people want to change default peers). At this point, power users can set up whatever they want in terms of config changes / key changes and so on. This step is not mandatory
gnoland start - starts the node, using the provided genesis.json, config.toml (and initialized secrets)

It's essentially a 2 step process to get up and running as a node (if we exclude the step where people change their default peers...).

I like this approach because:

it's simple, anybody can follow it and not mess up
it gives power users the tools to tinker and create their own gno networks if they want

Looking for thoughts @moul @ajnavarro @gfanton

zivkovicmilos · 2024-01-29T19:51:15Z

I've made this command suite as a subcommand of gnoland:
7d24dd4

albttx · 2024-01-30T09:33:41Z

I'm not really fan of gnoland secret, i really believe secrets generation must be under gnoland init only.

Here is a quick recap of my position on gnoland secret:

I believe it could lead to people making mistakes, gnoland init is enough for generating secrets.
If the idea behind gnoland secret is to have multiple secrets providers in the future (eg: Hashicorp vault). I think it's a mistake.
tm2 provide in his configuration priv_validator_laddr for setting up remote signer (see tmkms or hocrux on tendermint/cometbft) which should be use for setup a secret signer

# TCP or UNIX socket address for CometBFT to listen on for
# connections from an external PrivValidator process
priv_validator_laddr = ""

gnoland config p2p --permanent-peers ... is great 🎉

What do you think about about changing this into a flag ? something like

$ gnoland init --force-new-secrets
are you sure ? [y,N]

## Description Based on discussions in gnolang#1593, this PR introduces the CLI suite for manipulating the `config.toml`. Using this suite, we can build better workflows for power users. This PR is a series of lego blocks that are required for us to get a normal user chain connection going: - gnolang#1252 - solved `genesis.json` management and manipulation - gnolang#1593 - solved node secrets management and manipulation - this PR - solves `config.toml` management and manipulation All of these PRs get us to a point where the user can run: - `gnoland init` - `gnoland start` to get up and running with any Gno network. The added middle step is fine-tuning the configuration and genesis, but it's worth noting this step is optional. New commands: ```shell gnoland config --help USAGE config <subcommand> [flags] Gno config manipulation suite, for editing base and module configurations SUBCOMMANDS init Initializes the Gno node configuration set Edits the Gno node configuration get Shows the Gno node configuration ``` In short, the `gnoland config init` command initializes a default `config.toml`, while other subcommands allow editing viewing any field in the specific configuration. <details><summary>Contributors' checklist...</summary> - [x] Added new tests, or not needed, or not feasible - [x] Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory - [x] Updated the official documentation or not needed - [x] No breaking changes were made, or a `BREAKING CHANGE: xxx` message was included in the description - [x] Added references to related issues and PRs - [ ] Provided any useful hints for running manual tests - [ ] Added new benchmarks to [generated graphs](https://gnoland.github.io/benchmarks), if any. More info [here](https://github.com/gnolang/gno/blob/master/.benchmarks/README.md). </details>

zivkovicmilos · 2024-03-27T15:12:15Z

@thehowl

I've updated this PR after your suggestions, and our internal discussions:

generate secrets into a common --data-dir (validator key, node key, validator state). This directory is actually used for all node data (with subdirs of course, for DBs etc)

have the secrets command auto read the directory (subfolder) for these secrets (predefined file names)

The good part is we were able to reuse the entire testing suite from the previous iteration 😎

5367c44

zivkovicmilos · 2024-03-27T15:26:00Z

@thehowl

I'm not entirely sure what the best way to display available secrets keys (constants) is in the CLI itself. Do you think it's a good idea to show it in the help?

thehowl · 2024-03-27T16:24:19Z

I'm not entirely sure what the best way to display available secrets keys (constants) is in the CLI itself. Do you think it's a good idea to show it in the help?

Yes, let's put them in help, or have a error message like "invalid secret, valid ones: [1 2 3]", like we do for when you put a wrong key in gnoland config

zivkovicmilos · 2024-03-28T02:21:53Z

[ea258b3](/gnolang/gno/pull/1593/commits/ea258b35e13b956cf5fbd5ca87332a42c99ec9ed)

They were in the error message before, but I've added them to the long help now, so it's even more clear (before the user runs the command):

ea258b3

We will also have them on the docs somewhere when we add a secrets page

thehowl

the design overall looks good, some scattered notes on how to improve the code.

since I'm on holiday next week, I suggest you address the issues + look for another approval (since it has been a while since Guilhem approved)

tm2/pkg/bft/privval/file.go

gno.land/cmd/gnoland/secrets_common.go

gno.land/cmd/gnoland/secrets_get.go

gno.land/cmd/gnoland/secrets_common.go

gno.land/cmd/gnoland/secrets_init.go

gno.land/cmd/gnoland/secrets_verify.go

gno.land/cmd/gnoland/secrets.go

gfanton

Looking good! My only complaint would be that the return value of the secrets get command on a specific <key> doesn't produce an exploitable format. I would personally expect that when I use secrets get NodeKey, it produces something that I can pipe with any other command, like mycmd --with-nodekey "$(gnoland secrets getNodeKey)". But maybe that doesn't make sense for other <keys>.
In any case, this isn't mandatory for me in this PR. Feel free to merge it.

zivkovicmilos · 2024-04-02T11:52:10Z

Looking good! My only complaint would be that the return value of the secrets get command on a specific <key> doesn't produce an exploitable format. I would personally expect that when I use secrets get NodeKey, it produces something that I can pipe with any other command, like mycmd --with-nodekey "$(gnoland secrets getNodeKey)". But maybe that doesn't make sense for other <keys>. In any case, this isn't mandatory for me in this PR. Feel free to merge it.

I'll look into having a standard format that can be piped (ie. just the raw key value, address value...) in the future 🙏

zivkovicmilos added 7 commits January 26, 2024 00:17

Add secrets init all

00a36d8

Add secrets init single

a248ff5

Add secrets init unit tests

d21b713

Add verify subcommands and tests

dc2ccf9

Add show subcommands

d81e84b

Add show all unit tests

267a4f0

Add show subcommand unit tests

862e796

zivkovicmilos added 🌱 feature New update to Gno 📦 ⛰️ gno.land Issues or PRs gno.land package related labels Jan 26, 2024

zivkovicmilos requested review from ajnavarro and gfanton January 26, 2024 16:41

zivkovicmilos self-assigned this Jan 26, 2024

zivkovicmilos requested review from moul, jaekwon and piux2 as code owners January 26, 2024 16:41

github-actions bot added the 📦 🌐 tendermint v2 Issues or PRs tm2 related label Jan 26, 2024

gfanton approved these changes Jan 28, 2024

View reviewed changes

Merge branch 'master' into feat/secrets

13ee6be

Make 'secrets' a subcommand of 'gnoland'

7d24dd4

zivkovicmilos mentioned this pull request Jan 30, 2024

feat: add gnoland config command suite #1605

Merged

7 tasks

zivkovicmilos added this to the 🏗4️⃣ test4.gno.land milestone Mar 25, 2024

dongwon8247 mentioned this pull request Mar 25, 2024

Onbloc x Gno Core Workshop gnolang/hackerspace#61

Open

zivkovicmilos mentioned this pull request Mar 25, 2024

[chain] Chain Creation Flow Improvement #1836

Closed

zivkovicmilos added 2 commits March 27, 2024 22:36

Merge branch 'master' into feat/secrets

402feac

Standardize gnoland secrets

5367c44

Tidy error handling

08660a5

zivkovicmilos requested a review from thehowl March 27, 2024 15:26

Add available keys in the secrets long help

ea258b3

Merge branch 'master' into feat/secrets

88a6247

thehowl approved these changes Mar 30, 2024

View reviewed changes

zivkovicmilos added 8 commits April 1, 2024 11:16

Merge branch 'master' into feat/secrets

5096f07

Swap indent character in amino marshal

572c3b2

Add additional explanation on single-key manipulation

889693b

Move out generation logic to secrets_init

bda224e

Add warning about skipping validator signed state verification

888712c

Rename key value

6965f12

Move common methods to secrets_common

c058f86

Add overwrite protection

cf5abb6

gfanton self-requested a review April 2, 2024 09:10

gfanton approved these changes Apr 2, 2024

View reviewed changes

zivkovicmilos merged commit 831bb6f into gnolang:master Apr 2, 2024
183 checks passed

zivkovicmilos deleted the feat/secrets branch April 2, 2024 11:52

zivkovicmilos mentioned this pull request Apr 3, 2024

[chain] Standardize node directory structure #1882

Closed

r3v4s mentioned this pull request Apr 12, 2024

[chain] Add a get-node-id flag to gnoland secrets // multi-node related #1922

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add `gnoland secrets` command suite #1593

feat: add `gnoland secrets` command suite #1593

zivkovicmilos commented Jan 26, 2024 •

edited

Loading

codecov bot commented Jan 26, 2024 •

edited

Loading

gfanton left a comment

ajnavarro commented Jan 29, 2024

moul commented Jan 29, 2024

moul commented Jan 29, 2024

albttx commented Jan 29, 2024

zivkovicmilos commented Jan 29, 2024

zivkovicmilos commented Jan 29, 2024 •

edited

Loading

zivkovicmilos commented Jan 29, 2024 •

edited

Loading

zivkovicmilos commented Jan 29, 2024

albttx commented Jan 30, 2024

zivkovicmilos commented Mar 27, 2024

zivkovicmilos commented Mar 27, 2024

thehowl commented Mar 27, 2024

zivkovicmilos commented Mar 28, 2024

thehowl left a comment

gfanton left a comment

zivkovicmilos commented Apr 2, 2024

feat: add gnoland secrets command suite #1593

feat: add gnoland secrets command suite #1593

Conversation

zivkovicmilos commented Jan 26, 2024 • edited Loading

Description

codecov bot commented Jan 26, 2024 • edited Loading

Codecov Report

gfanton left a comment

Choose a reason for hiding this comment

ajnavarro commented Jan 29, 2024

moul commented Jan 29, 2024

moul commented Jan 29, 2024

albttx commented Jan 29, 2024

zivkovicmilos commented Jan 29, 2024

zivkovicmilos commented Jan 29, 2024 • edited Loading

zivkovicmilos commented Jan 29, 2024 • edited Loading

zivkovicmilos commented Jan 29, 2024

albttx commented Jan 30, 2024

zivkovicmilos commented Mar 27, 2024

zivkovicmilos commented Mar 27, 2024

thehowl commented Mar 27, 2024

zivkovicmilos commented Mar 28, 2024

thehowl left a comment

Choose a reason for hiding this comment

gfanton left a comment

Choose a reason for hiding this comment

zivkovicmilos commented Apr 2, 2024

feat: add `gnoland secrets` command suite #1593

feat: add `gnoland secrets` command suite #1593

zivkovicmilos commented Jan 26, 2024 •

edited

Loading

codecov bot commented Jan 26, 2024 •

edited

Loading

zivkovicmilos commented Jan 29, 2024 •

edited

Loading

zivkovicmilos commented Jan 29, 2024 •

edited

Loading