prop224: Make client and service pick same HSDir

With the latest change on how we use the HSDir index, the client and service
need to pick their responsible HSDir differently that is depending on if they
are before or after a new time period.

The overlap mode is active function has been renamed for this and test added.

Signed-off-by: David Goulet <dgoulet@torproject.org>

src/or/hs_client.c

@@ -162,6 +162,7 @@ static routerstatus_t *
 pick_hsdir_v3(const ed25519_public_key_t *onion_identity_pk)
 {
   int retval;
+  unsigned int is_new_tp = 0;
   char base64_blinded_pubkey[ED25519_BASE64_LEN + 1];
   uint64_t current_time_period = hs_get_time_period_num(0);
   smartlist_t *responsible_hsdirs;
@@ -182,8 +183,9 @@ pick_hsdir_v3(const ed25519_public_key_t *onion_identity_pk)
   }
 
   /* Get responsible hsdirs of service for this time period */
-  hs_get_responsible_hsdirs(&blinded_pubkey, current_time_period, 0, 1,
+  is_new_tp = hs_time_between_tp_and_srv(NULL, time(NULL));
-                            responsible_hsdirs);
+  hs_get_responsible_hsdirs(&blinded_pubkey, current_time_period,
+                            is_new_tp, 1, responsible_hsdirs);
 
   log_debug(LD_REND, "Found %d responsible HSDirs and about to pick one.",
            smartlist_len(responsible_hsdirs));

src/or/hs_common.c

@@ -589,7 +589,7 @@ compute_disaster_srv(uint64_t time_period_num, uint8_t *srv_out)
  *  would have to do it thousands of times in a row, we always cache the
  *  computer disaster SRV (and its corresponding time period num) in case we
  *  want to reuse it soon after. We need to cache two SRVs, one for each active
- *  time period (in case of overlap mode).
+ *  time period.
  */
 static uint8_t cached_disaster_srv[2][DIGEST256_LEN];
 static uint64_t cached_time_period_nums[2] = {0};
@@ -1034,10 +1034,22 @@ hs_build_blinded_keypair(const ed25519_keypair_t *kp,
   memwipe(param, 0, sizeof(param));
 }
 
-/* Return true if overlap mode is active given the date in consensus. If
+/* Return true if we are currently in the time segment between a new time
- * consensus is NULL, then we use the latest live consensus we can find. */
+ * period and a new SRV (in the real network that happens between 12:00 and
+ * 00:00 UTC). Here is a diagram showing exactly when this returns true:
+ *
+ *    +------------------------------------------------------------------+
+ *    |                                                                  |
+ *    | 00:00      12:00       00:00       12:00       00:00       12:00 |
+ *    | SRV#1      TP#1        SRV#2       TP#2        SRV#3       TP#3  |
+ *    |                                                                  |
+ *    |  $==========|-----------$===========|-----------$===========|    |
+ *    |             ^^^^^^^^^^^^            ^^^^^^^^^^^^                 |
+ *    |                                                                  |
+ *    +------------------------------------------------------------------+
+ */
 MOCK_IMPL(int,
-hs_overlap_mode_is_active, (const networkstatus_t *consensus, time_t now))
+hs_time_between_tp_and_srv, (const networkstatus_t *consensus, time_t now))
 {
   time_t valid_after;
   time_t srv_start_time, tp_start_time;
@@ -1049,19 +1061,18 @@ hs_overlap_mode_is_active, (const networkstatus_t *consensus, time_t now))
     }
   }
 
-  /* We consider to be in overlap mode when we are in the period of time
+  /* Get start time of next TP and of current SRV protocol run, and check if we
-   * between a fresh SRV and the beginning of the new time period (in the
+   * are between them. */
-   * normal network this is between 00:00 (inclusive) and 12:00 UTC
-   * (exclusive)) */
   valid_after = consensus->valid_after;
-  srv_start_time =sr_state_get_start_time_of_current_protocol_run(valid_after);
+  srv_start_time =
+    sr_state_get_start_time_of_current_protocol_run(valid_after);
   tp_start_time = hs_get_start_time_of_next_time_period(srv_start_time);
 
   if (valid_after >= srv_start_time && valid_after < tp_start_time) {
-    return 1;
+    return 0;
   }
 
