dns/bind: allow to set recursion no

[mdbraber]

Important notices
Before you add a new report, we ask you kindly to acknowledge the following:

• I have read the contributing guide lines at https://github.com/opnsense/plugins/blob/master/CONTRIBUTING.md
• I have searched the existing issues, open and closed, and I'm convinced that mine is new.
• When the request is meant for an existing plugin, I've added its name to the title.

Is your feature request related to a problem? Please describe.
When not setting recursion to an ACL it defaults to yes. There is no option to set it to no instead of defining a limited ACL (e.g. Local) but this not the same.

Describe the solution you'd like
Allow an option no when setting recursion. Possibly create yes with the option to select an ACL or no with no other options.

Describe alternatives you've considered

• A very limited local ACL
• Creating a manual configuration

[benyamin-codez]

I've been working on a solution which includes this for a couple of weeks.

Here's a screenshot of the configuration page:

I also added more rate limiting features and updated help text.

Just coming back to it after a week of testing with great results in mitigating DNS amplification and cache harvesting attacks.

Was looking at upgrading ACLs to AMLs (BIND language: Address Match Lists) with access to builtin labels for none, any, localhost and localnets and any necessary migration scripts before raising the PR.

[benyamin-codez]

FYI - My topic in the forum: https://forum.opnsense.org/index.php?topic=48271.0

[mdbraber]

That would be a great update!

Btw - for my homelab I 'just' wanted to DROP any replies for zones that aren't mine, so I ended up making Bind from /usr/ports and patching it (I should probably make this part of named config, but for now it works):

--- lib/ns/query.c.orig	2025-07-04 11:42:08.449330850 +0200
+++ lib/ns/query.c	2025-08-11 14:25:02.293187000 +0200
@@ -5865,7 +5865,8 @@
 					  ns_statscounter_authrej);
 			}
 			if (!PARTIALANSWER(qctx->client)) {
-				QUERY_ERROR(qctx, DNS_R_REFUSED);
+				// QUERY_ERROR(qctx, DNS_R_REFUSED);
+				QUERY_ERROR(qctx, DNS_R_DROP);
 			}
 		} else {
 			CCTRACE(ISC_LOG_ERROR, "ns__query_start: query_getdb "

Another solution for anyone looking into this, might be to check dnsdist

[benyamin-codez]

That patch is a common solution but I didn't think a custom build of BIND would get up here.

A popular alternate solution when using iptables is to inspect the RAW packet and drop any REFUSED replies.
I don't believe there is a comparable solution with pf in FreeBSD.

As it stands, the migration is somewhat complicated because the ACLs need to be made from config data and then the model data updated with the relevant new ACL. I might raise the PR and ask @fichtner and @mimugmail for their feedback and also for advice wrt overloading the NetworkType field NetworkField type.

[mdbraber]

No, a patched version of Bind should only be local. The possible alternative of dnsdist is actually the most future-proof way to solve this

[fichtner]

I'm not sure what "also for advice wrt overloading the NetworkType field" means but for ACL advanced settings it would be good to dump the current generated output and the expected output in order to introduce the relevant option.

[benyamin-codez]

@fichtner

I wanted to make the builtin ACLs available, i.e. none, any, localhost and localnets.
Those ACLs could then be selected in the other tabs as needed - notably in General.
My initial thought was to overload the NetworkField type. My bad on my previous typo. 8^d>~
However, an alternative might be to expand the model at mvc/app/models/OPNsense/Bind/Acl.xml...

Anyway, I've raised draft PR #4892, so perhaps we can discuss it there...

I also managed to fix the faulty zone check and also tidied up the Primary Zone table so the Commands aren't truncated.

[Monviech]

@benyamin-codez If you would go one step at a time with smaller changes the likelyhood of a successful review is higher. Dumping everything into a single PR makes it hard.

[fichtner]

@benyamin-codez yes we don't want to overload field types and it breaks delegation of validation.

I also think if you want to fix something make a single PR first. The PR currently has structural issues changing the model in a backwards incompatible way that not a lot of people will appreciate.

Cheers,
Franco

[benyamin-codez]

@Monviech @fichtner

Thanks for the feedback. I'm willing to split the PR if need be.

I've updated the PR with a squashed commit.

Any suggestions on how to address the backwards compatibility concerns?
Is this because the plugin updates independently of the OPNsense version...
...with older OPNsense boxes using a new plugin?
Or is it a downgrade concern?

I was hoping the migration script would handle the model upgrade.
If can keep the model refactoring squashed this will help.

Perhaps these questions are better answered in the PR...
Also hoping you can provide some insight as to how I might add some static entries to the TextField type...

With kind regards,
Ben

cc: @mimugmail

[benyamin-codez]

I've split the recursion no; and allow-query-cache { address-match-entry; ...} part to PR #4897.

I left the model alone insofar as I didn't change general.recursion to general.recursionallowedacls.
As such, there shouldn't be any backwards compatibility issues AFAICT...

[benyamin-codez]

The possible alternative of dnsdist is actually the most future-proof way to solve this

@mdbraber

I agree Maarten, dnsdist is probably the best approach for FreeBSD-based platforms. Once my efforts with named are complete, I'll see if I can get a rudimentary dnsdist plugin working. This might give a little more time for v2.0.0 to get ported too... ;^D

In the meantime, the BIND-based rate-limiting is working sufficiently for my needs...
...but it would be nice to actually drop those pesky ones..!

[mdbraber]

Awesome! Please ping me if you're building a dnsdist plugin. Thanks for this refactoring effort with named!