Make repeated/rate limited HSFETCH queries fail with QUERY_RATE_LIMITED

6 files changed, 41 insertions, 9 deletions

diff --git a/changes/bug28269 b/changes/bug28269
new file mode 100644
index 0000000000..bdfe9e1aae
--- /dev/null
+++ b/changes/bug28269
@@ -0,0 +1,7 @@
+  o Minor bugfixes (onion services):
+    - If we are launching repeated HSFETCH queries and are rate-limited,
+      we introduce a new controller response QUERY_RATE_LIMITED instead
+      of QUERY_NO_HSDIR, while keeping the latter for when onion service
+      directories are missing a descriptor. Previously, we returned
+      QUERY_NO_HSDIR for both cases. Fixes bug 28269; bugfix on
+      0.3.1.1-alpha. Patch by Neel Chauhan
diff --git a/src/feature/hs/hs_client.c b/src/feature/hs/hs_client.c
index c34271efca..b4b9f0a948 100644
--- a/src/feature/hs/hs_client.c
+++ b/src/feature/hs/hs_client.c
@@ -434,7 +434,7 @@ pick_hsdir_v3(const ed25519_public_key_t *onion_identity_pk)
 
   /* Pick an HSDir from the responsible ones. The ownership of
    * responsible_hsdirs is given to this function so no need to free it. */
-  hsdir_rs = hs_pick_hsdir(responsible_hsdirs, base64_blinded_pubkey);
+  hsdir_rs = hs_pick_hsdir(responsible_hsdirs, base64_blinded_pubkey, NULL);
 
   return hsdir_rs;
 }
diff --git a/src/feature/hs/hs_common.c b/src/feature/hs/hs_common.c
index b2227432d2..cffec2b878 100644
--- a/src/feature/hs/hs_common.c
+++ b/src/feature/hs/hs_common.c
@@ -1589,20 +1589,25 @@ hs_purge_last_hid_serv_requests(void)
 /** Given the list of responsible HSDirs in <b>responsible_dirs</b>, pick the
  *  one that we should use to fetch a descriptor right now. Take into account
  *  previous failed attempts at fetching this descriptor from HSDirs using the
- *  string identifier <b>req_key_str</b>.
+ *  string identifier <b>req_key_str</b>. We return whether we are rate limited
+ *  into *<b>is_rate_limited</b> if it is not NULL.
  *
  *  Steals ownership of <b>responsible_dirs</b>.
  *
  *  Return the routerstatus of the chosen HSDir if successful, otherwise return
  *  NULL if no HSDirs are worth trying right now. */
 routerstatus_t *
-hs_pick_hsdir(smartlist_t *responsible_dirs, const char *req_key_str)
+hs_pick_hsdir(smartlist_t *responsible_dirs, const char *req_key_str,
+              int *is_rate_limited)
 {
   smartlist_t *usable_responsible_dirs = smartlist_new();
   const or_options_t *options = get_options();
   routerstatus_t *hs_dir;
   time_t now = time(NULL);
   int excluded_some;
+  int rate_limited;
+  int rate_limited_count = 0;
+  int responsible_dirs_count = smartlist_len(responsible_dirs);
 
   tor_assert(req_key_str);
 
@@ -1622,6 +1627,7 @@ hs_pick_hsdir(smartlist_t *responsible_dirs, const char *req_key_str)
     if (last + hs_hsdir_requery_period(options) >= now ||
         !node || !node_has_preferred_descriptor(node, 0)) {
       SMARTLIST_DEL_CURRENT(responsible_dirs, dir);
+      rate_limited_count++;
       continue;
     }
     if (!routerset_contains_node(options->ExcludeNodes, node)) {
@@ -1629,6 +1635,7 @@ hs_pick_hsdir(smartlist_t *responsible_dirs, const char *req_key_str)
     }
   } SMARTLIST_FOREACH_END(dir);
 
+  rate_limited = rate_limited_count == responsible_dirs_count;
   excluded_some =
     smartlist_len(usable_responsible_dirs) < smartlist_len(responsible_dirs);
 
