[1614] handle adding of RRSIGs correctly. It now takes into account the

covered type and attaches the RRSIGs to the covered RRset.  It also performs
some additional validations on RRSIGs.  As a result an empty RRset is
now prohibited (which is correct in this context anyway), so some existing
tests were updated, too.

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/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) {
@@ -322,9 +339,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_;
@@ -332,6 +348,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.
      */
@@ -466,6 +485,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);
+    }
 };
 
 /**
@@ -543,9 +574,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,
@@ -776,11 +806,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));
     }
 
@@ -1120,9 +1150,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());
@@ -1164,4 +1193,90 @@ 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
+    EXPECT_THROW(zone_finder_.add(textToRRset(string(rrsig_a_txt) +
+                                              string(rrsig_ns_txt))),
+                 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);
+}
 }