client: Replace unsafe_rpc_expose with an RpcMethods enum (#5729)

* client: Replace `unsafe_rpc_expose` with an `RpcMethods` enum

which can be either Default, Safe or Unsafe. The idea is to have the
following:
|                       | --rpc-external=false  | --rpc-external=true   |
|---------------------  |-------------------    |-----------------      |
| --rpc-methods=Default |                       | unsafe calls denied   |
| --rpc-methods=Safe    | unsafe calls denied   | unsafe calls denied   |
| --rpc-methods=Unsafe  |                       |                       |
Since the previous `unsafe-rpc-expose` option was confusing.

* client: Only warn against exposing externally unsafe RPC method set

* Apply suggestions from code review

Co-Authored-By: Cecile Tonglet <cecile.tonglet@cecton.com>

* cli: Rephrase doc comment for rpc_methods config

* Improve debuggability of build_spec_works

...by printing to stderr the stderr of the command. This is normally
suppressed for succesful tests but not for failing ones - if that's the
case then it's useful to see the test failure reason inline rather than
having to execute the command separately ourselves.

* Rename RpcMethods::{Default => Auto} variant

* Update bin/node/cli/tests/build_spec_works.rs

Co-authored-by: Benjamin Kampmann <ben.kampmann@googlemail.com>
Co-authored-by: Cecile Tonglet <cecile.tonglet@cecton.com>
Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

substrate/client/cli/src/arg_enums.rs

@@ -122,6 +122,31 @@ impl ExecutionStrategy {
 	}
 }
 
+arg_enum! {
+	/// Available RPC methods.
+	#[allow(missing_docs)]
+	#[derive(Debug, Copy, Clone, PartialEq)]
+	pub enum RpcMethods {
+		// Expose every RPC method only when RPC is listening on `localhost`,
+		// otherwise serve only safe RPC methods.
+		Auto,
+		// Allow only a safe subset of RPC methods.
+		Safe,
+		// Expose every RPC method (even potentially unsafe ones).
+		Unsafe,
+	}
+}
+
+impl Into<sc_service::config::RpcMethods> for RpcMethods {
+	fn into(self) -> sc_service::config::RpcMethods {
+		match self {
+			RpcMethods::Auto => sc_service::config::RpcMethods::Auto,
+			RpcMethods::Safe => sc_service::config::RpcMethods::Safe,
+			RpcMethods::Unsafe => sc_service::config::RpcMethods::Unsafe,
+		}
+	}
+}
+
 arg_enum! {
 	/// Database backend
 	#[allow(missing_docs)]

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

@@ -296,9 +296,9 @@ macro_rules! substrate_cli_subcommands {
 				}
 			}
 
-			fn unsafe_rpc_expose(&self) -> $crate::Result<bool> {
+			fn rpc_methods(&self) -> $crate::Result<sc_service::config::RpcMethods> {
 				match self {
-					$($enum::$variant(cmd) => cmd.unsafe_rpc_expose()),*
+					$($enum::$variant(cmd) => cmd.rpc_methods()),*
 				}
 			}
 

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

@@ -14,6 +14,7 @@
 // You should have received a copy of the GNU General Public License
 // along with Substrate.  If not, see <http://www.gnu.org/licenses/>.
 
+use crate::arg_enums::RpcMethods;
 use crate::error::{Error, Result};
 use crate::params::ImportParams;
 use crate::params::KeystoreParams;
@@ -80,16 +81,24 @@ pub struct RunCmd {
 	/// Listen to all RPC interfaces.
 	///
 	/// Same as `--rpc-external`.
-	#[structopt(long = "unsafe-rpc-external")]
+	#[structopt(long)]
 	pub unsafe_rpc_external: bool,
 
-	/// Don't deny potentially unsafe RPCs when listening on external interfaces.
+	/// RPC methods to expose.
 	///
-	/// Default is false. This allows exposing RPC methods publicly (same as `--unsafe-{rpc,ws}-external` )
-	/// but will allow doing so even on validator nodes, which is prohibited by default.
-	/// Please do this if you know what you're doing.
-	#[structopt(long = "unsafe-rpc-expose")]
-	pub unsafe_rpc_expose: bool,
+	/// - `Unsafe`: Exposes every RPC method.
+	/// - `Safe`: Exposes only a safe subset of RPC methods, denying unsafe RPC methods.
+	/// - `Auto`: Acts as `Safe` if RPC is served externally, e.g. when `--{rpc,ws}-external` is passed,
+	///   otherwise acts as `Unsafe`.
+	#[structopt(
+		long,
+		value_name = "METHOD SET",
+		possible_values = &RpcMethods::variants(),
+		case_insensitive = true,
+		default_value = "Auto",
+		verbatim_doc_comment
+	)]
+	pub rpc_methods: RpcMethods,
 
 	/// Listen to all Websocket interfaces.
 	///
