BIND 10 trac1063, updated. 62432e71ef943744fd4ca9ce216da1b0a7250573 [1063] Fix NS delegation handling
BIND 10 source code commits
bind10-changes at lists.isc.org
Fri Aug 12 12:58:07 UTC 2011
The branch, trac1063 has been updated
via 62432e71ef943744fd4ca9ce216da1b0a7250573 (commit)
via 005c77dfe53b54cef92ce51d91f615eb9c2769c4 (commit)
via ce3bc8504d765ecc9b453398efb18662bd4f277a (commit)
via 94fc6d8d303053c47064c9408947cd49a8e11975 (commit)
from c5cf3cc081042fec0e2baea7cdf7f22a8a84664a (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 62432e71ef943744fd4ca9ce216da1b0a7250573
Author: Michal 'vorner' Vaner <michal.vaner at nic.cz>
Date: Fri Aug 12 14:56:01 2011 +0200
[1063] Fix NS delegation handling
Sometimes it was in wrong order or strange data wasn't found.
commit 005c77dfe53b54cef92ce51d91f615eb9c2769c4
Author: Michal 'vorner' Vaner <michal.vaner at nic.cz>
Date: Fri Aug 12 14:43:33 2011 +0200
[1063] Few more delegation corner-cases tested
commit ce3bc8504d765ecc9b453398efb18662bd4f277a
Author: Michal 'vorner' Vaner <michal.vaner at nic.cz>
Date: Fri Aug 12 14:35:33 2011 +0200
[1063] Ignore NS at origin
It was written, but the zone didn't know it's own origin. This is now
fixed and tested.
commit 94fc6d8d303053c47064c9408947cd49a8e11975
Author: Michal 'vorner' Vaner <michal.vaner at nic.cz>
Date: Fri Aug 12 14:05:05 2011 +0200
[1063] Document the internal function
Finder.getRRset is documented even when it is private and internal, as
it has complicated interface (which might use some simplification).
-----------------------------------------------------------------------
Summary of changes:
src/lib/datasrc/database.cc | 51 +++++++++++--------
src/lib/datasrc/database.h | 44 ++++++++++++++++-
src/lib/datasrc/tests/database_unittest.cc | 72 +++++++++++++++++++++++++++-
3 files changed, 142 insertions(+), 25 deletions(-)
-----------------------------------------------------------------------
diff --git a/src/lib/datasrc/database.cc b/src/lib/datasrc/database.cc
index f581d27..6afd3dc 100644
--- a/src/lib/datasrc/database.cc
+++ b/src/lib/datasrc/database.cc
@@ -49,16 +49,18 @@ DatabaseClient::findZone(const Name& name) const {
if (zone.first) {
return (FindResult(result::SUCCESS,
ZoneFinderPtr(new Finder(database_,
- zone.second))));
+ zone.second, name))));
}
// Than super domains
// Start from 1, as 0 is covered above
for (size_t i(1); i < name.getLabelCount(); ++i) {
- zone = database_->getZone(name.split(i));
+ isc::dns::Name superdomain(name.split(i));
+ zone = database_->getZone(superdomain);
if (zone.first) {
return (FindResult(result::PARTIALMATCH,
ZoneFinderPtr(new Finder(database_,
- zone.second))));
+ zone.second,
+ superdomain))));
}
}
// No, really nothing
@@ -66,9 +68,11 @@ DatabaseClient::findZone(const Name& name) const {
}
DatabaseClient::Finder::Finder(boost::shared_ptr<DatabaseAccessor>
- database, int zone_id) :
+ database, int zone_id,
+ const isc::dns::Name& origin) :
database_(database),
- zone_id_(zone_id)
+ zone_id_(zone_id),
+ origin_(origin)
{ }
namespace {
@@ -196,16 +200,30 @@ DatabaseClient::Finder::getRRset(const isc::dns::Name& name,
// of the 'type covered' field in the RRSIG Rdata).
//cur_sigtype(columns[SIGTYPE_COLUMN]);
- if (type != NULL && cur_type == *type) {
+ // Check for delegations before checking for the right type.
+ // This is needed to properly delegate request for the NS
+ // record itself.
+ //
+ // This happens with NS only, CNAME must be alone and DNAME
+ // is not checked in the exact queried domain.
+ if (want_ns && cur_type == isc::dns::RRType::NS()) {
+ if (result_rrset &&
+ result_rrset->getType() != isc::dns::RRType::NS()) {
+ isc_throw(DataSourceError, "NS found together with data"
+ " in non-apex domain " + name.toText());
+ }
+ addOrCreate(result_rrset, name, getClass(), cur_type, cur_ttl,
+ columns[DatabaseAccessor::RDATA_COLUMN],
+ *database_);
+ } else if (type != NULL && cur_type == *type) {
if (result_rrset &&
result_rrset->getType() == isc::dns::RRType::CNAME()) {
isc_throw(DataSourceError, "CNAME found but it is not "
"the only record for " + name.toText());
} else if (result_rrset && want_ns &&
result_rrset->getType() == isc::dns::RRType::NS()) {
- // In case there's a NS, we should find the delegation, not
- // the data
- continue;
+ isc_throw(DataSourceError, "NS found together with data"
+ " in non-apex domain " + name.toText());
}
addOrCreate(result_rrset, name, getClass(), cur_type, cur_ttl,
columns[DatabaseAccessor::RDATA_COLUMN],
@@ -220,16 +238,6 @@ DatabaseClient::Finder::getRRset(const isc::dns::Name& name,
addOrCreate(result_rrset, name, getClass(), cur_type, cur_ttl,
columns[DatabaseAccessor::RDATA_COLUMN],
*database_);
- } else if (want_ns && cur_type == isc::dns::RRType::NS()) {
- if (result_rrset &&
- // In case we already found some data here, we should
- // replace it with the NS, we should delegate
- result_rrset->getType() != isc::dns::RRType::NS()) {
- result_rrset = isc::dns::RRsetPtr();
- }
- addOrCreate(result_rrset, name, getClass(), cur_type, cur_ttl,
- columns[DatabaseAccessor::RDATA_COLUMN],
- *database_);
} else if (want_dname && cur_type == isc::dns::RRType::DNAME()) {
// There should be max one RR of DNAME present
if (result_rrset &&
@@ -321,7 +329,7 @@ DatabaseClient::Finder::find(const isc::dns::Name& name,
found = getRRset(name, &type, true, false, name != origin);
records_found = found.first;
result_rrset = found.second;
- if (result_rrset && type != isc::dns::RRType::NS() &&
+ if (result_rrset && name != origin &&
result_rrset->getType() == isc::dns::RRType::NS()) {
result_status = DELEGATION;
} else if (result_rrset && type != isc::dns::RRType::CNAME() &&
@@ -372,8 +380,7 @@ DatabaseClient::Finder::find(const isc::dns::Name& name,
Name
DatabaseClient::Finder::getOrigin() const {
- // TODO Implement
- return (Name("."));
+ return (origin_);
}
isc::dns::RRClass
diff --git a/src/lib/datasrc/database.h b/src/lib/datasrc/database.h
index 9e41f91..eaeecc5 100644
--- a/src/lib/datasrc/database.h
+++ b/src/lib/datasrc/database.h
@@ -17,6 +17,8 @@
#include <datasrc/client.h>
+#include <dns/name.h>
+
namespace isc {
namespace datasrc {
@@ -218,8 +220,12 @@ public:
* \param zone_id The zone ID which was returned from
* DatabaseAccessor::getZone and which will be passed to further
* calls to the database.
+ * \param origin The name of the origin of this zone. It could query
+ * it from database, but as the DatabaseClient just searched for
+ * the zone using the name, it should have it.
*/
- Finder(boost::shared_ptr<DatabaseAccessor> database, int zone_id);
+ Finder(boost::shared_ptr<DatabaseAccessor> database, int zone_id,
+ const isc::dns::Name& origin);
// The following three methods are just implementations of inherited
// ZoneFinder's pure virtual methods.
virtual isc::dns::Name getOrigin() const;
@@ -290,7 +296,41 @@ public:
private:
boost::shared_ptr<DatabaseAccessor> database_;
const int zone_id_;
- /// \brief Searches database for an RRset
+ const isc::dns::Name origin_;
+ /**
+ * \brief Searches database for an RRset
+ *
+ * This method scans RRs of single domain specified by name and finds
+ * RRset with given type or any of redirection RRsets that are
+ * requested.
+ *
+ * This function is used internally by find(), because this part is
+ * called multiple times with slightly different parameters.
+ *
+ * \param name Which domain name should be scanned.
+ * \param type The RRType which is requested. This can be NULL, in
+ * which case the method will look for the redirections only.
+ * \param want_cname If this is true, CNAME redirection may be returned
+ * instead of the RRset with given type. If there's CNAME and
+ * something else or the CNAME has multiple RRs, it throws
+ * DataSourceError.
+ * \param want_dname If this is true, DNAME redirection may be returned
+ * instead. This is with type = NULL only and is not checked in
+ * other circumstances. If the DNAME has multiple RRs, it throws
+ * DataSourceError.
+ * \param want_ns This allows redirection by NS to be returned. If
+ * any other data is met as well, DataSourceError is thrown.
+ * \note It may happen that some of the above error conditions are not
+ * detected in some circumstances. The goal here is not to validate
+ * the domain in DB, but to avoid bad behaviour resulting from
+ * broken data.
+ * \return First part of the result tells if the domain contains any
+ * RRs. This can be used to decide between NXDOMAIN and NXRRSET.
+ * The second part is the RRset found (if any) with any relevant
+ * signatures attached to it.
+ * \todo This interface doesn't look very elegant. Any better idea
+ * would be nice.
+ */
std::pair<bool, isc::dns::RRsetPtr> getRRset(const isc::dns::Name&
name,
const isc::dns::RRType*
diff --git a/src/lib/datasrc/tests/database_unittest.cc b/src/lib/datasrc/tests/database_unittest.cc
index 32ce8d9..e3d1393 100644
--- a/src/lib/datasrc/tests/database_unittest.cc
+++ b/src/lib/datasrc/tests/database_unittest.cc
@@ -265,7 +265,6 @@ private:
addCurName("badsigtype.example.org.");
// Data for testing delegation (with NS and DNAME)
- addRecord("A", "3600", "", "192.0.2.1");
addRecord("NS", "3600", "", "ns.example.com.");
addRecord("NS", "3600", "", "ns.delegation.example.org.");
addCurName("delegation.example.org.");
@@ -278,10 +277,24 @@ private:
addRecord("A", "3600", "", "192.0.2.1");
addCurName("below.dname.example.org.");
+ // Broken NS
+ addRecord("A", "3600", "", "192.0.2.1");
+ addRecord("NS", "3600", "", "ns.example.com.");
+ addCurName("brokenns1.example.org.");
+ addRecord("NS", "3600", "", "ns.example.com.");
+ addRecord("A", "3600", "", "192.0.2.1");
+ addCurName("brokenns2.example.org.");
+
// Now double DNAME, to test failure mode
addRecord("DNAME", "3600", "", "dname1.example.com.");
addRecord("DNAME", "3600", "", "dname2.example.com.");
addCurName("baddname.example.org.");
+
+ // Put some data into apex (including NS) so we can check our NS
+ // doesn't break anything
+ addRecord("NS", "3600", "", "ns.example.com.");
+ addRecord("A", "3600", "", "192.0.2.1");
+ addCurName("example.org.");
}
};
@@ -703,6 +716,25 @@ TEST_F(DatabaseClientTest, find) {
expected_rdatas, expected_sig_rdatas);
EXPECT_FALSE(current_database_->searchRunning());
+ // The apex should not be considered delegation point and we can access
+ // data
+ expected_rdatas.clear();
+ expected_sig_rdatas.clear();
+ expected_rdatas.push_back("192.0.2.1");
+ doFindTest(finder, isc::dns::Name("example.org."),
+ isc::dns::RRType::A(), isc::dns::RRType::A(),
+ isc::dns::RRTTL(3600), ZoneFinder::SUCCESS, expected_rdatas,
+ expected_sig_rdatas);
+ EXPECT_FALSE(current_database_->searchRunning());
+
+ expected_rdatas.clear();
+ expected_rdatas.push_back("ns.example.com.");
+ doFindTest(finder, isc::dns::Name("example.org."),
+ isc::dns::RRType::NS(), isc::dns::RRType::NS(),
+ isc::dns::RRTTL(3600), ZoneFinder::SUCCESS, expected_rdatas,
+ expected_sig_rdatas);
+ EXPECT_FALSE(current_database_->searchRunning());
+
// Check when we ask for something below delegation point, we get the NS
// (Both when the RRset there exists and doesn't)
expected_rdatas.clear();
@@ -733,6 +765,13 @@ TEST_F(DatabaseClientTest, find) {
expected_sig_rdatas);
EXPECT_FALSE(current_database_->searchRunning());
+ // And when we ask direcly for the NS, we should still get delegation
+ doFindTest(finder, isc::dns::Name("delegation.example.org."),
+ isc::dns::RRType::NS(), isc::dns::RRType::NS(),
+ isc::dns::RRTTL(3600), ZoneFinder::DELEGATION, expected_rdatas,
+ expected_sig_rdatas);
+ EXPECT_FALSE(current_database_->searchRunning());
+
// Now test delegation. If it is below the delegation point, we should get
// the DNAME (the one with data under DNAME is invalid zone, but we test
// the behaviour anyway just to make sure)
@@ -764,11 +803,42 @@ TEST_F(DatabaseClientTest, find) {
expected_sig_rdatas);
EXPECT_FALSE(current_database_->searchRunning());
+ // Asking direcly for DNAME should give SUCCESS
+ expected_rdatas.clear();
+ expected_rdatas.push_back("dname.example.com.");
+ doFindTest(finder, isc::dns::Name("dname.example.org."),
+ isc::dns::RRType::DNAME(), isc::dns::RRType::DNAME(),
+ isc::dns::RRTTL(3600), ZoneFinder::SUCCESS, expected_rdatas,
+ expected_sig_rdatas);
+
// This is broken dname, it contains two targets
EXPECT_THROW(finder->find(isc::dns::Name("below.baddname.example.org."),
isc::dns::RRType::A(), NULL,
ZoneFinder::FIND_DEFAULT),
DataSourceError);
+ EXPECT_FALSE(current_database_->searchRunning());
+
+ // Broken NS - it lives together with something else
+ EXPECT_FALSE(current_database_->searchRunning());
+ EXPECT_THROW(finder->find(isc::dns::Name("brokenns1.example.org."),
+ isc::dns::RRType::A(), NULL,
+ ZoneFinder::FIND_DEFAULT),
+ DataSourceError);
+ EXPECT_FALSE(current_database_->searchRunning());
+ EXPECT_THROW(finder->find(isc::dns::Name("brokenns2.example.org."),
+ isc::dns::RRType::A(), NULL,
+ ZoneFinder::FIND_DEFAULT),
+ DataSourceError);
+ EXPECT_FALSE(current_database_->searchRunning());
+}
+
+TEST_F(DatabaseClientTest, getOrigin) {
+ DataSourceClient::FindResult zone(client_->findZone(Name("example.org")));
+ ASSERT_EQ(result::SUCCESS, zone.code);
+ shared_ptr<DatabaseClient::Finder> finder(
+ dynamic_pointer_cast<DatabaseClient::Finder>(zone.zone_finder));
+ EXPECT_EQ(42, finder->zone_id());
+ EXPECT_EQ(isc::dns::Name("example.org"), finder->getOrigin());
}
}
More information about the bind10-changes
mailing list