[Freeipa-devel] [PATCH] 378-380 Improved CNAME and DNAME validation
Martin Kosek
mkosek at redhat.com
Wed Mar 6 08:32:21 UTC 2013
On 03/05/2013 01:04 PM, Petr Spacek wrote:
> 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 :-)
>
Thanks, fixed version attached.
Martin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mkosek-378-2-change-cname-and-dname-attributes-to-single-valued.patch
Type: text/x-patch
Size: 5397 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130306/e9feeed4/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mkosek-379-2-improve-cname-record-validation.patch
Type: text/x-patch
Size: 7427 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130306/e9feeed4/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mkosek-380-2-improve-dname-record-validation.patch
Type: text/x-patch
Size: 7917 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130306/e9feeed4/attachment-0002.bin>
More information about the Freeipa-devel
mailing list