From cdb528d841e753b6cc0b8d8b2aeb6bed183a1a8d Mon Sep 17 00:00:00 2001 From: "teor (Tim Wilson-Brown)" Date: Wed, 4 May 2016 16:47:28 +1000 Subject: [PATCH] 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;