From 6f9f1f3324d49ffbe0a5275a268111882ba8851f Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Thu, 19 Aug 2010 11:13:35 -0400 Subject: [PATCH 1/2] Make the "2 unknown, 7 missing key..." messages easier to understand This patch should fix the cases we care about for bugs 1290 and 1145. "30-56-99 are correct. Limited 4 and 8 are missing". --- changes/prettier-signature-log | 5 ++++ src/or/networkstatus.c | 48 +++++++++++++++++++++++++++------- 2 files changed, 44 insertions(+), 9 deletions(-) create mode 100644 changes/prettier-signature-log diff --git a/changes/prettier-signature-log b/changes/prettier-signature-log new file mode 100644 index 0000000000..cefaa38158 --- /dev/null +++ b/changes/prettier-signature-log @@ -0,0 +1,5 @@ + o Minor features + - Make the formerly ugly "2 unknown, 7 missing key, 0 good, 0 bad, + 2 no signature, 4 required" messages easier to read, and make sure + they get logged at the same severity as the messages explaining + which keys are which. diff --git a/src/or/networkstatus.c b/src/or/networkstatus.c index c0a3a28e4b..d7a8f70dcc 100644 --- a/src/or/networkstatus.c +++ b/src/or/networkstatus.c @@ -542,31 +542,61 @@ networkstatus_check_consensus_signature(networkstatus_t *consensus, if (warn >= 0) { SMARTLIST_FOREACH(unrecognized, networkstatus_voter_info_t *, voter, { - log_info(LD_DIR, "Consensus includes unrecognized authority '%s' " - "at %s:%d (contact %s; identity %s)", + log(severity, LD_DIR, "Consensus includes unrecognized authority " + "'%s' at %s:%d (contact %s; identity %s)", voter->nickname, voter->address, (int)voter->dir_port, voter->contact?voter->contact:"n/a", hex_str(voter->identity_digest, DIGEST_LEN)); }); SMARTLIST_FOREACH(need_certs_from, networkstatus_voter_info_t *, voter, { - log_info(LD_DIR, "Looks like we need to download a new certificate " - "from authority '%s' at %s:%d (contact %s; identity %s)", + log(severity, LD_DIR, "Looks like we need to download a new " + "certificate from authority '%s' at %s:%d (contact %s; " + "identity %s)", voter->nickname, voter->address, (int)voter->dir_port, voter->contact?voter->contact:"n/a", hex_str(voter->identity_digest, DIGEST_LEN)); }); SMARTLIST_FOREACH(missing_authorities, trusted_dir_server_t *, ds, { - log_info(LD_DIR, "Consensus does not include configured " + log(severity, LD_DIR, "Consensus does not include configured " "authority '%s' at %s:%d (identity %s)", ds->nickname, ds->address, (int)ds->dir_port, hex_str(ds->v3_identity_digest, DIGEST_LEN)); }); - log(severity, LD_DIR, - "%d unknown, %d missing key, %d good, %d bad, %d no signature, " - "%d required", n_unknown, n_missing_key, n_good, n_bad, - n_no_signature, n_required); + { + smartlist_t *sl = smartlist_create(); + char *cp; + tor_asprintf(&cp, "A consensus needs %d good signatures from recognized " + "authorities for us to accept it. This one has %d.", + n_required, n_good); + smartlist_add(sl,cp); + if (n_no_signature) { + tor_asprintf(&cp, "%d of the authorities we know didn't sign it.", + n_no_signature); + smartlist_add(sl,cp); + } + if (n_unknown) { + tor_asprintf(&cp, "It has %d signatures from authorities we don't " + "recognize.", n_unknown); + smartlist_add(sl,cp); + } + if (n_bad) { + tor_asprintf(&cp, "%d of the signatures on it didn't verify " + "correctly.", n_bad); + smartlist_add(sl,cp); + } + if (n_missing_key) { + tor_asprintf(&cp, "We were unable to check %d of the signatures, " + "because we were missing the keys.", n_missing_key); + smartlist_add(sl,cp); + } + cp = smartlist_join_strings(sl, " ", 0, NULL); + log(severity, LD_DIR, "%s", cp); + tor_free(cp); + SMARTLIST_FOREACH(sl, char *, c, tor_free(c)); + smartlist_free(sl); + } } smartlist_free(unrecognized); From bfa1962d8026e632e00760c1e14b39d154977adf Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Thu, 2 Sep 2010 16:42:18 -0400 Subject: [PATCH 2/2] Complicate the rules on WARN vs INFO in consensus verification It's normal when bootstrapping to have a lot of different certs missing, so we don't want missing certs to make us warn... unless the certs we're missing are ones that we've tried to fetch a couple of times and failed at. May fix bug 1145. --- changes/prettier-signature-log | 6 +++++- src/or/directory.c | 8 ++++++++ src/or/directory.h | 2 ++ src/or/networkstatus.c | 20 ++++++++++++++------ src/or/routerlist.c | 17 +++++++++++++++++ src/or/routerlist.h | 1 + 6 files changed, 47 insertions(+), 7 deletions(-) diff --git a/changes/prettier-signature-log b/changes/prettier-signature-log index cefaa38158..c008514fb9 100644 --- a/changes/prettier-signature-log +++ b/changes/prettier-signature-log @@ -2,4 +2,8 @@ - Make the formerly ugly "2 unknown, 7 missing key, 0 good, 0 bad, 2 no signature, 4 required" messages easier to read, and make sure they get logged at the same severity as the messages explaining - which keys are which. + which keys are which. Fixes bug 1290. + - Don't warn when we have a consensus that we can't verify because + of missing certificates, unless those certificates are ones + that we have been trying and failing to download. Fixes bug 1145. + diff --git a/src/or/directory.c b/src/or/directory.c index a3e575ac97..e46d08a7a5 100644 --- a/src/or/directory.c +++ b/src/or/directory.c @@ -3484,6 +3484,14 @@ download_status_reset(download_status_t *dls) dls->next_attempt_at = time(NULL) + schedule[0]; } +/** Return the number of failures on dls since the last success (if + * any). */ +int +download_status_get_n_failures(const download_status_t *dls) +{ + return dls->n_download_failures; +} + /** Called when one or more routerdesc (or extrainfo, if was_extrainfo) * fetches have failed (with uppercase fingerprints listed in failed, * either as descriptor digests or as identity digests based on diff --git a/src/or/directory.h b/src/or/directory.h index 36b4cf2b18..6fd2c0beff 100644 --- a/src/or/directory.h +++ b/src/or/directory.h @@ -104,5 +104,7 @@ download_status_mark_impossible(download_status_t *dl) dl->n_download_failures = IMPOSSIBLE_TO_DOWNLOAD; } +int download_status_get_n_failures(const download_status_t *dls); + #endif diff --git a/src/or/networkstatus.c b/src/or/networkstatus.c index d7a8f70dcc..1b42918525 100644 --- a/src/or/networkstatus.c +++ b/src/or/networkstatus.c @@ -464,7 +464,7 @@ networkstatus_check_consensus_signature(networkstatus_t *consensus, int warn) { int n_good = 0; - int n_missing_key = 0; + int n_missing_key = 0, n_dl_failed_key = 0; int n_bad = 0; int n_unknown = 0; int n_no_signature = 0; @@ -482,7 +482,7 @@ networkstatus_check_consensus_signature(networkstatus_t *consensus, voter) { int good_here = 0; int bad_here = 0; - int missing_key_here = 0; + int missing_key_here = 0, dl_failed_key_here = 0; SMARTLIST_FOREACH_BEGIN(voter->sigs, document_signature_t *, sig) { if (!sig->good_signature && !sig->bad_signature && sig->signature) { @@ -502,11 +502,15 @@ networkstatus_check_consensus_signature(networkstatus_t *consensus, } else if (!cert || cert->expires < now) { smartlist_add(need_certs_from, voter); ++missing_key_here; + if (authority_cert_dl_looks_uncertain(sig->identity_digest)) + ++dl_failed_key_here; continue; } if (networkstatus_check_document_signature(consensus, sig, cert) < 0) { smartlist_add(need_certs_from, voter); ++missing_key_here; + if (authority_cert_dl_looks_uncertain(sig->identity_digest)) + ++dl_failed_key_here; continue; } } @@ -519,9 +523,11 @@ networkstatus_check_consensus_signature(networkstatus_t *consensus, ++n_good; else if (bad_here) ++n_bad; - else if (missing_key_here) + else if (missing_key_here) { ++n_missing_key; - else + if (dl_failed_key_here) + ++n_dl_failed_key; + } else ++n_no_signature; } SMARTLIST_FOREACH_END(voter); @@ -534,10 +540,12 @@ networkstatus_check_consensus_signature(networkstatus_t *consensus, smartlist_add(missing_authorities, ds); }); - if (warn > 1 || (warn >= 0 && n_good < n_required)) + if (warn > 1 || (warn >= 0 && + (n_good + n_missing_key - n_dl_failed_key < n_required))) { severity = LOG_WARN; - else + } else { severity = LOG_INFO; + } if (warn >= 0) { SMARTLIST_FOREACH(unrecognized, networkstatus_voter_info_t *, voter, diff --git a/src/or/routerlist.c b/src/or/routerlist.c index 8808f56db9..5bdc973b2c 100644 --- a/src/or/routerlist.c +++ b/src/or/routerlist.c @@ -440,6 +440,23 @@ authority_cert_dl_failed(const char *id_digest, int status) download_status_failed(&cl->dl_status, status); } +/** Return true iff when we've been getting enough failures when trying to + * download the certificate with ID digest id_digest that we're willing + * to start bugging the user about it. */ +int +authority_cert_dl_looks_uncertain(const char *id_digest) +{ +#define N_AUTH_CERT_DL_FAILURES_TO_BUG_USER 2 + cert_list_t *cl; + int n_failures; + if (!trusted_dir_certs || + !(cl = digestmap_get(trusted_dir_certs, id_digest))) + return 0; + + n_failures = download_status_get_n_failures(&cl->dl_status); + return n_failures >= N_AUTH_CERT_DL_FAILURES_TO_BUG_USER; +} + /** How many times will we try to fetch a certificate before giving up? */ #define MAX_CERT_DL_FAILURES 8 diff --git a/src/or/routerlist.h b/src/or/routerlist.h index e31b07aef5..e3e9ddd778 100644 --- a/src/or/routerlist.h +++ b/src/or/routerlist.h @@ -24,6 +24,7 @@ void authority_cert_get_all(smartlist_t *certs_out); void authority_cert_dl_failed(const char *id_digest, int status); void authority_certs_fetch_missing(networkstatus_t *status, time_t now); int router_reload_router_list(void); +int authority_cert_dl_looks_uncertain(const char *id_digest); smartlist_t *router_get_trusted_dir_servers(void); routerstatus_t *router_pick_directory_server(authority_type_t type, int flags);