Opened 10 months ago

Closed 8 months ago

#20511 closed enhancement (implemented)

add a failsafe where if you're about to serve a consensus that you know is obsolete, don't do it

Reported by: arma Owned by:
Priority: Medium Milestone: Tor: 0.3.0.x-final
Component: Core Tor/Tor Version: Tor: 0.2.9.1-alpha
Severity: Normal Keywords: 029-backport
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

http://77.247.181.162/tor/status-vote/current/consensus-microdesc
is offering me a microdesc consensus document which is valid-until 2016-09-28 22:00:00. Today is Oct 31.

That relay knows what time it is -- when I establish my Tor channel to it and do a begindir fetch, it gives me good answers in the http header responses.

Yet it is happy to give me a consensus that's a month old. And my Tor client doesn't complain in any obvious useful way. I just get the cryptic line:

Oct 31 23:12:29.455 [notice] I learned some more directory information, but not enough to build a circuit: We have no recent usable consensus.

Is there a consensus age past which the correct answer for a dir cache in that situation should be "I'm really sorry, but it's for your own good: 404"?

We wouldn't want that age to be too small, or the network would fall over a lot faster in the case where it really has been 15 hours since the directory authorities made a consensus.

(Also, is 404 the right return code? It isn't that I don't have it, it's that you're not going to like it.)

Child Tickets

Change History (18)

comment:1 Changed 10 months ago by rubiate

Is there a consensus age past which the correct answer for a dir cache in that situation should be "I'm really sorry, but it's for your own good: 404"?

24 hours is how long past the valid-until time it can be before "We have no recent usable consensus" happens (per update_router_have_minimum_dir_info() and networkstatus_get_reasonably_live_consensus()).

comment:2 Changed 10 months ago by nickm

Milestone: Tor: 0.3.0.x-final

404 seems fine to me. You asked for a "current" consensus. I don't have one of those.

Tenatively assigning this to 0.3.0

comment:3 Changed 10 months ago by teor

Clock skew on relays becomes much more important after this fix, but the directory authorities already check for that (~3 hours).

comment:4 Changed 10 months ago by rubiate

Will this work? I'm not sure that this is all that's needed or if there's other places that it needs to be checked.

I thought Tor should warn that this has happened, but probably shouldn't do it at a normal log-level on every dir request as that could get noisy.

diff --git a/src/or/directory.c b/src/or/directory.c
index ba6d38c..16e33c5 100644
--- a/src/or/directory.c
+++ b/src/or/directory.c
@@ -2939,6 +2939,32 @@ handle_get_frontpage(dir_connection_t *conn, const get_handler_args_t *args)
   return 0;
 }
 
+/** Warn that the consensus <b>v</b> of type <b>flavor</b> is too old and will
+ * not be served. Log the message at a lower severity if we have already
+ * warned about this recently.
+ */
+static void
+warn_consensus_is_too_old(networkstatus_t *v, const char *flavor, time_t now)
+{
+  static time_t warned = 0;
+  int severity;
+  char timestamp[ISO_TIME_LEN+1];
+
+  if (now >= warned + 60 * 60) {
+    severity = LOG_WARN;
+    warned = now;
+  } else {
+    severity = LOG_DEBUG;
+  }
+
+  format_local_iso_time(timestamp, v->valid_until);
+  log_fn(severity, LD_DIRSERV,
+         "Our %s%sconsensus is too old, we will not serve it to clients. "
+         "It was valid until %s and we continued to serve it for up to 24 "
+         "hours after it expired.",
+         flavor ? flavor : "", flavor ? " " : "", timestamp);
+}
+
 /** Helper function for GET /tor/status-vote/current/consensus
  */
 static int
