[Freeipa-devel] [PATCH 0029-0046] Internationalized domain names in DNS plugin

Jan Cholasta jcholast at redhat.com
Thu Apr 3 13:35:23 UTC 2014

On 2.4.2014 14:07, Martin Basti wrote:
> Helo list,
> this patchset allows to use internationalized domian in DNS plugin.
> - dns names are stored in ACE form(punycoded) in LDAP
> - raw option shows dns data in ACE form, otherwise dns names are
> converted to unicode
> - plugin allow all characters in domain name, which are valid by IDN
> RFCs (almost everything including non-printable), should be validation
> more restrictive? (there is bug in dnspython with special characters,
> will be fixed soon)
> - TODO update WebUI to support DNSName objects
> Required patches:
> freeipa-jcholast-255-Allow-primary-keys-to-use-different-type-than-unicod.patch
> freeipa-jcholast-256-Support-API-version-specific-RPC-marshalling.patch
> freeipa-jcholast-257-Replace-get_syntax-method-of-IPASimpleObject-with-ne.patch
> freeipa-jcholast-258-Use-raw-attribute-values-in-command-result-when-raw-.patch
> freeipa-jcholast-259-Keep-original-name-when-setting-attribute-in-LDAPEnt.patch
> Patches attached.

First batch of comments, so far I have only read the code/patches, 
without doing actual testing.

Patch 30:


It might make sense to put all of this into a new module (e.g. 
dnsutil.py) rather than ipautil.


+        if isinstance(labels, str):
+            if not labels:
+                raise ValueError('empty string')
+        elif isinstance(labels, unicode):
+            if not labels:
+                raise ValueError('empty string')

It might be nicer to:

+        if isinstance(labels, basestring) and not labels:
+            raise ValueError('empty string')
+        if isinstance(labels, str):
+        elif isinstance(labels, unicode):


+    def __nonzero__(self):
+        return True

It would be nice to include a comment about why DNSName always evaluates 
to True (mention "@").


+    @staticmethod
+    def get_root():
+        return DNSName(dns.name.root)
+    @staticmethod
+    def get_origin_sign():
+        return DNSName(u'@')
+    @staticmethod
+    def get_rev_zone():
+        return DNSName(u'in-addr.arpa.')
+    @staticmethod
+    def get_ip6_rev_zone():
+        return DNSName(u'ip6.arpa.')

I think you should either drop the "get_" prefix from the name, or (even 
better) make these global constants.

I would shorten "origin_sign" to just "sign".

Can you please use tuples of str objects (i.e. what dns.name.Name uses 
internally) instead of unicode objects for the initialization? I think 
it should be the preferred style of initializing DNSName objects (DN 
objects do the same).


+    def __str__(self):
+        return super(DNSName, self).to_text()

You don't need to use super here.


