[master] Merge branch 'trac1614'

src/lib/datasrc/memory_datasrc.cc

@@ -21,6 +21,7 @@
 #include <exceptions/exceptions.h>
 
 #include <dns/name.h>
+#include <dns/rdataclass.h>
 #include <dns/rrclass.h>
 #include <dns/rrsetlist.h>
 #include <dns/masterload.h>
@@ -36,6 +37,7 @@
 
 using namespace std;
 using namespace isc::dns;
+using namespace isc::dns::rdata;
 using namespace isc::data;
 
 namespace isc {
@@ -181,8 +183,12 @@ struct InMemoryZoneFinder::InMemoryZoneFinderImpl {
         if (!rrset) {
             isc_throw(NullRRset, "The rrset provided is NULL");
         }
+        if (rrset->getRdataCount() == 0) {
+            isc_throw(AddError, "The rrset provided is empty: " <<
+                      rrset->getName() << "/" << rrset->getType());
+        }
         // Check for singleton RRs. It should probably handled at a different
-        // in future.
+        // layer in future.
         if ((rrset->getType() == RRType::CNAME() ||
             rrset->getType() == RRType::DNAME()) &&
             rrset->getRdataCount() > 1)
@@ -230,6 +236,61 @@ struct InMemoryZoneFinder::InMemoryZoneFinderImpl {
         }
     }
 
+    result::Result addRRsig(const ConstRRsetPtr sig_rrset,
+                            DomainTree& domains)
+    {
+        DomainNode* node = NULL;
+        if (domains.find(sig_rrset->getName(), &node) !=
+            DomainTree::EXACTMATCH || node == NULL || !node->getData()) {
+            isc_throw(AddError,
+                      "RRSIG is being added, but no RR to be covered: "
+                      << sig_rrset->getName());
+        }
+
+        // Check consistency of the type covered.
+        // We know the RRset isn't empty, so the following check is safe.
+        RdataIteratorPtr rit = sig_rrset->getRdataIterator();
+        const RRType covered = dynamic_cast<const generic::RRSIG&>(
+            rit->getCurrent()).typeCovered();
+        for (rit->next(); !rit->isLast(); rit->next()) {
+            if (dynamic_cast<const generic::RRSIG&>(
+                    rit->getCurrent()).typeCovered() != covered) {
+                isc_throw(AddError, "RRSIG contains mixed covered types: "
+                          << sig_rrset->toText());
+            }
+        }
+
+        // Find the RRset to be covered; if not found, treat it as an error
+        // for now.
+        const Domain::const_iterator it = node->getData()->find(covered);
+        if (it == node->getData()->end()) {
+            isc_throw(AddError,
+                      "RRSIG is being added, but no RR of covered type found: "
+                      << sig_rrset->toText());
+        }
+
+        // The current implementation doesn't allow an existing RRSIG to be
+        // overridden (or updated with additional ones).
+        if ((it->second)->getRRsig()) {
+            isc_throw(AddError,
+                      "RRSIG is being added to override an existing one: "
+                      << sig_rrset->toText());
+        }
+
+        // All okay, setting the RRSIG.
+        // XXX: we break const-ness of the covered RRsets.  In practice the
+        // ownership of these RRsets would have been given to us so it should
+        // be safe, but it's still a very bad practice.
+        // We'll fix this problem anyway when we update the underlying
+        // representation so that it's more space efficient.
+        // Note: there's a slight chance of getting an exception.
+        // As noted in add(), we give up strong exception guarantee in such
+        // cases.
+        boost::const_pointer_cast<RRset>(it->second)->addRRsig(sig_rrset);
+
+        return (result::SUCCESS);
+    }
+
     /*
      * Implementation of longer methods. We put them here, because the
      * access is without the impl_-> and it will get inlined anyway.
@@ -244,6 +305,12 @@ struct InMemoryZoneFinder::InMemoryZoneFinderImpl {
         LOG_DEBUG(logger, DBG_TRACE_DATA, DATASRC_MEM_ADD_RRSET).
             arg(rrset->getName()).arg(rrset->getType()).arg(origin_);
 
+        // RRSIGs are special in various points, so we handle it in a
+        // separate dedicated method.
+        if (rrset->getType() == RRType::RRSIG()) {
+            return (addRRsig(rrset, *domains));
+        }
+
         // Add wildcards possibly contained in the owner name to the domain
         // tree.
         // Note: this can throw an exception, breaking strong exception
@@ -283,7 +350,7 @@ struct InMemoryZoneFinder::InMemoryZoneFinderImpl {
             if (rrset->getType() == RRType::NS() &&
                 rrset->getName() != origin_) {
                 node->setFlag(DomainNode::FLAG_CALLBACK);
-            // If it is DNAME, we have a callback as well here
+                // If it is DNAME, we have a callback as well here
             } else if (rrset->getType() == RRType::DNAME()) {
                 node->setFlag(DomainNode::FLAG_CALLBACK);
             }

src/lib/datasrc/memory_datasrc.h

@@ -99,6 +99,15 @@ public:
     ///
     /// It puts another RRset into the zone.
     ///
+    /// In the current implementation, this method doesn't allow an existing
+    /// RRset to be updated or overridden.  So the caller must make sure that
+    /// all RRs of the same type and name must be given in the form of a
+    /// single RRset.  The current implementation will also require that
+    /// when an RRSIG is added the RRset to be covered has already been
+    /// added.  These restrictions are probably too strict when this data
+    /// source accepts various forms of input, so they should be revisited
+    /// later.
+    ///
     /// Except for NullRRset and OutOfZone, this method does not guarantee
     /// strong exception safety (it is currently not needed, if it is needed
     /// in future, it should be implemented).

src/lib/datasrc/tests/memory_datasrc_unittest.cc

@@ -33,12 +33,15 @@
 #include <datasrc/data_source.h>
 #include <datasrc/iterator.h>
 
+#include <testutils/dnsmessage_test.h>
+
 #include <gtest/gtest.h>
 
 using namespace std;
 using namespace isc::dns;
 using namespace isc::dns::rdata;
 using namespace isc::datasrc;
+using namespace isc::testutils;
 
 namespace {
 // Commonly used result codes (Who should write the prefix all the time)
@@ -251,6 +254,20 @@ TEST_F(InMemoryClientTest, startUpdateZone) {
                  isc::NotImplemented);
 }
 
+// Commonly used RRSIG data
+const char* const rrsig_a_txt =
+    "example.org. 300 IN RRSIG A 5 3 3600 20000101000000 20000201000000 12345 "
+    "example.org. FAKEFAKEFAKE\n";
+const char* const rrsig_ns_txt =
+    "example.org. 300 IN RRSIG NS 5 3 3600 20000101000000 20000201000000 "
+    "54321 example.org. FAKEFAKEFAKEFAKE\n";
+// This RRset has two RRSIGs
+const char* const rrsig_aaaa_txt =
+    "ns.example.org. 300 IN RRSIG AAAA 5 3 3600 20000101000000 20000201000000 "
+    "12345 example.org. FAKEFAKEFAKE\n"
+    "ns.example.org. 300 IN RRSIG AAAA 5 3 3600 20000101000000 20000201000000 "
+    "54321 example.org. FAKEFAKEFAKEFAKE\n";
+
 // A helper callback of masterLoad() used in InMemoryZoneFinderTest.
 void
 setRRset(RRsetPtr rrset, vector<RRsetPtr*>::iterator& it) {
@@ -324,9 +341,8 @@ public:
             rrsets.push_back(zone_data[i].rrset);
         }
 
-        vector<RRsetPtr*>::iterator it = rrsets.begin();
         masterLoad(zone_data_stream, Name::ROOT_NAME(), class_,
-                   boost::bind(setRRset, _1, it));
+                   boost::bind(setRRset, _1, rrsets.begin()));
     }
     // Some data to test with
     const RRClass class_;
@@ -334,6 +350,9 @@ public:
     // The zone finder to torture by tests
     InMemoryZoneFinder zone_finder_;
 
+    // Placeholder for storing RRsets to be checked with rrsetsCheck()
+    vector<ConstRRsetPtr> actual_rrsets_;
+
     /*
      * Some RRsets to put inside the zone.
      */
@@ -469,6 +488,18 @@ public:
     }
     // Internal part of the cancelWildcard test that is multiple times
     void doCancelWildcardTest();
