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

[Petr Spacek]

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

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.
>>
>
> Updated patch attached.
> Patch for tests is the same as previos.


-- 
Petr^2 Spacek