@@ -2983,6 +3009,15 @@ handle_get_current_consensus(dir_connection_t *conn,
 
       v = networkstatus_get_latest_consensus_by_flavor(flav);
 
+      if (v && !networkstatus_consensus_reasonably_live(v, now)) {
+        write_http_status_line(conn, 404, "Consensus is too old");
+        warn_consensus_is_too_old(v, flavor, now);
+        smartlist_free(dir_fps);
+        geoip_note_ns_response(GEOIP_REJECT_NOT_FOUND);
+        tor_free(flavor);
+        goto done;
+      }
+
       if (v && want_fps &&
           !client_likes_consensus(v, want_fps)) {
         write_http_status_line(conn, 404, "Consensus not signed by sufficient "
diff --git a/src/or/networkstatus.c b/src/or/networkstatus.c
index ed888fb..7f200b9 100644
--- a/src/or/networkstatus.c
+++ b/src/or/networkstatus.c
@@ -1342,6 +1342,21 @@ networkstatus_get_live_consensus,(time_t now))
     return NULL;
 }
 
+/** Determine if <b>consensus</b> is valid or expired recently enough that
+ * we can still use it.
+ *
+ * Return 1 if the consensus is reasonably live, or 0 if it is too old.
+ */
+int
+networkstatus_consensus_reasonably_live(networkstatus_t *consensus, time_t now)
+{
+#define REASONABLY_LIVE_TIME (24*60*60)
+  if (now <= consensus->valid_until + REASONABLY_LIVE_TIME)
+    return 1;
+
+  return 0;
+}
+
 /* 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. */
@@ -1350,12 +1365,11 @@ networkstatus_get_live_consensus,(time_t now))
 networkstatus_t *
 networkstatus_get_reasonably_live_consensus(time_t now, int flavor)
 {
-#define REASONABLY_LIVE_TIME (24*60*60)
   networkstatus_t *consensus =
     networkstatus_get_latest_consensus_by_flavor(flavor);
   if (consensus &&
       consensus->valid_after <= now &&
-      now <= consensus->valid_until+REASONABLY_LIVE_TIME)
+      networkstatus_consensus_reasonably_live(consensus, now))
     return consensus;
   else
     return NULL;
diff --git a/src/or/networkstatus.h b/src/or/networkstatus.h
index 71f36b6..172c0ea 100644
--- a/src/or/networkstatus.h
+++ b/src/or/networkstatus.h
@@ -79,6 +79,8 @@ MOCK_DECL(networkstatus_t *,networkstatus_get_latest_consensus,(void));
 MOCK_DECL(networkstatus_t *,networkstatus_get_latest_consensus_by_flavor,
           (consensus_flavor_t f));
 MOCK_DECL(networkstatus_t *, networkstatus_get_live_consensus,(time_t now));
+int networkstatus_consensus_reasonably_live(networkstatus_t *consensus,
+                                            time_t now);
 networkstatus_t *networkstatus_get_reasonably_live_consensus(time_t now,
                                                              int flavor);
 MOCK_DECL(int, networkstatus_consensus_is_bootstrapping,(time_t now));

Tests and changes file:

Note there's an existing test that needed to be changed to set the valid_until time or else it fails for it's consensus being too old.

diff --git a/changes/20511 b/changes/20511
new file mode 100644
index 0000000..d6e962e
--- /dev/null
+++ b/changes/20511
@@ -0,0 +1,3 @@
+  o Minor feature:
+    - Relays and bridges will now refuse to serve the consensus they have if
+      they know it is too old for a client to use. Closes ticket 20511.
diff --git a/src/test/test_dir_handle_get.c b/src/test/test_dir_handle_get.c
index c215fee..cab3fb2 100644
--- a/src/test/test_dir_handle_get.c
+++ b/src/test/test_dir_handle_get.c
@@ -30,6 +30,7 @@
 #include "dirserv.h"
 #include "torgzip.h"
 #include "dirvote.h"
+#include "log_test_helpers.h"
 
 #ifdef _WIN32
 /* For mkdir() */
@@ -53,6 +54,7 @@ ENABLE_GCC_WARNING(overlength-strings)
 #define NOT_FOUND "HTTP/1.0 404 Not found\r\n\r\n"
 #define BAD_REQUEST "HTTP/1.0 400 Bad request\r\n\r\n"
 #define SERVER_BUSY "HTTP/1.0 503 Directory busy, try again later\r\n\r\n"
+#define TOO_OLD "HTTP/1.0 404 Consensus is too old\r\n\r\n"
 #define NOT_ENOUGH_CONSENSUS_SIGNATURES "HTTP/1.0 404 " \
   "Consensus not signed by sufficient number of requested authorities\r\n\r\n"
 
@@ -1617,6 +1619,7 @@ test_dir_handle_get_status_vote_current_consensus_ns_not_enough_sigs(void* d)
   mock_ns_val = tor_malloc_zero(sizeof(networkstatus_t));
   mock_ns_val->flavor = FLAV_NS;
   mock_ns_val->voters = smartlist_new();
+  mock_ns_val->valid_until = time(NULL);
 
   /* init mock */
   init_mock_options();
@@ -1696,6 +1699,63 @@ test_dir_handle_get_status_vote_current_consensus_ns_not_found(void* data)
     or_options_free(mock_options); mock_options = NULL;
 }
 
