[Freeipa-devel] [PATCH 0083] perform an unlimited search for reverse zones when adding DNS records
Martin Babinsky
mbabinsk at redhat.com
Fri Oct 16 10:13:58 UTC 2015
On 10/14/2015 04:05 PM, Petr Spacek wrote:
> 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 <mbabinsk at redhat.com>
>>>> 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.
>
Upon Thy request I purged the Function that Gets the Name of the Zone
Reverse of the crawling evil thee caleth the Prefixlen.
The shallow impostor of The Function whose services no Module required
nor used was also banished to oblivion and His noisome vapors shall
bother the Kingdom of Identity no more.
Thou shalt rejoice when Thy review the rewritten Patch attached herein
or else I shall be smitten by the mighty NACK thee wieldeth.
--
Martin^3 Babinsky
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mbabinsk-0083.3-always-ask-the-resolver-for-the-reverse-zone-when-ma.patch
Type: text/x-patch
Size: 4568 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20151016/0dc5b405/attachment.bin>
More information about the Freeipa-devel
mailing list