[Freeipa-devel] [PATCH 0170] Detect and warn about invalid forwardzone configuration
Martin Basti
mbasti at redhat.com
Tue Dec 9 16:40:34 UTC 2014
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.
--
Martin Basti
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mbasti-0170.3-Detect-and-warn-about-invalid-DNS-forward-zone-confi.patch
Type: text/x-patch
Size: 17775 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20141209/e5ce6710/attachment.bin>
More information about the Freeipa-devel
mailing list