Merge remote-tracking branch 'tor-github/pr/536'

7 files changed, 93 insertions, 60 deletions

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/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/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 "
diff --git a/src/feature/nodelist/microdesc.c b/src/feature/nodelist/microdesc.c
index f331d5e109..eadca69548 100644
--- a/src/feature/nodelist/microdesc.c
+++ b/src/feature/nodelist/microdesc.c
@@ -109,10 +109,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;
   }
 
diff --git a/src/feature/nodelist/networkstatus.c b/src/feature/nodelist/networkstatus.c
index f1def9afb1..5e62903d48 100644
--- a/src/feature/nodelist/networkstatus.c
+++ b/src/feature/nodelist/networkstatus.c
@@ -1459,13 +1459,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 8802de2d65..147104fecf 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 4cd50463d4..16b14c0b73 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);
@@ -136,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;
@@ -191,15 +198,23 @@ 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);
   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;
 }
@@ -2691,7 +2706,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));
@@ -3012,39 +3027,42 @@ static const struct testcase_setup_t upgrade_circuits = {
   upgrade_circuits_setup, upgrade_circuits_cleanup
 };
 
+#define NO_PREFIX_TEST(name) \
+  { #name, test_ ## name, 0, NULL, NULL }
+
+#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) \
-  { #name, test_entry_guard_ ## 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)                                         \
-  { #name, test_entry_guard_ ## name, TT_FORK, &upgrade_circuits,       \
-      (void*)(arg) }
+#define UPGRADE_TEST(name, 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[] = {
-  { "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),
@@ -3058,7 +3076,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),
@@ -3081,8 +3101,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
 };