Merge remote-tracking branch 'teor/bug18963-remember-v2'

9 files changed, 142 insertions, 42 deletions

diff --git a/changes/bug18963 b/changes/bug18963
new file mode 100644
index 0000000000..f122288b1d
--- /dev/null
+++ b/changes/bug18963
@@ -0,0 +1,5 @@
+  o Minor bugfix (bootstrap):
+    - Remember the directory we fetched the consensus or previous
+      certificates from, and use it to fetch future authority
+      certificates.
+      Resolves ticket 18963; fix on #4483 in 0.2.8.1-alpha.
diff --git a/src/or/directory.c b/src/or/directory.c
index a3ade8f164..c9c07f336d 100644
--- a/src/or/directory.c
+++ b/src/or/directory.c
@@ -2019,7 +2019,8 @@ connection_dir_client_reached_eof(dir_connection_t *conn)
     }
     log_info(LD_DIR,"Received consensus directory (size %d) from server "
              "'%s:%d'", (int)body_len, conn->base_.address, conn->base_.port);
-    if ((r=networkstatus_set_current_consensus(body, flavname, 0))<0) {
+    if ((r=networkstatus_set_current_consensus(body, flavname, 0,
+                                               conn->identity_digest))<0) {
       log_fn(r<-1?LOG_WARN:LOG_INFO, LD_DIR,
              "Unable to load %s consensus directory downloaded from "
              "server '%s:%d'. I'll try again soon.",
@@ -2062,7 +2063,8 @@ connection_dir_client_reached_eof(dir_connection_t *conn)
     }
 
     if (src_code != -1) {
-      if (trusted_dirs_load_certs_from_string(body, src_code, 1)<0) {
+      if (trusted_dirs_load_certs_from_string(body, src_code, 1,
+                                              conn->identity_digest)<0) {
         log_warn(LD_DIR, "Unable to parse fetched certificates");
         /* if we fetched more than one and only some failed, the successful
          * ones got flushed to disk so it's safe to call this on them */
diff --git a/src/or/dirvote.c b/src/or/dirvote.c
index 9854af7d7f..4c64cc9c39 100644
--- a/src/or/dirvote.c
+++ b/src/or/dirvote.c
@@ -2916,7 +2916,8 @@ dirvote_add_vote(const char *vote_body, const char **msg_out, int *status_out)
     /* Hey, it's a new cert! */
     trusted_dirs_load_certs_from_string(
                                vote->cert->cache_info.signed_descriptor_body,
-                               TRUSTED_DIRS_CERTS_SRC_FROM_VOTE, 1 /*flush*/);
+                               TRUSTED_DIRS_CERTS_SRC_FROM_VOTE, 1 /*flush*/,
+                               NULL);
     if (!authority_cert_get_by_digests(vote->cert->cache_info.identity_digest,
                                        vote->cert->signing_key_digest)) {
       log_warn(LD_BUG, "We added a cert, but still couldn't find it.");
@@ -3373,7 +3374,7 @@ dirvote_publish_consensus(void)
       continue;
     }
 
-    if (networkstatus_set_current_consensus(pending->body, name, 0))
+    if (networkstatus_set_current_consensus(pending->body, name, 0, NULL))
       log_warn(LD_DIR, "Error publishing %s consensus", name);
     else
       log_notice(LD_DIR, "Published %s consensus", name);
diff --git a/src/or/networkstatus.c b/src/or/networkstatus.c
index 2975e7ebb4..1c6afe49b9 100644
--- a/src/or/networkstatus.c
+++ b/src/or/networkstatus.c
@@ -174,7 +174,7 @@ router_reload_consensus_networkstatus(void)
     }
     s = read_file_to_str(filename, RFTS_IGNORE_MISSING, NULL);
     if (s) {
-      if (networkstatus_set_current_consensus(s, flavor, flags) < -1) {
+      if (networkstatus_set_current_consensus(s, flavor, flags, NULL) < -1) {
         log_warn(LD_FS, "Couldn't load consensus %s networkstatus from \"%s\"",
                  flavor, filename);
       }
@@ -192,7 +192,8 @@ router_reload_consensus_networkstatus(void)
     s = read_file_to_str(filename, RFTS_IGNORE_MISSING, NULL);
     if (s) {
       if (networkstatus_set_current_consensus(s, flavor,
-                                     flags|NSSET_WAS_WAITING_FOR_CERTS)) {
+                                     flags|NSSET_WAS_WAITING_FOR_CERTS,
+                                     NULL)) {
       log_info(LD_FS, "Couldn't load consensus %s networkstatus from \"%s\"",
                flavor, filename);
     }
@@ -1201,13 +1202,13 @@ update_certificate_downloads(time_t now)
   for (i = 0; i < N_CONSENSUS_FLAVORS; ++i) {
     if (consensus_waiting_for_certs[i].consensus)
       authority_certs_fetch_missing(consensus_waiting_for_certs[i].consensus,
-                                    now);
+                                    now, NULL);
   }
 
   if (current_ns_consensus)
-    authority_certs_fetch_missing(current_ns_consensus, now);
+    authority_certs_fetch_missing(current_ns_consensus, now, NULL);
   if (current_md_consensus)
-    authority_certs_fetch_missing(current_md_consensus, now);
+    authority_certs_fetch_missing(current_md_consensus, now, NULL);
 }
 
 /** Return 1 if we have a consensus but we don't have enough certificates
@@ -1505,6 +1506,10 @@ networkstatus_set_current_consensus_from_ns(networkstatus_t *c,
  * If flags & NSSET_ACCEPT_OBSOLETE, then we should be willing to take this
  * consensus, even if it comes from many days in the past.
  *
+ * If source_dir is non-NULL, it's the identity digest for a directory that
+ * we've just successfully retrieved a consensus or certificates from, so try
+ * it first to fetch any missing certificates.
+ *
  * Return 0 on success, <0 on failure.  On failure, caller should increment
  * the failure count as appropriate.
  *
@@ -1514,7 +1519,8 @@ networkstatus_set_current_consensus_from_ns(networkstatus_t *c,
 int
 networkstatus_set_current_consensus(const char *consensus,
                                     const char *flavor,
-                                    unsigned flags)
+                                    unsigned flags,
+                                    const char *source_dir)
 {
   networkstatus_t *c=NULL;
   int r, result = -1;
@@ -1636,7 +1642,7 @@ networkstatus_set_current_consensus(const char *consensus,
           write_str_to_file(unverified_fname, consensus, 0);
         }
         if (dl_certs)
-          authority_certs_fetch_missing(c, now);
+          authority_certs_fetch_missing(c, now, source_dir);
         /* This case is not a success or a failure until we get the certs
          * or fail to get the certs. */
         result = 0;
@@ -1674,7 +1680,7 @@ networkstatus_set_current_consensus(const char *consensus,
 
   /* Are we missing any certificates at all? */
   if (r != 1 && dl_certs)
-    authority_certs_fetch_missing(c, now);
+    authority_certs_fetch_missing(c, now, source_dir);
 
   if (flav == usable_consensus_flavor()) {
     notify_control_networkstatus_changed(current_consensus, c);
@@ -1797,9 +1803,14 @@ networkstatus_set_current_consensus(const char *consensus,
 }
 
 /** Called when we have gotten more certificates: see whether we can
- * now verify a pending consensus. */
+ * now verify a pending consensus.
+ *
+ * If source_dir is non-NULL, it's the identity digest for a directory that
+ * we've just successfully retrieved certificates from, so try it first to
+ * fetch any missing certificates.
+ */
 void
-networkstatus_note_certs_arrived(void)
+networkstatus_note_certs_arrived(const char *source_dir)
 {
   int i;
   for (i=0; i<N_CONSENSUS_FLAVORS; ++i) {
@@ -1811,7 +1822,8 @@ networkstatus_note_certs_arrived(void)
       if (!networkstatus_set_current_consensus(
                                  waiting_body,
                                  networkstatus_get_flavor_name(i),
-                                 NSSET_WAS_WAITING_FOR_CERTS)) {
+                                 NSSET_WAS_WAITING_FOR_CERTS,
+                                 source_dir)) {
         tor_free(waiting_body);
       }
     }
diff --git a/src/or/networkstatus.h b/src/or/networkstatus.h
index f2f8af5c6b..86dda020fe 100644
--- a/src/or/networkstatus.h
+++ b/src/or/networkstatus.h
@@ -85,8 +85,9 @@ int networkstatus_consensus_is_downloading_usable_flavor(void);
 #define NSSET_REQUIRE_FLAVOR 16
 int networkstatus_set_current_consensus(const char *consensus,
                                         const char *flavor,
-                                        unsigned flags);
-void networkstatus_note_certs_arrived(void);
+                                        unsigned flags,
+                                        const char *source_dir);
+void networkstatus_note_certs_arrived(const char *source_dir);
 void routers_update_all_from_networkstatus(time_t now, int dir_version);
 void routers_update_status_from_consensus_networkstatus(smartlist_t *routers,
                                                         int reset_failures);
diff --git a/src/or/router.c b/src/or/router.c
index d70756fdbf..c30c1dd4d5 100644
--- a/src/or/router.c
+++ b/src/or/router.c
@@ -1054,7 +1054,8 @@ init_keys(void)
     log_info(LD_DIR, "adding my own v3 cert");
     if (trusted_dirs_load_certs_from_string(
                       cert->cache_info.signed_descriptor_body,
-                      TRUSTED_DIRS_CERTS_SRC_SELF, 0)<0) {
+                      TRUSTED_DIRS_CERTS_SRC_SELF, 0,
+                      NULL)<0) {
       log_warn(LD_DIR, "Unable to parse my own v3 cert! Failing.");
       return -1;
     }
diff --git a/src/or/routerlist.c b/src/or/routerlist.c
index dcf80a3aa8..5b32148cf1 100644
--- a/src/or/routerlist.c
+++ b/src/or/routerlist.c
@@ -287,7 +287,7 @@ trusted_dirs_reload_certs(void)
     return 0;
   r = trusted_dirs_load_certs_from_string(
         contents,
-        TRUSTED_DIRS_CERTS_SRC_FROM_STORE, 1);
+        TRUSTED_DIRS_CERTS_SRC_FROM_STORE, 1, NULL);
   tor_free(contents);
   return r;
 }
@@ -317,16 +317,21 @@ already_have_cert(authority_cert_t *cert)
  * or TRUSTED_DIRS_CERTS_SRC_DL_BY_ID_SK_DIGEST.  If <b>flush</b> is true, we
  * need to flush any changed certificates to disk now.  Return 0 on success,
  * -1 if any certs fail to parse.
+ *
+ * If source_dir is non-NULL, it's the identity digest for a directory that
+ * we've just successfully retrieved certificates from, so try it first to
+ * fetch any missing certificates.
  */
 
 int
 trusted_dirs_load_certs_from_string(const char *contents, int source,
-                                    int flush)
+                                    int flush, const char *source_dir)
 {
   dir_server_t *ds;
   const char *s, *eos;
   int failure_code = 0;
   int from_store = (source == TRUSTED_DIRS_CERTS_SRC_FROM_STORE);
+  int added_trusted_cert = 0;
 
   for (s = contents; *s; s = eos) {
     authority_cert_t *cert = authority_cert_parse_from_string(s, &eos);
@@ -386,6 +391,7 @@ trusted_dirs_load_certs_from_string(const char *contents, int source,
     }
 
     if (ds) {
+      added_trusted_cert = 1;
       log_info(LD_DIR, "Adding %s certificate for directory authority %s with "
                "signing key %s", from_store ? "cached" : "downloaded",
                ds->nickname, hex_str(cert->signing_key_digest,DIGEST_LEN));
@@ -430,8 +436,15 @@ trusted_dirs_load_certs_from_string(const char *contents, int source,
     trusted_dirs_flush_certs_to_disk();
 
   /* call this even if failure_code is <0, since some certs might have
-   * succeeded. */
-  networkstatus_note_certs_arrived();
+   * succeeded, but only pass source_dir if there were no failures,
+   * and at least one more authority certificate was added to the store.
+   * This avoids retrying a directory that's serving bad or entirely duplicate
+   * certificates. */
+  if (failure_code == 0 && added_trusted_cert) {
+    networkstatus_note_certs_arrived(source_dir);
+  } else {
+    networkstatus_note_certs_arrived(NULL);
+  }
 
   return failure_code;
 }
@@ -718,9 +731,14 @@ authority_cert_dl_looks_uncertain(const char *id_digest)
  * <b>status</b>.  Additionally, try to have a non-expired certificate for
  * every V3 authority in trusted_dir_servers.  Don't fetch certificates we
  * already have.
+ *
+ * If dir_hint is non-NULL, it's the identity digest for a directory that
+ * we've just successfully retrieved a consensus or certificates from, so try
+ * it first to fetch any missing certificates.
  **/
 void
-authority_certs_fetch_missing(networkstatus_t *status, time_t now)
+authority_certs_fetch_missing(networkstatus_t *status, time_t now,
+                              const char *dir_hint)
 {
   /*
    * The pending_id digestmap tracks pending certificate downloads by
@@ -884,6 +902,37 @@ authority_certs_fetch_missing(networkstatus_t *status, time_t now)
     } SMARTLIST_FOREACH_END(voter);
   }
 
+  /* Look up the routerstatus for the dir_hint  */
+  const routerstatus_t *rs = NULL;
+
+  /* If we still need certificates, try the directory that just successfully
+   * served us a consensus or certificates.
+   * As soon as the directory fails to provide additional certificates, we try
+   * another, randomly selected directory. This avoids continual retries.
+   * (We only ever have one outstanding request per certificate.)
+   *
+   * Bridge clients won't find their bridges using this hint, so they will
+   * fall back to using directory_get_from_dirserver, which selects a bridge.
+   */
+  if (dir_hint) {
+    /* First try the consensus routerstatus, then the fallback
+     * routerstatus */
+    rs = router_get_consensus_status_by_id(dir_hint);
+    if (!rs) {
+      /* This will also find authorities */
+      const dir_server_t *ds = router_get_fallback_dirserver_by_digest(
+                                                                    dir_hint);
+      if (ds) {
+        rs = &ds->fake_status;
+      }
+    }
+
+    if (!rs) {
+      log_warn(LD_BUG, "Directory %s delivered a consensus, but a "
+               "routerstatus could not be found for it.", dir_hint);
+    }
+  }
+
   /* Do downloads by identity digest */
   if (smartlist_len(missing_id_digests) > 0) {
     int need_plus = 0;
@@ -913,11 +962,25 @@ authority_certs_fetch_missing(networkstatus_t *status, time_t now)
 
     if (smartlist_len(fps) > 1) {
       resource = smartlist_join_strings(fps, "", 0, NULL);
-      /* We want certs from mirrors, because they will almost always succeed.
-       */
-      directory_get_from_dirserver(DIR_PURPOSE_FETCH_CERTIFICATE, 0,
-                                   resource, PDS_RETRY_IF_NO_SERVERS,
-                                   DL_WANT_ANY_DIRSERVER);
+
+      /* If we've just downloaded a consensus from a directory, re-use that
+       * directory */
+      if (rs) {
+        /* Certificate fetches are one-hop, unless AllDirActionsPrivate is 1 */
+        int get_via_tor = get_options()->AllDirActionsPrivate;
+        const dir_indirection_t indirection = get_via_tor ? DIRIND_ANONYMOUS
+                                                          : DIRIND_ONEHOP;
+        directory_initiate_command_routerstatus(rs,
+                                                DIR_PURPOSE_FETCH_CERTIFICATE,
+                                                0, indirection, resource, NULL,
+                                                0, 0);
+      } else {
+        /* Otherwise, we want certs from a random fallback or directory
+         * mirror, because they will almost always succeed. */
+        directory_get_from_dirserver(DIR_PURPOSE_FETCH_CERTIFICATE, 0,
+                                     resource, PDS_RETRY_IF_NO_SERVERS,
+                                     DL_WANT_ANY_DIRSERVER);
+      }
       tor_free(resource);
     }
     /* else we didn't add any: they were all pending */
@@ -960,11 +1023,25 @@ authority_certs_fetch_missing(networkstatus_t *status, time_t now)
 
     if (smartlist_len(fp_pairs) > 1) {
       resource = smartlist_join_strings(fp_pairs, "", 0, NULL);
-      /* We want certs from mirrors, because they will almost always succeed.
-       */
-      directory_get_from_dirserver(DIR_PURPOSE_FETCH_CERTIFICATE, 0,
-                                   resource, PDS_RETRY_IF_NO_SERVERS,
-                                   DL_WANT_ANY_DIRSERVER);
+
+      /* If we've just downloaded a consensus from a directory, re-use that
+       * directory */
+      if (rs) {
+        /* Certificate fetches are one-hop, unless AllDirActionsPrivate is 1 */
+        int get_via_tor = get_options()->AllDirActionsPrivate;
+        const dir_indirection_t indirection = get_via_tor ? DIRIND_ANONYMOUS
+                                                          : DIRIND_ONEHOP;
+        directory_initiate_command_routerstatus(rs,
+                                                DIR_PURPOSE_FETCH_CERTIFICATE,
+                                                0, indirection, resource, NULL,
+                                                0, 0);
+      } else {
+        /* Otherwise, we want certs from a random fallback or directory
+         * mirror, because they will almost always succeed. */
+        directory_get_from_dirserver(DIR_PURPOSE_FETCH_CERTIFICATE, 0,
+                                     resource, PDS_RETRY_IF_NO_SERVERS,
+                                     DL_WANT_ANY_DIRSERVER);
+      }
       tor_free(resource);
     }
     /* else they were all pending */
diff --git a/src/or/routerlist.h b/src/or/routerlist.h
index 2166cf5114..bd0fa0292f 100644
--- a/src/or/routerlist.h
+++ b/src/or/routerlist.h
@@ -29,7 +29,7 @@ int trusted_dirs_reload_certs(void);
 #define TRUSTED_DIRS_CERTS_SRC_FROM_VOTE 4
 
 int trusted_dirs_load_certs_from_string(const char *contents, int source,
-                                        int flush);
+                                        int flush, const char *source_dir);
 void trusted_dirs_flush_certs_to_disk(void);
 authority_cert_t *authority_cert_get_newest_by_id(const char *id_digest);
 authority_cert_t *authority_cert_get_by_sk_digest(const char *sk_digest);
@@ -38,7 +38,8 @@ authority_cert_t *authority_cert_get_by_digests(const char *id_digest,
 void authority_cert_get_all(smartlist_t *certs_out);
 void authority_cert_dl_failed(const char *id_digest,
                               const char *signing_key_digest, int status);
-void authority_certs_fetch_missing(networkstatus_t *status, time_t now);
+void authority_certs_fetch_missing(networkstatus_t *status, time_t now,
+                                   const char *dir_hint);
 int router_reload_router_list(void);
 int authority_cert_dl_looks_uncertain(const char *id_digest);
 const smartlist_t *router_get_trusted_dir_servers(void);
diff --git a/src/test/test_dir_handle_get.c b/src/test/test_dir_handle_get.c
index 05657ca452..1416b389aa 100644
--- a/src/test/test_dir_handle_get.c
+++ b/src/test/test_dir_handle_get.c
@@ -1237,7 +1237,7 @@ test_dir_handle_get_server_keys_all(void* data)
   base16_decode(ds->v3_identity_digest, DIGEST_LEN,
                 TEST_CERT_IDENT_KEY, HEX_DIGEST_LEN);
   tt_int_op(0, OP_EQ, trusted_dirs_load_certs_from_string(TEST_CERTIFICATE,
-    TRUSTED_DIRS_CERTS_SRC_DL_BY_ID_DIGEST, 1));
+    TRUSTED_DIRS_CERTS_SRC_DL_BY_ID_DIGEST, 1, NULL));
 
   conn = dir_connection_new(tor_addr_family(&MOCK_TOR_ADDR));
 
@@ -1396,7 +1396,7 @@ test_dir_handle_get_server_keys_fp(void* data)
                 TEST_CERT_IDENT_KEY, HEX_DIGEST_LEN);
 
   tt_int_op(0, OP_EQ, trusted_dirs_load_certs_from_string(TEST_CERTIFICATE,
-    TRUSTED_DIRS_CERTS_SRC_DL_BY_ID_DIGEST, 1));
+    TRUSTED_DIRS_CERTS_SRC_DL_BY_ID_DIGEST, 1, NULL));
 
   conn = dir_connection_new(tor_addr_family(&MOCK_TOR_ADDR));
   char req[71];
