[tor-bugs] #17668 [Tor]: moria1, with updated v3 cert: Bug: Generated a networkstatus consensus we couldn't parse.

[Tor Bug Tracker & Wiki]

#17668: moria1, with updated v3 cert: Bug: Generated a networkstatus consensus we
couldn't parse.
-------------------------------------------------+-------------------------
 Reporter:  arma                                 |          Owner:  nickm
     Type:  defect                               |         Status:
 Priority:  High                                 |  needs_revision
Component:  Tor                                  |      Milestone:
 Severity:  Blocker                              |        Version:
 Keywords:  201512-deferred, TorCoreTeam201602,  |     Resolution:
  must-fix-before-028-rc                         |  Actual Points:
Parent ID:                                       |         Points:
 Reviewer:                                       |        Sponsor:
-------------------------------------------------+-------------------------

Comment (by nickm):

 Replying to [comment:38 teor]:
 > Spec / Design Review:
 >
 > This addition to dir-spec.txt only makes sense if you've read the
 NoEdConsensus description, can we say "do not produce a majority consensus
 about a relay's Ed25519 key"?
 > {{{
 > +        * If consensus method 22 or later is used, and the votes do not
 > +          produce a majority consensus about Ed25519 key (see 3.8.0.1
 below), the
 > +          consensus must include a NoEdConsensus flag on the "s" line.
 > }}}

 Calling this `T1`.

 >
 > I'm not seeing how the NoEdConsensus flag actually affects tor's
 behaviour. So I assume the client/relay/authority modifications for using
 the NoEdConsensus flag to avoid certain ed25519 keya will be handled in a
 separate ticket?

 Yes, that's right.

 >
 > Code review:
 >
 > Things I think are wrong or incomplete:
 >
 > routers_make_ed_keys_unique claims to check IP:ORPort, but I don't see
 where we check it in that function.

 Will fix documentation.  Calling thar `T2`.  The issue is that it's not
 actually a disaster for IP:Port to be duplicated; only key duplication
 does horrible things to the consensus process.

 >
 > This check preserves the existing digestmap entry if the published times
 are equal. But different authorities could choose different descriptors-
 with-identical-published-times, depending on the order they arrived at the
 authority.
 > So let's find a consistent way of selecting a descriptor to omit if
 their published times are identical - how about comparing
 signed_descriptor_digest? (And if the digests are identical, it really
 doesn't matter which descriptor we omit, because they have the same
 content.)

 Good idea; Calling it `T3`.

 > We should do the same tie-breaking in the IP:Port checks too.
 > {{{
 > +    if ((ri2 = digest256map_get(by_ed_key, pk))) {
 > +      /* Duplicate; must omit one.  Omit the earlier. */
 > +      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;
 > +      }
 > }}}
 >
 > Things I don't understand:
 >
 > I have no idea what "treat all routers with that identity as the same"
 means here - does it mean: "a new descriptor with this id replaces any
 older descriptors with this id"?
 > {{{
 > + * The rule is:
 > + *    If an RSA identity key is listed by more than half of the
 authorities,
 > + *    include it, and treat all routers with that identity as the same.
 > + */
 > }}}

 Calling this `T4`.

 >
 > Should this read "no <ed,rsa> has been listed by more than half the
 authorities"?
 > Should it read "include <NULL, rsa>"?
 > {{{
 > + *    Otherwise, if an <*,rsa> or <rsa> identity is listed by more than
 > + *    half of the authorities, and no <ed,rsa> has been listed, include
 > + *    it.
 > }}}

 Calling this `T5`.

 > Typos:
 >
 > I think you mean `n'th` and `n'th` here (to match
 dircollator_get_votes_for_router), not `i'th` and `n'th`:
 > {{{
 > +  /* The i'th member of this array corresponds to the
 vote_routerstatus_t (if
 > +   * any) received for this digest pair from the n'th voter. */
 > }}}
 >
 > Can we decide whether to use i'th or n'th in all these added comments,
 or do they mean different things?

 Calling this `T6`.

 Thanks!

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/17668#comment:44>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online