Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#6475 defect closed fixed (fixed)

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)

Reported by: Richard Wall Owned by: Richard Wall
Priority: normal Milestone:
Component: names Keywords:
Cc: Branch: branches/stricter-nxdomain-check-6475-3
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 (14)

comment:1 Changed 4 years ago by Richard Wall

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

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

comment:2 Changed 4 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 4 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 4 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.

comment:5 Changed 4 years ago by Richard Wall

Branch: branches/stricter-nxdomain-check-6475branches/stricter-nxdomain-check-6475-2

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

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

Keywords: review added
Owner: Richard Wall deleted

Ready for review in log:branches/stricter-nxdomain-check-6475-2

  • I've added a new private function "_nameInName" to split names into their label sequences and compare for a common tail.
  • I'm not that happy with the name. I considered _isSubdomainOrEqual but that sounded clumsy. Anyway it's a private function which will hopefully be moved elsewhere in future branches.
  • I didn't add it to dns.Name because I'm not sure whether I want / need to use dns.Names here. Name encodes names to idna and I'm worried about how that might change the behaviour of FileAuthority.

Build Results:

Replying to tom.prince:

  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.

Done. I've changed the summary and description of this ticket to cover the first problem.

Raised #6580 and #6581 for other referral problems. I'll raise more tickets for the other issues later.

  • 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.
  1. 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.

I'll make a note of that point in the tickets I raise about CNAME handling (points 6 and 7 above)

  1. 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.

I think it would always be that "a in b" means "a is a descendant of b".

I suppose it might also be expressed as "a <= b" would allow names to be ordered in order of their labels.

The same comparisons and ordering behaviour might apply to FilePaths and URL segments I suppose, so perhaps a shared comparison mixin class could be useful.

-RichardW.

comment:7 Changed 4 years ago by Jean-Paul Calderone

I'd encourage you not to play tricky games with special methods like __contains__. a in b is extremely confusing compared to a.isSubdomainOf(b).

comment:8 Changed 4 years ago by Tom Prince

Keywords: review removed
Owner: set to Richard Wall
  1. You shouldn't say "The comment above is wrong". Instead, you should change the comment to explain what is going on, and give pointers to how it will be fixed.
  2. _nameInName seems generally useful, so should probably be public (in a different module) and have some direct tests. (It should have tests even if it isn't public, yet)
  3. What should _nameInName do in the case of a trailing period, or an empty name?
  4. _lookup is private: the news should refer to a public object. Perhaps just the class itself?
  5. I strongly agree with exarkun, that avoiding using in or <= is a good thing.

Please resubmit for review after addressing 1-3.

comment:9 in reply to:  8 Changed 4 years ago by Richard Wall

Keywords: review added
Owner: Richard Wall deleted

Thanks Tom.

Ready for another review in log:branches/stricter-nxdomain-check-6475-2

Replying to tom.prince:

  1. You shouldn't say "The comment above is wrong". Instead, you should change the comment to explain what is going on, and give pointers to how it will be fixed.

Fixed. I intend to refactor FileAuthority so that there is a separate Zone class which tracks its delegation points (subdomain NS records) and can therefore have a ".isAuthorityFor(Name)" method.

  1. _nameInName seems generally useful, so should probably be public (in a different module) and have some direct tests. (It should have tests even if it isn't public, yet)
  • I moved it to dns module (where I hope it can eventually be added to the Name class).
  • Renamed it to _isSubdomainOf - which I think is clearer.
  • Added another private helper function (_nameToLabels) for splitting names to labels. This assumes all names are fully qualified and adds a trailing blank label for the root zone...which helps answer the next point....
  1. What should _nameInName do in the case of a trailing period, or an empty name?

The _nameToLabels function now handles this by adding a trailing blank label for names without dots, returning a single blank label for a name consisting of either "" or ".".

Also added tests for these cases.

Another thing to consider is internationalized names, but I wasn't planning to handle that in this ticket.

The keys for records in the file authority and the names passed to _lookup would have to be encoded with IDNA.

dns.Name.init already does the IDNA encoding but it would be nice if dns.Name stored the name as a list of labels instead of a bytestring.

Not sure I can do that without breaking backwards compatibility.

  1. _lookup is private: the news should refer to a public object. Perhaps just the class itself?

Agreed. That's what I've done.

  1. I strongly agree with exarkun, that avoiding using in or <= is a good thing.

Yep I agree too.

Please resubmit for review after addressing 1-3.

comment:10 Changed 4 years ago by Tom Prince

Keywords: review removed
Owner: set to Richard Wall

Thanks for working on this.

  1. _nameToLabels:
    • Is there a good reason for always having an empty label at the end, rather than always not having an empty label at the end?
    • For the test name == b'.', would it make sense to just return from it, since we know what the response will be. (Rather than modifying the argument to something that will produce the correct response.)
  2. (optional) It might make sense to have an assert{Not}IsSubdomainOf in IsSubdomainOfTests, that passes a message to assert{True|False} that says what the failing compairison is.

Please merge after addressing 1 and optionally 2.

comment:11 in reply to:  10 Changed 4 years ago by Richard Wall

Status: newassigned

Replying to tom.prince:

Thanks for working on this.

  1. _nameToLabels:
    • Is there a good reason for always having an empty label at the end, rather than always not having an empty label at the end?

I wasn't sure about it either, but the blank label allows me to handle the test_emptySuperdomain case without having any special cases.

isSubdomainOf(b'foo.example.com', b''))

It's also a way to differentiate between FQDN (trailing dot) and relative domain names but then I decided to force all names to be FQDN so that's no longer a good reason.

I'll leave it for now. It's private so I can change it in a later branch if necessary.

  • For the test name == b'.', would it make sense to just return
  • from it, since we know what the response will be. (Rather than
  • modifying the argument to something that will produce the
  • correct response.)

Done. Also for empty domain name.

  1. (optional) It might make sense to have an assert{Not}IsSubdomainOf in IsSubdomainOfTests, that passes a message to assert{True|False} that says what the failing compairison is.

Done.

Please merge after addressing 1 and optionally 2.

If the build results look ok I'll merge it.

Thanks for the review.

comment:12 Changed 4 years ago by Richard Wall

Branch: branches/stricter-nxdomain-check-6475-2branches/stricter-nxdomain-check-6475-3

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

comment:13 Changed 4 years ago by Richard Wall

Resolution: fixed
Status: assignedclosed

(In [39071]) Merge stricter-nxdomain-check-6475-3

Author: rwall Reviewers: tom.prince Fixes: #6475

twisted.names.authority.FileAuthority now only returns AuthoritativeDomainError (NXDOMAIN) for names which are subdomains.

comment:14 Changed 4 years ago by Jean-Paul Calderone

  1. FileAuthority should return both A and AAAA additional host records for NS, MX and other SimpleRecord types.

I filed https://twistedmatrix.com/trac/ticket/6642 for this issue.

Note: See TracTickets for help on using tickets.