commit 7823c98a38556237a86c7235d411d7d2237cc2d6
Author: George Kadianakis <desnacked(a)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 },
