Opened 18 months ago

Closed 3 months ago

#24338 closed defect (fixed)

DirAuths that have IPv6 addresses don't include them in their vote on themself

Reported by: tom Owned by: neel
Priority: Medium Milestone: Tor: 0.4.1.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-dirauth, easy, intro, 034-triage-20180328, 034-removed-20180328, asn-merge
Cc: teor, neel Actual Points:
Parent ID: #24403 Points: 1
Reviewer: nickm Sponsor: SponsorV-can

Description

Check out https://consensus-health.torproject.org/consensus-health.html and Control+f for:

BD6A829255CB08E66FBE7D3748363586E46B3810
847B1F850344D7876491A54892F904934E4EB85D
24E2F139121D4394C54B5BCC368B3B411857C413
F2044413DAC2E02E3D6BCF4735A19BCA1DE97281

Also strange: dannenberg votes on ReachableIPv6 but is not itself granted ReachableIPv6

Child Tickets

Change History (20)

comment:1 Changed 18 months ago by teor

Keywords: IPv6 tor-dirauth added
Milestone: Tor: 0.3.3.x-final
Parent ID: #20916
Points: 1
Sponsor: SponsorV-can

Authorities should assume their configured IPv6 address is reachable, just like they do for IPv4.

Also strange: dannenberg votes on ReachableIPv6 but is not itself granted ReachableIPv6

That's not surprising: voting on ReachableIPv6 requires outbound IPv6 connectivity (like IPv6Exit).
But receiving ReachableIPv6 requires inbound connectivity to an IPv6 ORPort. Does it declare one in its descriptor?

If it does, I'll email the dirauth list to let them know there's a misconfiguration, because it could end up with dannenberg not being marked Running,

comment:2 Changed 18 months ago by teor

Keywords: easy intro added

dannenberg does not have an IPv6 ORPort in its descriptor, so this is the expected behaviour:

http://193.23.244.244/tor/server/authority

But we still need to fix the underlying bug:

  • if an authority has AuthDirHasIPv6Connectivity set, and
  • it has an IPv6 ORPort,
  • it should assume it's own IPv6 ORPort is reachable and vote for it

This is what all authorities do with their own Running flag (which represents IPv4 and IPv6 ORPort reachability).

comment:3 Changed 18 months ago by teor

Keywords: needs-mandatory-IPv6 added; IPv6 removed
Milestone: Tor: 0.3.3.x-finalTor: very long term
Sponsor: SponsorV-can

I think this is a feature, not a bug.

If an authority votes for its own IPv6 address, and it's the only one to do this, then its IPv6 address will always be in the consensus, even if it's actually unreachable. (A single IPv6-voting authority can determine an IPv6 address by voting for it.)

When we make IPv6 mandatory on authorities, and change the consensus method so IPv6 votes are a simple majority, then we can fix this issue.

comment:4 Changed 18 months ago by teor

Milestone: Tor: very long termTor: 0.3.3.x-final
Parent ID: #20916

Hang on, we will need authorities to unconditionally advertise their own IPv6 address.

When we implement relay IPv6 reachability checks, relays in a new network will need IPv6 addresses to bootstrap off.

If an authority votes for its own IPv6 address, and it's the only one to do this, then its IPv6 address will always be in the consensus, even if it's actually unreachable. (A single IPv6-voting authority can determine an IPv6 address by voting for it.)

This is ok, because if we have a majority of authorities on IPv6 (that doesn't include that authority), that authority won't be marked Running, and its operator will notice.

Also, we should do reachability checks on authority IPv6 addresses, but just warn if they fail. This is what we do for authority IPv4 addresses.

comment:5 Changed 18 months ago by teor

Keywords: needs-mandatory-IPv6 removed
Parent ID: #24403

comment:6 Changed 18 months ago by teor

Sponsor: SponsorV-can

Here's what we need to do to fix this bug:

If an authority has an IPv6 ORPort, it should assume its own IPv6 ORPort is reachable, and vote for it.
This is what authorities do for their own Reachable flag, so we should be able to use the same code for this fix.

comment:7 Changed 16 months ago by teor

Milestone: Tor: 0.3.3.x-finalTor: 0.3.4.x-final

The 0.3.3 freeze deadline has passed, all these children of #24403 belong in 0.3.4

comment:8 Changed 14 months ago by nickm

Keywords: 034-triage-20180328 added

comment:9 Changed 14 months ago by nickm

Keywords: 034-removed-20180328 added

Per our triage process, these tickets are pending removal from 0.3.4.

comment:10 Changed 14 months ago by nickm

Milestone: Tor: 0.3.4.x-finalTor: unspecified

These tickets, tagged with 034-removed-*, are no longer in-scope for 0.3.4. We can reconsider any of them, if time permits.

comment:11 Changed 3 months ago by neel

Cc: neel added
Owner: set to neel
Status: newassigned

comment:12 Changed 3 months ago by neel

Is the function dirserv_set_router_is_running() the function where a dirauth would set itself as running (e.g. the "Reachable" flag)?

comment:13 Changed 3 months ago by neel

I have a PR here: https://github.com/torproject/tor/pull/698

I am not sure whether or not it is correct, but here it is.

comment:14 Changed 3 months ago by nickm

Milestone: Tor: unspecifiedTor: 0.4.1.x-final
Status: assignedneeds_review

comment:15 Changed 3 months ago by dgoulet

Reviewer: nickm

comment:16 in reply to:  12 Changed 3 months ago by nickm

Status: needs_reviewneeds_revision

Hi! I'd suggest that instead of overriding the last_reachable6 flag in node, we should override the place where we assign the ipv6_addr in the routerstatus:

  if (options->AuthDirHasIPv6Connectivity == 1 &&
      !tor_addr_is_null(&ri->ipv6_addr) &&
      node->last_reachable6 >= now - REACHABLE_TIMEOUT) {
    /* We're configured as having IPv6 connectivity. There's an IPv6
       OR port and it's reachable so copy it to the routerstatus.  */
    tor_addr_copy(&rs->ipv6_addr, &ri->ipv6_addr);
    rs->ipv6_orport = ri->ipv6_orport;

(in set_routerstatus_from_routerinfo())

Instead of checking whether last_reachable6 is recent, we could check whether it is recent OR the router is ourself.

Note that I think this change will make the test expression above way too complicated, and we should extract it into its own function, maybe called something like should_publish_node_ipv6().

comment:17 Changed 3 months ago by neel

Status: needs_revisionneeds_review

I made the changes and pushed them.

comment:18 Changed 3 months ago by nickm

Status: needs_reviewmerge_ready

Yes; this looks great. Thanks!

comment:19 Changed 3 months ago by nickm

Keywords: asn-merge added

comment:20 Changed 3 months ago by asn

Resolution: fixed
Status: merge_readyclosed

Merged to master!

Note: See TracTickets for help on using tickets.