From d1ac5613fc832f4b298fbc243a35660dbd0976e3 Mon Sep 17 00:00:00 2001 From: teor Date: Mon, 5 Nov 2018 15:27:38 +1000 Subject: [PATCH 1/5] Entry Nodes: Use a reasonably live consensus to select guards Fixes bug 24661; bugfix on 0.3.0.1-alpha. --- changes/bug24661 | 3 +++ src/feature/client/entrynodes.c | 27 +++++++++++++++++---------- 2 files changed, 20 insertions(+), 10 deletions(-) create mode 100644 changes/bug24661 diff --git a/changes/bug24661 b/changes/bug24661 new file mode 100644 index 0000000000..a915a93e0e --- /dev/null +++ b/changes/bug24661 @@ -0,0 +1,3 @@ + o Minor bugfixes (client, guard selection): + - When Tor's consensus has expired, but is still reasonably live, use it + to select guards. Fixes bug 24661; bugfix on 0.3.0.1-alpha. diff --git a/src/feature/client/entrynodes.c b/src/feature/client/entrynodes.c index 20208ef613..e0fe1b9a58 100644 --- a/src/feature/client/entrynodes.c +++ b/src/feature/client/entrynodes.c @@ -287,7 +287,9 @@ create_initial_guard_context(void) guard_selection_type_t type = GS_TYPE_INFER; const char *name = choose_guard_selection( get_options(), - networkstatus_get_live_consensus(approx_time()), + networkstatus_get_reasonably_live_consensus( + approx_time(), + usable_consensus_flavor()), NULL, &type); tor_assert(name); // "name" can only be NULL if we had an old name. @@ -726,7 +728,9 @@ update_guard_selection_choice(const or_options_t *options) guard_selection_type_t type = GS_TYPE_INFER; const char *new_name = choose_guard_selection( options, - networkstatus_get_live_consensus(approx_time()), + networkstatus_get_reasonably_live_consensus( + approx_time(), + usable_consensus_flavor()), curr_guard_context, &type); tor_assert(new_name); @@ -1125,14 +1129,16 @@ select_and_add_guard_item_for_sample(guard_selection_t *gs, * or if we don't need a consensus because we're using bridges.) */ static int -live_consensus_is_missing(const guard_selection_t *gs) +reasonably_live_consensus_is_missing(const guard_selection_t *gs) { tor_assert(gs); if (gs->type == GS_TYPE_BRIDGE) { /* We don't update bridges from the consensus; they aren't there. */ return 0; } - return networkstatus_get_live_consensus(approx_time()) == NULL; + return networkstatus_get_reasonably_live_consensus( + approx_time(), + usable_consensus_flavor()) == NULL; } /** @@ -1147,9 +1153,9 @@ entry_guards_expand_sample(guard_selection_t *gs) tor_assert(gs); const or_options_t *options = get_options(); - if (live_consensus_is_missing(gs)) { + if (reasonably_live_consensus_is_missing(gs)) { log_info(LD_GUARD, "Not expanding the sample guard set; we have " - "no live consensus."); + "no reasonably live consensus."); return NULL; } @@ -1395,11 +1401,12 @@ sampled_guards_update_from_consensus(guard_selection_t *gs) { tor_assert(gs); - // It's important to use only a live consensus here; we don't want to - // make changes based on anything expired or old. - if (live_consensus_is_missing(gs)) { + // It's important to use a reasonably live consensus here; we want clients + // to bootstrap even if their clock is skewed by more than 2-3 hours. + // But we don't want to make changes based on anything that's really old. + if (reasonably_live_consensus_is_missing(gs)) { log_info(LD_GUARD, "Not updating the sample guard set; we have " - "no live consensus."); + "no reasonably live consensus."); return; } log_info(LD_GUARD, "Updating sampled guard status based on received " From 657618ba9bd96830b0a283c28db203bf9fffb15d Mon Sep 17 00:00:00 2001 From: teor Date: Wed, 21 Nov 2018 19:51:34 +1000 Subject: [PATCH 2/5] Entry Nodes: Mark outdated dirservers in reasonably live consensuses Fixes bug 28569; bugfix on Tor 0.3.2.5-alpha. --- changes/bug28569 | 3 +++ src/feature/nodelist/microdesc.c | 10 ++++++---- 2 files changed, 9 insertions(+), 4 deletions(-) create mode 100644 changes/bug28569 diff --git a/changes/bug28569 b/changes/bug28569 new file mode 100644 index 0000000000..45a57a80ae --- /dev/null +++ b/changes/bug28569 @@ -0,0 +1,3 @@ + o Minor bugfixes (unit tests, directory clients): + - Mark outdated dirservers when Tor only has a reasonably live consensus. + Fixes bug 28569; bugfix on 0.3.2.5-alpha. diff --git a/src/feature/nodelist/microdesc.c b/src/feature/nodelist/microdesc.c index 146c772daf..3f5085412f 100644 --- a/src/feature/nodelist/microdesc.c +++ b/src/feature/nodelist/microdesc.c @@ -108,10 +108,12 @@ microdesc_note_outdated_dirserver(const char *relay_digest) { char relay_hexdigest[HEX_DIGEST_LEN+1]; - /* Don't register outdated dirservers if we don't have a live consensus, - * since we might be trying to fetch microdescriptors that are not even - * currently active. */ - if (!networkstatus_get_live_consensus(approx_time())) { + /* If we have a reasonably live consensus, then most of our dirservers should + * still be caching all the microdescriptors in it. Reasonably live + * consensuses are up to a day old. But microdescriptors expire 7 days after + * the last consensus that referenced them. */ + if (!networkstatus_get_reasonably_live_consensus(approx_time(), + FLAV_MICRODESC)) { return; } From cebc39bcd56dde8f9824055fcd54707de56ab9b1 Mon Sep 17 00:00:00 2001 From: teor Date: Wed, 21 Nov 2018 15:55:43 +1000 Subject: [PATCH 3/5] Test: make unit tests use a reasonably live consensus Cleanup after 24661. --- src/feature/nodelist/networkstatus.c | 7 ++----- src/feature/nodelist/networkstatus.h | 5 +++-- src/test/test_entrynodes.c | 9 +++++---- 3 files changed, 10 insertions(+), 11 deletions(-) diff --git a/src/feature/nodelist/networkstatus.c b/src/feature/nodelist/networkstatus.c index de2451b79c..51e720a984 100644 --- a/src/feature/nodelist/networkstatus.c +++ b/src/feature/nodelist/networkstatus.c @@ -1448,13 +1448,10 @@ networkstatus_valid_until_is_reasonably_live(time_t valid_until, return (now <= valid_until + REASONABLY_LIVE_TIME); } -/* XXXX remove this in favor of get_live_consensus. But actually, - * leave something like it for bridge users, who need to not totally - * lose if they spend a while fetching a new consensus. */ /** As networkstatus_get_live_consensus(), but is way more tolerant of expired * consensuses. */ -networkstatus_t * -networkstatus_get_reasonably_live_consensus(time_t now, int flavor) +MOCK_IMPL(networkstatus_t *, +networkstatus_get_reasonably_live_consensus,(time_t now, int flavor)) { networkstatus_t *consensus = networkstatus_get_latest_consensus_by_flavor(flavor); diff --git a/src/feature/nodelist/networkstatus.h b/src/feature/nodelist/networkstatus.h index 6f8b2dc964..7b1a0ff72f 100644 --- a/src/feature/nodelist/networkstatus.h +++ b/src/feature/nodelist/networkstatus.h @@ -89,8 +89,9 @@ int networkstatus_consensus_reasonably_live(const networkstatus_t *consensus, time_t now); int networkstatus_valid_until_is_reasonably_live(time_t valid_until, time_t now); -networkstatus_t *networkstatus_get_reasonably_live_consensus(time_t now, - int flavor); +MOCK_DECL(networkstatus_t *,networkstatus_get_reasonably_live_consensus, + (time_t now, + int flavor)); MOCK_DECL(int, networkstatus_consensus_is_bootstrapping,(time_t now)); int networkstatus_consensus_can_use_multiple_directories( const or_options_t *options); diff --git a/src/test/test_entrynodes.c b/src/test/test_entrynodes.c index b67c9fae53..3653feaff5 100644 --- a/src/test/test_entrynodes.c +++ b/src/test/test_entrynodes.c @@ -74,9 +74,10 @@ bfn_mock_nodelist_get_list(void) } static networkstatus_t * -bfn_mock_networkstatus_get_live_consensus(time_t now) +bfn_mock_networkstatus_get_reasonably_live_consensus(time_t now, int flavor) { (void)now; + (void)flavor; return dummy_consensus; } @@ -118,7 +119,7 @@ big_fake_network_cleanup(const struct testcase_t *testcase, void *ptr) UNMOCK(nodelist_get_list); UNMOCK(node_get_by_id); UNMOCK(get_or_state); - UNMOCK(networkstatus_get_live_consensus); + UNMOCK(networkstatus_get_reasonably_live_consensus); or_state_free(dummy_state); dummy_state = NULL; tor_free(dummy_consensus); @@ -198,8 +199,8 @@ big_fake_network_setup(const struct testcase_t *testcase) MOCK(node_get_by_id, bfn_mock_node_get_by_id); MOCK(get_or_state, get_or_state_replacement); - MOCK(networkstatus_get_live_consensus, - bfn_mock_networkstatus_get_live_consensus); + MOCK(networkstatus_get_reasonably_live_consensus, + bfn_mock_networkstatus_get_reasonably_live_consensus); /* Return anything but NULL (it's interpreted as test fail) */ return (void*)testcase; } From 390112d07e7daad1ce1e79b37c3357ca936fd9ae Mon Sep 17 00:00:00 2001 From: teor Date: Thu, 22 Nov 2018 17:10:28 +1000 Subject: [PATCH 4/5] Entry Nodes: refactor tests to use macros Part of 24661. --- src/test/test_entrynodes.c | 69 +++++++++++++++++++------------------- 1 file changed, 35 insertions(+), 34 deletions(-) diff --git a/src/test/test_entrynodes.c b/src/test/test_entrynodes.c index 3653feaff5..1071b04ce1 100644 --- a/src/test/test_entrynodes.c +++ b/src/test/test_entrynodes.c @@ -2692,7 +2692,7 @@ test_entry_guard_upgrade_not_blocked_by_worse_circ_pending(void *arg) } static void -test_enty_guard_should_expire_waiting(void *arg) +test_entry_guard_should_expire_waiting(void *arg) { (void)arg; circuit_guard_state_t *fake_state = tor_malloc_zero(sizeof(*fake_state)); @@ -3010,39 +3010,38 @@ static const struct testcase_setup_t upgrade_circuits = { upgrade_circuits_setup, upgrade_circuits_cleanup }; -#define BFN_TEST(name) \ - { #name, test_entry_guard_ ## name, TT_FORK, &big_fake_network, NULL } +#define NO_PREFIX_TEST(name) \ + { #name, test_ ## name, 0, NULL, NULL } -#define UPGRADE_TEST(name, arg) \ - { #name, test_entry_guard_ ## name, TT_FORK, &upgrade_circuits, \ - (void*)(arg) } +#define EN_TEST_BASE(name, fork, setup, arg) \ + { #name, test_entry_guard_ ## name, fork, setup, (void*)(arg) } + +#define EN_TEST(name) EN_TEST_BASE(name, 0, NULL, NULL) +#define EN_TEST_FORK(name) EN_TEST_BASE(name, TT_FORK, NULL, NULL) + +#define BFN_TEST(name) \ + EN_TEST_BASE(name, TT_FORK, &big_fake_network, NULL) + +#define UPGRADE_TEST(name, arg) \ + EN_TEST_BASE(name, TT_FORK, &upgrade_circuits, arg) struct testcase_t entrynodes_tests[] = { - { "node_preferred_orport", - test_node_preferred_orport, - 0, NULL, NULL }, - { "entry_guard_describe", test_entry_guard_describe, 0, NULL, NULL }, - { "randomize_time", test_entry_guard_randomize_time, 0, NULL, NULL }, - { "encode_for_state_minimal", - test_entry_guard_encode_for_state_minimal, 0, NULL, NULL }, - { "encode_for_state_maximal", - test_entry_guard_encode_for_state_maximal, 0, NULL, NULL }, - { "parse_from_state_minimal", - test_entry_guard_parse_from_state_minimal, 0, NULL, NULL }, - { "parse_from_state_maximal", - test_entry_guard_parse_from_state_maximal, 0, NULL, NULL }, - { "parse_from_state_failure", - test_entry_guard_parse_from_state_failure, 0, NULL, NULL }, - { "parse_from_state_partial_failure", - test_entry_guard_parse_from_state_partial_failure, 0, NULL, NULL }, - { "parse_from_state_full", - test_entry_guard_parse_from_state_full, TT_FORK, NULL, NULL }, - { "parse_from_state_broken", - test_entry_guard_parse_from_state_broken, TT_FORK, NULL, NULL }, - { "get_guard_selection_by_name", - test_entry_guard_get_guard_selection_by_name, TT_FORK, NULL, NULL }, - { "number_of_primaries", - test_entry_guard_number_of_primaries, TT_FORK, NULL, NULL }, + NO_PREFIX_TEST(node_preferred_orport), + NO_PREFIX_TEST(entry_guard_describe), + + EN_TEST(randomize_time), + EN_TEST(encode_for_state_minimal), + EN_TEST(encode_for_state_maximal), + EN_TEST(parse_from_state_minimal), + EN_TEST(parse_from_state_maximal), + EN_TEST(parse_from_state_failure), + EN_TEST(parse_from_state_partial_failure), + + EN_TEST_FORK(parse_from_state_full), + EN_TEST_FORK(parse_from_state_broken), + EN_TEST_FORK(get_guard_selection_by_name), + EN_TEST_FORK(number_of_primaries), + BFN_TEST(choose_selection_initial), BFN_TEST(add_single_guard), BFN_TEST(node_filter), @@ -3056,7 +3055,9 @@ struct testcase_t entrynodes_tests[] = { BFN_TEST(sample_reachable_filtered_empty), BFN_TEST(retry_unreachable), BFN_TEST(manage_primary), - { "guard_preferred", test_entry_guard_guard_preferred, TT_FORK, NULL, NULL }, + + EN_TEST_FORK(guard_preferred), + BFN_TEST(select_for_circuit_no_confirmed), BFN_TEST(select_for_circuit_confirmed), BFN_TEST(select_for_circuit_highlevel_primary), @@ -3079,8 +3080,8 @@ struct testcase_t entrynodes_tests[] = { UPGRADE_TEST(upgrade_not_blocked_by_restricted_circ_pending, "c2-done"), UPGRADE_TEST(upgrade_not_blocked_by_worse_circ_pending, "c1-done"), - { "should_expire_waiting", test_enty_guard_should_expire_waiting, TT_FORK, - NULL, NULL }, + + EN_TEST_FORK(should_expire_waiting), END_OF_TESTCASES }; From 805f75182a87286a727df323be7c37add595bdff Mon Sep 17 00:00:00 2001 From: teor Date: Thu, 22 Nov 2018 17:47:12 +1000 Subject: [PATCH 5/5] Entry Nodes: Test on reasonably live consensuses As well as live consensuses. Tests for 24661. --- src/test/test_entrynodes.c | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-) diff --git a/src/test/test_entrynodes.c b/src/test/test_entrynodes.c index 1071b04ce1..52b87a84a8 100644 --- a/src/test/test_entrynodes.c +++ b/src/test/test_entrynodes.c @@ -137,6 +137,12 @@ big_fake_network_setup(const struct testcase_t *testcase) * that we need for entrynodes.c. */ const int N_NODES = 271; + const char *argument = testcase->setup_data; + int reasonably_live_consensus = 0; + if (argument) { + reasonably_live_consensus = strstr(argument, "reasonably-live") != NULL; + } + big_fake_net_nodes = smartlist_new(); for (i = 0; i < N_NODES; ++i) { curve25519_secret_key_t curve25519_secret_key; @@ -192,8 +198,16 @@ big_fake_network_setup(const struct testcase_t *testcase) dummy_state = tor_malloc_zero(sizeof(or_state_t)); dummy_consensus = tor_malloc_zero(sizeof(networkstatus_t)); - dummy_consensus->valid_after = approx_time() - 3600; - dummy_consensus->valid_until = approx_time() + 3600; + if (reasonably_live_consensus) { + /* Make the dummy consensus valid from 4 hours ago, but expired an hour + * ago. */ + dummy_consensus->valid_after = approx_time() - 4*3600; + dummy_consensus->valid_until = approx_time() - 3600; + } else { + /* Make the dummy consensus valid for an hour either side of now. */ + dummy_consensus->valid_after = approx_time() - 3600; + dummy_consensus->valid_until = approx_time() + 3600; + } MOCK(nodelist_get_list, bfn_mock_nodelist_get_list); MOCK(node_get_by_id, bfn_mock_node_get_by_id); @@ -3020,10 +3034,14 @@ static const struct testcase_setup_t upgrade_circuits = { #define EN_TEST_FORK(name) EN_TEST_BASE(name, TT_FORK, NULL, NULL) #define BFN_TEST(name) \ - EN_TEST_BASE(name, TT_FORK, &big_fake_network, NULL) + EN_TEST_BASE(name, TT_FORK, &big_fake_network, NULL), \ + { #name "_reasonably_live", test_entry_guard_ ## name, TT_FORK, \ + &big_fake_network, (void*)("reasonably-live") } #define UPGRADE_TEST(name, arg) \ - EN_TEST_BASE(name, TT_FORK, &upgrade_circuits, arg) + EN_TEST_BASE(name, TT_FORK, &upgrade_circuits, arg), \ + { #name "_reasonably_live", test_entry_guard_ ## name, TT_FORK, \ + &upgrade_circuits, (void*)(arg " reasonably-live") } struct testcase_t entrynodes_tests[] = { NO_PREFIX_TEST(node_preferred_orport),