Skip to content

Commit

Permalink
Problem: insecure RPCs are subject to MITM attacks
Browse files Browse the repository at this point in the history
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
  • Loading branch information
yrashk committed May 28, 2018
1 parent 863b92a commit cae13f9
Show file tree
Hide file tree
Showing 6 changed files with 38 additions and 15 deletions.
5 changes: 5 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 |
Expand Down
34 changes: 22 additions & 12 deletions bridge/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,13 @@ 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;
const DEFAULT_CONFIRMATIONS: usize = 12;
const DEFAULT_TIMEOUT: u64 = 3600;
const DEFAULT_RPC_PORT: u16 = 8545;
const DEFAULT_RPC_PORT: u16 = 443;
const DEFAULT_CONCURRENCY: usize = 100;
const DEFAULT_GAS_PRICE_SPEED: GasPriceSpeed = GasPriceSpeed::Fast;
const DEFAULT_GAS_PRICE_TIMEOUT_SECS: u64 = 10;
Expand All @@ -33,22 +33,22 @@ pub struct Config {
}

impl Config {
pub fn load<P: AsRef<Path>>(path: P) -> Result<Config, Error> {
pub fn load<P: AsRef<Path>>(path: P, allow_insecure_rpc_endpoints: bool) -> Result<Config, Error> {
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<Config, Error> {
fn load_from_str(s: &str, allow_insecure_rpc_endpoints: bool) -> Result<Config, Error> {
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<Config, Error> {
fn from_load_struct(config: load::Config, allow_insecure_rpc_endpoints: bool) -> Result<Config, Error> {
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 {
accounts: config.authorities.accounts,
required_signatures: config.authorities.required_signatures,
Expand Down Expand Up @@ -104,9 +104,9 @@ impl PartialEq for NodeInfo {
}

impl Node {
fn from_load_struct(node: load::Node) -> Result<Node, Error> {
fn from_load_struct(node: load::Node, allow_insecure_rpc_endpoints: bool) -> Result<Node, Error> {
let gas_price_oracle_url = node.gas_price_oracle_url.clone();

let gas_price_speed = match node.gas_price_speed {
Some(ref s) => GasPriceSpeed::from_str(s).unwrap(),
None => DEFAULT_GAS_PRICE_SPEED
Expand All @@ -119,6 +119,16 @@ impl Node {

let default_gas_price = node.default_gas_price.unwrap_or(DEFAULT_GAS_PRICE_WEI);

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")]
Expand All @@ -133,7 +143,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(),
Expand Down
4 changes: 4 additions & 0 deletions bridge/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,10 @@ error_chain! {
description("contextualized error")
display("{:?} in {}", err, context)
}
ConfigError(err: String) {
description("config error")
display("{}", err)
}
}
}

Expand Down
8 changes: 5 additions & 3 deletions cli/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -71,17 +71,19 @@ POA-Ethereum bridge.
Copyright 2018 POA Networks Ltd.
Usage:
bridge --config <config> --database <database>
bridge [options] --config <config> --database <database>
bridge -h | --help
Options:
-h, --help Display help message and exit.
-h, --help Display help message and exit.
--allow-insecure-rpc-endpoints Allow non-HTTPS endpoints
"#;

#[derive(Debug, Deserialize)]
pub struct Args {
arg_config: PathBuf,
arg_database: PathBuf,
flag_allow_insecure_rpc_endpoints: bool,
}

use std::sync::atomic::{AtomicBool, Ordering};
Expand Down Expand Up @@ -119,7 +121,7 @@ fn execute<S, I>(command: I, running: Arc<AtomicBool>) -> Result<String, UserFac
.and_then(|d| d.argv(command).deserialize()).map_err(|e| e.to_string())?;

info!(target: "bridge", "Loading config");
let config = Config::load(args.arg_config)?;
let config = Config::load(args.arg_config, args.flag_allow_insecure_rpc_endpoints)?;

info!(target: "bridge", "Starting event loop");
let mut event_loop = Core::new().unwrap();
Expand Down
1 change: 1 addition & 0 deletions integration-tests/tests/basic_deposit_then_withdraw.rs
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,7 @@ fn test_basic_deposit_then_withdraw() {
.env("RUST_LOG", "info")
.arg("--config").arg("bridge_config.toml")
.arg("--database").arg("tmp/bridge1_db.txt")
.arg("--allow-insecure-rpc-endpoints")
.spawn()
.expect("failed to spawn bridge process");

Expand Down
1 change: 1 addition & 0 deletions integration-tests/tests/insufficient_funds.rs
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,7 @@ fn test_insufficient_funds() {
.env("RUST_LOG", "info")
.arg("--config").arg("bridge_config_gas_price.toml")
.arg("--database").arg("tmp/bridge1_db.txt")
.arg("--allow-insecure-rpc-endpoints")
.spawn()
.expect("failed to spawn bridge process");

Expand Down

0 comments on commit cae13f9

Please sign in to comment.