threefoldtech / rmb-rs

RMB implementation in rust
Apache License 2.0
3 stars 1 forks source link

Enable (optionally) choosing a white list of commands when RMB starts #59

Closed sameh-farouk closed 1 year ago

sameh-farouk commented 2 years ago

I notice a comment in the code, that point to a possible attack, and suggests a fix for it.

A harmful twin can flood a server memory by sending many large messages to a cmd that is not handled by any application

the suggestion (in the comment) was to limit the length of the queue, but I think this won't be sufficient to prevent such attacks as a harmful twin can keep generate different/random commands easily.

My suggestion is to enable choosing a white list of commands when RMB start, using cmd line args or configuration file to mitigate the risk of this attack. if enabled, any message with a command not in this white list would be dropped, and a not found or not supported error returns (or other suitable error).

muhamadazmy commented 2 years ago

This also won't scale well. The idea is that applications can run behind rmb without RMB need to know about them. But your concerns are completely correct.

What might be a slightly better solutions is that when applications start, they can tell their local rmb what commands they expect hence the white list is basically dynamic.

This is not as easy to implement as it sounds. What about if rmb restarts (do we persist this white list, and other more concerns), what if a process crashed and it's not accepting commands anymore, etc...

sameh-farouk commented 2 years ago

What might be a slightly better solutions is that when applications start, they can tell their local rmb what commands they expect hence the white list is basically dynamic.

I agree this is better

This is not as easy to implement as it sounds. What about if rmb restarts (do we persist this white list, and other more concerns), what if a process crashed and it's not accepting commands anymore, etc...

Regarding these concerns ..

What about that We maintain a specific hash key (e.g. rmb:registered-commands) of commands to the tuple-like(PID, Process-Name) in Redis, and processes on start can register by simply setting/updating a field of this hash?

let's have an example of this suggested flow: ⁃ A process on start/restart would set a field of this hash with the command as a key which would be either added or updated when the same command name is used like when a process restarted and try to re-register.

HSET rmb:registerd-commands get-balance <PID>#<PROCCESS-NAME> // register `get-balance` command which handled by a process running with PID xx and proccess name xxx

⁃ In RMB, a simple routine is_still_alive would Periodically (maybe every minute or so) retrieve this hash hgetall rmb:registered-commands and check that all processes PIDs exist and match with the registered processes name (this could be done in a Platform Independence manner with sysinfo crate).

This flow achieves these goals:

Another issue that came to my mind with this flow is that we have to deal with the race condition that could arise when a process registered command at the same time is_still_alive doing its job. specifically, after is_still_alive check failed and before RMB deleted the hash field for that command? not sure how this is done in Redis but we need to lock the hash after retrieving it and till we do the checks and possibly remove some keys if needed.

sameh-farouk commented 2 years ago

@muhamadazmy suggested a much cleaner flow, I'll quote his reply from tg conversation below:

I would prefer instead of using hash table. instead just simple keys with TTL set by the app (say 30 sec) then the app keep updating the ttl as long as it's a live (say every 15 seconds) if it dies the keys will be gone in max of 15 seconds. hence rmb all what it need to do when it receives a command to check if there the key for that command still exists if it does, pushes it to the queue. if the key has expired then it replies with service not available error.