Consider circuit isolation when closing redundant intro points

When tor receives more than one request for connecting to an onion service within a short amount of time, and these requests are circuit-isolated (by sockauth, or something else), tor will launch multiple connections to an intro point (one for each isolated request). When the first introduction succeeds (intro acks), tor closes any circuits it considers redundant (rend_client_close_other_intros). Tor should only close circuits if the socks request isolation bits match.

changed milestone to %Tor: unspecified

added 041-can component::core tor/tor milestone::Tor: unspecified priority::low reviewer::dgoulet severity::normal status::needs-revision tbb-needs tor-client tor-hs type::defect labels

Trac:
Cc: N/A to gk

Trac:
Component: Core Tor to Core Tor/Tor
Milestone: N/A to Tor: unspecified
Keywords: N/A deleted, tor-client, 035-proposed, tor-hs, tbb-wants added

Trac:
Cc: gk to gk, mahrud

Trac:
Keywords: 035-proposed deleted, 035-roadmap-proposed added

Prefer the more common tbb-needs to tbb-wants. There doesn't appear to be any difference in how much TBB needs based on the flag.

Trac:
Keywords: tbb-wants deleted, tbb-needs added

Trac:
Status: new to assigned
Owner: N/A to neel
Cc: gk, mahrud to gk, mahrud, neel

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

Trac:
Status: assigned to needs_review

Trac:
Milestone: Tor: unspecified to Tor: 0.3.5.x-final

Hmm not sure this is accurate. I think we need to match both that is the .onion and the isolation flags.

If we happen to request 5 different .onions, we need to only close intro points that are for the same .onion on the intro circuit.

Trac:
Status: needs_review to needs_revision

I have pushed these changes.

Trac:
Status: needs_revision to needs_review

Replying to neel:

I have pushed these changes.

Oh not the dest_address but rather the .onion address that is the rend_pk_digest that was originally removed.

In other words, the .onion address of the SOCKS request and the flags need to be matched. Else, we'll close things that we shouldn't I think.

Trac:
Status: needs_review to needs_revision

I now compare the flags and rend_pk_digest and have pushed that.

I believe this is now accurate! Thanks neel!

Trac:
Status: needs_revision to merge_ready
Reviewer: N/A to dgoulet

Hi! Thanks for this patch!

I have a request and a question and a concern:

The request: Could somebody replace the "bugfix on 0.3.5.1-alpha" in the changelog with the actual version that first contained this bug? (If a bug first appears in A, and we fix it in B, the changelog should say "bugfix on A".)

The question: Does this apply to v3 onion services too? Should it?

The concern: I don't know if checking the "mixed" flags for equality is what we actually want here. (The flag "ISO_XYZ" is set in isolation_flags_mixed when the circuit have been used to connect streams with two different values for the "XYZ" parameter. Why are we looking at that?)

Trac:
Status: merge_ready to needs_revision

Replying to nickm:

The request: Could somebody replace the "bugfix on 0.3.5.1-alpha" in the changelog with the actual version that first contained this bug? (If a bug first appears in A, and we fix it in B, the changelog should say "bugfix on A".)

I opened #27761 (moved) to catch bug fix changes files on future releases. But #27761 (moved) might not have caught this changes file, because bugfixes can happen on the most recent release.

Trac:
Priority: Medium to High

I have pushed an updated version which checks the individual isolation flags. However, it requires nested for loops and a goto statement. It is committed already in the same PR (https://github.com/torproject/tor/pull/311).

I have also backed up the previous branch (rejected) branch here: https://github.com/neelchauhan/tor/tree/b26368_old. The reason why I backed it up was because I squash commits is to neaten up my commit history for a patch.

Trac:
Status: needs_revision to needs_review