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

RESOLVED FIXED

Status

()

enhancement
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: dylan, Assigned: umohm12)

Tracking

(Depends on 1 bug)

Production

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Posted patch show-bug-rate-limit.patch (obsolete) — Splinter Review
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.
Attachment #8856616 - Flags: review?(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.
Attachment #8856616 - Flags: review?(glob) → review-
(Reporter)

Updated

2 years ago
Depends on: 1355463
(Reporter)

Comment 2

2 years ago
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.
(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.
(Reporter)

Updated

2 years ago
Priority: P3 → P1
Summary: Add rate-limiting to show_bug.cgi → Add rate-limiting to show_bug.cgi and rest.cgi
See Also: → 1361908

Comment 4

2 years ago
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?
(Reporter)

Updated

2 years ago
No longer depends on: 1362198
See Also: → 1362198
(Reporter)

Comment 5

2 years ago
Posted patch 1355169_1.patch (obsolete) — Splinter Review
Attachment #8856616 - Attachment is obsolete: true
Attachment #8864706 - Flags: review?(glob)
(Reporter)

Comment 6

2 years ago
Posted patch 1355169_2.patch (obsolete) — Splinter Review
+ audit log code and a few minor nits I noticed.
Attachment #8864706 - Attachment is obsolete: true
Attachment #8864706 - Flags: review?(glob)
Attachment #8864707 - Flags: review?(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/
Attachment #8864707 - Flags: review?(glob) → review-
(Reporter)

Comment 8

2 years ago
Posted file PR
Attachment #8864707 - Attachment is obsolete: true
Attachment #8880389 - Flags: review?(dylan)
(Reporter)

Updated

2 years ago
Assignee: dylan → umohm12
(Reporter)

Updated

2 years ago
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
(Reporter)

Updated

2 years ago
Attachment #8880389 - Flags: review?(dylan) → review+
You need to log in before you can comment on or make changes to this bug.