Add a minimal implementation of ClientUseIPv4 so IPv6-only clients can bootstrap

When Tor bootstraps, it connects to directory servers on their IPv4 address, regardless of the ClientUseIPv6 option.

We can add a ClientUseIPv4 option, and set it to 1 by default. (This needs a config check: reject the config if ClientUseIPv4 and ClientUseIPv6 are both 0.)

Then we can add connection_get_address_family_for_purpose(), which returns the address family that should be used by outgoing connections. In this minimal implementation, we'll return AF_INET, unless ClientUseIPv4 is 0 and ClientUseIPv6 is 1 (an IPv6-only client).

For the moment, we'll ignore ClientPreferIPv6ORPort in connection_address_family_for_purpose. Once IPv6-only clients can bootstrap, we'll find out if the existing implementation of ClientPreferIPv6ORPort works.

This will implement parts of #9068 (moved), but only the minimal functionality needed to decide whether we are allowed to connect via IPv4 or IPv6. (There are many clever things we could do on dual-stack clients in connection_address_family_for_purpose(), to guess which IP version to use, or to alternate IP versions to find the best one. Some of these designs are described in #9068 (moved) and #17834 (moved). But first we need to get IPv6-using and IPv6-only clients bootstrapping.)

changed milestone to %Tor: 0.2.8.x-final

added TorCoreTeam201602 component::core tor/tor ipv6 milestone::Tor: 0.2.8.x-final owner::teor parent::17811 priority::high resolution::implemented severity::normal status::closed type::defect labels

My WIP branch here is feature17840-v2, it depends on the changes in #17327 (moved), it implements the changes in #9067 (moved) and #17834 (moved), and the majority of #9068 (moved).

It turns out that my initial design was over-complicated. Functions already exist that select relays and filter them by address. I just needed to modify them to check the IPv6 address as well.

Trac:
Status: new to accepted
Owner: N/A to teor

Please see my branch feature17840-v7 at https://github.com/teor2345/tor.git

TL;DR: Tor clients now bootstrap and make connections on IPv6-only hosts, if configured correctly.

Bootstrap on IPv6-only systems can be tested using variations on the following command-line: src/or/tor DataDirectory /tmp/tor-ipv6 ClientUseIPv4 0 ClientUseIPv6 1 SOCKSPort [::1]:9050

Commits in this branch:

• adds the ClientUseIPv4 and ClientPreferIPv6DirPort options,
• uses them and the existing ClientUseIPv6 option to bootstrap Dir servers
• uses the ClientUseIPv4/6 options and ClientPreferIPv6ORPort to choose and connect to entry guards,
• logs warnings when these preferences aren't met - they're for testing, we'll want to reconsider them before we merge.

The code and unit tests run on IPv4-only, dual-stack, and IPv6-only systems.

Bootstrap and connection works as long as the configured options match the available address families. We could make this more tolerant of misconfiguration,:

• for ClientUseIPv4, ClientUseIPv6, ClientPreferIPv6ORPort, ClientPreferIPv6DirPort, see #6772 (moved) and #17835 (moved)/#17217 (moved).
• for SOCKSPort, see #11360 (moved).

I'd test it using chutney, but chutney only runs on IPv4-only and dual-stack systems (and then mainly IPv4) at the moment. (See #17011 (moved)/#17153 (moved).) That said, nothing on IPv4-only or dual-stack systems breaks with this patch.

Tested on:

• OS X (IPv4-only & dual-stack)
• Linux (IPv4-only)

I also have the following configurations available, I'll test on them soon:

• Linux (IPv6-only)
• FreeBSD (IPv6-only)

Trac:
Status: accepted to needs_review

Testing on IPv6-only configs shows that we're still picking directory guards which only have IPv4 addresses. (Oops!)

This issue only manifests itself the second time we download the consensus. (The first time, we download from the authorities, and that works, because we select authorities with IPv6 addresses. The second time, we use directory guards, and some of the ones we select are IPv4-only.)

This should be easy to fix, I bet there's a fascist_firewall_* check missing in the directory guard selection code. (It's the weekend here now, and christmas next week, so it might take a little while.)

Trac:
Status: needs_review to needs_revision

It turns out that some of the IPv6 checking code was broken.

See my branch feature17840-v11, which redesigns the IPv6 checks and also adds some extra logging to catch bugs in the implementation. (We can leave that out or tone it down when we merge.)

