Prevent accidental change of network-key for active authorities

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")

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),

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<String>,
+	/// 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<PathBuf>,
+
+	/// 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<PathBuf>,
+	bin: bool,
+	chain_spec_id: Option<&str>,
+	base_path: &Option<PathBuf>,
+	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()
+		);
+	}
 }

substrate/client/cli/src/commands/inspect_node_key.rs

@@ -79,15 +79,17 @@ impl InspectNodeKeyCmd {
 
 #[cfg(test)]
 mod tests {
-	use super::{super::GenerateNodeKeyCmd, *};
+	use crate::commands::generate_node_key::GenerateNodeKeyCmd;
+
+	use super::*;
 
 	#[test]
 	fn inspect_node_key() {
 		let path = tempfile::tempdir().unwrap().into_path().join("node-id").into_os_string();
 		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());

substrate/client/cli/src/commands/key.rs

@@ -47,7 +47,10 @@ impl KeySubcommand {
 	/// run the key subcommands
 	pub fn run<C: SubstrateCli>(&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),

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,

substrate/client/cli/src/config.rs

@@ -428,8 +428,10 @@ pub trait CliConfiguration<DCV: DefaultConfigurationValues = ()>: 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<NodeKeyConfig> {
+		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<DCV: DefaultConfigurationValues = ()>: 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<BasePath>,
+	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<BasePath>,
+	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)
+}

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,21 @@ 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: <NODE_BINARY> key generate-node-key --base-path <YOUR_BASE_PATH>
+		\n 2. [Preferred] Separately generate the key with: <NODE_BINARY> key generate-node-key --file <YOUR_PATH_TO_NODE_KEY>
+		\n 3. [Preferred] Separately generate the key with: <NODE_BINARY> 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 {