Opened 4 months ago

Closed 5 weeks ago

#30924 closed enhancement (implemented)

hs-v3: Implement proposal 305 - ESTABLISH_INTRO Cell DoS Defense Extension

Reported by: dgoulet Owned by: dgoulet
Priority: Medium Milestone: Tor: 0.4.2.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-hs, tor-spec, prop305, network-team-roadmap-august, nickm-merge
Cc: Actual Points:
Parent ID: #29999 Points: 7
Reviewer: asn Sponsor: Sponsor27-must

Description

Ticket for implementing prop305 (see #30790).

Child Tickets

Change History (18)

comment:1 Changed 4 months ago by dgoulet

For this to be completed, it requires #15516 to be merged. There is quite a cross-over.

For now, on going development is in ticket30924_042_01.

comment:2 Changed 3 months ago by gaba

Keywords: network-team-roadmap-august added

comment:3 Changed 2 months ago by dgoulet

Reviewer: asn
Status: assignedneeds_review

Branch: ticket30924_042_01
PR: https://github.com/torproject/tor/pull/1232

This is working well with chutney. On today git master, the HS won't send the extension since no relay supports it.

The service side only honors the torrc options, it does NOT look at the consensus parameters. The intro point is the one looking at those if the cell extension is not seen.

comment:4 in reply to:  3 Changed 2 months ago by asn

Replying to dgoulet:

Branch: ticket30924_042_01
PR: https://github.com/torproject/tor/pull/1232

This is working well with chutney. On today git master, the HS won't send the extension since no relay supports it.

The service side only honors the torrc options, it does NOT look at the consensus parameters. The intro point is the one looking at those if the cell extension is not seen.

Did an initial high-level review of the code. I did not actually look at the nitty-gritty details (e.g. low-level parsing/encoding) yet. I'll also wait for the tor-dev thread with the intro point behavior.

comment:5 Changed 2 months ago by asn

Status: needs_reviewneeds_revision

comment:6 Changed 2 months ago by dgoulet

Status: needs_revisionneeds_review

Addressed most of the comments. I rebased/squashed the fixes and forced push (upon agreement with asn).

comment:7 Changed 2 months ago by asn

Status: needs_reviewneeds_revision

OK I did a second round of review and testing. No major bug found but maybe some weirdness on the intro-side checks, and also some abnoxious comments about (hopefully) reducing future tech-debt. Let me know if you want help with these.

Also, CI seems to be unhappy about the branches but maybe that's just practracker.

comment:8 Changed 8 weeks ago by dgoulet

Status: needs_revisionneeds_review

I've revised everything (I hope!).

Pushed everything. I added a practracker commit to make it happy but changes are there will be conflict with upstream merge so #yolo :).

Per asn's request, new PR rebased on master due to conflicts: ticket30924_042_02
PR: https://github.com/torproject/tor/pull/1243

Last edited 8 weeks ago by dgoulet (previous) (diff)

comment:9 Changed 8 weeks ago by asn

Status: needs_reviewneeds_revision

Some more comments there.

comment:10 Changed 8 weeks ago by dgoulet

asn will try to fix the MIN/MAX check madness with gcc. Rebased/squashed/reviewed branch is now here: ticket30924_042_03

comment:11 in reply to:  10 Changed 8 weeks ago by teor

Replying to dgoulet:

asn will try to fix the MIN/MAX check madness with gcc. Rebased/squashed/reviewed branch is now here: ticket30924_042_03

Here's one option:

/* gcc complains when MIN is 0, because this statement is always true */
#if MIN
do check with min &&
#endif
other checks

comment:12 Changed 7 weeks ago by asn

Status: needs_revisionneeds_review

Thanks teor. That worked for me: https://github.com/torproject/tor/pull/1262

comment:13 Changed 7 weeks ago by dgoulet

Keywords: nickm-merge added
Status: needs_reviewmerge_ready

Looking good! Travis is already giving us green, should be done soon.

comment:14 Changed 7 weeks ago by nickm

Which of the branches should I merge here?

comment:15 in reply to:  14 Changed 7 weeks ago by dgoulet

Replying to nickm:

Which of the branches should I merge here?

https://github.com/torproject/tor/pull/1262

It has 2 fixups.

comment:16 Changed 5 weeks ago by nickm

Okey, I've been through the branch and it looks plausible. I get conflicts when I try to squash the fixup commits, though. I've tried to resolve them in a way that looked plausible to me, but next time, it might be better to make sure that there is a final branch when we get the ticket to merge_ready.

I'm going to make sure that CI passes on my squashed-and-merged branch before I merge. I've got it at https://github.com/torproject/tor/pull/1304

comment:17 in reply to:  16 Changed 5 weeks ago by dgoulet

Replying to nickm:

Okey, I've been through the branch and it looks plausible. I get conflicts when I try to squash the fixup commits, though. I've tried to resolve them in a way that looked plausible to me, but next time, it might be better to make sure that there is a final branch when we get the ticket to merge_ready.

Sorry about that...

I'm going to make sure that CI passes on my squashed-and-merged branch before I merge. I've got it at https://github.com/torproject/tor/pull/1304

This lgtm. CI feeding...

comment:18 Changed 5 weeks ago by nickm

Resolution: implemented
Status: merge_readyclosed

CI passed; Merged to master!

Note: See TracTickets for help on using tickets.