Merge branch 'shuffle-hsdirs' into 'main'

tor-netdir: Shuffle the list of HS dirs used for downloading descriptors.

See merge request tpo/core/arti!1155

Cargo.lock

@@ -4165,8 +4165,8 @@ dependencies = [
  "either",
  "futures",
  "humantime 2.1.0",
- "itertools",
  "postage",
+ "rand 0.8.5",
  "rand_core 0.6.4",
  "retry-error",
  "safelog",
@@ -4175,6 +4175,7 @@ dependencies = [
  "thiserror",
  "tokio",
  "tor-async-utils",
+ "tor-basic-utils",
  "tor-chanmgr",
  "tor-checkable",
  "tor-circmgr",

crates/tor-basic-utils/src/test_rng.rs

@@ -64,7 +64,7 @@
 use rand::{RngCore, SeedableRng};
 // We'll use the same PRNG as the (current) standard.  We specify it here rather
 // than using StdRng, since we want determinism in the future.
-use rand_chacha::ChaCha12Rng as TestingRng;
+pub use rand_chacha::ChaCha12Rng as TestingRng;
 
 /// The seed type for the RNG we're returning.
 type Seed = <TestingRng as SeedableRng>::Seed;

crates/tor-hsclient/Cargo.toml

@@ -20,8 +20,8 @@ derive_more = "0.99.3"
 educe = "0.4.6"
 either = "1"
 futures = "0.3.14"
-itertools = "0.10.1"
 postage = { version = "0.5.0", default-features = false, features = ["futures-traits"] }
+rand = "0.8"
 rand_core = "0.6.2"
 retry-error = { path = "../retry-error", version = "0.4.0" }
 safelog = { path = "../safelog", version = "0.3.0" }
@@ -46,6 +46,7 @@ tracing = "0.1.36"
 humantime = "2"
 tokio-crate = { package = "tokio", version = "1.7", features = ["full"] }
 tor-async-utils = { path = "../tor-async-utils", version = "0.1.0" }
+tor-basic-utils = { path = "../tor-basic-utils", version = "0.7.0" }
 tor-chanmgr = { path = "../tor-chanmgr", version = "0.9.0" }
 tor-circmgr = { version = "0.8.0", path = "../tor-circmgr", features = ["hs-client", "testing"] }
 tor-guardmgr = { path = "../tor-guardmgr", version = "0.9.0", features = ["testing"] }

crates/tor-hsclient/src/connect.rs

@@ -10,7 +10,6 @@ use std::time::SystemTime;
 use async_trait::async_trait;
 use educe::Educe;
 use futures::{AsyncRead, AsyncWrite};
-use itertools::Itertools;
 use tor_hscrypto::Subcredential;
 use tracing::{debug, trace};
 
@@ -212,10 +211,12 @@ impl<'c, 'd, R: Runtime, M: MocksForConnect<R>> Context<'c, 'd, R, M> {
             // Seems to be not valid now.  Try to fetch a fresh one.
         }
 
-        let hs_dirs = self
-            .netdir
-            .hs_dirs(&self.hs_blind_id, HsDirOp::Download)
-            .collect_vec();
+        let hs_dirs = self.netdir.hs_dirs(
+            &self.hs_blind_id,
+            HsDirOp::Download,
+            &mut self.mocks.thread_rng(),
+        );
+
         trace!(
             "HS desc fetch for {}, using {} hsdirs",
             &self.hsid,
@@ -357,10 +358,17 @@ impl<'c, 'd, R: Runtime, M: MocksForConnect<R>> Context<'c, 'd, R, M> {
 trait MocksForConnect<R>: Clone {
     /// HS circuit pool
     type HsCircPool: MockableCircPool<R>;
+
+    /// A random number generator
+    type Rng: rand::Rng;
+
     /// Tell tests we got this descriptor text
     fn test_got_desc(&self, desc: &HsDesc) {
         eprintln!("HS DESC:\n{:?}\n", &desc); // TODO HS remove
     }
+
+    /// Return a random number generator
+    fn thread_rng(&self) -> Self::Rng;
 }
 /// Mock for `HsCircPool`
 #[async_trait]
@@ -384,6 +392,11 @@ trait MockableClientCirc {
 
 impl<R: Runtime> MocksForConnect<R> for () {
     type HsCircPool = HsCircPool<R>;
+    type Rng = rand::rngs::ThreadRng;
+
+    fn thread_rng(&self) -> Self::Rng {
+        rand::thread_rng()
+    }
 }
 #[async_trait]
 impl<R: Runtime> MockableCircPool<R> for HsCircPool<R> {
@@ -444,6 +457,7 @@ mod test {
     use std::{iter, panic::AssertUnwindSafe};
     use tokio_crate as tokio;
     use tor_async_utils::JoinReadWrite;
+    use tor_basic_utils::test_rng::{testing_rng, TestingRng};
     use tor_llcrypto::pk::curve25519;
     use tor_netdoc::doc::{hsdesc::test_data, netstatus::Lifetime};
     use tor_rtcompat::{tokio::TokioNativeTlsRuntime, CompoundRuntime};
@@ -473,9 +487,15 @@ mod test {
 
     impl<R: Runtime> MocksForConnect<R> for Mocks<()> {
         type HsCircPool = Mocks<()>;
+        type Rng = TestingRng;
+
         fn test_got_desc(&self, desc: &HsDesc) {
             self.mglobal.lock().unwrap().got_desc = Some(desc.clone());
         }
+
+        fn thread_rng(&self) -> Self::Rng {
+            testing_rng()
+        }
     }
     #[async_trait]
     impl<R: Runtime> MockableCircPool<R> for Mocks<()> {

crates/tor-netdir/src/lib.rs

@@ -68,6 +68,7 @@ use {hsdir_params::HsDirParams, hsdir_ring::HsDirRing, std::iter};
 use derive_more::{From, Into};
 use futures::stream::BoxStream;
 use num_enum::{IntoPrimitive, TryFromPrimitive};
+use rand::seq::SliceRandom;
 use serde::Deserialize;
 use std::collections::HashMap;
 use std::net::IpAddr;
@@ -258,7 +259,7 @@ impl From<u64> for RelayWeight {
 }
 
 /// An operation for which we might be requesting a hidden service directory.
-#[derive(Copy, Clone, Debug)]
+#[derive(Copy, Clone, Debug, PartialEq)]
 #[non_exhaustive]
 pub enum HsDirOp {
     /// Uploading an onion service descriptor.
@@ -1106,7 +1107,6 @@ impl NetDir {
         R: rand::Rng,
         P: FnMut(&Relay<'a>) -> bool,
     {
-        use rand::seq::SliceRandom;
         let relays: Vec<_> = self.relays().filter(usable).collect();
         // This algorithm uses rand::distributions::WeightedIndex, and uses
         // gives O(n) time and space  to build the index, plus O(log n)
@@ -1159,7 +1159,6 @@ impl NetDir {
         R: rand::Rng,
         P: FnMut(&Relay<'a>) -> bool,
     {
-        use rand::seq::SliceRandom;
         let relays: Vec<_> = self.relays().filter(usable).collect();
         // NOTE: See discussion in pick_relay().
         let mut relays = match relays[..].choose_multiple_weighted(rng, n, |r| {
@@ -1272,18 +1271,16 @@ impl NetDir {
     /// given onion service's descriptor at a given time period.
     ///
     /// When `op` is `Download`, the order is random.
-    // TODO HS ^ this is not in fact true right now.
     /// When `op` is `Upload`, the order is not specified.
     ///
     /// Return an error if the time period is not one returned by
     /// `onion_service_time_period` or `onion_service_secondary_time_periods`.
     #[cfg(feature = "hs-common")]
     #[allow(unused, clippy::missing_panics_doc)] // TODO hs: remove.
-    pub fn hs_dirs<'r>(
-        &'r self,
-        hsid: &'r HsBlindId,
-        op: HsDirOp,
-    ) -> impl Iterator<Item = Relay<'r>> + 'r {
+    pub fn hs_dirs<'r, R>(&'r self, hsid: &HsBlindId, op: HsDirOp, rng: &mut R) -> Vec<Relay<'r>>
+    where
+        R: rand::Rng,
+    {
         // Algorithm:
         //
         // 1. Determine which HsDirRing to use, based on the time period.
@@ -1310,7 +1307,8 @@ impl NetDir {
         //       service, any nodes already chosen are disregarded (i.e. skipped over)
         //       when choosing a replica's hsdir_spread_store nodes.
 
-        self.hsdir_rings
+        let mut hs_dirs = self
+            .hsdir_rings
             .iter_for_op(op)
             .cartesian_product(1..=n_replicas) // 1-indexed !
             .flat_map(move |(ring, replica): (&HsDirRing, u8)| {
@@ -1321,6 +1319,20 @@ impl NetDir {
                 // This ought not to be None but let's not panic or bail if it is
                 self.relay_by_rs_idx(*rs_idx)
             })
+            .collect_vec();
+
+        match op {
+            HsDirOp::Download => {
+                // When `op` is `Download`, the order is random.
+                hs_dirs.shuffle(rng);
+            }
+            #[cfg(feature = "hs-service")]
+            HsDirOp::Upload => {
+                // When `op` is `Upload`, the order is not specified.
+            }
+        }
+
+        hs_dirs
     }
 }