-  return 0;
+  return 1;
 }
 
 /* Return 1 if any virtual port in ports needs a circuit with good uptime.
@@ -1267,9 +1278,9 @@ node_has_hsdir_index(const node_t *node)
 /* For a given blinded key and time period number, get the responsible HSDir
  * and put their routerstatus_t object in the responsible_dirs list. If
  * is_next_period is true, the next hsdir_index of the node_t is used. If
- * is_client is true, the spread fetch consensus parameter is used else the
+ * 'for_fetching' is true, the spread fetch consensus parameter is used else
- * spread store is used which is only for upload. This function can't fail but
+ * the spread store is used which is only for upload. This function can't fail
- * it is possible that the responsible_dirs list contains fewer nodes than
+ * but it is possible that the responsible_dirs list contains fewer nodes than
  * expected.
  *
  * This function goes over the latest consensus routerstatus list and sorts it
@@ -1277,8 +1288,8 @@ node_has_hsdir_index(const node_t *node)
  * node. All of this makes it a bit CPU intensive so use it wisely. */
 void
 hs_get_responsible_hsdirs(const ed25519_public_key_t *blinded_pk,
-                          uint64_t time_period_num, int is_next_period,
+                          uint64_t time_period_num, int is_new_tp,
-                          int is_client, smartlist_t *responsible_dirs)
+                          int for_fetching, smartlist_t *responsible_dirs)
 {
   smartlist_t *sorted_nodes;
   /* The compare function used for the smartlist bsearch. We have two
@@ -1322,10 +1333,10 @@ hs_get_responsible_hsdirs(const ed25519_public_key_t *blinded_pk,
   /* First thing we have to do is sort all node_t by hsdir_index. The
    * is_next_period tells us if we want the current or the next one. Set the
    * bsearch compare function also while we are at it. */
-  if (is_client) {
+  if (for_fetching) {
     smartlist_sort(sorted_nodes, compare_node_fetch_hsdir_index);
     cmp_fct = compare_digest_to_fetch_hsdir_index;
-  } else if (is_next_period) {
+  } else if (is_new_tp) {
     smartlist_sort(sorted_nodes, compare_node_store_second_hsdir_index);
     cmp_fct = compare_digest_to_store_second_hsdir_index;
   } else {
@@ -1341,7 +1352,7 @@ hs_get_responsible_hsdirs(const ed25519_public_key_t *blinded_pk,
     uint8_t hs_index[DIGEST256_LEN] = {0};
     /* Number of node to add to the responsible dirs list depends on if we are
      * trying to fetch or store. A client always fetches. */
-    int n_to_add = (is_client) ? hs_get_hsdir_spread_fetch() :
+    int n_to_add = (for_fetching) ? hs_get_hsdir_spread_fetch() :
                                     hs_get_hsdir_spread_store();
 
     /* Get the index that we should use to select the node. */

src/or/hs_common.h

@@ -142,10 +142,12 @@ typedef struct rend_service_port_config_t {
 /* Hidden service directory index used in a node_t which is set once we set
  * the consensus. */
 typedef struct hsdir_index_t {
-  /* Index to use when fetching a descriptor. */
+  /* HSDir index to use when fetching a descriptor. */
   uint8_t fetch[DIGEST256_LEN];
 
-  /* Index to store the first and second descriptor. */
+  /* HSDir index used by services to store their first and second
+   * descriptor. The first descriptor is the one that uses older TP and SRV
+   * values than the second one. */
   uint8_t store_first[DIGEST256_LEN];
   uint8_t store_second[DIGEST256_LEN];
 } hsdir_index_t;
@@ -202,7 +204,7 @@ time_t hs_get_start_time_of_next_time_period(time_t now);
 
 link_specifier_t *hs_link_specifier_dup(const link_specifier_t *lspec);
 
-MOCK_DECL(int, hs_overlap_mode_is_active,
+MOCK_DECL(int, hs_time_between_tp_and_srv,
           (const networkstatus_t *consensus, time_t now));
 
 uint8_t *hs_get_current_srv(uint64_t time_period_num,
@@ -223,7 +225,7 @@ int32_t hs_get_hsdir_spread_store(void);
 
 void hs_get_responsible_hsdirs(const ed25519_public_key_t *blinded_pk,
                               uint64_t time_period_num, int is_next_period,
-                               int is_client, smartlist_t *responsible_dirs);
+                              int for_fetching, smartlist_t *responsible_dirs);
 routerstatus_t *hs_pick_hsdir(smartlist_t *responsible_dirs,
                               const char *req_key_str);
 

src/or/hs_service.c

@@ -2380,19 +2380,23 @@ set_descriptor_revision_counter(hs_descriptor_t *hs_desc)
  * if PublishHidServDescriptors is false. */
 STATIC void
 upload_descriptor_to_all(const hs_service_t *service,
-                         hs_service_descriptor_t *desc, int for_next_period)
+                         hs_service_descriptor_t *desc)
 {
+  unsigned int is_new_tp = 0;
   smartlist_t *responsible_dirs = NULL;
 
   tor_assert(service);
   tor_assert(desc);
 
+  /* Do we have a new TP that is are we between a new time period and the next
+   * SRV creation? */
+  is_new_tp = hs_time_between_tp_and_srv(NULL, approx_time());
   /* Get our list of responsible HSDir. */
   responsible_dirs = smartlist_new();
   /* The parameter 0 means that we aren't a client so tell the function to use
    * the spread store consensus paremeter. */
   hs_get_responsible_hsdirs(&desc->blinded_kp.pubkey, desc->time_period_num,
-                            for_next_period, 0, responsible_dirs);
+                            is_new_tp, 0, responsible_dirs);
 
   /** Clear list of previous hsdirs since we are about to upload to a new
    *  list. Let's keep it up to date. */
@@ -2539,8 +2543,6 @@ run_upload_descriptor_event(time_t now)
   /* Run v3+ check. */
   FOR_EACH_SERVICE_BEGIN(service) {
     FOR_EACH_DESCRIPTOR_BEGIN(service, desc) {
-      int for_next_period = 0;
-
       /* If we were asked to re-examine the hash ring, and it changed, then
          schedule an upload */
       if (consider_republishing_hs_descriptors &&
@@ -2566,12 +2568,7 @@ run_upload_descriptor_event(time_t now)
        * accurate because all circuits have been established. */
       build_desc_intro_points(service, desc, now);
 
-      /* The next descriptor needs the next time period for computing the
+      upload_descriptor_to_all(service, desc);
-       * corresponding hashring. */
-      if (desc == service->desc_next) {
-        for_next_period = 1;
-      }
-      upload_descriptor_to_all(service, desc, for_next_period);
     } FOR_EACH_DESCRIPTOR_END;
   } FOR_EACH_SERVICE_END;
 

src/or/hs_service.h

@@ -337,8 +337,7 @@ STATIC int
 write_address_to_file(const hs_service_t *service, const char *fname_);
 
 STATIC void upload_descriptor_to_all(const hs_service_t *service,
-                                     hs_service_descriptor_t *desc,
+                                     hs_service_descriptor_t *desc);
-                                     int for_next_period);
 
 STATIC void service_desc_schedule_upload(hs_service_descriptor_t *desc,
                                          time_t now,

src/or/nodelist.c

@@ -208,8 +208,7 @@ node_set_hsdir_index(node_t *node, const networkstatus_t *ns)
   /* We always use the current time period for fetching descs */
   fetch_tp = current_time_period_num;
 
-  /* Now extract the needed SRVs and time periods for building hsdir indices */
+  if (hs_time_between_tp_and_srv(ns, now)) {
-  if (!hs_overlap_mode_is_active(ns, now)) {
     fetch_srv = hs_get_current_srv(fetch_tp, ns);
 
     store_first_tp = hs_get_previous_time_period_num(0);

src/test/test_hs_common.c

@@ -465,7 +465,7 @@ test_desc_reupload_logic(void *arg)
   }
 
   /* Now let's upload our desc to all hsdirs */
-  upload_descriptor_to_all(service, desc, 0);
+  upload_descriptor_to_all(service, desc);
   /* Check that previous hsdirs were populated */
   tt_int_op(smartlist_len(desc->previous_hsdirs), OP_EQ, 6);
 
@@ -503,7 +503,7 @@ test_desc_reupload_logic(void *arg)
   tt_int_op(smartlist_len(desc->previous_hsdirs), OP_EQ, 6);
 
   /* Now order another upload and see that we keep having 6 prev hsdirs */
-  upload_descriptor_to_all(service, desc, 0);
+  upload_descriptor_to_all(service, desc);
   /* Check that previous hsdirs were populated */
   tt_int_op(smartlist_len(desc->previous_hsdirs), OP_EQ, 6);
 
@@ -536,7 +536,7 @@ test_desc_reupload_logic(void *arg)
   tt_int_op(smartlist_len(desc->previous_hsdirs), OP_EQ, 0);
 
   /* Now reupload again: see that the prev hsdir set got populated again. */
-  upload_descriptor_to_all(service, desc, 0);
+  upload_descriptor_to_all(service, desc);
   tt_int_op(smartlist_len(desc->previous_hsdirs), OP_EQ, 6);
 
  done:
@@ -740,6 +740,55 @@ test_parse_extended_hostname(void *arg)
  done: ;
 }
 
+static void
+test_time_between_tp_and_srv(void *arg)
+{
+  int ret;
+  networkstatus_t ns;
+  (void) arg;
+
+  /* This function should be returning true where "^" are:
+   *
+   *    +------------------------------------------------------------------+
+   *    |                                                                  |
+   *    | 00:00      12:00       00:00       12:00       00:00       12:00 |
+   *    | SRV#1      TP#1        SRV#2       TP#2        SRV#3       TP#3  |
+   *    |                                                                  |
+   *    |  $==========|-----------$===========|-----------$===========|    |
+   *    |             ^^^^^^^^^^^^            ^^^^^^^^^^^^                 |
+   *    |                                                                  |
+   *    +------------------------------------------------------------------+
+   */
+
+  ret = parse_rfc1123_time("Sat, 26 Oct 1985 00:00:00 UTC", &ns.valid_after);
+  tt_int_op(ret, OP_EQ, 0);
+  ret = hs_time_between_tp_and_srv(&ns, 0);
+  tt_int_op(ret, OP_EQ, 0);
+
+  ret = parse_rfc1123_time("Sat, 26 Oct 1985 11:00:00 UTC", &ns.valid_after);
+  tt_int_op(ret, OP_EQ, 0);
+  ret = hs_time_between_tp_and_srv(&ns, 0);
+  tt_int_op(ret, OP_EQ, 0);
+
+  ret = parse_rfc1123_time("Sat, 26 Oct 1985 12:00:00 UTC", &ns.valid_after);
+  tt_int_op(ret, OP_EQ, 0);
+  ret = hs_time_between_tp_and_srv(&ns, 0);
+  tt_int_op(ret, OP_EQ, 1);
+
+  ret = parse_rfc1123_time("Sat, 26 Oct 1985 23:00:00 UTC", &ns.valid_after);
+  tt_int_op(ret, OP_EQ, 0);
+  ret = hs_time_between_tp_and_srv(&ns, 0);
+  tt_int_op(ret, OP_EQ, 1);
+
+  ret = parse_rfc1123_time("Sat, 26 Oct 1985 00:00:00 UTC", &ns.valid_after);
+  tt_int_op(ret, OP_EQ, 0);
+  ret = hs_time_between_tp_and_srv(&ns, 0);
+  tt_int_op(ret, OP_EQ, 0);
+
+ done:
+  ;
+}
+
 struct testcase_t hs_common_tests[] = {
   { "build_address", test_build_address, TT_FORK,
     NULL, NULL },
@@ -759,6 +808,8 @@ struct testcase_t hs_common_tests[] = {
     NULL, NULL },
   { "parse_extended_hostname", test_parse_extended_hostname, TT_FORK,
     NULL, NULL },
+  { "time_between_tp_and_srv", test_time_between_tp_and_srv, TT_FORK,
+    NULL, NULL },
 
   END_OF_TESTCASES
 };