[master] Merge branch 'trac1612'

src/bin/auth/auth_messages.mes

@@ -73,6 +73,10 @@ attempt to parse the header of a received DNS packet has failed. (The
 reason for the failure is given in the message.) The server will drop the
 packet.
 
+% AUTH_INVALID_STATISTICS_DATA invalid specification of statistics data specified
+An error was encountered when the authoritiative server specified
+statistics data which is invalid for the auth specification file.
+
 % AUTH_LOAD_TSIG loading TSIG keys
 This is a debug message indicating that the authoritative server
 has requested the keyring holding TSIG keys from the configuration
@@ -92,6 +96,18 @@ discovered that the memory data source is disabled for the given class.
 This is a debug message reporting that the authoritative server has
 discovered that the memory data source is enabled for the given class.
 
+% AUTH_NOTIFY_QUESTIONS invalid number of questions (%1) in incoming NOTIFY
+This debug message is logged by the authoritative server when it receives
+a NOTIFY packet that contains zero or more than one question. (A valid
+NOTIFY packet contains one question.) The server will return a FORMERR
+error to the sender.
+
+% AUTH_NOTIFY_RRTYPE invalid question RR type (%1) in incoming NOTIFY
+This debug message is logged by the authoritative server when it receives
+a NOTIFY packet that an RR type of something other than SOA in the
+question section. (The RR type received is included in the message.) The
+server will return a FORMERR error to the sender.
+
 % AUTH_NO_STATS_SESSION session interface for statistics is not available
 The authoritative server had no session with the statistics module at the
 time it attempted to send it data: the attempt has been abandoned. This
@@ -102,18 +118,6 @@ This is a debug message produced by the authoritative server when it receives
 a NOTIFY packet but the XFRIN process is not running. The packet will be
 dropped and nothing returned to the sender.
 
-% AUTH_NOTIFY_RRTYPE invalid question RR type (%1) in incoming NOTIFY
-This debug message is logged by the authoritative server when it receives
-a NOTIFY packet that an RR type of something other than SOA in the
-question section. (The RR type received is included in the message.) The
-server will return a FORMERR error to the sender.
-
-% AUTH_NOTIFY_QUESTIONS invalid number of questions (%1) in incoming NOTIFY
-This debug message is logged by the authoritative server when it receives
-a NOTIFY packet that contains zero or more than one question. (A valid
-NOTIFY packet contains one question.) The server will return a FORMERR
-error to the sender.
-
 % AUTH_PACKET_PARSE_ERROR unable to parse received DNS packet: %1
 This is a debug message, generated by the authoritative server when an
 attempt to parse a received DNS packet has failed due to something other
@@ -154,6 +158,19 @@ a command from the statistics module to send it data. The 'sendstats'
 command is handled differently to other commands, which is why the debug
 message associated with it has its own code.
 
+% AUTH_RESPONSE_FAILURE exception while building response to query: %1
+This is a debug message, generated by the authoritative server when an
+attempt to create a response to a received DNS packet has failed. The
+reason for the failure is given in the log message. A SERVFAIL response
+is sent back. The most likely cause of this is an error in the data
+source implementation; it is either creating bad responses or raising
+exceptions itself.
+
+% AUTH_RESPONSE_FAILURE_UNKNOWN unknown exception while building response to query
+This debug message is similar to AUTH_RESPONSE_FAILURE, but further
+details about the error are unknown, because it was signaled by something
+which is not an exception. This is definitely a bug.
+
 % AUTH_RESPONSE_RECEIVED received response message, ignoring
 This is a debug message, this is output if the authoritative server
 receives a DNS packet with the QR bit set, i.e. a DNS response. The
@@ -260,7 +277,3 @@ This is a debug message output during the processing of a NOTIFY
 request. The zone manager component has been informed of the request,
 but has returned an error response (which is included in the message). The
 NOTIFY request will not be honored.
-
-% AUTH_INVALID_STATISTICS_DATA invalid specification of statistics data specified
-An error was encountered when the authoritiative server specified
-statistics data which is invalid for the auth specification file.

