prop224: Improve descriptor reupload logic.

We want to reupload our descriptor if its set of responsible HSDirs
changed to minimize reachability issues.

This patch adds a callback everytime we get new dirinfo which checks if
the hash ring changed and reuploads descriptor if needed.

src/or/hs_descriptor.c

@@ -2357,10 +2357,10 @@ static int
  *
  * Return 0 on success and encoded_out is a valid pointer. On error, -1 is
  * returned and encoded_out is set to NULL. */
-int
+MOCK_IMPL(int,
-hs_desc_encode_descriptor(const hs_descriptor_t *desc,
+hs_desc_encode_descriptor,(const hs_descriptor_t *desc,
-                          const ed25519_keypair_t *signing_kp,
+                           const ed25519_keypair_t *signing_kp,
-                          char **encoded_out)
+                           char **encoded_out))
 {
   int ret = -1;
   uint32_t version;

src/or/hs_descriptor.h

@@ -211,9 +211,10 @@ hs_desc_link_specifier_t *hs_desc_link_specifier_new(
                                   const extend_info_t *info, uint8_t type);
 void hs_descriptor_clear_intro_points(hs_descriptor_t *desc);
 
-int hs_desc_encode_descriptor(const hs_descriptor_t *desc,
+MOCK_DECL(int,
-                              const ed25519_keypair_t *signing_kp,
+          hs_desc_encode_descriptor,(const hs_descriptor_t *desc,
-                              char **encoded_out);
+                                     const ed25519_keypair_t *signing_kp,
+                                     char **encoded_out));
 
 int hs_desc_decode_descriptor(const char *encoded,
                               const uint8_t *subcredential,

src/or/hs_service.c

@@ -972,6 +972,10 @@ service_descriptor_free(hs_service_descriptor_t *desc)
   /* Cleanup all intro points. */
   digest256map_free(desc->intro_points.map, service_intro_point_free_);
   digestmap_free(desc->intro_points.failed_id, tor_free_);
+  if (desc->previous_hsdirs) {
+    SMARTLIST_FOREACH(desc->previous_hsdirs, char *, s, tor_free(s));
+    smartlist_free(desc->previous_hsdirs);
+  }
   tor_free(desc);
 }
 
@@ -985,6 +989,7 @@ service_descriptor_new(void)
   sdesc->intro_points.map = digest256map_new();
   sdesc->intro_points.failed_id = digestmap_new();
   sdesc->hsdir_missing_info = smartlist_new();
+  sdesc->previous_hsdirs = smartlist_new();
   return sdesc;
 }
 
@@ -1511,6 +1516,52 @@ pick_needed_intro_points(hs_service_t *service,
   return i;
 }
 
+/** Clear previous cached HSDirs in <b>desc</b>. */
+static void
+service_desc_clear_previous_hsdirs(hs_service_descriptor_t *desc)
+{
+  if (BUG(!desc->previous_hsdirs)) {
+    return;
+  }
+
+  SMARTLIST_FOREACH(desc->previous_hsdirs, char*, s, tor_free(s));
+  smartlist_clear(desc->previous_hsdirs);
+}
+
+/** Note that we attempted to upload <b>desc</b> to <b>hsdir</b>. */
+static void
+service_desc_note_upload(hs_service_descriptor_t *desc, const node_t *hsdir)
+{
+  char b64_digest[BASE64_DIGEST_LEN+1] = {0};
+  digest_to_base64(b64_digest, hsdir->identity);
+
+  if (BUG(!desc->previous_hsdirs)) {
+    return;
+  }
+
+  if (!smartlist_contains_string(desc->previous_hsdirs, b64_digest)) {
+    smartlist_add_strdup(desc->previous_hsdirs, b64_digest);
+    smartlist_sort_strings(desc->previous_hsdirs);
+  }
+}
+
+/** Schedule an upload of <b>desc</b>. If <b>descriptor_changed</b> is set, it
+ *  means that this descriptor is dirty. */
+STATIC void
+service_desc_schedule_upload(hs_service_descriptor_t *desc,
+                             time_t now,
+                             int descriptor_changed)
+
+{
+  desc->next_upload_time = now;
+
+  /* If the descriptor changed, clean up the old HSDirs list. We want to
+   * re-upload no matter what. */
+  if (descriptor_changed) {
+    service_desc_clear_previous_hsdirs(desc);
+  }
+}
+
 /* Update the given descriptor from the given service. The possible update
  * actions includes:
  *    - Picking missing intro points if needed.
@@ -1543,7 +1594,7 @@ update_service_descriptor(hs_service_t *service,
       /* We'll build those introduction point into the descriptor once we have
        * confirmation that the circuits are opened and ready. However,
        * indicate that this descriptor should be uploaded from now on. */
