[Freeipa-devel] [PATCH 0170] Detect and warn about invalid forwardzone configuration
Petr Spacek
pspacek at redhat.com
Mon Nov 24 16:24:00 UTC 2014
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.
--
Petr^2 Spacek
More information about the Freeipa-devel
mailing list