diff --git a/crates/tor-chanmgr/src/builder.rs b/crates/tor-chanmgr/src/builder.rs index 54e610106..8d138b826 100644 --- a/crates/tor-chanmgr/src/builder.rs +++ b/crates/tor-chanmgr/src/builder.rs @@ -9,7 +9,7 @@ use crate::{event::ChanMgrEventSender, Error}; use std::result::Result as StdResult; use std::time::Duration; use tor_error::{bad_api_usage, internal}; -use tor_linkspec::{HasAddrs, OwnedChanTarget}; +use tor_linkspec::{HasAddrs, HasRelayIds, OwnedChanTarget}; use tor_llcrypto::pk; use tor_proto::channel::params::ChannelsParamsUpdates; use tor_rtcompat::{tls::TlsConnector, Runtime, TcpProvider, TlsProvider}; @@ -242,7 +242,9 @@ impl ChanBuilder { impl crate::mgr::AbstractChannel for tor_proto::channel::Channel { type Ident = pk::ed25519::Ed25519Identity; fn ident(&self) -> &Self::Ident { - self.peer_ed25519_id() + self.target() + .ed_identity() + .expect("This channel had an Ed25519 identity when we created it, but now it doesn't!?") } fn is_usable(&self) -> bool { !self.is_closing() diff --git a/crates/tor-chanmgr/src/err.rs b/crates/tor-chanmgr/src/err.rs index 09c3c9612..25880dffc 100644 --- a/crates/tor-chanmgr/src/err.rs +++ b/crates/tor-chanmgr/src/err.rs @@ -76,6 +76,14 @@ pub enum Error { #[source] cause: Arc, }, + + /// A relay did not have the set of identity keys that we expected. + /// + /// (Currently, `tor-chanmgr` only works to manage channels with a known + /// expected Ed25519 identity.) + #[error("Could not identify relay by identity key")] + MissingId, + /// An internal error of some kind that should never occur. #[error("Internal error")] Internal(#[from] tor_error::Bug), @@ -103,6 +111,7 @@ impl tor_error::HasKind for Error { E::Proto { source, .. } => source.kind(), E::PendingFailed { .. } => EK::TorAccessFailed, E::UnusableTarget(_) | E::Internal(_) => EK::Internal, + E::MissingId => EK::BadApiUsage, Error::ChannelBuild { .. } => EK::TorAccessFailed, } } @@ -134,7 +143,7 @@ impl tor_error::HasRetryTime for Error { E::UnusableTarget(_) => RT::Never, // These aren't recoverable at all. - E::Spawn { .. } | E::Internal(_) => RT::Never, + E::Spawn { .. } | E::MissingId | E::Internal(_) => RT::Never, } } } diff --git a/crates/tor-chanmgr/src/lib.rs b/crates/tor-chanmgr/src/lib.rs index 62342416c..ec6657618 100644 --- a/crates/tor-chanmgr/src/lib.rs +++ b/crates/tor-chanmgr/src/lib.rs @@ -155,7 +155,12 @@ impl ChanMgr { &self, target: &T, ) -> Result<(Channel, ChanProvenance)> { - let ed_identity = target.ed_identity(); + // TODO(nickm): We will need to change the way that we index our map + // when we eventually support channels that are _not_ primarily + // identified by their ed25519 key. That could be in the distant future + // when we make Ed25519 keys optional: But more likely it will be when + // we implement bridges. + let ed_identity = target.ed_identity().ok_or(Error::MissingId)?; let targetinfo = OwnedChanTarget::from_chan_target(target); let (chan, provenance) = self.mgr.get_or_launch(*ed_identity, targetinfo).await?; diff --git a/crates/tor-circmgr/src/path/dirpath.rs b/crates/tor-circmgr/src/path/dirpath.rs index 144cb7d52..590cf97e7 100644 --- a/crates/tor-circmgr/src/path/dirpath.rs +++ b/crates/tor-circmgr/src/path/dirpath.rs @@ -100,7 +100,7 @@ mod test { use std::collections::HashSet; use tor_basic_utils::test_rng::testing_rng; use tor_guardmgr::fallback::{FallbackDir, FallbackList}; - use tor_linkspec::HasRelayIds; + use tor_linkspec::RelayIds; use tor_netdir::testnet; #[test] @@ -200,7 +200,7 @@ mod test { .pick_path(&mut rng, dirinfo, Some(&guards)) .unwrap(); if let crate::path::TorPathInner::OwnedOneHop(relay) = path.inner { - distinct_guards.insert(relay.ed_identity().clone()); + distinct_guards.insert(RelayIds::from_relay_ids(&relay)); mon.unwrap().succeeded(); assert!(usable.unwrap().await.unwrap()); } else { diff --git a/crates/tor-circmgr/src/path/exitpath.rs b/crates/tor-circmgr/src/path/exitpath.rs index f7efdeb34..bdd09500a 100644 --- a/crates/tor-circmgr/src/path/exitpath.rs +++ b/crates/tor-circmgr/src/path/exitpath.rs @@ -263,7 +263,7 @@ mod test { use crate::test::OptDummyGuardMgr; use std::collections::HashSet; use tor_basic_utils::test_rng::testing_rng; - use tor_linkspec::HasRelayIds; + use tor_linkspec::{HasRelayIds, RelayIds}; use tor_netdir::testnet; use tor_rtcompat::SleepProvider; @@ -431,9 +431,9 @@ mod test { assert_same_path_when_owned(&path); if let TorPathInner::Path(p) = path.inner { assert_exit_path_ok(&p[..]); - distinct_guards.insert(p[0].ed_identity().clone()); - distinct_mid.insert(p[1].ed_identity().clone()); - distinct_exit.insert(p[2].ed_identity().clone()); + distinct_guards.insert(RelayIds::from_relay_ids(&p[0])); + distinct_mid.insert(RelayIds::from_relay_ids(&p[1])); + distinct_exit.insert(RelayIds::from_relay_ids(&p[2])); } else { panic!("Wrong kind of path"); } @@ -450,9 +450,9 @@ mod test { assert_ne!(distinct_exit.len(), 1); let guard_relay = netdir - .by_id(distinct_guards.iter().next().unwrap()) + .by_ids(distinct_guards.iter().next().unwrap()) .unwrap(); - let exit_relay = netdir.by_id(distinct_exit.iter().next().unwrap()).unwrap(); + let exit_relay = netdir.by_ids(distinct_exit.iter().next().unwrap()).unwrap(); // Now we'll try a forced exit that is not the same as our // actual guard. diff --git a/crates/tor-guardmgr/src/fallback.rs b/crates/tor-guardmgr/src/fallback.rs index 816ef1708..493422451 100644 --- a/crates/tor-guardmgr/src/fallback.rs +++ b/crates/tor-guardmgr/src/fallback.rs @@ -58,7 +58,7 @@ impl FallbackDir { /// Return a copy of this FallbackDir as a [`FirstHop`](crate::FirstHop) pub fn as_guard(&self) -> crate::FirstHop { crate::FirstHop { - id: FallbackId::from_chan_target(self).into(), + id: FallbackId::from_relay_ids(self).into(), orports: self.orports.clone(), } } @@ -119,7 +119,7 @@ impl tor_linkspec::HasAddrs for FallbackDir { &self.orports[..] } } -impl tor_linkspec::HasRelayIds for FallbackDir { +impl tor_linkspec::HasRelayIdsLegacy for FallbackDir { fn ed_identity(&self) -> &Ed25519Identity { &self.ed_identity } diff --git a/crates/tor-guardmgr/src/fallback/set.rs b/crates/tor-guardmgr/src/fallback/set.rs index 2034f0c69..b8fa3b542 100644 --- a/crates/tor-guardmgr/src/fallback/set.rs +++ b/crates/tor-guardmgr/src/fallback/set.rs @@ -278,7 +278,7 @@ mod test { rand_fb(&mut rng), ]; let fb_other = rand_fb(&mut rng); - let id_other = FallbackId::from_chan_target(&fb_other); + let id_other = FallbackId::from_relay_ids(&fb_other); // basic case: construct a set let list: FallbackList = fbs.clone().into(); @@ -294,7 +294,7 @@ mod test { // use the constructed set a little. for fb in fbs.iter() { - let id = FallbackId::from_chan_target(fb); + let id = FallbackId::from_relay_ids(fb); assert_eq!(set.get_mut(&id).unwrap().id(), &id); } assert!(set.get_mut(&id_other).is_none()); @@ -431,7 +431,7 @@ mod test { let mut fbs2: Vec<_> = fbs .into_iter() // (Remove the fallback with id==ids[2]) - .filter(|fb| FallbackId::from_chan_target(fb) != ids[2]) + .filter(|fb| FallbackId::from_relay_ids(fb) != ids[2]) .collect(); // add 2 new ones. let fbs_new = vec![rand_fb(&mut rng), rand_fb(&mut rng), rand_fb(&mut rng)]; @@ -450,7 +450,7 @@ mod test { // Make sure that the new fbs are there. for new_fb in fbs_new { assert!(set2 - .get_mut(&FallbackId::from_chan_target(&new_fb)) + .get_mut(&FallbackId::from_relay_ids(&new_fb)) .unwrap() .status .usable_at(now)); diff --git a/crates/tor-guardmgr/src/guard.rs b/crates/tor-guardmgr/src/guard.rs index f228384b4..1f5c7bbcf 100644 --- a/crates/tor-guardmgr/src/guard.rs +++ b/crates/tor-guardmgr/src/guard.rs @@ -1,7 +1,6 @@ //! Code to represent its single guard node and track its status. use tor_basic_utils::retry::RetryDelay; -use tor_llcrypto::pk::{ed25519::Ed25519Identity, rsa::RsaIdentity}; use tor_netdir::{NetDir, Relay, RelayWeight}; use educe::Educe; @@ -220,7 +219,7 @@ impl Guard { ); Self::new( - GuardId::from_chan_target(relay), + GuardId::from_relay_ids(relay), relay.addrs().into(), added_at, ) @@ -659,7 +658,7 @@ 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.0.rsa_identity(), tor_netdir::WeightRole::Guard) + dir.weight_by_rsa_id(self.id.0.rsa_identity()?, tor_netdir::WeightRole::Guard) } /// Return a [`FirstHop`](crate::FirstHop) object to represent this guard. @@ -695,11 +694,11 @@ impl tor_linkspec::HasAddrs for Guard { } impl tor_linkspec::HasRelayIds for Guard { - fn ed_identity(&self) -> &Ed25519Identity { - self.id.0.ed_identity() - } - fn rsa_identity(&self) -> &RsaIdentity { - self.id.0.rsa_identity() + fn identity( + &self, + key_type: tor_linkspec::RelayIdType, + ) -> Option> { + self.id.0.identity(key_type) } } diff --git a/crates/tor-guardmgr/src/ids.rs b/crates/tor-guardmgr/src/ids.rs index c4d6bd319..58046c156 100644 --- a/crates/tor-guardmgr/src/ids.rs +++ b/crates/tor-guardmgr/src/ids.rs @@ -2,7 +2,8 @@ use derive_more::AsRef; use serde::{Deserialize, Serialize}; -use tor_linkspec::{HasRelayIds, RelayIds}; +use tor_linkspec::RelayIds; +#[cfg(test)] use tor_llcrypto::pk; /// An identifier for a fallback directory cache. @@ -14,11 +15,8 @@ pub(crate) struct FallbackId(pub(crate) RelayIds); impl FallbackId { /// Return a new, manually constructed `FallbackId` - pub(crate) fn new(ed25519: pk::ed25519::Ed25519Identity, rsa: pk::rsa::RsaIdentity) -> Self { - Self(RelayIds::new(ed25519, rsa)) - } /// Extract a `FallbackId` from a ChanTarget object. - pub(crate) fn from_chan_target(target: &T) -> Self { + pub(crate) fn from_relay_ids(target: &T) -> Self { Self(RelayIds::from_relay_ids(target)) } } @@ -33,12 +31,13 @@ pub(crate) struct GuardId(pub(crate) RelayIds); impl GuardId { /// Return a new, manually constructed `GuardId` + #[cfg(test)] pub(crate) fn new(ed25519: pk::ed25519::Ed25519Identity, rsa: pk::rsa::RsaIdentity) -> Self { Self(RelayIds::new(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()) + pub(crate) fn from_relay_ids(target: &T) -> Self { + Self(RelayIds::from_relay_ids(target)) } } @@ -56,8 +55,12 @@ pub(crate) enum FirstHopIdInner { /// 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.) +/// (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); @@ -83,13 +86,12 @@ impl AsRef for FirstHopId { } } } -impl HasRelayIds for FirstHopId { - fn ed_identity(&self) -> &pk::ed25519::Ed25519Identity { - self.as_ref().ed_identity() - } - - fn rsa_identity(&self) -> &pk::rsa::RsaIdentity { - self.as_ref().rsa_identity() +impl tor_linkspec::HasRelayIds for FirstHopId { + fn identity( + &self, + key_type: tor_linkspec::RelayIdType, + ) -> Option> { + self.as_ref().identity(key_type) } } diff --git a/crates/tor-guardmgr/src/lib.rs b/crates/tor-guardmgr/src/lib.rs index 5ecc32eac..902834b1f 100644 --- a/crates/tor-guardmgr/src/lib.rs +++ b/crates/tor-guardmgr/src/lib.rs @@ -148,7 +148,6 @@ use tracing::{debug, info, trace, warn}; use tor_config::impl_standard_builder; use tor_config::{define_list_builder_accessors, define_list_builder_helper}; -use tor_llcrypto::pk; use tor_netdir::{params::NetParameters, NetDir, Relay}; use tor_persist::{DynStorageHandle, StateMgr}; use tor_rtcompat::Runtime; @@ -579,11 +578,9 @@ impl GuardMgr { identity: &impl tor_linkspec::HasRelayIds, external_failure: ExternalActivity, ) { - let ed_identity = identity.ed_identity(); - let rsa_identity = identity.rsa_identity(); let now = self.runtime.now(); let mut inner = self.inner.lock().expect("Poisoned lock"); - let ids = inner.lookup_ids(ed_identity, rsa_identity); + let ids = inner.lookup_ids(identity); for id in ids { match &id.0 { FirstHopIdInner::Guard(id) => { @@ -609,17 +606,9 @@ impl GuardMgr { identity: &impl tor_linkspec::HasRelayIds, external_activity: ExternalActivity, ) { - let ed_identity = identity.ed_identity(); - let rsa_identity = identity.rsa_identity(); - let mut inner = self.inner.lock().expect("Poisoned lock"); - inner.record_external_success( - ed_identity, - rsa_identity, - external_activity, - self.runtime.wallclock(), - ); + inner.record_external_success(identity, external_activity, self.runtime.wallclock()); } /// Return a stream of events about our estimated clock skew; these events @@ -984,12 +973,11 @@ impl GuardMgrInner { /// we have `mut self` borrowed.) fn record_external_success( &mut self, - ed_identity: &pk::ed25519::Ed25519Identity, - rsa_identity: &pk::rsa::RsaIdentity, + identity: &impl tor_linkspec::HasRelayIds, external_activity: ExternalActivity, now: SystemTime, ) { - for id in self.lookup_ids(ed_identity, rsa_identity) { + for id in self.lookup_ids(identity) { match &id.0 { FirstHopIdInner::Guard(id) => { self.guards.active_guards_mut().record_success( @@ -1104,19 +1092,15 @@ 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. - fn lookup_ids( - &self, - ed_identity: &pk::ed25519::Ed25519Identity, - rsa_identity: &pk::rsa::RsaIdentity, - ) -> Vec { + fn lookup_ids(&self, identity: &impl tor_linkspec::HasRelayIds) -> Vec { let mut vec = Vec::with_capacity(2); - let id = ids::GuardId::new(*ed_identity, *rsa_identity); + let id = ids::GuardId::from_relay_ids(identity); if self.guards.active_guards().contains(&id) { vec.push(id.into()); } - let id = ids::FallbackId::new(*ed_identity, *rsa_identity); + let id = ids::FallbackId::from_relay_ids(identity); if self.fallbacks.contains(&id) { vec.push(id.into()); } @@ -1329,11 +1313,11 @@ impl tor_linkspec::HasAddrs for FirstHop { } } impl tor_linkspec::HasRelayIds for FirstHop { - fn ed_identity(&self) -> &pk::ed25519::Ed25519Identity { - self.id.ed_identity() - } - fn rsa_identity(&self) -> &pk::rsa::RsaIdentity { - self.id.rsa_identity() + fn identity( + &self, + key_type: tor_linkspec::RelayIdType, + ) -> Option> { + self.id.identity(key_type) } } impl tor_linkspec::ChanTarget for FirstHop {} diff --git a/crates/tor-guardmgr/src/sample.rs b/crates/tor-guardmgr/src/sample.rs index bbc7dceb8..0f393129a 100644 --- a/crates/tor-guardmgr/src/sample.rs +++ b/crates/tor-guardmgr/src/sample.rs @@ -258,7 +258,7 @@ impl GuardSet { fn contains_relay(&self, relay: &Relay<'_>) -> bool { // Note: Could implement Borrow instead, but I don't think it'll // matter. - let id = GuardId::from_chan_target(relay); + let id = GuardId::from_relay_ids(relay); self.contains(&id) } @@ -404,7 +404,7 @@ 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_chan_target(relay); + let id = GuardId::from_relay_ids(relay); if self.guards.contains_key(&id) { return; } diff --git a/crates/tor-linkspec/src/ids.rs b/crates/tor-linkspec/src/ids.rs index 993bb6bda..509c194c0 100644 --- a/crates/tor-linkspec/src/ids.rs +++ b/crates/tor-linkspec/src/ids.rs @@ -116,30 +116,6 @@ impl RelayId { RelayId::Rsa(key) => key.as_bytes(), } } - - /// Extract the RsaIdentity from a RelayId that is known to hold one. - /// - /// # Panics - /// - /// Panics if this is not an RSA identity. - pub(crate) fn unwrap_rsa(self) -> RsaIdentity { - match self { - RelayId::Rsa(rsa) => rsa, - _ => panic!("Not an RSA identity."), - } - } - - /// Extract the Ed25519Identity from a RelayId that is known to hold one. - /// - /// # Panics - /// - /// Panics if this is not an Ed25519 identity. - pub(crate) fn unwrap_ed25519(self) -> Ed25519Identity { - match self { - RelayId::Ed25519(ed25519) => ed25519, - _ => panic!("Not an Ed25519 identity."), - } - } } impl<'a> RelayIdRef<'a> { diff --git a/crates/tor-linkspec/src/lib.rs b/crates/tor-linkspec/src/lib.rs index 526bf0c6e..45031ade2 100644 --- a/crates/tor-linkspec/src/lib.rs +++ b/crates/tor-linkspec/src/lib.rs @@ -80,4 +80,4 @@ mod traits; pub use ids::{set::RelayIdSet, RelayId, RelayIdError, RelayIdRef, RelayIdType, RelayIdTypeIter}; pub use ls::LinkSpec; pub use owned::{OwnedChanTarget, OwnedCircTarget, RelayIds}; -pub use traits::{ChanTarget, CircTarget, HasAddrs, HasRelayIds}; +pub use traits::{ChanTarget, CircTarget, HasAddrs, HasRelayIds, HasRelayIdsLegacy}; diff --git a/crates/tor-linkspec/src/owned.rs b/crates/tor-linkspec/src/owned.rs index 285b65dcc..d7b4d4a82 100644 --- a/crates/tor-linkspec/src/owned.rs +++ b/crates/tor-linkspec/src/owned.rs @@ -5,26 +5,29 @@ use std::fmt::{self, Display}; use std::net::SocketAddr; use tor_llcrypto::pk; -use crate::{ChanTarget, CircTarget, HasAddrs, HasRelayIds}; +use crate::{ChanTarget, CircTarget, HasAddrs, HasRelayIds, RelayIdRef, RelayIdType}; -/// RelayIds is an owned copy of the set of identities of a relay. +/// RelayIds is an owned copy of the set of known identities of a relay. +/// +/// Note that an object of this type will not necessarily have every type of +/// identity: it's possible that we don't know all the identities, or that one +/// of the identity types has become optional. #[derive(Debug, Clone, Eq, PartialEq, Hash, PartialOrd, Ord, Serialize, Deserialize)] pub struct RelayIds { /// Copy of the ed25519 id from the underlying ChanTarget. #[serde(rename = "ed25519")] - ed_identity: pk::ed25519::Ed25519Identity, + ed_identity: Option, /// Copy of the rsa id from the underlying ChanTarget. #[serde(rename = "rsa")] - rsa_identity: pk::rsa::RsaIdentity, + rsa_identity: Option, } impl HasRelayIds for RelayIds { - fn ed_identity(&self) -> &pk::ed25519::Ed25519Identity { - &self.ed_identity - } - - fn rsa_identity(&self) -> &pk::rsa::RsaIdentity { - &self.rsa_identity + fn identity(&self, key_type: RelayIdType) -> Option> { + match key_type { + RelayIdType::Ed25519 => self.ed_identity.as_ref().map(RelayIdRef::from), + RelayIdType::Rsa => self.rsa_identity.as_ref().map(RelayIdRef::from), + } } } @@ -35,15 +38,23 @@ impl RelayIds { rsa_identity: pk::rsa::RsaIdentity, ) -> Self { Self { - ed_identity, - rsa_identity, + ed_identity: Some(ed_identity), + rsa_identity: Some(rsa_identity), } } - /// Construct a new RelayIds object from another object that impements - /// [`HasRelayIds`] + /// Construct a new `RelayIds` object from another object that implements + /// [`HasRelayIds`]. + /// + /// Note that it is possible to construct an _empty_ `RelayIds` object if + /// the input does not contain any recognized identity type. pub fn from_relay_ids(other: &T) -> Self { - Self::new(*other.ed_identity(), *other.rsa_identity()) + Self { + ed_identity: other + .identity(RelayIdType::Ed25519) + .map(|r| *r.unwrap_ed25519()), + rsa_identity: other.identity(RelayIdType::Rsa).map(|r| *r.unwrap_rsa()), + } } } @@ -64,12 +75,8 @@ impl HasAddrs for OwnedChanTarget { } impl HasRelayIds for OwnedChanTarget { - fn ed_identity(&self) -> &pk::ed25519::Ed25519Identity { - self.ids.ed_identity() - } - - fn rsa_identity(&self) -> &pk::rsa::RsaIdentity { - self.ids.rsa_identity() + fn identity(&self, key_type: RelayIdType) -> Option> { + self.ids.identity(key_type) } } @@ -124,7 +131,9 @@ impl Display for OwnedChanTarget { [a] => write!(f, "{}", a)?, [a, ..] => write!(f, "{}+", a)?, }; - write!(f, "{}", self.ed_identity())?; // short enough to print + for ident in self.identities() { + write!(f, " {}", ident)?; + } write!(f, "]")?; Ok(()) } @@ -186,11 +195,8 @@ impl HasAddrs for OwnedCircTarget { } impl HasRelayIds for OwnedCircTarget { - fn ed_identity(&self) -> &pk::ed25519::Ed25519Identity { - self.chan_target.ed_identity() - } - fn rsa_identity(&self) -> &pk::rsa::RsaIdentity { - self.chan_target.rsa_identity() + fn identity(&self, key_type: RelayIdType) -> Option> { + self.chan_target.identity(key_type) } } diff --git a/crates/tor-linkspec/src/traits.rs b/crates/tor-linkspec/src/traits.rs index d0bfdc98c..ec0018231 100644 --- a/crates/tor-linkspec/src/traits.rs +++ b/crates/tor-linkspec/src/traits.rs @@ -6,19 +6,30 @@ use tor_llcrypto::pk; use crate::{RelayIdRef, RelayIdType, RelayIdTypeIter}; +/// Legacy implementation helper for HasRelayIds. +/// +/// Previously, we assumed that everything had these two identity types, which +/// is not an assumption we want to keep making in the future. +pub trait HasRelayIdsLegacy { + /// Return the ed25519 identity for this relay. + fn ed_identity(&self) -> &pk::ed25519::Ed25519Identity; + /// Return the RSA identity for this relay. + fn rsa_identity(&self) -> &pk::rsa::RsaIdentity; +} + /// An object containing information about a relay's identity keys. +/// +/// This trait has a fairly large number of methods, most of which you're not +/// actually expected to implement. The only one that you need to provide is +/// [`identity`](HasRelayIds::identity). pub trait HasRelayIds { /// Return the identity of this relay whose type is `key_type`, or None if /// the relay has no such identity. /// /// (Currently all relays have all recognized identity types, but we might /// implement or deprecate an identity type in the future.) - fn identity(&self, key_type: RelayIdType) -> Option> { - match key_type { - RelayIdType::Rsa => Some(self.rsa_identity().into()), - RelayIdType::Ed25519 => Some(self.ed_identity().into()), - } - } + fn identity(&self, key_type: RelayIdType) -> Option>; + /// Return an iterator over all of the identities held by this object. fn identities(&self) -> RelayIdIter<'_, Self> { RelayIdIter { @@ -26,6 +37,18 @@ pub trait HasRelayIds { next_key: RelayIdType::all_types(), } } + + /// Return the ed25519 identity for this relay if it has one. + fn ed_identity(&self) -> Option<&pk::ed25519::Ed25519Identity> { + self.identity(RelayIdType::Ed25519) + .map(RelayIdRef::unwrap_ed25519) + } + + /// Return the RSA identity for this relay if it has one. + fn rsa_identity(&self) -> Option<&pk::rsa::RsaIdentity> { + self.identity(RelayIdType::Rsa).map(RelayIdRef::unwrap_rsa) + } + /// Check whether the provided Id is a known identity of this relay. /// /// Remember that a given set of identity keys may be incomplete: some @@ -38,19 +61,44 @@ pub trait HasRelayIds { 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; - /// Return the RSA identity for this relay. - fn rsa_identity(&self) -> &pk::rsa::RsaIdentity; - /// 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 // should add a note saying that this function is usually not what you want // for many cases, since you might want to know "could this be the same // relay" vs "is this definitely the same relay." - fn same_relay_ids(&self, other: &T) -> bool { - self.ed_identity() == other.ed_identity() && self.rsa_identity() == other.rsa_identity() + // + // NOTE: We don't make this an `Eq` method, since we want to make callers + // choose carefully among this method, `has_all_relay_ids_from`, and any + // similar methods we add in the future. + fn same_relay_ids(&self, other: &T) -> bool { + RelayIdType::all_types().all(|key_type| self.identity(key_type) == other.identity(key_type)) + } + + /// Return true if this object has every relay ID that `other` does. + /// + /// (It still returns true if there are some IDs in this object that are not + /// present in `other`.) + fn has_all_relay_ids_from(&self, other: &T) -> bool { + RelayIdType::all_types().all(|key_type| { + match (self.identity(key_type), other.identity(key_type)) { + // If we both have the same key for this type, great. + (Some(mine), Some(theirs)) if mine == theirs => true, + // Uh oh. They do have a key for his type, but it's not ours. + (_, Some(_theirs)) => false, + // If they don't care what we have for this type, great. + (_, None) => true, + } + }) + } +} + +impl HasRelayIds for T { + fn identity(&self, key_type: RelayIdType) -> Option> { + match key_type { + RelayIdType::Rsa => Some(self.rsa_identity().into()), + RelayIdType::Ed25519 => Some(self.ed_identity().into()), + } } } @@ -136,7 +184,7 @@ mod test { &self.addrs[..] } } - impl HasRelayIds for Example { + impl HasRelayIdsLegacy for Example { fn ed_identity(&self) -> &pk::ed25519::Ed25519Identity { &self.ed_id } diff --git a/crates/tor-netdir/src/lib.rs b/crates/tor-netdir/src/lib.rs index 34e5e032c..e6b0f3e33 100644 --- a/crates/tor-netdir/src/lib.rs +++ b/crates/tor-netdir/src/lib.rs @@ -68,7 +68,7 @@ mod weight; #[cfg(any(test, feature = "testing"))] pub mod testnet; -use tor_linkspec::{ChanTarget, HasAddrs, HasRelayIds}; +use tor_linkspec::{ChanTarget, HasAddrs, HasRelayIds, RelayIdRef, RelayIdType}; use tor_llcrypto as ll; use tor_llcrypto::pk::{ed25519::Ed25519Identity, rsa::RsaIdentity}; use tor_netdoc::doc::microdesc::{MdDigest, Microdesc}; @@ -205,18 +205,26 @@ impl From for RelayWeight { } } -/// A view of the Tor directory, suitable for use in building -/// circuits. +/// A view of the Tor directory, suitable for use in building circuits. /// -/// Abstractly, a [`NetDir`] is a set of usable public [`Relay`]s, -/// each of which has its own properties, identity, and correct weighted -/// probability for use under different circumstances. +/// Abstractly, a [`NetDir`] is a set of usable public [`Relay`]s, each of which +/// has its own properties, identity, and correct weighted probability for use +/// under different circumstances. /// -/// A [`NetDir`] is constructed by making a [`PartialNetDir`] from a -/// consensus document, and then adding enough microdescriptors to -/// that `PartialNetDir` so that it can be used to build paths. -/// (Thus, if you have a NetDir, it is definitely adequate to build -/// paths.) +/// A [`NetDir`] is constructed by making a [`PartialNetDir`] from a consensus +/// document, and then adding enough microdescriptors to that `PartialNetDir` so +/// that it can be used to build paths. (Thus, if you have a NetDir, it is +/// definitely adequate to build paths.) +/// +/// # Limitations +/// +/// The current NetDir implementation assumes fairly strongly that every relay +/// has an Ed25519 identity and an RSA identity, that the consensus is indexed +/// by RSA identities, and that the Ed25519 identities are stored in +/// microdescriptors. +/// +/// If these assumptions someday change, then we'll have to revise the +/// implementation. #[derive(Debug, Clone)] pub struct NetDir { /// A microdescriptor consensus that lists the members of the network, @@ -630,15 +638,45 @@ impl NetDir { Some(relay) } + /// Return a relay matching a given identity, if we have a _usable_ relay + /// with that key. + /// + /// (Does not return unusable relays.) + /// + pub fn by_relay_id(&self, id: RelayIdRef<'_>) -> Option> { + match id { + RelayIdRef::Ed25519(ed25519) => self.by_id(ed25519), + RelayIdRef::Rsa(rsa) => self + .by_rsa_id_unchecked(rsa) + .and_then(UncheckedRelay::into_relay), + other_type => self.relays().find(|r| r.has_identity(other_type)), + } + } + /// Return a relay with the same identities as those in `target`, if one /// exists. /// /// Does not return unusable relays. + /// + /// # Limitations + /// + /// This will be very slow if `target` does not have an Ed25519 identity. pub fn by_ids(&self, target: &T) -> Option> where T: HasRelayIds + ?Sized, { - self.by_id_pair(target.ed_identity(), target.rsa_identity()) + if let Some(ed_id) = target.identity(RelayIdType::Ed25519) { + self.by_relay_id(ed_id).filter(|relay| { + // This is like "same_relay_ids", but it does not check the + // Ed25519 identity because that's already been checked. + RelayIdType::all_types().all(|id_type| { + id_type == RelayIdType::Ed25519 + || relay.identity(id_type) == target.identity(id_type) + }) + }) + } else { + self.relays().find(|r| r.same_relay_ids(target)) + } } /// Return a relay matching a given Ed25519 identity and RSA identity, @@ -660,15 +698,14 @@ impl NetDir { self.by_ids(chan_target) } - /// Return a boolean if this consensus definitely has (or does not - /// have) a relay matching both the given Ed25519 and RSA - /// identity. + /// Return a boolean if this consensus definitely has (or does not have) a + /// relay matching the listed identities. /// - /// If we can't yet tell for sure, return None. /// - /// Once function has returned `Some(b)`, it will always return that - /// value for the same `ed_id` and `rsa_id` on this `NetDir`. A `None` - /// answer may later become `Some(b)` if a microdescriptor arrives. + /// If we can't yet tell for sure, return None. Once function has returned + /// `Some(b)`, it will always return that value for the same `ed_id` and + /// `rsa_id` on this `NetDir`. A `None` answer may later become `Some(b)` + /// if a microdescriptor arrives. pub fn id_pair_listed(&self, ed_id: &Ed25519Identity, rsa_id: &RsaIdentity) -> Option { let r = self.by_rsa_id_unchecked(rsa_id); match r { @@ -689,11 +726,32 @@ impl NetDir { /// As `id_pair_listed`, but check whether a relay exists (or may exist) /// with the same identities as those in `target`. + /// + /// # Limitations + /// + /// This can be inefficient if the target does not have both an ed25519 and + /// an rsa identity key. pub fn ids_listed(&self, target: &T) -> Option where T: HasRelayIds + ?Sized, { - self.id_pair_listed(target.ed_identity(), target.rsa_identity()) + let rsa_id = target.rsa_identity(); + let ed25519_id = target.ed_identity(); + + match (rsa_id, ed25519_id) { + (Some(r), Some(e)) => self.id_pair_listed(e, r), + (Some(r), None) => Some(self.rsa_id_is_listed(r)), + (None, Some(e)) => { + if self.rs_idx_by_ed.contains_key(e) { + Some(true) + } else { + None + } + } + // TODO: If we later support more identity key types, this will + // become incorrect. + (None, None) => None, + } } /// Return true if we are currently missing a micro descriptor for the @@ -1137,7 +1195,7 @@ impl<'a> HasAddrs for Relay<'a> { self.rs.addrs() } } -impl<'a> HasRelayIds for Relay<'a> { +impl<'a> tor_linkspec::HasRelayIdsLegacy for Relay<'a> { fn ed_identity(&self) -> &Ed25519Identity { self.id() } @@ -1146,6 +1204,18 @@ impl<'a> HasRelayIds for Relay<'a> { } } +impl<'a> HasRelayIds for UncheckedRelay<'a> { + fn identity(&self, key_type: RelayIdType) -> Option> { + match key_type { + RelayIdType::Ed25519 if self.rs.ed25519_id_is_usable() => { + self.md.map(|m| m.ed25519_id().into()) + } + RelayIdType::Rsa => Some(self.rs.rsa_identity().into()), + _ => None, + } + } +} + impl<'a> ChanTarget for Relay<'a> {} impl<'a> tor_linkspec::CircTarget for Relay<'a> { @@ -1173,6 +1243,7 @@ mod test { use std::collections::HashSet; use std::time::Duration; use tor_basic_utils::test_rng; + use tor_linkspec::RelayIdType; // Basic functionality for a partial netdir: Add microdescriptors, // then you have a netdir. @@ -1351,7 +1422,7 @@ mod test { r.supports_exit_port_ipv4(80) }); let r = r.unwrap(); - let id_byte = r.rsa_identity().as_bytes()[0]; + let id_byte = r.identity(RelayIdType::Rsa).unwrap().as_bytes()[0]; picked[id_byte as usize] += 1; } // non-exits should never get picked. @@ -1383,7 +1454,7 @@ mod test { }); assert_eq!(relays.len(), 4); for r in relays { - let id_byte = r.rsa_identity().as_bytes()[0]; + let id_byte = r.identity(RelayIdType::Rsa).unwrap().as_bytes()[0]; picked[id_byte as usize] += 1; } } @@ -1607,8 +1678,11 @@ mod test { let r = netdir .by_id_pair(&[14; 32].into(), &[14; 20].into()) .unwrap(); - assert_eq!(r.rsa_identity(), &[14; 20].into()); - assert_eq!(r.ed_identity(), &[14; 32].into()); + assert_eq!(r.identity(RelayIdType::Rsa).unwrap().as_bytes(), &[14; 20]); + assert_eq!( + r.identity(RelayIdType::Ed25519).unwrap().as_bytes(), + &[14; 32] + ); let r = netdir.by_id_pair(&[14; 32].into(), &[99; 20].into()); assert!(r.is_none()); diff --git a/crates/tor-proto/semver.md b/crates/tor-proto/semver.md new file mode 100644 index 000000000..682b652e0 --- /dev/null +++ b/crates/tor-proto/semver.md @@ -0,0 +1 @@ +BREAKING: Remove key-specific accessors. diff --git a/crates/tor-proto/src/channel.rs b/crates/tor-proto/src/channel.rs index 92f9ef739..be54c24c8 100644 --- a/crates/tor-proto/src/channel.rs +++ b/crates/tor-proto/src/channel.rs @@ -77,9 +77,7 @@ use std::result::Result as StdResult; use std::time::Duration; use tor_cell::chancell::{msg, ChanCell, CircId}; use tor_error::internal; -use tor_linkspec::{ChanTarget, HasRelayIds, OwnedChanTarget}; -use tor_llcrypto::pk::ed25519::Ed25519Identity; -use tor_llcrypto::pk::rsa::RsaIdentity; +use tor_linkspec::{HasRelayIds, OwnedChanTarget}; use tor_rtcompat::SleepProvider; use asynchronous_codec as futures_codec; @@ -306,16 +304,6 @@ impl Channel { self.details.unique_id } - /// Return the Ed25519 identity for the peer of this channel. - pub fn peer_ed25519_id(&self) -> &Ed25519Identity { - self.details.peer_id.ed_identity() - } - - /// Return the (legacy) RSA identity for the peer of this channel. - pub fn peer_rsa_id(&self) -> &RsaIdentity { - self.details.peer_id.rsa_identity() - } - /// Return an OwnedChanTarget representing the actual handshake used to /// create this channel. pub fn target(&self) -> &OwnedChanTarget { @@ -347,23 +335,25 @@ impl Channel { /// Return an error if this channel is somehow mismatched with the /// given target. - pub fn check_match(&self, target: &T) -> Result<()> { - if self.peer_ed25519_id() != target.ed_identity() { - return Err(Error::ChanMismatch(format!( - "Identity {} does not match target {}", - self.peer_ed25519_id(), - target.ed_identity() - ))); + pub fn check_match(&self, target: &T) -> Result<()> { + for desired in target.identities() { + let id_type = desired.id_type(); + match self.details.peer_id.identity(id_type) { + Some(actual) if actual == desired => {} + Some(actual) => { + return Err(Error::ChanMismatch(format!( + "Identity {} does not match target {}", + actual, desired + ))); + } + None => { + return Err(Error::ChanMismatch(format!( + "Peer does not have {} identity", + id_type + ))) + } + } } - - if self.peer_rsa_id() != target.rsa_identity() { - return Err(Error::ChanMismatch(format!( - "Identity {} does not match target {}", - self.peer_rsa_id(), - target.rsa_identity() - ))); - } - Ok(()) } diff --git a/crates/tor-proto/src/channel/handshake.rs b/crates/tor-proto/src/channel/handshake.rs index dd4c17872..d9e166adb 100644 --- a/crates/tor-proto/src/channel/handshake.rs +++ b/crates/tor-proto/src/channel/handshake.rs @@ -19,7 +19,7 @@ use std::sync::Arc; use std::time::SystemTime; use tor_bytes::Reader; -use tor_linkspec::{ChanTarget, OwnedChanTarget}; +use tor_linkspec::{ChanTarget, HasRelayIds, OwnedChanTarget, RelayIds}; use tor_llcrypto as ll; use tor_llcrypto::pk::ed25519::Ed25519Identity; use tor_llcrypto::pk::rsa::RsaIdentity; @@ -504,14 +504,30 @@ impl Unver // We do this _last_, since "this is the wrong peer" is // usually a different situation than "this peer couldn't even // identify itself right." - if peer.ed_identity() != identity_key { - return Err(Error::HandshakeProto( - "Peer ed25519 id not as expected".into(), - )); - } - if *peer.rsa_identity() != rsa_id { - return Err(Error::HandshakeProto("Peer RSA id not as expected".into())); + // TODO: We'll need a different constructor if there are someday + // more/different identity keys. + let actual_identity = RelayIds::new(*identity_key, rsa_id); + + // We enforce that the relay proved that it has every ID that we wanted: + // it may also have additional IDs that we didn't ask for. + for desired_id in peer.identities() { + let id_type = desired_id.id_type(); + match actual_identity.identity(id_type) { + Some(actual) if actual == desired_id => {} + Some(_) => { + return Err(Error::HandshakeProto(format!( + "Peer {} id not as expected", + id_type + ))) + } + None => { + return Err(Error::HandshakeProto(format!( + "Peer did not present a {} id.", + id_type + ))) + } + } } // If we reach this point, the clock skew might be may now be considered @@ -928,7 +944,7 @@ pub(super) mod test { assert_eq!( format!("{}", err), - "Handshake protocol violation: Peer ed25519 id not as expected" + "Handshake protocol violation: Peer Ed25519 id not as expected" ); let err = certs_test( @@ -944,7 +960,7 @@ pub(super) mod test { assert_eq!( format!("{}", err), - "Handshake protocol violation: Peer RSA id not as expected" + "Handshake protocol violation: Peer RSA (legacy) id not as expected" ); let err = certs_test( diff --git a/crates/tor-proto/src/circuit.rs b/crates/tor-proto/src/circuit.rs index 5b3133e35..d766bbb06 100644 --- a/crates/tor-proto/src/circuit.rs +++ b/crates/tor-proto/src/circuit.rs @@ -62,7 +62,7 @@ use tor_cell::{ }; use tor_error::{bad_api_usage, internal, into_internal}; -use tor_linkspec::{CircTarget, LinkSpec, OwnedChanTarget}; +use tor_linkspec::{CircTarget, LinkSpec, OwnedChanTarget, RelayIdType}; use futures::channel::{mpsc, oneshot}; @@ -235,7 +235,9 @@ impl ClientCirc { Tg: CircTarget, { let key = NtorPublicKey { - id: *target.rsa_identity(), + id: *target + .rsa_identity() + .ok_or(Error::MissingId(RelayIdType::Ed25519))?, pk: *target.ntor_onion_key(), }; let mut linkspecs = target.linkspecs(); @@ -549,10 +551,14 @@ impl PendingClientCirc { recv_created: self.recvcreated, handshake: CircuitHandshake::Ntor { public_key: NtorPublicKey { - id: *target.rsa_identity(), + id: *target + .rsa_identity() + .ok_or(Error::MissingId(RelayIdType::Rsa))?, pk: *target.ntor_onion_key(), }, - ed_identity: *target.ed_identity(), + ed_identity: *target + .ed_identity() + .ok_or(Error::MissingId(RelayIdType::Ed25519))?, }, require_sendme_auth, params: params.clone(), diff --git a/crates/tor-proto/src/circuit/reactor.rs b/crates/tor-proto/src/circuit/reactor.rs index 905441ec4..148d4417c 100644 --- a/crates/tor-proto/src/circuit/reactor.rs +++ b/crates/tor-proto/src/circuit/reactor.rs @@ -35,7 +35,7 @@ use crate::crypto::handshake::ntor::{NtorClient, NtorPublicKey}; use crate::crypto::handshake::{ClientHandshake, KeyGenerator}; use tor_cell::chancell; use tor_cell::chancell::{ChanCell, CircId}; -use tor_linkspec::{LinkSpec, OwnedChanTarget}; +use tor_linkspec::{LinkSpec, OwnedChanTarget, RelayIds}; use tor_llcrypto::pk; use tracing::{debug, trace, warn}; @@ -781,21 +781,8 @@ impl Reactor { params: &CircParameters, ) -> Result<()> { // Exit now if we have an Ed25519 or RSA identity mismatch. - // FIXME(eta): this is copypasta from Channel::check_match! - if self.channel.peer_rsa_id() != &pubkey.id { - return Err(Error::ChanMismatch(format!( - "Identity {} does not match target {}", - self.channel.peer_rsa_id(), - pubkey.id, - ))); - } - if self.channel.peer_ed25519_id() != &ed_identity { - return Err(Error::ChanMismatch(format!( - "Identity {} does not match target {}", - self.channel.peer_ed25519_id(), - ed_identity - ))); - } + let target = RelayIds::new(ed_identity, pubkey.id); + self.channel.check_match(&target)?; let wrap = Create2Wrap { handshake_type: 0x0002, // ntor diff --git a/crates/tor-proto/src/util/err.rs b/crates/tor-proto/src/util/err.rs index a20d5ac44..63b432f80 100644 --- a/crates/tor-proto/src/util/err.rs +++ b/crates/tor-proto/src/util/err.rs @@ -3,6 +3,7 @@ use std::{sync::Arc, time::Duration}; use thiserror::Error; use tor_cell::relaycell::msg::EndReason; use tor_error::{ErrorKind, HasKind}; +use tor_linkspec::RelayIdType; /// An error type for the tor-proto crate. /// @@ -134,6 +135,10 @@ pub enum Error { /// Remote DNS lookup failed. #[error("Remote resolve failed")] ResolveError(#[source] ResolveError), + /// We tried to do something with a that we couldn't, because of an identity key type + /// that the relay doesn't have. + #[error("Relay has no {0} identity")] + MissingId(RelayIdType), } /// Error which indicates that the channel was closed. @@ -222,7 +227,8 @@ impl From for std::io::Error { | CellEncodeErr { .. } | EncodeErr { .. } | ChanMismatch(_) - | StreamProto(_) => ErrorKind::InvalidData, + | StreamProto(_) + | MissingId(_) => ErrorKind::InvalidData, Bug(ref e) if e.kind() == tor_error::ErrorKind::BadApiUsage => ErrorKind::InvalidData, @@ -269,6 +275,7 @@ impl HasKind for Error { E::ResolveError(ResolveError::Nontransient) => EK::RemoteHostNotFound, E::ResolveError(ResolveError::Transient) => EK::RemoteHostResolutionFailed, E::ResolveError(ResolveError::Unrecognized) => EK::RemoteHostResolutionFailed, + E::MissingId(_) => EK::BadApiUsage, E::Bug(e) => e.kind(), } }