On 09/17/2012 09:35 PM, Rob Crittenden wrote: > Martin Kosek wrote: >> On 09/05/2012 01:02 PM, Jan Cholasta wrote: >>> Dne 5.9.2012 12:48, Martin Kosek napsal(a): >>>> On 09/05/2012 12:36 PM, Jan Cholasta wrote: >>>>> Dne 5.9.2012 12:22, Petr Spacek napsal(a): >>>>>> On 09/05/2012 11:30 AM, Jan Cholasta wrote: >>>>>>> Dne 5.9.2012 10:04, Martin Kosek napsal(a): >>>>>>>> We allowed IP addresses without network specification which lead >>>>>>>> to unexpected results when the zone was being created. We should rather >>>>>>>> strictly require the prefix/netmask specifying the IP network that >>>>>>>> the reverse zone should be created for. This is already done in >>>>>>>> Web UI. >>>>>>>> >>>>>>>> A unit test exercising this new validation was added. >>>>>>>> >>>>>>>> https://fedorahosted.org/freeipa/ticket/2461 >>>>>>>> >>>>>>> >>>>>>> I don't like this much. I would suggest using CheckedIPAddress and not >>>>>>> forcing >>>>>>> the user to enter the prefix length instead. >>>>>>> >>>>>>> CheckedIPAddress uses a sensible default prefix length if one is not >>>>>>> specified >>>>>>> (class-based for IPv4, /64 for IPv6) as opposed to IPNetwork (/32 for >>>>>>> IPv4, >>>>>>> /128 for IPv6 - this causes the erroneous reverse zones to be created as >>>>>>> described in the ticket). >>>>>>> >>>>>> Hello, >>>>>> >>>>>> I don't like automatic netmask guessing. I have met class-based guessing >>>>>> in Windows (XP?) and I was forced to overwrite default mask all the time >>>>>> ... >>>>> >>>>> If there was no guessing, you would have to write the netmask anyway, so I >>>>> don't see any harm in guessing here. >>>>> >>>>>> >>>>>> IMHO there is no "sensible default prefix" in real world. I sitting on >>>>>> network with /23 prefix right now. Also, I have never seen 10.x network >>>>>> with /8 prefix. >>>>>> >>>>> >>>>> While this might be true for IPv4 in some cases, /64 is perfectly sensible >>>>> for >>>>> IPv6. Also, I have never seen 192.168.x.x network with non-/24 prefix. >>>>> >>>>> Honza >>>>> >>>> >>>> While this may be true for 192.168.x.x, it does not apply for 10.x.x.x >>>> networks >>>> as Petr already pointed out. I don't think that there will be many people >>>> expecting that a reverse zone of 10.0.0.0/24 would be created. >>> >>> And they would be correct, because the default prefix length for a class A >>> network is /8, not /24. >>> >>>> >>>> And since FreeIPA is mainly deployed to internal networks, I assume this >>>> will >>>> be the case of most users. >>>> >>>> Martin >>>> >>> >>> OK, but what about IPv6? Correct me if I'm wrong, but the prefix length is >>> going to be /64 99% of the time for IPv6. >>> >>> The installer uses /24 for IPv4 addresses and /64 for IPv6 addresses, maybe >>> this should be used as a default here as well. >>> >>> Honza >>> >> >> In the end, I choose a more liberal approach and instead of defining a more >> stricter validator for IPv4 only I rather used approach already implemented >> in >> the installers, i.e. default length of network prefix is 24 for IPv4 and 64 >> for >> IPv6. >> >> Updated patch attached. >> >> Martin > > Works for me. I wonder if this is a candidate for some more unit tests... > > rob >
One more test should not hurt. Updated patch attached. Martin
From 8e9908b4c4d22921227ed4a10adb780a88821043 Mon Sep 17 00:00:00 2001 From: Martin Kosek <mko...@redhat.com> Date: Wed, 5 Sep 2012 09:56:27 +0200 Subject: [PATCH] Use default reverse zone consistently When a new reverse zone is to be generated based on an IP address without a network prefix length, we need to use some default value. While netaddr library default ones (32b for IPv4 and 128b for IPv6) are not very sensible we should use the defaults already applied in installers. That is 24b for IPv6 and 64 for IPv6. Test case has been added to cover the new default. https://fedorahosted.org/freeipa/ticket/2461 --- install/tools/ipa-dns-install | 2 +- install/tools/ipa-replica-install | 2 +- install/tools/ipa-server-install | 2 +- ipalib/plugins/dns.py | 11 +++++++- ipalib/util.py | 18 ++++++++++++ ipaserver/install/bindinstance.py | 20 ++----------- tests/test_xmlrpc/test_dns_plugin.py | 54 ++++++++++++++++++++++++++++++------ 7 files changed, 78 insertions(+), 31 deletions(-) diff --git a/install/tools/ipa-dns-install b/install/tools/ipa-dns-install index d4795f72e03eed1b460a3751fc5596ac6da70900..84d1bdc2eb5729896ecb62f65feb11122aacf77d 100755 --- a/install/tools/ipa-dns-install +++ b/install/tools/ipa-dns-install @@ -213,7 +213,7 @@ def main(): else: reverse_zone = bindinstance.find_reverse_zone(ip) if reverse_zone is None and not options.no_reverse: - reverse_zone = bindinstance.get_reverse_zone_default(ip) + reverse_zone = util.get_reverse_zone_default(ip) if not options.unattended and bindinstance.create_reverse(): reverse_zone = bindinstance.read_reverse_zone(reverse_zone, ip) diff --git a/install/tools/ipa-replica-install b/install/tools/ipa-replica-install index 267a70d8b60d96de9a9bde83b15c81ae59da1a96..57a8de16344821ad142a820d7c84a4b31a1fe274 100755 --- a/install/tools/ipa-replica-install +++ b/install/tools/ipa-replica-install @@ -222,7 +222,7 @@ def install_bind(config, options): else: reverse_zone = bindinstance.find_reverse_zone(config.ip) if reverse_zone is None and not options.no_reverse: - reverse_zone = bindinstance.get_reverse_zone_default(config.ip) + reverse_zone = util.get_reverse_zone_default(config.ip) if not options.unattended and bindinstance.create_reverse(): reverse_zone = bindinstance.read_reverse_zone(reverse_zone, config.ip) diff --git a/install/tools/ipa-server-install b/install/tools/ipa-server-install index f07aeadf8a18962541c5e548845d612d330e8669..f642a361e1c11dbceaaa2741da1dd0f89af08be3 100755 --- a/install/tools/ipa-server-install +++ b/install/tools/ipa-server-install @@ -807,7 +807,7 @@ def main(): if options.reverse_zone: reverse_zone = bindinstance.normalize_zone(options.reverse_zone) elif not options.no_reverse: - reverse_zone = bindinstance.get_reverse_zone_default(ip) + reverse_zone = util.get_reverse_zone_default(ip) if not options.unattended and bindinstance.create_reverse(): reverse_zone = bindinstance.read_reverse_zone(reverse_zone, ip) diff --git a/ipalib/plugins/dns.py b/ipalib/plugins/dns.py index 562f23a015f8fd6f6a11fceb3a472df39185ed6a..5484119d40f50e450a7ddaa07258942500668bec 100644 --- a/ipalib/plugins/dns.py +++ b/ipalib/plugins/dns.py @@ -33,7 +33,8 @@ from ipalib.plugins.baseldap import * from ipalib import _, ngettext from ipalib.util import (validate_zonemgr, normalize_zonemgr, validate_hostname, validate_dns_label, validate_domain_name, - get_dns_forward_zone_update_policy, get_dns_reverse_zone_update_policy) + get_dns_forward_zone_update_policy, get_dns_reverse_zone_update_policy, + get_reverse_zone_default) from ipapython.ipautil import valid_ip, CheckedIPAddress, is_host_resolvable __doc__ = _(""" @@ -254,6 +255,14 @@ def _create_zone_serial(): return int(time.time()) def _reverse_zone_name(netstr): + try: + netaddr.IPAddress(netstr) + except (netaddr.AddrFormatError, ValueError): + pass + else: + # use more sensible default prefix than netaddr default + return unicode(get_reverse_zone_default(netstr)) + net = netaddr.IPNetwork(netstr) items = net.ip.reverse_dns.split('.') if net.version == 4: diff --git a/ipalib/util.py b/ipalib/util.py index 1d5900924135f427da48df6b5693b686ae89eb7f..df8791ba0b2fda818100f8869774ded767fb1777 100644 --- a/ipalib/util.py +++ b/ipalib/util.py @@ -27,6 +27,7 @@ import time import socket import re import decimal +import netaddr from types import NoneType from weakref import WeakKeyDictionary from dns import resolver, rdatatype @@ -172,6 +173,12 @@ def normalize_zonemgr(zonemgr): return zonemgr +def normalize_zone(zone): + if zone[-1] != '.': + return zone + '.' + else: + return zone + def validate_dns_label(dns_label, allow_underscore=False): label_chars = r'a-z0-9' underscore_err_msg = '' @@ -487,6 +494,17 @@ def get_dns_reverse_zone_update_policy(realm, reverse_zone, rrtypes=('PTR',)): return policy +def get_reverse_zone_default(ip_address): + ip = netaddr.IPAddress(ip_address) + items = ip.reverse_dns.split('.') + + if ip.version == 4: + items = items[1:] # /24 for IPv4 + elif ip.version == 6: + items = items[16:] # /64 for IPv6 + + return normalize_zone('.'.join(items)) + def validate_rdn_param(ugettext, value): try: rdn = RDN(value) diff --git a/ipaserver/install/bindinstance.py b/ipaserver/install/bindinstance.py index 8284f3eaa5851ab55cf35e846fc51cd559c35893..c2c4a86b4b49ec044969cf4a748a062874031f2b 100644 --- a/ipaserver/install/bindinstance.py +++ b/ipaserver/install/bindinstance.py @@ -33,7 +33,8 @@ from ipapython import sysrestore from ipapython import ipautil from ipalib.parameters import IA5Str from ipalib.util import (validate_zonemgr, normalize_zonemgr, - get_dns_forward_zone_update_policy, get_dns_reverse_zone_update_policy) + get_dns_forward_zone_update_policy, get_dns_reverse_zone_update_policy, + normalize_zone, get_reverse_zone_default) from ipapython.ipa_log_manager import * from ipalib.text import _ @@ -72,12 +73,6 @@ def check_inst(unattended): return True -def normalize_zone(zone): - if zone[-1] != '.': - return zone + '.' - else: - return zone - def create_reverse(): return ipautil.user_input("Do you want to configure the reverse zone?", True) @@ -231,17 +226,6 @@ def verify_reverse_zone(zone, ip_address): return True -def get_reverse_zone_default(ip_address): - ip = netaddr.IPAddress(ip_address) - items = ip.reverse_dns.split('.') - - if ip.version == 4: - items = items[1:] # /24 for IPv4 - elif ip.version == 6: - items = items[16:] # /64 for IPv6 - - return normalize_zone('.'.join(items)) - def find_reverse_zone(ip_address): ip = netaddr.IPAddress(ip_address) zone = normalize_zone(ip.reverse_dns) diff --git a/tests/test_xmlrpc/test_dns_plugin.py b/tests/test_xmlrpc/test_dns_plugin.py index e8c0b241cc56261061de3cf4397ec097683f10a9..6a54bcb638da9463e5b6a094a7789dc7b0f23310 100644 --- a/tests/test_xmlrpc/test_dns_plugin.py +++ b/tests/test_xmlrpc/test_dns_plugin.py @@ -27,7 +27,7 @@ from tests.test_xmlrpc import objectclasses from xmlrpc_test import Declarative, fuzzy_digits, fuzzy_uuid dnszone1 = u'dnszone.test' -dnszone1_dn = DN(('idnsname',dnszone1),('cn','dns'),api.env.basedn) +dnszone1_dn = DN(('idnsname',dnszone1), api.env.container_dns, api.env.basedn) dnszone1_mname = u'ns1.%s.' % dnszone1 dnszone1_mname_dn = DN(('idnsname','ns1'), dnszone1_dn) dnszone1_rname = u'root.%s.' % dnszone1 @@ -35,12 +35,15 @@ dnszone1_permission = u'Manage DNS zone %s' % dnszone1 dnszone1_permission_dn = DN(('cn',dnszone1_permission), api.env.container_permission,api.env.basedn) dnszone2 = u'dnszone2.test' -dnszone2_dn = DN(('idnsname',dnszone2),('cn','dns'),api.env.basedn) +dnszone2_dn = DN(('idnsname', dnszone2), api.env.container_dns, api.env.basedn) dnszone2_mname = u'ns1.%s.' % dnszone2 dnszone2_rname = u'root.%s.' % dnszone2 revdnszone1 = u'15.142.80.in-addr.arpa.' revdnszone1_ip = u'80.142.15.0/24' -revdnszone1_dn = DN(('idnsname',revdnszone1),('cn','dns'),api.env.basedn) +revdnszone1_dn = DN(('idnsname', revdnszone1), api.env.container_dns, api.env.basedn) +revdnszone2 = u'16.142.80.in-addr.arpa.' +revdnszone2_ip = u'80.142.16.0' +revdnszone2_dn = DN(('idnsname',revdnszone2), api.env.container_dns, api.env.basedn) dnsres1 = u'testdnsres' dnsres1_dn = DN(('idnsname',dnsres1), dnszone1_dn) dnsres1_renamed = u'testdnsres-renamed' @@ -72,11 +75,8 @@ class test_dns(Declarative): pass cleanup_commands = [ - ('dnszone_del', [dnszone1], {}), - ('dnsrecord_del', [dnszone1, dnsres1], {'del_all' : True}), - ('dnsrecord_del', [dnszone1, dnsres1_renamed], {'del_all' : True}), - ('dnszone_del', [dnszone2], {}), - ('dnszone_del', [revdnszone1], {}), + ('dnszone_del', [dnszone1, dnszone2, revdnszone1, revdnszone2], + {'continue': True}), ('dnsconfig_mod', [], {'idnsforwarders' : None, 'idnsforwardpolicy' : None, 'idnsallowsyncptr' : None, @@ -949,7 +949,7 @@ class test_dns(Declarative): ), dict( - desc='Create reverse from IP %s zone using name_from_ip option' % revdnszone1_ip, + desc='Create reverse zone from IP/netmask %r using name_from_ip option' % revdnszone1_ip, command=( 'dnszone_add', [], { 'name_from_ip': revdnszone1_ip, @@ -985,6 +985,42 @@ class test_dns(Declarative): dict( + desc='Create reverse zone from IP %r using name_from_ip option' % revdnszone2_ip, + command=( + 'dnszone_add', [], { + 'name_from_ip': revdnszone2_ip, + 'idnssoamname': dnszone1_mname, + 'idnssoarname': dnszone1_rname, + 'ip_address' : u'1.2.3.4', + } + ), + expected={ + 'value': revdnszone2, + 'summary': None, + 'result': { + 'dn': revdnszone2_dn, + 'idnsname': [revdnszone2], + 'idnszoneactive': [u'TRUE'], + 'idnssoamname': [dnszone1_mname], + 'nsrecord': [dnszone1_mname], + 'idnssoarname': [dnszone1_rname], + 'idnssoaserial': [fuzzy_digits], + 'idnssoarefresh': [fuzzy_digits], + 'idnssoaretry': [fuzzy_digits], + 'idnssoaexpire': [fuzzy_digits], + 'idnssoaminimum': [fuzzy_digits], + 'idnsallowdynupdate': [u'FALSE'], + 'idnsupdatepolicy': [u'grant %(realm)s krb5-subdomain %(zone)s PTR;' + % dict(realm=api.env.realm, zone=revdnszone2)], + 'idnsallowtransfer': [u'none;'], + 'idnsallowquery': [u'any;'], + 'objectclass': objectclasses.dnszone, + }, + }, + ), + + + dict( desc='Try to add invalid PTR %r to %r using dnsrecord_add' % (dnsrev1, revdnszone1), command=('dnsrecord_add', [revdnszone1, dnsrev1], {'ptrrecord': u'-.example.com' }), expected=errors.ValidationError(name='hostname', -- 1.7.11.4
_______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
