guzba/mummy

FR: Timeout for request handlers

turbo opened this issue · 2 comments

turbo commented

I'm slowly moving an app I created with mummy into production. Lots of endpoints accept user-controlled data and perform database calls based on it. While I can do my best to limit their complexity with validation, I'd like to set an absolute limit on the runtime of worker procs dealing with requests to limit the impact of worst-case inputs and preventing (or at least making it very difficult to) DoS.

I could wrap the entire handler in another thread and kill it based on timing, but since requests are already threaded, my suggestion would be to add a timeout to or timeout-enabled version of the router.x register methods for request handlers.

guzba commented

Hi, I hope you're having success with Mummy. Regarding the suggestion here, I advise against this approach.

There are several reasons this is not a good idea, but it can be long-winded to explain too much so I'll focus on a few concrete things. I'll refer to this suggestion as a response deadline.

  • First, a deadline must be long enough for reasonable requests to finish. Lets say 1 second? Well, at any reasonable time such as that, it will not have any value at all in stopping DoS attacks. It is trivial to send many 10s of thousands of requests per second at a machine. Any deadline/timeout based approach will never work for this problem. There are ways to mitigate this kind of DoS but it is not a request deadline.
  • Second, aborting / stopping / killing a thread can and will leave shared resources in broken states. For example, if you have a shared pool of Redis connections, if you abort the thread after a command was sent but before the reply was received, that shared connection is in a permanently broken state (the matching command+reply is out of sync forever).
  • Third, Mummy only starts a fixed number of threads in advance and never creates more. Creating threads is very expensive and can fail. Aborting or killing a thread will simply simply result in less and less worker threads until none are left. You may actually only want to interrupt blocking things, but that cannot be guaranteed at all. Thread interruption is complex. The idea of creating a thread for every request received is also very not a good idea.

The actual way to think about this is to think about each piece of your server's logic and ensure it is not trivially vulnerable. For RPC requests, ensure some reasonable timeouts. Same for database connections / queries. Lots more to say here but this is already a long reply.

Thread killing is not a solution. It may seem like a simple high-level solution but it actually does not solve anything and breaks many many things.

I agree with Guzba that stopping threads is not a good idea. Instead, a better approach would be to implement rate-limiting based on the user's IP address, API key, or user credentials. It's important to check if a user has hit their rate limit before allowing them to perform any real work. To enforce rate limiting, you can add a cost in terms of time or some other unit for each operation. For instance, you could limit an IP address to only one second of work per second. This will help ensure that the system is protected from overload and that all users are able to access it fairly.