1393643 - Add whitelist to rate limiting code

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: General, enhancement)

Description

[Dylan Hardison [:dylan] (he/him)]

We should be able to whitelist an arbitrary number of ips from being rate limited.

Comment 1

[Dylan Hardison [:dylan] (he/him)]

Attachment: PR

Comment 2

[:Atoll]

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'}));

Comment 3

[:Atoll]

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.

Comment 4

[Dylan Hardison [:dylan] (he/him)]

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)

Comment 5

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8901012 [details] [diff] [review]
PR

r=atoll, r=mary