+
+    ConstRRsetPtr textToRRset(const string& text_rrset,
+                              const RRClass& rrclass = RRClass::IN()) const
+    {
+        stringstream ss(text_rrset);
+        RRsetPtr rrset;
+        vector<RRsetPtr*> rrsets;
+        rrsets.push_back(&rrset);
+        masterLoad(ss, Name::ROOT_NAME(), rrclass,
+                   boost::bind(setRRset, _1, rrsets.begin()));
+        return (rrset);
+    }
 };
 
 /**
@@ -546,9 +577,8 @@ TEST_F(InMemoryZoneFinderTest, findCNAMEUnderZoneCut) {
     // (with FIND_GLUE_OK).  The behavior is different from BIND 9,
     // so we test this case explicitly.
     EXPECT_EQ(SUCCESS, zone_finder_.add(rr_child_ns_));
-    RRsetPtr rr_cname_under_cut_(new RRset(Name("cname.child.example.org"),
-                                           class_, RRType::CNAME(),
-                                           RRTTL(300)));
+    ConstRRsetPtr rr_cname_under_cut_ = textToRRset(
+        "cname.child.example.org. 300 IN CNAME target.child.example.org.");
     EXPECT_EQ(SUCCESS, zone_finder_.add(rr_cname_under_cut_));
     findTest(Name("cname.child.example.org"), RRType::AAAA(),
              ZoneFinder::CNAME, true, rr_cname_under_cut_, NULL,
@@ -799,11 +829,11 @@ TEST_F(InMemoryZoneFinderTest, emptyNode) {
 
     // Construct the test zone
     const char* const names[] = {
-        "bar.example.org", "x.foo.example.org", "aaa.baz.example.org",
+        "bar.example.org.", "x.foo.example.org.", "aaa.baz.example.org.",
         "bbb.baz.example.org.", NULL};
     for (int i = 0; names[i] != NULL; ++i) {
-        ConstRRsetPtr rrset(new RRset(Name(names[i]), class_, RRType::A(),
-                                      RRTTL(300)));
+        ConstRRsetPtr rrset = textToRRset(string(names[i]) +
+                                          " 300 IN A 192.0.2.1");
         EXPECT_EQ(SUCCESS, zone_finder_.add(rrset));
     }
 
@@ -1143,9 +1173,8 @@ TEST_F(InMemoryZoneFinderTest, swap) {
     ASSERT_NE(origin_, other_origin); // make sure these two are different
     InMemoryZoneFinder finder2(RRClass::CH(), other_origin);
     EXPECT_EQ(result::SUCCESS,
-              finder2.add(RRsetPtr(new RRset(Name("version.bind"),
-                                           RRClass::CH(), RRType::TXT(),
-                                           RRTTL(0)))));
+              finder2.add(textToRRset("version.bind. 0 CH TXT \"test\"",
+                                      RRClass::CH())));
 
     finder1.swap(finder2);
     EXPECT_EQ(other_origin, finder1.getOrigin());
@@ -1187,4 +1216,95 @@ TEST_F(InMemoryZoneFinderTest, getFileName) {
     EXPECT_EQ(TEST_DATA_DIR "/root.zone", zone_finder_.getFileName());
     EXPECT_TRUE(rootzone.getFileName().empty());
 }
+
+TEST_F(InMemoryZoneFinderTest, addRRsig) {
+    // A simple valid case: adding an RRset to be signed followed by an RRSIG
+    // that covers the first RRset
+    zone_finder_.add(rr_a_);
+    zone_finder_.add(textToRRset(rrsig_a_txt));
+    ZoneFinder::FindResult result = zone_finder_.find(origin_, RRType::A(),
+                                                      ZoneFinder::FIND_DNSSEC);
+    EXPECT_EQ(ZoneFinder::SUCCESS, result.code);
+    ASSERT_TRUE(result.rrset);
+    ASSERT_TRUE(result.rrset->getRRsig());
+    actual_rrsets_.push_back(result.rrset->getRRsig());
+    rrsetsCheck(rrsig_a_txt, actual_rrsets_.begin(), actual_rrsets_.end());
+
+    // Confirm a separate RRISG for a different type can be added
+    actual_rrsets_.clear();
+    zone_finder_.add(rr_ns_);
+    zone_finder_.add(textToRRset(rrsig_ns_txt));
+    ZoneFinder::FindResult result2 =
+        zone_finder_.find(origin_, RRType::NS(), ZoneFinder::FIND_DNSSEC);
+    EXPECT_EQ(ZoneFinder::SUCCESS, result2.code);
+    ASSERT_TRUE(result2.rrset);
+    ASSERT_TRUE(result2.rrset->getRRsig());
+    actual_rrsets_.push_back(result2.rrset->getRRsig());
+    rrsetsCheck(rrsig_ns_txt, actual_rrsets_.begin(), actual_rrsets_.end());
+
+    // Check a case with multiple RRSIGs
+    actual_rrsets_.clear();
+    zone_finder_.add(rr_ns_aaaa_);
+    zone_finder_.add(textToRRset(rrsig_aaaa_txt));
+    ZoneFinder::FindResult result3 =
+        zone_finder_.find(Name("ns.example.org"), RRType::AAAA(),
+                          ZoneFinder::FIND_DNSSEC);
+    EXPECT_EQ(ZoneFinder::SUCCESS, result3.code);
+    ASSERT_TRUE(result3.rrset);
+    ASSERT_TRUE(result3.rrset->getRRsig());
+    actual_rrsets_.push_back(result3.rrset->getRRsig());
+    rrsetsCheck(rrsig_aaaa_txt, actual_rrsets_.begin(), actual_rrsets_.end());
+}
+
+TEST_F(InMemoryZoneFinderTest, addRRsigWithoutCovered) {
+    // The current implementation rejects attempts of adding RRSIG without
+    // covered RRsets already in the zone.
+
+    // Name doesn't exist
+    EXPECT_THROW(zone_finder_.add(
+                     textToRRset("notexist.example.org. 300 IN RRSIG A 5 3 "
+                                 "3600 20000101000000 20000201000000 12345 "
+                                 "example.org. FAKEFAKEFAKE\n")),
+                 InMemoryZoneFinder::AddError);
+
+    // Name exists, but is empty.
+    zone_finder_.add(rr_emptywild_);
+    EXPECT_THROW(zone_finder_.add(
+                     textToRRset("foo.example.org. 300 IN RRSIG A 5 3 "
+                                 "3600 20000101000000 20000201000000 12345 "
+                                 "example.org. FAKEFAKEFAKE\n")),
+                 InMemoryZoneFinder::AddError);
+
+    // Add RRSIG RRset without covered RR
+    zone_finder_.add(rr_a_);
+    EXPECT_THROW(zone_finder_.add(textToRRset(rrsig_ns_txt)),
+                 InMemoryZoneFinder::AddError);
+}
+
+TEST_F(InMemoryZoneFinderTest, addbadRRsig) {
+    // Tests with other types of bogus input
+
+    // Empty RRSIG RRset.
+    EXPECT_THROW(zone_finder_.add(RRsetPtr(new RRset(origin_, class_,
+                                                     RRType::RRSIG(),
+                                                     RRTTL(300)))),
+                                  InMemoryZoneFinder::AddError);
+
+    // RRSIG with mixed covered types
+    zone_finder_.add(rr_a_);    // make sure the covered name exists
+    // textToRRset() doesn't work as intended for this pathological case,
+    // so we need to construct the RRset by hand.
+    RRsetPtr rrset(new RRset(origin_, class_, RRType::RRSIG(), RRTTL(300)));
+    rrset->addRdata(generic::RRSIG("A 5 3 3600 20000101000000 20000201000000 "
+                                   "12345 example.org. FAKEFAKEFAKE"));
+    rrset->addRdata(generic::RRSIG("NS 5 3 3600 20000101000000 20000201000000 "
+                                   "54321 example.org. FAKEFAKEFAKEFAKE"));
+    EXPECT_THROW(zone_finder_.add(rrset), InMemoryZoneFinder::AddError);
+
+    // An attempt of overriding an existing RRSIG.  The current implementation
+    // prohibits that.
+    zone_finder_.add(textToRRset(rrsig_a_txt));
+    EXPECT_THROW(zone_finder_.add(textToRRset(rrsig_a_txt)),
+                 InMemoryZoneFinder::AddError);
+}
 }

src/lib/dns/masterload.cc

@@ -25,6 +25,7 @@
 #include <dns/masterload.h>
 #include <dns/name.h>
 #include <dns/rdata.h>
+#include <dns/rdataclass.h>
 #include <dns/rrclass.h>
 #include <dns/rrset.h>
 #include <dns/rrttl.h>
@@ -58,6 +59,7 @@ masterLoad(istream& input, const Name& origin, const RRClass& zone_class,
            MasterLoadCallback callback)
 {
     RRsetPtr rrset;
+    ConstRdataPtr prev_rdata;   // placeholder for special case of RRSIGs
     string line;
     unsigned int line_count = 1;
 
@@ -145,8 +147,20 @@ masterLoad(istream& input, const Name& origin, const RRClass& zone_class,
         // Everything is okay.  Now create/update RRset with the new RR.
         // If this is the first RR or the RR type/name is new, we are seeing
         // a new RRset.
+        bool new_rrset = false;
         if (!rrset || rrset->getType() != *rrtype ||
             rrset->getName() != *owner) {
+            new_rrset = true;
+        } else if (rrset->getType() == RRType::RRSIG()) {
+            // We are seeing two consecutive RRSIGs of the same name.
+            // They can be combined iff they have the same type covered.
+            if (dynamic_cast<const generic::RRSIG&>(*rdata).typeCovered() !=
+                dynamic_cast<const generic::RRSIG&>(*prev_rdata).typeCovered())
+            {
+                new_rrset = true;
+            }
+        }
+        if (new_rrset) {
             // Commit the previous RRset, if any.
             if (rrset) {
                 callback(rrset);
@@ -154,6 +168,7 @@ masterLoad(istream& input, const Name& origin, const RRClass& zone_class,
             rrset = RRsetPtr(new RRset(*owner, *rrclass, *rrtype, *ttl));
         }
         rrset->addRdata(rdata);
+        prev_rdata = rdata;
     } while (++line_count, !input.eof());
 
     // Commit the last RRset, if any.

src/lib/dns/masterload.h

@@ -206,9 +206,8 @@ typedef boost::function<void(RRsetPtr)> MasterLoadCallback;
 /// - We may want to support incremental loading.
 /// - If we add these optional features we may want to introduce a class
 ///   that encapsulates loading status and options.
-/// - RRSIGs are currently identified as their owner name and RR type (RRSIG).
-///   In practice it should be sufficient, but technically we should also
-///   consider the Type Covered field.
+/// - RRSIGs are handled as separate RRsets, i.e. they are not included in
+///   the RRset they cover.
 ///
 /// \param filename A path to a master zone file to be loaded.
 /// \param origin The origin name of the zone.

src/lib/dns/rrset.h

@@ -693,7 +693,7 @@ public:
     }
 
     /// \brief Adds an RRSIG RR to this RRset's signatures
-    virtual void addRRsig(const rdata::RdataPtr rdata) {
+    virtual void addRRsig(const rdata::ConstRdataPtr rdata) {
         if (!rrsig_) {
             rrsig_ = RRsetPtr(new RRset(getName(), getClass(),
                                         RRType::RRSIG(), getTTL()));
@@ -702,7 +702,7 @@ public:
     }
 
     /// \brief Adds an RRSIG RRset to this RRset
-    void addRRsig(AbstractRRset& sigs) {
+    void addRRsig(const AbstractRRset& sigs) {
         RdataIteratorPtr it = sigs.getRdataIterator();
 
         if (!rrsig_) {
@@ -715,7 +715,7 @@ public:
         }
     }
 
-    void addRRsig(RRsetPtr sigs) { addRRsig(*sigs); }
+    void addRRsig(ConstRRsetPtr sigs) { addRRsig(*sigs); }
 
     /// \brief Clear the RRSIGs for this RRset
     void removeRRsig() { rrsig_ = RRsetPtr(); }

src/lib/dns/tests/masterload_unittest.cc

@@ -72,6 +72,13 @@ const char* const a_rr2 = "www.example.com. 60 IN A 192.0.2.2\n";
 const char* const a_rr3 = "ftp.example.com. 60 IN A 192.0.2.3\n";
 // multi-field RR case
 const char* const soa_rr = "example.com. 7200 IN SOA . . 0 0 0 0 0\n";
+// A couple of RRSIGs, different type covered
+const char* const rrsig_rr1 =
+    "www.example.com. 60 IN RRSIG A 5 3 3600 20000101000000 20000201000000 "
+    "12345 example.com. FAKEFAKEFAKE\n";
+const char* const rrsig_rr2 =
+    "www.example.com. 60 IN RRSIG AAAA 5 3 3600 20000101000000 20000201000000 "
+    "12345 example.com. FAKEFAKEFAKE\n";
 
 TEST_F(MasterLoadTest, loadRRs) {
     // a simple case: loading 3 RRs, each consists of a single RRset.
@@ -147,6 +154,13 @@ TEST_F(MasterLoadTest, loadRRsetsInterleaved) {
     EXPECT_EQ(a_rr2, results[2]->toText());
 }
 
+TEST_F(MasterLoadTest, loadRRsigs) {
+    // RRSIGs of different types covered should be separated
+    rr_stream << rrsig_rr1 << rrsig_rr2;
+    masterLoad(rr_stream, origin, zclass, callback);
+    EXPECT_EQ(2, results.size());
+}
+
 TEST_F(MasterLoadTest, loadWithNoEOF) {
     // the input stream doesn't end with a new line (and the following blank
     // line).  It should be accepted.