[tor-commits] [tor/master] Improve tolerance for dirauths with skewed clocks

[nickm at torproject.org]

commit de343b4e421c0c651eaac1d52d23c3c792bee73a
Author: Taylor Yu <catalyst at torproject.org>
Date:   Fri May 4 17:16:06 2018 -0500

    Improve tolerance for dirauths with skewed clocks
    
    Previously, an authority with a clock more than 60 seconds ahead could
    cause a client with a correct clock to warn that the client's clock
    was behind.  Now the clocks of a majority of directory authorities
    have to be ahead of the client before this warning will occur.
    
    Relax the early-consensus check so that a client's clock must be 60
    seconds behind the earliest time that a given sufficiently-signed
    consensus could possibly be available.
    
    Add a new unit test that calls warn_early_consensus() directly.
    
    Fixes bug 25756; bugfix on 0.2.2.25-alpha.
---
 changes/bug25756           |  7 +++++
 src/or/networkstatus.c     | 14 +++++++---
 src/test/test_routerlist.c | 64 +++++++++++++++++++++++++++++++++++++++++++---
 3 files changed, 79 insertions(+), 6 deletions(-)

diff --git a/changes/bug25756 b/changes/bug25756
new file mode 100644
index 000000000..ff5ac0391
--- /dev/null
+++ b/changes/bug25756
@@ -0,0 +1,7 @@
+  o Minor bugfixes (error reporting):
+    - Improve tolerance for directory authorities with skewed clocks.
+      Previously, an authority with a clock more than 60 seconds ahead
+      could cause a client with a correct clock to warn that the
+      client's clock was behind.  Now the clocks of a majority of
+      directory authorities have to be ahead of the client before this
+      warning will occur.  Fixes bug 25756; bugfix on 0.2.2.25-alpha.
diff --git a/src/or/networkstatus.c b/src/or/networkstatus.c
index d1c217119..51b2f4af1 100644
--- a/src/or/networkstatus.c
+++ b/src/or/networkstatus.c
@@ -1777,10 +1777,18 @@ warn_early_consensus(const networkstatus_t *c, const char *flavor,
   long delta = now - c->valid_after;
   char *flavormsg = NULL;
 
-/** If a consensus appears more than this many seconds before its declared
- * valid-after time, declare that our clock is skewed. */
+/** If a consensus appears more than this many seconds before it could
+ * possibly be a sufficiently-signed consensus, declare that our clock
+ * is skewed. */
 #define EARLY_CONSENSUS_NOTICE_SKEW 60
-  if (now >= c->valid_after - EARLY_CONSENSUS_NOTICE_SKEW)
+
+  /* We assume that if a majority of dirauths have accurate clocks,
+   * the earliest that a dirauth with a skewed clock could possibly
+   * publish a sufficiently-signed consensus is (valid_after -
+   * dist_seconds).  Before that time, the skewed dirauth would be
+   * unable to obtain enough authority signatures for the consensus to
+   * be valid. */
+  if (now >= c->valid_after - c->dist_seconds - EARLY_CONSENSUS_NOTICE_SKEW)
     return;
 
   format_iso_time(tbuf, c->valid_after);
diff --git a/src/test/test_routerlist.c b/src/test/test_routerlist.c
index 6dd58d313..701227c1c 100644
--- a/src/test/test_routerlist.c
+++ b/src/test/test_routerlist.c
@@ -692,6 +692,62 @@ test_early_consensus(void *arg)
   UNMOCK(clock_skew_warning);
 }
 
+/** Test warn_early_consensus(), expecting no warning  */
+static void
+test_warn_early_consensus_no(const networkstatus_t *c, time_t now,
+                             long offset)
+{
+  mock_apparent_skew = 0;
+  setup_capture_of_logs(LOG_WARN);
+  warn_early_consensus(c, "microdesc", now + offset);
+  expect_no_log_msg_containing("behind the time published in the consensus");
+  tt_int_op(mock_apparent_skew, OP_EQ, 0);
+ done:
+  teardown_capture_of_logs();
+}
+
+/** Test warn_early_consensus(), expecting a warning */
+static void
+test_warn_early_consensus_yes(const networkstatus_t *c, time_t now,
+                              long offset)
+{
+  mock_apparent_skew = 0;
+  setup_capture_of_logs(LOG_WARN);
+  warn_early_consensus(c, "microdesc", now + offset);
+  /* Can't use expect_single_log_msg() because of unrecognized authorities */
+  expect_log_msg_containing("behind the time published in the consensus");
+  tt_int_op(mock_apparent_skew, OP_EQ, offset);
+ done:
+  teardown_capture_of_logs();
+}
+
+/**
+ * Test warn_early_consensus() directly, checking both the non-warning
+ * case (consensus is not early) and the warning case (consensus is
+ * early).  Depends on EARLY_CONSENSUS_NOTICE_SKEW=60.
+ */
+static void
+test_warn_early_consensus(void *arg)
+{
+  networkstatus_t *c = NULL;
+  time_t now = time(NULL);
+
+  (void)arg;
+  c = tor_malloc_zero(sizeof *c);
+  c->valid_after = now;
+  c->dist_seconds = 300;
+  mock_apparent_skew = 0;
+  MOCK(clock_skew_warning, mock_clock_skew_warning);
+  test_warn_early_consensus_no(c, now, 60);
+  test_warn_early_consensus_no(c, now, 0);
+  test_warn_early_consensus_no(c, now, -60);
+  test_warn_early_consensus_no(c, now, -360);
+  test_warn_early_consensus_yes(c, now, -361);
+  test_warn_early_consensus_yes(c, now, -600);
+  UNMOCK(clock_skew_warning);
+  tor_free(c);
+}
+
 #define NODE(name, flags) \
   { #name, test_routerlist_##name, (flags), NULL, NULL }
 #define ROUTER(name,flags) \
@@ -711,11 +767,13 @@ struct testcase_t routerlist_tests[] = {
   ROUTER(pick_directory_server_impl, TT_FORK),
   { "directory_guard_fetch_with_no_dirinfo",
     test_directory_guard_fetch_with_no_dirinfo, TT_FORK, NULL, NULL },
-  /* These depend on construct_consensus() setting valid_after=now+1000 */
+  /* These depend on construct_consensus() setting
+   * valid_after=now+1000 and dist_seconds=250 */
   TIMELY("timely_consensus1", "1010"),
   TIMELY("timely_consensus2", "1000"),
-  TIMELY("timely_consensus3", "940"),
-  EARLY("early_consensus1", "939"),
+  TIMELY("timely_consensus3", "690"),
+  EARLY("early_consensus1", "689"),
+  { "warn_early_consensus", test_warn_early_consensus, 0, NULL, NULL },
   END_OF_TESTCASES
 };