[Freeipa-devel] [PATCH 0083] perform an unlimited search for reverse zones when adding DNS records

Wed Oct 14 14:05:35 UTC 2015

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.

-- 
Petr^2 Spacek