[tor-bugs] #17840 [Tor]: Add a minimal implementation of ClientUseIPv4 so IPv6-only clients can bootstrap

[Tor Bug Tracker & Wiki]

#17840: Add a minimal implementation of ClientUseIPv4 so IPv6-only clients can
bootstrap
--------------------+------------------------------------
 Reporter:  teor    |          Owner:  teor
     Type:  defect  |         Status:  needs_revision
 Priority:  Medium  |      Milestone:  Tor: 0.2.8.x-final
Component:  Tor     |        Version:
 Severity:  Normal  |     Resolution:
 Keywords:  ipv6    |  Actual Points:
Parent ID:  #17811  |         Points:
  Sponsor:          |
--------------------+------------------------------------

Comment (by teor):

 Replying to [comment:8 nickm]:
 > b6dda7afbd27 Fix *_get_all_orports to use ipv6_orport

 Pushed a fixup.

 >  * Hmm. What if the md has an IPv6 address but the rs doesn't?

 Fixed:
 * add tor_addr_port_is_valid and similar (this also helps with the
 addr/port validity checks in subsequent commits)
 * check that each ri/rs/md is non-NULL and has a a valid address
 * fall back to the next address source if a valid address wasn't found

 >  * Also, even macros should have documentation.

 Fixed.

 >
 > 53b0788505ee Add ClientUseIPv4 and ClientPreferIPv6DirPort torrc options

 Pushed a fixup commit.

 >  * 'node_has_ipv6_addr' has a similar issue to the one from the previous
 patch -- what if the md has no ipv6 address but the rs does?

 Fixed in a similar way to the previous commit
 * Reorder checks to be consistent with other functions (order is
 irrelevant to the result after this fix)

 >  * In the previous patch we looked at ports to see if the address was
 real; here we look at the address (in node_has_ipv6_addr).  Why? If
 there's a good reason let's document it.

 The previous patch now looks at both address and port.
 Modified node_ipv6_or/dir_preferred to check the address and port.
 Modified similar functions to check for valid addresses and ports using
 the new tor_addr_port_is_valid_* functions.

 node_has_ipv6_addr and node_get_prim_addr_ipv4h are not port-specific, so
 they don't know which port to check. Added comments documenting them as
 exceptional cases.

 I also found a formatting inconsistency in node_get_address_string, fixed
 it in a separate commit.

 >  * Removing firewall_is_fascist_or() from choose_good_entry_server()
 makes us iterate over the whole nodelist needlessly sometimes.  Does that
 hurt performance ?

 No, because the next commit removes the iteration which adds unreachable
 nodes to excluded, in favour of checking every node for
 preferred/reachable addresses as it's added to the included list in
 router_choose_random_node.

 Still, it's worth having to use elsewhere, but it is another place where
 we embed the assumption that every non-bridge relay has an IPv4 address.

 I put it back in, modified to work with
 ClientUseIPv4/ClientUseIPv6/UseBridges.
 (This also allows us to do some optimisations to the rest of the
 fascist_firewall_* functions, I'll add that as a final commit.)

 >  *  The stuff in options_validate() to set reachable_knows_ipv6 seems
 pretty horrible.  We should be parsing these to see whether they're ipv6,
 right?  Doing strstr and strcmpstart seems like it's guaranteed to miss
 something.

 Yes, that's ugly. Implemented a simpler check in parse_reachable_addresses
 instead:
 * if ClientUseIPv4 is 1 but fascist firewall blocks all IPv4 addresses, or
 * if ClientUseIPv6 is 1 (or UseBridges is 1) but fascist firewall blocks
 all IPv6 addresses,
 then warn the user they're not getting what they asked for.

 >  * nodelist_prefer_ipv6() seems a little icky; tristates can be
 confusing, especially when the name implies that the function returns a
 boolean.  Maybe rename it to end with "_impl" to make it clear that this
 is an internal-use-only function that the other functions will refer to.

 Done!

 >  * Also, it's against house style to say "refer to bug X" in the Tor
 code.  If it's important enough to refer to in a comment, it's probably
 important enough to explain in the comment.

 Deleted that part of the comment, it's not important.

 >  * Why is nodelist_prefer_* a nodelist function?  It doesn't seem to use
 or manipulate nodes or the nodelist.

 Renamed to fascist_firewall_prefer_* and moved to policies.[ch].
 We can do more renaming when we rename the entire fascist_firewall_*
 function set.

 >  * Can we remove the fascist_firewall_allows_... ri, rs, and md cases in
 favor of fascist_firewall_allows_node ?  Whenever an ri, rs, or md exists,
 a node should exist too.   At the very list, nobody should be using a raw
 md to refer to a node.
 >  * Same as above for choose_address...

 We need fascist_firewall_allows/choose_address_rs because it's used by
 fascist_firewall_allows_dir_server (which passes it
 dir_server_t.fake_status) and
 directory_initiate_command_routerstatus_rend.

 The others can be removed.

 >  * The name fascist_firewall_... is less appropriate than it used to be.
 Let's add a new ticket to rename them though.

 I particularly dislike fascist_firewall_choose_address_impl &
 fascist_firewall_choose_address_base. I'd run out of creativity by that
 point.

 Split off as #18106.

 >
 > 1d81c63c85b4 Choose OR Entry Guards using IPv4/IPv6 preferences
 >   * LGTM
 > 4d453a7a9041 Choose directory servers by IPv4/IPv6 preferences
 >   * I'll return to this after I review all the little ones.
 > dede8d405348 Log when IPv4/IPv6 restrictions or preferences weren't met
 >   * Seems worthwhile.
 > a8a1cb70ce95 fixup! Add ClientUseIPv4 and ClientPreferIPv6DirPort torrc
 options
 >   * LGTM
 > 0204c9dca838 Choose bridge addresses by IPv4/IPv6 preferences
 >   * LGTM
 > fb1cb1bd3046 Make entry_guard_set_status consistent with entry_is_live
 >   * Yup, lgtm.
 > 5ffe801801da Use fascist firewall and ClientUseIPv4 for bridge clients
 >   * I'll return to this too after I've reviewed all the little ones.
 >
 > Looks like I've got a couple more commits to review!

 And I have a few more to fixup!

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