commit 7823c98a38556237a86c7235d411d7d2237cc2d6 Author: George Kadianakis desnacked@riseup.net Date: Sat Aug 19 16:26:46 2017 +0300

prop224: Improve descriptor reupload logic.

We want to reupload our descriptor if its set of responsible HSDirs changed to minimize reachability issues.

This patch adds a callback everytime we get new dirinfo which checks if the hash ring changed and reuploads descriptor if needed. --- src/or/hs_descriptor.c | 8 +-- src/or/hs_descriptor.h | 7 +- src/or/hs_service.c | 136 +++++++++++++++++++++++++++++++++++++- src/or/hs_service.h | 15 +++++ src/or/nodelist.c | 1 + src/test/test_hs_common.c | 163 +++++++++++++++++++++++++++++++++++++++++++++- 6 files changed, 320 insertions(+), 10 deletions(-)

diff --git a/src/or/hs_descriptor.c b/src/or/hs_descriptor.c index 9a1e37715..3cdc023f7 100644 --- a/src/or/hs_descriptor.c +++ b/src/or/hs_descriptor.c @@ -2357,10 +2357,10 @@ static int * * Return 0 on success and encoded_out is a valid pointer. On error, -1 is * returned and encoded_out is set to NULL. */ -int -hs_desc_encode_descriptor(const hs_descriptor_t *desc, - const ed25519_keypair_t *signing_kp, - char **encoded_out) +MOCK_IMPL(int, +hs_desc_encode_descriptor,(const hs_descriptor_t *desc, + const ed25519_keypair_t *signing_kp, + char **encoded_out)) { int ret = -1; uint32_t version; diff --git a/src/or/hs_descriptor.h b/src/or/hs_descriptor.h index fa211d391..19e52333b 100644 --- a/src/or/hs_descriptor.h +++ b/src/or/hs_descriptor.h @@ -211,9 +211,10 @@ hs_desc_link_specifier_t *hs_desc_link_specifier_new( const extend_info_t *info, uint8_t type); void hs_descriptor_clear_intro_points(hs_descriptor_t *desc);

-int hs_desc_encode_descriptor(const hs_descriptor_t *desc, - const ed25519_keypair_t *signing_kp, - char **encoded_out); +MOCK_DECL(int, + hs_desc_encode_descriptor,(const hs_descriptor_t *desc, + const ed25519_keypair_t *signing_kp, + char **encoded_out));

int hs_desc_decode_descriptor(const char *encoded, const uint8_t *subcredential, diff --git a/src/or/hs_service.c b/src/or/hs_service.c index cf5f319f8..e213efe06 100644 --- a/src/or/hs_service.c +++ b/src/or/hs_service.c @@ -972,6 +972,10 @@ service_descriptor_free(hs_service_descriptor_t *desc) /* Cleanup all intro points. */ digest256map_free(desc->intro_points.map, service_intro_point_free_); digestmap_free(desc->intro_points.failed_id, tor_free_); + if (desc->previous_hsdirs) { + SMARTLIST_FOREACH(desc->previous_hsdirs, char *, s, tor_free(s)); + smartlist_free(desc->previous_hsdirs); + } tor_free(desc); }

@@ -985,6 +989,7 @@ service_descriptor_new(void) sdesc->intro_points.map = digest256map_new(); sdesc->intro_points.failed_id = digestmap_new(); sdesc->hsdir_missing_info = smartlist_new(); + sdesc->previous_hsdirs = smartlist_new(); return sdesc; }

@@ -1511,6 +1516,52 @@ pick_needed_intro_points(hs_service_t *service, return i; }

