Merge remote-tracking branch 'tor-gitlab/mr/148' into master

10 files changed, 83 insertions, 11 deletions

diff --git a/changes/ticket25061 b/changes/ticket25061
new file mode 100644
index 0000000000..9ab0e660bb
--- /dev/null
+++ b/changes/ticket25061
@@ -0,0 +1,6 @@
+  o Minor features (bootstrap reporting):
+    - When reporting bootstrapping status on a relay, do not consider
+      connections that have never been the target of an origin circuit.
+      Previously, all connection failures were treated as potential
+      bootstrapping failures, including those that had been opened because of
+      client requests. Closes ticket 25061.
diff --git a/src/core/or/channel.c b/src/core/or/channel.c
index d082174dc8..c163f53488 100644
--- a/src/core/or/channel.c
+++ b/src/core/or/channel.c
@@ -2395,12 +2395,16 @@ channel_is_better(channel_t *a, channel_t *b)
  * *msg_out to a message describing the channel's state and our next action,
  * and set *launch_out to a boolean indicated whether the caller should try to
  * launch a new channel with channel_connect().
+ *
+ * If `for_origin_circ` is set, mark the channel as interesting for origin
+ * circuits, and therefore interesting for our bootstrapping reports.
  */
 MOCK_IMPL(channel_t *,
 channel_get_for_extend,(const char *rsa_id_digest,
                         const ed25519_public_key_t *ed_id,
                         const tor_addr_t *target_ipv4_addr,
                         const tor_addr_t *target_ipv6_addr,
+                        bool for_origin_circ,
                         const char **msg_out,
                         int *launch_out))
 {
@@ -2440,8 +2444,15 @@ channel_get_for_extend,(const char *rsa_id_digest,
     if (!CHANNEL_IS_OPEN(chan)) {
       /* If the address matches, don't launch a new connection for this
        * circuit. */
-      if (matches_target)
+      if (matches_target) {
         ++n_inprogress_goodaddr;
+        if (for_origin_circ) {
+          /* We were looking for a connection for an origin circuit; this one
+           * matches, so we'll note that we decided to use it for an origin
+           * circuit. */
+          channel_mark_as_used_for_origin_circuit(chan);
+        }
+      }
       continue;
     }
 
diff --git a/src/core/or/channel.h b/src/core/or/channel.h
index 606b0730b8..206d0fdc97 100644
--- a/src/core/or/channel.h
+++ b/src/core/or/channel.h
@@ -526,6 +526,7 @@ void channel_mark_for_close(channel_t *chan);
 int channel_write_packed_cell(channel_t *chan, packed_cell_t *cell);
 
 void channel_listener_mark_for_close(channel_listener_t *chan_l);
+void channel_mark_as_used_for_origin_circuit(channel_t *chan);
 
 /* Channel callback registrations */
 
@@ -661,6 +662,7 @@ MOCK_DECL(channel_t *, channel_get_for_extend,(
                                    const struct ed25519_public_key_t *ed_id,
                                    const tor_addr_t *target_ipv4_addr,
                                    const tor_addr_t *target_ipv6_addr,
+                                   bool for_origin_circ,
                                    const char **msg_out,
                                    int *launch_out));
 
diff --git a/src/core/or/channeltls.c b/src/core/or/channeltls.c
index a0debf8d22..32723fed1e 100644
--- a/src/core/or/channeltls.c
+++ b/src/core/or/channeltls.c
@@ -360,6 +360,31 @@ channel_tls_handle_incoming(or_connection_t *orconn)
   return chan;
 }
 
+/**
+ * Set the `potentially_used_for_bootstrapping` flag on the or_connection_t
+ * corresponding to the provided channel.
+ *
+ * This flag indicates that if the connection fails, it might be interesting
+ * to the bootstrapping subsystem.  (The bootstrapping system only cares about
+ * channels that we have tried to use for our own circuits.  Other channels
+ * may have been launched in response to EXTEND cells from somebody else, and
+ * if they fail, it won't necessarily indicate a bootstrapping problem.)
+ **/
+void
+channel_mark_as_used_for_origin_circuit(channel_t *chan)
+{
+  if (BUG(!chan))
+    return;
+  if (chan->magic != TLS_CHAN_MAGIC)
+    return;
+  channel_tls_t *tlschan = channel_tls_from_base(chan);
+  if (BUG(!tlschan))
+    return;
+
+  if (tlschan->conn)
+    tlschan->conn->potentially_used_for_bootstrapping = 1;
+}
+
 /*********
  * Casts *
  ********/
diff --git a/src/core/or/circuitbuild.c b/src/core/or/circuitbuild.c
index 3df0f9be8f..a3a7a8cf58 100644
--- a/src/core/or/circuitbuild.c
+++ b/src/core/or/circuitbuild.c
@@ -574,6 +574,7 @@ circuit_handle_first_hop(origin_circuit_t *circ)
                           &firsthop->extend_info->ed_identity,
                           orport4 ? &orport4->addr : NULL,
                           orport6 ? &orport6->addr : NULL,
+                          true,
                           &msg,
                           &should_launch);
 
