From 8b8e39e04b365eefd7d69a488c04405cd6371645 Mon Sep 17 00:00:00 2001 From: George Kadianakis Date: Thu, 24 Aug 2017 16:16:44 +0300 Subject: [PATCH 1/3] prop224: Refactor descriptor rotation logic. The problem was that when we went from overlap mode to non-overlap mode, we were not wiping the 'desc_next' descriptor and instead we left it on the service. This meant that all functions that iterated service descriptors were also inspecting the useless 'desc_next' descriptor that should have been deleted. This commit refactors rotate_all_descriptors() so that it rotates descriptor both when entering overlap mode and also when leaving it. --- src/or/hs_service.c | 98 +++++++++++++++++++++++++++++++++------------ 1 file changed, 72 insertions(+), 26 deletions(-) diff --git a/src/or/hs_service.c b/src/or/hs_service.c index 0d0db1cd6b..e530a10a16 100644 --- a/src/or/hs_service.c +++ b/src/or/hs_service.c @@ -1714,10 +1714,35 @@ rotate_service_descriptors(hs_service_t *service) service->desc_next = NULL; } -/* Rotate descriptors for each service if needed. If we are just entering - * the overlap period, rotate them that is point the previous descriptor to - * the current and cleanup the previous one. A non existing current - * descriptor will trigger a descriptor build for the next time period. */ +/** Return true if service **just** entered overlap mode. */ +static int +service_just_entered_overlap_mode(const hs_service_t *service, + int overlap_mode_is_active) +{ + if (overlap_mode_is_active && !service->state.in_overlap_period) { + return 1; + } + + return 0; +} + +/** Return true if service **just** left overlap mode. */ +static int +service_just_left_overlap_mode(const hs_service_t *service, + int overlap_mode_is_active) +{ + if (!overlap_mode_is_active && service->state.in_overlap_period) { + return 1; + } + + return 0; +} + +/* Rotate descriptors for each service if needed. If we are just entering or + * leaving the overlap period, rotate them that is point the previous + * descriptor to the current and cleanup the previous one. A non existing + * current descriptor will trigger a descriptor build for the next time + * period. */ STATIC void rotate_all_descriptors(time_t now) { @@ -1725,35 +1750,56 @@ rotate_all_descriptors(time_t now) * be wise, to rotate service descriptors independently to hide that all * those descriptors are on the same tor instance */ - FOR_EACH_SERVICE_BEGIN(service) { - /* We are _not_ in the overlap period so skip rotation. */ - if (!hs_overlap_mode_is_active(NULL, now)) { - service->state.in_overlap_period = 0; - continue; - } - /* We've entered the overlap period already so skip rotation. */ - if (service->state.in_overlap_period) { - continue; - } - /* It's the first time the service encounters the overlap period so flag - * it in order to make sure we don't rotate at next check. */ - service->state.in_overlap_period = 1; + int overlap_mode_is_active = hs_overlap_mode_is_active(NULL, now); - /* We just entered overlap period: recompute all HSDir indices. We need to - * do this otherwise nodes can get stuck with old HSDir indices until we - * fetch a new consensus, and we might need to reupload our desc before - * that. */ - /* XXX find a better place than rotate_all_descriptors() to do this */ - nodelist_recompute_all_hsdir_indices(); + FOR_EACH_SERVICE_BEGIN(service) { + int service_entered_overlap = service_just_entered_overlap_mode(service, + overlap_mode_is_active); + int service_left_overlap = service_just_left_overlap_mode(service, + overlap_mode_is_active); + /* This should not be possible */ + if (BUG(service_entered_overlap && service_left_overlap)) { + return; + } + + /* No changes in overlap mode: nothing to do here */ + if (!service_entered_overlap && !service_left_overlap) { + return; + } + + /* To get down there means that some change happened to overlap mode */ + tor_assert(service_entered_overlap || service_left_overlap); + + /* Update the overlap marks on this service */ + if (service_entered_overlap) { + /* It's the first time the service encounters the overlap period so flag + * it in order to make sure we don't rotate at next check. */ + service->state.in_overlap_period = 1; + } else if (service_left_overlap) { + service->state.in_overlap_period = 0; + } + + if (service_entered_overlap) { + /* We just entered overlap period: recompute all HSDir indices. We need to + * do this otherwise nodes can get stuck with old HSDir indices until we + * fetch a new consensus, and we might need to reupload our desc before + * that. */ + /* XXX find a better place than rotate_all_descriptors() to do this */ + nodelist_recompute_all_hsdir_indices(); + } + + /* If we just entered or left overlap mode, rotate our descriptors */ + log_info(LD_REND, "We just %s overlap period. About to rotate %s " + "descriptors (%p / %p).", + service_entered_overlap ? "entered" : "left", + safe_str_client(service->onion_address), + service->desc_current, service->desc_next); /* If we have a next descriptor lined up, rotate the descriptors so that it * becomes current. */ if (service->desc_next) { rotate_service_descriptors(service); } - log_info(LD_REND, "We've just entered the overlap period. Service %s " - "descriptors have been rotated!", - safe_str_client(service->onion_address)); } FOR_EACH_SERVICE_END; } From c980be951157b68e6bd658ce01850e96ce97d422 Mon Sep 17 00:00:00 2001 From: George Kadianakis Date: Thu, 24 Aug 2017 16:17:26 +0300 Subject: [PATCH 2/3] prop224: Refactor descriptor reupload logic. We refactor the descriptor reupload logic to be similar to the v2 logic where we update a global 'consider_republishing_rend_descriptors' flag and then we use that to check for hash ring changes during the global hidden service callbacks. This fixes bugs where we would inspect the hash ring immediately as we receive new dirinfo (e.g. consensus) but before running the hidden service housekeeping events. That was leaving us in an inconsistent state wrt hsdir indices and causing bugs all around. --- src/or/hs_service.c | 48 +++++++++++++++++--------------------- src/or/hs_service.h | 4 ++++ src/test/test_hs_common.c | 7 +++--- src/test/test_hs_service.c | 13 +++++++++++ 4 files changed, 41 insertions(+), 31 deletions(-) diff --git a/src/or/hs_service.c b/src/or/hs_service.c index e530a10a16..0fc5a91445 100644 --- a/src/or/hs_service.c +++ b/src/or/hs_service.c @@ -72,6 +72,11 @@ static const char address_tld[] = "onion"; * loading keys requires that we are an actual running tor process. */ static smartlist_t *hs_service_staging_list; +/** True if the list of available router descriptors might have changed which + * might result in an altered hash ring. Check if the hash ring changed and + * reupload if needed */ +static int consider_republishing_hs_descriptors = 0; + static void set_descriptor_revision_counter(hs_descriptor_t *hs_desc); /* Helper: Function to compare two objects in the service map. Return 1 if the @@ -2429,7 +2434,14 @@ run_upload_descriptor_event(time_t now) FOR_EACH_DESCRIPTOR_BEGIN(service, desc) { int for_next_period = 0; - /* Can this descriptor be uploaed? */ + /* If we were asked to re-examine the hash ring, and it changed, then + schedule an upload */ + if (consider_republishing_hs_descriptors && + service_desc_hsdirs_changed(service, desc)) { + service_desc_schedule_upload(desc, now, 0); + } + + /* Can this descriptor be uploaded? */ if (!should_service_upload_descriptor(service, desc, now)) { continue; } @@ -2456,6 +2468,9 @@ run_upload_descriptor_event(time_t now) upload_descriptor_to_all(service, desc, for_next_period); } FOR_EACH_DESCRIPTOR_END; } FOR_EACH_SERVICE_END; + + /* We are done considering whether to republish rend descriptors */ + consider_republishing_hs_descriptors = 0; } /* Called when the introduction point circuit is done building and ready to be @@ -2738,7 +2753,7 @@ service_add_fnames_to_list(const hs_service_t *service, smartlist_t *list) /** The set of HSDirs have changed: check if the change affects our descriptor * HSDir placement, and if it does, reupload the desc. */ -static int +int service_desc_hsdirs_changed(const hs_service_t *service, const hs_service_descriptor_t *desc) { @@ -2788,34 +2803,13 @@ service_desc_hsdirs_changed(const hs_service_t *service, /* ========== */ /* We just received a new batch of descriptors which might affect the shape of - * the HSDir hash ring. Signal that we should re-upload our HS descriptors. */ + * the HSDir hash ring. Signal that we should reexamine the hash ring and + * re-upload our HS descriptors if needed. */ void hs_hsdir_set_changed_consider_reupload(void) { - time_t now = approx_time(); - - /* Check if HS subsystem is initialized */ - if (!hs_service_map) { - return; - } - - /* Basic test: If we have not bootstrapped 100% yet, no point in even trying - to upload descriptor. */ - if (!router_have_minimum_dir_info()) { - return; - } - - log_info(LD_GENERAL, "Received new dirinfo: Checking hash ring for changes"); - - /* Go over all descriptors and check if the set of HSDirs changed for any of - * them. Schedule reupload if so. */ - FOR_EACH_SERVICE_BEGIN(service) { - FOR_EACH_DESCRIPTOR_BEGIN(service, desc) { - if (service_desc_hsdirs_changed(service, desc)) { - service_desc_schedule_upload(desc, now, 0); - } - } FOR_EACH_DESCRIPTOR_END; - } FOR_EACH_SERVICE_END; + log_info(LD_REND, "New dirinfo arrived: consider reuploading descriptor"); + consider_republishing_hs_descriptors = 1; } /* Return the number of service we have configured and usable. */ diff --git a/src/or/hs_service.h b/src/or/hs_service.h index 5bd19931fc..d902e0b2c7 100644 --- a/src/or/hs_service.h +++ b/src/or/hs_service.h @@ -280,6 +280,10 @@ int hs_service_receive_introduce2(origin_circuit_t *circ, void hs_service_intro_circ_has_closed(origin_circuit_t *circ); +int service_desc_hsdirs_changed(const hs_service_t *service, + const hs_service_descriptor_t *desc); + + #ifdef HS_SERVICE_PRIVATE #ifdef TOR_UNIT_TESTS diff --git a/src/test/test_hs_common.c b/src/test/test_hs_common.c index ec938ff5d1..b1e47adf24 100644 --- a/src/test/test_hs_common.c +++ b/src/test/test_hs_common.c @@ -626,12 +626,11 @@ test_desc_reupload_logic(void *arg) curr_hsdir_index, nickname, 1); } - /* Now call router_dir_info_changed() again and see that it detected the hash - ring change and updated the upload time */ + /* Now call service_desc_hsdirs_changed() and see that it detected the hash + ring change */ time_t now = approx_time(); tt_assert(now); - router_dir_info_changed(); - tt_int_op(desc->next_upload_time, OP_EQ, now); + tt_int_op(service_desc_hsdirs_changed(service, desc), OP_EQ, 1); /* Now pretend that the descriptor changed, and order a reupload to all HSDirs. Make sure that the set of previous HSDirs was cleared. */ diff --git a/src/test/test_hs_service.c b/src/test/test_hs_service.c index cf1c36a528..4c7880c0ee 100644 --- a/src/test/test_hs_service.c +++ b/src/test/test_hs_service.c @@ -1005,6 +1005,13 @@ test_rotate_descriptors(void *arg) OP_EQ, 0); tt_assert(service->desc_next == NULL); + /* Now let's re-create desc_next and get out of overlap period. We should + test that desc_current gets replaced by desc_next, and desc_next becomes + NULL. */ + desc_next = service_descriptor_new(); + desc_next->next_upload_time = 240; /* Our marker to recognize it. */ + service->desc_next = desc_next; + /* Going out of the overlap period. */ ret = parse_rfc1123_time("Sat, 26 Oct 1985 12:00:00 UTC", &mock_ns.valid_after); @@ -1017,6 +1024,12 @@ test_rotate_descriptors(void *arg) tt_mem_op(service->desc_current, OP_EQ, desc_next, sizeof(*desc_next)); tt_assert(service->desc_next == NULL); + /* Calling rotate_all_descriptors() another time should do nothing */ + rotate_all_descriptors(now); + tt_int_op(service->state.in_overlap_period, OP_EQ, 0); + tt_mem_op(service->desc_current, OP_EQ, desc_next, sizeof(*desc_next)); + tt_assert(service->desc_next == NULL); + done: hs_free_all(); UNMOCK(circuit_mark_for_close_); From e07b677bd9d557d2ab15474ce96c825a5a034a7f Mon Sep 17 00:00:00 2001 From: George Kadianakis Date: Thu, 24 Aug 2017 19:32:33 +0300 Subject: [PATCH 3/3] prop224: Move service_desc_hsdirs_changed() and make it static. That function could be static but needed to be moved to the top. --- src/or/hs_service.c | 94 ++++++++++++++++++++++----------------------- src/or/hs_service.h | 7 ++-- 2 files changed, 50 insertions(+), 51 deletions(-) diff --git a/src/or/hs_service.c b/src/or/hs_service.c index 0fc5a91445..04248eaa06 100644 --- a/src/or/hs_service.c +++ b/src/or/hs_service.c @@ -2360,6 +2360,53 @@ upload_descriptor_to_all(const hs_service_t *service, return; } +/** The set of HSDirs have changed: check if the change affects our descriptor + * HSDir placement, and if it does, reupload the desc. */ +STATIC int +service_desc_hsdirs_changed(const hs_service_t *service, + const hs_service_descriptor_t *desc) +{ + int retval = 0; + smartlist_t *responsible_dirs = smartlist_new(); + smartlist_t *b64_responsible_dirs = smartlist_new(); + + /* No desc upload has happened yet: it will happen eventually */ + if (!desc->previous_hsdirs || !smartlist_len(desc->previous_hsdirs)) { + goto done; + } + + /* Get list of responsible hsdirs */ + hs_get_responsible_hsdirs(&desc->blinded_kp.pubkey, desc->time_period_num, + service->desc_next == desc, 0, responsible_dirs); + + /* Make a second list with their b64ed identity digests, so that we can + * compare it with out previous list of hsdirs */ + SMARTLIST_FOREACH_BEGIN(responsible_dirs, const routerstatus_t *, hsdir_rs) { + char b64_digest[BASE64_DIGEST_LEN+1] = {0}; + digest_to_base64(b64_digest, hsdir_rs->identity_digest); + smartlist_add_strdup(b64_responsible_dirs, b64_digest); + } SMARTLIST_FOREACH_END(hsdir_rs); + + /* Sort this new smartlist so that we can compare it with the other one */ + smartlist_sort_strings(b64_responsible_dirs); + + /* Check whether the set of HSDirs changed */ + if (!smartlist_strings_eq(b64_responsible_dirs, desc->previous_hsdirs)) { + log_warn(LD_GENERAL, "Received new dirinfo and set of hsdirs changed!"); + retval = 1; + } else { + log_warn(LD_GENERAL, "No change in hsdir set!"); + } + + done: + smartlist_free(responsible_dirs); + + SMARTLIST_FOREACH(b64_responsible_dirs, char*, s, tor_free(s)); + smartlist_free(b64_responsible_dirs); + + return retval; +} + /* Return 1 if the given descriptor from the given service can be uploaded * else return 0 if it can not. */ static int @@ -2751,53 +2798,6 @@ service_add_fnames_to_list(const hs_service_t *service, smartlist_t *list) smartlist_add(list, hs_path_from_filename(s_dir, fname)); } -/** The set of HSDirs have changed: check if the change affects our descriptor - * HSDir placement, and if it does, reupload the desc. */ -int -service_desc_hsdirs_changed(const hs_service_t *service, - const hs_service_descriptor_t *desc) -{ - int retval = 0; - smartlist_t *responsible_dirs = smartlist_new(); - smartlist_t *b64_responsible_dirs = smartlist_new(); - - /* No desc upload has happened yet: it will happen eventually */ - if (!desc->previous_hsdirs || !smartlist_len(desc->previous_hsdirs)) { - goto done; - } - - /* Get list of responsible hsdirs */ - hs_get_responsible_hsdirs(&desc->blinded_kp.pubkey, desc->time_period_num, - service->desc_next == desc, 0, responsible_dirs); - - /* Make a second list with their b64ed identity digests, so that we can - * compare it with out previous list of hsdirs */ - SMARTLIST_FOREACH_BEGIN(responsible_dirs, const routerstatus_t *, hsdir_rs) { - char b64_digest[BASE64_DIGEST_LEN+1] = {0}; - digest_to_base64(b64_digest, hsdir_rs->identity_digest); - smartlist_add_strdup(b64_responsible_dirs, b64_digest); - } SMARTLIST_FOREACH_END(hsdir_rs); - - /* Sort this new smartlist so that we can compare it with the other one */ - smartlist_sort_strings(b64_responsible_dirs); - - /* Check whether the set of HSDirs changed */ - if (!smartlist_strings_eq(b64_responsible_dirs, desc->previous_hsdirs)) { - log_info(LD_GENERAL, "Received new dirinfo and set of hsdirs changed!"); - retval = 1; - } else { - log_debug(LD_GENERAL, "No change in hsdir set!"); - } - - done: - smartlist_free(responsible_dirs); - - SMARTLIST_FOREACH(b64_responsible_dirs, char*, s, tor_free(s)); - smartlist_free(b64_responsible_dirs); - - return retval; -} - /* ========== */ /* Public API */ /* ========== */ diff --git a/src/or/hs_service.h b/src/or/hs_service.h index d902e0b2c7..57717fc927 100644 --- a/src/or/hs_service.h +++ b/src/or/hs_service.h @@ -280,10 +280,6 @@ int hs_service_receive_introduce2(origin_circuit_t *circ, void hs_service_intro_circ_has_closed(origin_circuit_t *circ); -int service_desc_hsdirs_changed(const hs_service_t *service, - const hs_service_descriptor_t *desc); - - #ifdef HS_SERVICE_PRIVATE #ifdef TOR_UNIT_TESTS @@ -357,6 +353,9 @@ STATIC void service_desc_schedule_upload(hs_service_descriptor_t *desc, time_t now, int descriptor_changed); +STATIC int service_desc_hsdirs_changed(const hs_service_t *service, + const hs_service_descriptor_t *desc); + #endif /* TOR_UNIT_TESTS */ #endif /* HS_SERVICE_PRIVATE */