-
Notifications
You must be signed in to change notification settings - Fork 3
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
Bad Actor Split and many new features/fixes #4
Conversation
src/metrics.ts
Outdated
need for `userLag` as well. | ||
*/ | ||
|
||
// hosts |
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: could DRY into getTopOffenders(records, split) { ... }
src/connect.ts
Outdated
}; | ||
} | ||
|
||
shouldThrottleRequest(req): string|boolean { |
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: maybe some more use of literal types
shouldThrottleRequest(req): BadActorType | 'userLag' | false
ipWhitelist?: Set<string>; | ||
} | ||
|
||
export class Metrics { |
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.
could timestamps be helpful? to help track snapshots and differentiate between spikes and slow buildups
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 with the current algorithm since it uses a sliding window of time. But future versions might use a progressive algorithm and have need for time. We might even be able to move away from N requests and use a time window instead, but out of scope for this PR.
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.
Nice
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.
👍
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
hitRatio
has been replaced byhostBadActorSplit
andipBadActorSplit
so that we're throttling the top offenders regardlessif they hit an arbitrary percentage of traffic
waitForHistory
(enabled by default) whichprevents
userLag
from being triggered prematurely before we havesufficient evidence/history
isBadHost
orisBadIp
will now update history.This will make for more accurate bad actor detection for scenarios that
leverage pre-request tracking (such as TLS SNI) in cases that result
in large volumes of pre-middlware rejections
hostWhitelist
andipWhitelist
optionsif you want to prevent certains hosts or IP's from ever being blocked
Philosophy
The basic idea is that for a given tracking window that we identify
the TOP OFFENDERS, regardless of ratios. During nominal windows
bad actors are irrelevant as they won't be looked at until the system
becomes too busy. But during an attack window, we want to be on the
lookout for as many possible actors as the amount of throughput
reduction desired is substantial. The default
BadActorSplit
of 0.5simply means that we consider the most active 50% of traffic to
be susceptible to throttling.
Future
There is an opportunity to support tiered throttling so the
aggressiveness of the throttling is porportional to how busy the system
becomes. For example, instead of a flat 50% split @ 70ms lag, it could
look something like:
* 10% split @ 70ms
* 20% split @ 80ms
* ...
* 90% split @ 160ms