[Freeipa-devel] [PATCHES 0024, 0025] Classless support for reverse domains
Martin Basti
mbasti at redhat.com
Tue Feb 11 15:23:34 UTC 2014
On Tue, 2014-02-11 at 15:42 +0100, Jan Cholasta wrote:
> On 11.2.2014 14:29, Martin Basti wrote:
> > On Mon, 2014-02-10 at 14:28 +0100, Jan Cholasta wrote:
> >> On 10.2.2014 13:14, Martin Basti wrote:
> >>> On Mon, 2014-02-10 at 12:22 +0100, Jan Cholasta wrote:
> >>>> On 10.2.2014 08:50, Petr Spacek wrote:
> >>>>> On 7.2.2014 10:42, Martin Basti wrote:
> >>>>>> On Thu, 2014-02-06 at 17:04 +0100, Martin Basti wrote:
> >>>>>>> On Thu, 2014-02-06 at 16:37 +0100, Jan Cholasta wrote:
> >>>>>>>> On 6.2.2014 15:57, Martin Basti wrote:
> >>>>>>>>> On Thu, 2014-02-06 at 10:59 +0100, Jan Cholasta wrote:
> >>>>>>>>>> Hi,
> >>>>>>>>>>
> >>>>>>>>>> On 31.1.2014 16:06, Martin Basti wrote:
> >>>>>>>>>>> Reverse domain names in form "0/28.0.10.10.in-addr.arpa." are now
> >>>>>>>>>>> allowed.
> >>>>>>>>>>>
> >>>>>>>>>>> Ticket: https://fedorahosted.org/freeipa/ticket/4143
> >>>>>>>>>>> Patches attached.
> >>>>>>>>>>
> >>>>>>>>> I add Petr2 to CC, to inspect RFC issues, with allowing '/' in IPv6
> >>>>>>>>>
> >>>>>>>>>> I think the validation should be more strict. IPv4 reverse zones
> >>>>>>>>>> should
> >>>>>>>>>> allow slash only in the label for the last octet (i.e.
> >>>>>>>>>> 0/25.1.168.192 is
> >>>>>>>>>> valid, 0.1/25.168.192 is not). IPv6 reverse zones should not allow
> >>>>>>>>>> slash
> >>>>>>>>>> at all.
> >>>>>>>>>>
> >>>>>>>>> I havent found anything about IPv6, RFCs don't forbids it.
> >>>>>>>>
> >>>>>>>> AFAIK the RFCs do not forbid anything, but we do validation anyway, so
> >>>>>>>> we might as well do it right, otherwise there is no point in doing it.
> >>>>>>>>
> >>>>>>> OK, I leave there only IPv4
> >>>>
> >>>> For the record, we discussed this off-line with Martin and Petr and
> >>>> figured out it would be best to allow slashes in IPv6 reverse zones
> >>>> after all.
> >>>>
> >>>>>>>
> >>>>>>>>> 1.0/25.1.168.192.in-addr.arpa. is also valid, it could be used to
> >>>>>>>>> CNAME
> >>>>>>>>> records
> >>>>>>>>
> >>>>>>>> Yes, obviously. It's 1.0.1/25.168.192.in-addr.arpa. I'm concerned
> >>>>>>>> about.
> >>>>>>>>
> >>>>>>>
> >>>>>>> http://tools.ietf.org/html/rfc6672#section-6.2
> >>>>>>> This can give a very strange positions of / in FQDN
> >>>>
> >>>> Point taken.
> >>>>
> >>>>>>>
> >>>>>>> Optionally, I could permit only 1 slash in domain name, but I have to
> >>>>>>> inspect first if user can do something useful with subnet of subnet in
> >>>>>>> DNS, like 1.0/25.128/25.168.192.in-addr.arpa
> >>>>> Multiple slashes has to be allowed, without limitation to last octet.
> >>>>> Imagine situation when split subnet is later split to even smaller pieces.
> >>>>>
> >>>>> Guys, do not over-engineer it. IMHO this validator should produce a
> >>>>> warning is something is not as we expect but it should not block user
> >>>>> from adding a record.
> >>>>>
> >>>>> We have had enough problems with too strict validators in the past and
> >>>>> IMHO warning is the way to go.
> >>>>
> >>>> I agree, but it's too late to get such change into 3.3.x.
> >>>>
> >>>>>
> >>>>> Petr^2 Spacek
> >>>>>
> >>>>>>>>> The slashes in domain names are referenced as the best practise in
> >>>>>>>>> RFC,
> >>>>>>>>> there are not strict rules.
> >>>>>>>>>>
> >>>>>>>>>> +def _cname_hostname_validator(ugettext, value):
> >>>>>>>>>>
> >>>>>>>>>> Can you name this _bind_cname_hostname_validator, so that it is
> >>>>>>>>>> clear it
> >>>>>>>>>> is related to _bind_hostname_validator?
> >>>>>>>>>>
> >>>>>>>>> I will rename it
> >>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> + #classless reverse zones can contain slash '/'
> >>>>>>>>>> + if not zone_is_reverse(normalized_zone) and
> >>>>>>>>>> (normalized_zone.count('/') > 0):
> >>>>>>>>>> + raise errors.ValidationError(name='name',
> >>>>>>>>>> + error=_("Only reverse zones can contain
> >>>>>>>>>> '/' in
> >>>>>>>>>> labels"))
> >>>>>>>>>>
> >>>>>>>>>> This should be handled in _domain_name_validator. Validation in
> >>>>>>>>>> pre_callback should be done only when the validation depends on
> >>>>>>>>>> values
> >>>>>>>>>> of multiple parameters, which is not this case.
> >>>>>>>>>>
> >>>>>>>>> I will move it
> >>>>>>>>>>
> >>>>>>>>>> + def _reverse_zone_pre_callback(self, ldap, dn, entry_attrs,
> >>>>>>>>>> *keys,
> >>>>>>>>>> **options):
> >>>>>>>>>>
> >>>>>>>>>> Rename this to _idnsname_pre_callback and you won't have to call it
> >>>>>>>>>> explicitly in run_precallback_validators.
> >>>>>>>>>>
> >>>>>>>>> I will rename it
> >>>>>>>>>>
> >>>>>>>>>> + if addr.count('/') > 0:
> >>>>>>>>>>
> >>>>>>>>>> I think "if '/' in addr:" would be better.
> >>>>>>>>>>
> >>>>>>>>> I will change it
> >>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> -def validate_dns_label(dns_label, allow_underscore=False):
> >>>>>>>>>> +def validate_dns_label(dns_label, allow_underscore=False,
> >>>>>>>>>> allow_slash=False):
> >>>>>>>>>>
> >>>>>>>>>> IMO instead of adding a new boolean argument, it would be nicer to
> >>>>>>>>>> replace allow_underscore with an argument (e.g. allowed_chars) which
> >>>>>>>>>> takes a string of extra allowed characters.
> >>>>>>>>>>
> >>>>>>>>> But I have to handle not only allowed chars, but position of the chars
> >>>>>>>>> in the label string too.
> >>>>>>>>
> >>>>>>>> Why? Is there a RFC that forbids it?
> >>>>>>>>
> >>>>>>>> My point is, adding a new argument for each extra character is bad,
> >>>>>>>> there should be a better way of doing that.
> >>>>>>>>
> >>>>>>> I agree, but for example: "_" should be at start (it is not required be
> >>>>>>> at the start in IPA), "/" and "-" in the middle.
> >>>>
> >>>> OK then. (But I still don't like it.)
> >>>>
> >>>>>>>
> >>>>>>
> >>>>>> Updated patch attached.
> >>>>>> Patch for tests is the same as previos.
> >>>>
> >>>> + validate_domain_name(value, allow_slash=True)
> >>>> +
> >>>> + #classless reverse zones can contain slash '/'
> >>>> + normalized_zone = normalize_zone(value)
> >>>> + if not zone_is_reverse(normalized_zone) and ('/' in
> >>>> normalized_zone):
> >>>> + return u"Only reverse zones can contain '/' in labels"
> >>>>
> >>>> You don't need to enclose "x in y" in parentheses. Also I don't think
> >>>> there is any value in pointing out that slash can be used for reverse
> >>>> zones when giving an error for non-reverse zones. I would prefer
> >>>> something like this instead:
> >>>>
> >>>> normalized_zone = normalize_zone(value)
> >>>> validate_domain_mame(value,
> >>>> allow_slash=zone_is_reverse(normalized_zone))
> >>>>
> >>>>
> >>>> + def _idnsname_pre_callback(self, ldap, dn, entry_attrs, *keys,
> >>>> **options):
> >>>> + #in reverse zone can a record name contains /, (and -)
> >>>> +
> >>>> + if self.is_pkey_zone_record(*keys):
> >>>> + addr = u''
> >>>> + else:
> >>>> + addr = keys[-1]
> >>>> +
> >>>> + zone = keys[-2]
> >>>> + zone = normalize_zone(zone)
> >>>> + if (not zone_is_reverse(zone)) and ('/' in addr):
> >>>> + raise errors.ValidationError(name='name',
> >>>> + error=unicode(_('Only domain names in reverse zones
> >>>> can contain \'/\'')) )
> >>>>
> >>>> I think you can do better (from the top of my head and untested):
> >>>>
> >>>> if not self.is_pkey_zone_record(*keys):
> >>>> zone, addr = normalize_zone(keys[-2]), keys[-1]
> >>>> try:
> >>>> validate_domain_name(addr + zone,
> >>>> allow_slash=zone_is_reverse(zone))
> >>>> except ValueError, e:
> >>>> raise errors.ValidationError(name='idnsname',
> >>>> error=str(e))
> >>>>
> >>>>
> >>>> + #Classless zones (0/25.0.0.10.in-addr.arpa.) -> skip check
> >>>> + #zone has to be checked without reverse domain suffix
> >>>> (in-addr.arpa.)
> >>>> + if '/' in addr or '/' in zone or '-' in addr or '-' in zone:
> >>>> + pass
> >>>> + else:
> >>>> + ip_addr_comp_count = addr_len + len(zone.split('.'))
> >>>> + if ip_addr_comp_count != zone_len:
> >>>> + raise errors.ValidationError(name='ptrrecord',
> >>>> + error=unicode(_('Reverse zone %(name)s requires
> >>>> exactly %(count)d IP address components, %(user_count)d given')
> >>>> + % dict(name=zone_name, count=zone_len,
> >>>> user_count=ip_addr_comp_count)))
> >>>>
> >>>> Is there anything preventing us from dropping this whole check? I don't
> >>>> think it makes sense anymore.
> >>>>
> >>> IMO it doesnt have sense with classless reverse domains, but it is
> >>> useful with classfull reverse domains, which are used more, to prevent
> >>> users making mistakes.
> >>
> >> OK, but please use a simple if instead of if/pass/else.
> >>
> >>>
> >>>>
> >>>> IMHO validate_dns_label is very ugly. I would prefer if it looked
> >>>> something like this instead (again, from the top of my head and untested):
> >>>>
> >>>> def validate_dns_label(dns_label, allow_underscore=False,
> >>>> allow_slash=False):
> >>>> base_chars = 'a-z0-9'
> >>>> extra_chars = ''
> >>>> middle_chars = ''
> >>>>
> >>>> if allow_underscore:
> >>>> extra_chars += "_"
> >>>> if allow_slash:
> >>>> middle_chars += '/'
> >>>>
> >>>> middle_chars += '-'
> >>>>
> >>>> label_regex =
> >>>> r'^[%(base)s%(extra)s]([%(base)s%(extra)%(middle)s]?[%(base)s%(extra)s])*$'
> >>>> % dict(base=base_chars, extra=extra_chars, middle=middle_chars)
> >>>> regex = re.compile(label_regex, re.IGNORECASE)
> >>>>
> >>>> if not dns_label:
> >>>> raise ValueError(_('empty DNS label'))
> >>>>
> >>>> if len(dns_label) > 63:
> >>>> raise ValueError(_('DNS label cannot be longer that 63
> >>>> characters'))
> >>>>
> >>>> if not regex.match(dns_label):
> >>>> chars = ', '.join('"%s"' % c for c in base_chars + extra_chars
> >>>> + middle_chars)
> >>>> chars2 = ', '.join('"%s"' % c for c in middle_chars)
> >>>> raise ValueError(_('only letters, numbers, %(chars)s are
> >>>> allowed. ' \
> >>>> 'DNS label may not start or end with
> >>>> %(chars2)s') \
> >>>> % dict(chars=chars, chars2=chars2))
> >>>>
> >>>>
> >>>
> >>> Thank you for the review, I will fix it.
> >>>
> >>
> >>
> >
> > All fixed. Patches attached.
> >
>
> Thanks!
>
>
> I see some new test failures caused by the error message change:
>
> ======================================================================
> FAIL: test_netgroup[17]: netgroup_add_member: Add invalid host
> u'+invalid&host' to netgroup u'netgroup1'
> ----------------------------------------------------------------------
> Traceback (most recent call last):
> File "/usr/lib/python2.7/site-packages/nose/case.py", line 197, in
> runTest
> self.test(*self.arg)
> File
> "/usr/lib/python2.7/site-packages/ipatests/test_xmlrpc/xmlrpc_test.py",
> line 284, in <lambda>
> func = lambda: self.check(nice, **test)
> File
> "/usr/lib/python2.7/site-packages/ipatests/test_xmlrpc/xmlrpc_test.py",
> line 298, in check
> self.check_exception(nice, cmd, args, options, expected)
> File
> "/usr/lib/python2.7/site-packages/ipatests/test_xmlrpc/xmlrpc_test.py",
> line 324, in check_exception
> assert_deepequal(expected.strerror, e.strerror)
> File "/usr/lib/python2.7/site-packages/ipatests/util.py", line 352,
> in assert_deepequal
> VALUE % (doc, expected, got, stack)
> AssertionError: assert_deepequal: expected != got.
>
> expected = u"invalid 'host': only letters, numbers, _, and - are
> allowed. DNS label may not start or end with -"
> got = u"invalid 'host': only letters, numbers, '_', '-' are allowed.
> DNS label may not start or end with '-'"
> path = ()
>
> ======================================================================
> FAIL: test_netgroup[32]: netgroup_mod: Add invalid host u'+invalid&host'
> to netgroup u'netgroup1' using setattr
> ----------------------------------------------------------------------
> Traceback (most recent call last):
> File "/usr/lib/python2.7/site-packages/nose/case.py", line 197, in
> runTest
> self.test(*self.arg)
> File
> "/usr/lib/python2.7/site-packages/ipatests/test_xmlrpc/xmlrpc_test.py",
> line 284, in <lambda>
> func = lambda: self.check(nice, **test)
> File
> "/usr/lib/python2.7/site-packages/ipatests/test_xmlrpc/xmlrpc_test.py",
> line 298, in check
> self.check_exception(nice, cmd, args, options, expected)
> File
> "/usr/lib/python2.7/site-packages/ipatests/test_xmlrpc/xmlrpc_test.py",
> line 324, in check_exception
> assert_deepequal(expected.strerror, e.strerror)
> File "/usr/lib/python2.7/site-packages/ipatests/util.py", line 352,
> in assert_deepequal
> VALUE % (doc, expected, got, stack)
> AssertionError: assert_deepequal: expected != got.
>
> expected = u"invalid 'externalhost': only letters, numbers, _, and -
> are allowed. DNS label may not start or end with -"
> got = u"invalid 'externalhost': only letters, numbers, '_', '-' are
> allowed. DNS label may not start or end with '-'"
> path = ()
>
> ======================================================================
> FAIL: test_raduisproxy[21]: radiusproxy_mod: Set server string of
> testradius to testradius.test:1:2:3 (invalid)
> ----------------------------------------------------------------------
> Traceback (most recent call last):
> File "/usr/lib/python2.7/site-packages/nose/case.py", line 197, in
> runTest
> self.test(*self.arg)
> File
> "/usr/lib/python2.7/site-packages/ipatests/test_xmlrpc/xmlrpc_test.py",
> line 284, in <lambda>
> func = lambda: self.check(nice, **test)
> File
> "/usr/lib/python2.7/site-packages/ipatests/test_xmlrpc/xmlrpc_test.py",
> line 298, in check
> self.check_exception(nice, cmd, args, options, expected)
> File
> "/usr/lib/python2.7/site-packages/ipatests/test_xmlrpc/xmlrpc_test.py",
> line 324, in check_exception
> assert_deepequal(expected.strerror, e.strerror)
> File "/usr/lib/python2.7/site-packages/ipatests/util.py", line 352,
> in assert_deepequal
> VALUE % (doc, expected, got, stack)
> AssertionError: assert_deepequal: expected != got.
>
> expected = u"invalid 'ipatokenradiusserver': only letters, numbers,
> _, and - are allowed. DNS label may not start or end with -"
> got = u"invalid 'ipatokenradiusserver': only letters, numbers, '_',
> '-' are allowed. DNS label may not start or end with '-'"
> path = ()
>
> ======================================================================
> FAIL: Test adding an invalid external host to Sudo rule using
> ----------------------------------------------------------------------
> Traceback (most recent call last):
> File "/usr/lib/python2.7/site-packages/nose/case.py", line 197, in
> runTest
> self.test(*self.arg)
> File
> "/usr/lib/python2.7/site-packages/ipatests/test_xmlrpc/test_sudorule_plugin.py",
> line 500, in test_a_sudorule_mod_externalhost_invalid_addattr
> "DNS label may not start or end with -")
> AssertionError
>
>
> And one nitpick: please don't add the extra newlines around
> _domain_name_validator.
>
>
> Honza
>
Thank you for review.
Patches attached.
Extra blank lines removed.
The others test are fixed now.
--
Martin^2 Basti
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mbasti-0024-4-DNS-classless-support-for-reverse-domains.patch
Type: text/x-patch
Size: 9291 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140211/b0956f8f/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mbasti-0025-3-DNS-tests-for-classless-reverse-domains.patch
Type: text/x-patch
Size: 18030 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140211/b0956f8f/attachment-0001.bin>
More information about the Freeipa-devel
mailing list