From 74d4c73d090f28ba5a8dca4fda3f170a36c0fe16 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Thu, 6 Oct 2022 16:35:52 -0400 Subject: [PATCH] Change multiplicity of ChannelMethod and addresses Now each `ChanTarget` has at most one `ChannelMethod`, and only `Direct` `ChannelMethods` can have multiple addresses. Closes #600. --- crates/tor-chanmgr/src/builder.rs | 20 +++++++--------- crates/tor-guardmgr/src/bridge/config.rs | 12 +++++++--- crates/tor-guardmgr/src/bridge/relay.rs | 10 ++++---- crates/tor-linkspec/src/lib.rs | 2 +- crates/tor-linkspec/src/owned.rs | 27 +++++++++++++--------- crates/tor-linkspec/src/traits.rs | 28 +++++++++-------------- crates/tor-linkspec/src/transport.rs | 12 ++++++---- crates/tor-proto/src/channel.rs | 8 +++---- crates/tor-proto/src/channel/handshake.rs | 4 ++-- 9 files changed, 63 insertions(+), 60 deletions(-) diff --git a/crates/tor-chanmgr/src/builder.rs b/crates/tor-chanmgr/src/builder.rs index a71e69b6b..1749b5d70 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 safelog::sensitive as sv; use std::time::Duration; use tor_error::{bad_api_usage, internal}; -use tor_linkspec::{ChannelMethod, HasChanMethods, HasRelayIds, OwnedChanTarget}; +use tor_linkspec::{ChannelMethod, HasChanMethod, HasRelayIds, OwnedChanTarget}; use tor_llcrypto::pk; use tor_proto::channel::params::ChannelPaddingInstructionsUpdates; use tor_rtcompat::{tls::TlsConnector, Runtime, TcpProvider, TlsProvider}; @@ -148,15 +148,11 @@ impl ChanBuilder { } // TODO pt-client: right now this only handles the Direct method. - let direct_addrs: Vec<_> = target - .chan_methods() - .iter() - .filter_map(|method| match method { - ChannelMethod::Direct(addr) => Some(*addr), - #[allow(unreachable_patterns)] // TODO pt-client - _ => None, - }) - .collect(); + let direct_addrs: Vec<_> = match target.chan_method() { + ChannelMethod::Direct(addrs) => addrs, + #[allow(unreachable_patterns)] // TODO pt-client + _ => vec![], // TODO pt-client + }; let (stream, addr) = connect_to_one(&self.runtime, &direct_addrs).await?; let using_target = match target.restrict_addr(&addr) { Ok(v) => v, @@ -199,7 +195,7 @@ impl ChanBuilder { // 2. Set up the channel. let mut builder = ChannelBuilder::new(); - builder.set_declared_method(tor_linkspec::ChannelMethod::Direct(addr)); + builder.set_declared_method(tor_linkspec::ChannelMethod::Direct(vec![addr])); let chan = builder .launch( tls, @@ -304,7 +300,7 @@ mod test { let tls_cert = msgs::X509_CERT.into(); let target = OwnedChanTarget::builder() .addrs(vec![orport]) - .methods(vec![ChannelMethod::Direct(orport)]) + .method(ChannelMethod::Direct(vec![orport])) .ed_identity(ed) .rsa_identity(rsa) .build() diff --git a/crates/tor-guardmgr/src/bridge/config.rs b/crates/tor-guardmgr/src/bridge/config.rs index 75e53b79c..3009c742d 100644 --- a/crates/tor-guardmgr/src/bridge/config.rs +++ b/crates/tor-guardmgr/src/bridge/config.rs @@ -57,6 +57,8 @@ pub struct Bridge { // /// Address and transport via which the bridge can be reached, and /// the parameters for those transports. + /// + /// Restriction: This `addrs` may NOT contain more than one address. addrs: ChannelMethod, /// The RSA identity of the bridge. @@ -220,7 +222,7 @@ impl FromStr for Bridge { word: word.to_string(), addr_error, })?; - ChannelMethod::Direct(addr) + ChannelMethod::Direct(vec![addr]) } else { #[cfg(not(feature = "pt-client"))] return Err(BPE::PluggableTransportsNotSupported { @@ -333,7 +335,11 @@ impl Display for Bridge { let settings = match addrs { ChannelMethod::Direct(a) => { - write!(f, "{}", a)?; + if a.len() == 1 { + write!(f, "{}", a[0])?; + } else { + panic!("Somehow created a Bridge config with multiple addrs."); + } None } @@ -388,7 +394,7 @@ mod test { } fn mk_direct(s: &str) -> ChannelMethod { - ChannelMethod::Direct(s.parse().unwrap()) + ChannelMethod::Direct(vec![s.parse().unwrap()]) } fn mk_rsa(s: &str) -> RsaIdentity { diff --git a/crates/tor-guardmgr/src/bridge/relay.rs b/crates/tor-guardmgr/src/bridge/relay.rs index 042606000..f497a56cd 100644 --- a/crates/tor-guardmgr/src/bridge/relay.rs +++ b/crates/tor-guardmgr/src/bridge/relay.rs @@ -3,7 +3,7 @@ use std::sync::Arc; use tor_linkspec::{ - ChanTarget, CircTarget, HasAddrs, HasChanMethods, HasRelayIds, RelayIdRef, RelayIdType, + ChanTarget, CircTarget, HasAddrs, HasChanMethod, HasRelayIds, RelayIdRef, RelayIdType, }; use super::{Bridge, BridgeDesc}; @@ -65,8 +65,8 @@ impl HasAddrs for BridgeRelay { } } -impl HasChanMethods for BridgeRelay { - fn chan_methods(&self) -> Vec { +impl HasChanMethod for BridgeRelay { + fn chan_method(&self) -> tor_linkspec::ChannelMethod { todo!() } } @@ -92,8 +92,8 @@ impl<'a> HasAddrs for BridgeRelayWithDesc<'a> { &[] } } -impl<'a> HasChanMethods for BridgeRelayWithDesc<'a> { - fn chan_methods(&self) -> Vec { +impl<'a> HasChanMethod for BridgeRelayWithDesc<'a> { + fn chan_method(&self) -> tor_linkspec::ChannelMethod { todo!() } } diff --git a/crates/tor-linkspec/src/lib.rs b/crates/tor-linkspec/src/lib.rs index bfe031852..b237f17dd 100644 --- a/crates/tor-linkspec/src/lib.rs +++ b/crates/tor-linkspec/src/lib.rs @@ -105,7 +105,7 @@ pub use ids::{ pub use ls::LinkSpec; pub use owned::{OwnedChanTarget, OwnedChanTargetBuilder, OwnedCircTarget, RelayIds}; pub use traits::{ - ChanTarget, CircTarget, DirectChanMethodsHelper, HasAddrs, HasChanMethods, HasRelayIds, + ChanTarget, CircTarget, DirectChanMethodsHelper, HasAddrs, HasChanMethod, HasRelayIds, HasRelayIdsLegacy, }; pub use transport::{ChannelMethod, PtAddrError, PtTargetAddr, TransportId, TransportIdError}; diff --git a/crates/tor-linkspec/src/owned.rs b/crates/tor-linkspec/src/owned.rs index 1d1385dfa..e0f552c4c 100644 --- a/crates/tor-linkspec/src/owned.rs +++ b/crates/tor-linkspec/src/owned.rs @@ -7,7 +7,7 @@ use tor_config::impl_standard_builder; use tor_llcrypto::pk; use crate::{ - ChanTarget, ChannelMethod, CircTarget, HasAddrs, HasChanMethods, HasRelayIds, RelayIdRef, + ChanTarget, ChannelMethod, CircTarget, HasAddrs, HasChanMethod, HasRelayIds, RelayIdRef, RelayIdType, }; @@ -89,8 +89,8 @@ pub struct OwnedChanTarget { // // TODO: in many cases this will be redundant with addrs; if we allocate a // lot of these objects, we might want to handle that. - #[builder(default)] - methods: Vec, + #[builder(default = "self.make_method()")] + method: ChannelMethod, /// Identities that this relay provides. #[builder(sub_builder)] ids: RelayIds, @@ -109,6 +109,11 @@ impl OwnedChanTargetBuilder { self.ids().rsa_identity(id); self } + + /// Helper: make a channel method if none was specified. + fn make_method(&self) -> ChannelMethod { + ChannelMethod::Direct(self.addrs.clone().unwrap_or_default()) + } } impl HasAddrs for OwnedChanTarget { @@ -117,9 +122,9 @@ impl HasAddrs for OwnedChanTarget { } } -impl HasChanMethods for OwnedChanTarget { - fn chan_methods(&self) -> Vec { - self.methods.clone() +impl HasChanMethod for OwnedChanTarget { + fn chan_method(&self) -> ChannelMethod { + self.method.clone() } } @@ -139,7 +144,7 @@ impl OwnedChanTarget { { OwnedChanTarget { addrs: target.addrs().to_vec(), - methods: target.chan_methods(), + method: target.chan_method(), ids: RelayIds::from_relay_ids(target), } } @@ -154,7 +159,7 @@ impl OwnedChanTarget { if self.addrs.contains(addr) { Ok(OwnedChanTarget { addrs: vec![*addr], - methods: self.methods.clone(), + method: self.method.clone(), ids: self.ids.clone(), }) } else { @@ -229,9 +234,9 @@ impl HasRelayIds for OwnedCircTarget { self.chan_target.identity(key_type) } } -impl HasChanMethods for OwnedCircTarget { - fn chan_methods(&self) -> Vec { - self.chan_target.chan_methods() +impl HasChanMethod for OwnedCircTarget { + fn chan_method(&self) -> ChannelMethod { + self.chan_target.chan_method() } } diff --git a/crates/tor-linkspec/src/traits.rs b/crates/tor-linkspec/src/traits.rs index 0bbbdb195..c15455066 100644 --- a/crates/tor-linkspec/src/traits.rs +++ b/crates/tor-linkspec/src/traits.rs @@ -144,12 +144,12 @@ pub trait HasAddrs { } /// An object that can be connected to via [`ChannelMethod`]s. -pub trait HasChanMethods { +pub trait HasChanMethod { /// Return the known ways to contact this - // TODO: See notes on HasAddrs above. TODO pt-client: I don't like having - // this return a Vec, but I don't see a great alternative. Let's revisit - // that.-nickm. - fn chan_methods(&self) -> Vec; + // TODO: See notes on HasAddrs above. + // TODO pt-client: I don't like having this return a new ChannelMethod, but I + // don't see a great alternative. Let's revisit that.-nickm. + fn chan_method(&self) -> ChannelMethod; } /// Implement `HasChanMethods` for an object with `HasAddr` whose addresses @@ -160,12 +160,9 @@ pub trait HasChanMethods { // should instead suggest that we _only_ have direct methods? pub trait DirectChanMethodsHelper: HasAddrs {} -impl HasChanMethods for D { - fn chan_methods(&self) -> Vec { - self.addrs() - .iter() - .map(|a| ChannelMethod::Direct(*a)) - .collect() +impl HasChanMethod for D { + fn chan_method(&self) -> ChannelMethod { + ChannelMethod::Direct(self.addrs().to_vec()) } } @@ -174,7 +171,7 @@ impl HasChanMethods for D { /// Anything that implements 'ChanTarget' can be used as the /// identity of a relay for the purposes of launching a new /// channel. -pub trait ChanTarget: HasRelayIds + HasAddrs + HasChanMethods {} +pub trait ChanTarget: HasRelayIds + HasAddrs + HasChanMethod {} /// Information about a Tor relay used to extend a circuit to it. /// @@ -187,11 +184,8 @@ pub trait CircTarget: ChanTarget { // doing so correctly would require default associated types. fn linkspecs(&self) -> Vec { let mut result: Vec<_> = self.identities().map(|id| id.to_owned().into()).collect(); - for method in self.chan_methods().iter() { - #[allow(irrefutable_let_patterns)] // TODO pt-client - if let ChannelMethod::Direct(addr) = method { - result.push(addr.into()); - } + if let ChannelMethod::Direct(addrs) = self.chan_method() { + result.extend(addrs.into_iter().map(crate::LinkSpec::from)); } result } diff --git a/crates/tor-linkspec/src/transport.rs b/crates/tor-linkspec/src/transport.rs index 6bb125053..f68364295 100644 --- a/crates/tor-linkspec/src/transport.rs +++ b/crates/tor-linkspec/src/transport.rs @@ -383,18 +383,20 @@ impl PtTarget { #[derive(Clone, Debug, Eq, PartialEq)] #[allow(clippy::exhaustive_enums)] // TODO pt-client: I am not in love with this enum name --nm. +// TODO pt-client: Maybe "ContactMethod" would be better? pub enum ChannelMethod { - /// Connect to the relay directly at a given address. - Direct(std::net::SocketAddr), + /// Connect to the relay directly at one of several addresses. + Direct(Vec), + // TODO pt-client: We may want to have a third variant for "Direct" with a + // single address. Maybe? /// Connect to a bridge relay via a pluggable transport. #[cfg(feature = "pt-client")] Pluggable(PtTarget), } impl ChannelMethod { - /// Return the advertised socket address that this method connects to, if - /// there is one. + /// Return an advertised socket address that this method connects to. /// /// NOTE that this is not necessarily an address to which you can open a /// TCP connection! If this `ChannelMethod` is using a non-`Direct` @@ -402,7 +404,7 @@ impl ChannelMethod { /// implementation. pub fn declared_peer_addr(&self) -> Option<&std::net::SocketAddr> { match self { - ChannelMethod::Direct(addr) => Some(addr), + ChannelMethod::Direct(addr) if !addr.is_empty() => Some(&addr[0]), #[cfg(feature = "pt-client")] ChannelMethod::Pluggable(PtTarget { diff --git a/crates/tor-proto/src/channel.rs b/crates/tor-proto/src/channel.rs index 0f8357b3f..76ca4075f 100644 --- a/crates/tor-proto/src/channel.rs +++ b/crates/tor-proto/src/channel.rs @@ -295,7 +295,7 @@ impl ChannelBuilder { /// connection to a given socket address. #[deprecated(note = "use set_declared_method instead", since = "0.7.1")] pub fn set_declared_addr(&mut self, target: std::net::SocketAddr) { - self.set_declared_method(tor_linkspec::ChannelMethod::Direct(target)); + self.set_declared_method(tor_linkspec::ChannelMethod::Direct(vec![target])); } /// Set the declared target method of this channel. @@ -736,9 +736,9 @@ pub(crate) mod test { fn chanbuilder() { let rt = PreferredRuntime::create().unwrap(); let mut builder = ChannelBuilder::default(); - builder.set_declared_method(tor_linkspec::ChannelMethod::Direct( - "127.0.0.1:9001".parse().unwrap(), - )); + builder.set_declared_method(tor_linkspec::ChannelMethod::Direct(vec!["127.0.0.1:9001" + .parse() + .unwrap()])); let tls = MsgBuf::new(&b""[..]); let _outbound = builder.launch(tls, rt); } diff --git a/crates/tor-proto/src/channel/handshake.rs b/crates/tor-proto/src/channel/handshake.rs index d6bace957..3a1a7e626 100644 --- a/crates/tor-proto/src/channel/handshake.rs +++ b/crates/tor-proto/src/channel/handshake.rs @@ -595,7 +595,7 @@ impl Verif if let Some(addr) = target_method.declared_peer_addr() { peer_builder.addrs(vec![*addr]); } - peer_builder.methods(vec![target_method]); + peer_builder.method(target_method); } let peer_id = peer_builder .ed_identity(self.ed25519_id) @@ -1078,7 +1078,7 @@ pub(super) mod test { link_protocol: 4, tls: futures_codec::Framed::new(MsgBuf::new(&b""[..]), ChannelCodec::new(4)), unique_id: UniqId::new(), - target_method: Some(ChannelMethod::Direct(peer_addr)), + target_method: Some(ChannelMethod::Direct(vec![peer_addr])), ed25519_id, rsa_id, clock_skew: ClockSkew::None,