BIND 10 trac1177, updated. 06a24c688282b61dd2ce5b6c00608bee34ae3563 [1177] Don't assume findPreviousName contains NSEC
BIND 10 source code commits
bind10-changes at lists.isc.org
Fri Sep 23 11:22:40 UTC 2011
The branch, trac1177 has been updated
via 06a24c688282b61dd2ce5b6c00608bee34ae3563 (commit)
via b902e70583a9dfb1ee410e297e2da4c8b944ba8d (commit)
via 09349cf206ee9e68618713b97e621b7ef2a6c0a9 (commit)
via ff1bd2a00278bc753a7d035fd5020ff936df1882 (commit)
via c89f3a2f43fd7fe70bcb199fad0ccf94364b1ebe (commit)
from 4c86025464db4603ec07490169aaf4b77868057b (commit)
Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.
- Log -----------------------------------------------------------------
commit 06a24c688282b61dd2ce5b6c00608bee34ae3563
Author: Michal 'vorner' Vaner <michal.vaner at nic.cz>
Date: Fri Sep 23 12:16:36 2011 +0200
[1177] Don't assume findPreviousName contains NSEC
So don't say it's necessary bug, it might be badly signed zone as well.
commit b902e70583a9dfb1ee410e297e2da4c8b944ba8d
Author: Michal 'vorner' Vaner <michal.vaner at nic.cz>
Date: Fri Sep 23 11:48:43 2011 +0200
[1177] Test for skipping glue data
commit 09349cf206ee9e68618713b97e621b7ef2a6c0a9
Author: Michal 'vorner' Vaner <michal.vaner at nic.cz>
Date: Fri Sep 23 11:35:12 2011 +0200
[1177] Don't wrap around in previous
commit ff1bd2a00278bc753a7d035fd5020ff936df1882
Author: Michal 'vorner' Vaner <michal.vaner at nic.cz>
Date: Fri Sep 23 11:15:39 2011 +0200
[1177] Don't overload operator+
It seems I have a strange sense of what is intuitive, not everybody
finds it as intuitive as me O:-).
commit c89f3a2f43fd7fe70bcb199fad0ccf94364b1ebe
Author: Michal 'vorner' Vaner <michal.vaner at nic.cz>
Date: Fri Sep 23 11:14:20 2011 +0200
[1177] Comment about ignoring NSEC3 in check
-----------------------------------------------------------------------
Summary of changes:
src/lib/datasrc/database.cc | 94 ++++++++++++-------
src/lib/datasrc/database.h | 13 ++-
src/lib/datasrc/sqlite3_accessor.cc | 44 ++-------
src/lib/datasrc/tests/database_unittest.cc | 9 +--
src/lib/datasrc/tests/sqlite3_accessor_unittest.cc | 13 ++--
src/lib/datasrc/zone.h | 5 +-
6 files changed, 89 insertions(+), 89 deletions(-)
-----------------------------------------------------------------------
diff --git a/src/lib/datasrc/database.cc b/src/lib/datasrc/database.cc
index 50b2a4d..591099c 100644
--- a/src/lib/datasrc/database.cc
+++ b/src/lib/datasrc/database.cc
@@ -253,6 +253,9 @@ DatabaseClient::Finder::getRRsets(const string& name, const WantedTypes& types,
// NSEC and RRSIG can coexist with anything, otherwise
// we've seen something that can't live together with potential
// CNAME or NS
+ //
+ // NSEC3 lives in separate namespace from everything, therefore
+ // we just ignore it here for these checks as well.
seen_other = true;
}
} catch (const InvalidRRType&) {
@@ -303,20 +306,47 @@ DatabaseClient::Finder::hasSubdomains(const std::string& name) {
// Some manipulation with RRType sets
namespace {
-// To conveniently put the RRTypes into the sets. This is not really clean
-// design, but it is hidden inside this file and makes the calls much more
-// convenient.
-//
-// While this is not straightforward use of the + operator, some mathematical
-// conventions do allow summing sets with elements (usually in commutative
-// way, we define only one order of the operator parameters, as the other
-// isn't used).
-//
-// This arguably produces more readable code than having bunch of proxy
-// functions and set.insert calls scattered through the code.
-std::set<RRType> operator +(std::set<RRType> set, const RRType& type) {
- set.insert(type);
- return (set);
+// Bunch of functions to construct specific sets of RRTypes we will
+// ask from it.
+typedef std::set<RRType> WantedTypes;
+
+const WantedTypes&
+NSEC_TYPES() {
+ static bool initialized(false);
+ static WantedTypes result;
+
+ if (!initialized) {
+ result.insert(RRType::NSEC());
+ initialized = true;
+ }
+ return (result);
+}
+
+const WantedTypes&
+DELEGATION_TYPES() {
+ static bool initialized(false);
+ static WantedTypes result;
+
+ if (!initialized) {
+ result.insert(RRType::DNAME());
+ result.insert(RRType::NS());
+ initialized = true;
+ }
+ return (result);
+}
+
+const WantedTypes&
+FINAL_TYPES() {
+ static bool initialized(false);
+ static WantedTypes result;
+
+ if (!initialized) {
+ result.insert(RRType::CNAME());
+ result.insert(RRType::NS());
+ result.insert(RRType::NSEC());
+ initialized = true;
+ }
+ return (result);
}
}
@@ -341,8 +371,6 @@ DatabaseClient::Finder::find(const isc::dns::Name& name,
// In case we are in GLUE_OK mode and start matching wildcards,
// we can't do it under NS, so we store it here to check
isc::dns::RRsetPtr first_ns;
- // This is used at multiple places
- static const WantedTypes nsec_types(WantedTypes() + RRType::NSEC());
// First, do we have any kind of delegation (NS/DNAME) here?
const Name origin(getOrigin());
@@ -360,10 +388,7 @@ DatabaseClient::Finder::find(const isc::dns::Name& name,
for (int i(remove_labels); i > 0; --i) {
Name superdomain(name.split(i));
// Look if there's NS or DNAME (but ignore the NS in origin)
- static const WantedTypes delegation_types(WantedTypes() +
- RRType::DNAME() +
- RRType::NS());
- found = getRRsets(superdomain.toText(), delegation_types,
+ found = getRRsets(superdomain.toText(), DELEGATION_TYPES(),
i != remove_labels);
if (found.first) {
// It contains some RRs, so it exists.
@@ -409,9 +434,9 @@ DatabaseClient::Finder::find(const isc::dns::Name& name,
// It is special if there's a CNAME or NS, DNAME is ignored here
// And we don't consider the NS in origin
- static const WantedTypes final_types(WantedTypes() + RRType::CNAME() +
- RRType::NS() + RRType::NSEC());
- found = getRRsets(name.toText(), final_types + type, name != origin);
+ WantedTypes final_types(FINAL_TYPES());
+ final_types.insert(type);
+ found = getRRsets(name.toText(), final_types, name != origin);
records_found = found.first;
// NS records, CNAME record and Wanted Type records
@@ -460,11 +485,8 @@ DatabaseClient::Finder::find(const isc::dns::Name& name,
const string wildcard("*." + superdomain.toText());
const string construct_name(name.toText());
// TODO What do we do about DNAME here?
- static const WantedTypes wildcard_types(WantedTypes() +
- RRType::CNAME() +
- RRType::NS() +
- RRType::NSEC());
- found = getRRsets(wildcard, wildcard_types + type, true,
+ // The types are the same as with original query
+ found = getRRsets(wildcard, final_types, true,
&construct_name);
if (found.first) {
if (first_ns) {
@@ -514,7 +536,7 @@ DatabaseClient::Finder::find(const isc::dns::Name& name,
// However, we need to get the RRset in the
// name of the wildcard, not the constructed
// one, so we walk it again
- found = getRRsets(wildcard, nsec_types,
+ found = getRRsets(wildcard, NSEC_TYPES(),
true);
result_rrset =
found.second.find(RRType::NSEC())->
@@ -545,7 +567,7 @@ DatabaseClient::Finder::find(const isc::dns::Name& name,
const Name
coverName(findPreviousName(Name(wildcard)));
// Get the record and copy it out
- found = getRRsets(coverName.toText(), nsec_types,
+ found = getRRsets(coverName.toText(), NSEC_TYPES(),
true);
const FoundIterator
nci(found.second.find(RRType::NSEC()));
@@ -553,11 +575,12 @@ DatabaseClient::Finder::find(const isc::dns::Name& name,
result_status = WILDCARD_NXRRSET;
result_rrset = nci->second;
} else {
- // The previous doesn't contain NSEC, bug?
+ // The previous doesn't contain NSEC.
+ // Badly signed zone or a bug?
isc_throw(DataSourceError, "No NSEC in " +
coverName.toText() + ", but it was "
"returned as previous - "
- "accessor error?");
+ "accessor error? Badly signed zone?");
}
}
break;
@@ -589,18 +612,19 @@ DatabaseClient::Finder::find(const isc::dns::Name& name,
// Which one should contain the NSEC record?
const Name coverName(findPreviousName(name));
// Get the record and copy it out
- found = getRRsets(coverName.toText(), nsec_types, true);
+ found = getRRsets(coverName.toText(), NSEC_TYPES(), true);
const FoundIterator
nci(found.second.find(RRType::NSEC()));
if (nci != found.second.end()) {
result_status = NXDOMAIN;
result_rrset = nci->second;
} else {
- // The previous doesn't contain NSEC, bug?
+ // The previous doesn't contain NSEC.
+ // Badly signed zone or a bug?
isc_throw(DataSourceError, "No NSEC in " +
coverName.toText() + ", but it was "
"returned as previous - "
- "accessor error?");
+ "accessor error? Badly signed zone?");
}
}
catch (const isc::NotImplemented&) {
diff --git a/src/lib/datasrc/database.h b/src/lib/datasrc/database.h
index 1284f57..5e66f33 100644
--- a/src/lib/datasrc/database.h
+++ b/src/lib/datasrc/database.h
@@ -487,13 +487,18 @@ public:
* and the DNSSEC order correspond (eg. org.example.a is followed
* by org.example.a.b which is followed by org.example.b, etc).
* \param zone_id The zone to look through.
- * \return The previous name, or the last name in the zone, if there's
- * no previous one (including out-of-zone cases).
- * \note This function must return previous/last name even in case
+ * \return The previous name.
+ * \note This function must return previous name even in case
* the queried rname does not exist in the zone.
+ * \note This method must skip under-the-zone-cut data (glue data).
+ * This might be implemented by looking for NSEC records (as glue
+ * data don't have them) in the zone or in some other way.
*
* \throw DataSourceError if there's a problem with the database.
- * \throw NotImplemented if this database doesn't support DNSSEC.
+ * \throw NotImplemented if this database doesn't support DNSSEC
+ * or there's no previous name for the queried one (the NSECs
+ * might be missing or the queried name is less or equal the
+ * apex of the zone).
*/
virtual std::string findPreviousName(int zone_id,
const std::string& rname) const = 0;
diff --git a/src/lib/datasrc/sqlite3_accessor.cc b/src/lib/datasrc/sqlite3_accessor.cc
index ed2f84e..22e1f30 100644
--- a/src/lib/datasrc/sqlite3_accessor.cc
+++ b/src/lib/datasrc/sqlite3_accessor.cc
@@ -48,8 +48,7 @@ enum StatementID {
DEL_RECORD = 8,
ITERATE = 9,
FIND_PREVIOUS = 10,
- FIND_PREVIOUS_WRAP = 11,
- NUM_STATEMENTS = 12
+ NUM_STATEMENTS = 11
};
const char* const text_statements[NUM_STATEMENTS] = {
@@ -72,17 +71,13 @@ const char* const text_statements[NUM_STATEMENTS] = {
"SELECT rdtype, ttl, sigtype, rdata, name FROM records " // ITERATE
"WHERE zone_id = ?1 ORDER BY name, rdtype",
/*
- * The ones for finding previous name. The first of them takes
- * biggest smaller than something (therefore previous to the something),
- * the second takes biggest (used in case when there's no previous,
- * to "wrap around").
+ * This one looks for previous name with NSEC record. It is done by
+ * using the reversed name. The NSEC is checked because we need to
+ * skip glue data, which don't have the NSEC.
*/
"SELECT name FROM records " // FIND_PREVIOUS
"WHERE zone_id=?1 AND rdtype = 'NSEC' AND "
- "rname < $2 ORDER BY rname DESC LIMIT 1",
- "SELECT name FROM records " // FIND_PREVIOUS_WRAP
- "WHERE zone_id = ?1 AND rdtype = 'NSEC' "
- "ORDER BY rname DESC LIMIT 1"
+ "rname < $2 ORDER BY rname DESC LIMIT 1"
};
struct SQLite3Parameters {
@@ -702,31 +697,10 @@ SQLite3Accessor::findPreviousName(int zone_id, const std::string& rname)
sqlite3_reset(dbparameters_->statements_[FIND_PREVIOUS]);
if (rc == SQLITE_DONE) {
- // Nothing previous, wrap around (is it needed for anything?
- // Well, just for completeness)
- sqlite3_reset(dbparameters_->statements_[FIND_PREVIOUS_WRAP]);
- sqlite3_clear_bindings(dbparameters_->statements_[FIND_PREVIOUS_WRAP]);
-
- if (sqlite3_bind_int(
- dbparameters_->statements_[FIND_PREVIOUS_WRAP], 1, zone_id) !=
- SQLITE_OK) {
- isc_throw(SQLite3Error, "Could not bind zone ID " << zone_id <<
- " to SQL statement (find previous wrap): " <<
- sqlite3_errmsg(dbparameters_->db_));
- }
-
- rc = sqlite3_step(dbparameters_->statements_[FIND_PREVIOUS_WRAP]);
- if (rc == SQLITE_ROW) {
- // We found it
- result =
- convertToPlainChar(sqlite3_column_text(dbparameters_->
- statements_[FIND_PREVIOUS_WRAP], 0), dbparameters_->db_);
- }
- sqlite3_reset(dbparameters_->statements_[FIND_PREVIOUS_WRAP]);
- if (rc == SQLITE_DONE) {
- // No NSEC records, this DB doesn't support DNSSEC
- isc_throw(isc::NotImplemented, "The zone doesn't support DNSSEC");
- }
+ // No NSEC records here, this DB doesn't support DNSSEC or
+ // we asked before the apex
+ isc_throw(isc::NotImplemented, "The zone doesn't support DNSSEC or "
+ "query before apex");
}
if (rc != SQLITE_ROW && rc != SQLITE_DONE) {
diff --git a/src/lib/datasrc/tests/database_unittest.cc b/src/lib/datasrc/tests/database_unittest.cc
index f9c81e7..69151ce 100644
--- a/src/lib/datasrc/tests/database_unittest.cc
+++ b/src/lib/datasrc/tests/database_unittest.cc
@@ -175,8 +175,6 @@ const char* const TEST_RECORDS[][5] = {
{"bao.example.org.", "NSEC", "3600", "", "wild.*.foo.*.bar.example.org. NSEC"},
{"*.cnamewild.example.org.", "CNAME", "3600", "", "www.example.org."},
{"*.nswild.example.org.", "NS", "3600", "", "ns.example.com."},
- // For finding previous, this one is the last one in the zone
- {"zzz.example.org.", "NSEC", "3600", "", "example.org NSEC"},
// For NSEC empty non-terminal
{"l.example.org.", "NSEC", "3600", "", "empty.nonterminal.example.org. NSEC"},
{"empty.nonterminal.example.org.", "A", "3600", "", "192.0.2.1"},
@@ -558,9 +556,7 @@ public:
if (id == -1) {
isc_throw(isc::NotImplemented, "Test not implemented behaviour");
} else if (id == 42) {
- if (rname == "org.example.") {
- return ("zzz.example.org.");
- } else if (rname == "org.example.nonterminal.") {
+ if (rname == "org.example.nonterminal.") {
return ("l.example.org.");
} else if (rname == "org.example.www2." ||
rname == "org.example.www1.") {
@@ -2331,9 +2327,6 @@ TYPED_TEST(DatabaseClientTest, previous) {
EXPECT_EQ(Name("www.example.org."),
finder->findPreviousName(Name("www2.example.org.")));
- // Check wrap around
- EXPECT_EQ(Name("zzz.example.org."),
- finder->findPreviousName(Name("example.org.")));
// Check a name that doesn't exist there
EXPECT_EQ(Name("www.example.org."),
finder->findPreviousName(Name("www1.example.org.")));
diff --git a/src/lib/datasrc/tests/sqlite3_accessor_unittest.cc b/src/lib/datasrc/tests/sqlite3_accessor_unittest.cc
index 4bcf252..87708c7 100644
--- a/src/lib/datasrc/tests/sqlite3_accessor_unittest.cc
+++ b/src/lib/datasrc/tests/sqlite3_accessor_unittest.cc
@@ -360,12 +360,7 @@ TEST_F(SQLite3AccessorTest, findPrevious) {
// Largest name
EXPECT_EQ("www.example.com.",
accessor->findPreviousName(1, "com.example.wwww"));
- // Wrap around
- EXPECT_EQ("www.example.com.",
- accessor->findPreviousName(1, "com.example."));
- // Out of zone before and after
- EXPECT_EQ("www.example.com.",
- accessor->findPreviousName(1, "bb.example."));
+ // Out of zone after the last name
EXPECT_EQ("www.example.com.",
accessor->findPreviousName(1, "org.example."));
// Case insensitive?
@@ -374,6 +369,12 @@ TEST_F(SQLite3AccessorTest, findPrevious) {
// A name that doesn't exist
EXPECT_EQ("dns01.example.com.",
accessor->findPreviousName(1, "com.exaMple.DNS01X."));
+ // The DB contains foo.bar.example.com., which would be in between
+ // these two names. However, that one does not have an NSEC record,
+ // which is how this database recognizes glue data, so it should
+ // be skipped.
+ EXPECT_EQ("example.com.",
+ accessor->findPreviousName(1, "com.example.cname-ext."));
}
TEST_F(SQLite3AccessorTest, findPreviousNoData) {
diff --git a/src/lib/datasrc/zone.h b/src/lib/datasrc/zone.h
index 794e46d..89e4003 100644
--- a/src/lib/datasrc/zone.h
+++ b/src/lib/datasrc/zone.h
@@ -219,12 +219,15 @@ public:
/// however it is recommended to stick to the ones listed here. The user
/// of this method should be able to handle any exceptions.
///
+ /// This method does not include under-zone-cut data (glue data).
+ ///
/// \param query The name for which one we look for a previous one. The
/// queried name doesn't have to exist in the zone.
/// \return The preceding name
///
/// \throw NotImplemented in case the data source backend doesn't support
- /// DNSSEC.
+ /// DNSSEC or there is no previous in the zone (NSEC records might be
+ /// missing in the DB, the queried name is less or equal to the apex).
/// \throw DataSourceError for low-level or internal datasource errors
/// (like broken connection to database, wrong data living there).
/// \throw std::bad_alloc For allocation errors.
More information about the bind10-changes
mailing list