Opened 3 years ago

Closed 3 years ago

#25824 closed enhancement (implemented)

Integrate circuit max_cell_queue_size killer with DoS heartbeats

Reported by: arma Owned by: dgoulet
Priority: Medium Milestone: Tor: 0.3.3.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-relay, tor-dos
Cc: dgoulet Actual Points:
Parent ID: Points:
Reviewer: Sponsor:


We just merged #25226 so relays can kill circuits that queue up too many cells.

But we're nervous about picking a threshold that isn't super high, since there's no feedback about whether relays are hitting this event in the wild: the logs are protocol-warn, so nobody, not us not them, will even know if it happens.

teor suggested that we could tie the circuit killing into the DoS heartbeat logs. Something like "and I killed x circuits that had too many cells queued".

This sounds to me like a great idea.

Child Tickets

Change History (7)

comment:1 Changed 3 years ago by dgoulet

Milestone: Tor: 0.3.3.x-final

Agree. I think we can get that into 033 before stable. This isn't a big patch and I can work on it right away.

comment:2 Changed 3 years ago by dgoulet

Keywords: tor-relay tor-dos added
Status: newneeds_review

This patch will now output this:

[notice] DoS mitigation since startup: 0 circuits killed with too many cells. 0 circuits rejected, 0 marked addresses. 0 connections closed. 0 single hop clients refused.

See branch: ticket25824_033_01

comment:3 Changed 3 years ago by dgoulet

Owner: set to dgoulet
Status: needs_reviewaccepted

comment:4 Changed 3 years ago by dgoulet

Status: acceptedneeds_review

comment:5 Changed 3 years ago by nickm

Status: needs_reviewneeds_revision

Quick review: this code looks okay but it makes unit tests fail:

  FAIL src/test/test_status.c:443: unexpected call to logv()
  FAIL src/test/test_status.c:343: assert(CALLED(logv) OP_EQ 5): 6 vs 5
  [log_heartbeat__not_in_consensus FAILED]
1/1076 TESTS FAILED. (31 skipped)

comment:6 Changed 3 years ago by dgoulet

I've pushed a fixup commit to be squashed. Because the DoS mitigation line is now always in the heartbeat, I had to fix the test that checks for those specific lines and format.

See branch: ticket25824_033_01 with fixup e88ffdb211

comment:7 Changed 3 years ago by nickm

Resolution: implemented
Status: needs_revisionclosed

Merged to 033 and forward!

Note: See TracTickets for help on using tickets.