[tor-bugs] #27490 [Core Tor/Tor]: When ClientPreferIPv6ORPort is set to auto, and a relay is being chosen for a directory or orport connection, prefer IPv4 or IPv6 at random

[Tor Bug Tracker & Wiki]

#27490: When ClientPreferIPv6ORPort is set to auto, and a relay is being chosen for
a directory or orport connection, prefer IPv4 or IPv6 at random
--------------------------+--------------------------------
 Reporter:  neel          |          Owner:  neel
     Type:  enhancement   |         Status:  needs_revision
 Priority:  Medium        |      Milestone:
Component:  Core Tor/Tor  |        Version:
 Severity:  Normal        |     Resolution:
 Keywords:                |  Actual Points:
Parent ID:  #17835        |         Points:
 Reviewer:  teor          |        Sponsor:
--------------------------+--------------------------------
Changes (by teor):

 * status:  needs_review => needs_revision


Comment:

 I am very confused by this branch, and I'm not sure if I can review it
 properly.

 It seems to have:
 * one commit that makes most of the changes
 * three commits that make some log and comment changes
 * the same three commits that make some log and comment changes. Please
 don't add duplicate commits to a branch.
 * a merge
 * a bunch of extra commits that choose IP addresses randomly, but in
 different places all through the code
 * a commit that fixes the unit tests

 Some of this is my fault, because I asked you to add ClientAutoIPv6ORPort,
 but then I talked about ClientPreferIPv6ORPort in the rest of my feedback.

 Here's what this patch needs to do:
 * ClientPreferIPv6ORPort is an existing option, this patch must not change
 what it does
 * ClientAutoIPv6ORPort is a new option, it can do new things

 Here's how we can move forward:
 * please open a new pull request like commit 1a98526, but using
 ClientAutoIPv6ORPort, rather than changing ClientPreferIPv6ORPort
 * please add the logging changes from commit 2311a42
 * please add new unit tests for ClientAutoIPv6ORPort (if the unit tests
 for ClientPreferIPv6ORPort fail, then you have changed how
 ClientPreferIPv6ORPort works)

 Adding all those extra comments isn't as helpful as I thought it would be.
 Let's just change the function comment on
 fascist_firewall_prefer_ipv6_orport().

 I am not sure why the 4 "assign it a random preference" commits are
 needed.
 I think it is enough for fascist_firewall_prefer_ipv6_orport() to return a
 random value when ClientAutoIPv6ORPort is 1.
 Why do we need to choose at random in these places as well?

 But we might need to change rewrite_node_address_for_bridge() so that we
 choose a random address when ClientAutoIPv6ORPort is 1.
 But that is a simple change, because the code already calls
 fascist_firewall_prefer_ipv6_orport().

 All we need to do it change:
 "if (options->ClientPreferIPv6ORPort == -1) {"
 to
 "if (options->ClientPreferIPv6ORPort == -1 &&
 options->ClientAutoIPv6ORPort == 0) {"
 Then, when ClientAutoIPv6ORPort is 1, the code will choose at random.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/27490#comment:7>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online