[ETCM-331] Add Rate Limit for rpc requests by SoseMagarian · Pull Request #806 · input-output-hk/mantis

SoseMagarian · 2020-11-20T17:31:17Z

Description

In a previous refactor, IP limit tracking was removed due to the nature of the refactor and because this isn't a problem that mantis codebase should be handling. As a temporary solution, this PR brings back the IP limit tracking for faucet_sendFunds requests.

Proposed Solution

First approach [Deprecated]

Two new traits extending the JsonRpcHttpServer were created to separate the behavior for the node and the faucet

FaucetJsonRpcHttpServer
NodeJsonRpcHttpServer

Their child classes (BasicJsonRpcHttpServer and JsonRpcHttpsServer) also need to be extended for the node and for the faucet. The main goal is being able to handle the route val and extract the Ip and limit the requests by ip, but only for the faucet.

Final Approach

Add ip limit tracking as a JsonRpc capability. It can be enabled/disabled by configuration.
Some decisions to take into consideration:

Batch requests are disabled with IP-restriction enabled
Requesting faucet's status does not add the ip to the tracked ips

Testing

Run the faucet with a node and try to execute requests in less than the minimum time interval allowed between requests (10 seconds by default). Also, verify that faucet_status has no requests limit and should not affect the faucet_sendFunds requests.

dmitry-worker · 2020-11-22T10:47:43Z

+
+  override def route: Route = cors(corsSettings) {
+    (pathEndOrSingleSlash & post) {
+      (extractClientIP & entity(as[JsonRpcRequest])) { (clientAddress: RemoteAddress, request: JsonRpcRequest) =>


Is it enough to check IP address only in HTTP header, or maybe we shouldn't trust them?
Maybe we have to read AttributeKeys.remoteAddress? What do you think?

HI! Thanks for the comment! I think you're right. I'll create a different task for it (since it might escape the scope of this one -> just bringing back old behavior)

…-331-mantis-protection

ntallar · 2020-11-30T19:01:52Z

+
+  val latestTimestampCacheSize: Int
+
+  val latestRequestTimestamps: LruMap[RemoteAddress, Long]


Maybe this is included in the above comment by Maxi, but also this can be derived from the config here without having to override it on it's sons

ntallar · 2020-11-30T19:07:07Z

  }

+  def handleRequest(clientAddress: RemoteAddress, request: JsonRpcRequest): StandardRoute = {
+    if (ipTrackingEnabled && request.method != FaucetJsonRpcController.Status) {


Why are we only disabling this for the faucet status? Maybe it's because it's used as the healthcheck? I though the healthcheck was something separately

I added a FIXME for this, the faucet Status should somehow work as a healthcheck so I left it out of the rate limiting in case it's being used by some of our services. Eventually it should be part of the healthcheck or be handled more elegantly

ntallar · 2020-11-30T19:13:45Z

+      status shouldEqual StatusCodes.TooManyRequests
+    }
+
+    FakeClock.advanceTime(10)


Minor: maybe we should set this to 2* config.minRequestInterval instead of the hardcoded 10?

ntallar · 2020-11-30T19:16:21Z

+  }
+}
+
+object FakeClock extends Clock {


Maybe this should be a class FakeClock extends Clock and on the TestSetup we create an instance of it instead of using this singleton?

…-331-mantis-protection

ntallar

Only a minor comment apart from which LGTM! I played around with it and it worked as expected!

 .\\            //.
. \ \          / /.
.\  ,\     /` /,.-
 -.   \  /'/ /  .
 ` -   `-'  \  -
   '.       /.\`
      -    .-
      :`//.'
      .`.'
      .' BP

ntallar · 2020-12-01T19:00:06Z

+    val timeMillis = clock.instant().toEpochMilli
+    val latestRequestTimestamp = latestRequestTimestamps.getOrElse(clientAddress, 0L)
+
+    val response = latestRequestTimestamp + config.rateLimit.minRequestInterval.toMillis < timeMillis


Minor: maybe we should rename it: isBelowRateLimit?

…-331-mantis-protection

[ETCM-331] Add ip limit in the faucet for sendFunds requests

84929cb

SoseMagarian requested a review from biandratti November 20, 2020 17:31

dmitry-worker reviewed Nov 22, 2020

View reviewed changes