On 11.5.2011 15:58, Martin Kosek wrote:
On Tue, 2011-05-10 at 20:10 +0200, Jan Cholasta wrote:
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

Thanks for splitting of the patches, it is now much clearer what is done
and where. Please fix pylint errors first before the review, there were
several of them when I applied all 3 patches:

./make-lint
ipalib/plugins/host.py:122: [E1120, remove_fwd_ptr] No value passed for 
parameter 'ip_prefix_len' in function call
ipalib/plugins/host.py:325: [E1120, host_add.pre_callback] No value passed for 
parameter 'ip_prefix_len' in function call
ipalib/plugins/host.py:384: [E1120, host_add.post_callback] No value passed for 
parameter 'ip_prefix_len' in function call

Martin


Rewrote host.py so that it doesn't use get_reverse_zone from ipaserver.bindinstance (which fixes the pylint errors).

Honza

--
Jan Cholasta
>From 23593bb03b54f043b1799e12f973ac9d9b826309 Mon Sep 17 00:00:00 2001
From: Jan Cholasta <jchol...@redhat.com>
Date: Thu, 12 May 2011 14:37:18 +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 +-
 ipalib/plugins/host.py            |   31 +++++++++++++++++++----
 ipaserver/install/bindinstance.py |   49 ++++++++++++++++++++++++-------------
 6 files changed, 64 insertions(+), 30 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/ipalib/plugins/host.py b/ipalib/plugins/host.py
index 52830de..82cb07a 100644
--- a/ipalib/plugins/host.py
+++ b/ipalib/plugins/host.py
@@ -91,8 +91,6 @@ from ipalib import _, ngettext
 from ipalib import x509
 from ipapython.ipautil import ipa_generate_password
 from ipalib.request import context
-if api.env.context in ['lite', 'server']:
-    from ipaserver.install.bindinstance import get_reverse_zone
 import base64
 import nss.nss as nss
 import netaddr
@@ -111,16 +109,37 @@ def is_forward_record(zone, str_address):
     if addr.version == 4:
         result = api.Command['dnsrecord_find'](zone, arecord=str_address)
     elif addr.version == 6:
-        result = api.Command['dnsrecord_find'](zone, aaarecord=str_address)
+        result = api.Command['dnsrecord_find'](zone, aaaarecord=str_address)
     else:
         raise ValueError('Invalid address family')
 
     return result['count'] > 0
 
+def find_reverse_zone(ipaddr):
+    ip = netaddr.IPAddress(ipaddr)
+    revdns = unicode(ip.reverse_dns)
+
+    revzone = u''
+
+    result = api.Command['dnszone_find']()['result']
+    for zone in result:
+        zonename = zone['idnsname'][0]
+        if revdns.endswith(zonename) and len(zonename) > len(revzone):
+            revzone = zonename
+
+    if len(revzone) == 0:
+        raise errors.NotFound(
+            reason=_('DNS reverse zone for IP address %(addr) not found') % dict(addr=ipaddr)
+        )
+
+    revname = revdns[:-len(revzone)-1]
+
+    return revzone, revname
+
 def remove_fwd_ptr(ipaddr, host, domain, recordtype):
     api.log.debug('deleting ipaddr %s' % ipaddr)
-    revzone, revname = get_reverse_zone(ipaddr)
     try:
+        revzone, revname = find_reverse_zone(ipaddr)
         delkw = { 'ptrrecord' : "%s.%s." % (host, domain) }
         api.Command['dnsrecord_del'](revzone, revname, **delkw)
     except errors.NotFound:
@@ -322,7 +341,7 @@ class host_add(LDAPCreate):
                 )
             if not options.get('no_reverse', False):
                 # we prefer lookup of the IP through the reverse zone
-                revzone, revname = get_reverse_zone(options['ip_address'])
+                revzone, revname = find_reverse_zone(options['ip_address'])
                 # Verify that our reverse zone exists
                 match = False
                 for zone in result:
@@ -381,7 +400,7 @@ class host_add(LDAPCreate):
                 add_forward_record(domain, parts[0], options['ip_address'])
 
                 if not options.get('no_reverse', False):
-                    revzone, revname = get_reverse_zone(options['ip_address'])
+                    revzone, revname = find_reverse_zone(options['ip_address'])
                     try:
                         addkw = { 'ptrrecord' : keys[-1]+'.' }
                         api.Command['dnsrecord_add'](revzone, revname, **addkw)
diff --git a/ipaserver/install/bindinstance.py b/ipaserver/install/bindinstance.py
index 3208688..817ff7f 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)
+
+            ip = netaddr.IPAddress(rdata)
+            rzone = ip.reverse_dns
+            record = ''
+
+            while True:
+                part, dot, rzone = rzone.partition('.')
+                if len(rzone) == 0:
+                    break
+                record = (record + '.' + part).lstrip('.')
 
-                rzone, record = get_reverse_zone(rdata)
                 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