[Freeipa-devel] [PATCHES 0024, 0025] Classless support for reverse domains
Martin Kosek
mkosek at redhat.com
Tue Feb 11 16:51:17 UTC 2014
On 02/11/2014 05:11 PM, Jan Cholasta wrote:
> On 11.2.2014 16:23, Martin Basti wrote:
>> 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.
>>
>
> ACK.
>
Pushed both patches to master, but just the first to ipa-3-3 as the test
updating patch did not apply (a lot).
Martin, you will need to check if DNS tests pass in ipa-3-3, I assume there are
changes required.
Martin
More information about the Freeipa-devel
mailing list