-      desc->next_upload_time = now;
+      service_desc_schedule_upload(desc, now, 1);
     }
     /* Were we able to pick all the intro points we needed? If not, we'll
      * flag the descriptor that it's missing intro points because it
@@ -1972,6 +2023,9 @@ upload_descriptor_to_hsdir(const hs_service_t *service,
   directory_initiate_request(dir_req);
   directory_request_free(dir_req);
 
+  /* Add this node to previous_hsdirs list */
+  service_desc_note_upload(desc, hsdir);
+
   /* Logging so we know where it was sent. */
   {
     int is_next_desc = (service->desc_next == desc);
@@ -2189,7 +2243,7 @@ set_descriptor_revision_counter(hs_descriptor_t *hs_desc)
  * responsible hidden service directories. If for_next_period is true, the set
  * of directories are selected using the next hsdir_index. This does nothing
  * if PublishHidServDescriptors is false. */
-static void
+STATIC void
 upload_descriptor_to_all(const hs_service_t *service,
                          hs_service_descriptor_t *desc, int for_next_period)
 {
@@ -2629,10 +2683,88 @@ service_add_fnames_to_list(const hs_service_t *service, smartlist_t *list)
   smartlist_add(list, hs_path_from_filename(s_dir, fname));
 }
 
+/** The set of HSDirs have changed: check if the change affects our descriptor
+ *  HSDir placement, and if it does, reupload the desc. */
+static int
+service_desc_hsdirs_changed(const hs_service_t *service,
+                            const hs_service_descriptor_t *desc)
+{
+  int retval = 0;
+  smartlist_t *responsible_dirs = smartlist_new();
+  smartlist_t *b64_responsible_dirs = smartlist_new();
+
+  /* No desc upload has happened yet: it will happen eventually */
+  if (!desc->previous_hsdirs || !smartlist_len(desc->previous_hsdirs)) {
+    goto done;
+  }
+
+  /* Get list of responsible hsdirs */
+  hs_get_responsible_hsdirs(&desc->blinded_kp.pubkey, desc->time_period_num,
+                            service->desc_next == desc, 0, responsible_dirs);
+
+  /* Make a second list with their b64ed identity digests, so that we can
+   * compare it with out previous list of hsdirs */
+  SMARTLIST_FOREACH_BEGIN(responsible_dirs, const routerstatus_t *, hsdir_rs) {
+    char b64_digest[BASE64_DIGEST_LEN+1] = {0};
+    digest_to_base64(b64_digest, hsdir_rs->identity_digest);
+    smartlist_add_strdup(b64_responsible_dirs, b64_digest);
+  } SMARTLIST_FOREACH_END(hsdir_rs);
+
+  /* Sort this new smartlist so that we can compare it with the other one */
+  smartlist_sort_strings(b64_responsible_dirs);
+
+  /* Check whether the set of HSDirs changed */
+  if (!smartlist_strings_eq(b64_responsible_dirs, desc->previous_hsdirs)) {
+    log_warn(LD_GENERAL, "Received new dirinfo and set of hsdirs changed!");
+    retval = 1;
+  } else {
+    log_warn(LD_GENERAL, "No change in hsdir set!");
+  }
+
+ done:
+  smartlist_free(responsible_dirs);
+
+  SMARTLIST_FOREACH(b64_responsible_dirs, char*, s, tor_free(s));
+  smartlist_free(b64_responsible_dirs);
+
+  return retval;
+}
+
 /* ========== */
 /* Public API */
 /* ========== */
 
+/* We just received a new batch of descriptors which might affect the shape of
+ * the HSDir hash ring. Signal that we should re-upload our HS descriptors. */
+void
+hs_hsdir_set_changed_consider_reupload(void)
+{
+  time_t now = approx_time();
+
+  /* Check if HS subsystem is initialized */
+  if (!hs_service_map) {
+    return;
+  }
+
+  /* Basic test: If we have not bootstrapped 100% yet, no point in even trying
+     to upload descriptor. */
+  if (!router_have_minimum_dir_info()) {
+    return;
+  }
+
+  log_info(LD_GENERAL, "Received new descriptors. Set of HSdirs changed.");
+
+  /* Go over all descriptors and check if the set of HSDirs changed for any of
+   * them. Schedule reupload if so. */
+  FOR_EACH_SERVICE_BEGIN(service) {
+    FOR_EACH_DESCRIPTOR_BEGIN(service, desc) {
+      if (service_desc_hsdirs_changed(service, desc)) {
+        service_desc_schedule_upload(desc, now, 0);
+      }
+    } FOR_EACH_DESCRIPTOR_END;
+  } FOR_EACH_SERVICE_END;
+}
+
 /* Return the number of service we have configured and usable. */
 unsigned int
 hs_service_get_num_services(void)

src/or/hs_service.h

@@ -129,6 +129,12 @@ typedef struct hs_service_descriptor_t {
    * list are re-tried to upload this descriptor when our directory information
    * have been updated. */
   smartlist_t *hsdir_missing_info;
+
+  /** List of the responsible HSDirs (their b64ed identity digest) last time we
+   *  uploaded this descriptor. If the set of responsible HSDirs is different
+   *  from this list, this means we received new dirinfo and we need to
+   *  reupload our descriptor. This list is always sorted lexicographically. */
+  smartlist_t *previous_hsdirs;
 } hs_service_descriptor_t;
 
 /* Service key material. */
@@ -260,6 +266,7 @@ void hs_service_lists_fnames_for_sandbox(smartlist_t *file_list,
                                          smartlist_t *dir_list);
 int hs_service_set_conn_addr_port(const origin_circuit_t *circ,
                                   edge_connection_t *conn);
+void hs_hsdir_set_changed_consider_reupload(void);
 
 void hs_service_dir_info_changed(void);
 void hs_service_run_scheduled_events(time_t now);
@@ -338,6 +345,14 @@ check_state_line_for_service_rev_counter(const char *state_line,
 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,
+                                     int for_next_period);
+
+STATIC void service_desc_schedule_upload(hs_service_descriptor_t *desc,
+                                         time_t now,
+                                         int descriptor_changed);
+
 #endif /* TOR_UNIT_TESTS */
 
 #endif /* HS_SERVICE_PRIVATE */

src/or/nodelist.c

@@ -1789,6 +1789,7 @@ router_dir_info_changed(void)
 {
   need_to_update_have_min_dir_info = 1;
   rend_hsdir_routers_changed();
+  hs_hsdir_set_changed_consider_reupload();
 }
 
 /** Return a string describing what we're missing before we have enough

src/test/test_hs_common.c

@@ -18,7 +18,9 @@
 #include "hs_service.h"
 #include "config.h"
 #include "networkstatus.h"
+#include "directory.h"
 #include "nodelist.h"
+#include "statefile.h"
 
 /** Test the validation of HS v3 addresses */
 static void
@@ -486,6 +488,163 @@ test_responsible_hsdirs(void *arg)
   networkstatus_vote_free(mock_ns);
 }
 
