Opened 8 years ago

Closed 8 years ago

#6642 enhancement closed fixed (fixed)

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

Reported by: Jean-Paul Calderone Owned by: Jean-Paul Calderone
Priority: normal Milestone:
Component: names Keywords:
Cc: Branch: branches/aaaa-additionals-6642
branch-diff, diff-cov, branch-cov, buildbot
Author: exarkun

Description

FileAuthority._lookup does the various kinds of processing to construct a proper response for some particular query.

This includes the "additional section processing" described in <http://tools.ietf.org/html/rfc2181#section-9> (perhaps described better elsewhere).

http://tools.ietf.org/html/rfc4472#appendix-B expands on this to explain that AAAA records may be considered useful additional records (expanding the set from just A records).

FileAuthority._lookup will include any A records it has, but it currently does not try to include any AAAA records.

Since including this records will increase the size of the response, it may also be worth considering the "courtesy" vs "critical" distinction. However, since Twisted Names currently blithly ignores size limits, fixing this issue here perhaps need not be addressed for this ticket specifically.

First reported <https://twistedmatrix.com/trac/ticket/6475#comment:2>.

Change History (12)

comment:1 Changed 8 years ago by Jean-Paul Calderone

Author: exarkun
Branch: branches/aaaa-additionals-6642

(In [39304]) Branching to 'aaaa-additionals-6642'

comment:2 Changed 8 years ago by Jean-Paul Calderone

Keywords: review added

comment:3 Changed 8 years ago by Richard Wall

Owner: set to Richard Wall
Status: newassigned

comment:4 Changed 8 years ago by Richard Wall

Keywords: review removed
Owner: changed from Richard Wall to Jean-Paul Calderone
Status: assignednew

Code Review

Notes:

  • Coverage seems complete.

Points:

  1. Twistedchecker warnings: https://buildbot.twistedmatrix.com/builders/twistedchecker/builds/1050/steps/run-twistedchecker/logs/new%20twistedchecker%20errors
    ************* Module twisted.names.test.test_names
    W9202:727,4:AdditionalProcessingTests._additionalMXTest: Missing epytext markup @param for argument "addresses"
    W9202:760,4:AdditionalProcessingTests._additionalCNAMETest: Missing epytext markup @param for argument "addresses"
    W9202:793,4:AdditionalProcessingTests._additionalNSTest: Missing epytext markup @param for argument "addresses"
    
  1. source:branches/aaaa-additionals-6642/twisted/names/authority.py
  1. "ADDITIONAL_PROCESSING_TYPES": I quite like the UPPER_CASE convention for class constants, but it doesn't follow the coding standard: https://twistedmatrix.com/documents/current/core/development/policy/coding-standard.html#auto21
  1. "section = {dns.NS: additional, dns.CNAME: results, dns.MX: additional}.get(record.type)": the new "_additionalRecords" method changes the behaviour for CNAMEs. The original code resolved CNAMES and added the target to "results". You now add it to "additional" which is wrong.
    1. I think it would be fine to postpone resolving CNAMEs. I think they require special treatment to handle chains of of CNAMES and detection of circular references.
  1. Raise a ticket to add additional processing for other record types that contain references to hostnames eg SRV, PTR, SOA https://en.wikipedia.org/wiki/List_of_DNS_record_types although SOA may be a special case according to this throw away line in https://tools.ietf.org/html/rfc1035#section-3.3.13 "SOA records cause no additional section processing"...I'm not sure why mname can't be added to additional section.
  1. I think we're misusing the soa minimum ttl. It should be max(rec.ttl, default_ttl) not rec.ttl or default_ttl. Consider doing the right thing in _additionalRecords...but maybe it's a separate ticket. https://tools.ietf.org/html/rfc1035#section-3.3.13 "Thus MINIMUM is a lower bound on the TTL field for all RRs in a zone"
  1. I don't think the additional records should be marked authoritative if we are responding with a delegation. Ie if they are associated with NS records in the authority section. I think the side effect of this will be to mark the response Message authoritative which it is not. I think this is an existing bug however and should be sorted in a separate ticket.
    $ dig @a.gtld-servers.net. twistedmatrix.com NS +norecurse
    ;; flags: qr; QUERY: 1, ANSWER: 0, AUTHORITY: 2, ADDITIONAL: 3
    
    $ dig @ns1.twistedmatrix.com. twistedmatrix.com NS +norecurse
    ;; flags: qr aa; QUERY: 1, ANSWER: 2, AUTHORITY: 0, ADDITIONAL: 2
    
  1. source:branches/aaaa-additionals-6642/twisted/names/test/test_names.py
    1. "return self.response": This change seems unnecessary.
  1. you removed "testCNAMEAdditional" which was misnamed, but which was testing that CNAME targets got added to the answers section.

