On 21.4.2011 09:36, Jan Cholasta wrote:
On 20.4.2011 22:08, Rob Crittenden wrote:
Jan Cholasta wrote:
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.

This looks ok, just one question. Should we add a dependency on the
iproute package because of the /sbin/ip package?

Yes, we should.


rob



Split the patch to 3 smaller pieces:

Patch 18 adds the ability to parse netmasks in IP addresses passed to server install.
https://fedorahosted.org/freeipa/ticket/1212

This patch requires patch 18 and fixes DNS reverse zone setup to honor the netmask.
https://fedorahosted.org/freeipa/ticket/910

Patch 19 requires patch 18 and adds stricter checking of IP addresses.
https://fedorahosted.org/freeipa/ticket/1213

Honza

--
Jan Cholasta
>From 7ece02d6bce0ba80075cf7b570bbef22b74d381e Mon Sep 17 00:00:00 2001
From: Jan Cholasta <jchol...@redhat.com>
Date: Tue, 10 May 2011 19:59:56 +0200
Subject: [PATCH] Honor netmask in DNS reverse zone setup.

ticket 910
---
 install/tools/ipa-dns-install     |    2 +-
 install/tools/ipa-replica-install |    2 +-
 install/tools/ipa-replica-prepare |    8 +++---
 install/tools/ipa-server-install  |    2 +-
 ipaserver/install/bindinstance.py |   49 ++++++++++++++++++++++++-------------
 5 files changed, 39 insertions(+), 24 deletions(-)

diff --git a/install/tools/ipa-dns-install b/install/tools/ipa-dns-install
index 256c670..69b05bc 100755
--- a/install/tools/ipa-dns-install
+++ b/install/tools/ipa-dns-install
@@ -182,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 722da37..a767519 100755
--- a/install/tools/ipa-replica-install
+++ b/install/tools/ipa-replica-install
@@ -296,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 6a4e413..faafc30 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
@@ -429,9 +429,9 @@ def main():
         ip_address = str(ip)
 
         zone = add_zone(domain, nsaddr=ip_address)
-        add_rr(zone, name, "A", ip_address)
-        add_reverse_zone(ip_address)
-        add_ptr_rr(ip_address, replica_fqdn)
+        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 c256d0e..2f73b70 100755
--- a/install/tools/ipa-server-install
+++ b/install/tools/ipa-server-install
@@ -908,7 +908,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 3208688..76435db 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
@@ -390,11 +396,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
@@ -482,16 +488,25 @@ 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+".")
                     # remove also master NS record from the reverse zone
                     del_rr(rzone, "@", "NS", fqdn+".")
+                    break
 
 
     def uninstall(self):
-- 
1.7.4.4

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

Reply via email to