Opened 4 years ago

Closed 4 years ago

Last modified 3 years ago

#18921 closed defect (fixed)

Fix IPv6 bridge client directory address selection

Reported by: teor Owned by:
Priority: Medium Milestone: Tor: 0.2.8.x-final
Component: Core Tor/Tor Version: Tor: 0.2.8.1-alpha
Severity: Normal Keywords: must-fix-before-028-rc, TorCoreTeam201605, TorCoreTeam-postponed-201604
Cc: isis Actual Points: 1 hour
Parent ID: #18483 Points: 1
Reviewer: isis Sponsor:

Description

After #17840 in 0.2.8.1-alpha, we incorrectly chose an IPv4 address for all DIRIND_ONEHOP directory connections, even if the routerstatus didn't have an IPv4 address.

This likely affected bridge clients with IPv6 bridges.

Child Tickets

Change History (15)

comment:1 Changed 4 years ago by teor

Status: newneeds_review

Please see commit
e155948 Choose the correct address for one-hop connections
in my branch feature18483 on https://github.com/teor2345/tor.git

comment:2 Changed 4 years ago by isis

Reviewer: isis

comment:3 Changed 4 years ago by isis

Keywords: TorCoreTeam201604 added

comment:4 Changed 4 years ago by teor

Status: needs_reviewneeds_revision

isis correctly points out that I have DIRIND_ANONYMOUS connections always using IPv4 addresses. This won't work for IPv6 bridges.

I should use node_get_pref_orport() to get the canonical address of the node that we'll use to connect to it as a guard. This correctly uses IPv6 when it's available and preferred (including when bridge clients only have an IPv6 address), and IPv4 otherwise.

comment:5 Changed 4 years ago by teor

Status: needs_revisionneeds_review

Oh, hang on, we're getting the primary address because we want to put it in an extend_info when creating a circuit to the third hop. So that's OK, but I obviously need a better comment there.

7e5f0bb fixup! Choose the correct address for one-hop connection
on my branch feature18483.

And we don't have a node, so the node_get_prim_orport() function is out.

comment:6 in reply to:  5 Changed 4 years ago by isis

Status: needs_reviewmerge_ready

Replying to teor:

Oh, hang on, we're getting the primary address because we want to put it in an extend_info when creating a circuit to the third hop. So that's OK, but I obviously need a better comment there.

7e5f0bb fixup! Choose the correct address for one-hop connection
on my branch feature18483.

And we don't have a node, so the node_get_prim_orport() function is out.


Oh, because an extend is happening. This makes more sense now.

Alright, everything looks good to me.

comment:7 Changed 4 years ago by nickm

Keywords: TorCoreTeam201605 added

Calling all non-needs_information tickets for May.

comment:8 Changed 4 years ago by nickm

Keywords: TorCoreTeam-postponed-201604 added; TorCoreTeam201604 removed

April is over; calling these april tickets postponed into may.

comment:9 Changed 4 years ago by nickm

This is kinda big for an rc/stable patch. It looks okay and I think we need it and I'm okay with taking it anyway, but I'm also nominating it for my "I-told-you-so" award in case there is a bug here that none of us successfully found.

Initial Questions:

  • e726459eaf57ffe98c36730af58287229c9531d6: Are we really really sure that every possible case of the big complicated function directory_fetches_from_authorities() should be replaced with !directory_must_use_begindir() ?
  • e155948d0ac54b3102d6efa3d4a689d8a177f0f4: This really really needs comments in the new code here.

Okay, I need to come back and review the rest of this later. Big branch!

comment:10 Changed 4 years ago by teor

I split #18483, #18921, and #18929 into three separate branches for 0.2.8, and a branch for 0.2.9.
Please see bug18921 on https://github.com/teor2345/tor.git for this issue. It has no dependencies.

comment:11 in reply to:  9 Changed 4 years ago by teor

(Replying to nickm's comments on #18483, as they refer to commits in that branch.)

comment:12 in reply to:  9 Changed 4 years ago by teor

Replying to nickm:

This is kinda big for an rc/stable patch. It looks okay and I think we need it and I'm okay with taking it anyway, but I'm also nominating it for my "I-told-you-so" award in case there is a bug here that none of us successfully found.

You can blame me for a point release.
I also split it into 3 branches so it's easier to review.

Initial Questions:

  • e726459eaf57ffe98c36730af58287229c9531d6: Are we really really sure that every possible case of the big complicated function directory_fetches_from_authorities() should be replaced with !directory_must_use_begindir() ?

See my comment in #18483.

  • e155948d0ac54b3102d6efa3d4a689d8a177f0f4: This really really needs comments in the new code here.

e155948d0ac54b3102d6efa3d4a689d8a177f0f4 is Choose the correct address for one-hop connections.
This is now 4e0019e1409dffa20ddd6813b873565c0217a877 in my branch bug18921.

I am happy to add comments, but I can't see where they are needed.
Perhaps you meant another commit?

comment:13 Changed 4 years ago by nickm

Okay, this is much easier to review. Thanks!

This looks right to me. I'm squashing and merging it in 0.2.8.

comment:14 Changed 4 years ago by nickm

Resolution: fixed
Status: merge_readyclosed

comment:15 Changed 3 years ago by isabela

Points: small1
Note: See TracTickets for help on using tickets.