[9496] Names SecondaryAuthority traceback #1212

twm · 2019-12-30T05:56:43Z

There is a lot of string type confusion in twisted.names. This is mostly about how domain names (and labels) are represented.

I'll be using the Python 3 names for types here: bytes and str.

The twisted.internet.interfaces.IResolver interface defines a bunch of methods, about one per DNS record type. These all take the same arguments, most importantly name, but the types aren't consistent — the lookupAddress() method defines name as bytes, but every other method (including lookupIPV6Address() documents it as str. Presumably this is a Python 3 porting oversight, but the confusion goes deeper.

Since IResolver has so many methods all of the implementers in Twisted subclass twisted.names.common.ResolverBase, which maps all the methods to a _lookup() bottleneck method that can be easily overridden. This includes lookupAddress(), so clearly its type signature is supposed to align with the rest.

Internally the query is usually translated into a dns.Query instance, which represents the name as a dns.Name instance. dns.Name coerces the name to bytes, encoding Unicode using the idna codec. So most implementations simply deal with bytes.

However, some implementers override a few methods directly. For example, twisted.names.hosts.Resolver only deals in IP addresses, so it overrides lookupAddress() and lookupIPV6Address() directly. This produces inconsistent behavior: the hosts implementation only accepts bytes, not str.

All of this confusion is borne out in the tests: some pass name as str, a few as bytes. There is no systematic coverage of both types.

This is basically what's going on in twisted.names.secondary.SecondaryAuthority in Twisted #9496: it's not coercing the type of its domain parameter to bytes, so it doesn't match the query.

The design intent here seems to be that everything is bytes. However, the reality is that most things support both bytes and str, so code relies on that. I tried rejecting str, but stuff broke (and that wouldn't be backwards compatible anyway. I have therefore:

Updated IResolver to permit "bytes and str" for name parameters. We should not mislead third-party implementers.
Added a public function, twisted.names.dns.domainString(), to do the coercion in a consistent way. It's public so third-party IResolver implementations can use it.
Added coercion to all of the common.BaseResolver wrapper methods, and document _lookup()'s type to be only bytes. This makes it easier for third-party implementers.
Updated miscellaneous IResolver implementers to also do coercion using domainString().
Use the dns.Name.name attribute instead of calling str() to coerce dns.Name to a string, the former being bytes.

Contributor Checklist:

The associated ticket in Trac is here: https://twistedmatrix.com/trac/ticket/9496
The changes pass minimal style checks (see: https://twistedmatrix.com/trac/wiki/TwistedDevelopment#GettingYourPatchAccepted )
I have created a newsfragment in src/twisted/newsfragments/ (see: https://twistedmatrix.com/trac/wiki/ReviewProcess#Newsfiles )
I have updated the automated tests.
I have submitted the associated Trac ticket for review by adding the word review to the keywords field in Trac, and putting a link to this PR in the comment; it shows up in https://twisted.reviews/ now.

glyph

This looks great. I have some minor feedback inline that you can respond to, but please land when you have addressed to your satisfaction. Thanks for fixing it!

glyph · 2020-01-06T09:08:26Z

src/twisted/names/authority.py

@@ -26,6 +26,9 @@ def getSerial(filename='/tmp/twisted-names.serial'):
    State is stored in the given file.  If it does not exist, it is
    created with rw-/---/--- permissions.

+    This manipulates process-global state by calling C{os.umask()}, so it isn't


Thanks for adding this note.

glyph · 2020-01-06T09:09:31Z

src/twisted/names/authority.py

@@ -60,6 +63,9 @@ class FileAuthority(common.ResolverBase):
    """
    An Authority that is loaded from a file.

+    This is an abstract class that implements record search logic. To create
+    a functional resolver, subclass it and override the L{loadFile()} method.


(I don't think the parens are necessary for an epytext reference.)

glyph · 2020-01-06T09:10:44Z

src/twisted/names/authority.py

+        Load DNS records from a file.
+
+        This method populates the I{soa} and I{records} attributes. It must be
+        overridden in a subclass. It is called once from the initializer.


Do you want to add a raise NotImplementedError just to make this clearer?

I considered that, but didn't think that would be backward-compatible. Right now a you can subclass FileAuthority, ignore loadFile(), and populate soa and records in your __init__ instead.

Or something even weirder if you like!

twisted/src/twisted/names/test/test_names.py

Lines 38 to 42 in 2d584b9

class NoFileAuthority(authority.FileAuthority):

def __init__(self, soa, records):

# Yes, skip FileAuthority

common.ResolverBase.__init__(self)

self.soa, self.records = soa, records

glyph · 2020-01-06T09:10:51Z

src/twisted/names/authority.py

+
+        @param filename: The I{filename} parameter that was passed to the
+        initilizer.
+        """


an explicit @return might also be nice.

glyph · 2020-01-06T09:11:21Z

src/twisted/names/common.py

+    This is an implementation detail of L{ResolverBase.getHostByName}.
+
+    @param resolver: The resolver to use for the next query (unless handling
+    an I{NS} referral).


I{} or C{} ?

I{} is used in most of the places record types are given any syntax, so I stuck with that.

glyph · 2020-01-06T09:15:24Z

src/twisted/names/dns.py

+    @returns: L{bytes} suitable for network transmission.
+    @rtype: L{bytes}
+
+    @since: Twisted NEXT


👍 thank you for remembering the compat marker and the right syntax for it :)

glyph · 2020-01-06T09:17:21Z

src/twisted/names/dns.py

+    @since: Twisted NEXT
+    """
+    if isinstance(domain, unicode):
+        domain = domain.encode('idna')


You might want to use twisted/internet/_idna.py here rather than a straight .encode. (The python stdlib codec is a much older version of the spec.)

Sure, will do.

glyph · 2020-01-06T09:18:16Z

src/twisted/names/newsfragments/9496.bugfix

@@ -0,0 +1 @@
+twisted.names.secondary.SecondaryAuthority now accepts str for its domain parameter, so twist dns --secondary now functions on Python 3.


Love this explanation.

glyph · 2020-01-06T09:18:41Z

src/twisted/names/secondary.py

+        @returns: A L{Deferred} that fires with L{None} when attempted zone
+            transfer has completed.
+        """
+        if self.transferring:  # <-- never true


File a bug for this, too?

glyph · 2020-01-06T09:19:00Z

src/twisted/names/secondary.py

+        @returns: A L{Deferred} that fires with L{None} when attempted zone
+            transfer has completed.
+        """
+        if self.transferring:  # <-- never true
            return


This seems like it would make the docstring a lie, if it actually worked 😬

I'm afraid that 😬 summarizes this module too well. :(

twm

I've done what I can for the moment. Filing tickets will have to wait until dornkirk is back up.

twm · 2020-01-07T03:27:45Z

src/twisted/names/authority.py

+        Load DNS records from a file.
+
+        This method populates the I{soa} and I{records} attributes. It must be
+        overridden in a subclass. It is called once from the initializer.


I considered that, but didn't think that would be backward-compatible. Right now a you can subclass FileAuthority, ignore loadFile(), and populate soa and records in your __init__ instead.

Or something even weirder if you like!

twisted/src/twisted/names/test/test_names.py

Lines 38 to 42 in 2d584b9

class NoFileAuthority(authority.FileAuthority):

def __init__(self, soa, records):

# Yes, skip FileAuthority

common.ResolverBase.__init__(self)

self.soa, self.records = soa, records

twm · 2020-01-07T03:29:38Z

src/twisted/names/common.py

+    This is an implementation detail of L{ResolverBase.getHostByName}.
+
+    @param resolver: The resolver to use for the next query (unless handling
+    an I{NS} referral).


I{} is used in most of the places record types are given any syntax, so I stuck with that.

twm · 2020-01-07T04:37:06Z

src/twisted/names/dns.py

+    @since: Twisted NEXT
+    """
+    if isinstance(domain, unicode):
+        domain = domain.encode('idna')


Sure, will do.

twm · 2020-01-07T04:41:50Z

src/twisted/names/secondary.py

+        @returns: A L{Deferred} that fires with L{None} when attempted zone
+            transfer has completed.
+        """
+        if self.transferring:  # <-- never true
            return


I'm afraid that 😬 summarizes this module too well. :(

twm · 2020-01-07T06:07:51Z

Hmm, so switching to _idnaBytes results in test failures. This one in a test I added looks meritorious:

[ERROR]
Traceback (most recent call last):
  File "/home/twm/twisted/build/py36-alldeps-nocov/lib/python3.6/site-packages/twisted/names/test/test_dns.py", line 70, in test_idna
    self.assertEqual(b'xn--fwg.test', dns.domainString(u'\u203D.test'))
  File "/home/twm/twisted/build/py36-alldeps-nocov/lib/python3.6/site-packages/twisted/names/dns.py", line 280, in domainString
    domain = _idnaBytes(domain)
  File "/home/twm/twisted/build/py36-alldeps-nocov/lib/python3.6/site-packages/twisted/internet/_idna.py", line 30, in _idnaBytes
    return idna.encode(text)
  File "/home/twm/twisted/build/py36-alldeps-nocov/lib/python3.6/site-packages/idna/core.py", line 358, in encode
    s = alabel(label)
  File "/home/twm/twisted/build/py36-alldeps-nocov/lib/python3.6/site-packages/idna/core.py", line 281, in alabel
    check_label(label)
  File "/home/twm/twisted/build/py36-alldeps-nocov/lib/python3.6/site-packages/idna/core.py", line 261, in check_label
    raise InvalidCodepoint('Codepoint {0} at position {1} of {2} not allowed'.format(_unot(cp_value), pos+1, repr(label)))
idna.core.InvalidCodepoint: Codepoint U+203D at position 1 of '‽' not allowed

twisted.names.test.test_dns.DomainStringTests.test_idna

However most of the failures are to do with SRV support:

Traceback (most recent call last):
  File "/home/twm/twisted/build/py36-alldeps-nocov/lib/python3.6/site-packages/twisted/names/test/test_srvconnect.py", line 118, in test_SRVPresent
    self.connector.connect()
  File "/home/twm/twisted/build/py36-alldeps-nocov/lib/python3.6/site-packages/twisted/names/srvconnect.py", line 109, in connect
    nativeString(self.domain)),
  File "/home/twm/twisted/build/py36-alldeps-nocov/lib/python3.6/site-packages/twisted/names/client.py", line 731, in lookupService
    return getResolver().lookupService(name, timeout)
  File "/home/twm/twisted/build/py36-alldeps-nocov/lib/python3.6/site-packages/twisted/names/common.py", line 138, in lookupService
    return self._lookup(dns.domainString(name), dns.IN, dns.SRV, timeout)
  File "/home/twm/twisted/build/py36-alldeps-nocov/lib/python3.6/site-packages/twisted/names/dns.py", line 280, in domainString
    domain = _idnaBytes(domain)
  File "/home/twm/twisted/build/py36-alldeps-nocov/lib/python3.6/site-packages/twisted/internet/_idna.py", line 30, in _idnaBytes
    return idna.encode(text)
  File "/home/twm/twisted/build/py36-alldeps-nocov/lib/python3.6/site-packages/idna/core.py", line 358, in encode
    s = alabel(label)
  File "/home/twm/twisted/build/py36-alldeps-nocov/lib/python3.6/site-packages/idna/core.py", line 270, in alabel
    ulabel(label)
  File "/home/twm/twisted/build/py36-alldeps-nocov/lib/python3.6/site-packages/idna/core.py", line 304, in ulabel
    check_label(label)
  File "/home/twm/twisted/build/py36-alldeps-nocov/lib/python3.6/site-packages/idna/core.py", line 261, in check_label
    raise InvalidCodepoint('Codepoint {0} at position {1} of {2} not allowed'.format(_unot(cp_value), pos+1, repr(label)))
idna.core.InvalidCodepoint: Codepoint U+005F at position 1 of '_xmpp-server' not allowed

twisted.names.test.test_srvconnect.SRVConnectorTests.test_SRVPresent

This looks like kjd/idna#17, which indicates that this is a little more complex than it seemed to me at first:

IDNA encoding applies only to hostnames
Hostnames must not contain underscores
DNS names may contain underscores, e.g. as a prefix like in SRV

Sadly, it looks like the name handling behavior we want actually varies by record type. We can't apply the idna package blindly (as dns.Name did with str.encode('idna')), since it actually validates its inputs as hostnames.

I think that I will back out this change and file a ticket once dornkirk returns.

twm · 2020-01-08T04:59:28Z

Some quality time with RFC 5890 has convinced me that handling IDNA is entirely inappropriate at this layer. (Indeed, a more technically correct representation of DNS names would be Tuple[bytes], as the dots are a convention of representation, though I doubt the practical utility of that.) I have backed out the use of _idnaBytes. We'll stick with the still incorrect — but compatible — use of IDNA2003. I'll go ahead and merge when the build is green.

twm · 2020-01-08T05:14:42Z

The build is broken because of hamcrest/PyHamcrest#131

Work around hamcrest/PyHamcrest#131

twm · 2020-01-08T05:50:55Z

Now the coverage service is failing. 😠

glyph · 2020-01-09T21:22:18Z

@twm I hopefully have addressed the thing that was breaking coverage and kicked the build via the trunk merge, so hopefully you can land when it completes.

twm · 2020-01-10T03:56:31Z

Kicking build due to https://twistedmatrix.com/trac/ticket/8879#comment:36

twm · 2020-01-10T05:20:31Z

Thank you @glyph!

twm added 9 commits December 29, 2019 21:48

Port IResolverSimple and IResolver to Python 3

98d13c7

Oh god

67b797c

Document randomSource() return type

5cdeb68

Improve IEncodable docs

47d25ee

Make names name types more consistent

723fdcd

More docstrings, test cleanup

a2af5c6

Newsfile, @SInCE

006ef90

Fix lint

9cd368f

Don't forget IResolverSimple

1c25820

twm marked this pull request as ready for review December 30, 2019 06:24

twm added 4 commits December 30, 2019 14:50

Lint clean names.common

ae95ce8

Lint clean names.dns

9ea0f83

Lint, lint, lint

adc8fd6

Python 2.7, *sigh*

8840402

twm added the needs-review label Dec 31, 2019

Merge branch 'trunk' into 9496-names-traceback

2d584b9

glyph approved these changes Jan 6, 2020

View reviewed changes

twm commented Jan 7, 2020

View reviewed changes

twm added 3 commits January 7, 2020 20:41

Docstring tweaks

d3b56a3

Reference tickets

5353b8f

Another ticket

8aa9941

twm force-pushed the 9496-names-traceback branch from f4cc847 to 8aa9941 Compare January 8, 2020 04:55

Blacklist PyHamcrest 1.10.0

9638494

Work around hamcrest/PyHamcrest#131

twm closed this Jan 8, 2020

twm reopened this Jan 8, 2020

Merge branch 'trunk' into 9496-names-traceback

fecfc15

twm closed this Jan 10, 2020

twm reopened this Jan 10, 2020

twm merged commit 1d68f66 into trunk Jan 10, 2020

twm deleted the 9496-names-traceback branch January 10, 2020 05:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[9496] Names SecondaryAuthority traceback #1212

[9496] Names SecondaryAuthority traceback #1212

twm commented Dec 30, 2019 •

edited

Loading

glyph left a comment

glyph Jan 6, 2020

glyph Jan 6, 2020

glyph Jan 6, 2020

twm Jan 7, 2020

glyph Jan 6, 2020

glyph Jan 6, 2020

twm Jan 7, 2020

glyph Jan 6, 2020

glyph Jan 6, 2020

twm Jan 7, 2020

glyph Jan 6, 2020

glyph Jan 6, 2020

glyph Jan 6, 2020

twm Jan 7, 2020

twm left a comment

twm Jan 7, 2020

twm Jan 7, 2020

twm Jan 7, 2020

twm Jan 7, 2020

twm commented Jan 7, 2020

twm commented Jan 8, 2020

twm commented Jan 8, 2020

twm commented Jan 8, 2020

glyph commented Jan 9, 2020

twm commented Jan 10, 2020

twm commented Jan 10, 2020

	class NoFileAuthority(authority.FileAuthority):
	def __init__(self, soa, records):
	# Yes, skip FileAuthority
	common.ResolverBase.__init__(self)
	self.soa, self.records = soa, records

		@@ -0,0 +1 @@
		twisted.names.secondary.SecondaryAuthority now accepts str for its domain parameter, so twist dns --secondary now functions on Python 3.

[9496] Names SecondaryAuthority traceback #1212

[9496] Names SecondaryAuthority traceback #1212

Conversation

twm commented Dec 30, 2019 • edited Loading

Contributor Checklist:

glyph left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twm commented Jan 7, 2020

twm commented Jan 8, 2020

twm commented Jan 8, 2020

twm commented Jan 8, 2020

glyph commented Jan 9, 2020

twm commented Jan 10, 2020

twm commented Jan 10, 2020

twm commented Dec 30, 2019 •

edited

Loading