Opened 11 months ago

Closed 12 days ago

#29801 closed enhancement (fixed)

Write a proposal for IPv6 "Happy Eyeballs" on Tor clients

Reported by: neel Owned by: teor
Priority: Medium Milestone: Tor: 0.4.3.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: ipv6, prop299, 042-deferred-20190918
Cc: neel Actual Points: 1.5
Parent ID: #17835 Points:
Reviewer: neel Sponsor:

Description

Teor has asked for updates in https://github.com/torproject/torspec/pull/61, but this PR has already been merged.

I have created a new PR for his suggestions: https://github.com/torproject/torspec/pull/63

I don't believe you can add to a merged PR so that's why a new PR exists.

Child Tickets

TicketTypeStatusOwnerSummary
#31039taskclosedReview proposal 306: IPv6 "Happy Eyeballs" for Tor clients

Change History (36)

comment:1 Changed 11 months ago by teor

Status: assignedneeds_review

comment:2 Changed 11 months ago by teor

Please don't assign me as the reviewer for this proposal.
I have already reviewed it, and I would like another reviewer to review it.

comment:3 Changed 11 months ago by teor

I have reviewed the new changes.

Before you make more changes, I want to discuss the issues on the pull request. And I want a review from another person.

comment:4 Changed 11 months ago by asn

Reviewer: nickm

comment:5 Changed 10 months ago by nickm

Hi! Here are my initial questions on the proposal.

First off, I'd like to understand how this interacts with the guard-selection algorithms in guard-spec.txt. At what stage(s) during guard selection _exactly_ do we take these probabilities into account?

Second, I'd like to understand better why we are picking the constants we have here. Why are werounding everything to multiples of 1/4, and rounding up to 1/4. I understand that we don't want to reduce the probability to 0, but why 1/4 and not some other value? Similarly, why are we counting "no route" as twice as bad as a regular failure, instead of 1.5x or 3x or something?

Third, I'm assuming that we only do this algorithm when we detect that we are dual-stack. If we only have an ipv6 address or an ipv4 address, then we should just assume that's the only one we can use, right?

Fourth, the halving algorithm seems kind of complicated to me. In some other places, we halve things like this based on the passage of time, rather than on their totals. Would that make sense here?

Fifth, I don't understand section 7. Why would a client need to make 4 connections at once to a guard?

Sixth, what should the default be for ClientAutoIPv6ORPort?

comment:6 Changed 10 months ago by nickm

Status: needs_reviewneeds_revision

comment:7 in reply to:  5 Changed 10 months ago by teor

Replying to nickm:

Third, I'm assuming that we only do this algorithm when we detect that we are dual-stack. If we only have an ipv6 address or an ipv4 address, then we should just assume that's the only one we can use, right?

And what do we do if we can't detect any of our addresses?
(Some OSes allow administrators to block the APIs that Tor uses to detect local addresses. I know of at least one Tor user who does this.)

Using local addresses is covered by ticket #27492 - Try IPv4 or IPv6 more often based on public or private IP addresses. I think it makes sense to include all the children of #17835 in this proposal. (It's the only missing child that adds a new feature. All the rest are refactoring or bugs.)

Fourth, the halving algorithm seems kind of complicated to me. In some other places, we halve things like this based on the passage of time, rather than on their totals. Would that make sense here?

Fifth, I don't understand section 7. Why would a client need to make 4 connections at once to a guard?

You're right. This section is not very clear.

Tor limits the number of simultaneous connection attempts *when bootstrapping* using the option ClientBootstrapConsensusMaxInProgressTries. The default is 3, which is too low for networks that drop all IPv4 or all IPv6 packets.

Tor doesn't limit the number of simultaneous connection attempts once it has bootstrapped. Unless the DDoS code added a limit on the client side?

Sixth, what should the default be for ClientAutoIPv6ORPort?

0 until it has been tested and we are sure we want the feature on by default.
Similarly, each flag should be off by default, until we have tested it. Then we can decide if it should be on by default.

comment:8 Changed 10 months ago by neel

Status: needs_revisionneeds_review

I have pushed your suggestions as two new commits to https://github.com/torproject/torspec/pull/63. I hope I didn't miss anything.

Setting as "needs review".

comment:9 Changed 10 months ago by neel

Have you (the Core Tor team) been busy? I haven't gotten a review for this proposal in its current form yet.

Could one of you please review this proposal?

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

Replying to neel:

Have you (the Core Tor team) been busy? I haven't gotten a review for this proposal in its current form yet.

Yes, we have been very busy finishing off sbws, and fixing bugs in release 0.4.0.
We've also reviewed a bunch of your patches on other tickets.

