Prevent accidental change of network-key for active authorities

polkadot/.rpm/polkadot.spec

@@ -37,6 +37,9 @@ 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

polkadot/scripts/packaging/deb-maintainer-scripts/postinst

@@ -14,4 +14,7 @@ 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

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 <admin@parity.io>".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::{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<PathBuf>,
 
 	/// 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<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 +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<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(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()))
 	}
 

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. Separately generate the key with: polkadot key generate-node-key --file <YOUR_PATH_TO_NODE_KEY>"
+	)]
+	NetworkKeyNotFound(PathBuf),
 }
 
 impl From<&str> for Error {