From 0372d24eedbef7541e12ea046abad9b33749ac4b Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 19 Nov 2021 02:11:17 -0500 Subject: [PATCH] Make arti-client config object match arti config better. Now every section that the two configuration objects share has the same type and name. This should help us in documenting our configuration in a way that doesn't confuse people. There is still lots of API work to go. --- crates/arti-client/src/client.rs | 15 ++-- crates/arti-client/src/config.rs | 120 ++++++++++++++++++++---------- crates/arti-client/src/err.rs | 2 +- crates/arti-config/src/options.rs | 42 ++--------- crates/tor-config/src/path.rs | 13 ++++ 5 files changed, 107 insertions(+), 85 deletions(-) diff --git a/crates/arti-client/src/client.rs b/crates/arti-client/src/client.rs index a7d5594cc..a986a7b11 100644 --- a/crates/arti-client/src/client.rs +++ b/crates/arti-client/src/client.rs @@ -159,16 +159,10 @@ impl TorClient { /// /// Return a client once there is enough directory material to /// connect safely over the Tor network. - pub async fn bootstrap( - runtime: R, - TorClientConfig { - state_cfg, - dir_cfg, - circ_cfg, - addr_cfg, - }: TorClientConfig, - ) -> Result> { - let statemgr = FsStateMgr::from_path(state_cfg)?; + pub async fn bootstrap(runtime: R, config: TorClientConfig) -> Result> { + let circ_cfg = config.get_circmgr_config()?; + let dir_cfg = config.get_dirmgr_config()?; + let statemgr = FsStateMgr::from_path(config.storage.expand_state_dir()?)?; if statemgr.try_lock()?.held() { debug!("It appears we have the lock on our state files."); } else { @@ -176,6 +170,7 @@ impl TorClient { "Another process has the lock on our state files. We'll proceed in read-only mode." ); } + let addr_cfg = config.address_filter.clone(); let chanmgr = Arc::new(tor_chanmgr::ChanMgr::new(runtime.clone())); let circmgr = tor_circmgr::CircMgr::new(circ_cfg, statemgr.clone(), &runtime, Arc::clone(&chanmgr))?; diff --git a/crates/arti-client/src/config.rs b/crates/arti-client/src/config.rs index c2c436691..d3503a922 100644 --- a/crates/arti-client/src/config.rs +++ b/crates/arti-client/src/config.rs @@ -2,9 +2,10 @@ //! //! Some of these are re-exported from lower-level crates. -use crate::Error; use derive_builder::Builder; use serde::Deserialize; +use std::collections::HashMap; +use std::path::Path; use std::path::PathBuf; use tor_config::CfgPath; @@ -55,16 +56,24 @@ impl Default for ClientAddrConfig { /// Configuration for where information should be stored on disk. /// /// This section is for read/write storage. -#[derive(Deserialize, Debug, Clone)] +#[derive(Deserialize, Debug, Clone, Builder)] #[serde(deny_unknown_fields)] +#[builder(build_fn(error = "ConfigBuildError"))] pub struct StorageConfig { /// Location on disk for cached directory information + #[builder(setter(into))] cache_dir: CfgPath, + #[builder(setter(into))] /// Location on disk for less-sensitive persistent state information. state_dir: CfgPath, } impl StorageConfig { + /// Return a new StorageConfigBuilder. + pub fn builder() -> StorageConfigBuilder { + StorageConfigBuilder::default() + } + /// Try to expand `state_dir` to be a path buffer. // TODO(nickm): This won't be public once we're done. pub fn expand_state_dir(&self) -> Result { @@ -105,32 +114,37 @@ impl StorageConfig { #[derive(Clone, Debug, Builder)] #[builder(build_fn(error = "ConfigBuildError"))] pub struct TorClientConfig { - /// A directory suitable for storing persistent Tor state in. - /// - /// This is distinct from the cache directory set in `dir_cfg`: - /// it is _not_ safe to delete this information regularly. - /// - /// Multiple instances of Arti may share the same state directory. - pub(crate) state_cfg: PathBuf, + /// Information about the Tor network we want to connect to. + #[builder(default)] + tor_network: dir::NetworkConfig, - /// Configuration for the network directory manager. - /// - /// This includes information on how to find and authenticate the - /// Tor network, how to frequently to retry directory downloads, - /// and where to store cached directory information. - pub(crate) dir_cfg: dir::DirMgrConfig, + /// Directories for storing information on disk + pub(crate) storage: StorageConfig, - /// Configuration for the network circuit manager. - /// - /// This includes information about how to build paths through the - /// Tor network, and how to retry failed circuits. - pub(crate) circ_cfg: circ::CircMgrConfig, + /// Information about when and how often to download directory information + download_schedule: dir::DownloadScheduleConfig, - /// Configures how the client interprets addresses on the network. - pub(crate) addr_cfg: ClientAddrConfig, + /// Facility to override network parameters from the values set in the + /// consensus. + #[builder(default)] + override_net_params: HashMap, + + /// Information about how to build paths through the network. + path_rules: circ::PathConfig, + + /// Information about how to retry and expire circuits and request for circuits. + circuit_timing: circ::CircuitTiming, + + /// Rules about which addresses the client is willing to connect to. + pub(crate) address_filter: ClientAddrConfig, } impl TorClientConfig { + /// Return a new TorClientConfigBuilder. + pub fn builder() -> TorClientConfigBuilder { + TorClientConfigBuilder::default() + } + /// Returns a `TorClientConfig` using reasonably sane defaults. /// /// This gies the same result as using `tor_config`'s definitions @@ -140,12 +154,15 @@ impl TorClientConfig { /// (On unix, this usually works out to `~/.local/share/arti` and /// `~/.cache/arti`, depending on your environment. We use the /// `directories` crate for reasonable defaults on other platforms.) - pub fn sane_defaults() -> crate::Result { + pub fn sane_defaults() -> Result { // Note: this must stay in sync with project_dirs() in the // tor-config crate. let dirs = directories::ProjectDirs::from("org", "torproject", "Arti").ok_or_else(|| { - Error::Configuration("Could not determine default directories".to_string()) + ConfigBuildError::Invalid { + field: "directories".to_string(), + problem: "Could not determine default directories".to_string(), + } })?; let state_dir = dirs.data_local_dir(); @@ -157,26 +174,49 @@ impl TorClientConfig { /// Returns a `TorClientConfig` using the specified state and cache directories. /// /// All other configuration options are set to their defaults. - pub fn with_directories(state_dir: P, cache_dir: Q) -> crate::Result + pub fn with_directories(state_dir: P, cache_dir: Q) -> Result where - P: Into, - Q: Into, + P: AsRef, + Q: AsRef, { - Ok(Self { - state_cfg: state_dir.into(), - dir_cfg: dir::DirMgrConfig::builder() - .cache_path(cache_dir.into()) - .build() - .map_err(|e| { - Error::Configuration(format!("failed to build DirMgrConfig: {}", e)) + let storage_cfg = StorageConfig::builder() + .cache_dir( + CfgPath::from_path(cache_dir).map_err(|e| ConfigBuildError::Invalid { + field: "cache_dir".to_owned(), + problem: e.to_string(), })?, - circ_cfg: Default::default(), - addr_cfg: Default::default(), - }) + ) + .state_dir( + CfgPath::from_path(state_dir).map_err(|e| ConfigBuildError::Invalid { + field: "state_dir".to_owned(), + problem: e.to_string(), + })?, + ) + .build() + .map_err(|e| e.within("storage"))?; + + Self::builder().storage(storage_cfg).build() } - /// Return a new builder to construct a `TorClientConfig`. - pub fn builder() -> TorClientConfigBuilder { - TorClientConfigBuilder::default() + /// Build a DirMgrConfig from this configuration. + pub(crate) fn get_dirmgr_config(&self) -> Result { + let mut dircfg = dir::DirMgrConfigBuilder::default(); + dircfg.network_config(self.tor_network.clone()); + dircfg.schedule_config(self.download_schedule.clone()); + dircfg.cache_path(self.storage.expand_cache_dir()?); + for (k, v) in self.override_net_params.iter() { + dircfg.override_net_param(k.clone(), *v); + } + dircfg.build() + } + + /// Return a [`CircMgrConfig`] object based on the user's selected + /// configuration. + pub(crate) fn get_circmgr_config(&self) -> Result { + let mut builder = circ::CircMgrConfigBuilder::default(); + builder + .path_rules(self.path_rules.clone()) + .circuit_timing(self.circuit_timing.clone()) + .build() } } diff --git a/crates/arti-client/src/err.rs b/crates/arti-client/src/err.rs index 569da14d7..e319b0c08 100644 --- a/crates/arti-client/src/err.rs +++ b/crates/arti-client/src/err.rs @@ -49,7 +49,7 @@ pub enum Error { /// Building configuration for the client failed. #[error("Configuration failed: {0}")] - Configuration(String), + Configuration(#[from] tor_config::ConfigBuildError), } impl From for Error { diff --git a/crates/arti-config/src/options.rs b/crates/arti-config/src/options.rs index 9263b9d7e..e659acb3a 100644 --- a/crates/arti-config/src/options.rs +++ b/crates/arti-config/src/options.rs @@ -1,8 +1,7 @@ //! Handling for arti's configuration formats. use arti_client::config::{ - circ::{CircMgrConfig, CircMgrConfigBuilder}, - dir::{DirMgrConfig, DirMgrConfigBuilder, DownloadScheduleConfig, NetworkConfig}, + dir::{DownloadScheduleConfig, NetworkConfig}, StorageConfig, TorClientConfig, }; use serde::Deserialize; @@ -94,41 +93,16 @@ pub struct ArtiConfig { } impl ArtiConfig { - /// Return a [`DirMgrConfig`] object based on the user's selected - /// configuration. - fn get_dir_config(&self) -> Result { - let mut dircfg = DirMgrConfigBuilder::default(); - dircfg.network_config(self.tor_network.clone()); - dircfg.schedule_config(self.download_schedule.clone()); - dircfg.cache_path(self.storage.expand_cache_dir()?); - for (k, v) in self.override_net_params.iter() { - dircfg.override_net_param(k.clone(), *v); - } - dircfg.build() - } - - /// Return a [`CircMgrConfig`] object based on the user's selected - /// configuration. - fn get_circ_config(&self) -> Result { - let mut builder = CircMgrConfigBuilder::default(); - builder - .path_rules(self.path_rules.clone()) - .circuit_timing(self.circuit_timing.clone()) - .build() - } - /// Construct a [`TorClientConfig`] based on this configuration. pub fn tor_client_config(&self) -> Result { - let statecfg = self.storage.expand_state_dir()?; - let dircfg = self.get_dir_config()?; - let circcfg = self.get_circ_config()?; - let addrcfg = self.address_filter.clone(); - TorClientConfig::builder() - .state_cfg(statecfg) - .dir_cfg(dircfg) - .circ_cfg(circcfg) - .addr_cfg(addrcfg) + .storage(self.storage.clone()) + .address_filter(self.address_filter.clone()) + .path_rules(self.path_rules.clone()) + .circuit_timing(self.circuit_timing.clone()) + .override_net_params(self.override_net_params.clone()) + .download_schedule(self.download_schedule.clone()) + .tor_network(self.tor_network.clone()) .build() } diff --git a/crates/tor-config/src/path.rs b/crates/tor-config/src/path.rs index b0a65da34..2476b199e 100644 --- a/crates/tor-config/src/path.rs +++ b/crates/tor-config/src/path.rs @@ -69,6 +69,19 @@ impl CfgPath { pub fn path(&self) -> Result { Ok(self.0.into()) } + + /// Construct a new `CfgPath` from a system path. + pub fn from_path>(path: P) -> Result { + // TODO(nickm) This could wind up with trouble if somebody wanted + // a path that had a shell escape inside. Maybe this type should have + // an enum inside. + Ok(Self( + path.as_ref() + .to_str() + .ok_or_else(|| CfgPathError::BadUtf8("path".to_string()))? + .to_string(), + )) + } } /// Shellexpand helper: return the user's home directory if we can.