From ceafd05a0067118333f8e655f5d990e2fcc7eb61 Mon Sep 17 00:00:00 2001 From: John Fieber Date: Sun, 26 Jan 2025 16:45:43 -0800 Subject: [PATCH 1/4] dns/bind: Add BindAddressMatchField and matching validator (#4435) BindAddressMatchField and matching validator is derived from the standard NetworkField and validator. Modifications permit supporting negation (!) and referencing built in ACLs. At this time, it does not support referencing other user defined ACLs. --- .../Bind/FieldTypes/BindAddressMatchField.php | 158 ++++++++++++++++++ .../Validators/BindAddressMatchValidator.php | 149 +++++++++++++++++ 2 files changed, 307 insertions(+) create mode 100644 dns/bind/src/opnsense/mvc/app/models/OPNsense/Bind/FieldTypes/BindAddressMatchField.php create mode 100644 dns/bind/src/opnsense/mvc/app/models/OPNsense/Bind/Validators/BindAddressMatchValidator.php diff --git a/dns/bind/src/opnsense/mvc/app/models/OPNsense/Bind/FieldTypes/BindAddressMatchField.php b/dns/bind/src/opnsense/mvc/app/models/OPNsense/Bind/FieldTypes/BindAddressMatchField.php new file mode 100644 index 0000000000..5cbcb9a8c3 --- /dev/null +++ b/dns/bind/src/opnsense/mvc/app/models/OPNsense/Bind/FieldTypes/BindAddressMatchField.php @@ -0,0 +1,158 @@ +internalFieldSeparator, array_map('trim', explode("\n", trim(strtolower($value)))))); + } + + /** + * setter for net mask required + * @param integer $value + */ + public function setNetMaskRequired($value) + { + if (trim(strtoupper($value)) == "Y") { + $this->internalNetMaskRequired = true; + } else { + $this->internalNetMaskRequired = false; + } + } + + /** + * setter for net mask required + * @param integer $value + */ + public function setNetMaskAllowed($value) + { + $this->internalNetMaskAllowed = (trim(strtoupper($value)) == "Y"); + } + + /** + * setter for address family + * @param $value address family [ipv4, ipv6, empty for all] + */ + public function setAddressFamily($value) + { + $this->internalAddressFamily = trim(strtolower($value)); + } + + /** + * select if host bits are allowed in the notation + * @param $value + */ + public function setStrict($value) + { + if (trim(strtoupper($value)) == "Y") { + $this->internalStrict = true; + } else { + $this->internalStrict = false; + } + } + + /** + * get valid options, descriptions and selected value + * @return array + */ + public function getNodeData() + { + return join("\n", array_map('trim', explode($this->internalFieldSeparator, $this->internalValue))); + } + + /** + * {@inheritdoc} + */ + protected function defaultValidationMessage() + { + return gettext('Please specify a valid network segment or IP address.'); + } + + /** + * retrieve field validators for this field type + * @return array returns Text/regex validator + */ + public function getValidators() + { + $validators = parent::getValidators(); + if ($this->internalValue != null) { + $validators[] = new BindAddressMatchValidator([ + 'message' => $this->getValidationMessage(), + 'split' => $this->internalFieldSeparator, + 'netMaskRequired' => $this->internalNetMaskRequired, + 'netMaskAllowed' => $this->internalNetMaskAllowed, + 'version' => $this->internalAddressFamily, + 'strict' => $this->internalStrict + ]); + } + return $validators; + } +} diff --git a/dns/bind/src/opnsense/mvc/app/models/OPNsense/Bind/Validators/BindAddressMatchValidator.php b/dns/bind/src/opnsense/mvc/app/models/OPNsense/Bind/Validators/BindAddressMatchValidator.php new file mode 100644 index 0000000000..83f0ca9e55 --- /dev/null +++ b/dns/bind/src/opnsense/mvc/app/models/OPNsense/Bind/Validators/BindAddressMatchValidator.php @@ -0,0 +1,149 @@ +getOption('message'); + $fieldSplit = $this->getOption('split', null); + if ($fieldSplit == null) { + $values = array($validator->getValue($attribute)); + } else { + $values = explode($fieldSplit, $validator->getValue($attribute)); + } + foreach ($values as $value) { + // strip off negation before address validation + $value = ltrim($value, '!'); + + // short-circuit on built-in ACLs + if (in_array($value, $this->builtinAcls)) { + continue; + } + + // parse filter options + $filterOpt = 0; + switch (strtolower($this->getOption('version') ?? '')) { + case "ipv4": + $filterOpt |= FILTER_FLAG_IPV4; + break; + case "ipv6": + $filterOpt |= FILTER_FLAG_IPV6; + break; + default: + $filterOpt |= FILTER_FLAG_IPV4 | FILTER_FLAG_IPV6; + } + + // split network + if (strpos($value, "/") !== false) { + if ($this->getOption('netMaskAllowed') === false) { + $result = false; + } else { + $cidr = $value; + $parts = explode("/", $value); + if (count($parts) > 2 || !ctype_digit($parts[1])) { + // more parts then expected or second part is not numeric + $result = false; + } else { + $mask = $parts[1]; + $value = $parts[0]; + if (strpos($parts[0], ":") !== false) { + // probably ipv6, mask must be between 0..128 + if ($mask < 0 || $mask > 128) { + $result = false; + } + } else { + // most likely ipv4 address, mask must be between 0..32 + if ($mask < 0 || $mask > 32) { + $result = false; + } + } + } + + if ($this->getOption('strict') === true && !Util::isSubnetStrict($cidr)) { + $result = false; + } + } + } elseif ($this->getOption('netMaskRequired') === true) { + $result = false; + } + + + if (filter_var($value, FILTER_VALIDATE_IP, $filterOpt) === false) { + $result = false; + } + + if (!$result) { + // append validation message + $validator->appendMessage(new Message($msg, $attribute, 'BindAddressMatchValidator')); + } + } + + return $result; + } +} From ec18c75a94e522e1e850b74738d55a53e17f84d6 Mon Sep 17 00:00:00 2001 From: John Fieber Date: Sun, 26 Jan 2025 16:52:14 -0800 Subject: [PATCH 2/4] dns/bind: Use BindAddressMatchField for ACL definitions (#4435) Switch the UI for ACL definitions and ACL for filter-aaaa to use the BindAddressMatchField type. Because the introduction of negation makes the ACL entry order critical, this switches the user interface to a textbox, with one entry per line instead of the tokenized list. This interface allows much easier ordering of the entries. This change intorduces no model changes and thus no upgrade migrations are necessary. If ACLs are created with negation or references to the built-in ACLs, and the plugin is downgrated, the configuration templates will render correctly, but updating the configuration will require removing the negation and/or built-in ACL references to pass validation. --- .../controllers/OPNsense/Bind/forms/dialogEditBindAcl.xml | 6 ++---- .../mvc/app/controllers/OPNsense/Bind/forms/general.xml | 6 ++---- dns/bind/src/opnsense/mvc/app/models/OPNsense/Bind/Acl.xml | 4 +--- .../src/opnsense/mvc/app/models/OPNsense/Bind/General.xml | 5 +---- 4 files changed, 6 insertions(+), 15 deletions(-) diff --git a/dns/bind/src/opnsense/mvc/app/controllers/OPNsense/Bind/forms/dialogEditBindAcl.xml b/dns/bind/src/opnsense/mvc/app/controllers/OPNsense/Bind/forms/dialogEditBindAcl.xml index 1931d97ef1..00aa2bb76f 100644 --- a/dns/bind/src/opnsense/mvc/app/controllers/OPNsense/Bind/forms/dialogEditBindAcl.xml +++ b/dns/bind/src/opnsense/mvc/app/controllers/OPNsense/Bind/forms/dialogEditBindAcl.xml @@ -14,9 +14,7 @@ acl.networks - - select_multiple - true - List of networks for this ACL. + textbox + List of addresses and network prefixes, one address or prefix per line. Use a leading exclamation mark (!) for negation. These built in ACLs may also be used: any, none, localhost, and localnets. If more than one element in an ACL is found to match a given IP address or prefix, preference is given to the one that came first in the ACL definition. diff --git a/dns/bind/src/opnsense/mvc/app/controllers/OPNsense/Bind/forms/general.xml b/dns/bind/src/opnsense/mvc/app/controllers/OPNsense/Bind/forms/general.xml index 23e9c92026..f4b472a43f 100644 --- a/dns/bind/src/opnsense/mvc/app/controllers/OPNsense/Bind/forms/general.xml +++ b/dns/bind/src/opnsense/mvc/app/controllers/OPNsense/Bind/forms/general.xml @@ -84,10 +84,8 @@ general.filteraaaaacl - - select_multiple - true - Specifies a list of client addresses for which AAAA filtering is to be applied. + textbox + Specifies a list of client addresses, one per line, for which AAAA filtering is to be applied. general.logsize diff --git a/dns/bind/src/opnsense/mvc/app/models/OPNsense/Bind/Acl.xml b/dns/bind/src/opnsense/mvc/app/models/OPNsense/Bind/Acl.xml index 870ee9ec03..9619569c23 100644 --- a/dns/bind/src/opnsense/mvc/app/models/OPNsense/Bind/Acl.xml +++ b/dns/bind/src/opnsense/mvc/app/models/OPNsense/Bind/Acl.xml @@ -20,10 +20,8 @@ - - , + Y - Y diff --git a/dns/bind/src/opnsense/mvc/app/models/OPNsense/Bind/General.xml b/dns/bind/src/opnsense/mvc/app/models/OPNsense/Bind/General.xml index 72e771fbc8..8a23f84f95 100644 --- a/dns/bind/src/opnsense/mvc/app/models/OPNsense/Bind/General.xml +++ b/dns/bind/src/opnsense/mvc/app/models/OPNsense/Bind/General.xml @@ -59,10 +59,7 @@ 0 Y - - , - Y - + 5 Y From d7d7aa82b2f0c180ce3a5416e8921344e259d75c Mon Sep 17 00:00:00 2001 From: John Fieber Date: Sun, 2 Feb 2025 14:32:17 -0800 Subject: [PATCH 3/4] dns/bind: Disallow deleting referenced ACLs Deleting a referenced ACL will generally cause all sorts of things to break. --- .../mvc/app/controllers/OPNsense/Bind/Api/AclController.php | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/dns/bind/src/opnsense/mvc/app/controllers/OPNsense/Bind/Api/AclController.php b/dns/bind/src/opnsense/mvc/app/controllers/OPNsense/Bind/Api/AclController.php index b7a213ef0e..179d802f8f 100644 --- a/dns/bind/src/opnsense/mvc/app/controllers/OPNsense/Bind/Api/AclController.php +++ b/dns/bind/src/opnsense/mvc/app/controllers/OPNsense/Bind/Api/AclController.php @@ -34,6 +34,7 @@ class AclController extends ApiMutableModelControllerBase { protected static $internalModelName = 'acl'; protected static $internalModelClass = '\OPNsense\Bind\Acl'; + protected static $internalModelUseSafeDelete = true; public function searchAclAction() { @@ -64,4 +65,9 @@ public function toggleAclAction($uuid) { return $this->toggleBase('acls.acl', $uuid); } + + protected function checkAndThrowValueInUse($token, $contains = true, $only_mvc = false, $exclude_refs = []) + { + parent::checkAndThrowValueInUse($token, $contains, $only_mvc, $exclude_refs); + } } From b4f29041f3e7309f36984ef013ab8b6ded6b4aef Mon Sep 17 00:00:00 2001 From: John Fieber Date: Sat, 1 Feb 2025 17:19:03 -0800 Subject: [PATCH 4/4] dns/bind: Improve handling of disabled ACLs in the named.conf template For a configuration item whose value is an ACL list, ensure that: - If a referenced ACL is disabled, it is not included in the configuration. - If all referenced ACLs are disabled, the configured item is treated as if no ACLs were specified. --- .../templates/OPNsense/Bind/named.conf | 115 ++++++++++-------- 1 file changed, 65 insertions(+), 50 deletions(-) diff --git a/dns/bind/src/opnsense/service/templates/OPNsense/Bind/named.conf b/dns/bind/src/opnsense/service/templates/OPNsense/Bind/named.conf index 6b833e19a9..e645e94f3b 100644 --- a/dns/bind/src/opnsense/service/templates/OPNsense/Bind/named.conf +++ b/dns/bind/src/opnsense/service/templates/OPNsense/Bind/named.conf @@ -8,6 +8,36 @@ acl "{{ acl_list.name }}" { {{ acl_list.networks.replace(',', '; ') }}; }; {% endfor %} {% endif %} +{# +Given a comma separated list of ACL UUIDs, if any named ACLs in the list are enabled, +evaluate the call block, otherwise skip. Use this to wrap configuration block that don't +make sense to include if there are no enabled ACLs. +#} +{% macro ACLListEnabled(aclList) %} +{% if aclList is defined and aclList != '' %} +{% set ns = namespace() %} +{% set ns.found = 0 %} +{% for aclUUID in aclList.split(',') if helpers.getUUID(aclUUID).enabled == '1' %} +{% set ns.found = ns.found + 1 %} +{% endfor %} +{% if ns.found > 0 %} +{{ caller() }} +{% endif %} +{% endif %} +{% endmacro %} + +{# +Given a comma separated list of ACL UUIDs, return a semicolon separated +list of the enabled ACL names for use in a an address match field context. +#} +{% macro ACLNames(aclList, joiner = "; ") %} +{% set ns = namespace() %} +{% set ns.names = [] %} +{% if aclList is defined and aclList != '' %} +{% for aclUUID in aclList.split(',') %}{% do ns.names.append(helpers.getUUID(aclUUID)) %}{% endfor %} +{% endif %} +{{ ns.names | selectattr("enabled", "eq", "1") | map(attribute="name") | join(joiner) }}{% endmacro %} + options { directory "/usr/local/etc/namedb/working"; @@ -46,33 +76,24 @@ options { response-policy { {% if helpers.exists('OPNsense.bind.dnsbl.type') and OPNsense.bind.dnsbl.type != '' %}zone "whitelist.localdomain"; zone "blacklist.localdomain";{% endif %}{% if helpers.exists('OPNsense.bind.dnsbl.forcesafegoogle') and OPNsense.bind.dnsbl.forcesafegoogle == '1' %}zone "rpzgoogle";{% endif %}{% if helpers.exists('OPNsense.bind.dnsbl.forcesafeduckduckgo') and OPNsense.bind.dnsbl.forcesafeduckduckgo == '1' %}zone "rpzduckduckgo";{% endif %}{% if helpers.exists('OPNsense.bind.dnsbl.forcesafeyoutube') and OPNsense.bind.dnsbl.forcesafeyoutube == '1' %}zone "rpzyoutube";{% endif %}{% if helpers.exists('OPNsense.bind.dnsbl.forcestrictbing') and OPNsense.bind.dnsbl.forcestrictbing == '1' %}zone "rpzbing";{% endif %} }; {% endif %} -{% if helpers.exists('OPNsense.bind.general.recursion') and OPNsense.bind.general.recursion != '' %} +{% if helpers.exists('OPNsense.bind.general.recursion') %} +{% call ACLListEnabled(OPNsense.bind.general.recursion) %} recursion yes; - allow-recursion { -{% for acl in OPNsense.bind.general.recursion.split(',') %} -{% set recursion_acl = helpers.getUUID(acl) %} - {{ recursion_acl.name }}; -{% endfor %} - }; -{% endif %} + allow-recursion { {{ ACLNames(OPNsense.bind.general.recursion) }}; }; +{% endcall %} +{% endif -%} -{% if helpers.exists('OPNsense.bind.general.allowtransfer') and OPNsense.bind.general.allowtransfer != '' %} - allow-transfer { -{% for acl in OPNsense.bind.general.allowtransfer.split(',') %} -{% set transfer_acl = helpers.getUUID(acl) %} - {{ transfer_acl.name }}; -{% endfor %} - }; -{% endif %} +{% if helpers.exists('OPNsense.bind.general.allowtransfer') %} +{% call ACLListEnabled(OPNsense.bind.general.allowtransfer) %} + allow-transfer { {{ ACLNames(OPNsense.bind.general.allowtransfer) }}; }; +{% endcall %} +{% endif -%} -{% if helpers.exists('OPNsense.bind.general.allowquery') and OPNsense.bind.general.allowquery != '' %} - allow-query { -{% for acl in OPNsense.bind.general.allowquery.split(',') %} -{% set query_acl = helpers.getUUID(acl) %} - {{ query_acl.name }}; -{% endfor %} - }; -{% endif %} +{% if helpers.exists('OPNsense.bind.general.allowquery') %} +{% call ACLListEnabled(OPNsense.bind.general.allowquery) %} + allow-query { {{ ACLNames(OPNsense.bind.general.allowquery) }}; }; +{% endcall %} +{% endif -%} {% if helpers.exists('OPNsense.bind.general.maxcachesize') and OPNsense.bind.general.maxcachesize != '' %} max-cache-size {{ OPNsense.bind.general.maxcachesize }}%; @@ -91,7 +112,7 @@ options { {% endif %} {% if helpers.exists('OPNsense.bind.general.enableratelimiting') and OPNsense.bind.general.enableratelimiting == '1' %} {% if helpers.exists('OPNsense.bind.general.ratelimitcount') and OPNsense.bind.general.ratelimitcount != '' %} - rate-limit { + rate-limit { responses-per-second {{ OPNsense.bind.general.ratelimitcount }}; {% if helpers.exists('OPNsense.bind.general.ratelimitexcept') and OPNsense.bind.general.ratelimitexcept != '' %} exempt-clients { {{ OPNsense.bind.general.ratelimitexcept.replace(',', '; ') }}; }; @@ -108,7 +129,7 @@ key "rndc-key" { }; controls { inet 127.0.0.1 port 9530 - allow { 127.0.0.1; } keys { "rndc-key"; }; + allow { 127.0.0.1; } keys { "rndc-key"; }; }; {% endif %} @@ -164,30 +185,24 @@ zone "{{ domain.domainname }}" { {% else %} file "/usr/local/etc/namedb/primary/{{ domain.domainname }}.db"; {% endif %} -{% if domain.allowtransfer is defined or (domain.allowrndctransfer is defined and domain.allowrndctransfer == "1") %} +{% if domain.allowrndctransfer is defined and domain.allowrndctransfer == "1" %} allow-transfer { -{% if domain.allowrndctransfer is defined and domain.allowrndctransfer == "1" %} - key "rndc-key"; -{% endif %} -{% if domain.allowtransfer is defined %} -{% for acl in domain.allowtransfer.split(',') %} -{% set transfer_acl = helpers.getUUID(acl) %} - {{ transfer_acl.name }}; -{% endfor %} -{% endif %} - }; -{% endif %} -{% if domain.allowquery is defined %} - allow-query { -{% for acl in domain.allowquery.split(',') %} -{% set query_acl = helpers.getUUID(acl) %} - {{ query_acl.name }}; -{% endfor %} + key "rndc-key"; +{% call ACLListEnabled(domain.allowtransfer) %} + {{ ACLNames(domain.allowtransfer) }}; +{% endcall %} }; +{% else %} +{% call ACLListEnabled(domain.allowtransfer) %} + allow-transfer { {{ ACLNames(domain.allowtransfer) }}; }; +{% endcall %} {% endif %} +{% call ACLListEnabled(domain.allowquery) %} + allow-query { {{ ACLNames(domain.allowquery) }}; }; +{% endcall %} {% if domain.allowrndcupdate is defined and domain.allowrndcupdate == "1" and domain.type != 'secondary' %} update-policy { - grant rndc-key zonesub ANY; + grant rndc-key zonesub ANY; }; {% endif %} }; @@ -244,20 +259,20 @@ logging { plugin query "/usr/local/lib/bind/filter-aaaa.so" { {% if helpers.exists('OPNsense.bind.general.filteraaaav4') and OPNsense.bind.general.filteraaaav4 == '1' %} {% if OPNsense.bind.general.dnssecvalidation == 'no' %} - filter-aaaa-on-v4 break-dnssec; + filter-aaaa-on-v4 break-dnssec; {% else %} - filter-aaaa-on-v4 yes; + filter-aaaa-on-v4 yes; {% endif %} {% endif %} {% if helpers.exists('OPNsense.bind.general.filteraaaav6') and OPNsense.bind.general.filteraaaav6 == '1' %} {% if OPNsense.bind.general.dnssecvalidation == 'no' %} - filter-aaaa-on-v6 break-dnssec; + filter-aaaa-on-v6 break-dnssec; {% else %} - filter-aaaa-on-v6 yes; + filter-aaaa-on-v6 yes; {% endif %} {% endif %} {% if helpers.exists('OPNsense.bind.general.filteraaaaacl') and OPNsense.bind.general.filteraaaaacl != '' %} filter-aaaa { {{ OPNsense.bind.general.filteraaaaacl.replace(',', '; ') }}; }; {% endif %} - }; +}; {% endif %}