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.
> > 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. -- Martin^2 Basti _______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel