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