@@ -406,7 +415,7 @@ impl CliConfiguration for RunCmd {
 		let interface = rpc_interface(
 			self.rpc_external,
 			self.unsafe_rpc_external,
-			self.unsafe_rpc_expose,
+			self.rpc_methods,
 			self.validator
 		)?;
 
@@ -417,15 +426,15 @@ impl CliConfiguration for RunCmd {
 		let interface = rpc_interface(
 			self.ws_external,
 			self.unsafe_ws_external,
-			self.unsafe_rpc_expose,
+			self.rpc_methods,
 			self.validator
 		)?;
 
 		Ok(Some(SocketAddr::new(interface, self.ws_port.unwrap_or(9944))))
 	}
 
-	fn unsafe_rpc_expose(&self) -> Result<bool> {
-		Ok(self.unsafe_rpc_expose)
+	fn rpc_methods(&self) -> Result<sc_service::config::RpcMethods> {
+		Ok(self.rpc_methods.into())
 	}
 
 	fn transaction_pool(&self) -> Result<TransactionPoolOptions> {
@@ -462,23 +471,26 @@ pub fn is_node_name_valid(_name: &str) -> std::result::Result<(), &str> {
 fn rpc_interface(
 	is_external: bool,
 	is_unsafe_external: bool,
-	is_unsafe_rpc_expose: bool,
+	rpc_methods: RpcMethods,
 	is_validator: bool,
 ) -> Result<IpAddr> {
-	if is_external && is_validator && !is_unsafe_rpc_expose {
+	if is_external && is_validator && rpc_methods != RpcMethods::Unsafe {
 		return Err(Error::Input(
 			"--rpc-external and --ws-external options shouldn't be \
-		used if the node is running as a validator. Use `--unsafe-rpc-external` if you understand \
-		the risks. See the options description for more information."
+		used if the node is running as a validator. Use `--unsafe-rpc-external` \
+		or `--rpc-methods=unsafe` if you understand the risks. See the options \
+		description for more information."
 				.to_owned(),
 		));
 	}
 
 	if is_external || is_unsafe_external {
-		log::warn!(
-			"It isn't safe to expose RPC publicly without a proxy server that filters \
-		available set of RPC methods."
-		);
+		if rpc_methods == RpcMethods::Unsafe {
+			log::warn!(
+				"It isn't safe to expose RPC publicly without a proxy server that filters \
+			available set of RPC methods."
+			);
+		}
 
 		Ok(Ipv4Addr::UNSPECIFIED.into())
 	} else {

substrate/client/cli/src/config.rs

@@ -27,8 +27,8 @@ use names::{Generator, Name};
 use sc_client_api::execution_extensions::ExecutionStrategies;
 use sc_service::config::{
 	Configuration, DatabaseConfig, ExtTransport, KeystoreConfig, NetworkConfiguration,
-	NodeKeyConfig, OffchainWorkerConfig, PrometheusConfig, PruningMode, Role, TaskType,
-	TelemetryEndpoints, TransactionPoolOptions, WasmExecutionMethod,
+	NodeKeyConfig, OffchainWorkerConfig, PrometheusConfig, PruningMode, Role, RpcMethods,
+	TaskType, TelemetryEndpoints, TransactionPoolOptions, WasmExecutionMethod,
 };
 use sc_service::{ChainSpec, TracingReceiver};
 use std::future::Future;
@@ -265,10 +265,11 @@ pub trait CliConfiguration: Sized {
 		Ok(Default::default())
 	}
 
-	/// Returns `Ok(true) if potentially unsafe RPC is to be exposed.
+	/// Returns the RPC method set to expose.
 	///
-	/// By default this is `false`.
-	fn unsafe_rpc_expose(&self) -> Result<bool> {
+	/// By default this is `RpcMethods::Auto` (unsafe RPCs are denied iff
+	/// `{rpc,ws}_external` returns true, respectively).
+	fn rpc_methods(&self) -> Result<RpcMethods> {
 		Ok(Default::default())
 	}
 
@@ -449,7 +450,7 @@ pub trait CliConfiguration: Sized {
 			execution_strategies: self.execution_strategies(is_dev)?,
 			rpc_http: self.rpc_http()?,
 			rpc_ws: self.rpc_ws()?,
-			unsafe_rpc_expose: self.unsafe_rpc_expose()?,
+			rpc_methods: self.rpc_methods()?,
 			rpc_ws_max_connections: self.rpc_ws_max_connections()?,
 			rpc_cors: self.rpc_cors(is_dev)?,
 			prometheus_config: self.prometheus_config()?,

substrate/client/service/src/config.rs

@@ -59,8 +59,6 @@ pub struct Configuration {
 	pub wasm_method: WasmExecutionMethod,
 	/// Execution strategies.
 	pub execution_strategies: ExecutionStrategies,
-	/// Whether potentially unsafe RPC is considered safe to be exposed.
-	pub unsafe_rpc_expose: bool,
 	/// RPC over HTTP binding address. `None` if disabled.
 	pub rpc_http: Option<SocketAddr>,
 	/// RPC over Websockets binding address. `None` if disabled.
@@ -69,6 +67,8 @@ pub struct Configuration {
 	pub rpc_ws_max_connections: Option<usize>,
 	/// CORS settings for HTTP & WS servers. `None` if all origins are allowed.
 	pub rpc_cors: Option<Vec<String>>,
+	/// RPC methods to expose (by default only a safe subset or all of them).
+	pub rpc_methods: RpcMethods,
 	/// Prometheus endpoint configuration. `None` if disabled.
 	pub prometheus_config: Option<PrometheusConfig>,
 	/// Telemetry service URL. `None` if disabled.
@@ -171,3 +171,21 @@ impl Configuration {
 		self.role.to_string()
 	}
 }
+
+/// Available RPC methods.
+#[derive(Debug, Copy, Clone)]
+pub enum RpcMethods {
+	/// Expose every RPC method only when RPC is listening on `localhost`,
+	/// otherwise serve only safe RPC methods.
+	Auto,
+	/// Allow only a safe subset of RPC methods.
+	Safe,
+	/// Expose every RPC method (even potentially unsafe ones).
+	Unsafe,
+}
+
+impl Default for RpcMethods {
+	fn default() -> RpcMethods {
+		RpcMethods::Auto
+	}
+}

substrate/client/service/src/lib.rs

@@ -64,7 +64,7 @@ pub use self::builder::{
 	ServiceBuilder, ServiceBuilderCommand, TFullClient, TLightClient, TFullBackend, TLightBackend,
 	TFullCallExecutor, TLightCallExecutor,
 };
-pub use config::{Configuration, Role, PruningMode, DatabaseConfig, TaskType};
+pub use config::{Configuration, DatabaseConfig, PruningMode, Role, RpcMethods, TaskType};
 pub use sc_chain_spec::{
 	ChainSpec, GenericChainSpec, Properties, RuntimeGenesis, Extension as ChainSpecExtension,
 	NoExtension, ChainType,
@@ -551,12 +551,12 @@ fn start_rpc_servers<H: FnMut(sc_rpc::DenyUnsafe) -> sc_rpc_server::RpcHandler<s
 		})
 	}
 
-	fn deny_unsafe(addr: &Option<SocketAddr>, unsafe_rpc_expose: bool) -> sc_rpc::DenyUnsafe {
+	fn deny_unsafe(addr: &Option<SocketAddr>, methods: &RpcMethods) -> sc_rpc::DenyUnsafe {
 		let is_exposed_addr = addr.map(|x| x.ip().is_loopback()).unwrap_or(false);
-		if is_exposed_addr && !unsafe_rpc_expose {
-			sc_rpc::DenyUnsafe::Yes
-		} else {
-			sc_rpc::DenyUnsafe::No
+		match (is_exposed_addr, methods) {
+			| (_, RpcMethods::Unsafe)
+			| (false, RpcMethods::Auto) => sc_rpc::DenyUnsafe::No,
+			_ => sc_rpc::DenyUnsafe::Yes
 		}
 	}
 
@@ -566,7 +566,7 @@ fn start_rpc_servers<H: FnMut(sc_rpc::DenyUnsafe) -> sc_rpc_server::RpcHandler<s
 			|address| sc_rpc_server::start_http(
 				address,
 				config.rpc_cors.as_ref(),
-				gen_handler(deny_unsafe(&config.rpc_http, config.unsafe_rpc_expose)),
+				gen_handler(deny_unsafe(&config.rpc_http, &config.rpc_methods)),
 			),
 		)?.map(|s| waiting::HttpServer(Some(s))),
 		maybe_start_server(
@@ -575,7 +575,7 @@ fn start_rpc_servers<H: FnMut(sc_rpc::DenyUnsafe) -> sc_rpc_server::RpcHandler<s
 				address,
 				config.rpc_ws_max_connections,
 				config.rpc_cors.as_ref(),
-				gen_handler(deny_unsafe(&config.rpc_ws, config.unsafe_rpc_expose)),
+				gen_handler(deny_unsafe(&config.rpc_ws, &config.rpc_methods)),
 			),
 		)?.map(|s| waiting::WsServer(Some(s))),
 	)))

substrate/client/service/test/src/lib.rs

@@ -191,11 +191,11 @@ fn node_config<G: RuntimeGenesis + 'static, E: ChainSpecExtension + Clone + 'sta
 		chain_spec: Box::new((*spec).clone()),
 		wasm_method: sc_service::config::WasmExecutionMethod::Interpreted,
 		execution_strategies: Default::default(),
-		unsafe_rpc_expose: false,
 		rpc_http: None,
 		rpc_ws: None,
 		rpc_ws_max_connections: None,
 		rpc_cors: None,
+		rpc_methods: Default::default(),
 		prometheus_config: None,
 		telemetry_endpoints: None,
 		telemetry_external_transport: None,

substrate/utils/browser/src/lib.rs

@@ -86,8 +86,8 @@ where
 		rpc_cors: Default::default(),
 		rpc_http: Default::default(),
 		rpc_ws: Default::default(),
-		unsafe_rpc_expose: false,
 		rpc_ws_max_connections: Default::default(),
+		rpc_methods: Default::default(),
 		state_cache_child_ratio: Default::default(),
 		state_cache_size: Default::default(),
 		tracing_receiver: Default::default(),