BIND 10 #1570: DS query handling in auth::Query

[BIND 10 Development]

#1570: DS query handling in auth::Query
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:         |  Sprint-20120207
  critical                           |            Resolution:
                  Component:         |             Sensitive:  0
  b10-auth                           |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  6
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by stephen):

 * owner:  stephen => jinmei


Comment:

 As suggested, this has been reviewed in several stages:

 '''ee0782b''' to '''848e9d9'''[[BR]]
 OK

 '''848e9d9''' to '''00de2fb'''[[BR]]
 '''query_unittest.cc'''[[BR]]
 A really trivial request: in the list of RRs at the top of the file, some
 are of the form:
 {{{
 const char* rr = "xxxxx..."
 }}}
 ... but most are of are of the form
 {{{
 const char* rr =
     "xxx...."
 }}}
 The contents of the mock zone as a whole would be easier to read if they
 were all of the second form.  (My editor does syntax highlighting so text
 strings stand out in a different colour.  It is relatively easy to focus
 on these strings, especially if they all start at the same offset from the
 left-hand side of the screen.  In the same vein, the contents of the mock
 zone would also be easier to read if the fields in the individual RRs were
 aligned under one another.)

 '''00de2fb''' to '''47bca52'''[[BR]]
 OK

 '''47bca52''' to '''32d9c52'''[[BR]]
 OK

 '''32d9c52''' to '''0d1406f'''[[BR]]
 '''src/bin/auth/tests/query_unittest.cc'''[[BR]]
 The comment above dsBelowDelegation test:  although modified from "This
 one checks a DS record at the apex is not returned, even if it exists" to
 "This one checks a DS record at the apex is not returned", the comment
 still implies that a DS record exists at the apex.

 dsBelowDelegation: although the check in this test is worthwhile, it is
 not clear why the nameserver should return a DS record when one does not
 exist.  If we do test this, we should really test the following cases:
 * No DS record in either child or parent - nothing returned
 * DS record in parent but not child - DS record returned
 * DS record in child but not in parent - nothing returned
 * Parent and child each have a (different) DS record - DS from parent
 returned.

 ... plus of course the case tested in dsNoZone, i.e. DS query received in
 an inapplicable zone.

 Also, what about a test for the case of multiple DS records in the parent
 zone (as could be present in the case of a KSK rollover.)

 '''0d1406f''' to '''efd87c8'''[[BR]]
 It would be helpful to explain how the case of a query for a grandchild
 zone with the server having authority for the child zone differs from the
 case of a query for the child zone with the server having authority for
 the parent.

 '''efd87c8''' to '''efd87c8'''[[BR]]
 dsAtRootZone: as you say, pathological and it doesn't matter in practice
 how we respond.  There is no need to change anything, but it is an
 interesting case.  The rule used for DS records in the code is that a
 query for a zone's DS record is sent to the parent zone.  The behaviour in
 this test implies that the parent of the root zone is the root zone.  On
 the other hand, if we do any search up the tree, we always terminate the
 search at the root zone - which could be interpreted as the root zone
 having no parent. So there appears to be a slight discrepancy in the
 interpretation of the idea of a root zone parent.

 '''efd87c8''' to '''53cde03'''[[BR]]
 '''src/bin/auth/query.h'''[[BR]]
 Header for processDSAtChild() - first line should end "and is called if".

 '''!ChangeLog Entry'''[[BR]]
 This is OK.

-- 
Ticket URL: <http://bind10.isc.org/ticket/1570#comment:10>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development