hsv3: Client do not report failing circuit back into HS subsystem

This is a subtask of the bigger larger problem in #25882 (moved).

A v2 client does report intro point failures within circuit_about_to_free() but not v3.

Actually, any HS circuit client side is not looked at. The hs_circ_cleanup() was intended for this as the entry point in the HS subsystem but only the service uses it.

Intro circuit failure needs to be noted in the failure cache (hs_cache_client_intro_state_note()).

Rendezvous circuit need to be also somehow handled. If the RP circuit keeps closing on us, we might want to stop trying maybe?

Same goes for HSDir circuit, if they close, client needs to be notified and launch a refetch.

changed milestone to %Tor: 0.4.3.x-final

Trac:
Parent Ticket: #30200 (moved)

added actualpoints::1 component::core tor/tor milestone::Tor: 0.4.3.x-final owner::dgoulet parent::30200 points::1 priority::medium resolution::fixed reviewer::asn severity::normal sponsor::27-must status::closed tor-client tor-hs type::defect labels

In the origin_circuit_t struct, is struct hs_ident_circuit_t *hs_ident; to version 3 what rend_data_t *rend_data; is to version 2?

Trac:
Owner: N/A to dgoulet
Status: new to assigned

Related is #26806 (moved) which mentions that possibly because the HSv3 client is not noticing the introduction timeout (as in the ACK never came back), we resend onto that same intro point. Good or bad?

Branch: ticket32020_043_01 PR: https://github.com/torproject/tor/pull/1490

@asn: The initial commits are mostly moving and refactoring code on how we handle the circuit cleanup. It is not that simple so let me know if the approach is sensible. But overall, there needs to be nuanced cleanup between close/free/repurpose.

Also, out of this work, I found #32349 (moved) in v2.

Trac:
Cc: asn to N/A
Status: assigned to needs_review
Reviewer: N/A to asn
Actualpoints: N/A to 1

Replying to dgoulet:

Related is #26806 (moved) which mentions that possibly because the HSv3 client is not noticing the introduction timeout (as in the ACK never came back), we resend onto that same intro point. Good or bad?

Hmm, questions and answers:

1. Why doesn't the ACK or NACK come to the client? Is it because the intro point never sent it (why?)? Or because we timeout before receiving it? Or just general Tor network SNAFU?

2. If the above happens, why would the client decide to resend on the same intro point and same circuit? Is this an explicit decision?

3. Regarding "Good or bad?" I would say it's bad-ish because if the NACK never came back, I would prefer to retry a different intro point since that one might be suffering networking issues, or being overloaded, or downright maliciously DoSing the service.

PS: #26806 (moved) mentions "rendezvous circuits" in the title, but I think it should be intro circuits

Replying to asn:

Replying to dgoulet:

Related is #26806 (moved) which mentions that possibly because the HSv3 client is not noticing the introduction timeout (as in the ACK never came back), we resend onto that same intro point. Good or bad?

Hmm, questions and answers:

1. Why doesn't the ACK or NACK come to the client? Is it because the intro point never sent it (why?)? Or because we timeout before receiving it? Or just general Tor network SNAFU?

SNAFU is probably the answer. Circuit collapsing, timing out, etc...

2. If the above happens, why would the client decide to resend on the same intro point and same circuit? Is this an explicit decision?

It doesn't in theory. Depending on the SNAFU (see patch I did), we either flag the intro point in the failure cache (see patch I did) or we go on with our lives maybe retrying a new one.

3. Regarding "Good or bad?" I would say it's bad-ish because if the NACK never came back, I would prefer to retry a different intro point since that one might be suffering networking issues, or being overloaded, or downright maliciously DoSing the service.

Yes, in theory, that is what is suppose to happen. The patch I did would fix this that is note down the intro point in the failure cache.

PS: #26806 (moved) mentions "rendezvous circuits" in the title, but I think it should be intro circuits

Yes.

Great branch! Review submitted!

Trac:
Status: needs_review to needs_revision

Replying to asn:

Great branch! Review submitted!

I replied. Would be great to get your feedback on my answers before I jump in and provide a new version of the branch. Thanks!!

Trac:
Status: needs_revision to needs_review

Replied!

Trac:
Status: needs_review to needs_revision

As discussed with asn on IRC, new PR/branch since much have changed from asn's review.

Branch: ticket32020_043_02 PR: https://github.com/torproject/tor/pull/1554

Trac:
Status: needs_revision to needs_review

Looks really good. Just a bit more of nitpicking and we are done.

Trac:
Status: needs_review to needs_revision

Done. Hope this works out :)

Trac:
Status: needs_revision to needs_review

OK. Please find latest PR here: https://github.com/torproject/tor/pull/1573

It squashed the latest branch, removed the reverted commits, fixed a typo (wrods -> words), and improved a bit the changes file (to clarify that the reachability delta is positive).

I made a PR just to make sure that CI passes. I will merge when it finishes.

Merged! Thanks!

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

see #32847 (moved) for a new bug that might have been introduced by this fix.

closed

changed time estimate to 8h

added 8h of time spent

mentioned in issue #32349 (moved)

mentioned in issue #32614 (moved)

mentioned in issue #32847 (moved)

mentioned in issue #30200 (moved)

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

mentioned in issue tpo/core/tor#32349 (closed)