From 730cfeb6bd3d8060e44026469ec6d75695c00179 Mon Sep 17 00:00:00 2001 From: "teor (Tim Wilson-Brown)" Date: Wed, 4 May 2016 16:38:27 +1000 Subject: Fetch certificates from the same directory as the consensus Resolves ticket 18963; fix on #4483 in 0.2.8.1-alpha. --- changes/bug18963 | 4 +++ src/or/directory.c | 3 +- src/or/dirvote.c | 2 +- src/or/networkstatus.c | 25 +++++++++------ src/or/networkstatus.h | 3 +- src/or/routerlist.c | 86 +++++++++++++++++++++++++++++++++++++++++++------- src/or/routerlist.h | 3 +- 7 files changed, 102 insertions(+), 24 deletions(-) create mode 100644 changes/bug18963 diff --git a/changes/bug18963 b/changes/bug18963 new file mode 100644 index 0000000000..115ea01eba --- /dev/null +++ b/changes/bug18963 @@ -0,0 +1,4 @@ + o Minor bugfix (bootstrap): + - Remember the directory we fetched the consensus from, and use + it to fetch authority certificates as well. + 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 8dc018a662..4eeb3eae8c 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.", diff --git a/src/or/dirvote.c b/src/or/dirvote.c index 9854af7d7f..f28bff92db 100644 --- a/src/or/dirvote.c +++ b/src/or/dirvote.c @@ -3373,7 +3373,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 3967f56edd..cca45539aa 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); } @@ -1200,13 +1201,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 @@ -1504,6 +1505,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 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. * @@ -1513,7 +1518,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; @@ -1635,7 +1641,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; @@ -1673,7 +1679,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); @@ -1810,7 +1816,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, + NULL)) { tor_free(waiting_body); } } diff --git a/src/or/networkstatus.h b/src/or/networkstatus.h index 9bbb9a389e..03b041e5d6 100644 --- a/src/or/networkstatus.h +++ b/src/or/networkstatus.h @@ -85,7 +85,8 @@ 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); + unsigned flags, + const char *source_dir); void networkstatus_note_certs_arrived(void); void routers_update_all_from_networkstatus(time_t now, int dir_version); void routers_update_status_from_consensus_networkstatus(smartlist_t *routers, diff --git a/src/or/routerlist.c b/src/or/routerlist.c index d40d704a1d..0f722d99dc 100644 --- a/src/or/routerlist.c +++ b/src/or/routerlist.c @@ -718,9 +718,14 @@ authority_cert_dl_looks_uncertain(const char *id_digest) * status. 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 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 +889,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 +949,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 +1010,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 bc48c2087c..534dbca5f9 100644 --- a/src/or/routerlist.h +++ b/src/or/routerlist.h @@ -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); -- cgit v1.2.3-54-g00ecf From cdb528d841e753b6cc0b8d8b2aeb6bed183a1a8d Mon Sep 17 00:00:00 2001 From: "teor (Tim Wilson-Brown)" Date: Wed, 4 May 2016 16:47:28 +1000 Subject: Fetch certificates from the same directory as previous certificates Improves the fix to #18963. --- changes/bug18963 | 5 +++-- src/or/directory.c | 3 ++- src/or/dirvote.c | 3 ++- src/or/networkstatus.c | 15 ++++++++++----- src/or/networkstatus.h | 2 +- src/or/router.c | 3 ++- src/or/routerlist.c | 25 +++++++++++++++++++------ src/or/routerlist.h | 2 +- src/test/test_dir_handle_get.c | 14 +++++++------- 9 files changed, 47 insertions(+), 25 deletions(-) diff --git a/changes/bug18963 b/changes/bug18963 index 115ea01eba..f122288b1d 100644 --- a/changes/bug18963 +++ b/changes/bug18963 @@ -1,4 +1,5 @@ o Minor bugfix (bootstrap): - - Remember the directory we fetched the consensus from, and use - it to fetch authority certificates as well. + - 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 4eeb3eae8c..373318f7e5 100644 --- a/src/or/directory.c +++ b/src/or/directory.c @@ -2063,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 f28bff92db..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."); diff --git a/src/or/networkstatus.c b/src/or/networkstatus.c index cca45539aa..a3d11f4421 100644 --- a/src/or/networkstatus.c +++ b/src/or/networkstatus.c @@ -1506,8 +1506,8 @@ networkstatus_set_current_consensus_from_ns(networkstatus_t *c, * 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 from, so try it first to - * fetch any missing certificates. + * 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. @@ -1802,9 +1802,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; icache_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 0f722d99dc..bba7d60f40 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 flush 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; } @@ -720,8 +733,8 @@ authority_cert_dl_looks_uncertain(const char *id_digest) * already have. * * If dir_hint is non-NULL, it's the identity digest for a directory that - * we've just successfully retrieved a consensus from, so try it first to - * fetch any missing certificates. + * 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, diff --git a/src/or/routerlist.h b/src/or/routerlist.h index 534dbca5f9..d7af8c807a 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); 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; -- cgit v1.2.3-54-g00ecf