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      |
-------------------------------------+-------------------------------------
Changes (by vorner):

 * owner:  vorner => jinmei


Comment:

 Hello

 Replying to [comment:13 jinmei]:
 > Hmm, that's true for NSEC3, but I see some subtle difference for that
 > case.  NSEC3 RRs effectively belong to a separate name space of the
 > zone, so the rationale why the owner name of an NSEC3 can have other
 > types is different from the reason for RRSIG and NSEC.  In that sense
 > I'd update the code comment to reflect the subtlety:
 > {{{
 >                 // NSEC and RRSIG can coexist with anything, otherwise
 >                 // we've seen something that can't live together with
 potential
 >                 // CNAME or NS
 > }}}
 > (maybe it's sufficient to add NSEC3 in addition to NSEC though)

 I added a comment there why they are ignored right now. I think it is
 enough to just ignore them and not care.

 > But when I first saw the code, it was just confusing; I first thought
 > it was one of the magical class methods of std::map and went to see an
 > online manual (unsuccessfully), then I found the private definition in
 > database.cc with my natural expectation that it should be a symmetric
 > operator, then found it was not, surprised, read the definition more
 > closely, then finally understood it.

 OK, seems like my sense of intuitiveness is not so usual. This code is
 longer, but hopefully more people find it readable.

 > (Not sure what you mean by we can't - C++ allows overloading
 > "operator," but that doesn't matter much in this context anyway) I
 > don't claim the proxy function approach is obviously better.  While
 > personally I prefer that because it's more straightforward and can
 > understand it without a surprise, I also see its drawbacks.  And
 > although I less prefer the overloading approach, I don't see a
 > technical flaw in it (unlike statically initialized namespace-scope
 > objects).  If you still believe the approach with an overloaded
 > operator with taking account into all I explained above, especially
 > that different people have different senses of clarity and it may not
 > be so obvious as the original code author tends to think (to whom it
 > tends to be so obvious because it's based on his/her own sense), I
 > won't fight against that further.  But I have two final
 > suggestions/comments in that case:

 I didn't know the comma can actually be overloaded, but it seems it is
 strange operator then anyway. It could be made to create lists of stuff,
 but it would look even more counter-intuitive in C++ to call it with (this
 one would show the true power of C++, but would have quite hight „What the
 hell“ effect):
 {{{#!C++
 findRRsets(name, (RRType::A(), RRType::NS(), RRType::NSEC()), ...)
 }}}

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

 I did not do any modifications to it on purpose, nor I know of any. I just
 found the original code complicated enough to read it, understand the
 purpose and write it again with the use of the new function instead of
 trying to adapt it, because adapting it would be more work.

 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. I did
 try to write it so it acts the same. I know of one place where I'm not
 sure if I preserved the exact same behaviour ‒ if there are DNAME and NS
 records in non-apex domain, I'm not sure they were rejected before or one
 of them was picked up. Now they should be rejected, as this is invalid.

 I might try to identify the differences in behaviour, if it helps you in
 reviewing it, though (or find out that there are none).

 Replying to [comment:14 jinmei]:
 > Replying to [comment:8 jinmei]:
 >
 > One quick point I clearly need to correct (myself):
 >
 > > > - Is this a question of what's the expected proof of empty
 > > >   non-terminal wildcard match?
 > > >   [...]
 > > >   If so, I'm not 100% sure either, but I guess it's basically no
 > > >   different from other empty non-terminal case.  That is, with
 having
 > > >   wild.*.foo.example.org, the negative proof for a.foo.example.org
 > > >   would be "prev_name(wild.*.foo.example.org) NSEC
 wild.*.foo.example.org..."
 > > >   I'd also suggest you check this behavior with other
 implementation.
 > > >   (Note, however, that you'll need "check-names master ignore;" for
 > > >   BIND 9 to force it to accept empty wilds)
 > >
 > > After an hour of fighting bind9, I didn't manage to make it return
 signatures and NSEC data, so I gave up and implemented it this way. I seem
 to be a bad admin :-|.
 >
 > After playing with existing implementations and re-reading RFC4035, I
 > realized I had been a bit confused when I made this comment.  Using
 > the above example, this should be the case for Section 3.1.3.2 of
 > RFC4035.  And,
 > {{{
 >    o  An NSEC RR proving that there is no exact match for <SNAME,
 >       SCLASS>.
 > }}}
 > should be:
 >
 > {{{
 > prev_name(a.foo.example.org.) NSEC next_name(a.foo.example.org.) ...
 > }}}
 > while
 > {{{
 >    o  An NSEC RR proving that the zone contains no RRsets that would
 >       match <SNAME, SCLASS> via wildcard name expansion.
 > }}}
 > should be
 > {{{
 > prev_name(*.foo.example.org) NSEC wild.*.foo.example.org. ...
 > }}}
 >
 > And, in my understanding of the current branch, the latter is out of
 > scope (deferred to #1244), right?  So, in the context of this branch
 > what should be returned is the former.  Note that
 > prev_name(a.foo.example.org.) may not be equal to
 > wild.*.foo.example.org.  If there's an RR for 1.foo.example.org, for
 > example, that will be the previous name.

 Well, to be consistent with the rest, I think it should return the second
 (eg. prev_name(*.foo.example.org)). Then the WILDCARD_NXRRSET or WILDCARD
 result status indicates that the #1244 should add an NSEC proving that the
 exact match doesn't exist. This is because the Query class won't know the
 exact name for the wildcard, but it will know the exact name for the
 original query. Regarding this, I'm afraid we might need to signal
 WILDCARD_CNAME and WILDCARD_DELEGATION as well for the same reason, but
 I'm not sure, so I left it out for now. It shouldn't be problem to add
 when needed.

 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.

 So, I think this does return what it should. Or did I overlook some
 difference?

 Replying to [comment:15 jinmei]:
 > 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.

 Well, adding a common ancestor should be enough, we wouldn't need to
 change the current throws/catches and future ones could be simplified.

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

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

 I might have confused it with DS records. There's a lot to learn in the
 DNSSEC world for me.

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

 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.

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

 I added a test for it to make sure they are really skipped and commented
 it is a heuristic to skip under-the-cut data.

 > 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"},
 > }}}

 This one tests that DS is actually allowed in the same node as non-apex
 NS. Some tests (like the ones checking delegations) would fail if it
 existed there and wouldn't be allowed. I didn't want to write a whole new
 test for it, just misused one already in place to do it. Should I add some
 kind of comment?

 Thank you

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