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:12 vorner]:

 > Replying to [comment:8 jinmei]:
 > > - This should be beyond the scope of this ticket anyway, [...]
 >
 > Crap. That complicates things.
 >
 > As I don't see an obvious way how to solve this, I agree we want to put
 it to the dev list to discuss a possible solution. Also, the original
 SQLit3 implementation has this problem.

 Okay, and I know the problem has been in the original implementation.

 > > - In the current implementation the negative proof is always looked
 > >   for when FIND_DNSSEC flag is on.  [...]
 >
 > OK, let's skip it. Anyway, we may want to set the flag depending on if
 the DB is expected to support it/the zone is signed, so this might even be
 modification somewhere else.

 Okay.

 > > - DatabaseClient::Finder::findPreviousName(): what if the
 > >   accessor_->findPreviousName() returns a bogus name (like
 > >   "example..com")?  okay to propagate an exception from the Name
 class?
 >
 > Good catch. I added a test and code to handle it (the code is little bit
 awkward, but commented. It's due to Name(...) throwing quite a bunch of
 different exceptions which don't have common NameError ancestor).

 I'd personally catch isc::Exception as a whole, but I'm okay with your
 code, too.  Also, I've been aware that we probably need some more
 class hierarchies for exceptions (and some exception classes are too
 detailed - we probably only need one "NameError" exception class).
 Hopefully we can revisit this in near future and simplify the catching
 side of code.

 > > - find(): relating to whether to consider the existence of NSEC in the
 > >   accessor's findPreviousName() (see the comment for
 > >   sqlite3_accessor), I'd explicitly expect the case where NSEC doesn't
 > >   exist here rather than letting the accessor implementation check
 > >   that:
 >
 > For one, I copied it from the original code, so I thought there was a
 reason for it protocol-wise.

 As far as I know there's no protocol restriction in this context
 (except the fact that only protocol elements using the notion of
 "previous name" would probably be NSEC (and deprecated NXT)).  I know
 it's derived from the original implementation.  And, as far as I can
 see, the original code doesn't exploit the fact that
 findPreviousName() includes the existence of an NSEC RR in the
 condition of "previous"; the caller doesn't assume the existence.

 > For second, I kind of expected if there are unsigned delegations or
 unsigned glue data, they wouldn't have NSEC even if the zone was properly
 signed. In that case, we should skip them, right? Or am I wrong at the
 assumption? Or you think the higher level should iterate until it finds
 the correct one with NSEC? What if there's no NSEC in the whole zone?
 Wouldn't it be expensive?

 Hmm, for glues (under a zone cut), you're probably right.  They'll not
 have an NSEC (or RRSIG for that matter), and if we didn't include the
 existence of NSEC as part of the "previous name" condition the lookup
 result would be incorrect or the process would be even more expensive.

 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).  Using the existence (or
 non existence) of NSEC is one practical way to implement the concept,
 but at this level of abstraction it seems to me too restrictive to
 assume that way of implementation - a different database backend may
 introduce an explicit notion of zone cut (for efficiency
 reasons or something) and may still be able to meet the requirement of
 findPreviousName() without relying on NSEC.

 So, my revised suggestion is:
 - add the documentation of DatabaseAccessor::findPreviousName() that
   names under a zone cut be excluded (but ones on the parent half of
   cuts must be included).  Also describe that it's up to specific
   derived implementation how to realize that, but one practical way is
   to include the existence of an NSEC RR.
 - in the caller side, don't assume the existence of NSEC even if
   findPreviousName() successfully returns.  If NSEC RR cannot be found
   on the name, either simply skip including NSEC or throw an
   exception, which would subsequently result in SERVFAIL in practice.
   (in the latter case the end result is the same as the current
   implementation, but the assumption is different - it's not
   necessarily an implementation bug, but may be broken or not properly
   signed database).

 I don't know what you mean by "unsigned delegations", btw.  Do you
 mean opt-out?  If so, my understanding is that it's NSEC3 specific.
 Or perhaps are you confused with DS?

 > > - "previous" test: Like the sqlite3 (or accessor in general) case, I
 > >   suspect the "wrap around" behavior is too much at this moment.
 > >   Depending on how we should do this, the test may also have to be
 > >   adjusted.
 >
 > Well, it depends. We might want to prove that something is before the
 origin of zone in some way. In that case, we should get the last one
 somehow and know it was the case before anything. What do you propose to
 do about it? Or that it should never get into this part of code anyway?

 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.  We *might* want that behavior in future for some yet-to-know
 reason, but until then I'd rather keep the interface and
 implementation smaller and simpler.  Currently, and in practice, this
 interface would be used after identifying a zone containing the query
 name, so it shouldn't happen the query name is "smaller" than zone
 origin.  Should that happen, I'd just throw an exception at the moment.

 > > - I don't see why we need to include the condition of "rdtype =
 > >   'NSEC'" in the FIND_PREVIOUS statement.
 >
 > As I noted above, we may want the behaviour or not. I'm not completely
 sure if it is needed, if it isn't, I'm OK with removing it. If it is, I'll
 write a test checking this and add it to the interface requirement.

 Now I see why:-) Let's keep it.

 > > - Do we really need FIND_PREVIOUS_WRAP?  As the code comment seemingly
 > >   indicates, it doesn't seem to be a functional requirement.  And
 > >   since the implementation of this part has somewhat lengthy (although
 > >   it's quite straightforward), I'd not include this behavior at the
 > >   moment until/unless we see the real need for it through out
 > >   experiences.  In either case, I believe the expected behavior in
 > >   this case (i.e., what happens if the given name is "smaller" than
 > >   the zone's origin name) should be clearly documented at the abstract
 > >   interface level.
 >
 > You think it won't be needed? I'm actually OK with both options (better
 documentation/removing it), but I kept it for now, because I think it will
 be needed. Do you see a way how to do it without this support?

 See above.  If you can show a specific usage where we need it, I
 wouldn't object.  If you simply feel we might need it, I'd suggest
 leaving it out of scope (with exception) unless and until we really
 need it.

 Finally, one small question.  What's the purpose of this record?  It's
 not clear from the diff itself (f16de89).

 {{{
 +    {"delegation.example.org.", "DS", "3600", "", "1 RSAMD5 2 abcd"},
 }}}

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