unbound: add configurable dnsbl response

[shonjir]

The default DNSBL response of a static NULL A record causes some poorly written sites and mobile apps to spin needlessly on connection failures to 0.0.0.0. Further, the DNSBL does not block IPv6 requests which utilize different record types.

This PR adds the ability to configure different response types including NXDOMAIN, full IPv4/IPv6 null response and request logging. It includes the same "log only" functionality as PR #4715 but is more generalized. The configuration is marked as 'advanced' and retains current OPNsense behavior by default.

Response types added by this PR are 'always_nxdomain', 'always_null', 'redirect', 'inform_redirect', and 'inform'. Additional types may be easily added should the need arise, although few of the other types are useful in a DNSBL context.

[kulikov-a]

@shonjir sorry, so (if I understood the code correctly) there is no way to keep the current behavior (static 0.0.0.0) and add logging (inform zone type) as #4715 do?

[shonjir]

Hi, @kulikov-a!

With the current code only a single record is generated for each zone, either a static A record (and an implicit transparent zone) or an explicit zone definition with no local data.

For the inform, inform_redirect, and redirect zone types if local data is specified after the zone type definition then that data is returned instead of the default behavior. The always_nxdomain and always_null zone types do not support local data under the zone and will always return either NXDOMAIN or a null record.

It is possible to add an A 0.0.0.0 record after the zone type definition for some of these types. This would return an A 0.0.0.0 record for A record requests, and (most likely) NOERROR NODATA for all other record requests. I can't say for certain whether this would produce the desired DNSBL behavior under all circumstances.

The code could be updated to optionally return a null record for the types that support it but in my opinion this would result in suboptimal client behavior versus NXDOMAIN. That same code could be extended to support client connection logging by changing the record to point to an arbitrary target address (or addresses). However, this will most definitely lead to the same suboptimal client connection behavior that I used to experience when I was running pfBlocker-NG on pfSense.

I honestly think that the suboptimal behavior isn't worth the effort but hey, open source is all about options, right?

[kulikov-a]

@shonjir hi!)

It is possible to add an A 0.0.0.0 record after the zone type definition for some of these types. This would return an A 0.0.0.0 record for A record requests, and (most likely) NOERROR NODATA for all other record requests. I can't say for certain whether this would produce the desired DNSBL behavior under all circumstances.

yes, I'm only talking about adding the "Return static 0.0.0.0 record (default) and log the request" option.
in such a way that the current behavior (implicit transparent type with local-data in it) will be preserved but logging will be added (by replacing the type with an explicit inform type and local-data in it). there were repeated questions about the ability to track blocked requests

but hey, open source is all about options, right?

absolutely! I'm just afraid that the devs will not be happy to return to the discussion of zone types and dnsbl.conf file again and therefore I got into your request, sorry)

[shonjir]

I can add the functionality into this PR. I only created this one because your PR #4715 needed to be refactored for current master and it was easier to create a new PR with the features I wanted.

The main difficulty is how best to arrange the UI elements so that the combinations are clearly indicated to the user and invalid combinations are avoided. I think that the fact that the checkbox does nothing on some zone types should be clearly indicated in the UI.

[shonjir]

I've updated the PR to include an option that will add static null A and AAAA records for the redirect and inform response types. Testing is appreciated.

@kulikov-a, I think you'll get the behavior you're looking for if you select "Log and return local data or resolve normally" and tick the "Static Local Data Record" option.

Here's an example of the output.

local-zone: "privetadblock.ru" inform_redirect
local-data: "privetadblock.ru A 0.0.0.0"
local-data: "privetadblock.ru AAAA ::"

[kulikov-a]

@shonjir I like it, thank you very much!)

I think you'll get the behavior you're looking for

yep. inform type with local-data is what I was looking for (although adding an AAAA record makes the dnsbl.conf even larger and further increases the unbound load time. but the way to load the blocklist is a separate discussion imho)

thanks again! remains to wait for the devs opinion )

[fichtner]

This manipulates model data. Not great for maintenance or extensibility. Would be better to add constraints to model itself.

[shonjir]

I admit that I was not really happy with this since it results in a UI checkbox that does nothing for some settings but didn't know how to address that in the UI. I'm more than happy to move this into the model. I'll look for a similar example in the code for inspiration but would be appreciative if you could point me to a best-practice example that you'd like me to follow.

In the UI I've currently set the localzone option as a dropdown. Do you think it would be better to expose this as radio buttons since the list is short?

[shonjir]

I was looking into the model documentation and examples and I'm not finding any standard constraints that appear to support the required logic. I need to either ensure that the localdata setting is unset (set to false) if the localzone setting is default, always_null or always_nxdomain, or alternatively the localdata setting should only be enabled if the localzone setting is redirect, inform_redirect or inform, and otherwise set to false.

The SetIfConstraint logic seems mainly intended to ensure a field is set if another field has a given value but there doesn't seem to be a way to specify what value the field should be set to. I could reverse the logic and use the boolean to disable adding custom data, but I would rather not as I was trying to set this up to be extended with the ability to add user-specified responses later on.

At this point I suspect the logic in blocklist.py might be the easiest way to implement this since adding records is only supported for certain zone types.

@fichtner How would you suggest I proceed?

[kulikov-a]

@shonjir @fichtner may DependConstraint work if we change logic a bit: replace "DNSBL Null Response" checkbox with "Block subdomains" checkbox (or some). and then use this field to constrain the selection of Response (zones) Types?

[shonjir]

Whether we create two constrained lists or roll it into a single list the net effect is the same - we'd need to embed the boolean flag into the option values (think "inform:null" vs "inform:nxdomain") and add logic to blocklist.py to take action based on the embedded flag value. The net effect is that we're really just passing the boolean value in a different way and moving the test to a slightly different place, but it does avoid passing invalid combinations to blocklist.py that need to be tested for there.

It's pretty crufty in my opinion to embed flags in what are otherwise clean option values since you have to make some assumptions as to option format that you didn't need to before. It would be cleaner if the necessary conditions and/or target values could be specified directly in the constraints.

[kulikov-a]

agree. since it still need to hide the localdata checkbox at certain localzone option values, maybe it makes sense to add some js-magic to the .volt to set localdata<->localzone dependencies? then the data will get into the model already "clean"

[kulikov-a]

and we can try to extend model with AllowIfConstraint. @fichtner are you talking about it?)

[kulikov-a]

something like kulikov-a@a8d91dc ?

[kulikov-a]

@fichtner hi! could you please clarify your point?
@shonjir hi! I am very sorry if the request is stuck due to my interference. I would like the work to continue. I may not appear if it interferes with the process

[shonjir]

I'm thinking about putting the null response changes into a new PR so that the base functionality could be implemented without needing the model updates. This would eliminate the tickbox and the variable UI for this PR. @fichtner, would this be an acceptable path forward for this PR?

[shonjir]

I couldn't figure out how to apply the current model capabilities to the logic I had and extending the model is beyond what I want to tackle with this PR. I have instead removed the code that @fichtner objected to and modified it to accept redirect values supplied by the model. The blocklist.py script checks for local zone types of 'redirect', 'inform_redirect' or 'inform' since local data records are only valid with these types and if override values have been supplied will generate A and/or AAAA records using the supplied values. The net effect is that one can now customize the redirect behavior by supplying IPv4 and/or IPv6 records to return instead of returning NXDOMAIN.

@fichtner I hope this addresses your concerns in a way that is acceptable.

[AdSchellevis]

partially included in #5736