+static void
+test_dir_handle_get_status_vote_current_consensus_too_old(void *data)
+{
+  dir_connection_t *conn = NULL;
+  char *header = NULL;
+  (void)data;
+
+  mock_ns_val = tor_malloc_zero(sizeof(networkstatus_t));
+  mock_ns_val->flavor = FLAV_MICRODESC;
+  mock_ns_val->valid_until = time(NULL) - (60 * 60 * 24) - 1;
+
+  init_mock_options();
+  MOCK(get_options, mock_get_options);
+  MOCK(connection_write_to_buf_impl_, connection_write_to_buf_mock);
+  MOCK(networkstatus_get_latest_consensus_by_flavor, mock_ns_get_by_flavor);
+
+  conn = new_dir_conn();
+
+  setup_capture_of_logs(LOG_WARN);
+
+  tt_int_op(0, OP_EQ, directory_handle_command_get(conn,
+    GET("/tor/status-vote/current/consensus-microdesc"), NULL, 0));
+
+  fetch_from_buf_http(TO_CONN(conn)->outbuf, &header, MAX_HEADERS_SIZE,
+                      NULL, NULL, 1, 0);
+  tt_assert(header);
+  tt_str_op(TOO_OLD, OP_EQ, header);
+
+  expect_log_msg_containing("too old");
+
+  tor_free(header);
+  header = NULL;
+  teardown_capture_of_logs();
+
+  setup_capture_of_logs(LOG_WARN);
+
+  tt_int_op(0, OP_EQ, directory_handle_command_get(conn,
+    GET("/tor/status-vote/current/consensus"), NULL, 0));
+
+  fetch_from_buf_http(TO_CONN(conn)->outbuf, &header, MAX_HEADERS_SIZE,
+                      NULL, NULL, 1, 0);
+  tt_assert(header);
+  tt_str_op(TOO_OLD, OP_EQ, header);
+
+  expect_no_log_entry();
+
+  done:
+    teardown_capture_of_logs();
+    UNMOCK(networkstatus_get_latest_consensus_by_flavor);
+    UNMOCK(connection_write_to_buf_impl_);
+    UNMOCK(get_options);
+    connection_free_(TO_CONN(conn));
+    tor_free(header);
+    tor_free(mock_ns_val);
+    or_options_free(mock_options); mock_options = NULL;
+}
+
 NS_DECL(int, geoip_get_country_by_addr, (const tor_addr_t *addr));
 
 int
@@ -2481,6 +2541,7 @@ struct testcase_t dir_handle_get_tests[] = {
   DIR_HANDLE_CMD(status_vote_next_authority, 0),
   DIR_HANDLE_CMD(status_vote_current_consensus_ns_not_enough_sigs, 0),
   DIR_HANDLE_CMD(status_vote_current_consensus_ns_not_found, 0),
+  DIR_HANDLE_CMD(status_vote_current_consensus_too_old, 0),
   DIR_HANDLE_CMD(status_vote_current_consensus_ns_busy, 0),
   DIR_HANDLE_CMD(status_vote_current_consensus_ns, 0),
   DIR_HANDLE_CMD(status_vote_current_d_not_found, 0),


comment:5 Changed 10 months ago by nickm

Status: newneeds_review

Thanks; putting this on the review queue!

(Next time, if you use 'git format-patch' it produces something much easier to merge.)

comment:6 Changed 10 months ago by teor

Status: needs_reviewneeds_revision

This patch looks good.
Since networkstatus_consensus_reasonably_live depends on consensus being non-NULL, I suggest you do something like:

if (BUG(!consensus)) {
  return 0;
}

A nitpick, I would make the 60*60 log severity time in a #define.

Another (separate) issue is whether clients should reject consensuses that are obviously too old. This was fixed in #20533: clients that receive a consensus after its valid_until time (or that try to download certificates for an expired consensus) will stop downloading certificates and consider the consensus a failure. I think we also reject old consensuses as soon as we parse them, but we should check this. See #20601.

Last edited 10 months ago by teor (previous) (diff)

comment:7 Changed 10 months ago by rubiate

Here's an updated patch using a define for the warning timeout and a NULL consensus check:

diff --git a/changes/20511 b/changes/20511
new file mode 100644
index 0000000..d6e962e
--- /dev/null
+++ b/changes/20511
@@ -0,0 +1,3 @@
+  o Minor feature:
+    - Relays and bridges will now refuse to serve the consensus they have if
+      they know it is too old for a client to use. Closes ticket 20511.
diff --git a/src/or/directory.c b/src/or/directory.c
index ba6d38c..24393eb 100644
--- a/src/or/directory.c
+++ b/src/or/directory.c
@@ -2939,6 +2939,33 @@ handle_get_frontpage(dir_connection_t *conn, const get_handler_args_t *args)
   return 0;
 }
 
+/** Warn that the consensus <b>v</b> of type <b>flavor</b> is too old and will
+ * not be served. Log the message at a lower severity if we have already
+ * warned about this recently.
+ */
+static void
+warn_consensus_is_too_old(networkstatus_t *v, const char *flavor, time_t now)
+{
+  static time_t warned = 0;
+  int severity;
+  char timestamp[ISO_TIME_LEN+1];
+
+#define TOO_OLD_WARNING_TIMEOUT 60*60
+  if (now >= warned + TOO_OLD_WARNING_TIMEOUT) {
+    severity = LOG_WARN;
+    warned = now;
+  } else {
+    severity = LOG_DEBUG;
+  }
+
+  format_local_iso_time(timestamp, v->valid_until);
+  log_fn(severity, LD_DIRSERV,
+         "Our %s%sconsensus is too old, we will not serve it to clients. "
+         "It was valid until %s and we continued to serve it for up to 24 "
+         "hours after it expired.",
+         flavor ? flavor : "", flavor ? " " : "", timestamp);
+}
+
 /** Helper function for GET /tor/status-vote/current/consensus
  */
 static int
@@ -2983,6 +3010,15 @@ handle_get_current_consensus(dir_connection_t *conn,
 
       v = networkstatus_get_latest_consensus_by_flavor(flav);
 
+      if (v && !networkstatus_consensus_reasonably_live(v, now)) {
+        write_http_status_line(conn, 404, "Consensus is too old");
+        warn_consensus_is_too_old(v, flavor, now);
+        smartlist_free(dir_fps);
+        geoip_note_ns_response(GEOIP_REJECT_NOT_FOUND);
+        tor_free(flavor);
+        goto done;
+      }
+
       if (v && want_fps &&
           !client_likes_consensus(v, want_fps)) {
         write_http_status_line(conn, 404, "Consensus not signed by sufficient "
diff --git a/src/or/networkstatus.c b/src/or/networkstatus.c
index ed888fb..fde0b18 100644
--- a/src/or/networkstatus.c
+++ b/src/or/networkstatus.c
@@ -1342,6 +1342,24 @@ networkstatus_get_live_consensus,(time_t now))
     return NULL;
 }
 
+/** Determine if <b>consensus</b> is valid or expired recently enough that
+ * we can still use it.
+ *
+ * Return 1 if the consensus is reasonably live, or 0 if it is too old.
+ */
+int
+networkstatus_consensus_reasonably_live(networkstatus_t *consensus, time_t now)
+{
+#define REASONABLY_LIVE_TIME (24*60*60)
+  if (BUG(!consensus))
+    return 0;
+
+  if (now <= consensus->valid_until + REASONABLY_LIVE_TIME)
+    return 1;
+
+  return 0;
+}
+
 /* 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. */
@@ -1350,12 +1368,11 @@ networkstatus_get_live_consensus,(time_t now))
 networkstatus_t *
 networkstatus_get_reasonably_live_consensus(time_t now, int flavor)
 {
-#define REASONABLY_LIVE_TIME (24*60*60)
   networkstatus_t *consensus =
     networkstatus_get_latest_consensus_by_flavor(flavor);
   if (consensus &&
       consensus->valid_after <= now &&
-      now <= consensus->valid_until+REASONABLY_LIVE_TIME)
+      networkstatus_consensus_reasonably_live(consensus, now))
     return consensus;
   else
     return NULL;
diff --git a/src/or/networkstatus.h b/src/or/networkstatus.h
index 71f36b6..172c0ea 100644
--- a/src/or/networkstatus.h
+++ b/src/or/networkstatus.h
@@ -79,6 +79,8 @@ MOCK_DECL(networkstatus_t *,networkstatus_get_latest_consensus,(void));
 MOCK_DECL(networkstatus_t *,networkstatus_get_latest_consensus_by_flavor,
           (consensus_flavor_t f));
 MOCK_DECL(networkstatus_t *, networkstatus_get_live_consensus,(time_t now));
+int networkstatus_consensus_reasonably_live(networkstatus_t *consensus,
+                                            time_t now);
 networkstatus_t *networkstatus_get_reasonably_live_consensus(time_t now,
                                                              int flavor);
 MOCK_DECL(int, networkstatus_consensus_is_bootstrapping,(time_t now));
diff --git a/src/test/test_dir_handle_get.c b/src/test/test_dir_handle_get.c
index c215fee..cab3fb2 100644
--- a/src/test/test_dir_handle_get.c
+++ b/src/test/test_dir_handle_get.c
@@ -30,6 +30,7 @@
 #include "dirserv.h"
 #include "torgzip.h"
 #include "dirvote.h"
+#include "log_test_helpers.h"
 
 #ifdef _WIN32
 /* For mkdir() */
@@ -53,6 +54,7 @@ ENABLE_GCC_WARNING(overlength-strings)
 #define NOT_FOUND "HTTP/1.0 404 Not found\r\n\r\n"
 #define BAD_REQUEST "HTTP/1.0 400 Bad request\r\n\r\n"
 #define SERVER_BUSY "HTTP/1.0 503 Directory busy, try again later\r\n\r\n"
+#define TOO_OLD "HTTP/1.0 404 Consensus is too old\r\n\r\n"
 #define NOT_ENOUGH_CONSENSUS_SIGNATURES "HTTP/1.0 404 " \
   "Consensus not signed by sufficient number of requested authorities\r\n\r\n"
 
@@ -1617,6 +1619,7 @@ test_dir_handle_get_status_vote_current_consensus_ns_not_enough_sigs(void* d)
   mock_ns_val = tor_malloc_zero(sizeof(networkstatus_t));
   mock_ns_val->flavor = FLAV_NS;
   mock_ns_val->voters = smartlist_new();
+  mock_ns_val->valid_until = time(NULL);
 
   /* init mock */
   init_mock_options();
@@ -1696,6 +1699,63 @@ test_dir_handle_get_status_vote_current_consensus_ns_not_found(void* data)
     or_options_free(mock_options); mock_options = NULL;
 }
 
+static void
+test_dir_handle_get_status_vote_current_consensus_too_old(void *data)
+{
+  dir_connection_t *conn = NULL;
+  char *header = NULL;
+  (void)data;
+
+  mock_ns_val = tor_malloc_zero(sizeof(networkstatus_t));
+  mock_ns_val->flavor = FLAV_MICRODESC;
+  mock_ns_val->valid_until = time(NULL) - (60 * 60 * 24) - 1;
+
+  init_mock_options();
+  MOCK(get_options, mock_get_options);
+  MOCK(connection_write_to_buf_impl_, connection_write_to_buf_mock);
+  MOCK(networkstatus_get_latest_consensus_by_flavor, mock_ns_get_by_flavor);
+
+  conn = new_dir_conn();
+
+  setup_capture_of_logs(LOG_WARN);
+
+  tt_int_op(0, OP_EQ, directory_handle_command_get(conn,
+    GET("/tor/status-vote/current/consensus-microdesc"), NULL, 0));
+
+  fetch_from_buf_http(TO_CONN(conn)->outbuf, &header, MAX_HEADERS_SIZE,
+                      NULL, NULL, 1, 0);
+  tt_assert(header);
+  tt_str_op(TOO_OLD, OP_EQ, header);
+
+  expect_log_msg_containing("too old");
+
+  tor_free(header);
+  header = NULL;
+  teardown_capture_of_logs();
+
+  setup_capture_of_logs(LOG_WARN);
+
+  tt_int_op(0, OP_EQ, directory_handle_command_get(conn,
+    GET("/tor/status-vote/current/consensus"), NULL, 0));
+
+  fetch_from_buf_http(TO_CONN(conn)->outbuf, &header, MAX_HEADERS_SIZE,
+                      NULL, NULL, 1, 0);
+  tt_assert(header);
+  tt_str_op(TOO_OLD, OP_EQ, header);
+
+  expect_no_log_entry();
+
+  done:
+    teardown_capture_of_logs();
+    UNMOCK(networkstatus_get_latest_consensus_by_flavor);
+    UNMOCK(connection_write_to_buf_impl_);
+    UNMOCK(get_options);
+    connection_free_(TO_CONN(conn));
+    tor_free(header);
+    tor_free(mock_ns_val);
+    or_options_free(mock_options); mock_options = NULL;
+}
+
 NS_DECL(int, geoip_get_country_by_addr, (const tor_addr_t *addr));
 
 int
@@ -2481,6 +2541,7 @@ struct testcase_t dir_handle_get_tests[] = {
   DIR_HANDLE_CMD(status_vote_next_authority, 0),
   DIR_HANDLE_CMD(status_vote_current_consensus_ns_not_enough_sigs, 0),
   DIR_HANDLE_CMD(status_vote_current_consensus_ns_not_found, 0),
+  DIR_HANDLE_CMD(status_vote_current_consensus_too_old, 0),
   DIR_HANDLE_CMD(status_vote_current_consensus_ns_busy, 0),
   DIR_HANDLE_CMD(status_vote_current_consensus_ns, 0),
   DIR_HANDLE_CMD(status_vote_current_d_not_found, 0),

comment:8 Changed 10 months ago by teor

Keywords: 029-proposed added
Status: needs_revisionmerge_ready
Version: Tor: 0.2.9.1-alpha

Looks ok to me. I am not sure whether we should backport this to 0.2.9, but given that's where the trouble started, maybe it would be a good idea to do that as a precaution.

That said, I think it should be tested in master first, and not shoved straight into 0.2.9.5-alpha.

comment:9 in reply to:  7 Changed 9 months ago by arma

Replying to rubiate:

+#define TOO_OLD_WARNING_TIMEOUT 60*60

Putting parens around this value will make you a happier camper in the future. The above line is straight out of some arithmetic precedence example bug.

Also, for your static time_t warned, did you know about the rate_limit_log() function? It might be what you wanted here.

+ format_local_iso_time(timestamp, v->valid_until);
+ log_fn(severity, LD_DIRSERV,
+ "Our %s%sconsensus is too old, we will not serve it to clients. "
+ "It was valid until %s and we continued to serve it for up to 24 "
+ "hours after it expired.",
+ flavor ? flavor : "", flavor ? " " : "", timestamp);

A) s/old, we/old, so we/

B) I think format_local_iso_time() produces a time in the local time zone, right? So we should clarify in the log message by adding a phrase like "(in local time zone)". Otherwise we've left it ambiguous whether we mean the UTC time that consensuses use, or the local time that log lines sometimes use.

+ tor_free(header);
+ header = NULL;

tor_free sets its argument to NULL already, right?

Good to see unit tests!

comment:10 in reply to:  8 Changed 9 months ago by arma

Replying to teor:

I am not sure whether we should backport this to 0.2.9, but given that's where the trouble started, maybe it would be a good idea to do that as a precaution.

I would turn that around and ask: is it likely that there's a bug in 0.2.9 that *isn't* in 0.3.0? That angle makes me think 0.3.0 is fine.

That said, I'm ok with either plan.

comment:11 Changed 9 months ago by rubiate

It might be what you wanted here.

Yes! I did look if there was something to do this, and apparently I should have looked harder. Thanks.

New patch:

  • Parens in define
  • Using rate_limit_log()
  • Log message changed
  • Removed unnecessary NULL assignment

(edit: there's now a branch with this patch in the repo https://viennan.net/git/tor.git named ticket20511, if that makes it easier).

diff --git a/changes/20511 b/changes/20511
new file mode 100644
index 0000000..d6e962e
--- /dev/null
+++ b/changes/20511
@@ -0,0 +1,3 @@
+  o Minor feature:
+    - Relays and bridges will now refuse to serve the consensus they have if
+      they know it is too old for a client to use. Closes ticket 20511.
diff --git a/src/or/directory.c b/src/or/directory.c
index ba6d38c..b5c9d49 100644
--- a/src/or/directory.c
+++ b/src/or/directory.c
@@ -2939,6 +2939,28 @@ handle_get_frontpage(dir_connection_t *conn, const get_handler_args_t *args)
   return 0;
 }
 
+/** Warn that the consensus <b>v</b> of type <b>flavor</b> is too old and will
+ * not be served to clients. Rate-limit the warning to avoid logging an entry
+ * on every request.
+ */
+static void
+warn_consensus_is_too_old(networkstatus_t *v, const char *flavor, time_t now)
+{
+#define TOO_OLD_WARNING_INTERVAL (60*60)
+  static ratelim_t warned = RATELIM_INIT(TOO_OLD_WARNING_INTERVAL);
+  char timestamp[ISO_TIME_LEN+1];
+  char *dupes;
+
+  if ((dupes = rate_limit_log(&warned, now))) {
+    format_local_iso_time(timestamp, v->valid_until);
+    log_warn(LD_DIRSERV, "Our %s%sconsensus is too old, so we will not "
+             "serve it to clients. It was valid until %s local time and we "
+             "continued to serve it for up to 24 hours after it expired.%s",
+             flavor ? flavor : "", flavor ? " " : "", timestamp, dupes);
+    tor_free(dupes);
+  }
+}
+
 /** Helper function for GET /tor/status-vote/current/consensus
  */
 static int
