[CDN] Implement hooks for DDoS protection #3664
Conversation
rob-maron
changed the title
| use cdn_broker::reexports::def::hook::{HookResult, MessageHookDef}; | ||
| use cdn_broker::reexports::message::{Broadcast, Direct, Message as PushCdnMessage}; | ||
| use lru::LruCache; | ||
| use parking_lot::Mutex; |
There was a problem hiding this comment.
is this better performance than async_std mutex? just curious why we use this libraries mutex
There was a problem hiding this comment.
Yeah, related comment here. I can just go with the std mutex if we need
There was a problem hiding this comment.
no need more out of curiosity
| let mut hasher = Hash64::default(); | ||
| hasher.write(&direct.message); | ||
| hasher.write(&direct.recipient); | ||
|
|
||
| // Make sure we have not already seen it | ||
| if self.message_hash_cache.put(hasher.finish(), ()).is_some() { | ||
| return Ok(HookResult::SkipMessage); | ||
| } |
There was a problem hiding this comment.
nit: we are doing same hashing logic in process_broadcast_message and process_direct_message can we extract this to another method?
There was a problem hiding this comment.
not sure if itll be easy since we are doing hash on Broadcast and Direct structs, wonder if we can make them params somehow if not no worries
There was a problem hiding this comment.
Good idea, let me see if I can abstract this out
| // fn deserialize_message(message: &[u8]) -> Result<(Message<T>, Version)> { | ||
| // // Hack off the version | ||
| // let (version, message) = | ||
| // Version::deserialize(&message).with_context(|| "failed to deserialize message")?; | ||
|
|
||
| // // Deserialize the message | ||
| // let message = Serializer::<StaticVersion<0, 1>>::deserialize_no_version(&message) | ||
| // .with_context(|| "failed to deserialize message")?; | ||
|
|
||
| // // Return the version and message | ||
| // Ok((message, version)) | ||
| // } | ||
| } |
There was a problem hiding this comment.
i see some stuff commented out will this be used in the future or can we clean up?
There was a problem hiding this comment.
Yeah - we may want to use it in the future.
Right now the hooks are completely agnostic to HotShot because of serialization/versioning. If we want to know what kind of HotShot message the user sent, we have to basically run consensus to figure out what version we should be asking for
In the future this may become more important (e.g. only let a node vote once per view or something) but we're not doing it quite yet
|
Drafting while I fix up |
| let now = Instant::now(); | ||
|
|
||
| // Commit the sample if that interval has elapsed | ||
| if now.duration_since(sample.last_committed_time) >= self.sample_commit_interval { |
There was a problem hiding this comment.
even if we are in cool we still add to global sma? if we are in cooldown for a while wont bytes_sent for sample always be 0?
There was a problem hiding this comment.
also seems below we have this logic probably want to remove it as we reset sample and the second check (line 246) shouldnt get hit IIUC
There was a problem hiding this comment.
For adding to the global during cooldown: I wanted to avoid the chance that everyone is in cooldown and nobody is committing anything. Even if you send a really big message once and get put in jail I still think it should count
There was a problem hiding this comment.
Good catch on the duplicate logic, will remove it
Reviews
The only file that actually needs review is
message_hook.rs. Everything else is either removing unused code or moving it around.This PR:
Allows for "hooking" incoming messages from both brokers and users to implement custom logic. This will be the main source of DDoS protection measures.
push_cdn_network.rsinto more relevant, smaller filesgeneratorargumentsThe current hooking functionality:
Uses an SMA algorithm. If the connection's average bytes per second (separate for direct/broadcast) is larger than the global average bytes per second (within a sliding window of 1,000), the user is disconnected.
Testing
This PR has unit tests for expected functionality, and has been ran on the local demo to make sure it doesn't cause any issues.