commit 5dbcd48f0ee2f57557f6bcce6ee3ec11a76727e4 Author: David Goulet <dgoulet@torproject.org> Date: Wed Oct 4 16:22:49 2017 -0400 hs-v3: Attempt descriptor refetch when dirinfo changes When the directory information changes, callback to the HS client subsystem so it can check if any pending SOCKS connections are waiting for a descriptor. If yes, attempt a refetch for those. Fixes #23762 Signed-off-by: David Goulet <dgoulet@torproject.org> --- changes/bug23762 | 4 +++ src/or/connection_edge.c | 9 +++---- src/or/hs_client.c | 59 ++++++++++++++++++++++++++++++++++++++++++ src/or/hs_client.h | 1 + src/or/nodelist.c | 2 ++ src/test/test_channelpadding.c | 12 ++++++--- src/test/test_entryconn.c | 5 ++-- 7 files changed, 81 insertions(+), 11 deletions(-) diff --git a/changes/bug23762 b/changes/bug23762 new file mode 100644 index 000000000..bf35e5ec1 --- /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. Closes ticket #23762; bugfix on + 0.3.2.1-alpha. diff --git a/src/or/connection_edge.c b/src/or/connection_edge.c index 77dac62b0..b9d8eeaff 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 d66e56352..18b76e8b3 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 @@ -1529,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 164accc0e..2523568ad 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 eae74e18b..fd5a8f0c0 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" @@ -1973,6 +1974,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 d54c9cc52..d5713688a 100644 --- a/src/test/test_channelpadding.c +++ b/src/test/test_channelpadding.c @@ -193,7 +193,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; @@ -280,7 +281,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(); @@ -570,7 +572,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); @@ -928,7 +931,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 86c0c3dca..c29b1a712 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");