From 781ab9eea49b07b1925d7d8dcbad06348896344e Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 18 Sep 2020 10:03:57 -0400 Subject: Add flag for whether an OR conn "counts" for bootstrap tracking We set this flag if we've launched the connection in order to satisfy an origin circuit, or when we decide the connection _would_ satisfy an origin circuit. These are the only or_connections we want to consider for bootstrapping: other or_connections are opened because of client EXTEND requests, and they may succeed or fail because of the clients' confusion or misconfiguration. Closes #25061. --- changes/ticket25061 | 6 ++++++ src/core/or/channel.c | 10 +++++++++- src/core/or/channel.h | 2 ++ src/core/or/channeltls.c | 22 ++++++++++++++++++++++ src/core/or/circuitbuild.c | 3 +++ src/core/or/or_connection_st.h | 5 +++++ src/feature/control/control_bootstrap.c | 3 +++ src/feature/relay/circuitbuild_relay.c | 1 + src/test/test_channel.c | 20 ++++++++++---------- src/test/test_circuitbuild.c | 2 ++ 10 files changed, 63 insertions(+), 11 deletions(-) create mode 100644 changes/ticket25061 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..0765f222c1 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,12 @@ 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) { + 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..2c52c07bb5 100644 --- a/src/core/or/channeltls.c +++ b/src/core/or/channeltls.c @@ -360,6 +360,28 @@ 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. + **/ +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 ab4ce9f784..225a0112f7 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,7 @@ circuit_handle_first_hop(origin_circuit_t *circ) log_info(LD_CIRC,"connect to firsthop failed. Closing."); return -END_CIRC_REASON_CONNECTFAILED; } + channel_mark_as_used_for_origin_circuit(n_chan); circuit_chan_publish(circ, n_chan); } @@ -602,6 +604,7 @@ 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; + 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..cca2a81b1f 100644 --- a/src/feature/control/control_bootstrap.c +++ b/src/feature/control/control_bootstrap.c @@ -348,6 +348,9 @@ control_event_bootstrap_prob_or, (const char *warn, int reason, { int dowarn = 0; + if (! or_conn->potentially_used_for_bootstrapping) + 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); -- cgit v1.2.3-54-g00ecf From cb4cedae686bd227d42997840b3a6b0b3bc5e936 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 19 Oct 2020 11:45:24 -0400 Subject: Explain why we use "mark_as_used_for_origin_circuit" where we do Also, explain why it's relevant for bootstrapping. This is a comments-only patch. --- src/core/or/channel.c | 3 +++ src/core/or/channeltls.c | 5 ++++- src/core/or/circuitbuild.c | 5 +++++ src/feature/control/control_bootstrap.c | 11 ++++++++++- 4 files changed, 22 insertions(+), 2 deletions(-) diff --git a/src/core/or/channel.c b/src/core/or/channel.c index 0765f222c1..c163f53488 100644 --- a/src/core/or/channel.c +++ b/src/core/or/channel.c @@ -2447,6 +2447,9 @@ channel_get_for_extend,(const char *rsa_id_digest, 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); } } diff --git a/src/core/or/channeltls.c b/src/core/or/channeltls.c index 2c52c07bb5..32723fed1e 100644 --- a/src/core/or/channeltls.c +++ b/src/core/or/channeltls.c @@ -365,7 +365,10 @@ channel_tls_handle_incoming(or_connection_t *orconn) * corresponding to the provided channel. * * This flag indicates that if the connection fails, it might be interesting - * to the bootstrapping subsystem. + * 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) diff --git a/src/core/or/circuitbuild.c b/src/core/or/circuitbuild.c index 225a0112f7..0b53a4cda8 100644 --- a/src/core/or/circuitbuild.c +++ b/src/core/or/circuitbuild.c @@ -591,6 +591,10 @@ 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); } @@ -604,6 +608,7 @@ 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.", diff --git a/src/feature/control/control_bootstrap.c b/src/feature/control/control_bootstrap.c index cca2a81b1f..d6dfdad94e 100644 --- a/src/feature/control/control_bootstrap.c +++ b/src/feature/control/control_bootstrap.c @@ -348,8 +348,17 @@ control_event_bootstrap_prob_or, (const char *warn, int reason, { int dowarn = 0; - if (! or_conn->potentially_used_for_bootstrapping) + 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; -- cgit v1.2.3-54-g00ecf