[tor-bugs] #31561 [Core Tor/Tor]: hs-v3: Service can keep unused intro points in its list

[Tor Bug Tracker & Wiki]

#31561: hs-v3: Service can keep unused intro points in its list
---------------------------------------+-----------------------------------
 Reporter:  dgoulet                    |          Owner:  dgoulet
     Type:  defect                     |         Status:  needs_revision
 Priority:  Medium                     |      Milestone:  Tor:
                                       |  0.4.2.x-final
Component:  Core Tor/Tor               |        Version:
 Severity:  Normal                     |     Resolution:
 Keywords:  tor-hs, hv-v3, 042-should  |  Actual Points:  0.1
Parent ID:  #30200                     |         Points:  0.2
 Reviewer:  asn                        |        Sponsor:  Sponsor27-must
---------------------------------------+-----------------------------------

Comment (by mikeperry):

 Replying to [comment:14 asn]:
 > Replying to [comment:13 nickm]:
 > > I'm concerned that this patch makes `purpose ==
 CIRCUIT_PURPOSE_HS_VANGUARDS` count as both a client purpose and a service
 purpose.  Won't that result in us sometimes calling
 `hs_service_circuit_timed_out` on a client circuit?
 >
 > Hmm, there is indeed a bug here! Thanks for catching that, and sorry for
 missing that.
 >
 > `CIRCUIT_PURPOSE_HS_VANGUARDS` is the purpose that Tor uses to pre-built
 vanguard circuits '''before''' they are used by the vanguard subsystem for
 specific HS-related purposes (they can be used for HS client or for HS
 service activities).
 >
 > However, while the circuit is in this vanguards purpose, it shouldn't
 have any duties as an HS circuit (i.e. be an active intro circuit) and
 hence we shouldn't call `hs_service_circuit_timed_out()` on it.

 This is a good reason to have vanguard purpose circuits count as neither
 client *nor* service HS circs. Maybe they should not even count as HS
 circs at all, for function sanity.

 > Vanguards circuits are taken into account in the
 `circuit_purpose_is_hidden_service()` function because it is used by the
 vanguard subsystem to take decision about vanguards; but for our use case
 it's safe to ignore vanguard circuits.

 I think for every HS case it is safe to ignore vanguards purpose circuits.
 They are pre-built only and not used yet.. This might be a good reason to
 have every place where vanguards calls circuit_purpose_is_hidden_service()
 *also* check explicitly for its vanguard purpose...

 > So takeaways and plans for the patch:
 > a) The `circuit_purpose_is_hidden_service()` should remain as is
 (behavior-wise) because it's used (correctly) by the vanguard subsystem.
 But IMO the name is a bit misleading because IIUC vanguard circuits don't
 have active hidden service duties, but they can be repurposed to HS circs
 in the future. Perhaps the function should have a vanguard-centric name,
 instead of an onion service-centric name.

 If we decide to leave this as including vanguards, they definitely should
 not count as hs client circuits (which the branch currently does). There
 should just be an extra check for them in this function.

 > b) Our new function `circuit_purpose_is_hs_service()` should not take
 into account vanguard circuits since we only care about active onion
 service circs. Perhaps `circuit_purpose_is_hidden_service()` can use our
 new function to avoid code dedup.
 > c) Let's ask Mike if he agrees with the above.

 Additionally, circuits can time out in
 circuit_build_times_handle_completed_hop(), where they can get marked as
 measurement circuits via circuit_change_purpose(). However, the fix for
 #29034 cause them to get removed from the hs circuit map whenever we
 change purposes. Is this enough to keep them out of the HS descriptor's
 intropoint list as well? If so, then I guess that part is handled, at
 least.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/31561#comment:15>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online