Commit bfa1962d authored by Nick Mathewson's avatar Nick Mathewson 🤹
Browse files

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.
parent 6f9f1f33

changes/prettier-signature-log

+5 −1
Original line number Diff line number Diff line
@@ -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.

src/or/directory.c

+8 −0
Original line number Diff line number Diff line
@@ -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 <b>dls</b> 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 <b>was_extrainfo</b>)

 * fetches have failed (with uppercase fingerprints listed in <b>failed</b>,

 * either as descriptor digests or as identity digests based on

src/or/directory.h

+2 −0
Original line number Diff line number Diff line
@@ -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

src/or/networkstatus.c

+14 −6
Original line number Diff line number Diff line
@@ -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,

src/or/routerlist.c

+17 −0
Original line number Diff line number Diff line
@@ -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 <b>id_digest</b> 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