From deaf8b657d7b84cc1fd1ad89c5b25afd71062ebf Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Thu, 4 Aug 2022 10:07:54 -0400 Subject: [PATCH] Teach guard restrictions about RelayId. This implementation is (sadly) too copy-heavy or now, because HashSet can't be indexed with RelayIdRef. --- crates/tor-circmgr/src/path/exitpath.rs | 9 +++++-- crates/tor-guardmgr/src/guard.rs | 31 ++++++++++++++++--------- crates/tor-guardmgr/src/lib.rs | 9 ++++--- crates/tor-linkspec/src/ids.rs | 16 +++++++++++++ crates/tor-linkspec/src/traits.rs | 11 +++++++++ 5 files changed, 60 insertions(+), 16 deletions(-) diff --git a/crates/tor-circmgr/src/path/exitpath.rs b/crates/tor-circmgr/src/path/exitpath.rs index 1b9064755..efa4941cb 100644 --- a/crates/tor-circmgr/src/path/exitpath.rs +++ b/crates/tor-circmgr/src/path/exitpath.rs @@ -7,6 +7,7 @@ use std::time::SystemTime; use tor_basic_utils::iter::FilterCount; use tor_error::{bad_api_usage, internal}; use tor_guardmgr::{GuardMgr, GuardMonitor, GuardUsable}; +use tor_linkspec::RelayId; use tor_netdir::{NetDir, Relay, SubnetConfig, WeightRole}; use tor_rtcompat::Runtime; @@ -166,9 +167,13 @@ impl<'a> ExitPathBuilder<'a> { guardmgr.update_network(netdir); // possibly unnecessary. if let Some(exit_relay) = chosen_exit { let mut family = std::collections::HashSet::new(); - family.insert(*exit_relay.id()); + family.insert(RelayId::from(*exit_relay.id())); // TODO(nickm): See "limitations" note on `known_family_members`. - family.extend(netdir.known_family_members(exit_relay).map(|r| *r.id())); + family.extend( + netdir + .known_family_members(exit_relay) + .map(|r| RelayId::from(*r.id())), + ); b.restrictions() .push(tor_guardmgr::GuardRestriction::AvoidAllIds(family)); } diff --git a/crates/tor-guardmgr/src/guard.rs b/crates/tor-guardmgr/src/guard.rs index 9c926fca4..c66577f8c 100644 --- a/crates/tor-guardmgr/src/guard.rs +++ b/crates/tor-guardmgr/src/guard.rs @@ -393,8 +393,15 @@ 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.0.ed_identity() != ed, - GuardRestriction::AvoidAllIds(ids) => !ids.contains(self.id.0.ed_identity()), + GuardRestriction::AvoidId(avoid_id) => !self.id.0.has_identity(avoid_id.as_ref()), + GuardRestriction::AvoidAllIds(avoid_ids) => { + // TODO(nickm): This copies all of our IDs! + // We should use a contains method on a RelayIdSet or something. + self.id + .0 + .identities() + .all(|id| !avoid_ids.contains(&id.to_owned())) + } } } @@ -788,7 +795,7 @@ impl CircHistory { mod test { #![allow(clippy::unwrap_used)] use super::*; - use tor_linkspec::HasRelayIds; + use tor_linkspec::{HasRelayIds, RelayId}; #[test] fn crate_id() { @@ -809,6 +816,9 @@ mod test { #[test] fn simple_accessors() { + fn ed(id: [u8; 32]) -> RelayId { + RelayId::Ed25519(id.into()) + } let id = basic_id(); let g = basic_guard(); @@ -820,34 +830,33 @@ mod test { use crate::GuardUsageBuilder; let mut usage1 = GuardUsageBuilder::new(); + usage1 .restrictions() - .push(GuardRestriction::AvoidId([22; 32].into())); + .push(GuardRestriction::AvoidId(ed([22; 32]))); let usage1 = usage1.build().unwrap(); let mut usage2 = GuardUsageBuilder::new(); usage2 .restrictions() - .push(GuardRestriction::AvoidId([13; 32].into())); + .push(GuardRestriction::AvoidId(ed([13; 32]))); let usage2 = usage2.build().unwrap(); let usage3 = GuardUsage::default(); let mut usage4 = GuardUsageBuilder::new(); usage4 .restrictions() - .push(GuardRestriction::AvoidId([22; 32].into())); + .push(GuardRestriction::AvoidId(ed([22; 32]))); usage4 .restrictions() - .push(GuardRestriction::AvoidId([13; 32].into())); + .push(GuardRestriction::AvoidId(ed([13; 32]))); let usage4 = usage4.build().unwrap(); let mut usage5 = GuardUsageBuilder::new(); usage5.restrictions().push(GuardRestriction::AvoidAllIds( - vec![[22; 32].into(), [13; 32].into()].into_iter().collect(), + vec![ed([22; 32]), ed([13; 32])].into_iter().collect(), )); let usage5 = usage5.build().unwrap(); let mut usage6 = GuardUsageBuilder::new(); usage6.restrictions().push(GuardRestriction::AvoidAllIds( - vec![[99; 32].into(), [100; 32].into()] - .into_iter() - .collect(), + vec![ed([99; 32]), ed([100; 32])].into_iter().collect(), )); let usage6 = usage6.build().unwrap(); diff --git a/crates/tor-guardmgr/src/lib.rs b/crates/tor-guardmgr/src/lib.rs index 9c0292dc1..69780ed04 100644 --- a/crates/tor-guardmgr/src/lib.rs +++ b/crates/tor-guardmgr/src/lib.rs @@ -141,6 +141,7 @@ use std::collections::{HashMap, HashSet}; use std::net::SocketAddr; use std::sync::{Arc, Mutex, Weak}; use std::time::{Duration, Instant, SystemTime}; +use tor_linkspec::RelayId; use tor_netdir::NetDirProvider; use tor_proto::ClockSkew; use tracing::{debug, info, trace, warn}; @@ -1410,10 +1411,12 @@ impl GuardUsageBuilder { #[derive(Clone, Debug, Serialize, Deserialize)] #[non_exhaustive] pub enum GuardRestriction { - /// Don't pick a guard with the provided Ed25519 identity. - AvoidId(pk::ed25519::Ed25519Identity), + /// Don't pick a guard with the provided identity. + AvoidId(RelayId), /// Don't pick a guard with any of the provided Ed25519 identities. - AvoidAllIds(HashSet), + // + // TODO(nickm): Switch this to a type that can actually work well with RelayId. + AvoidAllIds(HashSet), } #[cfg(test)] diff --git a/crates/tor-linkspec/src/ids.rs b/crates/tor-linkspec/src/ids.rs index 0cc1e4a9e..e94ff9554 100644 --- a/crates/tor-linkspec/src/ids.rs +++ b/crates/tor-linkspec/src/ids.rs @@ -94,6 +94,14 @@ impl RelayId { .into(), }) } + + /// Return the type of this relay identity. + pub fn id_type(&self) -> RelayIdType { + match self { + RelayId::Ed25519(_) => RelayIdType::Ed25519, + RelayId::Rsa(_) => RelayIdType::Rsa, + } + } } impl<'a> RelayIdRef<'a> { @@ -107,6 +115,14 @@ impl<'a> RelayIdRef<'a> { RelayIdRef::Rsa(key) => (*key).into(), } } + + /// Return the type of this relay identity. + pub fn id_type(&self) -> RelayIdType { + match self { + RelayIdRef::Ed25519(_) => RelayIdType::Ed25519, + RelayIdRef::Rsa(_) => RelayIdType::Rsa, + } + } } /// Expand to an implementation for PartialEq for a given key type. diff --git a/crates/tor-linkspec/src/traits.rs b/crates/tor-linkspec/src/traits.rs index 315dfdae3..aca4bd225 100644 --- a/crates/tor-linkspec/src/traits.rs +++ b/crates/tor-linkspec/src/traits.rs @@ -26,6 +26,17 @@ pub trait HasRelayIds { next_key: RelayIdType::all_types(), } } + /// Check whether the provided Id is a known identity of this relay. + /// + /// Remember that a given set of identity keys may be incomplete: some + /// objects that represent a relay have only a subset of the relay's + /// identities. Therefore, a "true" answer means that the relay has this + /// identity, but a "false" answer could mean that the relay has a + /// different identity of this type, or that it has _no_ known identity of + /// this type. + fn has_identity(&self, id: RelayIdRef<'_>) -> bool { + self.identity(id.id_type()).map(|my_id| my_id == id) == Some(true) + } /// Return the ed25519 identity for this relay. fn ed_identity(&self) -> &pk::ed25519::Ed25519Identity;