#24902: Denial of Service mitigation subsystem -------------------------------------------------+------------------------- Reporter: dgoulet | Owner: dgoulet Type: enhancement | Status: | needs_review Priority: Very High | Milestone: Tor: | 0.3.3.x-final Component: Core Tor/Tor | Version: Severity: Normal | Resolution: Keywords: ddos, tor-relay, review-group-30, | Actual Points: 029-backport, 031-backport, 032-backport, | review-group-31 | Parent ID: | Points: Reviewer: arma | Sponsor: -------------------------------------------------+------------------------- Changes (by dgoulet):
* reviewer: => arma Comment: First, I had to do a fixup unrelated to the review: `9d2699cad0` Replying to [comment:43 arma]: > Should we use END_CIRC_REASON_RESOURCELIMIT instead? Good point. Fixup commit `2d9b285f98`. > But it looks like the call to dos_should_refuse_single_hop_client() doesn't care whether public_server_mode(). Agree. Fixup commit: `ab7b9581f3` > get_circuit_rate_per_second() still isn't doing what I think you wanted. Yah so thanks to Hello71 on IRC, I realized that there was an issue indeed and all your "Edit:" were indeed discussed and proposed by Hello71 :). I kind of think dropping the "tenths" would be good because, as you said, at best we can fill the bucket every second (approx_time()). So everything is rounded off to the second anyway. And a considerable time gap between CREATE cell will make a small difference which probably won't matter at that point. Thus, going to a number of circuit per second instead of "tenths" seems the improvement to do. What about this commit? `2106a5eaa8` > (a) should be "Number of allocated circuits remaining for this address", i.e. it's not a rate, it's a size. Fixup commit: `c7099765b4` > (b) What's this about "plus a bit of random"? :) Don't know what you are talking about, I can't find this string in the code :S ... > In the future, I plan to advocate for merging dos_cc_new_create_cell() and dos_cc_get_defense_type() into a single function, which notes the existence of the new create cell and also tells us whether to apply a defense. And I plan to advocate for a second cc defense, which returns DOS_CC_DEFENSE_REFUSE_CELL simply when stats->cc_stats.circuit_bucket == 0, without any marking or checking of stats->concurrent_count. I think I will want to instrument a real relay to see how often it would trigger that new defense, though, and I am happy to delay my future plans so we can get this patch out the door. Agree on both! Once we get this merged, lets open tickets for that! Final fixup on the man page (thanks to Hello71!): `1b8835fccd` -- Ticket URL: <https://trac.torproject.org/projects/tor/ticket/24902#comment:48> Tor Bug Tracker & Wiki <https://trac.torproject.org/> The Tor Project: anonymity online
_______________________________________________ tor-bugs mailing list tor-bugs@lists.torproject.org https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs