BIND 10 #1177: NSEC support in new data source
BIND 10 Development
do-not-reply at isc.org
Wed Sep 14 06:00:32 UTC 2011
#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):
Overall the code looks okay, but (as usual) I have some points to
discuss.
I've also made some editorial/style changes directory on the branch.
'''Higher level points'''
- 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.
- 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.
'''auth/query.cc'''
Do we need a test case for the "default"?
'''zone.h'''
- "the DNSSEC/NSEC order" sounds a bit awkward:
{{{
/// Gets the previous name in the DNSSEC/NSEC order. This can be used
/// to find the correct NSEC records for proving nonexistence
/// of domains.
}}}
At least I've never heard the phrase of "NSEC order". To be fully
pedantic, it would be the "canonical order (as specified in RFC
4034)"; to be a bit informal (I'm okay with it) I'd simply say "the
DNSSEC order". The same comment applies to several other parts of
the diff.
'''database.cc/h'''
- According to lcov there are some untested code fragments:
{{{
} catch (const rdata::InvalidRdataText& ird) {
isc_throw(DataSourceError, "Invalid rdata in database for " <<
name << ": " << columns[DatabaseAccessor::
RDATA_COLUMN]);
[...]
if (cni != found.second.end() &&
type != RRType::CNAME()) {
result_rrset = cni->second;
result_status = CNAME;
} else if (nsi != found.second.end()) {
result_rrset = nsi->second;
result_status = DELEGATION;
[...]
// The previous doesn't contain NSEC, bug?
isc_throw(DataSourceError, "No NSEC in " +
}}}
- DatabaseAccessor::findPreviousName(), documentation: I suspect it's
not so obvious why we need to use the reversed form, or even what's
the "reversed form" in the first place. Some more details about the
rationale with a couple of examples would help.
- DatabaseAccessor::findPreviousName(): what if the name is out of
zone and there's actually no "previous name"?
- DatabaseClient::Finder::findPreviousName(): what if the
accessor_->findPreviousName() returns a bogus name (like
"example..com")? okay to propagate an exception from the Name class?
- 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:
{{{
} else {
// The previous doesn't contain NSEC, bug?
isc_throw(DataSourceError, "No NSEC in " +
coverName.toText() + ", but it was "
"returned as previous - "
"accessor error?");
}
}}}
and return NXDOMAIN/NXRRSET without a proof.
- find(): maybe we should log the event here?
{{{
catch (const isc::NotImplemented&) {
// Well, they want DNSSEC, but there is no available.
// So we don't provide anything.
}
}}}
'''database_unittest'''
- Is this a question of what's the expected proof of empty
non-terminal wildcard match?
{{{
// FIXME: What should be returned in this case? How does the
// DNSSEC logic handle it?
}}}
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)
- If I understand the question correctly...
{{{
// FIXME: Is the nonexistence of this node really the correct proof
// we need?
}}}
...I believe the answer is yes. The NSEC for "l.example.org." whose
next name is "empty.nonterminal.example.org" proves that both a node
"nonterminal.example.org" exists and it's empty. You can also see
how BIND 9 responds to this case by "dig @ns1.jinmei.org
pt.y.jinmei.org +dnssec". (If you send it to ns.jinmei.org, it's
BIND 10, and it doesn't seem to handle this case correctly:-). Like
the previous case, when you are not sure about a certain corner case
I'd strongly suggest seeing how other implementation does. Often
these things were already considered and handled, and we can simply
steal their lesson rather than wasting our time. In some rare cases
we find a bug of the other implementation, which is also not a bad
thing.
- Do you mean we might want to differentiate empty non terminal
(especially with a proof by NSEC) from "normal" NXRRSET cases?
{{{
doFindTest(*finder, isc::dns::Name("nonterminal.example.org."),
isc::dns::RRType::TXT(), isc::dns::RRType::NSEC(),
this->rrttl_,
ZoneFinder::NXRRSET, // FIXME: Do we want to have specific
code?
this->expected_rdatas_, this->expected_sig_rdatas_,
Name("l.example.org."), ZoneFinder::FIND_DNSSEC);
}}}
If so, I'm not sure. BIND 9 seems to have some intent to
differentiate these two cases, but it doesn't actually seem to
provide different behaviors for these cases.
- "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.
'''sqlite3_accessor (general)'''
- the different between FIND_PREVIOUS and FIND_PREVIOUS_WRAP doesn't
look so obvious. Some comments would help.
- comment for the wrong line?
{{{
"WHERE zone_id=?1 AND rdtype = 'NSEC' AND "
"rname < $2 ORDER BY rname DESC LIMIT 1", // FIND_PREVIOUS_WRAP
"SELECT name FROM records " <== the comment should be here
}}}
- convertToPlainCharInternal(): this function was moved to
SQLite3Accessor::Context() mainly for omitting specifying 'db'.
Now this advantage is effectively gone, I think it's even better to
move it back from SQLite3Accessor::Context(), simply naming it
convertToPlainChar().
'''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
{{{
int rc = sqlite3_bind_int(dbparameters_->statements_[FIND_PREVIOUS],
1,
zone_id);
if (rc != SQLITE_OK) {
isc_throw(SQLite3Error, "Could not bind zone ID " << zone_id <<
" to SQL statement (find previous)");
}
}}}
I'd do this:
{{{
if (sqlite3_bind_int(...) != SQLITE_OK) {
isc_throw(SQLite3Error, "Could not bind zone ID " << zone_id <<
" to SQL statement (find previous): " <<
sqlite3_errmsg(dbparameters_->db_));
}
}}}
- 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.
- 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.
- Do you mean "Could not get..." here?
{{{
if (rc != SQLITE_ROW && rc != SQLITE_DONE) {
// Some kind of error
isc_throw(SQLite3Error, "Could get data for previous name");
}
}}}
'''sqlite3_accessor_unittest'''
- I'd test the case where it would result in the largest name.
- I'd test the case for an out of domain name (which would be placed
before the origin name, such as previous name of "example.com" in
zone "example.org". Note that depending on whether to wrap around,
the expected result will be different.
- I'd check (confirm) the comparison is case insensitive.
- findPreviousNoData: this doesn't seem to be a good test case for the
intended scenario (because the input data is bogus not only because
it's missing NSEC but also is out of zone). I'd use something like
"com.example.sql2.www.", which would make the test succeed if the
"previous" name had NSEC. But note also that I personally suspect
we shouldn't include the consideration for NSEC at this level in the
first place. That discussion may affect the meaning of this test
more fundamentally.
--
Ticket URL: <http://bind10.isc.org/ticket/1177#comment:8>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list