Opened 20 months ago

Closed 8 weeks ago

#24963 closed enhancement (implemented)

dos: Block single hop clients at the intro point

Reported by: dgoulet Owned by: dgoulet
Priority: Medium Milestone: Tor: 0.4.2.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-dos, tor2web, tor-hs, 034-triage-20180328, 034-removed-20180328
Cc: nickm-merge Actual Points: 0.1
Parent ID: #24962 Points: 0.1
Reviewer: asn Sponsor: Sponsor27-must

Description (last modified by dgoulet)

This is so we remove load from spammy single hop clients at the Intro point.

Opened after discussion in #24902.

Lets consider this also as a possible backport.

Child Tickets

Change History (15)

comment:1 Changed 18 months ago by nickm

Keywords: 034-triage-20180328 added

comment:2 Changed 18 months ago by nickm

Keywords: 034-removed-20180328 added

Per our triage process, these tickets are pending removal from 0.3.4.

comment:3 Changed 18 months ago by nickm

Milestone: Tor: 0.3.4.x-finalTor: unspecified

These tickets, tagged with 034-removed-*, are no longer in-scope for 0.3.4. We can reconsider any of them, if time permits.

comment:4 Changed 3 months ago by dgoulet

Description: modified (diff)
Owner: set to dgoulet
Status: newaccepted
Summary: dos: Add an option to block tor2web requests at the intro pointdos: Block single hop clients at the intro point

comment:5 Changed 3 months ago by dgoulet

Milestone: Tor: unspecifiedTor: 0.4.2.x-final
Points: 0.1
Sponsor: Sponsor27-must

comment:6 Changed 3 months ago by dgoulet

Actual Points: 0.1
Status: acceptedneeds_review

Branch: ticket24963_042_01
PR: https://github.com/torproject/tor/pull/1121

comment:7 Changed 3 months ago by teor

Status: needs_reviewneeds_revision

I have the same question as:
https://trac.torproject.org/projects/tor/ticket/24964#comment:6

If we're going to do this check in multiple places, we probably need a channel_is_known_relay() function.

comment:8 in reply to:  7 Changed 3 months ago by dgoulet

Status: needs_revisionneeds_review

Replying to teor:

I have the same question as:
https://trac.torproject.org/projects/tor/ticket/24964#comment:6

If we're going to do this check in multiple places, we probably need a channel_is_known_relay() function.

Had a talk with teor on IRC. There was some confusion about this being applied to ESTABLISH_INTRO (service side) but this is only on the client side, the INTRODUCE1 cell requirement which we agree is what we want here.

comment:9 Changed 3 months ago by dgoulet

Reviewer: asn

comment:10 Changed 3 months ago by asn

Status: needs_reviewneeds_revision

Code looks good to me. Same as in #24694 I would really appreciate some real life testing of this feature on chutney or the real net, to make sure that legit clients can get through.

Also travis seems broken.

Last edited 3 months ago by asn (previous) (diff)

comment:11 Changed 3 months ago by dgoulet

Status: needs_revisionneeds_review

Unit tests fixed and added one also to actually test the feature ;).

comment:12 Changed 2 months ago by asn

Status: needs_reviewmerge_ready

LGTM. According to IRC this has also been tested on chutney which resolves my testing concerns from comment:10.

comment:13 Changed 8 weeks ago by nickm

Status: merge_readyneeds_review

The testing fix here is a bit ugly; it's best not to make fake objects and bypass the channel_new() interface.

Instead, I suggest a change to hs_intropoint.c. What do you think of the branch ticket24963_042_02 with PR at https://github.com/torproject/tor/pull/1188 ?

comment:14 in reply to:  13 Changed 8 weeks ago by dgoulet

Cc: nickm-merge added
Status: needs_reviewmerge_ready

Replying to nickm:

The testing fix here is a bit ugly; it's best not to make fake objects and bypass the channel_new() interface.

Instead, I suggest a change to hs_intropoint.c. What do you think of the branch ticket24963_042_02 with PR at https://github.com/torproject/tor/pull/1188 ?

Indeed much simpler! Lets go with this! Nick I think you can just go ahead and merge this :). Travis is still feeding...

comment:15 Changed 8 weeks ago by nickm

Resolution: implemented
Status: merge_readyclosed

Travis seems pleased; merging this now.

Note: See TracTickets for help on using tickets.