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:8 jinmei]:
 > - This should be beyond the scope of this ticket anyway, but I've
 >   noticed our approach to findPreviousName() using the reversed name
 >   may not work for labels using the \ddd notation.  For example,
 >   while \001.example.com. < a.example.com. < \200.example.com.
 >   (where '<' should be considered isc::dns::Name::operator<)
 >   our current DB schema and findPrevious implementation cannot order
 >   them correctly.  We should probably discuss this at the dev list.

 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.

 > - In the current implementation the negative proof is always looked
 >   for when FIND_DNSSEC flag is on.  Assuming it's on when the
 >   corresponding query from the client at the higher level has the DO
 >   bit, the flag would actually be often on (both BIND 9 and unbound
 >   set it by default, as far as I know) while on the other hand many
 >   zones would actually not be signed.  So the attempt of getting the
 >   proof is in many cases unnecessary overhead, which may be too costly
 >   even though the database involved lookup is already generally
 >   expensive.  So, in order to avoid that, I wonder we should probably
 >   introduce the notion of a flag for a zone that identifies whether
 >   it's "signed" or not.  BIND 9 has this flag depending on whether the
 >   zone has a DNSKEY record.  For database backed zones this may not be
 >   trivial because the DNSKEY may be added or deleted bypassing the
 >   BIND 10 API, but maybe we should still consider it even if the
 >   result is an incomplete solution as a compromise.  In any case, this
 >   point would also be beyond the scope of this ticket.  I'm fine with
 >   skipping this point for now.

 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.

 > '''auth/query.cc'''
 >
 > Do we need a test case for the "default"?

 I don't think so. This is only temporary, so it compiles. #1244 should
 handle it (part split off this ticket) and we can't really put the new
 code into use without it. Writing test for this temporary behaviour for
 the next week or so would be a waste of effort.

 > - According to lcov there are some untested code fragments:

 I added some more tests which should test it.

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

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

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

 > - Do you mean we might want to differentiate empty non terminal
 >   (especially with a proof by NSEC) from "normal" NXRRSET cases?

 Well, it kind of depends if we need to add another NSEC to prove
 nonexistence of something else. But I guess we don't so it's OK. I'm still
 not completely clear of how to prove nonexistence of each single scenario,
 I'll need to draw some pictures and diagrams if I take #1244.

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

 > '''sqlite3_accessor: findPreviousName()'''
 > - when we solely expect SQLITE_OK, I wouldn't bother to get the actual
 >   return code (consuming a mutable variable).  I would also use
 >   sqlite3_errmsg() to provide more informative error message.  That
 >   is, instead of

 OK, but I don't think the mutable variable is a problem to anything. This
 might be slightly simpler code, the original one slightly more consistent,
 but I don't care here.

 > - I don't see why we need to include the condition of "rdtype =
 >   'NSEC'" in the FIND_PREVIOUS statement.  In fact, at least the
 >   current set of unittests all passes even without this condition.
 >   The notion of "previous name" is independent of whether the name has
 >   NSEC or not (although in practice the only use of it may be DNSSEC
 >   related), and it's also more consistent to the "keep it dumb" design
 >   principle to exclude this condition.  So, I'd suggest simplifying
 >   the statement by removing "rdtype = 'NSEC'".  On the other hand, if
 >   there is a specific reason why we really need to include this
 >   condition, we should document it as part of the abstract interface
 >   requirement.  The same discussion applies to FIND_PREVIOUS_WRAP, but
 >   see the next bullet first.

 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.

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

 Thank you & sorry for taking so long

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