From 938623004b025c5c85745703d817e33a5308da5a Mon Sep 17 00:00:00 2001 From: David Goulet Date: Tue, 19 Jan 2021 12:24:19 -0500 Subject: [PATCH 1/2] relay: Keep all ORPorts that are on different ports We used to actually discard ORPorts that were the same port and same family but they could have different address. Instead, we need to keep all different ORPorts so we can bind a listener on each of them. We will publish only one of these in our descriptor though. Related to #40246 Signed-off-by: David Goulet --- src/feature/relay/relay_config.c | 25 ++++++--- src/feature/relay/relay_config.h | 6 +++ src/test/test_config.c | 89 +++++++++++++++++++++++++++++++- 3 files changed, 113 insertions(+), 7 deletions(-) diff --git a/src/feature/relay/relay_config.c b/src/feature/relay/relay_config.c index e8c29fa7ed..e007a3bd0d 100644 --- a/src/feature/relay/relay_config.c +++ b/src/feature/relay/relay_config.c @@ -151,7 +151,7 @@ describe_portnum(int port) /** Return a static buffer containing the human readable logging string that * describes the given port object. */ -static const char * +STATIC const char * describe_relay_port(const port_cfg_t *port) { IF_BUG_ONCE(!port) { @@ -198,6 +198,16 @@ describe_relay_port(const port_cfg_t *port) * First one binds to both v4 and v6 address but second one is specific to an * address superseding the global bind one. * + * Another example is this one: + * + * ORPort 9001 + * ORPort [4242::1]:9002 + * ORPort [4242::2]:9003 + * + * In this case, all IPv4 and IPv6 are kept since we do allow multiple ORPorts + * but the published port will be the first explicit one if any to be + * published or else the implicit. + * * The following is O(n^2) but it is done at bootstrap or config reload and * the list is not very long usually. */ STATIC void @@ -231,11 +241,14 @@ remove_duplicate_orports(smartlist_t *ports) if (next->type != CONN_TYPE_OR_LISTENER) { continue; } - /* Same address family and same port number, we have a match. */ - if (tor_addr_family(¤t->addr) == tor_addr_family(&next->addr) && - current->port == next->port) { - /* Remove current because next is explicitly set. */ - removing[i] = true; + /* Don't compare addresses of different family. */ + if (tor_addr_family(¤t->addr) != tor_addr_family(&next->addr)) { + continue; + } + + /* Same port, we keep the explicit one. */ + if (current->port == next->port) { + removing[j] = true; if (!current->explicit_addr && next->explicit_addr) { char *next_str = tor_strdup(describe_relay_port(next)); log_warn(LD_CONFIG, "Configuration port %s superseded by %s", diff --git a/src/feature/relay/relay_config.h b/src/feature/relay/relay_config.h index 671399ac0a..d36863a1a1 100644 --- a/src/feature/relay/relay_config.h +++ b/src/feature/relay/relay_config.h @@ -88,6 +88,12 @@ STATIC void remove_duplicate_orports(struct smartlist_t *ports); STATIC int check_bridge_distribution_setting(const char *bd); STATIC int have_enough_mem_for_dircache(const struct or_options_t *options, size_t total_mem, char **msg); +#ifdef TOR_UNIT_TESTS + +struct port_cfg_t; +STATIC const char *describe_relay_port(const struct port_cfg_t *port); + +#endif /* TOR_UNIT_TESTS */ #endif /* defined(RELAY_CONFIG_PRIVATE) */ diff --git a/src/test/test_config.c b/src/test/test_config.c index 49d7b87410..4eb4ac9cf5 100644 --- a/src/test/test_config.c +++ b/src/test/test_config.c @@ -6868,8 +6868,95 @@ test_config_duplicate_orports(void *arg) /* This will remove the [::] and the extra [::1]. */ remove_duplicate_orports(ports); - // The explicit IPv6 port should have replaced the implicit IPv6 port. tt_int_op(smartlist_len(ports), OP_EQ, 2); + tt_str_op(describe_relay_port(smartlist_get(ports, 0)), OP_EQ, + "ORPort 9050"); + tt_str_op(describe_relay_port(smartlist_get(ports, 1)), OP_EQ, + "ORPort [::1]:9050"); + + /* Reset. Test different ORPort value. */ + SMARTLIST_FOREACH(ports, port_cfg_t *, p, port_cfg_free(p)); + smartlist_free(ports); + config_free_lines(config_port); + config_port = NULL; + ports = smartlist_new(); + + /* Implicit port and then specific IPv6 addresses but more than one. */ + config_line_append(&config_port, "ORPort", "9050"); // two implicit entries. + config_line_append(&config_port, "ORPort", "[4242::1]:9051"); + config_line_append(&config_port, "ORPort", "[4242::2]:9051"); + + // Parse IPv4, then IPv6. + port_parse_config(ports, config_port, "OR", CONN_TYPE_OR_LISTENER, "0.0.0.0", + 0, CL_PORT_SERVER_OPTIONS); + port_parse_config(ports, config_port, "OR", CONN_TYPE_OR_LISTENER, "[::]", + 0, CL_PORT_SERVER_OPTIONS); + + /* There should be 6 ports at this point that is: + * - 0.0.0.0:9050 + * - [::]:9050 + * - [4242::1]:9051 + * - [4242::1]:9051 + * - [4242::2]:9051 + * - [4242::2]:9051 + */ + tt_int_op(smartlist_len(ports), OP_EQ, 6); + + /* This will remove the [::] and the duplicates. */ + remove_duplicate_orports(ports); + + /* We have four address here, 1 IPv4 on 9050, IPv6 on 9050, IPv6 on 9051 and + * a different IPv6 on 9051. */ + tt_int_op(smartlist_len(ports), OP_EQ, 3); + tt_str_op(describe_relay_port(smartlist_get(ports, 0)), OP_EQ, + "ORPort 9050"); + tt_str_op(describe_relay_port(smartlist_get(ports, 1)), OP_EQ, + "ORPort [4242::1]:9051"); + tt_str_op(describe_relay_port(smartlist_get(ports, 2)), OP_EQ, + "ORPort 9050"); + + /* Reset. Test different ORPort value. */ + SMARTLIST_FOREACH(ports, port_cfg_t *, p, port_cfg_free(p)); + smartlist_free(ports); + config_free_lines(config_port); + config_port = NULL; + ports = smartlist_new(); + + /* Three different ports. */ + config_line_append(&config_port, "ORPort", "9050"); // two implicit entries. + config_line_append(&config_port, "ORPort", "[4242::1]:9051"); + config_line_append(&config_port, "ORPort", "[4242::2]:9052"); + + // Parse IPv4, then IPv6. + port_parse_config(ports, config_port, "OR", CONN_TYPE_OR_LISTENER, "0.0.0.0", + 0, CL_PORT_SERVER_OPTIONS); + port_parse_config(ports, config_port, "OR", CONN_TYPE_OR_LISTENER, "[::]", + 0, CL_PORT_SERVER_OPTIONS); + + /* There should be 6 ports at this point that is: + * - 0.0.0.0:9050 + * - [::]:9050 + * - [4242::1]:9051 + * - [4242::1]:9051 + * - [4242::2]:9052 + * - [4242::2]:9052 + */ + tt_int_op(smartlist_len(ports), OP_EQ, 6); + + /* This will remove the [::] and the duplicates. */ + remove_duplicate_orports(ports); + + /* We have four address here, 1 IPv4 on 9050, IPv6 on 9050, IPv6 on 9051 and + * IPv6 on 9052. */ + tt_int_op(smartlist_len(ports), OP_EQ, 4); + tt_str_op(describe_relay_port(smartlist_get(ports, 0)), OP_EQ, + "ORPort 9050"); + tt_str_op(describe_relay_port(smartlist_get(ports, 1)), OP_EQ, + "ORPort [4242::1]:9051"); + tt_str_op(describe_relay_port(smartlist_get(ports, 2)), OP_EQ, + "ORPort [4242::2]:9052"); + tt_str_op(describe_relay_port(smartlist_get(ports, 3)), OP_EQ, + "ORPort 9050"); done: SMARTLIST_FOREACH(ports,port_cfg_t *,pf,port_cfg_free(pf)); From 9321ddf3a1cae9e8f0e59b368ff3c57aed5ac1a8 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Tue, 19 Jan 2021 12:36:44 -0500 Subject: [PATCH 2/2] config: Prioritize port with explicit address When selecting the first advertised port, we always prefer the one with an explicit address. Closes #40246 Signed-off-by: David Goulet --- src/app/config/config.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/src/app/config/config.c b/src/app/config/config.c index d8bc5f6025..7db5e5cfa8 100644 --- a/src/app/config/config.c +++ b/src/app/config/config.c @@ -6651,20 +6651,28 @@ get_first_listener_addrport_string(int listener_type) static const port_cfg_t * portconf_get_first_advertised(int listener_type, int address_family) { + const port_cfg_t *first_port = NULL; + const port_cfg_t *first_port_explicit_addr = NULL; + if (address_family == AF_UNSPEC) return NULL; const smartlist_t *conf_ports = get_configured_ports(); SMARTLIST_FOREACH_BEGIN(conf_ports, const port_cfg_t *, cfg) { - if (cfg->type == listener_type && - !cfg->server_cfg.no_advertise) { + if (cfg->type == listener_type && !cfg->server_cfg.no_advertise) { if ((address_family == AF_INET && port_binds_ipv4(cfg)) || (address_family == AF_INET6 && port_binds_ipv6(cfg))) { - return cfg; + if (cfg->explicit_addr && !first_port_explicit_addr) { + first_port_explicit_addr = cfg; + } else if (!first_port) { + first_port = cfg; + } } } } SMARTLIST_FOREACH_END(cfg); - return NULL; + + /* Prefer the port with the explicit address if any. */ + return (first_port_explicit_addr) ? first_port_explicit_addr : first_port; } /** Return the first advertised port of type listener_type in