Merge branch 'safelogging' into 'main'

Implement a safe-logging facility.

Closes #189

See merge request tpo/core/arti!485

Cargo.lock

@@ -86,6 +86,7 @@ dependencies = [
  "notify",
  "once_cell",
  "rlimit",
+ "safelog",
  "serde",
  "tokio",
  "tor-config",
@@ -135,6 +136,7 @@ dependencies = [
  "once_cell",
  "pin-project",
  "postage",
+ "safelog",
  "serde",
  "tempfile",
  "thiserror",
@@ -1158,6 +1160,12 @@ version = "0.7.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "c1d53499e94f9a7828e63c574adf62bcade7f358c3738f9ea70d7c2edb61023d"
 
+[[package]]
+name = "fluid-let"
+version = "1.0.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "749cff877dc1af878a0b31a41dd221a753634401ea0ef2f87b62d3171522485a"
+
 [[package]]
 name = "fnv"
 version = "1.0.7"
@@ -2353,6 +2361,30 @@ version = "0.2.16"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "eb9f9e6e233e5c4a35559a617bf40a4ec447db2e84c20b55a6f83167b7e57872"
 
+[[package]]
+name = "proc-macro-error"
+version = "1.0.4"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "da25490ff9892aab3fcf7c36f08cfb902dd3e71ca0f9f9517bea02a73a5ce38c"
+dependencies = [
+ "proc-macro-error-attr",
+ "proc-macro2",
+ "quote",
+ "syn",
+ "version_check",
+]
+
+[[package]]
+name = "proc-macro-error-attr"
+version = "1.0.4"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "a1be40180e52ecc98ad80b184934baf3d0d29f979574e439af5a55274b35f869"
+dependencies = [
+ "proc-macro2",
+ "quote",
+ "version_check",
+]
+
 [[package]]
 name = "proc-macro-hack"
 version = "0.5.19"
@@ -2653,6 +2685,18 @@ version = "1.0.9"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "73b4b750c782965c211b42f022f59af1fbceabdd026623714f104152f1ec149f"
 
+[[package]]
+name = "safelog"
+version = "0.2.0"
+dependencies = [
+ "educe",
+ "fluid-let",
+ "serde",
+ "serial_test",
+ "static_assertions",
+ "thiserror",
+]
+
 [[package]]
 name = "same-file"
 version = "1.0.6"
@@ -2767,6 +2811,30 @@ dependencies = [
  "serde",
 ]
 
+[[package]]
+name = "serial_test"
+version = "0.6.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "e5bcc41d18f7a1d50525d080fd3e953be87c4f9f1a974f3c21798ca00d54ec15"
+dependencies = [
+ "lazy_static",
+ "parking_lot 0.11.2",
+ "serial_test_derive",
+]
+
+[[package]]
+name = "serial_test_derive"
+version = "0.6.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "2881bccd7d60fb32dfa3d7b3136385312f8ad75e2674aab2852867a09790cae8"
+dependencies = [
+ "proc-macro-error",
+ "proc-macro2",
+ "quote",
+ "rustversion",
+ "syn",
+]
+
 [[package]]
 name = "sha-1"
 version = "0.10.0"
@@ -3308,6 +3376,7 @@ dependencies = [
  "pin-project",
  "rand 0.8.5",
  "retry-error",
+ "safelog",
  "serde",
  "static_assertions",
  "thiserror",

Cargo.toml

@@ -9,6 +9,7 @@ members = [
     "crates/tor-basic-utils",
     "crates/caret",
     "crates/fs-mistrust",
+    "crates/safelog",
     "crates/retry-error",
     "crates/tor-error",
     "crates/tor-config",

crates/arti-client/Cargo.toml

@@ -31,6 +31,7 @@ error_detail = []
 experimental-api = []
 
 [dependencies]
+safelog = { path = "../safelog", version = "0.2.0" }
 tor-basic-utils = { path = "../tor-basic-utils", version = "0.2.0" }
 tor-circmgr = { path = "../tor-circmgr", version = "0.2.0" }
 tor-config = { path = "../tor-config", version = "0.2.0" }

crates/arti-client/src/client.rs

@@ -7,6 +7,7 @@
 use crate::address::IntoTorAddr;
 
 use crate::config::{ClientAddrConfig, StreamTimeoutConfig, TorClientConfig};
+use safelog::sensitive;
 use tor_circmgr::isolation::Isolation;
 use tor_circmgr::{isolation::StreamIsolationBuilder, IsolationToken, TargetPort};
 use tor_config::MutCfg;
@@ -675,7 +676,7 @@ impl<R: Runtime> TorClient<R> {
             .get_or_launch_exit_circ(&exit_ports, prefs)
             .await
             .map_err(wrap_err)?;
-        info!("Got a circuit for {}:{}", addr, port);
+        info!("Got a circuit for {}:{}", sensitive(&addr), port);
 
         let stream_future = circ.begin_stream(&addr, port, Some(prefs.stream_parameters()));
         // This timeout is needless but harmless for optimistic streams.

crates/arti/Cargo.toml

@@ -23,6 +23,7 @@ static-native-tls = ["arti-client/static-native-tls", "native-tls"]
 journald = ["tracing-journald"]
 
 [dependencies]
+safelog = { path = "../safelog", version = "0.2.0" }
 arti-client = { package = "arti-client", path = "../arti-client", version = "0.2.0", default-features = false }
 tor-config = { path = "../tor-config", version = "0.2.0" }
 tor-error = { path = "../tor-error", version = "0.2.0", default-features = false }

crates/arti/src/lib.rs

@@ -130,6 +130,7 @@ pub use logging::{LoggingConfig, LoggingConfigBuilder};
 
 use arti_client::{TorClient, TorClientConfig};
 use arti_config::default_config_file;
+use safelog::with_safe_logging_suppressed;
 use tor_rtcompat::{BlockOn, Runtime};
 
 use anyhow::{Context, Result};
@@ -364,5 +365,5 @@ pub fn main_main() -> Result<()> {
 
 /// Main program, callable directly from a binary crate's `main`
 pub fn main() {
-    main_main().unwrap_or_else(tor_error::report_and_exit);
+    main_main().unwrap_or_else(|e| with_safe_logging_suppressed(|| tor_error::report_and_exit(e)));
 }

crates/arti/src/logging.rs

@@ -7,7 +7,7 @@ use std::path::Path;
 use std::str::FromStr;
 use tor_config::{define_list_builder_accessors, define_list_builder_helper};
 use tor_config::{CfgPath, ConfigBuildError};
-use tracing::Subscriber;
+use tracing::{warn, Subscriber};
 use tracing_appender::non_blocking::WorkerGuard;
 use tracing_subscriber::layer::SubscriberExt;
 use tracing_subscriber::prelude::*;
@@ -40,6 +40,16 @@ pub struct LoggingConfig {
     #[builder_field_attr(serde(default))]
     #[builder(sub_builder, setter(custom))]
     files: LogfileListConfig,
+
+    /// If set to true, we disable safe logging on _all logs_, and store
+    /// potentially sensitive information at level `info` or higher.
+    ///
+    /// This can be useful for debugging, but it increases the value of your
+    /// logs to an attacker.  Do not turn this on in production unless you have
+    /// a good log rotation mechanism.
+    #[builder_field_attr(serde(default))]
+    #[builder(default)]
+    log_sensitive_information: bool,
 }
 
 /// Return a default tracing filter value for `logging.console`.
@@ -135,6 +145,28 @@ fn filt_from_opt_str(s: &Option<String>, source: &str) -> Result<Option<Targets>
     })
 }
 
+/// Helper to disable safe-logging when formatting an event to be logged to the
+/// console.
+struct FormatWithSafeLoggingSuppressed<F> {
+    /// An inner formatting type that does the actual formatting.
+    inner: F,
+}
+impl<S, N, F> fmt::FormatEvent<S, N> for FormatWithSafeLoggingSuppressed<F>
+where
+    F: fmt::FormatEvent<S, N>,
+    N: for<'writer> fmt::FormatFields<'writer> + 'static,
+    S: Subscriber + for<'span> tracing_subscriber::registry::LookupSpan<'span>,
+{
+    fn format_event(
+        &self,
+        ctx: &fmt::FmtContext<'_, S, N>,
+        writer: fmt::format::Writer<'_>,
+        event: &tracing::Event<'_>,
+    ) -> std::fmt::Result {
+        safelog::with_safe_logging_suppressed(|| self.inner.format_event(ctx, writer, event))
+    }
+}
+
 /// Try to construct a tracing [`Layer`] for logging to stdout.
 fn console_layer<S>(config: &LoggingConfig, cli: Option<&str>) -> Result<impl Layer<S>>
 where
@@ -144,7 +176,14 @@ where
         .map(|s| filt_from_str_verbose(s, "--log-level command line parameter"))
         .or_else(|| filt_from_opt_str(&config.console, "logging.console").transpose())
         .unwrap_or_else(|| Ok(Targets::from_str("debug").expect("bad default")))?;
-    Ok(fmt::Layer::default().with_filter(filter))
+    // We suppress safe logging when formatting messages for the console,
+    // which we assume to be volatile.
+    let format = FormatWithSafeLoggingSuppressed {
+        inner: fmt::format(),
+    };
+    Ok(fmt::Layer::default()
+        .event_format(format)
+        .with_filter(filter))
 }
 
 /// Try to construct a tracing [`Layer`] for logging to journald, if one is
@@ -234,6 +273,10 @@ pub struct LogGuards {
     /// The actual list of guards we're returning.
     #[allow(unused)]
     guards: Vec<WorkerGuard>,
+
+    /// A safelog guard, for use if we have decided to disable safe logging.
+    #[allow(unused)]
+    safelog_guard: Option<safelog::Guard>,
 }
 
 /// Set up logging.
@@ -259,5 +302,22 @@ pub fn setup_logging(config: &LoggingConfig, cli: Option<&str>) -> Result<LogGua
 
     registry.init();
 
-    Ok(LogGuards { guards })
+    let safelog_guard = if config.log_sensitive_information {
+        match safelog::disable_safe_logging() {
+            Ok(guard) => Some(guard),
+            Err(e) => {
+                // We don't need to propagate this error; it isn't the end of
+                // the world if we were unable to disable safe logging.
+                warn!("Unable to disable safe logging: {}", e);
+                None
+            }
+        }
+    } else {
+        None
+    };
+
+    Ok(LogGuards {
+        guards,
+        safelog_guard,
+    })
 }

crates/arti/src/socks.rs

@@ -7,6 +7,7 @@ use futures::future::FutureExt;
 use futures::io::{AsyncRead, AsyncReadExt, AsyncWrite, AsyncWriteExt, Error as IoError};
 use futures::stream::StreamExt;
 use futures::task::SpawnExt;
+use safelog::sensitive;
 use std::io::Result as IoResult;
 use std::net::{IpAddr, Ipv4Addr, Ipv6Addr, SocketAddr};
 use tracing::{error, info, warn};
@@ -162,7 +163,7 @@ See <a href="https://gitlab.torproject.org/tpo/core/arti/#todo-need-to-change-wh
     info!(
         "Got a socks request: {} {}:{}",
         request.command(),
-        addr,
+        sensitive(&addr),
         port
     );
 
@@ -202,7 +203,7 @@ See <a href="https://gitlab.torproject.org/tpo/core/arti/#todo-need-to-change-wh
                 }
             };
             // Okay, great! We have a connection over the Tor network.
