Opened 3 years ago

Closed 3 years ago

#5524 defect closed fixed (fixed)

DNS server does not properly recurse

Reported by: greezybacon Owned by: exarkun
Priority: normal Milestone:
Component: names Keywords: dns names recurse
Cc: Branch: branches/dns-recursion-5524-2
(diff, github, buildbot, log)
Author: exarkun Launchpad Bug:

Description

The DNS server recurses before checking the hosts file. I added the following to a hosts.txt file:

127.0.0.1 bing.com
127.0.0.1 bogus.domain.tld

The server was kicked off to serve from the hosts file

sudo bin/twistd -n dns --hosts-file=hosts.txt -i 127.0.0.1

Then query with dig

Project/Twisted [ dig @localhost bing.com                             ] 7:57 PM

; <<>> DiG 9.7.3-P3 <<>> @localhost bing.com
; (3 servers found)
;; global options: +cmd
;; Got answer:
;; ->>HEADER<<- opcode: QUERY, status: NOERROR, id: 30843
;; flags: qr rd ra; QUERY: 1, ANSWER: 1, AUTHORITY: 0, ADDITIONAL: 0

;; QUESTION SECTION:
;bing.com.			IN	A

;; ANSWER SECTION:
bing.com.		3600	IN	A	127.0.0.1

;; Query time: 1 msec
;; SERVER: 127.0.0.1#53(127.0.0.1)
;; WHEN: Sun Mar 11 19:57:23 2012
;; MSG SIZE  rcvd: 42

And the correct IP address is retrieved. However, if twisted is asked to recurse

sudo bin/twistd -n dns --hosts-file=hosts.txt -i 127.0.0.1 -r

And query for the same address

Project/Twisted [ dig @localhost bing.com                             ] 8:03 PM

; <<>> DiG 9.7.3-P3 <<>> @localhost bing.com
; (3 servers found)
;; global options: +cmd
;; Got answer:
;; ->>HEADER<<- opcode: QUERY, status: NOERROR, id: 44932
;; flags: qr rd ra; QUERY: 1, ANSWER: 1, AUTHORITY: 0, ADDITIONAL: 0

;; QUESTION SECTION:
;bing.com.			IN	A

;; ANSWER SECTION:
bing.com.		179	IN	A	65.52.107.149

;; Query time: 98 msec
;; SERVER: 127.0.0.1#53(127.0.0.1)
;; WHEN: Sun Mar 11 20:05:20 2012
;; MSG SIZE  rcvd: 42

It appears that the recurse is being done before checking the hosts file. For unknown addresses (ones not really on the internet), everything appears to work correctly, since the recurse fails. The hosts file entry will then be used:

Project/Twisted [ dig @localhost bogus.domain.tld                     ] 8:14 PM

; <<>> DiG 9.7.3-P3 <<>> @localhost bogus.domain.tld
; (3 servers found)
;; global options: +cmd
;; Got answer:
;; ->>HEADER<<- opcode: QUERY, status: NOERROR, id: 58695
;; flags: qr rd ra; QUERY: 1, ANSWER: 1, AUTHORITY: 0, ADDITIONAL: 0

;; QUESTION SECTION:
;bogus.domain.tld.		IN	A

;; ANSWER SECTION:
bogus.domain.tld.	3600	IN	A	127.0.0.1

;; Query time: 119 msec
;; SERVER: 127.0.0.1#53(127.0.0.1)
;; WHEN: Sun Mar 11 20:14:21 2012
;; MSG SIZE  rcvd: 50

Attachments (4)

dns-hosts-recursive-fix.patch (672 bytes) - added by greezybacon 3 years ago.
Server should check hosts file FIRST, then recurse
dns-hosts-recursive-fix.2.patch (2.9 KB) - added by greezybacon 3 years ago.
Better patch, with an associated test case
dns-hosts-recursive-fix.3.patch (3.0 KB) - added by greezybacon 3 years ago.
Even better patch, now only cleans up the parseConfig delayed call for the Resolver
dns-hosts-recursive-fix.4.patch (2.8 KB) - added by greezybacon 3 years ago.
Move import statement back into the _buildConfig function

Download all attachments as: .zip

Change History (18)

Changed 3 years ago by greezybacon

Server should check hosts file FIRST, then recurse

comment:1 Changed 3 years ago by thijs

  • Keywords review added

Changed 3 years ago by greezybacon

Better patch, with an associated test case

comment:2 Changed 3 years ago by acapnotic

  • Owner set to acapnotic

Changed 3 years ago by greezybacon

Even better patch, now only cleans up the parseConfig delayed call for the Resolver

comment:3 Changed 3 years ago by acapnotic