Trac:
Status: needs_revision to needs_review

We can drop / reduce the severity / remove the backtraces from the log messages in the final commit. I haven't seen any of these messages in feature17840-v11 while running a client over IPv6 for a week, but I'd like to leave them enabled catch any edge cases.

I pushed two further commits to feature17840-v11 for bridge clients:

• Bridge clients should get directory documents via IPv6 (#17963 (moved))
• Bridge clients should respect fascist firewall and ClientUseIPv4
  • This commit preserves the existing bridge client preference for IPv6 addresses

b6dda7afbd27 Fix *_get_all_orports to use ipv6_orport

• Hmm. What if the md has an IPv6 address but the rs doesn't?
• Also, even macros should have documentation.

53b0788505ee Add ClientUseIPv4 and ClientPreferIPv6DirPort torrc options

• wow this is a big one.
• '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?
• 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.
• Removing firewall_is_fascist_or() from choose_good_entry_server() makes us iterate over the whole nodelist needlessly sometimes. Does that hurt performance ?
• 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.
• 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.
• 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.
• Why is nodelist_prefer_* a nodelist function? It doesn't seem to use or manipulate nodes or the nodelist.
• 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...
• The name fascist_firewall_... is less appropriate than it used to be. Let's add a new ticket to rename them though.

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!

Returning to 4d453a7a9041 Choose directory servers by IPv4/IPv6 preferences

• directory_initiate_command_routerstatus_rend() -- it seems like the new code here might be better to extract into a new function?
• Can it ever be right to use the directory_initiate_command() interface any more, since it doesn't know about IPv6/IPv4 issues?
• We used to have a notion of "anonymized connection not using begindir" -- we went to a node and told it to make a connection to its own dirport. Did we rip out all vestiges of this? If not, the code here is a bit sketchy.

Returning to 5ffe801801da Use fascist firewall and ClientUseIPv4 for bridge clients

• This whole thing is a bit scary.
• In the autobool conversion, did you grep for ClientUseIPv4 and ClientUseIPv6 to make sure you found them all?
• In lieu of the majority of this patch, maybe it would be better to have the torrc's ClientUseIPv[46] options set or_options_t's ClientUseIPv[46]_option fields, and then have options_validate() or something set ClientUseIPv[46] based on that? Not sure.

Trac:
Status: needs_review to needs_revision

Replying to 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 (moved).

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!

Replying to nickm:

Returning to 4d453a7a9041 Choose directory servers by IPv4/IPv6 preferences

• directory_initiate_command_routerstatus_rend() -- it seems like the new code here might be better to extract into a new function?

Done!

• Can it ever be right to use the directory_initiate_command() interface any more, since it doesn't know about IPv6/IPv4 issues?

We really should make callers specify a separate OR and Dir address here. Fixed that.

I checked the cases where it's used when I was writing this patch:

• In directory_get_from_dirserver(), bridge clients use it to request directory info from their bridge based on its routerinfo:
  • Since there may be no routerstatus for the bridge, they can't use directory_initiate_command_routerstatus_rend() directly.
  • My patch 0204c9dca838 in this branch chooses an appropriate IPv4/IPv6 address before calling directory_initiate_command().
• In launch_direct_bridge_descriptor_fetch(), bridge clients use the configured address for the bridge. There's only one configured address. It's in a bridge_info_t, not a routerstatus_t.
  • This is tried as a fallback mechanism after trying the bridge authority.
  • I added a notice-level log message in launch_direct_bridge_descriptor_fetch() if we try an unreachable bridge address. (Clients can still use the bridge without an up to date descriptor.)
• In consider_testing_reachability(), relays use their IPv4 address for reachability testing.
  • We'll resolve the missing IPv6 reachability tests in #6939 (moved).
  • Relays aren't allowed to have a fascist firewall or similar config, so it's ok for the moment.

• We used to have a notion of "anonymized connection not using begindir" -- we went to a node and told it to make a connection to its own dirport. Did we rip out all vestiges of this? If not, the code here is a bit sketchy.

I think it's all gone, but I'm not exactly sure what code you're talking about, I've never seen it. (Which likely means it was removed in or before 0.2.6).

DIRIND_ANON_DIRPORT is similar to what you're suggesting, but we only use that in consider_testing_reachability() for relay self-testing.

Returning to 5ffe801801da Use fascist firewall and ClientUseIPv4 for bridge clients

• This whole thing is a bit scary.
• In the autobool conversion, did you grep for ClientUseIPv4 and ClientUseIPv6 to make sure you found them all?

Yes, all ClientUseIPv6 instances. ClientUseIPv4 was not changed in this patch.

• In lieu of the majority of this patch, maybe it would be better to have the torrc's ClientUseIPv[46] options set or_options_t's ClientUseIPv[46]_option fields, and then have options_validate() or something set ClientUseIPv[46] based on that? Not sure.

I agree it's messy. I think we should do what we already do for ClientPreferIPv6OR/DirPort, and create an accessor function fascist_firewall_use_ipv6(). Then the logic is all in one place if we ever need to change it. (Done!)

Now the only references to ClientUseIPv6 are in config.c (options_validate) and policy.c (fascist_firewall_use_ipv6), in a log message in connection.c, and in or.h and the unit tests.

ClientPreferIPv6OR/DirPort already uses accessor functions, except for a limited number of cases (which are the same as those for ClientUseIPv6).

ClientUseIPv4 doesn't have an accessor function, but it is only used in a limited number of cases, similar to those above, with the addition of router_pick_directory_server_impl() and router_pick_trusteddirserver_impl() in routerlist.c.

Further optimisation:

Keeping firewall_is_fascist_or() (and adding firewall_is_fascist_dir()) allows us to do some optimisations in the OR and Dir nodelist searches. I've added these as a final commit.

Please see my branch feature17840-v11, which has these changes appended to it. (No reordering or rebases - the reviewed commits are all the same.)

(I suspect it won't merge cleanly into master, and perhaps won't autosquash. I can rebase and squash it manually when it's ready for merge.)

Trac:
Status: needs_revision to needs_review
Keywords: N/A deleted, TorCoreTeam201601 added

Replying to teor:

Replying to nickm:

Returning to 5ffe801801da Use fascist firewall and ClientUseIPv4 for bridge clients

• This whole thing is a bit scary.
• In the autobool conversion, did you grep for ClientUseIPv4 and ClientUseIPv6 to make sure you found them all?

Yes, all ClientUseIPv6 instances. ClientUseIPv4 was not changed in this patch.

• In lieu of the majority of this patch, maybe it would be better to have the torrc's ClientUseIPv[46] options set or_options_t's ClientUseIPv[46]_option fields, and then have options_validate() or something set ClientUseIPv[46] based on that? Not sure.

I agree it's messy. I think we should do what we already do for ClientPreferIPv6OR/DirPort, and create an accessor function fascist_firewall_use_ipv6(). Then the logic is all in one place if we ever need to change it. (Done!)

Now the only references to ClientUseIPv6 are in config.c (options_validate) and policy.c (fascist_firewall_use_ipv6), in a log message in connection.c, and in or.h and the unit tests.

Which allows me to perform one last very neat trick:

• If a user sets ClientUseIPv4 0, use IPv6 instead of IPv4. This makes configurations that disable both IPv4 and IPv6 impossible.

Please see my branch feature17840-v11.

Controllers could induce clients to connect to addresses not allowed by the firewall. The latest commit fixes that, and catches invalid addresses earlier in the connection process.

#17076 (moved) merged tests for options_validate, but this branch is based on master before it merged. I'll need to update those tests after this merges, split off into #18132 (moved).

Trac:
Priority: Medium to High

Okay, I think this is good to merge...

...Except that when I go to squash the fixup commits, I get a bunch of conflicts. Could you make a v11-squashed that squashes (but does not rebase), and I'll merge it into master?

Trac:
Status: needs_review to needs_revision

Ugh, sorry about that, see feature17840-v11-squashed.

There were some minor merge conflicts when I attempted a merge to master, please see the merge commit in feature17840-v11-merged for the resolution of these merge conflicts.

Please also see feature17840-v11-merged for the options_validate unit tests for ClientUseIPv4, ClientUseIPv6, ClientPreferIPv6ORPort and ClientPreferIPv6DirPort.

Trac:
Status: needs_revision to needs_review

The changes in #17076 (moved) made a mess of my unit test changes. I'll reopen #18132 (moved) to fix that up.

Please see feature17840-v11-squashed for the squashed but not rebased branch.

Please see the final commit in feature17840-v11-merged-v2 for the merge changes needed to merge that branch with the current master.