From 60efce445b17d4b4153e91527887873812f5599f Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 22 Feb 2016 10:07:42 -0500 Subject: Enable ed25519 collator in voting. Previously, I had left in some debugging code with /*XXX*/ after it, which nobody noticed. Live and learn! Next time I will use /*XXX DO NOT COMMIT*/ or something. We need to define a new consensus method for this; consensus method 21 shouldn't actually be used. Fixes bug 17702; bugfix on 0.2.7.2-alpha. --- changes/bug17702 | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 changes/bug17702 (limited to 'changes') diff --git a/changes/bug17702 b/changes/bug17702 new file mode 100644 index 0000000000..4fda36f736 --- /dev/null +++ b/changes/bug17702 @@ -0,0 +1,6 @@ + o Major bugfixes: + - Actually enable Ed25519-based directory collation. + Previously, the code had been written, but some debugging code that had + accidentally been left in the codebase made it stay turned off. + Fixes bug 17702; bugfix on 0.2.7.2-alpha. + -- cgit v1.2.3-54-g00ecf From c20e34e1894bed07982fe64d60a1b3fe9403d269 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 22 Feb 2016 10:59:27 -0500 Subject: Fix log message subjects in networkstatus_parse_vote_from_string() Some of these messages called the thing being parsed a "vote" whether it is a vote or a consensus. Fixes bug 18368. --- changes/bug18368 | 5 +++++ src/or/routerparse.c | 25 ++++++++++++------------- 2 files changed, 17 insertions(+), 13 deletions(-) create mode 100644 changes/bug18368 (limited to 'changes') diff --git a/changes/bug18368 b/changes/bug18368 new file mode 100644 index 0000000000..17218d432f --- /dev/null +++ b/changes/bug18368 @@ -0,0 +1,5 @@ + o Minor bugfixes: + - When logging information about an unparseable networkstatus vote or + consensus, do not say "vote" when we mean consensus. Fixes bug + 18368; bugfix on 0.2.0.8-alpha. + diff --git a/src/or/routerparse.c b/src/or/routerparse.c index f898ef8aef..6b6e21d5d0 100644 --- a/src/or/routerparse.c +++ b/src/or/routerparse.c @@ -2862,7 +2862,7 @@ networkstatus_parse_vote_from_string(const char *s, const char **eos_out, (ns_type == NS_TYPE_CONSENSUS) ? networkstatus_consensus_token_table : networkstatus_token_table, 0)) { - log_warn(LD_DIR, "Error tokenizing network-status vote header"); + log_warn(LD_DIR, "Error tokenizing network-status header"); goto err; } @@ -3085,7 +3085,7 @@ networkstatus_parse_vote_from_string(const char *s, const char **eos_out, base16_decode(voter->identity_digest, sizeof(voter->identity_digest), tok->args[1], HEX_DIGEST_LEN) < 0) { log_warn(LD_DIR, "Error decoding identity digest %s in " - "network-status vote.", escaped(tok->args[1])); + "network-status document.", escaped(tok->args[1])); goto err; } if (ns->type != NS_TYPE_CONSENSUS && @@ -3144,7 +3144,7 @@ networkstatus_parse_vote_from_string(const char *s, const char **eos_out, voter = NULL; } if (smartlist_len(ns->voters) == 0) { - log_warn(LD_DIR, "Missing dir-source elements in a vote networkstatus."); + log_warn(LD_DIR, "Missing dir-source elements in a networkstatus."); goto err; } else if (ns->type != NS_TYPE_CONSENSUS && smartlist_len(ns->voters) != 1) { log_warn(LD_DIR, "Too many dir-source elements in a vote networkstatus."); @@ -3205,8 +3205,7 @@ networkstatus_parse_vote_from_string(const char *s, const char **eos_out, } if (fast_memcmp(rs1->identity_digest, rs2->identity_digest, DIGEST_LEN) >= 0) { - log_warn(LD_DIR, "Vote networkstatus entries not sorted by identity " - "digest"); + log_warn(LD_DIR, "Networkstatus entries not sorted by identity digest"); goto err; } } @@ -3319,12 +3318,12 @@ networkstatus_parse_vote_from_string(const char *s, const char **eos_out, base16_decode(declared_identity, sizeof(declared_identity), id_hexdigest, HEX_DIGEST_LEN) < 0) { log_warn(LD_DIR, "Error decoding declared identity %s in " - "network-status vote.", escaped(id_hexdigest)); + "network-status document.", escaped(id_hexdigest)); goto err; } if (!(v = networkstatus_get_voter_by_id(ns, declared_identity))) { - log_warn(LD_DIR, "ID on signature on network-status vote does not match " - "any declared directory source."); + log_warn(LD_DIR, "ID on signature on network-status document does " + "not match any declared directory source."); goto err; } sig = tor_malloc_zero(sizeof(document_signature_t)); @@ -3334,7 +3333,7 @@ networkstatus_parse_vote_from_string(const char *s, const char **eos_out, base16_decode(sig->signing_key_digest, sizeof(sig->signing_key_digest), sk_hexdigest, HEX_DIGEST_LEN) < 0) { log_warn(LD_DIR, "Error decoding declared signing key digest %s in " - "network-status vote.", escaped(sk_hexdigest)); + "network-status document.", escaped(sk_hexdigest)); tor_free(sig); goto err; } @@ -3353,8 +3352,8 @@ networkstatus_parse_vote_from_string(const char *s, const char **eos_out, /* We already parsed a vote with this algorithm from this voter. Use the first one. */ log_fn(LOG_PROTOCOL_WARN, LD_DIR, "We received a networkstatus " - "that contains two votes from the same voter with the same " - "algorithm. Ignoring the second vote."); + "that contains two signatures from the same voter with the same " + "algorithm. Ignoring the second signature."); tor_free(sig); continue; } @@ -3362,7 +3361,7 @@ networkstatus_parse_vote_from_string(const char *s, const char **eos_out, if (ns->type != NS_TYPE_CONSENSUS) { if (check_signature_token(ns_digests.d[DIGEST_SHA1], DIGEST_LEN, tok, ns->cert->signing_key, 0, - "network-status vote")) { + "network-status document")) { tor_free(sig); goto err; } @@ -3381,7 +3380,7 @@ networkstatus_parse_vote_from_string(const char *s, const char **eos_out, } SMARTLIST_FOREACH_END(_tok); if (! n_signatures) { - log_warn(LD_DIR, "No signatures on networkstatus vote."); + log_warn(LD_DIR, "No signatures on networkstatus document."); goto err; } else if (ns->type == NS_TYPE_VOTE && n_signatures != 1) { log_warn(LD_DIR, "Received more than one signature on a " -- cgit v1.2.3-54-g00ecf From 13a31e72db1b009623aa55bd52ffe7390a22623d Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 22 Feb 2016 11:43:59 -0500 Subject: Never vote for an ed key twice. When generating a vote, and we have two routerinfos with the same ed key, omit the one published earlier. This was supposed to have been solved by key pinning, but when I made key pinning optional, I didn't realize that this would jump up and bite us. It is part of bug 18318, and the root cause of 17668. --- changes/bug18318_ed | 7 +++++++ src/or/dirserv.c | 39 +++++++++++++++++++++++++++++++++++++++ src/or/or.h | 4 ++++ 3 files changed, 50 insertions(+) create mode 100644 changes/bug18318_ed (limited to 'changes') diff --git a/changes/bug18318_ed b/changes/bug18318_ed new file mode 100644 index 0000000000..af39234d53 --- /dev/null +++ b/changes/bug18318_ed @@ -0,0 +1,7 @@ + o Major bugfixes: + - When generating a vote with keypinning disabled, never include two + entries for the same ed25519 identity. This bug was causing + authorities to generate votes that they could not parse when a router + violated key pinning by changing its RSA identity but keeping its + Ed25519 identity. Fixes bug 17668; fixes part of bug 18318. Bugfix on + 0.2.7.2-alpha. diff --git a/src/or/dirserv.c b/src/or/dirserv.c index 8d9f166556..016514f10f 100644 --- a/src/or/dirserv.c +++ b/src/or/dirserv.c @@ -2126,6 +2126,44 @@ get_possible_sybil_list(const smartlist_t *routers) return omit_as_sybil; } +/** If there are entries in routers with exactly the same ed25519 keys, + * remove the older one. May alter the order of the list. */ +static void +routers_make_ed_keys_unique(smartlist_t *routers) +{ + routerinfo_t *ri2; + digest256map_t *by_ed_key = digest256map_new(); + + SMARTLIST_FOREACH_BEGIN(routers, routerinfo_t *, ri) { + ri->omit_from_vote = 0; + if (ri->signing_key_cert == NULL) + continue; /* No ed key */ + const uint8_t *pk = ri->signing_key_cert->signing_key.pubkey; + if ((ri2 = digest256map_get(by_ed_key, pk))) { + /* Duplicate; must omit one. Set the omit_from_vote flag in whichever + * one has the earlier published_on. */ + if (ri2->cache_info.published_on < ri->cache_info.published_on) { + digest256map_set(by_ed_key, pk, ri); + ri2->omit_from_vote = 1; + } else { + ri->omit_from_vote = 1; + } + } else { + /* Add to map */ + digest256map_set(by_ed_key, pk, ri); + } + } SMARTLIST_FOREACH_END(ri); + + digest256map_free(by_ed_key, NULL); + + /* Now remove every router where the omit_from_vote flag got set. */ + SMARTLIST_FOREACH_BEGIN(routers, const routerinfo_t *, ri) { + if (ri->omit_from_vote) { + SMARTLIST_DEL_CURRENT(routers, ri); + } + } SMARTLIST_FOREACH_END(ri); +} + /** Extract status information from ri and from other authority * functions and store it in rs>. * @@ -2815,6 +2853,7 @@ dirserv_generate_networkstatus_vote_obj(crypto_pk_t *private_key, routers = smartlist_new(); smartlist_add_all(routers, rl->routers); + routers_make_ed_keys_unique(routers); routers_sort_by_identity(routers); omit_as_sybil = get_possible_sybil_list(routers); diff --git a/src/or/or.h b/src/or/or.h index 4496cbcec3..b6d6ec074f 100644 --- a/src/or/or.h +++ b/src/or/or.h @@ -2081,6 +2081,10 @@ typedef struct { * tests for it. */ unsigned int needs_retest_if_added:1; + /** Used during voting to indicate that we should not include an entry for + * this routerinfo. Used only during voting. */ + unsigned int omit_from_vote:1; + /** Tor can use this router for general positions in circuits; we got it * from a directory server as usual, or we're an authority and a server * uploaded it. */ -- cgit v1.2.3-54-g00ecf From fa07c60c67d69ff25c4e64172e3a38b29a2e6143 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 23 Feb 2016 09:31:23 -0500 Subject: Fix another case of 17668: Add NoEdConsensus I had a half-built mechanism to track, during the voting process, whether the Ed25519 value (or lack thereof) reflected a true consensus among the authorities. But we never actually inserted this field in the consensus. The key idea here is that we first attempt to match up votes by pairs of , where can be NULL if we're told that there is no Ed key. If this succeeds, then we can treat all those votes as 'a consensus for Ed'. And we can include all other votes with a matching RSA key and no statement about Ed keys as being "also about the same relay." After that, we look for RSA keys we haven't actually found an entry for yet, and see if there are enough votes for them, NOT considering Ed keys. If there are, we match them as before, but we treat them as "not a consensus about ed". When we include an entry in a consensus, if it does not reflect a consensus about ed keys, then we include a new NoEdConsensus flag on it. This is all only for consensus method 22 or later. Also see corresponding dir-spec patch. --- changes/bug17668 | 5 +++++ src/or/dircollate.c | 4 +++- src/or/dirvote.c | 28 ++++++++++++++++++++++++++++ src/or/or.h | 5 ++++- 4 files changed, 40 insertions(+), 2 deletions(-) create mode 100644 changes/bug17668 (limited to 'changes') diff --git a/changes/bug17668 b/changes/bug17668 new file mode 100644 index 0000000000..fa5c1c8081 --- /dev/null +++ b/changes/bug17668 @@ -0,0 +1,5 @@ + o Major bugfixes (voting): + - When collating votes by Ed25519 identities, authorities now + include a "NoEdConsensus" flag if the ed25519 value (or lack thereof) + for a server does not reflect the majority consensus. Related to bug + 17668; bugfix on 0.2.7.2-alpha. diff --git a/src/or/dircollate.c b/src/or/dircollate.c index eeb0c24008..ca8b7ca847 100644 --- a/src/or/dircollate.c +++ b/src/or/dircollate.c @@ -81,6 +81,8 @@ dircollator_add_routerstatus(dircollator_t *dc, { const char *id = vrs->status.identity_digest; + vrs->ed25519_reflects_consensus = 0; + (void) vote; vote_routerstatus_t **vrs_lst = digestmap_get(dc->by_rsa_sha1, id); if (NULL == vrs_lst) { @@ -92,7 +94,7 @@ dircollator_add_routerstatus(dircollator_t *dc, const uint8_t *ed = vrs->ed25519_id; - if (tor_mem_is_zero((char*)ed, DIGEST256_LEN)) + if (! vrs->has_ed25519_listing) return; ddmap_entry_t search, *found; diff --git a/src/or/dirvote.c b/src/or/dirvote.c index be0635d92b..654d461dd6 100644 --- a/src/or/dirvote.c +++ b/src/or/dirvote.c @@ -1242,6 +1242,9 @@ networkstatus_compute_consensus(smartlist_t *votes, smartlist_free(combined_server_versions); smartlist_free(combined_client_versions); + if (consensus_method >= MIN_METHOD_FOR_ED25519_ID_VOTING) + smartlist_add(flags, tor_strdup("NoEdConsensus")); + smartlist_sort_strings(flags); smartlist_uniq_strings(flags); @@ -1539,6 +1542,8 @@ networkstatus_compute_consensus(smartlist_t *votes, num_bandwidths = 0; num_mbws = 0; num_guardfraction_inputs = 0; + int ed_consensus = 0; + const uint8_t *ed_consensus_val = NULL; /* Okay, go through all the entries for this digest. */ for (int voter_idx = 0; voter_idx < smartlist_len(votes); ++voter_idx) { @@ -1580,6 +1585,17 @@ networkstatus_compute_consensus(smartlist_t *votes, if (rs->status.has_bandwidth) bandwidths_kb[num_bandwidths++] = rs->status.bandwidth_kb; + + /* Count number for which ed25519 is canonical. */ + if (rs->ed25519_reflects_consensus) { + ++ed_consensus; + if (ed_consensus_val) { + tor_assert(fast_memeq(ed_consensus_val, rs->ed25519_id, + ED25519_PUBKEY_LEN)); + } else { + ed_consensus_val = rs->ed25519_id; + } + } } /* We don't include this router at all unless more than half of @@ -1587,6 +1603,14 @@ networkstatus_compute_consensus(smartlist_t *votes, if (n_listing <= total_authorities/2) continue; + if (ed_consensus > 0) { + tor_assert(consensus_method >= MIN_METHOD_FOR_ED25519_ID_VOTING); + if (ed_consensus <= total_authorities / 2) { + log_warn(LD_BUG, "Not enough entries had ed_consensus set; how " + "can we have a consensus of %d?", ed_consensus); + } + } + /* The clangalyzer can't figure out that this will never be NULL * if n_listing is at least 1 */ tor_assert(current_rsa_id); @@ -1640,6 +1664,10 @@ networkstatus_compute_consensus(smartlist_t *votes, } else if (!strcmp(fl, "Unnamed")) { if (is_unnamed) smartlist_add(chosen_flags, (char*)fl); + } else if (!strcmp(fl, "NoEdConsensus") && + consensus_method >= MIN_METHOD_FOR_ED25519_ID_VOTING) { + if (ed_consensus <= total_authorities/2) + smartlist_add(chosen_flags, (char*)fl); } else { if (flag_counts[fl_sl_idx] > n_flag_voters[fl_sl_idx]/2) { smartlist_add(chosen_flags, (char*)fl); diff --git a/src/or/or.h b/src/or/or.h index b24b6a85e4..431927c7e7 100644 --- a/src/or/or.h +++ b/src/or/or.h @@ -2380,7 +2380,10 @@ typedef struct vote_routerstatus_t { /** True iff the vote included an entry for ed25519 ID, or included * "id ed25519 none" to indicate that there was no ed25519 ID. */ unsigned int has_ed25519_listing:1; - unsigned int ed25519_reflects_consensus:1; /** DOCDOC */ + /** True if the Ed25519 listing here is the consensus-opinion for the + * Ed25519 listing; false if there was no consensus on Ed25519 key status, + * or if this VRS doesn't reflect it. */ + unsigned int ed25519_reflects_consensus:1; uint32_t measured_bw_kb; /**< Measured bandwidth (capacity) of the router */ /** The hash or hashes that the authority claims this microdesc has. */ vote_microdesc_hash_t *microdesc; -- cgit v1.2.3-54-g00ecf