-            info!("Got a stream for {}:{}", addr, port);
+            info!("Got a stream for {}:{}", sensitive(&addr), port);
             // TODO: Should send a SOCKS reply if something fails. See #258.
 
             // Send back a SOCKS response, telling the client that it

crates/safelog/Cargo.toml

+[package]
+name = "safelog"
+version = "0.2.0"
+authors = ["The Tor Project, Inc.", "Nick Mathewson <nickm@torproject.org>"]
+edition = "2021"
+rust-version = "1.56"
+license = "MIT OR Apache-2.0"
+homepage = "https://gitlab.torproject.org/tpo/core/arti/-/wikis/home"
+description = "Conditionally suppress confidential information from logs"
+keywords = ["tor", "arti", "logging", "privacy"]
+# We must put *something* here and this will do
+categories = ["rust-patterns"]
+repository = "https://gitlab.torproject.org/tpo/core/arti.git/"
+
+[features]
+default = []
+
+[dependencies]
+educe = "0.4.6"
+thiserror = "1"
+fluid-let = "1"
+serde = { version = "1.0.103", optional = true, features = ["derive"] }
+
+[dev-dependencies]
+serial_test = "0.6"
+static_assertions = "1"

crates/safelog/README.md

+Temporary file, to be replaced.

crates/safelog/src/err.rs

