commit 16f3f6a1afe5dcd75536039029f51392d05ce153 Author: teor <teor@torproject.org> Date: Wed Apr 15 13:04:33 2020 +1000 relay/circuitbuild: Re-use IPv6 connections for circuits Search for existing connections using the remote IPv4 and IPv6 addresses. Part of 33817. --- src/core/or/channel.c | 57 ++++++++++++++++++++++------------ src/core/or/channel.h | 3 +- src/core/or/circuitbuild.c | 16 +++++++--- src/feature/relay/circuitbuild_relay.c | 13 +++++++- src/lib/net/address.h | 4 +++ src/test/test_channel.c | 37 +++++++++++++++------- src/test/test_circuitbuild.c | 6 ++-- 7 files changed, 96 insertions(+), 40 deletions(-) diff --git a/src/core/or/channel.c b/src/core/or/channel.c index 89804826a..93245ce81 100644 --- a/src/core/or/channel.c +++ b/src/core/or/channel.c @@ -85,8 +85,10 @@ /* Static function prototypes */ -static int channel_matches_target_addr_for_extend(channel_t *chan, - const tor_addr_t *target); +static int channel_matches_target_addr_for_extend( + channel_t *chan, + const tor_addr_t *target_ipv4_addr, + const tor_addr_t *target_ipv6_addr); /* Global lists of channels */ @@ -2365,9 +2367,9 @@ channel_is_better(channel_t *a, channel_t *b) * Get a channel to extend a circuit. * * Given the desired relay identity, pick a suitable channel to extend a - * circuit to the target address requsted by the client. Search for an - * existing channel for the requested endpoint. Make sure the channel is - * usable for new circuits, and matches the target address. + * circuit to the target IPv4 or IPv6 address requsted by the client. Search + * for an existing channel for the requested endpoint. Make sure the channel + * is usable for new circuits, and matches one of the target addresses. * * Try to return the best channel. But if there is no good channel, set * *msg_out to a message describing the channel's state and our next action, @@ -2377,7 +2379,8 @@ channel_is_better(channel_t *a, channel_t *b) MOCK_IMPL(channel_t *, channel_get_for_extend,(const char *rsa_id_digest, const ed25519_public_key_t *ed_id, - const tor_addr_t *target_addr, + const tor_addr_t *target_ipv4_addr, + const tor_addr_t *target_ipv6_addr, const char **msg_out, int *launch_out)) { @@ -2409,11 +2412,15 @@ channel_get_for_extend,(const char *rsa_id_digest, continue; } + const int matches_target = + channel_matches_target_addr_for_extend(chan, + target_ipv4_addr, + target_ipv6_addr); /* Never return a non-open connection. */ if (!CHANNEL_IS_OPEN(chan)) { /* If the address matches, don't launch a new connection for this * circuit. */ - if (channel_matches_target_addr_for_extend(chan, target_addr)) + if (matches_target) ++n_inprogress_goodaddr; continue; } @@ -2424,22 +2431,21 @@ channel_get_for_extend,(const char *rsa_id_digest, continue; } - /* Never return a non-canonical connection using a recent link protocol - * if the address is not what we wanted. + /* If the connection is using a recent link protocol, only return canonical + * connections, when the address is one of the addresses we wanted. * * The channel_is_canonical_is_reliable() function asks the lower layer - * if we should trust channel_is_canonical(). The below is from the - * comments of the old circuit_or_get_for_extend() and applies when + * if we should trust channel_is_canonical(). It only applies when * the lower-layer transport is channel_tls_t. * - * (For old link protocols, we can't rely on is_canonical getting + * For old link protocols, we can't rely on is_canonical getting * set properly if we're talking to the right address, since we might * have an out-of-date descriptor, and we will get no NETINFO cell to - * tell us about the right address.) + * tell us about the right address. */ if (!channel_is_canonical(chan) && channel_is_canonical_is_reliable(chan) && - !channel_matches_target_addr_for_extend(chan, target_addr)) { + !matches_target) { ++n_noncanonical; continue; } @@ -3302,20 +3308,33 @@ channel_matches_extend_info(channel_t *chan, extend_info_t *extend_info) } /** - * Check if a channel matches a given target address; return true iff we do. + * Check if a channel matches the given target IPv4 or IPv6 addresses. + * If either address matches, return true. If neither address matches, + * return false. + * + * Both addresses can't be NULL. * * This function calls into the lower layer and asks if this channel thinks - * it matches a given target address for circuit extension purposes. + * it matches the target addresses for circuit extension purposes. */ int channel_matches_target_addr_for_extend(channel_t *chan, - const tor_addr_t *target) + const tor_addr_t *target_ipv4_addr, + const tor_addr_t *target_ipv6_addr) { tor_assert(chan); tor_assert(chan->matches_target); - tor_assert(target); - return chan->matches_target(chan, target); + IF_BUG_ONCE(!target_ipv4_addr && !target_ipv6_addr) + return 0; + + if (target_ipv4_addr && chan->matches_target(chan, target_ipv4_addr)) + return 1; + + if (target_ipv6_addr && chan->matches_target(chan, target_ipv6_addr)) + return 1; + + return 0; } /** diff --git a/src/core/or/channel.h b/src/core/or/channel.h index f86e77992..4968c8714 100644 --- a/src/core/or/channel.h +++ b/src/core/or/channel.h @@ -661,7 +661,8 @@ channel_t * channel_connect(const tor_addr_t *addr, uint16_t port, MOCK_DECL(channel_t *, channel_get_for_extend,( const char *rsa_id_digest, const struct ed25519_public_key_t *ed_id, - const tor_addr_t *target_addr, + const tor_addr_t *target_ipv4_addr, + const tor_addr_t *target_ipv6_addr, const char **msg_out, int *launch_out)); diff --git a/src/core/or/circuitbuild.c b/src/core/or/circuitbuild.c index ce0f9618f..0381a4dc3 100644 --- a/src/core/or/circuitbuild.c +++ b/src/core/or/circuitbuild.c @@ -559,11 +559,17 @@ circuit_handle_first_hop(origin_circuit_t *circ) fmt_addrport(&firsthop->extend_info->addr, firsthop->extend_info->port)); - n_chan = channel_get_for_extend(firsthop->extend_info->identity_digest, - &firsthop->extend_info->ed_identity, - &firsthop->extend_info->addr, - &msg, - &should_launch); + /* We'll cleanup this code in #33220, when we add an IPv6 address to + * extend_info_t. */ + const bool addr_is_ipv4 = + (tor_addr_family(&firsthop->extend_info->addr) == AF_INET); + n_chan = channel_get_for_extend( + firsthop->extend_info->identity_digest, + &firsthop->extend_info->ed_identity, + addr_is_ipv4 ? &firsthop->extend_info->addr : NULL, + addr_is_ipv4 ? NULL : &firsthop->extend_info->addr, + &msg, + &should_launch); if (!n_chan) { /* not currently connected in a useful way. */ diff --git a/src/feature/relay/circuitbuild_relay.c b/src/feature/relay/circuitbuild_relay.c index 7d3d58977..a926a1d81 100644 --- a/src/feature/relay/circuitbuild_relay.c +++ b/src/feature/relay/circuitbuild_relay.c @@ -330,9 +330,20 @@ circuit_extend(struct cell_t *cell, struct circuit_t *circ) if (circuit_extend_lspec_valid_helper(&ec, circ) < 0) return -1; + /* Check the addresses, without logging */ + const int ipv4_valid = + (circuit_extend_addr_port_helper(&ec.orport_ipv4, false, false, 0) == 0); + const int ipv6_valid = + (circuit_extend_addr_port_helper(&ec.orport_ipv6, false, false, 0) == 0); + IF_BUG_ONCE(!ipv4_valid && !ipv6_valid) { + /* circuit_extend_lspec_valid_helper() should have caught this */ + return -1; + } + n_chan = channel_get_for_extend((const char*)ec.node_id, &ec.ed_pubkey, - &ec.orport_ipv4.addr, + ipv4_valid ? &ec.orport_ipv4.addr : NULL, + ipv6_valid ? &ec.orport_ipv6.addr : NULL, &msg, &should_launch); diff --git a/src/lib/net/address.h b/src/lib/net/address.h index 611d1ca1e..651d6bc71 100644 --- a/src/lib/net/address.h +++ b/src/lib/net/address.h @@ -104,6 +104,10 @@ int tor_addr_from_sockaddr(tor_addr_t *a, const struct sockaddr *sa, uint16_t *port_out); void tor_addr_make_unspec(tor_addr_t *a); void tor_addr_make_null(tor_addr_t *a, sa_family_t family); +#define tor_addr_port_make_null(addr, port, family) \ + (void)(tor_addr_make_null(addr, family), (port) = 0) +#define tor_addr_port_make_null_ap(ap, family) \ + tor_addr_port_make_null(&(ap)->addr, (ap)->port, family) char *tor_sockaddr_to_str(const struct sockaddr *sa); /** Return an in6_addr* equivalent to <b>a</b>, or NULL if <b>a</b> is not diff --git a/src/test/test_channel.c b/src/test/test_channel.c index f7efbd7ab..849cc497f 100644 --- a/src/test/test_channel.c +++ b/src/test/test_channel.c @@ -1326,7 +1326,7 @@ test_channel_for_extend(void *arg) channel_t *ret_chan = NULL; char digest[DIGEST_LEN]; ed25519_public_key_t ed_id; - tor_addr_t addr; + tor_addr_t ipv4_addr, ipv6_addr; const char *msg; int launch; time_t now = time(NULL); @@ -1336,6 +1336,9 @@ test_channel_for_extend(void *arg) memset(digest, 'A', sizeof(digest)); memset(&ed_id, 'B', sizeof(ed_id)); + tor_addr_make_null(&ipv4_addr, AF_INET); + tor_addr_make_null(&ipv6_addr, AF_INET6); + chan1 = new_fake_channel(); tt_assert(chan1); /* Need to be registered to get added to the id map. */ @@ -1366,7 +1369,8 @@ test_channel_for_extend(void *arg) tt_ptr_op(channel_find_by_remote_identity(digest, &ed_id), OP_EQ, chan1); /* The expected result is chan2 because it is older than chan1. */ - ret_chan = channel_get_for_extend(digest, &ed_id, &addr, &msg, &launch); + ret_chan = channel_get_for_extend(digest, &ed_id, &ipv4_addr, &ipv6_addr, + &msg, &launch); tt_assert(ret_chan); tt_ptr_op(ret_chan, OP_EQ, chan2); tt_int_op(launch, OP_EQ, 0); @@ -1374,7 +1378,8 @@ test_channel_for_extend(void *arg) /* Switch that around from previous test. */ chan2->timestamp_created = chan1->timestamp_created + 1; - ret_chan = channel_get_for_extend(digest, &ed_id, &addr, &msg, &launch); + ret_chan = channel_get_for_extend(digest, &ed_id, &ipv4_addr, &ipv6_addr, + &msg, &launch); tt_assert(ret_chan); tt_ptr_op(ret_chan, OP_EQ, chan1); tt_int_op(launch, OP_EQ, 0); @@ -1383,7 +1388,8 @@ test_channel_for_extend(void *arg) /* Same creation time, num circuits will be used and they both have 0 so the * channel 2 should be picked due to how channel_is_better() works. */ chan2->timestamp_created = chan1->timestamp_created; - ret_chan = channel_get_for_extend(digest, &ed_id, &addr, &msg, &launch); + ret_chan = channel_get_for_extend(digest, &ed_id, &ipv4_addr, &ipv6_addr, + &msg, &launch); tt_assert(ret_chan); tt_ptr_op(ret_chan, OP_EQ, chan1); tt_int_op(launch, OP_EQ, 0); @@ -1394,7 +1400,8 @@ test_channel_for_extend(void *arg) /* Condemned the older channel. */ chan1->state = CHANNEL_STATE_CLOSING; - ret_chan = channel_get_for_extend(digest, &ed_id, &addr, &msg, &launch); + ret_chan = channel_get_for_extend(digest, &ed_id, &ipv4_addr, &ipv6_addr, + &msg, &launch); tt_assert(ret_chan); tt_ptr_op(ret_chan, OP_EQ, chan2); tt_int_op(launch, OP_EQ, 0); @@ -1403,7 +1410,8 @@ test_channel_for_extend(void *arg) /* Make the older channel a client one. */ channel_mark_client(chan1); - ret_chan = channel_get_for_extend(digest, &ed_id, &addr, &msg, &launch); + ret_chan = channel_get_for_extend(digest, &ed_id, &ipv4_addr, &ipv6_addr, + &msg, &launch); tt_assert(ret_chan); tt_ptr_op(ret_chan, OP_EQ, chan2); tt_int_op(launch, OP_EQ, 0); @@ -1413,8 +1421,9 @@ test_channel_for_extend(void *arg) /* Non matching ed identity with valid digest. */ ed25519_public_key_t dumb_ed_id; memset(&dumb_ed_id, 0, sizeof(dumb_ed_id)); - ret_chan = channel_get_for_extend(digest, &dumb_ed_id, &addr, &msg, - &launch); + ret_chan = channel_get_for_extend(digest, &dumb_ed_id, + &ipv4_addr, &ipv6_addr, + &msg, &launch); tt_assert(!ret_chan); tt_str_op(msg, OP_EQ, "Not connected. Connecting."); tt_int_op(launch, OP_EQ, 1); @@ -1423,7 +1432,8 @@ test_channel_for_extend(void *arg) test_chan_should_match_target = 1; chan1->state = CHANNEL_STATE_OPENING; chan2->state = CHANNEL_STATE_OPENING; - ret_chan = channel_get_for_extend(digest, &ed_id, &addr, &msg, &launch); + ret_chan = channel_get_for_extend(digest, &ed_id, &ipv4_addr, &ipv6_addr, + &msg, &launch); tt_assert(!ret_chan); tt_str_op(msg, OP_EQ, "Connection in progress; waiting."); tt_int_op(launch, OP_EQ, 0); @@ -1432,7 +1442,8 @@ test_channel_for_extend(void *arg) /* Mark channel 1 as bad for circuits. */ channel_mark_bad_for_new_circs(chan1); - ret_chan = channel_get_for_extend(digest, &ed_id, &addr, &msg, &launch); + ret_chan = channel_get_for_extend(digest, &ed_id, &ipv4_addr, &ipv6_addr, + &msg, &launch); tt_assert(ret_chan); tt_ptr_op(ret_chan, OP_EQ, chan2); tt_int_op(launch, OP_EQ, 0); @@ -1442,7 +1453,8 @@ test_channel_for_extend(void *arg) /* Mark both channels as unusable. */ channel_mark_bad_for_new_circs(chan1); channel_mark_bad_for_new_circs(chan2); - ret_chan = channel_get_for_extend(digest, &ed_id, &addr, &msg, &launch); + ret_chan = channel_get_for_extend(digest, &ed_id, &ipv4_addr, &ipv6_addr, + &msg, &launch); tt_assert(!ret_chan); tt_str_op(msg, OP_EQ, "Connections all too old, or too non-canonical. " " Launching a new one."); @@ -1453,7 +1465,8 @@ test_channel_for_extend(void *arg) /* Non canonical channels. */ test_chan_should_match_target = 0; test_chan_canonical_should_be_reliable = 1; - ret_chan = channel_get_for_extend(digest, &ed_id, &addr, &msg, &launch); + ret_chan = channel_get_for_extend(digest, &ed_id, &ipv4_addr, &ipv6_addr, + &msg, &launch); tt_assert(!ret_chan); tt_str_op(msg, OP_EQ, "Connections all too old, or too non-canonical. " " Launching a new one."); diff --git a/src/test/test_circuitbuild.c b/src/test/test_circuitbuild.c index a8d6323e4..668d9869d 100644 --- a/src/test/test_circuitbuild.c +++ b/src/test/test_circuitbuild.c @@ -993,13 +993,15 @@ static channel_t *mock_channel_get_for_extend_nchan = NULL; static channel_t * mock_channel_get_for_extend(const char *rsa_id_digest, const ed25519_public_key_t *ed_id, - const tor_addr_t *target_addr, + const tor_addr_t *target_ipv4_addr, + const tor_addr_t *target_ipv6_addr, const char **msg_out, int *launch_out) { (void)rsa_id_digest; (void)ed_id; - (void)target_addr; + (void)target_ipv4_addr; + (void)target_ipv6_addr; /* channel_get_for_extend() requires non-NULL arguments */ tt_ptr_op(msg_out, OP_NE, NULL);