Merge pull request #6168 from paritytech/secretstore_stresstest

SecretStore: bunch of fixes and improvements

parity/cli/config.full.toml

@@ -76,8 +76,10 @@ path = "$HOME/.parity/dapps"
 user = "test_user"
 pass = "test_pass"
 
-[secretstore]
+[secretstore]	
 disable = false
+disable_http = false
+disable_acl_check = false
 nodes = []
 http_interface = "local"
 http_port = 8082

parity/cli/mod.rs

@@ -217,6 +217,10 @@ usage! {
 		// Secret Store
 		flag_no_secretstore: bool = false,
 			or |c: &Config| otry!(c.secretstore).disable.clone(),
+		flag_no_secretstore_http: bool = false,
+			or |c: &Config| otry!(c.secretstore).disable_http.clone(),
+		flag_no_secretstore_acl_check: bool = false,
+			or |c: &Config| otry!(c.secretstore).disable_acl_check.clone(),
 		flag_secretstore_secret: Option<String> = None,
 			or |c: &Config| otry!(c.secretstore).self_secret.clone().map(Some),
 		flag_secretstore_nodes: String = "",
@@ -520,6 +524,8 @@ struct Dapps {
 #[derive(Default, Debug, PartialEq, Deserialize)]
 struct SecretStore {
 	disable: Option<bool>,
+	disable_http: Option<bool>,
+	disable_acl_check: Option<bool>,
 	self_secret: Option<String>,
 	nodes: Option<Vec<String>>,
 	interface: Option<String>,
@@ -796,6 +802,8 @@ mod tests {
 			flag_no_dapps: false,
 
 			flag_no_secretstore: false,
+			flag_no_secretstore_http: false,
+			flag_no_secretstore_acl_check: false,
 			flag_secretstore_secret: None,
 			flag_secretstore_nodes: "".into(),
 			flag_secretstore_interface: "local".into(),
@@ -1031,6 +1039,8 @@ mod tests {
 			}),
 			secretstore: Some(SecretStore {
 				disable: None,
+				disable_http: None,
+				disable_acl_check: None,
 				self_secret: None,
 				nodes: None,
 				interface: None,

parity/cli/usage.txt

@@ -234,6 +234,8 @@ API and Console Options:
 
 Secret Store Options:
   --no-secretstore                 Disable Secret Store functionality. (default: {flag_no_secretstore})
+  --no-secretstore-http            Disable Secret Store HTTP API. (default: {flag_no_secretstore_http})
+  --no-acl-check                   Disable ACL check (useful for test environments). (default: {flag_no_secretstore_acl_check})
   --secretstore-secret SECRET      Hex-encoded secret key of this node.
                                    (required, default: {flag_secretstore_secret:?}).
   --secretstore-nodes NODES        Comma-separated list of other secret store cluster nodes in form

parity/configuration.rs

@@ -609,6 +609,8 @@ impl Configuration {
 	fn secretstore_config(&self) -> Result<SecretStoreConfiguration, String> {
 		Ok(SecretStoreConfiguration {
 			enabled: self.secretstore_enabled(),
+			http_enabled: self.secretstore_http_enabled(),
+			acl_check_enabled: self.secretstore_acl_check_enabled(),
 			self_secret: self.secretstore_self_secret()?,
 			nodes: self.secretstore_nodes()?,
 			interface: self.secretstore_interface(),
@@ -1071,6 +1073,14 @@ impl Configuration {
 		!self.args.flag_no_secretstore && cfg!(feature = "secretstore")
 	}
 
+	fn secretstore_http_enabled(&self) -> bool {
+		!self.args.flag_no_secretstore_http && cfg!(feature = "secretstore")
+	}
+
+	fn secretstore_acl_check_enabled(&self) -> bool {
+		!self.args.flag_no_secretstore_acl_check
+	}
+
 	fn ui_enabled(&self) -> bool {
 		if self.args.flag_force_ui {
 			return true;
@@ -1376,6 +1386,7 @@ mod tests {
 			whisper: Default::default(),
 		};
 		expected.secretstore_conf.enabled = cfg!(feature = "secretstore");
+		expected.secretstore_conf.http_enabled = cfg!(feature = "secretstore");
 		assert_eq!(conf.into_command().unwrap().cmd, Cmd::Run(expected));
 	}
 

parity/secretstore.rs

@@ -37,6 +37,10 @@ pub enum NodeSecretKey {
 pub struct Configuration {
 	/// Is secret store functionality enabled?
 	pub enabled: bool,
+	/// Is HTTP API enabled?
+	pub http_enabled: bool,
+	/// Is ACL check enabled.
+	pub acl_check_enabled: bool,
 	/// This node secret.
 	pub self_secret: Option<NodeSecretKey>,
 	/// Other nodes IDs + addresses.
@@ -83,6 +87,7 @@ mod server {
 	use std::sync::Arc;
 	use ethcore_secretstore;
 	use ethkey::KeyPair;
+	use ansi_term::Colour::Red;
 	use super::{Configuration, Dependencies, NodeSecretKey};
 
 	/// Key server
@@ -93,6 +98,10 @@ mod server {
 	impl KeyServer {
 		/// Create new key server
 		pub fn new(mut conf: Configuration, deps: Dependencies) -> Result<Self, String> {
+			if !conf.acl_check_enabled {
+				warn!("Running SecretStore with disabled ACL check: {}", Red.bold().paint("everyone has access to stored keys"));
+			}
+
 			let self_secret: Arc<ethcore_secretstore::NodeKeyPair> = match conf.self_secret.take() {
 				Some(NodeSecretKey::Plain(secret)) => Arc::new(ethcore_secretstore::PlainNodeKeyPair::new(
 					KeyPair::from_secret(secret).map_err(|e| format!("invalid secret: {}", e))?)),
@@ -117,12 +126,14 @@ mod server {
 				None => return Err("self secret is required when using secretstore".into()),
 			};
 
-			let mut conf = ethcore_secretstore::ServiceConfiguration {
-				listener_address: ethcore_secretstore::NodeAddress {
+			let key_server_name = format!("{}:{}", conf.interface, conf.port);
+			let mut cconf = ethcore_secretstore::ServiceConfiguration {
+				listener_address: if conf.http_enabled { Some(ethcore_secretstore::NodeAddress {
 					address: conf.http_interface.clone(),
 					port: conf.http_port,
-				},
+				}) } else { None },
 				data_path: conf.data_path.clone(),
+				acl_check_enabled: conf.acl_check_enabled,
 				cluster_config: ethcore_secretstore::ClusterConfiguration {
 					threads: 4,
 					listener_address: ethcore_secretstore::NodeAddress {
@@ -137,10 +148,10 @@ mod server {
 				},
 			};
 
-			conf.cluster_config.nodes.insert(self_secret.public().clone(), conf.cluster_config.listener_address.clone());
+			cconf.cluster_config.nodes.insert(self_secret.public().clone(), cconf.cluster_config.listener_address.clone());
 
-			let key_server = ethcore_secretstore::start(deps.client, self_secret, conf)
-				.map_err(Into::<String>::into)?;
+			let key_server = ethcore_secretstore::start(deps.client, self_secret, cconf)
+				.map_err(|e| format!("Error starting KeyServer {}: {}", key_server_name, e))?;
 
 			Ok(KeyServer {
 				_key_server: key_server,
@@ -156,6 +167,8 @@ impl Default for Configuration {
 		let data_dir = default_data_path();
 		Configuration {
 			enabled: true,
+			http_enabled: true,
+			acl_check_enabled: true,
 			self_secret: None,
 			nodes: BTreeMap::new(),
 			interface: "127.0.0.1".to_owned(),

secret_store/src/acl_storage.rs

@@ -15,8 +15,9 @@
 // along with Parity.  If not, see <http://www.gnu.org/licenses/>.
 
 use std::sync::{Arc, Weak};
+use std::collections::{HashMap, HashSet};
 use futures::{future, Future};
-use parking_lot::Mutex;
+use parking_lot::{Mutex, RwLock};
 use ethkey::public_to_address;
 use ethcore::client::{Client, BlockChainClient, BlockId, ChainNotify};
 use native_contracts::SecretStoreAclStorage;
@@ -47,6 +48,12 @@ struct CachedContract {
 	contract: Option<SecretStoreAclStorage>,
 }
 
+/// Dummy ACL storage implementation (check always passed).
+#[derive(Default, Debug)]
+pub struct DummyAclStorage {
+	prohibited: RwLock<HashMap<Public, HashSet<ServerKeyId>>>,
+}
+
 impl OnChainAclStorage {
 	pub fn new(client: &Arc<Client>) -> Arc<Self> {
 		let acl_storage = Arc::new(OnChainAclStorage {
@@ -113,36 +120,22 @@ impl CachedContract {
 	}
 }
 
-#[cfg(test)]
-pub mod tests {
-	use std::collections::{HashMap, HashSet};
-	use parking_lot::RwLock;
-	use types::all::{Error, ServerKeyId, Public};
-	use super::AclStorage;
-
-	#[derive(Default, Debug)]
-	/// Dummy ACL storage implementation
-	pub struct DummyAclStorage {
-		prohibited: RwLock<HashMap<Public, HashSet<ServerKeyId>>>,
-	}
-
-	impl DummyAclStorage {
-		#[cfg(test)]
-		/// Prohibit given requestor access to given document
-		pub fn prohibit(&self, public: Public, document: ServerKeyId) {
-			self.prohibited.write()
-				.entry(public)
-				.or_insert_with(Default::default)
-				.insert(document);
-		}
+impl DummyAclStorage {
+	/// Prohibit given requestor access to given documents
+	#[cfg(test)]
+	pub fn prohibit(&self, public: Public, document: ServerKeyId) {
+		self.prohibited.write()
+			.entry(public)
+			.or_insert_with(Default::default)
+			.insert(document);
 	}
+}
 
-	impl AclStorage for DummyAclStorage {
-		fn check(&self, public: &Public, document: &ServerKeyId) -> Result<bool, Error> {
-			Ok(self.prohibited.read()
-				.get(public)
-				.map(|docs| !docs.contains(document))
-				.unwrap_or(true))
-		}
+impl AclStorage for DummyAclStorage {
+	fn check(&self, public: &Public, document: &ServerKeyId) -> Result<bool, Error> {
+		Ok(self.prohibited.read()
+			.get(public)
+			.map(|docs| !docs.contains(document))
+			.unwrap_or(true))
 	}
 }

secret_store/src/http_listener.rs

@@ -39,7 +39,7 @@ use types::all::{Error, Public, MessageHash, EncryptedMessageSignature, NodeAddr
 /// To sign message with server key:				GET			/{server_key_id}/{signature}/{message_hash}
 
 pub struct KeyServerHttpListener<T: KeyServer + 'static> {
-	_http_server: HttpListening,
+	http_server: Option<HttpListening>,
 	handler: Arc<KeyServerSharedHttpHandler<T>>,
 }
 
@@ -74,19 +74,20 @@ struct KeyServerSharedHttpHandler<T: KeyServer + 'static> {
 
 impl<T> KeyServerHttpListener<T> where T: KeyServer + 'static {
 	/// Start KeyServer http listener
-	pub fn start(listener_address: &NodeAddress, key_server: T) -> Result<Self, Error> {
+	pub fn start(listener_address: Option<NodeAddress>, key_server: T) -> Result<Self, Error> {
 		let shared_handler = Arc::new(KeyServerSharedHttpHandler {
 			key_server: key_server,
 		});
-		let handler = KeyServerHttpHandler {
-			handler: shared_handler.clone(),
-		};
 
-		let listener_addr: &str = &format!("{}:{}", listener_address.address, listener_address.port);
-		let http_server = HttpServer::http(&listener_addr).expect("cannot start HttpServer");
-		let http_server = http_server.handle(handler).expect("cannot start HttpServer");
+		let http_server = listener_address
+			.map(|listener_address| format!("{}:{}", listener_address.address, listener_address.port))
+			.map(|listener_address| HttpServer::http(&listener_address).expect("cannot start HttpServer"))
+			.map(|http_server| http_server.handle(KeyServerHttpHandler {
+				handler: shared_handler.clone(),
+			}).expect("cannot start HttpServer"));
+
 		let listener = KeyServerHttpListener {
-			_http_server: http_server,
+			http_server: http_server,
 			handler: shared_handler,
 		};
 		Ok(listener)
@@ -128,7 +129,7 @@ impl <T> MessageSigner for KeyServerHttpListener<T> where T: KeyServer + 'static
 impl<T> Drop for KeyServerHttpListener<T> where T: KeyServer + 'static {
 	fn drop(&mut self) {
 		// ignore error as we are dropping anyway
-		let _ = self._http_server.close();
+		self.http_server.take().map(|mut s| { let _ = s.close(); });
 	}
 }
 
@@ -318,7 +319,7 @@ mod tests {
 	fn http_listener_successfully_drops() {
 		let key_server = DummyKeyServer;
 		let address = NodeAddress { address: "127.0.0.1".into(), port: 9000 };
-		let listener = KeyServerHttpListener::start(&address, key_server).unwrap();
+		let listener = KeyServerHttpListener::start(Some(address), key_server).unwrap();
 		drop(listener);
 	}
 

secret_store/src/key_server.rs

@@ -196,7 +196,7 @@ pub mod tests {
 	use std::collections::BTreeMap;
 	use ethcrypto;
 	use ethkey::{self, Secret, Random, Generator};
-	use acl_storage::tests::DummyAclStorage;
+	use acl_storage::DummyAclStorage;
 	use key_storage::tests::DummyKeyStorage;
 	use node_key_pair::PlainNodeKeyPair;
 	use key_server_set::tests::MapKeyServerSet;

secret_store/src/key_server_cluster/cluster.rs

@@ -262,7 +262,7 @@ impl ClusterCore {
 	fn connect_future(handle: &Handle, data: Arc<ClusterData>, node_address: SocketAddr) -> BoxedEmptyFuture {
 		let disconnected_nodes = data.connections.disconnected_nodes().keys().cloned().collect();
 		net_connect(&node_address, handle, data.self_key_pair.clone(), disconnected_nodes)
-			.then(move |result| ClusterCore::process_connection_result(data, false, result))
+			.then(move |result| ClusterCore::process_connection_result(data, Some(node_address), result))
 			.then(|_| finished(()))
 			.boxed()
 	}
@@ -290,7 +290,7 @@ impl ClusterCore {
 	/// Accept connection future.
 	fn accept_connection_future(handle: &Handle, data: Arc<ClusterData>, stream: TcpStream, node_address: SocketAddr) -> BoxedEmptyFuture {
 		net_accept_connection(node_address, stream, handle, data.self_key_pair.clone())
-			.then(move |result| ClusterCore::process_connection_result(data, true, result))
+			.then(move |result| ClusterCore::process_connection_result(data, None, result))
 			.then(|_| finished(()))
 			.boxed()
 	}
@@ -370,26 +370,32 @@ impl ClusterCore {
 	}
 
 	/// Process connection future result.
-	fn process_connection_result(data: Arc<ClusterData>, is_inbound: bool, result: Result<DeadlineStatus<Result<NetConnection, Error>>, io::Error>) -> IoFuture<Result<(), Error>> {
+	fn process_connection_result(data: Arc<ClusterData>, outbound_addr: Option<SocketAddr>, result: Result<DeadlineStatus<Result<NetConnection, Error>>, io::Error>) -> IoFuture<Result<(), Error>> {
 		match result {
 			Ok(DeadlineStatus::Meet(Ok(connection))) => {
-				let connection = Connection::new(is_inbound, connection);
+				let connection = Connection::new(outbound_addr.is_none(), connection);
 				if data.connections.insert(connection.clone()) {
 					ClusterCore::process_connection_messages(data.clone(), connection)
 				} else {
 					finished(Ok(())).boxed()
 				}
 			},
 			Ok(DeadlineStatus::Meet(Err(err))) => {
-				warn!(target: "secretstore_net", "{}: protocol error {} when establishind connection", data.self_key_pair.public(), err);
+				warn!(target: "secretstore_net", "{}: protocol error {} when establishing {} connection{}",
+					data.self_key_pair.public(), err, if outbound_addr.is_some() { "outbound" } else { "inbound" },
+					outbound_addr.map(|a| format!(" with {}", a)).unwrap_or_default());
 				finished(Ok(())).boxed()
 			},
 			Ok(DeadlineStatus::Timeout) => {
-				warn!(target: "secretstore_net", "{}: timeout when establishind connection", data.self_key_pair.public());
+				warn!(target: "secretstore_net", "{}: timeout when establishing {} connection{}",
+					data.self_key_pair.public(), if outbound_addr.is_some() { "outbound" } else { "inbound" },
+					outbound_addr.map(|a| format!(" with {}", a)).unwrap_or_default());
 				finished(Ok(())).boxed()
 			},
 			Err(err) => {
-				warn!(target: "secretstore_net", "{}: network error {} when establishind connection", data.self_key_pair.public(), err);
+				warn!(target: "secretstore_net", "{}: network error {} when establishing {} connection{}",
+					data.self_key_pair.public(), err, if outbound_addr.is_some() { "outbound" } else { "inbound" },
+					outbound_addr.map(|a| format!(" with {}", a)).unwrap_or_default());
 				finished(Ok(())).boxed()
 			},
 		}

secret_store/src/key_server_cluster/decryption_session.rs

@@ -467,7 +467,7 @@ impl Ord for DecryptionSessionId {
 mod tests {
 	use std::sync::Arc;
 	use std::collections::BTreeMap;
-	use super::super::super::acl_storage::tests::DummyAclStorage;
+	use acl_storage::DummyAclStorage;
 	use ethkey::{self, KeyPair, Random, Generator, Public, Secret};
 	use key_server_cluster::{NodeId, DocumentKeyShare, SessionId, Error, EncryptedDocumentKeyShadow, SessionMeta};
 	use key_server_cluster::cluster::tests::DummyCluster;

secret_store/src/key_server_cluster/generation_session.rs

@@ -399,7 +399,7 @@ impl SessionImpl {
 		// check state
 		if data.state != SessionState::WaitingForKeysDissemination {
 			match data.state {
-				SessionState::WaitingForInitializationComplete => return Err(Error::TooEarlyForRequest),
+				SessionState::WaitingForInitializationComplete | SessionState::WaitingForInitializationConfirm(_) => return Err(Error::TooEarlyForRequest),
 				_ => return Err(Error::InvalidStateForRequest),
 			}
 		}
@@ -1104,7 +1104,7 @@ pub mod tests {
 			secret1: math::generate_random_scalar().unwrap().into(),
 			secret2: math::generate_random_scalar().unwrap().into(),
 			publics: vec![math::generate_random_point().unwrap().into()],
-		}).unwrap_err(), Error::InvalidStateForRequest);
+		}).unwrap_err(), Error::TooEarlyForRequest);
 	}
 
 	#[test]

secret_store/src/key_server_cluster/mod.rs

@@ -36,7 +36,7 @@ pub use super::node_key_pair::PlainNodeKeyPair;
 #[cfg(test)]
 pub use super::key_storage::tests::DummyKeyStorage;
 #[cfg(test)]
-pub use super::acl_storage::tests::DummyAclStorage;
+pub use super::acl_storage::DummyAclStorage;
 #[cfg(test)]
 pub use super::key_server_set::tests::MapKeyServerSet;