From 4b981407335865795bb99c7297ec49d80455d693 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Fri, 18 Sep 2020 11:50:12 -0400 Subject: [PATCH 1/4] relay: Use testing circuit instead of dummy descriptor fetch Tor now can learn its address from a NETINFO cell coming from an authority. Thus, instead from launching a dummy descriptor fetch to learn the address from the directory response (unauthenticated), we simply now launch a one-hop testing circuit. Related to #40071 Signed-off-by: David Goulet --- src/core/or/circuitlist.h | 4 +- src/feature/nodelist/routerlist.c | 69 +++++++++++++++++++------------ 2 files changed, 46 insertions(+), 27 deletions(-) diff --git a/src/core/or/circuitlist.h b/src/core/or/circuitlist.h index fd7e22e4c0..3178e6cd0d 100644 --- a/src/core/or/circuitlist.h +++ b/src/core/or/circuitlist.h @@ -114,7 +114,9 @@ #define CIRCUIT_PURPOSE_S_HSDIR_POST 20 #define CIRCUIT_PURPOSE_S_HS_MAX_ 20 -/** A testing circuit; not meant to be used for actual traffic. */ +/** A testing circuit; not meant to be used for actual traffic. It is used for + * bandwidth measurement, reachability test and address discovery from an + * authority using the NETINFO cell. */ #define CIRCUIT_PURPOSE_TESTING 21 /** A controller made this circuit and Tor should not use it. */ #define CIRCUIT_PURPOSE_CONTROLLER 22 diff --git a/src/feature/nodelist/routerlist.c b/src/feature/nodelist/routerlist.c index 3f6e31bc3a..c1b02b586d 100644 --- a/src/feature/nodelist/routerlist.c +++ b/src/feature/nodelist/routerlist.c @@ -65,6 +65,9 @@ #include "app/config/config.h" #include "core/mainloop/connection.h" #include "core/mainloop/mainloop.h" +#include "core/or/circuitlist.h" +#include "core/or/circuituse.h" +#include "core/or/extendinfo.h" #include "core/or/policies.h" #include "feature/client/bridges.h" #include "feature/control/control_events.h" @@ -137,7 +140,7 @@ static int signed_desc_digest_is_recognized(signed_descriptor_t *desc); static const char *signed_descriptor_get_body_impl( const signed_descriptor_t *desc, int with_annotations); -static void launch_dummy_descriptor_download_as_needed(time_t now, +static void launch_dummy_circuit_as_needed(time_t now, const or_options_t *options); /****************************************************************************/ @@ -2306,7 +2309,7 @@ update_all_descriptor_downloads(time_t now) return; update_router_descriptor_downloads(now); update_microdesc_downloads(now); - launch_dummy_descriptor_download_as_needed(now, get_options()); + launch_dummy_circuit_as_needed(now, get_options()); } /** Clear all our timeouts for fetching v3 directory stuff, and then @@ -2760,23 +2763,20 @@ update_consensus_router_descriptor_downloads(time_t now, int is_vote, smartlist_free(no_longer_old); } -/** How often should we launch a server/authority request to be sure of getting +/** How often should we launch a circuit to an authority to be sure of getting * a guess for our IP? */ -/*XXXX+ this info should come from netinfo cells or something, or we should - * do this only when we aren't seeing incoming data. see bug 652. */ #define DUMMY_DOWNLOAD_INTERVAL (20*60) /** As needed, launch a dummy router descriptor fetch to see if our * address has changed. */ static void -launch_dummy_descriptor_download_as_needed(time_t now, - const or_options_t *options) +launch_dummy_circuit_as_needed(time_t now, const or_options_t *options) { - static time_t last_dummy_download = 0; + static time_t last_dummy_circuit = 0; bool have_addr; tor_addr_t addr_out; - /* This dummy fetch only matter for relays. */ + /* This dummy circuit only matter for relays. */ if (!server_mode(options)) { return; } @@ -2784,27 +2784,44 @@ launch_dummy_descriptor_download_as_needed(time_t now, /* Lookup the address cache to learn if we have a good usable address. We * still force relays to have an IPv4 so that alone is enough to learn if we * need a lookup. In case we don't have one, we might want to attempt a - * dummy fetch to learn our address as a suggestion from an authority. */ + * dummy circuit to learn our address as a suggestion from an authority. */ have_addr = relay_find_addr_to_publish(options, AF_INET, RELAY_FIND_ADDR_CACHE_ONLY, &addr_out); - /* XXXX+ we could be smarter here; see notes on bug 652. */ - /* If we're a server that doesn't have an address, we rely on directory - * fetches to learn when our address changes. So if we haven't tried to get - * any routerdescs in a long time, try a dummy fetch now. */ - if (!have_addr && - last_descriptor_download_attempted + DUMMY_DOWNLOAD_INTERVAL < now && - last_dummy_download + DUMMY_DOWNLOAD_INTERVAL < now) { - last_dummy_download = now; - /* XX/teor - do we want an authority here, because they are less likely - * to give us the wrong address? (See #17782) - * I'm leaving the previous behaviour intact, because I don't like - * the idea of some relays contacting an authority every 20 minutes. */ - directory_get_from_dirserver(DIR_PURPOSE_FETCH_SERVERDESC, - ROUTER_PURPOSE_GENERAL, "authority.z", - PDS_RETRY_IF_NO_SERVERS, - DL_WANT_ANY_DIRSERVER); + /* If we're a relay or bridge for which we were unable to discover our + * public address, we rely on learning our address from a directory + * authority from the NETINFO cell. */ + if (!have_addr && last_dummy_circuit + DUMMY_DOWNLOAD_INTERVAL < now) { + last_dummy_circuit = now; + + const routerstatus_t *rs = router_pick_trusteddirserver(V3_DIRINFO, 0); + if (BUG(!rs)) { + /* We should really always have trusted directories configured at this + * stage. They are loaded early either from default list or the one + * given in the configuration file. */ + return; + } + const node_t *node = node_get_by_id(rs->identity_digest); + if (BUG(!node)) { + /* If there is a routerstatus_t, there is a node_t thus this should + * never fail. */ + return; + } + extend_info_t *ei = extend_info_from_node(node, 1); + if (BUG(!ei)) { + return; + } + + log_debug(LD_GENERAL, "Attempting dummy testing circuit to an authority " + "in order to learn our address."); + + /* Launch a one-hop testing circuit to a trusted authority so we can learn + * our address through the NETINFO cell. */ + circuit_launch_by_extend_info(CIRCUIT_PURPOSE_TESTING, ei, + CIRCLAUNCH_IS_INTERNAL | + CIRCLAUNCH_ONEHOP_TUNNEL); + extend_info_free(ei); } } From a5538a36037641e49ca05aa3e90fec256794412b Mon Sep 17 00:00:00 2001 From: David Goulet Date: Fri, 18 Sep 2020 12:22:23 -0400 Subject: [PATCH 2/4] relay: Look at our cache when looking for an IP change Regularly, tor looks if its IP has changed. It does the entire auto discovery process again. However, it is possible that it does not find anything. Instead of thinking the IP changed to an unknown address, look at our cache and see if that value has changed. The reason for this is because if tor gets its address as a suggestion from a directory authority, it is because the auto discovery failed and thus that address should be consider for the IP change check. Related to #40071 Signed-off-by: David Goulet --- src/feature/relay/router.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/src/feature/relay/router.c b/src/feature/relay/router.c index 29103ed6c6..259c38f162 100644 --- a/src/feature/relay/router.c +++ b/src/feature/relay/router.c @@ -2679,8 +2679,21 @@ check_descriptor_ipaddress_changed(time_t now) /* Ignore returned value because we want to notice not only an address * change but also if an address is lost (current == UNSPEC). */ - find_my_address(get_options(), family, LOG_INFO, ¤t, &method, - &hostname); + bool found = find_my_address(get_options(), family, LOG_INFO, ¤t, + &method, &hostname); + if (!found) { + /* Address was possibly not found because it is simply not configured or + * discoverable. Fallback to our cache, which includes any suggestion + * sent by a trusted directory server. */ + found = relay_find_addr_to_publish(get_options(), family, + RELAY_FIND_ADDR_CACHE_ONLY, + ¤t); + } + + /* The "current" address might be UNSPEC meaning it was not discovered nor + * found in our current cache. If we had an address before and we have + * none now, we consider this an IP change since it appears the relay lost + * its address. */ if (!tor_addr_eq(previous, ¤t)) { char *source; From bc5f26ff7006f50acd23f9eb1a99449612b95198 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Fri, 18 Sep 2020 13:07:11 -0400 Subject: [PATCH 3/4] relay: Launch dummy circuit only when descriptor build fails First, this commit moves the launch_dummy_circuit_as_needed() function into relay_find_addr.c and renames it to relay_addr_learn_from_dirauth(). This is an attempt to centralize anything relate with address discovery in the right module. Second, when building a descriptor and we fail to discover our address, immediately launch a dummy circuit to an authority in an attempt to learn our descriptor. It is still only done every 20 minutes even though the descriptor build is done every minute. We ought to avoid load on the authority and if we can't learn in the first place our address from them, chances are more things are wrong. Related to #40071 Signed-off-by: David Goulet --- src/feature/nodelist/routerlist.c | 65 --------------------------- src/feature/relay/relay_find_addr.c | 68 +++++++++++++++++++++++++++++ src/feature/relay/relay_find_addr.h | 2 + src/feature/relay/router.c | 4 +- 4 files changed, 73 insertions(+), 66 deletions(-) diff --git a/src/feature/nodelist/routerlist.c b/src/feature/nodelist/routerlist.c index c1b02b586d..a1a348edb9 100644 --- a/src/feature/nodelist/routerlist.c +++ b/src/feature/nodelist/routerlist.c @@ -140,8 +140,6 @@ static int signed_desc_digest_is_recognized(signed_descriptor_t *desc); static const char *signed_descriptor_get_body_impl( const signed_descriptor_t *desc, int with_annotations); -static void launch_dummy_circuit_as_needed(time_t now, - const or_options_t *options); /****************************************************************************/ @@ -2309,7 +2307,6 @@ update_all_descriptor_downloads(time_t now) return; update_router_descriptor_downloads(now); update_microdesc_downloads(now); - launch_dummy_circuit_as_needed(now, get_options()); } /** Clear all our timeouts for fetching v3 directory stuff, and then @@ -2763,68 +2760,6 @@ update_consensus_router_descriptor_downloads(time_t now, int is_vote, smartlist_free(no_longer_old); } -/** How often should we launch a circuit to an authority to be sure of getting - * a guess for our IP? */ -#define DUMMY_DOWNLOAD_INTERVAL (20*60) - -/** As needed, launch a dummy router descriptor fetch to see if our - * address has changed. */ -static void -launch_dummy_circuit_as_needed(time_t now, const or_options_t *options) -{ - static time_t last_dummy_circuit = 0; - bool have_addr; - tor_addr_t addr_out; - - /* This dummy circuit only matter for relays. */ - if (!server_mode(options)) { - return; - } - - /* Lookup the address cache to learn if we have a good usable address. We - * still force relays to have an IPv4 so that alone is enough to learn if we - * need a lookup. In case we don't have one, we might want to attempt a - * dummy circuit to learn our address as a suggestion from an authority. */ - have_addr = relay_find_addr_to_publish(options, AF_INET, - RELAY_FIND_ADDR_CACHE_ONLY, - &addr_out); - - /* If we're a relay or bridge for which we were unable to discover our - * public address, we rely on learning our address from a directory - * authority from the NETINFO cell. */ - if (!have_addr && last_dummy_circuit + DUMMY_DOWNLOAD_INTERVAL < now) { - last_dummy_circuit = now; - - const routerstatus_t *rs = router_pick_trusteddirserver(V3_DIRINFO, 0); - if (BUG(!rs)) { - /* We should really always have trusted directories configured at this - * stage. They are loaded early either from default list or the one - * given in the configuration file. */ - return; - } - const node_t *node = node_get_by_id(rs->identity_digest); - if (BUG(!node)) { - /* If there is a routerstatus_t, there is a node_t thus this should - * never fail. */ - return; - } - extend_info_t *ei = extend_info_from_node(node, 1); - if (BUG(!ei)) { - return; - } - - log_debug(LD_GENERAL, "Attempting dummy testing circuit to an authority " - "in order to learn our address."); - - /* Launch a one-hop testing circuit to a trusted authority so we can learn - * our address through the NETINFO cell. */ - circuit_launch_by_extend_info(CIRCUIT_PURPOSE_TESTING, ei, - CIRCLAUNCH_IS_INTERNAL | - CIRCLAUNCH_ONEHOP_TUNNEL); - extend_info_free(ei); - } -} - /** Launch downloads for router status as needed. */ void update_router_descriptor_downloads(time_t now) diff --git a/src/feature/relay/relay_find_addr.c b/src/feature/relay/relay_find_addr.c index 43b958d563..9c2c8b281c 100644 --- a/src/feature/relay/relay_find_addr.c +++ b/src/feature/relay/relay_find_addr.c @@ -12,10 +12,16 @@ #include "app/config/resolve_addr.h" #include "core/mainloop/mainloop.h" +#include "core/or/circuitlist.h" +#include "core/or/circuituse.h" +#include "core/or/extendinfo.h" #include "feature/control/control_events.h" #include "feature/dircommon/dir_connection_st.h" #include "feature/nodelist/dirlist.h" +#include "feature/nodelist/node_select.h" +#include "feature/nodelist/nodelist.h" +#include "feature/nodelist/routerstatus_st.h" #include "feature/relay/relay_find_addr.h" #include "feature/relay/router.h" #include "feature/relay/routermode.h" @@ -151,3 +157,65 @@ relay_has_address_set(int family) return relay_find_addr_to_publish(get_options(), family, RELAY_FIND_ADDR_CACHE_ONLY, &addr); } + +/** How often should we launch a circuit to an authority to be sure of getting + * a guess for our IP? */ +#define DUMMY_DOWNLOAD_INTERVAL (20*60) + +void +relay_addr_learn_from_dirauth(void) +{ + static time_t last_dummy_circuit = 0; + const or_options_t *options = get_options(); + time_t now = time(NULL); + bool have_addr; + tor_addr_t addr_out; + + /* This dummy circuit only matter for relays. */ + if (BUG(!server_mode(options))) { + return; + } + + /* Lookup the address cache to learn if we have a good usable address. We + * still force relays to have an IPv4 so that alone is enough to learn if we + * need a lookup. In case we don't have one, we might want to attempt a + * dummy circuit to learn our address as a suggestion from an authority. */ + have_addr = relay_find_addr_to_publish(options, AF_INET, + RELAY_FIND_ADDR_CACHE_ONLY, + &addr_out); + + /* If we're a relay or bridge for which we were unable to discover our + * public address, we rely on learning our address from a directory + * authority from the NETINFO cell. */ + if (!have_addr && last_dummy_circuit + DUMMY_DOWNLOAD_INTERVAL < now) { + last_dummy_circuit = now; + + const routerstatus_t *rs = router_pick_trusteddirserver(V3_DIRINFO, 0); + if (BUG(!rs)) { + /* We should really always have trusted directories configured at this + * stage. They are loaded early either from default list or the one + * given in the configuration file. */ + return; + } + const node_t *node = node_get_by_id(rs->identity_digest); + if (BUG(!node)) { + /* If there is a routerstatus_t, there is a node_t thus this should + * never fail. */ + return; + } + extend_info_t *ei = extend_info_from_node(node, 1); + if (BUG(!ei)) { + return; + } + + log_debug(LD_GENERAL, "Attempting dummy testing circuit to an authority " + "in order to learn our address."); + + /* Launch a one-hop testing circuit to a trusted authority so we can learn + * our address through the NETINFO cell. */ + circuit_launch_by_extend_info(CIRCUIT_PURPOSE_TESTING, ei, + CIRCLAUNCH_IS_INTERNAL | + CIRCLAUNCH_ONEHOP_TUNNEL); + extend_info_free(ei); + } +} diff --git a/src/feature/relay/relay_find_addr.h b/src/feature/relay/relay_find_addr.h index 3d30946b05..34890cd34e 100644 --- a/src/feature/relay/relay_find_addr.h +++ b/src/feature/relay/relay_find_addr.h @@ -24,6 +24,8 @@ MOCK_DECL(bool, relay_find_addr_to_publish, bool relay_has_address_set(int family); +void relay_addr_learn_from_dirauth(void); + #ifdef RELAY_FIND_ADDR_PRIVATE #endif /* RELAY_FIND_ADDR_PRIVATE */ diff --git a/src/feature/relay/router.c b/src/feature/relay/router.c index 259c38f162..3aa9561f4f 100644 --- a/src/feature/relay/router.c +++ b/src/feature/relay/router.c @@ -2071,7 +2071,9 @@ router_build_fresh_unsigned_routerinfo,(routerinfo_t **ri_out)) /* Tor requires a relay to have an IPv4 so bail if we can't find it. */ if (!have_v4) { - log_warn(LD_CONFIG, "Don't know my address while generating descriptor"); + log_info(LD_CONFIG, "Don't know my address while generating descriptor. " + "Launching circuit to authority to learn it."); + relay_addr_learn_from_dirauth(); result = TOR_ROUTERINFO_ERROR_NO_EXT_ADDR; goto err; } From 9aaac94cc3d2b4768bbac1e40396b74373f7bc5a Mon Sep 17 00:00:00 2001 From: David Goulet Date: Thu, 12 Nov 2020 12:39:25 -0500 Subject: [PATCH 4/4] changes: Add file for #40071 Signed-off-by: David Goulet --- changes/ticket40071 | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 changes/ticket40071 diff --git a/changes/ticket40071 b/changes/ticket40071 new file mode 100644 index 0000000000..1e294a68e7 --- /dev/null +++ b/changes/ticket40071 @@ -0,0 +1,7 @@ + o Minor bugfixes (relay, address): + - Don't trigger an IP change if no new valid IP can be found. Fixes bug + 40071; bugfix on 0.4.5.1-alpha. + - When attempting to discover our IP, don't launch a descriptor fetch + anymore but rather a simple test circuit since the address discovery is + through the NETINFO cell now from the authorities. Fixes bug 40071; bugfix + on 0.4.5.1-alpha.