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.

Sign up for GitHub

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

Jump to bottom

Better detection of request recursion #3185

Closed
ainar-g opened this issue May 24, 2021 · 2 comments
Closed

Better detection of request recursion #3185

ainar-g opened this issue May 24, 2021 · 2 comments
Assignees
@EugeneOne1
Labels
enhancement P2: High
Milestone
v0.107.0

Comments

@ainar-g
Copy link
Contributor

ainar-g commented May 24, 2021 •
edited

The two mechanisms that were proposed by our dev team are:

  1. Compare client IPs to the list of known upstreams. This is easy, but doesn't work in situations where there are more than two hops.
  2. Have a LRU map of IDs of requests, along with information like query type and domain. A bit more complex, but provides better overall support.

I personally think that we should go with option two. @ameshkov, do you have anything to add?
@ainar-g ainar-g added this to the v0.107.0 milestone May 24, 2021
@EugeneOne1
Copy link
Member

The second option (i.e. LRU cache) have been chosen and implemented in 48b8579.

@ameshkov
Copy link
Member

ameshkov commented May 30, 2021 •
edited

The way it's implemented is dangerous.

This is completely normal to receive DNS queries with the same ID, DNS clients retransmit the same query when they do not receive the response in N seconds (timeout is implementation-specific).

Consider this point: we are only interested in the recursion detection when the query is sent to "Private reverse DNS servers". We are not interested in other cases.

@ameshkov ameshkov reopened this May 30, 2021
heyxkhoa pushed a commit to heyxkhoa/AdGuardHome that referenced this issue Mar 20, 2023
@EugeneOne1 @heyxkhoa
Pull request: 3185 detecting recursion
eff3e71 
Merge in DNS/adguard-home from 3185-recursion to master

Closes AdguardTeam#3185.

Squashed commit of the following:

commit 2fa4422
Merge: 7975957 7a48e92
Author: Eugene Burkov <e.burkov@adguard.com>
Date:   Thu May 27 19:04:44 2021 +0300

    Merge branch 'master' into 3185-recursion

commit 7975957
Author: Eugene Burkov <e.burkov@adguard.com>
Date:   Thu May 27 17:36:22 2021 +0300

    dnsforward: imp docs

commit 1af7131
Author: Eugene Burkov <e.burkov@adguard.com>
Date:   Thu May 27 17:15:00 2021 +0300

    dnsforward: imp code, tests, docs

commit f3f9145
Author: Eugene Burkov <e.burkov@adguard.com>
Date:   Thu May 27 15:45:44 2021 +0300

    dnsforward: add recursion detector
heyxkhoa pushed a commit to heyxkhoa/AdGuardHome that referenced this issue Mar 20, 2023
@EugeneOne1 @heyxkhoa
Pull request: 3185 fix recursion vol.2
dc2372c 
Merge in DNS/adguard-home from fix-recursion to master

Closes AdguardTeam#3185.

Squashed commit of the following:

commit c78650b
Author: Eugene Burkov <e.burkov@adguard.com>
Date:   Mon May 31 13:12:46 2021 +0300

    all: fix changelog

commit e43017a
Author: Eugene Burkov <e.burkov@adguard.com>
Date:   Sun May 30 22:39:05 2021 +0300

    dnsforward: reduce recursion ttl

commit 79208a8
Author: Eugene Burkov <e.burkov@adguard.com>
Date:   Sun May 30 22:29:27 2021 +0300

    dnsforward: only check recursion for private rdns

commit 1b8075b
Author: Eugene Burkov <e.burkov@adguard.com>
Date:   Sun May 30 22:18:11 2021 +0300

    dnsforward: rm recursion detecting from upstream
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@EugeneOne1 EugeneOne1

Labels
enhancement P2: High
Projects
None yet
Milestone
v0.107.0
Development

No branches or pull requests
3 participants
@ainar-g @ameshkov @EugeneOne1