Re: [Freeipa-devel] [PATCH 0170] Detect and warn about invalid forwardzone configuration
On 12/12/14 13:50, Martin Kosek wrote: On 12/11/2014 05:44 PM, Petr Spacek wrote: On 11.12.2014 16:50, Martin Basti wrote: Updated aptch attached: Nice work, ACK! Can we also add some tests? This is a lot of new code that could break stuff. We can, in week maybe or after christmas, currently I do some work with tests and adding new tests required by QE, I will add forwardzone warnings tests when finish this. -- Martin Basti ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0170] Detect and warn about invalid forwardzone configuration
On 12/11/2014 05:44 PM, Petr Spacek wrote: On 11.12.2014 16:50, Martin Basti wrote: Updated aptch attached: Nice work, ACK! Can we also add some tests? This is a lot of new code that could break stuff. ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0170] Detect and warn about invalid forwardzone configuration
On 11.12.2014 16:50, Martin Basti wrote: > Updated aptch attached: Nice work, ACK! -- Petr^2 Spacek ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0170] Detect and warn about invalid forwardzone configuration
Updated aptch attached:
diff with previous:
diff --git a/ipalib/plugins/dns.py b/ipalib/plugins/dns.py
index f9d8321..7a80036 100644
--- a/ipalib/plugins/dns.py
+++ b/ipalib/plugins/dns.py
@@ -1735,7 +1735,7 @@ def _normalize_zone(zone):
def _get_auth_zone_ldap(name):
"""
-Find authoritative zone in LDAP for name
+Find authoritative zone in LDAP for name. Only active zones are
considered.
:param name:
:return: (zone, truncated)
zone: authoritative zone, or None if authoritative zone is not in LDAP
@@ -1781,10 +1781,10 @@ def _get_auth_zone_ldap(name):
def _get_longest_match_ns_delegation_ldap(zone, name):
"""
-Finds record in LDAP which has the longest match with name.
+Searches for deepest delegation for name in LDAP zone.
-NOTE: does not search in zone apex, returns None if there is no NS
-delegation outside of zone apex
+NOTE: NS record in zone apex is not considered as delegation.
+It returns None if there is no delegation outside of zone apex.
Example:
zone: example.com.
@@ -1799,9 +1799,8 @@ def _get_longest_match_ns_delegation_ldap(zone, name):
:param zone: zone name
:param name:
-:return: (record, truncated);
-record: record name if success, or None if no such record exists, or
-record is zone apex record
+:return: (match, truncated);
+match: delegation name if success, or None if no delegation record
exists
"""
assert isinstance(zone, DNSName)
assert isinstance(name, DNSName)
@@ -1846,7 +1845,6 @@ def _get_longest_match_ns_delegation_ldap(zone, name):
# test if entry contains NS records
for entry in entries:
-print entry
if entry.get('nsrecord'):
matched_records.append(entry.single_value['idnsname'])
@@ -3444,7 +3442,7 @@ class dnsrecord(LDAPObject):
def warning_if_ns_change_cause_fwzone_ineffective(self, result, *keys,
**options):
"""Detect if NS record change can make forward zones
ineffective due
-missing delegation. Run after parent's execute method method.
+missing delegation. Run after parent's execute method.
"""
record_name_absolute = keys[-1]
zone = keys[-2]
--
Martin Basti
From c85d7639e62ca5871e0598db973c9540b056b197 Mon Sep 17 00:00:00 2001
From: Martin Basti
Date: Fri, 21 Nov 2014 16:54:09 +0100
Subject: [PATCH] Detect and warn about invalid DNS forward zone configuration
Shows warning if forward and parent authoritative zone do not have
proper NS record delegation, which can cause the forward zone will be
ineffective and forwarding will not work.
Ticket: https://fedorahosted.org/freeipa/ticket/4721
---
ipalib/messages.py| 13 ++
ipalib/plugins/dns.py | 330 --
2 files changed, 332 insertions(+), 11 deletions(-)
diff --git a/ipalib/messages.py b/ipalib/messages.py
index 102e35275dbe37328c84ecb3cd5b2a8d8578056f..b44beca729f5483a7241e4c98a9f724ed663e70f 100644
--- a/ipalib/messages.py
+++ b/ipalib/messages.py
@@ -200,6 +200,19 @@ class DNSServerDoesNotSupportDNSSECWarning(PublicMessage):
u"If DNSSEC validation is enabled on IPA server(s), "
u"please disable it.")
+class ForwardzoneIsNotEffectiveWarning(PublicMessage):
+"""
+**13008** Forwardzone is not effective, forwarding will not work because
+there is authoritative parent zone, without proper NS delegation
+"""
+
+errno = 13008
+type = "warning"
+format = _(u"forward zone \"%(fwzone)s\" is not effective because of "
+ u"missing proper NS delegation in authoritative zone "
+ u"\"%(authzone)s\". Please add NS record "
+ u"\"%(ns_rec)s\" to parent zone \"%(authzone)s\".")
+
def iter_messages(variables, base):
"""Return a tuple with all subclasses
diff --git a/ipalib/plugins/dns.py b/ipalib/plugins/dns.py
index 34afc189866993481229bb68a5edd77e0a4eaff3..7a80036c94432a01ea8781101712ea1135134948 100644
--- a/ipalib/plugins/dns.py
+++ b/ipalib/plugins/dns.py
@@ -1733,6 +1733,239 @@ def _normalize_zone(zone):
return zone
+def _get_auth_zone_ldap(name):
+"""
+Find authoritative zone in LDAP for name. Only active zones are considered.
+:param name:
+:return: (zone, truncated)
+zone: authoritative zone, or None if authoritative zone is not in LDAP
+"""
+assert isinstance(name, DNSName)
+ldap = api.Backend.ldap2
+
+# Create all possible parent zone names
+search_name = name.make_absolute()
+zone_names = []
+for i in xrange(len(search_name)):
+zone_name_abs = DNSName(search_name[i:]).ToASCII()
+zone_names.append(zone_name_abs)
+# compatibility with IPA < 4.0, zone name can be relative
+zone_names.append(zone_name_abs[:-1])
+
+# Create filters
+objectclass_filter = ldap.make_filter({'objectclass':'idnszone'
Re: [Freeipa-devel] [PATCH 0170] Detect and warn about invalid forwardzone configuration
Hello,
I have only few nitpicks and one minor non-nitpick. Rest is in-line.
On 10.12.2014 18:20, Martin Basti wrote:
> freeipa-mbasti-0170.4-Detect-and-warn-about-invalid-DNS-forward-zone-confi.patch
>
>
> From a1b70e7a12ffdb08941d43587a05d7e36b57ab2b Mon Sep 17 00:00:00 2001
> From: Martin Basti
> Date: Fri, 21 Nov 2014 16:54:09 +0100
> Subject: [PATCH] Detect and warn about invalid DNS forward zone configuration
>
> Shows warning if forward and parent authoritative zone do not have
> proper NS record delegation, which can cause the forward zone will be
> ineffective and forwarding will not work.
>
> Ticket: https://fedorahosted.org/freeipa/ticket/4721
> ---
> ipalib/messages.py| 13 ++
> ipalib/plugins/dns.py | 332
> --
> 2 files changed, 334 insertions(+), 11 deletions(-)
>
> diff --git a/ipalib/messages.py b/ipalib/messages.py
> index
> 102e35275dbe37328c84ecb3cd5b2a8d8578056f..b44beca729f5483a7241e4c98a9f724ed663e70f
> 100644
> --- a/ipalib/messages.py
> +++ b/ipalib/messages.py
> @@ -200,6 +200,19 @@ class
> DNSServerDoesNotSupportDNSSECWarning(PublicMessage):
> u"If DNSSEC validation is enabled on IPA server(s), "
> u"please disable it.")
>
> +class ForwardzoneIsNotEffectiveWarning(PublicMessage):
> +"""
> +**13008** Forwardzone is not effective, forwarding will not work because
> +there is authoritative parent zone, without proper NS delegation
> +"""
> +
> +errno = 13008
> +type = "warning"
> +format = _(u"forward zone \"%(fwzone)s\" is not effective because of "
> + u"missing proper NS delegation in authoritative zone "
> + u"\"%(authzone)s\". Please add NS record "
> + u"\"%(ns_rec)s\" to parent zone \"%(authzone)s\".")
> +
>
> def iter_messages(variables, base):
> """Return a tuple with all subclasses
> diff --git a/ipalib/plugins/dns.py b/ipalib/plugins/dns.py
> index
> c5d96a8c4fcdf101254ecefb60cb83d63bee6310..5c3a017989b23a1c6076d9dc4d93be65dd66cc67
> 100644
> --- a/ipalib/plugins/dns.py
> +++ b/ipalib/plugins/dns.py
> @@ -1725,6 +1725,241 @@ def _normalize_zone(zone):
> return zone
>
>
> +def _get_auth_zone_ldap(name):
> +"""
> +Find authoritative zone in LDAP for name
Nitpick: Please add this note:
. Only active zones are considered.
> +:param name:
> +:return: (zone, truncated)
> +zone: authoritative zone, or None if authoritative zone is not in LDAP
> +"""
> +assert isinstance(name, DNSName)
> +ldap = api.Backend.ldap2
> +
> +# Create all possible parent zone names
> +search_name = name.make_absolute()
> +zone_names = []
> +for i in xrange(len(search_name)):
> +zone_name_abs = DNSName(search_name[i:]).ToASCII()
> +zone_names.append(zone_name_abs)
> +# compatibility with IPA < 4.0, zone name can be relative
> +zone_names.append(zone_name_abs[:-1])
> +
> +# Create filters
> +objectclass_filter = ldap.make_filter({'objectclass':'idnszone'})
> +zonenames_filter = ldap.make_filter({'idnsname': zone_names})
> +zoneactive_filter = ldap.make_filter({'idnsZoneActive': 'true'})
> +complete_filter = ldap.combine_filters(
> +[objectclass_filter, zonenames_filter, zoneactive_filter],
> +rules=ldap.MATCH_ALL
> +)
> +
> +try:
> +entries, truncated = ldap.find_entries(
> +filter=complete_filter,
> +attrs_list=['idnsname'],
> +base_dn=DN(api.env.container_dns, api.env.basedn),
> +scope=ldap.SCOPE_ONELEVEL
> +)
> +except errors.NotFound:
> +return None, False
> +
> +# always use absolute zones
> +matched_auth_zones = [entry.single_value['idnsname'].make_absolute()
> + for entry in entries]
> +
> +# return longest match
> +return max(matched_auth_zones, key=len), truncated
> +
> +
> +def _get_longest_match_ns_delegation_ldap(zone, name):
> +"""
> +Finds record in LDAP which has the longest match with name.
> +
> +NOTE: does not search in zone apex, returns None if there is no NS
> +delegation outside of zone apex
Nitpick:
Searches for deepest delegation for name in LDAP zone.
NOTE: NS record in zone apex is not considered as delegation. It returns None
if there is no delegation outside of zone apex.
> +
> +Example:
> +zone: example.com.
> +name: ns.sub.example.com.
> +
> +records:
> +extra.ns.sub.example.com.
> +sub.example.com.
> +example.com
> +
> +result: sub.example.com.
> +
> +:param zone: zone name
> +:param name:
> +:return: (record, truncated);
> +record: record name if success, or None if no such record exists, or
> +record is zone apex record
Nitpick:
:return: (match, truncated);
match: delegation name if success, or None if no delegation record exists
> +"""
> +assert isinstance(zon
Re: [Freeipa-devel] [PATCH 0170] Detect and warn about invalid forwardzone configuration
On 09/12/14 17:40, Martin Basti wrote: On 01/12/14 17:44, Petr Spacek wrote: On 1.12.2014 14:39, Martin Basti wrote: On 24/11/14 17:24, Petr Spacek wrote: Hello! Thank you for the patch. It is not ready yet but the overall direction seems good. Please see my comments in-line. On 24.11.2014 14:35, Martin Basti wrote: Ticket: https://fedorahosted.org/freeipa/ticket/4721 Patch attached -- Martin Basti freeipa-mbasti-0170-Detect-and-warn-about-invalid-DNS-forward-zone-confi.patch From a5a19137e3ddf2d2d48cfbdb2968b6f68ac8f772 Mon Sep 17 00:00:00 2001 From: Martin Basti Date: Fri, 21 Nov 2014 16:54:09 +0100 Subject: [PATCH] Detect and warn about invalid DNS forward zone configuration Shows warning if forward and parent authoritative zone do not have proper NS record delegation, which can cause the forward zone will be ineffective and forwarding will not work. The chande in the test was required due python changes order of elemnts in dict (python doesnt guarantee an order in dict) Ticket: https://fedorahosted.org/freeipa/ticket/4721 --- ipalib/messages.py | 12 +++ ipalib/plugins/dns.py | 147 +--- ipatests/test_xmlrpc/test_dns_plugin.py | 2 +- 3 files changed, 149 insertions(+), 12 deletions(-) diff --git a/ipalib/messages.py b/ipalib/messages.py index 102e35275dbe37328c84ecb3cd5b2a8d8578056f..cc9bae3d6e5c7d3a7401dab89fafb1b60dc1e15f 100644 --- a/ipalib/messages.py +++ b/ipalib/messages.py @@ -200,6 +200,18 @@ class DNSServerDoesNotSupportDNSSECWarning(PublicMessage): u"If DNSSEC validation is enabled on IPA server(s), " u"please disable it.") +class ForwardzoneIsNotEffectiveWarning(PublicMessage): +""" +**13008** Forwardzone is not effective, forwarding will not work because +there is authoritative parent zone, without proper NS delegation +""" + +errno = 13008 +type = "warning" +format = _(u"forward zone \"%(fwzone)s\" is not effective (missing proper " + u"NS delegation in authoritative zone \"%(authzone)s\"). " + u"Forwarding may not work.") I think that the error message could be made mode useful: "Forwarding will not work. Please add NS record to parent zone %(authzone)s" (or something like that). + def iter_messages(variables, base): """Return a tuple with all subclasses diff --git a/ipalib/plugins/dns.py b/ipalib/plugins/dns.py index c5d96a8c4fcdf101254ecefb60cb83d63bee6310..4f92da645e0faf784c7deb4b8ce386c1440f4229 100644 --- a/ipalib/plugins/dns.py +++ b/ipalib/plugins/dns.py @@ -1725,6 +1725,79 @@ def _normalize_zone(zone): return zone +def _find_zone_which_makes_fw_zone_ineffective(fwzonename): Generally, this method finds an authoritative zone for given "fwzonename". What about method name _find_auth_zone_ldap(name) ? +""" +Check if forward zone is effective. + +If parent zone exists as authoritative zone, forward zone will not +forward queries. It is necessary to delegate authority of forward zone I would clarify it: "forward queries by default." +to another server (non IPA DNS server). I would personally omit "(non IPA DNS server)" because this mechanism is not related to IPA domain at all. +Example: + +Forward zone: sub.example.com +Zone: example.com + +Forwarding will not work, because server thinks it is authoritative +and returns NXDOMAIN + +Adding record: sub.example.com NS nameserver.out.of.ipa.domain. Again, this is not related to IPA domain at all. I propose this text: "Adding record: sub.example.com NS ns.sub.example.com." +will delegate authority to another server, and IPA DNS server will +forward DNS queries. + +:param fwzonename: forwardzone +:return: None if effective, name of authoritative zone otherwise +""" +assert isinstance(fwzonename, DNSName) + +# get all zones +zones = api.Command.dnszone_find(pkey_only=True, + sizelimit=0)['result'] Ooooh, are you serious? :-) I don't like this approach, I would rather chop left-most labels from "name" and so dnszone_find() one by one. What if you have many DNS zones? This could be thrown away if you iterate only over relevant zones. +zonenames = [zone['idnsname'][0].make_absolute() for zone in zones] +zonenames.sort(reverse=True, key=len) # sort, we need longest match + +# check if is there a zone which can cause the forward zone will +# be ineffective +sub_of_auth_zone = None +for name in zonenames: +if fwzonename.is_subdomain(name): +sub_of_auth_zone = name +break + +if sub_of_auth_zone is None: +return None + +# check if forwardzone is delegated in authoritative zone +# test if exists and get NS record +try: +ns_records = api.Command.dnsrecord_show( +sub_of_auth_zone, fwzonename, raw=True)['
Re: [Freeipa-devel] [PATCH 0170] Detect and warn about invalid forwardzone configuration
On 01/12/14 17:44, Petr Spacek wrote: On 1.12.2014 14:39, Martin Basti wrote: On 24/11/14 17:24, Petr Spacek wrote: Hello! Thank you for the patch. It is not ready yet but the overall direction seems good. Please see my comments in-line. On 24.11.2014 14:35, Martin Basti wrote: Ticket: https://fedorahosted.org/freeipa/ticket/4721 Patch attached -- Martin Basti freeipa-mbasti-0170-Detect-and-warn-about-invalid-DNS-forward-zone-confi.patch From a5a19137e3ddf2d2d48cfbdb2968b6f68ac8f772 Mon Sep 17 00:00:00 2001 From: Martin Basti Date: Fri, 21 Nov 2014 16:54:09 +0100 Subject: [PATCH] Detect and warn about invalid DNS forward zone configuration Shows warning if forward and parent authoritative zone do not have proper NS record delegation, which can cause the forward zone will be ineffective and forwarding will not work. The chande in the test was required due python changes order of elemnts in dict (python doesnt guarantee an order in dict) Ticket: https://fedorahosted.org/freeipa/ticket/4721 --- ipalib/messages.py | 12 +++ ipalib/plugins/dns.py | 147 +--- ipatests/test_xmlrpc/test_dns_plugin.py | 2 +- 3 files changed, 149 insertions(+), 12 deletions(-) diff --git a/ipalib/messages.py b/ipalib/messages.py index 102e35275dbe37328c84ecb3cd5b2a8d8578056f..cc9bae3d6e5c7d3a7401dab89fafb1b60dc1e15f 100644 --- a/ipalib/messages.py +++ b/ipalib/messages.py @@ -200,6 +200,18 @@ class DNSServerDoesNotSupportDNSSECWarning(PublicMessage): u"If DNSSEC validation is enabled on IPA server(s), " u"please disable it.") +class ForwardzoneIsNotEffectiveWarning(PublicMessage): +""" +**13008** Forwardzone is not effective, forwarding will not work because +there is authoritative parent zone, without proper NS delegation +""" + +errno = 13008 +type = "warning" +format = _(u"forward zone \"%(fwzone)s\" is not effective (missing proper " + u"NS delegation in authoritative zone \"%(authzone)s\"). " + u"Forwarding may not work.") I think that the error message could be made mode useful: "Forwarding will not work. Please add NS record to parent zone %(authzone)s" (or something like that). + def iter_messages(variables, base): """Return a tuple with all subclasses diff --git a/ipalib/plugins/dns.py b/ipalib/plugins/dns.py index c5d96a8c4fcdf101254ecefb60cb83d63bee6310..4f92da645e0faf784c7deb4b8ce386c1440f4229 100644 --- a/ipalib/plugins/dns.py +++ b/ipalib/plugins/dns.py @@ -1725,6 +1725,79 @@ def _normalize_zone(zone): return zone +def _find_zone_which_makes_fw_zone_ineffective(fwzonename): Generally, this method finds an authoritative zone for given "fwzonename". What about method name _find_auth_zone_ldap(name) ? +""" +Check if forward zone is effective. + +If parent zone exists as authoritative zone, forward zone will not +forward queries. It is necessary to delegate authority of forward zone I would clarify it: "forward queries by default." +to another server (non IPA DNS server). I would personally omit "(non IPA DNS server)" because this mechanism is not related to IPA domain at all. +Example: + +Forward zone: sub.example.com +Zone: example.com + +Forwarding will not work, because server thinks it is authoritative +and returns NXDOMAIN + +Adding record: sub.example.com NS nameserver.out.of.ipa.domain. Again, this is not related to IPA domain at all. I propose this text: "Adding record: sub.example.com NS ns.sub.example.com." +will delegate authority to another server, and IPA DNS server will +forward DNS queries. + +:param fwzonename: forwardzone +:return: None if effective, name of authoritative zone otherwise +""" +assert isinstance(fwzonename, DNSName) + +# get all zones +zones = api.Command.dnszone_find(pkey_only=True, +sizelimit=0)['result'] Ooooh, are you serious? :-) I don't like this approach, I would rather chop left-most labels from "name" and so dnszone_find() one by one. What if you have many DNS zones? This could be thrown away if you iterate only over relevant zones. +zonenames = [zone['idnsname'][0].make_absolute() for zone in zones] +zonenames.sort(reverse=True, key=len) # sort, we need longest match + +# check if is there a zone which can cause the forward zone will +# be ineffective +sub_of_auth_zone = None +for name in zonenames: +if fwzonename.is_subdomain(name): +sub_of_auth_zone = name +break + +if sub_of_auth_zone is None: +return None + +# check if forwardzone is delegated in authoritative zone +# test if exists and get NS record +try: +ns_records = api.Command.dnsrecord_show( +sub_of_auth_zone, fwzonename, raw=True)['result']['nsrecord'] +except (errors
Re: [Freeipa-devel] [PATCH 0170] Detect and warn about invalid forwardzone configuration
On 1.12.2014 14:39, Martin Basti wrote: > On 24/11/14 17:24, Petr Spacek wrote: >> Hello! >> >> Thank you for the patch. It is not ready yet but the overall direction seems >> good. Please see my comments in-line. >> >> On 24.11.2014 14:35, Martin Basti wrote: >>> Ticket: https://fedorahosted.org/freeipa/ticket/4721 >>> Patch attached >>> >>> -- >>> Martin Basti >>> >>> >>> freeipa-mbasti-0170-Detect-and-warn-about-invalid-DNS-forward-zone-confi.patch >>> >>> >>> From a5a19137e3ddf2d2d48cfbdb2968b6f68ac8f772 Mon Sep 17 00:00:00 2001 >>> From: Martin Basti >>> Date: Fri, 21 Nov 2014 16:54:09 +0100 >>> Subject: [PATCH] Detect and warn about invalid DNS forward zone >>> configuration >>> >>> Shows warning if forward and parent authoritative zone do not have >>> proper NS record delegation, which can cause the forward zone will be >>> ineffective and forwarding will not work. >>> >>> The chande in the test was required due python changes order of elemnts >>> in dict (python doesnt guarantee an order in dict) >>> >>> Ticket: https://fedorahosted.org/freeipa/ticket/4721 >>> --- >>> ipalib/messages.py | 12 +++ >>> ipalib/plugins/dns.py | 147 >>> +--- >>> ipatests/test_xmlrpc/test_dns_plugin.py | 2 +- >>> 3 files changed, 149 insertions(+), 12 deletions(-) >>> >>> diff --git a/ipalib/messages.py b/ipalib/messages.py >>> index >>> 102e35275dbe37328c84ecb3cd5b2a8d8578056f..cc9bae3d6e5c7d3a7401dab89fafb1b60dc1e15f >>> 100644 >>> --- a/ipalib/messages.py >>> +++ b/ipalib/messages.py >>> @@ -200,6 +200,18 @@ class >>> DNSServerDoesNotSupportDNSSECWarning(PublicMessage): >>> u"If DNSSEC validation is enabled on IPA server(s), " >>> u"please disable it.") >>> +class ForwardzoneIsNotEffectiveWarning(PublicMessage): >>> +""" >>> +**13008** Forwardzone is not effective, forwarding will not work >>> because >>> +there is authoritative parent zone, without proper NS delegation >>> +""" >>> + >>> +errno = 13008 >>> +type = "warning" >>> +format = _(u"forward zone \"%(fwzone)s\" is not effective (missing >>> proper " >>> + u"NS delegation in authoritative zone \"%(authzone)s\"). " >>> + u"Forwarding may not work.") >> I think that the error message could be made mode useful: >> >> "Forwarding will not work. Please add NS record >> >> to parent zone %(authzone)s" (or something like that). >> >>> + >>> def iter_messages(variables, base): >>> """Return a tuple with all subclasses >>> diff --git a/ipalib/plugins/dns.py b/ipalib/plugins/dns.py >>> index >>> c5d96a8c4fcdf101254ecefb60cb83d63bee6310..4f92da645e0faf784c7deb4b8ce386c1440f4229 >>> 100644 >>> --- a/ipalib/plugins/dns.py >>> +++ b/ipalib/plugins/dns.py >>> @@ -1725,6 +1725,79 @@ def _normalize_zone(zone): >>> return zone >>> +def _find_zone_which_makes_fw_zone_ineffective(fwzonename): >> Generally, this method finds an authoritative zone for given "fwzonename". >> What about method name _find_auth_zone_ldap(name) ? >> >>> +""" >>> +Check if forward zone is effective. >>> + >>> +If parent zone exists as authoritative zone, forward zone will not >>> +forward queries. It is necessary to delegate authority of forward zone >> I would clarify it: "forward queries by default." >> >> >>> +to another server (non IPA DNS server). >> I would personally omit "(non IPA DNS server)" because this mechanism is not >> related to IPA domain at all. >> >>> +Example: >>> + >>> +Forward zone: sub.example.com >>> +Zone: example.com >>> + >>> +Forwarding will not work, because server thinks it is authoritative >>> +and returns NXDOMAIN >>> + >>> +Adding record: sub.example.com NS nameserver.out.of.ipa.domain. >> Again, this is not related to IPA domain at all. I propose this text: >> "Adding record: sub.example.com NS ns.sub.example.com." >> >>> +will delegate authority to another server, and IPA DNS server will >>> +forward DNS queries. >>> + >>> +:param fwzonename: forwardzone >>> +:return: None if effective, name of authoritative zone otherwise >>> +""" >>> +assert isinstance(fwzonename, DNSName) >>> + >>> +# get all zones >>> +zones = api.Command.dnszone_find(pkey_only=True, >>> +sizelimit=0)['result'] >> Ooooh, are you serious? :-) I don't like this approach, I would rather chop >> left-most labels from "name" and so dnszone_find() one by one. What if you >> have many DNS zones? >> >> >> This could be thrown away if you iterate only over relevant zones. >>> +zonenames = [zone['idnsname'][0].make_absolute() for zone in zones] >>> +zonenames.sort(reverse=True, key=len) # sort, we need longest match >>> + >>> +# check if is there a zone which can cause the forward zone will >>> +# be ineffective >>> +sub_of_auth_zone = None >>> +for name in zonenames: >>> +
Re: [Freeipa-devel] [PATCH 0170] Detect and warn about invalid forwardzone configuration
On 24/11/14 17:24, Petr Spacek wrote: Hello! Thank you for the patch. It is not ready yet but the overall direction seems good. Please see my comments in-line. On 24.11.2014 14:35, Martin Basti wrote: Ticket: https://fedorahosted.org/freeipa/ticket/4721 Patch attached -- Martin Basti freeipa-mbasti-0170-Detect-and-warn-about-invalid-DNS-forward-zone-confi.patch From a5a19137e3ddf2d2d48cfbdb2968b6f68ac8f772 Mon Sep 17 00:00:00 2001 From: Martin Basti Date: Fri, 21 Nov 2014 16:54:09 +0100 Subject: [PATCH] Detect and warn about invalid DNS forward zone configuration Shows warning if forward and parent authoritative zone do not have proper NS record delegation, which can cause the forward zone will be ineffective and forwarding will not work. The chande in the test was required due python changes order of elemnts in dict (python doesnt guarantee an order in dict) Ticket: https://fedorahosted.org/freeipa/ticket/4721 --- ipalib/messages.py | 12 +++ ipalib/plugins/dns.py | 147 +--- ipatests/test_xmlrpc/test_dns_plugin.py | 2 +- 3 files changed, 149 insertions(+), 12 deletions(-) diff --git a/ipalib/messages.py b/ipalib/messages.py index 102e35275dbe37328c84ecb3cd5b2a8d8578056f..cc9bae3d6e5c7d3a7401dab89fafb1b60dc1e15f 100644 --- a/ipalib/messages.py +++ b/ipalib/messages.py @@ -200,6 +200,18 @@ class DNSServerDoesNotSupportDNSSECWarning(PublicMessage): u"If DNSSEC validation is enabled on IPA server(s), " u"please disable it.") +class ForwardzoneIsNotEffectiveWarning(PublicMessage): +""" +**13008** Forwardzone is not effective, forwarding will not work because +there is authoritative parent zone, without proper NS delegation +""" + +errno = 13008 +type = "warning" +format = _(u"forward zone \"%(fwzone)s\" is not effective (missing proper " + u"NS delegation in authoritative zone \"%(authzone)s\"). " + u"Forwarding may not work.") I think that the error message could be made mode useful: "Forwarding will not work. Please add NS record to parent zone %(authzone)s" (or something like that). + def iter_messages(variables, base): """Return a tuple with all subclasses diff --git a/ipalib/plugins/dns.py b/ipalib/plugins/dns.py index c5d96a8c4fcdf101254ecefb60cb83d63bee6310..4f92da645e0faf784c7deb4b8ce386c1440f4229 100644 --- a/ipalib/plugins/dns.py +++ b/ipalib/plugins/dns.py @@ -1725,6 +1725,79 @@ def _normalize_zone(zone): return zone +def _find_zone_which_makes_fw_zone_ineffective(fwzonename): Generally, this method finds an authoritative zone for given "fwzonename". What about method name _find_auth_zone_ldap(name) ? +""" +Check if forward zone is effective. + +If parent zone exists as authoritative zone, forward zone will not +forward queries. It is necessary to delegate authority of forward zone I would clarify it: "forward queries by default." +to another server (non IPA DNS server). I would personally omit "(non IPA DNS server)" because this mechanism is not related to IPA domain at all. +Example: + +Forward zone: sub.example.com +Zone: example.com + +Forwarding will not work, because server thinks it is authoritative +and returns NXDOMAIN + +Adding record: sub.example.com NS nameserver.out.of.ipa.domain. Again, this is not related to IPA domain at all. I propose this text: "Adding record: sub.example.com NS ns.sub.example.com." +will delegate authority to another server, and IPA DNS server will +forward DNS queries. + +:param fwzonename: forwardzone +:return: None if effective, name of authoritative zone otherwise +""" +assert isinstance(fwzonename, DNSName) + +# get all zones +zones = api.Command.dnszone_find(pkey_only=True, +sizelimit=0)['result'] Ooooh, are you serious? :-) I don't like this approach, I would rather chop left-most labels from "name" and so dnszone_find() one by one. What if you have many DNS zones? This could be thrown away if you iterate only over relevant zones. +zonenames = [zone['idnsname'][0].make_absolute() for zone in zones] +zonenames.sort(reverse=True, key=len) # sort, we need longest match + +# check if is there a zone which can cause the forward zone will +# be ineffective +sub_of_auth_zone = None +for name in zonenames: +if fwzonename.is_subdomain(name): +sub_of_auth_zone = name +break + +if sub_of_auth_zone is None: +return None + +# check if forwardzone is delegated in authoritative zone +# test if exists and get NS record +try: +ns_records = api.Command.dnsrecord_show( +sub_of_auth_zone, fwzonename, raw=True)['result']['nsrecord'] +except (errors.NotFound, KeyError): +return sub_of_auth_zone Note: This function should
Re: [Freeipa-devel] [PATCH 0170] Detect and warn about invalid forwardzone configuration
Hello! Thank you for the patch. It is not ready yet but the overall direction seems good. Please see my comments in-line. On 24.11.2014 14:35, Martin Basti wrote: > Ticket: https://fedorahosted.org/freeipa/ticket/4721 > Patch attached > > -- > Martin Basti > > > freeipa-mbasti-0170-Detect-and-warn-about-invalid-DNS-forward-zone-confi.patch > > > From a5a19137e3ddf2d2d48cfbdb2968b6f68ac8f772 Mon Sep 17 00:00:00 2001 > From: Martin Basti > Date: Fri, 21 Nov 2014 16:54:09 +0100 > Subject: [PATCH] Detect and warn about invalid DNS forward zone configuration > > Shows warning if forward and parent authoritative zone do not have > proper NS record delegation, which can cause the forward zone will be > ineffective and forwarding will not work. > > The chande in the test was required due python changes order of elemnts > in dict (python doesnt guarantee an order in dict) > > Ticket: https://fedorahosted.org/freeipa/ticket/4721 > --- > ipalib/messages.py | 12 +++ > ipalib/plugins/dns.py | 147 > +--- > ipatests/test_xmlrpc/test_dns_plugin.py | 2 +- > 3 files changed, 149 insertions(+), 12 deletions(-) > > diff --git a/ipalib/messages.py b/ipalib/messages.py > index > 102e35275dbe37328c84ecb3cd5b2a8d8578056f..cc9bae3d6e5c7d3a7401dab89fafb1b60dc1e15f > 100644 > --- a/ipalib/messages.py > +++ b/ipalib/messages.py > @@ -200,6 +200,18 @@ class > DNSServerDoesNotSupportDNSSECWarning(PublicMessage): > u"If DNSSEC validation is enabled on IPA server(s), " > u"please disable it.") > > +class ForwardzoneIsNotEffectiveWarning(PublicMessage): > +""" > +**13008** Forwardzone is not effective, forwarding will not work because > +there is authoritative parent zone, without proper NS delegation > +""" > + > +errno = 13008 > +type = "warning" > +format = _(u"forward zone \"%(fwzone)s\" is not effective (missing > proper " > + u"NS delegation in authoritative zone \"%(authzone)s\"). " > + u"Forwarding may not work.") I think that the error message could be made mode useful: "Forwarding will not work. Please add NS record to parent zone %(authzone)s" (or something like that). > + > > def iter_messages(variables, base): > """Return a tuple with all subclasses > diff --git a/ipalib/plugins/dns.py b/ipalib/plugins/dns.py > index > c5d96a8c4fcdf101254ecefb60cb83d63bee6310..4f92da645e0faf784c7deb4b8ce386c1440f4229 > 100644 > --- a/ipalib/plugins/dns.py > +++ b/ipalib/plugins/dns.py > @@ -1725,6 +1725,79 @@ def _normalize_zone(zone): > return zone > > > +def _find_zone_which_makes_fw_zone_ineffective(fwzonename): Generally, this method finds an authoritative zone for given "fwzonename". What about method name _find_auth_zone_ldap(name) ? > +""" > +Check if forward zone is effective. > + > +If parent zone exists as authoritative zone, forward zone will not > +forward queries. It is necessary to delegate authority of forward zone I would clarify it: "forward queries by default." > +to another server (non IPA DNS server). I would personally omit "(non IPA DNS server)" because this mechanism is not related to IPA domain at all. > +Example: > + > +Forward zone: sub.example.com > +Zone: example.com > + > +Forwarding will not work, because server thinks it is authoritative > +and returns NXDOMAIN > + > +Adding record: sub.example.com NS nameserver.out.of.ipa.domain. Again, this is not related to IPA domain at all. I propose this text: "Adding record: sub.example.com NS ns.sub.example.com." > +will delegate authority to another server, and IPA DNS server will > +forward DNS queries. > + > +:param fwzonename: forwardzone > +:return: None if effective, name of authoritative zone otherwise > +""" > +assert isinstance(fwzonename, DNSName) > + > +# get all zones > +zones = api.Command.dnszone_find(pkey_only=True, > +sizelimit=0)['result'] Ooooh, are you serious? :-) I don't like this approach, I would rather chop left-most labels from "name" and so dnszone_find() one by one. What if you have many DNS zones? This could be thrown away if you iterate only over relevant zones. > +zonenames = [zone['idnsname'][0].make_absolute() for zone in zones] > +zonenames.sort(reverse=True, key=len) # sort, we need longest match > + > +# check if is there a zone which can cause the forward zone will > +# be ineffective > +sub_of_auth_zone = None > +for name in zonenames: > +if fwzonename.is_subdomain(name): > +sub_of_auth_zone = name > +break > + > +if sub_of_auth_zone is None: > +return None > + > +# check if forwardzone is delegated in authoritative zone > +# test if exists and get NS record > +try: > +ns_records = api.Command.dnsrecord_show( > +su
