From 18802df14c40f7e5ffbd9a01db266f4354e970ca Mon Sep 17 00:00:00 2001 From: Yurii Rashkovskii Date: Wed, 23 May 2018 23:03:20 -0700 Subject: [PATCH] Problem: insecure RPCs are subject to MITM attacks Solution: by default, disallow use of non-TLS RPC endpoints For testing, there's an escape hatch of a command line argument `--allow-insecure-rpc-endpoints` (purposefully long) that will reduce the severity of using a non-TLS RPC endpoint to a warning in a log file. It was not made to be a configuration file option to reduce the risk of this option slipping into a production configuration file by mistake. Closes #79 --- README.md | 5 +++ bridge/src/config.rs | 34 ++++++++++++------- bridge/src/error.rs | 4 +++ cli/src/main.rs | 10 +++--- .../tests/basic_deposit_then_withdraw.rs | 1 + integration-tests/tests/insufficient_funds.rs | 1 + 6 files changed, 39 insertions(+), 16 deletions(-) diff --git a/README.md b/README.md index b9fcbe3..35f7fc4 100644 --- a/README.md +++ b/README.md @@ -54,6 +54,11 @@ bridge --config config.toml --database db.toml - `--config` - location of the configuration file. configuration file must exist - `--database` - location of the database file. +Bridge forces TLS for RPC connections by default. However, in some limited scenarios (like local testing), +this might be undesirable. In this case, you can use `--allow-insecure-rpc-endpoints` option to allow non-TLS +endpoints to be used. Ensure, however, that this option is not going to be used in production. + + #### Exit Status Codes | Code | Meaning | diff --git a/bridge/src/config.rs b/bridge/src/config.rs index 3a75f8c..b092b28 100644 --- a/bridge/src/config.rs +++ b/bridge/src/config.rs @@ -8,7 +8,7 @@ use rustc_hex::FromHex; use web3::types::Address; #[cfg(feature = "deploy")] use web3::types::Bytes; -use error::{ResultExt, Error}; +use error::{ResultExt, Error, ErrorKind}; use {toml}; const DEFAULT_POLL_INTERVAL: u64 = 1; @@ -33,22 +33,22 @@ pub struct Config { } impl Config { - pub fn load>(path: P) -> Result { + pub fn load>(path: P, allow_insecure_rpc_endpoints: bool) -> Result { let mut file = fs::File::open(path).chain_err(|| "Cannot open config")?; let mut buffer = String::new(); file.read_to_string(&mut buffer).expect("TODO"); - Self::load_from_str(&buffer) + Self::load_from_str(&buffer, allow_insecure_rpc_endpoints) } - fn load_from_str(s: &str) -> Result { + fn load_from_str(s: &str, allow_insecure_rpc_endpoints: bool) -> Result { let config: load::Config = toml::from_str(s).chain_err(|| "Cannot parse config")?; - Config::from_load_struct(config) + Config::from_load_struct(config, allow_insecure_rpc_endpoints) } - fn from_load_struct(config: load::Config) -> Result { + fn from_load_struct(config: load::Config, allow_insecure_rpc_endpoints: bool) -> Result { let result = Config { - home: Node::from_load_struct(config.home)?, - foreign: Node::from_load_struct(config.foreign)?, + home: Node::from_load_struct(config.home, allow_insecure_rpc_endpoints)?, + foreign: Node::from_load_struct(config.foreign, allow_insecure_rpc_endpoints)?, authorities: Authorities { #[cfg(feature = "deploy")] accounts: config.authorities.accounts, @@ -106,7 +106,7 @@ impl PartialEq for NodeInfo { } impl Node { - fn from_load_struct(node: load::Node) -> Result { + fn from_load_struct(node: load::Node, allow_insecure_rpc_endpoints: bool) -> Result { let gas_price_oracle_url = node.gas_price_oracle_url.clone(); let gas_price_speed = match node.gas_price_speed { @@ -122,6 +122,16 @@ impl Node { let default_gas_price = node.default_gas_price.unwrap_or(DEFAULT_GAS_PRICE_WEI); let concurrent_http_requests = node.concurrent_http_requests.unwrap_or(DEFAULT_CONCURRENCY); + let rpc_host = node.rpc_host.unwrap(); + + if !rpc_host.starts_with("https://") { + if !allow_insecure_rpc_endpoints { + return Err(ErrorKind::ConfigError(format!("RPC endpoints must use TLS, {} doesn't", rpc_host)).into()); + } else { + warn!("RPC endpoints must use TLS, {} doesn't", rpc_host); + } + } + let result = Node { account: node.account, #[cfg(feature = "deploy")] @@ -136,7 +146,7 @@ impl Node { request_timeout: Duration::from_secs(node.request_timeout.unwrap_or(DEFAULT_TIMEOUT)), poll_interval: Duration::from_secs(node.poll_interval.unwrap_or(DEFAULT_POLL_INTERVAL)), required_confirmations: node.required_confirmations.unwrap_or(DEFAULT_CONFIRMATIONS), - rpc_host: node.rpc_host.unwrap(), + rpc_host, rpc_port: node.rpc_port.unwrap_or(DEFAULT_RPC_PORT), password: node.password, info: Default::default(), @@ -398,7 +408,7 @@ required_signatures = 2 keystore: "/keys/".into(), }; - let config = Config::load_from_str(toml).unwrap(); + let config = Config::load_from_str(toml, true).unwrap(); assert_eq!(expected, config); } @@ -461,7 +471,7 @@ required_signatures = 2 keystore: "/keys/".into(), }; - let config = Config::load_from_str(toml).unwrap(); + let config = Config::load_from_str(toml, true).unwrap(); assert_eq!(expected, config); } } diff --git a/bridge/src/error.rs b/bridge/src/error.rs index 63209c0..ce69d0c 100644 --- a/bridge/src/error.rs +++ b/bridge/src/error.rs @@ -61,6 +61,10 @@ error_chain! { OtherError(error: String) { description("other error") display("{}", error) + } + ConfigError(err: String) { + description("config error") + display("{}", err) } } } diff --git a/cli/src/main.rs b/cli/src/main.rs index a24b24c..3fbf22d 100644 --- a/cli/src/main.rs +++ b/cli/src/main.rs @@ -73,13 +73,14 @@ POA-Ethereum bridge. Copyright 2018 POA Networks Ltd. Usage: - bridge --config --database + bridge [options] --config --database bridge -h | --help bridge -v | --version Options: - -h, --help Display help message and exit. - -v, --version Print version and exit. + -h, --help Display help message and exit. + -v, --version Print version and exit. + --allow-insecure-rpc-endpoints Allow non-HTTPS endpoints "#; #[derive(Debug, Deserialize)] @@ -87,6 +88,7 @@ pub struct Args { arg_config: PathBuf, arg_database: PathBuf, flag_version: bool, + flag_allow_insecure_rpc_endpoints: bool, } use std::sync::atomic::{AtomicBool, Ordering}; @@ -128,7 +130,7 @@ fn execute(command: I, running: Arc) -> Result