Opened 3 years ago

Closed 15 months 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 3 years ago by nickm

Keywords: 034-triage-20180328 added

comment:2 Changed 3 years ago by nickm

Keywords: 034-removed-20180328 added

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

comment:3 Changed 3 years 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 16 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 16 months ago by dgoulet

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

comment:6 Changed 16 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 16 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 16 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 16 months ago by dgoulet

Reviewer: asn

comment:10 Changed 16 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 16 months ago by asn (previous) (diff)

comment:11 Changed 16 months ago by dgoulet

Status: needs_revisionneeds_review

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

comment:12 Changed 16 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 15 months 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 15 months 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 15 months ago by nickm

Resolution: implemented
Status: merge_readyclosed

Travis seems pleased; merging this now.

Note: See TracTickets for help on using tickets.