+    def ToASCII(self, omit_final_dot=False):
+        return super(DNSName, 
+    def ToUnicode(self, omit_final_dot=False):
+        return super(DNSName, 

What was the reason for the unusual naming again? I would prefer PEP-8 
compatible names (e.g. "to_ascii" and "to_unicode"), but if the current 
names absolutely have to stay, please add a comment with explanation.

I don't like the "omit_final_dot" flag. IMHO it should be dropped and 
whether the result includes a final dot or not should depend solely on 
whether the name is absolute or relative. You can still use e.g. 
"name.derelativize(root).ToUnicode()" to drop the final dot, which is 
more explanatory.

In ToUnicode, the call to dns.name.Name.to_unicode already returns a 
unicode object, no need to call decode on it.


+    def concatenate(self, other):
+        return DNSName(super(DNSName, self).concatenate(other).labels)
+    def relativize(self, origin):
+        return DNSName(super(DNSName, self).relativize(origin).labels)
+    def derelativize(self, origin):
+        return DNSName(super(DNSName, self).derelativize(origin).labels)
+    def choose_relativity(self, origin=None, relativize=True):
+        return DNSName(super(DNSName, 
self).choose_relativity(origin=origin, relativize=relativize).labels)

Why use ".labels" here? The DNSName constructor knows how to deal with 
dns.name.Name objects, right?


+    def is_ip_reverse(self):
+        if self.is_subdomain(self.get_rev_zone()):
+            return True
+        return False
+    def is_ip6_reverse(self):
+        if self.is_subdomain(self.get_ip6_rev_zone()):
+            return True
+        return False
+    def is_reverse(self):
+        if self.is_ip_reverse() or self.is_ip6_reverse():
+            return True
+        return False

The ifs are all redundant. Return the result of the check directly 
("return self.is_subdomain ..." etc.)

Patch 31:


+    kwargs = Param.kwargs + (
+        ('require_absolute', bool, False),
+        ('require_relative', bool, False),
+    )

What about renaming these to 'only_absolute' and 'only_relative'? IMO it 
better captures the meaning (yes I know we already discussed the naming 
in length :-)


+                except (dns.name.LabelTooLong, UnicodeError):
+                    #dnspython bug?, punycoded label longer than 63 
ASCII-chars returns Unicode Error
+                    #instead of LabelTooLong

If that's true, it should be handled in DNSName constructor, not here.


All of the dns.name exceptions inherit from dns.exception.SyntaxError, I 
think you should add an except for it as well.


+                #compare if IDN normalized and original domain match
+                normalized_domain_name = encodings.idna.nameprep(value)
+                if(value != normalized_domain_name):
+                    raise ValueError( _("domain name '%(domain)s' and 
normalized domain name "
+                                            "'%(normalized)s' do not 
match. Please use only "
+                                            "normalized domains")  % 
+ 'normalized':normalized_domain_name})

Can we instead try to normalize the name in the beginning of 
_convert_scalar rather than raise an error later?


+            except Exception as e:
+                raise ValidationError(name=self.name,
+                                  value=value,
+                                  index=index,
+                                  error=unicode(e))

Since this is _convert_scalar, I think this should be ConversionError 
(even if the errors are technically validation errors).

Also, use self.get_param_name() instead of self.name.


I'm not a fan of the layout of _convert_scalar, can you please use 
something like this instead (includes changes requested in the previous 

     def _convert_scalar(self, value, index):
         if isinstance(value, unicode):
             value = encodings.idna.nameprep(value)

             error = None
                 value = DNSName(value)
             except dns.name.BadEscape:
                 error = _("invalid escape code")
             except dns.exception.SyntaxError:
                 error = _("invalid syntax")

             if error:
                 raise ConversionError(
                     name=self.get_param_name(), index=index, error=error)

         return super(DNSNameParam, self)._convert_scalar(value, index)

Patch 33:

Why is this patch necessary? It does not seem right to me.

Patch 34:

This patch should be squashed with patch 32. They don't make sense 
without each other.

Patch 37:


-def _rname_validator(ugettext, zonemgr):

What's up with this renaming of everything? Don't do it please, unless 
absolutely necessary.


You don't need to redefine has_output for dnszone_add, dnszone_mod, 
dnszone_show, dnsrecord_add, dnsrecord_mod and dnsrecord_show. They 
already use standard_entry.


+    def post_callback(self, ldap, dn, entry_attrs, *keys, **options):
+        assert isinstance(dn, DN)
+        _ns_zone_post_convert(entry_attrs, **options)
+        return dn

Please don't use stuff that is not defined in this or previous patches. 
IPA should be at least buildable after each patch is applied.


      has_output = output.standard_value
      msg_summary = _('Disabled DNS zone "%(value)s"')

No new whitespace please (seen 2 times in the patch).

Patch 38:


+_dns_zone_record = DNSName(u'@')

You know you already defined a constant for this in patch 30, right?


No new whitespace please (seen 2 times in the patch).


+    #TODO This is used by realmdomains.py (but it shouldnt allow 
classless), not used in DNS

+    #TODO This is used by Host.py

I don't think you should add these comments, since they are resolved in 
patches 41 and 42.


+def _hostname_validator_idn(ugettext, value):
+    assert isinstance(value, DNSName)
+    req_len = 2
+    if value.is_absolute():
+        req_len = 3
+    if len(value.labels) < req_len:
+        return _('invalid domain-name: not fully qualified')
+    return None

Why the "_idn" postfix? It does not seem to be related to IDN.

However, it does seem like it does the same thing as 
_hostname_validator, so I think they should be merged.


              zonename = zone['idnsname'][0]
-            if revdns.endswith(zonename) and len(zonename) > len(revzone):
-                revzone = zonename
+            if revdns.is_subdomain(zonename.make_absolute()):
+                if revzone is None:
+                    revzone = zonename
+                elif zonename.is_subdomain(revzone):
+                    revzone = zonename

The idnsname returned from dnszone_find should already be absolute, so 
why "zonename.make_absolute()"?

You can shorten the condition to:

     if revdns.is_subdomain(zonename) and (revzone is None or 


-        revzone = u'.'.join(items[pos:])
+        revzone = DNSName(u'.'.join(items[pos:]))

You join the items here into string which is split again in DNSName. Use 
this instead:

     revzone = DNSName(items[pos:])


-    return revzone, revname
+    return DNSName(revzone), DNSName(revname)

Aren't both revzone and revname DNSName objects already?


-            reason=_('DNS zone %(zone)s not found') % dict(zone=domain)
+            reason=_('DNS zone %(zone)s not found') % 

Is the ".ToUnicode()" necessary? (seen 4 times in the patch)


IMO normalize_zonemgr_idn and validate_zonemgr_idn should be merged into 
normalize_zonemgr and validate_zonemgr, respectively.


  def zone_is_reverse(zone_name):
-    zone_name = normalize_zone(zone_name)
-    if any(zone_name.endswith(name) for name in REVERSE_DNS_ZONES):
-        return True
+    if(isinstance(zone_name, DNSName)):
+        if any(zone_name.is_subdomain(DNSName(name)) for name in 
+            return True
+    else:
+        zone_name = normalize_zone(zone_name)
+        if any(zone_name.endswith(name) for name in REVERSE_DNS_ZONES):
+            return True

      return False

Something like this would definitely be better:

     def zone_is_reverse(zone_name):
         if not isinstance(zone_name, DNSName):
             zone_name = DNSName(zone_name)
         return zone_name.is_reverse()

Patch 39:


-        Str('hostname',
-            _hostname_validator,
-            normalizer=_normalize_hostname,
+        DNSNameParam('hostname',
+            #RFC 2317 section 5.2 -- don't have to be FQDN

It seems you are changing behavior here. Such things belong in separate 

Patch 40:


+            addr = DNSName(u'')

This will always raise ValueError with the current implementation of 


No new whitespace please (seen 5 times in the patch).

Patch 43:

I think this patch should be squashed into patch 38.

Patch 44:

This patch should be squashed into patch 39.


+<<<<<<< HEAD
  output: PrimaryKey('value', None, None)
+output: Output('value', <class 'ipapython.ipautil.DNSName'>, None)
+>>>>>>> 5df7ed2... API change

It seems you overlooked this.

This is all for now. Expect more later ;-)


Jan Cholasta

More information about the Freeipa-devel mailing list