-
Notifications
You must be signed in to change notification settings - Fork 31
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.
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
[CDN] Implement hooks for DDoS protection #3664
base: main
Are you sure you want to change the base?
Conversation
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this better performance than async_std mutex? just curious why we use this libraries mutex
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, related comment here. I can just go with the std
mutex if we need
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked over the message_hook.rs file pretty closely (as described in your description) and it looks good to me. Just some questions and nits. Also took a glance over everything else which also seemed good
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch on the duplicate logic, will remove it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks good to me
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.rs
into more relevant, smaller filesgenerator
argumentsThe 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.