Skip to content
Snippets Groups Projects

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

    • Closed (moved) Issue created by Neel Chauhan @neel

    Suggested by teor at #17835 (moved):

    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
    To upload designs, you'll need to enable LFS and have an admin enable hashed storage. More information

    Child items ...

    • Activity

    • Neel Chauhan changed milestone to %Tor: 0.4.0.x-final

      changed milestone to %Tor: 0.4.0.x-final

    • Neel Chauhan added component::core tor/tor ipv6 milestone::Tor: 0.4.0.x-final owner::neel parent::17835 priority::medium resolution::implemented reviewer::teor severity::normal status::closed type::enhancement labels

      added component::core tor/tor ipv6 milestone::Tor: 0.4.0.x-final owner::neel parent::17835 priority::medium resolution::implemented reviewer::teor severity::normal status::closed type::enhancement labels

    • N
      Neel Chauhan @neel ·
      Author

      Trac:
      Description: Suggested by teor at #17385 (moved):

      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

      to

      Suggested by teor at #17835 (moved):

      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
    • N
      Neel Chauhan @neel ·
      Author

      Trac:
      Type: defect to enhancement

    • N
      Neel Chauhan @neel ·
      Author

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

      I do know that there are other parts to #17835 (moved) 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.

    • T
      teor @teor ·

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

      Trac:
      Status: assigned to needs_revision
      Reviewer: N/A to teor

    • N
      Neel Chauhan @neel ·
      Author

      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.

    • N
      Neel Chauhan @neel ·
      Author

      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).

      Trac:
      Status: needs_revision to needs_review

    • T
      teor @teor ·

      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.

      Trac:
      Status: needs_review to needs_revision

    • N
      Neel Chauhan @neel ·
      Author

      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.

    • N
      Neel Chauhan @neel ·
      Author

      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?

    • N
      Neel Chauhan @neel ·
      Author

      Trac:
      Status: needs_revision to needs_review

    • T
      teor @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.

    • T
      teor @teor ·

      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.

      Trac:
      Milestone: N/A to Tor: 0.3.6.x-final

    • T
      teor @teor ·

      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.

      Trac:
      Status: needs_review to needs_revision

    • N
      Neel Chauhan @neel ·
      Author

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

    • T
      teor @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....
    • N
      Neel Chauhan @neel ·
      Author

      I also pushed the new changes fixing the space.

    • T
      teor @teor ·

      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.

      Trac:
      Status: needs_revision to needs_review

    • T
      teor @teor ·

      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.

      Trac:
      Keywords: N/A deleted, ipv6 added
      Status: needs_review to merge_ready

    • Loading
    • Loading
    • Loading
    • Loading
    • Loading
    • Loading
    • Loading
    • Loading
    • Loading
    • Loading
    Please register or sign in to reply