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 <mba...@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. -- Petr^2 Spacek _______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
