Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[ETCM-331] Add Rate Limit for rpc requests #806

Merged
merged 11 commits into from
Dec 1, 2020
Merged

[ETCM-331] Add Rate Limit for rpc requests #806

merged 11 commits into from
Dec 1, 2020

Conversation

SoseMagarian
Copy link
Contributor

@SoseMagarian SoseMagarian commented Nov 20, 2020 •
edited
Loading

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

  1. FaucetJsonRpcHttpServer
  2. 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
dmitry-worker reviewed Nov 22, 2020
View reviewed changes
src/main/scala/io/iohk/ethereum/jsonrpc/server/http/FaucetJsonRpcHttpServer.scala Outdated

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

@SoseMagarian SoseMagarian requested a review from ntallar November 30, 2020 13:55
@SoseMagarian SoseMagarian force-pushed the etcm-331-mantis-protection branch from 799f20c to 0877ceb Compare November 30, 2020 14:24
ntallar
ntallar requested changes Nov 30, 2020
View reviewed changes
src/main/scala/io/iohk/ethereum/jsonrpc/server/http/InsecureJsonRpcHttpServer.scala Show resolved Hide resolved
src/main/scala/io/iohk/ethereum/jsonrpc/server/http/JsonRpcHttpServer.scala Outdated

val latestTimestampCacheSize: Int

val latestRequestTimestamps: LruMap[RemoteAddress, Long]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/main/scala/io/iohk/ethereum/jsonrpc/server/http/JsonRpcHttpServer.scala Outdated
} ~ entity(as[Seq[JsonRpcRequest]]) { request =>
handleBatchRequest(request)
}
}
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/test/scala/io/iohk/ethereum/jsonrpc/server/http/JsonRpcHttpServerSpec.scala Outdated
status shouldEqual StatusCodes.TooManyRequests
}

FakeClock.advanceTime(10)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/test/scala/io/iohk/ethereum/jsonrpc/server/http/JsonRpcHttpServerSpec.scala Outdated
}
}

object FakeClock extends Clock {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ntallar ntallar marked this pull request as ready for review December 1, 2020 13:48
@SoseMagarian SoseMagarian force-pushed the etcm-331-mantis-protection branch from 16cd9d5 to ac6f7e7 Compare December 1, 2020 17:49
@SoseMagarian SoseMagarian force-pushed the etcm-331-mantis-protection branch from ac6f7e7 to 341db30 Compare December 1, 2020 17:55
@SoseMagarian SoseMagarian force-pushed the etcm-331-mantis-protection branch from 053890d to 6e82731 Compare December 1, 2020 18:24
ntallar
ntallar approved these changes Dec 1, 2020
View reviewed changes
Copy link

@ntallar ntallar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

src/main/scala/io/iohk/ethereum/jsonrpc/server/http/RateLimit.scala
val timeMillis = clock.instant().toEpochMilli
val latestRequestTimestamp = latestRequestTimestamps.getOrElse(clientAddress, 0L)

val response = latestRequestTimestamp + config.rateLimit.minRequestInterval.toMillis < timeMillis
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: maybe we should rename it: isBelowRateLimit?

@SoseMagarian SoseMagarian changed the title [ETCM-331] Add ip limit in the faucet for sendFunds requests [ETCM-331] Add Rate Limit for rpc requests Dec 1, 2020
@SoseMagarian SoseMagarian merged commit 78c13d8 into develop Dec 1, 2020
@ntallar ntallar added the BREAKS CONFIG Affects the default configuration label Dec 2, 2020
@dzajkowski dzajkowski deleted the etcm-331-mantis-protection branch April 9, 2021 12:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dmitry-worker dmitry-worker dmitry-worker left review comments

@biandratti biandratti biandratti approved these changes

@ntallar ntallar ntallar approved these changes

Assignees
No one assigned
Labels
BREAKS CONFIG Affects the default configuration
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@SoseMagarian @dmitry-worker @ntallar @biandratti