Add rate limit support to API
diff mbox

Message ID 20171207223859.6256-1-bluewind@xinu.at
State Superseded, archived
Headers show

Commit Message

Florian Pritz Dec. 7, 2017, 10:38 p.m. UTC
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.

Signed-off-by: Florian Pritz <bluewind@xinu.at>
---

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.

 conf/config.proto         |  4 +++
 upgrading/4.7.0.txt       | 11 ++++++++
 web/lib/aurjson.class.php | 71 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 86 insertions(+)
 create mode 100644 upgrading/4.7.0.txt

Comments

Lukas Fleischer Jan. 26, 2018, 7:39 p.m. UTC | #1
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 <bluewind@xinu.at>
> ---
> 
> 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
Florian Pritz Jan. 26, 2018, 9:02 p.m. UTC | #2
On 26.01.2018 20:39, Lukas Fleischer wrote:
> 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?

They don't send one request every 5 or 10 seconds, they send one request
for each aur package they have installed because they use cower -u. So
it's more like 20-100 requests from a single users each 5 or 10 seconds
in the bad cases.

The most noticeable impact is that the log file contains millions of
requests per day and thus 1-2GiB of text per day. I don't so much care
about the performance impact since I have no idea how to measure that
with all the things we have on the box anyways. What I really care about
is people not abusing server resources just because they are lazy and
put cower -u into their conky config and conky then runs the command all
the time for no good reason.

Here's the number of requests that a single ipv6 address made today in
the last 20.5 hours: 1808234. That's a whopping 24 requests per second
averaged over the time frame.

Looking for a specific package name in the requests shows that this user
ran cower -u 5713 times in that time period. That's an average delay of
~13 seconds between runs. Looking at the start and end of each batch of
requests, it takes roughly 2 seconds for one cower -u run to complete.

While this one is certainly a quite extreme case (not the worst yet,
mind you!), there are 47 more that would already have reached the 4000
request limit today.

I can only imagine that this problem will grow worse in the future
unless we start implementing rate limiting. Even then, users may take a
while to notice they run into the limit and they might not adjust their
scripts that quickly. Still, I think being able to limit API usage is
important because otherwise we can only block them completely which is
arguably difficult to debug for them and might be interpreted as our
servers not working in the worst case.

I plan to eventually reduce the limit once cower is used less, but first
we need the feature.

>> [...]
>> --- /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 "=".

I'll look into fixing all those things.

>> @@ -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.

They are so much nicer to write and I believe they work with all PDO
drivers. I'll look into testing that.

> 
>> +               $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"?

I didn't know sqlite was supported. I'll look into it.

> 
>> +               $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?

The query above removes all limits for IPs that have not visited in a
long time. The one in check_ratelimit removes only the limit for a
single IP if that limit is no longer relevant. Afterwards a new one is
created.

> 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?

It adds a select and update/insert for most cases. True, those add load,
but without them we can not limit API usage at all which leads to wild
growth without any consideration as shown above. I didn't look at the
performance impact, but I think we need the feature nevertheless. Unless
you want to compare this to an implementation with a different
technology (maybe memcached/redis or something), it doesn't matter much.

Are you interested in the feature implemented like this (with the fixes)
or do you want it built differently/not at all/...? Another idea I have
would be to remove the API and distribute the database similar to the
pacman repo databases via our mirrors. However, I'm not interested in
working on that. It's just an idea.

In case you generally accept the patch idea, I'll work on fixing the
issues you mentioned.

Florian
Lukas Fleischer Jan. 28, 2018, 12:12 p.m. UTC | #3
Hi Florian,

On Fri, 26 Jan 2018 at 22:02:42, Florian Pritz wrote:
> [...]
> Here's the number of requests that a single ipv6 address made today in
> the last 20.5 hours: 1808234. That's a whopping 24 requests per second
> averaged over the time frame.
> 
> Looking for a specific package name in the requests shows that this user
> ran cower -u 5713 times in that time period. That's an average delay of
> ~13 seconds between runs. Looking at the start and end of each batch of
> requests, it takes roughly 2 seconds for one cower -u run to complete.
> 
> While this one is certainly a quite extreme case (not the worst yet,
> mind you!), there are 47 more that would already have reached the 4000
> request limit today.

Ok, that sounds quite bad indeed. Thanks for the clarification!

> [...]
> I'll look into fixing all those things.
> [...]
> They are so much nicer to write and I believe they work with all PDO
> drivers. I'll look into testing that.
> [...]
> I didn't know sqlite was supported. I'll look into it.

Great!

> 
> > 
> >> +               $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?
> 
> The query above removes all limits for IPs that have not visited in a
> long time. The one in check_ratelimit removes only the limit for a
> single IP if that limit is no longer relevant. Afterwards a new one is
> created.

It is not clear to me why this needs to be done in two separate steps.
Can't we simply remove all outdated entries with a single query before
doing anything else?

> 
> > 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?
> 
> It adds a select and update/insert for most cases. True, those add load,
> but without them we can not limit API usage at all which leads to wild
> growth without any consideration as shown above. I didn't look at the
> performance impact, but I think we need the feature nevertheless. Unless
> you want to compare this to an implementation with a different
> technology (maybe memcached/redis or something), it doesn't matter much.
> 
> Are you interested in the feature implemented like this (with the fixes)
> or do you want it built differently/not at all/...? Another idea I have
> would be to remove the API and distribute the database similar to the
> pacman repo databases via our mirrors. However, I'm not interested in
> working on that. It's just an idea.
> 
> In case you generally accept the patch idea, I'll work on fixing the
> issues you mentioned.

I like the idea and I think we should give your implementation (modulo
the small issues I mentioned) a try. If it turns out to produce too much
load, we can still switch to "something better" later.

Regards,
Lukas

Patch
diff mbox

diff --git a/conf/config.proto b/conf/config.proto
index 1750929..934d369 100644
--- a/conf/config.proto
+++ b/conf/config.proto
@@ -36,6 +36,10 @@  enable-maintenance = 1
 maintenance-exceptions = 127.0.0.1
 render-comment-cmd = /usr/local/bin/aurweb-rendercomment
 
+[ratelimit]
+request_limit = 4000
+window_length = 86400
+
 [notifications]
 notify-cmd = /usr/local/bin/aurweb-notify
 sendmail = /usr/bin/sendmail
diff --git a/upgrading/4.7.0.txt b/upgrading/4.7.0.txt
new file mode 100644
index 0000000..1b51cd2
--- /dev/null
+++ b/upgrading/4.7.0.txt
@@ -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,
+  PRIMARY KEY (`ip`),
+  KEY `window_start_idx` (`window_start`)
+) ENGINE=InnoDB;
+---
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);
+		$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");
+		$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();
+	}
+
 	/*
 	 * Returns a JSON formatted error string.
 	 *