[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [tor-bugs] #17668 [Tor]: moria1, with updated v3 cert: Bug: Generated a networkstatus consensus we couldn't parse.
#17668: moria1, with updated v3 cert: Bug: Generated a networkstatus consensus we
couldn't parse.
------------------------------------------------+--------------------------
Reporter: arma | Owner: nickm
Type: defect | Status: accepted
Priority: High | Milestone: Tor:
Component: Tor | 0.2.7.x-final
Severity: Blocker | Version:
Keywords: 201512-deferred, TorCoreTeam201602 | Resolution:
Parent ID: | Actual Points:
Sponsor: | Points:
------------------------------------------------+--------------------------
Comment (by 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.
}}}
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?
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.
That comment is ambiguous, we should be checking IPv4:ORPort,
IPv4:DirPort, IPv6:ORPort, and IPv6:DirPort for uniqueness. But I think we
only want to do that check among routerstatuses with the Running flag
(those we;ve checked reachable). Which means IPv4 ORPorts only for the
moment, as we don't check anything else.
(Otherwise, this opens up a denial of service opportunity where a relay
claims to have a popular relay's IPv4:ORPort, IPv4:DirPort, IPv6:ORPort,
or IPv6:DirPort.)
Maybe this is best tackled by a different function in a different ticket.
So let's fix the comment in this one.
{{{
+/** If there are entries in <b>routers</b> with exactly the same
IP:OrPort or
+ * exactly the same ed25519 keys, remove the older one. May alter the
order
+ * of the list. */
}}}
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.)
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.
+ */
}}}
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.
}}}
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?
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/17668#comment:38>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs