From d3f495672835c87366f17c20ea2741b971852eda Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 21 Nov 2022 13:23:09 -0500 Subject: [PATCH 01/10] safelog: Define a "Redactable" trait A "redactable" object is one that can be _partially_ scrubbed in sensitive contexts. This can be very helpful for UX, but is not risk-free: see comments. --- crates/safelog/semver.md | 2 + crates/safelog/src/lib.rs | 91 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 93 insertions(+) create mode 100644 crates/safelog/semver.md diff --git a/crates/safelog/semver.md b/crates/safelog/semver.md new file mode 100644 index 000000000..b0e2f7cd8 --- /dev/null +++ b/crates/safelog/semver.md @@ -0,0 +1,2 @@ +MODIFIED: added Redacted, Redactable. + diff --git a/crates/safelog/src/lib.rs b/crates/safelog/src/lib.rs index 365569eb2..34018edde 100644 --- a/crates/safelog/src/lib.rs +++ b/crates/safelog/src/lib.rs @@ -186,6 +186,97 @@ impl_display_traits! { Display, Debug, Binary, Octal, LowerHex, UpperHex, LowerExp, UpperExp, Pointer } +/// A `redactable` object is one where we know a way to display _part_ of it +/// when we are running with safe logging enabled. +/// +/// For example, instead of referring to a user as `So-and-So` or `[scrubbed]`, +/// this trait would allow referring to the user as `S[...]`. +/// +/// # Privacy notes +/// +/// Displaying some information about an object is always less safe than +/// displaying no information about it! +/// +/// For example, in an environment with only a small number of users, the first +/// letter of a user's name might be plenty of information to identify them +/// uniquely. +/// +/// Even if a piece of redacted information is safe on its own, several pieces +/// of redacted information, when taken together, can be enough for an adversary +/// to infer more than you want. For example, if you log somebody's first +/// initial, month of birth, and last-two-digits of ID number, you have just +/// discarded 99.9% of potential individuals from the attacker's consideration. +pub trait Redactable: std::fmt::Display + std::fmt::Debug { + /// As `Display::fmt`, but produce a redacted representation. + fn display_redacted(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result; + /// As `Debug::fmt`, but produce a redacted representation. + fn debug_redacted(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result; + /// Return a smart pointer that will display or debug this object as its + /// redacted form. + fn redacted(&self) -> Redacted<&Self> { + Redacted(self) + } +} + +impl<'a, T: Redactable + ?Sized> Redactable for &'a T { + fn display_redacted(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + (*self).display_redacted(f) + } + + fn debug_redacted(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + (*self).debug_redacted(f) + } +} + +/// A wrapper around a `Redactable` that displays it in redacted format. +#[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 Redacted(T); + +impl Redacted { + /// Create a new `Redacted`. + pub fn new(value: T) -> Self { + Self(value) + } + + /// Consume this wrapper and return its inner value. + pub fn unwrap(self) -> T { + self.0 + } +} + +impl std::fmt::Display for Redacted { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + if flags::unsafe_logging_enabled() { + self.0.display_redacted(f) + } else { + std::fmt::Display::fmt(&self.0, f) + } + } +} + +impl std::fmt::Debug for Redacted { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + if flags::unsafe_logging_enabled() { + self.0.debug_redacted(f) + } else { + std::fmt::Debug::fmt(&self.0, f) + } + } +} + #[cfg(test)] mod test { #![allow(clippy::unwrap_used)] From f91218c78d11fd738d995dba1fca3abe867a01a9 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 21 Nov 2022 13:27:30 -0500 Subject: [PATCH 02/10] safelog: implement Redactable for network addr types. --- crates/safelog/src/impls.rs | 72 +++++++++++++++++++++++++++++++++++++ crates/safelog/src/lib.rs | 1 + 2 files changed, 73 insertions(+) create mode 100644 crates/safelog/src/impls.rs diff --git a/crates/safelog/src/impls.rs b/crates/safelog/src/impls.rs new file mode 100644 index 000000000..ab8eb3a34 --- /dev/null +++ b/crates/safelog/src/impls.rs @@ -0,0 +1,72 @@ +//! Implement `Redactable` for various types. + +use super::Redactable; +use std::fmt::{self, Formatter}; + +// Network types. + +impl Redactable for std::net::Ipv4Addr { + fn display_redacted(&self, f: &mut Formatter<'_>) -> fmt::Result { + write!(f, "{}.x.x.x", self.octets()[0]) + } + + fn debug_redacted(&self, f: &mut Formatter<'_>) -> fmt::Result { + self.display_redacted(f) + } +} + +impl Redactable for std::net::Ipv6Addr { + fn display_redacted(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{:x}:x:x:…", self.segments()[0]) + } + + fn debug_redacted(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + self.display_redacted(f) + } +} + +impl Redactable for std::net::IpAddr { + fn display_redacted(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + std::net::IpAddr::V4(v4) => v4.display_redacted(f), + std::net::IpAddr::V6(v6) => v6.display_redacted(f), + } + } + + fn debug_redacted(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + self.display_redacted(f) + } +} + +impl Redactable for std::net::SocketAddrV4 { + fn display_redacted(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}:{}", self.ip().redacted(), self.port()) + } + + fn debug_redacted(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + self.display_redacted(f) + } +} + +impl Redactable for std::net::SocketAddrV6 { + fn display_redacted(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "[{}]:{}", self.ip().redacted(), self.port()) + } + + fn debug_redacted(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + self.display_redacted(f) + } +} + +impl Redactable for std::net::SocketAddr { + fn display_redacted(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + std::net::SocketAddr::V4(v4) => v4.display_redacted(f), + std::net::SocketAddr::V6(v6) => v6.display_redacted(f), + } + } + + fn debug_redacted(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + self.display_redacted(f) + } +} diff --git a/crates/safelog/src/lib.rs b/crates/safelog/src/lib.rs index 34018edde..05492b76d 100644 --- a/crates/safelog/src/lib.rs +++ b/crates/safelog/src/lib.rs @@ -45,6 +45,7 @@ use serde::{Deserialize, Serialize}; mod err; mod flags; +mod impls; pub use err::Error; pub use flags::{disable_safe_logging, enforce_safe_logging, with_safe_logging_suppressed, Guard}; From f2c91ef56e64b3523746cf14f956c84dde834a3f Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 21 Nov 2022 14:07:49 -0500 Subject: [PATCH 03/10] safelog: Add a MaybeRedacted wrapper too. This is super helpful for cases where we want to write two nearly identical implementations to format a type. --- crates/safelog/src/lib.rs | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/crates/safelog/src/lib.rs b/crates/safelog/src/lib.rs index 05492b76d..60ad14d5b 100644 --- a/crates/safelog/src/lib.rs +++ b/crates/safelog/src/lib.rs @@ -217,6 +217,14 @@ pub trait Redactable: std::fmt::Display + std::fmt::Debug { fn redacted(&self) -> Redacted<&Self> { Redacted(self) } + /// Return a smart pointer that redacts this object if `redact` is true. + fn maybe_redacted(&self, redact: bool) -> MaybeRedacted<&Self> { + if redact { + MaybeRedacted::Redacted(Redacted(self)) + } else { + MaybeRedacted::NotRedacted(self) + } + } } impl<'a, T: Redactable + ?Sized> Redactable for &'a T { @@ -278,6 +286,36 @@ impl std::fmt::Debug for Redacted { } } +/// An object that may or may not be redacted. +/// +/// Used to implement conditional redaction +#[derive(Clone)] +#[allow(clippy::exhaustive_enums)] +pub enum MaybeRedacted { + /// A variant where we are redacting the object. + Redacted(Redacted), + /// A variant where we are not redacting the object. + NotRedacted(T), +} + +impl std::fmt::Display for MaybeRedacted { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + MaybeRedacted::Redacted(v) => std::fmt::Display::fmt(v, f), + MaybeRedacted::NotRedacted(v) => std::fmt::Display::fmt(v, f), + } + } +} + +impl std::fmt::Debug for MaybeRedacted { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + MaybeRedacted::Redacted(v) => std::fmt::Debug::fmt(v, f), + MaybeRedacted::NotRedacted(v) => std::fmt::Debug::fmt(v, f), + } + } +} + #[cfg(test)] mod test { #![allow(clippy::unwrap_used)] From 8d36fa9996a2ca76e18513003ea77eea6146114a Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 21 Nov 2022 14:25:30 -0500 Subject: [PATCH 04/10] llcrypto: Make key id types Redactable. --- crates/tor-llcrypto/Cargo.toml | 1 + crates/tor-llcrypto/semver.md | 1 + crates/tor-llcrypto/src/pk/ed25519.rs | 16 ++++++++++++++++ crates/tor-llcrypto/src/pk/rsa.rs | 12 ++++++++++++ 4 files changed, 30 insertions(+) create mode 100644 crates/tor-llcrypto/semver.md diff --git a/crates/tor-llcrypto/Cargo.toml b/crates/tor-llcrypto/Cargo.toml index 950ef5e80..ecbc7ef87 100644 --- a/crates/tor-llcrypto/Cargo.toml +++ b/crates/tor-llcrypto/Cargo.toml @@ -36,6 +36,7 @@ old_rand_core = { package = "rand_core", version = "0.5.1" } openssl = { version = "0.10.30", optional = true } rand_core = "0.6.2" rsa = "0.7.1" +safelog = { version = "0.1.3", path = "../safelog" } serde = "1.0.103" sha-1 = "0.10.0" sha2 = "0.10.0" diff --git a/crates/tor-llcrypto/semver.md b/crates/tor-llcrypto/semver.md new file mode 100644 index 000000000..221558e71 --- /dev/null +++ b/crates/tor-llcrypto/semver.md @@ -0,0 +1 @@ +MODIFIED: Implement Redactable for key ID types. diff --git a/crates/tor-llcrypto/src/pk/ed25519.rs b/crates/tor-llcrypto/src/pk/ed25519.rs index 51bc6e139..ef3864352 100644 --- a/crates/tor-llcrypto/src/pk/ed25519.rs +++ b/crates/tor-llcrypto/src/pk/ed25519.rs @@ -132,6 +132,22 @@ impl Debug for Ed25519Identity { } } +impl safelog::Redactable for Ed25519Identity { + /// Warning: This displays 12 bits of the ed25519 identity, which is + /// enough to narrow down a public relay by a great deal. + fn display_redacted(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!( + f, + "{}…", + &base64::encode_config(self.id, base64::STANDARD_NO_PAD)[..2] + ) + } + + fn debug_redacted(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "Ed25519Identity {{ {} }}", self.redacted()) + } +} + impl serde::Serialize for Ed25519Identity { fn serialize(&self, serializer: S) -> Result where diff --git a/crates/tor-llcrypto/src/pk/rsa.rs b/crates/tor-llcrypto/src/pk/rsa.rs index 63874ac1f..6c4a5e02a 100644 --- a/crates/tor-llcrypto/src/pk/rsa.rs +++ b/crates/tor-llcrypto/src/pk/rsa.rs @@ -63,6 +63,18 @@ impl fmt::Debug for RsaIdentity { } } +impl safelog::Redactable for RsaIdentity { + /// Warning: This displays 16 bits of the ed25519 identity, which is + /// enough to narrow down a public relay by a great deal. + fn display_redacted(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "${}…", hex::encode(&self.id[..1])) + } + + fn debug_redacted(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "RsaIdentity {{ {} }}", self.redacted()) + } +} + impl serde::Serialize for RsaIdentity { fn serialize(&self, serializer: S) -> Result where From 2aa0ae40167e47af178668e9351e5f153895b0cc Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 21 Nov 2022 14:31:33 -0500 Subject: [PATCH 05/10] linkspec: Give a Redacted form for chantarget. --- Cargo.lock | 1 + crates/tor-linkspec/Cargo.toml | 2 +- crates/tor-linkspec/src/ids.rs | 29 ++++++++++++++++++++++++ crates/tor-linkspec/src/owned.rs | 11 +++++++++ crates/tor-linkspec/src/traits.rs | 34 +++++++++++++++++++++++----- crates/tor-linkspec/src/transport.rs | 15 ++++++++++++ 6 files changed, 85 insertions(+), 7 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index c9a90646d..7a2e10f6c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3917,6 +3917,7 @@ dependencies = [ "rand_core 0.5.1", "rand_core 0.6.4", "rsa", + "safelog", "serde", "serde_test", "sha-1", diff --git a/crates/tor-linkspec/Cargo.toml b/crates/tor-linkspec/Cargo.toml index 56b217831..4a9426529 100644 --- a/crates/tor-linkspec/Cargo.toml +++ b/crates/tor-linkspec/Cargo.toml @@ -25,7 +25,7 @@ derive_builder = { version = "0.11.2", package = "derive_builder_fork_arti" } derive_more = "0.99.3" educe = "0.4.6" hex = "0.4" -safelog = { path = "../safelog", version = "0.1.0" } +safelog = { path = "../safelog", version = "0.1.3" } serde = { version = "1.0.103", features = ["derive"] } serde_with = "2.0.1" strum = { version = "0.24", features = ["derive"] } diff --git a/crates/tor-linkspec/src/ids.rs b/crates/tor-linkspec/src/ids.rs index 58b7db675..c9c503ee3 100644 --- a/crates/tor-linkspec/src/ids.rs +++ b/crates/tor-linkspec/src/ids.rs @@ -6,6 +6,7 @@ //! over those types, and over other new types that may exist in the future. use derive_more::{Display, From}; +use safelog::Redactable; use tor_llcrypto::pk::{ ed25519::{Ed25519Identity, ED25519_ID_LEN}, rsa::{RsaIdentity, RSA_ID_LEN}, @@ -195,6 +196,34 @@ impl<'a> From<&'a RelayId> for RelayIdRef<'a> { } } +impl Redactable for RelayId { + fn display_redacted(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + self.as_ref().display_redacted(f) + } + + fn debug_redacted(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + self.as_ref().debug_redacted(f) + } +} + +impl<'a> Redactable for RelayIdRef<'a> { + fn display_redacted(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + use std::fmt::Display as _; + match self { + RelayIdRef::Ed25519(k) => k.redacted().fmt(f), + RelayIdRef::Rsa(k) => k.redacted().fmt(f), + } + } + + fn debug_redacted(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + use std::fmt::Debug as _; + match self { + RelayIdRef::Ed25519(k) => k.redacted().fmt(f), + RelayIdRef::Rsa(k) => k.redacted().fmt(f), + } + } +} + /// Expand to an implementation for PartialEq for a given key type. macro_rules! impl_eq_variant { { $var:ident($type:ty) } => { diff --git a/crates/tor-linkspec/src/owned.rs b/crates/tor-linkspec/src/owned.rs index 025fd7d68..595ca230a 100644 --- a/crates/tor-linkspec/src/owned.rs +++ b/crates/tor-linkspec/src/owned.rs @@ -1,5 +1,6 @@ //! Owned variants of [`ChanTarget`] and [`CircTarget`]. +use safelog::Redactable; use serde::{Deserialize, Serialize}; use std::fmt::{self, Display}; use std::net::SocketAddr; @@ -163,6 +164,16 @@ impl Display for OwnedChanTarget { } } +impl Redactable for OwnedChanTarget { + fn display_redacted(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + self.display_chan_target().display_redacted(f) + } + + fn debug_redacted(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + self.display_chan_target().debug_redacted(f) + } +} + /// OwnedCircTarget is a summary of a [`CircTarget`] that owns all its /// members. #[derive(Debug, Clone, derive_builder::Builder)] diff --git a/crates/tor-linkspec/src/traits.rs b/crates/tor-linkspec/src/traits.rs index e22d3f070..f91f2669b 100644 --- a/crates/tor-linkspec/src/traits.rs +++ b/crates/tor-linkspec/src/traits.rs @@ -1,6 +1,7 @@ //! Declare traits to be implemented by types that describe a place //! that Tor can connect to, directly or indirectly. +use safelog::Redactable; use std::{fmt, iter::FusedIterator, net::SocketAddr}; use strum::IntoEnumIterator; use tor_llcrypto::pk; @@ -240,21 +241,24 @@ pub struct DisplayChanTarget<'a, T> { inner: &'a T, } -impl<'a, T: ChanTarget> fmt::Display for DisplayChanTarget<'a, T> { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { +impl<'a, T: ChanTarget> DisplayChanTarget<'a, T> { + /// helper: output `self` in a possibly redacted way. + fn fmt_impl(&self, f: &mut fmt::Formatter<'_>, redact: bool) -> fmt::Result { write!(f, "[")?; // We look at the chan_method() (where we would connect to) rather than // the addrs() (where the relay is, nebulously, "located"). This lets us // give a less surprising description. match self.inner.chan_method() { ChannelMethod::Direct(v) if v.is_empty() => write!(f, "?")?, - ChannelMethod::Direct(v) if v.len() == 1 => write!(f, "{}", v[0])?, - ChannelMethod::Direct(v) => write!(f, "{}+", v[0])?, + ChannelMethod::Direct(v) if v.len() == 1 => { + write!(f, "{}", v[0].maybe_redacted(redact))?; + } + ChannelMethod::Direct(v) => write!(f, "{}+", v[0].maybe_redacted(redact))?, #[cfg(feature = "pt-client")] ChannelMethod::Pluggable(target) => { match target.addr() { crate::PtTargetAddr::None => {} - other => write!(f, "{} ", other)?, + other => write!(f, "{} ", other.maybe_redacted(redact))?, } write!(f, "via {}", target.transport())?; // This deliberately doesn't include the PtTargetSettings, since @@ -263,13 +267,31 @@ impl<'a, T: ChanTarget> fmt::Display for DisplayChanTarget<'a, T> { } for ident in self.inner.identities() { - write!(f, " {}", ident)?; + write!(f, " {}", ident.maybe_redacted(redact))?; + if redact { + break; + } } write!(f, "]") } } +impl<'a, T: ChanTarget> fmt::Display for DisplayChanTarget<'a, T> { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + self.fmt_impl(f, false) + } +} + +impl<'a, T: ChanTarget + std::fmt::Debug> safelog::Redactable for DisplayChanTarget<'a, T> { + fn display_redacted(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + self.fmt_impl(f, true) + } + fn debug_redacted(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "ChanTarget({:?})", self.redacted().to_string()) + } +} + #[cfg(test)] mod test { #![allow(clippy::unwrap_used)] diff --git a/crates/tor-linkspec/src/transport.rs b/crates/tor-linkspec/src/transport.rs index 679ddfd83..9abc693a1 100644 --- a/crates/tor-linkspec/src/transport.rs +++ b/crates/tor-linkspec/src/transport.rs @@ -11,6 +11,7 @@ use std::net::SocketAddr; use std::slice; use std::str::FromStr; +use safelog::Redactable; use serde::{Deserialize, Serialize}; use thiserror::Error; @@ -291,6 +292,20 @@ impl Display for BridgeAddr { } } +impl Redactable for BridgeAddr { + fn display_redacted(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + BridgeAddr::IpPort(a) => a.redacted().fmt(f), + BridgeAddr::HostPort(host, port) => write!(f, "{}…:{}", &host[..2], port), + BridgeAddr::None => write!(f, "{}", NONE_ADDR), + } + } + + fn debug_redacted(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + self.display_redacted(f) + } +} + /// A set of options to be passed along to a pluggable transport along with a /// single target bridge relay. /// From 29f903bdac93e0dfd712fcd83a192a0703ef6763 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 21 Nov 2022 14:39:38 -0500 Subject: [PATCH 06/10] Display guards in redacted form in guard status messages. Closes #627, again. Closes #648. --- crates/tor-guardmgr/src/guard.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/tor-guardmgr/src/guard.rs b/crates/tor-guardmgr/src/guard.rs index f5d96bae9..8c573f569 100644 --- a/crates/tor-guardmgr/src/guard.rs +++ b/crates/tor-guardmgr/src/guard.rs @@ -15,7 +15,7 @@ use crate::skew::SkewObservation; use crate::util::randomize_time; use crate::{ids::GuardId, GuardParams, GuardRestriction, GuardUsage}; use crate::{sample, ExternalActivity, GuardSetSelector, GuardUsageKind}; -use safelog::sensitive as sv; +use safelog::Redactable as _; use tor_linkspec::{ ChanTarget, ChannelMethod, HasAddrs, HasChanMethod, HasRelayIds, PtTarget, RelayIds, }; @@ -419,11 +419,11 @@ impl Guard { match (self.reachable, r) { (_, R::Reachable) => info!( "We have found that {} is usable.", - sv(self.display_chan_target()) + self.display_chan_target().redacted() ), (R::Untried | R::Reachable, R::Unreachable) => warn!( "Could not connect to {}. We'll retry later, and let you know if it succeeds.", - sv(self.display_chan_target()) + self.display_chan_target().redacted() ), (_, _) => {} // not interesting. } From c62958c76f57a2aa4b63769d752799d8ef524d62 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 23 Nov 2022 13:04:18 -0500 Subject: [PATCH 07/10] Add a default debug_redacted impl to save space. --- crates/safelog/src/impls.rs | 24 ------------------------ crates/safelog/src/lib.rs | 8 +++----- crates/tor-linkspec/src/transport.rs | 4 ---- 3 files changed, 3 insertions(+), 33 deletions(-) diff --git a/crates/safelog/src/impls.rs b/crates/safelog/src/impls.rs index ab8eb3a34..5ca01ed1f 100644 --- a/crates/safelog/src/impls.rs +++ b/crates/safelog/src/impls.rs @@ -9,20 +9,12 @@ impl Redactable for std::net::Ipv4Addr { fn display_redacted(&self, f: &mut Formatter<'_>) -> fmt::Result { write!(f, "{}.x.x.x", self.octets()[0]) } - - fn debug_redacted(&self, f: &mut Formatter<'_>) -> fmt::Result { - self.display_redacted(f) - } } impl Redactable for std::net::Ipv6Addr { fn display_redacted(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { write!(f, "{:x}:x:x:…", self.segments()[0]) } - - fn debug_redacted(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - self.display_redacted(f) - } } impl Redactable for std::net::IpAddr { @@ -32,30 +24,18 @@ impl Redactable for std::net::IpAddr { std::net::IpAddr::V6(v6) => v6.display_redacted(f), } } - - fn debug_redacted(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - self.display_redacted(f) - } } impl Redactable for std::net::SocketAddrV4 { fn display_redacted(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { write!(f, "{}:{}", self.ip().redacted(), self.port()) } - - fn debug_redacted(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - self.display_redacted(f) - } } impl Redactable for std::net::SocketAddrV6 { fn display_redacted(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { write!(f, "[{}]:{}", self.ip().redacted(), self.port()) } - - fn debug_redacted(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - self.display_redacted(f) - } } impl Redactable for std::net::SocketAddr { @@ -65,8 +45,4 @@ impl Redactable for std::net::SocketAddr { std::net::SocketAddr::V6(v6) => v6.display_redacted(f), } } - - fn debug_redacted(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - self.display_redacted(f) - } } diff --git a/crates/safelog/src/lib.rs b/crates/safelog/src/lib.rs index 60ad14d5b..787f72e42 100644 --- a/crates/safelog/src/lib.rs +++ b/crates/safelog/src/lib.rs @@ -211,7 +211,9 @@ pub trait Redactable: std::fmt::Display + std::fmt::Debug { /// As `Display::fmt`, but produce a redacted representation. fn display_redacted(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result; /// As `Debug::fmt`, but produce a redacted representation. - fn debug_redacted(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result; + fn debug_redacted(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + self.display_redacted(f) + } /// Return a smart pointer that will display or debug this object as its /// redacted form. fn redacted(&self) -> Redacted<&Self> { @@ -231,10 +233,6 @@ impl<'a, T: Redactable + ?Sized> Redactable for &'a T { fn display_redacted(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { (*self).display_redacted(f) } - - fn debug_redacted(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - (*self).debug_redacted(f) - } } /// A wrapper around a `Redactable` that displays it in redacted format. diff --git a/crates/tor-linkspec/src/transport.rs b/crates/tor-linkspec/src/transport.rs index 9abc693a1..0ec226557 100644 --- a/crates/tor-linkspec/src/transport.rs +++ b/crates/tor-linkspec/src/transport.rs @@ -300,10 +300,6 @@ impl Redactable for BridgeAddr { BridgeAddr::None => write!(f, "{}", NONE_ADDR), } } - - fn debug_redacted(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - self.display_redacted(f) - } } /// A set of options to be passed along to a pluggable transport along with a From cf9f29158f2958a7e8bfe19789289bec3de2dd8d Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 23 Nov 2022 13:19:43 -0500 Subject: [PATCH 08/10] GuardMgr: Treat Guards as sensitive and Bridges as redacted. This machinery is a bit inelegant, but it is all confined to be within the GuardMgr crate, so IMO it should be fine for now. --- crates/tor-guardmgr/src/bridge/descs.rs | 2 + crates/tor-guardmgr/src/guard.rs | 45 ++++++++++++++++++--- crates/tor-guardmgr/src/sample/candidate.rs | 4 ++ 3 files changed, 46 insertions(+), 5 deletions(-) diff --git a/crates/tor-guardmgr/src/bridge/descs.rs b/crates/tor-guardmgr/src/bridge/descs.rs index 8652981f6..c4e9d91db 100644 --- a/crates/tor-guardmgr/src/bridge/descs.rs +++ b/crates/tor-guardmgr/src/bridge/descs.rs @@ -232,6 +232,7 @@ impl Universe for BridgeSet { is_dir_cache: true, // all bridges are directory caches. full_dir_info: bridge_relay.has_descriptor(), owned_target: OwnedChanTarget::from_chan_target(&bridge_relay), + sensitivity: crate::guard::DisplayRule::Redacted, }), CandidateStatus::Absent => CandidateStatus::Absent, CandidateStatus::Uncertain => CandidateStatus::Uncertain, @@ -291,6 +292,7 @@ impl Universe for BridgeSet { is_dir_cache: true, full_dir_info: relay.has_descriptor(), owned_target: OwnedChanTarget::from_chan_target(&relay), + sensitivity: crate::guard::DisplayRule::Redacted, }, RelayWeight::from(0), ) diff --git a/crates/tor-guardmgr/src/guard.rs b/crates/tor-guardmgr/src/guard.rs index 8c573f569..31c23cd50 100644 --- a/crates/tor-guardmgr/src/guard.rs +++ b/crates/tor-guardmgr/src/guard.rs @@ -68,6 +68,27 @@ impl CrateId { } } +/// What rule do we use when we're displaying information about a guard? +#[derive(Clone, Debug, Educe)] +#[educe(Default)] +pub(crate) enum DisplayRule { + /// The guard is Sensitive; we should display it as "[scrubbed]". + /// + /// We use this for public relays on the network, since displaying even the + /// redacted info about them can enough to identify them uniquely within the + /// NetDir. + /// + /// This should not be too much of a hit for UX (we hope), since the user is + /// not typically expected to work around issues with these guards themself. + #[educe(Default)] + Sensitive, + /// The guard should be Redacted; we display it as something like "192.x.x.x + /// $ab...". + /// + /// We use this for bridges. + Redacted, +} + /// A single guard node, as held by the guard manager. /// /// A Guard is a Tor relay that clients use for the first hop of their circuits. @@ -206,6 +227,10 @@ pub(crate) struct Guard { #[serde(skip)] clock_skew: Option, + /// How should we display information about this guard? + #[serde(skip)] + sensitivity: DisplayRule, + /// Fields from the state file that was used to make this `Guard` that /// this version of Arti doesn't understand. #[serde(flatten)] @@ -308,6 +333,7 @@ impl Guard { suspicious_behavior_warned: false, clock_skew: None, unknown_fields: Default::default(), + sensitivity: DisplayRule::Sensitive, } } @@ -403,6 +429,7 @@ impl Guard { suspicious_behavior_warned: other.suspicious_behavior_warned, dir_status: other.dir_status, clock_skew: other.clock_skew, + sensitivity: other.sensitivity, // Note that we _could_ remove either of the above blocks and add // `..self` or `..other`, but that would be risky: it would increase // the odds that we would forget to add some persistent or @@ -417,13 +444,10 @@ impl Guard { if self.reachable != r { // High-level logs, if change is interesting to user. match (self.reachable, r) { - (_, R::Reachable) => info!( - "We have found that {} is usable.", - self.display_chan_target().redacted() - ), + (_, R::Reachable) => info!("We have found that {} is usable.", self), (R::Untried | R::Reachable, R::Unreachable) => warn!( "Could not connect to {}. We'll retry later, and let you know if it succeeds.", - self.display_chan_target().redacted() + self ), (_, _) => {} // not interesting. } @@ -542,6 +566,7 @@ impl Guard { is_dir_cache, full_dir_info, owned_target, + sensitivity, }) => { // Update address information. self.orports = owned_target.addrs().into(); @@ -557,6 +582,7 @@ impl Guard { assert!(owned_target.has_all_relay_ids_from(self)); self.id = GuardId(RelayIds::from_relay_ids(&owned_target)); self.dir_info_missing = !full_dir_info; + self.sensitivity = sensitivity; listed_as_guard } @@ -817,6 +843,15 @@ impl tor_linkspec::HasChanMethod for Guard { impl tor_linkspec::ChanTarget for Guard {} +impl std::fmt::Display for Guard { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self.sensitivity { + DisplayRule::Sensitive => safelog::sensitive(self.display_chan_target()).fmt(f), + DisplayRule::Redacted => self.display_chan_target().redacted().fmt(f), + } + } +} + /// A reason for permanently disabling a guard. #[derive(Clone, Debug, Serialize, Deserialize)] #[serde(tag = "type")] diff --git a/crates/tor-guardmgr/src/sample/candidate.rs b/crates/tor-guardmgr/src/sample/candidate.rs index b0acd8297..5a83715be 100644 --- a/crates/tor-guardmgr/src/sample/candidate.rs +++ b/crates/tor-guardmgr/src/sample/candidate.rs @@ -73,6 +73,8 @@ pub(crate) struct Candidate { /// Information about connecting to the candidate and using it to build /// a channel. pub(crate) owned_target: OwnedChanTarget, + /// How should we display information about this candidate if we select it? + pub(crate) sensitivity: crate::guard::DisplayRule, } /// Information about how much of the universe we are using in a guard sample, @@ -107,6 +109,7 @@ impl Universe for NetDir { is_dir_cache: relay.is_dir_cache(), owned_target: OwnedChanTarget::from_chan_target(&relay), full_dir_info: true, + sensitivity: crate::guard::DisplayRule::Sensitive, }), None => match NetDir::ids_listed(self, guard) { Some(true) => panic!("ids_listed said true, but by_ids said none!"), @@ -180,6 +183,7 @@ impl Universe for NetDir { is_dir_cache: true, full_dir_info: true, owned_target: OwnedChanTarget::from_chan_target(relay), + sensitivity: crate::guard::DisplayRule::Sensitive, }, weight(self, relay).unwrap_or_else(|| RelayWeight::from(0)), ) From a77172e20dd1a7d3c59aabd888a3485313f75282 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Sun, 27 Nov 2022 14:54:30 -0500 Subject: [PATCH 09/10] safelog: Make MaybeRedacted opaque. --- Cargo.lock | 2 ++ crates/safelog/Cargo.toml | 2 ++ crates/safelog/src/lib.rs | 30 ++++++++---------------------- 3 files changed, 12 insertions(+), 22 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 7a2e10f6c..942eca38c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2812,7 +2812,9 @@ dependencies = [ name = "safelog" version = "0.1.3" dependencies = [ + "derive_more", "educe", + "either", "fluid-let", "serde", "serial_test", diff --git a/crates/safelog/Cargo.toml b/crates/safelog/Cargo.toml index 4f18fb875..b38e0e078 100644 --- a/crates/safelog/Cargo.toml +++ b/crates/safelog/Cargo.toml @@ -16,7 +16,9 @@ repository = "https://gitlab.torproject.org/tpo/core/arti.git/" default = [] [dependencies] +derive_more = "0.99.3" educe = "0.4.6" +either = "1" fluid-let = "1" serde = { version = "1.0.103", optional = true, features = ["derive"] } thiserror = "1" diff --git a/crates/safelog/src/lib.rs b/crates/safelog/src/lib.rs index 787f72e42..ca4601382 100644 --- a/crates/safelog/src/lib.rs +++ b/crates/safelog/src/lib.rs @@ -222,9 +222,9 @@ pub trait Redactable: std::fmt::Display + std::fmt::Debug { /// Return a smart pointer that redacts this object if `redact` is true. fn maybe_redacted(&self, redact: bool) -> MaybeRedacted<&Self> { if redact { - MaybeRedacted::Redacted(Redacted(self)) + MaybeRedacted(either::Either::Right(Redacted(self))) } else { - MaybeRedacted::NotRedacted(self) + MaybeRedacted(either::Either::Left(self)) } } } @@ -287,29 +287,15 @@ impl std::fmt::Debug for Redacted { /// An object that may or may not be redacted. /// /// Used to implement conditional redaction -#[derive(Clone)] -#[allow(clippy::exhaustive_enums)] -pub enum MaybeRedacted { - /// A variant where we are redacting the object. - Redacted(Redacted), - /// A variant where we are not redacting the object. - NotRedacted(T), -} - -impl std::fmt::Display for MaybeRedacted { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - match self { - MaybeRedacted::Redacted(v) => std::fmt::Display::fmt(v, f), - MaybeRedacted::NotRedacted(v) => std::fmt::Display::fmt(v, f), - } - } -} +#[derive(Clone, derive_more::Display)] +pub struct MaybeRedacted(either::Either>); impl std::fmt::Debug for MaybeRedacted { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - match self { - MaybeRedacted::Redacted(v) => std::fmt::Debug::fmt(v, f), - MaybeRedacted::NotRedacted(v) => std::fmt::Debug::fmt(v, f), + use std::fmt::Debug; + match &self.0 { + either::Either::Left(v) => Debug::fmt(v, f), + either::Either::Right(v) => Debug::fmt(v, f), } } } From 5d6044f3fc4be51fe7d5973dd90f3a65a8ac0b52 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 28 Nov 2022 09:36:15 -0500 Subject: [PATCH 10/10] Rustdoc fix: escape []s. --- crates/tor-guardmgr/src/guard.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/tor-guardmgr/src/guard.rs b/crates/tor-guardmgr/src/guard.rs index 31c23cd50..d008aa28e 100644 --- a/crates/tor-guardmgr/src/guard.rs +++ b/crates/tor-guardmgr/src/guard.rs @@ -72,7 +72,7 @@ impl CrateId { #[derive(Clone, Debug, Educe)] #[educe(Default)] pub(crate) enum DisplayRule { - /// The guard is Sensitive; we should display it as "[scrubbed]". + /// The guard is Sensitive; we should display it as "\[scrubbed\]". /// /// We use this for public relays on the network, since displaying even the /// redacted info about them can enough to identify them uniquely within the