Merge branch 'ipt-misc' into 'main'

tor-hsservice: Pass correct parameters to Establisher

See merge request tpo/core/arti!1597

5 files changed, 45 insertions, 54 deletions

diff --git a/crates/tor-hscrypto/src/macros.rs b/crates/tor-hscrypto/src/macros.rs
index fa7ca9945..ffb7fd09a 100644
--- a/crates/tor-hscrypto/src/macros.rs
+++ b/crates/tor-hscrypto/src/macros.rs
@@ -16,7 +16,7 @@ macro_rules! define_pk_keypair {
 
             #[doc = concat!("The private counterpart of a [`", stringify!($pk), "Key'].")]
             $(#[$sk_meta])*
-            #[derive(derive_more::From,derive_more::Into,derive_more::AsRef)]
+            #[derive(derive_more::From, derive_more::Into, derive_more::AsRef)]
             pub struct $sk ($skt);
 
             impl std::fmt::Debug for $sk
diff --git a/crates/tor-hsservice/src/ipt_mgr.rs b/crates/tor-hsservice/src/ipt_mgr.rs
index eddf39fb0..cc2761008 100644
--- a/crates/tor-hsservice/src/ipt_mgr.rs
+++ b/crates/tor-hsservice/src/ipt_mgr.rs
@@ -186,7 +186,12 @@ struct Ipt {
     establisher: Box<dyn Any + Send + Sync + 'static>,
 
     /// `KS_hs_ipt_sid`, `KP_hs_ipt_sid`
-    k_sid: HsIntroPtSessionIdKeypair,
+    ///
+    /// This is an `Arc` because:
+    ///  * The manager needs a copy so that it can save it to disk.
+    ///  * The establisher needs a copy to actually use.
+    ///  * The underlying secret key type is not `Clone`.
+    k_sid: Arc<HsIntroPtSessionIdKeypair>,
 
     /// `KS_hss_ntor`, `KP_hss_ntor`
     // TODO HSS how do we provide the private half to the recipients of our rend reqs?
@@ -257,7 +262,7 @@ struct IsCurrent;
 
 /// Record of intro point establisher state, as stored on disk
 #[derive(Serialize, Deserialize)]
-#[allow(dead_code)] // TODO HSS remove
+#[allow(dead_code)] // TODO HSS-IPT-PERSIST remove
 struct StateRecord {
     /// Relays
     ipt_relays: Vec<RelayRecord>,
@@ -265,7 +270,7 @@ struct StateRecord {
 
 /// Record of a selected intro point relay, as stored on disk
 #[derive(Serialize, Deserialize)]
-#[allow(dead_code)] // TODO HSS remove
+#[allow(dead_code)] // TODO HSS-IPT-PERSIST remove
 struct RelayRecord {
     /// Which relay?
     relay: RelayIds,
@@ -275,11 +280,11 @@ struct RelayRecord {
 
 /// Record of a single intro point, as stored on disk
 #[derive(Serialize, Deserialize)]
-#[allow(dead_code)] // TODO HSS remove
+#[allow(dead_code)] // TODO HSS-IPT-PERSIST remove
 struct IptRecord {
     /// Used to find the cryptographic keys, amongst other things
     lid: IptLocalId,
-    // TODO HSS other fields need to be here!
+    // TODO HSS-IPT-PERSIST other fields need to be here!
 }
 
 /// Return value from one call to the main loop iteration
@@ -337,26 +342,26 @@ impl IptRelay {
         imm: &Immutable<R>,
         mockable: &mut M,
     ) -> Result<(), FatalError> {
-        let params = IptParameters {
-            netdir_provider: imm.dirprovider.clone(),
-            introduce_tx: imm.output_rend_reqs.clone(),
-            // TODO HSS IntroPointId lacks a constructor and maybe should change anyway
-            intro_pt_id: todo!(),
-            target: self.relay.clone(),
-            ipt_sid_keypair: todo!(),    // TODO HSS
-            accepting_requests: todo!(), // TODO HSS
-        };
-        let (establisher, mut watch_rx) = mockable.make_new_ipt(imm, params)?;
-
         // we'll treat it as Establishing until we find otherwise
         let status_last = TS::Establishing {
             started: imm.runtime.now(),
         };
 
-        let rng = mockable.thread_rng();
+        let mut rng = mockable.thread_rng();
         let lid: IptLocalId = rng.gen();
         let k_hss_ntor = HsSvcNtorKeypair::generate(&mut rng);
         let k_sid = ed25519::Keypair::generate(&mut rng.rng_compat()).into();
+        let k_sid: Arc<HsIntroPtSessionIdKeypair> = Arc::new(k_sid);
+
+        let params = IptParameters {
+            netdir_provider: imm.dirprovider.clone(),
+            introduce_tx: imm.output_rend_reqs.clone(),
+            lid,
+            target: self.relay.clone(),
+            k_sid: k_sid.clone(),
+            accepting_requests: ipt_establish::RequestDisposition::NotAdvertised,
+        };
+        let (establisher, mut watch_rx) = mockable.make_new_ipt(imm, params)?;
 
         imm.runtime
             .spawn({
@@ -410,7 +415,7 @@ impl Ipt {
 
     /// Construct the information needed by the publisher for this intro point
     fn for_publish(&self, details: &ipt_establish::GoodIptDetails) -> Result<ipt_set::Ipt, Bug> {
-        let k_sid: &ed25519::Keypair = self.k_sid.as_ref();
+        let k_sid: &ed25519::Keypair = (*self.k_sid).as_ref();
         tor_netdoc::doc::hsdesc::IntroPointDesc::builder()
             .link_specifiers(details.link_specifiers.clone())
             .ipt_kp_ntor(details.ipt_kp_ntor)
@@ -433,7 +438,7 @@ impl<R: Runtime, M: Mockable<R>> IptManager<R, M> {
         shutdown: oneshot::Receiver<Void>,
         mockable: M,
     ) -> Result<Self, StartupError> {
-        // TODO HSS load persistent state
+        // TODO HSS-IPT-PERSIST load persistent state
 
         // We don't need buffering; since this is written to by dedicated tasks which
         // are reading watches.
@@ -847,7 +852,7 @@ impl<R: Runtime, M: Mockable<R>> IptManager<R, M> {
 
         //---------- store persistent state ----------
 
-        // TODO HSS store persistent state
+        // TODO HSS-IPT-PERSIST store persistent state
 
         Ok(())
     }
@@ -1102,11 +1107,6 @@ pub(crate) trait Mockable<R>: Debug + Send + Sync + Sized + 'static {
         imm: &Immutable<R>,
         params: IptParameters,
     ) -> Result<(Self::IptEstablisher, watch::Receiver<IptStatus>), FatalError>;
-
-    /// Call `Publisher::new_intro_points`
-    fn new_intro_points(&mut self, ipts: ()) {
-        todo!() // TODO HSS there should be no default impl; code should be in Real's impl
-    }
 }
 
 impl<R: Runtime> Mockable<R> for Real<R> {
diff --git a/crates/tor-hsservice/src/req.rs b/crates/tor-hsservice/src/req.rs
index c6eb12fb2..4f8a5c5c2 100644
--- a/crates/tor-hsservice/src/req.rs
+++ b/crates/tor-hsservice/src/req.rs
@@ -10,10 +10,7 @@ use tor_cell::relaycell::msg::Introduce2;
 use tor_error::Bug;
 use tor_proto::{circuit::handshake::hs_ntor::HsNtorServiceInput, stream::DataStream};
 
-use crate::{
-    svc::{rend_handshake, IntroPointId},
-    ClientError,
-};
+use crate::{svc::rend_handshake, ClientError, IptLocalId};
 
 /// Request to complete an introduction/rendezvous handshake.
 ///
@@ -27,7 +24,7 @@ use crate::{
 #[derive(Debug)]
 pub struct RendRequest {
     /// The introduction point that sent this request.
-    intro_point: IntroPointId,
+    ipt_lid: IptLocalId,
 
     /// The message as received from the remote introduction point.
     raw: Introduce2,
@@ -96,9 +93,9 @@ pub struct OnionServiceDataStream {
 
 impl RendRequest {
     /// Construct a new RendRequest from its parts.
-    pub(crate) fn new(source: IntroPointId, msg: Introduce2) -> Self {
+    pub(crate) fn new(ipt_lid: IptLocalId, msg: Introduce2) -> Self {
         Self {
-            intro_point: source,
+            ipt_lid,
             raw: msg,
             expanded: Default::default(),
         }
diff --git a/crates/tor-hsservice/src/svc.rs b/crates/tor-hsservice/src/svc.rs
index 304ee3c3e..6f8d868a9 100644
--- a/crates/tor-hsservice/src/svc.rs
+++ b/crates/tor-hsservice/src/svc.rs
@@ -136,12 +136,6 @@ struct IntroPointState {
     // TODO HSS: use diziet's structures  from `hssvc-ipt-algorithms.md` once those are more settled.
 }
 
-/// Identifier for a single introduction point of an onion point.
-//
-// TODO HSS maybe use a nicer type, like a generational arena index.
-#[derive(Debug, Clone)]
-pub(crate) struct IntroPointId(RelayIds);
-
 impl<R: Runtime> OnionService<R> {
     /// Create (but do not launch) a new onion service.
     pub fn new(config: (), netdir_provider: (), circmgr: ()) -> Self {
diff --git a/crates/tor-hsservice/src/svc/ipt_establish.rs b/crates/tor-hsservice/src/svc/ipt_establish.rs
index bcea4fda1..17b2ceb45 100644
--- a/crates/tor-hsservice/src/svc/ipt_establish.rs
+++ b/crates/tor-hsservice/src/svc/ipt_establish.rs
@@ -31,10 +31,9 @@ use tracing::debug;
 use void::{ResultVoidErrExt as _, Void};
 
 use crate::svc::{LinkSpecs, NtorPublicKey};
-use crate::{FatalError, RendRequest};
+use crate::{FatalError, IptLocalId, RendRequest};
 
 use super::netdir::{wait_for_netdir, wait_for_netdir_to_list, NetdirProviderShutdown};
-use super::IntroPointId;
 
 /// Handle onto the task which is establishing and maintaining one IPT
 pub(crate) struct IptEstablisher {
@@ -166,11 +165,12 @@ impl IptError {
 pub(crate) struct IptParameters {
     pub(crate) netdir_provider: Arc<dyn NetDirProvider>,
     pub(crate) introduce_tx: mpsc::Sender<RendRequest>,
-    pub(crate) intro_pt_id: IntroPointId,
+    pub(crate) lid: IptLocalId,
     // TODO HSS: Should this and the following elements be part of some
     // configuration object?
     pub(crate) target: RelayIds,
-    pub(crate) ipt_sid_keypair: HsIntroPtSessionIdKeypair,
+    /// `K_hs_ipt_sid`
+    pub(crate) k_sid: Arc<HsIntroPtSessionIdKeypair>,
     pub(crate) accepting_requests: RequestDisposition,
 }
 
@@ -198,9 +198,9 @@ impl IptEstablisher {
         let IptParameters {
             netdir_provider,
             introduce_tx,
-            intro_pt_id,
+            lid,
             target,
-            ipt_sid_keypair,
+            k_sid,
             accepting_requests,
         } = params;
         if matches!(accepting_requests, RequestDisposition::Shutdown) {
@@ -216,9 +216,9 @@ impl IptEstablisher {
             runtime: runtime.clone(),
             pool,
             netdir_provider,
-            intro_pt_id,
+            lid,
             target,
-            ipt_sid_keypair,
+            k_sid,
             introduce_tx,
             // TODO HSS This should come from the configuration.
             extensions: EstIntroExtensionSet { dos_params: None },
@@ -442,14 +442,14 @@ struct Reactor<R: Runtime> {
     ///
     /// TODO HSS: I am assuming that this type will be a unique identifier, and
     /// will change whenever RelayIds and/or HsIntroPtSessionIdKeypair changes.
-    intro_pt_id: IntroPointId,
+    lid: IptLocalId,
     /// The target introduction point.
     target: RelayIds,
     /// The keypair to use when establishing the introduction point.
     ///
     /// Knowledge of this private key prevents anybody else from impersonating
     /// us to the introduction point.
-    ipt_sid_keypair: HsIntroPtSessionIdKeypair,
+    k_sid: Arc<HsIntroPtSessionIdKeypair>,
     /// The extensions to use when establishing the introduction point.
     ///
     /// TODO: Should this be able to change over time if we re-establish this
@@ -562,7 +562,7 @@ impl<R: Runtime> Reactor<R> {
             .map_err(into_internal!("Somehow built a circuit with no hops!?"))?;
 
         let establish_intro = {
-            let ipt_sid_id = self.ipt_sid_keypair.as_ref().public.into();
+            let ipt_sid_id = (*self.k_sid).as_ref().public.into();
             let mut details = EstablishIntroDetails::new(ipt_sid_id);
             if let Some(dos_params) = &self.extensions.dos_params {
                 details.set_extension_dos(dos_params.clone());
@@ -571,7 +571,7 @@ impl<R: Runtime> Reactor<R> {
                 .binding_key(intro_pt_hop)
                 .ok_or(internal!("No binding key for introduction point!?"))?;
             let body: Vec<u8> = details
-                .sign_and_encode(self.ipt_sid_keypair.as_ref(), circuit_binding_key.hs_mac())
+                .sign_and_encode((*self.k_sid).as_ref(), circuit_binding_key.hs_mac())
                 .map_err(IptError::CreateEstablishIntro)?;
 
             // TODO HSS: This is ugly, but it is the sensible way to munge the above
@@ -596,7 +596,7 @@ impl<R: Runtime> Reactor<R> {
             established_tx: Some(established_tx),
             introduce_tx: self.introduce_tx.clone(),
             state: self.state.clone(),
-            intro_pt_id: self.intro_pt_id.clone(),
+            lid: self.lid,
         };
         let conversation = circuit
             .start_conversation(Some(establish_intro), handler, intro_pt_hop)
@@ -656,7 +656,7 @@ struct IptMsgHandler {
 
     /// Unique identifier for the introduction point (including the current
     /// keys).  Used to tag requests.
-    intro_pt_id: IntroPointId,
+    lid: IptLocalId,
 }
 
 impl tor_proto::circuit::MsgHandler for IptMsgHandler {
@@ -699,7 +699,7 @@ impl tor_proto::circuit::MsgHandler for IptMsgHandler {
                     RequestDisposition::Advertised => {}
                 }
 
-                let request = RendRequest::new(self.intro_pt_id.clone(), introduce2);
+                let request = RendRequest::new(self.lid, introduce2);
                 match self.introduce_tx.try_send(request) {
                     Ok(()) => Ok(()),
                     Err(e) => {