@@ -1468,7 +1468,7 @@ test_dir_handle_get_server_keys_sk(void* data)
   routerlist_free_all();
 
   tt_int_op(0, OP_EQ, trusted_dirs_load_certs_from_string(TEST_CERTIFICATE,
-    TRUSTED_DIRS_CERTS_SRC_DL_BY_ID_DIGEST, 1));
+    TRUSTED_DIRS_CERTS_SRC_DL_BY_ID_DIGEST, 1, NULL));
 
   conn = dir_connection_new(tor_addr_family(&MOCK_TOR_ADDR));
   char req[71];
@@ -1550,7 +1550,7 @@ test_dir_handle_get_server_keys_fpsk(void* data)
   dir_server_add(ds);
 
   tt_int_op(0, OP_EQ, trusted_dirs_load_certs_from_string(TEST_CERTIFICATE,
-    TRUSTED_DIRS_CERTS_SRC_DL_BY_ID_DIGEST, 1));
+    TRUSTED_DIRS_CERTS_SRC_DL_BY_ID_DIGEST, 1, NULL));
 
   conn = dir_connection_new(tor_addr_family(&MOCK_TOR_ADDR));
 
@@ -1606,7 +1606,7 @@ test_dir_handle_get_server_keys_busy(void* data)
   dir_server_add(ds);
 
   tt_int_op(0, OP_EQ, trusted_dirs_load_certs_from_string(TEST_CERTIFICATE,
-    TRUSTED_DIRS_CERTS_SRC_DL_BY_ID_DIGEST, 1));
+    TRUSTED_DIRS_CERTS_SRC_DL_BY_ID_DIGEST, 1, NULL));
 
   MOCK(get_options, mock_get_options);
   MOCK(connection_write_to_buf_impl_, connection_write_to_buf_mock);
