Opened 12 months ago

Closed 9 months ago

#27490 closed enhancement (implemented)

When ClientPreferIPv6ORPort is set to auto, and a relay is being chosen for a directory or orport connection, prefer IPv4 or IPv6 at random

Reported by: neel Owned by: neel
Priority: Medium Milestone: Tor: 0.4.0.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: ipv6
Cc: neel Actual Points:
Parent ID: #17835 Points:
Reviewer: teor Sponsor:

Description (last modified by neel)

Suggested by teor at #17835:

  1. Make ClientPreferIPv6ORPort into an autobool
  2. When ClientPreferIPv6ORPort is set to auto, and a relay is being chosen for a directory or orport connection, prefer IPv4 or IPv6 at random

Child Tickets

Change History (25)

comment:1 Changed 12 months ago by neel

Description: modified (diff)

comment:2 Changed 11 months ago by neel

Type: defectenhancement

comment:3 Changed 11 months ago by neel

My PR is here: https://github.com/torproject/tor/pull/317

I do know that there are other parts to #17835 and I will do them as separate pull requests. Changes to individual tickets will (probably) be done on the same PR as the original.

About testing:

I have tested this on a laptop running FreeBSD 12 connected to a router running Tomato Shibby and a Hurricane Electric IPv6 tunnel on a IPv4-only Verizon FiOS connection (FiOS is FTTH/GPON).

How I tested this: I used a custom torrc with ClientPreferIPv6ORPort to auto and EntryNodes to an IPv6-capable guard. As this is only the first step, there are times where I get IPv4 connections to the IPv6-supported guard and other times where I get IPv6 connections to the same guard.

I also set EntryNodes to an IPv4-only guard and get only IPv4 connections.

For the Tor developers reviewing this patch, you may or may not have IPv6 in your home or office to test this patch. If you don't, you could use HE's IPv6 tunnel or a testing network.

comment:4 in reply to:  3 ; Changed 11 months ago by teor

Reviewer: teor
Status: assignedneeds_revision

Replying to neel:

My PR is here: https://github.com/torproject/tor/pull/317

Thanks, I did a review on the pull request.

I do know that there are other parts to #17835 and I will do them as separate pull requests. Changes to individual tickets will (probably) be done on the same PR as the original.

About testing:

I have tested this on a laptop running FreeBSD 12 connected to a router running Tomato Shibby and a Hurricane Electric IPv6 tunnel on a IPv4-only Verizon FiOS connection (FiOS is FTTH/GPON).

How I tested this: I used a custom torrc with ClientPreferIPv6ORPort to auto and EntryNodes to an IPv6-capable guard. As this is only the first step, there are times where I get IPv4 connections to the IPv6-supported guard and other times where I get IPv6 connections to the same guard.

I also set EntryNodes to an IPv4-only guard and get only IPv4 connections.

What happens when you don't set EntryNodes?

For the Tor developers reviewing this patch, you may or may not have IPv6 in your home or office to test this patch. If you don't, you could use HE's IPv6 tunnel or a testing network.

Some Tor developers have native IPv6 on their servers, and they can give shell + compiler access.

comment:5 in reply to:  4 Changed 11 months ago by neel

Replying to teor:

What happens when you don't set EntryNodes?

I default to an IPv4 guard.

Some Tor developers have native IPv6 on their servers, and they can give shell + compiler access.

I also have a lot of servers myself (home servers and VPS servers) so I am fine on my own for this patch. UPDATE: A lot of servers with IPv6, that is.

I will work on the changes needed.

Last edited 11 months ago by neel (previous) (diff)

comment:6 Changed 11 months ago by neel

Status: needs_revisionneeds_review

The necessary changes have been made and committed. This has been done over a course of several days but I believe it can be reviewed now (and hopefully accepted, but it could also get rejected).

comment:7 Changed 11 months ago by teor

Status: needs_reviewneeds_revision

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.

comment:8 Changed 11 months ago by neel

As per your request, I have a new PR: https://github.com/torproject/tor/pull/373

However, this still lacks tests. At least right now, I don't see how I can fit tests into ClientAutoIPv6ORPort as the output can be random as a result of this function.

comment:9 Changed 11 months ago by neel

Also, should I check for ClientAutoIPv6ORPort is 1 in fascist_firewall_use_ipv6()? I have added an commit for it. If not, is it okay if I delete the commit 67e5a36?

comment:10 Changed 11 months ago by neel

Status: needs_revisionneeds_review

comment:11 Changed 11 months ago by teor

Hi, I'm at the Tor Meeting, so I might not get a chance to review the new pull request for a few weeks.

comment:12 in reply to:  9 Changed 10 months ago by teor

Milestone: Tor: 0.3.6.x-final

If this code works, and we think it doesn't introduce too much technical debt, it will go in 0.3.6.

