From 69b64a2795bcfcad91025cd70572312dc360fc8c Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 12 Oct 2022 10:36:14 -0400 Subject: [PATCH] chanmgr: clean up some TODO pt-client items and documentation. --- crates/tor-chanmgr/src/err.rs | 4 -- crates/tor-chanmgr/src/factory.rs | 92 ++++++------------------------- crates/tor-chanmgr/src/lib.rs | 7 ++- 3 files changed, 23 insertions(+), 80 deletions(-) diff --git a/crates/tor-chanmgr/src/err.rs b/crates/tor-chanmgr/src/err.rs index 3b5fcffc2..b7991402b 100644 --- a/crates/tor-chanmgr/src/err.rs +++ b/crates/tor-chanmgr/src/err.rs @@ -149,10 +149,6 @@ impl tor_error::HasRetryTime for Error { // This one can't succeed until the bridge, or our set of // transports, is reconfigured. - // - // TODO pt-client: Double-check this, to make sure that it doesn't - // cause us to reject a multi-transport bridge if only one of its - // transports fails. E::NoSuchTransport(_) => RT::Never, // These aren't recoverable at all. diff --git a/crates/tor-chanmgr/src/factory.rs b/crates/tor-chanmgr/src/factory.rs index fdc4da836..b9e1270d4 100644 --- a/crates/tor-chanmgr/src/factory.rs +++ b/crates/tor-chanmgr/src/factory.rs @@ -3,11 +3,12 @@ use std::sync::Arc; +use crate::Error; + use async_trait::async_trait; use futures::{AsyncRead, AsyncWrite}; use tor_linkspec::{HasChanMethod, OwnedChanTarget, TransportId}; use tor_proto::channel::Channel; -use tor_rtcompat::Runtime; /// An object that knows how to build Channels to ChanTargets. /// @@ -16,11 +17,12 @@ use tor_rtcompat::Runtime; pub trait ChannelFactory { /// Open an authenticated channel to `target`. /// - /// We need this method to take a dyn ChanTarget so it is - /// object-safe. - // - // TODO pt-client: How does this handle multiple addresses? Do we - // parallelize here, or at a higher level? + /// This method does does not necessarily handle retries or timeouts, + /// although some of its implementations may. + /// + /// This method does not necessarily handle every kind of transport. + /// If the caller provides a target with the wrong [`TransportId`], this + /// method should return [`Error::NoSuchTransport`]. async fn connect_via_transport(&self, target: &OwnedChanTarget) -> crate::Result; } @@ -31,10 +33,8 @@ pub trait ChannelFactory { /// This is the trait you should probably implement if you want to define a new /// [`ChannelFactory`] that performs Tor over TLS over some stream-like type, /// and you only want to define the stream-like type. -// -// TODO pt-client: I originally had this parameterized on a Runtime. But I -// think instead we should have individual TransportHelper implementations be -// parameterized on a Runtime. +/// +/// To convert a [`TransportHelper`] into a [`ChannelFactory`], wrap it in a ChannelBuilder. #[async_trait] pub trait TransportHelper { /// The type of the resulting stream. @@ -42,69 +42,19 @@ pub trait TransportHelper { /// Implements the transport: makes a TCP connection (possibly /// tunneled over whatever protocol) if possible. - // - // TODO pt-client: How does this handle multiple addresses? Do we - // parallelize here, or at a higher level? - // - // TODO pt-client: We could make the address an associated type: would that - // help anything? + /// + /// This method does does not necessarily handle retries or timeouts, + /// although some of its implementations may. + /// + /// This method does not necessarily handle every kind of transport. + /// If the caller provides a target with the wrong [`TransportId`], this + /// method should return [`Error::NoSuchTransport`]. async fn connect( &self, target: &OwnedChanTarget, ) -> crate::Result<(OwnedChanTarget, Self::Stream)>; } -// We define an implementation so that every TransportHelper -// can be wrapped as a ChannelFactory... -#[async_trait] -impl ChannelFactory for H -where - H: TransportHelper + Sync, -{ - async fn connect_via_transport(&self, target: &OwnedChanTarget) -> crate::Result { - let _stream = self.connect(target).await?; - - // Now do the logic from - // `tor_chanmgr::builder::ChanBuilder::build_channel_no_timeout`: - // Negotiate TLS, call tor_proto::ChannelBuilder::build, ... - - // TODO: Hang on, where do we get a pre-built TlsConnector in - // this method? We may need a different signature, or some - // kind of wrapper type. - // - // TODO: We may also need access to other stuff, like the contents - // of `ChanBuilder`. - - todo!("TODO pt-client: implement this") - } -} - -/// A ChannelFactory implementing Tor's default channel protocol. -#[derive(Clone, Debug)] -#[non_exhaustive] -pub struct DefaultChannelFactory { - /// The runtime that we use to make connections. - #[allow(dead_code)] // TODO pt-client: this will be removed. - runtime: R, -} -#[async_trait] -impl TransportHelper for DefaultChannelFactory { - type Stream = R::TcpStream; - async fn connect( - &self, - _target: &OwnedChanTarget, - ) -> crate::Result<(OwnedChanTarget, Self::Stream)> { - // Call connect_one() as in `build_channel_no_timeout`. - - // TODO pt-client: This is another place where we need to figure out - // multiple addresses and "happy eyeballs". - - // Call restrict_addr() as in `build_channel_no_timeout`. - - todo!("TODO pt-client: implement this") - } -} - /// An object that knows about one or more ChannelFactories. pub trait TransportRegistry { /// Return a ChannelFactory that can make connections via a chosen @@ -114,9 +64,6 @@ pub trait TransportRegistry { fn get_factory(&self, transport: &TransportId) -> Option<&(dyn ChannelFactory + Sync)>; } -// TODO pt-client: implement a DefaultTransportRegistry that returns a -// DefaultChannelFactory for TransportId::Builtin, and nothing otherwise. - /// Helper type: Wrap a `TransportRegistry` so that it can be used as a /// `ChannelFactory`. /// @@ -130,10 +77,7 @@ impl ChannelFactory for RegistryAsFactory { async fn connect_via_transport(&self, target: &OwnedChanTarget) -> crate::Result { let method = target.chan_method(); let id = method.transport_id(); - let factory = self - .0 - .get_factory(&id) - .ok_or(crate::Error::NoSuchTransport(id))?; + let factory = self.0.get_factory(&id).ok_or(Error::NoSuchTransport(id))?; factory.connect_via_transport(target).await } diff --git a/crates/tor-chanmgr/src/lib.rs b/crates/tor-chanmgr/src/lib.rs index b7bb60c1a..fd26a53c4 100644 --- a/crates/tor-chanmgr/src/lib.rs +++ b/crates/tor-chanmgr/src/lib.rs @@ -313,16 +313,19 @@ impl ChanMgr { /// This method can be used to e.g. tell Arti to use a proxy for /// outgoing connections. pub fn set_default_transport(&self, _factory: impl factory::ChannelFactory) { + // TODO pt-client: Perhaps we actually want to remove this and have it + // be part of the constructor? The only way to actually implement it is + // to make the channel factory in AbstractChanMgr mutable, which seels a + // little ugly. Do we ever want to change this on a _running_ ChanMgr? #![allow(clippy::missing_panics_doc, clippy::needless_pass_by_value)] todo!("TODO pt-client: implement this.") } /// Replace the transport registry with one that may know about /// more transports. - // - // TODO::pt_client (Alternatively, move this functionality into ChanMgr::new?) #[cfg(feature = "pt-client")] pub fn set_transport_registry(&self, _registry: impl factory::TransportRegistry) { + // TODO pt-client: See set_default_transport above. #![allow(clippy::missing_panics_doc, clippy::needless_pass_by_value)] todo!("TODO pt-client: implement this.") }