On 11.4.2011 12:48, Jan Cholasta wrote:
On 8.4.2011 16:26, Rob Crittenden wrote:
Jan Cholasta wrote:
On 29.3.2011 22:15, Rob Crittenden wrote:
Jan Cholasta wrote:
Sorry, forgot to attach the patch.


Is this why you have some blind excepts?

installutils._IPAddressWithPrefix('192.168.0.1/33')
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "ipaserver/install/installutils.py", line 167, in __init__
net = netaddr.IPNetwork(addr)
File "/usr/lib/python2.7/site-packages/netaddr/ip/__init__.py", line
919, in __init__
implicit_prefix, flags)
File "/usr/lib/python2.7/site-packages/netaddr/ip/__init__.py", line
782, in parse_ip_network
value = ip._value
UnboundLocalError: local variable 'ip' referenced before assignment

We should get an upstream bug filed on python-netaddr about this.

https://github.com/drkjam/netaddr/issues/closed#issue/5
https://github.com/drkjam/netaddr/issues/closed#issue/6
https://github.com/drkjam/netaddr/issues/closed#issue/8

Apparently it's already been fixed for the next release.

IMHO it's not much of an issue for us, because the exception gets caught
in parse_ip_address and that's currently the only place where
_IPAddressWithPrefix is used.


Shoudl parse_ip_address() raise an exception on bad data rather than
returning 0.0.0.0?

I've been down that road and it would need a rewrite of the fragile IP
address handling logic of ipa-server-install, which is something I'd
rather avoid.


>>> installutils.parse_ip_address('355.555.3.3')
_IPAddressWithPrefix('0.0.0.0')

or

>>> installutils.parse_ip_address('192.168.0.1/55')
_IPAddressWithPrefix('0.0.0.0')

Should it disallow net addresses like 192.168.0.0?

If you mean network and broadcast addresses, it probably should. It
might be a good idea to disallow localhost, multicast and/or link-local
addresses too.

Are you going to resubmit the patch with these added or should we open a
separate ticket?

I'm going to resubmit it. Right now it disallows loopback, IANA
reserved, link-local, network, multicast and broadcast IP addresses.
Does it make sense to also allow only IP addresses attached to one of
the local network interfaces? Perhaps it would be sufficient just to
print a warning. Or should we not care about that at all?

Sending the updated patch.



rob




--
Jan Cholasta
>From 764ba2c6906749761ab6f2ffdc96d442ec5f4fa6 Mon Sep 17 00:00:00 2001
From: Jan Cholasta <jchol...@redhat.com>
Date: Wed, 13 Apr 2011 13:12:03 +0200
Subject: [PATCH] Add ability to specify netmask/prefix length with IP addresses during install for proper DNS reverse zone setup.

ticket 910
---
 install/tools/ipa-dns-install     |    8 ++-
 install/tools/ipa-replica-install |    4 +-
 install/tools/ipa-replica-prepare |   13 ++--
 install/tools/ipa-server-install  |   32 ++++-----
 ipaserver/install/bindinstance.py |   49 +++++++++-----
 ipaserver/install/installutils.py |  135 +++++++++++++++++++++++++++++++-----
 tests/test_install/test_utils.py  |   66 ++++++++++++++++++
 7 files changed, 244 insertions(+), 63 deletions(-)
 create mode 100644 tests/test_install/test_utils.py

diff --git a/install/tools/ipa-dns-install b/install/tools/ipa-dns-install
index aac85bf..69b05bc 100755
--- a/install/tools/ipa-dns-install
+++ b/install/tools/ipa-dns-install
@@ -131,11 +131,13 @@ def main():
         ip_address = options.ip_address
     else:
         ip_address = resolve_host(api.env.host)
-    if not ip_address or not verify_ip_address(ip_address):
+    ip = parse_ip_address(ip_address)
+    if not verify_ip_address(ip):
         if options.unattended:
             sys.exit("Unable to resolve IP address for host name")
         else:
-            ip_address = read_ip_address(api.env.host, fstore)
+            ip = read_ip_address(api.env.host, fstore)
+    ip_address = str(ip)
     logging.debug("will use ip_address: %s\n", ip_address)
 
     if options.no_forwarders:
@@ -180,7 +182,7 @@ def main():
         create_reverse = not options.no_reverse
     elif not options.no_reverse:
         create_reverse = bindinstance.create_reverse()
-    bind.setup(api.env.host, ip_address, api.env.realm, api.env.domain, dns_forwarders, conf_ntp, create_reverse, zonemgr=options.zonemgr)
+    bind.setup(api.env.host, ip_address, ip.prefixlen, api.env.realm, api.env.domain, dns_forwarders, conf_ntp, create_reverse, zonemgr=options.zonemgr)
 
     if bind.dm_password:
         api.Backend.ldap2.connect(bind_dn="cn=Directory Manager", bind_pw=bind.dm_password)
diff --git a/install/tools/ipa-replica-install b/install/tools/ipa-replica-install
index 999b5ee..b6b1407 100755
--- a/install/tools/ipa-replica-install
+++ b/install/tools/ipa-replica-install
@@ -285,6 +285,8 @@ def install_bind(config, options):
     ip_address = resolve_host(config.host_name)
     if not ip_address:
         sys.exit("Unable to resolve IP address for host name")
+    ip = installutils.parse_ip_address(ip_address)
+    ip_address = str(ip)
 
     create_reverse = True
     if options.unattended:
@@ -294,7 +296,7 @@ def install_bind(config, options):
         # In interactive mode, if the flag was not explicitly specified, ask the user
         create_reverse = bindinstance.create_reverse()
 
