diff --git a/Cargo.lock b/Cargo.lock index f9300bcff..5e09d63c8 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3968,6 +3968,7 @@ name = "tor-hsclient" version = "0.1.1" dependencies = [ "async-trait", + "educe", "futures", "postage", "rand_core 0.6.4", diff --git a/crates/tor-hsclient/Cargo.toml b/crates/tor-hsclient/Cargo.toml index 5be80250d..9ed28bc56 100644 --- a/crates/tor-hsclient/Cargo.toml +++ b/crates/tor-hsclient/Cargo.toml @@ -18,6 +18,7 @@ default = [] [dependencies] async-trait = "0.1.2" +educe = "0.4.6" futures = "0.3.14" postage = { version = "0.5.0", default-features = false, features = ["futures-traits"] } rand_core = "0.6.2" diff --git a/crates/tor-hsclient/src/connect.rs b/crates/tor-hsclient/src/connect.rs index bddaa29ea..4caefa69a 100644 --- a/crates/tor-hsclient/src/connect.rs +++ b/crates/tor-hsclient/src/connect.rs @@ -2,15 +2,20 @@ //use std::time::SystemTime; +use async_trait::async_trait; + use tor_proto::circuit::ClientCirc; use tor_rtcompat::Runtime; +use crate::state::MockableConnectorData; use crate::{HsClientConnError, HsClientConnector, HsClientSecretKeys}; /// Information about a hidden service, including our connection history #[allow(dead_code, unused_variables)] // TODO hs remove. #[derive(Default, Debug)] -pub(crate) struct Data { +// This type is actually crate-private, since it isn't re-exported, but it must +// be `pub` because it appears as a default for a type parameter in HsClientConnector. +pub struct Data { // /// A time when we should check whether this descriptor is still the latest. // desc_fresh_until: SystemTime, // /// A time when we should expire this entry completely. @@ -47,3 +52,21 @@ pub(crate) async fn connect( // - Return the rendezvous circuit. todo!() } + +#[async_trait] +impl MockableConnectorData for Data { + type ClientCirc = ClientCirc; + type MockGlobalState = (); + + async fn connect( + connector: &HsClientConnector, + data: &mut Self, + secret_keys: HsClientSecretKeys, + ) -> Result { + connect(connector, data, secret_keys).await + } + + fn circuit_is_ok(circuit: &Self::ClientCirc) -> bool { + !circuit.is_closing() + } +} diff --git a/crates/tor-hsclient/src/lib.rs b/crates/tor-hsclient/src/lib.rs index d3dd51654..b8806b351 100644 --- a/crates/tor-hsclient/src/lib.rs +++ b/crates/tor-hsclient/src/lib.rs @@ -46,6 +46,7 @@ mod state; use std::sync::{Arc, Mutex}; use async_trait::async_trait; +use educe::Educe; use tor_circmgr::isolation::Isolation; use tor_circmgr::{CircMgr, OnionConnectError, OnionServiceConnector}; @@ -60,8 +61,9 @@ pub use keys::{HsClientSecretKeys, HsClientSecretKeysBuilder}; use state::Services; /// An object that negotiates connections with onion services -#[derive(Clone)] -pub struct HsClientConnector { +#[derive(Educe)] +#[educe(Clone)] +pub struct HsClientConnector { /// The runtime runtime: R, /// A [`CircMgr`] that we use to build circuits to HsDirs, introduction @@ -83,10 +85,12 @@ pub struct HsClientConnector { // // TODO hs: if we implement cache isolation or state isolation, we might // need multiple instances of this. - services: Arc>, + services: Arc>>, + /// For mocking in tests of `state.rs` + mock_for_state: D::MockGlobalState, } -impl HsClientConnector { +impl HsClientConnector { /// Create a new `HsClientConnector` pub fn new( runtime: R, @@ -100,6 +104,7 @@ impl HsClientConnector { circmgr, netdir_provider, services: Arc::new(Mutex::new(Services::default())), + mock_for_state: (), }) } diff --git a/crates/tor-hsclient/src/state.rs b/crates/tor-hsclient/src/state.rs index 20a62818c..0877cc7b7 100644 --- a/crates/tor-hsclient/src/state.rs +++ b/crates/tor-hsclient/src/state.rs @@ -2,6 +2,7 @@ //! about onion service history. use std::collections::HashMap; +use std::fmt::Debug; use std::mem; use std::panic::AssertUnwindSafe; use std::sync::{Arc, Mutex}; @@ -9,6 +10,7 @@ use std::time::Instant; use futures::FutureExt as _; +use async_trait::async_trait; use postage::stream::Stream as _; use slotmap::dense::DenseSlotMap; use tracing::{debug, error}; @@ -16,10 +18,8 @@ use tracing::{debug, error}; use tor_circmgr::isolation::Isolation; use tor_error::{internal, Bug, ErrorReport as _}; use tor_hscrypto::pk::HsId; -use tor_proto::circuit::ClientCirc; use tor_rtcompat::Runtime; -use crate::connect::{self, Data}; use crate::{HsClientConnError, HsClientConnector, HsClientSecretKeys}; slotmap::new_key_type! { @@ -54,7 +54,7 @@ const MAX_ATTEMPTS: u32 = 10; /// linear search |________________| | ... .... | /// ``` |________________________| #[derive(Default)] -pub(crate) struct Services { +pub(crate) struct Services { /// Index, mapping key to entry in the data tble /// /// There's a HashMap from HsId. @@ -69,7 +69,7 @@ pub(crate) struct Services { /// place to put its results, without having to re-traverse the data structure. /// It also doesn't need a complete copy of the data structure key, /// nor to get involved with `Isolation` edge cases. - table: DenseSlotMap, + table: DenseSlotMap>, } /// Entry in the 2nd-level lookup array @@ -90,11 +90,11 @@ struct IndexRecord { // TODO HS actually expire old data // // TODO unify this with channels and circuits. See arti#778. -enum ServiceState { +enum ServiceState { /// We don't have a circuit Closed { /// The state - data: Data, + data: D, /// Last time we touched this, including reuse #[allow(dead_code)] // TODO hs remove, when we do expiry last_used: Instant, @@ -102,9 +102,9 @@ enum ServiceState { /// We have an open circuit, which we can (hopefully) just use Open { /// The state - data: Data, + data: D, /// The circuit - circuit: ClientCirc, + circuit: D::ClientCirc, /// Last time we touched this, including reuse last_used: Instant, }, @@ -123,23 +123,23 @@ enum ServiceState { Dummy, } -impl Services { +impl Services { /// Connect to a hidden service // We *do* drop guard. There is *one* await point, just after drop(guard). #[allow(clippy::await_holding_lock)] pub(crate) async fn get_or_launch_connection( - connector: &HsClientConnector, + connector: &HsClientConnector, hs_id: HsId, isolation: Box, secret_keys: HsClientSecretKeys, - ) -> Result { + ) -> Result { let mut guard = connector.services.lock() .map_err(|_| internal!("HS connector poisoned"))?; let services = &mut *guard; let records = services.index.entry(hs_id).or_default(); let blank_state = || ServiceState::Closed { - data: Data::default(), + data: D::default(), last_used: connector.runtime.now(), }; @@ -176,7 +176,7 @@ impl Services { let (data, barrier_send) = match state { ServiceState::Open { data:_, circuit, last_used } => { let now = connector.runtime.now(); - if circuit.is_closing() { + if !D::circuit_is_ok(circuit) { // Well that's no good, we need a fresh one, but keep the data let data = match mem::replace(state, ServiceState::Dummy) { ServiceState::Open { data, last_used: _, circuit: _ } => data, @@ -231,18 +231,18 @@ impl Services { // Make a connection attempt let runtime = &connector.runtime; - let connector = connector.clone(); + let connector = (*connector).clone(); let secret_keys = secret_keys.clone(); let connect_future = async move { let mut data = data; let got = AssertUnwindSafe( - connect::connect(&connector, &mut data, secret_keys) + D::connect(&connector, &mut data, secret_keys) ) .catch_unwind() .await .unwrap_or_else(|_| { - data = Data::default(); + data = D::default(); Err(internal!("hidden service connector task panicked!").into()) }); let last_used = connector.runtime.now(); @@ -311,3 +311,30 @@ impl Services { Err(internal!("HS connector state management malfunction (exceeded MAX_ATTEMPTS").into()) } } + +/// Mocking for actual HS connection work, to let us test the `Services` state machine +// +// Does *not* mock circmgr, chanmgr, etc. - those won't be used by the tests, since our +// `connect` won't call them. But mocking them pollutes many types with `R` and is +// generally tiresome. So let's not. Instead the tests can make dummy ones. +// +// This trait is actually crate-private, since it isn't re-exported, but it must +// be `pub` because it appears as a default for a type parameter in HsClientConnector. +#[async_trait] +pub trait MockableConnectorData: Default + Debug + Send + Sync + 'static { + /// Client circuit + type ClientCirc: Clone + Sync + Send + 'static; + + /// Mock state + type MockGlobalState: Clone + Sync + Send + 'static; + + /// Connect + async fn connect( + connector: &HsClientConnector, + data: &mut Self, + secret_keys: HsClientSecretKeys, + ) -> Result; + + /// Is circuit OK? Ie, not `.is_closing()`. + fn circuit_is_ok(circuit: &Self::ClientCirc) -> bool; +}