That's all for now. Please re-submit for review after addressing or responding to at least 1, 2,1, 2.2, 2.3, and 3 above.

-RichardW.

comment:5 Changed 8 years ago by Richard Wall

Sorry I meant to add a reference to the information about CNAMEs and their targets in the answers section rather than additional section.

If the data at the node is a CNAME, and QTYPE doesn't match CNAME, copy the CNAME RR into the answer section of the response, change QNAME to the canonical name in the CNAME RR, and go back to step 1.

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

Replying to rwall:

  1. I think we're misusing the soa minimum ttl. It should be max(rec.ttl, default_ttl) not rec.ttl or default_ttl. Consider doing the right thing in _additionalRecords...but maybe it's a separate ticket. https://tools.ietf.org/html/rfc1035#section-3.3.13 "Thus MINIMUM is a lower bound on the TTL field for all RRs in a zone"

Actually the SOA minimum TTL value has been redefined to mean the negative caching TTL.

Anyway it's nothing to do with this branch and I'll create a new ticket about it.

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

(In [39355]) Move the CNAME information back into the answer section

refs #6642

comment:8 Changed 8 years ago by Jean-Paul Calderone

(In [39356]) Add @param for the addresses parameter to the various helpers

refs #6642

comment:9 Changed 8 years ago by Jean-Paul Calderone

Keywords: review added
Owner: Jean-Paul Calderone deleted

Thanks for the review.

  1. Twistedchecker warnings

Fixed in r39356.

2.1 "ADDITIONAL_PROCESSING_TYPES": I quite like the UPPER_CASE convention for class constants, but it doesn't follow the coding standard.

This seems to be an oversight in the documentation for the coding standard. The convention is used widely throughout Twisted.

2.2 The original code resolved CNAMES and added the target to "results". You now add it to "additional" which is wrong.

I restored the old behavior in r39355.

2.3 Raise a ticket to add additional processing for other record types that contain references to hostnames eg SRV, PTR, SOA

https://twistedmatrix.com/trac/ticket/6650 / r39358.

2.4 I think we're misusing the soa minimum ttl. It should be max(rec.ttl, default_ttl) not rec.ttl or default_ttl. Consider doing the right thing in _additionalRecords...but maybe it's a separate ticket

I think this deserves its own ticket. There's probably a lot of missing or wrong tests for ttls. I think you have a better understanding of the misbehavior than I do - do you mind filing this one?

2.5 I don't think the additional records should be marked authoritative if we are responding with a delegation. Ie if they are associated with NS records in the authority section. I think the side effect of this will be to mark the response Message authoritative which it is not. I think this is an existing bug however and should be sorted in a separate ticket.

I could believe that. Do you have an RFC reference? I agree a separate ticket seems to make sense here as well. Can I ask you to file this ticket as well? :)

3.1 "return self.response": This change seems unnecessary.

Indeed, unnecessary but correct. The ability/convention to raise a Failure instance is crazy and doesn't work on Python 3. I can split it onto its ticket, if you prefer.

3.2 you removed "testCNAMEAdditional" which was misnamed, but which was testing that CNAME targets got added to the answers section.

