From 6051375ccac45f68df48e6ce008f6849d160ce63 Mon Sep 17 00:00:00 2001 From: George Kadianakis Date: Tue, 26 Sep 2017 15:07:44 +0300 Subject: [PATCH 1/6] prop224: Update some function docs with the right retval. --- src/or/hs_client.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/or/hs_client.c b/src/or/hs_client.c index 942e332f0c..201e808180 100644 --- a/src/or/hs_client.c +++ b/src/or/hs_client.c @@ -131,9 +131,9 @@ note_connection_attempt_succeeded(const hs_ident_edge_conn_t *hs_conn_ident) } /* Given the pubkey of a hidden service in onion_identity_pk, fetch its - * descriptor by launching a dir connection to hsdir. Return 1 on - * success or -1 on error. */ -static int + * descriptor by launching a dir connection to hsdir. Return a + * hs_client_fetch_status_t status code depending on how it went. */ +static hs_client_fetch_status_t directory_launch_v3_desc_fetch(const ed25519_public_key_t *onion_identity_pk, const routerstatus_t *hsdir) { @@ -224,9 +224,9 @@ pick_hsdir_v3(const ed25519_public_key_t *onion_identity_pk) /** Fetch a v3 descriptor using the given onion_identity_pk. * - * On success, 1 is returned. If no hidden service is left to ask, return 0. - * On error, -1 is returned. */ -static int + * On success, HS_CLIENT_FETCH_LAUNCHED is returned. Otherwise, an error from + * hs_client_fetch_status_t is returned. */ +static hs_client_fetch_status_t fetch_v3_desc(const ed25519_public_key_t *onion_identity_pk) { routerstatus_t *hsdir_rs =NULL; From 178a2bbcff74b7f9df2a192f45f1f91f3a9f935d Mon Sep 17 00:00:00 2001 From: George Kadianakis Date: Tue, 26 Sep 2017 15:23:23 +0300 Subject: [PATCH 2/6] prop224: Fix one last instance of get_time_period_num() with now. When purging last HSDir requests, we used time(NULL) for computing the service blinded key but in all other places in our codebase we actually use the consensus times. That can cause wrong behavior if the consensus is in a different time period than time(NULL). This commit is required for proper purging of HSDir requests. --- src/or/hs_client.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/or/hs_client.c b/src/or/hs_client.c index 201e808180..560ccf8499 100644 --- a/src/or/hs_client.c +++ b/src/or/hs_client.c @@ -100,7 +100,7 @@ purge_hid_serv_request(const ed25519_public_key_t *identity_pk) * from the previous time period. That is fine because they will expire at * some point and we don't care about those anymore. */ hs_build_blinded_pubkey(identity_pk, NULL, 0, - hs_get_time_period_num(approx_time()), &blinded_pk); + hs_get_time_period_num(0), &blinded_pk); if (BUG(ed25519_public_to_base64(base64_blinded_pk, &blinded_pk) < 0)) { return; } From cf75c1af666b983c3a828f13683240153646247b Mon Sep 17 00:00:00 2001 From: David Goulet Date: Tue, 26 Sep 2017 10:54:46 -0400 Subject: [PATCH 3/6] hs-v3: Don't fetch descriptor if we have a pending request If 6 SOCKS requests are opened at once, it would have triggered 6 fetches which ultimately poke all 6 HSDir. We don't want that, if we have multiple SOCKS requests for the same service, do one fetch only. Signed-off-by: David Goulet --- src/or/connection_edge.c | 1 + src/or/hs_client.c | 34 ++++++++++++++++++++++++++++++++++ src/or/hs_client.h | 2 ++ 3 files changed, 37 insertions(+) diff --git a/src/or/connection_edge.c b/src/or/connection_edge.c index 9098cb6908..77dac62b09 100644 --- a/src/or/connection_edge.c +++ b/src/or/connection_edge.c @@ -1576,6 +1576,7 @@ connection_ap_handle_onion(entry_connection_t *conn, * connection. */ goto end; case HS_CLIENT_FETCH_LAUNCHED: + case HS_CLIENT_FETCH_PENDING: case HS_CLIENT_FETCH_HAVE_DESC: return 0; case HS_CLIENT_FETCH_ERROR: diff --git a/src/or/hs_client.c b/src/or/hs_client.c index 560ccf8499..8ed19529b5 100644 --- a/src/or/hs_client.c +++ b/src/or/hs_client.c @@ -108,6 +108,34 @@ purge_hid_serv_request(const ed25519_public_key_t *identity_pk) hs_purge_hid_serv_from_last_hid_serv_requests(base64_blinded_pk); } +/* Return true iff there is at least one pending directory descriptor request + * for the service identity_pk. */ +static int +directory_request_is_pending(const ed25519_public_key_t *identity_pk) +{ + int ret = 0; + smartlist_t *conns = + connection_list_by_type_purpose(CONN_TYPE_DIR, DIR_PURPOSE_FETCH_HSDESC); + + SMARTLIST_FOREACH_BEGIN(conns, connection_t *, conn) { + const hs_ident_dir_conn_t *ident = TO_DIR_CONN(conn)->hs_ident; + if (BUG(ident == NULL)) { + /* A directory connection fetching a service descriptor can't have an + * empty hidden service identifier. */ + continue; + } + if (!ed25519_pubkey_eq(identity_pk, &ident->identity_pk)) { + continue; + } + ret = 1; + break; + } SMARTLIST_FOREACH_END(conn); + + /* No ownership of the objects in this list. */ + smartlist_free(conns); + return ret; +} + /* A v3 HS circuit successfully connected to the hidden service. Update the * stream state at hs_conn_ident appropriately. */ static void @@ -1046,6 +1074,12 @@ hs_client_refetch_hsdesc(const ed25519_public_key_t *identity_pk) } } + /* 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; + } + return fetch_v3_desc(identity_pk); } diff --git a/src/or/hs_client.h b/src/or/hs_client.h index 08ab7736b6..1d0201903b 100644 --- a/src/or/hs_client.h +++ b/src/or/hs_client.h @@ -27,6 +27,8 @@ typedef enum { HS_CLIENT_FETCH_NOT_ALLOWED = 3, /* We are missing information to be able to launch a request. */ HS_CLIENT_FETCH_MISSING_INFO = 4, + /* There is a pending fetch for the requested service. */ + HS_CLIENT_FETCH_PENDING = 5, } hs_client_fetch_status_t; void hs_client_note_connection_attempt_succeeded( From c232b6b59cc136f7053922b3efc14afd5c34b6ec Mon Sep 17 00:00:00 2001 From: David Goulet Date: Tue, 26 Sep 2017 10:45:24 -0400 Subject: [PATCH 4/6] hs-v3: Close pending SOCKS connection on descriptor fetch error Signed-off-by: David Goulet --- src/or/hs_client.c | 111 ++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 110 insertions(+), 1 deletion(-) diff --git a/src/or/hs_client.c b/src/or/hs_client.c index 8ed19529b5..6f81ea09ed 100644 --- a/src/or/hs_client.c +++ b/src/or/hs_client.c @@ -32,6 +32,58 @@ #include "hs_ntor.h" #include "circuitbuild.h" #include "networkstatus.h" +#include "reasons.h" + +/* Return a human-readable string for the client fetch status code. */ +static const char * +fetch_status_to_string(hs_client_fetch_status_t status) +{ + switch (status) { + case HS_CLIENT_FETCH_ERROR: + return "Internal error"; + case HS_CLIENT_FETCH_LAUNCHED: + return "Descriptor fetch launched"; + case HS_CLIENT_FETCH_HAVE_DESC: + return "Already have descriptor"; + case HS_CLIENT_FETCH_NO_HSDIRS: + return "No more HSDir available to query"; + case HS_CLIENT_FETCH_NOT_ALLOWED: + return "Fetching descriptors is not allowed"; + case HS_CLIENT_FETCH_MISSING_INFO: + return "Missing directory information"; + case HS_CLIENT_FETCH_PENDING: + return "Pending descriptor fetch"; + default: + return "(Unknown client fetch status code)"; + } +} + +/* Return true iff tor should close the SOCKS request(s) for the descriptor + * fetch that ended up with this given status code. */ +static int +fetch_status_should_close_socks(hs_client_fetch_status_t status) +{ + switch (status) { + case HS_CLIENT_FETCH_NO_HSDIRS: + /* No more HSDir to query, we can't complete the SOCKS request(s). */ + case HS_CLIENT_FETCH_ERROR: + /* The fetch triggered an internal error. */ + case HS_CLIENT_FETCH_NOT_ALLOWED: + /* Client is not allowed to fetch (FetchHidServDescriptors 0). */ + goto close; + case HS_CLIENT_FETCH_MISSING_INFO: + case HS_CLIENT_FETCH_HAVE_DESC: + case HS_CLIENT_FETCH_PENDING: + case HS_CLIENT_FETCH_LAUNCHED: + /* The rest doesn't require tor to close the SOCKS request(s). */ + goto no_close; + } + + no_close: + return 0; + close: + return 1; +} /* Cancel all descriptor fetches currently in progress. */ static void @@ -136,6 +188,51 @@ directory_request_is_pending(const ed25519_public_key_t *identity_pk) return ret; } +/* We failed to fetch a descriptor for the service with identity_pk + * because of status. Find all pending SOCKS connections for this + * service that are waiting on the descriptor and close them with + * reason. */ +static void +close_all_socks_conns_waiting_for_desc(const ed25519_public_key_t *identity_pk, + hs_client_fetch_status_t status, + int reason) +{ + unsigned int count = 0; + time_t now = approx_time(); + smartlist_t *conns = + connection_list_by_type_state(CONN_TYPE_AP, AP_CONN_STATE_RENDDESC_WAIT); + + SMARTLIST_FOREACH_BEGIN(conns, connection_t *, base_conn) { + entry_connection_t *entry_conn = TO_ENTRY_CONN(base_conn); + const edge_connection_t *edge_conn = ENTRY_TO_EDGE_CONN(entry_conn); + + /* Only consider the entry connections that matches the service for which + * we tried to get the descriptor */ + if (!edge_conn->hs_ident || + !ed25519_pubkey_eq(identity_pk, + &edge_conn->hs_ident->identity_pk)) { + continue; + } + assert_connection_ok(base_conn, now); + /* Unattach the entry connection which will close for the reason. */ + connection_mark_unattached_ap(entry_conn, reason); + count++; + } SMARTLIST_FOREACH_END(base_conn); + + if (count > 0) { + char onion_address[HS_SERVICE_ADDR_LEN_BASE32 + 1]; + hs_build_address(identity_pk, HS_VERSION_THREE, onion_address); + log_notice(LD_REND, "Closed %u streams for service %s.onion " + "for reason %s. Fetch status: %s.", + count, safe_str_client(onion_address), + stream_end_reason_to_string(reason), + fetch_status_to_string(status)); + } + + /* No ownership of the object(s) in this list. */ + smartlist_free(conns); +} + /* A v3 HS circuit successfully connected to the hidden service. Update the * stream state at hs_conn_ident appropriately. */ static void @@ -1037,6 +1134,8 @@ 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; + tor_assert(identity_pk); /* Are we configured to fetch descriptors? */ @@ -1080,7 +1179,17 @@ hs_client_refetch_hsdesc(const ed25519_public_key_t *identity_pk) return HS_CLIENT_FETCH_PENDING; } - return fetch_v3_desc(identity_pk); + /* 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, + 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; } /* This is called when we are trying to attach an AP connection to these From 30d2e0b71e9c42ff93af5239f54268bb24ce1744 Mon Sep 17 00:00:00 2001 From: David Goulet Date: Tue, 26 Sep 2017 12:03:16 -0400 Subject: [PATCH 5/6] test: Add an HS v3 descriptor fetch unit test Signed-off-by: David Goulet --- src/or/hs_client.c | 4 +- src/or/hs_client.h | 3 + src/test/test_hs_client.c | 131 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 136 insertions(+), 2 deletions(-) diff --git a/src/or/hs_client.c b/src/or/hs_client.c index 6f81ea09ed..93a913b34c 100644 --- a/src/or/hs_client.c +++ b/src/or/hs_client.c @@ -351,8 +351,8 @@ pick_hsdir_v3(const ed25519_public_key_t *onion_identity_pk) * * On success, HS_CLIENT_FETCH_LAUNCHED is returned. Otherwise, an error from * hs_client_fetch_status_t is returned. */ -static hs_client_fetch_status_t -fetch_v3_desc(const ed25519_public_key_t *onion_identity_pk) +MOCK_IMPL(STATIC hs_client_fetch_status_t, +fetch_v3_desc, (const ed25519_public_key_t *onion_identity_pk)) { routerstatus_t *hsdir_rs =NULL; diff --git a/src/or/hs_client.h b/src/or/hs_client.h index 1d0201903b..164accc0e6 100644 --- a/src/or/hs_client.h +++ b/src/or/hs_client.h @@ -82,6 +82,9 @@ desc_intro_point_to_extend_info(const hs_desc_intro_point_t *ip); STATIC int handle_rendezvous2(origin_circuit_t *circ, const uint8_t *payload, size_t payload_len); +MOCK_DECL(STATIC hs_client_fetch_status_t, + fetch_v3_desc, (const ed25519_public_key_t *onion_identity_pk)); + #endif /* defined(HS_CLIENT_PRIVATE) */ #endif /* !defined(TOR_HS_CLIENT_H) */ diff --git a/src/test/test_hs_client.c b/src/test/test_hs_client.c index 38878d6ed5..750920fac0 100644 --- a/src/test/test_hs_client.c +++ b/src/test/test_hs_client.c @@ -23,6 +23,8 @@ #include "config.h" #include "crypto.h" #include "channeltls.h" +#include "main.h" +#include "nodelist.h" #include "routerset.h" #include "hs_circuit.h" @@ -44,6 +46,14 @@ mock_connection_ap_handshake_send_begin(entry_connection_t *ap_conn) static networkstatus_t mock_ns; +/* Always return NULL. */ +static networkstatus_t * +mock_networkstatus_get_live_consensus_false(time_t now) +{ + (void) now; + return NULL; +} + static networkstatus_t * mock_networkstatus_get_live_consensus(time_t now) { @@ -456,6 +466,125 @@ test_client_pick_intro(void *arg) hs_descriptor_free(desc); } +static int +mock_router_have_minimum_dir_info_false(void) +{ + return 0; +} +static int +mock_router_have_minimum_dir_info_true(void) +{ + return 1; +} + +static hs_client_fetch_status_t +mock_fetch_v3_desc_error(const ed25519_public_key_t *key) +{ + (void) key; + return HS_CLIENT_FETCH_ERROR; +} + +static void +mock_connection_mark_unattached_ap_(entry_connection_t *conn, int endreason, + int line, const char *file) +{ + (void) line; + (void) file; + conn->edge_.end_reason = endreason; +} + +static void +test_descriptor_fetch(void *arg) +{ + int ret; + entry_connection_t *ec = NULL; + ed25519_public_key_t service_pk; + ed25519_secret_key_t service_sk; + + (void) arg; + + hs_init(); + memset(&service_sk, 'A', sizeof(service_sk)); + ret = ed25519_public_key_generate(&service_pk, &service_sk); + tt_int_op(ret, OP_EQ, 0); + + /* Initialize this so get_voting_interval() doesn't freak out. */ + ret = parse_rfc1123_time("Sat, 26 Oct 1985 13:00:00 UTC", + &mock_ns.valid_after); + tt_int_op(ret, OP_EQ, 0); + ret = parse_rfc1123_time("Sat, 26 Oct 1985 14:00:00 UTC", + &mock_ns.fresh_until); + tt_int_op(ret, OP_EQ, 0); + + ec = entry_connection_new(CONN_TYPE_AP, AF_INET); + tt_assert(ec); + ENTRY_TO_EDGE_CONN(ec)->hs_ident = hs_ident_edge_conn_new(&service_pk); + tt_assert(ENTRY_TO_EDGE_CONN(ec)->hs_ident); + TO_CONN(ENTRY_TO_EDGE_CONN(ec))->state = AP_CONN_STATE_RENDDESC_WAIT; + smartlist_add(get_connection_array(), &ec->edge_.base_); + + /* 1. FetchHidServDescriptors is false so we shouldn't be able to fetch. */ + get_options_mutable()->FetchHidServDescriptors = 0; + ret = hs_client_refetch_hsdesc(&service_pk); + tt_int_op(ret, OP_EQ, HS_CLIENT_FETCH_NOT_ALLOWED); + get_options_mutable()->FetchHidServDescriptors = 1; + + /* 2. We don't have a live consensus. */ + MOCK(networkstatus_get_live_consensus, + mock_networkstatus_get_live_consensus_false); + ret = hs_client_refetch_hsdesc(&service_pk); + UNMOCK(networkstatus_get_live_consensus); + tt_int_op(ret, OP_EQ, HS_CLIENT_FETCH_MISSING_INFO); + + /* From now on, return a live consensus. */ + MOCK(networkstatus_get_live_consensus, + mock_networkstatus_get_live_consensus); + + /* 3. Not enough dir information. */ + MOCK(router_have_minimum_dir_info, + mock_router_have_minimum_dir_info_false); + ret = hs_client_refetch_hsdesc(&service_pk); + UNMOCK(router_have_minimum_dir_info); + tt_int_op(ret, OP_EQ, HS_CLIENT_FETCH_MISSING_INFO); + + /* From now on, we do have enough directory information. */ + MOCK(router_have_minimum_dir_info, + mock_router_have_minimum_dir_info_true); + + /* 4. We do have a pending directory request. */ + { + dir_connection_t *dir_conn = dir_connection_new(AF_INET); + dir_conn->hs_ident = tor_malloc_zero(sizeof(hs_ident_dir_conn_t)); + TO_CONN(dir_conn)->purpose = DIR_PURPOSE_FETCH_HSDESC; + ed25519_pubkey_copy(&dir_conn->hs_ident->identity_pk, &service_pk); + smartlist_add(get_connection_array(), TO_CONN(dir_conn)); + ret = hs_client_refetch_hsdesc(&service_pk); + smartlist_remove(get_connection_array(), TO_CONN(dir_conn)); + connection_free_(TO_CONN(dir_conn)); + tt_int_op(ret, OP_EQ, HS_CLIENT_FETCH_PENDING); + } + + /* 5. We'll trigger an error on the fetch_desc_v3 and force to close all + * pending SOCKS request. */ + MOCK(router_have_minimum_dir_info, + mock_router_have_minimum_dir_info_true); + MOCK(fetch_v3_desc, mock_fetch_v3_desc_error); + MOCK(connection_mark_unattached_ap_, + mock_connection_mark_unattached_ap_); + ret = hs_client_refetch_hsdesc(&service_pk); + UNMOCK(fetch_v3_desc); + UNMOCK(connection_mark_unattached_ap_); + tt_int_op(ret, OP_EQ, HS_CLIENT_FETCH_ERROR); + /* The close waiting for descriptor function has been called. */ + tt_int_op(ec->edge_.end_reason, OP_EQ, END_STREAM_REASON_RESOLVEFAILED); + + done: + connection_free_(ENTRY_TO_CONN(ec)); + UNMOCK(networkstatus_get_live_consensus); + UNMOCK(router_have_minimum_dir_info); + hs_free_all(); +} + struct testcase_t hs_client_tests[] = { { "e2e_rend_circuit_setup_legacy", test_e2e_rend_circuit_setup_legacy, TT_FORK, NULL, NULL }, @@ -463,6 +592,8 @@ struct testcase_t hs_client_tests[] = { TT_FORK, NULL, NULL }, { "client_pick_intro", test_client_pick_intro, TT_FORK, NULL, NULL }, + { "descriptor_fetch", test_descriptor_fetch, + TT_FORK, NULL, NULL }, END_OF_TESTCASES }; From 6691af1855da7fc35afb4e51251a6a87dc77d4aa Mon Sep 17 00:00:00 2001 From: David Goulet Date: Wed, 27 Sep 2017 08:38:52 -0400 Subject: [PATCH 6/6] hs: Add changes file for 23653 Signed-off-by: David Goulet --- changes/bug23653 | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 changes/bug23653 diff --git a/changes/bug23653 b/changes/bug23653 new file mode 100644 index 0000000000..81760cbb82 --- /dev/null +++ b/changes/bug23653 @@ -0,0 +1,7 @@ + o Minor bugfixes (hidden service client): + - When getting multiple SOCKS request for the same .onion address, don't + trigger multiple descriptor fetches. + - When the descriptor fetch fails with an internal error, no more HSDir to + query or we aren't allowed to fetch (FetchHidServDescriptors 0), close + all pending SOCKS request for that .onion. Fixes bug 23653; bugfix on + 0.3.2.1-alpha.