src/bin/auth/auth_srv.cc

@@ -481,35 +481,43 @@ AuthSrv::processMessage(const IOMessage& io_message, MessagePtr message,
         return;
     }
 
-    // update per opcode statistics counter.  This can only be reliable after
-    // TSIG check succeeds.
-    impl_->counters_.inc(message->getOpcode());
-
     bool send_answer = true;
-    if (message->getOpcode() == Opcode::NOTIFY()) {
-        send_answer = impl_->processNotify(io_message, message, buffer,
-                                           tsig_context);
-    } else if (message->getOpcode() != Opcode::QUERY()) {
-        LOG_DEBUG(auth_logger, DBG_AUTH_DETAIL, AUTH_UNSUPPORTED_OPCODE)
-                  .arg(message->getOpcode().toText());
-        makeErrorMessage(message, buffer, Rcode::NOTIMP(), tsig_context);
-    } else if (message->getRRCount(Message::SECTION_QUESTION) != 1) {
-        makeErrorMessage(message, buffer, Rcode::FORMERR(), tsig_context);
-    } else {
-        ConstQuestionPtr question = *message->beginQuestion();
-        const RRType &qtype = question->getType();
-        if (qtype == RRType::AXFR()) {
-            send_answer = impl_->processXfrQuery(io_message, message, buffer,
-                                                 tsig_context);
-        } else if (qtype == RRType::IXFR()) {
-            send_answer = impl_->processXfrQuery(io_message, message, buffer,
-                                                 tsig_context);
+    try {
+        // update per opcode statistics counter.  This can only be reliable
+        // after TSIG check succeeds.
+        impl_->counters_.inc(message->getOpcode());
+
+        if (message->getOpcode() == Opcode::NOTIFY()) {
+            send_answer = impl_->processNotify(io_message, message, buffer,
+                                               tsig_context);
+        } else if (message->getOpcode() != Opcode::QUERY()) {
+            LOG_DEBUG(auth_logger, DBG_AUTH_DETAIL, AUTH_UNSUPPORTED_OPCODE)
+                      .arg(message->getOpcode().toText());
+            makeErrorMessage(message, buffer, Rcode::NOTIMP(), tsig_context);
+        } else if (message->getRRCount(Message::SECTION_QUESTION) != 1) {
+            makeErrorMessage(message, buffer, Rcode::FORMERR(), tsig_context);
         } else {
-            send_answer = impl_->processNormalQuery(io_message, message,
-                                                    buffer, tsig_context);
+            ConstQuestionPtr question = *message->beginQuestion();
+            const RRType &qtype = question->getType();
+            if (qtype == RRType::AXFR()) {
+                send_answer = impl_->processXfrQuery(io_message, message,
+                                                     buffer, tsig_context);
+            } else if (qtype == RRType::IXFR()) {
+                send_answer = impl_->processXfrQuery(io_message, message,
+                                                     buffer, tsig_context);
+            } else {
+                send_answer = impl_->processNormalQuery(io_message, message,
+                                                        buffer, tsig_context);
+            }
         }
+    } catch (const std::exception& ex) {
+        LOG_DEBUG(auth_logger, DBG_AUTH_DETAIL, AUTH_RESPONSE_FAILURE)
+                  .arg(ex.what());
+        makeErrorMessage(message, buffer, Rcode::SERVFAIL());
+    } catch (...) {
+        LOG_DEBUG(auth_logger, DBG_AUTH_DETAIL, AUTH_RESPONSE_FAILURE_UNKNOWN);
+        makeErrorMessage(message, buffer, Rcode::SERVFAIL());
     }
-
     impl_->resumeServer(server, message, send_answer);
 }
 

src/bin/auth/tests/auth_srv_unittest.cc

@@ -87,7 +87,11 @@ protected:
         server.setXfrinSession(&notify_session);
         server.setStatisticsSession(&statistics_session);
     }
+
     virtual void processMessage() {
+        // If processMessage has been called before, parse_message needs
+        // to be reset. If it hasn't, there's no harm in doing so
+        parse_message->clear(Message::PARSE);
         server.processMessage(*io_message, parse_message, response_obuffer,
                               &dnsserv);
     }
