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 <[email protected]> > 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(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, False > + > + # create list of possible record names > + possible_record_names = [DNSName(relative_record_name[i:]).ToASCII() > + for i in xrange(len(relative_record_name))] > + > + # 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 > + ) > + except errors.NotFound: > + return None, False > + > + matched_records = [] > + > + # test if entry contains NS records > + for entry in entries: > + print entry Not-a-nitpick :-) ^^^ > + if entry.get('nsrecord'): > + matched_records.append(entry.single_value['idnsname']) > + > + if not matched_records: > + return None, truncated > + > + # return longest match > + return max(matched_records, key=len), truncated > + > + > +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, truncated), list is empty if no zone 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() > + # 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 > + ) > + except errors.NotFound: > + return [], False > + > + result = [entry.single_value['idnsname'].make_absolute() > + for entry in entries] > + > + return result, truncated > + > + > +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: (zone, truncated) > + zone: None if effective, name of authoritative zone otherwise > + """ > + assert isinstance(fwzonename, DNSName) > + > + auth_zone, truncated_zone = _get_auth_zone_ldap(fwzonename) > + if not auth_zone: > + return None, truncated_zone > + > + delegation_record_name, truncated_ns =\ > + _get_longest_match_ns_delegation_ldap(auth_zone, fwzonename) > + > + truncated = truncated_ns or truncated_zone > + > + if delegation_record_name: > + return None, truncated > + > + return auth_zone, truncated > + > + > +def _add_warning_fw_zone_is_not_effective(result, fwzone, version): > + """ > + Adds warning message to result, if required > + """ > + authoritative_zone, truncated = \ > + _get_zone_which_makes_fw_zone_ineffective(fwzone) > + if authoritative_zone: > + # forward zone is not effective and forwarding will not work > + messages.add_message( > + version, result, > + messages.ForwardzoneIsNotEffectiveWarning( > + fwzone=fwzone, authzone=authoritative_zone, > + ns_rec=fwzone.relativize(authoritative_zone) > + ) > + ) > + > + > class DNSZoneBase(LDAPObject): > """ > Base class for DNS Zone > @@ -2376,6 +2611,22 @@ 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, truncated = _find_subtree_forward_zones_ldap( > + zone, child_zones_only=True) > + if not affected_fw_zones: > + return > + > + for fwzone in affected_fw_zones: > + _add_warning_fw_zone_is_not_effective(result, fwzone, > + options['version']) > + > + > @register() > class dnszone_add(DNSZoneBase_add): > __doc__ = _('Create new DNS zone (SOA record).') > @@ -2445,6 +2696,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 +2727,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 +2854,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 +3433,25 @@ 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): > + """Detect if NS record change can make forward zones ineffective due > + missing delegation. Run after parent's execute method method. Nitpick: method. > + """ > + record_name_absolute = keys[-1] > + zone = keys[-2] > + > + if not record_name_absolute.is_absolute(): > + record_name_absolute = record_name_absolute.derelativize(zone) > + > + affected_fw_zones, truncated = _find_subtree_forward_zones_ldap( > + record_name_absolute) > + if not affected_fw_zones: > + return > + > + for fwzone in affected_fw_zones: > + _add_warning_fw_zone_is_not_effective(result, fwzone, > + options['version']) > > > @register() > @@ -3487,7 +3775,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 +3945,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 +4293,11 @@ 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] > + _add_warning_fw_zone_is_not_effective(result, fwzone, > + options['version']) > + > > @register() > class dnsforwardzone_add(DNSZoneBase_add): > @@ -4019,6 +4319,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 +4388,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 Thank you for patience! -- Petr^2 Spacek _______________________________________________ Freeipa-devel mailing list [email protected] https://www.redhat.com/mailman/listinfo/freeipa-devel