Could one of you please review this proposal?

I don't have time to review this week, but maybe nickm does?

comment:11 Changed 10 months ago by teor

Status: needs_reviewneeds_information

Here is my review:

I am not sure if we should implement this proposal.

I think this proposal is really complex. It risks destabilising Tor's network code. It uses a lot of randomness, which has led to hard-to-diagnose network bugs in the past. (I suggested many of the ideas in the proposal, so this complexity and risk is my fault.)

The proposal is also non-standard: it claims to be "Happy Eyeballs", but it does not implement RFC 8305. (The simplest version of RFC 8305 uses IPv4 and IPv6 addresses for the same machine. It tries IPv6, waits 250ms, then tries IPv4.)

I'd like to see an alternative proposal for implementing Happy Eyeballs in Tor. (Neel, you don't have to write that proposal.) Then we can decide which alternative to accept.

Here's a quick sketch of what a minimal Happy Eyeballs proposal would look like:

When selecting addresses:

  1. Modify extend_info_t so it contains an IPv4 and an IPv6 address
  2. When a bridge or relay has multiple addresses, add them both to the extend_info_t.

When connecting using an extend_info_t:

  1. If there is an existing authenticated connection, use it.
  2. If not, connect using the first available, allowed, and preferred address. (IPv4 by default.)
  3. Then, schedule a timer for connecting using the other address, if it is available and allowed. We should choose a timer value that is higher than most clients successful TLS authentication time.

When a connection successfully authenticates using TLS:

  1. Cancel any other connection timers
  2. Cancel any other in-progress connections

When all available and allowed connection attempts fail:

  1. Tell the rest of Tor that the connection has failed

comment:12 Changed 10 months ago by nickm

Milestone: Tor: unspecified

comment:13 in reply to:  11 Changed 10 months ago by neel

Sorry for the delay in responding.

Replying to teor:

Here is my review:

I am not sure if we should implement this proposal.

I think this proposal is really complex. It risks destabilising Tor's network code. It uses a lot of randomness, which has led to hard-to-diagnose network bugs in the past. (I suggested many of the ideas in the proposal, so this complexity and risk is my fault.)

The proposal is also non-standard: it claims to be "Happy Eyeballs", but it does not implement RFC 8305. (The simplest version of RFC 8305 uses IPv4 and IPv6 addresses for the same machine. It tries IPv6, waits 250ms, then tries IPv4.)

Makes sense.

I'd like to see an alternative proposal for implementing Happy Eyeballs in Tor. (Neel, you don't have to write that proposal.) Then we can decide which alternative to accept.

Sounds good.

I am happy to write a proposal if nobody else is willing to volunteer for that role. Whether or not I write the new proposal, I still plan to implement it and write the code for the new proposal.

Here's a quick sketch of what a minimal Happy Eyeballs proposal would look like:

When selecting addresses:

  1. Modify extend_info_t so it contains an IPv4 and an IPv6 address
  2. When a bridge or relay has multiple addresses, add them both to the extend_info_t.

When connecting using an extend_info_t:

  1. If there is an existing authenticated connection, use it.
  2. If not, connect using the first available, allowed, and preferred address. (IPv4 by default.)
  3. Then, schedule a timer for connecting using the other address, if it is available and allowed. We should choose a timer value that is higher than most clients successful TLS authentication time.

When a connection successfully authenticates using TLS:

  1. Cancel any other connection timers
  2. Cancel any other in-progress connections

When all available and allowed connection attempts fail:

  1. Tell the rest of Tor that the connection has failed

Sounds good.

comment:14 Changed 9 months ago by neel

If nobody else has volunteered, I plan to write the "new" proposal based on teor's feedback in Comment 11.

comment:15 Changed 7 months ago by neel

Status: needs_informationneeds_review

I have a revised proposal based on Comment 11. The torspec PR is here: https://github.com/torproject/torspec/pull/86

Sorry for the 3-month delay in this, I was really busy.

comment:16 in reply to:  15 Changed 7 months ago by teor

Status: needs_reviewneeds_revision

Replying to neel:

I have a revised proposal based on Comment 11. The torspec PR is here: https://github.com/torproject/torspec/pull/86

Sorry for the 3-month delay in this, I was really busy.

Thanks for this proposal!

We review proposals on the tor-dev mailing list, before they are merged to the repository:
https://gitweb.torproject.org/torspec.git/tree/proposals/001-process.txt#n69

Please send a copy of the proposal to the list.
Then wait a week or two for comments.
Then make any necessary revisions, and re-submit the proposal to the list.

