Improve CNAME record validation

Refactor DNS RR conflict validator so that it is better extensible in
the future. Also check that there is only one CNAME defined for
a DNS record.

PTR+CNAME record combination is no longer allowed as we found out it
does not make sense to have this combination.

https://fedorahosted.org/freeipa/ticket/3450

2 files changed, 41 insertions, 40 deletions

diff --git a/ipalib/plugins/dns.py b/ipalib/plugins/dns.py
index dabab8405..7d9956504 100644
--- a/ipalib/plugins/dns.py
+++ b/ipalib/plugins/dns.py
@@ -2269,23 +2269,34 @@ class dnsrecord(LDAPObject):
 
     def check_record_type_collisions(self, old_entry, entry_attrs):
         # Test that only allowed combination of record types was created
-        attrs = set(attr for attr in entry_attrs.keys() if attr in _record_attributes
-                        and entry_attrs[attr])
-        attrs.update(attr for attr in old_entry.keys() if attr not in entry_attrs)
+        rrattrs = {}
+        if old_entry is not None:
+            old_rrattrs = dict((key, value) for key, value in old_entry.iteritems()
+                            if key in self.params and
+                            isinstance(self.params[key], DNSRecord))
+            rrattrs.update(old_rrattrs)
+        new_rrattrs = dict((key, value) for key, value in entry_attrs.iteritems()
+                        if key in self.params and
+                        isinstance(self.params[key], DNSRecord))
+        rrattrs.update(new_rrattrs)
+
+        # CNAME record validation
         try:
-            attrs.remove('cnamerecord')
+            cnames = rrattrs['cnamerecord']
         except KeyError:
-            rec_has_cname = False
+            pass
         else:
-            rec_has_cname = True
-        # CNAME and PTR record combination is allowed
-        attrs.discard('ptrrecord')
-        rec_has_other_types = True if attrs else False
-
-        if rec_has_cname and rec_has_other_types:
-            raise errors.ValidationError(name='cnamerecord',
-                      error=_('CNAME record is not allowed to coexist with any other '
-                              'records except PTR'))
+            if cnames is not None:
+                if len(cnames) > 1:
+                    raise errors.ValidationError(name='cnamerecord',
+                        error=_('only one CNAME record is allowed per name '
+                                '(RFC 2136, section 1.1.5)'))
+                if any(rrvalue is not None
+                       and rrattr != 'cnamerecord'
+                       for rrattr, rrvalue in rrattrs.iteritems()):
+                    raise errors.ValidationError(name='cnamerecord',
+                          error=_('CNAME record is not allowed to coexist '
+                                  'with any other record (RFC 1034, section 3.6.2)'))
 
 api.register(dnsrecord)
 
@@ -2435,7 +2446,7 @@ class dnsrecord_add(LDAPCreate):
         try:
             (dn_, old_entry) = ldap.get_entry(dn, _record_attributes)
         except errors.NotFound:
-            pass
+            old_entry = None
         else:
             for attr in entry_attrs.keys():
                 if attr not in _record_attributes:
@@ -2448,7 +2459,7 @@ class dnsrecord_add(LDAPCreate):
                     vals = list(entry_attrs[attr])
                 entry_attrs[attr] = list(set(old_entry.get(attr, []) + vals))
 
-            self.obj.check_record_type_collisions(old_entry, entry_attrs)
+        self.obj.check_record_type_collisions(old_entry, entry_attrs)
         return dn
 
     def exc_callback(self, keys, options, exc, call_func, *call_args, **call_kwargs):
diff --git a/tests/test_xmlrpc/test_dns_plugin.py b/tests/test_xmlrpc/test_dns_plugin.py
index 945bca384..2e7d5466a 100644
--- a/tests/test_xmlrpc/test_dns_plugin.py
+++ b/tests/test_xmlrpc/test_dns_plugin.py
@@ -773,7 +773,8 @@ class test_dns(Declarative):
             desc='Try to add CNAME record to %r using dnsrecord_add' % (dnsres1),
             command=('dnsrecord_add', [dnszone1, dnsres1], {'cnamerecord': u'foo-1.example.com.'}),
             expected=errors.ValidationError(name='cnamerecord',
-                error=u'CNAME record is not allowed to coexist with any other records except PTR'),
+                error=u'CNAME record is not allowed to coexist with any other '
+                      u'record (RFC 1034, section 3.6.2)'),
         ),
 
         dict(
@@ -785,6 +786,14 @@ class test_dns(Declarative):
         ),
 
         dict(
+            desc='Try to add multiple CNAME record %r using dnsrecord_add' % (dnsrescname),
+            command=('dnsrecord_add', [dnszone1, dnsrescname], {'cnamerecord':
+                [u'1.example.com.', u'2.example.com.']}),
+            expected=errors.ValidationError(name='cnamerecord',
+                error=u'only one CNAME record is allowed per name (RFC 2136, section 1.1.5)'),
+        ),
+
+        dict(
             desc='Add CNAME record to %r using dnsrecord_add' % (dnsrescname),
             command=('dnsrecord_add', [dnszone1, dnsrescname], {'cnamerecord': u'foo-1.example.com.'}),
             expected={
@@ -803,14 +812,16 @@ class test_dns(Declarative):
             desc='Try to add other record to CNAME record %r using dnsrecord_add' % (dnsrescname),
             command=('dnsrecord_add', [dnszone1, dnsrescname], {'arecord': u'10.0.0.1'}),
             expected=errors.ValidationError(name='cnamerecord',
-                error=u'CNAME record is not allowed to coexist with any other records except PTR'),
+                error=u'CNAME record is not allowed to coexist with any other '
+                      u'record (RFC 1034, section 3.6.2)'),
         ),
 
         dict(
             desc='Try to add other record to CNAME record %r using dnsrecord_mod' % (dnsrescname),
             command=('dnsrecord_mod', [dnszone1, dnsrescname], {'arecord': u'10.0.0.1'}),
             expected=errors.ValidationError(name='cnamerecord',
-                error=u'CNAME record is not allowed to coexist with any other records except PTR'),
+                error=u'CNAME record is not allowed to coexist with any other '
+                      u'record (RFC 1034, section 3.6.2)'),
         ),
 
         dict(
@@ -1063,22 +1074,6 @@ class test_dns(Declarative):
         ),
 
         dict(
-            desc='Test that CNAME/PTR record type combination in record %r is allowed' % (dnsrev1),
-            command=('dnsrecord_add', [revdnszone1, dnsrev1], {'cnamerecord': u'foo-1.example.com.' }),
-            expected={
-                'value': dnsrev1,
-                'summary': None,
-                'result': {
-                    'objectclass': objectclasses.dnsrecord,
-                    'dn': dnsrev1_dn,
-                    'idnsname': [dnsrev1],
-                    'ptrrecord': [u'foo-1.example.com.'],
-                    'cnamerecord': [u'foo-1.example.com.'],
-                },
-            },
-        ),
-
-        dict(
             desc='Show record %r in zone %r with --structured and --all options'\
                     % (dnsrev1, revdnszone1),
             command=('dnsrecord_show', [revdnszone1, dnsrev1],
@@ -1096,11 +1091,6 @@ class test_dns(Declarative):
                             'dnsdata': u'foo-1.example.com.',
                             'ptr_part_hostname': u'foo-1.example.com.'
                         },
-                        {
-                            'dnstype': u'CNAME',
-                            'dnsdata': u'foo-1.example.com.',
-                            'cname_part_hostname': u'foo-1.example.com.'
-                        }
                     ],
                 },
             },