From 606d64eac5936a9f0bfec32b8b5a85f3dea77559 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 6 Dec 2021 13:54:53 -0500 Subject: [PATCH] Sketch API for reconfiguration. This patch doesn't actually make anything reconfigurable, but it does create an API that will tell you "you can't change the value of that!" If the API looks reasonable, I can start making it possible to change the values of individual items. --- Cargo.lock | 1 + crates/arti-client/src/client.rs | 58 +++++++++++++++++++++++++++++--- crates/arti-client/src/err.rs | 4 +++ crates/tor-circmgr/src/build.rs | 10 +++--- crates/tor-circmgr/src/lib.rs | 19 +++++++++++ crates/tor-circmgr/src/mgr.rs | 7 ++++ crates/tor-config/Cargo.toml | 11 +++--- crates/tor-config/src/err.rs | 14 +++++++- crates/tor-config/src/lib.rs | 32 +++++++++++++++++- crates/tor-dirmgr/src/config.rs | 5 +++ crates/tor-dirmgr/src/lib.rs | 27 +++++++++++++++ crates/tor-persist/src/fs.rs | 4 +++ 12 files changed, 176 insertions(+), 16 deletions(-) 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 {