From c18e52af7ce178e0dc78830e41948a9298e6d314 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Mon, 20 Jul 2020 14:20:53 -0400 Subject: [PATCH 01/16] addr: Continue discovery if Address exits but not for wanted family Commit b14b1f2b1d9 was a mistake. In case an Address statement is missing for the wanted family but another one exists for another family, simply continue the address discovery. It is not a mistake to be missing an Address statement for a family because the address could simply be discovered by the next methods. Not all address family requires a specific Address statement. However, we do bail if we couldn't find any valid address for the requested family _and_ a resolve failed meaning we had a hostname but couldn't resolve it. In that case, we can't know if that hostname would have been for v4 or v6 thus we can't continue the address discovery properly. Couple unit tests case were removed to match this reality. Related #40025 Signed-off-by: David Goulet --- src/app/config/resolve_addr.c | 20 +++++++++------ src/test/test_config.c | 47 ----------------------------------- 2 files changed, 12 insertions(+), 55 deletions(-) diff --git a/src/app/config/resolve_addr.c b/src/app/config/resolve_addr.c index 7ec5ae565a..7368b019d6 100644 --- a/src/app/config/resolve_addr.c +++ b/src/app/config/resolve_addr.c @@ -194,7 +194,7 @@ get_address_from_config(const or_options_t *options, int warn_severity, char **hostname_out, tor_addr_t *addr_out) { int ret; - bool explicit_ip = false; + bool explicit_ip = false, resolve_failure = false; int num_valid_addr = 0; tor_assert(options); @@ -246,6 +246,7 @@ get_address_from_config(const or_options_t *options, int warn_severity, continue; } else { /* Hostname that can't be resolved, this is a fatal error. */ + resolve_failure = true; log_fn(warn_severity, LD_CONFIG, "Could not resolve local Address '%s'. Failing.", cfg->value); continue; @@ -253,13 +254,16 @@ get_address_from_config(const or_options_t *options, int warn_severity, } if (!num_valid_addr) { - log_fn(warn_severity, LD_CONFIG, - "No Address option found for family %s in configuration.", - fmt_af_family(family)); - /* No Address statement for family but one exists since Address is not - * NULL thus we have to stop now and not attempt to send back a guessed - * address. */ - return FN_RET_BAIL; + if (resolve_failure) { + /* We found no address but we got a resolution failure. This means we + * can know if the hostname given was v4 or v6 so we can't continue. */ + return FN_RET_BAIL; + } + log_info(LD_CONFIG, + "No Address option found for family %s in configuration.", + fmt_af_family(family)); + /* No Address statement for family so move on to try next method. */ + return FN_RET_NEXT; } if (num_valid_addr >= MAX_CONFIG_ADDRESS) { diff --git a/src/test/test_config.c b/src/test/test_config.c index cdf668be49..a50e6ac927 100644 --- a/src/test/test_config.c +++ b/src/test/test_config.c @@ -1290,10 +1290,6 @@ test_config_find_my_address_mixed(void *arg) "2a01:4f8:fff0:4f:266:37ff:fe2c:5d19"); tor_addr_parse(&test_addr, "2a01:4f8:fff0:4f:266:37ff:fe2c:5d19"); - /* IPv4 address not guessed since one Address statement exists. */ - retval = find_my_address(options, AF_INET, LOG_NOTICE, &resolved_addr, - &method_used, &hostname_out); - VALIDATE_FOUND_ADDRESS(false, NULL, NULL); /* IPv6 address should be found and considered configured. */ retval = find_my_address(options, AF_INET6, LOG_NOTICE, &resolved_addr, &method_used, &hostname_out); @@ -1542,49 +1538,6 @@ test_config_find_my_address(void *arg) VALIDATE_FOUND_ADDRESS(false, NULL, NULL); CLEANUP_FOUND_ADDRESS; - /* - * Case 6: Another address family is configured. Expected to fail. - */ - if (p->family == AF_INET) { - config_line_append(&options->Address, "Address", "4242::4242"); - } else { - config_line_append(&options->Address, "Address", "1.1.1.1"); - } - - setup_full_capture_of_logs(LOG_NOTICE); - - retval = find_my_address(options, p->family, LOG_NOTICE, &resolved_addr, - &method_used, &hostname_out); - - expect_log_msg_containing("No Address option found for family"); - teardown_capture_of_logs(); - - VALIDATE_FOUND_ADDRESS(false, NULL, NULL); - CLEANUP_FOUND_ADDRESS; - - /* - * Case 7: Address is a non resolvable hostname. Expected to fail. - */ - MOCK(tor_addr_lookup, tor_addr_lookup_failure); - - config_line_append(&options->Address, "Address", "www.torproject.org"); - prev_n_hostname_failure = n_hostname_failure; - - setup_full_capture_of_logs(LOG_NOTICE); - - retval = find_my_address(options, p->family, LOG_NOTICE, &resolved_addr, - &method_used, &hostname_out); - - expect_log_msg_containing("Could not resolve local Address " - "'www.torproject.org'. Failing."); - teardown_capture_of_logs(); - - tt_int_op(n_hostname_failure, OP_EQ, ++prev_n_hostname_failure); - VALIDATE_FOUND_ADDRESS(false, NULL, NULL); - CLEANUP_FOUND_ADDRESS; - - UNMOCK(tor_addr_lookup); - /* * Case 8: * 1. Address is NULL From b239f178a22eb4c5b36e9e1490f237ac42f42449 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Mon, 13 Jul 2020 08:23:20 -0400 Subject: [PATCH 02/16] addr: New function to find address to publish In order for a relay to find which address to publish in its descriptor, router_pick_published_address() is used. However, that function only supports AF_INET and uses the directory server suggested address discovery mechanism. This new function uses a new interface so that the caller can request an address family and get the tor_addr_t object. Furthermore, it drops the use of directory servers address discovery (tor#33244) and instead uses the new suggested cache that is populated at the moment from data in the NETINFO cell coming from the directory authorities. At this commit, function is unused. Related to #40025 Signed-off-by: David Goulet --- src/feature/relay/relay_find_addr.c | 52 +++++++++++++++++++++++++++++ src/feature/relay/relay_find_addr.h | 3 ++ 2 files changed, 55 insertions(+) diff --git a/src/feature/relay/relay_find_addr.c b/src/feature/relay/relay_find_addr.c index 16d0a4733b..dabf985751 100644 --- a/src/feature/relay/relay_find_addr.c +++ b/src/feature/relay/relay_find_addr.c @@ -164,6 +164,58 @@ router_new_address_suggestion(const char *suggestion, } } +/** Find our address to be published in our descriptor. Three places are + * looked at: + * + * 1. Resolved cache. Populated by find_my_address() during the relay + * periodic event that attempts to learn if our address has changed. + * + * 2. If cache_only is false, attempt to find the address using the + * relay_find_addr.h interface. + * + * 3. Finally, if all fails, use the suggested address cache which is + * populated by the NETINFO cell values. + * + * 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. */ +bool +relay_find_addr_to_publish(const or_options_t *options, int family, + bool cache_only, tor_addr_t *addr_out) +{ + tor_assert(options); + tor_assert(addr_out); + + tor_addr_make_unspec(addr_out); + + /* 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); + if (!tor_addr_is_null(addr_out)) { + goto found; + } + + /* Second, attempt to find our address. The following can do a DNS resolve + * thus only do it when the no cache only flag is flipped. */ + if (!cache_only) { + if (find_my_address(options, family, LOG_INFO, addr_out, NULL, NULL)) { + goto found; + } + } + + /* Third, consider address from our suggestion cache. */ + resolved_addr_get_suggested(family, addr_out); + if (!tor_addr_is_null(addr_out)) { + goto found; + } + + /* No publishable address was found. */ + return false; + + found: + return true; +} + /** Make a current best guess at our address, either because * it's configured in torrc, or because we've learned it from * dirserver headers. Place the answer in *addr and return diff --git a/src/feature/relay/relay_find_addr.h b/src/feature/relay/relay_find_addr.h index 6f298e6c79..e28ceb933a 100644 --- a/src/feature/relay/relay_find_addr.h +++ b/src/feature/relay/relay_find_addr.h @@ -19,6 +19,9 @@ void relay_address_new_suggestion(const tor_addr_t *suggested_addr, const tor_addr_t *peer_addr, const char *identity_digest); +bool relay_find_addr_to_publish(const or_options_t *options, int family, + bool cache_only, tor_addr_t *addr_out); + #ifdef RELAY_FIND_ADDR_PRIVATE #endif /* RELAY_FIND_ADDR_PRIVATE */ From 9f61a6bdc3ba7d6ef623d9fce8486058c5cefdcd Mon Sep 17 00:00:00 2001 From: David Goulet Date: Tue, 14 Jul 2020 11:03:24 -0400 Subject: [PATCH 03/16] pt: Use new address discovery interface when creating extrainfo In case the transport has no usable address configured (likely 0.0.0.0 or [::]), attempt to find the IPv4 and on failure, fallback to the IPv6. If none are found, a log error is emitted and the transport is skiped. Related to #40025 Signed-off-by: David Goulet --- src/feature/client/transports.c | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/src/feature/client/transports.c b/src/feature/client/transports.c index 2bdc0ae151..ecc952e2ac 100644 --- a/src/feature/client/transports.c +++ b/src/feature/client/transports.c @@ -1643,17 +1643,25 @@ pt_get_extra_info_descriptor_string(void) SMARTLIST_FOREACH_BEGIN(mp->transports, const transport_t *, t) { char *transport_args = NULL; + const char *addrport = NULL; /* If the transport proxy returned "0.0.0.0" as its address, and * we know our external IP address, use it. Otherwise, use the * returned address. */ - const char *addrport = NULL; - uint32_t external_ip_address = 0; - if (tor_addr_is_null(&t->addr) && - router_pick_published_address(get_options(), - &external_ip_address, 0) >= 0) { + if (tor_addr_is_null(&t->addr)) { tor_addr_t addr; - tor_addr_from_ipv4h(&addr, external_ip_address); + /* Attempt to find the IPv4 and then attempt to find the IPv6 if we + * can't find it. */ + bool found = relay_find_addr_to_publish(get_options(), AF_INET, 0, + &addr); + if (!found) { + found = relay_find_addr_to_publish(get_options(), AF_INET6, 0, + &addr); + } + if (!found) { + log_err(LD_PT, "Unable to find address for transport %s", t->name); + continue; + } addrport = fmt_addrport(&addr, t->port); } else { addrport = fmt_addrport(&t->addr, t->port); From 502f3f5afe84d7b5ad02f8fc2262232bbd9beb13 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Tue, 14 Jul 2020 11:26:40 -0400 Subject: [PATCH 04/16] relay: Publish IPv4/IPv6 from resolved address cache When a relay builds a new descriptor, use the new relay_find_addr_to_publish() interface to find the address to publish per family. This commit also make the check for address consistency to also work for a configured IPv6 for which before it was IPv4 only. Related to #40025 Signed-off-by: David Goulet --- src/feature/relay/router.c | 121 ++++++++++++++++++++----------------- 1 file changed, 66 insertions(+), 55 deletions(-) diff --git a/src/feature/relay/router.c b/src/feature/relay/router.c index 4f4ba8559b..cebc44b1f5 100644 --- a/src/feature/relay/router.c +++ b/src/feature/relay/router.c @@ -1822,54 +1822,55 @@ router_get_descriptor_gen_reason(void) * ORPort or DirPort. * listener_type is either CONN_TYPE_OR_LISTENER or CONN_TYPE_DIR_LISTENER. */ static void -router_check_descriptor_address_port_consistency(uint32_t ipv4h_desc_addr, +router_check_descriptor_address_port_consistency(const tor_addr_t *addr, int listener_type) { + int family, port_cfg; + + tor_assert(addr); tor_assert(listener_type == CONN_TYPE_OR_LISTENER || listener_type == CONN_TYPE_DIR_LISTENER); - /* The first advertised Port may be the magic constant CFG_AUTO_PORT. - */ - int port_v4_cfg = get_first_advertised_port_by_type_af(listener_type, - AF_INET); - if (port_v4_cfg != 0 && - !port_exists_by_type_addr32h_port(listener_type, - ipv4h_desc_addr, port_v4_cfg, 1)) { - const tor_addr_t *port_addr = get_first_advertised_addr_by_type_af( - listener_type, - AF_INET); - /* If we're building a descriptor with no advertised address, - * something is terribly wrong. */ - tor_assert(port_addr); + family = tor_addr_family(addr); + /* The first advertised Port may be the magic constant CFG_AUTO_PORT. */ + port_cfg = get_first_advertised_port_by_type_af(listener_type, family); + if (port_cfg != 0 && + !port_exists_by_type_addr_port(listener_type, addr, port_cfg, 1)) { + const tor_addr_t *port_addr = + get_first_advertised_addr_by_type_af(listener_type, family); + /* If we're building a descriptor with no advertised address, + * something is terribly wrong. */ + tor_assert(port_addr); - tor_addr_t desc_addr; - char port_addr_str[TOR_ADDR_BUF_LEN]; - char desc_addr_str[TOR_ADDR_BUF_LEN]; + char port_addr_str[TOR_ADDR_BUF_LEN]; + char desc_addr_str[TOR_ADDR_BUF_LEN]; - tor_addr_to_str(port_addr_str, port_addr, TOR_ADDR_BUF_LEN, 0); + tor_addr_to_str(port_addr_str, port_addr, TOR_ADDR_BUF_LEN, 0); + tor_addr_to_str(desc_addr_str, addr, TOR_ADDR_BUF_LEN, 0); - tor_addr_from_ipv4h(&desc_addr, ipv4h_desc_addr); - tor_addr_to_str(desc_addr_str, &desc_addr, TOR_ADDR_BUF_LEN, 0); - - const char *listener_str = (listener_type == CONN_TYPE_OR_LISTENER ? - "OR" : "Dir"); - log_warn(LD_CONFIG, "The IPv4 %sPort address %s does not match the " - "descriptor address %s. If you have a static public IPv4 " - "address, use 'Address ' and 'OutboundBindAddress " - "'. If you are behind a NAT, use two %sPort lines: " - "'%sPort NoListen' and '%sPort " - "NoAdvertise'.", - listener_str, port_addr_str, desc_addr_str, listener_str, - listener_str, listener_str); - } + const char *listener_str = (listener_type == CONN_TYPE_OR_LISTENER ? + "OR" : "Dir"); + const char *af_str = fmt_af_family(family); + log_warn(LD_CONFIG, "The %s %sPort address %s does not match the " + "descriptor address %s. If you have a static public IPv4 " + "address, use 'Address <%s>' and 'OutboundBindAddress " + "<%s>'. If you are behind a NAT, use two %sPort lines: " + "'%sPort NoListen' and '%sPort " + "NoAdvertise'.", + af_str, listener_str, port_addr_str, desc_addr_str, af_str, + af_str, listener_str, listener_str, listener_str); + } } -/* Tor relays only have one IPv4 address in the descriptor, which is derived - * from the Address torrc option, or guessed using various methods in - * router_pick_published_address(). - * Warn the operator if there is no ORPort on the descriptor address - * ipv4h_desc_addr. +/** Tor relays only have one IPv4 or/and one IPv6 address in the descriptor, + * which is derived from the Address torrc option, or guessed using various + * methods in relay_find_addr_to_publish(). + * + * Warn the operator if there is no ORPort associated with the given address + * in addr. + * * Warn the operator if there is no DirPort on the descriptor address. + * * This catches a few common config errors: * - operators who expect ORPorts and DirPorts to be advertised on the * ports' listen addresses, rather than the torrc Address (or guessed @@ -1878,20 +1879,22 @@ router_check_descriptor_address_port_consistency(uint32_t ipv4h_desc_addr, * addresses; * - discrepancies between guessed addresses and configured listen * addresses (when the Address option isn't set). + * * If a listener is listening on all IPv4 addresses, it is assumed that it * is listening on the configured Address, and no messages are logged. + * * If an operators has specified NoAdvertise ORPorts in a NAT setting, * no messages are logged, unless they have specified other advertised * addresses. + * * The message tells operators to configure an ORPort and DirPort that match - * the Address (using NoListen if needed). - */ + * the Address (using NoListen if needed). */ static void -router_check_descriptor_address_consistency(uint32_t ipv4h_desc_addr) +router_check_descriptor_address_consistency(const tor_addr_t *addr) { - router_check_descriptor_address_port_consistency(ipv4h_desc_addr, + router_check_descriptor_address_port_consistency(addr, CONN_TYPE_OR_LISTENER); - router_check_descriptor_address_port_consistency(ipv4h_desc_addr, + router_check_descriptor_address_port_consistency(addr, CONN_TYPE_DIR_LISTENER); } @@ -2033,7 +2036,7 @@ MOCK_IMPL(STATIC int, router_build_fresh_unsigned_routerinfo,(routerinfo_t **ri_out)) { routerinfo_t *ri = NULL; - uint32_t addr; + tor_addr_t ipv4_addr, ipv6_addr; char platform[256]; int hibernating = we_are_hibernating(); const or_options_t *options = get_options(); @@ -2044,22 +2047,37 @@ router_build_fresh_unsigned_routerinfo,(routerinfo_t **ri_out)) goto err; } - if (router_pick_published_address(options, &addr, 0) < 0) { + /* Find our resolved address both IPv4 and IPv6. In case the address is not + * found, the object is set to an UNSPEC address. */ + bool have_v4 = relay_find_addr_to_publish(options, AF_INET, 0, &ipv4_addr); + bool have_v6 = relay_find_addr_to_publish(options, AF_INET6, 0, &ipv6_addr); + + /* 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"); result = TOR_ROUTERINFO_ERROR_NO_EXT_ADDR; goto err; } - /* 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(addr); + router_check_descriptor_address_consistency(&ipv4_addr); + router_check_descriptor_address_consistency(&ipv6_addr); ri = tor_malloc_zero(sizeof(routerinfo_t)); ri->cache_info.routerlist_index = -1; ri->nickname = tor_strdup(options->Nickname); - tor_addr_from_ipv4h(&ri->ipv4_addr, addr); - ri->ipv4_orport = router_get_advertised_or_port(options); + + /* IPv4. */ + tor_addr_copy(&ri->ipv4_addr, &ipv4_addr); + ri->ipv4_orport = router_get_advertised_or_port_by_af(options, AF_INET); ri->ipv4_dirport = router_get_advertised_dir_port(options, 0); + + /* IPv6. */ + if (have_v6) { + tor_addr_copy(&ri->ipv6_addr, &ipv6_addr); + ri->ipv6_orport = router_get_advertised_or_port_by_af(options, AF_INET6); + } + ri->supports_tunnelled_dir_requests = directory_permits_begindir_requests(options); ri->cache_info.published_on = time(NULL); @@ -2071,13 +2089,6 @@ router_build_fresh_unsigned_routerinfo,(routerinfo_t **ri_out)) tor_memdup(&get_current_curve25519_keypair()->pubkey, sizeof(curve25519_public_key_t)); - /* For now, at most one IPv6 or-address is being advertised. */ - tor_addr_port_t ipv6_orport; - router_get_advertised_ipv6_or_ap(options, &ipv6_orport); - /* If there is no valid IPv6 ORPort, the address and port are null. */ - tor_addr_copy(&ri->ipv6_addr, &ipv6_orport.addr); - ri->ipv6_orport = ipv6_orport.port; - ri->identity_pkey = crypto_pk_dup_key(get_server_identity_key()); if (BUG(crypto_pk_get_digest(ri->identity_pkey, ri->cache_info.identity_digest) < 0)) { From 35871e46e89ae0182703b2e20fe9e9c1e0f20cef Mon Sep 17 00:00:00 2001 From: David Goulet Date: Tue, 14 Jul 2020 12:49:14 -0400 Subject: [PATCH 05/16] relay: Don't lookup our address before rebuilding our descriptor Tor periodic events have moved to a role base model where relays have specific events. One of those is to rebuild the descriptor and that is ran every minute. This removes the call to router_rebuild_descriptor() from router_get_my_routerinfo_with_err() because that is the only code path that can call for a rebuild every second. Instead, when we mark the descriptor as dirty, immediately reschedule the descriptor check periodic event so it can be rebuilt that way instead of randomly when router_get_my_routerinfo_with_err() is called. Related to #40025 Signed-off-by: David Goulet --- src/feature/relay/router.c | 26 ++++---------------------- 1 file changed, 4 insertions(+), 22 deletions(-) diff --git a/src/feature/relay/router.c b/src/feature/relay/router.c index cebc44b1f5..2d79a9ba4d 100644 --- a/src/feature/relay/router.c +++ b/src/feature/relay/router.c @@ -38,6 +38,7 @@ #include "feature/relay/dns.h" #include "feature/relay/relay_config.h" #include "feature/relay/relay_find_addr.h" +#include "feature/relay/relay_periodic.h" #include "feature/relay/router.h" #include "feature/relay/routerkeys.h" #include "feature/relay/routermode.h" @@ -1757,16 +1758,6 @@ router_get_my_routerinfo_with_err,(int *err)) return NULL; } - if (!desc_clean_since) { - int rebuild_err = router_rebuild_descriptor(0); - if (rebuild_err < 0) { - if (err) - *err = rebuild_err; - - return NULL; - } - } - if (!desc_routerinfo) { if (err) *err = TOR_ROUTERINFO_ERROR_DESC_REBUILDING; @@ -2400,21 +2391,10 @@ router_rebuild_descriptor(int force) int err = 0; routerinfo_t *ri; extrainfo_t *ei; - uint32_t addr; - const or_options_t *options = get_options(); if (desc_clean_since && !force) return 0; - if (router_pick_published_address(options, &addr, 0) < 0 || - router_get_advertised_or_port(options) == 0) { - /* Stop trying to rebuild our descriptor every second. We'll - * learn that it's time to try again when ip_address_changed() - * marks it dirty. */ - desc_clean_since = time(NULL); - return TOR_ROUTERINFO_ERROR_DESC_REBUILDING; - } - log_info(LD_OR, "Rebuilding relay descriptor%s", force ? " (forced)" : ""); err = router_build_fresh_descriptor(&ri, &ei); @@ -2514,11 +2494,13 @@ mark_my_descriptor_dirty(const char *reason) if (BUG(reason == NULL)) { reason = "marked descriptor dirty for unspecified reason"; } - if (server_mode(options) && options->PublishServerDescriptor_) + if (server_mode(options) && options->PublishServerDescriptor_) { log_info(LD_OR, "Decided to publish new relay descriptor: %s", reason); + } desc_clean_since = 0; if (!desc_dirty_reason) desc_dirty_reason = reason; + reschedule_descriptor_update_check(); } /** How frequently will we republish our descriptor because of large (factor From 1a347b4790719d6ae1576c0b023dac343b65e5a1 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Tue, 14 Jul 2020 12:58:36 -0400 Subject: [PATCH 06/16] relay: Deciding to fetch from authority, use new find address API Use the new relay_has_address_set() interface when deciding if we need to fetch directory information from an authority as a relay. If no IPv4 address is found, we'll proceed with a fetch so we can learn our address in the HTTP header or NETINFO cell that a trusted authority will send us back. Related to #40025 Signed-off-by: David Goulet --- src/feature/dirclient/dirclient_modes.c | 12 ++++++++---- src/feature/relay/relay_find_addr.c | 11 +++++++++++ src/feature/relay/relay_find_addr.h | 2 ++ 3 files changed, 21 insertions(+), 4 deletions(-) diff --git a/src/feature/dirclient/dirclient_modes.c b/src/feature/dirclient/dirclient_modes.c index 31a3f8af58..62cdad6c36 100644 --- a/src/feature/dirclient/dirclient_modes.c +++ b/src/feature/dirclient/dirclient_modes.c @@ -40,15 +40,19 @@ int dirclient_fetches_from_authorities(const or_options_t *options) { const routerinfo_t *me; - uint32_t addr; int refuseunknown; if (options->FetchDirInfoEarly) return 1; if (options->BridgeRelay == 1) return 0; - if (server_mode(options) && - router_pick_published_address(options, &addr, 1) < 0) - return 1; /* we don't know our IP address; ask an authority. */ + /* We don't know our IP address; ask an authority. IPv4 is still mandatory + * to have thus if we don't have it, we ought to learn it from an authority + * through the NETINFO cell or the HTTP header it sends us back. + * + * Note that at the moment, relay do a direct connection so no NETINFO cell + * for now. */ + if (server_mode(options) && !relay_has_address_set(AF_INET)) + return 1; refuseunknown = ! router_my_exit_policy_is_reject_star() && should_refuse_unknown_exits(options); if (!dir_server_mode(options) && !refuseunknown) diff --git a/src/feature/relay/relay_find_addr.c b/src/feature/relay/relay_find_addr.c index dabf985751..d685145934 100644 --- a/src/feature/relay/relay_find_addr.c +++ b/src/feature/relay/relay_find_addr.c @@ -254,3 +254,14 @@ router_pick_published_address, (const or_options_t *options, uint32_t *addr, /* We have no useful cached answers. Return failure. */ return -1; } + +/** Return true iff this relay has an address set for the given family. + * + * This only checks the caches so it will not trigger a full discovery of the + * address. */ +bool +relay_has_address_set(int family) +{ + tor_addr_t addr; + return relay_find_addr_to_publish(get_options(), family, 1, &addr); +} diff --git a/src/feature/relay/relay_find_addr.h b/src/feature/relay/relay_find_addr.h index e28ceb933a..399ac8dc44 100644 --- a/src/feature/relay/relay_find_addr.h +++ b/src/feature/relay/relay_find_addr.h @@ -22,6 +22,8 @@ void relay_address_new_suggestion(const tor_addr_t *suggested_addr, bool relay_find_addr_to_publish(const or_options_t *options, int family, bool cache_only, tor_addr_t *addr_out); +bool relay_has_address_set(int family); + #ifdef RELAY_FIND_ADDR_PRIVATE #endif /* RELAY_FIND_ADDR_PRIVATE */ From 75a2e7fcb7092790c81db91861f526208612fa2a Mon Sep 17 00:00:00 2001 From: David Goulet Date: Tue, 14 Jul 2020 13:02:21 -0400 Subject: [PATCH 07/16] control: GETINFO address/ use new find address API At the moment, this command only returns the IPv4. Do so by using the new relay_find_addr_to_publish(). New commands to return IPv4 and IPv6 will be done with the work in tor#40039. Related to #40025 Signed-off-by: David Goulet --- src/feature/control/control_getinfo.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/feature/control/control_getinfo.c b/src/feature/control/control_getinfo.c index daf71f04c9..c489dfa865 100644 --- a/src/feature/control/control_getinfo.c +++ b/src/feature/control/control_getinfo.c @@ -132,12 +132,12 @@ getinfo_helper_misc(control_connection_t *conn, const char *question, } else if (!strcmp(question, "features/names")) { *answer = tor_strdup("VERBOSE_NAMES EXTENDED_EVENTS"); } else if (!strcmp(question, "address")) { - uint32_t addr; - if (router_pick_published_address(get_options(), &addr, 0) < 0) { + tor_addr_t addr; + if (!relay_find_addr_to_publish(get_options(), AF_INET, false, &addr)) { *errmsg = "Address unknown"; return -1; } - *answer = tor_dup_ip(addr); + *answer = tor_addr_to_str_dup(&addr); tor_assert_nonfatal(*answer); } else if (!strcmp(question, "traffic/read")) { tor_asprintf(answer, "%"PRIu64, (get_bytes_read())); From 4a41761101bc4bd3614ae309f436015b9c4799da Mon Sep 17 00:00:00 2001 From: David Goulet Date: Tue, 14 Jul 2020 13:07:53 -0400 Subject: [PATCH 08/16] test: Move unit tests to new find address interface Remove use of router_pick_published_address() and use relay_find_addr_to_publish instead. Related to #40025 Signed-off-by: David Goulet --- src/feature/relay/relay_find_addr.c | 6 +++--- src/feature/relay/relay_find_addr.h | 5 +++-- src/test/test_config.c | 33 +++++++++++++++-------------- 3 files changed, 23 insertions(+), 21 deletions(-) diff --git a/src/feature/relay/relay_find_addr.c b/src/feature/relay/relay_find_addr.c index d685145934..840896f23b 100644 --- a/src/feature/relay/relay_find_addr.c +++ b/src/feature/relay/relay_find_addr.c @@ -179,9 +179,9 @@ router_new_address_suggestion(const char *suggestion, * 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. */ -bool -relay_find_addr_to_publish(const or_options_t *options, int family, - bool cache_only, tor_addr_t *addr_out) +MOCK_IMPL(bool, +relay_find_addr_to_publish, (const or_options_t *options, int family, + bool cache_only, tor_addr_t *addr_out)) { tor_assert(options); tor_assert(addr_out); diff --git a/src/feature/relay/relay_find_addr.h b/src/feature/relay/relay_find_addr.h index 399ac8dc44..ad147686a5 100644 --- a/src/feature/relay/relay_find_addr.h +++ b/src/feature/relay/relay_find_addr.h @@ -19,8 +19,9 @@ void relay_address_new_suggestion(const tor_addr_t *suggested_addr, const tor_addr_t *peer_addr, const char *identity_digest); -bool relay_find_addr_to_publish(const or_options_t *options, int family, - bool cache_only, tor_addr_t *addr_out); +MOCK_DECL(bool, relay_find_addr_to_publish, + (const or_options_t *options, int family, bool cache_only, + tor_addr_t *addr_out)); bool relay_has_address_set(int family); diff --git a/src/test/test_config.c b/src/test/test_config.c index a50e6ac927..7496c7c57c 100644 --- a/src/test/test_config.c +++ b/src/test/test_config.c @@ -3842,16 +3842,17 @@ test_config_default_dir_servers(void *arg) or_options_free(opts); } -static int mock_router_pick_published_address_result = 0; +static bool mock_relay_find_addr_to_publish_result = true; -static int -mock_router_pick_published_address(const or_options_t *options, - uint32_t *addr, int cache_only) +static bool +mock_relay_find_addr_to_publish(const or_options_t *options, int family, + bool cache_only, tor_addr_t *addr_out) { - (void)options; - (void)addr; - (void)cache_only; - return mock_router_pick_published_address_result; + (void) options; + (void) family; + (void) cache_only; + (void) addr_out; + return mock_relay_find_addr_to_publish_result; } static int mock_router_my_exit_policy_is_reject_star_result = 0; @@ -3887,11 +3888,11 @@ test_config_directory_fetch(void *arg) or_options_t *options = options_new(); routerinfo_t routerinfo; memset(&routerinfo, 0, sizeof(routerinfo)); - mock_router_pick_published_address_result = -1; + mock_relay_find_addr_to_publish_result = false; mock_router_my_exit_policy_is_reject_star_result = 1; mock_advertised_server_mode_result = 0; mock_router_get_my_routerinfo_result = NULL; - MOCK(router_pick_published_address, mock_router_pick_published_address); + MOCK(relay_find_addr_to_publish, mock_relay_find_addr_to_publish); MOCK(router_my_exit_policy_is_reject_star, mock_router_my_exit_policy_is_reject_star); MOCK(advertised_server_mode, mock_advertised_server_mode); @@ -3947,14 +3948,14 @@ test_config_directory_fetch(void *arg) options = options_new(); options->ORPort_set = 1; - mock_router_pick_published_address_result = -1; + mock_relay_find_addr_to_publish_result = false; tt_assert(server_mode(options) == 1); tt_assert(public_server_mode(options) == 1); tt_int_op(dirclient_fetches_from_authorities(options), OP_EQ, 1); tt_int_op(networkstatus_consensus_can_use_multiple_directories(options), OP_EQ, 0); - mock_router_pick_published_address_result = 0; + mock_relay_find_addr_to_publish_result = true; tt_assert(server_mode(options) == 1); tt_assert(public_server_mode(options) == 1); tt_int_op(dirclient_fetches_from_authorities(options), OP_EQ, 0); @@ -3968,7 +3969,7 @@ test_config_directory_fetch(void *arg) options = options_new(); options->ORPort_set = 1; options->ExitRelay = 1; - mock_router_pick_published_address_result = 0; + mock_relay_find_addr_to_publish_result = true; mock_router_my_exit_policy_is_reject_star_result = 0; mock_advertised_server_mode_result = 1; mock_router_get_my_routerinfo_result = &routerinfo; @@ -3983,7 +3984,7 @@ test_config_directory_fetch(void *arg) OP_EQ, 0); options->RefuseUnknownExits = 0; - mock_router_pick_published_address_result = 0; + mock_relay_find_addr_to_publish_result = true; tt_assert(server_mode(options) == 1); tt_assert(public_server_mode(options) == 1); tt_int_op(dirclient_fetches_from_authorities(options), OP_EQ, 0); @@ -4000,7 +4001,7 @@ test_config_directory_fetch(void *arg) options->DirPort_set = 1; options->ORPort_set = 1; options->DirCache = 1; - mock_router_pick_published_address_result = 0; + mock_relay_find_addr_to_publish_result = true; mock_router_my_exit_policy_is_reject_star_result = 1; mock_advertised_server_mode_result = 1; @@ -4051,7 +4052,7 @@ test_config_directory_fetch(void *arg) done: or_options_free(options); - UNMOCK(router_pick_published_address); + UNMOCK(relay_find_addr_to_publish); UNMOCK(router_get_my_routerinfo); UNMOCK(advertised_server_mode); UNMOCK(router_my_exit_policy_is_reject_star); From 8178a34b800331cf2f7dc4cddcca6df3de0c829d Mon Sep 17 00:00:00 2001 From: David Goulet Date: Tue, 14 Jul 2020 13:09:26 -0400 Subject: [PATCH 09/16] relay: Remove router_pick_published_address() Unused at this commit. Closes #40025 Signed-off-by: David Goulet --- src/feature/relay/relay_find_addr.c | 53 ----------------------------- src/feature/relay/relay_find_addr.h | 3 -- 2 files changed, 56 deletions(-) diff --git a/src/feature/relay/relay_find_addr.c b/src/feature/relay/relay_find_addr.c index 840896f23b..f6cafe5315 100644 --- a/src/feature/relay/relay_find_addr.c +++ b/src/feature/relay/relay_find_addr.c @@ -24,20 +24,6 @@ * headers. */ static tor_addr_t last_guessed_ip = TOR_ADDR_NULL; -/** We failed to resolve our address locally, but we'd like to build - * a descriptor and publish / test reachability. If we have a guess - * about our address based on directory headers, answer it and return - * 0; else return -1. */ -static int -router_guess_address_from_dir_headers(uint32_t *guess) -{ - if (!tor_addr_is_null(&last_guessed_ip)) { - *guess = tor_addr_to_ipv4h(&last_guessed_ip); - return 0; - } - return -1; -} - /** Consider the address suggestion suggested_addr as a possible one to use as * our address. * @@ -216,45 +202,6 @@ relay_find_addr_to_publish, (const or_options_t *options, int family, return true; } -/** Make a current best guess at our address, either because - * it's configured in torrc, or because we've learned it from - * dirserver headers. Place the answer in *addr and return - * 0 on success, else return -1 if we have no guess. - * - * If cache_only is true, just return any cached answers, and - * don't try to get any new answers. - */ -MOCK_IMPL(int, -router_pick_published_address, (const or_options_t *options, uint32_t *addr, - int cache_only)) -{ - tor_addr_t last_resolved_addr; - - /* First, check the cached output from find_my_address(). */ - resolved_addr_get_last(AF_INET, &last_resolved_addr); - if (!tor_addr_is_null(&last_resolved_addr)) { - *addr = tor_addr_to_ipv4h(&last_resolved_addr); - return 0; - } - - /* Second, consider doing a resolve attempt right here. */ - if (!cache_only) { - tor_addr_t my_addr; - if (find_my_address(options, AF_INET, LOG_INFO, &my_addr, NULL, NULL)) { - log_info(LD_CONFIG,"Success: chose address '%s'.", fmt_addr(&my_addr)); - *addr = tor_addr_to_ipv4h(&my_addr); - return 0; - } - } - - /* Third, check the cached output from router_new_address_suggestion(). */ - if (router_guess_address_from_dir_headers(addr) >= 0) - return 0; - - /* We have no useful cached answers. Return failure. */ - return -1; -} - /** Return true iff this relay has an address set for the given family. * * This only checks the caches so it will not trigger a full discovery of the diff --git a/src/feature/relay/relay_find_addr.h b/src/feature/relay/relay_find_addr.h index ad147686a5..5ad9f0deb7 100644 --- a/src/feature/relay/relay_find_addr.h +++ b/src/feature/relay/relay_find_addr.h @@ -9,9 +9,6 @@ #ifndef TOR_RELAY_FIND_ADDR_H #define TOR_RELAY_FIND_ADDR_H -MOCK_DECL(int, router_pick_published_address, - (const or_options_t *options, uint32_t *addr, int cache_only)); - void router_new_address_suggestion(const char *suggestion, const dir_connection_t *d_conn); From c98cffbc07d65a1324ec2e2dcf84e1101b7a9b0c Mon Sep 17 00:00:00 2001 From: David Goulet Date: Tue, 14 Jul 2020 13:58:03 -0400 Subject: [PATCH 10/16] test: Unit test for relay_address_new_suggestion() Signed-off-by: David Goulet --- src/app/config/resolve_addr.c | 10 ++++ src/app/config/resolve_addr.h | 6 ++ src/test/test_relay.c | 103 ++++++++++++++++++++++++++++++++++ 3 files changed, 119 insertions(+) diff --git a/src/app/config/resolve_addr.c b/src/app/config/resolve_addr.c index 7368b019d6..df9af231bf 100644 --- a/src/app/config/resolve_addr.c +++ b/src/app/config/resolve_addr.c @@ -733,3 +733,13 @@ is_local_to_resolve_addr, (const tor_addr_t *addr)) return false; } } + +#ifdef TOR_UNIT_TESTS + +void +resolve_addr_reset_suggested(int family) +{ + tor_addr_make_unspec(&last_suggested_addrs[af_to_idx(family)]); +} + +#endif /* TOR_UNIT_TESTS */ diff --git a/src/app/config/resolve_addr.h b/src/app/config/resolve_addr.h index d9a3e28924..369f621fa9 100644 --- a/src/app/config/resolve_addr.h +++ b/src/app/config/resolve_addr.h @@ -33,6 +33,12 @@ MOCK_DECL(bool, is_local_to_resolve_addr, (const tor_addr_t *addr)); #ifdef RESOLVE_ADDR_PRIVATE +#ifdef TOR_UNIT_TESTS + +void resolve_addr_reset_suggested(int family); + +#endif /* TOR_UNIT_TESTS */ + #endif /* RESOLVE_ADDR_PRIVATE */ #endif /* TOR_CONFIG_RESOLVE_ADDR_H */ diff --git a/src/test/test_relay.c b/src/test/test_relay.c index 060ca1b75d..98b407feb3 100644 --- a/src/test/test_relay.c +++ b/src/test/test_relay.c @@ -17,6 +17,14 @@ #include "core/or/cell_st.h" #include "core/or/or_circuit_st.h" +#define RESOLVE_ADDR_PRIVATE +#include "feature/nodelist/dirlist.h" +#include "feature/relay/relay_find_addr.h" +#include "feature/relay/routermode.h" +#include "feature/dirclient/dir_server_st.h" + +#include "app/config/resolve_addr.h" + /* Test suite stuff */ #include "test/test.h" #include "test/fakechans.h" @@ -24,6 +32,13 @@ static void test_relay_append_cell_to_circuit_queue(void *arg); +static int +mock_server_mode_true(const or_options_t *options) +{ + (void) options; + return 1; +} + static void assert_circuit_ok_mock(const circuit_t *c) { @@ -192,10 +207,98 @@ test_relay_append_cell_to_circuit_queue(void *arg) return; } +static void +test_suggested_address(void *arg) +{ + int ret; + const char *untrusted_id = "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"; + dir_server_t *ds = NULL; + tor_addr_t ipv4_addr, ipv6_addr, cache_addr; + tor_addr_t trusted_addr, untrusted_addr; + tor_addr_port_t trusted_ap_v6 = { .port = 443 }; + + (void) arg; + + MOCK(server_mode, mock_server_mode_true); + + /* Unstrusted relay source. */ + ret = tor_addr_parse(&untrusted_addr, "8.8.8.8"); + tt_int_op(ret, OP_EQ, AF_INET); + + /* Add gabelmoo as a trusted directory authority. */ + ret = tor_addr_parse(&trusted_addr, "[2001:638:a000:4140::ffff:189]"); + tt_int_op(ret, OP_EQ, AF_INET6); + tor_addr_copy(&trusted_ap_v6.addr, &trusted_addr); + + ds = trusted_dir_server_new("gabelmoo", "131.188.40.189", 80, 443, + &trusted_ap_v6, + "F2044413DAC2E02E3D6BCF4735A19BCA1DE97281", + "ED03BB616EB2F60BEC80151114BB25CEF515B226", + V3_DIRINFO, 1.0); + tt_assert(ds); + dir_server_add(ds); + + /* 1. Valid IPv4 from a trusted authority (gabelmoo). */ + ret = tor_addr_parse(&ipv4_addr, "1.2.3.4"); + relay_address_new_suggestion(&ipv4_addr, &ds->ipv4_addr, ds->digest); + resolved_addr_get_suggested(AF_INET, &cache_addr); + tt_assert(tor_addr_eq(&cache_addr, &ipv4_addr)); + resolve_addr_reset_suggested(AF_INET); + + /* 2. Valid IPv6 from a trusted authority (gabelmoo). */ + ret = tor_addr_parse(&ipv6_addr, "[4242::4242]"); + relay_address_new_suggestion(&ipv6_addr, &ds->ipv6_addr, ds->digest); + resolved_addr_get_suggested(AF_INET6, &cache_addr); + tt_assert(tor_addr_eq(&cache_addr, &ipv6_addr)); + resolve_addr_reset_suggested(AF_INET6); + + /* 3. Valid IPv4 but untrusted source. */ + ret = tor_addr_parse(&ipv4_addr, "1.2.3.4"); + relay_address_new_suggestion(&ipv4_addr, &untrusted_addr, untrusted_id); + resolved_addr_get_suggested(AF_INET, &cache_addr); + tt_assert(tor_addr_is_unspec(&cache_addr)); + + /* 4. Valid IPv6 but untrusted source. */ + ret = tor_addr_parse(&ipv6_addr, "[4242::4242]"); + relay_address_new_suggestion(&ipv6_addr, &untrusted_addr, untrusted_id); + resolved_addr_get_suggested(AF_INET6, &cache_addr); + tt_assert(tor_addr_is_unspec(&cache_addr)); + + /* 5. Internal IPv4 from a trusted authority (gabelmoo). */ + ret = tor_addr_parse(&ipv4_addr, "127.0.0.1"); + relay_address_new_suggestion(&ipv4_addr, &ds->ipv4_addr, ds->digest); + resolved_addr_get_suggested(AF_INET, &cache_addr); + tt_assert(tor_addr_is_unspec(&cache_addr)); + + /* 6. Internal IPv6 from a trusted authority (gabelmoo). */ + ret = tor_addr_parse(&ipv6_addr, "[::1]"); + relay_address_new_suggestion(&ipv6_addr, &ds->ipv6_addr, ds->digest); + resolved_addr_get_suggested(AF_INET6, &cache_addr); + tt_assert(tor_addr_is_unspec(&cache_addr)); + + /* 7. IPv4 from a trusted authority (gabelmoo). */ + relay_address_new_suggestion(&ds->ipv4_addr, &ds->ipv4_addr, ds->digest); + resolved_addr_get_suggested(AF_INET, &cache_addr); + tt_assert(tor_addr_is_unspec(&cache_addr)); + + /* 8. IPv6 from a trusted authority (gabelmoo). */ + relay_address_new_suggestion(&ds->ipv6_addr, &ds->ipv6_addr, ds->digest); + resolved_addr_get_suggested(AF_INET6, &cache_addr); + tt_assert(tor_addr_is_unspec(&cache_addr)); + + done: + dirlist_free_all(); + + UNMOCK(server_mode); +} + struct testcase_t relay_tests[] = { { "append_cell_to_circuit_queue", test_relay_append_cell_to_circuit_queue, TT_FORK, NULL, NULL }, { "close_circ_rephist", test_relay_close_circuit, TT_FORK, NULL, NULL }, + { "suggested_address", test_suggested_address, + TT_FORK, NULL, NULL }, + END_OF_TESTCASES }; From a37a027e61bf96f233bf86dd33fa67890fb3c457 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Tue, 14 Jul 2020 14:10:16 -0400 Subject: [PATCH 11/16] test: Unit test for relay_find_addr_to_publish() Signed-off-by: David Goulet --- src/test/test_relay.c | 61 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 61 insertions(+) diff --git a/src/test/test_relay.c b/src/test/test_relay.c index 98b407feb3..af48eb5a44 100644 --- a/src/test/test_relay.c +++ b/src/test/test_relay.c @@ -292,6 +292,65 @@ test_suggested_address(void *arg) UNMOCK(server_mode); } +static void +test_find_addr_to_publish(void *arg) +{ + int family; + bool ret; + tor_addr_t ipv4_addr, ipv6_addr, cache_addr; + or_options_t options; + + (void) arg; + + /* Populate our resolved cache with a valid IPv4 and IPv6. */ + family = tor_addr_parse(&ipv4_addr, "1.2.3.4"); + tt_int_op(family, OP_EQ, AF_INET); + resolved_addr_set_last(&ipv4_addr, "NA", NULL); + resolved_addr_get_last(AF_INET, &cache_addr); + tt_assert(tor_addr_eq(&ipv4_addr, &cache_addr)); + + family = tor_addr_parse(&ipv6_addr, "[4242::4242]"); + tt_int_op(family, OP_EQ, AF_INET6); + resolved_addr_set_last(&ipv6_addr, "NA", NULL); + resolved_addr_get_last(AF_INET6, &cache_addr); + tt_assert(tor_addr_eq(&ipv6_addr, &cache_addr)); + + /* 1. Address located in the resolved cache. */ + ret = relay_find_addr_to_publish(&options, AF_INET, 1, &cache_addr); + tt_assert(ret); + tt_assert(tor_addr_eq(&ipv4_addr, &cache_addr)); + + ret = relay_find_addr_to_publish(&options, AF_INET6, 1, &cache_addr); + tt_assert(ret); + tt_assert(tor_addr_eq(&ipv6_addr, &cache_addr)); + resolved_addr_reset_last(AF_INET); + resolved_addr_reset_last(AF_INET6); + + /* 2. No IP in the resolve cache, go to the suggested cache. We will ignore + * 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, 1, &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, 1, &cache_addr); + tt_assert(ret); + tt_assert(tor_addr_eq(&ipv6_addr, &cache_addr)); + resolve_addr_reset_suggested(AF_INET); + resolve_addr_reset_suggested(AF_INET6); + + /* 3. No IP anywhere. */ + ret = relay_find_addr_to_publish(&options, AF_INET, 1, &cache_addr); + tt_assert(!ret); + ret = relay_find_addr_to_publish(&options, AF_INET6, 1, &cache_addr); + tt_assert(!ret); + + done: + ; +} + struct testcase_t relay_tests[] = { { "append_cell_to_circuit_queue", test_relay_append_cell_to_circuit_queue, TT_FORK, NULL, NULL }, @@ -299,6 +358,8 @@ struct testcase_t relay_tests[] = { TT_FORK, NULL, NULL }, { "suggested_address", test_suggested_address, TT_FORK, NULL, NULL }, + { "find_addr_to_publish", test_find_addr_to_publish, + TT_FORK, NULL, NULL }, END_OF_TESTCASES }; From 0b89eba7d55839c4b746bbe5efd8fe1531592bf7 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Thu, 16 Jul 2020 09:26:58 -0400 Subject: [PATCH 12/16] addr: Use false/true with relay_find_addr_to_publish() Previous development introduced the error of using 0/1 for a boolean parameter. Fix that everywhere Related #40025 Signed-off-by: David Goulet --- src/feature/client/transports.c | 4 ++-- src/feature/relay/router.c | 6 ++++-- src/test/test_relay.c | 12 ++++++------ 3 files changed, 12 insertions(+), 10 deletions(-) diff --git a/src/feature/client/transports.c b/src/feature/client/transports.c index ecc952e2ac..8a0694449a 100644 --- a/src/feature/client/transports.c +++ b/src/feature/client/transports.c @@ -1652,10 +1652,10 @@ pt_get_extra_info_descriptor_string(void) tor_addr_t addr; /* Attempt to find the IPv4 and then attempt to find the IPv6 if we * can't find it. */ - bool found = relay_find_addr_to_publish(get_options(), AF_INET, 0, + bool found = relay_find_addr_to_publish(get_options(), AF_INET, false, &addr); if (!found) { - found = relay_find_addr_to_publish(get_options(), AF_INET6, 0, + found = relay_find_addr_to_publish(get_options(), AF_INET6, false, &addr); } if (!found) { diff --git a/src/feature/relay/router.c b/src/feature/relay/router.c index 2d79a9ba4d..039aeb7343 100644 --- a/src/feature/relay/router.c +++ b/src/feature/relay/router.c @@ -2040,8 +2040,10 @@ router_build_fresh_unsigned_routerinfo,(routerinfo_t **ri_out)) /* Find our resolved address both IPv4 and IPv6. In case the address is not * found, the object is set to an UNSPEC address. */ - bool have_v4 = relay_find_addr_to_publish(options, AF_INET, 0, &ipv4_addr); - bool have_v6 = relay_find_addr_to_publish(options, AF_INET6, 0, &ipv6_addr); + bool have_v4 = relay_find_addr_to_publish(options, AF_INET, false, + &ipv4_addr); + bool have_v6 = relay_find_addr_to_publish(options, AF_INET6, false, + &ipv6_addr); /* Tor requires a relay to have an IPv4 so bail if we can't find it. */ if (!have_v4) { diff --git a/src/test/test_relay.c b/src/test/test_relay.c index af48eb5a44..5ea35b6ad0 100644 --- a/src/test/test_relay.c +++ b/src/test/test_relay.c @@ -316,11 +316,11 @@ test_find_addr_to_publish(void *arg) tt_assert(tor_addr_eq(&ipv6_addr, &cache_addr)); /* 1. Address located in the resolved cache. */ - ret = relay_find_addr_to_publish(&options, AF_INET, 1, &cache_addr); + ret = relay_find_addr_to_publish(&options, AF_INET, true, &cache_addr); tt_assert(ret); tt_assert(tor_addr_eq(&ipv4_addr, &cache_addr)); - ret = relay_find_addr_to_publish(&options, AF_INET6, 1, &cache_addr); + ret = relay_find_addr_to_publish(&options, AF_INET6, true, &cache_addr); tt_assert(ret); tt_assert(tor_addr_eq(&ipv6_addr, &cache_addr)); resolved_addr_reset_last(AF_INET); @@ -330,21 +330,21 @@ 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, 1, &cache_addr); + ret = relay_find_addr_to_publish(&options, AF_INET, true, &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, 1, &cache_addr); + ret = relay_find_addr_to_publish(&options, AF_INET6, true, &cache_addr); tt_assert(ret); tt_assert(tor_addr_eq(&ipv6_addr, &cache_addr)); resolve_addr_reset_suggested(AF_INET); resolve_addr_reset_suggested(AF_INET6); /* 3. No IP anywhere. */ - ret = relay_find_addr_to_publish(&options, AF_INET, 1, &cache_addr); + ret = relay_find_addr_to_publish(&options, AF_INET, true, &cache_addr); tt_assert(!ret); - ret = relay_find_addr_to_publish(&options, AF_INET6, 1, &cache_addr); + ret = relay_find_addr_to_publish(&options, AF_INET6, true, &cache_addr); tt_assert(!ret); done: From 433a1949e87190f1732ea426ed359c93f28cb578 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Fri, 17 Jul 2020 10:34:30 -0400 Subject: [PATCH 13/16] relay: Handle dir address suggestion with new interface We now use relay_address_new_suggestion() when a suggested address is received from a directory. Signed-off-by: David Goulet --- src/feature/dirclient/dirclient.c | 8 ++- src/feature/relay/relay_find_addr.c | 84 +++-------------------------- src/feature/relay/relay_find_addr.h | 3 -- 3 files changed, 14 insertions(+), 81 deletions(-) diff --git a/src/feature/dirclient/dirclient.c b/src/feature/dirclient/dirclient.c index bc4162cf52..7a026d3c52 100644 --- a/src/feature/dirclient/dirclient.c +++ b/src/feature/dirclient/dirclient.c @@ -2106,7 +2106,13 @@ connection_dir_client_reached_eof(dir_connection_t *conn) if (conn->dirconn_direct) { char *guess = http_get_header(headers, X_ADDRESS_HEADER); if (guess) { - router_new_address_suggestion(guess, conn); + tor_addr_t addr; + if (tor_addr_parse(&addr, guess) < 0) { + log_debug(LD_DIR, "Malformed X-Your-Address-Is header %s. Ignoring.", + escaped(guess)); + } else { + relay_address_new_suggestion(&addr, &TO_CONN(conn)->addr, NULL); + } tor_free(guess); } } diff --git a/src/feature/relay/relay_find_addr.c b/src/feature/relay/relay_find_addr.c index f6cafe5315..9a279d2277 100644 --- a/src/feature/relay/relay_find_addr.c +++ b/src/feature/relay/relay_find_addr.c @@ -20,15 +20,12 @@ #include "feature/relay/router.h" #include "feature/relay/routermode.h" -/** The most recently guessed value of our IP address, based on directory - * headers. */ -static tor_addr_t last_guessed_ip = TOR_ADDR_NULL; - /** Consider the address suggestion suggested_addr as a possible one to use as * our address. * - * This is called when a valid NETINFO cell is recevied containing a candidate - * for our address. + * This is called when a valid NETINFO cell is received containing a candidate + * for our address or when a directory sends us back the X-Your-Address-Is + * header. * * The suggested address is ignored if it does NOT come from a trusted source. * At the moment, we only look a trusted directory authorities. @@ -37,6 +34,9 @@ static tor_addr_t last_guessed_ip = TOR_ADDR_NULL; * given peer_addr which is the address from the endpoint that sent the * NETINFO cell. * + * The identity_digest is NULL if this is an address suggested by a directory + * since this is a plaintext connection. + * * The suggested address is set in our suggested address cache if everything * passes. */ void @@ -48,7 +48,6 @@ relay_address_new_suggestion(const tor_addr_t *suggested_addr, tor_assert(suggested_addr); tor_assert(peer_addr); - tor_assert(identity_digest); /* Non server should just ignore this suggestion. Clients don't need to * learn their address let alone cache it. */ @@ -59,7 +58,7 @@ relay_address_new_suggestion(const tor_addr_t *suggested_addr, /* Is the peer a trusted source? Ignore anything coming from non trusted * source. In this case, we only look at trusted directory authorities. */ if (!router_addr_is_trusted_dir(peer_addr) || - !router_digest_is_trusted_dir(identity_digest)) { + (identity_digest && !router_digest_is_trusted_dir(identity_digest))) { return; } @@ -81,75 +80,6 @@ relay_address_new_suggestion(const tor_addr_t *suggested_addr, resolved_addr_set_suggested(suggested_addr); } -/** A directory server d_conn told us our IP address is - * suggestion. - * If this address is different from the one we think we are now, and - * if our computer doesn't actually know its IP address, then switch. */ -void -router_new_address_suggestion(const char *suggestion, - const dir_connection_t *d_conn) -{ - tor_addr_t addr, my_addr, last_resolved_addr; - const or_options_t *options = get_options(); - - /* first, learn what the IP address actually is */ - if (tor_addr_parse(&addr, suggestion) == -1) { - log_debug(LD_DIR, "Malformed X-Your-Address-Is header %s. Ignoring.", - escaped(suggestion)); - return; - } - - log_debug(LD_DIR, "Got X-Your-Address-Is: %s.", suggestion); - - if (!server_mode(options)) { - tor_addr_copy(&last_guessed_ip, &addr); - return; - } - - /* XXXX ipv6 */ - resolved_addr_get_last(AF_INET, &last_resolved_addr); - if (!tor_addr_is_null(&last_resolved_addr)) { - /* Lets use this one. */ - tor_addr_copy(&last_guessed_ip, &last_resolved_addr); - return; - } - - /* Attempt to find our address. */ - if (find_my_address(options, AF_INET, LOG_INFO, &my_addr, NULL, NULL)) { - /* We're all set -- we already know our address. Great. */ - tor_addr_copy(&last_guessed_ip, &my_addr); /* store it in case we - need it later */ - return; - } - - /* Consider the suggestion from the directory. */ - if (tor_addr_is_internal(&addr, 0)) { - /* Don't believe anybody who says our IP is, say, 127.0.0.1. */ - return; - } - if (tor_addr_eq(&d_conn->base_.addr, &addr)) { - /* Don't believe anybody who says our IP is their IP. */ - log_debug(LD_DIR, "A directory server told us our IP address is %s, " - "but they are just reporting their own IP address. Ignoring.", - suggestion); - return; - } - - /* Okay. We can't resolve our own address, and X-Your-Address-Is is giving - * us an answer different from what we had the last time we managed to - * resolve it. */ - if (!tor_addr_eq(&last_guessed_ip, &addr)) { - control_event_server_status(LOG_NOTICE, - "EXTERNAL_ADDRESS ADDRESS=%s METHOD=DIRSERV", - suggestion); - log_addr_has_changed(LOG_NOTICE, &last_guessed_ip, &addr, - d_conn->base_.address); - ip_address_changed(0); - tor_addr_copy(&last_guessed_ip, &addr); /* router_rebuild_descriptor() - will fetch it */ - } -} - /** Find our address to be published in our descriptor. Three places are * looked at: * diff --git a/src/feature/relay/relay_find_addr.h b/src/feature/relay/relay_find_addr.h index 5ad9f0deb7..294ae4db57 100644 --- a/src/feature/relay/relay_find_addr.h +++ b/src/feature/relay/relay_find_addr.h @@ -9,9 +9,6 @@ #ifndef TOR_RELAY_FIND_ADDR_H #define TOR_RELAY_FIND_ADDR_H -void router_new_address_suggestion(const char *suggestion, - const dir_connection_t *d_conn); - void relay_address_new_suggestion(const tor_addr_t *suggested_addr, const tor_addr_t *peer_addr, const char *identity_digest); From 230293c169f73923b290598a1df4872acf1dee68 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Fri, 17 Jul 2020 10:56:22 -0400 Subject: [PATCH 14/16] control: With GETINFO, don't trigger an address resolve Tell the relay find address interface to only use the cache so we don't trigger an address resolve everytime the "GETINFO address" is called. Related #40025 Signed-off-by: David Goulet --- src/feature/control/control_getinfo.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/feature/control/control_getinfo.c b/src/feature/control/control_getinfo.c index c489dfa865..4ac4f48a86 100644 --- a/src/feature/control/control_getinfo.c +++ b/src/feature/control/control_getinfo.c @@ -133,7 +133,7 @@ getinfo_helper_misc(control_connection_t *conn, const char *question, *answer = tor_strdup("VERBOSE_NAMES EXTENDED_EVENTS"); } else if (!strcmp(question, "address")) { tor_addr_t addr; - if (!relay_find_addr_to_publish(get_options(), AF_INET, false, &addr)) { + if (!relay_find_addr_to_publish(get_options(), AF_INET, true, &addr)) { *errmsg = "Address unknown"; return -1; } From 75434a1df18e3007c286ce48a1e981a4e96e3f82 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Fri, 17 Jul 2020 11:10:56 -0400 Subject: [PATCH 15/16] relay: Use flags in relay_find_addr_to_publish() Instead of a boolean saying "cache_only" add the concept of flags so we add semantic through out the code and allow ourselves to have more options in the future. Signed-off-by: David Goulet --- src/feature/client/transports.c | 7 ++++--- src/feature/control/control_getinfo.c | 3 ++- src/feature/relay/relay_find_addr.c | 14 ++++++++------ src/feature/relay/relay_find_addr.h | 7 ++++++- src/feature/relay/router.c | 6 ++++-- src/test/test_config.c | 4 ++-- src/test/test_relay.c | 18 ++++++++++++------ 7 files changed, 38 insertions(+), 21 deletions(-) diff --git a/src/feature/client/transports.c b/src/feature/client/transports.c index 8a0694449a..2eb05d6a67 100644 --- a/src/feature/client/transports.c +++ b/src/feature/client/transports.c @@ -1652,11 +1652,12 @@ pt_get_extra_info_descriptor_string(void) tor_addr_t addr; /* Attempt to find the IPv4 and then attempt to find the IPv6 if we * can't find it. */ - bool found = relay_find_addr_to_publish(get_options(), AF_INET, false, + bool found = relay_find_addr_to_publish(get_options(), AF_INET, + RELAY_FIND_ADDR_NO_FLAG, &addr); if (!found) { - found = relay_find_addr_to_publish(get_options(), AF_INET6, false, - &addr); + found = relay_find_addr_to_publish(get_options(), AF_INET6, + RELAY_FIND_ADDR_NO_FLAG, &addr); } if (!found) { log_err(LD_PT, "Unable to find address for transport %s", t->name); diff --git a/src/feature/control/control_getinfo.c b/src/feature/control/control_getinfo.c index 4ac4f48a86..3e4feadded 100644 --- a/src/feature/control/control_getinfo.c +++ b/src/feature/control/control_getinfo.c @@ -133,7 +133,8 @@ getinfo_helper_misc(control_connection_t *conn, const char *question, *answer = tor_strdup("VERBOSE_NAMES EXTENDED_EVENTS"); } else if (!strcmp(question, "address")) { tor_addr_t addr; - if (!relay_find_addr_to_publish(get_options(), AF_INET, true, &addr)) { + if (!relay_find_addr_to_publish(get_options(), AF_INET, + RELAY_FIND_ADDR_CACHE_ONLY, &addr)) { *errmsg = "Address unknown"; return -1; } diff --git a/src/feature/relay/relay_find_addr.c b/src/feature/relay/relay_find_addr.c index 9a279d2277..48f28b182a 100644 --- a/src/feature/relay/relay_find_addr.c +++ b/src/feature/relay/relay_find_addr.c @@ -86,18 +86,19 @@ relay_address_new_suggestion(const tor_addr_t *suggested_addr, * 1. Resolved cache. Populated by find_my_address() during the relay * periodic event that attempts to learn if our address has changed. * - * 2. If cache_only is false, attempt to find the address using the - * relay_find_addr.h interface. + * 2. If flags is set with RELAY_FIND_ADDR_CACHE_ONLY, only the resolved + * and suggested cache are looked at. No address discovery will be done. * * 3. Finally, if all fails, use the suggested address cache which is - * populated by the NETINFO cell values. + * populated by the NETINFO cell content or HTTP header from a + * directory. * * 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. */ MOCK_IMPL(bool, relay_find_addr_to_publish, (const or_options_t *options, int family, - bool cache_only, tor_addr_t *addr_out)) + int flags, tor_addr_t *addr_out)) { tor_assert(options); tor_assert(addr_out); @@ -113,7 +114,7 @@ relay_find_addr_to_publish, (const or_options_t *options, int family, /* Second, attempt to find our address. The following can do a DNS resolve * thus only do it when the no cache only flag is flipped. */ - if (!cache_only) { + if (!(flags & RELAY_FIND_ADDR_CACHE_ONLY)) { if (find_my_address(options, family, LOG_INFO, addr_out, NULL, NULL)) { goto found; } @@ -140,5 +141,6 @@ bool relay_has_address_set(int family) { tor_addr_t addr; - return relay_find_addr_to_publish(get_options(), family, 1, &addr); + return relay_find_addr_to_publish(get_options(), family, + RELAY_FIND_ADDR_CACHE_ONLY, &addr); } diff --git a/src/feature/relay/relay_find_addr.h b/src/feature/relay/relay_find_addr.h index 294ae4db57..3d30946b05 100644 --- a/src/feature/relay/relay_find_addr.h +++ b/src/feature/relay/relay_find_addr.h @@ -9,12 +9,17 @@ #ifndef TOR_RELAY_FIND_ADDR_H #define TOR_RELAY_FIND_ADDR_H +typedef enum { + RELAY_FIND_ADDR_NO_FLAG = (1U << 0), + RELAY_FIND_ADDR_CACHE_ONLY = (1U << 1), +} relay_find_addr_flags_t; + void relay_address_new_suggestion(const tor_addr_t *suggested_addr, const tor_addr_t *peer_addr, const char *identity_digest); MOCK_DECL(bool, relay_find_addr_to_publish, - (const or_options_t *options, int family, bool cache_only, + (const or_options_t *options, int family, int flags, tor_addr_t *addr_out)); bool relay_has_address_set(int family); diff --git a/src/feature/relay/router.c b/src/feature/relay/router.c index 039aeb7343..b75241160f 100644 --- a/src/feature/relay/router.c +++ b/src/feature/relay/router.c @@ -2040,9 +2040,11 @@ router_build_fresh_unsigned_routerinfo,(routerinfo_t **ri_out)) /* Find our resolved address both IPv4 and IPv6. In case the address is not * found, the object is set to an UNSPEC address. */ - bool have_v4 = relay_find_addr_to_publish(options, AF_INET, false, + 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, false, + 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. */ diff --git a/src/test/test_config.c b/src/test/test_config.c index 7496c7c57c..71b2cdf2f4 100644 --- a/src/test/test_config.c +++ b/src/test/test_config.c @@ -3846,11 +3846,11 @@ static bool mock_relay_find_addr_to_publish_result = true; static bool mock_relay_find_addr_to_publish(const or_options_t *options, int family, - bool cache_only, tor_addr_t *addr_out) + int flags, tor_addr_t *addr_out) { (void) options; (void) family; - (void) cache_only; + (void) flags; (void) addr_out; return mock_relay_find_addr_to_publish_result; } diff --git a/src/test/test_relay.c b/src/test/test_relay.c index 5ea35b6ad0..60db98aec3 100644 --- a/src/test/test_relay.c +++ b/src/test/test_relay.c @@ -316,11 +316,13 @@ test_find_addr_to_publish(void *arg) tt_assert(tor_addr_eq(&ipv6_addr, &cache_addr)); /* 1. Address located in the resolved cache. */ - ret = relay_find_addr_to_publish(&options, AF_INET, true, &cache_addr); + 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, true, &cache_addr); + 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)); resolved_addr_reset_last(AF_INET); @@ -330,21 +332,25 @@ 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, true, &cache_addr); + 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, true, &cache_addr); + 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)); resolve_addr_reset_suggested(AF_INET); resolve_addr_reset_suggested(AF_INET6); /* 3. No IP anywhere. */ - ret = relay_find_addr_to_publish(&options, AF_INET, true, &cache_addr); + 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, true, &cache_addr); + ret = relay_find_addr_to_publish(&options, AF_INET6, + RELAY_FIND_ADDR_CACHE_ONLY, &cache_addr); tt_assert(!ret); done: From a576f37cfeff9f6b0a13a9475b5eed424ee44db9 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Mon, 20 Jul 2020 10:58:54 -0400 Subject: [PATCH 16/16] relay: Don't log at warn level when we find an address Dirauth code use the warn log severity when calling find_my_address() which made it that every time we would find an address, it would log a warning. These are not needed below info level and thus set them to info level. An IP change is set to notice by default. Signed-off-by: David Goulet --- src/app/config/resolve_addr.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/app/config/resolve_addr.c b/src/app/config/resolve_addr.c index df9af231bf..ba1c854d77 100644 --- a/src/app/config/resolve_addr.c +++ b/src/app/config/resolve_addr.c @@ -287,8 +287,8 @@ get_address_from_config(const or_options_t *options, int warn_severity, } /* Address can be used. We are done. */ - log_fn(warn_severity, LD_CONFIG, "Address found in configuration: %s", - fmt_addr(addr_out)); + log_info(LD_CONFIG, "Address found in configuration: %s", + fmt_addr(addr_out)); return FN_RET_OK; } @@ -350,8 +350,8 @@ get_address_from_hostname(const or_options_t *options, int warn_severity, *hostname_out = tor_strdup(hostname); /* Found it! */ - log_fn(warn_severity, LD_CONFIG, "Address found from local hostname: %s", - fmt_addr(addr_out)); + log_info(LD_CONFIG, "Address found from local hostname: %s", + fmt_addr(addr_out)); return FN_RET_OK; } @@ -402,8 +402,7 @@ get_address_from_interface(const or_options_t *options, int warn_severity, *method_out = "INTERFACE"; /* Found it! */ - log_fn(warn_severity, LD_CONFIG, "Address found from interface: %s", - fmt_addr(addr_out)); + log_info(LD_CONFIG, "Address found from interface: %s", fmt_addr(addr_out)); return FN_RET_OK; }