Once that process is complete, we'll assign your proposal a number, and merge it into the torspec repository.
(It can't be proposal 302, because the most recent proposal is proposal 305.)
https://gitweb.torproject.org/torspec.git/tree/proposals

comment:17 Changed 7 months ago by neel

I have submitted the proposal (the number is 306) to the mailing list.

comment:18 Changed 7 months ago by teor

Parent ID: #27491#17835
Summary: Add teor's suggestions for Prop#299 (referring IPv4 or IPv6 based on IP Version Failure Count)Write a proposal for IPv6 "Happy Eyeballs" on Tor clients

comment:19 Changed 7 months ago by neel

Status: needs_revisionneeds_review

I have an revised proposal (same GitHub PR) based on the reviews from the tor-dev@ mailing list.

https://lists.torproject.org/pipermail/tor-dev/2019-July/013918.html
https://lists.torproject.org/pipermail/tor-dev/2019-July/013919.html

comment:20 Changed 7 months ago by nickm

Milestone: Tor: unspecifiedTor: 0.4.2.x-final

comment:21 Changed 4 months ago by teor

Status: needs_reviewneeds_revision

We have at least 2 outstanding reviews on this proposal, but the author has not updated it.
We merged it to tor-spec as a draft.

comment:22 Changed 4 months ago by neel

Thank you so much!

I could not update Prop306 as I was busy and couldn't attend to it.

comment:23 Changed 4 months ago by nickm

Keywords: 042-deferred-20190918 added
Milestone: Tor: 0.4.2.x-finalTor: 0.4.3.x-final

Defer numerous 0.4.2 tickets to 0.4.3.

comment:24 Changed 8 weeks ago by neel

Status: needs_revisionneeds_review

Sorry for the delay.

Some of the suggestions in GitHub PR #87 rebased on the current torspec tree are in this new PR: https://github.com/torproject/torspec/pull/98

Setting as needs review.

comment:25 Changed 7 weeks ago by teor

Reviewer: nickmnickm, asn

I think these are the most recent reviews on tor-dev:
https://lists.torproject.org/pipermail/tor-dev/2019-August/013959.html

comment:26 Changed 6 weeks ago by teor

Reviewer: nickm, asnnickm, asn, teor

The proposal is large, and it has a lot of different changes. Some of these changes are not required.

It's really important that we tell developers which changes are necessary, important, optional, and experimental/alternative. Optional, and experimental/alternative changes should be moved to an appendix, or deleted.

See my email:
https://lists.torproject.org/pipermail/tor-dev/2019-December/014110.html

comment:27 Changed 6 weeks ago by teor

Status: needs_reviewneeds_revision

comment:28 Changed 5 weeks ago by neel

Status: needs_revisionneeds_review

It probably isn't perfect, but I have made revisions: https://lists.torproject.org/pipermail/tor-dev/2019-December/014113.html

comment:29 Changed 3 weeks ago by nickm

Reviewer: nickm, asn, teorteor

setting teor as reviewer. likely next step is to post to tor-dev.

comment:30 Changed 3 weeks ago by teor

I need to review the revised proposal here:
https://github.com/torproject/torspec/pull/98

I think we should probably merge it soon, it's been through a bunch of revisions. We will likely end up implementing parts of it for our IPv6 grant. (Or re-use parts of it in the tor change proposals for the IPv6 grant.)

comment:31 Changed 2 weeks ago by teor

Owner: changed from neel to teor
Reviewer: teorneel
Status: needs_reviewassigned

Hi Neel, I did a review, and made some changes to the proposal here:

The major changes include:

  • adding a bootstrap section
  • adding load balancing calculations (and changes)
  • simplifying the ConnectionAttemptDelay design
  • minor changes to match tor's typical design patterns

If you're happy with these changes, we can send a copy to tor-dev, and then merge it to the torspec repository.

comment:32 Changed 2 weeks ago by teor

Status: assignedneeds_review

comment:33 Changed 2 weeks ago by teor

Actual Points: 1

comment:34 Changed 2 weeks ago by neel

The new proposal looks good.

comment:35 Changed 2 weeks ago by nickm

Status: needs_reviewmerge_ready

comment:36 Changed 12 days ago by teor

Actual Points: 11.5
Resolution: fixed
Status: merge_readyclosed

These statistics were in a previous draft, and they seemed useful, so I added them back as optional:

  • client and relay connections
  • statistics in the heartbeat logs

I also improved the explanation of the "extra prop 306 connections" statistic.

I squashed this PR in:

Then I merged it to master.

I also emailed to the tor-dev list:
https://lists.torproject.org/pipermail/tor-dev/2020-January/014125.html

Note: See TracTickets for help on using tickets.