Wildcard $dnsrewrite interfering with non-wildcard $dnsrewrite

[ruifung]

Prerequisites

• I have checked the Wiki and Discussions and found no answer

• I have searched other issues and found no duplicates

• I want to request a feature or enhancement and not ask a question

Description

What problem are you trying to solve?

Given the following rewrite rules:

|*.services.home.lan|$dnstype=A,dnsrewrite=NOERROR;A;192.168.0.5
|unifi.services.home.lan|$dnstype=A,dnsrewrite=NOERROR;A;192.168.1.2

nslookup unifi.services.home.lan will return both 192.168.1.2 and 192.168.0.5

This behavior is not present when using the Filters > DNS Rewrites functionality.

Proposed solution

Perhaps by specifying a grouping-id to allow only specific rules to apply their records to the rewrite.

Alternatives considered

Or perhaps rewrite rules should only apply their results for the exact same rule match. With more specific rules be preferred over shorter ones.

Additional information

This is undesirable behaviour when the 2 IPs are completely different things, with one being a IP dedicated to a unifi network controller and the other being a general reverse proxy.

[ainar-g]

Hello. You can disable the rule for that subdomain using the denyallow modifier:

||services.home.lan^$dnstype=A,dnsrewrite=NOERROR;A;192.168.0.5,denyallow=unifi.services.home.lan

But I do think that there may be an issue here that should be inspected. In particular, as we are now moving the Filters → DNS rewrites feature to use dnsrewrite rules internally, this may conflict with the current implementation.

@ameshkov, what do you think?

[ameshkov]

One more solution is to disable conflicting dnsrewrite for that particular domain: unifi.services.home.lan

|*.services.home.lan|$dnstype=A,dnsrewrite=NOERROR;A;192.168.0.5
|unifi.services.home.lan|$dnstype=A,dnsrewrite=NOERROR;A;192.168.1.2
@@|unifi.services.home.lan|$dnstype=A,dnsrewrite=NOERROR;A;192.168.0.5

@ainar-g one way to "fix" this is by allowing specifying several records in a single $dnsrewrite rule.

Something like this:

|unifi.services.home.lan|$dnstype=A,dnsrewrite=NOERROR;A;192.168.1.2|NOERROR;A;192.168.1.3

In this case we won't need "merging" dnsrewrite rules and it will simplify everything a lot.
In addition, it will also eliminate the need for @@||example.com^$dnsrewrite type of rules.

The downsides are:

1. This is a breaking change.
2. A rule with many records becomes kinda unreadable.

Anyways, seeing what kind of issues we're having with the current implementation, I tend to think that we should simplify it.

@ainar-g @Mizzick @EugeneOne1 let's vote.

[ruifung]

Hello. You can disable the rule for that subdomain using the denyallow modifier:

That might work well with 1 rule to override, but might make the rule a tad bit long when dealing with multiple rules to override.

I.e. agh-dns.services.home.lan, mqtt.services.home.lan, postgres.services.home.lan, etc.

[Mizzick]

As long as it's considered ok to return a few matching dnsrewrite rules from urlfilter engine, I vote for denyallow or @@-rule solutions.

Otherwise, we might think about to return only one rule with the most priority.

[ainar-g]

@ameshkov, after looking into it, I think that this behaviour is actually caused by the way AdGuard Home processes dnsrewrite rules, and I don't think that module urlfilter has anything to do with this, as its current API simply returns all matched rules.

My proposal is to basically repeat the logic that we currently have for the legacy DNS rewrites. That is, prefer non-wildcard rules to wildcard ones (including || rules). This logic shouldn't be too hard to implement, I think.

Re. multiple rules and breaking changes, I'd rather not go down that road, at least for now.

[ameshkov]

@ainar-g I'd disagree here. The whole point is that urlfilter returns multiple rules, i.e. it expects that all those rules will be applied to the DNS query.

Btw, in any case we should add a better explanation of the modifier behavior here:
https://adguard-dns.io/kb/general/dns-filtering-syntax/#dnsrewrite

And I still argue that we should change the modifier definition better sooner than later.

[ainar-g]

@ameshkov, sorry it took me too long to respond. I don't really get your point about multiple rules, and I do think that breaking people's rules is rather bad, at least in a patch release. Retaining the current legacy-rewrite logic will not only break fewer things but will also make the rules behave in a less surprising way.

Another downside to stacking multiple rewrites onto a single rule is that there is currently a 1024-character limit on a rule length, and a simple DDR response can easily go near or over this limit. For example, with AdGuard DNS:

|_dns.resolver.arpa^$dnsrewrite=NOERROR;SVCB;1 unfiltered.adguard-dns.com. alpn="h3,h2,http/1.1" port=443 ipv4hint=94.140.14.49,94.140.14.59 ipv6hint=2a10:50c0::ded:ff,2a10:50c0::dad:ff key7="/dns-query{?dns}"|NOERROR;SVCB;2 unfiltered.adguard-dns.com. alpn="dot" port=853 ipv4hint=94.140.14.49,94.140.14.59 ipv6hint=2a10:50c0::ded:ff,2a10:50c0::dad:ff|NOERROR;SVCB;3 unfiltered.adguard-dns.com. alpn="doq" port=853 ipv4hint=94.140.14.49,94.140.14.59 ipv6hint=2a10:50c0::ded:ff,2a10:50c0::dad:ff

Yet another way in which this breaks the current implementation is the fact that TXT and some other types can already use | characters, so your solution would also break these:

|txt.example^$dnsrewrite=NOERROR;TXT;some_encoded_token=abcd|1234|qwer

[ameshkov]

@ainar-g makes sense indeed.

Let's keep it as it is then, there's a solution after all.