Re: [PATCH] Add rate limit support to API

On Thu, 07 Dec 2017 at 23:38:59, Florian Pritz wrote:

This allows us to prevent users from hammering the API every few seconds to check if any of their packages were updated. Real world users check as often as every 5 or 10 seconds.

First of all, thanks for the patch! Out of curiosity: is there a noticeable performance impact caused by these requests? Our is this just a safety measure for the future? I am also asking because below, your are setting the default limit to one request every ~20 seconds. Do you think a reduction by a factor of two is sufficient to mitigate the issue?

Signed-off-by: Florian Pritz ---

Basic idea for a rate limiting solution. Currently the cleanup of old entries is done on each api request. That may be a bit excessive, but I didn't find a cronjob to put it and given the table is indexed, the query is probably fast enough. At least for a PoC this should be fine. Tell me what you think.

The overall idea sounds good to me. More comments below.

[...] --- /dev/null +++ b/upgrading/4.7.0.txt

You also need to create the table in our database schema (which is located under schema/aur-schema.sql).

@@ -0,0 +1,11 @@ +1. Add ApiRateLimit table: + +--- +CREATE TABLE `ApiRateLimit` ( + `ip` varchar(45) CHARACTER SET ascii COLLATE ascii_bin NOT NULL, + `requests` int(11) NOT NULL, + `window_start` bigint(20) NOT NULL,

We use for the field names in all other tables and I would like to keep this consistent. Upper case is used for the data types.

+ PRIMARY KEY (`ip`), + KEY `window_start_idx` (`window_start`)

Same applies to the key.

+) ENGINE=InnoDB;

Space before and after the "=".

diff --git a/web/lib/aurjson.class.php b/web/lib/aurjson.class.php index 9eeaafd..8a6dd7b 100644 --- a/web/lib/aurjson.class.php +++ b/web/lib/aurjson.class.php @@ -96,6 +96,11 @@ public function handle($http_data) {

$this->dbh = DB::connect();

+ if ($this->check_ratelimit($_SERVER['REMOTE_ADDR'])) { + header("HTTP/1.1 429 Too Many Requests"); + return $this->json_error('Rate limit reached'); + } + $type = str_replace('-', '_', $http_data['type']); if ($type == 'info' && $this->version >= 5) { $type = 'multiinfo'; @@ -130,6 +135,72 @@ public function handle($http_data) { } }

+ /* + * Check if an IP needs to be rate limited. + * + * @param $ip IP of the current request + * + * @return true if IP needs to be rate limited, false otherwise. + */ + private function check_ratelimit($ip) { + $limit = config_get("ratelimit", "request_limit"); + $window_length = config_get("ratelimit", "window_length"); + $this->update_ratelimit($ip); + $stmt = $this->dbh->prepare(" + SELECT requests,window_start FROM ApiRateLimit + WHERE ip = :ip"); + $stmt->bindParam(":ip", $ip);

We usually do not use prepared statements but I think it does not hurt -- unless it breaks with SQLite.

+ $result = $stmt->execute(); + + if (!$result) { + return false; + } + + $row = $stmt->fetch(PDO::FETCH_ASSOC); + + if ($row['window_start'] < time() - $window_length) { + $stmt = $this->dbh->prepare(" + DELETE FROM ApiRateLimit + WHERE ip = :ip"); + $stmt->bindParam(":ip", $ip); + $stmt->execute(); + return false; + } + + if ($row['requests'] > $limit) { + return true; + } + return false; + } + + /* + * Update a rate limit for an IP by increasing it's requests value by one. + * + * @param $ip IP of the current request + * + * @return void + */ + private function update_ratelimit($ip) { + $window_length = config_get("ratelimit", "window_length"); + $time = time(); + $deletion_time = $time - $window_length; + $stmt = $this->dbh->prepare(" + INSERT INTO ApiRateLimit + (ip, requests, window_start) + VALUES (:ip, 0, :window_start) + ON DUPLICATE KEY UPDATE requests=requests+1");

I do not think this works with SQLite. Is there a more standardized way of doing this? Maybe "INSERT OR REPLACE"?

+ $stmt->bindParam(":ip", $ip); + $stmt->bindParam(":window_start", $time); + $stmt->execute(); + + // TODO: this should move into some cronjob + $stmt = $this->dbh->prepare(" + DELETE FROM ApiRateLimit + WHERE window_start < :time"); + $stmt->bindParam(":time", $deletion_time); + $stmt->execute();

I am quite confused. There is a similar piece of code in check_ratelimit() already. Is either of these statements a leftover or do they serve different purposes? I am also curious about the general performance impact of this patch. It reduces request flooding but at the same time adds a couple of additional database queries to each request. Do you happen to have any statistics?

+ } + /* * Returns a JSON formatted error string. * -- 2.15.1