Opened 6 months ago

Last modified 3 months ago

#32614 needs_review defect

hs-v3: Consider flagging an intro point as bad if rendezvous fails multiple times

Reported by: dgoulet Owned by: neel
Priority: Medium Milestone: Tor: 0.4.4.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-circuit, tor-hs
Cc: neel Actual Points:
Parent ID: Points: 0.2
Reviewer: dgoulet Sponsor: Sponsor27-can

Description

This comes from #25882.

Should a client claim that an IP is failing if the RP never connected? In other words, handling the CIRCUIT_PURPOSE_C_REND_READY_INTRO_ACKED purpose when cleaning up a circuit.

I would think that having a maximum amount of retries let say (arbitrarily) after 2 times that you introduce, the IP sends back the ACK but then you end up timing out the RP circuit, we would flag the IP in the failure cache and move on to a new IP.

This would not be a "catch all" issues with connectivity but would at least zone out buggy or malicious IP that do not relay the introduction.

Child Tickets

Change History (20)

comment:1 Changed 6 months ago by neel

Cc: neel added
Owner: set to neel
Status: newassigned

comment:2 Changed 6 months ago by neel

Keywords: tor-hs added; -tor-hs removed

Fix keywords.

comment:3 Changed 5 months ago by neel

Status: assignedneeds_review

PR: https://github.com/torproject/tor/pull/1601

I just handle CIRCUIT_PURPOSE_C_REND_READY_INTRO_ACKED on cleanup in here. Is that okay, or is there more I need to do?

comment:4 Changed 5 months ago by teor

Milestone: Tor: unspecifiedTor: 0.4.3.x-final

I think this PR needs a changes file?

comment:5 Changed 5 months ago by neel

I don't believe so, as this feature was introduced in the commits 7f83c43594dcf13fb04352f5faa8db2cd86354c1 and cbc495453cb522db1584525a06d26386f5819e05, and aren't introduced in a released version yet (from the information I got on GitHub).

Sources:

comment:6 Changed 5 months ago by teor

Status: needs_reviewneeds_revision

The changes file for #32020 does not mention failing rendezvous points, so we need another changelog entry for this ticket:
https://github.com/torproject/tor/blob/master/changes/ticket32020

comment:7 Changed 5 months ago by neel

Status: needs_revisionneeds_review

Made the change. Setting as needs review.

comment:8 Changed 5 months ago by dgoulet

Reviewer: dgoulet
Status: needs_reviewneeds_revision

comment:9 Changed 5 months ago by neel

Status: needs_revisionneeds_review

Made the changes. Setting as needs review.

comment:10 Changed 5 months ago by neel

Status: needs_reviewneeds_revision

comment:11 Changed 5 months ago by neel

Status: needs_revisionneeds_review

comment:12 Changed 4 months ago by neel

Old PR has merge conflicts. New PR: https://github.com/torproject/tor/pull/1653

comment:13 Changed 4 months ago by nickm

Keywords: 043-can added

tag all currently needs_review tickets with 043-can. (Since there's code before the feature freeze, maybe we can take it.)

comment:14 Changed 4 months ago by dgoulet

Keywords: 043-can removed
Milestone: Tor: 0.4.3.x-finalTor: unspecified
Status: needs_reviewneeds_revision

comment:15 Changed 4 months ago by neel

New simplified PR: https://github.com/torproject/tor/pull/1682

No tests, however.

comment:16 Changed 4 months ago by teor

Milestone: Tor: unspecifiedTor: 0.4.4.x-final
Status: needs_revisionneeds_review

comment:17 Changed 4 months ago by asn

Status: needs_reviewneeds_revision

Hmm chceked the PR and made a small comment.

I'm generally concerned here about the lack of unittests.

Also I'd appreciate a torspec branch (or a thorough post on this ticket) that analyzes the effects of this new behavior. For example, what is the old behavior vs the new behavior in the following two scenarios:

  • Intro node is broken and does not forward rend requests.
  • Service is overloaded and cannot handle rendezvous.

comment:18 Changed 3 months ago by neel

Sorry for the delay.

I have extended the ticket changes writeup in a fixup commit.

I do not have tests, I don't know of any test that calls hs_client_circuit_cleanup_on_free()

comment:19 Changed 3 months ago by neel

Also, I did not see your comment

comment:20 Changed 3 months ago by asn

Status: needs_revisionneeds_review
Note: See TracTickets for help on using tickets.