hs-v3: Remove the circuit_established intro flag

Only use the HS circuit map to know if an introduction circuit is established
or not. No need for a flag to keep state of something we already have in the
circuit map. Furthermore, the circuit map gets cleaned up properly so it will
always have the "latest truth".

This commit also removes a unit test that was testing specifically that flag
but now we rely solely on the HS circuit map which is also tested few lines
below the removed test.

Fixes #32094

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

diff --git a/changes/ticket32094 b/changes/ticket32094
new file mode 100644 (file)
index 0000000..f6d0aba
--- /dev/null
+++ b/changes/ticket32094
@@ -0,0 +1,4 @@
+  o Minor bugfixes (hidden service v3):
+    - Do not rely on a "circuit established" flag for intro circuit but instead
+      always query the HS circuit map. This is to avoid sync issue with that
+      flag and the map. Fixes bug 32094; bugfix on 0.3.2.1-alpha.

diff --git a/src/feature/hs/hs_circuit.c b/src/feature/hs/hs_circuit.c
index 5e213b5abaf1aceb03399b42e7ebfb19e4d2154f..e1e9c7c790b70854e4b1b15459d96bccf76ea06c 100644 (file)
--- a/src/feature/hs/hs_circuit.c
+++ b/src/feature/hs/hs_circuit.c
@@ -637,6 +637,27 @@ hs_circ_service_get_intro_circ(const hs_service_intro_point_t *ip)
   }
 }
 
+/* Return an introduction point established circuit matching the given intro
+ * point object. The circuit purpose has to be CIRCUIT_PURPOSE_S_INTRO. NULL
+ * is returned is no such circuit can be found. */
+origin_circuit_t *
+hs_circ_service_get_established_intro_circ(const hs_service_intro_point_t *ip)
+{
+  origin_circuit_t *circ;
+
+  tor_assert(ip);
+
+  if (ip->base.is_only_legacy) {
+    circ = hs_circuitmap_get_intro_circ_v2_service_side(ip->legacy_key_digest);
+  } else {
+    circ = hs_circuitmap_get_intro_circ_v3_service_side(
+                                        &ip->auth_key_kp.pubkey);
+  }
+
+  /* Only return circuit if it is established. */
+  return (TO_CIRCUIT(circ)->purpose == CIRCUIT_PURPOSE_S_INTRO) ? circ : NULL;
+}
+
 /* Called when we fail building a rendezvous circuit at some point other than
  * the last hop: launches a new circuit to the same rendezvous point. This
  * supports legacy service.

diff --git a/src/feature/hs/hs_circuit.h b/src/feature/hs/hs_circuit.h
index e168b301f145b60e144b6a7f03fd39ce11847563..c817f3e37a777235c09defc93167a7613b8fbd24 100644 (file)
--- a/src/feature/hs/hs_circuit.h
+++ b/src/feature/hs/hs_circuit.h
@@ -35,6 +35,8 @@ void hs_circ_retry_service_rendezvous_point(origin_circuit_t *circ);
 
 origin_circuit_t *hs_circ_service_get_intro_circ(
                                       const hs_service_intro_point_t *ip);
+origin_circuit_t *hs_circ_service_get_established_intro_circ(
+                                      const hs_service_intro_point_t *ip);
 
 /* Cell API. */
 int hs_circ_handle_intro_established(const hs_service_t *service,

diff --git a/src/feature/hs/hs_service.c b/src/feature/hs/hs_service.c
index 15db5dd1dedbfc1007f4421d79e6763ae518ae4a..6587d57ec20656a88a7eeb8cbdf941dcd40563fd 100644 (file)
--- a/src/feature/hs/hs_service.c
+++ b/src/feature/hs/hs_service.c
@@ -709,7 +709,7 @@ count_desc_circuit_established(const hs_service_descriptor_t *desc)
 
   DIGEST256MAP_FOREACH(desc->intro_points.map, key,
                        const hs_service_intro_point_t *, ip) {
-    count += ip->circuit_established;
+    count += !!hs_circ_service_get_established_intro_circ(ip);
   } DIGEST256MAP_FOREACH_END;
 
   return count;
@@ -1660,7 +1660,7 @@ build_desc_intro_points(const hs_service_t *service,
 
   DIGEST256MAP_FOREACH(desc->intro_points.map, key,
                        const hs_service_intro_point_t *, ip) {
-    if (!ip->circuit_established) {
+    if (!hs_circ_service_get_established_intro_circ(ip)) {
       /* Ignore un-established intro points. They can linger in that list
        * because their circuit has not opened and they haven't been removed
        * yet even though we have enough intro circuits.
@@ -2370,10 +2370,6 @@ should_remove_intro_point(hs_service_intro_point_t *ip, time_t now)
    * remove it because it might simply be valid and opened at the previous
    * scheduled event for the last retry. */
 
-  /* Did we established already? */
-  if (ip->circuit_established) {
-    goto end;
-  }
   /* Do we simply have an existing circuit regardless of its state? */
   if (hs_circ_service_get_intro_circ(ip)) {
     goto end;
@@ -3326,11 +3322,6 @@ service_handle_intro_established(origin_circuit_t *circ,
     goto err;
   }
 
-  /* Flag that we have an established circuit for this intro point. This value
-   * is what indicates the upload scheduled event if we are ready to build the
-   * intro point into the descriptor and upload. */
-  ip->circuit_established = 1;
-
   log_info(LD_REND, "Successfully received an INTRO_ESTABLISHED cell "
                     "on circuit %u for service %s",
            TO_CIRCUIT(circ)->n_circ_id,
@@ -3725,10 +3716,6 @@ hs_service_intro_circ_has_closed(origin_circuit_t *circ)
   /* Can't have an intro point object without a descriptor. */
   tor_assert(desc);
 
-  /* Circuit disappeared so make sure the intro point is updated. By
-   * keeping the object in the descriptor, we'll be able to retry. */
-  ip->circuit_established = 0;
-
  end:
   return;
 }

diff --git a/src/feature/hs/hs_service.h b/src/feature/hs/hs_service.h
index 7d865f375477e7b94c489c6e42da6ec9675eed04..193e08546fe6b19b5c40b7e680ed46f146e76273 100644 (file)
--- a/src/feature/hs/hs_service.h
+++ b/src/feature/hs/hs_service.h
@@ -69,9 +69,6 @@ typedef struct hs_service_intro_point_t {
    * consensus. After MAX_INTRO_POINT_CIRCUIT_RETRIES, we give up on it. */
   uint32_t circuit_retries;
 
-  /** Set if this intro point has an established circuit. */
-  unsigned int circuit_established : 1;
-
   /** Replay cache recording the encrypted part of an INTRODUCE2 cell that the
    * circuit associated with this intro point has received. This is used to
    * prevent replay attacks. */

diff --git a/src/test/test_hs_service.c b/src/test/test_hs_service.c
index 66194cee3d0d782ea3e9e1d063a0071a55c44fc3..45c8cb9846b7328fb11823e8d3535cf910db4524 100644 (file)
--- a/src/test/test_hs_service.c
+++ b/src/test/test_hs_service.c
@@ -1013,7 +1013,6 @@ test_intro_established(void *arg)
   /* Send an empty payload. INTRO_ESTABLISHED cells are basically zeroes. */
   ret = hs_service_receive_intro_established(circ, payload, sizeof(payload));
   tt_int_op(ret, OP_EQ, 0);
-  tt_u64_op(ip->circuit_established, OP_EQ, 1);
   tt_int_op(TO_CIRCUIT(circ)->purpose, OP_EQ, CIRCUIT_PURPOSE_S_INTRO);
 
  done:
@@ -1296,18 +1295,11 @@ test_service_event(void *arg)
      * descriptor map so we can retry it. */
     ip = helper_create_service_ip();
     service_intro_point_add(service->desc_current->intro_points.map, ip);
-    ip->circuit_established = 1;  /* We'll test that, it MUST be 0 after. */
-    run_housekeeping_event(now);
-    tt_int_op(digest256map_size(service->desc_current->intro_points.map),
-              OP_EQ, 1);
-    /* No removal if we have an established circuit after retries. */
-    ip->circuit_retries = MAX_INTRO_POINT_CIRCUIT_RETRIES + 1;
     run_housekeeping_event(now);
     tt_int_op(digest256map_size(service->desc_current->intro_points.map),
               OP_EQ, 1);
     /* Remove the IP object at once for the next test. */
     ip->circuit_retries = MAX_INTRO_POINT_CIRCUIT_RETRIES + 1;
-    ip->circuit_established = 0;
     run_housekeeping_event(now);
     tt_int_op(digest256map_size(service->desc_current->intro_points.map),
               OP_EQ, 0);