Page MenuHomePhabricator

Update rate limiting for APCu and X-Forwarded-For
ClosedPublic

Authored by epriestley on Apr 21 2017, 8:59 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Apr 25, 2:44 AM
Unknown Object (File)
Sat, Apr 13, 6:48 PM
Unknown Object (File)
Sat, Apr 13, 6:22 PM
Unknown Object (File)
Sat, Apr 13, 5:27 PM
Unknown Object (File)
Sat, Apr 13, 4:51 PM
Unknown Object (File)
Sat, Apr 13, 4:51 PM
Unknown Object (File)
Sat, Apr 13, 2:46 PM
Unknown Object (File)
Sat, Apr 13, 2:13 PM
Subscribers
None

Details

Summary

Ref T12612. This updates the rate limiting code to:

  • Support a customizable token, like the client's X-Forwarded-For address, rather than always using REMOTE_ADDR.
  • Support APCu.
  • Report a little more rate limiting information.
  • Not reference nonexistent documentation (removed in D16403).

I'm planning to put this into production on secure for now and then we can deploy it more broadly if things work well.

Test Plan
  • Enabled it locally, used ab -n 100 to hit the limit, saw the limit enforced.
  • Waited a while, was allowed to browse again.

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Obligatory "but how much traffic can the rate-limiting infrastructure handle before that itself triggers a DoS condition?" comment, even though I'm sure APCu can handle it.

support/PhabricatorStartup.php
945

Probably better not to send this information (and maybe log it instead). Legit users with over-aggressive bots that get 429'd will probably go from oblivious to "oops, I'll cut my rate by 10x" without needing to know the details, but abuse traffic just gets a clue about how our rate limiting works and what the limits are.

This revision now requires changes to proceed.Apr 22 2017, 3:03 AM
epriestley edited edge metadata.
  • Don't disclose score or limit.
This revision is now accepted and ready to land.Apr 22 2017, 3:12 AM

I see some weird PHP 7.1 / APCu interaction locally so this may only be in the ballpark of how things will work in production, but ab -n 1000 can legitimately burn through 1K requests serially in ~200ms:

$ ab -n 1000 http://local.phacility.com/
This is ApacheBench, Version 2.3 <$Revision: 1757674 $>
Copyright 1996 Adam Twiss, Zeus Technology Ltd, http://www.zeustech.net/
Licensed to The Apache Software Foundation, http://www.apache.org/

Benchmarking local.phacility.com (be patient)
Completed 100 requests
Completed 200 requests
Completed 300 requests
Completed 400 requests
Completed 500 requests
Completed 600 requests
Completed 700 requests
Completed 800 requests
Completed 900 requests
Completed 1000 requests
Finished 1000 requests


Server Software:        Apache/2.4.16
Server Hostname:        local.phacility.com
Server Port:            80

Document Path:          /
Document Length:        73 bytes

Concurrency Level:      1
Time taken for tests:   0.222 seconds
Complete requests:      1000
Failed requests:        0
Non-2xx responses:      1000
Total transferred:      285000 bytes
HTML transferred:       73000 bytes
Requests per second:    4507.83 [#/sec] (mean)
Time per request:       0.222 [ms] (mean)
Time per request:       0.222 [ms] (mean, across all concurrent requests)
Transfer rate:          1254.62 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:     0    0   0.0      0       0
Waiting:        0    0   0.0      0       0
Total:          0    0   0.0      0       1

Percentage of the requests served within a certain time (ms)
  50%      0
  66%      0
  75%      0
  80%      0
  90%      0
  95%      0
  98%      0
  99%      0
 100%      1 (longest request)

So I think the cost of this is low enough to handle any reasonable non-attack traffic. If we're actually seeing attack traffic which can overwhelm this, I think the next step would be to try to block it in the VPC ACL? As far as I can tell, there's no way to block traffic from ELBs / ALBs, although maybe I'm just not looking in the right place.

This revision was automatically updated to reflect the committed changes.