moscajs / mosca

MQTT broker as a module
mosca.io
3.2k stars 508 forks source link

Password Hashing Kills CPU When Serving Many (thousands of) Clients #565

Closed isdrupter closed 7 years ago

isdrupter commented 7 years ago

I'm surprised that I don't see anyone else mentioning this, because it's been an issue ever since I started using mosca. When I run mosca with over ~4000 clients, and the server is restarted, the CPU locks up when the clients begin to reconnect, and mosca goes into and endless loop-- about 5000 clients will connect, and than that number falls back down to about 2500, and than bounces back up to 5000, and repeats. This is because the server is recomputing the client's password hash every time a single client connects, which takes up enormous CPU resources when there are thousands, or tens of thousands of clients.

I was able to fix this issue by editing the authorizer.js, and disabling the hashing of the passwords, and instead authenticate by comparing the password to a plaintext string. Obviously this hackish work-around is neither ideal nor secure.

Perhaps a better solution would be to add an option to hash the passwords once at startup, and keep them in memory, to eliminate the need to recompute the hash upon every single client connection. I know it's not quite as secure, but at least it would be functional.

mcollina commented 7 years ago

@isdrupter the password is hashed only when the login happen, with the password given by the user: https://github.com/mcollina/mosca/blob/master/lib/authorizer.js#L99-L102. I do not understand what you are proposing. The problem with hashing password is that the longer hashing takes, the more secure your application is.

I'm glad that you find the internal authorizer useful! If you have any particular suggestion, feel free to send a PR.

We might as well stop accepting new clients if we are too busy, maybe integrating http://npm.im/loopbench. in the connect phase. In this way, we will avoid the event loop starvation that you are experiencing.

isdrupter commented 7 years ago

Perhaps my usage scenario is different. I have a lot of clients that are using the same username and password, so I'm proposing that when mosca starts, it reads the list of passwords in the credentials file, and than keeps the hashes in memory until the first client connects, at which point it would compare the hash against the password, and keep than keep the password in memory, or write it to a file descriptor, so that when the next client connects, with an identical password to the one that has been previously hashed, mosca won't have to rehash the same password again, as opposed to rehashing it every time a user connects. I realize it isn't as secure, and would not make sense in a scenario where you have many different clients with unique passwords. But in my case, since the clients are sharing the same password, it is redundant and resource consuming to hash every time a client connects. Does that make sense?

mcollina commented 7 years ago

I recommend you to write your own authorization method. The current one is more of a demo, and your case seems quite different from the one the default one was designed for. I wrote to verify the authorizarion API.