hs-v3: Service can keep unused intro points in its list

Tor always selects an extra number of intro points in addition to the configured HiddenServiceNumIntroductionPoints.

It launches all of them and the first NumIntro... to finish are used (considering #31548 (moved) is resolved).

Once the circuit of the remaining intro opens, we notice that we have too many and then re-purpose the extra ones.

However, I've noticed that sometimes establishing an intro circuit timeouts during build, basically expiring due to our CBT policy. In that case, the circuit is simply close but the intro point remains in the service descriptor list.

This is bad because of #31548 (moved), this means an intro point can end up in the descriptor even though the service never established any circuits to it...

We simply need to callback into the HS subsystem when we are expiring an HS circuit so appropriate actions can be taken such as in this case, removing the IP from the list.

changed milestone to %Tor: unspecified

Trac:
Parent Ticket: #30200 (moved)

added actualpoints::0.1 component::core tor/tor hv-v3 milestone::Tor: unspecified owner::dgoulet parent::30200 points::0.2 priority::medium resolution::fixed reviewer::asn reviewer::mikeperry severity::normal sponsor::27-must status::closed tor-hs type::defect labels

PR: https://github.com/torproject/tor/pull/1275 Branch: ticket31561_042_01

No backport since #31548 (moved) mitigates this issue.

Trac:
Actualpoints: N/A to 0.1

Trac:
Status: assigned to needs_review

Looks reasonable, but I left a few questions and suggestions in the PR!

Trac:
Status: needs_review to needs_revision

Back in needs_review. Answered the PR. One bit I did not change, let me know if you agree or not.

Trac:
Status: needs_revision to needs_review

A few more corrections are needed I believe.

Also, would it be possible to write a small unittest for hs_service_circuit_timed_out() just to make sure that the codeflow works properly?

Trac:
Status: needs_review to needs_revision

All fixed. Pushed unit test in commit e55235f72f. Sorry about that, I had one but it was stuck in the stash :(.

Trac:
Status: needs_revision to needs_review

Seems like the circ timeout unittest failed in clang. I also added a minor issue as a comment. After this we are all good for merge.

Trac:
Status: needs_review to needs_revision

Mark some needs_revision tickets as 042-should

Trac:
Keywords: tor-hs hv-v3 deleted, 042-should, hv-v3, tor-hs added

Trac:
Status: needs_revision to needs_review

Great! Just one small discussion item remaining about the vanguard thing. Marking as needs_revision so that you reply/address the topic.

Trac:
Status: needs_review to needs_revision

Fixed.

Trac:
Status: needs_revision to needs_review

LGTM! Thanks!

Trac:
Status: needs_review to merge_ready

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?

Replying to 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.

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.

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

Trac:
Cc: N/A to mikeperry
Status: merge_ready to needs_revision

Replying to asn:

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

Thanks Mike!!!

I'm passing this review on to you since asn is away and you have a good grasp on this problem.

I basically removed the HS_VANGUARD purpose from the new functions and put it back in the is_hidden_service() like it is today.

New PR with branch rebased/squashed on latest 042: https://github.com/torproject/tor/pull/1411 Branch: ticket31561_042_02

Trac:
Status: needs_revision to needs_review
Reviewer: asn to asn, mikeperry

As per discussion on IRC, Mike spotted that circuit_build_times_handle_completed_hop() also can timeout a circuit.

Which means, it would go through circuit_change_purpose() and the intro point would get retried instead of ditched.

In other words, we have two different behaviors with this patch with regards to the intro circuit:

1. Time out: intro point is ditched, a new one is picked later.
2. Closes/Re-purposed: intro point is set as "not established" and then will be retried later.

The change in behavior with this ticket is (1) which makes the service ditch an intro point on timeout issues instead of retrying it later.

I'll delay this one until asn comes back for more discussion.

Trac:
Status: needs_review to needs_information

A related question to comment:17: Why do we try hard to hold on to specific intropoints at all? If their circuit collapses, fails, or stops carrying data, why do we try to build a new circuit to the old intropoint as opposed to just picking a whole new intropoint?

Replying to dgoulet:

As per discussion on IRC, Mike spotted that circuit_build_times_handle_completed_hop() also can timeout a circuit.

Which means, it would go through circuit_change_purpose() and the intro point would get retried instead of ditched.

In other words, we have two different behaviors with this patch with regards to the intro circuit:

1. Time out: intro point is ditched, a new one is picked later.
2. Closes/Re-purposed: intro point is set as "not established" and then will be retried later.

I am a bit confused by the above because circuit_build_times_handle_completed_hop() is also about "time out" and above you say that we retry instead of ditching, but (1) above says the opposite. What is the case?

Maybe you want to distinguish between timeout while building a circuit, and timeout on a completed circuit? Or maybe I'm a bit confuse.

The change in behavior with this ticket is (1) which makes the service ditch an intro point on timeout issues instead of retrying it later.

Hmm, I don't mind this behavior that much as long as we document it somewhere and we are OK with it. It does seem a bit arbitrary to me, because if we want to keep our intro points, then we should do it in this case too. Would it be hard to do?

Replying to mikeperry:

A related question to comment:17: Why do we try hard to hold on to specific intropoints at all? If their circuit collapses, fails, or stops carrying data, why do we try to build a new circuit to the old intropoint as opposed to just picking a whole new intropoint?

Some history here at #8239 (moved).

After discussing on IRC with asn, we decided to put this on hold until we get a consensus on the retry vs ditch behavior.

I've just written a tor-dev@ thread that tries to summarize it all but it needs input from mikeperry on the "ditch argument". Until then, this ticket goes into dormancy.

https://lists.torproject.org/pipermail/tor-dev/2019-October/014073.html

Trac:
Keywords: 042-should deleted, N/A added
Milestone: Tor: 0.4.2.x-final to Tor: unspecified

Discussed with asn and based on what we have, IP retry is the way to go.

Trac:
Status: needs_information to needs_revision

Need for #32020 (moved) to be merged upstream before I can fix this.

Ok good news. We don't need new code here. This has been fixed through #32094 (moved) and #32020 (moved)

Short summary is that:

1. #32020 (moved): Added the code to remove a circuit from the HS circuitmap on close, free and repurpose.

2. #32094 (moved): Made it that we only use the HS circuitmap when launching introduction points so the launch_intro_point_circuits() actually notices now that the IP that timed out has not more circuit and thus a retry is done up to MAX_INTRO_POINT_CIRCUIT_RETRIES = 3.

Combining both, this means that cleanup_intro_points() will remove the IP after 3 fail retries and thus clean it up properly the service list.

This is bad because of #31548 (moved), this means an intro point can end up in the descriptor even though the service never established any circuits to it...

This is not the case now since we use the HS circuitmap to learn if the circuit is established or not.

We are done here. Resolving as "fixed" even though this ticket resulted in no actual patches applied to master.

Trac:
Status: needs_revision to closed
Resolution: N/A to fixed

closed

changed time estimate to 1h 36m

added 48m of time spent

mentioned in issue #30200 (moved)

moved to tpo/core/tor#31561 (closed)