@@ -120,6 +124,17 @@ protected:
             }
         }
     }
+
+    // Convenience method for tests that expect to return SERVFAIL
+    // It calls processMessage, checks if there is an answer, and
+    // check the header for default SERVFAIL data
+    void processAndCheckSERVFAIL() {
+        processMessage();
+        EXPECT_TRUE(dnsserv.hasAnswer());
+        headerCheck(*parse_message, default_qid, Rcode::SERVFAIL(),
+                    opcode.getCode(), QR_FLAG, 1, 0, 0, 0);
+    }
+
     IOService ios_;
     DNSService dnss_;
     MockSession statistics_session;
@@ -479,17 +494,17 @@ TEST_F(AuthSrvTest, AXFRSendFail) {
 }
 
 TEST_F(AuthSrvTest, AXFRDisconnectFail) {
-    // In our usage disconnect() shouldn't fail.  So we'll see the exception
-    // should it be thrown.
+    // In our usage disconnect() shouldn't fail. But even if it does,
+    // it should not disrupt service (so processMessage should have caught it)
     xfrout.disableSend();
     xfrout.disableDisconnect();
     UnitTestUtil::createRequestMessage(request_message, opcode, default_qid,
                                        Name("example.com"), RRClass::IN(),
                                        RRType::AXFR());
     createRequestPacket(request_message, IPPROTO_TCP);
-    EXPECT_THROW(server.processMessage(*io_message, parse_message,
-                                       response_obuffer, &dnsserv),
-                 XfroutError);
+    EXPECT_NO_THROW(server.processMessage(*io_message, parse_message,
+                                          response_obuffer, &dnsserv));
+    // Since the disconnect failed, we should still be 'connected'
     EXPECT_TRUE(xfrout.isConnected());
     // XXX: we need to re-enable disconnect.  otherwise an exception would be
     // thrown via the destructor of the server.
@@ -537,17 +552,16 @@ TEST_F(AuthSrvTest, IXFRSendFail) {
 }
 
 TEST_F(AuthSrvTest, IXFRDisconnectFail) {
-    // In our usage disconnect() shouldn't fail.  So we'll see the exception
-    // should it be thrown.
+    // In our usage disconnect() shouldn't fail, but even if it does,
+    // procesMessage() should catch it.
     xfrout.disableSend();
     xfrout.disableDisconnect();
     UnitTestUtil::createRequestMessage(request_message, opcode, default_qid,
                                        Name("example.com"), RRClass::IN(),
                                        RRType::IXFR());
     createRequestPacket(request_message, IPPROTO_TCP);
-    EXPECT_THROW(server.processMessage(*io_message, parse_message,
-                                       response_obuffer, &dnsserv),
-                 XfroutError);
+    EXPECT_NO_THROW(server.processMessage(*io_message, parse_message,
+                                          response_obuffer, &dnsserv));
     EXPECT_TRUE(xfrout.isConnected());
     // XXX: we need to re-enable disconnect.  otherwise an exception would be
     // thrown via the destructor of the server.
@@ -747,7 +761,8 @@ updateConfig(AuthSrv* server, const char* const config_data,
 
     ConstElementPtr result = config_answer->get("result");
     EXPECT_EQ(Element::list, result->getType());
-    EXPECT_EQ(expect_success ? 0 : 1, result->get(0)->intValue());
+    EXPECT_EQ(expect_success ? 0 : 1, result->get(0)->intValue()) <<
+        "Bad result from updateConfig: " << result->str();
 }
 
 // Install a Sqlite3 data source with testing data.
@@ -987,11 +1002,10 @@ getDummyUnknownSocket() {
     return (socket);
 }
 
-// Submit unexpected type of query and check it throws isc::Unexpected
+// Submit unexpected type of query and check it is ignored
 TEST_F(AuthSrvTest, queryCounterUnexpected) {
     // This code isn't exception safe, but we'd rather keep the code
-    // simpler and more readable as this is only for tests and if it throws
-    // the program would immediately terminate anyway.
+    // simpler and more readable as this is only for tests
 
     // Create UDP query packet.
     UnitTestUtil::createRequestMessage(request_message, Opcode::QUERY(),
@@ -1007,9 +1021,7 @@ TEST_F(AuthSrvTest, queryCounterUnexpected) {
                                request_renderer.getLength(),
                                getDummyUnknownSocket(), *endpoint);
 
-    EXPECT_THROW(server.processMessage(*io_message, parse_message,
-                                       response_obuffer, &dnsserv),
-                 isc::Unexpected);
+    EXPECT_FALSE(dnsserv.hasAnswer());
 }
 
 TEST_F(AuthSrvTest, stop) {
@@ -1038,4 +1050,231 @@ TEST_F(AuthSrvTest, listenAddresses) {
                                 "Released tokens");
 }
 
+//
+// Tests for catching exceptions in various stages of the query processing
+//
+// These tests work by defining two proxy classes, that act as an in-memory
+// client by default, but can throw exceptions at various points.
+//
+namespace {
+
+/// A the possible methods to throw in, either in FakeInMemoryClient or
+/// FakeZoneFinder
+enum ThrowWhen {
+    THROW_NEVER,
+    THROW_AT_FIND_ZONE,
+    THROW_AT_GET_ORIGIN,
+    THROW_AT_GET_CLASS,
+    THROW_AT_FIND,
+    THROW_AT_FIND_ALL,
+    THROW_AT_FIND_NSEC3
+};
+
+/// convenience function to check whether and what to throw
+void
+checkThrow(ThrowWhen method, ThrowWhen throw_at, bool isc_exception) {
+    if (method == throw_at) {
+        if (isc_exception) {
+            isc_throw(isc::Exception, "foo");
+        } else {
+            throw std::exception();
+        }
+    }
+}
+
+/// \brief proxy class for the ZoneFinder returned by the InMemoryClient
+///        proxied by FakeInMemoryClient
+///
+/// See the documentation for FakeInMemoryClient for more information,
+/// all methods simply check whether they should throw, and if not, call
+/// their proxied equivalent.
+class FakeZoneFinder : public isc::datasrc::ZoneFinder {
+public:
+    FakeZoneFinder(isc::datasrc::ZoneFinderPtr zone_finder,
+                   ThrowWhen throw_when,
+                   bool isc_exception) :
+        real_zone_finder_(zone_finder),
+        throw_when_(throw_when),
+        isc_exception_(isc_exception)
+    {}
+
+    virtual isc::dns::Name
+    getOrigin() const {
+        checkThrow(THROW_AT_GET_ORIGIN, throw_when_, isc_exception_);
+        return (real_zone_finder_->getOrigin());
+    }
+
+    virtual isc::dns::RRClass
+    getClass() const {
+        checkThrow(THROW_AT_GET_CLASS, throw_when_, isc_exception_);
+        return (real_zone_finder_->getClass());
+    }
+
+    virtual isc::datasrc::ZoneFinder::FindResult
+    find(const isc::dns::Name& name,
+         const isc::dns::RRType& type,
+         isc::datasrc::ZoneFinder::FindOptions options)
+    {
+        checkThrow(THROW_AT_FIND, throw_when_, isc_exception_);
+        return (real_zone_finder_->find(name, type, options));
+    }
+
+    virtual FindResult
+    findAll(const isc::dns::Name& name,
+            std::vector<isc::dns::ConstRRsetPtr> &target,
+            const FindOptions options = FIND_DEFAULT)
+    {
+        checkThrow(THROW_AT_FIND_ALL, throw_when_, isc_exception_);
+        return (real_zone_finder_->findAll(name, target, options));
+    };
+
+    virtual FindNSEC3Result
+    findNSEC3(const isc::dns::Name& name, bool recursive) {
+        checkThrow(THROW_AT_FIND_NSEC3, throw_when_, isc_exception_);
+        return (real_zone_finder_->findNSEC3(name, recursive));
+    };
+
+    virtual isc::dns::Name
+    findPreviousName(const isc::dns::Name& query) const {
+        return (real_zone_finder_->findPreviousName(query));
+    }
+
+private:
+    isc::datasrc::ZoneFinderPtr real_zone_finder_;
+    ThrowWhen throw_when_;
+    bool isc_exception_;
+};
+
+/// \brief Proxy InMemoryClient that can throw exceptions at specified times
+///
+/// It is based on the memory client since that one is easy to override
+/// (with setInMemoryClient) with the current design of AuthSrv.
+class FakeInMemoryClient : public isc::datasrc::InMemoryClient {
+public:
+    /// \brief Create a proxy memory client
+    ///
+    /// \param real_client The real in-memory client to proxy
+    /// \param throw_when if set to any value other than never, that is
+    ///        the method that will throw an exception (either in this
+    ///        class or the related FakeZoneFinder)
+    /// \param isc_exception if true, throw isc::Exception, otherwise,
+    ///                      throw std::exception
+    FakeInMemoryClient(AuthSrv::InMemoryClientPtr real_client,
+                       ThrowWhen throw_when,
+                       bool isc_exception) :
+        real_client_(real_client),
+        throw_when_(throw_when),
+        isc_exception_(isc_exception)
+    {}
+
+    /// \brief proxy call for findZone
+    ///
+    /// if this instance was constructed with throw_when set to find_zone,
+    /// this method will throw. Otherwise, it will return a FakeZoneFinder
+    /// instance which will throw at the method specified at the
+    /// construction of this instance.
+    virtual FindResult
+    findZone(const isc::dns::Name& name) const {
+        checkThrow(THROW_AT_FIND_ZONE, throw_when_, isc_exception_);
+        const FindResult result = real_client_->findZone(name);
+        return (FindResult(result.code, isc::datasrc::ZoneFinderPtr(
+                                        new FakeZoneFinder(result.zone_finder,
+                                        throw_when_,
+                                        isc_exception_))));
+    }
+
+private:
+    AuthSrv::InMemoryClientPtr real_client_;
+    ThrowWhen throw_when_;
+    bool isc_exception_;
+};
+
+} // end anonymous namespace for throwing proxy classes
+
+// Test for the tests
+//
+// Set the proxies to never throw, this should have the same result as
+// queryWithInMemoryClientNoDNSSEC, and serves to test the two proxy classes
+TEST_F(AuthSrvTest, queryWithInMemoryClientProxy) {
+    // Set real inmem client to proxy
+    updateConfig(&server, CONFIG_INMEMORY_EXAMPLE, true);
+
+    AuthSrv::InMemoryClientPtr fake_client(
+        new FakeInMemoryClient(server.getInMemoryClient(rrclass),
+                               THROW_NEVER,
+                               false));
+
+    ASSERT_NE(AuthSrv::InMemoryClientPtr(), server.getInMemoryClient(rrclass));
+    server.setInMemoryClient(rrclass, fake_client);
+
+    createDataFromFile("nsec3query_nodnssec_fromWire.wire");
+    server.processMessage(*io_message, parse_message, response_obuffer,
+                          &dnsserv);
+
+    EXPECT_TRUE(dnsserv.hasAnswer());
+    headerCheck(*parse_message, default_qid, Rcode::NOERROR(),
+                opcode.getCode(), QR_FLAG | AA_FLAG, 1, 1, 2, 1);
+}
+
+// Convenience function for the rest of the tests, set up a proxy
+// to throw in the given method
+// If isc_exception is true, it will throw isc::Exception, otherwise
+// it will throw std::exception
+void
+setupThrow(AuthSrv* server, const char *config, ThrowWhen throw_when,
+                   bool isc_exception)
+{
+    // Set real inmem client to proxy
+    updateConfig(server, config, true);
+
+    // Set it to throw on findZone(), this should result in
+    // SERVFAIL on any exception
+    AuthSrv::InMemoryClientPtr fake_client(
+        new FakeInMemoryClient(
+            server->getInMemoryClient(isc::dns::RRClass::IN()),
+            throw_when,
+            isc_exception));
+
+    ASSERT_NE(AuthSrv::InMemoryClientPtr(),
+              server->getInMemoryClient(isc::dns::RRClass::IN()));
+    server->setInMemoryClient(isc::dns::RRClass::IN(), fake_client);
+}
+
+TEST_F(AuthSrvTest, queryWithThrowingProxyServfails) {
+    // Test the common cases, all of which should simply return SERVFAIL
+    // Use THROW_NEVER as end marker
+    ThrowWhen throws[] = { THROW_AT_FIND_ZONE,
+                           THROW_AT_GET_ORIGIN,
+                           THROW_AT_FIND,
+                           THROW_AT_FIND_NSEC3,
+                           THROW_NEVER };
+    UnitTestUtil::createDNSSECRequestMessage(request_message, opcode,
+                                             default_qid, Name("foo.example."),
+                                             RRClass::IN(), RRType::TXT());
+    for (ThrowWhen* when(throws); *when != THROW_NEVER; ++when) {
+        createRequestPacket(request_message, IPPROTO_UDP);
+        setupThrow(&server, CONFIG_INMEMORY_EXAMPLE, *when, true);
+        processAndCheckSERVFAIL();
+        // To be sure, check same for non-isc-exceptions
+        createRequestPacket(request_message, IPPROTO_UDP);
+        setupThrow(&server, CONFIG_INMEMORY_EXAMPLE, *when, false);
+        processAndCheckSERVFAIL();
+    }
+}
+
+// Throw isc::Exception in getClass(). (Currently?) getClass is not called
+// in the processMessage path, so this should result in a normal answer
+TEST_F(AuthSrvTest, queryWithInMemoryClientProxyGetClass) {
+    createDataFromFile("nsec3query_nodnssec_fromWire.wire");
+    setupThrow(&server, CONFIG_INMEMORY_EXAMPLE, THROW_AT_GET_CLASS, true);
+
+    // getClass is not called so it should just answer
+    server.processMessage(*io_message, parse_message, response_obuffer,
+                          &dnsserv);
+
+    EXPECT_TRUE(dnsserv.hasAnswer());
+    headerCheck(*parse_message, default_qid, Rcode::NOERROR(),
+                opcode.getCode(), QR_FLAG | AA_FLAG, 1, 1, 2, 1);
+}
+
 }

src/lib/dns/tests/unittest_util.cc

@@ -191,3 +191,21 @@ UnitTestUtil::createRequestMessage(Message& message,
     message.addQuestion(Question(name, rrclass, rrtype));
 }
 
+void
+UnitTestUtil::createDNSSECRequestMessage(Message& message,
+                                         const Opcode& opcode,
+                                         const uint16_t qid,
+                                         const Name& name,
+                                         const RRClass& rrclass,
+                                         const RRType& rrtype)
+{
+    message.clear(Message::RENDER);
+    message.setOpcode(opcode);
+    message.setRcode(Rcode::NOERROR());
+    message.setQid(qid);
+    message.addQuestion(Question(name, rrclass, rrtype));
+    EDNSPtr edns(new EDNS());
+    edns->setUDPSize(4096);
+    edns->setDNSSECAwareness(true);
+    message.setEDNS(edns);
+}

src/lib/dns/tests/unittest_util.h

@@ -93,6 +93,22 @@ public:
                          const isc::dns::Name& name,
                          const isc::dns::RRClass& rrclass,
                          const isc::dns::RRType& rrtype);
+
+    ///
+    /// Populate a DNSSEC request message
+    ///
+    /// Create a request message in 'request_message' using the
+    /// opcode 'opcode' and the name/class/type query tuple specified in
+    /// 'name', 'rrclass' and 'rrtype.
+    /// EDNS will be added with DO=1 and bufsize 4096
+    static void
+    createDNSSECRequestMessage(isc::dns::Message& request_message,
+                               const isc::dns::Opcode& opcode,
+                               const uint16_t qid,
+                               const isc::dns::Name& name,
+                               const isc::dns::RRClass& rrclass,
+                               const isc::dns::RRType& rrtype);
+
 };
 }
 #endif // __UNITTEST_UTIL_H