commit f50de3a91872014f03856cf4c889f029ec5a1892 Author: David Goulet dgoulet@torproject.org Date: Tue Sep 10 14:40:40 2019 -0400
hs-v3: Do not remove intro point if circuit exists
When considering introduction point of a service's descriptor, do not remove an intro point that has an established or pending circuit.
Fixes #31652
Signed-off-by: David Goulet dgoulet@torproject.org --- changes/bug31652 | 5 +++ src/feature/hs/hs_service.c | 87 +++++++++++++++++++++++++++++++++------------ src/test/test_hs_service.c | 1 + 3 files changed, 70 insertions(+), 23 deletions(-)
diff --git a/changes/bug31652 b/changes/bug31652 new file mode 100644 index 000000000..c4eca7994 --- /dev/null +++ b/changes/bug31652 @@ -0,0 +1,5 @@ + o Minor bugfixes (onion services): + - When we clean up intro circuits for a v3 onion service, don't remove + circuits that have an established or pending circuit even if ran out of + retries. This way, we don't cleanup the circuit of the last retry. Fixes + bug 31652; bugfix on 0.3.2.1-alpha. diff --git a/src/feature/hs/hs_service.c b/src/feature/hs/hs_service.c index f81987f69..18c38ebc0 100644 --- a/src/feature/hs/hs_service.c +++ b/src/feature/hs/hs_service.c @@ -2333,15 +2333,70 @@ intro_point_should_expire(const hs_service_intro_point_t *ip, return 1; }
-/* Go over the given set of intro points for each service and remove any - * invalid ones. The conditions for removal are: +/* Return true iff we should remove the intro point ip from its service. * - * - The node doesn't exists anymore (not in consensus) - * OR - * - The intro point maximum circuit retry count has been reached and no - * circuit can be found associated with it. - * OR - * - The intro point has expired and we should pick a new one. + * We remove an intro point from the service descriptor list if one of + * these criteria is met: + * - It has expired (either in INTRO2 count or in time). + * - No node was found (fell off the consensus). + * - We are over the maximum amount of retries. + * + * If an established or pending circuit is found for the given ip object, this + * return false indicating it should not be removed. */ +static bool +should_remove_intro_point(hs_service_intro_point_t *ip, time_t now) +{ + bool ret = false; + + tor_assert(ip); + + /* Any one of the following needs to be True to furfill the criteria to + * remove an intro point. */ + bool has_no_retries = (ip->circuit_retries > + MAX_INTRO_POINT_CIRCUIT_RETRIES); + bool has_no_node = (get_node_from_intro_point(ip) == NULL); + bool has_expired = intro_point_should_expire(ip, now); + + /* If the node fell off the consensus or the IP has expired, we have to + * remove it now. */ + if (has_no_node || has_expired) { + ret = true; + goto end; + } + + /* Pass this point, even though we might be over the retry limit, we check + * if a circuit (established or pending) exists. In that case, we should not + * remove it because it might simply be valid and opened at the previous + * scheduled event for the last retry. */ + + /* Did we established already? */ + if (ip->circuit_established) { + goto end; + } + /* Do we simply have an existing circuit regardless of its state? */ + if (hs_circ_service_get_intro_circ(ip)) { + goto end; + } + + /* Getting here means we have _no_ circuits so then return if we have any + * remaining retries. */ + ret = has_no_retries; + + end: + /* Meaningful log in case we are about to remove the IP. */ + if (ret) { + log_info(LD_REND, "Intro point %s%s (retried: %u times). " + "Removing it.", + describe_intro_point(ip), + has_expired ? " has expired" : + (has_no_node) ? " fell off the consensus" : "", + ip->circuit_retries); + } + return ret; +} + +/* Go over the given set of intro points for each service and remove any + * invalid ones. * * If an intro point is removed, the circuit (if any) is immediately close. * If a circuit can't be found, the intro point is kept if it hasn't reached @@ -2366,21 +2421,7 @@ cleanup_intro_points(hs_service_t *service, time_t now) * valid and remove any of them that aren't. */ DIGEST256MAP_FOREACH_MODIFY(desc->intro_points.map, key, hs_service_intro_point_t *, ip) { - const node_t *node = get_node_from_intro_point(ip); - int has_expired = intro_point_should_expire(ip, now); - - /* We cleanup an intro point if it has expired or if we do not know the - * node_t anymore (removed from our latest consensus) or if we've - * reached the maximum number of retry with a non existing circuit. */ - if (has_expired || node == NULL || - ip->circuit_retries > MAX_INTRO_POINT_CIRCUIT_RETRIES) { - log_info(LD_REND, "Intro point %s%s (retried: %u times). " - "Removing it.", - describe_intro_point(ip), - has_expired ? " has expired" : - (node == NULL) ? " fell off the consensus" : "", - ip->circuit_retries); - + if (should_remove_intro_point(ip, now)) { /* We've retried too many times, remember it as a failed intro point * so we don't pick it up again for INTRO_CIRC_RETRY_PERIOD sec. */ if (ip->circuit_retries > MAX_INTRO_POINT_CIRCUIT_RETRIES) { diff --git a/src/test/test_hs_service.c b/src/test/test_hs_service.c index c5854f0ff..a2594ed6a 100644 --- a/src/test/test_hs_service.c +++ b/src/test/test_hs_service.c @@ -1300,6 +1300,7 @@ test_service_event(void *arg) OP_EQ, 1); /* Remove the IP object at once for the next test. */ ip->circuit_retries = MAX_INTRO_POINT_CIRCUIT_RETRIES + 1; + ip->circuit_established = 0; run_housekeeping_event(now); tt_int_op(digest256map_size(service->desc_current->intro_points.map), OP_EQ, 0);