Refactor rend_cache_lookup_entry() and how it's used

Here is why:

1) v0 descriptors are deprecated since 0.2.2.1 and not suppose to be alive
in the network anymore. This function should only serve v2 version for now
as the default.

2) It should return different error code depending on what's the actual
error is. Right now, there is no distinction between a cache entry not found
and an invalid query.

3) This function should NOT test if the intro points are usable or not. This
adds some load on a function that should be "O(1)" and do one job.
Furthermore, multiple callsites actually already test that doing twice the
job...

4) While adding control event, it would be useful to be able to lookup a
cache entry without having it checking the intro points. There are also
places in the code that do want to lookup the cache entry without doing
that.

Fixes #14391

Signed-off-by: David Goulet <dgoulet@ev0ke.net>

src/or/connection_edge.c

@@ -1502,23 +1502,45 @@ connection_ap_handshake_rewrite_and_attach(entry_connection_t *conn,
     log_info(LD_REND,"Got a hidden service request for ID '%s'",
              safe_str_client(rend_data->onion_address));
 
-    /* see if we already have a hidden service descriptor cached for this
-     * address. */
+    /* Lookup the given onion address. If invalid, stop right now else we
+     * might have it in the cache or not, it will be tested later on. */
+    unsigned int refetch_desc = 0;
     rend_cache_entry_t *entry = NULL;
     const int rend_cache_lookup_result =
       rend_cache_lookup_entry(rend_data->onion_address, -1, &entry);
     if (rend_cache_lookup_result < 0) {
-      /* We should already have rejected this address! */
-      log_warn(LD_BUG,"Invalid service name '%s'",
-               safe_str_client(rend_data->onion_address));
-      connection_mark_unattached_ap(conn, END_STREAM_REASON_TORPROTOCOL);
-      return -1;
+      switch (-rend_cache_lookup_result) {
+      case EINVAL:
+        /* We should already have rejected this address! */
+        log_warn(LD_BUG,"Invalid service name '%s'",
+            safe_str_client(rend_data->onion_address));
+        connection_mark_unattached_ap(conn, END_STREAM_REASON_TORPROTOCOL);
+        return -1;
+      case ENOENT:
+        refetch_desc = 1;
+        break;
+      default:
+        log_warn(LD_BUG, "Unknown cache lookup error %d",
+            rend_cache_lookup_result);
+        return -1;
+      }
     }
 
     /* Help predict this next time. We're not sure if it will need
      * a stable circuit yet, but we know we'll need *something*. */
     rep_hist_note_used_internal(now, 0, 1);
 
+    /* Now we have a descriptor but is it usable or not? If not, refetch.
+     * Also, a fetch could have been requested if the onion address was not
+     * found in the cache previously. */
+    if (refetch_desc || !rend_client_any_intro_points_usable(entry)) {
+      base_conn->state = AP_CONN_STATE_RENDDESC_WAIT;
+      log_info(LD_REND, "Unknown descriptor %s. Fetching.",
+          safe_str_client(rend_data->onion_address));
+      rend_client_refetch_v2_renddesc(rend_data);
+      return 0;
+    }
+
     /* Look up if we have client authorization configured for this hidden
      * service.  If we do, associate it with the rend_data. */
     rend_service_authorization_t *client_auth =
@@ -1532,22 +1554,13 @@ connection_ap_handshake_rewrite_and_attach(entry_connection_t *conn,
       rend_data->auth_type = client_auth->auth_type;
     }
 
-    /* Now, we either launch an attempt to connect to the hidden service,
-     * or we launch an attempt to look up its descriptor, depending on
-     * whether we had the descriptor. */
-    if (rend_cache_lookup_result == 0) {
-      base_conn->state = AP_CONN_STATE_RENDDESC_WAIT;
-      log_info(LD_REND, "Unknown descriptor %s. Fetching.",
-               safe_str_client(rend_data->onion_address));
-      rend_client_refetch_v2_renddesc(rend_data);
-    } else { /* rend_cache_lookup_result > 0 */
-      base_conn->state = AP_CONN_STATE_CIRCUIT_WAIT;
-      log_info(LD_REND, "Descriptor is here. Great.");
-      if (connection_ap_handshake_attach_circuit(conn) < 0) {
-        if (!base_conn->marked_for_close)
-          connection_mark_unattached_ap(conn, END_STREAM_REASON_CANT_ATTACH);
-        return -1;
-      }
+    /* We have the descriptor so launch a connection to the HS. */
+    base_conn->state = AP_CONN_STATE_CIRCUIT_WAIT;
+    log_info(LD_REND, "Descriptor is here. Great.");
+    if (connection_ap_handshake_attach_circuit(conn) < 0) {
+      if (!base_conn->marked_for_close)
+        connection_mark_unattached_ap(conn, END_STREAM_REASON_CANT_ATTACH);
+      return -1;
     }
     return 0;
   }

src/or/rendclient.c

@@ -141,7 +141,7 @@ rend_client_send_introduction(origin_circuit_t *introcirc,
   int r, v3_shift = 0;
   char payload[RELAY_PAYLOAD_SIZE];
   char tmp[RELAY_PAYLOAD_SIZE];
-  rend_cache_entry_t *entry;
+  rend_cache_entry_t *entry = NULL;
   crypt_path_t *cpath;
   off_t dh_offset;
   crypto_pk_t *intro_key = NULL;
@@ -158,8 +158,13 @@ rend_client_send_introduction(origin_circuit_t *introcirc,
   tor_assert(!(rendcirc->build_state->onehop_tunnel));
 #endif
 
-  if (rend_cache_lookup_entry(introcirc->rend_data->onion_address, -1,
-                              &entry) < 1) {
+  r = rend_cache_lookup_entry(introcirc->rend_data->onion_address, -1,
+                              &entry);
+  /* An invalid onion address is not possible else we have a big issue. */
+  tor_assert(r != -EINVAL);
+  if (r < 0 || !rend_client_any_intro_points_usable(entry)) {
+    /* If the descriptor is not found or the intro points are not usable
+     * anymore, trigger a fetch. */
     log_info(LD_REND,
              "query %s didn't have valid rend desc in cache. "
              "Refetching descriptor.",
@@ -734,7 +739,7 @@ rend_client_refetch_v2_renddesc(const rend_data_t *rend_query)
     return;
   }
   /* Before fetching, check if we already have a usable descriptor here. */
-  if (rend_cache_lookup_entry(rend_query->onion_address, -1, &e) > 0 &&
+  if (rend_cache_lookup_entry(rend_query->onion_address, -1, &e) == 0 &&
       rend_client_any_intro_points_usable(e)) {
     log_info(LD_REND, "We would fetch a v2 rendezvous descriptor, but we "
                       "already have a usable descriptor here. Not fetching.");
@@ -848,17 +853,26 @@ rend_client_report_intro_point_failure(extend_info_t *failed_intro,
   connection_t *conn;
 
   r = rend_cache_lookup_entry(rend_query->onion_address, -1, &ent);
-  if (r<0) {
-    log_warn(LD_BUG, "Malformed service ID %s.",
-             escaped_safe_str_client(rend_query->onion_address));
-    return -1;
-  }
-  if (r==0) {
-    log_info(LD_REND, "Unknown service %s. Re-fetching descriptor.",
-             escaped_safe_str_client(rend_query->onion_address));
-    rend_client_refetch_v2_renddesc(rend_query);
-    return 0;
+  if (r < 0) {
+    /* Either invalid onion address or cache entry not found. */
+    switch (-r) {
+    case EINVAL:
+      log_warn(LD_BUG, "Malformed service ID %s.",
+          escaped_safe_str_client(rend_query->onion_address));
+      return -1;
+    case ENOENT:
+      log_info(LD_REND, "Unknown service %s. Re-fetching descriptor.",
+          escaped_safe_str_client(rend_query->onion_address));
+      rend_client_refetch_v2_renddesc(rend_query);
+      return 0;
+    default:
+      log_warn(LD_BUG, "Unknown cache lookup returned code: %d", r);
+      return -1;
+    }
   }
+  /* The intro points are not checked here if they are usable or not because
+   * this is called when an intro point circuit is closed thus there must be
+   * at least one intro point that is usable and is about to be flagged. */
 
   for (i = 0; i < smartlist_len(ent->parsed->intro_nodes); i++) {
     rend_intro_point_t *intro = smartlist_get(ent->parsed->intro_nodes, i);
@@ -1057,7 +1071,7 @@ rend_client_desc_trynow(const char *query)
       continue;
     assert_connection_ok(base_conn, now);
     if (rend_cache_lookup_entry(rend_data->onion_address, -1,
-                                &entry) == 1 &&
+                                &entry) == 0 &&
         rend_client_any_intro_points_usable(entry)) {
       /* either this fetch worked, or it failed but there was a
        * valid entry from before which we should reuse */
@@ -1121,13 +1135,17 @@ rend_client_note_connection_attempt_ended(const char *onion_address)
 extend_info_t *
 rend_client_get_random_intro(const rend_data_t *rend_query)
 {
+  int ret;
   extend_info_t *result;
   rend_cache_entry_t *entry;
 
-  if (rend_cache_lookup_entry(rend_query->onion_address, -1, &entry) < 1) {
-      log_warn(LD_REND,
-               "Query '%s' didn't have valid rend desc in cache. Failing.",
-               safe_str_client(rend_query->onion_address));
+  ret = rend_cache_lookup_entry(rend_query->onion_address, -1, &entry);
+  if (ret < 0 || !rend_client_any_intro_points_usable(entry)) {
+    log_warn(LD_REND,
+             "Query '%s' didn't have valid rend desc in cache. Failing.",
+             safe_str_client(rend_query->onion_address));
+    /* XXX: Should we refetch the descriptor here if the IPs are not usable
+     * anymore ?. */
     return NULL;
   }
 

src/or/rendcommon.c

@@ -920,36 +920,52 @@ rend_valid_service_id(const char *query)
   return 1;
 }
 
-/** If we have a cached rend_cache_entry_t for the service ID <b>query</b>
- * with <b>version</b>, set *<b>e</b> to that entry and return 1.
- * Else return 0. If <b>version</b> is nonnegative, only return an entry
- * in that descriptor format version. Otherwise (if <b>version</b> is
- * negative), return the most recent format we have.
- */
+/** Lookup in the client cache the given service ID <b>query</b> for
+ * <b>version</b>.
+ *
+ * Return 0 if found and if <b>e</b> is non NULL, set it with the entry
+ * found. Else, a negative value is returned and <b>e</b> is untouched.
+ * -EINVAL means that <b>query</b> is not a valid service id.
+ * -ENOENT means that no entry in the cache was found. */
 int
 rend_cache_lookup_entry(const char *query, int version, rend_cache_entry_t **e)
 {
-  char key[REND_SERVICE_ID_LEN_BASE32+2]; /* <version><query>\0 */
+  int ret = 0;
+  char key[REND_SERVICE_ID_LEN_BASE32 + 2]; /* <version><query>\0 */
+  rend_cache_entry_t *entry = NULL;
+  static const int default_version = 2;
+
   tor_assert(rend_cache);
-  if (!rend_valid_service_id(query))
-    return -1;
-  *e = NULL;
-  if (version != 0) {
-    tor_snprintf(key, sizeof(key), "2%s", query);
-    *e = strmap_get_lc(rend_cache, key);
+  tor_assert(query);
+
+  if (!rend_valid_service_id(query)) {
+    ret = -EINVAL;
+    goto end;
   }
-  if (!*e && version != 2) {
-    tor_snprintf(key, sizeof(key), "0%s", query);
-    *e = strmap_get_lc(rend_cache, key);
+
+  switch (version) {
+  case 0:
+    log_warn(LD_REND, "Cache lookup of a v0 renddesc is deprecated.");
+    break;
+  case 2:
+    /* Default is version 2. */
+  default:
+    tor_snprintf(key, sizeof(key), "%d%s", default_version, query);
+    entry = strmap_get_lc(rend_cache, key);
+    break;
   }
-  if (!*e)
-    return 0;
-  tor_assert((*e)->parsed && (*e)->parsed->intro_nodes);
-  /* XXX023 hack for now, to return "not found" if there are no intro
-   * points remaining. See bug 997. */
-  if (! rend_client_any_intro_points_usable(*e))
-    return 0;
-  return 1;
+  if (!entry) {
+    ret = -ENOENT;
+    goto end;
+  }
+  tor_assert(entry->parsed && entry->parsed->intro_nodes);
+
+  if (e) {
+    *e = entry;
+  }
+
+end:
+  return ret;
 }
 
 /** Lookup the v2 service descriptor with base32-encoded <b>desc_id</b> and