[Freeipa-devel] [PATCHES 0024, 0025] Classless support for reverse domains

[Jan Cholasta]

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.

> 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.

> 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.

-- 
Jan Cholasta