Opened 12 months ago

Last modified 3 months ago

#23975 assigned defect

Make node_get_pref_ipv6_orport() check addresses in the right order

Reported by: teor Owned by:
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version: Tor: 0.2.8.1-alpha
Severity: Normal Keywords: ipv6, review-group-29, 034-triage-20180328, 034-removed-20180328
Cc: Actual Points: 2
Parent ID: Points: 1
Reviewer: nickm Sponsor: SponsorV-can

Description

node_get_pref_ipv6_orport() and node_get_prim_orport() check addresses in the following order:

  • routerinfo / descriptor (modified by rewrite_bridge_address_for_node())
  • routerstatus / consensus
  • microdesc

Clients, bridge clients, and relays usually get the correct address from node_get_pref_ipv6_orport() and node_get_prim_orport(), whether they are using microdescriptors or not.

But there are a few cases when this breaks:

  • bridge clients should only check routerinfo for configured bridges
  • all clients and relays that use microdescs should never check full descriptors (and vice versa, except for the bridge case)
  • all clients and relays that use full descriptors should ignore the IPv6 address in the descriptor
  • all clients and relays that use microdescriptors should ignore the IPv6 address in the microdescriptor, when using a consensus method that puts IPv6 addresses in the microdesc consensus, otherwise they should use the microdescriptor

Child Tickets

TicketStatusOwnerSummaryComponent
#23874closedClear the address when node_get_prim_orport() returns earlyCore Tor/Tor
#24736closedteorClear the address when fascist_firewall_choose_address_base() can't find an addressCore Tor/Tor

Change History (19)

comment:1 Changed 12 months ago by teor

Owner: set to teor
Status: newassigned

comment:2 Changed 12 months ago by teor

These are all mine

comment:3 Changed 12 months ago by teor

Status: assignedneeds_revision

These have code in bug20916, but it needs to be chopped out and refactored after the other children of #20916 merge

comment:4 Changed 11 months ago by teor

I wrote a draft branch bug23975_tree, which uses the new consensus methods from bug23826-23828.

It still needs revision to fix some unit test failures, and it needs a changes file.

Last edited 11 months ago by teor (previous) (diff)

comment:5 Changed 11 months ago by teor

Actual Points: 1
Keywords: regression removed

This branch also obsoletes ReachableDirAddresses and removes all IPv6 DirPort code.
Removing it was easier than trying to fix it.

comment:6 Changed 10 months ago by teor

We should probably fix the onion key functions as well when we do this, and any other functions that implement a similar pattern of directory document checks. This depends on #23966.

Maybe there's even a way to make these generic?

comment:7 Changed 10 months ago by teor

This branch rapidly got out of control.
I want to split it into:

  • a branch that avoids using microdesc IPv6 addresses when the consensus method is high enough
  • a branch that gets rid of IPv6 DirPorts
  • a branch that deals with the bridge stuff
  • a branch that does any other stuff

comment:8 Changed 10 months ago by teor

Actual Points: 12
Sponsor: SponsorV-can
Summary: Make node_get_*_orport() check addresses in the right orderMake node_get_pref_ipv6_orport() check addresses in the right order

Making tor stop looking at unused directory documents is now #24731. This includes the bridge routerinfo checks.

Removing unused IPv6 DirPort code is now #24732.

comment:9 Changed 10 months ago by teor

Status: needs_revisionneeds_review

Please see my branch bug23975-redux.

It makes sure we ignore microdescs when the consensus has IPv6 ORPorts.
And it refactors some of the address functions so they depend on the revised code.

It also contains the cherry-picked commit from #24736 branch bug24736_028, because it is required for the unit tests to pass.

comment:10 Changed 10 months ago by nickm

Keywords: review-group-28 added

comment:11 Changed 9 months ago by nickm

Reviewer: nickm
Status: needs_reviewneeds_revision

Looks plausible! It's good to see so much complexity removed here.

I think we already merged #24736, so I'll assume we rebase before the merge to drop 189127acf2014907eac747e25d32668741dc2c5e ?

Let's add a comment to document SET_IPV6_AP. We should especially document that it conditionally returns, possibly by renaming it: macros that alter control flow should make it obvious.

Other than that, I think I like this. How is the test coverage for all the functions in policies.c that we modify?

Edit: fix ticket number (teor)

Last edited 9 months ago by teor (previous) (diff)

comment:12 Changed 9 months ago by nickm

Keywords: review-group-29 added; review-group-28 removed

comment:13 Changed 9 months ago by teor

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

The 0.3.3 feature freeze deadline has passed. We can do the remaining work in this ticket in 0.3.4. (The old code works, so deferring it is ok.)

comment:14 Changed 7 months ago by nickm

Keywords: 034-triage-20180328 added

comment:15 Changed 7 months ago by nickm

Keywords: 034-removed-20180328 added

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

comment:16 Changed 6 months ago by nickm

Milestone: Tor: 0.3.4.x-finalTor: unspecified

These needs_revision, tickets, tagged with 034-removed-*, are no longer in-scope for 0.3.4. We can reconsider any of them, if somebody does the necessary revision.

comment:17 Changed 6 months ago by teor

When #25691 merges, we should rebase this branch on top of it, and use the new has_preferred_descriptor() function.
(Or perhaps create a similar function that actually returns some of the fields.)

comment:18 Changed 3 months ago by teor

Owner: teor deleted
Status: needs_revisionassigned

Not on any roadmap yet.

comment:19 Changed 3 months ago by teor

Parent ID: #20916
Note: See TracTickets for help on using tickets.