Skip to content
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] Rate limiting - DDoS protection #3774

Open
wants to merge 24 commits into
base: main
Choose a base branch
from
Open

[CDN] Rate limiting - DDoS protection #3774

wants to merge 24 commits into from

Conversation

rob-maron
Copy link
Collaborator

@rob-maron rob-maron commented Oct 18, 2024

Successor to #3664

This PR:

(Includes all of the changes in #3770)

Implements CDN rate limiting. Let me describe it:

  • The rate limiting is done with a generic-cell-rate algorithm (a variant of leaky bucket) with an almost unbounded burst capacity. This lets us allow bursty traffic (not breaking anything) through while also keeping a leash on the rate of messages coming in. It is applied independently for direct and broadcast traffic.
  • The "refill rate" is dynamically adjusted through tracking of the average bytes processed per node per message type. Every minute (the commit interval) we adjust the refill rate to 2x the global average of the last minute. Since we allow burst traffic, it will only hinder you if you're consistently over this for a long period of time.
  • To prevent nodes from reconnecting and clearing their limits, on drop we store their current limits along with their CDN-supplied unique identifier (hash of public key) so the same limit can be assigned on reconnect. In the event we have multiple rate limits (e.g. the limit gets dropped after a reconnect), we use the one with the least capacity
  • One issue I can think of is that this technically bounds the upper limit of bytes/minute to 4GB, which is 533Mbps. I think this should be fine

Reviewers:

Most of the changes are in a single file, message_hook.rs. The majority of the rest of the changes are breaking out the CDN code into smaller, easily-digestible files

@rob-maron rob-maron marked this pull request as ready for review November 1, 2024 18:26
@rob-maron rob-maron changed the title [Draft] CDN GCRA [CDN] Rate limiting - DDoS protection Nov 1, 2024
Comment on lines 8 to 18
async-trait = { workspace = true }
clap = { workspace = true }
committable = { workspace = true }
derive_more = { workspace = true }
futures = { workspace = true }
hotshot-types = { path = "../types" }
serde = { workspace = true }
thiserror = { workspace = true }
tagged-base64 = { workspace = true }
thiserror = { workspace = true }
tide-disco = { workspace = true }
toml = { workspace = true }
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cargo-sort did this

Comment on lines -15 to +32
async-trait = { workspace = true }
anyhow = { workspace = true }
sha3 = "^0.10"
async-lock = { workspace = true }
async-trait = { workspace = true }
committable = { workspace = true }
hotshot = { path = "../hotshot" }
hotshot-types = { path = "../types" }
hotshot-task-impls = { path = "../task-impls", version = "0.5.36", default-features = false }
hotshot-types = { path = "../types" }
jf-vid = { workspace = true }
rand = { workspace = true }
thiserror = { workspace = true }
reqwest = { workspace = true }
serde = { workspace = true }
sha2 = { workspace = true }
sha3 = "^0.10"
thiserror = { workspace = true }
time = { workspace = true }
async-lock = { workspace = true }
jf-vid = { workspace = true }
vbs = { workspace = true }
url = { workspace = true }
reqwest = { workspace = true }
tokio = { workspace = true }
url = { workspace = true }
vbs = { workspace = true }
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also cargo-sort

crates/hotshot/Cargo.toml Outdated Show resolved Hide resolved
@@ -0,0 +1,577 @@
#![allow(clippy::unnecessary_wraps)]
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file contains the majority of the changes

Comment on lines +96 to +104
/// A sample representing the number of bytes processed per period
#[derive(Clone)]
pub struct Sample {
/// The number of bytes processed
num_bytes: f64,

/// The time of the last commit
last_committed_time: Instant,
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This Sample is held locally for each CDN connection and is used to occasionally the global average

};

// Check against the `GCR` instance, skipping if we're rate limited
// If we hit a parameter error, we'll process the message anyway
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A parameter error is internal to Gcr, it can be hit if, say, you try to update the average to zero. This makes it so we don't accidentally block all messages everywhere if there was an issue with that

@@ -0,0 +1,90 @@
use std::marker::PhantomData;

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file has just been moved

use bincode::Options;
use cdn_broker::reexports::crypto::signature::{Serializable, SignatureScheme};
use hotshot_types::{traits::signature_key::SignatureKey, utils::bincode_opts};

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file has just been moved

@@ -0,0 +1,28 @@
use hotshot_types::traits::metrics::{Counter, Metrics, NoMetrics};

/// CDN-specific metrics
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file has just been moved

pub mod definition;
/// The metrics for the Push CDN
pub mod metrics;

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was the big file everything else moved from

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant