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

Move rate-limiting from per-client to per-client-per-domain #1468

Closed
wants to merge 3 commits into from

Conversation

DL6ER
Copy link
Member

@DL6ER DL6ER commented Nov 6, 2022

What does this implement/fix?

Let me be clear here: Rate-limit is there it protect you. It is not meant as a method to cause pain. What are we protecting against? Basically against Denial of Service (DoS) "attacks" either by a malicious attacker or by some client that has a bug or crashed or ... (you never know, we have seen so may things). If one attacker or or misbehaving client can take down your entire network because your DNS resolver runs out of memory - that doesn't sound like something you'd want, right? - and this is where FTL's rate-limiting kicks in, simply discarding queries when there are too many. The default settings are suitable both for home and enterprise, small and huge networks, however, they can always be adjusted (or even disabled) to your liking if you feel you need no DoS-protection.

This PR proposes to change FTL's rate-limiting mechanism from a pure "per-client" to a "per-client and per-domain" scheme. If a client queries domain.com like hell, it will get rate-limited as before. However, any other domain (say example.com) can still resolved (except, obviously, it it gets queried too often, too).

This reduces the "there is not Internet connection" effect some users reported at the costs of a much less effective rate-limiting as clients and/or malicious attackers can simply evade it by querying varying domains. We have seen this by buggy clients querying seemingly random subdomain - each of them would be counted on their own.

Related issue or feature (if applicable): N/A

Pull request in docs with documentation (if applicable): to be done


By submitting this pull request, I confirm the following:

  1. I have read and understood the contributors guide, as well as this entire template. I understand which branch to base my commits and Pull Requests against.
  2. I have commented my proposed changes within the code.
  3. I am willing to help maintain this change if there are issues with it later.
  4. It is compatible with the EUPL 1.2 license
  5. I have squashed any insignificant commits. (git rebase)

Checklist:

  • The code change is tested and works locally.
  • I based my code and PRs against the repositories developmental branch.
  • I signed off all commits. Pi-hole enforces the DCO for all contributions
  • I signed all my commits. Pi-hole requires signatures to verify authorship
  • I have read the above and my PR is ready for review.

…f an array of strings to ensure we cannot forget to update the strings when we add new types in the future (the compiler would complain about not having handled all possible switch cases)

Signed-off-by: DL6ER <dl6er@dl6er.de>
…er-client but also per-domain

Signed-off-by: DL6ER <dl6er@dl6er.de>
@DL6ER
Copy link
Member Author

DL6ER commented Nov 21, 2022

Rebased on latest development

return "HOSTNAME";
case DNSMASQ_CONFIG_MESSAGE:
return "DNSMASQ_CONFIG";
case RATE_LIMIT_MESSAGE:
Copy link
Member

Choose a reason for hiding this comment

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

Why do we keep this here? RATE_LIMIT_MESSAGE is not used anymore.

return false;
}

// Get child's DNS cache entry (we eed to create a new cache entry here
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Get child's DNS cache entry (we eed to create a new cache entry here
// Get child's DNS cache entry (we need to create a new cache entry here

@@ -211,6 +211,7 @@ enum message_type {
SHMEM_MESSAGE,
Copy link
Member

Choose a reason for hiding this comment

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

Still need L208 "RATE_LIMIT_MESSAGE"?

@DL6ER
Copy link
Member Author

DL6ER commented Nov 21, 2022

The unique change we added recently won't work here as uniqueness is now composed out of three fields for this new message type (type + client + domain). Needs more work.

@github-actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot added the stale label May 11, 2023
@github-actions
Copy link

Existing merge conflicts have not been addressed. This PR is considered abandoned.

@github-actions github-actions bot closed this May 11, 2023
@DL6ER DL6ER deleted the tweak/rate-limiting branch October 29, 2023 19:51
@yubiuser yubiuser mentioned this pull request Jan 4, 2024
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants