From 3f53718d107f0fc96ca9a189e669c5a7f3d64cc4 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 11 Oct 2017 14:23:09 -0700 Subject: [PATCH] Modularize rate/connection limits in Phabricator Summary: Depends on D18702. Ref T13008. This replaces the old hard-coded single rate limit with multiple flexible limits, and defines two types of limits: - Rate: reject requests if a client has completed too many requests recently. - Connection: reject requests if a client has too many more connections than disconnections recently. The connection limit adds +1 to the score for each connection, then adds -1 for each disconnection. So the overall number is how many open connections they have, at least approximately. Supporting multiple limits will let us do limiting by Hostname and by remote address (e.g., a specific IP can't exceed a low limit, and all requests to a hostname can't exceed a higher limit). Configuring the new limits looks something like this: ``` PhabricatorStartup::addRateLimit(new PhabricatorClientRateLimit()) ->setLimitKey('rate') ->setClientKey($_SERVER['REMOTE_ADDR']) ->setLimit(5); PhabricatorStartup::addRateLimit(new PhabricatorClientConnectionLimit()) ->setLimitKey('conn') ->setClientKey($_SERVER['REMOTE_ADDR']) ->setLimit(2); ``` Test Plan: - Configured limits as above. - Made a lot of requests, got cut off by the rate limit. - Used `curl --limit-rate -F 'data=@the_letter_m.txt' ...` to upload files really slowly. Got cut off by the connection limit. With `enable_post_data_reading` off, this correctly killed the connections //before// the uploads finished. - I'll send this stuff to `secure` before production to give it more of a chance. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13008 Differential Revision: https://secure.phabricator.com/D18703 --- .../AphrontApplicationConfiguration.php | 18 +- .../PhabricatorAccessControlTestCase.php | 2 +- .../PhabricatorClientConnectionLimit.php | 44 +++ support/startup/PhabricatorClientLimit.php | 291 +++++++++++++++++ .../startup/PhabricatorClientRateLimit.php | 58 ++++ support/{ => startup}/PhabricatorStartup.php | 299 +++--------------- webroot/index.php | 7 +- 7 files changed, 446 insertions(+), 273 deletions(-) create mode 100644 support/startup/PhabricatorClientConnectionLimit.php create mode 100644 support/startup/PhabricatorClientLimit.php create mode 100644 support/startup/PhabricatorClientRateLimit.php rename support/{ => startup}/PhabricatorStartup.php (73%) diff --git a/src/aphront/configuration/AphrontApplicationConfiguration.php b/src/aphront/configuration/AphrontApplicationConfiguration.php index c0cd259992..60b12557c9 100644 --- a/src/aphront/configuration/AphrontApplicationConfiguration.php +++ b/src/aphront/configuration/AphrontApplicationConfiguration.php @@ -204,20 +204,10 @@ abstract class AphrontApplicationConfiguration extends Phobject { DarkConsoleXHProfPluginAPI::saveProfilerSample($access_log); - // Add points to the rate limits for this request. - $rate_token = PhabricatorStartup::getRateLimitToken(); - if ($rate_token !== null) { - // The base score for a request allows users to make 30 requests per - // minute. - $score = (1000 / 30); - - // If the user was logged in, let them make more requests. - if ($request->getUser() && $request->getUser()->getPHID()) { - $score = $score / 5; - } - - PhabricatorStartup::addRateLimitScore($rate_token, $score); - } + PhabricatorStartup::disconnectRateLimits( + array( + 'viewer' => $request->getUser(), + )); if ($processing_exception) { throw $processing_exception; diff --git a/src/applications/base/controller/__tests__/PhabricatorAccessControlTestCase.php b/src/applications/base/controller/__tests__/PhabricatorAccessControlTestCase.php index 27e716556e..b6c268d88c 100644 --- a/src/applications/base/controller/__tests__/PhabricatorAccessControlTestCase.php +++ b/src/applications/base/controller/__tests__/PhabricatorAccessControlTestCase.php @@ -10,7 +10,7 @@ final class PhabricatorAccessControlTestCase extends PhabricatorTestCase { public function testControllerAccessControls() { $root = dirname(phutil_get_library_root('phabricator')); - require_once $root.'/support/PhabricatorStartup.php'; + require_once $root.'/support/startup/PhabricatorStartup.php'; $application_configuration = new AphrontDefaultApplicationConfiguration(); diff --git a/support/startup/PhabricatorClientConnectionLimit.php b/support/startup/PhabricatorClientConnectionLimit.php new file mode 100644 index 0000000000..4a66d05739 --- /dev/null +++ b/support/startup/PhabricatorClientConnectionLimit.php @@ -0,0 +1,44 @@ + $this->getLimit()); + } + + protected function getConnectScore() { + return 1; + } + + protected function getPenaltyScore() { + return 0; + } + + protected function getDisconnectScore(array $request_state) { + return -1; + } + + protected function getRateLimitReason($score) { + $client_key = $this->getClientKey(); + + // NOTE: This happens before we load libraries, so we can not use pht() + // here. + + return + "TOO MANY CONCURRENT CONNECTIONS\n". + "You (\"{$client_key}\") have too many concurrent ". + "connections.\n"; + } + +} diff --git a/support/startup/PhabricatorClientLimit.php b/support/startup/PhabricatorClientLimit.php new file mode 100644 index 0000000000..bab90c6f15 --- /dev/null +++ b/support/startup/PhabricatorClientLimit.php @@ -0,0 +1,291 @@ +limitKey = $limit_key; + return $this; + } + + final public function getLimitKey() { + return $this->limitKey; + } + + final public function setClientKey($client_key) { + $this->clientKey = $client_key; + return $this; + } + + final public function getClientKey() { + return $this->clientKey; + } + + final public function setLimit($limit) { + $this->limit = $limit; + return $this; + } + + final public function getLimit() { + return $this->limit; + } + + final public function didConnect() { + // NOTE: We can not use pht() here because this runs before libraries + // load. + + if (!function_exists('apc_fetch') && !function_exists('apcu_fetch')) { + throw new Exception( + 'You can not configure connection rate limits unless APC/APCu are '. + 'available. Rate limits rely on APC/APCu to track clients and '. + 'connections.'); + } + + if ($this->getClientKey() === null) { + throw new Exception( + 'You must configure a client key when defining a rate limit.'); + } + + if ($this->getLimitKey() === null) { + throw new Exception( + 'You must configure a limit key when defining a rate limit.'); + } + + if ($this->getLimit() === null) { + throw new Exception( + 'You must configure a limit when defining a rate limit.'); + } + + $points = $this->getConnectScore(); + if ($points) { + $this->addScore($points); + } + + $score = $this->getScore(); + if (!$this->shouldRejectConnection($score)) { + // Client has not hit the limit, so continue processing the request. + return null; + } + + $penalty = $this->getPenaltyScore(); + if ($penalty) { + $this->addScore($penalty); + $score += $penalty; + } + + return $this->getRateLimitReason($score); + } + + final public function didDisconnect(array $request_state) { + $score = $this->getDisconnectScore($request_state); + if ($score) { + $this->addScore($score); + } + } + + + /** + * Get the number of seconds for each rate bucket. + * + * For example, a value of 60 will create one-minute buckets. + * + * @return int Number of seconds per bucket. + */ + abstract protected function getBucketDuration(); + + + /** + * Get the total number of rate limit buckets to retain. + * + * @return int Total number of rate limit buckets to retain. + */ + abstract protected function getBucketCount(); + + + /** + * Get the score to add when a client connects. + * + * @return double Connection score. + */ + abstract protected function getConnectScore(); + + + /** + * Get the number of penalty points to add when a client hits a rate limit. + * + * @return double Penalty score. + */ + abstract protected function getPenaltyScore(); + + + /** + * Get the score to add when a client disconnects. + * + * @return double Connection score. + */ + abstract protected function getDisconnectScore(array $request_state); + + + /** + * Get a human-readable explanation of why the client is being rejected. + * + * @return string Brief rejection message. + */ + abstract protected function getRateLimitReason($score); + + + /** + * Determine whether to reject a connection. + * + * @return bool True to reject the connection. + */ + abstract protected function shouldRejectConnection($score); + + + /** + * Get the APC key for the smallest stored bucket. + * + * @return string APC key for the smallest stored bucket. + * @task ratelimit + */ + private function getMinimumBucketCacheKey() { + $limit_key = $this->getLimitKey(); + return "limit:min:{$limit_key}"; + } + + + /** + * Get the current bucket ID for storing rate limit scores. + * + * @return int The current bucket ID. + */ + private function getCurrentBucketID() { + return (int)(time() / $this->getBucketDuration()); + } + + + /** + * Get the APC key for a given bucket. + * + * @param int Bucket to get the key for. + * @return string APC key for the bucket. + */ + private function getBucketCacheKey($bucket_id) { + $limit_key = $this->getLimitKey(); + return "limit:bucket:{$limit_key}:{$bucket_id}"; + } + + + /** + * Add points to the rate limit score for some client. + * + * @param string Some key which identifies the client making the request. + * @param float The cost for this request; more points pushes them toward + * the limit faster. + * @return this + */ + private function addScore($score) { + $is_apcu = (bool)function_exists('apcu_fetch'); + + $current = $this->getCurrentBucketID(); + $bucket_key = $this->getBucketCacheKey($current); + + // There's a bit of a race here, if a second process reads the bucket + // before this one writes it, but it's fine if we occasionally fail to + // record a client's score. If they're making requests fast enough to hit + // rate limiting, we'll get them soon enough. + + if ($is_apcu) { + $bucket = apcu_fetch($bucket_key); + } else { + $bucket = apc_fetch($bucket_key); + } + + if (!is_array($bucket)) { + $bucket = array(); + } + + $client_key = $this->getClientKey(); + if (empty($bucket[$client_key])) { + $bucket[$client_key] = 0; + } + + $bucket[$client_key] += $score; + + if ($is_apcu) { + apcu_store($bucket_key, $bucket); + } else { + apc_store($bucket_key, $bucket); + } + + return $this; + } + + + /** + * Get the current rate limit score for a given client. + * + * @return float The client's current score. + * @task ratelimit + */ + private function getScore() { + $is_apcu = (bool)function_exists('apcu_fetch'); + + // Identify the oldest bucket stored in APC. + $min_key = $this->getMinimumBucketCacheKey(); + if ($is_apcu) { + $min = apcu_fetch($min_key); + } else { + $min = apc_fetch($min_key); + } + + // If we don't have any buckets stored yet, store the current bucket as + // the oldest bucket. + $cur = $this->getCurrentBucketID(); + if (!$min) { + if ($is_apcu) { + apcu_store($min_key, $cur); + } else { + apc_store($min_key, $cur); + } + $min = $cur; + } + + // Destroy any buckets that are older than the minimum bucket we're keeping + // track of. Under load this normally shouldn't do anything, but will clean + // up an old bucket once per minute. + $count = $this->getBucketCount(); + for ($cursor = $min; $cursor < ($cur - $count); $cursor++) { + $bucket_key = $this->getBucketCacheKey($cursor); + if ($is_apcu) { + apcu_delete($bucket_key); + apcu_store($min_key, $cursor + 1); + } else { + apc_delete($bucket_key); + apc_store($min_key, $cursor + 1); + } + } + + $client_key = $this->getClientKey(); + + // Now, sum up the client's scores in all of the active buckets. + $score = 0; + for (; $cursor <= $cur; $cursor++) { + $bucket_key = $this->getBucketCacheKey($cursor); + if ($is_apcu) { + $bucket = apcu_fetch($bucket_key); + } else { + $bucket = apc_fetch($bucket_key); + } + if (isset($bucket[$client_key])) { + $score += $bucket[$client_key]; + } + } + + return $score; + } + +} diff --git a/support/startup/PhabricatorClientRateLimit.php b/support/startup/PhabricatorClientRateLimit.php new file mode 100644 index 0000000000..85a6def878 --- /dev/null +++ b/support/startup/PhabricatorClientRateLimit.php @@ -0,0 +1,58 @@ +getLimit(); + + // Reject connections if the average score across all buckets exceeds the + // limit. + $average_score = $score / $this->getBucketCount(); + + return ($average_score > $limit); + } + + protected function getConnectScore() { + return 0; + } + + protected function getPenaltyScore() { + return 1; + } + + protected function getDisconnectScore(array $request_state) { + $score = 1; + + // If the user was logged in, let them make more requests. + if (isset($request_state['viewer'])) { + $viewer = $request_state['viewer']; + if ($viewer->isLoggedIn()) { + $score = 0.25; + } + } + + return $score; + } + + protected function getRateLimitReason($score) { + $client_key = $this->getClientKey(); + + // NOTE: This happens before we load libraries, so we can not use pht() + // here. + + return + "TOO MANY REQUESTS\n". + "You (\"{$client_key}\") are issuing too many requests ". + "too quickly.\n"; + } + +} diff --git a/support/PhabricatorStartup.php b/support/startup/PhabricatorStartup.php similarity index 73% rename from support/PhabricatorStartup.php rename to support/startup/PhabricatorStartup.php index 3d771a24f7..41164fbd84 100644 --- a/support/PhabricatorStartup.php +++ b/support/startup/PhabricatorStartup.php @@ -46,11 +46,7 @@ final class PhabricatorStartup { private static $oldMemoryLimit; private static $phases; - // TODO: For now, disable rate limiting entirely by default. We need to - // iterate on it a bit for Conduit, some of the specific score levels, and - // to deal with NAT'd offices. - private static $maximumRate = 0; - private static $rateLimitToken; + private static $limits = array(); /* -( Accessing Request Information )-------------------------------------- */ @@ -138,10 +134,7 @@ final class PhabricatorStartup { // we can switch over to relying on our own exception recovery mechanisms. ini_set('display_errors', 0); - $rate_token = self::getRateLimitToken(); - if ($rate_token !== null) { - self::rateLimitRequest($rate_token); - } + self::connectRateLimits(); self::normalizeInput(); @@ -193,7 +186,7 @@ final class PhabricatorStartup { } public static function loadCoreLibraries() { - $phabricator_root = dirname(dirname(__FILE__)); + $phabricator_root = dirname(dirname(dirname(__FILE__))); $libraries_root = dirname($phabricator_root); $root = null; @@ -683,269 +676,66 @@ final class PhabricatorStartup { /** - * Adjust the permissible rate limit score. + * Add a new client limits. * - * By default, the limit is `1000`. You can use this method to set it to - * a larger or smaller value. If you set it to `2000`, users may make twice - * as many requests before rate limiting. + * @param PhabricatorClientLimit New limit. + * @return PhabricatorClientLimit The limit. + */ + public static function addRateLimit(PhabricatorClientLimit $limit) { + self::$limits[] = $limit; + return $limit; + } + + + /** + * Apply configured rate limits. + * + * If any limit is exceeded, this method terminates the request. * - * @param int Maximum score before rate limiting. * @return void * @task ratelimit */ - public static function setMaximumRate($rate) { - self::$maximumRate = $rate; - } + private static function connectRateLimits() { + $limits = self::$limits; - - /** - * Set a token to identify the client for purposes of rate limiting. - * - * By default, the `REMOTE_ADDR` is used. If your install is behind a load - * balancer, you may want to parse `X-Forwarded-For` and use that address - * instead. - * - * @param string Client identity for rate limiting. - */ - public static function setRateLimitToken($token) { - self::$rateLimitToken = $token; - } - - - /** - * Get the current client identity for rate limiting. - */ - public static function getRateLimitToken() { - if (self::$rateLimitToken !== null) { - return self::$rateLimitToken; + $reason = null; + $connected = array(); + foreach ($limits as $limit) { + $reason = $limit->didConnect(); + $connected[] = $limit; + if ($reason !== null) { + break; + } } - if (isset($_SERVER['REMOTE_ADDR'])) { - return $_SERVER['REMOTE_ADDR']; - } + // If we're killing the request here, disconnect any limits that we + // connected to try to keep the accounting straight. + if ($reason !== null) { + foreach ($connected as $limit) { + $limit->didDisconnect(array()); + } - return null; - } - - - /** - * Check if the user (identified by `$user_identity`) has issued too many - * requests recently. If they have, end the request with a 429 error code. - * - * The key just needs to identify the user. Phabricator uses both user PHIDs - * and user IPs as keys, tracking logged-in and logged-out users separately - * and enforcing different limits. - * - * @param string Some key which identifies the user making the request. - * @return void If the user has exceeded the rate limit, this method - * does not return. - * @task ratelimit - */ - public static function rateLimitRequest($user_identity) { - if (!self::canRateLimit()) { - return; - } - - $score = self::getRateLimitScore($user_identity); - $limit = self::$maximumRate * self::getRateLimitBucketCount(); - if ($score > $limit) { - // Give the user some bonus points for getting rate limited. This keeps - // bad actors who keep slamming the 429 page locked out completely, - // instead of letting them get a burst of requests through every minute - // after a bucket expires. - $penalty = 50; - - self::addRateLimitScore($user_identity, $penalty); - $score += $penalty; - - self::didRateLimit($user_identity, $score, $limit); + self::didRateLimit($reason); } } /** - * Add points to the rate limit score for some user. + * Tear down rate limiting and allow limits to score the request. * - * If users have earned more than 1000 points per minute across all the - * buckets they'll be locked out of the application, so awarding 1 point per - * request roughly corresponds to allowing 1000 requests per second, while - * awarding 50 points roughly corresponds to allowing 20 requests per second. - * - * @param string Some key which identifies the user making the request. - * @param float The cost for this request; more points pushes them toward - * the limit faster. + * @param map Additional, freeform request state. * @return void * @task ratelimit */ - public static function addRateLimitScore($user_identity, $score) { - if (!self::canRateLimit()) { - return; - } + public static function disconnectRateLimits(array $request_state) { + $limits = self::$limits; - $is_apcu = (bool)function_exists('apcu_fetch'); - $current = self::getRateLimitBucket(); - - // There's a bit of a race here, if a second process reads the bucket - // before this one writes it, but it's fine if we occasionally fail to - // record a user's score. If they're making requests fast enough to hit - // rate limiting, we'll get them soon enough. - - $bucket_key = self::getRateLimitBucketKey($current); - if ($is_apcu) { - $bucket = apcu_fetch($bucket_key); - } else { - $bucket = apc_fetch($bucket_key); - } - - if (!is_array($bucket)) { - $bucket = array(); - } - - if (empty($bucket[$user_identity])) { - $bucket[$user_identity] = 0; - } - - $bucket[$user_identity] += $score; - - if ($is_apcu) { - apcu_store($bucket_key, $bucket); - } else { - apc_store($bucket_key, $bucket); + foreach ($limits as $limit) { + $limit->didDisconnect($request_state); } } - /** - * Determine if rate limiting is available. - * - * Rate limiting depends on APC, and isn't available unless the APC user - * cache is available. - * - * @return bool True if rate limiting is available. - * @task ratelimit - */ - private static function canRateLimit() { - - if (!self::$maximumRate) { - return false; - } - - if (!function_exists('apc_fetch') && !function_exists('apcu_fetch')) { - return false; - } - - return true; - } - - - /** - * Get the current bucket for storing rate limit scores. - * - * @return int The current bucket. - * @task ratelimit - */ - private static function getRateLimitBucket() { - return (int)(time() / 60); - } - - - /** - * Get the total number of rate limit buckets to retain. - * - * @return int Total number of rate limit buckets to retain. - * @task ratelimit - */ - private static function getRateLimitBucketCount() { - return 5; - } - - - /** - * Get the APC key for a given bucket. - * - * @param int Bucket to get the key for. - * @return string APC key for the bucket. - * @task ratelimit - */ - private static function getRateLimitBucketKey($bucket) { - return 'rate:bucket:'.$bucket; - } - - - /** - * Get the APC key for the smallest stored bucket. - * - * @return string APC key for the smallest stored bucket. - * @task ratelimit - */ - private static function getRateLimitMinKey() { - return 'rate:min'; - } - - - /** - * Get the current rate limit score for a given user. - * - * @param string Unique key identifying the user. - * @return float The user's current score. - * @task ratelimit - */ - private static function getRateLimitScore($user_identity) { - $is_apcu = (bool)function_exists('apcu_fetch'); - - $min_key = self::getRateLimitMinKey(); - - // Identify the oldest bucket stored in APC. - $cur = self::getRateLimitBucket(); - if ($is_apcu) { - $min = apcu_fetch($min_key); - } else { - $min = apc_fetch($min_key); - } - - // If we don't have any buckets stored yet, store the current bucket as - // the oldest bucket. - if (!$min) { - if ($is_apcu) { - apcu_store($min_key, $cur); - } else { - apc_store($min_key, $cur); - } - $min = $cur; - } - - // Destroy any buckets that are older than the minimum bucket we're keeping - // track of. Under load this normally shouldn't do anything, but will clean - // up an old bucket once per minute. - $count = self::getRateLimitBucketCount(); - for ($cursor = $min; $cursor < ($cur - $count); $cursor++) { - $bucket_key = self::getRateLimitBucketKey($cursor); - if ($is_apcu) { - apcu_delete($bucket_key); - apcu_store($min_key, $cursor + 1); - } else { - apc_delete($bucket_key); - apc_store($min_key, $cursor + 1); - } - } - - // Now, sum up the user's scores in all of the active buckets. - $score = 0; - for (; $cursor <= $cur; $cursor++) { - $bucket_key = self::getRateLimitBucketKey($cursor); - if ($is_apcu) { - $bucket = apcu_fetch($bucket_key); - } else { - $bucket = apc_fetch($bucket_key); - } - if (isset($bucket[$user_identity])) { - $score += $bucket[$user_identity]; - } - } - - return $score; - } - /** * Emit an HTTP 429 "Too Many Requests" response (indicating that the user @@ -954,18 +744,13 @@ final class PhabricatorStartup { * @return exit This method **does not return**. * @task ratelimit */ - private static function didRateLimit($user_identity, $score, $limit) { - $message = - "TOO MANY REQUESTS\n". - "You (\"{$user_identity}\") are issuing too many requests ". - "too quickly.\n"; - + private static function didRateLimit($reason) { header( 'Content-Type: text/plain; charset=utf-8', $replace = true, $http_error = 429); - echo $message; + echo $reason; exit(1); } diff --git a/webroot/index.php b/webroot/index.php index 59e5b71075..5c7d79bfa1 100644 --- a/webroot/index.php +++ b/webroot/index.php @@ -37,7 +37,12 @@ function phabricator_startup() { // Load the PhabricatorStartup class itself. $t_startup = microtime(true); $root = dirname(dirname(__FILE__)); - require_once $root.'/support/PhabricatorStartup.php'; + require_once $root.'/support/startup/PhabricatorStartup.php'; + + // Load client limit classes so the preamble can configure limits. + require_once $root.'/support/startup/PhabricatorClientLimit.php'; + require_once $root.'/support/startup/PhabricatorClientRateLimit.php'; + require_once $root.'/support/startup/PhabricatorClientConnectionLimit.php'; // If the preamble script exists, load it. $t_preamble = microtime(true);