Merge remote-tracking branch 'dgoulet/ticket23310_032_02'

src/or/hs_client.c

@@ -521,7 +521,7 @@ client_rendezvous_circ_has_opened(origin_circuit_t *circ)
  * to a newly allocated extend_info_t object fully initialized. Return NULL if
  * we can't convert it for which chances are that we are missing or malformed
  * link specifiers. */
-static extend_info_t *
+STATIC extend_info_t *
 desc_intro_point_to_extend_info(const hs_desc_intro_point_t *ip)
 {
   extend_info_t *ei;
@@ -594,7 +594,7 @@ intro_point_is_usable(const ed25519_public_key_t *service_pk,
 /* Using a descriptor desc, return a newly allocated extend_info_t object of a
  * randomly picked introduction point from its list. Return NULL if none are
  * usable. */
-static extend_info_t *
+STATIC extend_info_t *
 client_get_random_intro(const ed25519_public_key_t *service_pk)
 {
   extend_info_t *ei = NULL, *ei_excluded = NULL;
@@ -646,6 +646,12 @@ client_get_random_intro(const ed25519_public_key_t *service_pk)
       /* If this pick is in the ExcludeNodes list, we keep its reference so if
        * we ever end up not being able to pick anything else and StrictNodes is
        * unset, we'll use it. */
+      if (ei_excluded) {
+        /* If something was already here free it. After the loop is gone we
+         * will examine the last excluded intro point, and that's fine since
+         * that's random anyway */
+        extend_info_free(ei_excluded);
+      }
       ei_excluded = ei;
       continue;
     }
@@ -662,6 +668,7 @@ client_get_random_intro(const ed25519_public_key_t *service_pk)
   if (options->StrictNodes) {
     log_warn(LD_REND, "Every introduction points are in the ExcludeNodes set "
              "and StrictNodes is set. We can't connect.");
+    extend_info_free(ei);
     ei = NULL;
   }
 

src/or/hs_client.h

@@ -71,7 +71,13 @@ void hs_client_free_all(void);
 STATIC routerstatus_t *
 pick_hsdir_v3(const ed25519_public_key_t *onion_identity_pk);
 
-#endif
+STATIC extend_info_t *
+client_get_random_intro(const ed25519_public_key_t *service_pk);
+
+STATIC extend_info_t *
+desc_intro_point_to_extend_info(const hs_desc_intro_point_t *ip);
+
+#endif /* HS_CLIENT_PRIVATE */
 
 #endif /* TOR_HS_CLIENT_H */
 

src/test/hs_indexes.py

@@ -0,0 +1,70 @@
+#
+# The hidden service subsystem has two type of index. The first type is a
+# value that each node in the network gets assigned to using their identity
+# key which is their position in the hashring. (hs_build_hsdir_index()).
+#
+# The second type is a value that both the client and service computes to
+# store/fetch the descriptor on the hashring. (hs_build_hs_index()).
+#
+
+import sys
+import hashlib
+import struct
+import base64
+
+# Python 3.6+, the SHA3 is available in hashlib natively. Else this requires
+# the pysha3 package (pip install pysha3).
+if sys.version_info < (3, 6):
+    import sha3
+    # Test vector to make sure the right sha3 version will be used. pysha3 < 1.0
+    # used the old Keccak implementation. During the finalization of SHA3, NIST
+    # changed the delimiter suffix from 0x01 to 0x06. The Keccak sponge function
+    # stayed the same. pysha3 1.0 provides the previous Keccak hash, too.
+    TEST_VALUE = "e167f68d6563d75bb25f3aa49c29ef612d41352dc00606de7cbd630bb2665f51"
+    if TEST_VALUE != sha3.sha3_256(b"Hello World").hexdigest():
+        print("pysha3 version is < 1.0. Please install from:")
+        print("https://github.com/tiran/pysha3https://github.com/tiran/pysha3")
+        sys.exit(1)
+
+# The first index we'll build is the position index in the hashring that is
+# constructed by the hs_build_hsdir_index() function. Construction is:
+#   SHA3-256("node-idx" | node_identity |
+#            shared_random_value | INT_8(period_length) | INT_8(period_num) )
+
+PREFIX = "node-idx".encode()
+# 32 bytes ed25519 pubkey.
+IDENTITY = ("\x42" * 32).encode()
+# SRV is 32 bytes.
+SRV = ("\x43" * 32).encode()
+# Time period length is a 8 bytes value.
+PERIOD_LEN = 1440
+# Period number is a 8 bytes value.
+PERIOD_NUM = 42
+
+data = struct.pack('!8s32s32sQQ', PREFIX, IDENTITY, SRV, PERIOD_NUM,
+                                  PERIOD_LEN)
+hsdir_index = hashlib.sha3_256(data).hexdigest()
+
+print("[hs_build_hsdir_index] %s" % (hsdir_index))
+
+# The second index we'll build is where the HS stores and the client fetches
+# the descriptor on the hashring. It is constructed by the hs_build_hs_index()
+# function and the construction is:
+#   SHA3-256("store-at-idx" | blinded_public_key |
+#            INT_8(replicanum) | INT_8(period_num) | INT_8(period_length) )
+
+PREFIX = "store-at-idx".encode()
+# 32 bytes ed25519 pubkey.
+PUBKEY = ("\x42" * 32).encode()
+# Replica number is a 8 bytes value.
+REPLICA_NUM = 1
+# Time period length is a 8 bytes value.
+PERIOD_LEN = 1440
+# Period number is a 8 bytes value.
+PERIOD_NUM = 42
+
+data = struct.pack('!12s32sQQQ', PREFIX, PUBKEY, REPLICA_NUM, PERIOD_LEN,
+                                   PERIOD_NUM)
+hs_index = hashlib.sha3_256(data).hexdigest()
+
+print("[hs_build_hs_index]   %s" % (hs_index))

src/test/hs_test_helpers.c

@@ -18,28 +18,29 @@ hs_helper_build_intro_point(const ed25519_keypair_t *signing_kp, time_t now,
   hs_desc_intro_point_t *intro_point = NULL;
   hs_desc_intro_point_t *ip = hs_desc_intro_point_new();
 
+  /* For a usable intro point we need at least two link specifiers: One legacy
+   * keyid and one ipv4 */
   {
-    hs_desc_link_specifier_t *ls = tor_malloc_zero(sizeof(*ls));
-    if (legacy) {
-      ls->type = LS_LEGACY_ID;
-      memcpy(ls->u.legacy_id, "0299F268FCA9D55CD157976D39AE92B4B455B3A8",
-             DIGEST_LEN);
-    } else {
-      ls->u.ap.port = 9001;
-      int family = tor_addr_parse(&ls->u.ap.addr, addr);
-      switch (family) {
-        case AF_INET:
-          ls->type = LS_IPV4;
+    hs_desc_link_specifier_t *ls_legacy = tor_malloc_zero(sizeof(*ls_legacy));
+    hs_desc_link_specifier_t *ls_v4 = tor_malloc_zero(sizeof(*ls_v4));
+    ls_legacy->type = LS_LEGACY_ID;
+    memcpy(ls_legacy->u.legacy_id, "0299F268FCA9D55CD157976D39AE92B4B455B3A8",
+           DIGEST_LEN);
+    ls_v4->u.ap.port = 9001;
+    int family = tor_addr_parse(&ls_v4->u.ap.addr, addr);
+    switch (family) {
+    case AF_INET:
+          ls_v4->type = LS_IPV4;
           break;
         case AF_INET6:
-          ls->type = LS_IPV6;
+          ls_v4->type = LS_IPV6;
           break;
         default:
           /* Stop the test, not suppose to have an error. */
           tt_int_op(family, OP_EQ, AF_INET);
-      }
     }
-    smartlist_add(ip->link_specifiers, ls);
+    smartlist_add(ip->link_specifiers, ls_legacy);
+    smartlist_add(ip->link_specifiers, ls_v4);
   }
 
   ret = ed25519_keypair_generate(&auth_kp, 0);
@@ -137,7 +138,7 @@ hs_helper_build_hs_desc_impl(unsigned int no_ip,
     smartlist_add(desc->encrypted_data.intro_points,
               hs_helper_build_intro_point(signing_kp, now, "3.2.1.4", 1));
     smartlist_add(desc->encrypted_data.intro_points,
-              hs_helper_build_intro_point(signing_kp, now, "", 1));
+              hs_helper_build_intro_point(signing_kp, now, "5.6.7.8", 1));
   }
 
   descp = desc;

src/test/include.am

@@ -332,6 +332,8 @@ EXTRA_DIST += \
 	src/test/bt_test.py \
 	src/test/ntor_ref.py \
 	src/test/hs_ntor_ref.py \
+	src/test/hs_build_address.py \
+	src/test/hs_indexes.py \
 	src/test/fuzz_static_testcases.sh \
 	src/test/slownacl_curve25519.py \
 	src/test/zero_length_keys.sh \

src/test/test_hs_client.c

@@ -8,6 +8,7 @@
 
 #define CRYPTO_PRIVATE
 #define MAIN_PRIVATE
+#define HS_CLIENT_PRIVATE
 #define TOR_CHANNEL_INTERNAL_
 #define CIRCUITBUILD_PRIVATE
 #define CIRCUITLIST_PRIVATE
@@ -17,17 +18,22 @@
 #include "test_helpers.h"
 #include "log_test_helpers.h"
 #include "rend_test_helpers.h"
+#include "hs_test_helpers.h"
 
 #include "config.h"
 #include "crypto.h"
 #include "channeltls.h"
+#include "routerset.h"
 
 #include "hs_circuit.h"
+#include "hs_client.h"
 #include "hs_ident.h"
+#include "hs_cache.h"
 #include "circuitlist.h"
 #include "circuitbuild.h"
 #include "connection.h"
 #include "connection_edge.h"
+#include "networkstatus.h"
 
 static int
 mock_connection_ap_handshake_send_begin(entry_connection_t *ap_conn)
@@ -36,6 +42,15 @@ mock_connection_ap_handshake_send_begin(entry_connection_t *ap_conn)
   return 0;
 }
 
+static networkstatus_t mock_ns;
+
+static networkstatus_t *
+mock_networkstatus_get_live_consensus(time_t now)
+{
+  (void) now;
+  return &mock_ns;
+}
+
 /* Test helper function: Setup a circuit and a stream with the same hidden
  * service destination, and put them in <b>circ_out</b> and
  * <b>conn_out</b>. Make the stream wait for circuits to be established to the
@@ -276,11 +291,176 @@ test_e2e_rend_circuit_setup(void *arg)
   circuit_free(TO_CIRCUIT(or_circ));
 }
 
+/** Test client logic for picking intro points from a descriptor. Also test how
+ *  ExcludeNodes and intro point failures affect picking intro points. */
+static void
+test_client_pick_intro(void *arg)
+{
+  int ret;
+  ed25519_keypair_t service_kp;
+  hs_descriptor_t *desc = NULL;
+
+  MOCK(networkstatus_get_live_consensus,
+       mock_networkstatus_get_live_consensus);
+
+  (void) arg;
+
+  hs_init();
+
+  /* Generate service keypair */
+  tt_int_op(0, OP_EQ, ed25519_keypair_generate(&service_kp, 0));
+
+  /* Set time */
+  ret = parse_rfc1123_time("Sat, 26 Oct 1985 13:00:00 UTC",
+                           &mock_ns.valid_after);
+  tt_int_op(ret, OP_EQ, 0);
+  ret = parse_rfc1123_time("Sat, 26 Oct 1985 14:00:00 UTC",
+                           &mock_ns.fresh_until);
+  tt_int_op(ret, OP_EQ, 0);
+
+  update_approx_time(mock_ns.fresh_until-10);
+  time_t now = approx_time();
+
+  /* Test logic:
+   *
+   * 1) Add our desc with intro points to the HS cache.
+   *
+   * 2) Mark all descriptor intro points except _the chosen one_ as
+   *    failed. Then query the desc to get a random intro: check that we got
+   *    _the chosen one_. Then fail the chosen one as well, and see that no
+   *    intros are returned.
+   *
+   * 3) Then clean the intro state cache and get an intro point.
+   *
+   * 4) Try fetching an intro with the wrong service key: shouldn't work
+   *
+   * 5) Set StrictNodes and put all our intro points in ExcludeNodes: see that
+   *    nothing is returned.
+   */
+
+  /* 1) Add desc to HS cache */
+  {
+    char *encoded = NULL;
+    desc = hs_helper_build_hs_desc_with_ip(&service_kp);
+    ret = hs_desc_encode_descriptor(desc, &service_kp, &encoded);
+    tt_int_op(ret, OP_EQ, 0);
+    tt_assert(encoded);
+
+    /* store it */
+    hs_cache_store_as_client(encoded, &service_kp.pubkey);
+
+    /* fetch it to make sure it works */
+    const hs_descriptor_t *fetched_desc =
+      hs_cache_lookup_as_client(&service_kp.pubkey);
+    tt_assert(fetched_desc);
+    tt_mem_op(fetched_desc->subcredential, OP_EQ, desc->subcredential,
+              DIGEST256_LEN);
+    tt_assert(!tor_mem_is_zero((char*)fetched_desc->subcredential,
+                               DIGEST256_LEN));
+    tor_free(encoded);
+  }
+
+  /* 2) Mark all intro points except _the chosen one_ as failed. Then query the
+   *   desc and get a random intro: check that we got _the chosen one_. */
+  {
+    /* Pick the chosen intro point and get its ei */
+    hs_desc_intro_point_t *chosen_intro_point =
+      smartlist_get(desc->encrypted_data.intro_points, 0);
+    extend_info_t *chosen_intro_ei =
+      desc_intro_point_to_extend_info(chosen_intro_point);
+    tt_assert(chosen_intro_point);
+    tt_assert(chosen_intro_ei);
+
+    /* Now mark all other intro points as failed */
+    SMARTLIST_FOREACH_BEGIN(desc->encrypted_data.intro_points,
+                            hs_desc_intro_point_t *, ip) {
+      /* Skip the chosen intro point */
+      if (ip == chosen_intro_point) {
+        continue;
+      }
+      ed25519_public_key_t *intro_auth_key = &ip->auth_key_cert->signed_key;
+      hs_cache_client_intro_state_note(&service_kp.pubkey,
+                                       intro_auth_key,
+                                       INTRO_POINT_FAILURE_GENERIC);
+    } SMARTLIST_FOREACH_END(ip);
+
+    /* Try to get a random intro: Should return the chosen one! */
+    extend_info_t *ip = client_get_random_intro(&service_kp.pubkey);
+    tor_assert(ip);
+    tt_assert(!tor_mem_is_zero((char*)ip->identity_digest, DIGEST_LEN));
+    tt_mem_op(ip->identity_digest, OP_EQ, chosen_intro_ei->identity_digest,
+              DIGEST_LEN);
+
+    extend_info_free(chosen_intro_ei);
+    extend_info_free(ip);
+
+    /* Now also mark the chosen one as failed: See that we can't get any intro
+       points anymore. */
+    hs_cache_client_intro_state_note(&service_kp.pubkey,
+                                &chosen_intro_point->auth_key_cert->signed_key,
+                                     INTRO_POINT_FAILURE_TIMEOUT);
+    ip = client_get_random_intro(&service_kp.pubkey);
+    tor_assert(!ip);
+  }
+
+  /* 3) Clean the intro state cache and get an intro point */
+  {
+    /* Pretend we are 5 mins in the future and order a cleanup of the intro
+     * state. This should clean up the intro point failures and allow us to get
+     * an intro. */
+    hs_cache_client_intro_state_clean(now + 5*60);
+
+    /* Get an intro. It should work! */
+    extend_info_t *ip = client_get_random_intro(&service_kp.pubkey);
+    tor_assert(ip);
+    extend_info_free(ip);
+  }
+
+  /* 4) Try fetching an intro with the wrong service key: shouldn't work */
+  {
+    ed25519_keypair_t dummy_kp;
+    tt_int_op(0, OP_EQ, ed25519_keypair_generate(&dummy_kp, 0));
+    extend_info_t *ip = client_get_random_intro(&dummy_kp.pubkey);
+    tor_assert(!ip);
+  }
+
+  /* 5) Set StrictNodes and put all our intro points in ExcludeNodes: see that
+   *    nothing is returned. */
+  {
+    get_options_mutable()->ExcludeNodes = routerset_new();
+    get_options_mutable()->StrictNodes = 1;
+    SMARTLIST_FOREACH_BEGIN(desc->encrypted_data.intro_points,
+                            hs_desc_intro_point_t *, ip) {
+      extend_info_t *intro_ei = desc_intro_point_to_extend_info(ip);
+      if (intro_ei) {
+        const char *ptr;
+        char ip_addr[TOR_ADDR_BUF_LEN];
+        /* We need to decorate in case it is an IPv6 else routerset_parse()
+         * doesn't like it. */
+        ptr = tor_addr_to_str(ip_addr, &intro_ei->addr, sizeof(ip_addr), 1);
+        tt_assert(ptr == ip_addr);
+        ret = routerset_parse(get_options_mutable()->ExcludeNodes,
+                              ip_addr, "");
+        tt_int_op(ret, OP_EQ, 0);
+        extend_info_free(intro_ei);
+      }
+    } SMARTLIST_FOREACH_END(ip);
+
+    extend_info_t *ip = client_get_random_intro(&service_kp.pubkey);
+    tt_assert(!ip);
+  }
+
+ done:
+  hs_descriptor_free(desc);
+}
+
 struct testcase_t hs_client_tests[] = {
   { "e2e_rend_circuit_setup_legacy", test_e2e_rend_circuit_setup_legacy,
     TT_FORK, NULL, NULL },
   { "e2e_rend_circuit_setup", test_e2e_rend_circuit_setup,
     TT_FORK, NULL, NULL },
+  { "client_pick_intro", test_client_pick_intro,
+    TT_FORK, NULL, NULL },
   END_OF_TESTCASES
 };
 

src/test/test_hs_common.c

@@ -1453,31 +1453,102 @@ helper_client_pick_hsdir(const ed25519_public_key_t *onion_identity_pk,
   ;
 }
 
-/** Set the consensus and system time based on <b>between_srv_and_tp</b>. If
- *  <b>between_srv_and_tp</b> is set, then set the time to be inside the time
- *  segment between SRV#N and TP#N. */
+static void
+test_hs_indexes(void *arg)
+{
+  int ret;
+  uint64_t period_num = 42;
+  ed25519_public_key_t pubkey;
+
+  (void) arg;
+
+  /* Build the hs_index */
+  {
+    uint8_t hs_index[DIGEST256_LEN];
+    const char *b32_test_vector =
+      "37e5cbbd56a22823714f18f1623ece5983a0d64c78495a8cfab854245e5f9a8a";
+    char test_vector[DIGEST256_LEN];
+    ret = base16_decode(test_vector, sizeof(test_vector), b32_test_vector,
+                        strlen(b32_test_vector));
+    tt_int_op(ret, OP_EQ, sizeof(test_vector));
+    /* Our test vector uses a public key set to 32 bytes of \x42. */
+    memset(&pubkey, '\x42', sizeof(pubkey));
+    hs_build_hs_index(1, &pubkey, period_num, hs_index);
+    tt_mem_op(hs_index, OP_EQ, test_vector, sizeof(hs_index));
+  }
+
+  /* Build the hsdir_index */
+  {
+    uint8_t srv[DIGEST256_LEN];
+    uint8_t hsdir_index[DIGEST256_LEN];
+    const char *b32_test_vector =
+      "db475361014a09965e7e5e4d4a25b8f8d4b8f16cb1d8a7e95eed50249cc1a2d5";
+    char test_vector[DIGEST256_LEN];
+    ret = base16_decode(test_vector, sizeof(test_vector), b32_test_vector,
+                        strlen(b32_test_vector));
+    tt_int_op(ret, OP_EQ, sizeof(test_vector));
+    /* Our test vector uses a public key set to 32 bytes of \x42. */
+    memset(&pubkey, '\x42', sizeof(pubkey));
+    memset(srv, '\x43', sizeof(srv));
+    hs_build_hsdir_index(&pubkey, srv, period_num, hsdir_index);
+    tt_mem_op(hsdir_index, OP_EQ, test_vector, sizeof(hsdir_index));
+  }
+
+ done:
+  ;
+}
+
+#define EARLY_IN_SRV_TO_TP 0
+#define LATE_IN_SRV_TO_TP 1
+#define EARLY_IN_TP_TO_SRV 2
+#define LATE_IN_TP_TO_SRV 3
+
+/** Set the consensus and system time based on <b>position</b>. See the
+ *    following diagram for details:
+ *
+ *  +------------------------------------------------------------------+
+ *  |                                                                  |
+ *  | 00:00      12:00       00:00       12:00       00:00       12:00 |
+ *  | SRV#1      TP#1        SRV#2       TP#2        SRV#3       TP#3  |
+ *  |                                                                  |
+ *  |  $==========|-----------$===========|----------$===========|     |
+ *  |                                                                  |
+ *  |                                                                  |
+ *  +------------------------------------------------------------------+
+ */
 static time_t
-helper_set_consensus_and_system_time(networkstatus_t *ns,
-                                     int between_srv_and_tp)
+helper_set_consensus_and_system_time(networkstatus_t *ns, int position)
 {
-  time_t real_time;
+  time_t real_time = 0;
 
   /* The period between SRV#N and TP#N is from 00:00 to 12:00 UTC. Consensus
    * valid_after is what matters here, the rest is just to specify the voting
    * period correctly. */
-  if (between_srv_and_tp) {
+  if (position == LATE_IN_SRV_TO_TP) {
     parse_rfc1123_time("Wed, 13 Apr 2016 11:00:00 UTC", &ns->valid_after);
     parse_rfc1123_time("Wed, 13 Apr 2016 12:00:00 UTC", &ns->fresh_until);
     parse_rfc1123_time("Wed, 13 Apr 2016 14:00:00 UTC", &ns->valid_until);
-  } else {
+  } else if (position == EARLY_IN_TP_TO_SRV) {
     parse_rfc1123_time("Wed, 13 Apr 2016 13:00:00 UTC", &ns->valid_after);
     parse_rfc1123_time("Wed, 13 Apr 2016 14:00:00 UTC", &ns->fresh_until);
     parse_rfc1123_time("Wed, 13 Apr 2016 16:00:00 UTC", &ns->valid_until);
+  } else if (position == LATE_IN_TP_TO_SRV) {
+    parse_rfc1123_time("Wed, 13 Apr 2016 23:00:00 UTC", &ns->valid_after);
+    parse_rfc1123_time("Wed, 14 Apr 2016 00:00:00 UTC", &ns->fresh_until);
+    parse_rfc1123_time("Wed, 14 Apr 2016 02:00:00 UTC", &ns->valid_until);
+  } else if (position == EARLY_IN_SRV_TO_TP) {
+    parse_rfc1123_time("Wed, 14 Apr 2016 01:00:00 UTC", &ns->valid_after);
+    parse_rfc1123_time("Wed, 14 Apr 2016 02:00:00 UTC", &ns->fresh_until);
+    parse_rfc1123_time("Wed, 14 Apr 2016 04:00:00 UTC", &ns->valid_until);
+  } else {
+    tt_assert(0);
   }
 
   /* Set system time: pretend to be just 2 minutes before consensus expiry */
   real_time = ns->valid_until - 120;
   update_approx_time(real_time);
+
+ done:
   return real_time;
 }
 
@@ -1485,8 +1556,7 @@ helper_set_consensus_and_system_time(networkstatus_t *ns,
  *  test_client_service_sync() */
 static void
 helper_test_hsdir_sync(networkstatus_t *ns,
-                       int service_between_srv_and_tp,
-                       int client_between_srv_and_tp,
+                       int service_position, int client_position,
                        int client_fetches_next_desc)
 {
   hs_service_descriptor_t *desc;
@@ -1503,8 +1573,7 @@ helper_test_hsdir_sync(networkstatus_t *ns,
    */
 
   /* 1) Initialize service time: consensus and real time */
-  time_t now = helper_set_consensus_and_system_time(ns,
-                                                   service_between_srv_and_tp);
+  time_t now = helper_set_consensus_and_system_time(ns, service_position);
   helper_initialize_big_hash_ring(ns);
 
   /* 2) Initialize service */
@@ -1519,7 +1588,7 @@ helper_test_hsdir_sync(networkstatus_t *ns,
   tt_int_op(smartlist_len(desc->previous_hsdirs), OP_EQ, 6);
 
   /* 3) Initialize client time */
-  now = helper_set_consensus_and_system_time(ns, client_between_srv_and_tp);
+  now = helper_set_consensus_and_system_time(ns, client_position);
 
   cleanup_nodelist();
   SMARTLIST_FOREACH(ns->routerstatus_list,
@@ -1527,22 +1596,28 @@ helper_test_hsdir_sync(networkstatus_t *ns,
   smartlist_clear(ns->routerstatus_list);
   helper_initialize_big_hash_ring(ns);
 
-  /* 4) Fetch desc as client */
-  char client_hsdir_b64_digest[BASE64_DIGEST_LEN+1] = {0};
-  helper_client_pick_hsdir(&service->keys.identity_pk,
-                          client_hsdir_b64_digest);
-  /* Cleanup right now so we don't memleak on error. */
-  cleanup_nodelist();
+  /* 4) Pick 6 HSDirs as a client and check that they were also chosen by the
+        service. */
+  for (int y = 0 ; y < 6 ; y++) {
+    char client_hsdir_b64_digest[BASE64_DIGEST_LEN+1] = {0};
+    helper_client_pick_hsdir(&service->keys.identity_pk,
+                             client_hsdir_b64_digest);
+
+    /* CHECK: Go through the hsdirs chosen by the service and make sure that it
+     * contains the one picked by the client! */
+    retval = smartlist_contains_string(desc->previous_hsdirs,
+                                       client_hsdir_b64_digest);
+    tt_int_op(retval, OP_EQ, 1);
+  }
 
-  /* CHECK: Go through the hsdirs chosen by the service and make sure that it
-   * contains the one picked by the client! */
-  retval = smartlist_contains_string(desc->previous_hsdirs,
-                                     client_hsdir_b64_digest);
-  tt_int_op(retval, OP_EQ, 1);
+  /* Finally, try to pick a 7th hsdir and see that NULL is returned since we
+   * exhausted all of them: */
+  tt_assert(!pick_hsdir_v3(&service->keys.identity_pk));
 
  done:
   /* At the end: free all services and initialize the subsystem again, we will
    * need it for next scenario. */
+  cleanup_nodelist();
   hs_service_free_all();
   hs_service_init();
   SMARTLIST_FOREACH(ns->routerstatus_list,
@@ -1606,7 +1681,7 @@ test_client_service_hsdir_set_sync(void *arg)
    *  |                                  S C                             |
    *  +------------------------------------------------------------------+
    */
-  helper_test_hsdir_sync(ns, 1, 1, 0);
+  helper_test_hsdir_sync(ns, LATE_IN_SRV_TO_TP, LATE_IN_SRV_TO_TP, 0);
 
   /*  b) Scenario where both client and service are in the time segment between
    *     TP#N and SRV#N+1. At this time the client fetches the second HS
@@ -1622,7 +1697,7 @@ test_client_service_hsdir_set_sync(void *arg)
    *  |                      S C                                         |
    *  +------------------------------------------------------------------+
    */
-  helper_test_hsdir_sync(ns, 0, 0, 1);
+  helper_test_hsdir_sync(ns, LATE_IN_TP_TO_SRV, LATE_IN_TP_TO_SRV, 1);
 
   /*  c) Scenario where service is between SRV#N and TP#N, but client is
    *     between TP#N and SRV#N+1. Client is forward in time so it fetches the
@@ -1638,7 +1713,7 @@ test_client_service_hsdir_set_sync(void *arg)
    *  |                                    S C                           |
    *  +------------------------------------------------------------------+
    */
-  helper_test_hsdir_sync(ns, 1, 0, 1);
+  helper_test_hsdir_sync(ns, LATE_IN_SRV_TO_TP, EARLY_IN_TP_TO_SRV, 1);
 
   /*  d) Scenario where service is between TP#N and SRV#N+1, but client is
    *     between SRV#N and TP#N. Client is backwards in time so it fetches the
@@ -1654,7 +1729,39 @@ test_client_service_hsdir_set_sync(void *arg)
    *  |                                    C S                           |
    *  +------------------------------------------------------------------+
    */
-  helper_test_hsdir_sync(ns, 0, 1, 0);
+  helper_test_hsdir_sync(ns, EARLY_IN_TP_TO_SRV, LATE_IN_SRV_TO_TP, 0);
+
+  /*  e) Scenario where service is between SRV#N and TP#N, but client is
+   *     between TP#N-1 and SRV#3. Client is backwards in time so it fetches
+   *     the first HS desc.
+   *
+   *  +------------------------------------------------------------------+
+   *  |                                                                  |
+   *  | 00:00      12:00       00:00       12:00       00:00       12:00 |
+   *  | SRV#1      TP#1        SRV#2       TP#2        SRV#3       TP#3  |
+   *  |                                                                  |
+   *  |  $==========|-----------$===========|-----------$===========|    |
+   *  |                        ^ ^                                       |
+   *  |                        C S                                       |
+   *  +------------------------------------------------------------------+
+   */
+  helper_test_hsdir_sync(ns, EARLY_IN_SRV_TO_TP, LATE_IN_TP_TO_SRV, 0);
+
+  /*  f) Scenario where service is between TP#N and SRV#N+1, but client is
+   *     between SRV#N+1 and TP#N+1. Client is forward in time so it fetches
+   *     the second HS desc.
+   *
+   *  +------------------------------------------------------------------+
+   *  |                                                                  |
+   *  | 00:00      12:00       00:00       12:00       00:00       12:00 |
+   *  | SRV#1      TP#1        SRV#2       TP#2        SRV#3       TP#3  |
+   *  |                                                                  |
+   *  |  $==========|-----------$===========|-----------$===========|    |
+   *  |                        ^ ^                                       |
+   *  |                        S C                                       |
+   *  +------------------------------------------------------------------+
+   */
+  helper_test_hsdir_sync(ns, LATE_IN_TP_TO_SRV, EARLY_IN_SRV_TO_TP, 1);
 
  done:
   networkstatus_vote_free(ns);
@@ -1687,6 +1794,9 @@ struct testcase_t hs_common_tests[] = {
     NULL, NULL },
   { "client_service_hsdir_set_sync", test_client_service_hsdir_set_sync,
     TT_FORK, NULL, NULL },
+  { "hs_indexes", test_hs_indexes, TT_FORK,
+    NULL, NULL },
+
   END_OF_TESTCASES
 };