From d66549c20855b4d75b1a26f18c852327f6a75a05 Mon Sep 17 00:00:00 2001 From: Roger Dingledine Date: Sat, 23 Oct 2021 04:18:00 -0400 Subject: [PATCH 1/6] fetch missing bridge descriptors without delay Without this change, if we have a working bridge, and we add a new bridge, we will schedule the fetch attempt for that new bridge descriptor for three hours(!) in the future. This change is especially needed because of bug #40396, where if you have one working bridge and one bridge whose descriptor you haven't fetched yet, your Tor will stall until you have successfully fetched that new descriptor -- in this case for hours. In the old design, we would put off all further bridge descriptor fetches once we had any working bridge descriptor. In this new design, we make the decision per bridge based on whether we successfully got *its* descriptor. To make this work, we need to also call learned_bridge_descriptor() every time we get a bridge descriptor, not just when it's a novel descriptor. Fixes bug 40396. Also happens to fix bug 40495 (redundant descriptor fetches for every bridge) since now we delay fetches once we succeed. A side effect of this change is that if we have any configured bridges that *aren't* working, we will keep trying to fetch their descriptors on the modern directory retry schedule -- every couple of seconds for the first half minute, then backing off after that -- which is a lot faster than before. --- doc/man/tor.1.txt | 11 ++++++----- src/feature/client/bridges.c | 4 +++- src/feature/dirclient/dlstatus.c | 17 ++++++++--------- src/feature/nodelist/routerlist.c | 7 +++++++ src/test/test_dir.c | 8 +------- 5 files changed, 25 insertions(+), 22 deletions(-) diff --git a/doc/man/tor.1.txt b/doc/man/tor.1.txt index c64a84d05f..3116957bc2 100644 --- a/doc/man/tor.1.txt +++ b/doc/man/tor.1.txt @@ -3556,14 +3556,15 @@ The following options are used for running a testing Tor network. [[TestingAuthKeySlop]] **TestingAuthKeySlop** __N__ **seconds**|**minutes**|**hours** + [[TestingBridgeBootstrapDownloadInitialDelay]] **TestingBridgeBootstrapDownloadInitialDelay** __N__:: - Initial delay in seconds for when clients should download each bridge descriptor when they - have just started, or when they can not contact any of their bridges. + Initial delay in seconds for how long clients should wait before + downloading a bridge descriptor for a new bridge. Changing this requires that **TestingTorNetwork** is set. (Default: 0) [[TestingBridgeDownloadInitialDelay]] **TestingBridgeDownloadInitialDelay** __N__:: - Initial delay in seconds for when clients should download each bridge descriptor when they - know that one or more of their configured bridges are running. Changing - this requires that **TestingTorNetwork** is set. (Default: 10800) + How long to wait (in seconds) once clients have successfully + downloaded a bridge descriptor, before trying another download for + that same bridge. Changing this requires that **TestingTorNetwork** + is set. (Default: 10800) [[TestingClientConsensusDownloadInitialDelay]] **TestingClientConsensusDownloadInitialDelay** __N__:: Initial delay in seconds for when clients should download consensuses. Changing this diff --git a/src/feature/client/bridges.c b/src/feature/client/bridges.c index d40bcc6c8e..bc065a94c4 100644 --- a/src/feature/client/bridges.c +++ b/src/feature/client/bridges.c @@ -961,12 +961,14 @@ learned_bridge_descriptor(routerinfo_t *ri, int from_cache) if (bridge) { /* if we actually want to use this one */ node_t *node; - /* it's here; schedule its re-fetch for a long time from now. */ if (!from_cache) { /* This schedules the re-fetch at a constant interval, which produces * a pattern of bridge traffic. But it's better than trying all * configured bridges several times in the first few minutes. */ download_status_reset(&bridge->fetch_status); + /* it's here; schedule its re-fetch for a long time from now. */ + bridge->fetch_status.next_attempt_at += + get_options()->TestingBridgeDownloadInitialDelay; } node = node_get_mutable_by_id(ri->cache_info.identity_digest); diff --git a/src/feature/dirclient/dlstatus.c b/src/feature/dirclient/dlstatus.c index 8be2983a5d..c21dd113b4 100644 --- a/src/feature/dirclient/dlstatus.c +++ b/src/feature/dirclient/dlstatus.c @@ -73,15 +73,14 @@ find_dl_min_delay(const download_status_t *dls, const or_options_t *options) } } case DL_SCHED_BRIDGE: - if (options->UseBridges && num_bridges_usable(0) > 0) { - /* A bridge client that is sure that one or more of its bridges are - * running can afford to wait longer to update bridge descriptors. */ - return options->TestingBridgeDownloadInitialDelay; - } else { - /* A bridge client which might have no running bridges, must try to - * get bridge descriptors straight away. */ - return options->TestingBridgeBootstrapDownloadInitialDelay; - } + /* Be conservative here: always return the 'during bootstrap' delay + * value, so we never delay while trying to fetch descriptors + * for new bridges. Once we do succeed at fetching a descriptor + * for our bridge, we will adjust its next_attempt_at based on + * the longer "TestingBridgeDownloadInitialDelay" value. See + * learned_bridge_descriptor() for details. + */ + return options->TestingBridgeBootstrapDownloadInitialDelay; default: tor_assert(0); } diff --git a/src/feature/nodelist/routerlist.c b/src/feature/nodelist/routerlist.c index fbf9e026c2..e64840b5dd 100644 --- a/src/feature/nodelist/routerlist.c +++ b/src/feature/nodelist/routerlist.c @@ -1617,6 +1617,13 @@ router_add_to_routerlist(routerinfo_t *router, const char **msg, "descriptor for router %s", router_describe(router)); } else { + if (router->purpose == ROUTER_PURPOSE_BRIDGE) { + /* Even if we're not going to keep this descriptor, we need to + * let the bridge descriptor fetch subsystem know that we + * succeeded at getting it -- so we can adjust the retry schedule + * to stop trying for a while. */ + learned_bridge_descriptor(router, from_cache); + } log_info(LD_DIR, "Dropping descriptor that we already have for router %s", router_describe(router)); diff --git a/src/test/test_dir.c b/src/test/test_dir.c index 0d2d6800ba..186e09f236 100644 --- a/src/test/test_dir.c +++ b/src/test/test_dir.c @@ -6652,13 +6652,7 @@ test_dir_find_dl_min_delay(void* data) dls.schedule = DL_SCHED_BRIDGE; /* client */ - mock_options->ClientOnly = 1; - mock_options->UseBridges = 1; - if (num_bridges_usable(0) > 0) { - tt_int_op(find_dl_min_delay(&dls, mock_options), OP_EQ, bridge); - } else { - tt_int_op(find_dl_min_delay(&dls, mock_options), OP_EQ, bridge_bootstrap); - } + tt_int_op(find_dl_min_delay(&dls, mock_options), OP_EQ, bridge_bootstrap); done: UNMOCK(networkstatus_consensus_is_bootstrapping); From 867c3c6f89ae67bb95cc3c714feb7898dd3c8e3b Mon Sep 17 00:00:00 2001 From: Roger Dingledine Date: Thu, 28 Oct 2021 20:53:26 -0400 Subject: [PATCH 2/6] only log "new bridge descriptor" if really new The bridge descriptor fetching codes ends up fetching a lot of duplicate bridge descriptors, because this is how we learn when the descriptor changes. This commit only changes comments plus whether we log that one line. It moves us back to the old behavior, before the previous commit for 30496, where we would only log that line when the bridge descriptor we're talking about is better than the one we already had (if any). --- src/feature/client/bridges.c | 18 ++++++++++++++---- src/feature/client/bridges.h | 3 ++- src/feature/nodelist/routerlist.c | 4 ++-- 3 files changed, 18 insertions(+), 7 deletions(-) diff --git a/src/feature/client/bridges.c b/src/feature/client/bridges.c index bc065a94c4..9e36d26929 100644 --- a/src/feature/client/bridges.c +++ b/src/feature/client/bridges.c @@ -943,9 +943,17 @@ rewrite_node_address_for_bridge(const bridge_info_t *bridge, node_t *node) } /** We just learned a descriptor for a bridge. See if that - * digest is in our entry guard list, and add it if not. */ + * digest is in our entry guard list, and add it if not. Schedule the + * next fetch for a long time from now, and initiate any follow-up + * activities like continuing to bootstrap. + * + * from_cache * tells us whether we fetched it from disk (else + * the network) + * + * desc_is_new tells us if we preferred it to the old version we + * had, if any. */ void -learned_bridge_descriptor(routerinfo_t *ri, int from_cache) +learned_bridge_descriptor(routerinfo_t *ri, int from_cache, int desc_is_new) { tor_assert(ri); tor_assert(ri->purpose == ROUTER_PURPOSE_BRIDGE); @@ -984,8 +992,10 @@ learned_bridge_descriptor(routerinfo_t *ri, int from_cache) entry_guard_learned_bridge_identity(&bridge->addrport_configured, (const uint8_t*)ri->cache_info.identity_digest); - log_notice(LD_DIR, "new bridge descriptor '%s' (%s): %s", ri->nickname, - from_cache ? "cached" : "fresh", router_describe(ri)); + if (desc_is_new) + log_notice(LD_DIR, "new bridge descriptor '%s' (%s): %s", + ri->nickname, + from_cache ? "cached" : "fresh", router_describe(ri)); /* If we didn't have a reachable bridge before this one, try directory * documents again. */ if (first) { diff --git a/src/feature/client/bridges.h b/src/feature/client/bridges.h index a42363f683..dd3e498a0a 100644 --- a/src/feature/client/bridges.h +++ b/src/feature/client/bridges.h @@ -46,7 +46,8 @@ void learned_router_identity(const tor_addr_t *addr, uint16_t port, void bridge_add_from_config(struct bridge_line_t *bridge_line); void retry_bridge_descriptor_fetch_directly(const char *digest); void fetch_bridge_descriptors(const or_options_t *options, time_t now); -void learned_bridge_descriptor(routerinfo_t *ri, int from_cache); +void learned_bridge_descriptor(routerinfo_t *ri, + int from_cache, int desc_is_new); const smartlist_t *get_socks_args_by_bridge_addrport(const tor_addr_t *addr, uint16_t port); diff --git a/src/feature/nodelist/routerlist.c b/src/feature/nodelist/routerlist.c index e64840b5dd..c00f7ffb26 100644 --- a/src/feature/nodelist/routerlist.c +++ b/src/feature/nodelist/routerlist.c @@ -1622,7 +1622,7 @@ router_add_to_routerlist(routerinfo_t *router, const char **msg, * let the bridge descriptor fetch subsystem know that we * succeeded at getting it -- so we can adjust the retry schedule * to stop trying for a while. */ - learned_bridge_descriptor(router, from_cache); + learned_bridge_descriptor(router, from_cache, 0); } log_info(LD_DIR, "Dropping descriptor that we already have for router %s", @@ -2054,7 +2054,7 @@ routerlist_descriptors_added(smartlist_t *sl, int from_cache) control_event_descriptors_changed(sl); SMARTLIST_FOREACH_BEGIN(sl, routerinfo_t *, ri) { if (ri->purpose == ROUTER_PURPOSE_BRIDGE) - learned_bridge_descriptor(ri, from_cache); + learned_bridge_descriptor(ri, from_cache, 1); if (ri->needs_retest_if_added) { ri->needs_retest_if_added = 0; dirserv_single_reachability_test(approx_time(), ri); From 3c8510e2c0b10637f4d2fdd81a6d0083968b7cc9 Mon Sep 17 00:00:00 2001 From: Roger Dingledine Date: Fri, 22 Oct 2021 02:33:49 -0400 Subject: [PATCH 3/6] reassess minimum-dir-info when a bridge fails When we try to fetch a bridge descriptor and we fail, we mark the guard as failed, but we never scheduled a re-compute for router_have_minimum_dir_info(). So if we had already decided we needed to wait for this new descriptor, we would just wait forever -- even if, counterintuitively, *losing* the bridge is just what we need to *resume* using the network, if we had it in state GUARD_REACHABLE_MAYBE and we were stalling to learn this outcome. See bug 40396 for more details. --- changes/bug40396 | 10 ++++++++++ src/feature/client/entrynodes.c | 7 +++++++ 2 files changed, 17 insertions(+) create mode 100644 changes/bug40396 diff --git a/changes/bug40396 b/changes/bug40396 new file mode 100644 index 0000000000..f9d5b8ecb2 --- /dev/null +++ b/changes/bug40396 @@ -0,0 +1,10 @@ + o Major bugfixes (bridges): + - Make Tor work reliably again when you have multiple bridges + configured and one or more of them are unreachable. The problem + came because we require that we have bridge descriptors for both + of our first two bridges (else we refuse to try to connect), but + in some cases we would wait three hours before trying to fetch + these missing descriptors, and/or never recover when we do try + to fetch them. Fixes bugs 40396 and 40495; bugfix on 0.3.0.5-rc + and 0.3.2.1-alpha. + diff --git a/src/feature/client/entrynodes.c b/src/feature/client/entrynodes.c index 315bd54674..efc1c07763 100644 --- a/src/feature/client/entrynodes.c +++ b/src/feature/client/entrynodes.c @@ -2271,6 +2271,13 @@ entry_guards_note_guard_failure(guard_selection_t *gs, guard->is_primary?"primary ":"", guard->confirmed_idx>=0?"confirmed ":"", entry_guard_describe(guard)); + + /* Schedule a re-assessment of whether we have enough dir info to + * use the network. Counterintuitively, *losing* a bridge might actually + * be just what we need to *resume* using the network, if we had it in + * state GUARD_REACHABLE_MAYBE and we were stalling to learn this + * outcome. See bug 40396 for more details. */ + router_dir_info_changed(); } /** From db34f6c9e82bf95bb0c0e24e9ea858f04919fbc4 Mon Sep 17 00:00:00 2001 From: Roger Dingledine Date: Thu, 28 Oct 2021 20:39:31 -0400 Subject: [PATCH 4/6] handle other de-sync cases from #40396 Specifically, every time a guard moves into or out of state GUARD_REACHABLE_MAYBE, it is an opportunity for the guard reachability state to get out of sync with the have-minimum-dir-info state. Fixes even more of #40396. --- src/feature/client/entrynodes.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/src/feature/client/entrynodes.c b/src/feature/client/entrynodes.c index efc1c07763..a05ec63512 100644 --- a/src/feature/client/entrynodes.c +++ b/src/feature/client/entrynodes.c @@ -132,6 +132,7 @@ #include "feature/client/entrynodes.h" #include "feature/client/transports.h" #include "feature/control/control_events.h" +#include "feature/dirclient/dlstatus.h" #include "feature/dircommon/directory.h" #include "feature/nodelist/describe.h" #include "feature/nodelist/microdesc.h" @@ -576,6 +577,18 @@ mark_guard_maybe_reachable(entry_guard_t *guard) guard->is_reachable = GUARD_REACHABLE_MAYBE; if (guard->is_filtered_guard) guard->is_usable_filtered_guard = 1; + + /* Check if it is a bridge and we don't have its descriptor yet */ + if (guard->bridge_addr && !guard_has_descriptor(guard)) { + /* Reset the descriptor fetch retry schedule, so it gives it another + * go soon. It's important to keep any "REACHABLE_MAYBE" bridges in + * sync with the descriptor fetch schedule, since we will refuse to + * use the network until our first primary bridges are either + * known-usable or known-unusable. See bug 40396. */ + download_status_t *dl = get_bridge_dl_status_by_id(guard->identity); + if (dl) + download_status_reset(dl); + } } /** @@ -2302,6 +2315,12 @@ entry_guards_note_guard_success(guard_selection_t *gs, /* If guard was not already marked as reachable, send a GUARD UP signal */ if (guard->is_reachable != GUARD_REACHABLE_YES) { control_event_guard(guard->nickname, guard->identity, "UP"); + + /* Schedule a re-assessment of whether we have enough dir info to + * use the network. One of our guards has just moved to + * GUARD_REACHABLE_YES, so maybe we can resume using the network + * now. */ + router_dir_info_changed(); } guard->is_reachable = GUARD_REACHABLE_YES; @@ -3545,6 +3564,11 @@ entry_guards_changed_for_guard_selection(guard_selection_t *gs) entry_guards_update_guards_in_state() */ or_state_mark_dirty(get_or_state(), when); + + /* Schedule a re-assessment of whether we have enough dir info to + * use the network. When we add or remove or disable or enable a + * guard, the decision could shift. */ + router_dir_info_changed(); } /** Our list of entry guards has changed for the default guard selection From f9cb7e3398a8c0941a2c489b16a9e8ef66bc1324 Mon Sep 17 00:00:00 2001 From: Roger Dingledine Date: Sat, 23 Oct 2021 05:24:29 -0400 Subject: [PATCH 5/6] do notice-level log when we resume having enough dir info we do a notice-level log when we decide we *don't* have enough dir info, but in 0.3.5.1-alpha (see commit eee62e13d97, #14950) we lost our corresponding notice-level log when things come back. bugfix on 0.3.5.1-alpha; fixes bug 40496. --- changes/bug40496 | 6 ++++++ src/feature/nodelist/nodelist.c | 10 ++++++++-- 2 files changed, 14 insertions(+), 2 deletions(-) create mode 100644 changes/bug40496 diff --git a/changes/bug40496 b/changes/bug40496 new file mode 100644 index 0000000000..b626cc51fe --- /dev/null +++ b/changes/bug40496 @@ -0,0 +1,6 @@ + o Minor bugfixes (logging): + - When we no longer have enough directory information to use the + network, we would log a notice-level message -- but we would not + reliably log a message when we recovered and resumed using the + network. Now make sure there is always a corresponding message + about recovering. Fixes bug 40496; bugfix on 0.3.5.1-alpha. diff --git a/src/feature/nodelist/nodelist.c b/src/feature/nodelist/nodelist.c index 121dc8823a..c676e8dfb4 100644 --- a/src/feature/nodelist/nodelist.c +++ b/src/feature/nodelist/nodelist.c @@ -2820,6 +2820,7 @@ update_router_have_minimum_dir_info(void) const networkstatus_t *consensus = networkstatus_get_reasonably_live_consensus(now,usable_consensus_flavor()); int using_md; + static int be_loud_when_things_work_again = 0; if (!consensus) { if (!networkstatus_get_latest_consensus()) @@ -2875,8 +2876,9 @@ update_router_have_minimum_dir_info(void) if (res && !have_min_dir_info) { control_event_client_status(LOG_NOTICE, "ENOUGH_DIR_INFO"); control_event_boot_dir(BOOTSTRAP_STATUS_ENOUGH_DIRINFO, 0); - log_info(LD_DIR, - "We now have enough directory information to build circuits."); + tor_log(be_loud_when_things_work_again ? LOG_NOTICE : LOG_INFO, LD_DIR, + "We now have enough directory information to build circuits."); + be_loud_when_things_work_again = 0; } /* If paths have just become unavailable in this update. */ @@ -2885,6 +2887,10 @@ update_router_have_minimum_dir_info(void) tor_log(quiet ? LOG_INFO : LOG_NOTICE, LD_DIR, "Our directory information is no longer up-to-date " "enough to build circuits: %s", dir_info_status); + if (!quiet) { + /* remember to do a notice-level log when things come back */ + be_loud_when_things_work_again = 1; + } /* a) make us log when we next complete a circuit, so we know when Tor * is back up and usable, and b) disable some activities that Tor From 7084ec871070e1f01a48b7735367b94abe1feb21 Mon Sep 17 00:00:00 2001 From: Roger Dingledine Date: Sat, 23 Oct 2021 20:32:36 -0400 Subject: [PATCH 6/6] don't retry entry guards if they're bridges without descriptors When we don't yet have a descriptor for one of our bridges, disable the entry guard retry schedule on that bridge. The entry guard retry schedule and the bridge descriptor retry schedule can conflict, e.g. where we mark a bridge as "maybe up" yet we don't try to fetch its descriptor yet, leading Tor to wait (refusing to do anything) until it becomes time to fetch the descriptor. Fixes bug 40497; bugfix on 0.3.0.3-alpha. --- changes/bug40497 | 8 ++++++++ src/feature/client/entrynodes.c | 8 ++++++++ 2 files changed, 16 insertions(+) create mode 100644 changes/bug40497 diff --git a/changes/bug40497 b/changes/bug40497 new file mode 100644 index 0000000000..d3004d0b72 --- /dev/null +++ b/changes/bug40497 @@ -0,0 +1,8 @@ + o Minor bugfixes (bridges): + - When we don't yet have a descriptor for one of our bridges, disable + the entry guard retry schedule on that bridge. The entry guard retry + schedule and the bridge descriptor retry schedule can conflict, + e.g. where we mark a bridge as "maybe up" yet we don't try to fetch + its descriptor yet, leading Tor to wait (refusing to do anything) + until it becomes time to fetch the descriptor. Fixes bug 40497; + bugfix on 0.3.0.3-alpha. diff --git a/src/feature/client/entrynodes.c b/src/feature/client/entrynodes.c index a05ec63512..32ecb4f705 100644 --- a/src/feature/client/entrynodes.c +++ b/src/feature/client/entrynodes.c @@ -2059,6 +2059,14 @@ entry_guard_consider_retry(entry_guard_t *guard) get_retry_schedule(guard->failing_since, now, guard->is_primary); const time_t last_attempt = guard->last_tried_to_connect; + /* Check if it is a bridge and we don't have its descriptor yet */ + if (guard->bridge_addr && !guard_has_descriptor(guard)) { + /* We want to leave the retry schedule to fetch_bridge_descriptors(), + * so we don't have two retry schedules clobbering each other. See + * bugs 40396 and 40497 for details of why we need this exception. */ + return; + } + if (BUG(last_attempt == 0) || now >= last_attempt + delay) { /* We should mark this retriable. */