diff --git a/crates/tor-guardmgr/src/guard.rs b/crates/tor-guardmgr/src/guard.rs index 1420eb531..a69260a6a 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::{ExternalActivity, GuardSetSelector, GuardUsageKind}; -use tor_linkspec::{HasAddrs, HasRelayIds}; +use tor_linkspec::{HasAddrs, HasRelayIds, RelayIds}; use tor_persist::{Futureproof, JsonValue}; /// Tri-state to represent whether a guard is believed to be reachable or not. @@ -68,24 +68,28 @@ impl CrateId { /// 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. It doesn't need to be a relay that's currently on the -/// network (that is, one that we could represent as a [`Relay`]): -/// guards might be temporarily unlisted. +/// A Guard is a Tor relay that clients use for the first hop of their circuits. +/// It doesn't need to be a relay that's currently on the network (that is, one +/// that we could represent as a [`Relay`]): guards might be temporarily +/// unlisted. /// -/// Some fields in guards are persistent; others are reset with every -/// process. +/// Some fields in guards are persistent; others are reset with every process. +/// +/// # Identity +/// +/// Every guard has at least one `RelayId`. A guard may _gain_ identities over +/// time, as we learn more about it, but it should never _lose_ or _change_ its +/// identities of a given type. /// /// # TODO /// -/// This structure uses [`Instant`] to represent non-persistent points -/// in time, and [`SystemTime`] to represent points in time that need -/// to be persistent. That's possibly undesirable; maybe we should -/// come up with a better solution. +/// This structure uses [`Instant`] to represent non-persistent points in time, +/// and [`SystemTime`] to represent points in time that need to be persistent. +/// That's possibly undesirable; maybe we should come up with a better solution. #[derive(Clone, Debug, Serialize, Deserialize)] pub(crate) struct Guard { /// The identity keys for this guard. - id: GuardId, // TODO: Maybe refactor this out as redundant someday. + id: GuardId, /// The most recently seen addresses for making OR connections to this /// guard. @@ -298,12 +302,27 @@ impl Guard { /// Copy all _non-persistent_ status from `other` to self. /// - /// Requires that the two `Guard`s have the same ID. - pub(crate) fn copy_status_from(self, other: Guard) -> Guard { - debug_assert_eq!(self.id, other.id); + /// We do this when we were not the owner of our persistent state, and we + /// have just reloaded it (as `self`), but we have some ephemeral knowledge + /// about this guard (as `other`). + /// + /// You should not invent new uses for this function; instead we should come + /// up with alternatives. + /// + /// # Panics + /// + /// Panics if the identities in `self` are not exactly the same as the + /// identities in `other`. + pub(crate) fn copy_ephemeral_status_into_newly_loaded_state(self, other: Guard) -> Guard { + // It is not safe to copy failure information unless these identities + // are a superset of those in `other`; but it is not safe to copy success + // information unless these identities are a subset of those in `other`. + // + // To simplify matters, we just insist that the identities have to be the same. + assert!(self.same_relay_ids(&other)); Guard { - // All persistent fields are taken from `self`. + // All other persistent fields are taken from `self`. id: self.id, orports: self.orports, added_at: self.added_at, @@ -428,14 +447,17 @@ impl Guard { netdir.ids_listed(&self.id.0) } - /// Change this guard's status based on a newly received or newly - /// updated [`NetDir`]. + /// Change this guard's status based on a newly received or newly updated + /// [`NetDir`]. /// - /// A guard may become "listed" or "unlisted": a listed guard is - /// one that appears in the consensus with the Guard flag. + /// A guard may become "listed" or "unlisted": a listed guard is one that + /// appears in the consensus with the Guard flag. /// - /// Additionally, a guard's orports may change, if the directory - /// lists a new address for the relay. + /// A guard may acquire additional identities if we learned them from the + /// netdir. + /// + /// Additionally, a guard's orports may change, if the directory lists a new + /// address for the relay. pub(crate) fn update_from_netdir(&mut self, netdir: &NetDir) { // This is a tricky check, since if we're missing a microdescriptor // for the RSA id, we won't know whether the ed25519 id is listed or @@ -450,6 +472,9 @@ impl Guard { self.orports = relay.addrs().into(); // Check whether we can currently use it as a directory cache. self.is_dir_cache = relay.is_dir_cache(); + // Update our IDs: the Relay will have strictly more. + assert!(relay.has_all_relay_ids_from(self)); + self.id = GuardId(RelayIds::from_relay_ids(&relay)); relay.is_flagged_guard() } diff --git a/crates/tor-guardmgr/src/ids.rs b/crates/tor-guardmgr/src/ids.rs index a9f0241ef..b8bd47a6d 100644 --- a/crates/tor-guardmgr/src/ids.rs +++ b/crates/tor-guardmgr/src/ids.rs @@ -64,6 +64,15 @@ impl GuardId { } } +impl HasRelayIds for GuardId { + fn identity( + &self, + key_type: tor_linkspec::RelayIdType, + ) -> Option> { + self.0.identity(key_type) + } +} + /// 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. @@ -79,11 +88,6 @@ pub(crate) enum FirstHopIdInner { /// directory. /// /// (This is implemented internally using all of the guard's known identities.) -/// -/// TODO(nickm): we may want a fuzzier match for this type in the future in our -/// maps, if we ever learn about more identity types. Right now we don't -/// recognize two `FirstHopId`s as "the same" if one has more IDs than the -/// other, even if all the IDs that they do have are the same. #[derive(Clone, Debug, Eq, PartialEq, Ord, PartialOrd)] pub struct FirstHopId(pub(crate) FirstHopIdInner); diff --git a/crates/tor-guardmgr/src/lib.rs b/crates/tor-guardmgr/src/lib.rs index 89b8224f8..3a2a2cfd9 100644 --- a/crates/tor-guardmgr/src/lib.rs +++ b/crates/tor-guardmgr/src/lib.rs @@ -641,7 +641,9 @@ impl GuardSets { use strum::IntoEnumIterator; for sample in GuardSetSelector::iter() { self.guards_mut(&sample) - .copy_status_from(std::mem::take(other.guards_mut(&sample))); + .copy_ephemeral_status_into_newly_loaded_state(std::mem::take( + other.guards_mut(&sample), + )); } } } @@ -1055,7 +1057,11 @@ impl GuardMgrInner { } /// Return every currently extant FirstHopId for a guard or fallback - /// directory matching the provided keys. + /// directory matching (or possibly matching) the provided keys. + /// + /// An identity is _possibly matching_ if it contains some of the IDs in the + /// provided identity, and it has no _contradictory_ identities, but it does + /// not necessarily contain _all_ of those identities. /// /// # TODO /// @@ -1064,6 +1070,10 @@ impl GuardMgrInner { /// 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. + /// + /// TODO relay: we will have to make the change above when we implement + /// relays; otherwise, it would be possible for an attacker to exploit it to + /// mislead us about our guard status. fn lookup_ids(&self, identity: &T) -> Vec where T: tor_linkspec::HasRelayIds + ?Sized, @@ -1073,9 +1083,12 @@ impl GuardMgrInner { let id = ids::GuardId::from_relay_ids(identity); for sample in GuardSetSelector::iter() { - if self.guards.guards(&sample).contains(&id) { - vec.push(FirstHopId(FirstHopIdInner::Guard(sample, id.clone()))); - } + let guard_id = match self.guards.guards(&sample).contains(&id) { + Ok(true) => &id, + Err(other) => other, + Ok(false) => continue, + }; + vec.push(FirstHopId(FirstHopIdInner::Guard(sample, guard_id.clone()))); } let id = ids::FallbackId::from_relay_ids(identity); diff --git a/crates/tor-guardmgr/src/sample.rs b/crates/tor-guardmgr/src/sample.rs index d84e5af32..10b76f7ab 100644 --- a/crates/tor-guardmgr/src/sample.rs +++ b/crates/tor-guardmgr/src/sample.rs @@ -13,6 +13,7 @@ use crate::{ }; use crate::{FirstHop, GuardSetSelector}; use tor_basic_utils::iter::{FilterCount, IteratorExt as _}; +use tor_linkspec::{ByRelayIds, HasRelayIds}; use tor_netdir::{NetDir, Relay}; use itertools::Itertools; @@ -47,21 +48,78 @@ use tracing::{debug, info}; #[serde(from = "GuardSample")] pub(crate) struct GuardSet { /// Map from identities to guards, for every guard in this sample. - guards: HashMap, + /// + /// The key for each entry is a set of identities which we have + /// good (trustworthy-enough) reason to link together. + /// + /// When we connect to a guard we require it to demonstrate + /// that it has *all* of these identities; + /// and we do pinning, so that we note down the other identities we discover it has, + /// with the intent that we will require them in future. + /// + /// ### Sources of linkage: + /// + /// * If we connect to a relay and it proves a set of identities, + /// that necessarily will include at least the ones we have already. + /// We can add any other identities we have discovered. + /// Justification: the owners of the old ids have made a statement + /// (via the connection protocols) that these other ids are also theirs, + /// and should be required in future. + /// + /// * If we obtain a (full) descriptor for a relay, and check the + /// self-signatures by all the identities we have already, + /// we can add any other identities listed in the descriptor. + /// Justification: the owners of the old ids have made an explicit statement + /// that these other ids are also theirs, + /// and should be required in future. + /// + /// * For a relay in the netdir, if the netdir links some ids together, + /// we can combine the entries. + /// Justification: the netdir is authoritative for netdir-based relays. + /// + /// * For a configured bridge, if our configuration links some identities, + /// we must insist on all those identities. + /// So we combine them. + /// + /// ### Handling of conflicting entries: + /// + /// `ByRelayIds` will implicitly delete conflicting entries, + /// simply forgetting about them. + /// This is OK for netdir relays, since we do not expect this to occur in practice. + /// + /// For bridges, conflicts may in fact occur, + /// since bridge lines are not issued by a single authority, + /// and should be afforded limited trust. + /// + /// * If the configuration contains bridge lines that mutually conflict, + /// affected bridge lines should be disregarded, + /// or the configuration rejected. + /// + /// * If the configuration contains information which is inconsistent with + /// our past experience, we should discard the past experiences which + /// aren't reconcilable with the configuration. + /// + /// * We may discover a linkage which demonstrates that the configuration + /// is wrong: for example, two bridge lines for identities X and Y, + /// but in fact there is only one bridge with both identities. + /// In this situation it is OK to effectively disregard some the configuration + /// entries which are at variance with reality, maybe with a warning, + /// but keeping at least one of every usable id set (actually existing bridge) + /// would be good. + guards: ByRelayIds, /// 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. + /// This contains the same elements as the keys of `guards` 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()`. + /// This contains a subset of the values in `sample`. 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()`. + /// This contains a subset of the values in `sample`. primary: Vec, /// Currently active filter that restricts which guards we can use. /// @@ -126,21 +184,34 @@ impl GuardSet { ) } - /// Remove all elements from this `GuardSet` that ought to be - /// referenced by another element, but which are not. + /// Remove all elements from this `GuardSet` that ought to be referenced by + /// another element, but which are not. /// - /// This method only removes corrupted elements; it doesn't add or - /// fix anything. It won't do anything if the `GuardSet` is - /// well-formed. + /// This method only removes corrupted elements and updates IDs in the ID + /// list (possibly adding new IDs); it doesn't add guards or other data. + /// It won't do anything if the `GuardSet` is well-formed. fn fix_consistency(&mut self) { + /// Remove every element of `id_list` that does not belong to some guard + /// in `guards`, and update the others to have any extra identities + /// listed in `guards`. + fn fix_id_list(guards: &ByRelayIds, id_list: &mut Vec) { + // TODO: Use Vec::retain_mut when our MSRV >= 1.61 + #![allow(deprecated)] + use retain_mut::RetainMut; + RetainMut::retain_mut(id_list, |id| match guards.by_all_ids(id) { + Some(guard) => { + *id = guard.guard_id().clone(); + true + } + None => false, + }); + } + let sample_set: HashSet<_> = self.sample.iter().collect(); - self.guards - .retain(|id, g| g.guard_id() == id && sample_set.contains(id)); - let guards = &self.guards; // avoid borrow issues - // TODO: We should potentially de-duplicate these. - self.sample.retain(|id| guards.contains_key(id)); - self.confirmed.retain(|id| guards.contains_key(id)); - self.primary.retain(|id| guards.contains_key(id)); + self.guards.retain(|g| sample_set.contains(g.guard_id())); + fix_id_list(&self.guards, &mut self.sample); + fix_id_list(&self.guards, &mut self.confirmed); + fix_id_list(&self.guards, &mut self.primary); } /// Assert that this `GuardSet` is internally consistent. @@ -153,9 +224,9 @@ impl GuardSet { assert_eq!(len_pre, len_post); } - /// Return the guard whose id is `id`, if any. + /// Return the guard that has every identity in `id`, if any. pub(crate) fn get(&self, id: &GuardId) -> Option<&Guard> { - self.guards.get(id) + self.guards.by_all_ids(id) } /// Replace the filter used by this `GuardSet` with `filter`. @@ -173,7 +244,7 @@ impl GuardSet { let filt = &self.active_filter; self.primary.retain(|id| { guards - .get(id) + .by_all_ids(id) .map(|g| g.usable() && filt.permits(g)) .unwrap_or(false) }); @@ -187,16 +258,23 @@ impl GuardSet { } /// Copy non-persistent status from every guard shared with `other`. - pub(crate) fn copy_status_from(&mut self, mut other: GuardSet) { - let mut old_guards = HashMap::new(); - std::mem::swap(&mut old_guards, &mut self.guards); + /// + /// This is used as part of our reload process when we don't own our state + /// files, and we're reloading in order to find out what the other Arti + /// instance thinks the guards are. At that point, `self` is the set of + /// guards that we just loaded from state, and `other` is our old guards, + /// which we are using only for their status information. + pub(crate) fn copy_ephemeral_status_into_newly_loaded_state(&mut self, mut other: GuardSet) { + let old_guards = std::mem::take(&mut self.guards); self.guards = old_guards - .into_iter() - .map(|(id, guard)| { - if let Some(other_guard) = other.guards.remove(&id) { - (id, guard.copy_status_from(other_guard)) + .into_values() + .map(|guard| { + let id = guard.guard_id(); + + if let Some(other_guard) = other.guards.remove_exact(id) { + guard.copy_ephemeral_status_into_newly_loaded_state(other_guard) } else { - (id, guard) + guard } }) .collect(); @@ -208,7 +286,7 @@ impl GuardSet { let guards = self .sample .iter() - .map(|id| Cow::Borrowed(self.guards.get(id).expect("Inconsistent state"))) + .map(|id| Cow::Borrowed(self.guards.by_all_ids(id).expect("Inconsistent state"))) .collect(); GuardSample { @@ -220,11 +298,11 @@ impl GuardSet { /// Reconstruct a guard state from its serialized representation. fn from_state(state: GuardSample<'_>) -> Self { - let mut guards = HashMap::new(); + let mut guards = ByRelayIds::new(); let mut sample = Vec::new(); for guard in state.guards { sample.push(guard.guard_id().clone()); - guards.insert(guard.guard_id().clone(), guard.into_owned()); + guards.insert(guard.into_owned()); } let confirmed = state.confirmed.into_owned(); let primary = Vec::new(); @@ -258,17 +336,30 @@ impl GuardSet { guard_set } - /// Return true if `relay` is a member of this set. - fn contains_relay(&self, relay: &Relay<'_>) -> bool { - // Note: Could implement Borrow instead, but I don't think it'll - // matter. - let id = GuardId::from_relay_ids(relay); - self.contains(&id) + /// Return false if `relay` (or some other relay that shares an ID with it) + /// is a member if this set. + fn can_add_relay(&self, relay: &Relay<'_>) -> bool { + self.guards.all_overlapping(relay).is_empty() } - /// Return true if `id` is a member of this set. - pub(crate) fn contains(&self, id: &GuardId) -> bool { - self.guards.contains_key(id) + /// Return `Ok(true)` if `id` is definitely a member of this set, and + /// `Ok(false)` if it is definitely not a member. + /// + /// If we cannot tell, it's because there is a guard in this sample that has + /// a _subset_ of the IDs in `id`. In that case, we return + /// `Err(guard_ident)`, where `guard_ident` is the identity of that guard. + pub(crate) fn contains(&self, id: &GuardId) -> Result { + let overlapping = self.guards.all_overlapping(id); + match &overlapping[..] { + [singleton] => { + if singleton.has_all_relay_ids_from(id) { + Ok(true) + } else { + Err(singleton.guard_id()) + } + } + _ => Ok(false), + } } /// If there are not enough filter-permitted usable guards in this @@ -367,7 +458,7 @@ impl GuardSet { filter_ok && relay.is_flagged_guard() && relay.is_dir_cache() - && !self.contains_relay(relay) + && self.can_add_relay(relay) }, ); @@ -409,12 +500,12 @@ impl GuardSet { /// Does nothing if it is already a guard. fn add_guard(&mut self, relay: &Relay<'_>, now: SystemTime, params: &GuardParams) { let id = GuardId::from_relay_ids(relay); - if self.guards.contains_key(&id) { + if self.guards.by_all_ids(&id).is_some() { return; } debug!(guard_id=?id, "Adding guard to sample."); let guard = Guard::from_relay(relay, now, params); - self.guards.insert(id.clone(), guard); + self.guards.insert(guard); self.sample.push(id); self.primary_guards_invalidated = true; } @@ -425,7 +516,10 @@ impl GuardSet { self.primary .iter() .filter(|id| { - let g = self.guards.get(id).expect("Inconsistent guard state"); + let g = self + .guards + .by_all_ids(*id) + .expect("Inconsistent guard state"); g.listed_in(dir).is_none() }) .count() @@ -434,9 +528,16 @@ impl GuardSet { /// Update the status of every guard in this sample from a network /// directory. pub(crate) fn update_status_from_netdir(&mut self, dir: &NetDir) { - for g in self.guards.values_mut() { - g.update_from_netdir(dir); - } + let old_guards = std::mem::take(&mut self.guards); + self.guards = old_guards + .into_values() + .map(|mut guard| { + guard.update_from_netdir(dir); + guard + }) + .collect(); + // Call "fix consistency", in case any guards got a new ID. + self.fix_consistency(); } /// Re-build the list of primary guards. @@ -469,7 +570,10 @@ impl GuardSet { .unique() // We only consider usable guards that the filter allows. .filter_map(|id| { - let g = self.guards.get(id).expect("Inconsistent guard state"); + let g = self + .guards + .by_all_ids(id) + .expect("Inconsistent guard state"); if g.usable() && self.active_filter.permits(g) { Some(id.clone()) } else { @@ -486,9 +590,9 @@ impl GuardSet { // Clear exploratory_circ_pending for all primary guards. for id in &self.primary { - if let Some(guard) = self.guards.get_mut(id) { + self.guards.modify_by_all_ids(id, |guard| { guard.note_exploratory_circ(false); - } + }); } // TODO: Recalculate retry times, perhaps, since we may have changed @@ -503,11 +607,11 @@ impl GuardSet { pub(crate) fn expire_old_guards(&mut self, params: &GuardParams, now: SystemTime) { self.assert_consistency(); let n_pre = self.guards.len(); - self.guards.retain(|_, g| !g.is_expired(params, now)); - let guards = &self.guards; // to avoid borrowing issue - self.sample.retain(|id| guards.contains_key(id)); - self.confirmed.retain(|id| guards.contains_key(id)); - self.primary.retain(|id| guards.contains_key(id)); + self.guards.retain(|g| !g.is_expired(params, now)); + let guards = &self.guards; + self.sample.retain(|id| guards.by_all_ids(id).is_some()); + self.confirmed.retain(|id| guards.by_all_ids(id).is_some()); + self.primary.retain(|id| guards.by_all_ids(id).is_some()); self.assert_consistency(); if self.guards.len() < n_pre { @@ -521,7 +625,10 @@ impl GuardSet { /// is not known to be Unreachable. fn reachable_sample_ids(&self) -> impl Iterator { self.sample.iter().filter(move |id| { - let g = self.guards.get(id).expect("Inconsistent guard state"); + let g = self + .guards + .by_all_ids(*id) + .expect("Inconsistent guard state"); g.reachable() != Reachable::Unreachable }) } @@ -547,21 +654,30 @@ impl GuardSet { /// Like `preference_order_ids`, but yields `&Guard` instead of `&GuardId`. fn preference_order(&self) -> impl Iterator + '_ { self.preference_order_ids() - .filter_map(move |(p, id)| self.guards.get(id).map(|g| (p, g))) + .filter_map(move |(p, id)| self.guards.by_all_ids(id).map(|g| (p, g))) } - /// Return true if `guard_id` is the identity for a primary guard. + /// Return true if `guard_id` is an identity subset for any primary guard in this set. fn guard_is_primary(&self, guard_id: &GuardId) -> bool { + // (This could be yes/no/maybe.) + // This is O(n), but the list is short. - self.primary.contains(guard_id) + self.primary + .iter() + .any(|p| p.has_all_relay_ids_from(guard_id)) } /// For every guard that has been marked as `Unreachable` for too long, /// mark it as `Unknown`. pub(crate) fn consider_all_retries(&mut self, now: Instant) { - for guard in self.guards.values_mut() { - guard.consider_retry(now); - } + let old_guards = std::mem::take(&mut self.guards); + self.guards = old_guards + .into_values() + .map(|mut guard| { + guard.consider_retry(now); + guard + }) + .collect(); } /// Return the earliest time at which any guard will be retriable. @@ -575,9 +691,8 @@ impl GuardSet { /// Mark every `Unreachable` primary guard as `Unknown`. pub(crate) fn mark_primary_guards_retriable(&mut self) { for id in &self.primary { - if let Some(g) = self.guards.get_mut(id) { - g.mark_retriable(); - } + self.guards + .modify_by_all_ids(id, |guard| guard.mark_retriable()); } } @@ -586,28 +701,33 @@ impl GuardSet { pub(crate) fn all_primary_guards_are_unreachable(&mut self) -> bool { self.primary .iter() - .flat_map(|id| self.guards.get(id)) + .flat_map(|id| self.guards.by_all_ids(id)) .all(|g| g.reachable() == Reachable::Unreachable) } /// Mark every `Unreachable` guard as `Unknown`. pub(crate) fn mark_all_guards_retriable(&mut self) { - for g in self.guards.values_mut() { - g.mark_retriable(); - } + let old_guards = std::mem::take(&mut self.guards); + self.guards = old_guards + .into_values() + .map(|mut guard| { + guard.mark_retriable(); + guard + }) + .collect(); } /// Record that an attempt has begun to use the guard with /// `guard_id`. 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) { + self.guards.modify_by_all_ids(guard_id, |guard| { guard.record_attempt(now); if !is_primary { guard.note_exploratory_circ(true); } - } + }); } /// Record that an attempt to use the guard with `guard_id` has just @@ -623,20 +743,18 @@ impl GuardSet { now: SystemTime, ) { self.assert_consistency(); - if let Some(guard) = self.guards.get_mut(guard_id) { - match how { - Some(external) => guard.record_external_success(external), - None => { - let newly_confirmed = guard.record_success(now, params); + self.guards.modify_by_all_ids(guard_id, |guard| match how { + Some(external) => guard.record_external_success(external), + None => { + let newly_confirmed = guard.record_success(now, params); - if newly_confirmed == NewlyConfirmed::Yes { - self.confirmed.push(guard_id.clone()); - self.primary_guards_invalidated = true; - } + if newly_confirmed == NewlyConfirmed::Yes { + self.confirmed.push(guard_id.clone()); + self.primary_guards_invalidated = true; } } - self.assert_consistency(); - } + }); + self.assert_consistency(); } /// Record that an attempt to use the guard with `guard_id` has just failed. @@ -649,37 +767,33 @@ impl GuardSet { ) { // TODO use instant uniformly for in-process, and systemtime for storage? let is_primary = self.guard_is_primary(guard_id); - if let Some(guard) = self.guards.get_mut(guard_id) { - match how { - Some(external) => guard.record_external_failure(external, now), - None => guard.record_failure(now, is_primary), - } - } + self.guards.modify_by_all_ids(guard_id, |guard| match how { + Some(external) => guard.record_external_failure(external, now), + None => guard.record_failure(now, is_primary), + }); } /// 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: &GuardId) { - if let Some(guard) = self.guards.get_mut(guard_id) { - guard.note_exploratory_circ(false); - } + self.guards + .modify_by_all_ids(guard_id, |guard| guard.note_exploratory_circ(false)); } /// 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: &GuardId) { - if let Some(guard) = self.guards.get_mut(guard_id) { + self.guards.modify_by_all_ids(guard_id, |guard| { guard.note_exploratory_circ(false); guard.record_indeterminate_result(); - } + }); } /// Record that a given guard has told us about clock skew. pub(crate) fn record_skew(&mut self, guard_id: &GuardId, observation: SkewObservation) { - if let Some(guard) = self.guards.get_mut(guard_id) { - guard.note_skew(observation); - } + self.guards + .modify_by_all_ids(guard_id, |guard| guard.note_skew(observation)); } /// Return an iterator over all stored clock skew observations. @@ -956,12 +1070,12 @@ mod test { guards.assert_consistency(); // make sure all the guards are okay. - for (g, guard) in &guards.guards { - let id = FirstHopId::in_sample(GuardSetSelector::Default, g.clone()); + for guard in guards.guards.values() { + let id = FirstHopId::in_sample(GuardSetSelector::Default, guard.guard_id().clone()); let relay = id.get_relay(&netdir).unwrap(); assert!(relay.is_flagged_guard()); assert!(relay.is_dir_cache()); - assert!(guards.contains_relay(&relay)); + assert!(guards.guards.by_all_ids(&relay).is_some()); { assert!(!guard.is_expired(¶ms, SystemTime::now())); } @@ -1007,11 +1121,19 @@ mod test { assert_eq!(&guards2.confirmed, &guards.confirmed); assert_eq!(&guards2.confirmed, &[id1]); assert_eq!( - guards.guards.keys().collect::>(), - guards2.guards.keys().collect::>() + guards + .guards + .values() + .map(Guard::guard_id) + .collect::>(), + guards2 + .guards + .values() + .map(Guard::guard_id) + .collect::>() ); - for (k, g) in &guards.guards { - let g2 = guards2.guards.get(k).unwrap(); + for g in guards.guards.values() { + let g2 = guards2.guards.by_all_ids(g.guard_id()).unwrap(); assert_eq!(format!("{:?}", g), format!("{:?}", g2)); } } @@ -1363,7 +1485,7 @@ mod test { guards2.record_failure(&id2, None, Instant::now()); // Copy status: make sure non-persistent status changed, and persistent didn't. - guards1.copy_status_from(guards2); + guards1.copy_ephemeral_status_into_newly_loaded_state(guards2); { let g1 = guards1.get(&id1).unwrap(); let g2 = guards1.get(&id2).unwrap(); @@ -1376,14 +1498,22 @@ mod test { // Now make a new set of unrelated guards, and make sure that copying // from it doesn't change the membership of guards1. let mut guards3 = GuardSet::default(); - let g1_set: HashSet<_> = guards1.guards.keys().map(Clone::clone).collect(); + let g1_set: HashSet<_> = guards1 + .guards + .values() + .map(|g| g.guard_id().clone()) + .collect(); let mut g3_set: HashSet<_> = HashSet::new(); for _ in 0..4 { // There is roughly a 1-in-5000 chance of getting the same set // twice, so we loop until that doesn't happen. guards3.extend_sample_as_needed(SystemTime::now(), ¶ms, &netdir); guards3.select_primary_guards(¶ms); - g3_set = guards3.guards.keys().map(Clone::clone).collect(); + g3_set = guards3 + .guards + .values() + .map(|g| g.guard_id().clone()) + .collect(); // There is roughly a 1-in-5000 chance of getting the same set twice, so if g1_set == g3_set { @@ -1394,8 +1524,12 @@ mod test { } assert_ne!(g1_set, g3_set); // Do the copy; make sure that the membership is unchanged. - guards1.copy_status_from(guards3); - let g1_set_new: HashSet<_> = guards1.guards.keys().map(Clone::clone).collect(); + guards1.copy_ephemeral_status_into_newly_loaded_state(guards3); + let g1_set_new: HashSet<_> = guards1 + .guards + .values() + .map(|g| g.guard_id().clone()) + .collect(); assert_eq!(g1_set, g1_set_new); } } diff --git a/crates/tor-linkspec/src/ids/by_id.rs b/crates/tor-linkspec/src/ids/by_id.rs index e486c02d5..21d8d433b 100644 --- a/crates/tor-linkspec/src/ids/by_id.rs +++ b/crates/tor-linkspec/src/ids/by_id.rs @@ -51,6 +51,20 @@ impl ByRelayIds { } } + /// Modify the value in this set (if any) that has the key `key`. + /// + /// Return values are as for [`modify_by_ed25519`](Self::modify_by_ed25519) + pub fn modify_by_id<'a, T, F>(&mut self, key: T, func: F) -> Vec + where + T: Into>, + F: FnOnce(&mut H), + { + match key.into() { + RelayIdRef::Ed25519(ed) => self.modify_by_ed25519(ed, func), + RelayIdRef::Rsa(rsa) => self.modify_by_rsa(rsa, func), + } + } + /// Return the value in this set (if any) that has _all_ the relay IDs /// that `key` does. /// @@ -64,6 +78,26 @@ impl ByRelayIds { .filter(|val| val.has_all_relay_ids_from(key)) } + /// Modify the value in this set (if any) that has _all_ the relay IDs + /// that `key` does. + /// + /// Return values are as for [`modify_by_ed25519`](Self::modify_by_ed25519) + pub fn modify_by_all_ids(&mut self, key: &T, func: F) -> Vec + where + T: HasRelayIds, + F: FnOnce(&mut H), + { + let any_id = match key.identities().next() { + Some(id) => id, + None => return Vec::new(), + }; + self.modify_by_id(any_id, |val| { + if val.has_all_relay_ids_from(key) { + func(val); + } + }) + } + /// Remove the single value in this set (if any) that has _exactly the same_ /// relay IDs that `key` does pub fn remove_exact(&mut self, key: &T) -> Option @@ -82,6 +116,23 @@ impl ByRelayIds { } } + /// Remove the single value in this set (if any) that has all the same relay ids that + /// relay IDs that `key` does. + pub fn remove_by_all_ids(&mut self, key: &T) -> Option + where + T: HasRelayIds, + { + let any_id = key.identities().next()?; + if self + .by_id(any_id) + .filter(|ent| ent.has_all_relay_ids_from(key)) + .is_some() + { + self.remove_by_id(any_id) + } else { + None + } + } /// Return a reference to every element in this set that shares _any_ ID /// with `key`. /// diff --git a/crates/tor-linkspec/src/traits.rs b/crates/tor-linkspec/src/traits.rs index a3eff6511..fcf2328b4 100644 --- a/crates/tor-linkspec/src/traits.rs +++ b/crates/tor-linkspec/src/traits.rs @@ -2,6 +2,7 @@ //! that Tor can connect to, directly or indirectly. use std::{iter::FusedIterator, net::SocketAddr}; +use strum::IntoEnumIterator; use tor_llcrypto::pk; use crate::{ChannelMethod, OwnedChanTarget, RelayIdRef, RelayIdType, RelayIdTypeIter}; @@ -61,6 +62,11 @@ pub trait HasRelayIds { self.identity(id.id_type()).map(|my_id| my_id == id) == Some(true) } + /// Return true if this object has any known identity. + fn has_any_identity(&self) -> bool { + RelayIdType::iter().any(|id_type| self.identity(id_type).is_some()) + } + /// Return true if this object has exactly the same relay IDs as `other`. // // TODO: Once we make it so particular identity key types are optional, we @@ -101,7 +107,6 @@ pub trait HasRelayIds { /// If additional identities are added in the future, they may taken into /// consideration before _or_ after the current identity types. fn cmp_by_relay_ids(&self, other: &T) -> std::cmp::Ordering { - use strum::IntoEnumIterator; for key_type in RelayIdType::iter() { let ordering = Ord::cmp(&self.identity(key_type), &other.identity(key_type)); if ordering.is_ne() {