Merge branch 'maint-0.3.2'

changes/bug23762

@@ -0,0 +1,4 @@
+  o Minor bugfixes (hidden service v3):
+    - Properly retry HSv3 descriptor fetches in the case where we were initially
+      missing required directory information. Fixes bug 23762; bugfix on
+      0.3.2.1-alpha.

src/or/connection_edge.c

@@ -1571,10 +1571,10 @@ connection_ap_handle_onion(entry_connection_t *conn,
       int ret = hs_client_refetch_hsdesc(&edge_conn->hs_ident->identity_pk);
       switch (ret) {
       case HS_CLIENT_FETCH_MISSING_INFO:
-        /* By going to the end, the connection is put in waiting for a circuit
-         * state which means that it will be retried and consider as a pending
-         * connection. */
-        goto end;
+        /* Keeping the connection in descriptor wait state is fine because
+         * once we get enough dirinfo or a new live consensus, the HS client
+         * subsystem is notified and every connection in that state will
+         * trigger a fetch for the service key. */
       case HS_CLIENT_FETCH_LAUNCHED:
       case HS_CLIENT_FETCH_PENDING:
       case HS_CLIENT_FETCH_HAVE_DESC:
@@ -1591,7 +1591,6 @@ connection_ap_handle_onion(entry_connection_t *conn,
   /* We have the descriptor!  So launch a connection to the HS. */
   log_info(LD_REND, "Descriptor is here. Great.");
 
- end:
   base_conn->state = AP_CONN_STATE_CIRCUIT_WAIT;
   /* We'll try to attach it at the next event loop, or whenever
    * we call connection_ap_attach_pending() */

src/or/hs_client.c

@@ -233,6 +233,55 @@ close_all_socks_conns_waiting_for_desc(const ed25519_public_key_t *identity_pk,
   smartlist_free(conns);
 }
 
+/* Find all pending SOCKS connection waiting for a descriptor and retry them
+ * all. This is called when the directory information changed. */
+static void
+retry_all_socks_conn_waiting_for_desc(void)
+{
+  smartlist_t *conns =
+    connection_list_by_type_state(CONN_TYPE_AP, AP_CONN_STATE_RENDDESC_WAIT);
+
+  SMARTLIST_FOREACH_BEGIN(conns, connection_t *, base_conn) {
+    hs_client_fetch_status_t status;
+    const edge_connection_t *edge_conn =
+      ENTRY_TO_EDGE_CONN(TO_ENTRY_CONN(base_conn));
+
+    /* Ignore non HS or non v3 connection. */
+    if (edge_conn->hs_ident == NULL) {
+      continue;
+    }
+    /* In this loop, we will possibly try to fetch a descriptor for the
+     * pending connections because we just got more directory information.
+     * However, the refetch process can cleanup all SOCKS request so the same
+     * service if an internal error happens. Thus, we can end up with closed
+     * connections in our list. */
+    if (base_conn->marked_for_close) {
+      continue;
+    }
+
+    /* XXX: There is an optimization we could do which is that for a service
+     * key, we could check if we can fetch and remember that decision. */
+
+    /* Order a refetch in case it works this time. */
+    status = hs_client_refetch_hsdesc(&edge_conn->hs_ident->identity_pk);
+    if (BUG(status == HS_CLIENT_FETCH_HAVE_DESC)) {
+      /* This case is unique because it can NOT happen in theory. Once we get
+       * a new descriptor, the HS client subsystem is notified immediately and
+       * the connections waiting for it are handled which means the state will
+       * change from renddesc wait state. Log this and continue to next
+       * connection. */
+      continue;
+    }
+    /* In the case of an error, either all SOCKS connections have been
+     * closed or we are still missing directory information. Leave the
+     * connection in renddesc wait state so when we get more info, we'll be
+     * able to try it again. */
+  } SMARTLIST_FOREACH_END(base_conn);
+
+  /* We don't have ownership of those objects. */
+  smartlist_free(conns);
+}
+
 /* A v3 HS circuit successfully connected to the hidden service. Update the
  * stream state at <b>hs_conn_ident</b> appropriately. */
 static void
@@ -1029,6 +1078,73 @@ handle_rendezvous2(origin_circuit_t *circ, const uint8_t *payload,
   return ret;
 }
 
+/* Return true iff the client can fetch a descriptor for this service public
+ * identity key and status_out if not NULL is untouched. If the client can
+ * _not_ fetch the descriptor and if status_out is not NULL, it is set with
+ * the fetch status code. */
+static unsigned int
+can_client_refetch_desc(const ed25519_public_key_t *identity_pk,
+                        hs_client_fetch_status_t *status_out)
+{
+  hs_client_fetch_status_t status;
+
+  tor_assert(identity_pk);
+
+  /* Are we configured to fetch descriptors? */
+  if (!get_options()->FetchHidServDescriptors) {
+    log_warn(LD_REND, "We received an onion address for a hidden service "
+                      "descriptor but we are configured to not fetch.");
+    status = HS_CLIENT_FETCH_NOT_ALLOWED;
+    goto cannot;
+  }
+
+  /* Without a live consensus we can't do any client actions. It is needed to
+   * compute the hashring for a service. */
+  if (!networkstatus_get_live_consensus(approx_time())) {
+    log_info(LD_REND, "Can't fetch descriptor for service %s because we "
+                      "are missing a live consensus. Stalling connection.",
+             safe_str_client(ed25519_fmt(identity_pk)));
+    status = HS_CLIENT_FETCH_MISSING_INFO;
+    goto cannot;
+  }
+
+  if (!router_have_minimum_dir_info()) {
+    log_info(LD_REND, "Can't fetch descriptor for service %s because we "
+                      "dont have enough descriptors. Stalling connection.",
+             safe_str_client(ed25519_fmt(identity_pk)));
+    status = HS_CLIENT_FETCH_MISSING_INFO;
+    goto cannot;
+  }
+
+  /* Check if fetching a desc for this HS is useful to us right now */
+  {
+    const hs_descriptor_t *cached_desc = NULL;
+    cached_desc = hs_cache_lookup_as_client(identity_pk);
+    if (cached_desc && hs_client_any_intro_points_usable(identity_pk,
+                                                         cached_desc)) {
+      log_info(LD_GENERAL, "We would fetch a v3 hidden service descriptor "
+                           "but we already have a usable descriptor.");
+      status = HS_CLIENT_FETCH_HAVE_DESC;
+      goto cannot;
+    }
+  }
+
+  /* Don't try to refetch while we have a pending request for it. */
+  if (directory_request_is_pending(identity_pk)) {
+    log_info(LD_REND, "Already a pending directory request. Waiting on it.");
+    status = HS_CLIENT_FETCH_PENDING;
+    goto cannot;
+  }
+
+  /* Yes, client can fetch! */
+  return 1;
+ cannot:
+  if (status_out) {
+    *status_out = status;
+  }
+  return 0;
+}
+
 /* ========== */
 /* Public API */
 /* ========== */
@@ -1134,62 +1250,25 @@ hs_client_any_intro_points_usable(const ed25519_public_key_t *service_pk,
 int
 hs_client_refetch_hsdesc(const ed25519_public_key_t *identity_pk)
 {
-  int ret;
+  hs_client_fetch_status_t status;
 
   tor_assert(identity_pk);
 
-  /* Are we configured to fetch descriptors? */
-  if (!get_options()->FetchHidServDescriptors) {
-    log_warn(LD_REND, "We received an onion address for a hidden service "
-                      "descriptor but we are configured to not fetch.");
-    return HS_CLIENT_FETCH_NOT_ALLOWED;
-  }
-
-  /* Without a live consensus we can't do any client actions. It is needed to
-   * compute the hashring for a service. */
-  if (!networkstatus_get_live_consensus(approx_time())) {
-    log_info(LD_REND, "Can't fetch descriptor for service %s because we "
-                      "are missing a live consensus. Stalling connection.",
-               safe_str_client(ed25519_fmt(identity_pk)));
-    return HS_CLIENT_FETCH_MISSING_INFO;
-  }
-
-  if (!router_have_minimum_dir_info()) {
-    log_info(LD_REND, "Can't fetch descriptor for service %s because we "
-                      "dont have enough descriptors. Stalling connection.",
-               safe_str_client(ed25519_fmt(identity_pk)));
-    return HS_CLIENT_FETCH_MISSING_INFO;
+  if (!can_client_refetch_desc(identity_pk, &status)) {
+    return status;
   }
 
-  /* Check if fetching a desc for this HS is useful to us right now */
-  {
-    const hs_descriptor_t *cached_desc = NULL;
-    cached_desc = hs_cache_lookup_as_client(identity_pk);
-    if (cached_desc && hs_client_any_intro_points_usable(identity_pk,
-                                                         cached_desc)) {
-      log_info(LD_GENERAL, "We would fetch a v3 hidden service descriptor "
-                           "but we already have a usable descriptor.");
-      return HS_CLIENT_FETCH_HAVE_DESC;
-    }
-  }
-
-  /* Don't try to refetch while we have a pending request for it. */
-  if (directory_request_is_pending(identity_pk)) {
-    log_info(LD_REND, "Already a pending directory request. Waiting on it.");
-    return HS_CLIENT_FETCH_PENDING;
-  }
-
-  /* Try to fetch the desc and if we encounter an unrecoverable error, mark the
-   * desc as unavailable for now. */
-  ret = fetch_v3_desc(identity_pk);
-  if (fetch_status_should_close_socks(ret)) {
-    close_all_socks_conns_waiting_for_desc(identity_pk, ret,
+  /* Try to fetch the desc and if we encounter an unrecoverable error, mark
+   * the desc as unavailable for now. */
+  status = fetch_v3_desc(identity_pk);
+  if (fetch_status_should_close_socks(status)) {
+    close_all_socks_conns_waiting_for_desc(identity_pk, status,
                                            END_STREAM_REASON_RESOLVEFAILED);
     /* Remove HSDir fetch attempts so that we can retry later if the user
      * wants us to regardless of if we closed any connections. */
     purge_hid_serv_request(identity_pk);
   }
-  return ret;
+  return status;
 }
 
 /* This is called when we are trying to attach an AP connection to these
@@ -1499,3 +1578,13 @@ hs_client_purge_state(void)
   log_info(LD_REND, "Hidden service client state has been purged.");
 }
 
+/* Called when our directory information has changed. */
+void
+hs_client_dir_info_changed(void)
+{
+  /* We have possibly reached the minimum directory information or new
+   * consensus so retry all pending SOCKS connection in
+   * AP_CONN_STATE_RENDDESC_WAIT state in order to fetch the descriptor. */
+  retry_all_socks_conn_waiting_for_desc();
+}
+

src/or/hs_client.h

@@ -41,6 +41,7 @@ int hs_client_decode_descriptor(
 int hs_client_any_intro_points_usable(const ed25519_public_key_t *service_pk,
                                       const hs_descriptor_t *desc);
 int hs_client_refetch_hsdesc(const ed25519_public_key_t *identity_pk);
+void hs_client_dir_info_changed(void);
 
 int hs_client_send_introduce1(origin_circuit_t *intro_circ,
                               origin_circuit_t *rend_circ);

src/or/nodelist.c

@@ -48,6 +48,7 @@
 #include "entrynodes.h"
 #include "geoip.h"
 #include "hs_common.h"
+#include "hs_client.h"
 #include "main.h"
 #include "microdesc.h"
 #include "networkstatus.h"
@@ -1978,6 +1979,7 @@ router_dir_info_changed(void)
   need_to_update_have_min_dir_info = 1;
   rend_hsdir_routers_changed();
   hs_service_dir_info_changed();
+  hs_client_dir_info_changed();
 }
 
 /** Return a string describing what we're missing before we have enough

src/test/test_channelpadding.c

@@ -196,7 +196,8 @@ static void
 setup_mock_network(void)
 {
   routerstatus_t *relay;
-  connection_array = smartlist_new();
+  if (!connection_array)
+    connection_array = smartlist_new();
 
   relay1_relay2 = (channel_t*)new_fake_channeltls(2);
   relay1_relay2->write_cell = mock_channel_write_cell_relay1;
@@ -283,7 +284,8 @@ test_channelpadding_timers(void *arg)
 
   tor_libevent_postfork();
 
-  connection_array = smartlist_new();
+  if (!connection_array)
+    connection_array = smartlist_new();
 
   monotime_init();
   monotime_enable_test_mocking();
@@ -573,7 +575,8 @@ test_channelpadding_consensus(void *arg)
   monotime_coarse_set_mock_time_nsec(1);
   timers_initialize();
 
-  connection_array = smartlist_new();
+  if (!connection_array)
+    connection_array = smartlist_new();
   chan = (channel_t*)new_fake_channeltls(0);
   channel_timestamp_active(chan);
 
@@ -931,7 +934,8 @@ test_channelpadding_decide_to_pad_channel(void *arg)
    */
   channel_t *chan;
   int64_t new_time;
-  connection_array = smartlist_new();
+  if (!connection_array)
+    connection_array = smartlist_new();
   (void)arg;
 
   tor_libevent_postfork();

src/test/test_entryconn.c

@@ -790,8 +790,9 @@ test_entryconn_rewrite_onion_v3(void *arg)
   retval = connection_ap_handshake_rewrite_and_attach(conn, NULL, NULL);
   tt_int_op(retval, OP_EQ, 0);
 
-  /* Check connection state after rewrite */
-  tt_int_op(ENTRY_TO_CONN(conn)->state, OP_EQ, AP_CONN_STATE_CIRCUIT_WAIT);
+  /* Check connection state after rewrite. It should be in waiting for
+   * descriptor state. */
+  tt_int_op(ENTRY_TO_CONN(conn)->state, OP_EQ, AP_CONN_STATE_RENDDESC_WAIT);
   /* check that the address got rewritten */
   tt_str_op(conn->socks_request->address, OP_EQ,
             "25njqamcweflpvkl73j4szahhihoc4xt3ktcgjnpaingr5yhkenl5sid");