@@ -2983,6 +3005,15 @@ handle_get_current_consensus(dir_connection_t *conn,
 
       v = networkstatus_get_latest_consensus_by_flavor(flav);
 
+      if (v && !networkstatus_consensus_reasonably_live(v, now)) {
+        write_http_status_line(conn, 404, "Consensus is too old");
+        warn_consensus_is_too_old(v, flavor, now);
+        smartlist_free(dir_fps);
+        geoip_note_ns_response(GEOIP_REJECT_NOT_FOUND);
+        tor_free(flavor);
+        goto done;
+      }
+
       if (v && want_fps &&
           !client_likes_consensus(v, want_fps)) {
         write_http_status_line(conn, 404, "Consensus not signed by sufficient "
diff --git a/src/or/networkstatus.c b/src/or/networkstatus.c
index ed888fb..fde0b18 100644
--- a/src/or/networkstatus.c
+++ b/src/or/networkstatus.c
@@ -1342,6 +1342,24 @@ networkstatus_get_live_consensus,(time_t now))
     return NULL;
 }
 
+/** Determine if <b>consensus</b> is valid or expired recently enough that
+ * we can still use it.
+ *
+ * Return 1 if the consensus is reasonably live, or 0 if it is too old.
+ */
+int
+networkstatus_consensus_reasonably_live(networkstatus_t *consensus, time_t now)
+{
+#define REASONABLY_LIVE_TIME (24*60*60)
+  if (BUG(!consensus))
+    return 0;
+
+  if (now <= consensus->valid_until + REASONABLY_LIVE_TIME)
+    return 1;
+
+  return 0;
+}
+
 /* 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. */
@@ -1350,12 +1368,11 @@ networkstatus_get_live_consensus,(time_t now))
 networkstatus_t *
 networkstatus_get_reasonably_live_consensus(time_t now, int flavor)
 {
-#define REASONABLY_LIVE_TIME (24*60*60)
   networkstatus_t *consensus =
     networkstatus_get_latest_consensus_by_flavor(flavor);
   if (consensus &&
       consensus->valid_after <= now &&
-      now <= consensus->valid_until+REASONABLY_LIVE_TIME)
+      networkstatus_consensus_reasonably_live(consensus, now))
     return consensus;
   else
     return NULL;
diff --git a/src/or/networkstatus.h b/src/or/networkstatus.h
index 71f36b6..172c0ea 100644
--- a/src/or/networkstatus.h
+++ b/src/or/networkstatus.h
@@ -79,6 +79,8 @@ MOCK_DECL(networkstatus_t *,networkstatus_get_latest_consensus,(void));
 MOCK_DECL(networkstatus_t *,networkstatus_get_latest_consensus_by_flavor,
           (consensus_flavor_t f));
 MOCK_DECL(networkstatus_t *, networkstatus_get_live_consensus,(time_t now));
+int networkstatus_consensus_reasonably_live(networkstatus_t *consensus,
+                                            time_t now);
 networkstatus_t *networkstatus_get_reasonably_live_consensus(time_t now,
                                                              int flavor);
 MOCK_DECL(int, networkstatus_consensus_is_bootstrapping,(time_t now));
diff --git a/src/test/test_dir_handle_get.c b/src/test/test_dir_handle_get.c
index c215fee..a0868f9 100644
--- a/src/test/test_dir_handle_get.c
+++ b/src/test/test_dir_handle_get.c
@@ -30,6 +30,7 @@
 #include "dirserv.h"
 #include "torgzip.h"
 #include "dirvote.h"
+#include "log_test_helpers.h"
 
 #ifdef _WIN32
 /* For mkdir() */
@@ -53,6 +54,7 @@ ENABLE_GCC_WARNING(overlength-strings)
 #define NOT_FOUND "HTTP/1.0 404 Not found\r\n\r\n"
 #define BAD_REQUEST "HTTP/1.0 400 Bad request\r\n\r\n"
 #define SERVER_BUSY "HTTP/1.0 503 Directory busy, try again later\r\n\r\n"
+#define TOO_OLD "HTTP/1.0 404 Consensus is too old\r\n\r\n"
 #define NOT_ENOUGH_CONSENSUS_SIGNATURES "HTTP/1.0 404 " \
   "Consensus not signed by sufficient number of requested authorities\r\n\r\n"
 
@@ -1617,6 +1619,7 @@ test_dir_handle_get_status_vote_current_consensus_ns_not_enough_sigs(void* d)
   mock_ns_val = tor_malloc_zero(sizeof(networkstatus_t));
   mock_ns_val->flavor = FLAV_NS;
   mock_ns_val->voters = smartlist_new();
+  mock_ns_val->valid_until = time(NULL);
 
   /* init mock */
   init_mock_options();
@@ -1696,6 +1699,62 @@ test_dir_handle_get_status_vote_current_consensus_ns_not_found(void* data)
     or_options_free(mock_options); mock_options = NULL;
 }
 