@@ -590,6 +591,11 @@ circuit_handle_first_hop(origin_circuit_t *circ)
         log_info(LD_CIRC,"connect to firsthop failed. Closing.");
         return -END_CIRC_REASON_CONNECTFAILED;
       }
+      /* We didn't find a channel, but we're launching one for an origin
+       * circuit.  (If we decided not to launch a channel, then we found at
+       * least one once good in-progress channel use for this circuit, and
+       * marked it in channel_get_for_extend().) */
+      channel_mark_as_used_for_origin_circuit(n_chan);
       circuit_chan_publish(circ, n_chan);
     }
 
@@ -602,6 +608,8 @@ circuit_handle_first_hop(origin_circuit_t *circ)
   } else { /* it's already open. use it. */
     tor_assert(!circ->base_.n_hop);
     circ->base_.n_chan = n_chan;
+    /* We found a channel, and we're using it for an origin circuit. */
+    channel_mark_as_used_for_origin_circuit(n_chan);
     circuit_chan_publish(circ, n_chan);
     log_debug(LD_CIRC,"Conn open for %s. Delivering first onion skin.",
               safe_str_client(extend_info_describe(firsthop->extend_info)));
diff --git a/src/core/or/or_connection_st.h b/src/core/or/or_connection_st.h
index 8e012a6b85..253fe67020 100644
--- a/src/core/or/or_connection_st.h
+++ b/src/core/or/or_connection_st.h
@@ -74,6 +74,11 @@ struct or_connection_t {
   unsigned int is_outgoing:1;
   unsigned int proxy_type:3; /**< One of PROXY_NONE...PROXY_HAPROXY */
   unsigned int wide_circ_ids:1;
+  /** True iff a failure on this connection indicates a posssible
+   * bootstrapping problem.  We set this as true if we notice that this
+   * connection could handle a pending origin circuit, or if we launch it to
+   * handle an origin circuit. */
+  unsigned int potentially_used_for_bootstrapping:1;
   /** True iff this connection has had its bootstrap failure logged with
    * control_event_bootstrap_problem. */
   unsigned int have_noted_bootstrap_problem:1;
diff --git a/src/feature/control/control_bootstrap.c b/src/feature/control/control_bootstrap.c
index d4f2adde81..d6dfdad94e 100644
--- a/src/feature/control/control_bootstrap.c
+++ b/src/feature/control/control_bootstrap.c
@@ -348,6 +348,18 @@ control_event_bootstrap_prob_or, (const char *warn, int reason,
 {
   int dowarn = 0;
 
+  if (! or_conn->potentially_used_for_bootstrapping) {
+    /* We never decided that this channel was a good match for one of our
+     * origin_circuit_t objects.  That means that we probably launched it
+     * for somebody else, most likely in response to an EXTEND cell.
+     *
+     * Since EXTEND cells can contain arbitrarily broken descriptions of
+     * relays, a failure on this connection here won't necessarily indicate a
+     * bootstrapping problem.
+     */
+    return;
+  }
+
   if (or_conn->have_noted_bootstrap_problem)
     return;
 
diff --git a/src/feature/relay/circuitbuild_relay.c b/src/feature/relay/circuitbuild_relay.c
index 64f3c341ae..289a5be557 100644
--- a/src/feature/relay/circuitbuild_relay.c
+++ b/src/feature/relay/circuitbuild_relay.c
@@ -475,6 +475,7 @@ circuit_extend(struct cell_t *cell, struct circuit_t *circ)
                                   &ec.ed_pubkey,
                                   ipv4_valid ? &ec.orport_ipv4.addr : NULL,
                                   ipv6_valid ? &ec.orport_ipv6.addr : NULL,
+                                  false,
                                   &msg,
                                   &should_launch);
 
diff --git a/src/test/test_channel.c b/src/test/test_channel.c
index 042eb27d9d..c86327ceb4 100644
--- a/src/test/test_channel.c
+++ b/src/test/test_channel.c
@@ -1382,7 +1382,7 @@ test_channel_for_extend(void *arg)
 
   /* The expected result is chan2 because it is older than chan1. */
   ret_chan = channel_get_for_extend(digest, &ed_id, &ipv4_addr, &ipv6_addr,
-                                    &msg, &launch);
+                                    false, &msg, &launch);
   tt_assert(ret_chan);
   tt_ptr_op(ret_chan, OP_EQ, chan2);
   tt_int_op(launch, OP_EQ, 0);
@@ -1391,7 +1391,7 @@ test_channel_for_extend(void *arg)
   /* Switch that around from previous test. */
   chan2->timestamp_created = chan1->timestamp_created + 1;
   ret_chan = channel_get_for_extend(digest, &ed_id, &ipv4_addr, &ipv6_addr,
-                                    &msg, &launch);
+                                    false, &msg, &launch);
   tt_assert(ret_chan);
   tt_ptr_op(ret_chan, OP_EQ, chan1);
   tt_int_op(launch, OP_EQ, 0);
@@ -1401,7 +1401,7 @@ test_channel_for_extend(void *arg)
    * channel 2 should be picked due to how channel_is_better() works. */
   chan2->timestamp_created = chan1->timestamp_created;
   ret_chan = channel_get_for_extend(digest, &ed_id, &ipv4_addr, &ipv6_addr,
-                                    &msg, &launch);
+                                    false, &msg, &launch);
   tt_assert(ret_chan);
   tt_ptr_op(ret_chan, OP_EQ, chan1);
   tt_int_op(launch, OP_EQ, 0);
