1355169 - Add rate-limiting to show_bug.cgi and rest.cgi

Status: RESOLVED FIXED

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

Description

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

Attachment: show-bug-rate-limit.patch

This rate limits show_bug for non-logged-in users to 75 views over 60 seconds.

This rate was chosen because it is a limit never reached except by agents that are subsequently blocked.

This condition is also logged -- and if it begins happening to legitimate users,
we can increase it after we increase the number of workers or decrease the amount of time spent serving a request.

Comment 1

[:glob ✱]

Comment on attachment 8856616 [details] [diff] [review]
show-bug-rate-limit.patch

Review of attachment 8856616 [details] [diff] [review]:
-----------------------------------------------------------------

::: show_bug.cgi
@@ +30,5 @@
>  
> +unless ($user->id) {
> +    my $ip = remote_ip();
> +    my $bug_rate   = 75;
> +    my $bug_period = 60;

please make bug_rate an admin param.
that will make it easier to adjust the values or disable if required.

@@ +35,5 @@
> +
> +    my $should_block = Bugzilla->memcached->should_rate_limit("show-bug:$ip", $bug_rate, $bug_period);
> +    if ($should_block) {
> +        Bugzilla->audit("[too_many_bugs] $ip viewed more than $bug_rate bugs in $bug_period seconds");
> +        ThrowUserError("too_many_bugs");

too_many_bugs isn't that descriptive.  too_many_requests would be clearer.

Comment 2

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

Comment on attachment 8856616 [details] [diff] [review]
show-bug-rate-limit.patch

I'd like to reconsider the requirement of making this configurable in the short term. We can't scale rapidly right now anyway, and loading this as a preference will incur a cost that means that we're spending a non-trivial amount of time saying "no" to the client.

Comment 3

[:glob ✱]

(In reply to Dylan Hardison [:dylan] (he/him) from comment #2)
> Comment on attachment 8856616 [details] [diff] [review]
> show-bug-rate-limit.patch
> 
> I'd like to reconsider the requirement of making this configurable in the
> short term. We can't scale rapidly right now anyway, and loading this as a
> preference will incur a cost that means that we're spending a non-trivial
> amount of time saying "no" to the client.

you won't improve performance by making this not configurable as this check is happening after Bugzilla->login - the params file is already loaded to get user_info_class and user_verify_class.

i think it would be better to remove bug 1355463 from the blocking list and add the param.

Comment 4

[:Atoll]

Comment on attachment 8856616 [details] [diff] [review]
show-bug-rate-limit.patch

Review of attachment 8856616 [details] [diff] [review]:
-----------------------------------------------------------------

::: show_bug.cgi
@@ +32,5 @@
> +    my $ip = remote_ip();
> +    my $bug_rate   = 75;
> +    my $bug_period = 60;
> +
> +    my $should_block = Bugzilla->memcached->should_rate_limit("show-bug:$ip", $bug_rate, $bug_period);

Over an average business day, how many additional integer-increment memcached keys do you expect this to create? Are IPv6 addresses possible here?

Comment 5

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

Attachment: 1355169_1.patch

Comment 6

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

Attachment: 1355169_2.patch

+ audit log code and a few minor nits I noticed.

Comment 7

[:glob ✱]

Comment on attachment 8864707 [details] [diff] [review]
1355169_2.patch

Review of attachment 8864707 [details] [diff] [review]:
-----------------------------------------------------------------

i see you've expanded the scope of this to include api requests (good!).

did you rerun your analysis that determined 75req/min to see if there's legitimate requests that exceed this limit?
it's more likely that our automation is using an api to fetch bugs, so this limit may be too low.

::: Bugzilla.pm
@@ +763,5 @@
> +sub check_rate_limit {
> +    my ($class, $name, $id) = @_;
> +    my $params = Bugzilla->params;
> +    if ($params->{rate_limit_active}) {
> +        my $rules = decode_json($params->{rate_limit_rules});

performing a json decode for every check is a waste of compute.
(looks at code)
oh, Bugzilla->params are stored in the request cache not the process cache, so this decode only happens once.

when the caching work happens it might be valuable to add a post-read processing step to allow for strings like this to be morphed into objects.

@@ +764,5 @@
> +    my ($class, $name, $id) = @_;
> +    my $params = Bugzilla->params;
> +    if ($params->{rate_limit_active}) {
> +        my $rules = decode_json($params->{rate_limit_rules});
> +        my $limit = $rules->{$name} // [75, 60];

the default value should be set using the param's "default" value, not hard coded here.

@@ +766,5 @@
> +    if ($params->{rate_limit_active}) {
> +        my $rules = decode_json($params->{rate_limit_rules});
> +        my $limit = $rules->{$name} // [75, 60];
> +        if (Bugzilla->memcached->should_rate_limit("$name:$id", @$limit)) {
> +            Bugzilla->audit("[rate_limit] $id exceeds rate limit $name: " . join("/", @$limit));

for auditing purposes the ip address should always be logged, even when $id isn't remote_ip().
yes, currently $id is always remote_ip(), however that it's a param hints that in future it may not be.

also for clarity i suggest adding units to the rate.. $limit->[0] . 'requests/' . $limit->[1] . 'seconds'

::: Bugzilla/Config/Admin.pm
@@ +54,5 @@
> +
> +  {
> +    name => 'rate_limit_rules',
> +    type => 'l',
> +    default => '{}',

as per my comment below, the default value here should be the actual default..
'{ "get_bug": [75, 60], "show_bug": [75, 60] }'

@@ +74,5 @@
> +    return "failed to parse json" unless defined $val;
> +    return "value is not HASH"    unless ref $val && ref($val) eq 'HASH';
> +    return "rules are invalid"    unless all {
> +        ref($_) eq 'ARRAY' && looks_like_number( $_->[0] ) && looks_like_number( $_->[1] )
> +    } values %$val;

add a validation of the keys; both 'get_bug' and 'show_bug' should exist.

::: template/en/default/admin/params/admin.html.tmpl
@@ +23,5 @@
>     desc = "Set up account policies"
>  %]
>  
> +[% rate_limit_rules_desc = BLOCK %]
> +This parameter is a json object that allows for overriding the default rate limit.

s/json object/json string/

Comment 8

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

Attachment: PR