Closed
Bug 1393643
Opened 4 years ago
Closed 4 years ago
Add whitelist to rate limiting code
Categories
(bugzilla.mozilla.org :: General, enhancement)
Tracking
()
RESOLVED
FIXED
People
(Reporter: dylan, Assigned: dylan)
References
Details
Attachments
(1 file)
45 bytes,
patch
|
dylan
:
review+
|
Details | Diff | Splinter Review |
We should be able to whitelist an arbitrary number of ips from being rate limited.
Assignee | ||
Comment 1•4 years ago
|
||
Attachment #8901012 -
Flags: review?(rsoderberg)
Comment on attachment 8901012 [details] [diff] [review] PR Review of attachment 8901012 [details] [diff] [review]: ----------------------------------------------------------------- ::: Bugzilla.pm @@ +776,5 @@ > return 0; > } > + if (Bugzilla->memcached->should_rate_limit("$name:$ip", @$limit)) { > + my $action = 'block'; > + my $filter = Bugzilla::Bloomfilter->load("rate_limit_whitelist"); If we're blocking an attacker, we can't afford to load *from disk* the whitelist on each attacker request. How is this result cached? ::: Bugzilla/Bloomfilter.pm @@ +16,5 @@ > +use File::Temp qw(tempfile); > + > +sub _new_bloom_filter { > + my ($n) = @_; > + my $p = 0.01; I assume this numeric constant is well known to people who write bloom filters :) @@ +34,5 @@ > + my ($class, $name, $items) = @_; > + > + my $filter = _new_bloom_filter(@$items + 0); > + foreach my $item (@$items) { > + $filter->add($item); $filter->add(@$items); void add(bloom_t *bl, ...) PPCODE: for (i = 1; i < items; ++i) { value = ST(i); str = (const unsigned char *)SvPVbyte(value, len); bl_add(bl, str, len); @@ +37,5 @@ > + foreach my $item (@$items) { > + $filter->add($item); > + } > + > + my ($fh, $filename) = tempfile( "${name}XXXXXX", DIR => bz_locations->{datadir}, UNLINK => 0); I'm slightly surprised to see that there isn't a helper function from Bugzilla:: for getting safe tempfile paths, and/or that function would be useful here. @@ +38,5 @@ > + $filter->add($item); > + } > + > + my ($fh, $filename) = tempfile( "${name}XXXXXX", DIR => bz_locations->{datadir}, UNLINK => 0); > + binmode $fh, ':bytes'; "The file returned by File::Temp will have been opened in binary mode if such a mode is available." - does that save you a call here? Is any of this high-performance code or one-time startup / config reload penalty stuff? @@ +56,5 @@ > + my $s = <$fh>; > + close $fh; > + > + my $filter = Algorithm::BloomFilter->deserialize($s); > + return $filter; File::Slurp is handy for this, but not necessary or anything. return Algorithm::BloomFilter->deserialize(read_file($filename, {binmode => ':bytes'}));
Attachment #8901012 -
Attachment is patch: true
Attachment #8901012 -
Attachment mime type: text/x-github-pull-request → text/plain
Comment on attachment 8901012 [details] [diff] [review] PR (In reply to Richard Soderberg [:atoll] from comment #2) > Comment on attachment 8901012 [details] [diff] [review] > ::: Bugzilla.pm > @@ +776,5 @@ > > return 0; > > } > > + if (Bugzilla->memcached->should_rate_limit("$name:$ip", @$limit)) { > > + my $action = 'block'; > > + my $filter = Bugzilla::Bloomfilter->load("rate_limit_whitelist"); > > If we're blocking an attacker, we can't afford to load *from disk* the > whitelist on each attacker request. How is this result cached? r+ *except* for this one issue. If there's an urgent matter between now and morning, and you either accept this as a weakness - or disprove it through measurement - then you have my tacit r+ to proceed as long as you document here. Also, why didn't Bugzilla show me this was a GitHub post. Sigh.
Attachment #8901012 -
Flags: review?(rsoderberg) → review-
Assignee | ||
Comment 4•4 years ago
|
||
Comment on attachment 8901012 [details] [diff] [review] PR Now it hits memcached most of the time (obviously, it will hit the FS if memcached is running low on space, etc)
Attachment #8901012 -
Flags: review- → review?(rsoderberg)
Assignee | ||
Comment 5•4 years ago
|
||
Comment on attachment 8901012 [details] [diff] [review] PR r=atoll, r=mary
Attachment #8901012 -
Flags: review?(rsoderberg) → review+
Assignee | ||
Updated•4 years ago
|
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•