@@ -1413,7 +1413,7 @@ test_channel_for_extend(void *arg)
   /* Condemned the older channel. */
   chan1->state = CHANNEL_STATE_CLOSING;
   ret_chan = channel_get_for_extend(digest, &ed_id, &ipv4_addr, &ipv6_addr,
-                                    &msg, &launch);
+                                    false, &msg, &launch);
   tt_assert(ret_chan);
   tt_ptr_op(ret_chan, OP_EQ, chan2);
   tt_int_op(launch, OP_EQ, 0);
@@ -1423,7 +1423,7 @@ test_channel_for_extend(void *arg)
   /* Make the older channel a client one. */
   channel_mark_client(chan1);
   ret_chan = channel_get_for_extend(digest, &ed_id, &ipv4_addr, &ipv6_addr,
-                                    &msg, &launch);
+                                    false, &msg, &launch);
   tt_assert(ret_chan);
   tt_ptr_op(ret_chan, OP_EQ, chan2);
   tt_int_op(launch, OP_EQ, 0);
@@ -1435,7 +1435,7 @@ test_channel_for_extend(void *arg)
   memset(&dumb_ed_id, 0, sizeof(dumb_ed_id));
   ret_chan = channel_get_for_extend(digest, &dumb_ed_id,
                                     &ipv4_addr, &ipv6_addr,
-                                    &msg, &launch);
+                                    false, &msg, &launch);
   tt_assert(!ret_chan);
   tt_str_op(msg, OP_EQ, "Not connected. Connecting.");
   tt_int_op(launch, OP_EQ, 1);
@@ -1445,7 +1445,7 @@ test_channel_for_extend(void *arg)
   chan1->state = CHANNEL_STATE_OPENING;
   chan2->state = CHANNEL_STATE_OPENING;
   ret_chan = channel_get_for_extend(digest, &ed_id, &ipv4_addr, &ipv6_addr,
-                                    &msg, &launch);
+                                    false, &msg, &launch);
   tt_assert(!ret_chan);
   tt_str_op(msg, OP_EQ, "Connection in progress; waiting.");
   tt_int_op(launch, OP_EQ, 0);
@@ -1455,7 +1455,7 @@ test_channel_for_extend(void *arg)
   /* Mark channel 1 as bad for circuits. */
   channel_mark_bad_for_new_circs(chan1);
   ret_chan = channel_get_for_extend(digest, &ed_id, &ipv4_addr, &ipv6_addr,
-                                    &msg, &launch);
+                                    false, &msg, &launch);
   tt_assert(ret_chan);
   tt_ptr_op(ret_chan, OP_EQ, chan2);
   tt_int_op(launch, OP_EQ, 0);
@@ -1466,7 +1466,7 @@ test_channel_for_extend(void *arg)
   channel_mark_bad_for_new_circs(chan1);
   channel_mark_bad_for_new_circs(chan2);
   ret_chan = channel_get_for_extend(digest, &ed_id, &ipv4_addr, &ipv6_addr,
-                                    &msg, &launch);
+                                    false, &msg, &launch);
   tt_assert(!ret_chan);
   tt_str_op(msg, OP_EQ, "Connections all too old, or too non-canonical. "
                         " Launching a new one.");
@@ -1478,7 +1478,7 @@ test_channel_for_extend(void *arg)
   test_chan_should_be_canonical = 0;
   test_chan_should_match_target = 0;
   ret_chan = channel_get_for_extend(digest, &ed_id, &ipv4_addr, &ipv6_addr,
-                                    &msg, &launch);
+                                    false, &msg, &launch);
   tt_assert(!ret_chan);
   tt_str_op(msg, OP_EQ, "Connections all too old, or too non-canonical. "
                         " Launching a new one.");
diff --git a/src/test/test_circuitbuild.c b/src/test/test_circuitbuild.c
index 74824a1bc1..299908ce82 100644
--- a/src/test/test_circuitbuild.c
+++ b/src/test/test_circuitbuild.c
@@ -1214,6 +1214,7 @@ mock_channel_get_for_extend(const char *rsa_id_digest,
                             const ed25519_public_key_t *ed_id,
                             const tor_addr_t *target_ipv4_addr,
                             const tor_addr_t *target_ipv6_addr,
+                            bool for_origin_circ,
                             const char **msg_out,
                             int *launch_out)
 {
@@ -1221,6 +1222,7 @@ mock_channel_get_for_extend(const char *rsa_id_digest,
   (void)ed_id;
   (void)target_ipv4_addr;
   (void)target_ipv6_addr;
+  (void)for_origin_circ;
 
   /* channel_get_for_extend() requires non-NULL arguments */
   tt_ptr_op(msg_out, OP_NE, NULL);