+static void
+test_dir_handle_get_status_vote_current_consensus_too_old(void *data)
+{
+  dir_connection_t *conn = NULL;
+  char *header = NULL;
+  (void)data;
+
+  mock_ns_val = tor_malloc_zero(sizeof(networkstatus_t));
+  mock_ns_val->flavor = FLAV_MICRODESC;
+  mock_ns_val->valid_until = time(NULL) - (60 * 60 * 24) - 1;
+
+  init_mock_options();
+  MOCK(get_options, mock_get_options);
+  MOCK(connection_write_to_buf_impl_, connection_write_to_buf_mock);
+  MOCK(networkstatus_get_latest_consensus_by_flavor, mock_ns_get_by_flavor);
+
+  conn = new_dir_conn();
+
+  setup_capture_of_logs(LOG_WARN);
+
+  tt_int_op(0, OP_EQ, directory_handle_command_get(conn,
+    GET("/tor/status-vote/current/consensus-microdesc"), NULL, 0));
+
+  fetch_from_buf_http(TO_CONN(conn)->outbuf, &header, MAX_HEADERS_SIZE,
+                      NULL, NULL, 1, 0);
+  tt_assert(header);
+  tt_str_op(TOO_OLD, OP_EQ, header);
+
+  expect_log_msg_containing("too old");
+
+  tor_free(header);
+  teardown_capture_of_logs();
+
+  setup_capture_of_logs(LOG_WARN);
+
+  tt_int_op(0, OP_EQ, directory_handle_command_get(conn,
+    GET("/tor/status-vote/current/consensus"), NULL, 0));
+
+  fetch_from_buf_http(TO_CONN(conn)->outbuf, &header, MAX_HEADERS_SIZE,
+                      NULL, NULL, 1, 0);
+  tt_assert(header);
+  tt_str_op(TOO_OLD, OP_EQ, header);
+
+  expect_no_log_entry();
+
+  done:
+    teardown_capture_of_logs();
+    UNMOCK(networkstatus_get_latest_consensus_by_flavor);
+    UNMOCK(connection_write_to_buf_impl_);
+    UNMOCK(get_options);
+    connection_free_(TO_CONN(conn));
+    tor_free(header);
+    tor_free(mock_ns_val);
+    or_options_free(mock_options); mock_options = NULL;
+}
+
 NS_DECL(int, geoip_get_country_by_addr, (const tor_addr_t *addr));
 
 int
@@ -2481,6 +2540,7 @@ struct testcase_t dir_handle_get_tests[] = {
   DIR_HANDLE_CMD(status_vote_next_authority, 0),
   DIR_HANDLE_CMD(status_vote_current_consensus_ns_not_enough_sigs, 0),
   DIR_HANDLE_CMD(status_vote_current_consensus_ns_not_found, 0),
+  DIR_HANDLE_CMD(status_vote_current_consensus_too_old, 0),
   DIR_HANDLE_CMD(status_vote_current_consensus_ns_busy, 0),
   DIR_HANDLE_CMD(status_vote_current_consensus_ns, 0),
   DIR_HANDLE_CMD(status_vote_current_d_not_found, 0),
Last edited 9 months ago by rubiate (previous) (diff)

comment:12 Changed 9 months ago by teor

Status: merge_readyneeds_review

comment:13 Changed 9 months ago by nickm

Keywords: review-group-13 added

comment:14 Changed 9 months ago by teor

The bug that serves stale consensuses is still happening in 0.2.9.5-alpha and later, see #20909.
So we probably want this in 0.2.9.

comment:15 Changed 8 months ago by arma

Code looks good! I haven't looked at the unit test part. I recommend putting it in an 0.3.0 for a while before we consider a backport to 0.2.9. (And I hope that we'll choose to not backport it, since clients use 3 dirguards and we've survived this long with this bug in Tor.)

comment:16 Changed 8 months ago by nickm

LGTM. I'm going to merge this in 0.3.0, and not plan to backport.

comment:17 Changed 8 months ago by nickm

Keywords: 029-backport added; 029-proposed review-group-13 removed
Milestone: Tor: 0.3.0.x-finalTor: 0.2.9.x-final

(d46c1b49a459f1249ef358b3751ef656d9e19038 would be the thing to cherry-pick, if we _do_ backport.)

comment:18 Changed 8 months ago by nickm

Milestone: Tor: 0.2.9.x-finalTor: 0.3.0.x-final
Resolution: implemented
Status: needs_reviewclosed

Given no objections, not backporting.

Note: See TracTickets for help on using tickets.