+//! Declare an error type.
+
+/// An error returned when attempting to enforce or disable safe logging.
+#[derive(Clone, Debug, thiserror::Error)]
+#[non_exhaustive]
+pub enum Error {
+    /// Tried to call [`disable_safe_logging`](crate::disable_safe_logging), but
+    /// `enforce_safe_logging` was already called.
+    #[error("Cannot enable unsafe logging: safe logging is already enforced.")]
+    AlreadySafe,
+
+    /// Tried to call [`enforce_safe_logging`](crate::enforce_safe_logging), but
+    /// `disable_safe_logging` was already called.
+    #[error("Cannot enforce safe logging: unsafe logging is already enabled.")]
+    AlreadyUnsafe,
+
+    /// One of the `enable`/`disable` functions was called so many times that we
+    /// could not keep count of how many guards there were.
+    ///
+    /// This should generally be impossible, and probably represents an error in
+    /// your program.
+    #[error("Too many calls to enforce or disable safe logging.")]
+    Overflow,
+}

crates/safelog/src/flags.rs

+//! Code for turning safelogging on and off.
+//!
+//! By default, safelogging is on.  There are two ways to turn it off: Globally
+//! (with [`disable_safe_logging`]) and locally (with
+//! [`with_safe_logging_suppressed`]).
+
+use crate::{Error, Result};
+use fluid_let::fluid_let;
+use std::sync::atomic::{AtomicIsize, Ordering};
+
+/// A global atomic used to track locking guards for enabling and disabling
+/// safe-logging.
+///
+/// The value of this atomic is less than 0 if we have enabled unsafe logging.
+/// greater than 0 if we have enabled safe logging, and 0 if nobody cares.
+static LOGGING_STATE: AtomicIsize = AtomicIsize::new(0);
+
+fluid_let!(
+    /// A dynamic variable used to temporarily disable safe-logging.
+    static SAFE_LOGGING_SUPPRESSED_IN_THREAD: bool
+);
+
+/// Returns true if we are displaying sensitive values, false otherwise.
+pub(crate) fn unsafe_logging_enabled() -> bool {
+    LOGGING_STATE.load(Ordering::Relaxed) < 0
+        || SAFE_LOGGING_SUPPRESSED_IN_THREAD.get(|v| v == Some(&true))
+}
+
+/// Run a given function with the regular `safelog` functionality suppressed.
+///
+/// The provided function, and everything it calls, will display
+/// [`Sensitive`](crate::Sensitive) values as if they were not sensitive.
+///
+/// # Examples
+///
+/// ```
+/// use safelog::{Sensitive, with_safe_logging_suppressed};
+///
+/// let string = Sensitive::new("swordfish");
+///
+/// // Ordinarily, the string isn't displayed as normal
+/// assert_eq!(format!("The value is {}", string),
+///            "The value is [scrubbed]");
+///
+/// // But you can override that:
+/// assert_eq!(
+///     with_safe_logging_suppressed(|| format!("The value is {}", string)),
+///     "The value is swordfish"
+/// );
+/// ```
+pub fn with_safe_logging_suppressed<F, V>(func: F) -> V
+where
+    F: FnOnce() -> V,
+{
+    // This sets the value of the variable to Some(true) temporarily, for as
+    // long as `func` is being called.  It uses thread-local variables
+    // internally.
+    SAFE_LOGGING_SUPPRESSED_IN_THREAD.set(true, func)
+}
+
+/// Enum to describe what kind of a [`Guard`] we've created.
+#[derive(Debug, Copy, Clone)]
+enum GuardKind {
+    /// We are forcing safe-logging to be enabled, so that nobody
+    /// can turn it off with `disable_safe_logging`
+    Safe,
+    /// We have are turning safe-logging off with `disable_safe_logging`.
+    Unsafe,
+}
+
+/// A guard object used to enforce safe logging, or turn it off.
+///
+/// For as long as this object exists, the chosen behavior will be enforced.
+//
+// TODO: Should there be different types for "keep safe logging on" and "turn
+// safe logging off"?  Having the same type makes it easier to write code that
+// does stuff like this:
+//
+//     let g = if cfg.safe {
+//         enforce_safe_logging()
+//     } else {
+//         disable_safe_logging()
+//     };
+#[derive(Debug)]
+#[must_use = "If you drop the guard immediately, it won't do anything."]
+pub struct Guard {
+    /// What kind of guard is this?
+    kind: GuardKind,
+}
+
+impl GuardKind {
+    /// Return an error if `val` (as a value of `LOGGING_STATE`) indicates that
+    /// intended kind of guard cannot be created.
+    fn check(&self, val: isize) -> Result<()> {
+        match self {
+            GuardKind::Safe => {
+                if val < 0 {
+                    return Err(Error::AlreadyUnsafe);
+                }
+            }
+            GuardKind::Unsafe => {
+                if val > 0 {
+                    return Err(Error::AlreadySafe);
+                }
+            }
+        }
+        Ok(())
+    }
+    /// Return the value by which `LOGGING_STATE` should change while a guard of
+    /// this type exists.
+    fn increment(&self) -> isize {
+        match self {
+            GuardKind::Safe => 1,
+            GuardKind::Unsafe => -1,
+        }
+    }
+}
+
+impl Guard {
+    /// Helper: Create a guard of a given kind.
+    fn new(kind: GuardKind) -> Result<Self> {
+        let inc = kind.increment();
+        loop {
+            // Find the current value of LOGGING_STATE and see if this guard can
+            // be created.
+            let old_val = LOGGING_STATE.load(Ordering::SeqCst);
+            // Exit if this guard can't be created.
+            kind.check(old_val)?;
+            // Otherwise, try changing LOGGING_STATE to the new value that it
+            // _should_ have when this guard exists.
+            let new_val = match old_val.checked_add(inc) {
+                Some(v) => v,
+                None => return Err(Error::Overflow),
+            };
+            if let Ok(v) =
+                LOGGING_STATE.compare_exchange(old_val, new_val, Ordering::SeqCst, Ordering::SeqCst)
+            {
+                // Great, we set the value to what it should be; we're done.
+                debug_assert_eq!(v, old_val);
+                return Ok(Self { kind });
+            }
+            // Otherwise, somebody else altered this value concurrently: try
+            // again.
+        }
+    }
+}
+
+impl Drop for Guard {
+    fn drop(&mut self) {
+        let inc = self.kind.increment();
+        LOGGING_STATE.fetch_sub(inc, Ordering::SeqCst);
+    }
+}
+
+/// Create a new [`Guard`] to prevent anyone else from disabling safe logging.
+///
+/// Until the resulting `Guard` is dropped, any attempts to call
+/// `disable_safe_logging` will give an error.  This guard does _not_ affect
+/// calls to [`with_safe_logging_suppressed`].
+///
+/// This call will return an error if safe logging is _already_ disabled.
+///
+/// Note that this function is called "enforce", not "enable", since safe
+/// logging is enabled by default.  Its purpose is to make sure that nothing
+/// _else_ has called disable_safe_logging().
+pub fn enforce_safe_logging() -> Result<Guard> {
+    Guard::new(GuardKind::Safe)
+}
+
+/// Create a new [`Guard`] to disable safe logging.
+///
+/// Until the resulting `Guard` is dropped, all [`Sensitive`](crate::Sensitive)
+/// values will be displayed as if they were not sensitive.
+///
+/// This call will return an error if safe logging has been enforced with
+/// [`enforce_safe_logging`].
+pub fn disable_safe_logging() -> Result<Guard> {
+    Guard::new(GuardKind::Unsafe)
+}
+
+#[cfg(test)]
+mod test {
+    #![allow(clippy::unwrap_used)]
+    use super::*;
+    // We use "serial_test" to make sure that our tests here run one at a time,
+    // since they modify global state.
+    use serial_test::serial;
+
+    #[test]
+    #[serial]
+    fn guards() {
+        // Try operations with logging guards turned on and off, in a single
+        // thread.
+        assert!(!unsafe_logging_enabled());
+        let g1 = enforce_safe_logging().unwrap();
+        let g2 = enforce_safe_logging().unwrap();
+
+        assert!(!unsafe_logging_enabled());
+
+        let e = disable_safe_logging();
+        assert!(matches!(e, Err(Error::AlreadySafe)));
+        assert!(!unsafe_logging_enabled());
+
+        drop(g1);
+        drop(g2);
+        let _g3 = disable_safe_logging().unwrap();
+        assert!(unsafe_logging_enabled());
+        let e = enforce_safe_logging();
+        assert!(matches!(e, Err(Error::AlreadyUnsafe)));
+        assert!(unsafe_logging_enabled());
+        let _g4 = disable_safe_logging().unwrap();
+
+        assert!(unsafe_logging_enabled());
+    }
+
+    #[test]
+    #[serial]
+    fn suppress() {
+        // Try out `with_safe_logging_suppressed` and make sure it does what we want
+        // regardless of the initial state of logging.
+        {
+            let _g = enforce_safe_logging().unwrap();
+            with_safe_logging_suppressed(|| assert!(unsafe_logging_enabled()));
+            assert!(!unsafe_logging_enabled());
+        }
+
+        {
+            assert!(!unsafe_logging_enabled());
+            with_safe_logging_suppressed(|| assert!(unsafe_logging_enabled()));
+            assert!(!unsafe_logging_enabled());
+        }
+
+        {
+            let _g = disable_safe_logging().unwrap();
+            assert!(unsafe_logging_enabled());
+            with_safe_logging_suppressed(|| assert!(unsafe_logging_enabled()));
+        }
+    }
+
+    #[test]
+    #[serial]
+    fn interfere_1() {
+        // Make sure that two threads trying to enforce and disable safe logging
+        // can interfere with each other, but will never enter an incorrect
+        // state.
+        use std::thread::{spawn, yield_now};
+
+        let thread1 = spawn(|| {
+            for _ in 0..10_000 {
+                if let Ok(_g) = enforce_safe_logging() {
+                    assert!(!unsafe_logging_enabled());
+                    yield_now();
+                    assert!(disable_safe_logging().is_err());
+                }
+                yield_now();
+            }
+        });
+
+        let thread2 = spawn(|| {
+            for _ in 0..10_000 {
+                if let Ok(_g) = disable_safe_logging() {
+                    assert!(unsafe_logging_enabled());
+                    yield_now();
+                    assert!(enforce_safe_logging().is_err());
+                }
+                yield_now();
+            }
+        });
+
+        thread1.join().unwrap();
+        thread2.join().unwrap();
+    }
+
+    #[test]
+    #[serial]
+    fn interfere_2() {
+        // Make sure that two threads trying to disable safe logging don't
+        // interfere.
+        use std::thread::{spawn, yield_now};
+
+        let thread1 = spawn(|| {
+            for _ in 0..10_000 {
+                let g = disable_safe_logging().unwrap();
+                assert!(unsafe_logging_enabled());
+                yield_now();
+                drop(g);
+                yield_now();
+            }
+        });
+
+        let thread2 = spawn(|| {
+            for _ in 0..10_000 {
+                let g = disable_safe_logging().unwrap();
+                assert!(unsafe_logging_enabled());
+                yield_now();
+                drop(g);
+                yield_now();
+            }
+        });
+
+        thread1.join().unwrap();
+        thread2.join().unwrap();
+    }
+
+    #[test]
+    #[serial]
+    fn interfere_3() {
+        // Make sure that `with_safe_logging_suppressed` only applies to the
+        // current thread.
+        use std::thread::{spawn, yield_now};
+
+        let thread1 = spawn(|| {
+            for _ in 0..10_000 {
+                assert!(!unsafe_logging_enabled());
+                yield_now();
+            }
+        });
+
+        let thread2 = spawn(|| {
+            for _ in 0..10_000 {
+                assert!(!unsafe_logging_enabled());
+                with_safe_logging_suppressed(|| {
+                    assert!(unsafe_logging_enabled());
+                    yield_now();
+                });
+            }
+        });
+
+        thread1.join().unwrap();
+        thread2.join().unwrap();
+    }
+}

