This set of 3 DNS patches fixes 2 minor issues found during DNS test day (217, 218) and there is slightly longer patch (219) which improves and consolidates hostname/domain name validation.
The testing should be pretty straightforward in case of these 3 tickets. Martin
>From 5a3bcc08575ae6eca3cc4e759c0dfb7e2fbce1f9 Mon Sep 17 00:00:00 2001 From: Martin Kosek <mko...@redhat.com> Date: Tue, 21 Feb 2012 10:37:08 +0100 Subject: [PATCH 1/3] Improve dns error message Improve AttrValueNotFound exception error message raised in the DNS module when a deleted (modified) attribute value is not found in a record. In order to be consistent with previous DNS module implementation this error message should include an attribute label instead of an attribute name. https://fedorahosted.org/freeipa/ticket/2377 --- ipalib/plugins/dns.py | 10 ++++++++-- 1 files changed, 8 insertions(+), 2 deletions(-) diff --git a/ipalib/plugins/dns.py b/ipalib/plugins/dns.py index fe32efccdf9b0e0d7d8d58dd9896205ef369ec5a..ce03f3074e6374fb133a567abeafb60938915b81 100644 --- a/ipalib/plugins/dns.py +++ b/ipalib/plugins/dns.py @@ -1761,7 +1761,8 @@ class dnsrecord_mod(LDAPUpdate): old_dnsvalue, new_parts = updated_attrs[attr] if old_dnsvalue not in old_entry.get(attr, []): - raise errors.AttrValueNotFound(attr=attr, + attr_name = unicode(param.label or param.name) + raise errors.AttrValueNotFound(attr=attr_name, value=old_dnsvalue) old_entry[attr].remove(old_dnsvalue) @@ -1906,7 +1907,12 @@ class dnsrecord_del(LDAPUpdate): try: old_entry[attr].remove(val) except (KeyError, ValueError): - raise errors.AttrValueNotFound(attr=attr, + try: + param = self.params[attr] + attr_name = unicode(param.label or param.name) + except: + attr_name = attr + raise errors.AttrValueNotFound(attr=attr_name, value=val) entry_attrs[attr] = list(set(old_entry[attr])) -- 1.7.7.6
>From eed2a61dfdc897732121855228c36725f900b3a4 Mon Sep 17 00:00:00 2001 From: Martin Kosek <mko...@redhat.com> Date: Tue, 21 Feb 2012 10:51:58 +0100 Subject: [PATCH 2/3] Improve dnsrecord-add interactive mode When an invalid record type is entered during dnsrecord-add interactive mode, user is provided with a list of allowed values (record types). However, the provided list contains also unsupported record types (APL, DHCID, etc.) and any attempt to add such records would end with error. This patch limits the list to supported record types only. https://fedorahosted.org/freeipa/ticket/2378 --- ipalib/plugins/dns.py | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-) diff --git a/ipalib/plugins/dns.py b/ipalib/plugins/dns.py index ce03f3074e6374fb133a567abeafb60938915b81..1ceb7c45b4a0e9892af1dc31f848971e9a93fca9 100644 --- a/ipalib/plugins/dns.py +++ b/ipalib/plugins/dns.py @@ -1092,6 +1092,8 @@ def __dns_record_options_iter(): yield part _dns_record_options = tuple(__dns_record_options_iter()) +_dns_supported_record_types = tuple(record.rrtype for record in _dns_records \ + if record.supported) # dictionary of valid reverse zone -> number of address components _valid_reverse_zones = { @@ -1638,7 +1640,7 @@ class dnsrecord_add(LDAPCreate): if not isinstance(param, DNSRecord): raise ValueError() except KeyError, ValueError: - all_types = u', '.join(_record_types) + all_types = u', '.join(_dns_supported_record_types) self.Backend.textui.print_plain(_(u'Invalid type. Allowed values are: %s') % all_types) continue ok = True -- 1.7.7.6
>From 79156a57a40827dc20c15e535b3c021aa056f9c1 Mon Sep 17 00:00:00 2001 From: Martin Kosek <mko...@redhat.com> Date: Tue, 21 Feb 2012 15:28:04 +0100 Subject: [PATCH 3/3] Improve hostname and domain name validation DNS plugin did not check DNS zone and DNS record validity and user was thus able to create domains like "foo bar" or other invalid DNS labels which would really confuse both user and bind-dyndb-ldap plugin. This patch at first consolidates hostname/domain name validators so that they use common functions and we don't have regular expressions and other checks defined in several places. These new cleaned validators are then used for zone/record name validation. https://fedorahosted.org/freeipa/ticket/2384 --- ipalib/plugins/dns.py | 39 ++++++++++++++++++++++------- ipalib/plugins/host.py | 19 +++----------- ipalib/util.py | 44 ++++++++++++++++++++++++++------- tests/test_xmlrpc/test_dns_plugin.py | 20 +++++++++++++++ 4 files changed, 87 insertions(+), 35 deletions(-) diff --git a/ipalib/plugins/dns.py b/ipalib/plugins/dns.py index 1ceb7c45b4a0e9892af1dc31f848971e9a93fca9..97b9d2f23eae8dc8ef830232d82ef0bb6b925653 100644 --- a/ipalib/plugins/dns.py +++ b/ipalib/plugins/dns.py @@ -28,7 +28,8 @@ from ipalib import Command from ipalib.parameters import Flag, Bool, Int, Decimal, Str, StrEnum, Any from ipalib.plugins.baseldap import * from ipalib import _, ngettext -from ipalib.util import validate_zonemgr, normalize_zonemgr, validate_hostname +from ipalib.util import (validate_zonemgr, normalize_zonemgr, + validate_hostname, validate_dns_label, validate_domain_name) from ipapython import dnsclient from ipapython.ipautil import valid_ip from ldap import explode_dn @@ -219,7 +220,7 @@ def _validate_ipnet(ugettext, ipnet): return _('invalid IP network format') return None -def _domain_name_validator(ugettext, value): +def _bind_hostname_validator(ugettext, value): try: # Allow domain name which is not fully qualified. These are supported # in bind and then translated as <non-fqdn-name>.<domain>. @@ -230,6 +231,22 @@ def _domain_name_validator(ugettext, value): return None +def _dns_record_name_validator(ugettext, value): + if value == _dns_zone_record: + return + + try: + map(lambda label:validate_dns_label(label, allow_underscore=True), \ + value.split(u'.')) + except ValueError, e: + return unicode(e) + +def _domain_name_validator(ugettext, value): + try: + validate_domain_name(value) + except ValueError, e: + return unicode(e) + def _hostname_validator(ugettext, value): try: validate_hostname(value) @@ -496,7 +513,7 @@ class AFSDBRecord(DNSRecord): maxvalue=65535, ), Str('hostname', - _domain_name_validator, + _bind_hostname_validator, label=_('Hostname'), ), ) @@ -535,7 +552,7 @@ class CNAMERecord(DNSRecord): rfc = 1035 parts = ( Str('hostname', - _domain_name_validator, + _bind_hostname_validator, label=_('Hostname'), doc=_('A hostname which this alias hostname points to'), ), @@ -556,7 +573,7 @@ class DNAMERecord(DNSRecord): rfc = 2672 parts = ( Str('target', - _domain_name_validator, + _bind_hostname_validator, label=_('Target'), ), ) @@ -635,7 +652,7 @@ class KXRecord(DNSRecord): maxvalue=65535, ), Str('exchanger', - _domain_name_validator, + _bind_hostname_validator, label=_('Exchanger'), doc=_('A host willing to act as a key exchanger'), ), @@ -776,7 +793,7 @@ class MXRecord(DNSRecord): maxvalue=65535, ), Str('exchanger', - _domain_name_validator, + _bind_hostname_validator, label=_('Exchanger'), doc=_('A host willing to act as a mail exchanger'), ), @@ -788,7 +805,7 @@ class NSRecord(DNSRecord): parts = ( Str('hostname', - _domain_name_validator, + _bind_hostname_validator, label=_('Hostname'), ), ) @@ -802,7 +819,7 @@ class NSECRecord(DNSRecord): parts = ( Str('next', - _domain_name_validator, + _bind_hostname_validator, label=_('Next Domain Name'), ), StrEnum('types+', @@ -900,7 +917,7 @@ def _srv_target_validator(ugettext, value): if value == u'.': # service not available return - return _domain_name_validator(ugettext, value) + return _bind_hostname_validator(ugettext, value) class SRVRecord(DNSRecord): rrtype = 'SRV' @@ -1153,6 +1170,7 @@ class dnszone(LDAPObject): takes_params = ( Str('idnsname', + _domain_name_validator, cli_name='name', label=_('Zone name'), doc=_('Zone name (FQDN)'), @@ -1434,6 +1452,7 @@ class dnsrecord(LDAPObject): takes_params = ( Str('idnsname', + _dns_record_name_validator, cli_name='name', label=_('Record name'), doc=_('Record name'), diff --git a/ipalib/plugins/host.py b/ipalib/plugins/host.py index 682b81420577d7f4e153867c7435803f841ab86a..775d999f43a43611ebc5ccd7e167ff1b71d9dc43 100644 --- a/ipalib/plugins/host.py +++ b/ipalib/plugins/host.py @@ -32,7 +32,7 @@ from ipalib.plugins.service import split_principal from ipalib.plugins.service import validate_certificate from ipalib.plugins.service import set_certificate_attrs from ipalib.plugins.dns import dns_container_exists, _record_types -from ipalib.plugins.dns import add_forward_record +from ipalib.plugins.dns import add_forward_record, _hostname_validator from ipalib import _, ngettext from ipalib import x509 from ipalib.dn import * @@ -97,14 +97,6 @@ EXAMPLES: ipa host-add-managedby --hosts=test2 test """) -def validate_host(ugettext, fqdn): - """ - Require at least one dot in the hostname (to support localhost.localdomain) - """ - if fqdn.find('.') == -1: - return _('Fully-qualified hostname required') - return None - def is_forward_record(zone, str_address): addr = netaddr.IPAddress(str_address) if addr.version == 4: @@ -270,10 +262,7 @@ class host(LDAPObject): label_singular = _('Host') takes_params = ( - Str('fqdn', validate_host, - pattern='^[a-zA-Z0-9][a-zA-Z0-9-\.]{0,254}$', - pattern_errmsg='may only include letters, numbers, and -', - maxlength=255, + Str('fqdn', _hostname_validator, cli_name='hostname', label=_('Host name'), primary_key=True, @@ -555,7 +544,7 @@ class host_del(LDAPDelete): def pre_callback(self, ldap, dn, *keys, **options): # If we aren't given a fqdn, find it - if validate_host(None, keys[-1]) is not None: + if _hostname_validator(None, keys[-1]) is not None: hostentry = api.Command['host_show'](keys[-1])['result'] fqdn = hostentry['fqdn'][0] else: @@ -929,7 +918,7 @@ class host_disable(LDAPQuery): ldap = self.obj.backend # If we aren't given a fqdn, find it - if validate_host(None, keys[-1]) is not None: + if _hostname_validator(None, keys[-1]) is not None: hostentry = api.Command['host_show'](keys[-1])['result'] fqdn = hostentry['fqdn'][0] else: diff --git a/ipalib/util.py b/ipalib/util.py index eb6702dc96a7b6a7c3f5f24ddb8be8d85665564a..412ba1634d497f648f8b88723e4de72787cc71a0 100644 --- a/ipalib/util.py +++ b/ipalib/util.py @@ -213,6 +213,33 @@ def normalize_zonemgr(zonemgr): return zonemgr +def validate_dns_label(dns_label, allow_underscore=False): + label_chars = r'a-z0-9' + if allow_underscore: + label_chars += "_" + label_regex = r'^[%(chars)s]([%(chars)s-]?[%(chars)s])*$' % dict(chars=label_chars) + regex = re.compile(label_regex, re.IGNORECASE) + + if len(dns_label) > 63: + raise ValueError(_('DNS label cannot be longer that 63 characters')) + + if not regex.match(dns_label): + raise ValueError(_('only letters, numbers, and - are allowed. ' \ + '- must not be the last name character')) + +def validate_domain_name(domain_name): + if domain_name.endswith('.'): + domain_name = domain_name[:-1] + + domain_name = domain_name.split(".") + + # apply DNS name validator to every name part + map(lambda label:validate_dns_label(label), domain_name) + + if not domain_name[-1].isalpha(): + # see RFC 1123 + raise ValueError(_('top level domain label must be alphabetic')) + def validate_zonemgr(zonemgr): """ See RFC 1033, 1035 """ regex_domain = re.compile(r'^[a-z0-9]([a-z0-9-]?[a-z0-9])*$', re.IGNORECASE) @@ -252,8 +279,7 @@ def validate_zonemgr(zonemgr): if not regex_local_part.match(local_part): raise ValueError(local_part_errmsg) - if not all(regex_domain.match(part) for part in domain.split(".")): - raise ValueError(_('domain name may only include letters, numbers, and -')) + validate_domain_name(domain) def validate_hostname(hostname, check_fqdn=True): """ See RFC 952, 1123 @@ -261,20 +287,18 @@ def validate_hostname(hostname, check_fqdn=True): :param hostname Checked value :param check_fqdn Check if hostname is fully qualified """ - regex_name = re.compile(r'^[a-z0-9]([a-z0-9-]?[a-z0-9])*$', re.IGNORECASE) - if len(hostname) > 255: raise ValueError(_('cannot be longer that 255 characters')) if hostname.endswith('.'): hostname = hostname[:-1] - if check_fqdn and '.' not in hostname: - raise ValueError(_('not fully qualified')) - - if not all(regex_name.match(part) for part in hostname.split(".")): - raise ValueError(_('only letters, numbers, and - are allowed. ' \ - '- must not be the last name character')) + if '.' not in hostname: + if check_fqdn: + raise ValueError(_('not fully qualified')) + validate_dns_label(hostname) + else: + validate_domain_name(hostname) def validate_sshpubkey(ugettext, pubkey): try: diff --git a/tests/test_xmlrpc/test_dns_plugin.py b/tests/test_xmlrpc/test_dns_plugin.py index 9d83d924e924c7235df797a056cd7434a950bf1e..71e6ad40e82f0b756f6fa2604cab5d294a5cd7ba 100644 --- a/tests/test_xmlrpc/test_dns_plugin.py +++ b/tests/test_xmlrpc/test_dns_plugin.py @@ -90,6 +90,19 @@ class test_dns(Declarative): dict( + desc='Try to create zone with invalid name', + command=( + 'dnszone_add', [u'invalid zone'], { + 'idnssoamname': dnszone1_mname, + 'idnssoarname': dnszone1_rname, + 'ip_address' : u'1.2.3.4', + } + ), + expected=errors.ValidationError(name='idnsname', error=''), + ), + + + dict( desc='Create zone %r' % dnszone1, command=( 'dnszone_add', [dnszone1], { @@ -421,6 +434,13 @@ class test_dns(Declarative): dict( + desc='Try to create record with invalid name in zone %r' % dnszone1, + command=('dnsrecord_add', [dnszone1, u'invalid record'], {'arecord': u'127.0.0.1'}), + expected=errors.ValidationError(name='idnsname', error=''), + ), + + + dict( desc='Create record %r in zone %r' % (dnszone1, dnsres1), command=('dnsrecord_add', [dnszone1, dnsres1], {'arecord': u'127.0.0.1'}), expected={ -- 1.7.7.6
_______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
