prop224: When computing hsdir index and time period, use valid_after time

Use the valid_after time from the consensus to get the time period number else
we might get out of sync with the overlap period that uses valid_after.

Make it an optional feature since some functions require passing a
specific time (like hs_get_start_time_of_next_time_period()).

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

5 files changed, 44 insertions, 12 deletions

diff --git a/src/or/hs_client.c b/src/or/hs_client.c
index 0e8fc6c24b..9f4dba04d4 100644
--- a/src/or/hs_client.c
+++ b/src/or/hs_client.c
@@ -29,6 +29,7 @@
 #include "connection.h"
 #include "hs_ntor.h"
 #include "circuitbuild.h"
+#include "networkstatus.h"
 
 /* Get all connections that are waiting on a circuit and flag them back to
  * waiting for a hidden service descriptor for the given service key
@@ -110,7 +111,7 @@ static int
 directory_launch_v3_desc_fetch(const ed25519_public_key_t *onion_identity_pk,
                                const routerstatus_t *hsdir)
 {
-  uint64_t current_time_period = hs_get_time_period_num(approx_time());
+  uint64_t current_time_period = hs_get_time_period_num(0);
   ed25519_public_key_t blinded_pubkey;
   char base64_blinded_pubkey[ED25519_BASE64_LEN + 1];
   hs_ident_dir_conn_t hs_conn_dir_ident;
@@ -162,7 +163,7 @@ pick_hsdir_v3(const ed25519_public_key_t *onion_identity_pk)
 {
   int retval;
   char base64_blinded_pubkey[ED25519_BASE64_LEN + 1];
-  uint64_t current_time_period = hs_get_time_period_num(approx_time());
+  uint64_t current_time_period = hs_get_time_period_num(0);
   smartlist_t *responsible_hsdirs;
   ed25519_public_key_t blinded_pubkey;
   routerstatus_t *hsdir_rs = NULL;
@@ -906,7 +907,7 @@ hs_client_decode_descriptor(const char *desc_str,
 
   /* Create subcredential for this HS so that we can decrypt */
   {
-    uint64_t current_time_period = hs_get_time_period_num(approx_time());
+    uint64_t current_time_period = hs_get_time_period_num(0);
     hs_build_blinded_pubkey(service_identity_pk, NULL, 0, current_time_period,
                             &blinded_pubkey);
     hs_get_subcredential(service_identity_pk, &blinded_pubkey, subcredential);
diff --git a/src/or/hs_common.c b/src/or/hs_common.c
index 5ea44b97e7..36f94507e7 100644
--- a/src/or/hs_common.c
+++ b/src/or/hs_common.c
@@ -211,15 +211,26 @@ get_time_period_length(void)
   return (uint64_t) time_period_length;
 }
 
