From b4f4af6ec5ba16e7ba5191dbd271c1754e0c21d4 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Fri, 22 Jan 2021 10:23:07 -0500 Subject: [PATCH 1/4] relay: Skip address discovery if no ORPort is found In other words, if we don't have an ORPort configured for a specific family (IPv4/v6), we don't bother doing address discovery. Related to #40254 Signed-off-by: David Goulet --- src/feature/relay/relay_find_addr.c | 13 ++++++++++++ src/test/test_relay.c | 33 +++++++++++++++++++++-------- 2 files changed, 37 insertions(+), 9 deletions(-) diff --git a/src/feature/relay/relay_find_addr.c b/src/feature/relay/relay_find_addr.c index 2da2328b14..e942a8fa77 100644 --- a/src/feature/relay/relay_find_addr.c +++ b/src/feature/relay/relay_find_addr.c @@ -99,6 +99,13 @@ relay_address_new_suggestion(const tor_addr_t *suggested_addr, * populated by the NETINFO cell content or HTTP header from a * directory. * + * The AddressDisableIPv6 is checked here for IPv6 address discovery and if + * set, false is returned and addr_out is UNSPEC. + * + * Before doing any discovery, the configuration is checked for an ORPort of + * the given family. If none can be found, false is returned and addr_out is + * UNSPEC. + * * Return true on success and addr_out contains the address to use for the * given family. On failure to find the address, false is returned and * addr_out is set to an AF_UNSPEC address. */ @@ -118,6 +125,12 @@ relay_find_addr_to_publish, (const or_options_t *options, int family, return false; } + /* There is no point on attempting an address discovery to publish if we + * don't have an ORPort for this family. */ + if (!routerconf_find_or_port(options, family)) { + return false; + } + /* First, check our resolved address cache. It should contain the address * we've discovered from the periodic relay event. */ resolved_addr_get_last(family, addr_out); diff --git a/src/test/test_relay.c b/src/test/test_relay.c index 545cb4ac46..b287f0d38b 100644 --- a/src/test/test_relay.c +++ b/src/test/test_relay.c @@ -11,6 +11,7 @@ #include "feature/stats/bwhist.h" #include "core/or/relay.h" #include "lib/container/order.h" +#include "lib/encoding/confline.h" /* For init/free stuff */ #include "core/or/scheduler.h" @@ -23,6 +24,8 @@ #include "feature/relay/routermode.h" #include "feature/dirclient/dir_server_st.h" +#define CONFIG_PRIVATE +#include "app/config/config.h" #include "app/config/resolve_addr.h" /* Test suite stuff */ @@ -298,11 +301,12 @@ test_find_addr_to_publish(void *arg) int family; bool ret; tor_addr_t ipv4_addr, ipv6_addr, cache_addr; - or_options_t options; + or_options_t *options; (void) arg; - memset(&options, 0, sizeof(options)); + options = options_new(); + options_init(options); /* Populate our resolved cache with a valid IPv4 and IPv6. */ family = tor_addr_parse(&ipv4_addr, "1.2.3.4"); @@ -317,13 +321,24 @@ test_find_addr_to_publish(void *arg) resolved_addr_get_last(AF_INET6, &cache_addr); tt_assert(tor_addr_eq(&ipv6_addr, &cache_addr)); + /* Setup ORPort config. */ + { + int n, w, r; + char *msg = NULL; + + config_line_append(&options->ORPort_lines, "ORPort", "9001"); + + r = parse_ports(options, 0, &msg, &n, &w); + tt_int_op(r, OP_EQ, 0); + } + /* 1. Address located in the resolved cache. */ - ret = relay_find_addr_to_publish(&options, AF_INET, + ret = relay_find_addr_to_publish(options, AF_INET, RELAY_FIND_ADDR_CACHE_ONLY, &cache_addr); tt_assert(ret); tt_assert(tor_addr_eq(&ipv4_addr, &cache_addr)); - ret = relay_find_addr_to_publish(&options, AF_INET6, + ret = relay_find_addr_to_publish(options, AF_INET6, RELAY_FIND_ADDR_CACHE_ONLY, &cache_addr); tt_assert(ret); tt_assert(tor_addr_eq(&ipv6_addr, &cache_addr)); @@ -334,13 +349,13 @@ test_find_addr_to_publish(void *arg) * the find_my_address() code path because that is extensively tested in * another unit tests. */ resolved_addr_set_suggested(&ipv4_addr); - ret = relay_find_addr_to_publish(&options, AF_INET, + ret = relay_find_addr_to_publish(options, AF_INET, RELAY_FIND_ADDR_CACHE_ONLY, &cache_addr); tt_assert(ret); tt_assert(tor_addr_eq(&ipv4_addr, &cache_addr)); resolved_addr_set_suggested(&ipv6_addr); - ret = relay_find_addr_to_publish(&options, AF_INET6, + ret = relay_find_addr_to_publish(options, AF_INET6, RELAY_FIND_ADDR_CACHE_ONLY, &cache_addr); tt_assert(ret); tt_assert(tor_addr_eq(&ipv6_addr, &cache_addr)); @@ -348,15 +363,15 @@ test_find_addr_to_publish(void *arg) resolve_addr_reset_suggested(AF_INET6); /* 3. No IP anywhere. */ - ret = relay_find_addr_to_publish(&options, AF_INET, + ret = relay_find_addr_to_publish(options, AF_INET, RELAY_FIND_ADDR_CACHE_ONLY, &cache_addr); tt_assert(!ret); - ret = relay_find_addr_to_publish(&options, AF_INET6, + ret = relay_find_addr_to_publish(options, AF_INET6, RELAY_FIND_ADDR_CACHE_ONLY, &cache_addr); tt_assert(!ret); done: - ; + or_options_free(options); } struct testcase_t relay_tests[] = { From b4220a09b71a924e423798c4cf5001c0bdc07f89 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Fri, 22 Jan 2021 10:29:08 -0500 Subject: [PATCH 2/4] relay: Simplify IPv6 discovery when building descriptor Now that relay_find_addr_to_publish() checks if we actually have an ORPort, we can simplify the descriptor building phase for IPv6. This also avoid triggering an IPv6 discovery if the IPv4 can't be found in the first place. Related to #40254 Signed-off-by: David Goulet --- src/feature/relay/router.c | 22 +++++++--------------- 1 file changed, 7 insertions(+), 15 deletions(-) diff --git a/src/feature/relay/router.c b/src/feature/relay/router.c index eb1d5a63f1..e5111acd65 100644 --- a/src/feature/relay/router.c +++ b/src/feature/relay/router.c @@ -2047,12 +2047,11 @@ MOCK_IMPL(STATIC int, router_build_fresh_unsigned_routerinfo,(routerinfo_t **ri_out)) { routerinfo_t *ri = NULL; - tor_addr_t ipv4_addr, ipv6_addr; + tor_addr_t ipv4_addr; char platform[256]; int hibernating = we_are_hibernating(); const or_options_t *options = get_options(); int result = TOR_ROUTERINFO_ERROR_INTERNAL_BUG; - uint16_t ipv6_orport = 0; if (BUG(!ri_out)) { result = TOR_ROUTERINFO_ERROR_INTERNAL_BUG; @@ -2064,10 +2063,6 @@ router_build_fresh_unsigned_routerinfo,(routerinfo_t **ri_out)) bool have_v4 = relay_find_addr_to_publish(options, AF_INET, RELAY_FIND_ADDR_NO_FLAG, &ipv4_addr); - bool have_v6 = relay_find_addr_to_publish(options, AF_INET6, - RELAY_FIND_ADDR_NO_FLAG, - &ipv6_addr); - /* Tor requires a relay to have an IPv4 so bail if we can't find it. */ if (!have_v4) { log_info(LD_CONFIG, "Don't know my address while generating descriptor. " @@ -2079,24 +2074,21 @@ router_build_fresh_unsigned_routerinfo,(routerinfo_t **ri_out)) /* Log a message if the address in the descriptor doesn't match the ORPort * and DirPort addresses configured by the operator. */ router_check_descriptor_address_consistency(&ipv4_addr); - router_check_descriptor_address_consistency(&ipv6_addr); ri = tor_malloc_zero(sizeof(routerinfo_t)); + tor_addr_copy(&ri->ipv4_addr, &ipv4_addr); ri->cache_info.routerlist_index = -1; ri->nickname = tor_strdup(options->Nickname); /* IPv4. */ - tor_addr_copy(&ri->ipv4_addr, &ipv4_addr); ri->ipv4_orport = routerconf_find_or_port(options, AF_INET); ri->ipv4_dirport = routerconf_find_dir_port(options, 0); - /* IPv6. Do not publish an IPv6 if we don't have an ORPort that can be used - * with the address. This is possible for instance if the ORPort is - * IPv4Only. */ - ipv6_orport = routerconf_find_or_port(options, AF_INET6); - if (have_v6 && ipv6_orport != 0) { - tor_addr_copy(&ri->ipv6_addr, &ipv6_addr); - ri->ipv6_orport = ipv6_orport; + /* Optionally check for an IPv6. We still publish without one. */ + if (relay_find_addr_to_publish(options, AF_INET6, RELAY_FIND_ADDR_NO_FLAG, + &ri->ipv6_addr)) { + ri->ipv6_orport = routerconf_find_or_port(options, AF_INET6); + router_check_descriptor_address_consistency(&ri->ipv6_addr); } ri->supports_tunnelled_dir_requests = From 2e600019ea32277525c46809244491970869b084 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Fri, 22 Jan 2021 10:35:32 -0500 Subject: [PATCH 3/4] relay: Don't trigger an address discovery without an ORPort We would before do an address discovery and then a lookup in the cache if not found which is now simplified by calling relay_find_addr_to_publish() directly which does all those combined. Furthermore, by doing so, we won't trigger an address discovery every minute if we have no ORPort configured for the family. Fixes #40254 Signed-off-by: David Goulet --- changes/ticket40254 | 4 ++++ src/feature/relay/router.c | 17 +++++------------ 2 files changed, 9 insertions(+), 12 deletions(-) create mode 100644 changes/ticket40254 diff --git a/changes/ticket40254 b/changes/ticket40254 new file mode 100644 index 0000000000..9945e3d3b8 --- /dev/null +++ b/changes/ticket40254 @@ -0,0 +1,4 @@ + o Minor bugfixes (relay, configuration): + - Don't attempt to discover address (IPv4 or IPv6) if no ORPort for it can + be found in the configuration. Fixes bug 40254; bugfix on 0.4.5.1-alpha. + diff --git a/src/feature/relay/router.c b/src/feature/relay/router.c index e5111acd65..4bc71eb486 100644 --- a/src/feature/relay/router.c +++ b/src/feature/relay/router.c @@ -2671,18 +2671,11 @@ check_descriptor_ipaddress_changed(time_t now) previous = &my_ri->ipv6_addr; } - /* Ignore returned value because we want to notice not only an address - * change but also if an address is lost (current == UNSPEC). */ - 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); - } + /* Attempt to discovery the publishable address for the family which will + * actively attempt to discover the address if we are configured with a + * port for the family. */ + relay_find_addr_to_publish(get_options(), family, RELAY_FIND_ADDR_NO_FLAG, + ¤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 From f03047332ceaa465a13fac3ff3de6378f4f2794a Mon Sep 17 00:00:00 2001 From: David Goulet Date: Fri, 22 Jan 2021 10:59:05 -0500 Subject: [PATCH 4/4] relay: Log if we can't find an address for configured ORPort Everytime we try to discover an address we want to publish, emit a log notice if we are unable to find it even though an ORPort was configured for it. Because the function can be called quite often, we rate limit that notice to every hour so it gets annoying just enough so the operator fixes that. Related to #40254 Signed-off-by: David Goulet --- src/feature/relay/relay_find_addr.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/feature/relay/relay_find_addr.c b/src/feature/relay/relay_find_addr.c index e942a8fa77..39e1cc6a19 100644 --- a/src/feature/relay/relay_find_addr.c +++ b/src/feature/relay/relay_find_addr.c @@ -144,6 +144,17 @@ relay_find_addr_to_publish, (const or_options_t *options, int family, if (find_my_address(options, family, LOG_INFO, addr_out, NULL, NULL)) { goto found; } + /* No publishable address was found even though we have an ORPort thus + * print a notice log so operator can notice. We'll do that every hour so + * it is not too spammy but enough so operators address the issue. */ + static ratelim_t rlim = RATELIM_INIT(3600); + log_fn_ratelim(&rlim, LOG_NOTICE, LD_CONFIG, + "Unable to find %s address for ORPort %u. " + "You might want to specify %sOnly to it or set an " + "explicit address or set Address.", + fmt_af_family(family), + routerconf_find_or_port(options, family), + fmt_af_family(family)); } /* Third, consider address from our suggestion cache. */