From e7984803ed1c3bec85e86276a840bc642ca95723 Mon Sep 17 00:00:00 2001 From: Alexandru Gheorghe Date: Wed, 27 Mar 2024 11:05:18 +0200 Subject: [PATCH 01/21] Prevent accidental change of network-key for active authorities Signed-off-by: Alexandru Gheorghe --- substrate/client/cli/src/config.rs | 4 +- substrate/client/cli/src/error.rs | 12 ++++++ .../client/cli/src/params/node_key_params.rs | 40 +++++++++++++++---- 3 files changed, 48 insertions(+), 8 deletions(-) diff --git a/substrate/client/cli/src/config.rs b/substrate/client/cli/src/config.rs index 5def9ce9b726..411d567bdd23 100644 --- a/substrate/client/cli/src/config.rs +++ b/substrate/client/cli/src/config.rs @@ -428,8 +428,10 @@ pub trait CliConfiguration: Sized { /// By default this is retrieved from `NodeKeyParams` if it is available. Otherwise its /// `NodeKeyConfig::default()`. fn node_key(&self, net_config_dir: &PathBuf) -> Result { + let is_dev = self.is_dev()?; + let role = self.role(is_dev)?; self.node_key_params() - .map(|x| x.node_key(net_config_dir)) + .map(|x| x.node_key(net_config_dir, role, is_dev)) .unwrap_or_else(|| Ok(Default::default())) } diff --git a/substrate/client/cli/src/error.rs b/substrate/client/cli/src/error.rs index 6c0cfca4932e..5bf726c335fd 100644 --- a/substrate/client/cli/src/error.rs +++ b/substrate/client/cli/src/error.rs @@ -18,6 +18,8 @@ //! Initialization errors. +use std::path::PathBuf; + use sp_core::crypto; /// Result type alias for the CLI. @@ -78,6 +80,16 @@ pub enum Error { #[error(transparent)] GlobalLoggerError(#[from] sc_tracing::logging::Error), + + #[error( + "Starting an authorithy without network key in {0}. + \n This is not a safe operation because the old identity still lives in the dht for 36 hours. + \n Because of it your node might suffer from not being properly connected to other nodes for validation purposes. + \n If it is the first time running your node you could use one of the following methods. + \n 1. Pass --unsafe-force-node-key-generation and make sure you remove it for subsequent node restarts + \n 2. Separetly generate the key with: polkadot key generate-node-key --file " + )] + NetworkKeyNotFound(PathBuf), } impl From<&str> for Error { diff --git a/substrate/client/cli/src/params/node_key_params.rs b/substrate/client/cli/src/params/node_key_params.rs index 53f19f58e1fb..8d824180acad 100644 --- a/substrate/client/cli/src/params/node_key_params.rs +++ b/substrate/client/cli/src/params/node_key_params.rs @@ -18,10 +18,11 @@ use clap::Args; use sc_network::config::{identity::ed25519, NodeKeyConfig}; +use sc_service::Role; use sp_core::H256; use std::{path::PathBuf, str::FromStr}; -use crate::{arg_enums::NodeKeyType, error}; +use crate::{arg_enums::NodeKeyType, error, Error}; /// The file name of the node's Ed25519 secret key inside the chain-specific /// network config directory, if neither `--node-key` nor `--node-key-file` @@ -79,22 +80,47 @@ pub struct NodeKeyParams { /// the chosen type. #[arg(long, value_name = "FILE")] pub node_key_file: Option, + + /// Forces key generation if node-key-file file does not exist. + /// + /// This is an unsafe feature for producation networks because the previous + /// node address would still live in the DHT for 36h and the node would suffer + /// from bad connectivity to other nodes untill the old record expires. + /// For minimal node downtime if no custom `node-key-file` argument is provided + /// the network-key is usually persisted accross nodes restarts, + /// in the `network` folder from directory provided in `--base-path` + /// + /// This is safe to be used the first time you run your node or if you already + /// lost your node keys or if you don't care about the above problem. + #[arg(long)] + pub unsafe_force_node_key_generation: bool, } impl NodeKeyParams { /// Create a `NodeKeyConfig` from the given `NodeKeyParams` in the context /// of an optional network config storage directory. - pub fn node_key(&self, net_config_dir: &PathBuf) -> error::Result { + pub fn node_key( + &self, + net_config_dir: &PathBuf, + role: Role, + is_dev: bool, + ) -> error::Result { Ok(match self.node_key_type { NodeKeyType::Ed25519 => { let secret = if let Some(node_key) = self.node_key.as_ref() { parse_ed25519_secret(node_key)? } else { - sc_network::config::Secret::File( - self.node_key_file - .clone() - .unwrap_or_else(|| net_config_dir.join(NODE_KEY_ED25519_FILE)), - ) + let key_path = self + .node_key_file + .clone() + .unwrap_or_else(|| net_config_dir.join(NODE_KEY_ED25519_FILE)); + if !self.unsafe_force_node_key_generation && + role.is_authority() && !is_dev && + !key_path.exists() + { + return Err(Error::NetworkKeyNotFound(key_path)) + } + sc_network::config::Secret::File(key_path) }; NodeKeyConfig::Ed25519(secret) From f32aca3649e7cda4b5b5abce5be14e4897c25752 Mon Sep 17 00:00:00 2001 From: Alexandru Gheorghe Date: Wed, 27 Mar 2024 11:43:56 +0200 Subject: [PATCH 02/21] Make tests happy Signed-off-by: Alexandru Gheorghe --- .../client/cli/src/params/node_key_params.rs | 66 ++++++++++++++----- 1 file changed, 49 insertions(+), 17 deletions(-) diff --git a/substrate/client/cli/src/params/node_key_params.rs b/substrate/client/cli/src/params/node_key_params.rs index 8d824180acad..e013d7e98f9f 100644 --- a/substrate/client/cli/src/params/node_key_params.rs +++ b/substrate/client/cli/src/params/node_key_params.rs @@ -148,7 +148,8 @@ mod tests { use super::*; use clap::ValueEnum; use libp2p_identity::ed25519; - use std::fs; + use std::fs::{self, File}; + use tempfile::TempDir; #[test] fn test_node_key_config_input() { @@ -162,8 +163,9 @@ mod tests { node_key_type, node_key: Some(format!("{:x}", H256::from_slice(sk.as_ref()))), node_key_file: None, + unsafe_force_node_key_generation: false, }; - params.node_key(net_config_dir).and_then(|c| match c { + params.node_key(net_config_dir, Role::Authority, false).and_then(|c| match c { NodeKeyConfig::Ed25519(sc_network::config::Secret::Input(ref ski)) if node_key_type == NodeKeyType::Ed25519 && &sk[..] == ski.as_ref() => Ok(()), @@ -182,10 +184,11 @@ mod tests { node_key_type: NodeKeyType::Ed25519, node_key: None, node_key_file: Some(file), + unsafe_force_node_key_generation: false, }; let node_key = params - .node_key(&PathBuf::from("not-used")) + .node_key(&PathBuf::from("not-used"), Role::Authority, false) .expect("Creates node key config") .into_keypair() .expect("Creates node key pair"); @@ -212,29 +215,58 @@ mod tests { #[test] fn test_node_key_config_default() { - fn with_def_params(f: F) -> error::Result<()> + fn with_def_params(f: F, unsafe_force_node_key_generation: bool) -> error::Result<()> where F: Fn(NodeKeyParams) -> error::Result<()>, { NodeKeyType::value_variants().iter().try_for_each(|t| { let node_key_type = *t; - f(NodeKeyParams { node_key_type, node_key: None, node_key_file: None }) + f(NodeKeyParams { + node_key_type, + node_key: None, + node_key_file: None, + unsafe_force_node_key_generation, + }) }) } - fn some_config_dir(net_config_dir: &PathBuf) -> error::Result<()> { - with_def_params(|params| { - let dir = PathBuf::from(net_config_dir.clone()); - let typ = params.node_key_type; - params.node_key(net_config_dir).and_then(move |c| match c { - NodeKeyConfig::Ed25519(sc_network::config::Secret::File(ref f)) - if typ == NodeKeyType::Ed25519 && f == &dir.join(NODE_KEY_ED25519_FILE) => - Ok(()), - _ => Err(error::Error::Input("Unexpected node key config".into())), - }) - }) + fn some_config_dir( + net_config_dir: &PathBuf, + unsafe_force_node_key_generation: bool, + role: Role, + is_dev: bool, + ) -> error::Result<()> { + with_def_params( + |params| { + let dir = PathBuf::from(net_config_dir.clone()); + let typ = params.node_key_type; + let role = role.clone(); + params.node_key(net_config_dir, role, is_dev).and_then(move |c| match c { + NodeKeyConfig::Ed25519(sc_network::config::Secret::File(ref f)) + if typ == NodeKeyType::Ed25519 && + f == &dir.join(NODE_KEY_ED25519_FILE) => + Ok(()), + _ => Err(error::Error::Input("Unexpected node key config".into())), + }) + }, + unsafe_force_node_key_generation, + ) } - assert!(some_config_dir(&PathBuf::from_str("x").unwrap()).is_ok()); + assert!(some_config_dir(&PathBuf::from_str("x").unwrap(), false, Role::Full, false).is_ok()); + assert!( + some_config_dir(&PathBuf::from_str("x").unwrap(), false, Role::Authority, true).is_ok() + ); + assert!( + some_config_dir(&PathBuf::from_str("x").unwrap(), true, Role::Authority, false).is_ok() + ); + assert!(matches!( + some_config_dir(&PathBuf::from_str("x").unwrap(), false, Role::Authority, false), + Err(Error::NetworkKeyNotFound(_)) + )); + + let tempdir = TempDir::new().unwrap(); + let _file = File::create(tempdir.path().join(NODE_KEY_ED25519_FILE)).unwrap(); + assert!(some_config_dir(&tempdir.path().into(), false, Role::Authority, false).is_ok()); } } From ec2c1ccc4e45ca303527a0255aa10f287dd4961e Mon Sep 17 00:00:00 2001 From: Alexandru Gheorghe Date: Wed, 27 Mar 2024 13:53:21 +0200 Subject: [PATCH 03/21] Fix tests Signed-off-by: Alexandru Gheorghe --- polkadot/tests/benchmark_block.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/polkadot/tests/benchmark_block.rs b/polkadot/tests/benchmark_block.rs index 99f95ef611a4..bc2680259850 100644 --- a/polkadot/tests/benchmark_block.rs +++ b/polkadot/tests/benchmark_block.rs @@ -58,7 +58,13 @@ async fn build_chain(runtime: &str, base_path: &Path) { let mut cmd = Command::new(cargo_bin("polkadot")) .stdout(process::Stdio::piped()) .stderr(process::Stdio::piped()) - .args(["--chain", runtime, "--force-authoring", "--alice"]) + .args([ + "--chain", + runtime, + "--force-authoring", + "--alice", + "--unsafe-force-node-key-generation", + ]) .arg("-d") .arg(base_path) .arg("--no-hardware-benchmarks") From ffcabfd07043f4488da46cb0344d1d66c12299f0 Mon Sep 17 00:00:00 2001 From: Alexandru Gheorghe Date: Wed, 27 Mar 2024 17:29:52 +0200 Subject: [PATCH 04/21] Fixup node key as well Signed-off-by: Alexandru Gheorghe --- polkadot/.rpm/polkadot.spec | 1 + .../packaging/deb-maintainer-scripts/postinst | 1 + .../cli/src/commands/generate_node_key.rs | 110 ++++++++++++++++-- .../cli/src/commands/inspect_node_key.rs | 44 ++++++- substrate/client/cli/src/commands/key.rs | 2 +- .../client/cli/src/params/node_key_params.rs | 2 +- 6 files changed, 149 insertions(+), 11 deletions(-) diff --git a/polkadot/.rpm/polkadot.spec b/polkadot/.rpm/polkadot.spec index 06fa0f57504d..4ded9559c5b8 100644 --- a/polkadot/.rpm/polkadot.spec +++ b/polkadot/.rpm/polkadot.spec @@ -37,6 +37,7 @@ getent passwd polkadot >/dev/null || \ if [ ! -e "$config_file" ]; then echo 'POLKADOT_CLI_ARGS=""' > /etc/default/polkadot fi +polkadot key generate-node-key --default-base-path exit 0 %clean diff --git a/polkadot/scripts/packaging/deb-maintainer-scripts/postinst b/polkadot/scripts/packaging/deb-maintainer-scripts/postinst index 3ac5cd04c376..64279926a0df 100644 --- a/polkadot/scripts/packaging/deb-maintainer-scripts/postinst +++ b/polkadot/scripts/packaging/deb-maintainer-scripts/postinst @@ -13,5 +13,6 @@ if [ "$action" = "configure" ]; then --ingroup polkadot polkadot if [ ! -e "$config_file" ]; then echo 'POLKADOT_CLI_ARGS=""' > /etc/default/polkadot + /usr/bin/polkadot key generate-node-key --default-base-path fi fi diff --git a/substrate/client/cli/src/commands/generate_node_key.rs b/substrate/client/cli/src/commands/generate_node_key.rs index 43851dc1af5c..89be8702f35f 100644 --- a/substrate/client/cli/src/commands/generate_node_key.rs +++ b/substrate/client/cli/src/commands/generate_node_key.rs @@ -17,9 +17,10 @@ //! Implementation of the `generate-node-key` subcommand -use crate::Error; +use crate::{Error, SubstrateCli, DEFAULT_NETWORK_CONFIG_PATH, NODE_KEY_ED25519_FILE}; use clap::Parser; use libp2p_identity::{ed25519, Keypair}; +use sc_service::BasePath; use std::{ fs, io::{self, Write}, @@ -36,18 +37,28 @@ use std::{ pub struct GenerateNodeKeyCmd { /// Name of file to save secret key to. /// If not given, the secret key is printed to stdout. - #[arg(long)] + #[arg(long, conflicts_with_all = ["base_path", "default_base_path"])] file: Option, /// The output is in raw binary format. /// If not given, the output is written as an hex encoded string. #[arg(long)] bin: bool, + + /// A directory where the key should be saved, if a key already + /// exists in the directory it won't be overwritten. + #[arg(long, conflicts_with_all = ["file", "default_base_path"])] + base_path: Option, + + /// Save the key in the default directory, if a key already + /// exists in the directory it won't be overwritten + #[arg(long, conflicts_with_all = ["base_path", "file"])] + default_base_path: bool, } impl GenerateNodeKeyCmd { /// Run the command - pub fn run(&self) -> Result<(), Error> { + pub fn run(&self, _cli: &C) -> Result<(), Error> { let keypair = ed25519::Keypair::generate(); let secret = keypair.secret(); @@ -58,9 +69,32 @@ impl GenerateNodeKeyCmd { array_bytes::bytes2hex("", secret).into_bytes() }; - match &self.file { - Some(file) => fs::write(file, file_data)?, - None => io::stdout().lock().write_all(&file_data)?, + match (&self.file, &self.base_path, self.default_base_path) { + (Some(file), None, false) => fs::write(file, file_data)?, + (None, Some(_), false) | (None, None, true) => { + let network_path = self + .base_path + .clone() + .unwrap_or_else(|| { + BasePath::from_project("", "", &C::executable_name()).path().to_path_buf() + }) + .join(DEFAULT_NETWORK_CONFIG_PATH); + + fs::create_dir_all(network_path.as_path())?; + let key_path = network_path.join(NODE_KEY_ED25519_FILE); + if key_path.exists() { + eprintln!("Skip generation, a key already exists in {:?}", key_path); + return Ok(()); + } else { + eprintln!("Generating key in {:?}", key_path); + fs::write(key_path, file_data)? + } + }, + (None, None, false) => io::stdout().lock().write_all(&file_data)?, + (_, _, _) => { + // This should not happen, arguments are marked as mutually exclusive. + return Err(Error::Input("Mutually exclusive arguments provided".into())); + }, } eprintln!("{}", Keypair::from(keypair).public().to_peer_id()); @@ -70,19 +104,79 @@ impl GenerateNodeKeyCmd { } #[cfg(test)] -mod tests { +pub mod tests { use super::*; + use sc_service::{ChainSpec, ChainType, GenericChainSpec, NoExtension}; use std::io::Read; use tempfile::Builder; + struct Cli; + + impl SubstrateCli for Cli { + fn impl_name() -> String { + "test".into() + } + + fn impl_version() -> String { + "2.0".into() + } + + fn description() -> String { + "test".into() + } + + fn support_url() -> String { + "test.test".into() + } + + fn copyright_start_year() -> i32 { + 2021 + } + + fn author() -> String { + "test".into() + } + + fn load_spec(&self, _: &str) -> std::result::Result, String> { + Ok(Box::new( + GenericChainSpec::<()>::builder(Default::default(), NoExtension::None) + .with_name("test") + .with_id("test_id") + .with_chain_type(ChainType::Development) + .with_genesis_config_patch(Default::default()) + .build(), + )) + } + } + #[test] fn generate_node_key() { let mut file = Builder::new().prefix("keyfile").tempfile().unwrap(); let file_path = file.path().display().to_string(); let generate = GenerateNodeKeyCmd::parse_from(&["generate-node-key", "--file", &file_path]); - assert!(generate.run().is_ok()); + assert!(generate.run(&Cli).is_ok()); let mut buf = String::new(); assert!(file.read_to_string(&mut buf).is_ok()); assert!(array_bytes::hex2bytes(&buf).is_ok()); } + + #[test] + fn generate_node_key_base_path() { + let base_dir = Builder::new().prefix("keyfile").tempdir().unwrap(); + let key_path = + base_dir.path().join(DEFAULT_NETWORK_CONFIG_PATH).join(NODE_KEY_ED25519_FILE); + let base_path = base_dir.path().display().to_string(); + let generate = + GenerateNodeKeyCmd::parse_from(&["generate-node-key", "--base-path", &base_path]); + assert!(generate.run(&Cli).is_ok()); + let buf = fs::read_to_string(key_path.as_path()).unwrap(); + assert!(array_bytes::hex2bytes(&buf).is_ok()); + + assert!(generate.run(&Cli).is_ok()); + let new_buf = fs::read_to_string(key_path).unwrap(); + assert_eq!( + array_bytes::hex2bytes(&new_buf).unwrap(), + array_bytes::hex2bytes(&buf).unwrap() + ); + } } diff --git a/substrate/client/cli/src/commands/inspect_node_key.rs b/substrate/client/cli/src/commands/inspect_node_key.rs index 6cf025a2d115..93b4f3eb98f0 100644 --- a/substrate/client/cli/src/commands/inspect_node_key.rs +++ b/substrate/client/cli/src/commands/inspect_node_key.rs @@ -79,7 +79,49 @@ impl InspectNodeKeyCmd { #[cfg(test)] mod tests { + use crate::SubstrateCli; + use super::{super::GenerateNodeKeyCmd, *}; + use sc_service::{ChainSpec, ChainType, GenericChainSpec, NoExtension}; + + struct Cli; + + impl SubstrateCli for Cli { + fn impl_name() -> String { + "test".into() + } + + fn impl_version() -> String { + "2.0".into() + } + + fn description() -> String { + "test".into() + } + + fn support_url() -> String { + "test.test".into() + } + + fn copyright_start_year() -> i32 { + 2021 + } + + fn author() -> String { + "test".into() + } + + fn load_spec(&self, _: &str) -> std::result::Result, String> { + Ok(Box::new( + GenericChainSpec::<()>::builder(Default::default(), NoExtension::None) + .with_name("test") + .with_id("test_id") + .with_chain_type(ChainType::Development) + .with_genesis_config_patch(Default::default()) + .build(), + )) + } + } #[test] fn inspect_node_key() { @@ -87,7 +129,7 @@ mod tests { let path = path.to_str().unwrap(); let cmd = GenerateNodeKeyCmd::parse_from(&["generate-node-key", "--file", path]); - assert!(cmd.run().is_ok()); + assert!(cmd.run(&Cli).is_ok()); let cmd = InspectNodeKeyCmd::parse_from(&["inspect-node-key", "--file", path]); assert!(cmd.run().is_ok()); diff --git a/substrate/client/cli/src/commands/key.rs b/substrate/client/cli/src/commands/key.rs index d49b7e4072c8..f36aa1fb9cdd 100644 --- a/substrate/client/cli/src/commands/key.rs +++ b/substrate/client/cli/src/commands/key.rs @@ -47,7 +47,7 @@ impl KeySubcommand { /// run the key subcommands pub fn run(&self, cli: &C) -> Result<(), Error> { match self { - KeySubcommand::GenerateNodeKey(cmd) => cmd.run(), + KeySubcommand::GenerateNodeKey(cmd) => cmd.run(cli), KeySubcommand::Generate(cmd) => cmd.run(), KeySubcommand::Inspect(cmd) => cmd.run(), KeySubcommand::Insert(cmd) => cmd.run(cli), diff --git a/substrate/client/cli/src/params/node_key_params.rs b/substrate/client/cli/src/params/node_key_params.rs index e013d7e98f9f..a5b9a6b3e4f9 100644 --- a/substrate/client/cli/src/params/node_key_params.rs +++ b/substrate/client/cli/src/params/node_key_params.rs @@ -27,7 +27,7 @@ use crate::{arg_enums::NodeKeyType, error, Error}; /// The file name of the node's Ed25519 secret key inside the chain-specific /// network config directory, if neither `--node-key` nor `--node-key-file` /// is specified in combination with `--node-key-type=ed25519`. -const NODE_KEY_ED25519_FILE: &str = "secret_ed25519"; +pub(crate) const NODE_KEY_ED25519_FILE: &str = "secret_ed25519"; /// Parameters used to create the `NodeKeyConfig`, which determines the keypair /// used for libp2p networking. From f17fa1938ce7cd978f0e0f041258053184736615 Mon Sep 17 00:00:00 2001 From: Alexandru Gheorghe Date: Wed, 27 Mar 2024 17:57:45 +0200 Subject: [PATCH 05/21] Make clippy happy Signed-off-by: Alexandru Gheorghe --- substrate/bin/utils/subkey/src/lib.rs | 37 ++++++++++++++++++++++++--- 1 file changed, 34 insertions(+), 3 deletions(-) diff --git a/substrate/bin/utils/subkey/src/lib.rs b/substrate/bin/utils/subkey/src/lib.rs index 33f28ef46a5e..e54c4aa3980a 100644 --- a/substrate/bin/utils/subkey/src/lib.rs +++ b/substrate/bin/utils/subkey/src/lib.rs @@ -310,8 +310,8 @@ use clap::Parser; use sc_cli::{ - Error, GenerateCmd, GenerateNodeKeyCmd, InspectKeyCmd, InspectNodeKeyCmd, SignCmd, VanityCmd, - VerifyCmd, + Error, GenerateCmd, GenerateNodeKeyCmd, InspectKeyCmd, InspectNodeKeyCmd, SignCmd, + SubstrateCli, VanityCmd, VerifyCmd, }; #[derive(Debug, Parser)] @@ -344,11 +344,42 @@ pub enum Subkey { /// Verify a signature for a message, provided on STDIN, with a given (public or secret) key. Verify(VerifyCmd), } +struct SubkeyCli; +// Implemented because GenerateNodeKeyCmd requires an implementation of SubstrateCli +impl SubstrateCli for SubkeyCli { + fn impl_name() -> String { + "subkey".into() + } + + fn impl_version() -> String { + "1.0".into() + } + + fn description() -> String { + "Utility for generating and restoring with Substrate keys".into() + } + + fn author() -> String { + "Parity Team ".into() + } + + fn support_url() -> String { + "https://github.com/paritytech/polkadot-sdk/issues/new".into() + } + + fn copyright_start_year() -> i32 { + 2024 + } + + fn load_spec(&self, _id: &str) -> std::result::Result, String> { + Err("Unsupported".into()) + } +} /// Run the subkey command, given the appropriate runtime. pub fn run() -> Result<(), Error> { match Subkey::parse() { - Subkey::GenerateNodeKey(cmd) => cmd.run(), + Subkey::GenerateNodeKey(cmd) => cmd.run(&SubkeyCli), Subkey::Generate(cmd) => cmd.run(), Subkey::Inspect(cmd) => cmd.run(), Subkey::InspectNodeKey(cmd) => cmd.run(), From 41a37f7c246c40cdd2e4142dcffeb785ff4c8545 Mon Sep 17 00:00:00 2001 From: Alexandru Gheorghe Date: Wed, 27 Mar 2024 17:59:53 +0200 Subject: [PATCH 06/21] Fix typo Signed-off-by: Alexandru Gheorghe --- substrate/client/cli/src/error.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/client/cli/src/error.rs b/substrate/client/cli/src/error.rs index 5bf726c335fd..079972dd549e 100644 --- a/substrate/client/cli/src/error.rs +++ b/substrate/client/cli/src/error.rs @@ -87,7 +87,7 @@ pub enum Error { \n Because of it your node might suffer from not being properly connected to other nodes for validation purposes. \n If it is the first time running your node you could use one of the following methods. \n 1. Pass --unsafe-force-node-key-generation and make sure you remove it for subsequent node restarts - \n 2. Separetly generate the key with: polkadot key generate-node-key --file " + \n 2. Separately generate the key with: polkadot key generate-node-key --file " )] NetworkKeyNotFound(PathBuf), } From 6b5d8ff6fe6bb5571977976e7762fd75882e6b9c Mon Sep 17 00:00:00 2001 From: Alexandru Gheorghe <49718502+alexggh@users.noreply.github.com> Date: Wed, 27 Mar 2024 18:11:42 +0200 Subject: [PATCH 07/21] Update substrate/client/cli/src/commands/generate_node_key.rs Co-authored-by: Dmitry Markin --- substrate/client/cli/src/commands/generate_node_key.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/substrate/client/cli/src/commands/generate_node_key.rs b/substrate/client/cli/src/commands/generate_node_key.rs index 89be8702f35f..38729e84ddb8 100644 --- a/substrate/client/cli/src/commands/generate_node_key.rs +++ b/substrate/client/cli/src/commands/generate_node_key.rs @@ -45,8 +45,8 @@ pub struct GenerateNodeKeyCmd { #[arg(long)] bin: bool, - /// A directory where the key should be saved, if a key already - /// exists in the directory it won't be overwritten. + /// A directory where the key should be saved. If a key already + /// exists in the directory, it won't be overwritten. #[arg(long, conflicts_with_all = ["file", "default_base_path"])] base_path: Option, From 4771ad50181b7d7453e70892234c835a13507a00 Mon Sep 17 00:00:00 2001 From: Alexandru Gheorghe <49718502+alexggh@users.noreply.github.com> Date: Wed, 27 Mar 2024 18:11:47 +0200 Subject: [PATCH 08/21] Update substrate/client/cli/src/commands/generate_node_key.rs Co-authored-by: Dmitry Markin --- substrate/client/cli/src/commands/generate_node_key.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/substrate/client/cli/src/commands/generate_node_key.rs b/substrate/client/cli/src/commands/generate_node_key.rs index 38729e84ddb8..2e0ab7b5b049 100644 --- a/substrate/client/cli/src/commands/generate_node_key.rs +++ b/substrate/client/cli/src/commands/generate_node_key.rs @@ -50,8 +50,8 @@ pub struct GenerateNodeKeyCmd { #[arg(long, conflicts_with_all = ["file", "default_base_path"])] base_path: Option, - /// Save the key in the default directory, if a key already - /// exists in the directory it won't be overwritten + /// Save the key in the default directory. If a key already + /// exists in the directory, it won't be overwritten. #[arg(long, conflicts_with_all = ["base_path", "file"])] default_base_path: bool, } From 5a581bafe9ed246c02c3c128723d0a72dfa02e1c Mon Sep 17 00:00:00 2001 From: Alexandru Gheorghe <49718502+alexggh@users.noreply.github.com> Date: Thu, 28 Mar 2024 11:04:58 +0200 Subject: [PATCH 09/21] Update polkadot/.rpm/polkadot.spec Co-authored-by: s0me0ne-unkn0wn <48632512+s0me0ne-unkn0wn@users.noreply.github.com> --- polkadot/.rpm/polkadot.spec | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/polkadot/.rpm/polkadot.spec b/polkadot/.rpm/polkadot.spec index 4ded9559c5b8..3bf3c4e94d62 100644 --- a/polkadot/.rpm/polkadot.spec +++ b/polkadot/.rpm/polkadot.spec @@ -37,7 +37,9 @@ getent passwd polkadot >/dev/null || \ if [ ! -e "$config_file" ]; then echo 'POLKADOT_CLI_ARGS=""' > /etc/default/polkadot fi -polkadot key generate-node-key --default-base-path +if [ $1 -eq 1 ]; then + polkadot key generate-node-key --default-base-path +fi exit 0 %clean From f7b25ed1f196d67e015149e2ab64fb08938e543b Mon Sep 17 00:00:00 2001 From: Alexandru Gheorghe <49718502+alexggh@users.noreply.github.com> Date: Thu, 28 Mar 2024 11:05:15 +0200 Subject: [PATCH 10/21] Update polkadot/scripts/packaging/deb-maintainer-scripts/postinst Co-authored-by: s0me0ne-unkn0wn <48632512+s0me0ne-unkn0wn@users.noreply.github.com> --- polkadot/scripts/packaging/deb-maintainer-scripts/postinst | 2 ++ 1 file changed, 2 insertions(+) diff --git a/polkadot/scripts/packaging/deb-maintainer-scripts/postinst b/polkadot/scripts/packaging/deb-maintainer-scripts/postinst index 64279926a0df..a8a645154ebf 100644 --- a/polkadot/scripts/packaging/deb-maintainer-scripts/postinst +++ b/polkadot/scripts/packaging/deb-maintainer-scripts/postinst @@ -13,6 +13,8 @@ if [ "$action" = "configure" ]; then --ingroup polkadot polkadot if [ ! -e "$config_file" ]; then echo 'POLKADOT_CLI_ARGS=""' > /etc/default/polkadot + fi + if [ -z "$2" ]; then /usr/bin/polkadot key generate-node-key --default-base-path fi fi From f0c065354bf17868eee2019ebf65f186a1b7885f Mon Sep 17 00:00:00 2001 From: Alexandru Gheorghe Date: Thu, 28 Mar 2024 15:21:43 +0200 Subject: [PATCH 11/21] Generate key with PreExecStart Signed-off-by: Alexandru Gheorghe --- polkadot/.rpm/polkadot.spec | 3 --- polkadot/scripts/packaging/deb-maintainer-scripts/postinst | 3 --- polkadot/scripts/packaging/polkadot.service | 1 + 3 files changed, 1 insertion(+), 6 deletions(-) diff --git a/polkadot/.rpm/polkadot.spec b/polkadot/.rpm/polkadot.spec index 3bf3c4e94d62..06fa0f57504d 100644 --- a/polkadot/.rpm/polkadot.spec +++ b/polkadot/.rpm/polkadot.spec @@ -37,9 +37,6 @@ getent passwd polkadot >/dev/null || \ if [ ! -e "$config_file" ]; then echo 'POLKADOT_CLI_ARGS=""' > /etc/default/polkadot fi -if [ $1 -eq 1 ]; then - polkadot key generate-node-key --default-base-path -fi exit 0 %clean diff --git a/polkadot/scripts/packaging/deb-maintainer-scripts/postinst b/polkadot/scripts/packaging/deb-maintainer-scripts/postinst index a8a645154ebf..3ac5cd04c376 100644 --- a/polkadot/scripts/packaging/deb-maintainer-scripts/postinst +++ b/polkadot/scripts/packaging/deb-maintainer-scripts/postinst @@ -14,7 +14,4 @@ if [ "$action" = "configure" ]; then if [ ! -e "$config_file" ]; then echo 'POLKADOT_CLI_ARGS=""' > /etc/default/polkadot fi - if [ -z "$2" ]; then - /usr/bin/polkadot key generate-node-key --default-base-path - fi fi diff --git a/polkadot/scripts/packaging/polkadot.service b/polkadot/scripts/packaging/polkadot.service index 7fb549c97f8b..354cc52f23f1 100644 --- a/polkadot/scripts/packaging/polkadot.service +++ b/polkadot/scripts/packaging/polkadot.service @@ -5,6 +5,7 @@ Documentation=https://github.com/paritytech/polkadot [Service] EnvironmentFile=-/etc/default/polkadot +ExecStartPre=/usr/bin/polkadot key generate-node-key --default-base-path ExecStart=/usr/bin/polkadot $POLKADOT_CLI_ARGS User=polkadot Group=polkadot From 361255ebdf6b7a54d4492a9ae170a1c750da3f2b Mon Sep 17 00:00:00 2001 From: Alexandru Gheorghe Date: Thu, 28 Mar 2024 16:39:30 +0200 Subject: [PATCH 12/21] Address review feedback Signed-off-by: Alexandru Gheorghe --- .../cli/src/commands/generate_node_key.rs | 35 ++++++++++++------ substrate/client/cli/src/config.rs | 37 ++++++++++++++++--- 2 files changed, 55 insertions(+), 17 deletions(-) diff --git a/substrate/client/cli/src/commands/generate_node_key.rs b/substrate/client/cli/src/commands/generate_node_key.rs index 2e0ab7b5b049..2812ab12ada2 100644 --- a/substrate/client/cli/src/commands/generate_node_key.rs +++ b/substrate/client/cli/src/commands/generate_node_key.rs @@ -17,7 +17,7 @@ //! Implementation of the `generate-node-key` subcommand -use crate::{Error, SubstrateCli, DEFAULT_NETWORK_CONFIG_PATH, NODE_KEY_ED25519_FILE}; +use crate::{build_network_key_dir_or_default, Error, SubstrateCli, NODE_KEY_ED25519_FILE}; use clap::Parser; use libp2p_identity::{ed25519, Keypair}; use sc_service::BasePath; @@ -37,7 +37,7 @@ use std::{ pub struct GenerateNodeKeyCmd { /// Name of file to save secret key to. /// If not given, the secret key is printed to stdout. - #[arg(long, conflicts_with_all = ["base_path", "default_base_path"])] + #[arg(long, conflicts_with_all = ["base_path", "default_base_path", "chain"])] file: Option, /// The output is in raw binary format. @@ -45,6 +45,11 @@ pub struct GenerateNodeKeyCmd { #[arg(long)] bin: bool, + /// Specify the chain specification. + /// + /// It can be any of the predefined chains like dev, local, staging, polkadot, kusama. + #[arg(long, value_name = "CHAIN_SPEC")] + pub chain: Option, /// A directory where the key should be saved. If a key already /// exists in the directory, it won't be overwritten. #[arg(long, conflicts_with_all = ["file", "default_base_path"])] @@ -58,7 +63,7 @@ pub struct GenerateNodeKeyCmd { impl GenerateNodeKeyCmd { /// Run the command - pub fn run(&self, _cli: &C) -> Result<(), Error> { + pub fn run(&self, cli: &C) -> Result<(), Error> { let keypair = ed25519::Keypair::generate(); let secret = keypair.secret(); @@ -72,15 +77,16 @@ impl GenerateNodeKeyCmd { match (&self.file, &self.base_path, self.default_base_path) { (Some(file), None, false) => fs::write(file, file_data)?, (None, Some(_), false) | (None, None, true) => { - let network_path = self - .base_path - .clone() - .unwrap_or_else(|| { - BasePath::from_project("", "", &C::executable_name()).path().to_path_buf() - }) - .join(DEFAULT_NETWORK_CONFIG_PATH); + let chain_spec = cli.load_spec(self.chain.as_deref().unwrap_or(""))?; + + let network_path = build_network_key_dir_or_default( + self.base_path.clone().map(BasePath::new), + &chain_spec, + cli, + ); fs::create_dir_all(network_path.as_path())?; + let key_path = network_path.join(NODE_KEY_ED25519_FILE); if key_path.exists() { eprintln!("Skip generation, a key already exists in {:?}", key_path); @@ -105,6 +111,8 @@ impl GenerateNodeKeyCmd { #[cfg(test)] pub mod tests { + use crate::DEFAULT_NETWORK_CONFIG_PATH; + use super::*; use sc_service::{ChainSpec, ChainType, GenericChainSpec, NoExtension}; use std::io::Read; @@ -163,8 +171,11 @@ pub mod tests { #[test] fn generate_node_key_base_path() { let base_dir = Builder::new().prefix("keyfile").tempdir().unwrap(); - let key_path = - base_dir.path().join(DEFAULT_NETWORK_CONFIG_PATH).join(NODE_KEY_ED25519_FILE); + let key_path = base_dir + .path() + .join("chains/test_id/") + .join(DEFAULT_NETWORK_CONFIG_PATH) + .join(NODE_KEY_ED25519_FILE); let base_path = base_dir.path().display().to_string(); let generate = GenerateNodeKeyCmd::parse_from(&["generate-node-key", "--base-path", &base_path]); diff --git a/substrate/client/cli/src/config.rs b/substrate/client/cli/src/config.rs index 411d567bdd23..6b9dc94af3a7 100644 --- a/substrate/client/cli/src/config.rs +++ b/substrate/client/cli/src/config.rs @@ -465,11 +465,9 @@ pub trait CliConfiguration: Sized { let is_dev = self.is_dev()?; let chain_id = self.chain_id(is_dev)?; let chain_spec = cli.load_spec(&chain_id)?; - let base_path = self - .base_path()? - .unwrap_or_else(|| BasePath::from_project("", "", &C::executable_name())); - let config_dir = base_path.config_dir(chain_spec.id()); - let net_config_dir = config_dir.join(DEFAULT_NETWORK_CONFIG_PATH); + let base_path = base_path_or_default(self.base_path()?, cli); + let config_dir = build_config_dir(&base_path, &chain_spec); + let net_config_dir = build_net_config_dir(&config_dir); let client_id = C::client_id(); let database_cache_size = self.database_cache_size()?.unwrap_or(1024); let database = self.database()?.unwrap_or( @@ -667,3 +665,32 @@ pub fn generate_node_name() -> String { } } } + +/// Returns the value of `base_path` or the default_path if it is None +pub(crate) fn base_path_or_default( + base_path: Option, + _cli: &C, +) -> BasePath { + base_path.unwrap_or_else(|| BasePath::from_project("", "", &C::executable_name())) +} + +/// Returns the default path for configuration directory based on the chain_spec +pub(crate) fn build_config_dir(base_path: &BasePath, chain_spec: &Box) -> PathBuf { + base_path.config_dir(chain_spec.id()) +} + +/// Returns the default path for the network configuration inside the configuration dir +pub(crate) fn build_net_config_dir(config_dir: &PathBuf) -> PathBuf { + config_dir.join(DEFAULT_NETWORK_CONFIG_PATH) +} + +/// Returns the default path for the network directory starting from the provided base_path +/// or from the default base_path. +pub(crate) fn build_network_key_dir_or_default( + base_path: Option, + chain_spec: &Box, + cli: &C, +) -> PathBuf { + let config_dir = build_config_dir(&base_path_or_default(base_path, cli), chain_spec); + build_net_config_dir(&config_dir) +} From f14821bee89678400dc23336ef20df92f04b9d44 Mon Sep 17 00:00:00 2001 From: Alexandru Gheorghe Date: Fri, 29 Mar 2024 13:14:32 +0200 Subject: [PATCH 13/21] Trivial updates Signed-off-by: Alexandru Gheorghe --- substrate/client/cli/src/error.rs | 9 ++++++--- substrate/client/cli/src/params/node_key_params.rs | 4 ++-- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/substrate/client/cli/src/error.rs b/substrate/client/cli/src/error.rs index 079972dd549e..b4ffa018b092 100644 --- a/substrate/client/cli/src/error.rs +++ b/substrate/client/cli/src/error.rs @@ -85,9 +85,12 @@ pub enum Error { "Starting an authorithy without network key in {0}. \n This is not a safe operation because the old identity still lives in the dht for 36 hours. \n Because of it your node might suffer from not being properly connected to other nodes for validation purposes. - \n If it is the first time running your node you could use one of the following methods. - \n 1. Pass --unsafe-force-node-key-generation and make sure you remove it for subsequent node restarts - \n 2. Separately generate the key with: polkadot key generate-node-key --file " + \n If it is the first time running your node you could use one of the following methods: + \n 1. [Preferred] Separately generate the key with: polkadot key generate-node-key --default-base-path + \n 2. [Preferred] Separately generate the key with: polkadot key generate-node-key --base-path + \n 3. [Preferred] Separately generate the key with: polkadot key generate-node-key --file + \n 4. [Unsafe] Pass --unsafe-force-node-key-generation and make sure you remove it for subsequent node restarts" + )] NetworkKeyNotFound(PathBuf), } diff --git a/substrate/client/cli/src/params/node_key_params.rs b/substrate/client/cli/src/params/node_key_params.rs index a5b9a6b3e4f9..6fbc84916551 100644 --- a/substrate/client/cli/src/params/node_key_params.rs +++ b/substrate/client/cli/src/params/node_key_params.rs @@ -90,8 +90,8 @@ pub struct NodeKeyParams { /// the network-key is usually persisted accross nodes restarts, /// in the `network` folder from directory provided in `--base-path` /// - /// This is safe to be used the first time you run your node or if you already - /// lost your node keys or if you don't care about the above problem. + /// Warning!! If you ever run the node with this argument, make sure + /// you remove it for the subsequent restarts. #[arg(long)] pub unsafe_force_node_key_generation: bool, } From 4b96b9f9030523b4c9adcbf894273e4aebd27cb6 Mon Sep 17 00:00:00 2001 From: Alexandru Gheorghe Date: Fri, 29 Mar 2024 13:18:27 +0200 Subject: [PATCH 14/21] Remove PreStartExec Signed-off-by: Alexandru Gheorghe --- polkadot/scripts/packaging/polkadot.service | 1 - 1 file changed, 1 deletion(-) diff --git a/polkadot/scripts/packaging/polkadot.service b/polkadot/scripts/packaging/polkadot.service index 354cc52f23f1..7fb549c97f8b 100644 --- a/polkadot/scripts/packaging/polkadot.service +++ b/polkadot/scripts/packaging/polkadot.service @@ -5,7 +5,6 @@ Documentation=https://github.com/paritytech/polkadot [Service] EnvironmentFile=-/etc/default/polkadot -ExecStartPre=/usr/bin/polkadot key generate-node-key --default-base-path ExecStart=/usr/bin/polkadot $POLKADOT_CLI_ARGS User=polkadot Group=polkadot From 35291ca5706adcc251555a16ac5bef7775766ccb Mon Sep 17 00:00:00 2001 From: Alexandru Gheorghe Date: Thu, 4 Apr 2024 10:18:29 +0300 Subject: [PATCH 15/21] Address review feedback Signed-off-by: Alexandru Gheorghe --- substrate/bin/utils/subkey/src/lib.rs | 37 +----------- .../cli/src/commands/generate_node_key.rs | 58 +++---------------- .../cli/src/commands/inspect_node_key.rs | 2 +- substrate/client/cli/src/commands/key.rs | 5 +- substrate/client/cli/src/config.rs | 23 ++++---- substrate/client/cli/src/error.rs | 12 ++-- .../client/cli/src/params/node_key_params.rs | 7 ++- 7 files changed, 39 insertions(+), 105 deletions(-) diff --git a/substrate/bin/utils/subkey/src/lib.rs b/substrate/bin/utils/subkey/src/lib.rs index e54c4aa3980a..74fcd03fe4da 100644 --- a/substrate/bin/utils/subkey/src/lib.rs +++ b/substrate/bin/utils/subkey/src/lib.rs @@ -310,8 +310,8 @@ use clap::Parser; use sc_cli::{ - Error, GenerateCmd, GenerateNodeKeyCmd, InspectKeyCmd, InspectNodeKeyCmd, SignCmd, - SubstrateCli, VanityCmd, VerifyCmd, + Error, GenerateCmd, GenerateNodeKeyCmd, InspectKeyCmd, InspectNodeKeyCmd, SignCmd, VanityCmd, + VerifyCmd, }; #[derive(Debug, Parser)] @@ -344,42 +344,11 @@ pub enum Subkey { /// Verify a signature for a message, provided on STDIN, with a given (public or secret) key. Verify(VerifyCmd), } -struct SubkeyCli; -// Implemented because GenerateNodeKeyCmd requires an implementation of SubstrateCli -impl SubstrateCli for SubkeyCli { - fn impl_name() -> String { - "subkey".into() - } - - fn impl_version() -> String { - "1.0".into() - } - - fn description() -> String { - "Utility for generating and restoring with Substrate keys".into() - } - - fn author() -> String { - "Parity Team ".into() - } - - fn support_url() -> String { - "https://github.com/paritytech/polkadot-sdk/issues/new".into() - } - - fn copyright_start_year() -> i32 { - 2024 - } - - fn load_spec(&self, _id: &str) -> std::result::Result, String> { - Err("Unsupported".into()) - } -} /// Run the subkey command, given the appropriate runtime. pub fn run() -> Result<(), Error> { match Subkey::parse() { - Subkey::GenerateNodeKey(cmd) => cmd.run(&SubkeyCli), + Subkey::GenerateNodeKey(cmd) => cmd.run("", &String::default()), Subkey::Generate(cmd) => cmd.run(), Subkey::Inspect(cmd) => cmd.run(), Subkey::InspectNodeKey(cmd) => cmd.run(), diff --git a/substrate/client/cli/src/commands/generate_node_key.rs b/substrate/client/cli/src/commands/generate_node_key.rs index 2812ab12ada2..72ae3595af66 100644 --- a/substrate/client/cli/src/commands/generate_node_key.rs +++ b/substrate/client/cli/src/commands/generate_node_key.rs @@ -17,7 +17,7 @@ //! Implementation of the `generate-node-key` subcommand -use crate::{build_network_key_dir_or_default, Error, SubstrateCli, NODE_KEY_ED25519_FILE}; +use crate::{build_network_key_dir_or_default, Error, NODE_KEY_ED25519_FILE}; use clap::Parser; use libp2p_identity::{ed25519, Keypair}; use sc_service::BasePath; @@ -63,7 +63,7 @@ pub struct GenerateNodeKeyCmd { impl GenerateNodeKeyCmd { /// Run the command - pub fn run(&self, cli: &C) -> Result<(), Error> { + pub fn run(&self, chain_spec_id: &str, executable_name: &String) -> Result<(), Error> { let keypair = ed25519::Keypair::generate(); let secret = keypair.secret(); @@ -77,12 +77,10 @@ impl GenerateNodeKeyCmd { match (&self.file, &self.base_path, self.default_base_path) { (Some(file), None, false) => fs::write(file, file_data)?, (None, Some(_), false) | (None, None, true) => { - let chain_spec = cli.load_spec(self.chain.as_deref().unwrap_or(""))?; - let network_path = build_network_key_dir_or_default( self.base_path.clone().map(BasePath::new), - &chain_spec, - cli, + chain_spec_id, + executable_name, ); fs::create_dir_all(network_path.as_path())?; @@ -90,7 +88,7 @@ impl GenerateNodeKeyCmd { let key_path = network_path.join(NODE_KEY_ED25519_FILE); if key_path.exists() { eprintln!("Skip generation, a key already exists in {:?}", key_path); - return Ok(()); + return Err(Error::KeyAlreadyExistsInPath(key_path)); } else { eprintln!("Generating key in {:?}", key_path); fs::write(key_path, file_data)? @@ -114,55 +112,15 @@ pub mod tests { use crate::DEFAULT_NETWORK_CONFIG_PATH; use super::*; - use sc_service::{ChainSpec, ChainType, GenericChainSpec, NoExtension}; use std::io::Read; use tempfile::Builder; - struct Cli; - - impl SubstrateCli for Cli { - fn impl_name() -> String { - "test".into() - } - - fn impl_version() -> String { - "2.0".into() - } - - fn description() -> String { - "test".into() - } - - fn support_url() -> String { - "test.test".into() - } - - fn copyright_start_year() -> i32 { - 2021 - } - - fn author() -> String { - "test".into() - } - - fn load_spec(&self, _: &str) -> std::result::Result, String> { - Ok(Box::new( - GenericChainSpec::<()>::builder(Default::default(), NoExtension::None) - .with_name("test") - .with_id("test_id") - .with_chain_type(ChainType::Development) - .with_genesis_config_patch(Default::default()) - .build(), - )) - } - } - #[test] fn generate_node_key() { let mut file = Builder::new().prefix("keyfile").tempfile().unwrap(); let file_path = file.path().display().to_string(); let generate = GenerateNodeKeyCmd::parse_from(&["generate-node-key", "--file", &file_path]); - assert!(generate.run(&Cli).is_ok()); + assert!(generate.run("test", &String::from("test")).is_ok()); let mut buf = String::new(); assert!(file.read_to_string(&mut buf).is_ok()); assert!(array_bytes::hex2bytes(&buf).is_ok()); @@ -179,11 +137,11 @@ pub mod tests { let base_path = base_dir.path().display().to_string(); let generate = GenerateNodeKeyCmd::parse_from(&["generate-node-key", "--base-path", &base_path]); - assert!(generate.run(&Cli).is_ok()); + assert!(generate.run("test", &String::from("test")).is_ok()); let buf = fs::read_to_string(key_path.as_path()).unwrap(); assert!(array_bytes::hex2bytes(&buf).is_ok()); - assert!(generate.run(&Cli).is_ok()); + assert!(generate.run("test", &String::from("test")).is_ok()); let new_buf = fs::read_to_string(key_path).unwrap(); assert_eq!( array_bytes::hex2bytes(&new_buf).unwrap(), diff --git a/substrate/client/cli/src/commands/inspect_node_key.rs b/substrate/client/cli/src/commands/inspect_node_key.rs index 93b4f3eb98f0..42fecf27d245 100644 --- a/substrate/client/cli/src/commands/inspect_node_key.rs +++ b/substrate/client/cli/src/commands/inspect_node_key.rs @@ -129,7 +129,7 @@ mod tests { let path = path.to_str().unwrap(); let cmd = GenerateNodeKeyCmd::parse_from(&["generate-node-key", "--file", path]); - assert!(cmd.run(&Cli).is_ok()); + assert!(cmd.run("test", &String::from("test")).is_ok()); let cmd = InspectNodeKeyCmd::parse_from(&["inspect-node-key", "--file", path]); assert!(cmd.run().is_ok()); diff --git a/substrate/client/cli/src/commands/key.rs b/substrate/client/cli/src/commands/key.rs index f36aa1fb9cdd..52747b404622 100644 --- a/substrate/client/cli/src/commands/key.rs +++ b/substrate/client/cli/src/commands/key.rs @@ -47,7 +47,10 @@ impl KeySubcommand { /// run the key subcommands pub fn run(&self, cli: &C) -> Result<(), Error> { match self { - KeySubcommand::GenerateNodeKey(cmd) => cmd.run(cli), + KeySubcommand::GenerateNodeKey(cmd) => { + let chain_spec = cli.load_spec(cmd.chain.as_deref().unwrap_or(""))?; + cmd.run(chain_spec.id(), &C::executable_name()) + }, KeySubcommand::Generate(cmd) => cmd.run(), KeySubcommand::Inspect(cmd) => cmd.run(), KeySubcommand::Insert(cmd) => cmd.run(cli), diff --git a/substrate/client/cli/src/config.rs b/substrate/client/cli/src/config.rs index 6b9dc94af3a7..70a4885e5eef 100644 --- a/substrate/client/cli/src/config.rs +++ b/substrate/client/cli/src/config.rs @@ -465,8 +465,8 @@ pub trait CliConfiguration: Sized { let is_dev = self.is_dev()?; let chain_id = self.chain_id(is_dev)?; let chain_spec = cli.load_spec(&chain_id)?; - let base_path = base_path_or_default(self.base_path()?, cli); - let config_dir = build_config_dir(&base_path, &chain_spec); + let base_path = base_path_or_default(self.base_path()?, &C::executable_name()); + let config_dir = build_config_dir(&base_path, chain_spec.id()); let net_config_dir = build_net_config_dir(&config_dir); let client_id = C::client_id(); let database_cache_size = self.database_cache_size()?.unwrap_or(1024); @@ -667,16 +667,16 @@ pub fn generate_node_name() -> String { } /// Returns the value of `base_path` or the default_path if it is None -pub(crate) fn base_path_or_default( +pub(crate) fn base_path_or_default( base_path: Option, - _cli: &C, + executable_name: &String, ) -> BasePath { - base_path.unwrap_or_else(|| BasePath::from_project("", "", &C::executable_name())) + base_path.unwrap_or_else(|| BasePath::from_project("", "", executable_name)) } /// Returns the default path for configuration directory based on the chain_spec -pub(crate) fn build_config_dir(base_path: &BasePath, chain_spec: &Box) -> PathBuf { - base_path.config_dir(chain_spec.id()) +pub(crate) fn build_config_dir(base_path: &BasePath, chain_spec_id: &str) -> PathBuf { + base_path.config_dir(chain_spec_id) } /// Returns the default path for the network configuration inside the configuration dir @@ -686,11 +686,12 @@ pub(crate) fn build_net_config_dir(config_dir: &PathBuf) -> PathBuf { /// Returns the default path for the network directory starting from the provided base_path /// or from the default base_path. -pub(crate) fn build_network_key_dir_or_default( +pub(crate) fn build_network_key_dir_or_default( base_path: Option, - chain_spec: &Box, - cli: &C, + chain_spec_id: &str, + executable_name: &String, ) -> PathBuf { - let config_dir = build_config_dir(&base_path_or_default(base_path, cli), chain_spec); + let config_dir = + build_config_dir(&base_path_or_default(base_path, executable_name), chain_spec_id); build_net_config_dir(&config_dir) } diff --git a/substrate/client/cli/src/error.rs b/substrate/client/cli/src/error.rs index b4ffa018b092..8ea5cff50cca 100644 --- a/substrate/client/cli/src/error.rs +++ b/substrate/client/cli/src/error.rs @@ -83,16 +83,18 @@ pub enum Error { #[error( "Starting an authorithy without network key in {0}. - \n This is not a safe operation because the old identity still lives in the dht for 36 hours. - \n Because of it your node might suffer from not being properly connected to other nodes for validation purposes. + \n This is not a safe operation because other authorities in the network may depend on your node having a stable identity. + \n Otherwise these other authorities may not being able to reach you. \n If it is the first time running your node you could use one of the following methods: - \n 1. [Preferred] Separately generate the key with: polkadot key generate-node-key --default-base-path - \n 2. [Preferred] Separately generate the key with: polkadot key generate-node-key --base-path - \n 3. [Preferred] Separately generate the key with: polkadot key generate-node-key --file + \n 1. [Preferred] Separately generate the key with: key generate-node-key --base-path + \n 2. [Preferred] Separately generate the key with: key generate-node-key --file + \n 3. [Preferred] Separately generate the key with: key generate-node-key --default-base-path \n 4. [Unsafe] Pass --unsafe-force-node-key-generation and make sure you remove it for subsequent node restarts" )] NetworkKeyNotFound(PathBuf), + #[error("A network key already exists in path {0}")] + KeyAlreadyExistsInPath(PathBuf), } impl From<&str> for Error { diff --git a/substrate/client/cli/src/params/node_key_params.rs b/substrate/client/cli/src/params/node_key_params.rs index 6fbc84916551..7058af19f1d4 100644 --- a/substrate/client/cli/src/params/node_key_params.rs +++ b/substrate/client/cli/src/params/node_key_params.rs @@ -83,9 +83,10 @@ pub struct NodeKeyParams { /// Forces key generation if node-key-file file does not exist. /// - /// This is an unsafe feature for producation networks because the previous - /// node address would still live in the DHT for 36h and the node would suffer - /// from bad connectivity to other nodes untill the old record expires. + /// This is an unsafe feature for production networks, because as an active authority + /// other authorities may depend on your node having a stable identity and they might + /// not being able to reach you if your identity changes after entering the active set. + /// /// For minimal node downtime if no custom `node-key-file` argument is provided /// the network-key is usually persisted accross nodes restarts, /// in the `network` folder from directory provided in `--base-path` From c4b965826e9ab6892bc4c5a11e6bc9f0e9ae2076 Mon Sep 17 00:00:00 2001 From: Alexandru Gheorghe Date: Thu, 4 Apr 2024 12:32:25 +0300 Subject: [PATCH 16/21] Fixup unittest Signed-off-by: Alexandru Gheorghe --- substrate/client/cli/src/commands/generate_node_key.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/substrate/client/cli/src/commands/generate_node_key.rs b/substrate/client/cli/src/commands/generate_node_key.rs index 72ae3595af66..f2f48acba369 100644 --- a/substrate/client/cli/src/commands/generate_node_key.rs +++ b/substrate/client/cli/src/commands/generate_node_key.rs @@ -137,11 +137,11 @@ pub mod tests { let base_path = base_dir.path().display().to_string(); let generate = GenerateNodeKeyCmd::parse_from(&["generate-node-key", "--base-path", &base_path]); - assert!(generate.run("test", &String::from("test")).is_ok()); + assert!(generate.run("test_id", &String::from("test")).is_ok()); let buf = fs::read_to_string(key_path.as_path()).unwrap(); assert!(array_bytes::hex2bytes(&buf).is_ok()); - assert!(generate.run("test", &String::from("test")).is_ok()); + assert!(generate.run("test_id", &String::from("test")).is_err()); let new_buf = fs::read_to_string(key_path).unwrap(); assert_eq!( array_bytes::hex2bytes(&new_buf).unwrap(), From 369cc132bb43e9e0ad379c58e02ac5d679f445fe Mon Sep 17 00:00:00 2001 From: Alexandru Gheorghe Date: Fri, 5 Apr 2024 12:56:15 +0300 Subject: [PATCH 17/21] Cli update Signed-off-by: Alexandru Gheorghe --- substrate/bin/utils/subkey/src/lib.rs | 6 +- .../cli/src/commands/generate_node_key.rs | 135 +++++++++++------- .../cli/src/commands/inspect_node_key.rs | 43 +----- substrate/client/cli/src/commands/mod.rs | 2 +- 4 files changed, 91 insertions(+), 95 deletions(-) diff --git a/substrate/bin/utils/subkey/src/lib.rs b/substrate/bin/utils/subkey/src/lib.rs index 74fcd03fe4da..0ca65cd08a6b 100644 --- a/substrate/bin/utils/subkey/src/lib.rs +++ b/substrate/bin/utils/subkey/src/lib.rs @@ -310,7 +310,7 @@ use clap::Parser; use sc_cli::{ - Error, GenerateCmd, GenerateNodeKeyCmd, InspectKeyCmd, InspectNodeKeyCmd, SignCmd, VanityCmd, + Error, GenerateCmd, GenerateKeyCmdCommon, InspectKeyCmd, InspectNodeKeyCmd, SignCmd, VanityCmd, VerifyCmd, }; @@ -324,7 +324,7 @@ use sc_cli::{ pub enum Subkey { /// Generate a random node key, write it to a file or stdout and write the /// corresponding peer-id to stderr - GenerateNodeKey(GenerateNodeKeyCmd), + GenerateNodeKey(GenerateKeyCmdCommon), /// Generate a random account Generate(GenerateCmd), @@ -348,7 +348,7 @@ pub enum Subkey { /// Run the subkey command, given the appropriate runtime. pub fn run() -> Result<(), Error> { match Subkey::parse() { - Subkey::GenerateNodeKey(cmd) => cmd.run("", &String::default()), + Subkey::GenerateNodeKey(cmd) => cmd.run(), Subkey::Generate(cmd) => cmd.run(), Subkey::Inspect(cmd) => cmd.run(), Subkey::InspectNodeKey(cmd) => cmd.run(), diff --git a/substrate/client/cli/src/commands/generate_node_key.rs b/substrate/client/cli/src/commands/generate_node_key.rs index f2f48acba369..4569fb4a340e 100644 --- a/substrate/client/cli/src/commands/generate_node_key.rs +++ b/substrate/client/cli/src/commands/generate_node_key.rs @@ -18,7 +18,7 @@ //! Implementation of the `generate-node-key` subcommand use crate::{build_network_key_dir_or_default, Error, NODE_KEY_ED25519_FILE}; -use clap::Parser; +use clap::{Args, Parser}; use libp2p_identity::{ed25519, Keypair}; use sc_service::BasePath; use std::{ @@ -27,24 +27,30 @@ use std::{ path::PathBuf, }; -/// The `generate-node-key` command -#[derive(Debug, Parser)] -#[command( - name = "generate-node-key", - about = "Generate a random node key, write it to a file or stdout \ - and write the corresponding peer-id to stderr" -)] -pub struct GenerateNodeKeyCmd { +/// Common arguments accross all generate key commands, subkey and node. +#[derive(Debug, Args, Clone)] +pub struct GenerateKeyCmdCommon { /// Name of file to save secret key to. /// If not given, the secret key is printed to stdout. - #[arg(long, conflicts_with_all = ["base_path", "default_base_path", "chain"])] + #[arg(long)] file: Option, /// The output is in raw binary format. /// If not given, the output is written as an hex encoded string. #[arg(long)] bin: bool, +} +/// The `generate-node-key` command +#[derive(Debug, Clone, Parser)] +#[command( + name = "generate-node-key", + about = "Generate a random node key, write it to a file or stdout \ + and write the corresponding peer-id to stderr" +)] +pub struct GenerateNodeKeyCmd { + #[clap(flatten)] + pub common: GenerateKeyCmdCommon, /// Specify the chain specification. /// /// It can be any of the predefined chains like dev, local, staging, polkadot, kusama. @@ -61,52 +67,81 @@ pub struct GenerateNodeKeyCmd { default_base_path: bool, } +impl GenerateKeyCmdCommon { + /// Run the command + pub fn run(&self) -> Result<(), Error> { + generate_key(&self.file, self.bin, None, &None, false, None) + } +} + impl GenerateNodeKeyCmd { /// Run the command pub fn run(&self, chain_spec_id: &str, executable_name: &String) -> Result<(), Error> { - let keypair = ed25519::Keypair::generate(); - - let secret = keypair.secret(); - - let file_data = if self.bin { - secret.as_ref().to_owned() - } else { - array_bytes::bytes2hex("", secret).into_bytes() - }; - - match (&self.file, &self.base_path, self.default_base_path) { - (Some(file), None, false) => fs::write(file, file_data)?, - (None, Some(_), false) | (None, None, true) => { - let network_path = build_network_key_dir_or_default( - self.base_path.clone().map(BasePath::new), - chain_spec_id, - executable_name, - ); - - fs::create_dir_all(network_path.as_path())?; - - let key_path = network_path.join(NODE_KEY_ED25519_FILE); - if key_path.exists() { - eprintln!("Skip generation, a key already exists in {:?}", key_path); - return Err(Error::KeyAlreadyExistsInPath(key_path)); - } else { - eprintln!("Generating key in {:?}", key_path); - fs::write(key_path, file_data)? - } - }, - (None, None, false) => io::stdout().lock().write_all(&file_data)?, - (_, _, _) => { - // This should not happen, arguments are marked as mutually exclusive. - return Err(Error::Input("Mutually exclusive arguments provided".into())); - }, - } - - eprintln!("{}", Keypair::from(keypair).public().to_peer_id()); - - Ok(()) + generate_key( + &self.common.file, + self.common.bin, + Some(chain_spec_id), + &self.base_path, + self.default_base_path, + Some(executable_name), + ) } } +// Utility function for generating a key based on the provided CLI arguments +// +// `file` - Name of file to save secret key to +// `bin` +fn generate_key( + file: &Option, + bin: bool, + chain_spec_id: Option<&str>, + base_path: &Option, + default_base_path: bool, + executable_name: Option<&String>, +) -> Result<(), Error> { + let keypair = ed25519::Keypair::generate(); + + let secret = keypair.secret(); + + let file_data = if bin { + secret.as_ref().to_owned() + } else { + array_bytes::bytes2hex("", secret).into_bytes() + }; + + match (file, base_path, default_base_path) { + (Some(file), None, false) => fs::write(file, file_data)?, + (None, Some(_), false) | (None, None, true) => { + let network_path = build_network_key_dir_or_default( + base_path.clone().map(BasePath::new), + chain_spec_id.unwrap_or_default(), + executable_name.ok_or(Error::Input("Executable name not provided".into()))?, + ); + + fs::create_dir_all(network_path.as_path())?; + + let key_path = network_path.join(NODE_KEY_ED25519_FILE); + if key_path.exists() { + eprintln!("Skip generation, a key already exists in {:?}", key_path); + return Err(Error::KeyAlreadyExistsInPath(key_path)); + } else { + eprintln!("Generating key in {:?}", key_path); + fs::write(key_path, file_data)? + } + }, + (None, None, false) => io::stdout().lock().write_all(&file_data)?, + (_, _, _) => { + // This should not happen, arguments are marked as mutually exclusive. + return Err(Error::Input("Mutually exclusive arguments provided".into())); + }, + } + + eprintln!("{}", Keypair::from(keypair).public().to_peer_id()); + + Ok(()) +} + #[cfg(test)] pub mod tests { use crate::DEFAULT_NETWORK_CONFIG_PATH; diff --git a/substrate/client/cli/src/commands/inspect_node_key.rs b/substrate/client/cli/src/commands/inspect_node_key.rs index 42fecf27d245..521583361a96 100644 --- a/substrate/client/cli/src/commands/inspect_node_key.rs +++ b/substrate/client/cli/src/commands/inspect_node_key.rs @@ -79,50 +79,11 @@ impl InspectNodeKeyCmd { #[cfg(test)] mod tests { - use crate::SubstrateCli; + use crate::{commands::generate_node_key::GenerateNodeKeyCmd, SubstrateCli}; - use super::{super::GenerateNodeKeyCmd, *}; + use super::*; use sc_service::{ChainSpec, ChainType, GenericChainSpec, NoExtension}; - struct Cli; - - impl SubstrateCli for Cli { - fn impl_name() -> String { - "test".into() - } - - fn impl_version() -> String { - "2.0".into() - } - - fn description() -> String { - "test".into() - } - - fn support_url() -> String { - "test.test".into() - } - - fn copyright_start_year() -> i32 { - 2021 - } - - fn author() -> String { - "test".into() - } - - fn load_spec(&self, _: &str) -> std::result::Result, String> { - Ok(Box::new( - GenericChainSpec::<()>::builder(Default::default(), NoExtension::None) - .with_name("test") - .with_id("test_id") - .with_chain_type(ChainType::Development) - .with_genesis_config_patch(Default::default()) - .build(), - )) - } - } - #[test] fn inspect_node_key() { let path = tempfile::tempdir().unwrap().into_path().join("node-id").into_os_string(); diff --git a/substrate/client/cli/src/commands/mod.rs b/substrate/client/cli/src/commands/mod.rs index 9d48d2bdf644..2d7a0dc72ff5 100644 --- a/substrate/client/cli/src/commands/mod.rs +++ b/substrate/client/cli/src/commands/mod.rs @@ -42,7 +42,7 @@ mod verify; pub use self::{ build_spec_cmd::BuildSpecCmd, chain_info_cmd::ChainInfoCmd, check_block_cmd::CheckBlockCmd, export_blocks_cmd::ExportBlocksCmd, export_state_cmd::ExportStateCmd, generate::GenerateCmd, - generate_node_key::GenerateNodeKeyCmd, import_blocks_cmd::ImportBlocksCmd, + generate_node_key::GenerateKeyCmdCommon, import_blocks_cmd::ImportBlocksCmd, insert_key::InsertKeyCmd, inspect_key::InspectKeyCmd, inspect_node_key::InspectNodeKeyCmd, key::KeySubcommand, purge_chain_cmd::PurgeChainCmd, revert_cmd::RevertCmd, run_cmd::RunCmd, sign::SignCmd, vanity::VanityCmd, verify::VerifyCmd, From 93ac9204046023f548f71f4a40f3ccd2c79e0cd8 Mon Sep 17 00:00:00 2001 From: Alexandru Gheorghe Date: Fri, 5 Apr 2024 14:11:17 +0300 Subject: [PATCH 18/21] Fix unused Signed-off-by: Alexandru Gheorghe --- substrate/client/cli/src/commands/inspect_node_key.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/substrate/client/cli/src/commands/inspect_node_key.rs b/substrate/client/cli/src/commands/inspect_node_key.rs index 521583361a96..25a0a685650e 100644 --- a/substrate/client/cli/src/commands/inspect_node_key.rs +++ b/substrate/client/cli/src/commands/inspect_node_key.rs @@ -79,10 +79,9 @@ impl InspectNodeKeyCmd { #[cfg(test)] mod tests { - use crate::{commands::generate_node_key::GenerateNodeKeyCmd, SubstrateCli}; + use crate::commands::generate_node_key::GenerateNodeKeyCmd; use super::*; - use sc_service::{ChainSpec, ChainType, GenericChainSpec, NoExtension}; #[test] fn inspect_node_key() { From db5e1d7cfc62f58b0c98b84d7ee154a899b6e168 Mon Sep 17 00:00:00 2001 From: Alexandru Gheorghe Date: Tue, 9 Apr 2024 18:55:10 +0300 Subject: [PATCH 19/21] Cargo fmt Signed-off-by: Alexandru Gheorghe --- substrate/client/cli/src/commands/generate_node_key.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/client/cli/src/commands/generate_node_key.rs b/substrate/client/cli/src/commands/generate_node_key.rs index 4569fb4a340e..bdb94eec93b4 100644 --- a/substrate/client/cli/src/commands/generate_node_key.rs +++ b/substrate/client/cli/src/commands/generate_node_key.rs @@ -89,7 +89,7 @@ impl GenerateNodeKeyCmd { } // Utility function for generating a key based on the provided CLI arguments -// +// // `file` - Name of file to save secret key to // `bin` fn generate_key( From 6b94cc7ec2d4bb1c5161e43022925fe69b448591 Mon Sep 17 00:00:00 2001 From: Alexandru Gheorghe <49718502+alexggh@users.noreply.github.com> Date: Thu, 11 Apr 2024 09:30:56 +0300 Subject: [PATCH 20/21] Update substrate/client/cli/src/error.rs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Bastian Köcher --- substrate/client/cli/src/error.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/substrate/client/cli/src/error.rs b/substrate/client/cli/src/error.rs index 8ea5cff50cca..90ad048009ad 100644 --- a/substrate/client/cli/src/error.rs +++ b/substrate/client/cli/src/error.rs @@ -90,7 +90,6 @@ pub enum Error { \n 2. [Preferred] Separately generate the key with: key generate-node-key --file \n 3. [Preferred] Separately generate the key with: key generate-node-key --default-base-path \n 4. [Unsafe] Pass --unsafe-force-node-key-generation and make sure you remove it for subsequent node restarts" - )] NetworkKeyNotFound(PathBuf), #[error("A network key already exists in path {0}")] From 9f13c16b6a804b06c74ca88dc5a2852e71a6de15 Mon Sep 17 00:00:00 2001 From: Alexandru Gheorghe Date: Thu, 11 Apr 2024 10:00:45 +0300 Subject: [PATCH 21/21] Add prdoc Signed-off-by: Alexandru Gheorghe --- prdoc/pr_3852.prdoc | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) create mode 100644 prdoc/pr_3852.prdoc diff --git a/prdoc/pr_3852.prdoc b/prdoc/pr_3852.prdoc new file mode 100644 index 000000000000..f13e1766d518 --- /dev/null +++ b/prdoc/pr_3852.prdoc @@ -0,0 +1,25 @@ +title: (Breaking change)Enforce network key presence on authorities. + +doc: + - audience: Node Operator + description: | + (Breaking change) For all authority nodes, the node binary now enforces the presence + of a network key, instead of auto-generating when it is absent. + + Before this change, all node binaries were auto-generating the node key when it was not present, + that is dangerous because other nodes in the network expects a stable identity for authorities. + + To prevent accidental generation of node key, we removed this behaviour and node binary will now throw + an error if the network key is not present and operators will receive instructions to either persist + their network key or explicitly generate a new one with the `polkadot key generate-node-key`. + + To prevent this error on restart/upgrades node operators need to make sure their network key are always + persisted, if nodes already correctly persist all directories in `--base-path` then no action is needed. + +crates: + - name: sc-cli + bump: major + - name: polkadot + bump: major + - name: subkey + bump: minor \ No newline at end of file