On 14.10.2015 14:13, Martin Babinsky wrote: > On 10/13/2015 04:00 PM, Petr Spacek wrote: >> On 13.10.2015 13:37, Martin Babinsky wrote: >>> On 10/13/2015 09:36 AM, Petr Spacek wrote: >>>> On 12.10.2015 16:35, Martin Babinsky wrote: >>>>> https://fedorahosted.org/freeipa/ticket/5200 >>>>> --- >>>>> ipalib/plugins/dns.py | 3 ++- >>>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/ipalib/plugins/dns.py b/ipalib/plugins/dns.py >>>>> index >>>>> 84086f4c77d02922f237937d58031cc42d55410e..c36345faecfb9db7abced1c6bd72ddcf93473a74 >>>>> >>>>> 100644 >>>>> --- a/ipalib/plugins/dns.py >>>>> +++ b/ipalib/plugins/dns.py >>>>> @@ -537,7 +537,8 @@ def get_reverse_zone(ipaddr, prefixlen=None): >>>>> if prefixlen is None: >>>>> revzone = None >>>>> >>>>> - result = api.Command['dnszone_find']()['result'] >>>>> + result = api.Command['dnszone_find'](sizelimit=0)['result'] >>>>> + >>>> >>>> NACK, this just increases the limit because LDAP server will enforce a >>>> per-user limit. >>>> >>>>> for zone in result: >>>>> zonename = zone['idnsname'][0] >>>>> if (revdns.is_subdomain(zonename.make_absolute()) and >>>> >>>> Generic solution should use dns.resolver.zone_for_name() to find DNS zone >>>> matching the reverse name. This should also implicitly cover CNAME/DNAME >>>> redirections per RFC2317. >>>> >>>> Using DNS implicitly means that a zone will always be found (at least the >>>> root >>>> zone :-). For this reason I would change final error message >>>>> reason=_('DNS reverse zone for IP address %(addr)s not found') >>>> to something like: >>>> reason=_('DNS reverse zone %(zone)s for IP address %(addr)s is not >>>> managed >>>> by this server') >>>> >>>> >>>> These changes should fix it without adding other artificial limitation. >>>> >>> >>> Thank you for clarification Petr. >>> >>> Attaching the reworked patch. >>> >>> -- >>> Martin^3 Babinsky >>> >>> freeipa-mbabinsk-0083.1-perform-more-robust-search-for-reverse-zones-when-ad.patch >>> >>> >>> >>> From 463903f4ea4f74efeb02dbb705ca0a54fab81366 Mon Sep 17 00:00:00 2001 >>> From: Martin Babinsky <[email protected]> >>> Date: Mon, 12 Oct 2015 16:24:50 +0200 >>> Subject: [PATCH 1/3] perform more robust search for reverse zones when >>> adding >>> DNS record >>> >>> Instead of searching for all zones to identify the correct reverse zone, we >>> will first ask the resolver to return the name of zone that should contain >>> the >>> desired record and then see if IPA manages this zone. >>> >>> https://fedorahosted.org/freeipa/ticket/5200 >>> --- >>> ipalib/plugins/dns.py | 21 +++++++++++++++++---- >>> 1 file changed, 17 insertions(+), 4 deletions(-) >>> >>> diff --git a/ipalib/plugins/dns.py b/ipalib/plugins/dns.py >>> index >>> 84086f4c77d02922f237937d58031cc42d55410e..e3c6ce46168f8b6af607a61af1e3e431cfee32c8 >>> 100644 >>> --- a/ipalib/plugins/dns.py >>> +++ b/ipalib/plugins/dns.py >>> @@ -531,25 +531,35 @@ def add_forward_record(zone, name, str_address): >>> pass # the entry already exists and matches >>> >>> def get_reverse_zone(ipaddr, prefixlen=None): >>> + """ >>> + resolve the reverse zone for IP address and see if it is managed by IPA >>> + server >>> + :param ipaddr: host IP address >>> + :param prefixlen: network prefix length >>> + :return: tuple containing name of the reverse zone and the name of the >>> + record >>> + """ >>> ip = netaddr.IPAddress(str(ipaddr)) >>> revdns = DNSName(unicode(ip.reverse_dns)) >>> + resolved_zone = unicode(dns.resolver.zone_for_name(revdns)) >>> >>> if prefixlen is None: >>> revzone = None >>> + result = api.Command['dnszone_find'](resolved_zone)['result'] >>> >>> - result = api.Command['dnszone_find']()['result'] >>> for zone in result: >>> zonename = zone['idnsname'][0] >>> if (revdns.is_subdomain(zonename.make_absolute()) and >>> (revzone is None or zonename.is_subdomain(revzone))): >>> revzone = zonename >> >> Oh, wait, this might blow up if there is a disabled DNS zone which is deeper >> than the one returned from DNS query. >> >> Damn, we have opened a Pandora box! >> >> ipaserver/install/bindinstance.py has own implementation of >> get_reverse_zone() >> + additional menthods find_reverse_zone(). >> These are using get_reverse_zone_default() from ipalib/util.py which is >> duplicating code in 'if prefixlen is not None' branch. >> >> And of course, are not correct in light of this change. >> >> My expectation would be that disabled zones should be ignored... So we should >> get rid of this mess in the loop and use dnszone_show() which is called at >> the >> end. And fix the other places, preferably by deleting duplicate >> implementations. >> >> I would expect that you can store (converted) output of >> dns.resolver.zone_for_name(revdns) into revzone and delete whole 'if >> prefixlen >> is None' branch. >> >>> else: >>> + # if prefixlen is specified, determine the zone name for the >>> network >>> + # prefix >>> if ip.version == 4: >>> pos = 4 - prefixlen / 8 >>> elif ip.version == 6: >>> pos = 32 - prefixlen / 4 >>> - items = ip.reverse_dns.split('.') >>> - revzone = DNSName(items[pos:]) >>> + revzone = revdns[pos:] >> >> Hmm, and this logic will breaks CNAME/DNAME (RFC2317) handling ... Damn, we >> need different handling when we intend to create the zone and when we want to >> manipulate a record in a reverse zone. Grrr. >> >> When we want to manipulate a record in existing zone, the whole branch does >> not make sense and the result of DNS query is what we want and need. >> >> On the other hand, we need this when creating *a new zone* from IP address >> ... >> >> Mastin^2, what about zone names with missing period at the end? Is it handled >> by dnszone_show() internally? >> >> We have to discuss all this... >> >>> try: >>> api.Command['dnszone_show'](revzone) >>> @@ -558,7 +568,10 @@ def get_reverse_zone(ipaddr, prefixlen=None): >>> >>> if revzone is None: >>> raise errors.NotFound( >>> - reason=_('DNS reverse zone for IP address %(addr)s not found') >>> % dict(addr=ipaddr) >>> + reason=_( >>> + 'DNS reverse zone %(resolved_zone)s for IP address ' >>> + '%(addr)s is not manager by this server') % dict( >> typo s/manager/managed/ >> >>> + addr=ipaddr, resolved_zone=resolved_zone) >>> ) >>> >>> revname = revdns.relativize(revzone) >> > > After lengthy discussion with Petr^2 I we decided to rework get_reverse_zone > function a bit. Attaching updated patch.
Well, we should get rid of prefixlen parameter. Wipe it from Earth's surface! :-) The assumption here is that this function is always used only for manipulating *records in existing zones*, so the only way to find appropriate reverse zone name is to ask DNS. Derivation based on prefix length makes sense only for deriving reverse zone names from IP addresses *with intent to create the zone*. In all other cases it does not make sense to use prefix. I'm sorry that I was not clear. -- Petr^2 Spacek -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