Replying to neel:

As per your request, I have a new PR: https://github.com/torproject/tor/pull/373

However, this still lacks tests. At least right now, I don't see how I can fit tests into ClientAutoIPv6ORPort as the output can be random as a result of this function.

Here's how we can test functions with random outputs:

  • work out what the possible results are. Run the functions that have random results, and check that the actual result is one of the possible results.
  • mock the function that calls the random number generator, and check that other functions behave correctly with each of the possible inputs (in this case, 0 and 1)
  • test the tor binary in integration tests using shell scripts, python, or chutney

Replying to neel:

Also, should I check for ClientAutoIPv6ORPort is 1 in fascist_firewall_use_ipv6()? I have added an commit for it. If not, is it okay if I delete the commit 67e5a36?

fascist_firewall_use_ipv6() means "can this client use IPv6?"
So it needs to check for ClientAutoIPv6ORPort == 1. Let's keep the commit.

comment:13 Changed 10 months ago by teor

Status: needs_reviewneeds_revision

Thanks for this patch, I asked for a few minor changes on the pull request.

Please write a few tests for the new functions as well, and make sure the unit tests and the CI passes.

Reminder: make changes, add commits to the branch, and push them to github to update the pull request. Don't rebase or merge the branch.

comment:14 Changed 10 months ago by neel

I have made the changes and added a regression test. The tests pass.

comment:15 Changed 10 months ago by teor

The CI does not pass:
https://travis-ci.org/neelchauhan/tor/builds/442447926

Please run "make check"on your branches before submitting them:

make  check-TESTS check-local
make[1]: Entering directory `/home/travis/build/torproject/tor'
perl ./scripts/maint/checkSpace.pl -C \
		./src/lib/*/*.[ch] \
		./src/core/*/*.[ch] \
		./src/feature/*/*.[ch] \
		./src/app/*/*.[ch] \
		./src/test/*.[ch] \
		./src/test/*/*.[ch] \
		./src/tools/*.[ch]
...
 DoubleNL:./src/test/test_policy.c:2475
make[1]: *** [check-spaces] Error 1
make[1]: *** Waiting for unfinished jobs....

comment:16 Changed 10 months ago by neel

I also pushed the new changes fixing the space.

comment:17 Changed 10 months ago by teor

Status: needs_revisionneeds_review

Looks ok to me.

Let's merge this as an experimental option as soon as the CI passes:
https://github.com/torproject/tor/pull/373

We should also test it with chutney with ClientAutoIPv6ORPort 1 and 0. I'll do that this afternoon.

comment:18 in reply to:  17 Changed 10 months ago by teor

Keywords: ipv6 added
Status: needs_reviewmerge_ready

Replying to teor:

We should also test it with chutney with ClientAutoIPv6ORPort 1 and 0. I'll do that this afternoon.

Sorry, I ran out of time on Friday.

This branch passes all chutney networks with:

  • (ClientAutoIPv6ORPort default)
  • ClientAutoIPv6ORPort 1

There are no unexpected warnings in these chutney networks.

I'm still testing:

  • ClientAutoIPv6ORPort 0 (which is the default, so it should be the same as the first run)

Let's merge this experimental option, and see how it goes.

comment:19 Changed 10 months ago by neel

I have one more question: do I need to complete tickets like #27491 and #27492 before this will get merged?

comment:20 Changed 10 months ago by teor

I think we can merge this feature as an experimental option. Then we can improve it later.

I will ask what other people think at the team meeting tomorrow.

comment:21 Changed 10 months ago by teor

Here's what we discussed at the meeting:

Pathbias isn't affected, because it only triggers after two hops.

Guard selection may be affected, but:

  • IPv4-only clients will try 1-2 guards, and fail 0-1 times before connecting on average (all guards have IPv4)
  • IPv6-only clients will try 1-5 guards, and fail 0-5 times before connecting on average (20% of guard weight has IPv6 - https://metrics.torproject.org/advbw-ipv6.html )

We think this is acceptable, and if it causes bugs in the guard code, let's fix the guard code.

comment:22 Changed 10 months ago by nickm

Milestone: Tor: 0.3.6.x-finalTor: 0.4.0.x-final

Tor 0.3.6.x has been renamed to 0.4.0.x.

comment:23 Changed 9 months ago by teor

Hi nickm, what needs to be done before this branch is merged?

comment:24 Changed 9 months ago by nickm

Sorry -- I had been afraid that this would be harder to review than it actually is. I've squashed it and making a new PR at https://github.com/torproject/tor/pull/557 . If it passes CI, I'll merge.

comment:25 Changed 9 months ago by nickm

Resolution: implemented
Status: merge_readyclosed

CI seems happy; merging.

Note: See TracTickets for help on using tickets.