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