Indeed. I think this is addressed by r39355.

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

Keywords: review removed
Owner: set to Jean-Paul Calderone

The code and test changes look good but there's one failing test in another test module.

[ERROR]
Traceback (most recent call last):
  File "/home/richard/projects/Twisted/branches/aaaa-additionals-6642/twisted/names/test/test_names.py", line 203, in checkResults
    assert len(results) == len(r), "%s != %s" % (map(str, results), map(str, r))
exceptions.AssertionError: ['<MX preference=10 name=host.test-domain.com ttl=19283784>', '<A address=123.242.1.5 ttl=19283784>', '<A address=0.255.0.255 ttl=19283784>'] != ['<MX preference=10 name=host.test-domain.com ttl=19283784>']

twisted.names.test.test_names.ServerDNSTestCase.testMailExchangeRecord

I did some functional testing by making the following modification to the example pyzone and running with

./bin/twistd -n dns --port 10053 --pyzone=doc/names/howto/listings/names/example-domain.com

Index: doc/names/howto/listings/names/example-domain.com
===================================================================
--- doc/names/howto/listings/names/example-domain.com	(revision 39437)
+++ doc/names/howto/listings/names/example-domain.com	(working copy)
@@ -27,11 +27,19 @@
     ),

     A('example-domain.com', '127.0.0.1'),
+    AAAA('example-domain.com', '::1'),
+
     NS('example-domain.com', 'ns1.example-domain.com'),
+    A('ns1.example-domain.com', '127.0.0.1'),
+    AAAA('ns1.example-domain.com', '::1'),

     CNAME('www.example-domain.com', 'example-domain.com'),
     CNAME('ftp.example-domain.com', 'example-domain.com'),

     MX('example-domain.com', 0, 'mail.example-domain.com'),
-    A('mail.example-domain.com', '123.0.16.43')
+    A('mail.example-domain.com', '123.0.16.43'),
+    AAAA('mail.example-domain.com', '::1'),
+
+    NS('subdomain.example-domain.com', 'ns1.subdomain.example-domain.com'),
+    A('ns1.subdomain.example-domain.com', '123.0.16.43'),

Also worth noting that CNAME target is not restricted to host records. It could be TXT, SRV, etc. FileAuthority never handled that case, but it might influence the design of the _additionalRecords method and its tests.

If you don't feel like tackling that in this branch then you or I should raise a ticket about it. See ticket:6475#comment:2 (point 6)

Replying to exarkun:

2.4 I think we're misusing the soa minimum ttl. It should be max(rec.ttl, default_ttl) not rec.ttl or default_ttl. Consider doing the right thing in _additionalRecords...but maybe it's a separate ticket

I think this deserves its own ticket. There's probably a lot of missing or wrong tests for ttls. I think you have a better understanding of the misbehavior than I do - do you mind filing this one?

Done. #6661

2.5 I don't think the additional records should be marked authoritative if we are responding with a delegation. Ie if they are associated with NS records in the authority section. I think the side effect of this will be to mark the response Message authoritative which it is not. I think this is an existing bug however and should be sorted in a separate ticket.

I could believe that. Do you have an RFC reference? I agree a separate ticket seems to make sense here as well. Can I ask you to file this ticket as well? :)

Done. It's not quite what I thought. But there is a bug which I described in #6662

Please fix the broken test and merge. Unless you decide to have a go at fixing the alternative CNAME target type issue - in which case it probably should be reviewed again.

comment:11 Changed 8 years ago by Jean-Paul Calderone

If you don't feel like tackling that in this branch then you or I should raise a ticket about it.

Filed #6732.

comment:12 Changed 8 years ago by Jean-Paul Calderone

Resolution: fixed
Status: newclosed

(In [39893]) Merge aaaa-additionals-6642

Author: exarkun Reviewer: rwall Fixes: #6642

Teach the authorities in twisted.names.authority about AAAA records in their logic to populate the "additional" section of responses.

Note: See TracTickets for help on using tickets.