-
Notifications
You must be signed in to change notification settings - Fork 22
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
Make ping rate limit configurable #108
Make ping rate limit configurable #108
Conversation
d119d3a
to
11a166c
Compare
Network/HTTP2/H2/Settings.hs
Outdated
@@ -41,12 +43,13 @@ baseSettings = | |||
, initialWindowSize = defaultWindowSize -- 64K (65,535) | |||
, maxFrameSize = defaultPayloadLength -- 2^14 (16,384) | |||
, maxHeaderListSize = Nothing | |||
, pingRateLimit = 4 |
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.
If you think 4 is too small, you can increase the value.
@kazu-yamamoto , you asked a difficult question 😅
I didn't change the default as I wasn't sure what to change it to, and I wasn't comfortable suggesting something. But since you asked, I spent some time trying to see what other people do. Unfortunately, it is surprisingly difficult to find some concrete guidelines about this. Plenty of sites that tell you "one way to mitigate a ping flood attack is to impose rate limits", but what those rate limits should be, nobody seems to want to say. A summary of what I found: Alternative approachesIt seems most applications impose rate limits in a different manner than
Actual ping rate limitingFor actual rate limiting, I only found two suggestions:
Underlying cause of the many pingsI suspect that the reason I am seeing more pings than the 4/second that ConclusionI'm not sure what to suggest. Quite frankly, I made the ping rate limit a parameter in my library too, defaulting to 10/second for now, but of course that is basically passing the bucket further down the line to the application 😁 I agree with you that sensible defaults would be better. Option (5) from the gRPC guidelines -- that is, imposing a minimum time between successive ping frames without sending data/header frames -- does seem quite sensible, not just for gRPC but also for other applications. If you want, I can try to implement that and see if it works for me, although of course that would be a bigger change to Alternatively, I could imagine some kind of pluggable "rate limiting API" which could be instantiated to any of the 5 options above; I'd have to spend some time thinking about what such an API should look like. Let me know what you'd like me to do. |
@edsko Thank you for this research! Perhaps, it is easy for many programmers to measure time between packets and to count the number of a specific frame but it is hard to implement rate limiting (number of packets per second). I'm not sure it's worth exploring other approaches. |
11a166c
to
4389142
Compare
Ok, I've discussed this with my client and we're okay with this approach. I've force-pushed a commit that changes the default to 10 (and still keeps it configurable). |
4389142
to
ac3e68e
Compare
Also changed the test suite so that the ping protection test is a bit more aggressive so that it still exceeds the limit. |
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.
LGTM
Merged. |
Thanks @kazu-yamamoto ! |
I'm dealing with some gRPC servers (written in golang) that routine exceed the limit of 4 pings/sec (increasing the limit to 10 seems to do the trick). This PR makes the ping rate limit configurable.