Closed
Bug 1393643
Opened 8 years ago
Closed 8 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•8 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•8 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•8 years ago
|
||
Comment on attachment 8901012 [details] [diff] [review]
PR
r=atoll, r=mary
Attachment #8901012 -
Flags: review?(rsoderberg) → review+
Assignee | ||
Updated•8 years ago
|
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•