diff --git a/Cargo.lock b/Cargo.lock index de3de1811..aa56c4116 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2594,6 +2594,7 @@ dependencies = [ "serde", "shellexpand", "thiserror", + "tracing", ] [[package]] diff --git a/crates/arti-client/src/client.rs b/crates/arti-client/src/client.rs index af1bdebdb..68f739bfe 100644 --- a/crates/arti-client/src/client.rs +++ b/crates/arti-client/src/client.rs @@ -43,6 +43,8 @@ pub struct TorClient { circmgr: Arc>, /// Directory manager for keeping our directory material up to date. dirmgr: Arc>, + /// Location on disk where we store persistent data. + statemgr: FsStateMgr, /// Client address configuration addrcfg: ClientAddrConfig, /// Client DNS configuration @@ -197,7 +199,7 @@ impl TorClient { runtime.spawn(update_persistent_state( runtime.clone(), Arc::downgrade(&circmgr), - statemgr, + statemgr.clone(), ))?; runtime.spawn(continually_launch_timeout_testing_circuits( @@ -219,15 +221,63 @@ impl TorClient { client_isolation, circmgr, dirmgr, + statemgr, addrcfg: addr_cfg, timeoutcfg: timeout_cfg, }) } - /// Return a new isolated `TorClient` instance. + /// Change the configuration of this TorClient to `new_config`. /// - /// The two `TorClient`s will share some internal state, but their - /// streams will never share circuits with one another. + /// The `how` describes whether to perform an all-or-nothing + /// reconfiguration: either all of the configuration changes will be applied, + /// or none will. If you have disabled all-or-nothing changes, then only + /// fatal errors will be reported in this function's return value. + /// + /// This function applies its changes to **all** TorClient instances derived + /// from the same call to [`TorClient::bootstrap`]: even ones whose circuits are + /// isolated from this handle. + /// + /// # Limitations + /// + /// At present, no options can actually be reconfigured: this function is a lie! + /// It only exists to demonstrate its intended API. + pub fn reconfigure( + &self, + new_config: &TorClientConfig, + how: tor_config::Reconfigure, + ) -> Result<()> { + match how { + tor_config::Reconfigure::AllOrNothing => { + // We have to check before we make any changes. + self.reconfigure(new_config, tor_config::Reconfigure::CheckAllOrNothing)?; + } + tor_config::Reconfigure::CheckAllOrNothing => {} + tor_config::Reconfigure::WarnOnFailures => {} + _ => {} + } + + let circ_cfg = new_config.get_circmgr_config()?; + let dir_cfg = new_config.get_dirmgr_config()?; + let state_cfg = new_config.storage.expand_state_dir()?; + let addr_cfg = &new_config.address_filter; + + if state_cfg != self.statemgr.path() { + how.cannot_change("storage.state_dir")?; + } + if &self.addrcfg != addr_cfg { + how.cannot_change("address_filter.*")?; + } + self.circmgr.reconfigure(&circ_cfg, how)?; + self.dirmgr.reconfigure(&dir_cfg, how)?; + + Ok(()) + } + + /// Return a new isolated `TorClient` handle. + /// + /// The two `TorClient`s will share internal state and configuration, but + /// their streams will never share circuits with one another. /// /// Use this function when you want separate parts of your program to /// each have a TorClient handle, but where you don't want their diff --git a/crates/arti-client/src/err.rs b/crates/arti-client/src/err.rs index e319b0c08..b11511b5c 100644 --- a/crates/arti-client/src/err.rs +++ b/crates/arti-client/src/err.rs @@ -50,6 +50,10 @@ pub enum Error { /// Building configuration for the client failed. #[error("Configuration failed: {0}")] Configuration(#[from] tor_config::ConfigBuildError), + + /// Unable to change configuration. + #[error("Reconfiguration failed: {0}")] + Reconfigure(#[from] tor_config::ReconfigureError), } impl From for Error { diff --git a/crates/tor-circmgr/src/build.rs b/crates/tor-circmgr/src/build.rs index 80377010f..ac37a2c1e 100644 --- a/crates/tor-circmgr/src/build.rs +++ b/crates/tor-circmgr/src/build.rs @@ -299,6 +299,11 @@ impl CircuitBuilder { } } + /// Return this builder's [`PathConfig`](crate::PathConfig). + pub(crate) fn path_config(&self) -> &crate::PathConfig { + &self.path_config + } + /// Flush state to the state manager. pub(crate) fn save_state(&self) -> Result<()> { // TODO: someday we'll want to only do this if there is something @@ -366,11 +371,6 @@ impl CircuitBuilder { .await } - /// Return the path configuration used by this builder. - pub(crate) fn path_config(&self) -> &crate::PathConfig { - &self.path_config - } - /// Return true if this builder is currently learning timeout info. pub(crate) fn learning_timeouts(&self) -> bool { self.builder.timeouts.learning_timeouts() diff --git a/crates/tor-circmgr/src/lib.rs b/crates/tor-circmgr/src/lib.rs index faed4ab60..9858bf33f 100644 --- a/crates/tor-circmgr/src/lib.rs +++ b/crates/tor-circmgr/src/lib.rs @@ -212,6 +212,25 @@ impl CircMgr { Ok(circmgr) } + /// Try to change our configuration settings to `new_config`. + /// + /// The actual behavior here will depend on the value of `how`. + pub fn reconfigure( + &self, + new_config: &CircMgrConfig, + how: tor_config::Reconfigure, + ) -> std::result::Result<(), tor_config::ReconfigureError> { + let path_rules = self.mgr.peek_builder().path_config(); + let circuit_timing = self.mgr.circuit_timing(); + if path_rules != &new_config.path_rules { + how.cannot_change("path_rules.*")?; + } + if circuit_timing != &new_config.circuit_timing { + how.cannot_change("circuit_timing.*")?; + } + Ok(()) + } + /// Reload state from the state manager. /// /// We only call this method if we _don't_ have the lock on the state diff --git a/crates/tor-circmgr/src/mgr.rs b/crates/tor-circmgr/src/mgr.rs index 235782094..9ce638d62 100644 --- a/crates/tor-circmgr/src/mgr.rs +++ b/crates/tor-circmgr/src/mgr.rs @@ -686,6 +686,13 @@ impl AbstractCircMgr { *u = p.into(); } + /// Return this manager's [`CircuitTiming`]. + pub(crate) fn circuit_timing(&self) -> &CircuitTiming { + &self.circuit_timing + } + + /// + /// Return a circuit suitable for use with a given `usage`, /// creating that circuit if necessary, and restricting it /// under the assumption that it will be used for that spec. diff --git a/crates/tor-config/Cargo.toml b/crates/tor-config/Cargo.toml index 72f4ba762..5311a6648 100644 --- a/crates/tor-config/Cargo.toml +++ b/crates/tor-config/Cargo.toml @@ -6,13 +6,13 @@ edition = "2018" license = "MIT OR Apache-2.0" homepage = "https://gitlab.torproject.org/tpo/core/arti/-/wikis/home" description = "Low-level configuration for the Arti Tor implementation" -keywords = [ "tor", "arti" ] -categories = [ "config" ] -repository="https://gitlab.torproject.org/tpo/core/arti.git/" +keywords = ["tor", "arti"] +categories = ["config"] +repository = "https://gitlab.torproject.org/tpo/core/arti.git/" [features] -default = [ "expand-paths" ] -expand-paths = [ "shellexpand", "directories" ] +default = ["expand-paths"] +expand-paths = ["shellexpand", "directories"] [dependencies] thiserror = "1.0.24" @@ -20,6 +20,7 @@ derive_builder = "0.10.2" once_cell = "1.7.2" serde = { version = "1.0.124", features = ["derive"] } shellexpand = { version = "2.1.0", optional = true } +tracing = "0.1.26" directories = { version = "4.0.1", optional = true } [dev-dependencies] diff --git a/crates/tor-config/src/err.rs b/crates/tor-config/src/err.rs index 6114a7a66..284406236 100644 --- a/crates/tor-config/src/err.rs +++ b/crates/tor-config/src/err.rs @@ -1,4 +1,4 @@ -//! Declare an error type. +//! Declare error types. /// An error related to an option passed to Arti via a configuration /// builder. @@ -58,6 +58,18 @@ impl ConfigBuildError { } } +/// An error caused when attempting to reconfigure an existing Arti client, or one of its modules. +#[derive(Debug, Clone, thiserror::Error)] +#[non_exhaustive] +pub enum ReconfigureError { + /// Tried to change a field that cannot change on a running client. + #[error("Cannot change {field} on a running client.")] + CannotChange { + /// The field (or fields) that we tried to change. + field: String, + }, +} + #[cfg(test)] mod test { #![allow(clippy::unwrap_used)] diff --git a/crates/tor-config/src/lib.rs b/crates/tor-config/src/lib.rs index 30c275d30..5da41e95c 100644 --- a/crates/tor-config/src/lib.rs +++ b/crates/tor-config/src/lib.rs @@ -41,5 +41,35 @@ mod err; mod path; -pub use err::ConfigBuildError; +pub use err::{ConfigBuildError, ReconfigureError}; pub use path::CfgPath; + +/// Rules for reconfiguring a running Arti instance. +#[derive(Debug, Clone, Copy, Eq, PartialEq)] +#[non_exhaustive] +pub enum Reconfigure { + /// Perform no reconfiguration unless we can guarantee that all changes will be successful. + AllOrNothing, + /// Try to reconfigure as much as possible; warn on fields that we cannot reconfigure. + WarnOnFailures, + /// Don't reconfigure anything: Only check whether we can guarantee that all changes will be successful. + CheckAllOrNothing, +} + +impl Reconfigure { + /// Called when we see a disallowed attempt to change `field`: either give a ReconfigureError, + /// or warn and return `Ok(())`, depending on the value of `self`. + pub fn cannot_change>(self, field: S) -> Result<(), ReconfigureError> { + match self { + Reconfigure::AllOrNothing | Reconfigure::CheckAllOrNothing => { + Err(ReconfigureError::CannotChange { + field: field.as_ref().to_owned(), + }) + } + Reconfigure::WarnOnFailures => { + tracing::warn!("Cannot change field {} on a running client", field.as_ref()); + Ok(()) + } + } + } +} diff --git a/crates/tor-dirmgr/src/config.rs b/crates/tor-dirmgr/src/config.rs index 1e99b3a87..a32ba71c8 100644 --- a/crates/tor-dirmgr/src/config.rs +++ b/crates/tor-dirmgr/src/config.rs @@ -230,6 +230,11 @@ impl DirMgrConfig { SqliteStore::from_path(&self.cache_path, readonly) } + /// Return the configured cache path. + pub(crate) fn cache_path(&self) -> &std::path::Path { + self.cache_path.as_ref() + } + /// Return a slice of the configured authorities pub(crate) fn authorities(&self) -> &[Authority] { self.network_config.authorities() diff --git a/crates/tor-dirmgr/src/lib.rs b/crates/tor-dirmgr/src/lib.rs index f98fdb3ce..be4e21b50 100644 --- a/crates/tor-dirmgr/src/lib.rs +++ b/crates/tor-dirmgr/src/lib.rs @@ -401,6 +401,33 @@ impl DirMgr { .ok_or(Error::NoDownloadSupport) } + /// Try to change our configuration to `new_config`. + /// + /// Actual behavior will depend on the value of `how`. + pub fn reconfigure( + &self, + new_config: &DirMgrConfig, + how: tor_config::Reconfigure, + ) -> std::result::Result<(), tor_config::ReconfigureError> { + if new_config.cache_path() != self.config.cache_path() { + how.cannot_change("storage.cache_path")?; + } + if new_config.authorities() != self.config.authorities() { + how.cannot_change("network.authorities")?; + } + if new_config.fallbacks() != self.config.fallbacks() { + how.cannot_change("network.fallback_caches")?; + } + if new_config.schedule() != self.config.schedule() { + how.cannot_change("download_schedule.*")?; + } + if new_config.override_net_params() != self.config.override_net_params() { + how.cannot_change("override_net_params.*")?; + } + + Ok(()) + } + /// Try to make this a directory manager with read-write access to its /// storage. /// diff --git a/crates/tor-persist/src/fs.rs b/crates/tor-persist/src/fs.rs index 268764ba9..de96ee4a7 100644 --- a/crates/tor-persist/src/fs.rs +++ b/crates/tor-persist/src/fs.rs @@ -82,6 +82,10 @@ impl FsStateMgr { .statepath .join(sanitize_filename::sanitize(key) + ".json") } + /// Return the top-level directory for this storage manager. + pub fn path(&self) -> &Path { + self.inner.statepath.as_ref() + } } impl StateMgr for FsStateMgr {