From eee3bb8822dd22a48b58bfb9a42cb0eaa952138d Mon Sep 17 00:00:00 2001 From: Gabriela Moldovan Date: Mon, 31 Jul 2023 19:51:23 +0100 Subject: [PATCH 1/3] tor-proto: Make HopNum public. `HopNum` will be used in `ClientCirc`'s public API when we refactor `ClientCirc::start_conversation_last_hop` to use the provided hop rather than always using the last one. --- crates/tor-proto/semver.md | 1 + crates/tor-proto/src/crypto/cell.rs | 2 +- crates/tor-proto/src/lib.rs | 1 + 3 files changed, 3 insertions(+), 1 deletion(-) diff --git a/crates/tor-proto/semver.md b/crates/tor-proto/semver.md index 7bd39e98a..df03b656f 100644 --- a/crates/tor-proto/semver.md +++ b/crates/tor-proto/semver.md @@ -6,3 +6,4 @@ BREAKING: `IncomingStreamRequest::reject` is now async, takes `&mut self`, and returns a `Result` BREAKING: `ClientCirc::allow_stream_requests` now expects `self` to be `&Arc` +ADDED: `HopNum` is now public diff --git a/crates/tor-proto/src/crypto/cell.rs b/crates/tor-proto/src/crypto/cell.rs index 09269a116..710f62788 100644 --- a/crates/tor-proto/src/crypto/cell.rs +++ b/crates/tor-proto/src/crypto/cell.rs @@ -116,7 +116,7 @@ pub(crate) trait InboundClientLayer { /// /// Hop indices are zero-based: "0" denotes the first hop on the circuit. #[derive(Copy, Clone, Eq, PartialEq, Debug)] -pub(crate) struct HopNum(u8); +pub struct HopNum(u8); impl From for u8 { fn from(hop: HopNum) -> u8 { diff --git a/crates/tor-proto/src/lib.rs b/crates/tor-proto/src/lib.rs index 77e1fad98..741674437 100644 --- a/crates/tor-proto/src/lib.rs +++ b/crates/tor-proto/src/lib.rs @@ -50,6 +50,7 @@ pub use util::err::{Error, ResolveError}; pub use util::skew::ClockSkew; pub use channel::params::ChannelPaddingInstructions; +pub use crypto::cell::HopNum; /// A Result type for this crate. pub type Result = std::result::Result; From c23e85270e3f2c17cb97024ac07b557990514c89 Mon Sep 17 00:00:00 2001 From: Gabriela Moldovan Date: Mon, 31 Jul 2023 19:52:38 +0100 Subject: [PATCH 2/3] tor-proto: Add method for getting the HopNum of the last hop. --- crates/tor-proto/semver.md | 1 + crates/tor-proto/src/circuit.rs | 14 ++++++++++++++ 2 files changed, 15 insertions(+) diff --git a/crates/tor-proto/semver.md b/crates/tor-proto/semver.md index df03b656f..d8c59a803 100644 --- a/crates/tor-proto/semver.md +++ b/crates/tor-proto/semver.md @@ -7,3 +7,4 @@ and returns a `Result` BREAKING: `ClientCirc::allow_stream_requests` now expects `self` to be `&Arc` ADDED: `HopNum` is now public +ADDED: `ClientCirc::last_hop_num` diff --git a/crates/tor-proto/src/circuit.rs b/crates/tor-proto/src/circuit.rs index fb12f547b..3d02a9d05 100644 --- a/crates/tor-proto/src/circuit.rs +++ b/crates/tor-proto/src/circuit.rs @@ -283,6 +283,20 @@ impl ClientCirc { } } + /// Return the [`HopNum`](crate::HopNum) of the last hop of this circuit. + /// + /// Returns an error if there is no last hop. (This should be impossible outside of the + /// tor-proto crate, but within the crate it's possible to have a circuit with no hops.) + pub fn last_hop_num(&self) -> Result { + Ok(self + .mutable + .lock() + .expect("poisoned lock") + .path + .last_hop_num() + .ok_or_else(|| internal!("no last hop index"))?) + } + /// Return a description of all the hops in this circuit. /// /// This method is **deprecated** for several reasons: From ed5a051ebe94a8cf4e5e9f33c000a9bd2fddfd8f Mon Sep 17 00:00:00 2001 From: Gabriela Moldovan Date: Mon, 31 Jul 2023 19:53:30 +0100 Subject: [PATCH 3/3] tor-proto: Add ClientCirc::start_conversation(). This will enable hidden services to send `RENDEZVOUS1` messages to the `N`th hop of the circuit rather than the `N + 1`th virtual one (which can only used after the client and service have completed the introduction handshake). This also deprecates `start_conversation_last_hop`. Closes #959 --- crates/tor-hsclient/src/connect.rs | 7 ++-- crates/tor-proto/Cargo.toml | 2 +- crates/tor-proto/semver.md | 3 ++ crates/tor-proto/src/circuit.rs | 41 ++++++++++++++-------- crates/tor-proto/src/circuit/msghandler.rs | 4 +-- crates/tor-proto/src/circuit/reactor.rs | 2 +- 6 files changed, 37 insertions(+), 22 deletions(-) diff --git a/crates/tor-hsclient/src/connect.rs b/crates/tor-hsclient/src/connect.rs index 2badb5b29..7a261e981 100644 --- a/crates/tor-hsclient/src/connect.rs +++ b/crates/tor-hsclient/src/connect.rs @@ -1009,7 +1009,7 @@ impl<'c, R: Runtime, M: MocksForConnect> Context<'c, R, M> { .await .map_err(handle_proto_error)?; - // `start_conversation_last_hop` returns as soon as the control message has been sent. + // `start_conversation` returns as soon as the control message has been sent. // We need to obtain the RENDEZVOUS_ESTABLISHED message, which is "returned" via the oneshot. let _: RendezvousEstablished = rend_established_rx.recv(handle_proto_error).await?; @@ -1337,7 +1337,7 @@ trait MocksForConnect: Clone { /// Mock for `HsCircPool` /// /// Methods start with `m_` to avoid the following problem: -/// `ClientCirc::start_conversation_last_hop` (say) means +/// `ClientCirc::start_conversation` (say) means /// to use the inherent method if one exists, /// but will use a trait method if there isn't an inherent method. /// @@ -1435,7 +1435,8 @@ impl MockableClientCirc for ClientCirc { msg: Option, reply_handler: impl MsgHandler + Send + 'static, ) -> tor_proto::Result> { - ClientCirc::start_conversation_last_hop(self, msg, reply_handler).await + let last_hop = self.last_hop_num()?; + ClientCirc::start_conversation(self, msg, reply_handler, last_hop).await } type Conversation<'r> = tor_proto::circuit::Conversation<'r>; diff --git a/crates/tor-proto/Cargo.toml b/crates/tor-proto/Cargo.toml index 4851a6a07..04a9007ca 100644 --- a/crates/tor-proto/Cargo.toml +++ b/crates/tor-proto/Cargo.toml @@ -42,7 +42,7 @@ hs-client = ["hs-common"] hs-service = ["hs-common", "__is_experimental"] hs-common = ["tor-hscrypto"] experimental-api = ["__is_experimental"] -# start_conversation_last_hop etc.; TODO HS should be renamed +# start_conversation etc.; TODO HS should be renamed send-control-msg = ["visibility"] stream-ctrl = ["__is_experimental"] # Enable testing-only APIs. APIs under this feature are not diff --git a/crates/tor-proto/semver.md b/crates/tor-proto/semver.md index d8c59a803..6e0138568 100644 --- a/crates/tor-proto/semver.md +++ b/crates/tor-proto/semver.md @@ -8,3 +8,6 @@ BREAKING: `ClientCirc::allow_stream_requests` now expects `self` to be `&Arc` ADDED: `HopNum` is now public ADDED: `ClientCirc::last_hop_num` +DEPRECATED: `ClientCirc::start_conversation_last_hop()` +ADDED: `ClientCirc::start_conversation()` to eventually replace +`ClientCirc::start_conversation_last_hop()` diff --git a/crates/tor-proto/src/circuit.rs b/crates/tor-proto/src/circuit.rs index 3d02a9d05..3b4f3f1b3 100644 --- a/crates/tor-proto/src/circuit.rs +++ b/crates/tor-proto/src/circuit.rs @@ -339,7 +339,7 @@ impl ClientCirc { &self.channel } - /// Start an ad-hoc protocol exchange to the final hop on this circuit + /// Start an ad-hoc protocol exchange to the specified hop on this circuit /// /// To use this: /// @@ -347,7 +347,7 @@ impl ClientCirc { /// the outcome of your conversation, /// and bundle it into a [`MsgHandler`]. /// - /// 1. Call `start_conversation_last_hop`. + /// 1. Call `start_conversation`. /// This will install a your handler, for incoming messages, /// and send the outgoing message (if you provided one). /// After that, each message on the circuit @@ -355,7 +355,7 @@ impl ClientCirc { /// is passed to your provided `reply_handler`. /// /// 2. Possibly call `send_msg` on the [`Conversation`], - /// from the call site of `start_conversation_last_hop`, + /// from the call site of `start_conversation`, /// possibly multiple times, from time to time, /// to send further desired messages to the peer. /// @@ -390,7 +390,7 @@ impl ClientCirc { /// while the conversation is in progress. /// /// After the conversation has finished, the circuit may be extended. - /// Or, `start_conversation_last_hop` may be called again; + /// Or, `start_conversation` may be called again; /// but, in that case there will be a gap between the two conversations, /// during which no `MsgHandler` is installed, /// and unexpected incoming messages would close the circuit. @@ -399,7 +399,7 @@ impl ClientCirc { /// /// ## Precise definition of the lifetime of a conversation /// - /// A conversation is in progress from entry to `start_conversation_last_hop`, + /// A conversation is in progress from entry to `start_conversation`, /// until entry to the body of the [`MsgHandler::handle_msg`] /// call which returns [`ConversationFinished`](MetaCellDisposition::ConversationFinished). /// (*Entry* since `handle_msg` is synchronously embedded @@ -415,11 +415,24 @@ impl ClientCirc { // // TODO hs: it might be nice to avoid exposing tor-cell APIs in the // tor-proto interface. - // - // TODO hs: Possibly this function should use - // HopNum or similar to indicate which hop we're talking to, rather than - // just doing "the last hop". #[cfg(feature = "send-control-msg")] + pub async fn start_conversation( + &self, + msg: Option, + reply_handler: impl MsgHandler + Send + 'static, + hop_num: HopNum, + ) -> Result> { + let handler = Box::new(msghandler::UserMsgHandler::new(hop_num, reply_handler)); + let conversation = Conversation(self); + conversation.send_internal(msg, Some(handler)).await?; + Ok(conversation) + } + + /// Start an ad-hoc protocol exchange to the final hop on this circuit + /// + /// See the [`ClientCirc::start_conversation`] docs for more information. + #[cfg(feature = "send-control-msg")] + #[deprecated(since = "0.13.0", note = "Use start_conversation instead.")] pub async fn start_conversation_last_hop( &self, msg: Option, @@ -432,10 +445,8 @@ impl ClientCirc { .path .last_hop_num() .ok_or_else(|| internal!("no last hop index"))?; - let handler = Box::new(msghandler::UserMsgHandler::new(last_hop, reply_handler)); - let conversation = Conversation(self); - conversation.send_internal(msg, Some(handler)).await?; - Ok(conversation) + + self.start_conversation(msg, reply_handler, last_hop).await } /// Tell this circuit to begin allowing the final hop of the circuit to try @@ -795,7 +806,7 @@ impl ClientCirc { /// Handle to use during an ongoing protocol exchange with a circuit's last hop /// -/// This is obtained from [`ClientCirc::start_conversation_last_hop`], +/// This is obtained from [`ClientCirc::start_conversation`], /// and used to send messages to the last hop relay. /// /// See also [`ConversationInHandler`], which is a type used for the same purpose @@ -817,7 +828,7 @@ impl Conversation<'_> { /// Send a `SendMsgAndInstallHandler` to the reactor and wait for the outcome /// - /// The guts of `start_conversation_last_hop` and `Conversation::send_msg` + /// The guts of `start_conversation` and `Conversation::send_msg` async fn send_internal( &self, msg: Option, diff --git a/crates/tor-proto/src/circuit/msghandler.rs b/crates/tor-proto/src/circuit/msghandler.rs index 8c0620942..485c7f4d7 100644 --- a/crates/tor-proto/src/circuit/msghandler.rs +++ b/crates/tor-proto/src/circuit/msghandler.rs @@ -1,5 +1,5 @@ //! A message handler trait for use with -//! [`ClientCirc::start_conversation_last_hop`](super::ClientCirc::start_conversation_last_hop). +//! [`ClientCirc::start_conversation`](super::ClientCirc::start_conversation). //! //! Although this is similar to `stream::cmdcheck`, I am deliberately leaving //! them separate. Conceivably they should be unified at some point down the @@ -18,7 +18,7 @@ use super::{ConversationInHandler, MetaCellDisposition}; /// circuit, and delivers them to a client if so. /// /// The handler is supplied to -/// [`ClientCirc::start_conversation_last_hop`](super::ClientCirc::start_conversation_last_hop). It +/// [`ClientCirc::start_conversation`](super::ClientCirc::start_conversation). It /// is used to check any incoming message whose stream ID is 0, and which would /// otherwise not be accepted on a given circuit. /// diff --git a/crates/tor-proto/src/circuit/reactor.rs b/crates/tor-proto/src/circuit/reactor.rs index 352f9cc93..bb9891e67 100644 --- a/crates/tor-proto/src/circuit/reactor.rs +++ b/crates/tor-proto/src/circuit/reactor.rs @@ -287,7 +287,7 @@ impl CircHop { /// This is passed to `MsgHandler::handle_msg`. /// /// See also [`ConversationInHandler`], which is a type used for the same purpose -/// but available to the caller of `start_conversation_last_hop` +/// but available to the caller of `start_conversation` // // This is the subset of the arguments to MetaCellHandler::handle_msg // which are needed to be able to call send_relay_cell.