[Freeipa-devel] [PATCH 0083] perform an unlimited search for reverse zones when adding DNS records
Martin Babinsky
mbabinsk at redhat.com
Wed Oct 14 12:13:49 UTC 2015
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.
--
Martin^3 Babinsky
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mbabinsk-0083.2-perform-more-robust-search-for-reverse-zones-when-ad.patch
Type: text/x-patch
Size: 3493 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20151014/1962ae96/attachment.bin>
More information about the Freeipa-devel
mailing list