[Freeipa-devel] [PATCH 0083] perform an unlimited search for reverse zones when adding DNS records
Martin Basti
mbasti at redhat.com
Thu Oct 22 16:36:46 UTC 2015
On 22.10.2015 16:59, Petr Spacek wrote:
> On 16.10.2015 12:13, Martin Babinsky wrote:
>> 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.
> Mighty ACK! :-D
>
> Thank you for your patience with me.
>
Pushed to:
ipa-4-2: 2d4854480c1457a77dde2a3ed53f464521dd7254
master: c43dce3a61e17791cc31f45498bae2d52edcf969
More information about the Freeipa-devel
mailing list