Opened 4 years ago

Last modified 4 years ago

#6580 defect assigned

twisted.names.server.DNSServerFactory should query child zones before parent zones

Reported by: Richard Wall Owned by: Richard Wall
Priority: normal Milestone:
Component: names Keywords:
Cc: Lukasz Dobrzanski Branch: branches/zone-order-6580-2
branch-diff, diff-cov, branch-cov, buildbot
Author: rwall

Description

twisted.names.server.DNSServerFactory (via ResolverChain), queries authorities in the order they appear in the supplied list.

If a parent zone (FileAuthority) precedes its child zones in the list, it may answer queries with NXDOMAIN or referral for names which should actually be answered by a child zone that contains the authoritative records.

DNSServerFactory should reorder its list of authorities so that child zones appear before their parent zones.

The order should be determined by the labels of the SOA names for each FileAuthority / SecondaryAuthority. This applies to the whole sequence of BindAuthority, PySourceAuthority and SecondaryAuthorityService that get loaded in t.n.tap.py. and passed t.n.server.DNSServerFactory.

See ticket:6475#comment:2

Attachments (2)

by_name.py (420 bytes) - added by Lukasz Dobrzanski 4 years ago.
sorting using key only
test_names.py.patch (703 bytes) - added by Lukasz Dobrzanski 4 years ago.
minor, removing unrelated args

Download all attachments as: .zip

Change History (10)

comment:1 Changed 4 years ago by Richard Wall

Author: rwall
Branch: branches/zone-order-6580

(In [38857]) Branching to 'zone-order-6580'

comment:2 Changed 4 years ago by Richard Wall

Keywords: review added

Ready for review in log:branches/zone-order-6580

  1. Build Results: http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/zone-order-6580
  2. Added a private function _nameOrder to t.n.server which can be used as the cmp key to sorted to sort domain names in subdomain order.
    1. I couldn't think how to do this as a key function - not sure it's possible.
    2. This might be moved to t.n.dns along with the _nameInName from #6475 and maybe even added to t.n.dns.Name.
  3. Also another private function to extract the zone root names from FileAuthority and SecondaryAuthority instances which may be in the authorities list.
    1. It would be nice if there was an IAuthority interface with a rootName attribute. Perhaps that can be introduced later.
  4. Authorities are now sorted in t.n.server.DNSServerFactory before they are added to the ResolverChain.
    1. I suppose it isn't strictly necessary to enforce this here, I could have done it in t.n.tap so that it only affects twistd dns.
    2. Another option might be to introduce a new AuthoritativeResolverChain class which enforces the order.

-RichardW.

Changed 4 years ago by Lukasz Dobrzanski

Attachment: by_name.py added

sorting using key only

Changed 4 years ago by Lukasz Dobrzanski

Attachment: test_names.py.patch added

minor, removing unrelated args

comment:3 Changed 4 years ago by Lukasz Dobrzanski

Cc: Lukasz Dobrzanski added
  • no need to touch twisted/names/authority.py

comment:4 Changed 4 years ago by Tom Prince

Keywords: review removed
Owner: set to Richard Wall
  1. What happens if somebody passes something that is FileAuthority or SecondaryAuthority to DNSServerFactory?
    • authorities is documented as only taking one of those classes, but it really only cares that it implement IResolver.
    • I'm think of something that uses a database backend, for example.
    • Or a resolver that just responds to a handful of queries that aren't grouped hierarchically.
    • Reading through the code, I see that you place them at the beginning of the list.
    • When I originally though of the issue this ticket aims to address, the solution I had envisioned is that a parent domain wouldn't return an authorative failure. But I guess that the a FileAuthority might not bother to include that?
  2. _nameOrder should use the function from the other branch for splitting labels. (This will handle things like trailing .)
  3. It would be nice to avoid cmp, but it isn't obvious how to do that (the attached code is close, but doesn't handle all the cases). It would be fine to leave addressing this to a separate ticket.

Thanks for working on this. Please resubmit for review after responding to 1, and fixing 2.

comment:5 in reply to:  3 Changed 4 years ago by Richard Wall

Status: newassigned

Thanks for the review lukmdo.

I started writing this message this morning, but got distracted and I now see that Tom has reviewed the ticket.

So here are my responses to your points.

Replying to lukmdo:

  • no need to touch twisted/names/authority.py

I agree. I'll revert that change.

Interesting to learn that Python3 drops the cmp key.

I tried the "by_name" function. It seems wrong to have a special case for the top level domain and with certain domain name combinations it gives the wrong order (see below).