+static void
+mock_directory_initiate_request(directory_request_t *req)
+{
+  (void)req;
+  return;
+}
+
+static int
+mock_hs_desc_encode_descriptor(const hs_descriptor_t *desc,
+                           const ed25519_keypair_t *signing_kp,
+                           char **encoded_out)
+{
+  (void)desc;
+  (void)signing_kp;
+
+  tor_asprintf(encoded_out, "lulu");
+  return 0;
+}
+
+static or_state_t dummy_state;
+
+/* Mock function to get fake or state (used for rev counters) */
+static or_state_t *
+get_or_state_replacement(void)
+{
+  return &dummy_state;
+}
+
+static int
+mock_router_have_minimum_dir_info(void)
+{
+  return 1;
+}
+
+/** Test that we correctly detect when the HSDir hash ring changes so that we
+ *  reupload our descriptor. */
+static void
+test_desc_reupload_logic(void *arg)
+{
+  networkstatus_t *ns = NULL;
+
+  (void) arg;
+
+  hs_init();
+
+  MOCK(router_have_minimum_dir_info,
+       mock_router_have_minimum_dir_info);
+  MOCK(get_or_state,
+       get_or_state_replacement);
+  MOCK(networkstatus_get_latest_consensus,
+       mock_networkstatus_get_latest_consensus);
+  MOCK(directory_initiate_request,
+       mock_directory_initiate_request);
+  MOCK(hs_desc_encode_descriptor,
+       mock_hs_desc_encode_descriptor);
+
+  ns = networkstatus_get_latest_consensus();
+
+  /** Test logic:
+   *  1) Upload descriptor to HSDirs
+   *     CHECK that previous_hsdirs list was populated.
+   *  2) Then call router_dir_info_changed() without an HSDir set change.
+   *     CHECK that no reuplod occurs.
+   *  3) Now change the HSDir set, and call dir_info_changed() again.
+   *     CHECK that reupload occurs.
+   *  4) Finally call service_desc_schedule_upload().
+   *     CHECK that previous_hsdirs list was cleared.
+   **/
+
+  /* Let's start by building our descriptor and service */
+  hs_service_descriptor_t *desc = service_descriptor_new();
+  hs_service_t *service = NULL;
+  char onion_addr[HS_SERVICE_ADDR_LEN_BASE32 + 1];
+  ed25519_public_key_t pubkey;
+  memset(&pubkey, '\x42', sizeof(pubkey));
+  hs_build_address(&pubkey, HS_VERSION_THREE, onion_addr);
+  service = tor_malloc_zero(sizeof(hs_service_t));
+  memcpy(service->onion_address, onion_addr, sizeof(service->onion_address));
+  ed25519_secret_key_generate(&service->keys.identity_sk, 0);
+  ed25519_public_key_generate(&service->keys.identity_pk,
+                              &service->keys.identity_sk);
+  service->desc_current = desc;
+  /* Also add service to service map */
+  hs_service_ht *service_map = get_hs_service_map();
+  tt_assert(service_map);
+  tt_int_op(hs_service_get_num_services(), OP_EQ, 0);
+  register_service(service_map, service);
+  tt_int_op(hs_service_get_num_services(), OP_EQ, 1);
+
+  /* Now let's create our hash ring: */
+  { /* First HSDir */
+    uint8_t identity[DIGEST_LEN];
+    uint8_t curr_hsdir_index[DIGEST256_LEN];
+    char nickname[] = "let_me";
+    memset(identity, 1, sizeof(identity));
+    memset(curr_hsdir_index, 1, sizeof(curr_hsdir_index));
+
+    helper_add_hsdir_to_networkstatus(ns, identity,
+                                      curr_hsdir_index, nickname, 1);
+  }
+
+  { /* Second HSDir */
+    uint8_t identity[DIGEST_LEN];
+    uint8_t curr_hsdir_index[DIGEST256_LEN];
+    char nickname[] = "show_you";
+    memset(identity, 2, sizeof(identity));
+    memset(curr_hsdir_index, 2, sizeof(curr_hsdir_index));
+
+    helper_add_hsdir_to_networkstatus(ns, identity,
+                                      curr_hsdir_index, nickname, 1);
+  }
+
+  /* Now let's upload our desc to all hsdirs */
+  upload_descriptor_to_all(service, desc, 0);
+  /* Check that previous hsdirs were populated */
+  tt_int_op(smartlist_len(desc->previous_hsdirs), OP_EQ, 2);
+
+  /* Poison next upload time so that we can see if it was changed by
+   * router_dir_info_changed(). No changes in hash ring so far, so the upload
+   * time should stay as is. */
+  desc->next_upload_time = 42;
+  router_dir_info_changed();
+  tt_int_op(desc->next_upload_time, OP_EQ, 42);
+
+  /* Now change the HSDir hash ring by adding another node */
+
+  { /* Third HSDir */
+    uint8_t identity[DIGEST_LEN];
+    uint8_t curr_hsdir_index[DIGEST256_LEN];
+    char nickname[] = "how_to_dance";
+    memset(identity, 3, sizeof(identity));
+    memset(curr_hsdir_index, 3, sizeof(curr_hsdir_index));
+
+    helper_add_hsdir_to_networkstatus(ns, identity,
+                                      curr_hsdir_index, nickname, 1);
+  }
+
+  /* Now call router_dir_info_changed() again and see that it detected the hash
+     ring change and updated the upload time */
+  time_t now = approx_time();
+  tt_assert(now);
+  router_dir_info_changed();
+  tt_int_op(desc->next_upload_time, OP_EQ, now);
+
+  /* Now pretend that the descriptor changed, and order a reupload to all
+     HSDirs. Make sure that the set of previous HSDirs was cleared. */
+  service_desc_schedule_upload(desc, now, 1);
+  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);
+  tt_int_op(smartlist_len(desc->previous_hsdirs), OP_EQ, 3);
+
+ done:
+  hs_free_all();
+}
+
 /** Test disaster SRV computation and caching */
 static void
 test_disaster_srv(void *arg)
@@ -550,7 +709,9 @@ struct testcase_t hs_common_tests[] = {
     NULL, NULL },
   { "desc_overlap_period_testnet", test_desc_overlap_period_testnet, TT_FORK,
     NULL, NULL },
-  { "desc_responsible_hsdirs", test_responsible_hsdirs, TT_FORK,
+  { "responsible_hsdirs", test_responsible_hsdirs, TT_FORK,
+    NULL, NULL },
+  { "desc_reupload_logic", test_desc_reupload_logic, TT_FORK,
     NULL, NULL },
   { "disaster_srv", test_disaster_srv, TT_FORK, NULL, NULL },