diff options
| -rw-r--r-- | changes/bug23762 | 4 | ||||
| -rw-r--r-- | src/or/connection_edge.c | 9 | ||||
| -rw-r--r-- | src/or/hs_client.c | 181 | ||||
| -rw-r--r-- | src/or/hs_client.h | 1 | ||||
| -rw-r--r-- | src/or/nodelist.c | 2 | ||||
| -rw-r--r-- | src/test/test_channelpadding.c | 12 | ||||
| -rw-r--r-- | src/test/test_entryconn.c | 5 |
7 files changed, 157 insertions, 57 deletions
diff --git a/changes/bug23762 b/changes/bug23762 new file mode 100644 index 0000000000..741a88e21f --- /dev/null +++ b/changes/bug23762 @@ -0,0 +1,4 @@ + o Minor bugfixes (hidden service v3): + - Properly retry HSv3 descriptor fetches in the case where we were initially + missing required directory information. Fixes bug 23762; bugfix on + 0.3.2.1-alpha. diff --git a/src/or/connection_edge.c b/src/or/connection_edge.c index 7d6667dbc9..c63b25165f 100644 --- a/src/or/connection_edge.c +++ b/src/or/connection_edge.c @@ -1571,10 +1571,10 @@ connection_ap_handle_onion(entry_connection_t *conn, int ret = hs_client_refetch_hsdesc(&edge_conn->hs_ident->identity_pk); switch (ret) { case HS_CLIENT_FETCH_MISSING_INFO: - /* By going to the end, the connection is put in waiting for a circuit - * state which means that it will be retried and consider as a pending - * connection. */ - goto end; + /* Keeping the connection in descriptor wait state is fine because + * once we get enough dirinfo or a new live consensus, the HS client + * subsystem is notified and every connection in that state will + * trigger a fetch for the service key. */ case HS_CLIENT_FETCH_LAUNCHED: case HS_CLIENT_FETCH_PENDING: case HS_CLIENT_FETCH_HAVE_DESC: @@ -1591,7 +1591,6 @@ connection_ap_handle_onion(entry_connection_t *conn, /* We have the descriptor! So launch a connection to the HS. */ log_info(LD_REND, "Descriptor is here. Great."); - end: base_conn->state = AP_CONN_STATE_CIRCUIT_WAIT; /* We'll try to attach it at the next event loop, or whenever * we call connection_ap_attach_pending() */ diff --git a/src/or/hs_client.c b/src/or/hs_client.c index 93a913b34c..18b76e8b38 100644 --- a/src/or/hs_client.c +++ b/src/or/hs_client.c @@ -233,6 +233,55 @@ close_all_socks_conns_waiting_for_desc(const ed25519_public_key_t *identity_pk, smartlist_free(conns); } +/* Find all pending SOCKS connection waiting for a descriptor and retry them + * all. This is called when the directory information changed. */ +static void +retry_all_socks_conn_waiting_for_desc(void) +{ + smartlist_t *conns = + connection_list_by_type_state(CONN_TYPE_AP, AP_CONN_STATE_RENDDESC_WAIT); + + SMARTLIST_FOREACH_BEGIN(conns, connection_t *, base_conn) { + hs_client_fetch_status_t status; + const edge_connection_t *edge_conn = + ENTRY_TO_EDGE_CONN(TO_ENTRY_CONN(base_conn)); + + /* Ignore non HS or non v3 connection. */ + if (edge_conn->hs_ident == NULL) { + continue; + } + /* In this loop, we will possibly try to fetch a descriptor for the + * pending connections because we just got more directory information. + * However, the refetch process can cleanup all SOCKS request so the same + * service if an internal error happens. Thus, we can end up with closed + * connections in our list. */ + if (base_conn->marked_for_close) { + continue; + } + + /* XXX: There is an optimization we could do which is that for a service + * key, we could check if we can fetch and remember that decision. */ + + /* Order a refetch in case it works this time. */ + status = hs_client_refetch_hsdesc(&edge_conn->hs_ident->identity_pk); + if (BUG(status == HS_CLIENT_FETCH_HAVE_DESC)) { + /* This case is unique because it can NOT happen in theory. Once we get + * a new descriptor, the HS client subsystem is notified immediately and + * the connections waiting for it are handled which means the state will + * change from renddesc wait state. Log this and continue to next + * connection. */ + continue; + } + /* In the case of an error, either all SOCKS connections have been + * closed or we are still missing directory information. Leave the + * connection in renddesc wait state so when we get more info, we'll be + * able to try it again. */ + } SMARTLIST_FOREACH_END(base_conn); + + /* We don't have ownership of those objects. */ + smartlist_free(conns); +} + /* A v3 HS circuit successfully connected to the hidden service. Update the * stream state at <b>hs_conn_ident</b> appropriately. */ static void @@ -1029,6 +1078,73 @@ handle_rendezvous2(origin_circuit_t *circ, const uint8_t *payload, return ret; } +/* Return true iff the client can fetch a descriptor for this service public + * identity key and status_out if not NULL is untouched. If the client can + * _not_ fetch the descriptor and if status_out is not NULL, it is set with + * the fetch status code. */ +static unsigned int +can_client_refetch_desc(const ed25519_public_key_t *identity_pk, + hs_client_fetch_status_t *status_out) +{ + hs_client_fetch_status_t status; + + tor_assert(identity_pk); + + /* Are we configured to fetch descriptors? */ + if (!get_options()->FetchHidServDescriptors) { + log_warn(LD_REND, "We received an onion address for a hidden service " + "descriptor but we are configured to not fetch."); + status = HS_CLIENT_FETCH_NOT_ALLOWED; + goto cannot; + } + + /* Without a live consensus we can't do any client actions. It is needed to + * compute the hashring for a service. */ + if (!networkstatus_get_live_consensus(approx_time())) { + log_info(LD_REND, "Can't fetch descriptor for service %s because we " + "are missing a live consensus. Stalling connection.", + safe_str_client(ed25519_fmt(identity_pk))); + status = HS_CLIENT_FETCH_MISSING_INFO; + goto cannot; + } + + if (!router_have_minimum_dir_info()) { + log_info(LD_REND, "Can't fetch descriptor for service %s because we " + "dont have enough descriptors. Stalling connection.", + safe_str_client(ed25519_fmt(identity_pk))); + status = HS_CLIENT_FETCH_MISSING_INFO; + goto cannot; + } + + /* Check if fetching a desc for this HS is useful to us right now */ + { + const hs_descriptor_t *cached_desc = NULL; + cached_desc = hs_cache_lookup_as_client(identity_pk); + if (cached_desc && hs_client_any_intro_points_usable(identity_pk, + cached_desc)) { + log_info(LD_GENERAL, "We would fetch a v3 hidden service descriptor " + "but we already have a usable descriptor."); + status = HS_CLIENT_FETCH_HAVE_DESC; + goto cannot; + } + } + + /* Don't try to refetch while we have a pending request for it. */ + if (directory_request_is_pending(identity_pk)) { + log_info(LD_REND, "Already a pending directory request. Waiting on it."); + status = HS_CLIENT_FETCH_PENDING; + goto cannot; + } + + /* Yes, client can fetch! */ + return 1; + cannot: + if (status_out) { + *status_out = status; + } + return 0; +} + /* ========== */ /* Public API */ /* ========== */ @@ -1134,62 +1250,25 @@ hs_client_any_intro_points_usable(const ed25519_public_key_t *service_pk, int hs_client_refetch_hsdesc(const ed25519_public_key_t *identity_pk) { - int ret; + hs_client_fetch_status_t status; tor_assert(identity_pk); - /* Are we configured to fetch descriptors? */ - if (!get_options()->FetchHidServDescriptors) { - log_warn(LD_REND, "We received an onion address for a hidden service " - "descriptor but we are configured to not fetch."); - return HS_CLIENT_FETCH_NOT_ALLOWED; - } - - /* Without a live consensus we can't do any client actions. It is needed to - * compute the hashring for a service. */ - if (!networkstatus_get_live_consensus(approx_time())) { - log_info(LD_REND, "Can't fetch descriptor for service %s because we " - "are missing a live consensus. Stalling connection.", - safe_str_client(ed25519_fmt(identity_pk))); - return HS_CLIENT_FETCH_MISSING_INFO; - } - - if (!router_have_minimum_dir_info()) { - log_info(LD_REND, "Can't fetch descriptor for service %s because we " - "dont have enough descriptors. Stalling connection.", - safe_str_client(ed25519_fmt(identity_pk))); - return HS_CLIENT_FETCH_MISSING_INFO; + if (!can_client_refetch_desc(identity_pk, &status)) { + return status; } - /* Check if fetching a desc for this HS is useful to us right now */ - { - const hs_descriptor_t *cached_desc = NULL; - cached_desc = hs_cache_lookup_as_client(identity_pk); - if (cached_desc && hs_client_any_intro_points_usable(identity_pk, - cached_desc)) { - log_info(LD_GENERAL, "We would fetch a v3 hidden service descriptor " - "but we already have a usable descriptor."); - return HS_CLIENT_FETCH_HAVE_DESC; - } - } - - /* Don't try to refetch while we have a pending request for it. */ - if (directory_request_is_pending(identity_pk)) { - log_info(LD_REND, "Already a pending directory request. Waiting on it."); - return HS_CLIENT_FETCH_PENDING; - } - - /* Try to fetch the desc and if we encounter an unrecoverable error, mark the - * desc as unavailable for now. */ - ret = fetch_v3_desc(identity_pk); - if (fetch_status_should_close_socks(ret)) { - close_all_socks_conns_waiting_for_desc(identity_pk, ret, + /* Try to fetch the desc and if we encounter an unrecoverable error, mark + * the desc as unavailable for now. */ + status = fetch_v3_desc(identity_pk); + if (fetch_status_should_close_socks(status)) { + close_all_socks_conns_waiting_for_desc(identity_pk, status, END_STREAM_REASON_RESOLVEFAILED); /* Remove HSDir fetch attempts so that we can retry later if the user * wants us to regardless of if we closed any connections. */ purge_hid_serv_request(identity_pk); } - return ret; + return status; } /* This is called when we are trying to attach an AP connection to these @@ -1499,3 +1578,13 @@ hs_client_purge_state(void) log_info(LD_REND, "Hidden service client state has been purged."); } +/* Called when our directory information has changed. */ +void +hs_client_dir_info_changed(void) +{ + /* We have possibly reached the minimum directory information or new + * consensus so retry all pending SOCKS connection in + * AP_CONN_STATE_RENDDESC_WAIT state in order to fetch the descriptor. */ + retry_all_socks_conn_waiting_for_desc(); +} + diff --git a/src/or/hs_client.h b/src/or/hs_client.h index 164accc0e6..2523568ad1 100644 --- a/src/or/hs_client.h +++ b/src/or/hs_client.h @@ -41,6 +41,7 @@ int hs_client_decode_descriptor( int hs_client_any_intro_points_usable(const ed25519_public_key_t *service_pk, const hs_descriptor_t *desc); int hs_client_refetch_hsdesc(const ed25519_public_key_t *identity_pk); +void hs_client_dir_info_changed(void); int hs_client_send_introduce1(origin_circuit_t *intro_circ, origin_circuit_t *rend_circ); diff --git a/src/or/nodelist.c b/src/or/nodelist.c index 0743be1802..f5a2cddf48 100644 --- a/src/or/nodelist.c +++ b/src/or/nodelist.c @@ -48,6 +48,7 @@ #include "entrynodes.h" #include "geoip.h" #include "hs_common.h" +#include "hs_client.h" #include "main.h" #include "microdesc.h" #include "networkstatus.h" @@ -1978,6 +1979,7 @@ router_dir_info_changed(void) need_to_update_have_min_dir_info = 1; rend_hsdir_routers_changed(); hs_service_dir_info_changed(); + hs_client_dir_info_changed(); } /** Return a string describing what we're missing before we have enough diff --git a/src/test/test_channelpadding.c b/src/test/test_channelpadding.c index fe21057c8e..4346ee343f 100644 --- a/src/test/test_channelpadding.c +++ b/src/test/test_channelpadding.c @@ -196,7 +196,8 @@ static void setup_mock_network(void) { routerstatus_t *relay; - connection_array = smartlist_new(); + if (!connection_array) + connection_array = smartlist_new(); relay1_relay2 = (channel_t*)new_fake_channeltls(2); relay1_relay2->write_cell = mock_channel_write_cell_relay1; @@ -283,7 +284,8 @@ test_channelpadding_timers(void *arg) tor_libevent_postfork(); - connection_array = smartlist_new(); + if (!connection_array) + connection_array = smartlist_new(); monotime_init(); monotime_enable_test_mocking(); @@ -573,7 +575,8 @@ test_channelpadding_consensus(void *arg) monotime_coarse_set_mock_time_nsec(1); timers_initialize(); - connection_array = smartlist_new(); + if (!connection_array) + connection_array = smartlist_new(); chan = (channel_t*)new_fake_channeltls(0); channel_timestamp_active(chan); @@ -931,7 +934,8 @@ test_channelpadding_decide_to_pad_channel(void *arg) */ channel_t *chan; int64_t new_time; - connection_array = smartlist_new(); + if (!connection_array) + connection_array = smartlist_new(); (void)arg; tor_libevent_postfork(); diff --git a/src/test/test_entryconn.c b/src/test/test_entryconn.c index 86c0c3dca4..c29b1a7126 100644 --- a/src/test/test_entryconn.c +++ b/src/test/test_entryconn.c @@ -790,8 +790,9 @@ test_entryconn_rewrite_onion_v3(void *arg) retval = connection_ap_handshake_rewrite_and_attach(conn, NULL, NULL); tt_int_op(retval, OP_EQ, 0); - /* Check connection state after rewrite */ - tt_int_op(ENTRY_TO_CONN(conn)->state, OP_EQ, AP_CONN_STATE_CIRCUIT_WAIT); + /* Check connection state after rewrite. It should be in waiting for + * descriptor state. */ + tt_int_op(ENTRY_TO_CONN(conn)->state, OP_EQ, AP_CONN_STATE_RENDDESC_WAIT); /* check that the address got rewritten */ tt_str_op(conn->socks_request->address, OP_EQ, "25njqamcweflpvkl73j4szahhihoc4xt3ktcgjnpaingr5yhkenl5sid"); |