Opened 5 weeks ago

Last modified 2 weeks ago

#29801 needs_information enhancement

Add teor's suggestions for Prop#299 (referring IPv4 or IPv6 based on IP Version Failure Count)

Reported by: neel Owned by: neel
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Normal Keywords: ipv6, prop299
Cc: neel Actual Points:
Parent ID: #27491 Points:
Reviewer: nickm 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

Change History (13)

comment:1 Changed 5 weeks ago by teor

Status: assignedneeds_review

comment:2 Changed 5 weeks 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 5 weeks 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 5 weeks ago by asn

Reviewer: nickm

comment:5 Changed 5 weeks 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 5 weeks ago by nickm

Status: needs_reviewneeds_revision

comment:7 in reply to:  5 Changed 5 weeks 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 5 weeks 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 3 weeks 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 3 weeks 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 3 weeks 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 2 weeks ago by nickm

Milestone: Tor: unspecified

comment:13 in reply to:  11 Changed 2 weeks 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.

Note: See TracTickets for help on using tickets.