From 8dcbdf58a7fe3b1865b78387126cfe74dd0f0602 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Mon, 11 May 2015 14:46:15 -0400 Subject: [PATCH 1/7] Remove intro points adaptative algorithm Partially fixes #4862 Signed-off-by: David Goulet --- src/or/rendservice.c | 126 ++----------------------------------------- 1 file changed, 3 insertions(+), 123 deletions(-) diff --git a/src/or/rendservice.c b/src/or/rendservice.c index 89f95d7a00..a1c7af6d17 100644 --- a/src/or/rendservice.c +++ b/src/or/rendservice.c @@ -87,8 +87,6 @@ struct rend_service_port_config_s { /** Try to maintain this many intro points per service by default. */ #define NUM_INTRO_POINTS_DEFAULT 3 -/** Maintain no more than this many intro points per hidden service. */ -#define NUM_INTRO_POINTS_MAX 10 /** If we can't build our intro circuits, don't retry for this long. */ #define INTRO_CIRC_RETRY_PERIOD (60*5) @@ -177,17 +175,6 @@ num_rend_services(void) return smartlist_len(rend_service_list); } -/** Return a string identifying service, suitable for use in a - * log message. The result does not need to be freed, but may be - * overwritten by the next call to this function. */ -static const char * -rend_service_describe_for_log(rend_service_t *service) -{ - /* XXX024 Use this function throughout rendservice.c. */ - /* XXX024 Return a more useful description? */ - return safe_str_client(service->service_id); -} - /** Helper: free storage held by a single service authorized client entry. */ static void rend_authorized_client_free(rend_authorized_client_t *client) @@ -1417,107 +1404,6 @@ rend_check_authorization(rend_service_t *service, return 1; } -/** Called when intro will soon be removed from - * service's list of intro points. */ -static void -rend_service_note_removing_intro_point(rend_service_t *service, - rend_intro_point_t *intro) -{ - time_t now = time(NULL); - - /* Don't process an intro point twice here. */ - if (intro->rend_service_note_removing_intro_point_called) { - return; - } else { - intro->rend_service_note_removing_intro_point_called = 1; - } - - /* Update service->n_intro_points_wanted based on how long intro - * lasted and how many introductions it handled. */ - if (intro->time_published == -1) { - /* This intro point was never used. Don't change - * n_intro_points_wanted. */ - } else { - - /* We want to increase the number of introduction points service - * operates if intro was heavily used, or decrease the number of - * intro points if intro was lightly used. - * - * We consider an intro point's target 'usage' to be - * maximum of INTRODUCE2 cells divided by - * INTRO_POINT_LIFETIME_MIN_SECONDS seconds. To calculate intro's - * fraction of target usage, we divide the amount of INTRODUCE2 cells - * that it has handled by the fraction of _LIFETIME_MIN_SECONDS for - * which it existed. - * - * Then we multiply that fraction of desired usage by a fudge - * factor of 1.5, to decide how many new introduction points - * should ideally replace intro (which is now closed or soon to be - * closed). In theory, assuming that introduction load is - * distributed equally across all intro points and ignoring the - * fact that different intro points are established and closed at - * different times, that number of intro points should bring all - * of our intro points exactly to our target usage. - * - * Then we clamp that number to a number of intro points we might - * be willing to replace this intro point with and turn it into an - * integer. then we clamp it again to the number of new intro - * points we could establish now, then we adjust - * service->n_intro_points_wanted and let rend_services_introduce - * create the new intro points we want (if any). - */ - const double intro_point_usage = - intro_point_accepted_intro_count(intro) / - (double)(now - intro->time_published); - const double intro_point_target_usage = - intro->max_introductions / - (double)INTRO_POINT_LIFETIME_MIN_SECONDS; - const double fractional_n_intro_points_wanted_to_replace_this_one = - (1.5 * (intro_point_usage / intro_point_target_usage)); - unsigned int n_intro_points_wanted_to_replace_this_one; - unsigned int n_intro_points_wanted_now; - unsigned int n_intro_points_really_wanted_now; - int n_intro_points_really_replacing_this_one; - - if (fractional_n_intro_points_wanted_to_replace_this_one > - NUM_INTRO_POINTS_MAX) { - n_intro_points_wanted_to_replace_this_one = NUM_INTRO_POINTS_MAX; - } else if (fractional_n_intro_points_wanted_to_replace_this_one < 0) { - n_intro_points_wanted_to_replace_this_one = 0; - } else { - n_intro_points_wanted_to_replace_this_one = (unsigned) - fractional_n_intro_points_wanted_to_replace_this_one; - } - - n_intro_points_wanted_now = - service->n_intro_points_wanted + - n_intro_points_wanted_to_replace_this_one - 1; - - if (n_intro_points_wanted_now < NUM_INTRO_POINTS_DEFAULT) { - /* XXXX This should be NUM_INTRO_POINTS_MIN instead. Perhaps - * another use of NUM_INTRO_POINTS_DEFAULT should be, too. */ - n_intro_points_really_wanted_now = NUM_INTRO_POINTS_DEFAULT; - } else if (n_intro_points_wanted_now > NUM_INTRO_POINTS_MAX) { - n_intro_points_really_wanted_now = NUM_INTRO_POINTS_MAX; - } else { - n_intro_points_really_wanted_now = n_intro_points_wanted_now; - } - - n_intro_points_really_replacing_this_one = - n_intro_points_really_wanted_now - service->n_intro_points_wanted + 1; - - log_info(LD_REND, "Replacing closing intro point for service %s " - "with %d new intro points (wanted %g replacements); " - "service will now try to have %u intro points", - rend_service_describe_for_log(service), - n_intro_points_really_replacing_this_one, - fractional_n_intro_points_wanted_to_replace_this_one, - n_intro_points_really_wanted_now); - - service->n_intro_points_wanted = n_intro_points_really_wanted_now; - } -} - /****** * Handle cells ******/ @@ -3506,16 +3392,15 @@ rend_services_introduce(void) node = node_get_by_id(intro->extend_info->identity_digest); if (!node || !intro_circ) { - int removing_this_intro_point_changes_the_intro_point_set = 1; + intro_point_set_changed = 1; log_info(LD_REND, "Giving up on %s as intro point for %s" " (circuit disappeared).", safe_str_client(extend_info_describe(intro->extend_info)), safe_str_client(service->service_id)); - rend_service_note_removing_intro_point(service, intro); if (intro->time_expiring != -1) { log_info(LD_REND, "We were already expiring the intro point; " "no need to mark the HS descriptor as dirty over this."); - removing_this_intro_point_changes_the_intro_point_set = 0; + intro_point_set_changed = 0; } else if (intro->listed_in_last_desc) { log_info(LD_REND, "The intro point we are giving up on was " "included in the last published descriptor. " @@ -3525,17 +3410,12 @@ rend_services_introduce(void) rend_intro_point_free(intro); intro = NULL; /* SMARTLIST_DEL_CURRENT takes a name, not a value. */ SMARTLIST_DEL_CURRENT(service->intro_nodes, intro); - if (removing_this_intro_point_changes_the_intro_point_set) - intro_point_set_changed = 1; } if (intro != NULL && intro_point_should_expire_now(intro, now)) { log_info(LD_REND, "Expiring %s as intro point for %s.", safe_str_client(extend_info_describe(intro->extend_info)), safe_str_client(service->service_id)); - - rend_service_note_removing_intro_point(service, intro); - /* The polite (and generally Right) way to expire an intro * point is to establish a new one to replace it, publish a * new descriptor that doesn't list any expiring intro points, @@ -3561,7 +3441,7 @@ rend_services_introduce(void) } SMARTLIST_FOREACH_END(intro); if (!intro_point_set_changed && - (n_intro_points_unexpired >= service->n_intro_points_wanted)) { + (n_intro_points_unexpired == service->n_intro_points_wanted)) { continue; } From adc04580f860b5e8cfd6d49c83fdf73764a4f8cc Mon Sep 17 00:00:00 2001 From: David Goulet Date: Mon, 11 May 2015 15:48:04 -0400 Subject: [PATCH 2/7] Add the torrc option HiddenServiceNumIntroductionPoints This is a way to specify the amount of introduction points an hidden service can have. Maximum value is 10 and the default is 3. Fixes #4862 Signed-off-by: David Goulet --- changes/bug4862 | 8 ++++++++ doc/tor.1.txt | 4 ++++ src/or/config.c | 1 + src/or/rendservice.c | 19 ++++++++++++++++++- 4 files changed, 31 insertions(+), 1 deletion(-) create mode 100644 changes/bug4862 diff --git a/changes/bug4862 b/changes/bug4862 new file mode 100644 index 0000000000..e636395be3 --- /dev/null +++ b/changes/bug4862 @@ -0,0 +1,8 @@ + o Major feature (Hidden Service): + - Remove the introduction point adaptative algorithm which is leaking + popularity by changing the amount of introduction points depending on + the amount of traffic the HS sees. With this, we stick to only 3 + introduction points. + - Add the torrc option HiddenServiceNumIntroductionPoints for an + operatory to specify a fix amount of introduction points. Maximum + value is 10 and default is 3. diff --git a/doc/tor.1.txt b/doc/tor.1.txt index e7c08f5046..6cfad56f08 100644 --- a/doc/tor.1.txt +++ b/doc/tor.1.txt @@ -2177,6 +2177,10 @@ The following options are used to configure a hidden service. only owner is able to read the hidden service directory. (Default: 0) Has no effect on Windows. +[[HiddenServiceNumIntroductionPoints]] **HiddenServiceNumIntroductionPoints** __NUM__:: + Number of introduction points the hidden service will have. You can't + have more than 10. (Default: 3) + TESTING NETWORK OPTIONS ----------------------- diff --git a/src/or/config.c b/src/or/config.c index d81bc532b7..0d6c3003ff 100644 --- a/src/or/config.c +++ b/src/or/config.c @@ -288,6 +288,7 @@ static config_var_t option_vars_[] = { VAR("HiddenServiceAllowUnknownPorts",LINELIST_S, RendConfigLines, NULL), VAR("HiddenServiceMaxStreams",LINELIST_S, RendConfigLines, NULL), VAR("HiddenServiceMaxStreamsCloseCircuit",LINELIST_S, RendConfigLines, NULL), + VAR("HiddenServiceNumIntroductionPoints", LINELIST_S, RendConfigLines, NULL), V(HiddenServiceStatistics, BOOL, "0"), V(HidServAuth, LINELIST, NULL), V(CloseHSClientCircuitsImmediatelyOnTimeout, BOOL, "0"), diff --git a/src/or/rendservice.c b/src/or/rendservice.c index a1c7af6d17..aed01db693 100644 --- a/src/or/rendservice.c +++ b/src/or/rendservice.c @@ -87,6 +87,8 @@ struct rend_service_port_config_s { /** Try to maintain this many intro points per service by default. */ #define NUM_INTRO_POINTS_DEFAULT 3 +/** Maximum number of intro points per service. */ +#define NUM_INTRO_POINTS_MAX 10 /** If we can't build our intro circuits, don't retry for this long. */ #define INTRO_CIRC_RETRY_PERIOD (60*5) @@ -577,7 +579,22 @@ rend_config_services(const or_options_t *options, int validate_only) log_info(LD_CONFIG, "HiddenServiceMaxStreamsCloseCircuit=%d for %s", (int)service->max_streams_close_circuit, service->directory); - + } else if (!strcasecmp(line->key, "HiddenServiceNumIntroductionPoints")) { + service->n_intro_points_wanted = + (unsigned int) tor_parse_long(line->value, 10, + NUM_INTRO_POINTS_DEFAULT, + NUM_INTRO_POINTS_MAX, &ok, NULL); + if (!ok) { + log_warn(LD_CONFIG, + "HiddenServiceNumIntroductionPoints " + "should be between %d and %d, not %s", + NUM_INTRO_POINTS_DEFAULT, NUM_INTRO_POINTS_MAX, + line->value); + rend_service_free(service); + return -1; + } + log_info(LD_CONFIG, "HiddenServiceNumIntroductionPoints=%d for %s", + service->n_intro_points_wanted, service->directory); } else if (!strcasecmp(line->key, "HiddenServiceAuthorizeClient")) { /* Parse auth type and comma-separated list of client names and add a * rend_authorized_client_t for each client to the service's list From 7c7bb8b97ed1fd012fd8cd4cf16217a1757621ec Mon Sep 17 00:00:00 2001 From: David Goulet Date: Fri, 29 May 2015 17:45:45 -0400 Subject: [PATCH 3/7] Refactor rend_services_introduce() The reasoning for refactoring this function is that removing the introduction point adaptative algorithm (#4862) ended up changing quite a bit rend_services_introduce(). Also, to fix some open issues (#8239, #8864 and #13483), this work had to be done. First, this removes time_expiring variable in an intro point object and INTRO_POINT_EXPIRATION_GRACE_PERIOD trickery and use an expiring_nodes list where intro nodes that should expire are moved to that list and cleaned up only once the new descriptor is successfully uploaded. The previous scheme was adding complexity and arbitrary timing to when we expire an intro point. We keep the intro points until we are sure that the new descriptor is uploaded and thus ready to be used by clients. For this, rend_service_desc_has_uploaded() is added to notify the HS subsystem that the descriptor has been successfully uploaded. The purpose of this function is to cleanup the expiring nodes and circuits if any. Secondly, this adds the remove_invalid_intro_points() function in order to split up rend_services_introduce() a bit with an extra modification to it that fixes #8864. We do NOT close the circuit nor delete the intro point if the circuit is still alive but the node was removed from the consensus. Due to possible information leak, we let the circuit and intro point object expire instead. Finally, the whole code flow is simplified and large amount of documentation has been added to mostly explain the why of things in there. Fixes #8864 Signed-off-by: David Goulet --- src/or/directory.c | 2 + src/or/or.h | 15 -- src/or/rendservice.c | 340 +++++++++++++++++++++++-------------------- src/or/rendservice.h | 1 + 4 files changed, 182 insertions(+), 176 deletions(-) diff --git a/src/or/directory.c b/src/or/directory.c index 549d95a13c..8d7f9f4dea 100644 --- a/src/or/directory.c +++ b/src/or/directory.c @@ -23,6 +23,7 @@ #include "relay.h" #include "rendclient.h" #include "rendcommon.h" +#include "rendservice.h" #include "rephist.h" #include "router.h" #include "routerlist.h" @@ -2196,6 +2197,7 @@ connection_dir_client_reached_eof(dir_connection_t *conn) "Uploading rendezvous descriptor: finished with status " "200 (%s)", escaped(reason)); control_event_hs_descriptor_uploaded(conn->identity_digest); + rend_service_desc_has_uploaded(conn->rend_data); break; case 400: log_warn(LD_REND,"http status 400 (%s) response from dirserver " diff --git a/src/or/or.h b/src/or/or.h index d3a476ecf9..fc921a8e47 100644 --- a/src/or/or.h +++ b/src/or/or.h @@ -4905,11 +4905,6 @@ typedef struct rend_intro_point_t { * included in the last HS descriptor we generated. */ unsigned int listed_in_last_desc : 1; - /** (Service side only) Flag indicating that - * rend_service_note_removing_intro_point has been called for this - * intro point. */ - unsigned int rend_service_note_removing_intro_point_called : 1; - /** (Service side only) A replay cache recording the RSA-encrypted parts * of INTRODUCE2 cells this intro point's circuit has received. This is * used to prevent replay attacks. */ @@ -4935,16 +4930,6 @@ typedef struct rend_intro_point_t { * (start to) expire, or -1 if we haven't decided when this intro * point should expire. */ time_t time_to_expire; - - /** (Service side only) The time at which we decided that this intro - * point should start expiring, or -1 if this intro point is not yet - * expiring. - * - * This field also serves as a flag to indicate that we have decided - * to expire this intro point, in case intro_point_should_expire_now - * flaps (perhaps due to a clock jump; perhaps due to other - * weirdness, or even a (present or future) bug). */ - time_t time_expiring; } rend_intro_point_t; #define REND_PROTOCOL_VERSION_BITMASK_WIDTH 16 diff --git a/src/or/rendservice.c b/src/or/rendservice.c index aed01db693..2d6a458386 100644 --- a/src/or/rendservice.c +++ b/src/or/rendservice.c @@ -89,6 +89,9 @@ struct rend_service_port_config_s { #define NUM_INTRO_POINTS_DEFAULT 3 /** Maximum number of intro points per service. */ #define NUM_INTRO_POINTS_MAX 10 +/** Number of extra intro points we launch if our set of intro nodes is + * empty. See proposal 155, section 4. */ +#define NUM_INTRO_POINTS_EXTRA 2 /** If we can't build our intro circuits, don't retry for this long. */ #define INTRO_CIRC_RETRY_PERIOD (60*5) @@ -102,10 +105,6 @@ struct rend_service_port_config_s { * rendezvous point before giving up? */ #define MAX_REND_TIMEOUT 30 -/** How many seconds should we wait for new HS descriptors to reach - * our clients before we close an expiring intro point? */ -#define INTRO_POINT_EXPIRATION_GRACE_PERIOD (5*60) - /** Represents a single hidden service running at this OP. */ typedef struct rend_service_t { /* Fields specified in config file */ @@ -126,6 +125,10 @@ typedef struct rend_service_t { char pk_digest[DIGEST_LEN]; /**< Hash of permanent hidden-service key. */ smartlist_t *intro_nodes; /**< List of rend_intro_point_t's we have, * or are trying to establish. */ + /** List of rend_intro_point_t that are expiring. They are removed once + * the new descriptor is successfully uploaded. A node in this list CAN + * NOT appear in the intro_nodes list. */ + smartlist_t *expiring_nodes; time_t intro_period_started; /**< Start of the current period to build * introduction points. */ int n_intro_circuits_launched; /**< Count of intro circuits we have @@ -217,6 +220,11 @@ rend_service_free(rend_service_t *service) rend_intro_point_free(intro);); smartlist_free(service->intro_nodes); } + if (service->expiring_nodes) { + SMARTLIST_FOREACH(service->expiring_nodes, rend_intro_point_t *, intro, + rend_intro_point_free(intro);); + smartlist_free(service->expiring_nodes); + } rend_service_descriptor_free(service->desc); if (service->clients) { @@ -254,6 +262,7 @@ rend_add_service(rend_service_t *service) rend_service_port_config_t *p; service->intro_nodes = smartlist_new(); + service->expiring_nodes = smartlist_new(); if (service->max_streams_per_circuit < 0) { log_warn(LD_CONFIG, "Hidden service (%s) configured with negative max " @@ -769,6 +778,8 @@ rend_config_services(const or_options_t *options, int validate_only) !strcmp(old->directory, new->directory)) { smartlist_add_all(new->intro_nodes, old->intro_nodes); smartlist_clear(old->intro_nodes); + smartlist_add_all(new->expiring_nodes, old->expiring_nodes); + smartlist_clear(old->expiring_nodes); smartlist_add(surviving_services, old); break; } @@ -960,11 +971,6 @@ rend_service_update_descriptor(rend_service_t *service) /* This intro point won't be listed in the descriptor... */ intro_svc->listed_in_last_desc = 0; - if (intro_svc->time_expiring != -1) { - /* This intro point is expiring. Don't list it. */ - continue; - } - circ = find_intro_circuit(intro_svc, service->pk_digest); if (!circ || circ->base_.purpose != CIRCUIT_PURPOSE_S_INTRO) { /* This intro point's circuit isn't finished yet. Don't list it. */ @@ -2738,8 +2744,11 @@ rend_service_intro_has_opened(origin_circuit_t *circuit) } /* If we already have enough introduction circuits for this service, - * redefine this one as a general circuit or close it, depending. */ - if (count_established_intro_points(serviceid) > + * redefine this one as a general circuit or close it, depending. + * Substract the amount of expiring nodes here since the circuits are + * still opened. */ + if ((count_established_intro_points(serviceid) - + smartlist_len(service->expiring_nodes)) > (int)service->n_intro_points_wanted) { /* XXX023 remove cast */ const or_options_t *options = get_options(); if (options->ExcludeNodes) { @@ -3097,6 +3106,7 @@ directory_post_to_hs_dir(rend_service_descriptor_t *renddesc, char desc_id_base32[REND_DESC_ID_V2_LEN_BASE32 + 1]; char *hs_dir_ip; const node_t *node; + rend_data_t *rend_data; hs_dir = smartlist_get(responsible_dirs, j); if (smartlist_contains_digest(renddesc->successful_uploads, hs_dir->identity_digest)) @@ -3112,12 +3122,19 @@ directory_post_to_hs_dir(rend_service_descriptor_t *renddesc, continue; } /* Send publish request. */ - directory_initiate_command_routerstatus(hs_dir, - DIR_PURPOSE_UPLOAD_RENDDESC_V2, - ROUTER_PURPOSE_GENERAL, - DIRIND_ANONYMOUS, NULL, - desc->desc_str, - strlen(desc->desc_str), 0); + + /* We need the service ID to identify which service did the upload + * request. Lookup is made in rend_service_desc_has_uploaded(). */ + rend_data = rend_data_client_create(service_id, desc->desc_id, NULL, + REND_NO_AUTH); + directory_initiate_command_routerstatus_rend(hs_dir, + DIR_PURPOSE_UPLOAD_RENDDESC_V2, + ROUTER_PURPOSE_GENERAL, + DIRIND_ANONYMOUS, NULL, + desc->desc_str, + strlen(desc->desc_str), + 0, rend_data); + rend_data_free(rend_data); base32_encode(desc_id_base32, sizeof(desc_id_base32), desc->desc_id, DIGEST_LEN); hs_dir_ip = tor_dup_ip(hs_dir->addr); @@ -3300,12 +3317,6 @@ intro_point_should_expire_now(rend_intro_point_t *intro, return 0; } - if (intro->time_expiring != -1) { - /* We've already started expiring this intro point. *Don't* let - * this function's result 'flap'. */ - return 1; - } - if (intro_point_accepted_intro_count(intro) >= intro->max_introductions) { /* This intro point has been used too many times. Expire it now. */ @@ -3331,24 +3342,110 @@ intro_point_should_expire_now(rend_intro_point_t *intro, return (now >= intro->time_to_expire); } +/** Iterate over intro points in the given service and remove the invalid + * ones. For an intro point object to be considered invalid, the circuit + * needs to have disappeared. + * + * If the intro point should expire, it's placed into the expiring_nodes + * list of the service and removed from the active intro nodes list. + * + * If exclude_nodes is not NULL, add the valid nodes to it. */ +static void +remove_invalid_intro_points(rend_service_t *service, + smartlist_t *exclude_nodes, time_t now) +{ + tor_assert(service); + + SMARTLIST_FOREACH_BEGIN(service->intro_nodes, rend_intro_point_t *, + intro) { + /* Find the introduction point node object. */ + const node_t *node = + node_get_by_id(intro->extend_info->identity_digest); + /* Find the intro circuit, this might be NULL. */ + origin_circuit_t *intro_circ = + find_intro_circuit(intro, service->pk_digest); + + /* First, make sure we still have a valid circuit for this intro point. + * If we dont, we'll give up on it and make a new one. */ + if (intro_circ == NULL) { + log_info(LD_REND, "Giving up on %s as intro point for %s" + " (circuit disappeared).", + safe_str_client(extend_info_describe(intro->extend_info)), + safe_str_client(service->service_id)); + rend_intro_point_free(intro); + SMARTLIST_DEL_CURRENT(service->intro_nodes, intro); + /* Useful, it indicates if the intro point has been freed. */ + intro = NULL; + } + /* else, the circuit is valid so in both cases, node being alive or not, + * we leave the circuit and intro point object as is. Closing the + * circuit here would leak new consensus timing and freeing the intro + * point object would make the intro circuit unusable. */ + + /* Now, check if intro point should expire. If it does, queue it so + * it can be cleaned up once it has been replaced properly. */ + if (intro != NULL && intro_point_should_expire_now(intro, now)) { + log_info(LD_REND, "Expiring %s as intro point for %s.", + safe_str_client(extend_info_describe(intro->extend_info)), + safe_str_client(service->service_id)); + smartlist_add(service->expiring_nodes, intro); + SMARTLIST_DEL_CURRENT(service->intro_nodes, intro); + } + + /* Add the valid node to the exclusion list so we don't try to + * establish an introduction point to it again. */ + if (node && exclude_nodes) { + smartlist_add(exclude_nodes, (void*)node); + } + } SMARTLIST_FOREACH_END(intro); +} + +/** A new descriptor has been successfully uploaded for the given + * rend_data. Remove and free the expiring nodes from the associated + * service. */ +void +rend_service_desc_has_uploaded(const rend_data_t *rend_data) +{ + rend_service_t *service; + + tor_assert(rend_data); + + service = rend_service_get_by_service_id(rend_data->onion_address); + if (service == NULL) { + log_warn(LD_REND, "Service %s not found after descriptor upload", + safe_str_client(rend_data->onion_address)); + return; + } + + SMARTLIST_FOREACH_BEGIN(service->expiring_nodes, rend_intro_point_t *, + intro) { + origin_circuit_t *intro_circ = + find_intro_circuit(intro, service->pk_digest); + if (intro_circ != NULL) { + circuit_mark_for_close(TO_CIRCUIT(intro_circ), + END_CIRC_REASON_FINISHED); + } + SMARTLIST_DEL_CURRENT(service->expiring_nodes, intro); + rend_intro_point_free(intro); + } SMARTLIST_FOREACH_END(intro); +} + /** For every service, check how many intro points it currently has, and: + * - Invalidate introdution points based on specific criteria, see + * remove_invalid_intro_points comments. * - Pick new intro points as necessary. * - Launch circuits to any new intro points. + * + * This is called once a second by the main loop. */ void rend_services_introduce(void) { - int i,j,r; - const node_t *node; - rend_service_t *service; - rend_intro_point_t *intro; - int intro_point_set_changed, prev_intro_nodes; - unsigned int n_intro_points_unexpired; - unsigned int n_intro_points_to_open; + int i; time_t now; const or_options_t *options = get_options(); - /* List of nodes we need to _exclude_ when choosing a new node to establish - * an intro point to. */ + /* List of nodes we need to _exclude_ when choosing a new node to + * establish an intro point to. */ smartlist_t *exclude_nodes; if (!have_completed_a_circuit()) @@ -3357,22 +3454,20 @@ rend_services_introduce(void) exclude_nodes = smartlist_new(); now = time(NULL); - for (i=0; i < smartlist_len(rend_service_list); ++i) { + SMARTLIST_FOREACH_BEGIN(rend_service_list, rend_service_t *, service) { + /* Number of intro points we want to open and add to the intro nodes + * list of the service. */ + unsigned int n_intro_points_to_open; + /* Have an unsigned len so we can use it to compare values else gcc is + * not happy with unmatching signed comparaison. */ + unsigned int intro_nodes_len; + /* Different service are allowed to have the same introduction point as + * long as they are on different circuit thus why we clear this list. */ smartlist_clear(exclude_nodes); - service = smartlist_get(rend_service_list, i); - tor_assert(service); - - /* intro_point_set_changed becomes non-zero iff the set of intro - * points to be published in service's descriptor has changed. */ - intro_point_set_changed = 0; - - /* n_intro_points_unexpired collects the number of non-expiring - * intro points we have, so that we know how many new intro - * circuits we need to launch for this service. */ - n_intro_points_unexpired = 0; - - if (now > service->intro_period_started+INTRO_CIRC_RETRY_PERIOD) { + /* This retry period is important here so we don't stress circuit + * creation. */ + if (now > service->intro_period_started + INTRO_CIRC_RETRY_PERIOD) { /* One period has elapsed; we can try building circuits again. */ service->intro_period_started = now; service->n_intro_circuits_launched = 0; @@ -3383,110 +3478,41 @@ rend_services_introduce(void) continue; } - /* Find out which introduction points we have in progress for this - service. */ - SMARTLIST_FOREACH_BEGIN(service->intro_nodes, rend_intro_point_t *, - intro) { - origin_circuit_t *intro_circ = - find_intro_circuit(intro, service->pk_digest); + /* Cleanup the invalid intro points and save the node objects, if any, + * in the exclude_nodes list. */ + remove_invalid_intro_points(service, exclude_nodes, now); + intro_nodes_len = (unsigned int) smartlist_len(service->intro_nodes); - if (intro->time_expiring + INTRO_POINT_EXPIRATION_GRACE_PERIOD > now) { - /* This intro point has completely expired. Remove it, and - * mark the circuit for close if it's still alive. */ - if (intro_circ != NULL && - intro_circ->base_.purpose != CIRCUIT_PURPOSE_PATH_BIAS_TESTING) { - circuit_mark_for_close(TO_CIRCUIT(intro_circ), - END_CIRC_REASON_FINISHED); - } - rend_intro_point_free(intro); - intro = NULL; /* SMARTLIST_DEL_CURRENT takes a name, not a value. */ - SMARTLIST_DEL_CURRENT(service->intro_nodes, intro); - /* We don't need to set intro_point_set_changed here, because - * this intro point wouldn't have been published in a current - * descriptor anyway. */ - continue; - } - - node = node_get_by_id(intro->extend_info->identity_digest); - if (!node || !intro_circ) { - intro_point_set_changed = 1; - log_info(LD_REND, "Giving up on %s as intro point for %s" - " (circuit disappeared).", - safe_str_client(extend_info_describe(intro->extend_info)), - safe_str_client(service->service_id)); - if (intro->time_expiring != -1) { - log_info(LD_REND, "We were already expiring the intro point; " - "no need to mark the HS descriptor as dirty over this."); - intro_point_set_changed = 0; - } else if (intro->listed_in_last_desc) { - log_info(LD_REND, "The intro point we are giving up on was " - "included in the last published descriptor. " - "Marking current descriptor as dirty."); - service->desc_is_dirty = now; - } - rend_intro_point_free(intro); - intro = NULL; /* SMARTLIST_DEL_CURRENT takes a name, not a value. */ - SMARTLIST_DEL_CURRENT(service->intro_nodes, intro); - } - - if (intro != NULL && intro_point_should_expire_now(intro, now)) { - log_info(LD_REND, "Expiring %s as intro point for %s.", - safe_str_client(extend_info_describe(intro->extend_info)), - safe_str_client(service->service_id)); - /* The polite (and generally Right) way to expire an intro - * point is to establish a new one to replace it, publish a - * new descriptor that doesn't list any expiring intro points, - * and *then*, once our upload attempts for the new descriptor - * have ended (whether in success or failure), close the - * expiring intro points. - * - * Unfortunately, we can't find out when the new descriptor - * has actually been uploaded, so we'll have to settle for a - * five-minute timer. Start it. XXXX024 This sucks. */ - intro->time_expiring = now; - - intro_point_set_changed = 1; - } - - if (intro != NULL && intro->time_expiring == -1) - ++n_intro_points_unexpired; - - /* Add the valid node to the exclusion list so we don't try to establish - * an introduction point to it again. */ - if (node) - smartlist_add(exclude_nodes, (void*)node); - } SMARTLIST_FOREACH_END(intro); - - if (!intro_point_set_changed && - (n_intro_points_unexpired == service->n_intro_points_wanted)) { + /* Quiescent state, no node expiring and we have more or the amount of + * wanted node for this service. Proceed to the next service. Could be + * more because we launch two preemptive circuits if our intro nodes + * list is empty. */ + if (smartlist_len(service->expiring_nodes) == 0 && + intro_nodes_len >= service->n_intro_points_wanted) { continue; } - /* Remember how many introduction circuits we started with. - * - * prev_intro_nodes serves a different purpose than - * n_intro_points_unexpired -- this variable tells us where our - * previously-created intro points end and our new ones begin in - * the intro-point list, so we don't have to launch the circuits - * at the same time as we create the intro points they correspond - * to. XXXX This is daft. */ - prev_intro_nodes = smartlist_len(service->intro_nodes); + /* Number of intro points we want to open which is the wanted amount + * minus the current amount of valid nodes. */ + n_intro_points_to_open = service->n_intro_points_wanted - intro_nodes_len; + if (intro_nodes_len == 0) { + /* We want to end up with n_intro_points_wanted intro points, but if + * we have no intro points at all (chances are they all cycled or we + * are starting up), we launch NUM_INTRO_POINTS_EXTRA extra circuits + * and use the first n_intro_points_wanted that complete. See proposal + * #155, section 4 for the rationale of this which is purely for + * performance. + * + * The ones after the first n_intro_points_to_open will be converted + * to 'general' internal circuits in rend_service_intro_has_opened(), + * and then we'll drop them from the list of intro points. */ + n_intro_points_to_open += NUM_INTRO_POINTS_EXTRA; + } - /* We have enough directory information to start establishing our - * intro points. We want to end up with n_intro_points_wanted - * intro points, but if we're just starting, we launch two extra - * circuits and use the first n_intro_points_wanted that complete. - * - * The ones after the first three will be converted to 'general' - * internal circuits in rend_service_intro_has_opened(), and then - * we'll drop them from the list of intro points next time we - * go through the above "find out which introduction points we have - * in progress" loop. */ - n_intro_points_to_open = (service->n_intro_points_wanted + - (prev_intro_nodes == 0 ? 2 : 0)); - for (j = (int)n_intro_points_unexpired; - j < (int)n_intro_points_to_open; - ++j) { /* XXXX remove casts */ + for (i = 0; i < (int) n_intro_points_to_open; i++) { + int r; + const node_t *node; + rend_intro_point_t *intro; router_crn_flags_t flags = CRN_NEED_UPTIME|CRN_NEED_DESC; if (get_options()->AllowInvalid_ & ALLOW_INVALID_INTRODUCTION) flags |= CRN_ALLOW_INVALID; @@ -3494,16 +3520,15 @@ rend_services_introduce(void) options->ExcludeNodes, flags); if (!node) { log_warn(LD_REND, - "Could only establish %d introduction points for %s; " + "We only have %d introduction points established for %s; " "wanted %u.", smartlist_len(service->intro_nodes), safe_str_client(service->service_id), n_intro_points_to_open); break; } - intro_point_set_changed = 1; - /* Add the choosen node to the exclusion list in order to avoid to pick - * it again in the next iteration. */ + /* Add the choosen node to the exclusion list in order to avoid to + * pick it again in the next iteration. */ smartlist_add(exclude_nodes, (void*)node); intro = tor_malloc_zero(sizeof(rend_intro_point_t)); intro->extend_info = extend_info_from_node(node, 0); @@ -3512,7 +3537,6 @@ rend_services_introduce(void) tor_assert(!fail); intro->time_published = -1; intro->time_to_expire = -1; - intro->time_expiring = -1; intro->max_introductions = crypto_rand_int_range(INTRO_POINT_MIN_LIFETIME_INTRODUCTIONS, INTRO_POINT_MAX_LIFETIME_INTRODUCTIONS); @@ -3520,23 +3544,17 @@ rend_services_introduce(void) log_info(LD_REND, "Picked router %s as an intro point for %s.", safe_str_client(node_describe(node)), safe_str_client(service->service_id)); - } - - /* If there's no need to launch new circuits, stop here. */ - if (!intro_point_set_changed) - continue; - - /* Establish new introduction points. */ - for (j=prev_intro_nodes; j < smartlist_len(service->intro_nodes); ++j) { - intro = smartlist_get(service->intro_nodes, j); + /* Establish new introduction circuit to our chosen intro point. */ r = rend_service_launch_establish_intro(service, intro); - if (r<0) { + if (r < 0) { log_warn(LD_REND, "Error launching circuit to node %s for service %s.", safe_str_client(extend_info_describe(intro->extend_info)), safe_str_client(service->service_id)); + /* This funcion will be called again by the main loop so this intro + * point without a intro circuit will be removed. */ } } - } + } SMARTLIST_FOREACH_END(service); smartlist_free(exclude_nodes); } diff --git a/src/or/rendservice.h b/src/or/rendservice.h index b540d2c8ad..35a0bb25e6 100644 --- a/src/or/rendservice.h +++ b/src/or/rendservice.h @@ -125,6 +125,7 @@ int rend_service_del_ephemeral(const char *service_id); void directory_post_to_hs_dir(rend_service_descriptor_t *renddesc, smartlist_t *descs, smartlist_t *hs_dirs, const char *service_id, int seconds_valid); +void rend_service_desc_has_uploaded(const rend_data_t *rend_data); #endif From 1125a4876b455d41b4c858cc97e8f8feef0fa8d0 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Mon, 1 Jun 2015 12:08:13 -0400 Subject: [PATCH 4/7] Reuse intro points that failed but are still valid There is a case where if the introduction circuit fails but the node is still in the consensus, we clean up the intro point and choose an other one. This commit fixes that by trying to reuse the existing intro point with a maximum value of retry. A retry_nodes list is added to rend_services_introduce() and when we remove an invalid intro points that fits the use case mentionned before, we add the node to the retry list instead of removing it. Then, we retry on them before creating new ones. This means that the requirement to remove an intro point changes from "if no intro circuit" to "if no intro circuit then if no node OR we've reached our maximum circuit creation count". For now, the maximum retries is set to 3 which it completely arbitrary. It should also at some point be tied to the work done on detecting if our network is down or not. Fixes #8239 Signed-off-by: David Goulet --- src/or/or.h | 12 +++++++ src/or/rendservice.c | 85 ++++++++++++++++++++++++++++++++++---------- 2 files changed, 79 insertions(+), 18 deletions(-) diff --git a/src/or/or.h b/src/or/or.h index fc921a8e47..0deb4a79be 100644 --- a/src/or/or.h +++ b/src/or/or.h @@ -4881,6 +4881,11 @@ typedef struct rend_encoded_v2_service_descriptor_t { * XXX023 Should this be configurable? */ #define INTRO_POINT_LIFETIME_MAX_SECONDS (24*60*60) +/** The maximum number of circuit creation retry we do to an intro point + * before giving up. We try to reuse intro point that fails during their + * lifetime so this is a hard limit on the amount of time we do that. */ +#define MAX_INTRO_POINT_CIRCUIT_RETRIES 3 + /** Introduction point information. Used both in rend_service_t (on * the service side) and in rend_service_descriptor_t (on both the * client and service side). */ @@ -4930,6 +4935,13 @@ typedef struct rend_intro_point_t { * (start to) expire, or -1 if we haven't decided when this intro * point should expire. */ time_t time_to_expire; + + /** (Service side only) The amount of circuit creation we've made to this + * intro point. This is incremented every time we do a circuit relaunch on + * this object which is triggered when the circuit dies but the node is + * still in the consensus. After MAX_INTRO_POINT_CIRCUIT_RETRIES, we give + * up on it. */ + unsigned int circuit_retries; } rend_intro_point_t; #define REND_PROTOCOL_VERSION_BITMASK_WIDTH 16 diff --git a/src/or/rendservice.c b/src/or/rendservice.c index 2d6a458386..8c383a8c12 100644 --- a/src/or/rendservice.c +++ b/src/or/rendservice.c @@ -2751,6 +2751,14 @@ rend_service_intro_has_opened(origin_circuit_t *circuit) smartlist_len(service->expiring_nodes)) > (int)service->n_intro_points_wanted) { /* XXX023 remove cast */ const or_options_t *options = get_options(); + /* Remove the intro point associated with this circuit, it's being + * repurposed or closed thus cleanup memory. */ + rend_intro_point_t *intro = find_intro_point(circuit); + if (intro != NULL) { + smartlist_remove(service->intro_nodes, intro); + rend_intro_point_free(intro); + } + if (options->ExcludeNodes) { /* XXXX in some future version, we can test whether the transition is allowed or not given the actual nodes in the circuit. But for now, @@ -3344,15 +3352,19 @@ intro_point_should_expire_now(rend_intro_point_t *intro, /** Iterate over intro points in the given service and remove the invalid * ones. For an intro point object to be considered invalid, the circuit - * needs to have disappeared. + * _and_ node need to have disappeared. * * If the intro point should expire, it's placed into the expiring_nodes * list of the service and removed from the active intro nodes list. * - * If exclude_nodes is not NULL, add the valid nodes to it. */ + * If exclude_nodes is not NULL, add the valid nodes to it. + * + * If retry_nodes is not NULL, add the valid node to it if the + * circuit disappeared but the node is still in the consensus. */ static void remove_invalid_intro_points(rend_service_t *service, - smartlist_t *exclude_nodes, time_t now) + smartlist_t *exclude_nodes, + smartlist_t *retry_nodes, time_t now) { tor_assert(service); @@ -3365,6 +3377,12 @@ remove_invalid_intro_points(rend_service_t *service, origin_circuit_t *intro_circ = find_intro_circuit(intro, service->pk_digest); + /* Add the valid node to the exclusion list so we don't try to establish + * an introduction point to it again. */ + if (node && exclude_nodes) { + smartlist_add(exclude_nodes, (void*) node); + } + /* First, make sure we still have a valid circuit for this intro point. * If we dont, we'll give up on it and make a new one. */ if (intro_circ == NULL) { @@ -3372,10 +3390,23 @@ remove_invalid_intro_points(rend_service_t *service, " (circuit disappeared).", safe_str_client(extend_info_describe(intro->extend_info)), safe_str_client(service->service_id)); - rend_intro_point_free(intro); - SMARTLIST_DEL_CURRENT(service->intro_nodes, intro); - /* Useful, it indicates if the intro point has been freed. */ - intro = NULL; + /* Node is gone or we've reached our maximum circuit creationg retry + * count, clean up everything, we'll find a new one. */ + if (node == NULL || + intro->circuit_retries >= MAX_INTRO_POINT_CIRCUIT_RETRIES) { + rend_intro_point_free(intro); + SMARTLIST_DEL_CURRENT(service->intro_nodes, intro); + /* We've just killed the intro point, nothing left to do. */ + continue; + } + + /* The intro point is still alive so let's try to use it again because + * we have a published descriptor containing it. Keep the intro point + * in the intro_nodes list because it's still valid, we are rebuilding + * a circuit to it. */ + if (retry_nodes) { + smartlist_add(retry_nodes, intro); + } } /* else, the circuit is valid so in both cases, node being alive or not, * we leave the circuit and intro point object as is. Closing the @@ -3384,19 +3415,13 @@ remove_invalid_intro_points(rend_service_t *service, /* Now, check if intro point should expire. If it does, queue it so * it can be cleaned up once it has been replaced properly. */ - if (intro != NULL && intro_point_should_expire_now(intro, now)) { + if (intro_point_should_expire_now(intro, now)) { log_info(LD_REND, "Expiring %s as intro point for %s.", safe_str_client(extend_info_describe(intro->extend_info)), safe_str_client(service->service_id)); smartlist_add(service->expiring_nodes, intro); SMARTLIST_DEL_CURRENT(service->intro_nodes, intro); } - - /* Add the valid node to the exclusion list so we don't try to - * establish an introduction point to it again. */ - if (node && exclude_nodes) { - smartlist_add(exclude_nodes, (void*)node); - } } SMARTLIST_FOREACH_END(intro); } @@ -3447,14 +3472,19 @@ rend_services_introduce(void) /* List of nodes we need to _exclude_ when choosing a new node to * establish an intro point to. */ smartlist_t *exclude_nodes; + /* List of nodes we need to retry to build a circuit on them because the + * node is valid but circuit died. */ + smartlist_t *retry_nodes; if (!have_completed_a_circuit()) return; exclude_nodes = smartlist_new(); + retry_nodes = smartlist_new(); now = time(NULL); SMARTLIST_FOREACH_BEGIN(rend_service_list, rend_service_t *, service) { + int r; /* Number of intro points we want to open and add to the intro nodes * list of the service. */ unsigned int n_intro_points_to_open; @@ -3464,6 +3494,7 @@ rend_services_introduce(void) /* Different service are allowed to have the same introduction point as * long as they are on different circuit thus why we clear this list. */ smartlist_clear(exclude_nodes); + smartlist_clear(retry_nodes); /* This retry period is important here so we don't stress circuit * creation. */ @@ -3478,9 +3509,27 @@ rend_services_introduce(void) continue; } - /* Cleanup the invalid intro points and save the node objects, if any, - * in the exclude_nodes list. */ - remove_invalid_intro_points(service, exclude_nodes, now); + /* Cleanup the invalid intro points and save the node objects, if apply, + * in the exclude_nodes and retry_nodes list. */ + remove_invalid_intro_points(service, exclude_nodes, retry_nodes, now); + + /* Let's try to rebuild circuit on the nodes we want to retry on. */ + SMARTLIST_FOREACH_BEGIN(retry_nodes, rend_intro_point_t *, intro) { + r = rend_service_launch_establish_intro(service, intro); + if (r < 0) { + log_warn(LD_REND, "Error launching circuit to node %s for service %s.", + safe_str_client(extend_info_describe(intro->extend_info)), + safe_str_client(service->service_id)); + /* Unable to launch a circuit to that intro point, remove it from + * the valid list so we can create a new one. */ + smartlist_remove(service->intro_nodes, intro); + rend_intro_point_free(intro); + continue; + } + intro->circuit_retries++; + } SMARTLIST_FOREACH_END(intro); + + /* Avoid mismatched signed comparaison below. */ intro_nodes_len = (unsigned int) smartlist_len(service->intro_nodes); /* Quiescent state, no node expiring and we have more or the amount of @@ -3510,7 +3559,6 @@ rend_services_introduce(void) } for (i = 0; i < (int) n_intro_points_to_open; i++) { - int r; const node_t *node; rend_intro_point_t *intro; router_crn_flags_t flags = CRN_NEED_UPTIME|CRN_NEED_DESC; @@ -3556,6 +3604,7 @@ rend_services_introduce(void) } } SMARTLIST_FOREACH_END(service); smartlist_free(exclude_nodes); + smartlist_free(retry_nodes); } #define MIN_REND_INITIAL_POST_DELAY (30) From d67bf8b2f2376096ada2b1ea1c6cd19ddbed9ead Mon Sep 17 00:00:00 2001 From: David Goulet Date: Mon, 1 Jun 2015 13:17:37 -0400 Subject: [PATCH 5/7] Upload descriptor when all intro points are ready To upload a HS descriptor, this commits makes it that we wait for all introduction point to be fully established. Else, the HS ends up uploading a descriptor that may contain intro points that are not yet "valid" meaning not yet established or proven to work. It could also trigger three uploads for the *same* descriptor if every intro points takes more than 30 seconds to establish because of desc_is_dirty being set at each intro established. To achieve that, n_intro_points_established varialbe is added to the rend_service_t object that is incremented when we established introduction point and decremented when we remove a valid intro point from our list. The condition to upload a descriptor also changes to test if all intro points are ready by making sure we have equal or more wanted intro points that are ready. The desc_id_dirty flag is kept to be able to still use the RendInitialPostPeriod option. This partially fixes #13483. Signed-off-by: David Goulet --- src/or/or.h | 4 +++ src/or/rendservice.c | 58 ++++++++++++++++++++++++++++---------------- 2 files changed, 41 insertions(+), 21 deletions(-) diff --git a/src/or/or.h b/src/or/or.h index 0deb4a79be..55ad1ddefa 100644 --- a/src/or/or.h +++ b/src/or/or.h @@ -4942,6 +4942,10 @@ typedef struct rend_intro_point_t { * still in the consensus. After MAX_INTRO_POINT_CIRCUIT_RETRIES, we give * up on it. */ unsigned int circuit_retries; + + /** (Service side only) Set if this intro point has an established circuit + * and unset if it doesn't. */ + unsigned int circuit_established:1; } rend_intro_point_t; #define REND_PROTOCOL_VERSION_BITMASK_WIDTH 16 diff --git a/src/or/rendservice.c b/src/or/rendservice.c index 8c383a8c12..a86245b9f4 100644 --- a/src/or/rendservice.c +++ b/src/or/rendservice.c @@ -2689,24 +2689,16 @@ rend_service_launch_establish_intro(rend_service_t *service, } /** Return the number of introduction points that are or have been - * established for the given service address in query. */ -static int -count_established_intro_points(const char *query) + * established for the given service. */ +static unsigned int +count_established_intro_points(const rend_service_t *service) { - int num_ipos = 0; - SMARTLIST_FOREACH_BEGIN(circuit_get_global_list(), circuit_t *, circ) { - if (!circ->marked_for_close && - circ->state == CIRCUIT_STATE_OPEN && - (circ->purpose == CIRCUIT_PURPOSE_S_ESTABLISH_INTRO || - circ->purpose == CIRCUIT_PURPOSE_S_INTRO)) { - origin_circuit_t *oc = TO_ORIGIN_CIRCUIT(circ); - if (oc->rend_data && - !rend_cmp_service_ids(query, oc->rend_data->onion_address)) - num_ipos++; - } - } - SMARTLIST_FOREACH_END(circ); - return num_ipos; + unsigned int num = 0; + + SMARTLIST_FOREACH(service->intro_nodes, rend_intro_point_t *, intro, + num += intro->circuit_established + ); + return num; } /** Called when we're done building a circuit to an introduction point: @@ -2747,9 +2739,8 @@ rend_service_intro_has_opened(origin_circuit_t *circuit) * redefine this one as a general circuit or close it, depending. * Substract the amount of expiring nodes here since the circuits are * still opened. */ - if ((count_established_intro_points(serviceid) - - smartlist_len(service->expiring_nodes)) > - (int)service->n_intro_points_wanted) { /* XXX023 remove cast */ + if (count_established_intro_points(service) > + service->n_intro_points_wanted) { const or_options_t *options = get_options(); /* Remove the intro point associated with this circuit, it's being * repurposed or closed thus cleanup memory. */ @@ -2857,6 +2848,7 @@ rend_service_intro_established(origin_circuit_t *circuit, size_t request_len) { rend_service_t *service; + rend_intro_point_t *intro; char serviceid[REND_SERVICE_ID_LEN_BASE32+1]; (void) request; (void) request_len; @@ -2874,6 +2866,19 @@ rend_service_intro_established(origin_circuit_t *circuit, (unsigned)circuit->base_.n_circ_id); goto err; } + /* We've just successfully established a intro circuit to one of our + * introduction point, account for it. */ + intro = find_intro_point(circuit); + if (intro == NULL) { + log_warn(LD_REND, + "Introduction circuit established without a rend_intro_point_t " + "object for service %s on circuit %u", + safe_str_client(serviceid), (unsigned)circuit->base_.n_circ_id); + goto err; + } + intro->circuit_established = 1; + /* We might not have every introduction point ready but at this point we + * know that the descriptor needs to be uploaded. */ service->desc_is_dirty = time(NULL); circuit_change_purpose(TO_CIRCUIT(circuit), CIRCUIT_PURPOSE_S_INTRO); @@ -3390,6 +3395,10 @@ remove_invalid_intro_points(rend_service_t *service, " (circuit disappeared).", safe_str_client(extend_info_describe(intro->extend_info)), safe_str_client(service->service_id)); + /* We've lost the circuit for this intro point, flag it so it can be + * accounted for when considiring uploading a descriptor. */ + intro->circuit_established = 0; + /* Node is gone or we've reached our maximum circuit creationg retry * count, clean up everything, we'll find a new one. */ if (node == NULL || @@ -3421,6 +3430,9 @@ remove_invalid_intro_points(rend_service_t *service, safe_str_client(service->service_id)); smartlist_add(service->expiring_nodes, intro); SMARTLIST_DEL_CURRENT(service->intro_nodes, intro); + /* Intro point is expired, we need a new one thus don't consider it + * anymore has a valid established intro point. */ + intro->circuit_established = 0; } } SMARTLIST_FOREACH_END(intro); } @@ -3639,9 +3651,13 @@ rend_consider_services_upload(time_t now) service->next_upload_time = now + rendinitialpostdelay + crypto_rand_int(2*rendpostperiod); } + /* Does every introduction points have been established? */ + unsigned int intro_points_ready = + count_established_intro_points(service) >= service->n_intro_points_wanted; if (service->next_upload_time < now || (service->desc_is_dirty && - service->desc_is_dirty < now-rendinitialpostdelay)) { + service->desc_is_dirty < now-rendinitialpostdelay && + intro_points_ready)) { /* if it's time, or if the directory servers have a wrong service * descriptor and ours has been stable for rendinitialpostdelay seconds, * upload a new one of each format. */ From 7657194d77fb5bec5b8e3ef2f4171f5ea54fb519 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Mon, 15 Jun 2015 17:11:57 -0400 Subject: [PATCH 6/7] Count intro circuit and not only established ones When cleaning up extra circuits that we've opened for performance reason, we need to count all the introduction circuit and not only the established ones else we can end up with too many introduction points. This also adds the check for expiring nodes when serving an INTRODUCE cell since it's possible old clients are still using them before we have time to close them. Signed-off-by: David Goulet --- src/or/rendservice.c | 73 ++++++++++++++++++++++++++++++++++++-------- 1 file changed, 60 insertions(+), 13 deletions(-) diff --git a/src/or/rendservice.c b/src/or/rendservice.c index a86245b9f4..85f0fc7772 100644 --- a/src/or/rendservice.c +++ b/src/or/rendservice.c @@ -31,9 +31,12 @@ #include "routerparse.h" #include "routerset.h" +struct rend_service_t; static origin_circuit_t *find_intro_circuit(rend_intro_point_t *intro, const char *pk_digest); static rend_intro_point_t *find_intro_point(origin_circuit_t *circ); +static rend_intro_point_t *find_expiring_intro_point( + struct rend_service_t *service, origin_circuit_t *circ); static extend_info_t *find_rp_for_intro( const rend_intro_cell_t *intro, @@ -42,7 +45,6 @@ static extend_info_t *find_rp_for_intro( static int intro_point_accepted_intro_count(rend_intro_point_t *intro); static int intro_point_should_expire_now(rend_intro_point_t *intro, time_t now); -struct rend_service_t; static int rend_service_derive_key_digests(struct rend_service_t *s); static int rend_service_load_keys(struct rend_service_t *s); static int rend_service_load_auth_keys(struct rend_service_t *s, @@ -1503,12 +1505,15 @@ rend_service_introduce(origin_circuit_t *circuit, const uint8_t *request, intro_point = find_intro_point(circuit); if (intro_point == NULL) { - log_warn(LD_BUG, - "Internal error: Got an INTRODUCE2 cell on an " - "intro circ (for service %s) with no corresponding " - "rend_intro_point_t.", - escaped(serviceid)); - goto err; + intro_point = find_expiring_intro_point(service, circuit); + if (intro_point == NULL) { + log_warn(LD_BUG, + "Internal error: Got an INTRODUCE2 cell on an " + "intro circ (for service %s) with no corresponding " + "rend_intro_point_t.", + escaped(serviceid)); + goto err; + } } log_info(LD_REND, "Received INTRODUCE2 cell for service %s on circ %u.", @@ -2688,8 +2693,8 @@ rend_service_launch_establish_intro(rend_service_t *service, return 0; } -/** Return the number of introduction points that are or have been - * established for the given service. */ +/** Return the number of introduction points that are established for the + * given service. */ static unsigned int count_established_intro_points(const rend_service_t *service) { @@ -2701,6 +2706,30 @@ count_established_intro_points(const rend_service_t *service) return num; } +/** Return the number of introduction points that are or are being + * established for the given service. This function iterates over all + * circuit and count those that are linked to the service and are waiting + * for the intro point to respond. */ +static unsigned int +count_intro_point_circuits(const rend_service_t *service) +{ + unsigned int num_ipos = 0; + SMARTLIST_FOREACH_BEGIN(circuit_get_global_list(), circuit_t *, circ) { + if (!circ->marked_for_close && + circ->state == CIRCUIT_STATE_OPEN && + (circ->purpose == CIRCUIT_PURPOSE_S_ESTABLISH_INTRO || + circ->purpose == CIRCUIT_PURPOSE_S_INTRO)) { + origin_circuit_t *oc = TO_ORIGIN_CIRCUIT(circ); + if (oc->rend_data && + !rend_cmp_service_ids(service->service_id, + oc->rend_data->onion_address)) + num_ipos++; + } + } + SMARTLIST_FOREACH_END(circ); + return num_ipos; +} + /** Called when we're done building a circuit to an introduction point: * sends a RELAY_ESTABLISH_INTRO cell. */ @@ -2739,7 +2768,8 @@ rend_service_intro_has_opened(origin_circuit_t *circuit) * redefine this one as a general circuit or close it, depending. * Substract the amount of expiring nodes here since the circuits are * still opened. */ - if (count_established_intro_points(service) > + if ((count_intro_point_circuits(service) - + smartlist_len(service->expiring_nodes)) > service->n_intro_points_wanted) { const or_options_t *options = get_options(); /* Remove the intro point associated with this circuit, it's being @@ -3053,6 +3083,23 @@ find_intro_circuit(rend_intro_point_t *intro, const char *pk_digest) return NULL; } +/** Return the corresponding introdution point using the circuit circ + * found in the service. NULL is returned if not found. */ +static rend_intro_point_t * +find_expiring_intro_point(rend_service_t *service, origin_circuit_t *circ) +{ + tor_assert(service); + tor_assert(TO_CIRCUIT(circ)->purpose == CIRCUIT_PURPOSE_S_ESTABLISH_INTRO || + TO_CIRCUIT(circ)->purpose == CIRCUIT_PURPOSE_S_INTRO); + + SMARTLIST_FOREACH(service->intro_nodes, rend_intro_point_t *, intro_point, + if (crypto_pk_eq_keys(intro_point->intro_key, circ->intro_key)) { + return intro_point; + }); + + return NULL; +} + /** Return a pointer to the rend_intro_point_t corresponding to the * service-side introduction circuit circ. */ static rend_intro_point_t * @@ -3654,10 +3701,10 @@ rend_consider_services_upload(time_t now) /* Does every introduction points have been established? */ unsigned int intro_points_ready = count_established_intro_points(service) >= service->n_intro_points_wanted; - if (service->next_upload_time < now || + if (intro_points_ready && + (service->next_upload_time < now || (service->desc_is_dirty && - service->desc_is_dirty < now-rendinitialpostdelay && - intro_points_ready)) { + service->desc_is_dirty < now-rendinitialpostdelay))) { /* if it's time, or if the directory servers have a wrong service * descriptor and ours has been stable for rendinitialpostdelay seconds, * upload a new one of each format. */ From 5fa280f7adaaf9796b93f32327f10d18359e1f95 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Mon, 29 Jun 2015 11:12:25 -0400 Subject: [PATCH 7/7] Fix comments in rendservice.c Signed-off-by: David Goulet --- src/or/rendservice.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/or/rendservice.c b/src/or/rendservice.c index 85f0fc7772..f4fb860078 100644 --- a/src/or/rendservice.c +++ b/src/or/rendservice.c @@ -3438,7 +3438,7 @@ remove_invalid_intro_points(rend_service_t *service, /* First, make sure we still have a valid circuit for this intro point. * If we dont, we'll give up on it and make a new one. */ if (intro_circ == NULL) { - log_info(LD_REND, "Giving up on %s as intro point for %s" + log_info(LD_REND, "Attempting to retry on %s as intro point for %s" " (circuit disappeared).", safe_str_client(extend_info_describe(intro->extend_info)), safe_str_client(service->service_id)); @@ -3658,7 +3658,8 @@ rend_services_introduce(void) safe_str_client(extend_info_describe(intro->extend_info)), safe_str_client(service->service_id)); /* This funcion will be called again by the main loop so this intro - * point without a intro circuit will be removed. */ + * point without a intro circuit will be retried on or removed after + * a maximum number of attempts. */ } } } SMARTLIST_FOREACH_END(service);