Merge branch 'redacted' into 'main'

Define a "redactable" trait, and use it when logging guard info

Closes #648 and #627

See merge request tpo/core/arti!882

Cargo.lock

@@ -2812,7 +2812,9 @@ dependencies = [
 name = "safelog"
 version = "0.1.3"
 dependencies = [
+ "derive_more",
  "educe",
+ "either",
  "fluid-let",
  "serde",
  "serial_test",
@@ -3917,6 +3919,7 @@ dependencies = [
  "rand_core 0.5.1",
  "rand_core 0.6.4",
  "rsa",
+ "safelog",
  "serde",
  "serde_test",
  "sha-1",

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"

crates/safelog/semver.md

@@ -0,0 +1,2 @@
+MODIFIED: added Redacted, Redactable.
+

crates/safelog/src/impls.rs

@@ -0,0 +1,48 @@
+//! 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])
+    }
+}
+
+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])
+    }
+}
+
+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),
+        }
+    }
+}
+
+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())
+    }
+}
+
+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())
+    }
+}
+
+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),
+        }
+    }
+}

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};
@@ -186,6 +187,119 @@ 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 {
+        self.display_redacted(f)
+    }
+    /// Return a smart pointer that will display or debug this object as its
+    /// redacted form.
+    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(either::Either::Right(Redacted(self)))
+        } else {
+            MaybeRedacted(either::Either::Left(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)
+    }
+}
+
+/// 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: Redactable>(T);
+
+impl<T: Redactable> Redacted<T> {
+    /// 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<T: Redactable> std::fmt::Display for Redacted<T> {
+    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<T: Redactable> std::fmt::Debug for Redacted<T> {
+    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)
+        }
+    }
+}
+
+/// An object that may or may not be redacted.
+///
+/// Used to implement conditional redaction
+#[derive(Clone, derive_more::Display)]
+pub struct MaybeRedacted<T: Redactable>(either::Either<T, Redacted<T>>);
+
+impl<T: Redactable> std::fmt::Debug for MaybeRedacted<T> {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        use std::fmt::Debug;
+        match &self.0 {
+            either::Either::Left(v) => Debug::fmt(v, f),
+            either::Either::Right(v) => Debug::fmt(v, f),
+        }
+    }
+}
+
 #[cfg(test)]
 mod test {
     #![allow(clippy::unwrap_used)]

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),
                 )

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,
 };
@@ -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<SkewObservation>,
 
+    /// 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.",
-                    sv(self.display_chan_target())
-                ),
+                (_, 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.",
-                    sv(self.display_chan_target())
+                    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")]

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)),
             )

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"] }

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) } => {

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)]

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)]

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,16 @@ 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),
+        }
+    }
+}
+
 /// A set of options to be passed along to a pluggable transport along with a
 /// single target bridge relay.
 ///

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"

crates/tor-llcrypto/semver.md

@@ -0,0 +1 @@
+MODIFIED: Implement Redactable for key ID types.

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<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
     where

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<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
     where