diff --git a/crates/tor-circmgr/src/lib.rs b/crates/tor-circmgr/src/lib.rs index 925d6a9e8..cb9fbd526 100644 --- a/crates/tor-circmgr/src/lib.rs +++ b/crates/tor-circmgr/src/lib.rs @@ -51,6 +51,7 @@ #![deny(clippy::unwrap_used)] use tor_chanmgr::ChanMgr; +use tor_linkspec::ChanTarget; use tor_netdir::{DirEvent, NetDir, NetDirProvider}; use tor_proto::circuit::{CircParameters, ClientCirc, UniqId}; use tor_rtcompat::Runtime; @@ -695,20 +696,31 @@ impl CircMgr { /// Record that a failure occurred on a circuit with a given guard, in a way /// that makes us unwilling to use that guard for future circuits. - pub fn note_external_failure(&self, id: &FirstHopId, external_failure: ExternalActivity) { - self.mgr - .peek_builder() - .guardmgr() - .note_external_failure(id, external_failure); + /// + pub fn note_external_failure( + &self, + target: &impl ChanTarget, + external_failure: ExternalActivity, + ) { + self.mgr.peek_builder().guardmgr().note_external_failure( + target.ed_identity(), + target.rsa_identity(), + external_failure, + ); } /// Record that a success occurred on a circuit with a given guard, in a way /// that makes us possibly willing to use that guard for future circuits. - pub fn note_external_success(&self, id: &FirstHopId, external_activity: ExternalActivity) { - self.mgr - .peek_builder() - .guardmgr() - .note_external_success(id, external_activity); + pub fn note_external_success( + &self, + target: &impl ChanTarget, + external_activity: ExternalActivity, + ) { + self.mgr.peek_builder().guardmgr().note_external_success( + target.ed_identity(), + target.rsa_identity(), + external_activity, + ); } } diff --git a/crates/tor-dirmgr/src/lib.rs b/crates/tor-dirmgr/src/lib.rs index 0903edea9..d4f455870 100644 --- a/crates/tor-dirmgr/src/lib.rs +++ b/crates/tor-dirmgr/src/lib.rs @@ -981,7 +981,7 @@ impl DirMgr { /// Record that a problem has occurred because of a failure in an answer from `source`. fn note_cache_error(&self, source: &tor_dirclient::SourceInfo, problem: &Error) { - use tor_circmgr::{ExternalActivity, FirstHopId}; + use tor_circmgr::ExternalActivity; if !problem.indicates_cache_failure() { return; @@ -989,20 +989,18 @@ impl DirMgr { if let Some(circmgr) = &self.circmgr { info!("Marking {:?} as failed: {}", source, problem); - let guard_id = FirstHopId::from_chan_target(source.cache_id()); - circmgr.note_external_failure(&guard_id, ExternalActivity::DirCache); + circmgr.note_external_failure(source.cache_id(), ExternalActivity::DirCache); circmgr.retire_circ(source.unique_circ_id()); } } /// Record that `source` has successfully given us some directory info. fn note_cache_success(&self, source: &tor_dirclient::SourceInfo) { - use tor_circmgr::{ExternalActivity, FirstHopId}; + use tor_circmgr::ExternalActivity; if let Some(circmgr) = &self.circmgr { trace!("Marking {:?} as successful", source); - let guard_id = FirstHopId::from_chan_target(source.cache_id()); - circmgr.note_external_success(&guard_id, ExternalActivity::DirCache); + circmgr.note_external_success(source.cache_id(), ExternalActivity::DirCache); } } } diff --git a/crates/tor-guardmgr/src/fallback.rs b/crates/tor-guardmgr/src/fallback.rs index 0b7d62486..717e26694 100644 --- a/crates/tor-guardmgr/src/fallback.rs +++ b/crates/tor-guardmgr/src/fallback.rs @@ -13,6 +13,7 @@ mod set; mod status; +use crate::ids::FallbackId; use derive_builder::Builder; use tor_config::ConfigBuildError; use tor_llcrypto::pk::ed25519::Ed25519Identity; @@ -53,7 +54,7 @@ impl FallbackDir { /// Return a copy of this FallbackDir as a [`Guard`](crate::Guard) pub fn as_guard(&self) -> crate::FirstHop { crate::FirstHop { - id: crate::FirstHopId::from_chan_target(self), + id: FallbackId::from_chan_target(self).into(), orports: self.orports.clone(), } } diff --git a/crates/tor-guardmgr/src/fallback/set.rs b/crates/tor-guardmgr/src/fallback/set.rs index a8f1e09ed..2677964e5 100644 --- a/crates/tor-guardmgr/src/fallback/set.rs +++ b/crates/tor-guardmgr/src/fallback/set.rs @@ -4,7 +4,7 @@ use rand::seq::IteratorRandom; use std::time::Instant; use super::{FallbackDir, Status}; -use crate::{FirstHopId, PickGuardError}; +use crate::{ids::FallbackId, PickGuardError}; use serde::Deserialize; /// A list of fallback directories. @@ -62,6 +62,9 @@ pub(crate) struct FallbackState { #[derive(Debug, Clone)] pub(super) struct Entry { /// The inner fallback directory. + /// + /// (TODO: We represent this as a `FirstHop`, which could technically hold a + /// guard as well. Ought to fix that.) pub(super) fallback: crate::FirstHop, /// The status for the fallback directory. pub(super) status: Status, @@ -77,8 +80,12 @@ impl From for Entry { impl Entry { /// Return the identity for this fallback entry. - fn id(&self) -> &FirstHopId { - self.fallback.id() + fn id(&self) -> &FallbackId { + use crate::ids::FirstHopIdInner::*; + match &self.fallback.id().0 { + Fallback(id) => id, + _ => panic!("Somehow we constructed a fallback object with a non-fallback id!"), + } } } @@ -122,20 +129,33 @@ impl FallbackState { .min() } + /// Return a reference to the entry whose identity is `id`, if there is one. + fn get(&self, id: &FallbackId) -> Option<&Entry> { + match self.fallbacks.binary_search_by(|e| e.id().cmp(id)) { + Ok(idx) => Some(&self.fallbacks[idx]), + Err(_) => None, + } + } + /// Return a mutable reference to the entry whose identity is `id`, if there is one. - fn get_mut(&mut self, id: &FirstHopId) -> Option<&mut Entry> { + fn get_mut(&mut self, id: &FallbackId) -> Option<&mut Entry> { match self.fallbacks.binary_search_by(|e| e.id().cmp(id)) { Ok(idx) => Some(&mut self.fallbacks[idx]), Err(_) => None, } } + /// Return true if this set contains some entry with the given `id`. + pub(crate) fn contains(&self, id: &FallbackId) -> bool { + self.get(id).is_some() + } + /// Record that a success has occurred for the fallback with the given /// identity. /// /// Be aware that for fallbacks, we only count a successful directory /// operation as a success: a circuit success is not enough. - pub(crate) fn note_success(&mut self, id: &FirstHopId) { + pub(crate) fn note_success(&mut self, id: &FallbackId) { if let Some(entry) = self.get_mut(id) { entry.status.note_success(); } @@ -143,7 +163,7 @@ impl FallbackState { /// Record that a failure has occurred for the fallback with the given /// identity. - pub(crate) fn note_failure(&mut self, id: &FirstHopId, now: Instant) { + pub(crate) fn note_failure(&mut self, id: &FallbackId, now: Instant) { if let Some(entry) = self.get_mut(id) { entry.status.note_failure(now); } @@ -171,6 +191,7 @@ impl FallbackState { mod test { #![allow(clippy::unwrap_used)] use super::*; + use crate::FirstHopId; /// Construct a `FallbackDir` with random identity keys and addresses. /// @@ -197,7 +218,7 @@ mod test { // fabricate some fallbacks. let fbs = vec![rand_fb(), rand_fb(), rand_fb(), rand_fb()]; let fb_other = rand_fb(); - let id_other = FirstHopId::from_chan_target(&fb_other); + let id_other = FallbackId::from_chan_target(&fb_other); // basic case: construct a set let list: FallbackList = fbs.clone().into(); @@ -213,7 +234,7 @@ mod test { // use the constructed set a little. for fb in fbs.iter() { - let id = FirstHopId::from_chan_target(fb); + let id = FallbackId::from_chan_target(fb); assert_eq!(set.get_mut(&id).unwrap().id(), &id); } assert!(set.get_mut(&id_other).is_none()); @@ -247,7 +268,11 @@ mod test { let now = Instant::now(); fn lookup_idx(set: &FallbackState, id: &FirstHopId) -> Option { - set.fallbacks.binary_search_by(|ent| ent.id().cmp(id)).ok() + if let FirstHopId(crate::ids::FirstHopIdInner::Fallback(id)) = id { + set.fallbacks.binary_search_by(|ent| ent.id().cmp(id)).ok() + } else { + None + } } // Basic case: everybody is up. for _ in 0..100 { @@ -333,7 +358,7 @@ mod test { let mut fbs2: Vec<_> = fbs .into_iter() // (Remove the fallback with id==ids[2]) - .filter(|fb| FirstHopId::from_chan_target(fb) != ids[2]) + .filter(|fb| FallbackId::from_chan_target(fb) != ids[2]) .collect(); // add 2 new ones. let fbs_new = [rand_fb(), rand_fb(), rand_fb()]; @@ -352,7 +377,7 @@ mod test { // Make sure that the new fbs are there. for new_fb in fbs_new { assert!(set2 - .get_mut(&FirstHopId::from_chan_target(&new_fb)) + .get_mut(&FallbackId::from_chan_target(&new_fb)) .unwrap() .status .usable_at(now)); diff --git a/crates/tor-guardmgr/src/guard.rs b/crates/tor-guardmgr/src/guard.rs index 73f58243c..dea51c999 100644 --- a/crates/tor-guardmgr/src/guard.rs +++ b/crates/tor-guardmgr/src/guard.rs @@ -13,7 +13,8 @@ use std::time::{Duration, Instant, SystemTime}; use tracing::{trace, warn}; use crate::util::randomize_time; -use crate::{FirstHopId, GuardParams, GuardRestriction, GuardUsage}; +use crate::FirstHopId; +use crate::{ids::GuardId, GuardParams, GuardRestriction, GuardUsage}; use tor_persist::{Futureproof, JsonValue}; /// Tri-state to represent whether a guard is believed to be reachable or not. @@ -83,7 +84,7 @@ impl CrateId { #[derive(Clone, Debug, Serialize, Deserialize)] pub(crate) struct Guard { /// The identity keys for this guard. - id: FirstHopId, // TODO: Maybe refactor this out as redundant someday. + id: GuardId, // TODO: Maybe refactor this out as redundant someday. /// The most recently seen addresses for making OR connections to this /// guard. @@ -195,14 +196,14 @@ impl Guard { ); Self::new( - FirstHopId::from_chan_target(relay), + GuardId::from_chan_target(relay), relay.addrs().into(), added_at, ) } /// Return a new, manually constructed [`Guard`]. - fn new(id: FirstHopId, orports: Vec, added_at: SystemTime) -> Self { + fn new(id: GuardId, orports: Vec, added_at: SystemTime) -> Self { Guard { id, orports, @@ -225,7 +226,7 @@ impl Guard { } /// Return the identity of this Guard. - pub(crate) fn guard_id(&self) -> &FirstHopId { + pub(crate) fn guard_id(&self) -> &GuardId { &self.id } @@ -321,8 +322,8 @@ impl Guard { /// Return true if this guard obeys a single restriction. fn obeys_restriction(&self, r: &GuardRestriction) -> bool { match r { - GuardRestriction::AvoidId(ed) => &self.id.ed25519 != ed, - GuardRestriction::AvoidAllIds(ids) => !ids.contains(&self.id.ed25519), + GuardRestriction::AvoidId(ed) => &self.id.0.ed25519 != ed, + GuardRestriction::AvoidAllIds(ids) => !ids.contains(&self.id.0.ed25519), } } @@ -353,7 +354,7 @@ impl Guard { /// download another microdescriptor before we can be certain whether this /// guard is listed or not. pub(crate) fn listed_in(&self, netdir: &NetDir) -> Option { - netdir.id_pair_listed(&self.id.ed25519, &self.id.rsa) + netdir.id_pair_listed(&self.id.0.ed25519, &self.id.0.rsa) } /// Change this guard's status based on a newly received or newly @@ -370,11 +371,9 @@ impl Guard { // not. let listed_as_guard = match self.listed_in(netdir) { Some(true) => { + let id: FirstHopId = self.id.clone().into(); // Definitely listed. - let relay = self - .id - .get_relay(netdir) - .expect("Couldn't get a listed relay?!"); + let relay = id.get_relay(netdir).expect("Couldn't get a listed relay?!"); // Update address information. self.orports = relay.addrs().into(); // Check whether we can currently use it as a directory cache. @@ -570,13 +569,13 @@ impl Guard { /// We use this information to decide whether we are about to sample /// too much of the network as guards. pub(crate) fn get_weight(&self, dir: &NetDir) -> Option { - dir.weight_by_rsa_id(&self.id.rsa, tor_netdir::WeightRole::Guard) + dir.weight_by_rsa_id(&self.id.0.rsa, tor_netdir::WeightRole::Guard) } /// Return a [`crate::Guard`] object to represent this guard. pub(crate) fn get_external_rep(&self) -> crate::FirstHop { crate::FirstHop { - id: self.id.clone(), + id: self.id.clone().into(), orports: self.orports.clone(), } } @@ -587,10 +586,10 @@ impl tor_linkspec::ChanTarget for Guard { &self.orports[..] } fn ed_identity(&self) -> &Ed25519Identity { - &self.id.ed25519 + &self.id.0.ed25519 } fn rsa_identity(&self) -> &RsaIdentity { - &self.id.rsa + &self.id.0.rsa } } @@ -687,8 +686,8 @@ mod test { assert_eq!(Some(id.version.as_ref()), option_env!("CARGO_PKG_VERSION")); } - fn basic_id() -> FirstHopId { - FirstHopId::new([13; 32].into(), [37; 20].into()) + fn basic_id() -> GuardId { + GuardId::new([13; 32].into(), [37; 20].into()) } fn basic_guard() -> Guard { let id = basic_id(); @@ -703,8 +702,8 @@ mod test { let g = basic_guard(); assert_eq!(g.guard_id(), &id); - assert_eq!(g.ed_identity(), &id.ed25519); - assert_eq!(g.rsa_identity(), &id.rsa); + assert_eq!(g.ed_identity(), &id.0.ed25519); + assert_eq!(g.rsa_identity(), &id.0.rsa); assert_eq!(g.addrs(), &["127.0.0.7:7777".parse().unwrap()]); assert_eq!(g.reachable(), Reachable::Unknown); assert_eq!(g.reachable(), Reachable::default()); @@ -910,7 +909,8 @@ mod test { assert!(Some(guard22.added_at) <= Some(now)); // Can we still get the relay back? - let r = guard22.id.get_relay(&netdir).unwrap(); + let id: FirstHopId = guard22.id.clone().into(); + let r = id.get_relay(&netdir).unwrap(); assert_eq!(r.ed_identity(), relay22.ed_identity()); // Can we check on the guard's weight? @@ -919,11 +919,12 @@ mod test { // Now try a guard that isn't in the netdir. let guard255 = Guard::new( - FirstHopId::new([255; 32].into(), [255; 20].into()), + GuardId::new([255; 32].into(), [255; 20].into()), vec![], now, ); - assert!(guard255.id.get_relay(&netdir).is_none()); + let id: FirstHopId = guard255.id.clone().into(); + assert!(id.get_relay(&netdir).is_none()); assert!(guard255.get_weight(&netdir).is_none()); } @@ -960,7 +961,7 @@ mod test { // Try a guard that isn't in the netdir at all. let mut guard255 = Guard::new( - FirstHopId::new([255; 32].into(), [255; 20].into()), + GuardId::new([255; 32].into(), [255; 20].into()), vec!["8.8.8.8:53".parse().unwrap()], now, ); @@ -974,12 +975,9 @@ mod test { assert!(!guard255.orports.is_empty()); // Try a guard that is in netdir, but not netdir2. - let mut guard22 = Guard::new( - FirstHopId::new([22; 32].into(), [22; 20].into()), - vec![], - now, - ); - let relay22 = guard22.id.get_relay(&netdir).unwrap(); + let mut guard22 = Guard::new(GuardId::new([22; 32].into(), [22; 20].into()), vec![], now); + let id22: FirstHopId = guard22.id.clone().into(); + let relay22 = id22.get_relay(&netdir).unwrap(); assert_eq!(guard22.listed_in(&netdir), Some(true)); guard22.update_from_netdir(&netdir); assert_eq!(guard22.unlisted_since, None); // It's listed. @@ -994,11 +992,7 @@ mod test { assert!(!guard22.microdescriptor_missing); // Now see what happens for a guard that's in the consensus, but missing an MD. - let mut guard23 = Guard::new( - FirstHopId::new([23; 32].into(), [23; 20].into()), - vec![], - now, - ); + let mut guard23 = Guard::new(GuardId::new([23; 32].into(), [23; 20].into()), vec![], now); assert_eq!(guard23.listed_in(&netdir2), Some(true)); assert_eq!(guard23.listed_in(&netdir3), None); guard23.update_from_netdir(&netdir3); diff --git a/crates/tor-guardmgr/src/ids.rs b/crates/tor-guardmgr/src/ids.rs new file mode 100644 index 000000000..59f50d473 --- /dev/null +++ b/crates/tor-guardmgr/src/ids.rs @@ -0,0 +1,113 @@ +//! Identifier objects used to specify guards and/or fallbacks. + +use serde::{Deserialize, Serialize}; +use tor_llcrypto::pk; + +/// A pair of cryptographic identities used to distinguish a guard or fallback. +#[derive(Clone, Debug, Serialize, Deserialize, Eq, PartialEq, Hash, Ord, PartialOrd)] +pub(crate) struct IdPair { + /// Ed25519 identity key for a guard + pub(crate) ed25519: pk::ed25519::Ed25519Identity, + /// RSA identity fingerprint for a guard + pub(crate) rsa: pk::rsa::RsaIdentity, +} + +/// An identifier for a fallback directory cache. +/// +/// This is a separate type from GuardId and FirstHopId to avoid confusion +/// about what kind of object we're identifying. +#[derive(Clone, Debug, Eq, PartialEq, Hash, Ord, PartialOrd)] +pub(crate) struct FallbackId(pub(crate) IdPair); + +impl FallbackId { + /// Return a new, manually constructed `FallbackId` + pub(crate) fn new(ed25519: pk::ed25519::Ed25519Identity, rsa: pk::rsa::RsaIdentity) -> Self { + Self(IdPair { ed25519, rsa }) + } + /// Extract a `FallbackId` from a ChanTarget object. + pub(crate) fn from_chan_target(target: &T) -> Self { + Self::new(*target.ed_identity(), *target.rsa_identity()) + } +} +impl AsRef for FallbackId { + fn as_ref(&self) -> &IdPair { + &self.0 + } +} + +/// An identifier for a sampled guard. +/// +/// This is a separate type from GuardId and FirstHopId to avoid confusion +/// about what kind of object we're identifying. +#[derive(Clone, Debug, Serialize, Deserialize, Eq, PartialEq, Hash, Ord, PartialOrd)] +#[serde(transparent)] +pub(crate) struct GuardId(pub(crate) IdPair); + +impl GuardId { + /// Return a new, manually constructed `GuardId` + pub(crate) fn new(ed25519: pk::ed25519::Ed25519Identity, rsa: pk::rsa::RsaIdentity) -> Self { + Self(IdPair { ed25519, rsa }) + } + /// Extract a `GuardId` from a ChanTarget object. + pub(crate) fn from_chan_target(target: &T) -> Self { + Self::new(*target.ed_identity(), *target.rsa_identity()) + } +} +impl AsRef for GuardId { + fn as_ref(&self) -> &IdPair { + &self.0 + } +} + +/// Implementation type held inside of FirstHopId. +/// +/// This exists as a separate type from FirstHopId because Rust requires that a pub enum's variants are all public. +#[derive(Clone, Debug, Eq, PartialEq, Ord, PartialOrd)] +pub(crate) enum FirstHopIdInner { + /// Identifies a guard. + Guard(GuardId), + /// Identifies a fallback. + Fallback(FallbackId), +} + +/// A unique cryptographic identifier for a selected guard or fallback +/// directory. +/// +/// (This is implemented internally using both of the guard's Ed25519 and RSA +/// identities.) +#[derive(Clone, Debug, Eq, PartialEq, Ord, PartialOrd)] +pub struct FirstHopId(pub(crate) FirstHopIdInner); + +impl From for FirstHopId { + fn from(id: GuardId) -> Self { + Self(FirstHopIdInner::Guard(id)) + } +} +impl From for FirstHopId { + fn from(id: FallbackId) -> Self { + Self(FirstHopIdInner::Fallback(id)) + } +} +impl AsRef for FirstHopId { + /// Return the inner IdPair for this object. + /// + /// Only use this when it's okay to erase the type information about + /// whether this identifies a guard or a fallback. + fn as_ref(&self) -> &IdPair { + match &self.0 { + FirstHopIdInner::Guard(id) => id.as_ref(), + FirstHopIdInner::Fallback(id) => id.as_ref(), + } + } +} + +impl FirstHopId { + /// Return the relay in `netdir` that corresponds to this ID, if there + /// is one. + // + // We have to define this function so it'll be public. + pub fn get_relay<'a>(&self, netdir: &'a tor_netdir::NetDir) -> Option> { + let id = self.as_ref(); + netdir.by_id_pair(&id.ed25519, &id.rsa) + } +} diff --git a/crates/tor-guardmgr/src/lib.rs b/crates/tor-guardmgr/src/lib.rs index 940d9b5cf..20053f43a 100644 --- a/crates/tor-guardmgr/src/lib.rs +++ b/crates/tor-guardmgr/src/lib.rs @@ -151,17 +151,21 @@ mod err; pub mod fallback; mod filter; mod guard; +mod ids; mod pending; mod sample; mod util; pub use err::{GuardMgrError, PickGuardError}; pub use filter::GuardFilter; +pub use ids::FirstHopId; pub use pending::{GuardMonitor, GuardStatus, GuardUsable}; use pending::{PendingRequest, RequestId}; use sample::GuardSet; +use crate::ids::FirstHopIdInner; + /// A "guard manager" that selects and remembers a persistent set of /// guard nodes. /// @@ -499,48 +503,73 @@ impl GuardMgr { false }; - let pending_request = pending::PendingRequest::new( - guard.id.clone(), - origin, - usage, - usable_sender, - net_has_been_down, - ); + let pending_request = + pending::PendingRequest::new(guard.id.clone(), usage, usable_sender, net_has_been_down); inner.pending.insert(request_id, pending_request); - if origin.is_guard_sample() { - inner - .guards - .active_guards_mut() - .record_attempt(&guard.id, now); + match &guard.id.0 { + FirstHopIdInner::Guard(id) => inner.guards.active_guards_mut().record_attempt(id, now), + FirstHopIdInner::Fallback(_) => { + // We don't record attempts for fallbacks; we only care when + // they have failed. + } } Ok((guard, monitor, usable)) } - /// Record that _after_ we built a circuit with `guard`,something described + /// Record that _after_ we built a circuit with a guard, something described /// in `external_failure` went wrong with it. - pub fn note_external_failure(&self, guard: &FirstHopId, external_failure: ExternalActivity) { + pub fn note_external_failure( + &self, + ed_identity: &pk::ed25519::Ed25519Identity, + rsa_identity: &pk::rsa::RsaIdentity, + external_failure: ExternalActivity, + ) { let now = self.runtime.now(); let mut inner = self.inner.lock().expect("Poisoned lock"); - inner - .guards - .active_guards_mut() - .record_failure(guard, Some(external_failure), now); - - if external_failure == ExternalActivity::DirCache { - inner.fallbacks.note_failure(guard, now); + for id in inner.lookup_ids(ed_identity, rsa_identity) { + match &id.0 { + FirstHopIdInner::Guard(id) => { + inner.guards.active_guards_mut().record_failure( + id, + Some(external_failure), + now, + ); + } + FirstHopIdInner::Fallback(id) => { + if external_failure == ExternalActivity::DirCache { + inner.fallbacks.note_failure(id, now); + } + } + } } } - /// Record that _after_ we built a circuit with `guard`, some activity described in `external_activity` was successful with it. - /// - pub fn note_external_success(&self, guard: &FirstHopId, external_activity: ExternalActivity) { + /// Record that _after_ we built a circuit with a guard, some activity + /// described in `external_activity` was successful with it. + pub fn note_external_success( + &self, + ed_identity: &pk::ed25519::Ed25519Identity, + rsa_identity: &pk::rsa::RsaIdentity, + external_activity: ExternalActivity, + ) { let mut inner = self.inner.lock().expect("Poisoned lock"); - if external_activity == ExternalActivity::DirCache { - inner.fallbacks.note_success(guard); + for id in inner.lookup_ids(ed_identity, rsa_identity) { + match &id.0 { + FirstHopIdInner::Guard(_) => { + // TODO: Nothing to do in this case yet; there is not yet a + // separate "directory succeeding" status for guards. But there + // should be, eventually. This is part of #329. + } + FirstHopIdInner::Fallback(id) => { + if external_activity == ExternalActivity::DirCache { + inner.fallbacks.note_success(id); + } + } + } } } @@ -689,16 +718,16 @@ impl GuardMgrInner { let guard_id = pending.guard_id(); trace!(?guard_id, ?status, "Received report of guard status"); - match (status, pending.source()) { - (GuardStatus::Failure, sample::ListKind::Fallback) => { + match (status, &guard_id.0) { + (GuardStatus::Failure, FirstHopIdInner::Fallback(id)) => { // We used a fallback, and we weren't able to build a circuit through it. - self.fallbacks.note_failure(guard_id, runtime.now()); + self.fallbacks.note_failure(id, runtime.now()); } - (_, sample::ListKind::Fallback) => { + (_, FirstHopIdInner::Fallback(_)) => { // We don't record any other kind of circuit activity if we // took the entry from the fallback list. } - (GuardStatus::Success, _) => { + (GuardStatus::Success, FirstHopIdInner::Guard(id)) => { // If we had gone too long without any net activity when we // gave out this guard, and now we're seeing a circuit // succeed, tell the primary guards that they might be @@ -709,7 +738,7 @@ impl GuardMgrInner { // The guard succeeded. Tell the GuardSet. self.guards.active_guards_mut().record_success( - guard_id, + id, &self.params, runtime.wallclock(), ); @@ -726,22 +755,20 @@ impl GuardMgrInner { self.waiting.push(pending); } } - (GuardStatus::Failure, _) => { + (GuardStatus::Failure, FirstHopIdInner::Guard(id)) => { self.guards .active_guards_mut() - .record_failure(guard_id, None, runtime.now()); + .record_failure(id, None, runtime.now()); pending.reply(false); } - (GuardStatus::AttemptAbandoned, _) => { - self.guards - .active_guards_mut() - .record_attempt_abandoned(guard_id); + (GuardStatus::AttemptAbandoned, FirstHopIdInner::Guard(id)) => { + self.guards.active_guards_mut().record_attempt_abandoned(id); pending.reply(false); } - (GuardStatus::Indeterminate, _) => { + (GuardStatus::Indeterminate, FirstHopIdInner::Guard(id)) => { self.guards .active_guards_mut() - .record_indeterminate_result(guard_id); + .record_indeterminate_result(id); pending.reply(false); } }; @@ -771,12 +798,17 @@ impl GuardMgrInner { /// Return None if we can't yet give an answer about whether such /// a circuit is usable. fn guard_usability_status(&self, pending: &PendingRequest, now: Instant) -> Option { - self.guards.active_guards().circ_usability_status( - pending.guard_id(), - pending.usage(), - &self.params, - now, - ) + match &pending.guard_id().0 { + FirstHopIdInner::Guard(id) => self.guards.active_guards().circ_usability_status( + id, + pending.usage(), + &self.params, + now, + ), + // Fallback circuits are usable immediately, since we don't have to wait to + // see whether any _other_ circuit succeeds or fails. + FirstHopIdInner::Fallback(_) => Some(true), + } } /// For requests that have been "waiting" for an answer for too long, @@ -826,6 +858,36 @@ impl GuardMgrInner { std::mem::swap(&mut waiting, &mut self.waiting); } + /// Return every currently extant FirstHopId for a guard or fallback + /// directory matching the provided keys. + /// + /// # TODO + /// + /// This function should probably not exist; it's only used so that dirmgr + /// can report successes or failures, since by the time it observes them it + /// doesn't know whether its circuit came from a guard or a fallback. To + /// solve that, we'll need CircMgr to record and report which one it was + /// using, which will take some more plumbing. + fn lookup_ids( + &self, + ed_identity: &pk::ed25519::Ed25519Identity, + rsa_identity: &pk::rsa::RsaIdentity, + ) -> Vec { + let mut vec = Vec::with_capacity(2); + + let id = ids::GuardId::new(*ed_identity, *rsa_identity); + if self.guards.active_guards().contains(&id) { + vec.push(id.into()); + } + + let id = ids::FallbackId::new(*ed_identity, *rsa_identity); + if self.fallbacks.contains(&id) { + vec.push(id.into()); + } + + vec + } + /// Run any periodic events that update guard status, and return a /// duration after which periodic events should next be run. pub(crate) fn run_periodic_events(&mut self, wallclock: SystemTime, now: Instant) -> Duration { @@ -1006,37 +1068,6 @@ impl TryFrom<&NetParameters> for GuardParams { } } -/// A unique cryptographic identifier for a selected guard or fallback -/// directory. -/// -/// (This is implemented internally using both of the guard's Ed25519 and RSA -/// identities.) -#[derive(Clone, Debug, Serialize, Deserialize, Eq, PartialEq, Hash, Ord, PartialOrd)] -pub struct FirstHopId { - /// Ed25519 identity key for a guard - ed25519: pk::ed25519::Ed25519Identity, - /// RSA identity fingerprint for a guard - rsa: pk::rsa::RsaIdentity, -} - -impl FirstHopId { - /// Return a new, manually constructed GuardId - fn new(ed25519: pk::ed25519::Ed25519Identity, rsa: pk::rsa::RsaIdentity) -> Self { - Self { ed25519, rsa } - } - - /// Extract a GuardId from a ChanTarget object. - pub fn from_chan_target(target: &T) -> Self { - FirstHopId::new(*target.ed_identity(), *target.rsa_identity()) - } - - /// Return the relay in `netdir` that corresponds to this ID, if there - /// is one. - pub fn get_relay<'a>(&self, netdir: &'a NetDir) -> Option> { - netdir.by_id_pair(&self.ed25519, &self.rsa) - } -} - /// Representation of a guard or fallback, as returned by [`GuardMgr::select_guard()`]. #[derive(Debug, Clone, Eq, PartialEq)] pub struct FirstHop { @@ -1063,10 +1094,10 @@ impl tor_linkspec::ChanTarget for FirstHop { &self.orports[..] } fn ed_identity(&self) -> &pk::ed25519::Ed25519Identity { - &self.id.ed25519 + &self.id.as_ref().ed25519 } fn rsa_identity(&self) -> &pk::rsa::RsaIdentity { - &self.id.rsa + &self.id.as_ref().rsa } } @@ -1257,7 +1288,7 @@ mod test { let (guard, _mon, _usable) = guardmgr.select_guard(u, Some(&netdir)).unwrap(); // Make sure that the filter worked. - assert_eq!(guard.id().rsa.as_bytes()[0] % 4, 0); + assert_eq!(guard.id().as_ref().rsa.as_bytes()[0] % 4, 0); }); } } diff --git a/crates/tor-guardmgr/src/pending.rs b/crates/tor-guardmgr/src/pending.rs index 20cb3883e..7941eab85 100644 --- a/crates/tor-guardmgr/src/pending.rs +++ b/crates/tor-guardmgr/src/pending.rs @@ -7,7 +7,7 @@ //! then the circuit manager can't know whether to use a circuit built //! through that guard until the guard manager tells it. This is //! handled via [`GuardUsable`]. -use crate::{daemon, sample::ListKind, FirstHopId}; +use crate::{daemon, FirstHopId}; use educe::Educe; use futures::{ @@ -270,8 +270,6 @@ pub(crate) struct PendingRequest { /// than this one might be usable, we should only give it precedence /// if that guard is also allowable _for this usage_. usage: crate::GuardUsage, - /// Which list did we take this guard from? - source: ListKind, /// A oneshot channel used to tell the circuit manager that a circuit /// built through this guard can be used. /// @@ -293,7 +291,6 @@ impl PendingRequest { /// Create a new PendingRequest. pub(crate) fn new( guard_id: FirstHopId, - source: ListKind, usage: crate::GuardUsage, usable: Option>, net_has_been_down: bool, @@ -301,7 +298,6 @@ impl PendingRequest { PendingRequest { guard_id, usage, - source, usable, waiting_since: None, net_has_been_down, @@ -318,11 +314,6 @@ impl PendingRequest { &self.usage } - /// Return the source from which we took this guard from. - pub(crate) fn source(&self) -> ListKind { - self.source - } - /// Return the time (if any) when we were told that the guard /// was successful. pub(crate) fn waiting_since(&self) -> Option { diff --git a/crates/tor-guardmgr/src/sample.rs b/crates/tor-guardmgr/src/sample.rs index 34752692d..844519853 100644 --- a/crates/tor-guardmgr/src/sample.rs +++ b/crates/tor-guardmgr/src/sample.rs @@ -4,7 +4,7 @@ use crate::filter::GuardFilter; use crate::guard::{Guard, NewlyConfirmed, Reachable}; use crate::{ - ExternalActivity, FirstHopId, GuardParams, GuardUsage, GuardUsageKind, PickGuardError, + ids::GuardId, ExternalActivity, GuardParams, GuardUsage, GuardUsageKind, PickGuardError, }; use tor_netdir::{NetDir, Relay}; @@ -46,22 +46,22 @@ use tracing::{debug, info}; #[serde(from = "GuardSample")] pub(crate) struct GuardSet { /// Map from identities to guards, for every guard in this sample. - guards: HashMap, + guards: HashMap, /// Identities of all the guards in the sample, in sample order. /// /// This contains the same elements as `self.guards.keys()`, and /// only exists to define an ordering on the guards. - sample: Vec, + sample: Vec, /// Identities of all the confirmed guards in the sample, in /// confirmed order. /// /// This contains a subset of the values in `self.guards.keys()`. - confirmed: Vec, + confirmed: Vec, /// Identities of all the primary guards, in preference order /// (from best to worst). /// /// This contains a subset of the values in `self.guards.keys()`. - primary: Vec, + primary: Vec, /// Currently active filter that restricts which guards we can use. /// /// Note that all of the lists above (with the exception of `primary`) @@ -110,15 +110,6 @@ impl ListKind { ListKind::Confirmed | ListKind::Sample => false, } } - - /// Return true if this source indicates a guard that came from a sample - /// taken from a network directory at some point. - pub(crate) fn is_guard_sample(&self) -> bool { - match self { - ListKind::Primary | ListKind::Confirmed | ListKind::Sample => true, - ListKind::Fallback => false, - } - } } impl GuardSet { @@ -162,7 +153,7 @@ impl GuardSet { } /// Return the guard whose id is `id`, if any. - pub(crate) fn get(&self, id: &FirstHopId) -> Option<&Guard> { + pub(crate) fn get(&self, id: &GuardId) -> Option<&Guard> { self.guards.get(id) } @@ -258,8 +249,13 @@ impl GuardSet { fn contains_relay(&self, relay: &Relay<'_>) -> bool { // Note: Could implement Borrow instead, but I don't think it'll // matter. - let id = FirstHopId::from_chan_target(relay); - self.guards.contains_key(&id) + let id = GuardId::from_chan_target(relay); + self.contains(&id) + } + + /// Return true if `id` is a member of this set. + pub(crate) fn contains(&self, id: &GuardId) -> bool { + self.guards.contains_key(id) } /// If there are not enough filter-permitted usable guards in this @@ -391,7 +387,7 @@ impl GuardSet { /// /// Does nothing if it is already a guard. fn add_guard(&mut self, relay: &Relay<'_>, now: SystemTime, params: &GuardParams) { - let id = FirstHopId::from_chan_target(relay); + let id = GuardId::from_chan_target(relay); if self.guards.contains_key(&id) { return; } @@ -502,7 +498,7 @@ impl GuardSet { /// Return an iterator over the Id for every Guard in the sample that /// is not known to be Unreachable. - fn reachable_sample_ids(&self) -> impl Iterator { + fn reachable_sample_ids(&self) -> impl Iterator { self.sample.iter().filter(move |id| { let g = self.guards.get(id).expect("Inconsistent guard state"); g.reachable() != Reachable::Unreachable @@ -518,7 +514,7 @@ impl GuardSet { /// Note that this function will return guards that are not /// accepted by the current active filter: the caller must apply /// that filter if appropriate. - fn preference_order_ids(&self) -> impl Iterator { + fn preference_order_ids(&self) -> impl Iterator { self.primary .iter() .map(|id| (ListKind::Primary, id)) @@ -534,7 +530,7 @@ impl GuardSet { } /// Return true if `guard_id` is the identity for a primary guard. - fn guard_is_primary(&self, guard_id: &FirstHopId) -> bool { + fn guard_is_primary(&self, guard_id: &GuardId) -> bool { // This is O(n), but the list is short. self.primary.contains(guard_id) } @@ -579,7 +575,7 @@ impl GuardSet { /// Record that an attempt has begun to use the guard with /// `guard_id`. - pub(crate) fn record_attempt(&mut self, guard_id: &FirstHopId, now: Instant) { + pub(crate) fn record_attempt(&mut self, guard_id: &GuardId, now: Instant) { let is_primary = self.guard_is_primary(guard_id); if let Some(guard) = self.guards.get_mut(guard_id) { guard.record_attempt(now); @@ -594,7 +590,7 @@ impl GuardSet { /// just succeeded. pub(crate) fn record_success( &mut self, - guard_id: &FirstHopId, + guard_id: &GuardId, params: &GuardParams, now: SystemTime, ) { @@ -616,7 +612,7 @@ impl GuardSet { /// of the crate. pub(crate) fn record_failure( &mut self, - guard_id: &FirstHopId, + guard_id: &GuardId, how: Option, now: Instant, ) { @@ -635,7 +631,7 @@ impl GuardSet { /// Record that an attempt to use the guard with `guard_id` has /// just been abandoned, without learning whether it succeeded or failed. - pub(crate) fn record_attempt_abandoned(&mut self, guard_id: &FirstHopId) { + pub(crate) fn record_attempt_abandoned(&mut self, guard_id: &GuardId) { if let Some(guard) = self.guards.get_mut(guard_id) { guard.note_exploratory_circ(false); } @@ -644,7 +640,7 @@ impl GuardSet { /// Record that an attempt to use the guard with `guard_id` has /// just failed in a way that we could not definitively attribute to /// the guard. - pub(crate) fn record_indeterminate_result(&mut self, guard_id: &FirstHopId) { + pub(crate) fn record_indeterminate_result(&mut self, guard_id: &GuardId) { if let Some(guard) = self.guards.get_mut(guard_id) { guard.note_exploratory_circ(false); guard.record_indeterminate_result(); @@ -658,7 +654,7 @@ impl GuardSet { /// cannot yet be sure. pub(crate) fn circ_usability_status( &self, - guard_id: &FirstHopId, + guard_id: &GuardId, usage: &GuardUsage, params: &GuardParams, now: Instant, @@ -724,7 +720,7 @@ impl GuardSet { &self, usage: &GuardUsage, params: &GuardParams, - ) -> Result<(ListKind, FirstHopId), PickGuardError> { + ) -> Result<(ListKind, GuardId), PickGuardError> { debug_assert!(!self.primary_guards_invalidated); let n_options = match usage.kind { GuardUsageKind::OneHopDirectory => params.dir_parallelism, @@ -777,7 +773,7 @@ pub(crate) struct GuardSample<'a> { /// Equivalent to `GuardSet.guards.values()`, except in sample order. guards: Vec>, /// The identities for the confirmed members of `guards`, in confirmed order. - confirmed: Cow<'a, Vec>, + confirmed: Cow<'a, Vec>, /// Other data from the state file that this version of Arti doesn't recognize. #[serde(flatten)] remaining: HashMap, @@ -810,6 +806,7 @@ mod test { use tor_netdoc::doc::netstatus::{RelayFlags, RelayWeight}; use super::*; + use crate::FirstHopId; use std::time::Duration; fn netdir() -> NetDir { @@ -860,7 +857,7 @@ mod test { ..GuardParams::default() }; - let mut samples: Vec> = Vec::new(); + let mut samples: Vec> = Vec::new(); for _ in 0..3 { let mut guards = GuardSet::default(); guards.extend_sample_as_needed(SystemTime::now(), ¶ms, &netdir); @@ -871,7 +868,8 @@ mod test { // make sure all the guards are okay. for (g, guard) in &guards.guards { - let relay = g.get_relay(&netdir).unwrap(); + let id: FirstHopId = g.clone().into(); + let relay = id.get_relay(&netdir).unwrap(); assert!(relay.is_flagged_guard()); assert!(relay.is_dir_cache()); assert!(guards.contains_relay(&relay)); @@ -1229,7 +1227,7 @@ mod test { use tor_netdir::testnet; let netdir2 = testnet::construct_custom_netdir(|idx, bld| { - if idx == p_id1.ed25519.as_bytes()[0] as usize { + if idx == p_id1.0.ed25519.as_bytes()[0] as usize { bld.omit_md = true; } })