-
Notifications
You must be signed in to change notification settings - Fork 359
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
RateLimiter: Allow grouping of IP addresses to share a quota #4064
Conversation
$ipv4Octets = $policy['groupByIpv4Octets'] ?? null; | ||
$ipv6Hextets = $policy['groupByIpv6Hextets'] ?? null; | ||
if ($ipv4Octets || $ipv6Hextets) { | ||
$ipUtils = new \VuFind\Net\IpAddressUtils(); |
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'd like to just reuse the instance obtained in the factory above, but not sure how to do that without passing it as a parameter into getRateLimiter, which I don't love. Hoping there is a better option.
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.
You should be able to use $this->serviceLocator->get(\VuFind\Net\IpAddressUtils::class)
here, if I'm reading things correctly.
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.
D'oh, I missed that variable assignment in invoke! Fixed.
config/vufind/RateLimiter.yaml
Outdated
# botnets that control a range of IPs. This config defines | ||
# the group by truncating the IP address, if it is IPv4, to the | ||
# given number of octets. For example, setting this to 3 would | ||
# truncate 1.2.3.4 to 1.2.3 for the purpose of grouping. |
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.
Perhaps add a mention that you should only use values 1-3 to avoid extra logic in address handling (and similar for IPv6)?
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.
Yup, larger numbers would work fine but not 0 or negative. I added valid values to the docs. (Technically 4 is also valid for ipv4 but would be pointless, so I agree with saying 1-3.)
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'm afraid this was dropped from my radar without an active review request. I think we need this to counter some widespread abuse, so approving now!
No worries, I had it in my own production since the fall. |
RateLimiter can already apply a policy based on the client's IP address/range, but each IP address still has an individual quota of tokens. This allows a set of related IP address to share the same quota. So for example if botnet uses hundreds of different IPs from the same subnet, we can apply all that traffic to the same quota and shut it down faster.