BIND 10 #1431: NSEC3: closest provable encloser proof

[BIND 10 Development]

#1431: NSEC3: closest provable encloser proof
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  stephen                            |                Status:  reviewing
                       Type:         |             Milestone:
  enhancement                        |  Sprint-20120124
                   Priority:  major  |            Resolution:
                  Component:         |             Sensitive:  0
  Unclassified                       |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  6
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:  NSEC3  |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:16 vorner]:

 Thanks for the prompt review.

 > The idea of passing a hint through the code seems very reasonable (so we
 won't need to change the interface in future, if we're not going to use
 the hint itself for now too much). However, looking at the code and the
 interface, the fact we are returning the empty NSEC3 RRset to signal it is
 signed with NSEC3 and pass the hint feels like a misuse. It is both not
 obvious and inconsistent with what the NSEC case does.

 Point taken.  I admit it's not clean or intuitive to use the returned
 RRset as an optimization hint.

 > What I would propose would be extending the FindResult to include the
 following:
 >  * Some flags (eg. „zone is signed“, „it is signed with NSEC3“, possibly
 removing the flag of „The result is wildcard“ from the main status and
 including it as a separate flag).
 >  * A shared_ptr<Hints> (or scoped_ptr, or whatever). The Hints class
 would be either a base class or only declared and a implementation-
 dependant definition would be present in each different data source (the
 first one needs dynamic_cast on the accepting end, but is „by the
 handbook“, the other is little bit misuse, having multiple real types with
 the same name, so it's effectively a hidden anonymous pointer).

 I also agree it makes sense to introduce "flags" information.  In fact
 I've been wondering whether the information of "wildcard match" should
 be integrated to the result code (because it will make it harder to
 combine processing common to wildcard cases, such as for
 WILDCARD_CNAME and normal WILDCARD).

 As for the explicit Hints class, on thinking about it further, I guess
 we might (eventually) do something like this:

 - Define FindContext (or "Hints", naming is not important).  It's a
   base class, and actual data source implementation would encapsulate
   useful internal information in it for possible post-find processing
 - ZoneFinder::find() can return (a (shared) pointer to) FindContext in
   FindResult if it want to allow optimization in later processing.
   It's optional and can be NULL.
 - FindContext has methods like "findNSEC3()" or "findAdditional()",
   which are generally optimized version of ZoneFinder's counter part,
   and should be generally optimized exploiting the encapsulated
   information linked to its internal details.
 - The application may use the FindContext version of these methods if
   they want and it's returned in the hope that it may be more
   efficient.

 This way we can also avoid the need for down cast (dynamic_cast) and
 can make the interface type-safer.

 Would something like this make sense?  If so, what I'd propose for
 this ticket is:

 - introduce the flag field to FindResult and update the find()
   interface so that if the zone is signed with NSEC/NSEC3 the
   corresponding flags are set, and same for wildcard.
 - deprecate WILDCARD_xxx and have the caller refer to this flag
 - remove the idea of returning NSEC3 RRset from find() and allowing
   the caller to use it for findNSEC3() for now.  The idea of
   FindContext will be big enough (and non urgent), so it's probably
   better to postpone it.

 > Also, I'd have few code-level details:
 >
 > Indentation:
 > {{{
 > +        /// TBD
 > +       virtual FindNSEC3Result
 > +        findNSEC3(const isc::dns::Name& name, bool recursive,
 > +                  const isc::dns::ConstRRsetPtr known_encloser);
 > +
 > }}}

 Oops, actually this "TBD" should have been updated (with indentation
 fix as a side effect), but I forgot to commit/push it.  Now it should
 look okay.

 > I believe it expects there's NSEC3PARAMS RR in the apex, not NSEC3,
 because the matching NSEC3 is hard to guess without knowing the salt and
 algorithm:
 > {{{#!c++
 >     /// In general, this method expects the zone is properly signed with
 NSEC3
 >     /// RRs.  Specifically, it assumes at least the apex node has a
 matching
 >     /// NSEC3 RR.  So the search must always succeed; if the assumption
 isn't
 >     /// met, \c DataSourceError exception will be thrown
 > }}}

 This was indeed (intended to be) about NSEC3.  The point in this
 context is that in the recursive mode the search should always
 succeed.  I omitted that the parameters should be available, too,
 because it's already noted in the documentation, but for clarity I
 added that too.

 > It should be said that the known_encloser should be an RRset coming from
 the same data source as a result of find, not that a user would be allowed
 to construct it directly (eg. the caller shouldn't guess a name, create
 the RRset himself and pass it, because the findNSEC3 method might expect
 it to be a derived class with hints).

 As I proposed above, I think I'd cancel the idea of using RRset for
 this purpose for now, so I didn't update the doc on this point.

 > The !DataSourceError exception is usually raised on database errors and
 such, as well as bad data. However, this comment indicates that if the
 exception is raised, it always means it is improperly signed, but it might
 be a low-level database error in reality:
 >
 > {{{#!c++
 >     /// \exception DataSourceError The zone isn't properly signed with
 NSEC3
 >     /// (NSEC3 parameters cannot be found; no NSEC3s are available,
 etc).
 > }}}

 Okay, updated.  Hopefully the new version addresses the concern.

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