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      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:10 stephen]:

 Thanks for the review.

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

 This doesn't seem to be specific to this branch, but I have no problem
 with changing the style.  Done in b82c785.  I suspect the style policy
 adopted here will easily be ignored in future updates, however.  I
 added comments about it, but it will still be easily missed.

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

 Tried to clarify it in 42b7d6a.

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

 I'm afraid I don't understand this comment...first, do you mean
 dsBelowDelegationWithDS, not dsBelowDelegation?  Second, whichever
 test of dsBelow... you are referring to, I don't understand "why the
 nameserver should return a DS record when one does not exist".  Or, is
 this comment related to the previous version of the code comment for
 dsBelowDelegation and has been addressed in 42b7d6a?


 > If we do test this, we should really test the following cases:
 > * No DS record in either child or parent - nothing returned

 I agree "No DS record in parent" case should better be tested
 explicitly.  So I've added it 170556d.  I don't think we need to break
 it down further to cases where the server has authority for the child
 or not.  In either case, the server should not even look into the
 child zone, and it's confirmed in the dsAboveDelegation tests.

 In this sense, the following three are already covered in
 dsAboveDelegation and dsAboveDelegationNoData, and I don't think we
 need to explicitly reproduce the specific situations of the child
 zone.  Is that acceptable for you?

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

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

 I think it's out of scope of query logic tests.  As long as the
 underlying data source (its find() method, specifically) handles DS as
 an RRset this should be ensured.  The query logic should be able to
 rely on the abstraction of RRset.

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

 I don't understand this comment...would it be addressed if I added
 comments like this to query.c?
 {{{#!c++
 @@ -416,7 +416,9 @@ Query::process() {
              // If a DS query resulted in delegation, we also need to
 check
              // if we are an authority of the child, too.  If so, we need
 to
              // complete the process in the child as specified in Section
 -            // 2.2.1.2. of RFC3658.
 +            // 2.2.1.2. of RFC3658.  Note that this cannot happen if this
 +            // server is the parent, in which case find() should handle
 it
 +            // as an in-zone search, whether the result is positive or
 not.
              if (qtype_ == RRType::DS() && processDSAtChild()) {
                  return;
              }
 }}}

 > '''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,

 So I didn't update the code for this comment.

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

 Yes, in this case the root zone could be both considered a "parent" or
 "child".  In our code it treats as the parent, but in practice the
 answer would be the same anyway: "returning a no data response".  As
 explicitly tested in dsAtRootWithDS, if the root zone really has a DS,
 the behavior will be different from the implementation that would
 handle it as a child.  In any case that shouldn't matter in practice.

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

 Good catch, fixed.

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

 On second thought, actually, I now think it might be too blunt just
 like the infamous BIND 9 changelog entries.  Here's a revised
 proposed:
 {{{
 374.?   [bug]           jinmei, vorner
         The new query handling module of b10-auth did not handle type DS
         query correctly: It didn't look for it in the parent zone, and
         it incorrectly returned a DS from the child zone if it
         happened to exist there.  Both were corrected, and it now also
         handles the case of having authority for the child and a grand
         ancestor.
         (Trac #1570, git TBD)
 }}}

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