[Freeipa-devel] [PATCH 0170] Detect and warn about invalid forwardzone configuration
Martin Basti
mbasti at redhat.com
Wed Dec 10 17:20:38 UTC 2014
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 <mbasti at redhat.com>
>>>>> 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
>>>> <fwdzone.relativize(authzone)>
>>>> 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 process only deepest (existing)
>>>> sub-domain in LDAP
>>>> which is active (idnsZoneActive = TRUE).
>>>>
>>>> Example:
>>>> fwzonename = sub.fwd.example.com.
>>>> Existing LDAP master zone = fwd.example.com. - DISABLED
>>>> Existing LDAP master zone = example.com.
>>>> Existing LDAP master zone = com.
>>>>
>>>> 1) Check existence & activity of fwd.example.com. -> does not
>>>> exist, continue.
>>>> 2) Check existence & activity of example.com. -> exists, search for
>>>> NS records.
>>>> 3) [inner loop - searching for NS records]
>>>> 3a) sub.fwd.example.com. NS -> does not exist, continue inner loop.
>>>> 3b) fwd.example.com. NS -> does not exist, continue inner loop.
>>>> 3c) Inner loop ends: no more (relative) candidate names to try.
>>>> 4) Exit returning "example.com." - deepest authoritative
>>>> super-domain of
>>>> sub.fwd.example.com. in LDAP.
>>>>
>>>> AFAIK content of the NS record does not matter so this check can be
>>>> thrown
>>>> away. (Check this before doing so, please :-))
>>>>
>>>>> + # make sure the target is not IPA DNS server
>>>>> + # FIXME: what if aliases are used?
>>>>> + normalized_ns_records = set()
>>>>> + for record in ns_records:
>>>>> + if record.endswith('.'):
>>>>> + normalized_ns_records.add(record)
>>>>> + else:
>>>>> + normalized_record = "%s.%s" % (record, sub_of_auth_zone)
>>>>> + normalized_ns_records.add(normalized_record)
>>>>> +
>>>>> + nameservers = [normalize_zone(x) for x in
>>>>> + api.Object.dnsrecord.get_dns_masters()]
>>>>> +
>>>>> + if any(nameserver in normalized_ns_records
>>>>> + for nameserver in nameservers):
>>>>> + # NS record is pointing to IPA DNS server
>>>>> + return sub_of_auth_zone
>>>>> +
>>>>> + # authoritative zone contains NS records which delegates
>>>>> forwardzone to
>>>>> + # different server, forwardzone is effective
>>>>> + return None
>>>>> +
>>>>> +
>>>>> class DNSZoneBase(LDAPObject):
>>>>> """
>>>>> Base class for DNS Zone
>>>>> @@ -3164,6 +3237,29 @@ class dnsrecord(LDAPObject):
>>>>> dns_name = entry_name[1].derelativize(dns_domain)
>>>>> self.wait_for_modified_attrs(entry, dns_name,
>>>>> dns_domain)
>>>>> + def warning_if_ns_change_cause_fwzone_ineffective(self,
>>>>> keys, result,
>>>>> + options):
>>>>> + """after execute"""
>>>>> + record_name_absolute = keys[-1]
>>>>> + if not keys[-1].is_absolute():
>>>>> + record_name_absolute = keys[-1].derelativize(keys[-2])
>>>>> +
>>>>> + try:
>>>>> + api.Command.dnsforwardzone_show(record_name_absolute)
>>>>> + except errors.NotFound:
>>>>> + # no forward zone
>>>>> + return
>>>>> +
>>>>> + authoritative_zone = \
>>>>> + _find_zone_which_makes_fw_zone_ineffective(record_name_absolute)
>>>>> + if authoritative_zone:
>>>>> + # forward zone is not effective and forwarding will
>>>>> not work
>>>>> + messages.add_message(
>>>>> + options['version'], result,
>>>>> + messages.ForwardzoneIsNotEffectiveWarning(
>>>>> + fwzone=record_name_absolute,
>>>>> authzone=authoritative_zone
>>>>> + )
>>>>> + )
>>>>> @register()
>>>>> @@ -3358,6 +3454,14 @@ class dnsrecord_add(LDAPCreate):
>>>>> return
>>>>> raise exc
>>>>> + def execute(self, *keys, **options):
>>>>> + result = super(dnsrecord_add, self).execute(*keys,
>>>>> **options)
>>>>> + if 'nsrecord' in options:
>>>>> + self.obj.warning_if_ns_change_cause_fwzone_ineffective(keys,
>>>>> + result,
>>>>> + options)
>>>>> + return result
>>>>> +
>>>>> def post_callback(self, ldap, dn, entry_attrs, *keys,
>>>>> **options):
>>>>> assert isinstance(dn, DN)
>>>>> for attr in getattr(context,
>>>>> 'dnsrecord_precallback_attrs', []):
>>>>> @@ -3487,7 +3591,10 @@ class dnsrecord_mod(LDAPUpdate):
>>>>> if self.api.env['wait_for_dns']:
>>>>> self.obj.wait_for_modified_entries(context.dnsrecord_entry_mods)
>>>>> -
>>>>> + if 'nsrecord' in options:
>>>>> + self.obj.warning_if_ns_change_cause_fwzone_ineffective(keys,
>>>>> + result,
>>>>> + options)
>>>>> return result
>>>>> def post_callback(self, ldap, dn, entry_attrs, *keys,
>>>>> **options):
>>>>> @@ -3654,19 +3761,23 @@ class dnsrecord_del(LDAPUpdate):
>>>>> if self.api.env['wait_for_dns']:
>>>>> entries = {(keys[0], keys[1]): None}
>>>>> self.obj.wait_for_modified_entries(entries)
>>>>> - return result
>>>>> + else:
>>>>> + result = super(dnsrecord_del, self).execute(*keys,
>>>>> **options)
>>>>> + result['value'] = pkey_to_value([keys[-1]], options)
>>>>> - result = super(dnsrecord_del, self).execute(*keys,
>>>>> **options)
>>>>> - result['value'] = pkey_to_value([keys[-1]], options)
>>>>> + if getattr(context, 'del_all', False) and not \
>>>>> + self.obj.is_pkey_zone_record(*keys):
>>>>> + result = self.obj.methods.delentry(*keys,
>>>>> +
>>>>> version=options['version'])
>>>>> + context.dnsrecord_entry_mods[(keys[0], keys[1])]
>>>>> = None
>>>>> - if getattr(context, 'del_all', False) and not \
>>>>> - self.obj.is_pkey_zone_record(*keys):
>>>>> - result = self.obj.methods.delentry(*keys,
>>>>> - version=options['version'])
>>>>> - context.dnsrecord_entry_mods[(keys[0], keys[1])] = None
>>>>> + if self.api.env['wait_for_dns']:
>>>>> +
>>>>> self.obj.wait_for_modified_entries(context.dnsrecord_entry_mods)
>>>>> - if self.api.env['wait_for_dns']:
>>>>> - self.obj.wait_for_modified_entries(context.dnsrecord_entry_mods)
>>>>> + if 'nsrecord' in options or options.get('del_all', False):
>>>>> + self.obj.warning_if_ns_change_cause_fwzone_ineffective(keys,
>>>>> + result,
>>>>> + options)
>>>>> return result
>>>>> def post_callback(self, ldap, dn, entry_attrs, *keys,
>>>>> **options):
>>>>> @@ -4019,6 +4130,20 @@ class dnsforwardzone_add(DNSZoneBase_add):
>>>>> return dn
>>>>> + def execute(self, *keys, **options):
>>>>> + result = super(dnsforwardzone_add, self).execute(*keys,
>>>>> **options)
>>>>> + authoritative_zone =
>>>>> _find_zone_which_makes_fw_zone_ineffective(keys[-1])
>>>>> + if authoritative_zone:
>>>>> + # forward zone is not effective and forwarding will
>>>>> not work
>>>>> + messages.add_message(
>>>>> + options['version'], result,
>>>>> + messages.ForwardzoneIsNotEffectiveWarning(
>>>>> + fwzone=keys[-1], authzone=authoritative_zone
>>>>> + )
>>>>> + )
>>>>> +
>>>>> + return result
>>>>> +
>>>>> @register()
>>>>> class dnsforwardzone_del(DNSZoneBase_del):
>>>>> diff --git a/ipatests/test_xmlrpc/test_dns_plugin.py
>>>>> b/ipatests/test_xmlrpc/test_dns_plugin.py
>>>>> index
>>>>> fb53853147ecf663cf7015867131445f32364cfb..224a80d98273480f40fd78dea0f27e34ea36492f
>>>>>
>>>>> 100644
>>>>> --- a/ipatests/test_xmlrpc/test_dns_plugin.py
>>>>> +++ b/ipatests/test_xmlrpc/test_dns_plugin.py
>>>>> @@ -1060,7 +1060,7 @@ class test_dns(Declarative):
>>>>> 'srv_part_target' : u'foo.bar.',
>>>>> 'srvrecord': [u"1 100 1234 %s" \
>>>>> %
>>>>> zone1_ns]}),
>>>>> - expected=errors.ValidationError(name='srv_target',
>>>>> + expected=errors.ValidationError(name='srv_weight',
>>>>> error=u'Raw value of a DNS record was already
>>>>> set by ' +
>>>>> u'"srv_rec" option'),
>>>>> ),
>>>>> -- 1.8.3.1
>>>> The same check should be done also on zone de/activation.
>>>>
>>> Updated patch attached.
>>>
>>> --
>>> Martin Basti
>>>
>>>
>>> freeipa-mbasti-0170.2-Detect-and-warn-about-invalid-DNS-forward-zone-confi.patch
>>>
>> Hello,
>>
>> first of all - the code looks reasonable, I have only couple
>> nitpicks. See below.
>>
>>> From 2a5cf557840e8f578444039326ad90c76bdfb75a Mon Sep 17 00:00:00 2001
>>> From: Martin Basti <mbasti at redhat.com>
>>> 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 | 334
>>> ++++++++++++++++++++++++++++++++++++++++++++++++--
>>> 2 files changed, 336 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..b381dcc5e42761bb6d8d7ec35ef403c6e9b4d629
>>> 100644
>>> --- a/ipalib/plugins/dns.py
>>> +++ b/ipalib/plugins/dns.py
>>> @@ -1725,6 +1725,222 @@ def _normalize_zone(zone):
>>> return zone
>>> +def _get_auth_zone_ldap(name):
>>> + """
>>> + Find authoritative zone in LDAP for name
>>> + :param name:
>>> + :return:
>>> + """
>>> + assert isinstance(name, DNSName)
>>> + ldap = api.Backend.ldap2
>>> +
>>> + # Create all possible parent zone names
>>> + labels = name.make_absolute().ToASCII().split('.')
>> Please use python-dns interface and do not work with domain names as
>> with strings.
>>
>>> + zone_names = ['.', ] # always add root zone
>>> + # decrease len by 1, we already have root zone
>>> + for i in xrange(len(labels) - 1):
>>> + zone_name_abs = '.'.join(labels[i:])
>>> + 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
>>> + )
>> What about truncated return value? It would be better to throw a
>> exception and
>> optionally catch it in _warning* functions if you don't want throw fatal
>> errors from warning-generator.
>>
>>> + except errors.NotFound:
>>> + return None
>>> +
>>> + # 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)
>>> +
>>> +
>>> +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
>>> +
>>> + 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 name if success, or None if no such record
>>> exists, or
>>> + record is zone apex record
>>> + """
>>> + assert isinstance(zone, DNSName)
>>> + assert isinstance(name, DNSName)
>>> +
>>> + ldap = api.Backend.ldap2
>>> +
>>> + # get zone DN
>>> + zone_dn = api.Object.dnszone.get_dn(zone)
>>> +
>>> + if name.is_absolute():
>>> + relative_record_name = name.relativize(zone.make_absolute())
>>> + else:
>>> + relative_record_name = name
>>> +
>>> + # Name is zone apex
>>> + if relative_record_name.is_empty():
>>> + return None
>>> +
>>> + relative_record_name = relative_record_name.ToASCII()
>> Again, please do not work with DNS names as with strings.
>>
>>> + labels = relative_record_name.split('.')
>>> +
>>> + # create list of possible record names
>>> + possible_record_names = ['.'.join(labels[i:]) for i in
>>> xrange(len(labels))]
>>> +
>>> + # search filters
>>> + name_filter = ldap.make_filter({'idnsname':
>>> [possible_record_names]})
>>> + objectclass_filter = ldap.make_filter({'objectclass':
>>> 'idnsrecord'})
>>> + complete_filter = ldap.combine_filters(
>>> + [name_filter, objectclass_filter],
>>> + rules=ldap.MATCH_ALL
>>> + )
>>> +
>>> + try:
>>> + entries, truncated = ldap.find_entries(
>>> + filter=complete_filter,
>>> + attrs_list=['idnsname', 'nsrecord'],
>>> + base_dn=zone_dn,
>>> + scope=ldap.SCOPE_ONELEVEL
>>> + )
>> What about truncated?
>>
>>> + except errors.NotFound:
>>> + return None
>>> +
>>> + matched_records = []
>>> +
>>> + # test if entry contains NS records
>>> + for entry in entries:
>>> + print entry
>>> + if entry.get('nsrecord'):
>>> + matched_records.append(entry.single_value['idnsname'])
>>> +
>>> + if not matched_records:
>>> + return None
>>> +
>>> + # return longest match
>>> + return max(matched_records, key=len)
>>> +
>>> +
>>> +def _find_subtree_forward_zones_ldap(name, child_zones_only=False):
>>> + """
>>> + Search for forwardzone <name> and all child forwardzones
>>> + Filter: (|(*.<name>.)(<name>.))
>>> + :param name:
>>> + :param child_zones_only: search only for child zones
>>> + :return: list of zonenames, or empty list if no zone was found
>>> + """
>>> + assert isinstance(name, DNSName)
>>> + ldap = api.Backend.ldap2
>>> +
>>> + # prepare for filter "*.<name>."
>>> + search_name = u".%s" % name.make_absolute().ToASCII()
>>> + # we need to search zone with and without last dot, due
>>> compatibility
>>> + # with IPA < 4.0
>>> + search_names = [search_name, search_name[:-1]]
>>> +
>>> + # Create filters
>>> + objectclass_filter =
>>> ldap.make_filter({'objectclass':'idnsforwardzone'})
>>> + zonenames_filter = ldap.make_filter({'idnsname': search_names},
>>> exact=False,
>>> + trailing_wildcard=False)
>>> + if not child_zones_only:
>>> + # find also zone with exact name
>>> + exact_name = name.make_absolute().ToASCII()
>> Again, please do not work with DNS names as with strings.
>>
>>> + # we need to search zone with and without last dot, due
>>> compatibility
>>> + # with IPA < 4.0
>>> + exact_names = [exact_name, exact_name[-1]]
>>> + exact_name_filter = ldap.make_filter({'idnsname':
>>> exact_names})
>>> + zonenames_filter = ldap.combine_filters([zonenames_filter,
>>> + exact_name_filter])
>>> +
>>> + 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
>>> + )
>> What about truncated?
>>
>>> + except errors.NotFound:
>>> + return []
>>> +
>>> + result = [entry.single_value['idnsname'].make_absolute()
>>> + for entry in entries]
>>> +
>>> + return result
>>> +
>>> +
>>> +def _get_zone_which_makes_fw_zone_ineffective(fwzonename):
>>> + """
>>> + Check if forward zone is effective.
>>> +
>>> + If parent zone exists as authoritative zone, the forward zone
>>> will not
>>> + forward queries by default. It is necessary to delegate authority
>>> + to forward zone with a NS record.
>>> +
>>> + Example:
>>> +
>>> + Forward zone: sub.example.com
>>> + Zone: example.com
>>> +
>>> + Forwarding will not work, because the server thinks it is
>>> authoritative
>>> + for zone and will return NXDOMAIN
>>> +
>>> + Adding record: sub.example.com NS ns.sub.example.com.
>>> + will delegate authority, and IPA DNS server will forward DNS
>>> queries.
>>> +
>>> + :param fwzonename: forwardzone
>>> + :return: None if effective, name of authoritative zone otherwise
>>> + """
>>> + assert isinstance(fwzonename, DNSName)
>>> +
>>> + auth_zone = _get_auth_zone_ldap(fwzonename)
>>> + if not auth_zone:
>>> + return None
>>> +
>>> + delegation_record_name = _get_longest_match_ns_delegation_ldap(
>>> + auth_zone, fwzonename)
>>> +
>>> + if delegation_record_name:
>>> + return None
>>> +
>>> + return auth_zone
>>> +
>>> +
>>> class DNSZoneBase(LDAPObject):
>>> """
>>> Base class for DNS Zone
>>> @@ -2376,6 +2592,30 @@ class dnszone(DNSZoneBase):
>>> )
>>> )
>>> + def _warning_fw_zone_is_not_effective(self, result, *keys,
>>> **options):
>>> + """
>>> + Warning if any operation with zone causes, a child forward
>>> zone is
>>> + not effective
>>> + """
>>> + zone = keys[-1]
>>> + affected_fw_zones = _find_subtree_forward_zones_ldap(
>>> + zone, child_zones_only=True)
>>> + if not affected_fw_zones:
>>> + return
>>> +
>>> + for fwzone in affected_fw_zones:
>>> + authoritative_zone = \
>>> + _get_zone_which_makes_fw_zone_ineffective(fwzone)
>>> + if authoritative_zone:
>>> + # forward zone is not effective and forwarding will
>>> not work
>>> + messages.add_message(
>>> + options['version'], result,
>>> + messages.ForwardzoneIsNotEffectiveWarning(
>>> + fwzone=fwzone, authzone=authoritative_zone,
>>> + ns_rec=fwzone.relativize(authoritative_zone)
>>> + )
>>> + )
>>> +
>>> @register()
>>> class dnszone_add(DNSZoneBase_add):
>>> __doc__ = _('Create new DNS zone (SOA record).')
>>> @@ -2445,6 +2685,7 @@ class dnszone_add(DNSZoneBase_add):
>>> self.obj._warning_forwarding(result, **options)
>>> self.obj._warning_dnssec_experimental(result, *keys,
>>> **options)
>>> self.obj._warning_name_server_option(result, context,
>>> **options)
>>> + self.obj._warning_fw_zone_is_not_effective(result, *keys,
>>> **options)
>>> return result
>>> def post_callback(self, ldap, dn, entry_attrs, *keys,
>>> **options):
>>> @@ -2475,6 +2716,13 @@ class dnszone_del(DNSZoneBase_del):
>>> msg_summary = _('Deleted DNS zone "%(value)s"')
>>> + def execute(self, *keys, **options):
>>> + result = super(dnszone_del, self).execute(*keys, **options)
>>> + nkeys = keys[-1] # we can delete more zones
>>> + for key in nkeys:
>>> + self.obj._warning_fw_zone_is_not_effective(result, key, **options)
>>> + return result
>>> +
>>> def post_callback(self, ldap, dn, *keys, **options):
>>> super(dnszone_del, self).post_callback(ldap, dn, *keys,
>>> **options)
>>> @@ -2595,12 +2843,22 @@ class dnszone_disable(DNSZoneBase_disable):
>>> __doc__ = _('Disable DNS Zone.')
>>> msg_summary = _('Disabled DNS zone "%(value)s"')
>>> + def execute(self, *keys, **options):
>>> + result = super(dnszone_disable, self).execute(*keys,
>>> **options)
>>> + self.obj._warning_fw_zone_is_not_effective(result, *keys,
>>> **options)
>>> + return result
>>> +
>>> @register()
>>> class dnszone_enable(DNSZoneBase_enable):
>>> __doc__ = _('Enable DNS Zone.')
>>> msg_summary = _('Enabled DNS zone "%(value)s"')
>>> + def execute(self, *keys, **options):
>>> + result = super(dnszone_enable, self).execute(*keys, **options)
>>> + self.obj._warning_fw_zone_is_not_effective(result, *keys,
>>> **options)
>>> + return result
>>> +
>>> @register()
>>> class dnszone_add_permission(DNSZoneBase_add_permission):
>>> @@ -3164,6 +3422,30 @@ class dnsrecord(LDAPObject):
>>> dns_name = entry_name[1].derelativize(dns_domain)
>>> self.wait_for_modified_attrs(entry, dns_name, dns_domain)
>>> + def warning_if_ns_change_cause_fwzone_ineffective(self,
>>> result, *keys,
>>> + **options):
>>> + """after execute"""
>> Please add more descriptive comment to doc string.
>>
>>> + record_name_absolute = keys[-1]
>> a variable with zone name instead of keys[-2] would make it more
>> readable
>>
>>> + if not keys[-1].is_absolute():
>> record_name_absolute.is_absolute() would be better
>>
>>> + record_name_absolute = keys[-1].derelativize(keys[-2])
>> again, please replace keys[x] with sensible names
>>
>>> +
>>> + affected_fw_zones = _find_subtree_forward_zones_ldap(
>>> + record_name_absolute)
>>> + if not affected_fw_zones:
>>> + return
>>> +
>>> + for fwzone in affected_fw_zones:
>> Would it be possible to generalize & use
>> _warning_fw_zone_is_not_effective() here?
>>
>>> + authoritative_zone = \
>>> + _get_zone_which_makes_fw_zone_ineffective(fwzone)
>>> + if authoritative_zone:
>>> + # forward zone is not effective and forwarding will
>>> not work
>>> + messages.add_message(
>>> + options['version'], result,
>>> + messages.ForwardzoneIsNotEffectiveWarning(
>>> + fwzone=fwzone, authzone=authoritative_zone,
>>> + ns_rec=fwzone.relativize(authoritative_zone)
>>> + )
>>> + )
>>> @register()
>>> @@ -3487,7 +3769,10 @@ class dnsrecord_mod(LDAPUpdate):
>>> if self.api.env['wait_for_dns']:
>>> self.obj.wait_for_modified_entries(context.dnsrecord_entry_mods)
>>> -
>>> + if 'nsrecord' in options:
>>> + self.obj.warning_if_ns_change_cause_fwzone_ineffective(result,
>>> + *keys,
>>> + **options)
>>> return result
>>> def post_callback(self, ldap, dn, entry_attrs, *keys,
>>> **options):
>>> @@ -3654,19 +3939,23 @@ class dnsrecord_del(LDAPUpdate):
>>> if self.api.env['wait_for_dns']:
>>> entries = {(keys[0], keys[1]): None}
>>> self.obj.wait_for_modified_entries(entries)
>>> - return result
>>> + else:
>>> + result = super(dnsrecord_del, self).execute(*keys,
>>> **options)
>>> + result['value'] = pkey_to_value([keys[-1]], options)
>>> - result = super(dnsrecord_del, self).execute(*keys,
>>> **options)
>>> - result['value'] = pkey_to_value([keys[-1]], options)
>>> + if getattr(context, 'del_all', False) and not \
>>> + self.obj.is_pkey_zone_record(*keys):
>>> + result = self.obj.methods.delentry(*keys,
>>> + version=options['version'])
>>> + context.dnsrecord_entry_mods[(keys[0], keys[1])] =
>>> None
>>> - if getattr(context, 'del_all', False) and not \
>>> - self.obj.is_pkey_zone_record(*keys):
>>> - result = self.obj.methods.delentry(*keys,
>>> - version=options['version'])
>>> - context.dnsrecord_entry_mods[(keys[0], keys[1])] = None
>>> + if self.api.env['wait_for_dns']:
>>> + self.obj.wait_for_modified_entries(context.dnsrecord_entry_mods)
>>> - if self.api.env['wait_for_dns']:
>>> - self.obj.wait_for_modified_entries(context.dnsrecord_entry_mods)
>>> + if 'nsrecord' in options or options.get('del_all', False):
>>> + self.obj.warning_if_ns_change_cause_fwzone_ineffective(result,
>>> + *keys,
>>> + **options)
>>> return result
>>> def post_callback(self, ldap, dn, entry_attrs, *keys,
>>> **options):
>>> @@ -3998,6 +4287,19 @@ class dnsforwardzone(DNSZoneBase):
>>> # managed_permissions: permissions was apllied in dnszone
>>> class, do NOT
>>> # add them here, they should not be applied twice.
>>> + def _warning_fw_zone_is_not_effective(self, result, *keys,
>>> **options):
>>> + fwzone = keys[-1]
>>> + authoritative_zone =
>>> _get_zone_which_makes_fw_zone_ineffective(
>>> + fwzone)
>>> + if authoritative_zone:
>>> + # forward zone is not effective and forwarding will not
>>> work
>>> + messages.add_message(
>>> + options['version'], result,
>>> + messages.ForwardzoneIsNotEffectiveWarning(
>>> + fwzone=fwzone, authzone=authoritative_zone,
>>> + ns_rec=fwzone.relativize(authoritative_zone)
>>> + )
>>> + )
>>> @register()
>>> class dnsforwardzone_add(DNSZoneBase_add):
>>> @@ -4019,6 +4321,11 @@ class dnsforwardzone_add(DNSZoneBase_add):
>>> return dn
>>> + def execute(self, *keys, **options):
>>> + result = super(dnsforwardzone_add, self).execute(*keys,
>>> **options)
>>> + self.obj._warning_fw_zone_is_not_effective(result, *keys,
>>> **options)
>>> + return result
>>> +
>>> @register()
>>> class dnsforwardzone_del(DNSZoneBase_del):
>>> @@ -4083,6 +4390,11 @@ class dnsforwardzone_enable(DNSZoneBase_enable):
>>> __doc__ = _('Enable DNS Forward Zone.')
>>> msg_summary = _('Enabled DNS forward zone "%(value)s"')
>>> + def execute(self, *keys, **options):
>>> + result = super(dnsforwardzone_enable, self).execute(*keys,
>>> **options)
>>> + self.obj._warning_fw_zone_is_not_effective(result, *keys,
>>> **options)
>>> + return result
>>> +
>>> @register()
>>> class dnsforwardzone_add_permission(DNSZoneBase_add_permission):
>>> -- 1.8.3.1
>> NACK
>>
>> Thank you for your patience with me ;-)
>>
> Updated patch attached.
>
Updated patch attached.
Removes change in tests, which causes false positive error.
--
Martin Basti
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mbasti-0170.4-Detect-and-warn-about-invalid-DNS-forward-zone-confi.patch
Type: text/x-patch
Size: 17034 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20141210/877460b6/attachment.bin>
More information about the Freeipa-devel
mailing list