crates/safelog/src/lib.rs

+//! # `safelog`: Mark data as sensitive for logging purposes.
+//!
+//! Some information is too sensitive to routinely write to system logs, but
+//! must nonetheless sometimes be displayed.  This crate provides a way to mark
+//! such information, and log it conditionally, but not by default.
+//!
+//! ## Examples
+//!
+//! There are two main ways to mark a piece of data as sensitive: by storing it
+//! within a [`Sensitive`] object long-term, or by wrapping it in a
+//! [`Sensitive`] object right before passing it to a formatter:
+//!
+//! ```
+//! use safelog::{Sensitive, sensitive};
+//!
+//! // With this declaration, a student's name and gpa will be suppressed by default
+//! // when passing the student to Debug.
+//! #[derive(Debug)]
+//! struct Student {
+//!    name: Sensitive<String>,
+//!    grade: u8,
+//!    homeroom: String,
+//!    gpa: Sensitive<f32>,
+//! }
+//!
+//! // In this function, a user's IP will not be printed by default.
+//! fn record_login(username: &str, ip: &std::net::IpAddr) {
+//!     println!("Login from {} at {}", username, sensitive(ip));
+//! }
+//! ```
+//!
+//! You can disable safe-logging globally (across all threads) or locally
+//! (across a single thread).
+//!
+//! ```
+//! use safelog::{disable_safe_logging, with_safe_logging_suppressed};
+//! # let debug_mode = false;
+//! # fn log_encrypted_data(s: &str) {}
+//! # let big_secret = "swordfish";
+//!
+//! // If we're running in debug mode, turn off safe logging
+//! // globally.  Safe logging will remain disabled until the
+//! // guard object is dropped.
+//! let guard = if debug_mode {
+//!    // This call can fail if safe logging has already been enforced.
+//!    disable_safe_logging().ok()
+//! } else {
+//!    None
+//! };
+//!
+//! // If we know that it's safe to record sensitive data with a given API,
+//! // we can disable safe logging temporarily. This affects only the current thread.
+//! with_safe_logging_suppressed(|| log_encrypted_data(big_secret));
+//! ```
+//!
+//! ## An example deployment
+//!
+//! This crate was originally created for use in the `arti` project, which tries
+//! to implements the Tor anonymity protocol in Rust.  In `arti`, we want to
+//! avoid logging information by default if it could compromise users'
+//! anonymity, or create an incentive for attacking users and relays in order to
+//! access their logs.
+//!
+//! In general, Arti treats the following information as [`Sensitive`]:
+//!   * Client addresses.
+//!   * The destinations (target addresses) of client requests.
+//!
+//! Arti does _not_ label all private information as `Sensitive`: when
+//! information isn't _ever_ suitable for logging, we omit it entirely.
+
+#![deny(missing_docs)]
+#![warn(noop_method_call)]
+#![deny(unreachable_pub)]
+#![warn(clippy::all)]
+#![deny(clippy::await_holding_lock)]
+#![deny(clippy::cargo_common_metadata)]
+#![deny(clippy::cast_lossless)]
+#![deny(clippy::checked_conversions)]
+#![warn(clippy::cognitive_complexity)]
+#![deny(clippy::debug_assert_with_mut_call)]
+#![deny(clippy::exhaustive_enums)]
+#![deny(clippy::exhaustive_structs)]
+#![deny(clippy::expl_impl_clone_on_copy)]
+#![deny(clippy::fallible_impl_from)]
+#![deny(clippy::implicit_clone)]
+#![deny(clippy::large_stack_arrays)]
+#![warn(clippy::manual_ok_or)]
+#![deny(clippy::missing_docs_in_private_items)]
+#![deny(clippy::missing_panics_doc)]
+#![warn(clippy::needless_borrow)]
+#![warn(clippy::needless_pass_by_value)]
+#![warn(clippy::option_option)]
+#![warn(clippy::rc_buffer)]
+#![deny(clippy::ref_option_ref)]
+#![warn(clippy::semicolon_if_nothing_returned)]
+#![warn(clippy::trait_duplication_in_bounds)]
+#![deny(clippy::unnecessary_wraps)]
+#![warn(clippy::unseparated_literal_suffix)]
+#![deny(clippy::unwrap_used)]
+
+// TODO: Try making it not Deref and having expose+expose_mut instead; how bad is it?
+
+use educe::Educe;
+#[cfg(feature = "serde")]
+use serde::{Deserialize, Serialize};
+
+mod err;
+mod flags;
+
+pub use err::Error;
+pub use flags::{disable_safe_logging, enforce_safe_logging, with_safe_logging_suppressed, Guard};
+
+/// A `Result` returned by the flag-manipulation functions in `safelog`.
+pub type Result<T> = std::result::Result<T, Error>;
+
+/// A wrapper type for a sensitive value.
+///
+/// By default, a `Sensitive<T>` behaves the same as a regular `T`, except that
+/// attempts to turn it into a string (via `Display`, `Debug`, etc) all produce
+/// the string `[scrubbed]`.
+///
+/// This behavior can be overridden locally by using
+/// [`with_safe_logging_suppressed`] and globally with [`disable_safe_logging`].
+#[derive(Educe)]
+#[educe(
+    Clone(bound),
+    Default(bound),
+    Deref,
+    DerefMut,
+    Eq(bound),
+    Hash(bound),
+    Ord(bound),
+    PartialEq(bound),
+    PartialOrd(bound)
+)]
+#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
+#[cfg_attr(feature = "serde", serde(transparent))]
+pub struct Sensitive<T>(T);
+
+impl<T> Sensitive<T> {
+    /// Create a new `Sensitive<T>`, wrapping a provided `value`.
+    pub fn new(value: T) -> Self {
+        Sensitive(value)
+    }
+
+    /// Extract the inner value from this `Sensitive<T>`.
+    pub fn unwrap(sensitive: Sensitive<T>) -> T {
+        sensitive.0
+    }
+}
+
+/// Wrap a value as `Sensitive`.
+///
+/// This function is an alias for [`Sensitive::new`].
+pub fn sensitive<T>(value: T) -> Sensitive<T> {
+    Sensitive(value)
+}
+
+impl<T> From<T> for Sensitive<T> {
+    fn from(value: T) -> Self {
+        Sensitive::new(value)
+    }
+}
+
+/// Helper: Declare one or more Display-like implementations for a
+/// Sensitive-like type.  These implementations will delegate to their std::fmt
+/// types if safe logging is disabled, and write `[scrubbed]` otherwise.
+macro_rules! impl_display_traits {
+    { $($trait:ident),*  for $object:ident } => {
+    $(
+        impl<T: std::fmt::$trait> std::fmt::$trait for $object<T> {
+            fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+                if flags::unsafe_logging_enabled() {
+                    std::fmt::$trait::fmt(&self.0, f)
+                } else {
+                    write!(f, "[scrubbed]")
+                }
+            }
+        }
+   )*
+   }
+}
+
+impl_display_traits! {
+    Display, Debug, Binary, Octal, LowerHex, UpperHex, LowerExp, UpperExp, Pointer for Sensitive
+}
+
+#[cfg(test)]
+mod test {
+    #![allow(clippy::unwrap_used)]
+    use super::*;
+    use serial_test::serial;
+    use static_assertions::{assert_impl_all, assert_not_impl_any};
+
+    #[test]
+    fn clone_bound() {
+        // Here we'll make sure that educe bounds work about the way we expect.
+        #[derive(Clone)]
+        struct A;
+        struct B;
+
+        let _x = Sensitive(A).clone();
+        let _y = Sensitive(B);
+
+        assert_impl_all!(Sensitive<A> : Clone);
+        assert_not_impl_any!(Sensitive<B> : Clone);
+    }
+
+    #[test]
+    #[serial]
+    fn debug_vec() {
+        type SVec = Sensitive<Vec<u32>>;
+
+        let mut sv = SVec::default();
+        assert!(sv.is_empty());
+        sv.push(104);
+        sv.push(49);
+        assert_eq!(sv.len(), 2);
+
+        assert!(!flags::unsafe_logging_enabled());
+        assert_eq!(format!("{:?}", &sv), "[scrubbed]");
+        let normal = with_safe_logging_suppressed(|| format!("{:?}", &sv));
+        assert_eq!(normal, "[104, 49]");
+
+        let _g = disable_safe_logging().unwrap();
+        assert_eq!(format!("{:?}", &sv), "[104, 49]");
+
+        assert_eq!(sv, SVec::from(vec![104, 49]));
+        assert_eq!(SVec::unwrap(sv.clone()), vec![104, 49]);
+        assert_eq!(*sv, vec![104, 49]);
+    }
+
+    #[test]
+    #[serial]
+    fn display_various() {
+        let val = Sensitive::<u32>::new(0x0ed19a);
+
+        let closure1 = || {
+            format!(
+                "{:?}, {}, {:o}, {:x}, {:X}, {:b}",
+                &val, &val, &val, &val, &val, &val,
+            )
+        };
+        let s1 = closure1();
+        let s2 = with_safe_logging_suppressed(closure1);
+        assert_eq!(
+            s1,
+            "[scrubbed], [scrubbed], [scrubbed], [scrubbed], [scrubbed], [scrubbed]"
+        );
+        assert_eq!(
+            s2,
+            "971162, 971162, 3550632, ed19a, ED19A, 11101101000110011010"
+        );
+
+        let n = 1.0E32;
+        let val = Sensitive::<f64>::new(n);
+        let expect = format!("{:?}, {}, {:e}, {:E}", n, n, n, n);
+        let closure2 = || format!("{:?}, {}, {:e}, {:E}", &val, &val, &val, &val);
+        let s1 = closure2();
+        let s2 = with_safe_logging_suppressed(closure2);
+        assert_eq!(s1, "[scrubbed], [scrubbed], [scrubbed], [scrubbed]");
+        assert_eq!(s2, expect);
+
+        let ptr: *const u8 = std::ptr::null();
+        let val = Sensitive::new(ptr);
+        let expect = format!("{:?}, {:p}", ptr, ptr);
+        let closure3 = || format!("{:?}, {:p}", val, val);
+        let s1 = closure3();
+        let s2 = with_safe_logging_suppressed(closure3);
+        assert_eq!(s1, "[scrubbed], [scrubbed]");
+        assert_eq!(s2, expect);
+    }
+}

crates/tor-circmgr/Cargo.toml

@@ -19,6 +19,7 @@ repository = "https://gitlab.torproject.org/tpo/core/arti.git/"
 experimental-api = []
 
 [dependencies]
+safelog = { path = "../safelog", version = "0.2.0" }
 tor-basic-utils = { path = "../tor-basic-utils", version = "0.2.0" }
 tor-chanmgr = { path = "../tor-chanmgr", version = "0.2.0" }
 tor-config = { path = "../tor-config", version = "0.2.0" }