I also think that returning a list of mixed types as the key is potentially confusing. It makes the expected order ambiguous as I have to know how Python compares list types.

On the other hand, your "by_name" function has made me realise that I'm over complicating things. I don't need to order the zones so that subdomains are adjacent to their superdomains. I *think* I just need to make sure that the most specific zones are queried first. And for that I can just order by the number of labels (see below).

It also demonstrated that I haven't used diverse enough domain lists in my tests. I'll fix that.

In [56]: domains = ['a', 'c.a.a', 'b.a']

In [57]: sorted(domains, key=by_name)
Out[57]: ['c.a.a', 'a', 'b.a']

In [58]: sorted(domains, cmp=_nameOrder)
Out[58]: ['c.a.a', 'b.a', 'a']

In [59]: sorted(domains, key=lambda x: -len(x.split('.')))
Out[59]: ['c.a.a', 'b.a', 'a']

Yes I agree. I'll do that.

comment:6 in reply to:  4 ; Changed 4 years ago by Richard Wall

Thanks for the review.

Replying to tom.prince:

  1. What happens if somebody passes something that is FileAuthority or SecondaryAuthority to DNSServerFactory?
    • authorities is documented as only taking one of those
    • classes, but it really only cares that it implement
    • IResolver.

But for the zone ordering in this branch to work, they also have to provide an SOA attribute...which is not part of IResolver. And unfortunately, FileAuthority and SecondaryAuthority publish their SOA in different ways...which is why I add the "_nameOfAuthority" adaptor / key function in the branch.

  • I'm think of something that uses a database backend, for
  • example.

Yeah, I've just been concentrating simple fixes to get FileAuthority stuff working right. I think a database backend would represent multiple zones and be accessed via a single resolver instance. Its SOA might be which would place it last in the ResolverChain so that other (more specific) FileAuthority instances would still have the opportunity to answer authoritatively...but that needs some more thought. (see #4686).

  • Or a resolver that just responds to a handful of queries that
  • aren't grouped hierarchically.

This is certainly a valid use-case. Its equivalent to the "local-data" feature of Unbound and it's something that a lot of people want. (http://linux.die.net/man/5/unbound.conf).

This makes me think that I shouldn't be enforcing zone ordering in DNSServerFactory.

I suggested in one of my earlier comments that I can do the sorting in t.n.tap instead so that it only applies to twistd dns.

And I think that's what I'll do. Especially now I've realised that the sorting can simply be based on the length of the SOA domain label sequences.

  • Reading through the code, I see that you place them at the
  • beginning of the list.
  • When I originally though of the issue this ticket aims to
  • address, the solution I had envisioned is that a parent domain
  • wouldn't return an authorative failure. But I guess that the a
  • FileAuthority might not bother to include that?

I think I see what you're saying but here's the issue.

  • The parent zone (com) is queried for one of its subdomain names (www.example.com).
  • If the parent zone doesn't contain any relevant delegations (eg neither example.com IN NS nor www.example.com IN NS) it can raise AuthoritativeDomainError (NXDOMAIN) and the controlling ResolverChain will not query any of the remaining chained resolvers. Fine. (that's another ticket:6581)
  • If the parent domain does contain delegations then it has to return a result containing a referral. This also stops the controlling ResolverChain.
  • So if there happens to be another FileAuthority for example.com, later in the ResolverChain, it will never have a chance to give its authoritative response.

I should probably add this as a comment in the code.

And since I've decided not to enforce zone ordering, I can add somewhere in the documentation too.

  1. _nameOrder should use the function from the other branch for splitting labels. (This will handle things like trailing .)

Yep. I'll merge forward to pick up that new function.

  1. It would be nice to avoid cmp, but it isn't obvious how to do that (the attached code is close, but doesn't handle all the cases). It would be fine to leave addressing this to a separate ticket.

As mentioned above (comment:5) I can achieve the desired result by just sorting based on the length of the label list. I only need to ensure that the most specific zones are queried first.

Thanks for working on this. Please resubmit for review after responding to 1, and fixing 2.

comment:7 in reply to:  6 Changed 4 years ago by Tom Prince

Replying to rwall:

  • If the parent domain does contain delegations then it has to return a result containing a referral. This also stops the controlling ResolverChain.

I was imagining that ResolverChain could inspect the result, to see if it is a referral, and in that case, keep looking, before returning the result. But perhaps that is too complicated and/or not possible to do, compatibly.

comment:8 Changed 4 years ago by Richard Wall

Branch: branches/zone-order-6580branches/zone-order-6580-2

(In [39087]) Branching to 'zone-order-6580-2'

Note: See TracTickets for help on using tickets.