Fix IPv6 bridge client directory address selection
After #17840 (moved) 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.
Activity
changed milestone to %Tor: 0.2.8.x-final
added TorCoreTeam-postponed-201604 TorCoreTeam201605 actualpoints::1 hour component::core tor/tor milestone::Tor: 0.2.8.x-final must-fix-before-028-rc parent::18483 points::1 priority::medium resolution::fixed reviewer::isis severity::normal status::closed type::defect version::tor 0.2.8.1-alpha labels
Please see commit e155948 Choose the correct address for one-hop connections in my branch feature18483 on https://github.com/teor2345/tor.git
Trac:
Status: new to needs_reviewisis 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.
Trac:
Status: needs_review to needs_revisionOh, 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.
Trac:
Status: needs_revision to needs_reviewReplying 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.
Trac:
Status: needs_review to merge_ready
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!
I split #18483 (moved), #18921 (moved), and #18929 (moved) 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.
(Replying to nickm's comments on #18483 (moved), as they refer to commits in that branch.)
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 (moved).
- 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?
mentioned in issue #18929 (moved)
moved to tpo/core/tor#18921 (closed)
