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

Ticket for implementing prop305 (see #30790 (moved)).

changed milestone to %Tor: 0.4.2.x-final

Trac:
Parent Ticket: #29999 (moved)

added component::core tor/tor milestone::Tor: 0.4.2.x-final network-team-roadmap-august nickm-merge owner::dgoulet parent::29999 points::7 priority::medium prop305 resolution::implemented reviewer::asn severity::normal sponsor::27-must status::closed tor-hs tor-spec type::enhancement labels

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

For now, on going development is in ticket30924_042_01.

Trac:
Keywords: N/A deleted, network-team-roadmap-august added

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.

Trac:
Reviewer: N/A to asn
Status: assigned to needs_review

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.

Trac:
Status: needs_review to needs_revision

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

Trac:
Status: needs_revision to needs_review

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.

Trac:
Status: needs_review to needs_revision

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

Trac:
Status: needs_revision to needs_review

Some more comments there.

Trac:
Status: needs_review to needs_revision

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

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

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

Trac:
Status: needs_revision to needs_review

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

Trac:
Status: needs_review to merge_ready
Keywords: N/A deleted, nickm-merge added

Which of the branches should I merge here?

Replying to nickm:

Which of the branches should I merge here?

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

It has 2 fixups.

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

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...

CI passed; Merged to master!

Trac:
Status: merge_ready to closed
Resolution: N/A to implemented

closed

changed time estimate to 56h

mentioned in issue #31682 (moved)

mentioned in issue #32617 (moved)

mentioned in issue #29999 (moved)

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

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

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