+/** Clear previous cached HSDirs in <b>desc</b>. */ +static void +service_desc_clear_previous_hsdirs(hs_service_descriptor_t *desc) +{ + if (BUG(!desc->previous_hsdirs)) { + return; + } + + SMARTLIST_FOREACH(desc->previous_hsdirs, char*, s, tor_free(s)); + smartlist_clear(desc->previous_hsdirs); +} + +/** Note that we attempted to upload <b>desc</b> to <b>hsdir</b>. */ +static void +service_desc_note_upload(hs_service_descriptor_t *desc, const node_t *hsdir) +{ + char b64_digest[BASE64_DIGEST_LEN+1] = {0}; + digest_to_base64(b64_digest, hsdir->identity); + + if (BUG(!desc->previous_hsdirs)) { + return; + } + + if (!smartlist_contains_string(desc->previous_hsdirs, b64_digest)) { + smartlist_add_strdup(desc->previous_hsdirs, b64_digest); + smartlist_sort_strings(desc->previous_hsdirs); + } +} + +/** Schedule an upload of <b>desc</b>. If <b>descriptor_changed</b> is set, it + * means that this descriptor is dirty. */ +STATIC void +service_desc_schedule_upload(hs_service_descriptor_t *desc, + time_t now, + int descriptor_changed) + +{ + desc->next_upload_time = now; + + /* If the descriptor changed, clean up the old HSDirs list. We want to + * re-upload no matter what. */ + if (descriptor_changed) { + service_desc_clear_previous_hsdirs(desc); + } +} + /* Update the given descriptor from the given service. The possible update * actions includes: * - Picking missing intro points if needed. @@ -1543,7 +1594,7 @@ update_service_descriptor(hs_service_t *service, /* We'll build those introduction point into the descriptor once we have * confirmation that the circuits are opened and ready. However, * indicate that this descriptor should be uploaded from now on. */ - desc->next_upload_time = now; + service_desc_schedule_upload(desc, now, 1); } /* Were we able to pick all the intro points we needed? If not, we'll * flag the descriptor that it's missing intro points because it @@ -1972,6 +2023,9 @@ upload_descriptor_to_hsdir(const hs_service_t *service, directory_initiate_request(dir_req); directory_request_free(dir_req);

+ /* Add this node to previous_hsdirs list */ + service_desc_note_upload(desc, hsdir); + /* Logging so we know where it was sent. */ { int is_next_desc = (service->desc_next == desc); @@ -2189,7 +2243,7 @@ set_descriptor_revision_counter(hs_descriptor_t *hs_desc) * responsible hidden service directories. If for_next_period is true, the set * of directories are selected using the next hsdir_index. This does nothing * if PublishHidServDescriptors is false. */ -static void +STATIC void upload_descriptor_to_all(const hs_service_t *service, hs_service_descriptor_t *desc, int for_next_period) { @@ -2629,10 +2683,88 @@ 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. */ +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; +} + /* ========== */ /* Public API */ /* ========== */

+/* 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. */ +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 descriptors. Set of HSdirs changed."); + + /* 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; +} + /* Return the number of service we have configured and usable. */ unsigned int hs_service_get_num_services(void) diff --git a/src/or/hs_service.h b/src/or/hs_service.h index 8d613d23e..5bd19931f 100644 --- a/src/or/hs_service.h +++ b/src/or/hs_service.h @@ -129,6 +129,12 @@ typedef struct hs_service_descriptor_t { * list are re-tried to upload this descriptor when our directory information * have been updated. */ smartlist_t *hsdir_missing_info; + + /** 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. */ + smartlist_t *previous_hsdirs; } hs_service_descriptor_t;

/* Service key material. */ @@ -260,6 +266,7 @@ void hs_service_lists_fnames_for_sandbox(smartlist_t *file_list, smartlist_t *dir_list); int hs_service_set_conn_addr_port(const origin_circuit_t *circ, edge_connection_t *conn); +void hs_hsdir_set_changed_consider_reupload(void);

void hs_service_dir_info_changed(void); void hs_service_run_scheduled_events(time_t now); @@ -338,6 +345,14 @@ check_state_line_for_service_rev_counter(const char *state_line, STATIC int write_address_to_file(const hs_service_t *service, const char *fname_);

+STATIC void upload_descriptor_to_all(const hs_service_t *service, + hs_service_descriptor_t *desc, + int for_next_period); + +STATIC void service_desc_schedule_upload(hs_service_descriptor_t *desc, + time_t now, + int descriptor_changed); + #endif /* TOR_UNIT_TESTS */

#endif /* HS_SERVICE_PRIVATE */ diff --git a/src/or/nodelist.c b/src/or/nodelist.c index a9b77262c..d75b386e0 100644 --- a/src/or/nodelist.c +++ b/src/or/nodelist.c @@ -1789,6 +1789,7 @@ router_dir_info_changed(void) { need_to_update_have_min_dir_info = 1; rend_hsdir_routers_changed(); + hs_hsdir_set_changed_consider_reupload(); }

/** Return a string describing what we're missing before we have enough diff --git a/src/test/test_hs_common.c b/src/test/test_hs_common.c index db3766307..c60deb2ab 100644 --- a/src/test/test_hs_common.c +++ b/src/test/test_hs_common.c @@ -18,7 +18,9 @@ #include "hs_service.h" #include "config.h" #include "networkstatus.h" +#include "directory.h" #include "nodelist.h" +#include "statefile.h"

/** Test the validation of HS v3 addresses */ static void @@ -486,6 +488,163 @@ test_responsible_hsdirs(void *arg) networkstatus_vote_free(mock_ns); }

+static void +mock_directory_initiate_request(directory_request_t *req) +{ + (void)req; + return; +} + +static int +mock_hs_desc_encode_descriptor(const hs_descriptor_t *desc, + const ed25519_keypair_t *signing_kp, + char **encoded_out) +{ + (void)desc; + (void)signing_kp; + + tor_asprintf(encoded_out, "lulu"); + return 0; +} + +static or_state_t dummy_state; + +/* Mock function to get fake or state (used for rev counters) */ +static or_state_t * +get_or_state_replacement(void) +{ + return &dummy_state; +} + +static int +mock_router_have_minimum_dir_info(void) +{ + return 1; +} + +/** Test that we correctly detect when the HSDir hash ring changes so that we + * reupload our descriptor. */ +static void +test_desc_reupload_logic(void *arg) +{ + networkstatus_t *ns = NULL; + + (void) arg; + + hs_init(); + + MOCK(router_have_minimum_dir_info, + mock_router_have_minimum_dir_info); + MOCK(get_or_state, + get_or_state_replacement); + MOCK(networkstatus_get_latest_consensus, + mock_networkstatus_get_latest_consensus); + MOCK(directory_initiate_request, + mock_directory_initiate_request); + MOCK(hs_desc_encode_descriptor, + mock_hs_desc_encode_descriptor); + + ns = networkstatus_get_latest_consensus(); + + /** Test logic: + * 1) Upload descriptor to HSDirs + * CHECK that previous_hsdirs list was populated. + * 2) Then call router_dir_info_changed() without an HSDir set change. + * CHECK that no reuplod occurs. + * 3) Now change the HSDir set, and call dir_info_changed() again. + * CHECK that reupload occurs. + * 4) Finally call service_desc_schedule_upload(). + * CHECK that previous_hsdirs list was cleared. + **/ + + /* Let's start by building our descriptor and service */ + hs_service_descriptor_t *desc = service_descriptor_new(); + hs_service_t *service = NULL; + char onion_addr[HS_SERVICE_ADDR_LEN_BASE32 + 1]; + ed25519_public_key_t pubkey; + memset(&pubkey, '\x42', sizeof(pubkey)); + hs_build_address(&pubkey, HS_VERSION_THREE, onion_addr); + service = tor_malloc_zero(sizeof(hs_service_t)); + memcpy(service->onion_address, onion_addr, sizeof(service->onion_address)); + ed25519_secret_key_generate(&service->keys.identity_sk, 0); + ed25519_public_key_generate(&service->keys.identity_pk, + &service->keys.identity_sk); + service->desc_current = desc; + /* Also add service to service map */ + hs_service_ht *service_map = get_hs_service_map(); + tt_assert(service_map); + tt_int_op(hs_service_get_num_services(), OP_EQ, 0); + register_service(service_map, service); + tt_int_op(hs_service_get_num_services(), OP_EQ, 1); + + /* 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); + } + + { /* 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); + } + + /* 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); + + /* 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 + * time should stay as is. */ + desc->next_upload_time = 42; + router_dir_info_changed(); + tt_int_op(desc->next_upload_time, OP_EQ, 42); + + /* 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); + } + + /* Now call router_dir_info_changed() again and see that it detected the hash + ring change and updated the upload time */ + time_t now = approx_time(); + tt_assert(now); + router_dir_info_changed(); + tt_int_op(desc->next_upload_time, OP_EQ, now); + + /* Now pretend that the descriptor changed, and order a reupload to all + HSDirs. Make sure that the set of previous HSDirs was cleared. */ + service_desc_schedule_upload(desc, now, 1); + tt_int_op(smartlist_len(desc->previous_hsdirs), OP_EQ, 0); + + /* 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); + + done: + hs_free_all(); +} + /** Test disaster SRV computation and caching */ static void test_disaster_srv(void *arg) @@ -550,7 +709,9 @@ struct testcase_t hs_common_tests[] = { NULL, NULL }, { "desc_overlap_period_testnet", test_desc_overlap_period_testnet, TT_FORK, NULL, NULL }, - { "desc_responsible_hsdirs", test_responsible_hsdirs, TT_FORK, + { "responsible_hsdirs", test_responsible_hsdirs, TT_FORK, + NULL, NULL }, + { "desc_reupload_logic", test_desc_reupload_logic, TT_FORK, NULL, NULL }, { "disaster_srv", test_disaster_srv, TT_FORK, NULL, NULL },