BIND 10 #1177: NSEC support in new data source

[BIND 10 Development]

#1177: NSEC support in new data source
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:  major  |  Sprint-20110927
                  Component:  data   |            Resolution:
  source                             |             Sensitive:  0
                   Keywords:         |           Sub-Project:  DNS
            Defect Severity:  N/A    |  Estimated Difficulty:  5
Feature Depending on Ticket:         |           Total Hours:  0
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:17 vorner]:

 The latest code looks mostly okay.  I still have a few issues to
 discuss (thanks for your patience:-).

 Regarding these two points:

 > > So, now I see the need for it.  On thinking further with this in mind,
 > > however, it seems the essential point is that findPreviousName()
 > > should search names above and the parent half of zone cuts
 > > (i.e. excluding anything below a zone cut).  [...]
 >
 > I added a comment to the documentation saying so and modified the text
 of the exception thrown to say this might be a problem with the accessor
 or the zone. DataSourceError is probably correct exception for both these
 cases.

 > > I'd first point out that this behavior doesn't exist the current
 > > implementation, so this is adding a new behavior without a clear
 > > reason. [...]
 >
 > OK, changing to throwing an exception, because I don't know of specific
 scenario when this would be needed. If it turns out to be needed, it
 should be possible to extract it from the history.

 I'm afraid we need some clarification even though it may not have to be
 now.

 There are several possible error cases:

 1. The input is bogus, and smaller than the zone's origin name
 2. The zone is not intended to be signed in the first place (so
    there's no NSEC)
 3. The zone is supposed to be signed but is somehow broken, and NSEC
    RR does not exist on a "previous name"
 4. (Similar to 2, but) The specific accessor class doesn't support
    this feature

 Case 1 shouldn't happen in the normal query processing context.  In
 this case we should throw an exception that clearly indicates a
 program error such as InvalidParameter.

 Case 2 shouldn't trigger an exception because it's a completely valid
 (and actually today's most common) case.

 In case 3, we should probably thrown DataSourceError, which would
 subsequently result in SERVFAIL to the query.

 Case 4 should probably be handled like case 2.

 To implement this properly, I guess we'll need a notion of a flag that
 indicates whether a zone is signed (as we discussed in this ticket),
 and the code logic would be:

 {{{
     // below, zone_is_signed is false in case of 2 and 4
     if (dnssec_required && zone_is_signed) {
        prev_name = findPreviousName();
        nsec = getRRsets(prev_name, NSEC);
        if (!nsec) {
          throw(DataSourceError); // case 3
        }
        return (nsec); // good case for a signed zone
     } else {
        // case 2 or 4, don't have to do anything.
     }
 }}}

 And, in the implementation of findPreviousName(), we'd return a (new)
 exception "NotFound", which is expected to cover case 1 should that
 ever happen.  Case 2 would also cause this error, but we'd generally
 expect this case is avoided at a higher level (like the above sample
 code).

 But the notion of "is_signed" flag is beyond the scope of this ticket,
 I cannot implement this idea yet.  So, as a short term compromise, I'd
 suggest the following logic for now:

 {{{
     if (dnssec_required) {
        try {
            prev_name = findPreviousName();
            if (!nsec) {
              // depending on the implementation of findPreviousName(),
              // this is either case 2 or 3, but we cannot tell.  Since
              // we cannot SERVFAIL for case 2, we ignore that and simply
              // skip including NSEC.
              ;
            } else {
              return (nsec);
            }
       } catch (const NotImplemented&) {
            // This is case 4, or if the implementation of
            // findPreviousName() relies on NSEC, either 2 or 3.  For
            // the same reason as above, we'll ignore the exception.
            ;
       }
     }
 }}}

 And, in our sqlite3's findPreviousName(), if it cannot find the
 requested previous name with NSEC, it will throw NotImplemented.  It's
 not a good exception for cases 2 or 3, but as a short term workaround
 it would be okay.  Note also that this implementation would be very
 expensive for negative responses with today's common zones (i.e.,
 unsigned ones), because all negative processing involves exception
 handling.  Again, as a short term workaround it should be acceptable.
 Case 1 shouldn't happen in this context, so we ignore it for now.

 If the above makes sense, my latest suggestion is to slightly adjust
 the code to implement the latter logic (we probably need a couple of
 additional tests), create a new ticket for the notion of "is signed"
 flag of a zone, and refer to this discussion with a note that this
 issues should be solved in that ticket, too.

 Also, if you do this it will also be solved but I found a minor bug in
 the current code for the "empty non-terminal asterisk" case.

 {{{
                         if (dnssec_data) {
                             // Which one should contain the NSEC record?
                             const Name
 coverName(findPreviousName(Name(wildcard)));
                             // Get the record and copy it out
                             found = getRRsets(coverName.toText(),
 NSEC_TYPES(),
 ...
 }}}

 sqlite3 version of findPreviousName() would throw for an unsigned
 zone, so we need to protect the case with a try-catch block.

 And one more thing:

 - In sqlite3_accessor_unittest, I'd add the case for "com.example"
   (i.e. qname is smaller than the zone origin).  Also for the database
   test.

 Finally, responses to a couple of points of your comment:

 > Replying to [comment:13 jinmei]:

 > > That said, rewinding everything at this point may not be an effective
 > > way of development.  If you can list up things you changed with why,
 > > and if it's reasonable, that's probably sufficient.
 >
 > I probably didn't make myself clear enough. [...]
 >
 > So I admit that there might be some slight changes in observable
 behaviour in corner cases we did not think about before nor now, but I
 think the chance of the behaviour being wrong is the same with new and old
 version of code, simply because they are the cases we didn't think about.

 Hmm, I'm not entirely happy about this in terms of keeping the
 confidence about the code accuracy and quality, but I can live with
 that.  Hopefully we can identify and fix hidden bugs (either original
 or as a result of regression) in #1198.

 > > One quick point I clearly need to correct (myself): [...]
 >
 > Well, to be consistent with the rest, I think it should return the
 > second (eg. prev_name(*.foo.example.org)). [...]

 > It should look the same as with real WILDCARD_NXRRSET, were it returns
 NSEC to prove this node doesn't contain the required RRset and #1244 will
 add the nonexistence of the original query.

 Ah, okay, I was confused.  Then the code and test are okay (except the
 above bug regarding exception).  This also makes me wonder we should
 describe in ZoneFinder::find() more details about what's expected in
 various negative cases especially when FIND_DNSSEC is on.

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