[Freeipa-devel] [PATCHES 0024, 0025] Classless support for reverse domains
Martin Basti
mbasti at redhat.com
Tue Feb 11 13:29:49 UTC 2014
On Mon, 2014-02-10 at 14:28 +0100, Jan Cholasta wrote:
> 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.
> >
>
>
All fixed. Patches attached.
--
Martin^2 Basti
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mbasti-0024-3-DNS-classless-support-for-reverse-domains.patch
Type: text/x-patch
Size: 9404 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140211/4ede7a00/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-mbasti-0025-2-DNS-tests-for-classless-reverse-domains.patch
Type: text/x-patch
Size: 14910 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140211/4ede7a00/attachment-0001.bin>
More information about the Freeipa-devel
mailing list