[Freeipa-devel] [PATCHES 0024, 0025] Classless support for reverse domains
Martin Basti
mbasti at redhat.com
Mon Feb 10 12:14:19 UTC 2014
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
More information about the Freeipa-devel
mailing list