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

minor frame handler improvements #514

Merged
merged 2 commits into from
Jan 6, 2024

Conversation

schlawg
Copy link
Contributor

@schlawg schlawg commented Dec 30, 2023

  • exempt heartbeat (null) pings from rate limiting. budgeting for endpoints shouldn't have to accommodate these.
  • close connections when rate limits are exceeded or payload cannot be processed, releasing resources immediately and giving TCP FINs to clients.

@schlawg schlawg force-pushed the when-attacked-release-resources branch from 3cf841f to 61cfe67 Compare December 31, 2023 15:01
@schlawg schlawg changed the title close connections when rate limits are exceeded minor frame handler improvements Dec 31, 2023
@schlawg schlawg requested a review from niklasf December 31, 2023 15:11
@schlawg
Copy link
Contributor Author

schlawg commented Dec 31, 2023

I requested review due to the effect that rate limit exemption for pings will have on the endpoint budgets. I could adjust them all downwards by 10-15, but someone more familiar with the ovh infrastructure will know better.

Rate limit exemption for pings is desired (at least temporarily) to determine whether smaller ping intervals (but still north of 1 second) can mitigate connection losses when aggressive connection management policies are applied that we can't control.

It is critical to understand that any reduction in ping interval will be performed on a case by case basis, I am not proposing to reduce this value in the general case.

I think it's OK because I suspect heartbeat pings are more effort and less reward than other methods of DDoS attacks.

Copy link
Member

@niklasf niklasf left a comment

Choose a reason for hiding this comment

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

lgtm. ClientOut.Ping(None) will do only trivial in-memory updates, so only marginally more work than parsing the ws frame in the first place.

@schlawg
Copy link
Contributor Author

schlawg commented Jan 1, 2024

Do you think the rate limits should be tightened a bit now or are they still OK?

@niklasf
Copy link
Member

niklasf commented Jan 2, 2024

I think it's easier and safer to keep as is. Attackers wouldn't have had to spend budget on these, so it's not a regression. It would be an opportunity for hardening, but we don't really need it (at this time).

@ornicar ornicar merged commit 5f9ade6 into lichess-org:master Jan 6, 2024
1 check passed
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.

3 participants