-    bind.setup(config.host_name, ip_address, config.realm_name,
+    bind.setup(config.host_name, ip_address, ip.prefixlen, config.realm_name,
                config.domain_name, forwarders, options.conf_ntp, create_reverse)
     bind.create_instance()
 
diff --git a/install/tools/ipa-replica-prepare b/install/tools/ipa-replica-prepare
index e912235..3b7a240 100755
--- a/install/tools/ipa-replica-prepare
+++ b/install/tools/ipa-replica-prepare
@@ -28,7 +28,7 @@ from optparse import OptionParser
 
 from ipapython import ipautil
 from ipaserver.install import bindinstance, dsinstance, installutils, certs
-from ipaserver.install.bindinstance import add_zone, add_reverse_zone, add_rr, add_ptr_rr
+from ipaserver.install.bindinstance import add_zone, add_reverse_zone, add_fwd_rr, add_ptr_rr
 from ipaserver.install.replication import check_replication_plugin, enable_replication_version_checking
 from ipaserver.plugins.ldap2 import ldap2
 from ipapython import version
@@ -425,10 +425,13 @@ def main():
         name = domain.pop(0)
         domain = ".".join(domain)
 
-        zone = add_zone(domain, nsaddr=options.ip_address)
-        add_rr(zone, name, "A", options.ip_address)
-        add_reverse_zone(options.ip_address)
-        add_ptr_rr(options.ip_address, replica_fqdn)
+        ip = installutils.parse_ip_address(options.ip_address, match_local=False)
+        ip_address = str(ip)
+
+        zone = add_zone(domain, nsaddr=ip_address)
+        add_fwd_rr(zone, name, ip_address)
+        add_reverse_zone(ip_address, ip.prefixlen)
+        add_ptr_rr(ip_address, ip.prefixlen, replica_fqdn)
 
 try:
     if not os.geteuid()==0:
diff --git a/install/tools/ipa-server-install b/install/tools/ipa-server-install
index d083058..bde5ff7 100755
--- a/install/tools/ipa-server-install
+++ b/install/tools/ipa-server-install
@@ -603,37 +603,33 @@ def main():
     domain_name = domain_name.lower()
 
     # Check we have a public IP that is associated with the hostname
-    ip = resolve_host(host_name)
-    if ip is None:
-        if options.ip_address:
-            ip = options.ip_address
-    if ip is None and options.unattended:
+    ip = parse_ip_address(resolve_host(host_name))
+    ip_opt = parse_ip_address(options.ip_address)
+    if not ip and ip_opt:
+        ip = ip_opt
+    if not ip and options.unattended:
         sys.exit("Unable to resolve IP address for host name")
 
     if not verify_ip_address(ip):
-        ip = ""
+        ip = None
         if options.unattended:
             sys.exit(1)
 
-    if options.ip_address and options.ip_address != ip:
-        if options.setup_dns:
-            if not verify_ip_address(options.ip_address):
-                return 1
-            ip = options.ip_address
-        else:
+    if ip_opt:
+        if ip_opt != ip and not options.setup_dns:
             print >>sys.stderr, "Error: the hostname resolves to an IP address that is different"
             print >>sys.stderr, "from the one provided on the command line.  Please fix your DNS"
             print >>sys.stderr, "or /etc/hosts file and restart the installation."
             return 1
 
-    if options.unattended:
-        if not ip:
-            sys.exit("Unable to resolve IP address")
+        ip = ip_opt
+        if not verify_ip_address(ip):
+            return 1
 
     if not ip:
         ip = read_ip_address(host_name, fstore)
-        logging.debug("read ip_address: %s\n" % ip)
-    ip_address = ip
+        logging.debug("read ip_address: %s\n" % str(ip))
+    ip_address = str(ip)
 
     print "The IPA Master Server will be configured with"
     print "Hostname:    " + host_name
@@ -900,7 +896,7 @@ def main():
             # In interactive mode, if the flag was not explicitly specified, ask the user
             create_reverse = bindinstance.create_reverse()
 
-    bind.setup(host_name, ip_address, realm_name, domain_name, dns_forwarders, options.conf_ntp, create_reverse, zonemgr=options.zonemgr)
+    bind.setup(host_name, ip_address, ip.prefixlen, realm_name, domain_name, dns_forwarders, options.conf_ntp, create_reverse, zonemgr=options.zonemgr)
     if options.setup_dns:
         api.Backend.ldap2.connect(bind_dn="cn=Directory Manager", bind_pw=dm_password)
 
diff --git a/ipaserver/install/bindinstance.py b/ipaserver/install/bindinstance.py
index e005653..5d556e6 100644
--- a/ipaserver/install/bindinstance.py
+++ b/ipaserver/install/bindinstance.py
@@ -98,16 +98,20 @@ def dns_container_exists(fqdn, suffix):
 
     return ret
 
-def get_reverse_zone(ip_address_str):
+def get_reverse_zone(ip_address_str, ip_prefix_len):
     ip = netaddr.IPAddress(ip_address_str)
+    items = ip.reverse_dns.split('.')
+
     if ip.version == 4:
-        name, dot, zone = ip.reverse_dns.partition('.')
+        pos = 4 - ip_prefix_len / 8
     elif ip.version == 6:
-        name = '.'.join(ip.reverse_dns.split('.')[:8])
-        zone = '.'.join(ip.reverse_dns.split('.')[8:])
+        pos = 32 - ip_prefix_len / 4
     else:
         raise ValueError('Bad address format?')
 
+    name = '.'.join(items[:pos])
+    zone = '.'.join(items[pos:])
+
     return unicode(zone), unicode(name)
 
 def dns_zone_exists(name):
@@ -138,8 +142,8 @@ def add_zone(name, zonemgr=None, dns_backup=None, nsaddr=None, update_policy=Non
     add_rr(name, "@", "NS", api.env.host+'.', dns_backup, force=True)
     return name
 
-def add_reverse_zone(ip_address, update_policy=None, dns_backup=None):
-    zone, name = get_reverse_zone(ip_address)
+def add_reverse_zone(ip_address, ip_prefix_len, update_policy=None, dns_backup=None):
+    zone, name = get_reverse_zone(ip_address, ip_prefix_len)
     if not update_policy:
         update_policy = "grant %s krb5-subdomain %s. PTR;" % (api.env.realm, zone)
     try:
@@ -172,8 +176,8 @@ def add_fwd_rr(zone, host, ip_address):
     elif addr.version == 6:
         add_rr(zone, host, "AAAA", ip_address)
 
-def add_ptr_rr(ip_address, fqdn, dns_backup=None):
-    zone, name = get_reverse_zone(ip_address)
+def add_ptr_rr(ip_address, ip_prefix_len, fqdn, dns_backup=None):
+    zone, name = get_reverse_zone(ip_address, ip_prefix_len)
     add_rr(zone, name, "PTR", fqdn+".", dns_backup)
 
 def del_rr(zone, name, type, rdata):
@@ -249,6 +253,7 @@ class BindInstance(service.Service):
         self.domain = None
         self.host = None
         self.ip_address = None
+        self.ip_prefix_len = None
         self.realm = None
         self.forwarders = None
         self.sub_dict = None
@@ -259,10 +264,11 @@ class BindInstance(service.Service):
         else:
             self.fstore = sysrestore.FileStore('/var/lib/ipa/sysrestore')
 
-    def setup(self, fqdn, ip_address, realm_name, domain_name, forwarders, ntp, create_reverse, named_user="named", zonemgr=None):
+    def setup(self, fqdn, ip_address, ip_prefix_len, realm_name, domain_name, forwarders, ntp, create_reverse, named_user="named", zonemgr=None):
         self.named_user = named_user
         self.fqdn = fqdn
         self.ip_address = ip_address
+        self.ip_prefix_len = ip_prefix_len
         self.realm = realm_name
         self.domain = domain_name
         self.forwarders = forwarders
@@ -386,11 +392,11 @@ class BindInstance(service.Service):
 
         # Add forward and reverse records to self
         add_fwd_rr(zone, self.host, self.ip_address)
-        if dns_zone_exists(get_reverse_zone(self.ip_address)[0]):
-            add_ptr_rr(self.ip_address, self.fqdn)
+        if dns_zone_exists(get_reverse_zone(self.ip_address, self.ip_prefix_len)[0]):
+            add_ptr_rr(self.ip_address, self.ip_prefix_len, self.fqdn)
 
     def __setup_reverse_zone(self):
-        add_reverse_zone(self.ip_address, dns_backup=self.dns_backup)
+        add_reverse_zone(self.ip_address, self.ip_prefix_len, dns_backup=self.dns_backup)
 
     def __setup_principal(self):
         dns_principal = "DNS/" + self.fqdn + "@" + self.realm
@@ -477,14 +483,23 @@ class BindInstance(service.Service):
         for (record, type, rdata) in resource_records:
             del_rr(zone, record, type, rdata)
 
-        areclist = get_rr(zone, host, "A")
-        if len(areclist) != 0:
-            for rdata in areclist:
-                del_rr(zone, host, "A", rdata)
+        areclist = [("A", x) for x in get_rr(zone, host, "A")] + [("AAAA", x) for x in get_rr(zone, host, "AAAA")]
+        for (type, rdata) in areclist:
+            del_rr(zone, host, type, rdata)
 
-                rzone, record = get_reverse_zone(rdata)
+            ip = netaddr.IPAddress(rdata)
+            if ip.version == 4:
+                prefixrange = reversed(range(0, 32, 8))
+            elif ip.version == 6:
+                prefixrange = reversed(range(0, 128, 4))
+            else:
+                continue
+
+            for prefix in prefixrange:
+                rzone, record = get_reverse_zone(rdata, prefix)
                 if dns_zone_exists(rzone):
                     del_rr(rzone, record, "PTR", fqdn+".")
+                    break
 
 
     def uninstall(self):
diff --git a/ipaserver/install/installutils.py b/ipaserver/install/installutils.py
index 3868c4d..85c3337 100644
--- a/ipaserver/install/installutils.py
+++ b/ipaserver/install/installutils.py
@@ -148,17 +148,117 @@ def verify_fqdn(host_name,no_host_dns=False):
     else:
         print "Warning: Hostname (%s) not found in DNS" % host_name
 
-def verify_ip_address(ip):
-    is_ok = True
+def find_local_ip_address(addr):
     try:
-        socket.inet_pton(socket.AF_INET, ip)
-    except:
+        ip = netaddr.IPAddress(addr)
+    except netaddr.core.AddrFormatError, ValueError:
+        return None
+
+    if ip.version == 4:
+        family = 'inet'
+    elif ip.version == 6:
+        family = 'inet6'
+    else:
+        return None
+
+    ipresult = ipautil.run(['/sbin/ip', '-family', family, '-oneline', 'address', 'show'])
+    lines = ipresult[0].split('\n')
+    for line in lines:
+        fields = line.split()
+        if len(fields) < 4:
+            continue
+
+        net = netaddr.IPNetwork(fields[3])
+        if net.ip == ip:
+            return net
+
+    return None
+
+class _ParsedIPAddress(netaddr.IPAddress):
+    def __init__(self, net):
+        super(_ParsedIPAddress, self).__init__(net.ip)
+
+        self.prefixlen = net.prefixlen
+        self.network = net.network
+        self.broadcast = net.broadcast
+
+    def is_reserved(self):
+        if self in netaddr.ip.IPV4_6TO4:
+            return True
+        return super(_ParsedIPAddress, self).is_reserved()
+    
+    def is_network(self):
+        return self == self.network
+    
+    def is_broadcast(self):
+        return self == self.broadcast and self.version == 4
+
+def parse_ip_address(addr, match_local=True):
+    if addr is None:
+        return None
+    if isinstance(addr, _ParsedIPAddress):
+        return addr
+
+    if isinstance(addr, netaddr.IPNetwork):
+        net = addr
+    else:
         try:
-            socket.inet_pton(socket.AF_INET6, ip)
+            ip = netaddr.IPAddress(addr)
+            net = None
+
+            if match_local:
+                net = find_local_ip_address(ip)
+
+            if net is None:
+                if ip.version == 4:
+                    net = netaddr.IPNetwork(netaddr.cidr_abbrev_to_verbose(str(ip)))
+                elif ip.version == 6:
+                    net = netaddr.IPNetwork(ip)
+                    net.prefixlen = 64
+        except ValueError:
+            try:
+                net = netaddr.IPNetwork(addr)
+            except:
+                return None
         except:
-            print "Unable to verify IP address"
-            is_ok = False
-    return is_ok
+            return None
+
+    if net is None or net.version not in (4, 6):
+        return None
+
+    return _ParsedIPAddress(net)
+
+def verify_ip_address(addr, check_local=True):
+    ip = parse_ip_address(addr, match_local=check_local)
+
+    if ip is None:
+        print "Unable to verify IP address"
+        return False
+    elif ip.is_loopback():
+        print "Cannot use localhost IP address"
+        return False
+    elif ip.is_reserved():
+        print "Cannot use IANA reserved IP address"
+        return False
+    elif ip.is_link_local():
+        print "Cannot use link-local IP address"
+        return False
+    elif ip.is_network():
+        print "Cannot use IP network address"
+        return False
+    elif ip.is_multicast():
+        print "Cannot use multicast IP address"
+        return False
+    elif ip.is_broadcast():
+        print "Cannot use broadcast IP address"
+        return False
+
+    if check_local:
+        local = find_local_ip_address(ip)
+        if local is None or local.prefixlen != ip.prefixlen:
+            print "Warning: No network interface matches the provided IP address and netmask"
+
+    return True
 
 def record_in_hosts(ip, host_name, file="/etc/hosts"):
     hosts = open(file, 'r').readlines()
@@ -192,18 +292,16 @@ def read_ip_address(host_name, fstore):
     while True:
         ip = ipautil.user_input("Please provide the IP address to be used for this host name", allow_empty = False)
 
-        if ip == "127.0.0.1" or ip == "::1":
-            print "The IPA Server can't use localhost as a valid IP"
-            continue
-
-        if verify_ip_address(ip):
+        ip_parsed = parse_ip_address(ip)
+        if verify_ip_address(ip_parsed):
             break
 
+    ip = str(ip_parsed)
     print "Adding ["+ip+" "+host_name+"] to your /etc/hosts file"
     fstore.backup_file("/etc/hosts")
     add_record_to_hosts(ip, host_name)
 
-    return ip
+    return ip_parsed
 
 def read_dns_forwarders():
     addrs = []
@@ -215,15 +313,14 @@ def read_dns_forwarders():
                                     allow_empty=True)
             if not ip:
                 break
-            if ip == "127.0.0.1" or ip == "::1":
-                print "You cannot use localhost as a DNS forwarder"
-                continue
-            if not verify_ip_address(ip):
+
+            ip_parsed = parse_ip_address(ip, match_local=False)
+            if not verify_ip_address(ip_parsed, check_local=False):
                 print "DNS forwarder %s not added" % ip
                 continue
 
             print "DNS forwarder %s added" % ip
-            addrs.append(ip)
+            addrs.append(str(ip_parsed))
 
     if not addrs:
         print "No DNS forwarders configured"
diff --git a/tests/test_install/test_utils.py b/tests/test_install/test_utils.py
new file mode 100644
index 0000000..33ded86
--- /dev/null
+++ b/tests/test_install/test_utils.py
@@ -0,0 +1,66 @@
+# Authors:
+#   Jan Cholasta <jchol...@redhat.com>
+#
+# Copyright (C) 2011  Red Hat
+# see file 'COPYING' for use and warranty information
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+"""
+Test the `ipaserver/install/installutils.py` module.
+"""
+
+import nose
+
+from ipaserver.install import installutils
+
+class CheckIPAddress:
+    def __init__(self, addr):
+        self.description = "Test IP address parsing and verification (%s)" % addr
+
+    def __call__(self, addr, words=None, prefixlen=None, verified=None):
+        ip = installutils.parse_ip_address(addr, match_local=False)
+        if ip:
+            assert ip.words == words and ip.prefixlen == prefixlen
+            assert not verified or installutils.verify_ip_address(ip, check_local=False)
+        else:
+            assert words is None and prefixlen is None and verified is None
+
+def test_ip_address():
+    addrs = [
+        ('10.11.12.13',     (10, 11, 12, 13),   8,  True),
+        ('10.11.12.13/14',  (10, 11, 12, 13),   14, True),
+        ('127.0.0.1',       (127, 0, 0, 1),     8,  False),
+        ('241.1.2.3',       (241, 1, 2, 3),     32, False),
+        ('169.254.1.2',     (169, 254, 1, 2),   16, False),
+        ('10.11.12.0/24',   (10, 11, 12, 0),    24, False),
+        ('224.5.6.7',       (224, 5, 6, 7),     4,  False),
+        ('10.11.12.255/24', (10, 11, 12, 255),  24, False),
+        ('10.11.12.1337',),
+        ('10.11.12.13/33',),
+
+        ('2001::1',     (0x2001, 0, 0, 0, 0, 0, 0, 1), 64,  True),
+        ('2001::1/72',  (0x2001, 0, 0, 0, 0, 0, 0, 1), 72,  True),
+        ('::1',         (0, 0, 0, 0, 0, 0, 0, 1),      64,  False),
+        ('6789::1',     (0x6789, 0, 0, 0, 0, 0, 0, 1), 64,  False),
+        ('fe89::1',     (0xfe89, 0, 0, 0, 0, 0, 0, 1), 64,  False),
+        ('2001::/64',   (0x2001, 0, 0, 0, 0, 0, 0, 0), 64,  False),
+        ('ff01::1',     (0xff01, 0, 0, 0, 0, 0, 0, 1), 64,  False),
+        ('2001::1beef',),
+        ('2001::1/129',),
+
+        ('junk',)
+    ]
+
+    for addr in addrs:
+        yield (CheckIPAddress(addr[0]),) + addr
-- 
1.7.4.2

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to