From ac9066660e4780460ca6e96dc98c7dac7aadc722 Mon Sep 17 00:00:00 2001 From: George Kadianakis Date: Wed, 30 Aug 2017 14:14:58 +0300 Subject: [PATCH 1/4] prop224 test: Simplify interface for adding nodes to hash ring. We want to have tests with big hash rings so let's make it an one-liner to add nodes. --- src/test/test_hs_common.c | 64 ++++++++------------------------------- 1 file changed, 13 insertions(+), 51 deletions(-) diff --git a/src/test/test_hs_common.c b/src/test/test_hs_common.c index b9215ea187..f8870ca7be 100644 --- a/src/test/test_hs_common.c +++ b/src/test/test_hs_common.c @@ -362,15 +362,19 @@ test_desc_overlap_period_testnet(void *arg) static void helper_add_hsdir_to_networkstatus(networkstatus_t *ns, - const uint8_t *identity, - const uint8_t *curr_hsdir_index, + int identity_idx, const char *nickname, int is_hsdir) { routerstatus_t *rs = tor_malloc_zero(sizeof(routerstatus_t)); routerinfo_t *ri = tor_malloc_zero(sizeof(routerinfo_t)); - + uint8_t identity[DIGEST_LEN]; + uint8_t curr_hsdir_index[DIGEST256_LEN]; tor_addr_t ipv4_addr; + + memset(identity, identity_idx, sizeof(identity)); + memset(curr_hsdir_index, identity_idx, sizeof(curr_hsdir_index)); + memcpy(rs->identity_digest, identity, DIGEST_LEN); rs->is_hs_dir = is_hsdir; rs->supports_v3_hsdir = 1; @@ -435,36 +439,15 @@ test_responsible_hsdirs(void *arg) ns = networkstatus_get_latest_consensus(); { /* First router: HSdir */ - uint8_t identity[DIGEST_LEN]; - uint8_t curr_hsdir_index[DIGEST256_LEN]; - char nickname[] = "let_me"; - memset(identity, 1, sizeof(identity)); - memset(curr_hsdir_index, 1, sizeof(curr_hsdir_index)); - - helper_add_hsdir_to_networkstatus(ns, identity, - curr_hsdir_index, nickname, 1); + helper_add_hsdir_to_networkstatus(ns, 1, "igor", 1); } { /* Second HSDir */ - uint8_t identity[DIGEST_LEN]; - uint8_t curr_hsdir_index[DIGEST256_LEN]; - char nickname[] = "show_you"; - memset(identity, 2, sizeof(identity)); - memset(curr_hsdir_index, 2, sizeof(curr_hsdir_index)); - - helper_add_hsdir_to_networkstatus(ns, identity, - curr_hsdir_index, nickname, 1); + helper_add_hsdir_to_networkstatus(ns, 2, "victor", 1); } { /* Third relay but not HSDir */ - uint8_t identity[DIGEST_LEN]; - uint8_t curr_hsdir_index[DIGEST256_LEN]; - char nickname[] = "how_to_dance"; - memset(identity, 3, sizeof(identity)); - memset(curr_hsdir_index, 3, sizeof(curr_hsdir_index)); - - helper_add_hsdir_to_networkstatus(ns, identity, - curr_hsdir_index, nickname, 0); + helper_add_hsdir_to_networkstatus(ns, 3, "spyro", 0); } ed25519_keypair_t kp; @@ -580,25 +563,11 @@ test_desc_reupload_logic(void *arg) /* Now let's create our hash ring: */ { /* First HSDir */ - uint8_t identity[DIGEST_LEN]; - uint8_t curr_hsdir_index[DIGEST256_LEN]; - char nickname[] = "let_me"; - memset(identity, 1, sizeof(identity)); - memset(curr_hsdir_index, 1, sizeof(curr_hsdir_index)); - - helper_add_hsdir_to_networkstatus(ns, identity, - curr_hsdir_index, nickname, 1); + helper_add_hsdir_to_networkstatus(ns, 1, "dingus", 1); } { /* Second HSDir */ - uint8_t identity[DIGEST_LEN]; - uint8_t curr_hsdir_index[DIGEST256_LEN]; - char nickname[] = "show_you"; - memset(identity, 2, sizeof(identity)); - memset(curr_hsdir_index, 2, sizeof(curr_hsdir_index)); - - helper_add_hsdir_to_networkstatus(ns, identity, - curr_hsdir_index, nickname, 1); + helper_add_hsdir_to_networkstatus(ns, 2, "clive", 1); } /* Now let's upload our desc to all hsdirs */ @@ -616,14 +585,7 @@ test_desc_reupload_logic(void *arg) /* Now change the HSDir hash ring by adding another node */ { /* Third HSDir */ - uint8_t identity[DIGEST_LEN]; - uint8_t curr_hsdir_index[DIGEST256_LEN]; - char nickname[] = "how_to_dance"; - memset(identity, 3, sizeof(identity)); - memset(curr_hsdir_index, 3, sizeof(curr_hsdir_index)); - - helper_add_hsdir_to_networkstatus(ns, identity, - curr_hsdir_index, nickname, 1); + helper_add_hsdir_to_networkstatus(ns, 3, "ringo", 1); } /* Now call service_desc_hsdirs_changed() and see that it detected the hash From d3cefddfc5dd75fd0a8dbd27e7b846b26bb86c4d Mon Sep 17 00:00:00 2001 From: George Kadianakis Date: Wed, 30 Aug 2017 15:14:34 +0300 Subject: [PATCH 2/4] prop224 test: Improve desc_reupload_logic() test with more nodes. --- src/test/test_hs_common.c | 72 +++++++++++++++++++++++++++++++++------ 1 file changed, 62 insertions(+), 10 deletions(-) diff --git a/src/test/test_hs_common.c b/src/test/test_hs_common.c index f8870ca7be..6bb03f39d6 100644 --- a/src/test/test_hs_common.c +++ b/src/test/test_hs_common.c @@ -21,6 +21,7 @@ #include "networkstatus.h" #include "directory.h" #include "nodelist.h" +#include "routerlist.h" #include "statefile.h" /** Test the validation of HS v3 addresses */ @@ -378,8 +379,10 @@ helper_add_hsdir_to_networkstatus(networkstatus_t *ns, memcpy(rs->identity_digest, identity, DIGEST_LEN); rs->is_hs_dir = is_hsdir; rs->supports_v3_hsdir = 1; + strlcpy(rs->nickname, nickname, sizeof(rs->nickname)); tor_addr_parse(&ipv4_addr, "1.2.3.4"); ri->addr = tor_addr_to_ipv4h(&ipv4_addr); + rs->addr = tor_addr_to_ipv4h(&ipv4_addr); ri->nickname = tor_strdup(nickname); ri->protocol_list = tor_strdup("HSDir=1-2 LinkAuth=3"); memcpy(ri->cache_info.identity_digest, identity, DIGEST_LEN); @@ -392,7 +395,7 @@ helper_add_hsdir_to_networkstatus(networkstatus_t *ns, smartlist_add(ns->routerstatus_list, rs); done: - ; + routerinfo_free(ri); } static networkstatus_t *mock_ns = NULL; @@ -416,6 +419,7 @@ mock_networkstatus_get_latest_consensus(void) mock_ns->valid_until = now+2; /* Create routerstatus list */ mock_ns->routerstatus_list = smartlist_new(); + mock_ns->type = NS_TYPE_CONSENSUS; return mock_ns; } @@ -562,18 +566,19 @@ test_desc_reupload_logic(void *arg) tt_int_op(hs_service_get_num_services(), OP_EQ, 1); /* Now let's create our hash ring: */ - { /* First HSDir */ + { helper_add_hsdir_to_networkstatus(ns, 1, "dingus", 1); - } - - { /* Second HSDir */ helper_add_hsdir_to_networkstatus(ns, 2, "clive", 1); + helper_add_hsdir_to_networkstatus(ns, 3, "aaron", 1); + helper_add_hsdir_to_networkstatus(ns, 4, "lizzie", 1); + helper_add_hsdir_to_networkstatus(ns, 5, "daewon", 1); + helper_add_hsdir_to_networkstatus(ns, 6, "clarke", 1); } /* Now let's upload our desc to all hsdirs */ upload_descriptor_to_all(service, desc, 0); /* Check that previous hsdirs were populated */ - tt_int_op(smartlist_len(desc->previous_hsdirs), OP_EQ, 2); + tt_int_op(smartlist_len(desc->previous_hsdirs), OP_EQ, 6); /* Poison next upload time so that we can see if it was changed by * router_dir_info_changed(). No changes in hash ring so far, so the upload @@ -582,10 +587,23 @@ test_desc_reupload_logic(void *arg) router_dir_info_changed(); tt_int_op(desc->next_upload_time, OP_EQ, 42); - /* Now change the HSDir hash ring by adding another node */ + /* Now change the HSDir hash ring by swapping nora for aaron. + * Start by clearing the hash ring */ + { + SMARTLIST_FOREACH(ns->routerstatus_list, + routerstatus_t *, rs, routerstatus_free(rs)); + smartlist_clear(ns->routerstatus_list); + nodelist_free_all(); + routerlist_free_all(); + } - { /* Third HSDir */ - helper_add_hsdir_to_networkstatus(ns, 3, "ringo", 1); + { /* Now add back all the nodes */ + helper_add_hsdir_to_networkstatus(ns, 1, "dingus", 1); + helper_add_hsdir_to_networkstatus(ns, 2, "clive", 1); + helper_add_hsdir_to_networkstatus(ns, 4, "lizzie", 1); + helper_add_hsdir_to_networkstatus(ns, 5, "daewon", 1); + helper_add_hsdir_to_networkstatus(ns, 6, "clarke", 1); + helper_add_hsdir_to_networkstatus(ns, 7, "nora", 1); } /* Now call service_desc_hsdirs_changed() and see that it detected the hash @@ -593,6 +611,35 @@ test_desc_reupload_logic(void *arg) time_t now = approx_time(); tt_assert(now); tt_int_op(service_desc_hsdirs_changed(service, desc), OP_EQ, 1); + tt_int_op(smartlist_len(desc->previous_hsdirs), OP_EQ, 6); + + /* Now order another upload and see how we end up with 7 hsdirs */ + upload_descriptor_to_all(service, desc, 0); + /* Check that previous hsdirs were populated */ + tt_int_op(smartlist_len(desc->previous_hsdirs), OP_EQ, 7); + + /* Now restore the HSDir hash ring to its original state by swapping back + aaron for nora */ + /* First clear up the hash ring */ + { + SMARTLIST_FOREACH(ns->routerstatus_list, + routerstatus_t *, rs, routerstatus_free(rs)); + smartlist_clear(ns->routerstatus_list); + nodelist_free_all(); + routerlist_free_all(); + } + + { /* Now populate the hash ring again */ + helper_add_hsdir_to_networkstatus(ns, 1, "dingus", 1); + helper_add_hsdir_to_networkstatus(ns, 2, "clive", 1); + helper_add_hsdir_to_networkstatus(ns, 3, "aaron", 1); + helper_add_hsdir_to_networkstatus(ns, 4, "lizzie", 1); + helper_add_hsdir_to_networkstatus(ns, 5, "daewon", 1); + helper_add_hsdir_to_networkstatus(ns, 6, "clarke", 1); + } + + /* Check that our algorithm catches this change of hsdirs */ + 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. */ @@ -601,9 +648,14 @@ test_desc_reupload_logic(void *arg) /* Now reupload again: see that the prev hsdir set got populated again. */ upload_descriptor_to_all(service, desc, 0); - tt_int_op(smartlist_len(desc->previous_hsdirs), OP_EQ, 3); + tt_int_op(smartlist_len(desc->previous_hsdirs), OP_EQ, 6); done: + SMARTLIST_FOREACH(ns->routerstatus_list, + routerstatus_t *, rs, routerstatus_free(rs)); + smartlist_clear(ns->routerstatus_list); + networkstatus_vote_free(ns); + nodelist_free_all(); hs_free_all(); } From 1dc21b87904b527140446674876076f29c78b89f Mon Sep 17 00:00:00 2001 From: George Kadianakis Date: Wed, 30 Aug 2017 15:29:41 +0300 Subject: [PATCH 3/4] prop224: Simplify HSDir set change algo. Our logic for detecting hsdir set changes was needlessly compicated: we had to sort smartlists and compare them. Instead, we can simplify things by employing the following logic: "We should reupload our descriptor if the latest HSDir set contains nodes that were not previously there" --- src/or/hs_service.c | 32 +++++++++++--------------------- src/or/hs_service.h | 2 +- 2 files changed, 12 insertions(+), 22 deletions(-) diff --git a/src/or/hs_service.c b/src/or/hs_service.c index 5ff118222d..0fba0d7f90 100644 --- a/src/or/hs_service.c +++ b/src/or/hs_service.c @@ -1546,7 +1546,6 @@ service_desc_note_upload(hs_service_descriptor_t *desc, const node_t *hsdir) if (!smartlist_contains_string(desc->previous_hsdirs, b64_digest)) { smartlist_add_strdup(desc->previous_hsdirs, b64_digest); - smartlist_sort_strings(desc->previous_hsdirs); } } @@ -2366,9 +2365,8 @@ STATIC int service_desc_hsdirs_changed(const hs_service_t *service, const hs_service_descriptor_t *desc) { - int retval = 0; + int should_reupload = 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)) { @@ -2379,32 +2377,24 @@ service_desc_hsdirs_changed(const hs_service_t *service, 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 */ + /* Check if any new hsdirs have been added to the responsible hsdirs set: + * Iterate over the list of new hsdirs, and reupload if any of them is not + * present in the list of previous 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); + + if (!smartlist_contains_string(desc->previous_hsdirs, b64_digest)) { + should_reupload = 1; + break; + } } 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; + return should_reupload; } /* Return 1 if the given descriptor from the given service can be uploaded diff --git a/src/or/hs_service.h b/src/or/hs_service.h index 57717fc927..26f0bc0002 100644 --- a/src/or/hs_service.h +++ b/src/or/hs_service.h @@ -133,7 +133,7 @@ typedef struct hs_service_descriptor_t { /** List of the responsible HSDirs (their b64ed identity digest) last time we * uploaded this descriptor. If the set of responsible HSDirs is different * from this list, this means we received new dirinfo and we need to - * reupload our descriptor. This list is always sorted lexicographically. */ + * reupload our descriptor. */ smartlist_t *previous_hsdirs; } hs_service_descriptor_t; From b9f849bdee20f36264fe061498536e0d3a8616a6 Mon Sep 17 00:00:00 2001 From: George Kadianakis Date: Tue, 29 Aug 2017 16:02:01 +0300 Subject: [PATCH 4/4] prop224: Clear list of prev hsdirs before we upload all descs. This fixes a serious bug in our hsdir set change logic: We used to add nodes in the list of previous hsdirs everytime we uploaded to a new hsdir and we only cleared the list when we built a new descriptor. This means that our prev_hsdirs list could end up with 7 hsdirs, if for some reason we ended up uploading our desc to 7 hsdirs before rebuilding our descriptor (e.g. this can happen if the set of hsdirs changed). After our previous hdsir set had 7 nodes, then our old algorithm would always think that the set has changed since it was comparing a smartlist with 7 elements against a smartlist with 6 elements. This commit fixes this bug, by clearning the prev_hsdirs list before we upload to all hsdirs. This makes sure that our prev_hsdirs list always contains the latest hsdirs! --- src/or/hs_service.c | 4 ++++ src/test/test_hs_common.c | 4 ++-- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/or/hs_service.c b/src/or/hs_service.c index 0fba0d7f90..218d49ace3 100644 --- a/src/or/hs_service.c +++ b/src/or/hs_service.c @@ -2316,6 +2316,10 @@ upload_descriptor_to_all(const hs_service_t *service, hs_get_responsible_hsdirs(&desc->blinded_kp.pubkey, desc->time_period_num, for_next_period, 0, responsible_dirs); + /** Clear list of previous hsdirs since we are about to upload to a new + * list. Let's keep it up to date. */ + service_desc_clear_previous_hsdirs(desc); + /* For each responsible HSDir we have, initiate an upload command. */ SMARTLIST_FOREACH_BEGIN(responsible_dirs, const routerstatus_t *, hsdir_rs) { diff --git a/src/test/test_hs_common.c b/src/test/test_hs_common.c index 6bb03f39d6..9980892951 100644 --- a/src/test/test_hs_common.c +++ b/src/test/test_hs_common.c @@ -613,10 +613,10 @@ test_desc_reupload_logic(void *arg) tt_int_op(service_desc_hsdirs_changed(service, desc), OP_EQ, 1); tt_int_op(smartlist_len(desc->previous_hsdirs), OP_EQ, 6); - /* Now order another upload and see how we end up with 7 hsdirs */ + /* Now order another upload and see that we keep having 6 prev hsdirs */ upload_descriptor_to_all(service, desc, 0); /* Check that previous hsdirs were populated */ - tt_int_op(smartlist_len(desc->previous_hsdirs), OP_EQ, 7); + tt_int_op(smartlist_len(desc->previous_hsdirs), OP_EQ, 6); /* Now restore the HSDir hash ring to its original state by swapping back aaron for nora */