Opened 9 years ago

Last modified 8 years ago

#6475 defect new

— at FileAuthority._lookup incorrectly returns AuthoritativeDomainError for non-subdomain names which happen to share the same suffix as the zone (eg the-example.com and example.com)Version 4

Reported by: Richard Wall Owned by: Richard Wall
Priority: normal Milestone:
Component: names Keywords:
Cc: Branch: branches/stricter-nxdomain-check-6475
branch-diff, diff-cov, branch-cov, buildbot
Author: rwall

Description (last modified by Richard Wall)

t.n.authority.FileAuthority._lookup does the following check in source:trunk/twisted/names/authority.py#L132

132                 if name.lower().endswith(self.soa[0].lower()):

Instead it should test for a common tail in a sequence of domain name labels.

Change History (4)

comment:1 Changed 9 years ago by Richard Wall

Author: rwall
Branch: branches/stricter-nxdomain-check-6475

(In [38262]) Branching to 'stricter-nxdomain-check-6475'

comment:2 Changed 9 years ago by Richard Wall

Keywords: review added

Some notes on what I've been up to.

I've fixed various things in log:branches/stricter-nxdomain-check-6475

But I got a bit carried away and I'm thinking that I need to raise each of these as separate tickets with a series of smaller patches, test cases, rfc references and then close this ticket.

I'd appreciate feedback if anyone has time to look at my branch and the points below.

Here's the list:

  1. FileAuthority incorrectly returns AuthoritativeDomainError (NXDOMAIN) for unrelated but similarly named zones (eg the-example.com and example.com)
  2. FileAuthority incorrectly returns AuthoritativeDomainError (NXDOMAIN) for names in known child zones.
  3. FileAuthority does not return additional glue records for referrals to in zone (in bailiwick) NS server names.
  4. FileAuthority does not respond with a referral for names which are children of a known child zone - only for names that match the child zone root.
  5. FileAuthority should return both A and AAAA additional host records for NS, MX and other SimpleRecord types.
  6. FileAuthority CNAME handling assumes that the target is always an A records.
  7. FileAuthority CNAME handling returns extra target answers even when the original query type was CNAME.

Other things that occurred to me but which I haven't fixed.

  1. The test_domain_com test data in test_names misuses the mname record which should contain a primary nameserver domain name.
  2. The test data also defines NS records containing IP addresses. An IPv4 address dotted quad string might strictly be a valid domain name, but in practice, such an NS record can not work.
  3. FileAuthority happily accepts records for names which are outside it's zone. This is strictly allowed for out of zone glue records etc, but in practice modern recursive resolvers will discard such records. FileAuthority probably should issue warnings when it loads these records and probably should not return them (certainly not with authoritatively)
  4. dns.Name could usefully implement contains for easy testing of domain name parent child relationships.
  5. FileAuthority is mostly exercised indirectly via test_names.ServerDNSTestCase. Which could be useful if the same tests were reused for testing with SecondaryAuthority resolver, but AFAICS they are not.
  6. While testing, I've noticed that some recursive DNS servers filter out AAAA additional records when responding to IPv4 clients. Need to check whether this is documented anywhere and if so suggest the same behaviour for Twisted.

There are so many problems that I wonder if the whole _lookup function should be reimplemented according to the standard algorithm defined here:

PowerDNS is a very well regarded, modern authoritative nameserver so it might be useful to examine its code for inspiration and to identify other potential problems in twisted names:

-RichardW.

comment:3 Changed 9 years ago by Tom Prince

Keywords: review removed
Owner: set to Richard Wall
  1. I do think it would make sense to split this into multiple tickets. Certainly your first point should be it's own ticket. I haven't looked at the rest in detail.
    • I didn't really look at the code, since there are a lot of changes here, and it will be much easier to review in small chunks.
  2. From reading the linked algorithm, it seems that CNAME handing would preferably restart handling at the top-level. Unfortunately, I don't see an easy way to do that.
  3. In your additional points, I'm no sure that (4) is reasonable. It isn't obvious to me which way the relation should be, which suggests that something explicitly named would be better.

Please file individual tickets, and then close this one with reference to them.

comment:4 Changed 9 years ago by Richard Wall

Description: modified (diff)
Summary: twistd dns returns NXDomain for valid records when multiple zone names share a common suffix or parent domainFileAuthority._lookup incorrectly returns AuthoritativeDomainError for non-subdomain names which happen to share the same suffix as the zone (eg the-example.com and example.com)

Updated summary and description so that this ticket is specifically about the suffix problem.

I'll raise new tickets for the other issues and link them to this ticket.

Note: See TracTickets for help on using tickets.