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

Mention REFUSED has the TC bit set with unmatched allow_cookie acl in the manpage #1010

Merged
merged 5 commits into from Feb 20, 2024

Conversation

wtoorop
Copy link
Member

@wtoorop wtoorop commented Feb 7, 2024

Also moved the part about bypassing ip-ratelimit to the ip-ratelimit description as it will be bypassed with a valid DNS-Cookie regardless of the allow_cookie acl.

Also moved the part about bypassing ip-ratelimit to the ip-ratelimit
description as it will be bypassed with a valid DNS-Cookie regardless of the
allow_cookie acl.
doc/unbound.conf.5.in Outdated Show resolved Hide resolved
doc/unbound.conf.5.in Outdated Show resolved Hide resolved
doc/unbound.conf.5.in Outdated Show resolved Hide resolved
doc/unbound.conf.5.in Outdated Show resolved Hide resolved
doc/unbound.conf.5.in Outdated Show resolved Hide resolved
doc/unbound.conf.5.in Outdated Show resolved Hide resolved
Copy link
Member

@wcawijngaards wcawijngaards left a comment

Choose a reason for hiding this comment

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

Okay, glad my suggested part of the text made it through, mostly :-) . I feel it is okay to have a reference to the cookie ratelimit, or its difference to ordinary ratelimit at the allow_cookie description, but also the new location for the text is more genuine since other access control settings can have cookie traffic that uses the cookie ratelimit. So I am happy with the current solution.

@gthess
Copy link
Member

gthess commented Feb 7, 2024

Can we please squash before merging this? :)

@wtoorop
Copy link
Member Author

wtoorop commented Feb 7, 2024

Can we please squash before merging this? :)

Wait a minute, looking at the code I'm not convinced ip-ratelimit will be bypassed with a valid cookie regardless of the allow_cookie acl. @gthess perhaps pre_edns_ip_ratelimit should be set if worker->env.cfg->answer_cookie is set instead of based on allow_cookie acl, WDYT?

@gthess
Copy link
Member

gthess commented Feb 7, 2024

That's true. Currently only if you demand clients with cookies (i.e., allow_cookie in acl) you can bypass the ratelimit.
I am not sure what is the correct behavior.
You would expect that by presenting a cookie the client would be allowed through ratelimit even if demand for it is not necessary? But that makes sense only in case you do ip-ratelimit as a countermeasure for spoofing. I mean maybe you just want to ratelimit access for certain IPs. If they start doing cookies they are through.

@gthess
Copy link
Member

gthess commented Feb 7, 2024

But you are right, with the current code the text is wrong.

@wtoorop
Copy link
Member Author

wtoorop commented Feb 7, 2024

I mean maybe you just want to ratelimit access for certain IPs. If they start doing cookies they are through.

But currently ip-ratelimit is global anyway. There is no rate-limiting for certain IPs, right?
If you want to do it regardless of cookies, you can also set the ip-ratelimit-cookie setting. This would be conform the docs.

@wtoorop wtoorop requested a review from gthess February 7, 2024 15:48
@gthess
Copy link
Member

gthess commented Feb 7, 2024

Ah you are right, I was confused with regular ratelimit to the upstream where it is not global only.
Then your current change makes sense to me.

@gthess
Copy link
Member

gthess commented Feb 20, 2024

So this looks good, merging.

@gthess gthess merged commit e1229e3 into master Feb 20, 2024
1 check passed
@gthess gthess deleted the devel/fix-allow_cookie-doc branch February 20, 2024 14:29
gthess added a commit that referenced this pull request Feb 20, 2024
- Merge #1010: Mention REFUSED has the TC bit set with unmatched
  allow_cookie acl in the manpage. It also fixes the code to match the
  documentation about clients with a valid cookie that bypass the
  ratelimit regardless of the allow_cookie acl.
jedisct1 added a commit to jedisct1/unbound that referenced this pull request Feb 22, 2024
* nlnet/master:
  Changelog entry for NLnetLabs#1010: - Merge NLnetLabs#1010: Mention REFUSED has the TC bit set with unmatched   allow_cookie acl in the manpage. It also fixes the code to match the   documentation about clients with a valid cookie that bypass the   ratelimit regardless of the allow_cookie acl.
  Mention REFUSED has the TC bit set with unmatched allow_cookie acl in the manpage (NLnetLabs#1010)
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.

None yet

4 participants