On 09/12/14 17:40, Martin Basti wrote:
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 <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.
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 <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.
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.
Updated patch attached.
Removes change in tests, which causes false positive error.
--
Martin Basti
From a1b70e7a12ffdb08941d43587a05d7e36b57ab2b 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.
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
+ :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
+
+ 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
+ """
+ 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
+ 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.
+ """
+ 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
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
