[Freeipa-devel] [PATCH] 378-380 Improved CNAME and DNAME validation

Petr Spacek pspacek at redhat.com
Tue Mar 5 12:04:42 UTC 2013


Hello,

please see my comments in-line.

On 5.3.2013 12:23, Martin Kosek wrote:
> These relatively straightforward patches depend on each other, so I am sending
> them in bulk. Details can be found in commit messages.
>
> Martin
>
>
>
> freeipa-mkosek-379-improve-cname-record-validation.patch
>
>
>  From 5afde12a1a3d46a89af340e060fd1c687c7f4948 Mon Sep 17 00:00:00 2001
> From: Martin Kosek<mkosek at redhat.com>
> Date: Mon, 4 Mar 2013 15:05:49 +0100
> Subject: [PATCH 2/3] Improve CNAME record validation
>
> Refacto 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.
>
> https://fedorahosted.org/freeipa/ticket/3450
> ---
>   ipalib/plugins/dns.py                | 43 ++++++++++++++++++++++--------------
>   tests/test_xmlrpc/test_dns_plugin.py |  8 +++++++
>   2 files changed, 35 insertions(+), 16 deletions(-)
>
> diff --git a/ipalib/plugins/dns.py b/ipalib/plugins/dns.py
> index a23d1b8233eec14825ac6b43f509de51ad0ff1f7..a70d69fad181c90466467482a6ac604a166d728b 100644
> --- a/ipalib/plugins/dns.py
> +++ b/ipalib/plugins/dns.py
> @@ -2267,23 +2267,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
I remember some discussion about PTR and CNAMEs, but now I see that was silly. 
CNAME can't coexist with any other record (under same name).

> -        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 6672)'))
RFC 6672 defines DNAME, not CNAME. For CNAME please use "RFC 2136 section 
1.1.5". RFCs are huge, so section numbers are really handy!

> +                if any(rrvalue is not None
> +                       and rrattr != 'cnamerecord'
> +                       and rrattr != 'ptrrecord'
> +                       for rrattr, rrvalue in rrattrs.iteritems()):
> +                    raise errors.ValidationError(name='cnamerecord',
> +                          error=_('CNAME record is not allowed to coexist with any other '
> +                                  'records except PTR'))
The same applies here - CNAME is not allowed to co-exist with any other type.

>
>   api.register(dnsrecord)
>
> @@ -2433,7 +2444,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:
> @@ -2446,7 +2457,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 1902484949aeb0c96a0f2cda294fd3e6ae6e086f..7b2e5731395a52d26603d1d8fb2f061b7b7e1f8a 100644
> --- a/tests/test_xmlrpc/test_dns_plugin.py
> +++ b/tests/test_xmlrpc/test_dns_plugin.py
> @@ -785,6 +785,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 6672)'),
Please replace "RFC 6672" with "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={
> -- 1.8.1.4


 > freeipa-mkosek-380-improve-dname-record-validation.patch
 > +        # DNAME record validation
 > +        try:
 > +            dnames = rrattrs['dnamerecord']
 > +        except KeyError:
 > +            pass
 > +        else:
 > +            if dnames is not None:
 > +                if len(dnames) > 1:
 > +                    raise errors.ValidationError(name='dnamerecord',
 > +                        error=_('only one DNAME record is allowed per name '
 > +                                '(RFC 6672)'))
Please replace "RFC 6672" with "​RFC 6672 section 2.4".

 > +                # DNAME must not coexist with CNAME, but this is already 
checked earlier
 > +                if rrattrs.get('nsrecord') and keys[1] != _dns_zone_record:
 > +                    raise errors.ValidationError(name='dnamerecord',
 > +                          error=_('DNAME record is not allowed to coexist 
with an '
 > +                                  'NS record except when located in a zone 
root '
 > +                                  'record (RFC 6672)'))
Please replace "RFC 6672" with "​RFC 6672 section 2.3".

Sorry for nitpicking :-)

-- 
Petr^2 Spacek




More information about the Freeipa-devel mailing list