Prevent accidental change of network-key for active authorities

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

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,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 <[email protected]>".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<Box<dyn sc_cli::ChainSpec>, 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(),

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::{build_network_key_dir_or_default, Error, SubstrateCli, 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,33 @@ 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", "chain"])]
 	file: Option<PathBuf>,
 
 	/// The output is in raw binary format.
 	/// If not given, the output is written as an hex encoded string.
 	#[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<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 GenerateNodeKeyCmd {
 	/// Run the command
-	pub fn run(&self) -> Result<(), Error> {
+	pub fn run<C: SubstrateCli>(&self, cli: &C) -> Result<(), Error> {
 		let keypair = ed25519::Keypair::generate();
 
 		let secret = keypair.secret();
@@ -58,9 +74,33 @@ 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 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);
+					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 +110,84 @@ impl GenerateNodeKeyCmd {
 }
 
 #[cfg(test)]
-mod tests {
+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<Box<dyn ChainSpec>, 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("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(&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()
+		);
+	}
 }

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

@@ -79,15 +79,57 @@ 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<Box<dyn ChainSpec>, 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();
 		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());

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

@@ -47,7 +47,7 @@ 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) => cmd.run(cli),
 			KeySubcommand::Generate(cmd) => cmd.run(),
 			KeySubcommand::Inspect(cmd) => cmd.run(),
 			KeySubcommand::Insert(cmd) => cmd.run(cli),

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()?, 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(
@@ -665,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<C: SubstrateCli>(
+	base_path: Option<BasePath>,
+	_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<dyn ChainSpec>) -> 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<C: SubstrateCli>(
+	base_path: Option<BasePath>,
+	chain_spec: &Box<dyn ChainSpec>,
+	cli: &C,
+) -> PathBuf {
+	let config_dir = build_config_dir(&base_path_or_default(base_path, cli), chain_spec);
+	build_net_config_dir(&config_dir)
+}