You've moved the import statement up to the module level, which makes sense in most cases, but tap files are an exception. Reason being that all the tap files are loaded every time you run twistd, so you want to keep down the number of imports.

Oh, yow, you're still working on this. Was the "review" keyword added prematurely?

comment:4 Changed 3 years ago by acapnotic

  • Keywords review removed
  • Owner changed from acapnotic to greezybacon

The _buildResolvers refactoring looks like a good move for your testability, and the revision to the tests in the .3.patch is certainly a step in the right direction.

I'm trying to find someone to review the basic DNS policy question here, as it may have compatibility implications, and everyone I've asked about DNS so far says "talk to exarkun."

So I suggest fixing the imports, putting this back in review, and we'll see if we can get him to comment.

Changed 3 years ago by greezybacon

Move import statement back into the _buildConfig function

comment:5 Changed 3 years ago by greezybacon

  • Keywords review added

RFC 1034 http://www.ietf.org/rfc/rfc1034.txt, section 5.3.3 Step 1 indicates that information should be fetched from cache and local server's zone first, and then recurse the query to an appropriate server

I want to point out also, that the "recursive" resolver in this animal uses an instance of resolve.ResolverChain, which will actually have it own, separate instance of a cache resolver and a separate instance of a hosts resolver (which will use the system hosts file). In the ResolverChain, the local hosts file will be used in favor of recursing if the name is defined there.

The change this patch makes is, if the server is run with a separate --hosts-file option, then an additional hosts file will be used, and should be used in favor of recursing to the resolve.ResolverChain resolver to recurse.

So the chain before the fix would be:

Cache -> Resolver { Cache -> System Hosts -> Recurse } -> Hosts { extra file provided }

I'm suggesting that this flow more like

Cache -> Hosts { extra file provided } -> Resolver { Cache -> System Hosts -> Recurse }

Honestly, the whole chain should be refactored more like

Cache -> Hosts { extra file provided } -> Hosts { system hosts file } -> Recurse (client.Resolver)

So that the second cache lookup is avoided

comment:6 Changed 3 years ago by thijs

  • Owner greezybacon deleted

comment:7 Changed 3 years ago by exarkun

  • Owner set to exarkun
  • Status changed from new to assigned

comment:8 Changed 3 years ago by exarkun

  • Keywords review removed

Thanks! This patch looks good to me now. It needs a ReviewProcess#Newsfile, but I can easily add that. I also agree with your remarks in comment 5 about further changes that should be made.

comment:9 Changed 3 years ago by exarkun

  • Resolution set to fixed
  • Status changed from assigned to closed

(In [33835]) Apply dns-hosts-recursive-fix.4.patch - fixing hosts/upstream precedence rules for "twistd dns"

Author: greezybacon
Reviewer: acapnotic, exarkun
Fixes: #5524

Change twisted.names.tap to construct a chain resolver that uses a hosts file resolver first,
and then falls back to a recursive resolver if necessary. Previously a hosts file would not
override existing domain names that could be resolved over the network.

comment:10 Changed 3 years ago by exarkun

  • Resolution fixed deleted
  • Status changed from closed to reopened

(In [33880]) Revert r33835 - Windows test suite regression

Reopens: #5524

The new tests fail on Windows:

[ERROR]
Traceback (most recent call last):

File "c:\bot-glyph-5\winxp32-py2.5-select\Twisted\twisted\names\test\test_tap.py", line 94, in test_recursiveConfiguration

x.resolvers[-1]._parseCall.cancel()

File "c:\bot-glyph-5\winxp32-py2.5-select\Twisted\twisted\names\root.py", line 431, in getattr

raise AttributeError(name)

exceptions.AttributeError: _parseCall

twisted.names.test.test_tap.OptionsTests.test_recursiveConfiguration

comment:11 Changed 3 years ago by exarkun

  • Author set to exarkun
  • Branch set to branches/dns-recursion-5524-2

(In [33883]) Branching to 'dns-recursion-5524-2'

comment:12 Changed 3 years ago by exarkun

I added a check to avoid the cleanup on Windows. Let's see if that works any better.

comment:13 Changed 3 years ago by exarkun

(In [33925]) Correct api link; there is no usage.Config, and the real target should be the Options class in this module anyway.

refs #5524

comment:14 Changed 3 years ago by exarkun

  • Resolution set to fixed
  • Status changed from reopened to closed

(In [33926]) Merge dns-recursion-5524-2

Author: greezybacon, exarkun
Reviewer: acapnotic, exarkun
Fixes: #5524

Change twisted.names.tap to construct a chain resolver that uses a hosts file resolver first,
and then falls back to a recursive resolver if necessary. Previously a hosts file would not
override existing domain names that could be resolved over the network.

Note: See TracTickets for help on using tickets.