@@ -1640,9 +1647,10 @@ hs_pick_hsdir(smartlist_t *responsible_dirs, const char *req_key_str)
   smartlist_free(responsible_dirs);
   smartlist_free(usable_responsible_dirs);
   if (!hs_dir) {
+    const char *warn_str = (rate_limited) ? "we are rate limited." :
+                              "we requested them all recently without success";
     log_info(LD_REND, "Could not pick one of the responsible hidden "
-                      "service directories, because we requested them all "
-                      "recently without success.");
+                      "service directories, because %s.", warn_str);
     if (options->StrictNodes && excluded_some) {
       log_warn(LD_REND, "Could not pick a hidden service directory for the "
                "requested hidden service: they are all either down or "
@@ -1654,6 +1662,10 @@ hs_pick_hsdir(smartlist_t *responsible_dirs, const char *req_key_str)
     hs_lookup_last_hid_serv_request(hs_dir, req_key_str, now, 1);
   }
 
+  if (is_rate_limited != NULL) {
+    *is_rate_limited = rate_limited;
+  }
+
   return hs_dir;
 }
 
diff --git a/src/feature/hs/hs_common.h b/src/feature/hs/hs_common.h
index abf39fa431..f96fc8beb7 100644
--- a/src/feature/hs/hs_common.h
+++ b/src/feature/hs/hs_common.h
@@ -241,7 +241,7 @@ void hs_get_responsible_hsdirs(const struct ed25519_public_key_t *blinded_pk,
                               int use_second_hsdir_index,
                               int for_fetching, smartlist_t *responsible_dirs);
 routerstatus_t *hs_pick_hsdir(smartlist_t *responsible_dirs,
-                              const char *req_key_str);
+                              const char *req_key_str, int *is_rate_limited);
 
 time_t hs_hsdir_requery_period(const or_options_t *options);
 time_t hs_lookup_last_hid_serv_request(routerstatus_t *hs_dir,
diff --git a/src/feature/rend/rendclient.c b/src/feature/rend/rendclient.c
index 5a8b234544..9863fc1c11 100644
--- a/src/feature/rend/rendclient.c
+++ b/src/feature/rend/rendclient.c
@@ -469,16 +469,19 @@ directory_get_from_hs_dir(const char *desc_id,
 
   /* Automatically pick an hs dir if none given. */
   if (!rs_hsdir) {
+    int rate_limited;
+
     /* Determine responsible dirs. Even if we can't get all we want, work with
      * the ones we have. If it's empty, we'll notice in hs_pick_hsdir(). */
     smartlist_t *responsible_dirs = smartlist_new();
     hid_serv_get_responsible_directories(responsible_dirs, desc_id);
 
-    hs_dir = hs_pick_hsdir(responsible_dirs, desc_id_base32);
+    hs_dir = hs_pick_hsdir(responsible_dirs, desc_id_base32, &rate_limited);
     if (!hs_dir) {
       /* No suitable hs dir can be found, stop right now. */
-      control_event_hsv2_descriptor_failed(rend_query, NULL,
-                                           "QUERY_NO_HSDIR");
+      const char *query_response = (rate_limited) ? "QUERY_RATE_LIMITED" :
+                                                    "QUERY_NO_HSDIR";
+      control_event_hsv2_descriptor_failed(rend_query, NULL, query_response);
       control_event_hs_descriptor_content(rend_data_get_address(rend_query),
                                           desc_id_base32, NULL, NULL);
       return 0;
diff --git a/src/test/test_hs.c b/src/test/test_hs.c
index aeb3387471..5d3327c777 100644
--- a/src/test/test_hs.c
+++ b/src/test/test_hs.c
@@ -323,6 +323,16 @@ test_hs_desc_event(void *arg)
   tt_str_op(received_msg,OP_EQ, expected_msg);
   tor_free(received_msg);
 
+  /* test HSDir rate limited */
+  rend_query.auth_type = REND_NO_AUTH;
+  control_event_hsv2_descriptor_failed(&rend_query.base_, NULL,
+                                     "QUERY_RATE_LIMITED");
+  expected_msg = "650 HS_DESC FAILED "STR_HS_ADDR" NO_AUTH " \
+                 "UNKNOWN REASON=QUERY_RATE_LIMITED\r\n";
+  tt_assert(received_msg);
+  tt_str_op(received_msg,OP_EQ, expected_msg);
+  tor_free(received_msg);
+
   /* Test invalid content with no HSDir fingerprint. */
   char *exp_msg;
   control_event_hs_descriptor_content(rend_query.onion_address,