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") 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 diff --git a/substrate/bin/utils/subkey/src/lib.rs b/substrate/bin/utils/subkey/src/lib.rs index 33f28ef46a5e..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), diff --git a/substrate/client/cli/src/commands/generate_node_key.rs b/substrate/client/cli/src/commands/generate_node_key.rs index 43851dc1af5c..bdb94eec93b4 100644 --- a/substrate/client/cli/src/commands/generate_node_key.rs +++ b/substrate/client/cli/src/commands/generate_node_key.rs @@ -17,23 +17,19 @@ //! Implementation of the `generate-node-key` subcommand -use crate::Error; -use clap::Parser; +use crate::{build_network_key_dir_or_default, Error, NODE_KEY_ED25519_FILE}; +use clap::{Args, Parser}; use libp2p_identity::{ed25519, Keypair}; +use sc_service::BasePath; use std::{ fs, io::{self, Write}, 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)] @@ -45,32 +41,111 @@ pub struct GenerateNodeKeyCmd { bin: bool, } -impl GenerateNodeKeyCmd { +/// 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. + #[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"])] + 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 GenerateKeyCmdCommon { /// Run the command pub fn run(&self) -> Result<(), Error> { - let keypair = ed25519::Keypair::generate(); + 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> { + 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 secret = keypair.secret(); - let file_data = if self.bin { - secret.as_ref().to_owned() - } else { - array_bytes::bytes2hex("", secret).into_bytes() - }; + let file_data = if bin { + secret.as_ref().to_owned() + } else { + 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 (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()))?, + ); - eprintln!("{}", Keypair::from(keypair).public().to_peer_id()); + fs::create_dir_all(network_path.as_path())?; - Ok(()) + 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)] -mod tests { +pub mod tests { + use crate::DEFAULT_NETWORK_CONFIG_PATH; + use super::*; use std::io::Read; use tempfile::Builder; @@ -80,9 +155,32 @@ mod tests { 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("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()); } + + #[test] + fn generate_node_key_base_path() { + let base_dir = Builder::new().prefix("keyfile").tempdir().unwrap(); + 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]); + 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_id", &String::from("test")).is_err()); + 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..25a0a685650e 100644 --- a/substrate/client/cli/src/commands/inspect_node_key.rs +++ b/substrate/client/cli/src/commands/inspect_node_key.rs @@ -79,7 +79,9 @@ impl InspectNodeKeyCmd { #[cfg(test)] mod tests { - use super::{super::GenerateNodeKeyCmd, *}; + use crate::commands::generate_node_key::GenerateNodeKeyCmd; + + use super::*; #[test] fn inspect_node_key() { @@ -87,7 +89,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("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 d49b7e4072c8..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(), + 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/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, diff --git a/substrate/client/cli/src/config.rs b/substrate/client/cli/src/config.rs index 5def9ce9b726..70a4885e5eef 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())) } @@ -463,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()?, &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); let database = self.database()?.unwrap_or( @@ -665,3 +665,33 @@ 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, + executable_name: &String, +) -> BasePath { + 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_id: &str) -> 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_id: &str, + executable_name: &String, +) -> PathBuf { + 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 6c0cfca4932e..90ad048009ad 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,20 @@ 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 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: 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 53f19f58e1fb..7058af19f1d4 100644 --- a/substrate/client/cli/src/params/node_key_params.rs +++ b/substrate/client/cli/src/params/node_key_params.rs @@ -18,15 +18,16 @@ 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` /// 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. @@ -79,22 +80,48 @@ 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 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` + /// + /// 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, } 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) @@ -122,7 +149,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() { @@ -136,8 +164,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(()), @@ -156,10 +185,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"); @@ -186,29 +216,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()); } }