-/** Get the HS time period number at time <b>now</b> */
+/** Get the HS time period number at time <b>now</b>. If <b>now</b> is not set,
+ *  we try to get the time ourselves. */
 uint64_t
 hs_get_time_period_num(time_t now)
 {
   uint64_t time_period_num;
+  time_t current_time;
+
+  /* If no time is specified, set current time based on consensus time, and
+   * only fall back to system time if that fails. */
+  if (now != 0) {
+    current_time = now;
+  } else {
+    networkstatus_t *ns = networkstatus_get_live_consensus(approx_time());
+    current_time = ns ? ns->valid_after : approx_time();
+  }
 
   /* Start by calculating minutes since the epoch */
   uint64_t time_period_length = get_time_period_length();
-  uint64_t minutes_since_epoch = now / 60;
+  uint64_t minutes_since_epoch = current_time / 60;
 
   /* Apply the rotation offset as specified by prop224 (section
    * [TIME-PERIODS]), so that new time periods synchronize nicely with SRV
diff --git a/src/or/hs_service.c b/src/or/hs_service.c
index 706890e34f..49dfa2c4fe 100644
--- a/src/or/hs_service.c
+++ b/src/or/hs_service.c
@@ -1376,7 +1376,7 @@ build_all_descriptors(time_t now)
       /* This means we just booted up because else this descriptor will never
        * be NULL as it should always point to the descriptor that was in
        * desc_next after rotation. */
-      build_service_descriptor(service, now, hs_get_time_period_num(now),
+      build_service_descriptor(service, now, hs_get_time_period_num(0),
                                &service->desc_current);
 
       log_info(LD_REND, "Hidden service %s current descriptor successfully "
@@ -1387,7 +1387,7 @@ build_all_descriptors(time_t now)
      * we are in the overlap period for the _next_ time period since it means
      * we either just booted or we just rotated our descriptors. */
     if (hs_overlap_mode_is_active(NULL, now) && service->desc_next == NULL) {
-      build_service_descriptor(service, now, hs_get_next_time_period_num(now),
+      build_service_descriptor(service, now, hs_get_next_time_period_num(0),
                                &service->desc_next);
       log_info(LD_REND, "Hidden service %s next descriptor successfully "
                         "built. Now scheduled for upload.",
diff --git a/src/or/nodelist.c b/src/or/nodelist.c
index 03b315e682..80f3b2b0ae 100644
--- a/src/or/nodelist.c
+++ b/src/or/nodelist.c
@@ -200,9 +200,11 @@ node_set_hsdir_index(node_t *node, const networkstatus_t *ns)
     goto done;
   }
 
-  /* Get the current and next time period number, we might use them both. */
-  current_time_period_num = hs_get_time_period_num(now);
-  next_time_period_num = hs_get_next_time_period_num(now);
+  /* Get the current and next time period number, we might use them both. We
+   * use the valid_after time of the consensus because we use that time to
+   * detect if we are in the overlap period or not. */
+  current_time_period_num = hs_get_time_period_num(0);
+  next_time_period_num = hs_get_next_time_period_num(0);
 
   if (hs_overlap_mode_is_active(ns, now)) {
     /* We are in overlap mode, this means that our consensus has just cycled
diff --git a/src/test/test_hs_service.c b/src/test/test_hs_service.c
index 4c7880c0ee..357fade049 100644
--- a/src/test/test_hs_service.c
+++ b/src/test/test_hs_service.c
@@ -1043,8 +1043,6 @@ test_build_update_descriptors(void *arg)
 {
   int ret;
   time_t now = time(NULL);
-  uint64_t period_num = hs_get_time_period_num(now);
-  uint64_t next_period_num = hs_get_next_time_period_num(now);
   node_t *node;
   hs_service_t *service;
   hs_service_intro_point_t *ip_cur, *ip_next;
@@ -1055,9 +1053,20 @@ test_build_update_descriptors(void *arg)
   MOCK(hs_overlap_mode_is_active, mock_hs_overlap_mode_is_active_true);
   MOCK(get_or_state,
        get_or_state_replacement);
+  MOCK(networkstatus_get_live_consensus,
+       mock_networkstatus_get_live_consensus);
 
   dummy_state = tor_malloc_zero(sizeof(or_state_t));
 
+  ret = parse_rfc1123_time("Sat, 26 Oct 1985 13:00:00 UTC",
+                           &mock_ns.valid_after);
+  ret = parse_rfc1123_time("Sat, 26 Oct 1985 14:00:00 UTC",
+                           &mock_ns.fresh_until);
+  tt_int_op(ret, OP_EQ, 0);
+
+  uint64_t period_num = hs_get_time_period_num(0);
+  uint64_t next_period_num = hs_get_next_time_period_num(0);
+
   /* Create a service without a current descriptor to trigger a build. */
   service = hs_service_new(get_options());
   tt_assert(service);
@@ -1203,8 +1212,17 @@ test_upload_descriptors(void *arg)
   MOCK(hs_overlap_mode_is_active, mock_hs_overlap_mode_is_active_true);
   MOCK(get_or_state,
        get_or_state_replacement);
+  MOCK(networkstatus_get_live_consensus,
+       mock_networkstatus_get_live_consensus);
+
   dummy_state = tor_malloc_zero(sizeof(or_state_t));
 
+  ret = parse_rfc1123_time("Sat, 26 Oct 1985 13:00:00 UTC",
+                           &mock_ns.valid_after);
+  ret = parse_rfc1123_time("Sat, 26 Oct 1985 14:00:00 UTC",
+                           &mock_ns.fresh_until);
+  tt_int_op(ret, OP_EQ, 0);
+
   /* Create a service with no descriptor. It's added to the global map. */
   service = hs_service_new(get_options());
   tt_assert(service);