diff --git a/Cargo.lock b/Cargo.lock index 42a3c10cbfd64..1716378c5e479 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3566,6 +3566,7 @@ dependencies = [ "sc-finality-grandpa", "sc-finality-grandpa-rpc", "sc-keystore", + "sc-rpc-api", "sp-api", "sp-blockchain", "sp-consensus", @@ -6163,10 +6164,13 @@ dependencies = [ "jsonrpc-core", "jsonrpc-core-client", "jsonrpc-derive", + "sc-consensus", "sc-consensus-babe", "sc-consensus-epochs", "sc-keystore", + "sc-rpc-api", "serde", + "serde_json", "sp-api", "sp-application-crypto", "sp-blockchain", diff --git a/bin/node/cli/src/service.rs b/bin/node/cli/src/service.rs index 7798404ff93ab..b738b5cf1f4ec 100644 --- a/bin/node/cli/src/service.rs +++ b/bin/node/cli/src/service.rs @@ -43,7 +43,6 @@ macro_rules! new_full_start { ($config:expr) => {{ use std::sync::Arc; - type RpcExtension = jsonrpc_core::IoHandler; let mut import_setup = None; let mut rpc_setup = None; let inherent_data_providers = sp_inherents::InherentDataProviders::new(); @@ -99,30 +98,46 @@ macro_rules! new_full_start { import_setup = Some((block_import, grandpa_link, babe_link)); Ok(import_queue) })? - .with_rpc_extensions(|builder| -> std::result::Result { - let babe_link = import_setup.as_ref().map(|s| &s.2) - .expect("BabeLink is present for full services or set up failed; qed."); + .with_rpc_extensions_builder(|builder| { let grandpa_link = import_setup.as_ref().map(|s| &s.1) .expect("GRANDPA LinkHalf is present for full services or set up failed; qed."); - let shared_authority_set = grandpa_link.shared_authority_set(); + + let shared_authority_set = grandpa_link.shared_authority_set().clone(); let shared_voter_state = grandpa::SharedVoterState::empty(); - let deps = node_rpc::FullDeps { - client: builder.client().clone(), - pool: builder.pool(), - select_chain: builder.select_chain().cloned() - .expect("SelectChain is present for full services or set up failed; qed."), - babe: node_rpc::BabeDeps { - keystore: builder.keystore(), - babe_config: sc_consensus_babe::BabeLink::config(babe_link).clone(), - shared_epoch_changes: sc_consensus_babe::BabeLink::epoch_changes(babe_link).clone() - }, - grandpa: node_rpc::GrandpaDeps { - shared_voter_state: shared_voter_state.clone(), - shared_authority_set: shared_authority_set.clone(), - }, - }; - rpc_setup = Some((shared_voter_state)); - Ok(node_rpc::create_full(deps)) + + rpc_setup = Some((shared_voter_state.clone())); + + let babe_link = import_setup.as_ref().map(|s| &s.2) + .expect("BabeLink is present for full services or set up failed; qed."); + + let babe_config = babe_link.config().clone(); + let shared_epoch_changes = babe_link.epoch_changes().clone(); + + let client = builder.client().clone(); + let pool = builder.pool().clone(); + let select_chain = builder.select_chain().cloned() + .expect("SelectChain is present for full services or set up failed; qed."); + let keystore = builder.keystore().clone(); + + Ok(move |deny_unsafe| { + let deps = node_rpc::FullDeps { + client: client.clone(), + pool: pool.clone(), + select_chain: select_chain.clone(), + deny_unsafe, + babe: node_rpc::BabeDeps { + babe_config: babe_config.clone(), + shared_epoch_changes: shared_epoch_changes.clone(), + keystore: keystore.clone(), + }, + grandpa: node_rpc::GrandpaDeps { + shared_voter_state: shared_voter_state.clone(), + shared_authority_set: shared_authority_set.clone(), + }, + }; + + node_rpc::create_full(deps) + }) })?; (builder, import_setup, inherent_data_providers, rpc_setup) @@ -302,7 +317,6 @@ pub fn new_full(config: Configuration) /// Builds a new service for a light client. pub fn new_light(config: Configuration) -> Result { - type RpcExtension = jsonrpc_core::IoHandler; let inherent_data_providers = InherentDataProviders::new(); let service = ServiceBuilder::new_light::(config)? @@ -366,9 +380,7 @@ pub fn new_light(config: Configuration) let provider = client as Arc>; Ok(Arc::new(GrandpaFinalityProofProvider::new(backend, provider)) as _) })? - .with_rpc_extensions(|builder,| -> - Result - { + .with_rpc_extensions(|builder| { let fetcher = builder.fetcher() .ok_or_else(|| "Trying to start node RPC without active fetcher")?; let remote_blockchain = builder.remote_backend() @@ -380,6 +392,7 @@ pub fn new_light(config: Configuration) client: builder.client().clone(), pool: builder.pool(), }; + Ok(node_rpc::create_light(light_deps)) })? .build()?; diff --git a/bin/node/rpc/Cargo.toml b/bin/node/rpc/Cargo.toml index ef948cd009393..5eb0d271b99ca 100644 --- a/bin/node/rpc/Cargo.toml +++ b/bin/node/rpc/Cargo.toml @@ -30,3 +30,4 @@ sp-consensus = { version = "0.8.0-dev", path = "../../../primitives/consensus/co sp-blockchain = { version = "2.0.0-dev", path = "../../../primitives/blockchain" } sc-finality-grandpa = { version = "0.8.0-dev", path = "../../../client/finality-grandpa" } sc-finality-grandpa-rpc = { version = "0.8.0-dev", path = "../../../client/finality-grandpa/rpc" } +sc-rpc-api = { version = "0.8.0-dev", path = "../../../client/rpc-api" } diff --git a/bin/node/rpc/src/lib.rs b/bin/node/rpc/src/lib.rs index 02cb44d4020f7..259a792441d40 100644 --- a/bin/node/rpc/src/lib.rs +++ b/bin/node/rpc/src/lib.rs @@ -42,9 +42,10 @@ use sc_keystore::KeyStorePtr; use sp_consensus_babe::BabeApi; use sc_consensus_epochs::SharedEpochChanges; use sc_consensus_babe::{Config, Epoch}; -use sc_consensus_babe_rpc::BabeRPCHandler; +use sc_consensus_babe_rpc::BabeRpcHandler; use sc_finality_grandpa::{SharedVoterState, SharedAuthoritySet}; use sc_finality_grandpa_rpc::GrandpaRpcHandler; +use sc_rpc_api::DenyUnsafe; /// Light client extra dependencies. pub struct LightDeps { @@ -84,6 +85,8 @@ pub struct FullDeps { pub pool: Arc

, /// The SelectChain Strategy pub select_chain: SC, + /// Whether to deny unsafe calls + pub deny_unsafe: DenyUnsafe, /// BABE specific dependencies. pub babe: BabeDeps, /// GRANDPA specific dependencies. @@ -115,6 +118,7 @@ pub fn create_full( client, pool, select_chain, + deny_unsafe, babe, grandpa, } = deps; @@ -142,7 +146,14 @@ pub fn create_full( ); io.extend_with( sc_consensus_babe_rpc::BabeApi::to_delegate( - BabeRPCHandler::new(client, shared_epoch_changes, keystore, babe_config, select_chain) + BabeRpcHandler::new( + client, + shared_epoch_changes, + keystore, + babe_config, + select_chain, + deny_unsafe, + ), ) ); io.extend_with( diff --git a/client/consensus/babe/rpc/Cargo.toml b/client/consensus/babe/rpc/Cargo.toml index 2a0762e1a8374..900e29bfba95d 100644 --- a/client/consensus/babe/rpc/Cargo.toml +++ b/client/consensus/babe/rpc/Cargo.toml @@ -13,6 +13,7 @@ targets = ["x86_64-unknown-linux-gnu"] [dependencies] sc-consensus-babe = { version = "0.8.0-dev", path = "../" } +sc-rpc-api = { version = "0.8.0-dev", path = "../../../rpc-api" } jsonrpc-core = "14.0.3" jsonrpc-core-client = "14.0.5" jsonrpc-derive = "14.0.3" @@ -29,7 +30,9 @@ sp-core = { version = "2.0.0-dev", path = "../../../../primitives/core" } sc-keystore = { version = "2.0.0-dev", path = "../../../keystore" } [dev-dependencies] -substrate-test-runtime-client = { version = "2.0.0-dev", path = "../../../../test-utils/runtime/client" } +sc-consensus = { version = "0.8.0-dev", path = "../../../consensus/common" } +serde_json = "1.0.50" sp-application-crypto = { version = "2.0.0-dev", path = "../../../../primitives/application-crypto" } sp-keyring = { version = "2.0.0-dev", path = "../../../../primitives/keyring" } +substrate-test-runtime-client = { version = "2.0.0-dev", path = "../../../../test-utils/runtime/client" } tempfile = "3.1.0" diff --git a/client/consensus/babe/rpc/src/lib.rs b/client/consensus/babe/rpc/src/lib.rs index 925328a856f07..8e1282a8d79a7 100644 --- a/client/consensus/babe/rpc/src/lib.rs +++ b/client/consensus/babe/rpc/src/lib.rs @@ -33,6 +33,7 @@ use sp_consensus_babe::{ }; use serde::{Deserialize, Serialize}; use sc_keystore::KeyStorePtr; +use sc_rpc_api::DenyUnsafe; use sp_api::{ProvideRuntimeApi, BlockId}; use sp_runtime::traits::{Block as BlockT, Header as _}; use sp_consensus::{SelectChain, Error as ConsensusError}; @@ -50,8 +51,8 @@ pub trait BabeApi { fn epoch_authorship(&self) -> FutureResult>; } -/// Implements the BabeRPC trait for interacting with Babe. -pub struct BabeRPCHandler { +/// Implements the BabeRpc trait for interacting with Babe. +pub struct BabeRpcHandler { /// shared reference to the client. client: Arc, /// shared reference to EpochChanges @@ -62,9 +63,11 @@ pub struct BabeRPCHandler { babe_config: Config, /// The SelectChain strategy select_chain: SC, + /// Whether to deny unsafe calls + deny_unsafe: DenyUnsafe, } -impl BabeRPCHandler { +impl BabeRpcHandler { /// Creates a new instance of the BabeRpc handler. pub fn new( client: Arc, @@ -72,6 +75,7 @@ impl BabeRPCHandler { keystore: KeyStorePtr, babe_config: Config, select_chain: SC, + deny_unsafe: DenyUnsafe, ) -> Self { Self { client, @@ -79,11 +83,12 @@ impl BabeRPCHandler { keystore, babe_config, select_chain, + deny_unsafe, } } } -impl BabeApi for BabeRPCHandler +impl BabeApi for BabeRpcHandler where B: BlockT, C: ProvideRuntimeApi + HeaderBackend + HeaderMetadata + 'static, @@ -92,6 +97,10 @@ impl BabeApi for BabeRPCHandler SC: SelectChain + Clone + 'static, { fn epoch_authorship(&self) -> FutureResult> { + if let Err(err) = self.deny_unsafe.check_if_safe() { + return Box::new(rpc_future::err(err.into())); + } + let ( babe_config, keystore, @@ -214,7 +223,10 @@ fn epoch_data( mod tests { use super::*; use substrate_test_runtime_client::{ + runtime::Block, + Backend, DefaultTestClientBuilderExt, + TestClient, TestClientBuilderExt, TestClientBuilder, }; @@ -236,8 +248,9 @@ mod tests { (keystore, keystore_path) } - #[test] - fn rpc() { + fn test_babe_rpc_handler( + deny_unsafe: DenyUnsafe + ) -> BabeRpcHandler> { let builder = TestClientBuilder::new(); let (client, longest_chain) = builder.build_with_longest_chain(); let client = Arc::new(client); @@ -249,9 +262,21 @@ mod tests { ).expect("can initialize block-import"); let epoch_changes = link.epoch_changes().clone(); - let select_chain = longest_chain; let keystore = create_temp_keystore::(Ed25519Keyring::Alice).0; - let handler = BabeRPCHandler::new(client.clone(), epoch_changes, keystore, config, select_chain); + + BabeRpcHandler::new( + client.clone(), + epoch_changes, + keystore, + config, + longest_chain, + deny_unsafe, + ) + } + + #[test] + fn epoch_authorship_works() { + let handler = test_babe_rpc_handler(DenyUnsafe::No); let mut io = IoHandler::new(); io.extend_with(BabeApi::to_delegate(handler)); @@ -260,4 +285,19 @@ mod tests { assert_eq!(Some(response.into()), io.handle_request_sync(request)); } + + #[test] + fn epoch_authorship_is_unsafe() { + let handler = test_babe_rpc_handler(DenyUnsafe::Yes); + let mut io = IoHandler::new(); + + io.extend_with(BabeApi::to_delegate(handler)); + let request = r#"{"jsonrpc":"2.0","method":"babe_epochAuthorship","params": [],"id":1}"#; + + let response = io.handle_request_sync(request).unwrap(); + let mut response: serde_json::Value = serde_json::from_str(&response).unwrap(); + let error: RpcError = serde_json::from_value(response["error"].take()).unwrap(); + + assert_eq!(error, RpcError::method_not_found()) + } } diff --git a/client/rpc-api/src/policy.rs b/client/rpc-api/src/policy.rs index e6e3380e1a2b8..141dcfbc415f8 100644 --- a/client/rpc-api/src/policy.rs +++ b/client/rpc-api/src/policy.rs @@ -26,21 +26,21 @@ use jsonrpc_core as rpc; /// Signifies whether a potentially unsafe RPC should be denied. #[derive(Clone, Copy, Debug)] pub enum DenyUnsafe { - /// Denies only potentially unsafe RPCs. - Yes, - /// Allows calling every RPCs. - No + /// Denies only potentially unsafe RPCs. + Yes, + /// Allows calling every RPCs. + No, } impl DenyUnsafe { - /// Returns `Ok(())` if the RPCs considered unsafe are safe to call, - /// otherwise returns `Err(UnsafeRpcError)`. - pub fn check_if_safe(self) -> Result<(), UnsafeRpcError> { - match self { - DenyUnsafe::Yes => Err(UnsafeRpcError), - DenyUnsafe::No => Ok(()) - } - } + /// Returns `Ok(())` if the RPCs considered unsafe are safe to call, + /// otherwise returns `Err(UnsafeRpcError)`. + pub fn check_if_safe(self) -> Result<(), UnsafeRpcError> { + match self { + DenyUnsafe::Yes => Err(UnsafeRpcError), + DenyUnsafe::No => Ok(()), + } + } } /// Signifies whether an RPC considered unsafe is denied to be called externally. @@ -48,15 +48,15 @@ impl DenyUnsafe { pub struct UnsafeRpcError; impl std::fmt::Display for UnsafeRpcError { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - write!(f, "RPC call is unsafe to be called externally") - } + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "RPC call is unsafe to be called externally") + } } impl std::error::Error for UnsafeRpcError {} impl From for rpc::Error { - fn from(_: UnsafeRpcError) -> rpc::Error { - rpc::Error::method_not_found() - } + fn from(_: UnsafeRpcError) -> rpc::Error { + rpc::Error::method_not_found() + } } diff --git a/client/service/src/builder.rs b/client/service/src/builder.rs index cc7929e88c6e4..16d78c49e1f45 100644 --- a/client/service/src/builder.rs +++ b/client/service/src/builder.rs @@ -96,12 +96,61 @@ pub struct ServiceBuilder, finality_proof_provider: Option, transaction_pool: Arc, - rpc_extensions: TRpc, + rpc_extensions_builder: Box + Send>, remote_backend: Option>>, marker: PhantomData<(TBl, TRtApi)>, block_announce_validator_builder: Option) -> Box + Send> + Send>>, } +/// A utility trait for building an RPC extension given a `DenyUnsafe` instance. +/// This is useful since at service definition time we don't know whether the +/// specific interface where the RPC extension will be exposed is safe or not. +/// This trait allows us to lazily build the RPC extension whenever we bind the +/// service to an interface. +pub trait RpcExtensionBuilder { + /// The type of the RPC extension that will be built. + type Output: sc_rpc::RpcExtension; + + /// Returns an instance of the RPC extension for a particular `DenyUnsafe` + /// value, e.g. the RPC extension might not expose some unsafe methods. + fn build(&self, deny: sc_rpc::DenyUnsafe) -> Self::Output; +} + +impl RpcExtensionBuilder for F where + F: Fn(sc_rpc::DenyUnsafe) -> R, + R: sc_rpc::RpcExtension, +{ + type Output = R; + + fn build(&self, deny: sc_rpc::DenyUnsafe) -> Self::Output { + (*self)(deny) + } +} + +/// A utility struct for implementing an `RpcExtensionBuilder` given a cloneable +/// `RpcExtension`, the resulting builder will simply ignore the provided +/// `DenyUnsafe` instance and return a static `RpcExtension` instance. +struct NoopRpcExtensionBuilder(R); + +impl RpcExtensionBuilder for NoopRpcExtensionBuilder where + R: Clone + sc_rpc::RpcExtension, +{ + type Output = R; + + fn build(&self, _deny: sc_rpc::DenyUnsafe) -> Self::Output { + self.0.clone() + } +} + +impl From for NoopRpcExtensionBuilder where + R: sc_rpc::RpcExtension, +{ + fn from(e: R) -> NoopRpcExtensionBuilder { + NoopRpcExtensionBuilder(e) + } +} + + /// Full client type. pub type TFullClient = Client< TFullBackend, @@ -311,7 +360,7 @@ impl ServiceBuilder<(), (), (), (), (), (), (), (), (), (), ()> { finality_proof_request_builder: None, finality_proof_provider: None, transaction_pool: Arc::new(()), - rpc_extensions: Default::default(), + rpc_extensions_builder: Box::new(|_| ()), remote_backend: None, block_announce_validator_builder: None, marker: PhantomData, @@ -394,7 +443,7 @@ impl ServiceBuilder<(), (), (), (), (), (), (), (), (), (), ()> { finality_proof_request_builder: None, finality_proof_provider: None, transaction_pool: Arc::new(()), - rpc_extensions: Default::default(), + rpc_extensions_builder: Box::new(|_| ()), remote_backend: Some(remote_blockchain), block_announce_validator_builder: None, marker: PhantomData, @@ -467,7 +516,7 @@ impl finality_proof_request_builder: self.finality_proof_request_builder, finality_proof_provider: self.finality_proof_provider, transaction_pool: self.transaction_pool, - rpc_extensions: self.rpc_extensions, + rpc_extensions_builder: self.rpc_extensions_builder, remote_backend: self.remote_backend, block_announce_validator_builder: self.block_announce_validator_builder, marker: self.marker, @@ -512,7 +561,7 @@ impl finality_proof_request_builder: self.finality_proof_request_builder, finality_proof_provider: self.finality_proof_provider, transaction_pool: self.transaction_pool, - rpc_extensions: self.rpc_extensions, + rpc_extensions_builder: self.rpc_extensions_builder, remote_backend: self.remote_backend, block_announce_validator_builder: self.block_announce_validator_builder, marker: self.marker, @@ -550,7 +599,7 @@ impl finality_proof_request_builder: self.finality_proof_request_builder, finality_proof_provider, transaction_pool: self.transaction_pool, - rpc_extensions: self.rpc_extensions, + rpc_extensions_builder: self.rpc_extensions_builder, remote_backend: self.remote_backend, block_announce_validator_builder: self.block_announce_validator_builder, marker: self.marker, @@ -616,7 +665,7 @@ impl finality_proof_request_builder: fprb, finality_proof_provider: self.finality_proof_provider, transaction_pool: self.transaction_pool, - rpc_extensions: self.rpc_extensions, + rpc_extensions_builder: self.rpc_extensions_builder, remote_backend: self.remote_backend, block_announce_validator_builder: self.block_announce_validator_builder, marker: self.marker, @@ -680,21 +729,30 @@ impl finality_proof_request_builder: self.finality_proof_request_builder, finality_proof_provider: self.finality_proof_provider, transaction_pool: Arc::new(transaction_pool), - rpc_extensions: self.rpc_extensions, + rpc_extensions_builder: self.rpc_extensions_builder, remote_backend: self.remote_backend, block_announce_validator_builder: self.block_announce_validator_builder, marker: self.marker, }) } - /// Defines the RPC extensions to use. - pub fn with_rpc_extensions( + /// Defines the RPC extension builder to use. Unlike `with_rpc_extensions`, + /// this method is useful in situations where the RPC extensions need to + /// access to a `DenyUnsafe` instance to avoid exposing sensitive methods. + pub fn with_rpc_extensions_builder( self, - rpc_ext_builder: impl FnOnce(&Self) -> Result, - ) -> Result, Error> - where TSc: Clone, TFchr: Clone { - let rpc_extensions = rpc_ext_builder(&self)?; + rpc_extensions_builder: impl FnOnce(&Self) -> Result, + ) -> Result< + ServiceBuilder, + Error, + > + where + TSc: Clone, + TFchr: Clone, + URpcBuilder: RpcExtensionBuilder + Send + 'static, + URpc: sc_rpc::RpcExtension, + { + let rpc_extensions_builder = rpc_extensions_builder(&self)?; Ok(ServiceBuilder { config: self.config, @@ -708,13 +766,30 @@ impl finality_proof_request_builder: self.finality_proof_request_builder, finality_proof_provider: self.finality_proof_provider, transaction_pool: self.transaction_pool, - rpc_extensions, + rpc_extensions_builder: Box::new(rpc_extensions_builder), remote_backend: self.remote_backend, block_announce_validator_builder: self.block_announce_validator_builder, marker: self.marker, }) } + /// Defines the RPC extensions to use. + pub fn with_rpc_extensions( + self, + rpc_extensions: impl FnOnce(&Self) -> Result, + ) -> Result< + ServiceBuilder, + Error, + > + where + TSc: Clone, + TFchr: Clone, + URpc: Clone + sc_rpc::RpcExtension + Send + 'static, + { + let rpc_extensions = rpc_extensions(&self)?; + self.with_rpc_extensions_builder(|_| Ok(NoopRpcExtensionBuilder::from(rpc_extensions))) + } + /// Defines the `BlockAnnounceValidator` to use. `DefaultBlockAnnounceValidator` will be used by /// default. pub fn with_block_announce_validator( @@ -736,7 +811,7 @@ impl finality_proof_request_builder: self.finality_proof_request_builder, finality_proof_provider: self.finality_proof_provider, transaction_pool: self.transaction_pool, - rpc_extensions: self.rpc_extensions, + rpc_extensions_builder: self.rpc_extensions_builder, remote_backend: self.remote_backend, block_announce_validator_builder: Some(Box::new(block_announce_validator_builder)), marker: self.marker, @@ -816,7 +891,7 @@ ServiceBuilder< TSc: Clone, TImpQu: 'static + ImportQueue, TExPool: MaintainedTransactionPool::Hash> + MallocSizeOfWasm + 'static, - TRpc: sc_rpc::RpcExtension + Clone, + TRpc: sc_rpc::RpcExtension, { /// Set an ExecutionExtensionsFactory @@ -854,7 +929,7 @@ ServiceBuilder< finality_proof_request_builder, finality_proof_provider, transaction_pool, - rpc_extensions, + rpc_extensions_builder, remote_backend, block_announce_validator_builder, } = self; @@ -1160,7 +1235,7 @@ ServiceBuilder< maybe_offchain_rpc, author::AuthorApi::to_delegate(author), system::SystemApi::to_delegate(system), - rpc_extensions.clone(), + rpc_extensions_builder.build(deny_unsafe), )) }; let rpc = start_rpc_servers(&config, gen_handler)?; diff --git a/client/service/src/lib.rs b/client/service/src/lib.rs index c902e6bb90737..4f2be23f877ba 100644 --- a/client/service/src/lib.rs +++ b/client/service/src/lib.rs @@ -64,7 +64,7 @@ pub use self::error::Error; pub use self::builder::{ new_full_client, new_client, ServiceBuilder, ServiceBuilderCommand, TFullClient, TLightClient, TFullBackend, TLightBackend, - TFullCallExecutor, TLightCallExecutor, + TFullCallExecutor, TLightCallExecutor, RpcExtensionBuilder, }; pub use config::{Configuration, DatabaseConfig, PruningMode, Role, RpcMethods, TaskType}; pub use sc_chain_spec::{