@@ -2344,7 +2344,7 @@ test_dir_handle_get_status_vote_next_authority(void* data)
   base16_decode(ds->v3_identity_digest, DIGEST_LEN,
                 TEST_CERT_IDENT_KEY, HEX_DIGEST_LEN);
   tt_int_op(0, OP_EQ, trusted_dirs_load_certs_from_string(TEST_CERTIFICATE,
-    TRUSTED_DIRS_CERTS_SRC_DL_BY_ID_DIGEST, 1));
+    TRUSTED_DIRS_CERTS_SRC_DL_BY_ID_DIGEST, 1, NULL));
 
   init_mock_options();
   mock_options->AuthoritativeDir = 1;
@@ -2423,7 +2423,7 @@ test_dir_handle_get_status_vote_current_authority(void* data)
                 TEST_CERT_IDENT_KEY, HEX_DIGEST_LEN);
 
   tt_int_op(0, OP_EQ, trusted_dirs_load_certs_from_string(TEST_CERTIFICATE,
-    TRUSTED_DIRS_CERTS_SRC_DL_BY_ID_DIGEST, 1));
+    TRUSTED_DIRS_CERTS_SRC_DL_BY_ID_DIGEST, 1, NULL));
 
   init_mock_options();
   mock_options->AuthoritativeDir = 1;