[Freeipa-devel] [PATCHES 0024, 0025] Classless support for reverse domains
Jan Cholasta
jcholast at redhat.com
Mon Feb 10 13:28:02 UTC 2014
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.
>
--
Jan Cholasta
More information about the Freeipa-devel
mailing list